Re: style

From: Edward J Glamkowski (eglamkowski@angelfire.com)
Date: 03/26/00


>>1.  Initializations of pointers being removed.
>If you mean:
>
>act.comm.c, 'paper' is unconditionally
> initialized later.  act.informative.c: 'obj'
> is also.
>
>I could keep going but you get the idea.  If we
> don't initalize it, think of it as a guarantee
> that we won't just spontaneously use the
> variable.  It'll have something assigned to it
> (later) that is used.

This is a good case for C++ where you don't
need to declare a variable until you actually
need it.  But we aren't doing C++ (yet), and
I still don't like having uninitialized pointers
lying around, even if we are "guarenteed" to not
use it until we have initialized it.

Just because *you* guarantee the stock code
won't use until it is inited doesn't mean
someone else won't come along and modify the
code such that they try to use before your init,
only it hasn't been initialized so they get a
garbage pointer.

With a null pointer the game will just crash and
it is obvious there is a problem and where the
problem is; with a garbage pointer the game may
or may not crash and things may seem to be fine
for a while and then you add more stuff and more
stuff, and yet again still more stuff to the
code and then it finally starts crashing in the
old, bad code but it has been so long since you
looked at that section and you can't even be
sure any more that maybe you aren't getting a
memory stomp in some of the newer code instead
and you spend a whole day trying to track it
down.  Which has already happened to me :p

It doesn't take any effort to set a pointer to
NULL on declaration and it protects you from
future problems when you change things in
unexpected ways.

Maybe you don't care to do it in the stock code
and that is fine - you are the final arbiter of
these matters, but I'm certainly doing it for
my own code!


>>2.  Return statements at the end of void
>>functions are being removed.
> We're consistent, therefore they're useless.
> Your code may become unconsistant, but then
> you should fix the code, not add extra
> 'return;' lines that may be at the wrong
> indent level anyway.

But you still aren't entirely consistent:

void foo(...) {
  if (!GET_SKILL(ch, SKILL_whatever))
    return;

  one_argument(arg, buf)

  if (!*buf)
    return;

  /* etc. etc. blah blah blah */
}

You have returns for exiting the function in
the middle of the function, but not at the
end.  That, to me, is inconsistent - you exit
the function in some cases by using a return
statement, but not in all cases.  Obviuosly you
can't not have the returns in the middle, so the
only way to be consistent is to have it also at
the end.
In any event, the compiler *should* compile
the statement out (i.e. no extra instructions
will be generated), so it is really just a
matter of preference.  Again, you can do
whatever you want with the stock code, but
I prefer the self-consistency and so I'm
certainly doing it for my own code (and all
patches and snippets I upload :)


I also just noted in the patch the following:
      for (zvn = atoi(value), zrn = 0; zone_table[zrn].number != zvn && zrn <= top_of_zone_table; zrn++);  <-- semicolon at end of for loop :p
      if (zrn <= top_of_zone_table)
        print_zone_to_buf(buf, zrn);

which looks at first glance like some mis-
indented, i.e. broken, code.  If you have
a blank for loop, you really should comment
it, so nobody reports it later as a bug :p

      for (zvn = atoi(value), zrn = 0;
           zone_table[zrn].number != zvn &&
           zrn <= top_of_zone_table;
           zrn++)
        ; /* Just counting... */

Same thing for cases in a switch statement that
you are having fall through to the next case,
though I haven't explicitly seen that - it's
just a very similar situation that I feel should
always be commented when it is done.


But, I admit I am just nitpicking on all
presented issues here, so please don't hate
me  =)



Angelfire for your free web-based e-mail. http://www.angelfire.com


     +------------------------------------------------------------+
     | Ensure that you have read the CircleMUD Mailing List FAQ:  |
     |  http://qsilver.queensu.ca/~fletchra/Circle/list-faq.html  |
     +------------------------------------------------------------+



This archive was generated by hypermail 2b30 : 04/10/01 PDT