Re: [PATCH-REVIEW] Relative URL support for info command

On Sat, Oct 27, 2007 at 10:57:46AM -0500, Troy Curtis Jr wrote:
> **NOTE: I am asking for a quick review of my code.

I had a quick look over it.

Found nothing bad, except this piece:

        svn_boolean_t
        svn_path_is_relative_url(const char *url)
        {
            if(0 == strncmp("^/", url, 2))
                return TRUE;
            else
                return FALSE;
        }

First, you are missing a space after 'if'.
See http://subversion.tigris.org/hacking.html#coding-style
It helps if all code adheres to the conventions.

Second, you could simply do this instead:

        svn_boolean_t
        svn_path_is_relative_url(const char *url)
        {
            return (0 == strncmp("^/", url, 2));
        }

"If (true) return true; else return false;" is redundant.

But (as I assume) in case anyone ever swaps the values of TRUE and
FALSE in svn_types.h (oh, the horror), I've seen this idiom being
used instead in the subversion code:

        svn_boolean_t
        svn_path_is_relative_url(const char *url)
        {
            return (0 == strncmp("^/", url, 2) ? TRUE : FALSE);
        }

I think this looks ridiculous as well but it's being used
in many places, and it fits on one line. There is probably
some good reason for it... (Is there?)

> My function prototypes are not properly documented yet

My advice: Don't do this! Instead:

        Document your functions *before* writing them.

Then keep the docs in sync with the code whenever you change
something.

This helps you keep thinking about what you're doing, and you
actually have the docs ready by the time you want to send a patch.

The docs also help others because they explain your intentions.
Other people can use this information to help you verify that
your intentions match what the code actually does.
And no, a quick summary of what a function should do in the
body of your submission email does not count as documentation.

The amount and quality of the documentation inside the Subversion
code base is one of its best advantages. Don't miss out on it.

Lack of docs makes your code less accessible.
"Code without docs is like hardware without specs." :)

-- 
Stefan Sperling <stsp@elego.de>                 Software Developer
elego Software Solutions GmbH                            HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12        Tel:  +49 30 23 45 86 96 
13355 Berlin                              Fax:  +49 30 23 45 86 95
http://www.elego.de                 Geschaeftsfuehrer: Olaf Wagner