On 10/27/07, Troy Curtis Jr <troycurtisjr@gmail.com> wrote:
> 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? :)
I just noticed that it is "Karl Fogel", not Kogel. Sorry about that
Karl! That's what I get for trying to do it by memory.
>
> 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/)
>
--
"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:24:58 2007