Skip to main content
Correct last line of code; index and attrName were swapped
Source Link
kevin cline
  • 33.8k
  • 3
  • 73
  • 143

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[attrName]data[index], indexattrName, 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.

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[attrName], index, 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.

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.

Source Link
tdammers
  • 53k
  • 15
  • 112
  • 155

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[attrName], index, 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.