Navigation

Language Abuse

Monday, January 29th, 2007

Alright, this is one of the dumber things I’ve seen in a while.


/*!
 * @brief Send an auto-reply
 *
 * Sends our current auto-reply to the specified contact.
 * @param source Account sending the object
 * @param destination Contact receiving the object
 * @param chat Chat the communication is occuring over
 */
- (void)sendAutoReplyFromAccount:(id)source toContact:(id)destination onChat:(AIChat *)chat
{
    AIContentMessage *responseContent;
    NSAttributedString *autoReply;

    if ((autoReply = [[[chat account] statusState] autoReply])) {
        responseContent = [AIContentMessage messageInChat:chat
                                               withSource:source
                                              destination:destination
                                                     date:nil
                                                  message:autoReply
                                                autoreply:YES];
        
        [[adium contentController] sendContentObject:responseContent];
    }
}

Anyone spot the danger here? I’m inserting some code to make us handle autoreplies better when the IM service isn’t expecting it, so I do this:


...
NSAttributedString *autoReply;
BOOL supportsAutoreply = [source supportsAutoReplies];
        
//If the service isn't natively expecting an autoresponse, make it a bit clearer what's going on
if (!supportsAutoreply) {
    NSMutableAttributedString *mutableAutoReply = [[autoReply mutableCopy] autorelease];
    [mutableAutoReply replaceCharactersInRange:NSMakeRange(0, 0) 
                                    withString:AILocalizedString(@"(Autoreply) ", 
                                        "Prefix to place before autoreplies on services which do not natively support them")];
    autoReply = mutableAutoReply;
}
    
if ((autoReply = [[[chat account] statusState] autoReply])) {
    responseContent = [AIContentMessage messageInChat:chat
                                           withSource:source
...

And then I wonder why it’s throwing exceptions when I come back to test it a few days later. Sigh. Please initialize your variables, folks. I think this code is an unfortunate child of the following (useful) idiom:


NSEnumerator *enumerator = [array enumerator];
id obj = nil;
    
while ((obj = [enumerator nextObject])) {
    //...
}

I realize that it is my mistake for reading the code incorrectly, but you should always write your code assuming it will be read by a drooling idiot at 4 in the morning coming off a 5-cup coffee high. The fact that I have yet to encounter a programming language in-use that does not support comments is (partial) testament to this as well understood design practice. Seriously.

Update: Fixed in revision 18739.

Comments

  1. Stephen Holt replied on January 29th, 2007:

    I’ve actually seen this idiom used in some C code. It’s not wrong, per se, but it is a style issue, and I believe unnecessary/redundant with ObjC/Cocoa.

  2. Colin replied on January 29th, 2007:

    Well the idiom I pasted will become 100% irrelevant with Leopard’s for in loops:

    for (id obj in array) {
        //...
    }
    

    N.B. That last part is not under NDA, see this post, particularly the comment by Chris on 8/30/06 at 1pm.