|
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'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
|
|
|
|
|
Exactly that. 
|
|
|
|
|
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.
|
|
|
|
|
CodeWraith wrote: SEX R6
Writing a porn app then?
|
|
|
|
|
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???
|
|
|
|
|
Now that makes me miss programming!
Will Rogers never met me.
|
|
|
|
|
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()
);
|
|
|
|
|
My style too. Sometimes it gets a bit too indented, but it is far preferable to side-scrolling to make sense of what you are doing with LINQ extensions and Lambda expressions.
|
|
|
|
|
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
|
|
|
|
|
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 just throw some newlines in there:
OutputImage(file,
allFaces.Where(f => f.Proportion > lowerBound &&
f.Proportion < upperBound).
ToList().
OrderBy(f => f.Proportion).
ThenByDescending(f => f.Rectangle.Height).
FirstOrDefault());
|
|
|
|
|
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!
|
|
|
|
|
That's the way I do it. Except I move the && (or ||) to the start of the next line instead of leaving it trailing - it often helps when I want to comment out an option or two temporarily while testing.
- I would love to change the world, but they won’t give me the source code.
|
|
|
|
|
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()
);
|
|
|
|
|
|
Remember that the person who's doing the code review is a psychopath. 
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.
|
|
|
|
|
Hear hear Dave - I hate chained statements / method calls - set class properties and call the method - no need for parameters
"We can't stop here - this is bat country" - Hunter S Thompson - RIP
|
|
|
|
|
Which would refactor to something like this:
double lowerBound = double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString());
double upperBound = double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString());
List<Face> 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.
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
