[Bug 27135] review TplConfIface

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=27135

--- Comment #9 from Danielle Madeley <danielle.madeley at collabora.co.uk> 2010-04-29 20:37:12 PDT ---
In 88932c31afa436ae28c10327df1d229898d0687b:

You convert a GSList to a GList, which seems redundant. Do you ever go
backwards through the list? Why not leave it as a GSList?

Also, iterating and using a g_list_append() makes your conversion function
O(n^2). Use g_list_prepend() and reverse the list afterwards.

Why is your property a GPtrArray boxed type? Is this property ever exported
over D-Bus? Otherwise perhaps just make the property G_TYPE_POINTER and pass
across the GSList?

In 105cc67e5cbfa0e3eeec8fa996a391474fa905e0:

Why aren't these utilities one single utility (i.e. tpl-tool). You should use
the option parsing in GLib to parse your command line options, rather than
doing it yourself. Why isn't the binary in the same directory as the
telepathy-logger binary? Why create a new directory?

In 0ab957b0106cc205fd11854d1e5bbb886c4b1b03:

tpl_observer_conf_globally_enabled_update_cb: could 'val' have a better name?
'enabled' perhaps.

l = g_list_append (l, g_strdup (account));

g_list_prepend is better. Why are you dupping the account path? Who frees the
account path and the list, why doesn't
tpl_observer_get_open_channels_filtered_by_account() take a const GSList? If
you didn't return GPtrArrays, you could skip all of this code.

In 204b09848a26c17b17556e5ebdefd0b468dcc4ac:

Why not just include telepathy-glib.h ?

In 19a98b51f09d7e554e83faba6e25440d398c2bc7:

You can just put one single 'tags' in the list, which will match files called
'tags' in all subdirectories. Like we do for '.*.sw?'

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.