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.