|
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
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.
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.
|
|
|
|
|
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.
|
|
|
|
|
You think debugging is more difficult with more functions? I disagree. My IDE steps into functions, or over them...which makes debugging simpler, not harder. I can step over functions I've already eliminated as the problem, and into those which could be an issue. Rather than stepping on every line of code.
And I should know if the bug is in a function or not by writing tests for it. My "STPVolume" or "GetVolumeAtSTP" function should have test methods ensuring it's accurate.
As for the name, physics isn't my domain, so I didn't know the proper name, I made my best guess. The essence of my point still stands.
|
|
|
|
|
Yes. IDE stepping is the last resort of debugging, one should be able to find the suspect piece of code quickly without diving through a big tree of methods. (I'm suffering from this at the moment in a real project, in fact.) Obviously, you don't just want the whole application in one big main(), but taking something which is only one or two lines out of line does not (imo) help readability. (Taking it out to avoid duplicating the calculation is a separate matter and a good reason to do so, because the pain of having duplicate code massively outweighs the problem of having too many functions.)
Your tests will tell you if the function is 'accurate', but it won't tell you some things which may be important when you want to use it (that you didn't think of at the time). For example, just what is standard temperature and pressure? ... there are different definitions and to find which one is being used you need to find the calculation. This is more common with more complex methods, or order of event firing or other slightly tricky things like that.
|
|
|
|
|
I've never heard of "the problem of having too many functions"...
Have you read books like Clean Code or Code Complete, etc?
|
|
|
|
|
No ... I'd like to get hold of Clean Code, I've heard good things about it. Like all philosophies, though, the idea of making things 'cleaner' by putting things into subfunctions can be taken too far.
|
|
|
|
|
@annathor
I mean no disrespect but you are %100 wrong about good code not needing comments. It is this kind of mindset, that everyone thinks exactly alike, that leads to so many re-writes and problems. I used to work for a software company that employed many developers who had this same approach to commenting anything development related and it lead to numerous hours of unnecessary support simply because the developer assumed that the way they saw some scenario was the only way and therefore the same way all others would see it.
This doesn't mean that ALL code must be commented for there are some simple ones that are fairly self-explanatory but never assume code can be left uncommented if its good code.
|
|
|
|
|
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
|
|
|
|
|
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);
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
