Hacker News Reader

Comment by cFire | Hacker News Reader

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.

View on HN

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)

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.)

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

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.

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.

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

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

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

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.

		# 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.

Reply on news.ycombinator.com