|
annathor wrote: If you feel the need to comment the code,then the code is a horror
You should have worked on our old codebase.. 100,000+ lines of code and three comments. The only printable one was "// bitchin". Of course, one of the two foul language ones was actually helpful 
As far as I can tell, there's only three cases where you don't need comments:
1) you're the only one who has ever, and will ever, work with the code. You'll take it to your grave with you.
2) its a toy project or a throw-away experiemnt... though this is really just a subset of (1)
3) its an open source project.
We can program with only 1's, but if all you've got are zeros, you've got nothing.
|
|
|
|
|
If people maintaining this code are used to the "?" operator, then this code is quite readable, and nearly simple.
But if they're not, then they'll find it a coding horror 
|
|
|
|
|
A developer should know the language they work with
|
|
|
|
|
Definitely agree with you.
But at work, I'm always surprised to see the number of junior developers who don't understand (or don't want to try to understand) this simple construct.. (the "?" operator, I mean)
|
|
|
|
|
It's sad how many devs don't know about the null coalesce operator either
Which reminds me, I have to go teach a fellow "dev" how asynchronous programming works...
|
|
|
|
|
There seems to be a common opinion that the ternary is pure evil and should never be used, even as far more recent and difficult to understand concepts (anonymous delegates, lambdas, LINQ etc) are accepted. I don't understand why, it's a simple and elegant construction and easy to parse (for a person as well as a computer). I guess it's because it can be horribly abused (see some other posts in this forum), but so can pretty much every language feature.
|
|
|
|
|
While I mostly agree, this is not always possible. C++ in particular is such a large language that few people know it completely, but its perfectly possible to write excellent programs using a familiar subset. In such cases, use the arcane features of the language can be regarded as a codin horror - as the majority of developers will need to look up the meaning.
While programmers favouring other languages may scoff, this seems similar to knowing the libraries. I could learn C# syntax in at most a week for example, becoming familiar with the libraries takes longer, and is actually what I spend of my time doing. I'm absolutely sure I don't know them as well as I'd like, and there's whole areas I leave until I need to.
What makes all this work is MS's documentation that (normally) helps see usage conveniently.
Most of that is auto-generated from Comments, so comments seem fairly necessary.
I kind of hate the "everyone who doesn't know what I know is an idiot" approach to programming - I'd hate to be on a project with someone with that attitude, they tend to leave unmaintainable code in their wake.
|
|
|
|
|
What I don't like on this piece of code is that the verification part is missing.
If user is null -> NullReferenceException.
If inRoles is null && ((user.Roles & userRole) != 0) -> NullReferenceException.
If outRoles is null ((user.Roles & userRole) == 0) -> NullReferenceException.
Of course, one can use code contracts in .NET 4.0.
To overcome this, one will have to use an if-else block
if (user == null)
return false;
if (string.IsNullOrEmpty(roleName))
return false;
if ((user.Roles & userRole) != 0) {
if (inRoles == null)
return false;
inRoles.Add(roleName);
} else {
if (outRoles == null)
return false;
outRoles.Add(roleName);
}
return true;
Eusebiu
|
|
|
|
|
Letting the exception escape in such cases is often fine, particularly considering this is a private method and therefore its calling context is closely controlled (alliteration ftw!) and if necessary the arguments can be protected outside. Expanding the content like that makes it much less clear what the actual code element is.
|
|
|
|
|
No. It's good. Nice and simple, using the ternary to perform a simple switch between two results as it's designed to do. It took me no time to see what is happening there (you are adding the role to a list that is either roles the user is in or ones he's not depending on if he's in this one).
|
|
|
|
|
I don't think it's that bad tbh. Can it improve? Probably.
I'ld say that possible improvements could be:
1. rename the method to reflect the 'if' logic
2. extract the 'if' logic so that this method indeed does nothing but add a role to a specific list of roles
3. refactor it into 2 methods.
Also, looking at this small piece of code only, I spot a possible unhandled nullreference
But that's probably resharper being hardwired into my brain. :-S
Anyhow, if I were to do a code review and come accross this, I probably wouldn't get all fired up about it. So, no, it's certainly not a "horror".
It's actually quite elegant imo.
The 3 points above are just nitpicking (which is something you can do with almost any piece of code).
|
|
|
|
|
What is user or user.Roles or any other one parameter is null ? 
|
|
|
|
|
Then you'ld get a null reference...
Just like I said.
But seeing how this is a private method, it's ok to write it like that most of the time imo.
It would be a different story if the method was public.
|
|
|
|
|
No...
It's programmer porno..!! lol
|
|
|
|
|
I disagree with the sentiment that badly readable code can or should be augmented through comments. Comments are a means to increase maintainability, and if there is any reasonably easy way to rewrite the code into something better readable or even self-explaining, then that is a much better way to add maintainability than writing a comment.
Whether or not there is a comment, the line of code above might require several minutes of contemplation to grasp it's meaning for anyone unfamiliar with this code. A well written alternate code segment however will be understandable within seconds, even without comments.
|
|
|
|
|
(((user.Roles & userRole) != 0) ? inRoles : outRoles)
...this is the part that needs some 'splaining. Maybe something as simple as:
var rolesList = (user.Roles & userRole) != 0) ? inRoles : outRoles;
rolesList.Add(roleName);
|
|
|
|
|
This is what I was thinking as well, which is why I looked at all the responses. The only I would change, in .NET 4, is to use the HasFlags method of enum:
var rolesList = user.Roles.HasFlag(userRole) ? inRoles : outRoles;
rolesList.Add(roleName);
This also means no brackets are necessary.
|
|
|
|
|
Good catch ...I was actually unaware of the HasFlag method ...nice!
|
|
|
|
|
I like it. But why make it a function. Since it is just a single line, just leave it in place and you don't waste time with a function call. Only if called in several places does it need to be a function.
But, I have friends that tell people that I can (and am willing to) write a whole program on a single line. 
SS => Qualified in Submarines
"We sleep soundly in our beds because rough men stand ready in the night to visit violence on those who would do us harm". Winston Churchill
"Real programmers can write FORTRAN in any language". Unknown
|
|
|
|
|
First, the advantage of terse code is that you can more of the code on the screen at a time. If that advantage outweighs others, such as possibly the next guy getting confused, maybe it's worth it. Maybe you're writing code on a 40 column x 20 row terminal like an Atari 800. This likely may not be the case.
The next issue is optimization. Does this code optimize better than the equivalent:
if ((user.Roles & userRole) != 0)
inRoles.Add(roleName);
else
outRoles.Add(roleName);
I bet the rewritten version isn't any less optimized.
Third, which one is easier to single-step through? Some debuggers only let you step through lines, not statements.
And finally, if you're doing this so that people will go, "wow, you can do that? that works? wow", then you're likely to be breaking the Principle of least astonishment.
ken@kasajian.com / www.kasajian.com
|
|
|
|
|
Firstly, no, Visual Studio on a 22" monitor, so I don't have that excuse.
Secondly, if the two do not optimize to the same code, then someone as Microsoft should be up against the wall.
Thirdly, I think the VS compiler will allow single step only on the second version.
Finally, no, I did it because it seemed reasonable at the time, after I had removed a pile of code (which is why it has a dumb name - it didn't for long and got deleted completely soon afterwards) When I realized what I had left myself, the initial response was "Yeuch"! So I stuck it here.
It realized some interesting responses - from my point of view it is pretty nasty, and not something I would want to leave in production code. Interesting that some people seem to think it is fine, if it is commented!
Real men don't use instructions. They are only the manufacturers opinion on how to put the thing together.
Manfred R. Bihy: "Looks as if OP is learning resistant."
|
|
|
|
|
So why do you think some people think it's okay?
I've heard programmers tell me that swapping the value of x and y without using a temporary intermediate is somehow better, coding ending up looking something like this:
*x^=*y^=*x^=*y
eeek
ken@kasajian.com / www.kasajian.com
|
|
|
|
|
Kenneth Kasajian wrote: So why do you think some people think it's okay?
http://www.codeproject.com/Messages/4008174/Re-Is-this-a-coding-horror.aspx[^]
Kenneth Kasajian wrote: *x^=*y^=*x^=*y
In embedded assembler, when you are after every last clock cycle in a limited uProcessor, then swap by XOR can be a real time saver - since it only uses the ALU, there is less external memory access, which can save a lot of time. I don't like to work that close to the wind though - and if I do, it get commented to hell and back. In C++ or C#? Don't go there! 
Real men don't use instructions. They are only the manufacturers opinion on how to put the thing together.
Manfred R. Bihy: "Looks as if OP is learning resistant."
|
|
|
|
|
That's brilliant. Although in the spirit of this thread, it should surely be commented:
*x^=*y^=*x^=*y;
|
|
|
|
|
Your code is not a horror in any ways!
Instead, its a clever code and must be upvoted as I have given 5up for that!
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
