On Wed, 08 Feb 2012 17:17:24 +0100, Michael Haggerty
<mhagger@xxxxxxxxxxxx> wrote:
On 02/08/2012 11:43 AM, Frans Klaver wrote:
On Wed, Feb 8, 2012 at 11:00 AM, Michael Haggerty
<mhagger@xxxxxxxxxxxx> wrote:
On 02/08/2012 08:33 AM, Junio C Hamano wrote:
+ line = lines[i].rstrip('\n')
+ ix = line.find(' ')
+ if 0 <= ix:
+ key, value = line[0:ix], line[ix+1:]
The above five lines can be written
for line in lines:
if ' ' in line:
key, value = line.rstrip('\n').split(' ', 1)
or (if the lack of a space should be treated more like an exception)
for line in lines:
try:
key, value = line.rstrip('\n').split(' ', 1)
except ValueError:
continue
This is generally considered more pythonic: "It's easier to ask for
forgiveness than to get permission".
Given that Junio explicitly wanted to allow lines with no spaces, I
assume that lack of a space is not an error but rather a conceivable
future extension. If my assumption is correct, then it is misleading
(and inefficient) to handle it via an exception.
I find the documenting more convincing than the efficiency, but from the
phrasing I think you do too.
for line in lines:
if ' ' in line:
key, value = line.split(' ', 1)
if key == 'tree':
cd = cd.set_tree(repository.get_tree(value))
elif key == 'parent':
cd = cd.add_parent(repository.get_commit(value))
elif key == 'author':
cd = cd.set_author(Person.parse(value))
elif key == 'committer':
cd = cd.set_committer(Person.parse(value))
return cd
One could also take the recommended python approach for
switch/case-like if/elif/else statements:
updater = { 'tree': lambda cd, value:
cd.set_tree(repository.get_tree(value),
'parent': lambda cd, value:
cd.add_parent(repository.get_commit(value)),
'author': lambda cd, value:
cd.set_author(Person.parse(value)),
'committer': lambda cd, value:
cd.set_committer(Person.parse(value))
}
for line in lines:
try:
key, value = line.split(' ', 1)
cd = updater[key](cd, value)
except ValueError:
continue
except KeyError:
continue
It documents about the same, but adds checking on double 'case'
statements. The resulting for loop is rather cleaner and the exception
approach becomes even more logical. I rather like the result, but I
guess it's mostly a matter of taste.
I know this approach and use it frequently, but when one has to resort
to lambdas and there are only four cases, it becomes IMHO less readable
than the if..else version.
Well, as I said, its largely a matter of taste; four items is a corner
case to me when thinking maintainability vs. readability. On the other
hand, this doesn't seem like an oft-changing piece of code, so a longer
list of if..elif..else shouldn't be a problem either.
Frans
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html