Self Help (1)

[Michael Meeks <michael ximian com>]

	So ... Miguel asked me to post these on outside the company,
I've been trying to codify some useful accretions of programming
knowledge into weekly mails. Anyhow; perhaps interesting, perhaps not,
hopefully someone will learn something from them, sometime :-)

	I'll do 1 per week for a while perhaps.

Hi Guys,

        Since I'm boring and have to read a lot of code, I thought I'd
try and codify some of the more stupid things I've learned in my time
and note down some patterns that work for me and serialize these into
a tip for the week - so, since everyone reads dung, by concealing such
things behind an amusing title there is a chance that I can teach
someone something:


* Fatal instinct:

[ broken ]
        GSList *l;

        for (l = master; l; l = l->next) {

                if (matches (l->data, filter))

                        master = g_slist_remove (
                                master, l->data);
        }

        So ... the g_slist_remove modifies the list under the loop
walking the list, often the 'remove' is less obvious, but this is
always broken, a better way to do it is this:

        for (l = priv->listeners; l; l = next) {
                ListenerDesc *desc = l->data;

                next = l->next;

                if (desc->id == id) {
                        priv->listeners = g_list_remove_link (
                                priv->listeners, l);
                        g_list_free_1 (l);
                        desc_free (desc, ev);
                        return;
                }
        }

        Not only does this use the improved, and clearer 'next'
variable to move onto the next list item, but it also uses
g_list_remove_link, which removes the node without scanning
the whole list for elements matching that data pointer.


* A good pattern

        When iterating lists, there is no point in using the
g_list_first / next () macros; as so much code uses the direct
structure names, it is not possible to change them, and they only
clutter the code. I also find the "for ()" idiom very clear, in
contrast to the while idiom:

 Nice.                                  Nasty.
 ----                                   -----

 GSList *l                              GSList *l

 for (l = start; l; l = l->next) {      l = start;
        ...                             while (l) {
 }                                              ...
                                                l = l->next;
                                        }

        Not only is the latter longer, but the full body of the loop
needs to be inspected to ascertain what exactly is going on, and
clearly 'continue' cannot be used to nice effect.


* A lesson in taste

        By extensive experimentation, and careful measuring of code
quality across millions of lines of code, it has been conclusively
discovered that 2, 3 or 4 stop indents are the devil's tool ? tabs are
8 stops, so are indents.

cf. /usr/doc/linux/Documentation/CodingStyle - chapter 1.


* Did you know

        That in both glib 1 and 2, g_hash_table insert does a strange
thing, so - people who don't like leaks do things like this when they
insert into a table:

[ broken ]
void
my_insert_fn (GHashTable *ht, const char *foo, gpointer value)
{
        gpointer old_key;
        gpointer old_value;

        if (g_hash_table_lookup_extended (ht, foo, &old_key, &old_value))
                g_free (old_key);

        g_hash_table_insert (ht, g_strdup (foo), value);
}

        Sadly, this code is broken, since when a duplicate node is
inserted into a hash table it is the old key is retained, and the new
key is not inserted [ apparently this is a feature ].

        There are several ways to fix the code, one of the easiest is:

  void
  my_insert_fn (GHashTable *ht, const char *foo, gpointer value)
  {
        gpointer old_key;
        gpointer old_value;

        if (g_hash_table_lookup_extended (ht, foo, &old_key,
&old_value)) {
+               g_hash_table_remove (ht, foo);
+               g_free (old_key);
+       }

        g_hash_table_insert (ht, g_strdup (foo), value);
  }


        All of the above errors as seen in real code written by
supposedly competant people :-)

        Next week: bonobo_x_object, inheriting vs. aggregating, and
more.

        HTH,

                Michael.

-- 
 mmeeks gnu org  <><, Pseudo Engineer, itinerant idiot