|
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.
|
|
|
|
|
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
|
|
|
|
|
Marc Clifton wrote: The ToList() seems superfluous
It (and ToArray) nearly always is. (But I'm not about to try to read that code.
|
|
|
|
|
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.
|
|
|
|
|
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());
|
|
|
|
|
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
|
|
|
|
|
PeejayAdams wrote: double lowerBound = double.Parse(ConfigurationManager.AppSettings["LowerBound"].ToString());
double upperBound = double.Parse(ConfigurationManager.AppSettings["UpperBound"].ToString());
AppSettings is a NameValueCollection. 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 List when you only want one result? You can almost certainly drop the .ToList() call.
Also, are you sure the OutputImage method won't barf if you pass in null as 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
|
|
|
|
|
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 the AllFaces collection would probably be less expensive than the sort (.OrderBy(...)).
Software Zen: delete this;
|
|
|
|
|
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
|
|
|
|
|
Yes, you should do the latter, even though lowerBound and upperBound are only used once. The reason is maintainability. This shouldn't even be a question for anyone who has ever had to read and modify someone else's code.
|
|
|
|
|
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());
|
|
|
|
|
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 | [mastadon]](https://www.codeproject.com/script/Forums/Images/elephant.gif) 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.
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
