current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I have some code that I've included in a project I want to release. The problem is, I've only had time to code it late at night, when my brain is working at a less-than-optimal state. Therefore, I've ended up with some blocks of code that are themselves, less-than-optimal. Here is one such block:

NSString *path = nil;
if (delegate && [delegate respondsToSelector:@selector(toolbarAssociatedXibName:)]) {
    path = [delegate toolbarAssociatedXibName:_toolbar];
    if (![[NSBundle mainBundle] pathForResource:path ofType:@"nib"]) {
        IFLog(@"Delegate responded with an invalid value (%@) for `toolbarAssociatedXibName:`. Attempting default value (%@).", path, _identifier);
        path = _identifier;
    }
} else {
    path = _identifier;
}

Rewritten in pseudocode, for those who don't write in Objective-C:

Variable declaration (set to null value)
If there is an option to set the variable to a custom value
    Set it to that custom value
    If the value is not valid
        Log the error
        Set the value to a specified default
Otherwise
    Set the value to a specified default

Is it possible to shorten/compact this block of code? Normally, I would go for something like a ternary operator and some clever hacking, but I haven't been able to come up with something smarter than this (perhaps I should get some sleep). Any suggestions?

Edit: I have also tried combining this block into a single line of code, but that hasn't done any good either. I end up with something looking like this:

(delegate && [delegate respondsToSelector:@selector(toolbarAssociatedXibName:)] ? !![[NSBundle mainBundle] pathForResource:[delegate toolbarAssociatedXibName:_toolbar] ofType:@"nib"] ? path = [delegate toolbarAssociatedXibName:_toolbar] : IFLog(@"Delegate responded with an invalid value (%@) for `toolbarAssociatedXibName:`. Attempting default value (%@).", [delegate toolbarAssociatedXibName:_toolbar], _identifier), path = _identifier : path = _identifier);

That does absolutely nothing for readability. I guess there isn't much that can be done here...

share|improve this question

2 Answers 2

up vote 5 down vote accepted

In the spirit of DRY, how about:

NSString *path = nil;
if (delegate && [delegate respondsToSelector:@selector(toolbarAssociatedXibName:)]) {
    path = [delegate toolbarAssociatedXibName:_toolbar];
    if (![[NSBundle mainBundle] pathForResource:path ofType:@"nib"]) {
        IFLog(@"Delegate responded with an invalid value (%@) for `toolbarAssociatedXibName:`. Attempting default value (%@).", path, _identifier);
        path = nil;
    }
}
if (!path) path = _identifier;

In your pseudocode:

Variable declaration (set to null value)
If there is an option to set the variable to a custom value
    Set it to that custom value
    If the value is not valid
        Log the error
        Set the value back to null
If value is null
    Set the value to a specified default
share|improve this answer
    
Well, that's almost line-for-line the same as my original code, and achieves the same effect. Thanks for the effort, though... – Itai Ferber Jul 15 '11 at 18:07
    
Agreed; the difference is minor, but you asked about optimizing the style. Oversimplified, you're setting it to A then maybe to B then maybe back to A again. Instead, set it to B, and if that doesn't work, set it to A. Makes it easier to read and would run a tiny bit faster, and in similar cases, avoid any side effects of accessing identifier. – mackworth Jul 15 '11 at 18:39
    
That's certainly true, but in that case, why not use my original code? Instead of executing another if-statement, just use the else case. That's more of what I'm wondering about. – Itai Ferber Jul 15 '11 at 20:28
    
Because you still have two separate references to the default case. DRY suggests that anytime you repeat code, find a way to coalesce. My goal in writing code isn't to find the physically shortest code (e.g. your ternary scenario), but the simplest/clearest. Say at some future time, you need to change the default to a different version, then you have to change both places. Again, I certainly acknowledge this is not the most critical example of it, but every little bit helps... – mackworth Jul 15 '11 at 20:43
    
I see your point. – Itai Ferber Jul 15 '11 at 20:53
up vote 0 down vote

Maybe you want something like this, (you will also need to log the error though)

(delegate && [delegate respondsToSelector:@selector(toolbarAssociatedXibName)]) ? path = [delegate toolbarAssociatedXibName:_toolbar] : path = _identifier;
share|improve this answer
    
I went with this approach at first, but the problem is is that this does no checks for validity and does not log any problems... – Itai Ferber Jul 13 '11 at 15:27

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.