Long Lines
-
PeejayAdams wrote:
double lowerBound = double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString()); double upperBound = double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString());
AppSettingsis aNameValueCollection. The indexer returns a string. What are those extra.ToString()calls doing in there? :)PeejayAdams wrote:
.ToList().OrderBy(f => f.Proportion).ThenByDescending(f => f.Rectangle.Height).FirstOrDefault()
Why are you reading the whole thing into a
Listwhen you only want one result? You can almost certainly drop the.ToList()call. Also, are you sure theOutputImagemethod won't barf if you pass innullas the second parameter?double lowerBound = double.Parse(ConfigurationManager.AppSettings["LowerBound"]);
double upperBound = double.Parse(ConfigurationManager.AppSettings["UpperBound"]);var dirkBenedict = allFaces
.Where(f => f.Proportion > lowerBound && f.Proportion < upperBound)
.OrderBy(f => f.Proportion)
.ThenBy(f => f.Rectangle.Width)
.FirstOrDefault();OutputImage(file, dirkBenedict);
"These people looked deep within my soul and assigned me a number based on the order in which I joined." - Homer
It won't barf, but yes, those ToString()s are redundant. This is some code ripped from a very Q&D POC project that just seemed to be a good example of the more general issue of how to format long lines. Love the dirkBenedict variable name - my new mission is to find a way to get dwightSchultz in there!
Whenever you find yourself on the side of the majority, it is time to pause and reflect. - Mark Twain
-
Macros for the win. :cool:
You're sick and twisted. I like it. :-D
Software Zen:
delete this; -
Sometimes you find yourself writing a stupidly long line of code, in my case:
OutputImage(file, allFaces.Where(f => f.Proportion > double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString()) && f.Proportion < double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString())).ToList().OrderBy(f => f.Proportion).ThenByDescending(f => f.Rectangle.Height).FirstOrDefault());
That will auto-wrap on here but you get the idea. Well, obviously I can shorten that line (but lengthen the code slightly) by splitting out the config setting retrievals:
double lowerBound = double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString());
double upperBound = double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString());reducing the "offending" line to:
OutputImage(file, allFaces.Where(f => f.Proportion > lowerBound && f.Proportion < upperBound).ToList().OrderBy(f => f.Proportion).ThenByDescending(f => f.Rectangle.Height).FirstOrDefault());
But given that I'm not using those settings more than once, should I really be doing that? Two short-lived variables aren't going to make too much difference but I'm still doing it for cosmetic reasons and that can't be good ... Either way, it's easy enough to split a method call where there are half a ton of arguments over several lines of text but what about when it's just one of the arguments that's causing the thing to scroll for miles? (Lambdas being a common cause of this). Hardly a matter of life and death but I'd be interested to hear the thoughts of others on this.
Whenever you find yourself on the side of the majority, it is time to pause and reflect. - Mark Twain
OutputImage(file,
allFaces.Where(f =>
f.Proportion > double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString()) &&
f.Proportion < double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString()))
.ToList()
.OrderBy(f => f.Proportion)
.ThenByDescending(f => f.Rectangle.Height)
.FirstOrDefault());This is probably how I'd break this out without introducing temporary values. That said, to me the whole statement would be very difficult to debug. You've got several things going on where it would be useful to see the intermediate values. Also, the
.FirstOrDefault()at the end tells me that, since you want only one value from the set, simple iteration over theAllFacescollection would probably be less expensive than the sort (.OrderBy(...)).Software Zen:
delete this; -
Which would refactor to something like this: double lowerBound = double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString()); double upperBound = double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString()); List facesWithinRange = allFaces.Where(f => f.Proportion > lowerBound && f.Proportion < upperBound).ToList(); Face faceToUse = facesWithinRange.OrderBy(f => f.Proportion).ThenByDescending(f => f.Rectangle.Height).FirstOrDefault(); OutputImage(file, faceToUse); (Apologies for complete HTML fail) In this particular case, it's not really making it much more readable to my eyes but I can see that there are situations where it would.
Whenever you find yourself on the side of the majority, it is time to pause and reflect. - Mark Twain
It's a bit more readable, and in this case, it's a lot more debuggable. You now have your bounds and list in variables, making it easier to figure out why you're not getting the results you should be getting. On top of that, the code doesn't check for bounds being valid values or even readable from AppSettings and breaking it out into variables makes that easier to spot and fix.
Asking questions is a skill CodeProject Forum Guidelines Google: C# How to debug code Seriously, go read these articles.
Dave Kreskowiak -
That's the one I'd go with too. Except I'd put the dots on the new line:
OutputImage(file,
allFaces.Where(f => f.Proportion > lowerBound &&
f.Proportion < upperBound)
.ToList()
.OrderBy(f => f.Proportion)
.ThenByDescending(f => f.Rectangle.Height)
.FirstOrDefault());"I have no idea what I did, but I'm taking full credit for it." - ThisOldTony AntiTwitter: @DalekDave is now a follower!
-
Sometimes you find yourself writing a stupidly long line of code, in my case:
OutputImage(file, allFaces.Where(f => f.Proportion > double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString()) && f.Proportion < double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString())).ToList().OrderBy(f => f.Proportion).ThenByDescending(f => f.Rectangle.Height).FirstOrDefault());
That will auto-wrap on here but you get the idea. Well, obviously I can shorten that line (but lengthen the code slightly) by splitting out the config setting retrievals:
double lowerBound = double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString());
double upperBound = double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString());reducing the "offending" line to:
OutputImage(file, allFaces.Where(f => f.Proportion > lowerBound && f.Proportion < upperBound).ToList().OrderBy(f => f.Proportion).ThenByDescending(f => f.Rectangle.Height).FirstOrDefault());
But given that I'm not using those settings more than once, should I really be doing that? Two short-lived variables aren't going to make too much difference but I'm still doing it for cosmetic reasons and that can't be good ... Either way, it's easy enough to split a method call where there are half a ton of arguments over several lines of text but what about when it's just one of the arguments that's causing the thing to scroll for miles? (Lambdas being a common cause of this). Hardly a matter of life and death but I'd be interested to hear the thoughts of others on this.
Whenever you find yourself on the side of the majority, it is time to pause and reflect. - Mark Twain
class ConfigWrapper
{
public static double LowerBound { get { return double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString()); } }
public static double UpperBound { get { return double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString()); } }
...
}...
var face = allFaces.Where(f => f.Proportion > lowerBound && f.Proportion < upperBound).ToList()
.OrderBy(f => f.Proportion).ThenByDescending(f => f.Rectangle.Height).FirstOrDefault();
OutputImage(file, face);Wrap the config so that you can be sure it's always accessed correctly, and can potentially do lazy loading and data validation. This also makes changing hard coded app.config settings into user configurable ones later on a lot less painful. Unless I was doing something that LINQ couldn't translate to SQL or that caused it to do something terribly inefficient, I'd drop the `ToList()` in the middle of the method chain as useless overhead.
Did you ever see history portrayed as an old man with a wise brow and pulseless heart, weighing all things in the balance of reason? Is not rather the genius of history like an eternal, imploring maiden, full of fire, with a burning heart and flaming soul, humanly warm and humanly beautiful? --Zachris Topelius Training a telescope on one’s own belly button will only reveal lint. You like that? You go right on staring at it. I prefer looking at galaxies. -- Sarah Hoyt
-
PeejayAdams wrote:
Two short-lived variables
Lifetime should not influence readability. I almost always put things into variables not just for readability but that it makes it easier to debug. Given:
OutputImage(file, allFaces.Where(f => f.Proportion > double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString()) && f.Proportion < double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString())).ToList().OrderBy(f => f.Proportion).ThenByDescending(f => f.Rectangle.Height).FirstOrDefault());
I do have a penchant for extension methods. As in:
var face = allFaces
.AnyBetween(f => f.Proportion, "LowerBound".AppSetting().to_f(), "UpperBound".AppSetting().to_f())
.OrderBy(f => f.Proportion)
.ThenByDescending(f => f.Rectangle.Height)
.FirstOrDefault();
OutputImage(file, face);As bizarre as it is to extend
string, I find it a lot more readable. TheToList()seems superfluous. Not sure you want to output an image if no images meet the selection criteria. I might moveFirstOrDefaultintoOutputImage(file, face.FirstOrDefault());as well, who knows, the way you get the faces might be re-usable. I leave it to the reader to figure outto_f,AnyBetween, andAppSettingsShould be obvious.Latest Articles:
Client-Side TypeScript without ASP.NET, Angular, etc.Marc Clifton wrote:
The
ToList()seems superfluousIt (and ToArray) nearly always is. (But I'm not about to try to read that code.
-
It's a bit more readable, and in this case, it's a lot more debuggable. You now have your bounds and list in variables, making it easier to figure out why you're not getting the results you should be getting. On top of that, the code doesn't check for bounds being valid values or even readable from AppSettings and breaking it out into variables makes that easier to spot and fix.
Asking questions is a skill CodeProject Forum Guidelines Google: C# How to debug code Seriously, go read these articles.
Dave KreskowiakAs mentioned elsewhere, Dave, this is from a Q&D proof of concept demo thing, not production code and was intended to be purely illustrative of a very long line of code. No way would I release something into the real world without validating config sections but as I said, this thread ain't about a code review!
Whenever you find yourself on the side of the majority, it is time to pause and reflect. - Mark Twain
-
Sometimes you find yourself writing a stupidly long line of code, in my case:
OutputImage(file, allFaces.Where(f => f.Proportion > double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString()) && f.Proportion < double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString())).ToList().OrderBy(f => f.Proportion).ThenByDescending(f => f.Rectangle.Height).FirstOrDefault());
That will auto-wrap on here but you get the idea. Well, obviously I can shorten that line (but lengthen the code slightly) by splitting out the config setting retrievals:
double lowerBound = double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString());
double upperBound = double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString());reducing the "offending" line to:
OutputImage(file, allFaces.Where(f => f.Proportion > lowerBound && f.Proportion < upperBound).ToList().OrderBy(f => f.Proportion).ThenByDescending(f => f.Rectangle.Height).FirstOrDefault());
But given that I'm not using those settings more than once, should I really be doing that? Two short-lived variables aren't going to make too much difference but I'm still doing it for cosmetic reasons and that can't be good ... Either way, it's easy enough to split a method call where there are half a ton of arguments over several lines of text but what about when it's just one of the arguments that's causing the thing to scroll for miles? (Lambdas being a common cause of this). Hardly a matter of life and death but I'd be interested to hear the thoughts of others on this.
Whenever you find yourself on the side of the majority, it is time to pause and reflect. - Mark Twain
-
Sometimes you find yourself writing a stupidly long line of code, in my case:
OutputImage(file, allFaces.Where(f => f.Proportion > double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString()) && f.Proportion < double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString())).ToList().OrderBy(f => f.Proportion).ThenByDescending(f => f.Rectangle.Height).FirstOrDefault());
That will auto-wrap on here but you get the idea. Well, obviously I can shorten that line (but lengthen the code slightly) by splitting out the config setting retrievals:
double lowerBound = double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString());
double upperBound = double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString());reducing the "offending" line to:
OutputImage(file, allFaces.Where(f => f.Proportion > lowerBound && f.Proportion < upperBound).ToList().OrderBy(f => f.Proportion).ThenByDescending(f => f.Rectangle.Height).FirstOrDefault());
But given that I'm not using those settings more than once, should I really be doing that? Two short-lived variables aren't going to make too much difference but I'm still doing it for cosmetic reasons and that can't be good ... Either way, it's easy enough to split a method call where there are half a ton of arguments over several lines of text but what about when it's just one of the arguments that's causing the thing to scroll for miles? (Lambdas being a common cause of this). Hardly a matter of life and death but I'd be interested to hear the thoughts of others on this.
Whenever you find yourself on the side of the majority, it is time to pause and reflect. - Mark Twain
I don't think it's necessary to add the extra variables to make the code readable. Just split the lines where it makes sense. That should be enough.
OutputImage(file, allFaces.Where(f =>
f.Proportion > double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString()) &&
f.Proportion < double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString())).ToList()
.OrderBy(f => f.Proportion)
.ThenByDescending(f => f.Rectangle.Height)
.FirstOrDefault()); -
Sometimes you find yourself writing a stupidly long line of code, in my case:
OutputImage(file, allFaces.Where(f => f.Proportion > double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString()) && f.Proportion < double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString())).ToList().OrderBy(f => f.Proportion).ThenByDescending(f => f.Rectangle.Height).FirstOrDefault());
That will auto-wrap on here but you get the idea. Well, obviously I can shorten that line (but lengthen the code slightly) by splitting out the config setting retrievals:
double lowerBound = double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString());
double upperBound = double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString());reducing the "offending" line to:
OutputImage(file, allFaces.Where(f => f.Proportion > lowerBound && f.Proportion < upperBound).ToList().OrderBy(f => f.Proportion).ThenByDescending(f => f.Rectangle.Height).FirstOrDefault());
But given that I'm not using those settings more than once, should I really be doing that? Two short-lived variables aren't going to make too much difference but I'm still doing it for cosmetic reasons and that can't be good ... Either way, it's easy enough to split a method call where there are half a ton of arguments over several lines of text but what about when it's just one of the arguments that's causing the thing to scroll for miles? (Lambdas being a common cause of this). Hardly a matter of life and death but I'd be interested to hear the thoughts of others on this.
Whenever you find yourself on the side of the majority, it is time to pause and reflect. - Mark Twain
I haven't read all the responses yet, by here's my take in general terms. As devs, we sometimes encounter situations where competing priorities become evident. Like when the ideal of simplicity, readability, and maintainability run counter to runtime efficiency. I always bias my choices with empathy for the shmuck (fixer-person or enhancer-person) that comes later and must figure out what the :elephant: I was trying to do. Impossible-to-easily-parse code is a follow-on bug waiting to happen. There is nothing more insidiously degrading to a codebase than cascading bugs due to ill-informed, shoot-from-the-hip changes because the code is too difficult to grok. And we all know it happens. The kicker here is, unless it's message-loop-style code that runs continuously for the life of execution, it won't ever be an issue (AND, the compiler may even ultimately produce the same code.) endRant :)
Cheers, Mike Fidler "I intend to live forever - so far, so good." Steven Wright "I almost had a psychic girlfriend but she left me before we met." Also Steven Wright "I'm addicted to placebos. I could quit, but it wouldn't matter." Steven Wright yet again.
-
It's a bit more readable, and in this case, it's a lot more debuggable. You now have your bounds and list in variables, making it easier to figure out why you're not getting the results you should be getting. On top of that, the code doesn't check for bounds being valid values or even readable from AppSettings and breaking it out into variables makes that easier to spot and fix.
Asking questions is a skill CodeProject Forum Guidelines Google: C# How to debug code Seriously, go read these articles.
Dave KreskowiakNo, no, I wasn't looking at it from a code review perspective. I realize this is demo code. I was just using it's an example of why I would never have long lines like that, in any code. The validation check for the values doesn't have to be there because this is demo code. I was pointing out the case that breaking the lines out of the chain makes spotting such missing functionality easier.
Asking questions is a skill CodeProject Forum Guidelines Google: C# How to debug code Seriously, go read these articles.
Dave Kreskowiak -
Sometimes you find yourself writing a stupidly long line of code, in my case:
OutputImage(file, allFaces.Where(f => f.Proportion > double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString()) && f.Proportion < double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString())).ToList().OrderBy(f => f.Proportion).ThenByDescending(f => f.Rectangle.Height).FirstOrDefault());
That will auto-wrap on here but you get the idea. Well, obviously I can shorten that line (but lengthen the code slightly) by splitting out the config setting retrievals:
double lowerBound = double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString());
double upperBound = double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString());reducing the "offending" line to:
OutputImage(file, allFaces.Where(f => f.Proportion > lowerBound && f.Proportion < upperBound).ToList().OrderBy(f => f.Proportion).ThenByDescending(f => f.Rectangle.Height).FirstOrDefault());
But given that I'm not using those settings more than once, should I really be doing that? Two short-lived variables aren't going to make too much difference but I'm still doing it for cosmetic reasons and that can't be good ... Either way, it's easy enough to split a method call where there are half a ton of arguments over several lines of text but what about when it's just one of the arguments that's causing the thing to scroll for miles? (Lambdas being a common cause of this). Hardly a matter of life and death but I'd be interested to hear the thoughts of others on this.
Whenever you find yourself on the side of the majority, it is time to pause and reflect. - Mark Twain
Over the years, I've developed an attitude about this. If I want others (including myself in a later incarnation) to stay away, PLEASE do not think this is easy by being ""readable"", I densify, (un)justly believing in more efficient code (from a compiler or machine viewpoint). Intermediate stuff should be lighter, though.
