[cFire]

In my experience it's useful to add a little more documentation/comments to your code. It makes it a lot more readable for anyone else and more importantly, a lot easier for yourself when you're writing updates a while from now.

I like to have a little comment above every function that explains what it does at least. On top of that I usually put a single line above every logical block of code. Something like "Loop trough list of someobject." It seems obvious but it does make it a lot easier to read back code later.

[kyberias]

> On top of that I usually put a single line above every logical block of code. Something like "Loop trough list of someobject." It seems obvious but it does make it a lot easier to read back code later.

I have to respectfully disagree. This is really, really, REALLY BAD advice.

One should strive to write clean code with short functions, well-named variables and parameters, so that it's easy to understand without redundant comments like "loop trough (sic.) list...". Programming languages have looping constructs (e.g. for-statement). Therefore one does not need to write a comment that describes what a for-statement does.

Also, do not write comments that explain what a function does. Rather, name the function so that it is obvious from the name what it does. If the function does multiple things and name would be too long, split the function into logical subroutines and name them all properly.

[cFire]

To illustrate, here's something I wrote not too long ago: https://gitlab.insomnia247.nl/coolfire/pfsense-ident-proxy/b...

It's a little quick-and dirty but it's the way it's commented that makes the point I'm going for here.

[kyberias]

Here's an example. You write:

  # Check if it's something that looks like an ident response
  		if( input =~ /^(\d+)(|\s),(|\s)(\d+)$/ )
  			p1 = $1.to_i
  			p2 = $4.to_i
  		else
  			sane = false
  		end

Don't do this (comment a block of code).

Rather make a function that does the same and name it for example "ValidateIdentResponse". Write a test for it. Call that function from this function. You get shorter, less-complex main function and you don't need that silly comment.

(EDIT: Formatting)

[yoklov]

Worth keeping in mind that you don't always want to split something into a separate function. When you split out a function, you should be asking yourself if you're actually simplifying something, or if you're just sweeping the complexity under the rug. If the person reading the first function will typically need to see the definition of the helper, the helper's definition is non-obvious, and the helper wouldn't be used elsewhere, it's probably not worth splitting out.

Another important consideration is whether or not the code block you split out makes any assumptions about the current state of the program (or object or whatever), and if those assumptions not being met would result in non-obvious bugs. (If this is true, you should probably wait until there's actual duplication somewhere before splitting the function out, IMO.) This is obviously not an issue with pure functions, but for most programs, most functions aren't pure.

See http://number-none.com/blow/john_carmack_on_inlined_code.htm... for further perspective on this.

[cFire]

Valid point. In this case a separate function would be better. It's not always an option though.

Edit: Rather I should say; It's not always an option that makes things better.

If you're writing a comment for a block of code is definitely a good time to think "Should this not be a separate function?" (I clearly need to do this more myself too.)

[kyberias]

One good resource for learning and inspiration is Robert C. Martin's book Clean Code. Opinionated, yes, but a good read none the less.

[andrey-p]

Besides unit testing, with regexes I always put a quick comment like "matches foo but not bar or baz" which might not be comprehensive but allows the next person to get a quick understanding of what the code does at a glance.

[mediascreen]

I like what Jeff Atwood wrote: "Code Tells You How, Comments Tell You Why"

http://blog.codinghorror.com/code-tells-you-how-comments-tel...

[xavierd]

I would say you're documenting way too much: the obvious shouldn't need documentation.

[cFire]

It's more a matter of what you consider to be obvious in this case. In my experience, especially regex matching can become very not-so-obvious if you're coming back to code you wrote some time ago. Or if someone else is reading it.

[richardwhiuk]

		# Do NAT table lookup
		n = natlookup( p1, p2 )

is clearly unnecessary. If natlookup doesn't do a NAT table lookup, it's a badly named function.

[jacquesm]

Comments should clarify, not repeat what the code already says.