How do you decide between elegance and readability

Discussion in 'Mac Programming' started by Sydde, Apr 21, 2010.

  1. Sydde macrumors 68020

    Sydde

    Joined:
    Aug 17, 2009
    #1
    I just glanced at a fragment of code
    Code:
    				setScanner = [self objectEnumerator];
    while ( thing = [setScanner nextObject] )
      {
          if ( NSOrderedSame == [(NSString *)aValue caseInsensitiveCompare:thing] )
              break;
      }
    
    and thought, "well, I could re-write that loop in one line
    Code:
    while ( ( thing = [setScanner nextObject] )&&( NSOrderedSame != [(NSString *)aValue caseInsensitiveCompare:thing] ) ) ;
    
    AFAICT, these two forms compile to essentially the exact same object code. For me, "break" is usually a rather unattractive statement, yet at the same time, an empty loop is really no more appealing. So it got me to wondering, at what point does one choose one form over the other? I realize businesses typically have coding guidelines that leave the programmer little discretion, but from a personal standpoint, what is preferable?

    When writing method calls, do you personally prefer to nest them
    Code:
    [[[aThing aProperty] itsProperty] aMethod];
    or break them up
    Code:
    thingProperty = [aThing aProperty];
    otherObject = [thingProperty itsProperty];
    [otherObject aMethod];
    
    and how strict do company coding guidelines tend to be in such cases?

    Not being a professional, I tend to decide method nesting based on need. If I will be using an object reference again, I will grab a copy of it so that the accessor method does not get called repeatedly, but sometimes reliable nested methods are just quicker to write.

    (Edit) fixed the second loop example
     
  2. lee1210 macrumors 68040

    lee1210

    Joined:
    Jan 10, 2005
    Location:
    Dallas, TX
    #2
    Can you describe what the result of the first example should be? Having an iterator at the position in a collection of a particular string (case insensitive)? And then what? This looks to me like self is an NS(Ordered?Mutable?)Set, so it implements Fast Enumeration. That says to me that this should be:
    Code:
    for(NSString *toCompare in self) {
      if(NSOrderedSame == [toCompare caseInsensitiveCompare:(NSString *)aValue]) {
        break;
      }
    }
    
    I don't know if casting aValue is totally necessary, or if so, it might be good to have an NSString * that it's assigned to before if it's used more than a few times, to avoid clutter. The purpose of this still escapes me, and fast enumeration destroys any state of the enumeration so it may not be effective.

    If you're on 10.4 or below, i guess i'd prefer the first version because the longest line is about 80 characters, while the longest line in the second is 120. Yes, we have big monitors, but I still prefer to keep lines shorter if possible.

    As for nesting message passes... sometimes memory management prevents this, so you don't have a choice. But when you do have a choice... if you are never going to use the intermediate values again, I suppose I'd opt for the more terse version if the purpose/method names/etc. make it clear what's going on. My tune changes if the line grows too long, as noted above, though. I guess this doesn't speak well for consistency, but I think clarity should be strived for beyond religious adherence to a particular style.

    This is pretty subjective, though, so expect a wide variety of responses.

    -Lee
     
  3. Sydde thread starter macrumors 68020

    Sydde

    Joined:
    Aug 17, 2009
    #3
    The purpose of the loop was to scan through a set (NSCountedSet, to be specific, of which self is a subclass) and return "thing" as either the object that matches "aValue" or nil (meaning "aValue" was not found in the set).

    I write almost exclusively for 10.4+, though I run 10.5
     
  4. mfram macrumors 65816

    Joined:
    Jan 23, 2010
    Location:
    San Diego, CA USA
    #4
    If you are writing this code for your own use and won't share it with anyone, then it doesn't matter which method you choose. But if this code will be maintained by someone else in the future, then you should make it as straight-forward and readable as possible.

    Even if the code is just for you, it might be a good idea to make it so that it can be more easily modified in the future. In your example, what if you all of a sudden needed to check for two different strings instead of just one string? Modifying the code with the broken out while loop will be a lot easier than messing with the "fancy" version.

    As a software professional that works in a team environment, I can say it's hard enough to try and maintain someone else's code. But trying to maintain "clever" code can be a nightmare.
     
  5. lee1210 macrumors 68040

    lee1210

    Joined:
    Jan 10, 2005
    Location:
    Dallas, TX
    #5
    So if something matching aValue is found it's returned (but the Set version, not aValue itself, in case they are different cases?), if not, nil is returned?

    Code:
    setScanner = [self objectEnumerator];
    NSString *retVal = nil;
    while ( thing = [setScanner nextObject] && retVal == nil)
      {
          if ( NSOrderedSame == [(NSString *)aValue caseInsensitiveCompare:thing] ) {
              retVal = thing;
          }
      }
    
    I guess i'd prefer that to break, and you have your return value stored.

    -Lee
     
  6. Sydde thread starter macrumors 68020

    Sydde

    Joined:
    Aug 17, 2009
    #6
    But "thing" is the return value if it matches "aValue".
     
  7. lee1210 macrumors 68040

    lee1210

    Joined:
    Jan 10, 2005
    Location:
    Dallas, TX
    #7
    Is that different than what I have there?

    -Lee
     
  8. larkost macrumors 6502a

    Joined:
    Oct 13, 2007
    #8
    My usual rule-of-thumb goes about like this: if I stopped on this project right now, and did not come back to it for a year, would I understand what the line was doing immediately? If I can answer yes, then the line is probably allright, if not then taking it apart and commenting more is probably needed.

    The other thing that often breaks this up in production code is troubleshooting code (I use macro that change based on compile setting to make sure this they don't affect production code), and error checking.
     
  9. Sydde thread starter macrumors 68020

    Sydde

    Joined:
    Aug 17, 2009
    #9
    Only in terms of style.
     
  10. jpyc7 macrumors 6502

    Joined:
    Mar 8, 2009
    Location:
    Denver, CO
    #10
    In the given code with 2 versions, I think the first version (more readable) is definitely better. As larkost noted, the first version lets you insert code. A contrived example could be that you might want to capitalize the string object before returning its reference.

    Just like computers' caches, humans read more often than write so the general rule is to strive for readability instead of elegance. Another guideline is to avoid optimizing too early (potentially making code more elegant) since that effort is often wasted as one re-designs the code.
     
  11. mslide macrumors 6502a

    Joined:
    Sep 17, 2007
    #11
    I tend to think more explicit lines of code is better than trying to condense things into one or 2 lines. As for what I would do in this specific example (not that I necessarily think that any of the given choices are unreadable), I would do something like what lee suggested except I tend to not like to assign variables inside loop or if conditionals. I tend to leave the conditionals to do just that. Again, it may result in more lines of code, but it is very clear.
     
  12. Sydde thread starter macrumors 68020

    Sydde

    Joined:
    Aug 17, 2009
    #12
    Apple designed NSEnumerator to be used that way, with the assignment taking place in the loop test. Personally, as concise as it is, I do feel like it encourages bad programming style.
     

Share This Page