[Pidgin] #10891: gevolution plugin adds buddies to random account

[Pidgin]

#10891: gevolution plugin adds buddies to random account
-----------------------------------------+----------------------------------
 Reporter:  cedel                        |        Owner:         
     Type:  patch                        |       Status:  new    
Milestone:  Patches Needing Improvement  |    Component:  plugins
  Version:  2.6.3                        |   Resolution:         
 Keywords:  gevolution Evolution         |  
-----------------------------------------+----------------------------------

Comment(by darkrain42):

 That looks much better, thanks!

 I have a few comments:
 `if (( protocolname != NULL) && (strcmp(protocolname,
 (dialog->account)->protocol_id)) != 0)`
    * This should use the `purple_account_get_protocol_id` accessor
 function (there are a number of places where this is the case. In general,
 if you're doing "account->something", try to use an accessor function
 instead).
    * Can protocolname be NULL?  Under what circumstances does that happen?
 (Sorry, I'm not terribly familiar with the dialog in the first place.)

 The method you use to iterate over linked lists is inefficient. Doing:
 {{{
     int length = g_list_length(list);
     int i;
     for (i = 0; i < length; ++i) {
         data = g_list_nth_data(list, i);
         ...
     }
 }}}

 causes you to end up traversing the linked list a large number of times.
 For example, to retrieve the 2nd piece of data, g_list_nth_data has to
 traverse the first two links in the list, but to get the third, it has to
 traverse the first three links (this becomes inefficient very quickly).

 Instead, try something like this:
 {{{
     GList *node;

     node = list;
     while (node) {
         data = node->data;
         ...do something with data
         node = node->next;
     }
 }}}

 (hopefully that makes sense. If not, I can try to explain again)

 On line 445, {{{ t = find_account_for_protocol(dialog, protocolname); }}},
 but the value 't' does not appear to be used anywhere?  It looks like that
 function only ever returns 0, which is never used.  It should be void
 instead (i.e. returns no value).

-- 
Ticket URL: <http://developer.pidgin.im/ticket/10891#comment:6>
Pidgin <http://pidgin.im>
Pidgin