4

Given some (relatively) complicated line of code, I would often be inclined to split it onto multiple lines to make its function/purpose clear to others.

However, if it is quite clear from context what a line does, would you still do the same thing merely to make clearer how it does it? Or would you take the attitude that future developers probably don't know/care how it works, only that it works?

For some context, the particular line that's prompted this question is Python code that takes a string like "1.title" and uses it to modify self.data[1].title:

#IRL, these 2 values are already set somehow
id = "1.title"
val = "foo"

setattr(self.data[int(id[:id.index(".")])], id[id.index(".")+1:], val)

That last line is not the nicest thing in the world to read, so I was thinking of splitting it into:

index = int(id[:id.index(".")])
attrib = id[id.index(".")+1:]
setattr(self.data[index], attrib, val)

If you wanted to go crazy, you could even do dotLocation = id.index(".") first, and use that in the setting of index and attrib.

This is probably a bit clearer to someone else reading it for the first time, but I prefer it nice and concise on one line, and it's highly unlikely that anyone is going to need to modify this one-line utility function. Is there really much point to splitting it up like that?

(Note: I'm fully aware that this is not exactly life-or-death stuff. I'm not going to lose any sleep over it, I'm just wondering what others would do in this kind of situation.)

(Last thing: someone might want to add better tags to this, it's my first Programmers question :) )

1
  • I find that "int" annoying. Is there a reason you used integer indexing? Do you really want "1.title" and "01.title" to mean the same thing? Sep 15, 2011 at 18:35

4 Answers 4

14

In this case, big fat YES.

Reasons:

  • The original line actually violates DRY - on a micro scale, but still. You are performing the same calculation twice; factoring it out removes one potential source of bugs
  • You have a bracket nesting level of 5, which is way way too much. 3 is about as deep as I'd be willing to read without line breaks and indentation to help me

I'd even take it a step further and separate concerns a bit more, factoring out the "split at first occurrence of specified character" into a function in its own right, so it would boil down to something like:

def split_at_char(s, c):
    charIndex = s.index(c)
    left = s[:charIndex]
    right = s[charIndex + 1:]
    return left, right

...and then, using it:

indexStr, attrName = split_at_char(id, '.')
index = int(indexStr)
setattr(self.data[attrName], index, val)

In fact, there's an existing python function that does exactly what split_at_char does: http://docs.python.org/library/stdtypes.html?highlight=split#str.split No need to reinvent any wheels here. So:

indexStr, attrName = id.split('.', 2)
index = int(indexStr)
setattr(self.data[index], attrName, val)

The ultimate test is: can someone who is not familiar with the codebase fluently read through the code without having to re-read anything? If not, the parts that make you stumble need more work.

4
  • I definitely see your point about the readability, and agree that it is more readable when it's split up, but I guess my question was more: this is a 2 line function that's not going to change, with a nice descriptive name and doc. It's obvious what it does, so is there any reason to break it up to show how it does it? Probably the important question I'm ignoring is: why the hell wouldn't you split it up? If I'm honest with myself, I think I'm just hesitating because I felt kind of clever when my 5-nested-bracket LOC worked first try, and now I don't want to disassemble it :P Sep 14, 2011 at 5:44
  • 4
    If you read my answer carefully, you'll see how the refactoring suddenly led to the realization that there's a standard function (split) for half the problem your code is solving. It's not obvious in the original line, but when you start refactoring, it hits you. Also, if you're surprised it works, it's too complicated.
    – tdammers
    Sep 14, 2011 at 5:49
  • Good point and good point. I think I'll change it. Sep 14, 2011 at 6:05
  • There's also partition, which is useful.
    – Daenyth
    Sep 15, 2011 at 19:49
5

Single line statements are fine, when they can be read linearly, e.g.:

[r.name for r in rows].sort.splitIntoColumns(2)

I find that yours:

setattr(self.data[int(id[:id.index(".")])], id[id.index(".")+1:], val)

is not as clear. The repetition of id.index(".") breaks the reading flow, and it's a bit too low-level for me. Why didn't you use split?

 index, name = id.split(".")
 setattr(self.data[int(index)], name, val)

That's only slightly longer, but seems quite clear to me.

1
  • Yeah, this is essentially what I ended up doing. Sep 15, 2011 at 23:21
1

It does look neat to have things concise. However, look at the following features of splitting especially when your 1 line is composed of separate logical functions.

  1. Simple to read and maintain by others

  2. Easier to catch errors on the individual logical parts rather than on the entire expression (in your example, index and attrib are logically separate parts) - You can test the result of each part separately

  3. In a system with 10s of thousands of lines of code, what difference would it make if we add a few more?

A somewhat similar question was asked on: Formatting 'Complex' Math

Modification: I can't say that decomposition is always the right way. If the expression is obvious and you are not in identifying in which part it fails, make it all in one line.

1

Sometimes I base decisions like this on the ease of debugging the statements as separate lines. Every so often, while debugging, a single overloaded line of code becomes an impediment because it obscures the intermediate results.

Your Answer

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

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