|
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.
|
|
|
|
|
As 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
|
|
|
|
|
No, 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.
|
|
|
|
|
Dave 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.]
|
|
|
|
|
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
|
|
|
|
|
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
|
|
|
|
|
I never used to worry about long lines but since I now use a wide screen monitor in portrait mode for editing I find scrolling right so annoying!
In addition we used side by side code display for comparison during code reviews so shorter lines are better.
Splitting after "(", ",", ".", "=>" and occasionally "=" really helps but keeps the code easily readable. I even adopted this style at home where there are no code reviewers beyond me.
- I would love to change the world, but they won’t give me the source code.
modified 21-Oct-19 11:58am.
|
|
|
|
|
Splitting _before_ operators tends to help them stand out.
|
|
|
|
|
Actually I split before dots and most operators, but after equals, etc.
- I would love to change the world, but they won’t give me the source code.
|
|
|
|
|
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
|
|
|
|
|
Macros for the win. 
|
|
|
|
|
You're sick and twisted.
I like it. 
Software Zen: delete this;
|
|
|
|
|
Create an arguments class and pass that it. Then, when arguments changes (not 'if', but 'when'), then the method's signature stays the same.
If it's not broken, fix it until it is.
Everything makes sense in someone's mind.
Ya can't fix stupid.
|
|
|
|
|
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.
The ToList() seems superfluous. Not sure you want to output an image if no images meet the selection criteria. I might move FirstOrDefault into OutputImage(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 out to_f, AnyBetween, and AppSettings Should be obvious.
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
