Long Lines
-
I'd do nothing of the sort. Long-ass lines like that make the code harder to read, maintain, and debug. I'd bust the individual components out to their own variables and combine them in the final statement. When you compile for Release, all that gets optimized out in most cases. You're really not saving yourself anything by piling it all into a single statement.
Asking questions is a skill CodeProject Forum Guidelines Google: C# How to debug code Seriously, go read these articles.
Dave Kreskowiak -
Just setting the X register to a new value. This means that the value in register 6 is now the new stack pointer.
I have lived with several Zen masters - all of them were cats. His last invention was an evil Lasagna. It didn't kill anyone, and it actually tasted pretty good.
I think Richard was making a joke.
What do you get when you cross a joke with a rhetorical question? The metaphorical solid rear-end expulsions have impacted the metaphorical motorized bladed rotating air movement mechanism. Do questions with multiple question marks annoy you???
-
Strange. Much of my code looks like this:
Quote:
SEX R6 BN4 LOOP INP 04 OUT 04 B4 LOOP LDA R6 SDI 0F BNE LOOP NOP
The lines don't get too long, even if I use the old 64 x 64 pixel screen resolution. :-)
I have lived with several Zen masters - all of them were cats. His last invention was an evil Lasagna. It didn't kill anyone, and it actually tasted pretty good.
Now that makes me miss programming! :)
Will Rogers never met me.
-
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
You're my hero. I also find myself writing lines like this. I kinda consider them a guilty pleasure, they may not look nice, but I like them. Started all the way back at the end of my school time, I was in the Maple math class and my teacher always told me to use variables, they're aplenty and free. He kept telling me every single time.
-
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
In theory, you can write your entire application on a single line. You're writing for people though, not for machines. So yeah, make two separate lines that make it more readable and the compiler will inline them giving you no added memory or performance impacts. I'd probably also do this:
OutputImage(file, allFaces.Where(f => f.Proportion > lowerBound && f.Proportion < upperBound)
.ToList()
.OrderBy(f => f.Proportion)
.ThenByDescending(f => f.Rectangle.Height)
.FirstOrDefault());Best, Sander sanderrossel.com Continuous Integration, Delivery, and Deployment arrgh.js - Bringing LINQ to JavaScript Object-Oriented Programming in C# Succinctly
-
That's my preference as well, unless I'm still debugging the code, and I want to verify the output of one of the chained calls. Otherwise setting a breakpoint, say, on line 6 sets a breakpoint on the entire thing. But once I'm confident I won't have to revisit it, it all goes back to a single statement split among multiple lines.
When lines become long, I split before a binary operator, and make sure that binary operators always are in the same line, or, if the line is split, in the same column as the parentheses, if there are any. In addition, I usually put like binary operators in the same column. If a line is split at a binary operator, I indent both operands by the same amount, e.g. two characters to the right. Like so:
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'd split it as you have done to improve readability and not worry about short-lived variables, the compiler optimisation will take care of that and in-line your code for you.
Yes, I'd agree with that.
Whenever you find yourself on the side of the majority, it is time to pause and reflect. - Mark Twain
-
I'd split it as you have done to improve readability and not worry about short-lived variables, the compiler optimisation will take care of that and in-line your code for you.
Exactly that. :thumbsup:
-
My personal style (which I suspect will not agree with anyone else's ideas) is ...
OutputImage(
file,
allFaces
.Where(
f =>
f.Proportion > double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString())
&& f.Proportion < double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString())
)
.List()
.OrderBy(f => f.Proportion)
.ThenByDescending(f => f.Rectangle.Height)
.FirstOrDefault()
);Yes, I quite like that - definitely prefer the dot on the left to the right. Subconsciously, that maybe just because it reflects my SQL style where I always put the comma to the left, but for some reason it is more readable than having it the other way round.
Whenever you find yourself on the side of the majority, it is time to pause and reflect. - Mark Twain
-
Remember that the person who's doing the code review is a psychopath. :rolleyes: I'd split that bugger.
I'd rather be phishing!
You haven't met the bloke who's writing the code. He's worse! :)
Whenever you find yourself on the side of the majority, it is time to pause and reflect. - Mark Twain
-
I'd do nothing of the sort. Long-ass lines like that make the code harder to read, maintain, and debug. I'd bust the individual components out to their own variables and combine them in the final statement. When you compile for Release, all that gets optimized out in most cases. You're really not saving yourself anything by piling it all into a single statement.
Asking questions is a skill CodeProject Forum Guidelines Google: C# How to debug code Seriously, go read these articles.
Dave KreskowiakWhich 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
-
PeejayAdams wrote:
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
You're not doing it for cosmetic reasons. You're doing it to improve code clarity. And that, IMHO, has huge value. /ravi
My new year resolution: 2048 x 1536 Home | Articles | My .NET bits | Freeware ravib(at)ravib(dot)com
That is a very wise comment!
Whenever you find yourself on the side of the majority, it is time to pause and reflect. - Mark Twain
-
double lowerBound = double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString());
double upperBound = double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString());In my opinion this way would be better so that you can have error checking on it and provide a meaningful error message if one of the Appsettings is invalid.
Social Media - A platform that makes it easier for the crazies to find each other. Everyone is born right handed. Only the strongest overcome it. Fight for left-handed rights and hand equality.
That's a very fair point - this is something of a Q&D development (just a proof of concept demo) but yes, if it were a production piece I would definitely do that.
Whenever you find yourself on the side of the majority, it is time to pause and reflect. - Mark Twain
-
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.I do share your fondness for extension methods - definitely one of the best language features in C#!
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
Those variables are used in only one place, but they are accessed more than once, since they live in a Where lambda. So it makes good sense to extract them! Or can you assure that the Where lambda is called at most once? If yes, then drop it, because in that case, LinQ is overkill. Rolf
-
My personal style (which I suspect will not agree with anyone else's ideas) is ...
OutputImage(
file,
allFaces
.Where(
f =>
f.Proportion > double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString())
&& f.Proportion < double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString())
)
.List()
.OrderBy(f => f.Proportion)
.ThenByDescending(f => f.Rectangle.Height)
.FirstOrDefault()
);I dislike the deep indents ... but your code is immediately understandable, without having to figure out what each parameter is. IMO the winner is readability for the next person who has to mess with the code. [or for myself 3-12 months from now, wondering whutinthuheck I was doing]
-
I'd do nothing of the sort. Long-ass lines like that make the code harder to read, maintain, and debug. I'd bust the individual components out to their own variables and combine them in the final statement. When you compile for Release, all that gets optimized out in most cases. You're really not saving yourself anything by piling it all into a single statement.
Asking questions is a skill CodeProject Forum Guidelines Google: C# How to debug code Seriously, go read these articles.
Dave KreskowiakDave Kreskowiak wrote:
ou're really not saving yourself anything by piling it all into a single statement.
Agreed Debugging is key for me. A fellow coder loves to chain things together ... but if there is a problem with one parameter, it's harder to nail down when it's part of a large chain. Plus it's not immediately obvious what parameters are in many cases. Name the variables well and it lends to self documenting code. [Although I want a comment explaining the business and/or technical reason for the code.]
-
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
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
-
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;
