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
conditionals.
> 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
reason.
> 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? :)
Troy
--
"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