[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

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

From: Troy Curtis Jr <troycurtisjr_at_gmail.com>
Date: 2007-10-27 22:17:13 CEST

On 10/27/07, Stefan Sperling <stsp@elego.de> wrote:
> 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.

Yep, the Subversion style is a little different than I am used to. I
think I may like it better, at least I caught in my other

> 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 went with this one so that the return type is satisfied. I didn't
use the tertiary out of habit as they are discouraged at work for some

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

Well you are right of course. And my lack of documentation at the
beginning certainly makes it plain that I was "whacking and hacking"
when I started it!

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

I certainly agree with this. I've been using its documentation and
consistent style (if not the ACTUAL style definition) as a model for
my own work. It just takes some getting used to :).

> 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

Thanks for the pointers. The fact that you didn't say "Oh my goodness
you shouldn't have even bothered to code that!" or "Why did you do
THAT!" is promising. I guess I start in on the other commands. And
then I'll work on figuring out those tests. I know that kogel
recommended writing the tests before doing the code, but how do I test
that my tests are doing what they are supposed to be doing? :)


"Beware of spyware. If you can, use the Firefox browser." - USA Today
Download now at http://getfirefox.com
Registered Linux User #354814 ( http://counter.li.org/)
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 27 22:17:25 2007

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.