17 replies to this topic

[+ultimate99]

Can you please check my comments in this code?
Do I need to add more, be more specific?
What about JavaDoc, do I need any?


THanks!

Attached Files

•  TTT.zip   1.69K   26 downloads

[nik louch]

Not downloading. But comment as little or as much as you need, surely?

[still1]

The comments are ok but i have some suggestions I wouldn't comment that explains what the code is doing literally.

--Wouldn't use these--

//Increment by 1
//When count reaches 9 or above, check for winner -- this one is ok but its better to make it more generic

--Would use more like this--

//Check for placed "X" on buttons
//Display output if "X" wins


I would comment on modules or block of functionality rather than literally commenting what the code is doing

[Singh400]

Might be easier if you upload the code to PasteBin or use the [CODE] tags here.

[+Majesticmerc]

The golden rule of comments: The code should describe WHAT is happening, the comments should explain WHY its happening.

If you have to write a comment explaining what a piece of code is doing, you should be re-writing it to make it more obvious. Comments like "// Increment by 1" are redundant, as I could very easily look at the code to figure that out. On the other hand, if a piece of code is doing something, but it's not immediately obvious WHY it's doing it, just add a brief comment to explain why the code is the way it is (for example, if you have to write a hack to work around a bug in someone elses code, explain why the hack is there).

[+ultimate99]

THanks for the comments so far.
Here's pastebin: http://pastebin.com/fB05bkt9

[-Alex-]

Link is broken!

[firey]

I comment the code in a way anyone reading it (that knows the language or not) understands it. My comments are almost pseudo code. So I do things like:


//Okay, first we need to build our query
string q = "SELECT..........";

//Now, we need the result to give us the items
string result = clsMain.Database.dbQuery(q);

//Now split it up to get all the parts
string[] split = result.Split(';');

//And finally loop through the results
foreach (string s in split)
if (s != null)
blah;
else
break;

//Now we move to getting the users....

[vhane]

firey, on 17 May 2012 - 12:08, said:

I comment the code in a way anyone reading it (that knows the language or not) understands it. My comments are almost pseudo code. So I do things like:


//Okay, first we need to build our query
string q = "SELECT..........";

//Now, we need the result to give us the items
string result = clsMain.Database.dbQuery(q);

//Now split it up to get all the parts
string[] split = result.Split(';');

//And finally loop through the results
foreach (string s in split)
if (s != null)
blah;
else
break;

//Now we move to getting the users....

Your comments shouldn't read like pseudo code. I wouldn't cater for people who aren't familiar with the language/framework. By over-commenting you are making the code harder to read for people who actually know the language/framework. The latter should be your audience since they are the ones who are most likely to be reading your code and have a use for it in the first place.

Do not describe what the code is doing. Just document why you are doing what you are doing.

You should also pick better variable names. Once you do that you'll find that the above code doesn't need to be commented at all. So, instead of saying something like:

//Okay, first we need to build our query
string q = "SELECT..........";

You'd do:

string query = "SELECT..........";

Also, if you find yourself having to split strings that you've just grabbed from your database, you should consider moving that data out into another table.

[firey]

vhane, on 18 May 2012 - 03:40, said:

Your comments shouldn't read like pseudo code. I wouldn't cater for people who aren't familiar with the language/framework. By over-commenting you are making the code harder to read for people who actually know the language/framework. The latter should be your audience since they are the ones who are most likely to be reading your code and have a use for it in the first place.

Do not describe what the code is doing. Just document why you are doing what you are doing.

You should also pick better variable names. Once you do that you'll find that the above code doesn't need to be commented at all. So, instead of saying something like:

//Okay, first we need to build our query
string q = "SELECT..........";

You'd do:

string query = "SELECT..........";

Also, if you find yourself having to split strings that you've just grabbed from your database, you should consider moving that data out into another table.

Trust me, I go through code that other people write, the last thing I want to do is figure out what the hell they were doing. I would rather just read the comment and know what they were thinking when they wrote it. There are a million and one ways to write a set of code. I'd rather not have to run through the code in my head (even though I know the language). So I do what works for me.

Also, when I pull date from a database I often pull multiple fields. I then split it up as not all the fields will be used for one thing.

For example.. I may pull a userID, userFirstName, userLastName, userEmail, userDOB. I then split it up and fill in the appropriate input fields with the data. I don't use Datasets, I don't use gridviews, I don't use built in database connections.

[vhane]

firey, on 18 May 2012 - 04:00, said:

Trust me, I go through code that other people write, the last thing I want to do is figure out what the hell they were doing. I would rather just read the comment and know what they were thinking when they wrote it. There are a million and one ways to write a set of code. I'd rather not have to run through the code in my head (even though I know the language). So I do what works for me.

You can figure out what the code is doing by reading the code itself. Trust me, it becomes second nature after a while and you won't need to translate the code to english in your head.

The thing with comments is that you can only trust them so far. Comments have to be maintained to and they can get outdated. At the end of the day, the code is what gets executed.

Don't over-comment. Don't repeat yourself.

[Brian M]

I always stick to javadoc conventions, and attach comments to the end of lines if needed. I also tend to use very verbose method names (which I guess is an overhang since my primary language is Objective-c, but it makes it much easier to understand your code later on, and makes life much easier for anybody who needs to change/refactor things).

/**
* Brief description of what the method does. Followed by a
* short description of why it does something, or how it does it
* (if it's a weird way/important/nice to know).
*
* @param   x   Description of variable x
* @param   y   Description of variable y
* @return   What the method returns
* @throws  IOException   Reason for throwing an IOException
*/
public float calculateAreaOfCube(int x, int y) throws IOException
{
   return (x*y);
}

[sbauer]

firey, on 17 May 2012 - 12:08, said:

I comment the code in a way anyone reading it (that knows the language or not) understands it. My comments are almost pseudo code. So I do things like:


//Okay, first we need to build our query
string q = "SELECT..........";

//Now, we need the result to give us the items
string result = clsMain.Database.dbQuery(q);

//Now split it up to get all the parts
string[] split = result.Split(';');

//And finally loop through the results
foreach (string s in split)
if (s != null)
blah;
else
break;

//Now we move to getting the users....

I read through a lot of code as well and these comments would annoy me. They're not adding anything at all. clsMain bothers me as well.

//Okay, first we need to build our query
string q = "SELECT..........";

Like another person said you could just name the variable query and remove the comment.

//Now, we need the result to give us the items
string result = clsMain.Database.dbQuery(q);

What does the result represent? Also, dbQuery method is pretty redundant. You're in the context of the database already so the db in front of query isn't needed.

//Now split it up to get all the parts
string[] split = result.Split(';');

Your comment is telling us that we're splitting the result into "parts". Well yeah, the code tells us that much. Actually, the code tells us more. What are the parts? Again, what does the original result represent? Meaningful variable names would go a long way. You could also refactor this into a new method with a descriptive name. The comment just isn't needed.

//And finally loop through the results
foreach (string s in split)
if (s != null)
blah;
else
break;

The comment isn't very useful. The foreach is a loop so we know we're looping. The variable names leave a lot to be desired so it's hard to guess what this is trying to do.
You could refactor this into a method as well and give it a descriptive name.

[firey]

sbauer, on 04 June 2012 - 14:20, said:

I read through a lot of code as well and these comments would annoy me. They're not adding anything at all. clsMain bothers me as well.

//Okay, first we need to build our query
string q = "SELECT..........";

Like another person said you could just name the variable query and remove the comment.

//Now, we need the result to give us the items
string result = clsMain.Database.dbQuery(q);

What does the result represent? Also, dbQuery method is pretty redundant. You're in the context of the database already so the db in front of query isn't needed.

//Now split it up to get all the parts
string[] split = result.Split(';');

Your comment is telling us that we're splitting the result into "parts". Well yeah, the code tells us that much. Actually, the code tells us more. What are the parts? Again, what does the original result represent? Meaningful variable names would go a long way. You could also refactor this into a new method with a descriptive name. The comment just isn't needed.

//And finally loop through the results
foreach (string s in split)
if (s != null)
blah;
else
break;

The comment isn't very useful. The foreach is a loop so we know we're looping. The variable names leave a lot to be desired so it's hard to guess what this is trying to do.
You could refactor this into a method as well and give it a descriptive name.

As I described above.. my comments are like pesudo code. They explain my line of thinking and what I was doing when I wrote the code.

It's almost like a plain english version of the code. I try to reduce code as much as I can (methods only when required, non-temp variables when required, etc). I also name my things not based on the end context, but as it is now.

For example.. I call all my classes "cls[Name]" even though I could just do it as "Name". That's just how I program.

And the "clsMain" well it's the Main Class, and such it is named that.

[sbauer]

firey, on 04 June 2012 - 14:43, said:

As I described above.. my comments are like pesudo code. They explain my line of thinking and what I was doing when I wrote the code.

It's almost like a plain english version of the code. I try to reduce code as much as I can (methods only when required, non-temp variables when required, etc). I also name my things not based on the end context, but as it is now.

I know. I read your original post. I just disagree with it. I just don't think "that's just how I program" is a good reason for doing anything. Anybody could say the same thing and we would never improve on anything. In 97 I was writing Perl with flat files. The code was awful and needed improvement. I could have said, "Well, this is just how I program" and dismissed improvement opportunities.

Quote

For example.. I call all my classes "cls[Name]" even though I could just do it as "Name". That's just how I program.

But why? Why do you do that?

Quote

And the "clsMain" well it's the Main Class, and such it is named that.

What's a main class? Are we talking about your program's entry point or is it a god object?

Everyone should really check out the book "Clean Code" by Robert Martin. I think it should be required reading for everyone.