Is this a coding horror?
-
I don't know - it isn't in my code anymore... I wrote this wonder this morning, and realized I could prune it down to just the one line:
private void SetRole(SMUser user, UserRole userRole, string roleName, List<string> inRoles, List<string> outRoles)
{
(((user.Roles & userRole) != 0) ? inRoles : outRoles).Add(roleName);
}I mean, yes it works. But... It's not there anymore: I changed the whole way I handle roles for other reasons. But still, It's a bit impenetrable...
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."
-
I don't know - it isn't in my code anymore... I wrote this wonder this morning, and realized I could prune it down to just the one line:
private void SetRole(SMUser user, UserRole userRole, string roleName, List<string> inRoles, List<string> outRoles)
{
(((user.Roles & userRole) != 0) ? inRoles : outRoles).Add(roleName);
}I mean, yes it works. But... It's not there anymore: I changed the whole way I handle roles for other reasons. But still, It's a bit impenetrable...
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."
-
In my opinion, it's a nice piece of code, you should take it to the 'Clever code' forum. Readability is a problem though.
It's the readability I don't like at all - that is not clever, it's just downright nasty! :laugh:
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."
-
I don't know - it isn't in my code anymore... I wrote this wonder this morning, and realized I could prune it down to just the one line:
private void SetRole(SMUser user, UserRole userRole, string roleName, List<string> inRoles, List<string> outRoles)
{
(((user.Roles & userRole) != 0) ? inRoles : outRoles).Add(roleName);
}I mean, yes it works. But... It's not there anymore: I changed the whole way I handle roles for other reasons. But still, It's a bit impenetrable...
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."
I wouldn't qualify it as horror. It's just a little bit too clever. With a good comment to explain it quickly, or a more explicit method name, it would be fine by me.
'As programmers go, I'm fairly social. Which still means I'm a borderline sociopath by normal standards.' Jeff Atwood
-
I wouldn't qualify it as horror. It's just a little bit too clever. With a good comment to explain it quickly, or a more explicit method name, it would be fine by me.
'As programmers go, I'm fairly social. Which still means I'm a borderline sociopath by normal standards.' Jeff Atwood
-
I wouldn't qualify it as horror. It's just a little bit too clever. With a good comment to explain it quickly, or a more explicit method name, it would be fine by me.
'As programmers go, I'm fairly social. Which still means I'm a borderline sociopath by normal standards.' Jeff Atwood
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 ;P
-
I have to agree its not really horrible; as long as it is well commented it is ok.
Just because the code works, it doesn't mean that it is good code.
If you feel the need to comment the code,then the code is a horror. Good code shouldn't be dependent on comments to tell the system story. Or at least that is my 2 cents That said, this isn't a big horror, it clearly states what it does, 3 nitpick is the use of prefix in the SMUser class name, and that the name of the method is somewhat misleading, and that the method does two things that a) should be clearly specified in the method naming or b) refactored into two separate methods.
modified on Tuesday, August 30, 2011 6:21 AM
-
I don't know - it isn't in my code anymore... I wrote this wonder this morning, and realized I could prune it down to just the one line:
private void SetRole(SMUser user, UserRole userRole, string roleName, List<string> inRoles, List<string> outRoles)
{
(((user.Roles & userRole) != 0) ? inRoles : outRoles).Add(roleName);
}I mean, yes it works. But... It's not there anymore: I changed the whole way I handle roles for other reasons. But still, It's a bit impenetrable...
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."
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; // return should also say if the operation succeded or not => void->bool(or success/failure status).
if (string.IsNullOrEmpty(roleName)) // + other validation for 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
-
I don't know - it isn't in my code anymore... I wrote this wonder this morning, and realized I could prune it down to just the one line:
private void SetRole(SMUser user, UserRole userRole, string roleName, List<string> inRoles, List<string> outRoles)
{
(((user.Roles & userRole) != 0) ? inRoles : outRoles).Add(roleName);
}I mean, yes it works. But... It's not there anymore: I changed the whole way I handle roles for other reasons. But still, It's a bit impenetrable...
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."
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).
-
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; // return should also say if the operation succeded or not => void->bool(or success/failure status).
if (string.IsNullOrEmpty(roleName)) // + other validation for 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.
-
I don't know - it isn't in my code anymore... I wrote this wonder this morning, and realized I could prune it down to just the one line:
private void SetRole(SMUser user, UserRole userRole, string roleName, List<string> inRoles, List<string> outRoles)
{
(((user.Roles & userRole) != 0) ? inRoles : outRoles).Add(roleName);
}I mean, yes it works. But... It's not there anymore: I changed the whole way I handle roles for other reasons. But still, It's a bit impenetrable...
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."
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).
-
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 ? ;P
-
What is user or user.Roles or any other one parameter is null ? ;P
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.
-
I don't know - it isn't in my code anymore... I wrote this wonder this morning, and realized I could prune it down to just the one line:
private void SetRole(SMUser user, UserRole userRole, string roleName, List<string> inRoles, List<string> outRoles)
{
(((user.Roles & userRole) != 0) ? inRoles : outRoles).Add(roleName);
}I mean, yes it works. But... It's not there anymore: I changed the whole way I handle roles for other reasons. But still, It's a bit impenetrable...
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."
No... It's programmer porno..!! lol
-
If you feel the need to comment the code,then the code is a horror. Good code shouldn't be dependent on comments to tell the system story. Or at least that is my 2 cents That said, this isn't a big horror, it clearly states what it does, 3 nitpick is the use of prefix in the SMUser class name, and that the name of the method is somewhat misleading, and that the method does two things that a) should be clearly specified in the method naming or b) refactored into two separate methods.
modified on Tuesday, August 30, 2011 6:21 AM
annathor wrote:
If you feel the need to comment the code,then the code is a horror.
Surely you don't mean that commenting code makes any code a horror..?
-
I don't know - it isn't in my code anymore... I wrote this wonder this morning, and realized I could prune it down to just the one line:
private void SetRole(SMUser user, UserRole userRole, string roleName, List<string> inRoles, List<string> outRoles)
{
(((user.Roles & userRole) != 0) ? inRoles : outRoles).Add(roleName);
}I mean, yes it works. But... It's not there anymore: I changed the whole way I handle roles for other reasons. But still, It's a bit impenetrable...
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."
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.
-
I don't know - it isn't in my code anymore... I wrote this wonder this morning, and realized I could prune it down to just the one line:
private void SetRole(SMUser user, UserRole userRole, string roleName, List<string> inRoles, List<string> outRoles)
{
(((user.Roles & userRole) != 0) ? inRoles : outRoles).Add(roleName);
}I mean, yes it works. But... It's not there anymore: I changed the whole way I handle roles for other reasons. But still, It's a bit impenetrable...
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."
-
annathor wrote:
If you feel the need to comment the code,then the code is a horror.
Surely you don't mean that commenting code makes any code a horror..?
No, not always, there are some rare cases where cluttering the code with comments are justified. But if you look at some code and thinks "gosh I better comment it, so other people understands it" then it is an indication that the code is a horror, why do you need to explain the system whit a language that is not designed to describe a system, when you allready have described it with a language that is designed to excplain a system? Wouldn't it be better to left click, refactor->rename and clean up the code so it tells the story you want it to tell, insted of trying to excplain the story by including a secondary language into your source files? also comments tends to become misleading and/or misplaced as the system evolves, so instead of excplaining the system it just adds to the confusion. so, hyperboled, I say, a well commented code is a indication that the system is dirty. and commenting in itself can also make clean code dirty by breaking up the logic and lie to you while you read the code. In most cases you could explain what the sysem is supposed to do better whit a programming language than whit english or swahili. And if you, god forbid, uses comments to add todos, changelogs or similar there are plenty of good tools out there for that.
-
No, not always, there are some rare cases where cluttering the code with comments are justified. But if you look at some code and thinks "gosh I better comment it, so other people understands it" then it is an indication that the code is a horror, why do you need to explain the system whit a language that is not designed to describe a system, when you allready have described it with a language that is designed to excplain a system? Wouldn't it be better to left click, refactor->rename and clean up the code so it tells the story you want it to tell, insted of trying to excplain the story by including a secondary language into your source files? also comments tends to become misleading and/or misplaced as the system evolves, so instead of excplaining the system it just adds to the confusion. so, hyperboled, I say, a well commented code is a indication that the system is dirty. and commenting in itself can also make clean code dirty by breaking up the logic and lie to you while you read the code. In most cases you could explain what the sysem is supposed to do better whit a programming language than whit english or swahili. And if you, god forbid, uses comments to add todos, changelogs or similar there are plenty of good tools out there for that.
Comments are beyond important. As a simple proof, using your IDE of choice, check the intelisense for a command, where does that come from? Comments. During development, it's a good idea to free-form what a class/variable/method is for, but how? Comments. Write down what you intended to happen, where you intend it to be, how? Comments? In short, comments should describe, in the natural language of the developers what the code is doing in the machine language. Simples.
Panic, Chaos, Destruction. My work here is done. Drink. Get drunk. Fall over - P O'H OK, I will win to day or my name isn't Ethel Crudacre! - DD Ethel Crudacre I cannot live by bread alone. Bacon and ketchup are needed as well. - Trollslayer Have a bit more patience with newbies. Of course some of them act dumb - they're often *students*, for heaven's sake - Terry Pratchett
-
If you feel the need to comment the code,then the code is a horror. Good code shouldn't be dependent on comments to tell the system story. Or at least that is my 2 cents That said, this isn't a big horror, it clearly states what it does, 3 nitpick is the use of prefix in the SMUser class name, and that the name of the method is somewhat misleading, and that the method does two things that a) should be clearly specified in the method naming or b) refactored into two separate methods.
modified on Tuesday, August 30, 2011 6:21 AM
Even though you are entitled to your own thoughts, I think most programmers would agree in favor of documentation, imagine Java or .NET libraries without the javadoc or help respectively. I would imagine it would take a lot of trial and error to get to what a function actually did, especially, if you are not the author of the function. Documentation exists, mostly, for people who are using or referring to your code after you have written it. In some cases, it also helps the original programmers themselves to recall why they did something in a way they did in a function. The irony is that if the code is really a horror, it would be equally difficult to document or comment it to begin with, atleast in a way that solves the riddle of a function in a more comprehensive way. I personally like to comment and document my code wherever I think is necessary to explain any nitty-gritty of it. Programmers move so fast, that 6 months down the line our own code seems difficult to decode sometimes and hence documentation really helps to quickly understand in plain and simple English or whatever language one may use, what the code does. Unless, the only language you speak is code and you have difficulty interpreting natural language more than a coding language. Cheers
