Friday, May 6, 2011

Organizing Stepwise Considered Harmful

There are different ways to organize repeating code.

One is step-wise, in which near-identical steps are stacked together:


button1 = QPushButton('Button 1')
button2 = QPushButton('Button 2')
button1.clicked.connect(lambda: self.on_button(1))
button2.clicked.connect(lambda: self.on_button(2))



layout.addWidget(button1)
layout.addWidget(button2)


The other is subject-wise where the code for button1 is separated from the code for button2:

button1 = QPushButton('Button 1')
button1.clicked.connect(lambda: self.on_button(1))
layout.addWidget(button1)

button2 = QPushButton('Button 2')
button2.clicked.connect(lambda: self.on_button(2))
layout.addWidget(button2)
The step-wise version makes the code repetition more obvious, but the novice code-cleaner is likely to extract constants to variables and leave it alone.  In the second case (subject-wise) the code-cleaner is more likely to realize that there is a multiple-step process that can be extracted to a function, leaving something more like:

CreateButtonWithAction(layout, 'Button 1', lambda: self.on_button(1))
CreateButtonWithAction(layout, 'Button 2', lambda: self.on_button(2))

At this point it becomes obvious that the only difference in the two lines is two constant values, which might suggest more ways to simplify the code.  The astute observer will also recognize that the first parameter in the call is always the layout class, which suggests a wrapper class or "move method" which will in turn result in a more well-developed abstraction. Well-developed abstractions make writing client code easier and quicker.

layout.CreateButtonWithAction('Button 1', lambda: self.on_button(1))
layout.CreateButtonWithAction('Button 2', lambda: self.on_button(2))
I'm not saying that this is the ultimate, optimal form for the code to take.  I'm only saying that once it is organized subject-wise, the refactoring starts to flow very naturally.




1 comment:

  1. Refactoring after 2nd repeat _is_ harmful. Wait until repeat #3...

    ReplyDelete