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."
-
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
-
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.
I say you're just mostly wrong about that. Often it's as important, if not more so, to know WHY something is coded like it is as to see the code itself. Comments should tie the parts together and provide a concise lead-in to the code itself. Not to say that logical procedure and variable naming isn't a huge help, but good comments can make big difference. Of course the unfortunate thing is usually that bad code is accompanied by bad or no comments.
-
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
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.
-
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 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. :-D
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
-
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.
I agree with you in principle: the code should be readable, so comments, or at least those which simply describe what is being done, should be unnecessary. However, we need comments for two reasons:
-
To cover why we are doing it. For simple cases this might be obvious from a function name, but even for trivial calculations, describing why you're doing it is useful. For example, which of these is clearer?
surfaceVolume = volume * 298 * p / (T * 101325);
or
// Volume by ideal gas law at STP (25C, 1 atm)
surfaceVolume = volume * 298 * p / (T * 101325);(Yeah I know I should have consts for those values and in similar real code I do.)
-
Sometimes the language feature won't let us express our thought without a bunch of syntax. In a procedural scalar language like what most of us use, this is particularly true for array operations. E.g.
// a + b
double[] r = new double[a.Length];
for(int i = 0; i < r.Length; i++) r[i] = a + b;Experienced people might be able to read a loop and immediately process it to what it represents, but many won't be able to. Linq helps with certain categories of syntax (complex foreach loops to filter an enumeration), but array-based maths still needs comments for what you really mean.
-
-
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."
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.
-
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.
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."
-
I agree with you in principle: the code should be readable, so comments, or at least those which simply describe what is being done, should be unnecessary. However, we need comments for two reasons:
-
To cover why we are doing it. For simple cases this might be obvious from a function name, but even for trivial calculations, describing why you're doing it is useful. For example, which of these is clearer?
surfaceVolume = volume * 298 * p / (T * 101325);
or
// Volume by ideal gas law at STP (25C, 1 atm)
surfaceVolume = volume * 298 * p / (T * 101325);(Yeah I know I should have consts for those values and in similar real code I do.)
-
Sometimes the language feature won't let us express our thought without a bunch of syntax. In a procedural scalar language like what most of us use, this is particularly true for array operations. E.g.
// a + b
double[] r = new double[a.Length];
for(int i = 0; i < r.Length; i++) r[i] = a + b;Experienced people might be able to read a loop and immediately process it to what it represents, but many won't be able to. Linq helps with certain categories of syntax (complex foreach loops to filter an enumeration), but array-based maths still needs comments for what you really mean.
// Volume by ideal gas law at STP (25C, 1 atm)
surfaceVolume = volume * 298 * p / (T * 101325);Or even better...put that formula in a well named function and call it, so your call looks something like
surfaceVolume = CalculateVolumeByIdealGasLaw(volume, pressure, temperature);
Now you don't need the comment. And your code is easier to test.
-
-
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
-
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
-
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."
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!
-
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
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! :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."
-
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)
-
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
-
(((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.
-
// Volume by ideal gas law at STP (25C, 1 atm)
surfaceVolume = volume * 298 * p / (T * 101325);Or even better...put that formula in a well named function and call it, so your call looks something like
surfaceVolume = CalculateVolumeByIdealGasLaw(volume, pressure, temperature);
Now you don't need the comment. And your code is easier to test.
Up to a point. Code which contains nothing but several levels of 'well named functions' wrapped around a simple arithmetic calculation are a pain to debug (trying to actually find the places where calculations are done and therefore being able to see whether they're done right). It's kind of like trying to read a book by just looking at the contents page. It's hard to read such long names, too (unless you put in underscores to make it look like a sentence but then the arguments seem to be bound to the last word). This would go in a function for me if I were using it in more than one place (which in this example I almost certainly would be). Your proposed name misses the most important point, by the way. It should be called "STPVolume" or "GetVolumeAtSTP" or something like that.
-
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.
