5
\$\begingroup\$

I have an OpenClose class which just represents the hours of operation of a business by the opening and closing time. It takes the opening and closing times as arguments to its constructor, and each is a datetime object.

The data is coming from an external source in a string, formatted like "HH:MM (AM|PM)-HH:MM (AM|PM)"

I have the following function to turn this into an OpenClose object:

def __get_times(self, hours):
    return OpenClose(*map(lambda x: datetime.strptime(x, "%I:%M %p"), hours.split("-")))

What do you think of this? Is it too much for one line? Too confusing? Should I split it up into multiple lines?

It sort of bugged me when doing it with multiple lines, but I can certainly see how this would be more readable, despite my hatred of explicitly doing the datetime calculation twice:

format = "%I:%M %p"
open_time, close_time = hours.split("-")
open_time = datetime.strptime(format)
close_time = datetime.strptime(format)
return OpenClose(open_time, close_time)

An alternative would be to use a combination of these approaches:

format = "%I:%M %p"
hours = hours.split("-")
open_time, close_time = map(lambda x: datetime.strptime(x, format), hours)
return OpenClose(open_time, close_time)

Which of these is best?

\$\endgroup\$
10
  • \$\begingroup\$ Please define "best" so that we know what's important to you. \$\endgroup\$
    – S.Lott
    Jul 11, 2011 at 16:06
  • \$\begingroup\$ @S.Lott I'm asking what should be important to me. To what degree does readability trump brevity? At what point does expressiveness taper off into unmaintainability? \$\endgroup\$ Jul 12, 2011 at 5:41
  • \$\begingroup\$ "I'm asking what should be important to me." That's silly. What's important to you is to write code that gets me a new Bentley Continental. Brevity is awful -- no one wins at code golf. However, you have other people on a team, you have existing coding styles, you have hundreds of considerations we can never know anything about. Performance. Existing Code Base Compatibility. Reusability. Design Patterns in common use in your organization. It is your obligation to define best. We can only provide random advice. "No One Wins at Code Golf". \$\endgroup\$
    – S.Lott
    Jul 12, 2011 at 10:23
  • \$\begingroup\$ @S.Lott sure brevity can be awful, but that's the exception, not the rule -- otherwise, why have we spent decades attempting to do more things with less code? I see your point about there maybe being existing standards that I didn't disclose, but isn't that always the case? This is a code review site and I was asking whether I was trying to put too much on one line. In my last comment I was pointing out that I was aware of a line between expressiveness and code golf, and with this question I was merely asking if I had crossed it. \$\endgroup\$ Jul 12, 2011 at 10:33
  • \$\begingroup\$ "why have we spent decades attempting to do more things with less code?". I don't know. Why? It is universally awful. It is a maintenance burden, and maintenance is the bulk of the cost of ownership of software. "No One Wins at Code Golf" Only you can define "best". You actually need to actually think about what is best in your environment, culture and organization and actually share that every time you use the word "best" in an open forum like this. Sadly, we don't know you. For code review purposes, however, "No One Wins at Code Golf". \$\endgroup\$
    – S.Lott
    Jul 12, 2011 at 10:35

5 Answers 5

6
\$\begingroup\$

Yes, too much for one line, yes, too confusing. Making a map an lambda for two values like that is silly, unless you are trying to win an obfuscation contest.

So, the middle version is best.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I'd have to agree too. In most cases when I see someone chime in with a "one liner" answer to some problem, I think of all the times I've had to deal with someone's "cute" solution years later and dreamed about punching them for it... \$\endgroup\$ Jun 30, 2011 at 17:28
5
\$\begingroup\$

"No One Wins at Code Golf". One liners are simply a mistake waiting to happen. They don't create any value because they can be obscure. They're remarkably hard to maintain.

Sometimes, a one-liner (i.e., a simple list comprehension) is very clear and makes sense.

In this case,

open_time = datetime.strptime(open_time,format)
close_time = datetime.strptime(close_time,format)

Are not redundant enough to care about.

This has a fundamental error.

OpenClose(*map(lambda x: datetime.strptime(x, "%I:%M %p"), hours.split("-")))

The above has the error of hard-coding the format string into the statement. The format string is the most likely thing to change and should be in a configuration object of some kind so that it can be found and changed easily. If not in a configuration object, then a class-level attribute. Or a module-level global.

The next most common mutation is to go from what you have (which doesn't handle the ValueError) to this.

try: 
    open_time= datetime.strptime(open_time,format)
except ValueError, e:
    log.error( "Open time in %s is invalid: %s", hours, open_time )
    return
try:
    close_time = datetime.strptime(close_time,format)
except ValueError, e:
    log.error( "Close time in %s is invalid: %s, hours, close_time
    return

This is no better.

OpenClose( *map(lambda x: datetime.strptime(x, format), hours.split("-")) )

Nor is this.

OpenClose( *[datetime.strptime(x, format) for x in hours.split("-")] )

Is shorter, but no better.

\$\endgroup\$
1
  • \$\begingroup\$ Happily, I do handle the exception up the call chain a few levels. You're right about the hard-coded string, I'll change that. It's true that since there'll only ever be two items using map is silly, although I confess I didn't think of your list comprehension example... tempting but I'll have to save it for a more fitting opportunity :) \$\endgroup\$ Jul 12, 2011 at 11:07
2
\$\begingroup\$

One guideline of clean code: don't return expressions, only return variables (or constants or objects). Applying just that guideline dictates splitting into 2 lines.

The question to ask for any implementation is:

  • Does it improve performance, if so, how? See http://wiki.python.org/moin/PythonSpeed/PerformanceTips

  • The current code does not yield a performance improvement from putting everything on one line..

  • So it can be optimized for better readability.. (results in fewer bugs)

  • And it can be optimized for better maintainability.. (results in fewer bugs)

    • return() with a simple argument is easy to read for program flow (any odd side effects are easy to see) = better readability

    • Adding code prior to the return() (for example exception handling or new return values) can be done by creating logical expressions or adding new side effects = better maintainability

      • Typically the person maintaining the code (adding a new return value) may not be as familiar with the algorithm or original implementation, so there is higher likelihood of bugs being introduced..

See also :

Python Patterns - An Optimization Anecdote http://www.python.org/doc/essays/list2str.html

\$\endgroup\$
1
  • \$\begingroup\$ Why? Why shouldn't you return expressions? I prefer returning an expression to assigning into a variable and returning that on the next line. \$\endgroup\$ Jul 23, 2011 at 23:48
1
\$\begingroup\$

I'll go against the trend and say that it's ok, but cramped. There's a chunk of code that you don't want to repeat, and I agree with the sentiment. But since that chunk of code is a semantic unit, it should get more visibility. Give it a name, for documentation.

def __get_times(hours):
    def parse_time(s): return datetime.strptime(s, "%I:%M %p")
    return OpenClose(*map(parse_time, hours.split("-")))

There's still a bit of a smell in that OpenClose expects exactly two arguments, but split could in principle produce any number. Furthermore, strptime itself could fail if the string was improperly formatted. When you get around to implementing clean error reporting, you'll need to report an error before calling OpenClose; it'll be easier if you get the parsing out of the way first.

def __get_times(hours):
    def parse_time(s): return datetime.strptime(s, "%I:%M %p")
    opening, closing = map(parse_time, hours.split("-"))
    return OpenClose(opening, closing)

To distinguish between a wrong number of components and badly formatted times, it may be nicer to extract the times before checking the number:

def __get_times(hours):
    def parse_time(s): return datetime.strptime(s, "%I:%M %p")
    times = map(parse_time, hours.split("-"))
    if len(times) <> 2: raise …
    return OpenClose(*times)
\$\endgroup\$
0
\$\begingroup\$

Explicitly calling map with a lambda isn't usually necessary; using a list comprehension is usually more natural/Pythonic. Therefore you'd have this (which is very similar to your third iteration):

def __get_times(self, hours):
    open_time, close_time = [datetime.strptime(s, "%I:%M %p") for s in hours.split("-")]
    return OpenClose(open_time, close_time)

I didn't use the asterisk because it obfuscates for the reader what the expected call to the OpenClose constructor is.


In two of your examples, you've reused a variable to hold different value types. While perfectly legal, I think this reduces readability:

open_time, close_time = hours.split("-")
open_time = datetime.strptime(format)
close_time = datetime.strptime(format)

and then

hours = hours.split("-")

I'm interested in what variables are declared and what they are meant to store. So if asked, what does hours mean, instead of a simple answer you have to say: first it holds a string, then later it holds a two-element list of strings. Similarly, first open_time holds a string, then later it holds a datetime instance.


On the subject of brevity, I never think of brevity as a first-class virtue, but it definitely plays into readability. I'd rather read something short than something long, but making something too short can of course detract from readability.


But here's where things get dicey. I think we're maybe focusing on the wrong question. One line or five, I think there's something wrong with your program.

Some class has a private method called __get_times which takes one string argument, doesn't rely on current state, and returns an OpenClose instance. At a minimum, we shouldn't call it __get_times because that name implies that we'll get a return of multiple time values, but that's not what we get at all.

But why does the OpenClose class exist at all? It's just a pair of values; I can't imagine any legitimate behaviors/methods that should be owned by it. If brevity/readability is your goal, then it would certainly be enhanced by getting rid of near-useless classes. Yes, sometimes bundling values together into a structure can reduce mental overhead, but in this case it doesn't seem worth it. Whoever owns an OpenClose instance should probably just own an open and a close value, neither of which should be a new custom type.

And for that matter, whoever owns __get_times with a single string argument, why do they need a 1 or 2 line method which should probably only be called once?

Then there's the part (and I can't stress this enough) where it doesn't rely on any internal state. At best, this whole thing should be a free-standing function which takes one string argument and returns a pair of time instances (NOT datetimes; you're talking about time of day). It's also more easy to test, tweak, and verify this way.

def parse_open_close_times(range_string, format="%I:%M %p"):
    """Returns a pair of time-of-day instances based on RANGE_STRING."""
    return [datetime.strptime(s.strip(), format).time() for s in range_string.split("-")]

In short, brevity/readability is better addressed by not proliferating questionable classes than by playing code golf with methods with odd signatures.

\$\endgroup\$

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.