1
\$\begingroup\$

I have some code that I've included in a project I want to release. 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;
}

The equivalent 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 to a specified default
Otherwise
    Set the value to a specified default

Is it possible to shorten/compact this block of code? I have also tried combining this block into a single line of code, ending up with 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. Any suggestions?

\$\endgroup\$

2 Answers 2

Reset to default
5
\$\begingroup\$

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
\$\endgroup\$
5
  • \$\begingroup\$ Well, that's almost line-for-line the same as my original code, and achieves the same effect. Thanks for the effort, though... \$\endgroup\$
    – Itai Ferber
    Jul 15, 2011 at 18:07
  • \$\begingroup\$ 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. \$\endgroup\$
    – mackworth
    Jul 15, 2011 at 18:39
  • \$\begingroup\$ 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. \$\endgroup\$
    – Itai Ferber
    Jul 15, 2011 at 20:28
  • \$\begingroup\$ 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... \$\endgroup\$
    – mackworth
    Jul 15, 2011 at 20:43
  • \$\begingroup\$ I see your point. \$\endgroup\$
    – Itai Ferber
    Jul 15, 2011 at 20:53
0
\$\begingroup\$

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;
\$\endgroup\$
1
  • \$\begingroup\$ 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... \$\endgroup\$
    – Itai Ferber
    Jul 13, 2011 at 15:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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