Hi Bartosz,
On Tue, Nov 16, 2010, Bartosz Szatkowski wrote:
> +static void contact_init(struct phonebook_contact **contact, char **reply)
> +{
> + if (*contact == NULL)
> + *contact = g_new0(struct phonebook_contact, 1);
Could you do this initialization before calling this function and pass
just a simple pointer to it. I think it'd be a cleaner approach.
> +static void contact_add_numbers(struct phonebook_contact **contact, char **reply)
Over 79 column line. Please split it.
> + if ((g_strcmp0(reply[COL_OTHER_NUMBER], reply[COL_CELL_NUMBER]) != 0) &&
> + (g_strcmp0(reply[COL_OTHER_NUMBER], reply[COL_WORK_NUMBER]) != 0) &&
> + (g_strcmp0(reply[COL_OTHER_NUMBER], reply[COL_HOME_NUMBER]) != 0))
> + add_phone_number(*contact, reply[COL_OTHER_NUMBER], TEL_TYPE_OTHER);
> +}
Same here. However, I think you can make it considerably more readable
with something like:
if (g_strcmp0(reply[COL_OTHER_NUMBER], reply[COL_CELL_NUMBER]) == 0)
return;
if (g_strcmp0(reply[COL_OTHER_NUMBER], reply[COL_WORK_NUMBER]) == 0)
return;
if (g_strcmp0(reply[COL_OTHER_NUMBER], reply[COL_HOME_NUMBER]) == 0)
return;
add_phone_number(*contact, reply[COL_OTHER_NUMBER], TEL_TYPE_OTHER);
> +static void contact_add_emails(struct phonebook_contact **contact, char **reply)
Over 79 column line.
> +{
> + add_email(*contact, reply[COL_HOME_EMAIL], EMAIL_TYPE_HOME);
> + add_email(*contact, reply[COL_WORK_EMAIL], EMAIL_TYPE_WORK);
> + add_email(*contact, reply[COL_OTHER_EMAIL], EMAIL_TYPE_OTHER);
> +}
Why does the function take a pointer to pointer when a simple pointer
would be enough?
> +static void contact_add_addresses(struct phonebook_contact **contact, char **reply)
Over 79 columns and unnecessary ** pointer again.
> + home_addr = g_strdup_printf("%s;%s;%s;%s;%s;%s;%s",
> + reply[COL_HOME_ADDR_POBOX], reply[COL_HOME_ADDR_EXT],
> + reply[COL_HOME_ADDR_STREET], reply[COL_HOME_ADDR_LOCALITY],
> + reply[COL_HOME_ADDR_REGION], reply[COL_HOME_ADDR_CODE],
> + reply[COL_HOME_ADDR_COUNTRY]);
> +
> + work_addr = g_strdup_printf("%s;%s;%s;%s;%s;%s;%s",
> + reply[COL_WORK_ADDR_POBOX], reply[COL_WORK_ADDR_EXT],
> + reply[COL_WORK_ADDR_STREET], reply[COL_WORK_ADDR_LOCALITY],
> + reply[COL_WORK_ADDR_REGION], reply[COL_WORK_ADDR_CODE],
> + reply[COL_WORK_ADDR_COUNTRY]);
> +
> + other_addr = g_strdup_printf("%s;%s;%s;%s;%s;%s;%s",
> + reply[COL_OTHER_ADDR_POBOX], reply[COL_OTHER_ADDR_EXT],
> + reply[COL_OTHER_ADDR_STREET], reply[COL_OTHER_ADDR_LOCALITY],
> + reply[COL_OTHER_ADDR_REGION], reply[COL_OTHER_ADDR_CODE],
> + reply[COL_OTHER_ADDR_COUNTRY]);
All of these are over 79 columns.
> +static void contact_add_organization(struct phonebook_contact **contact, char **reply)
Same here. And a simple * pointer is enough.
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
