Hacker News Reader

Comment by wulczer | Hacker News Reader

https://github.com/postgres/postgres/blob/master/src/backend...

In general, most of Postgres code.

I prefer a far more stilted comment style otherwise you have trouble remembering the code between the comments and tend to scroll a lot more. I mean /* The error cases here shouldn't happen, but check anyway */ is really not that helpful.

Also, all the long descriptions before function calls are mostly redundant. If i want to know what a function does I can go there and look, but it really should have a sufficiently descriptive name that after the first time I don't need to check again most of the time. Unless something non obvious is going on.

I think it is helpful to know if the check is just a sanity check or if there's an already known set of conditions can lead to that particular error.

The function comments I also find very useful. Read a few of them and see how much information they carry. Preconditions for the function, reasons why it does what it does, assumptions it makes... These functions are used from throughout the code and it's important to document them well.

I'm purposefully not quoting specific parts of the file, because of course if you look at each and every one of them, you'll find a few that could be improved. But the OP asked for a well commented code base and if PostgreSQL is not one, then I don't know what would be.

I could argue that comments are best for exceptional behavior so if it's normal to do a lot of sanity checks there is little reason to comment on each one. However, my point was saying something like:

  if (error) //sanity check

Saves space and get's the same point across without padding the line count.

I actually rather like that comment. When reading code I find it useful to figure out how each error condition can be hit, so it's nice to know that something is just a sanity check and shouldn't ever actually happen.

At first brush those comments seem overly verbose and obvious:

    /* Do we have any named arguments? */
    ...
    /* If so, we must apply reorder_function_arguments */
    if (has_named_args)
    {
        args = reorder_function_arguments(args, func_tuple);

Though I do appreciate comments such as this:

  /* Fetch the function body */
  tmp = SysCacheGetAttr(PROCOID, func_tuple, num_pg_proc_prosrc,  &isNull);

Newcomers have be be quite familiar with the internals to immediately see that the line fetches the function body.

Reply on news.ycombinator.com