Because Radix asked, and with help from David McClosky, I present my complaint about comb code.
Comb code is program source code, written by a human, that repeats the same formation many times in a row, with only small differences. The name obviously comes from the fact that at a distance, the code looks like a sideways comb. The repeated code doesn't have to be a single line.
Here are some reasons I imagine comb code might come about:
- the language makes (or at least urges) the programmers do it, since its type
system is too strong to do the appropriate loop
- some ordinary code got replicated a few times and became
"accidental" comb code- the last person to touch it didn't want to rewrite it
Here's why it sucks:
- It took longer to type.
- It's harder to edit.
- It's harder to read. The reader in most cases is spending most
of his effort finding the differences between the repetitions (exactly what the writer should have emphasized).
- As a side effect of it being longer, it leads to less program
fitting on the screen at once.
There are times when I might argue for code that is comb code to some degree. Mostly I want to see clear code. If there's code that's currently repetitive, but the docs are different for each repeat, I'm in favor of preserving the docs. If the number of repetitions is small compared to how much it would obscure the code to rewrite it, perhaps it should be left alone. It's fine to leave a few reps and then rewrite when more reps get added later. I don't see any need to "plan for" compact code. The rewrite should never be so tricky that it's important to put it in the short code- it shall be easy to jump from repetitive code to a loop.
Why can't I find any other web pages like this one? Avoiding comb code should be a conclusion of the "programmers can write/maintain x lines of code, regardless of language" arguments, and it should be a conclusion of the "write as little code as you can" crowd. I expect those groups will also not recommend going to excess to make the shortest code possible, but I imagine they'd agree with the above arguments.
<hr>
Here's a funny case. The code gets longer by some chars when I made it a loop, and it got even less idiomatic:
items.sort(lambda a,b: cmp(*[str(x[1]) for x in a,b]))
The expanded code would have been like:
items.sort(lambda a,b: cmp(str(a[1]),str(b[1]))
But that repeats the str and the [1] such that if you wanted to do a case-insensitive sort, for example, you'd have to add two .lower() calls.
<hr>
From the python standard library:
def __lt__(self, other): return self.data < self.__cast(other) def __le__(self, other): return self.data <= self.__cast(other) def __eq__(self, other): return self.data == self.__cast(other) def __ne__(self, other): return self.data != self.__cast(other) def __gt__(self, other): return self.data > self.__cast(other) def __ge__(self, other): return self.data >= self.__cast(other) # ..and so on
Possibly you used to have to write this in much older python, although you don't anymore. I think it's actually from UserList?.py, which is obselete for other reasons.
<hr> Now, some examples I dug up for a talk once. My notes for the talk included "don't coast through any sections of code".
From the kino video editor, written in C++. I don't know if C++ provides a way to avoid this besides cpp macros. But even macros would plainly be better than making the reader page through this stuff, looking for the differences:
// (kino_common.cc)
void KinoCommon::videoRewind() {
if (! is_component_state_changing ) {
getCurrentPage()->videoRewind();
commitComponentState();
}
}
void KinoCommon::videoBack() {
if (! is_component_state_changing ) {
getCurrentPage()->videoBack();
commitComponentState();
}
}
void KinoCommon::videoPlay() {
if (! is_component_state_changing ) {
getCurrentPage()->videoPlay();
commitComponentState();
}
}
//(etc)
This is from the extensible user folder product for zope. It's in python:
def addUsersToGroup(self, usernames, groupname):
"""Adds users to a group"""
if self.currentGroupSource:
return self.currentGroupSource.addUsersToGroup(usernames, groupname)
def delUsersFromGroup(self, usernames, groupname):
"""Deletes users from a group"""
if self.currentGroupSource:
return self.currentGroupSource.delUsersFromGroup(usernames, groupname)
def setGroupsOfUser(self, groupnames, username):
"""Sets the groups of a user"""
if self.currentGroupSource:
return self.currentGroupSource.setGroupsOfUser(groupnames, username)
# (etc)
# (later on)
def doMembershipSourceForm(self, memberId):
""" doot de doo """
return exUserFolder.membershipSources[memberId].manage_addForm
def doGroupSourceForm(self,groupId):
""" la de da """
return exUserFolder.groupSources[groupId].manage_addForm
# (etc)
<hr> The pyWiew image viewer program, written in python, has all sorts of interesting comb situations:
self._tb.AddSimpleTool(1001, pyWiewGlobals.getNewBitmap(), "New", "Create a new Image") EVT_TOOL(self, 1001, self._handler.OnToolClick) EVT_TOOL_RCLICKED(self, 1001, self._handler.OnToolRClick) self._tb.AddSimpleTool(1002, pyWiewGlobals.getSaveBitmap(), "Save", "Save current image") EVT_TOOL(self, 1002, self._handler.OnToolClick) EVT_TOOL_RCLICKED(self, 1002, self._handler.OnToolRClick) self._tb.AddSimpleTool(1003, pyWiewGlobals.getCloseBitmap(), "Close", "Close current image") EVT_TOOL(self, 1003, self._handler.OnToolClick) EVT_TOOL_RCLICKED(self, 1003, self._handler.OnToolRClick) #...
That one's simple. Loop over the id number, the function, the name, and the long name. The obvious rewrite is like this:
for id,(func,name1,name2) in zip(range(1001,9999),
(('getNewBitmap',"New", "Create a new Image"),
('getSaveBitmap',"Save", "Save current image"),
('getCloseBitmap',"Close", "Close current image"))):
self._tb.AddSimpleTool(id, getattr(pyWiewGlobals,func)(), name1, name2)
EVT_TOOL(self, id, self._handler.OnToolClick)
EVT_TOOL_RCLICKED(self, id, self._handler.OnToolRClick)
I know that zip will truncate to the shortest list, so was able to use range() to make ids without even counting the upper bound. It would be fine to include the id in the menu item list too. I still can't understand what all the code does, but that's because I'm not familiar with wx, right?
Here's more from pyWiew:
imageMenu = wxMenu() self.AddMenuItem(imageMenu, 'Rotate &Right (right arrow)' , 'Rotate right', self.OnRotateRt) self.AddMenuItem(imageMenu, 'Rotate &Left (left arrow)', 'Rotate left', self.OnRotateLt) self.AddMenuItem(imageMenu, 'Flip &Horizontal (down arrow) ', 'Horizontal Flip', self.OnHorFlip) self.AddMenuItem(imageMenu, 'Flip &Vertical (up arrow) ', 'Vertical Flip', self.OnVerFlip) imageMenu.AppendSeparator()
This is comb code, but the repetition is small compared to the differences between lines. I'm not sure it would help to pack it into a list. In particular, if the programmer had used named arguments (he or she didn't), the spelled-out calls might be easier to follow than the compacted version. Compare these two versions:
# looks almost like the original:
for name,hint,command in (('Rotate &Right (right arrow)' , 'Rotate right', self.OnRotateRt),
('Rotate &Left (left arrow)', 'Rotate left', self.OnRotateLt),
('Flip &Horizontal (down arrow) ', 'Horizontal Flip', self.OnHorFlip),
('Flip &Vertical (up arrow) ', 'Vertical Flip', self.OnVerFlip)):
self.AddMenuItem(imageMenu, name, hint, command)
# the original, plus named arguments
self.AddMenuItem(imageMenu, name='Rotate &Right (right arrow)' , hint='Rotate right', command=self.OnRotateRt)
self.AddMenuItem(imageMenu, name='Rotate &Left (left arrow)', hint='Rotate left', command=self.OnRotateLt)
self.AddMenuItem(imageMenu, name='Flip &Horizontal (down arrow) ', hint='Horizontal Flip', command=self.OnHorFlip)
self.AddMenuItem(imageMenu, name='Flip &Vertical (up arrow) ', hint='Vertical Flip', command=self.OnVerFlip)
I daresay the second one is clearer than the first. Since they happen to appear first, it's easy to see what parts of the repetitive lines are constant, so that's not a readability problem. If you had to add another menu option, it would be very straightforward to add it to either version- the second version might be even easier.
Here's some comb code where the variation between reps is within some text:
submenu=wxMenu() submenu.AppendRadioItem(701, 'Zoom 25%', 'Zoom to 25 percent of image') submenu.AppendRadioItem(702, 'Zoom 50%', 'Zoom to 50 percent of image') submenu.AppendRadioItem(703, 'Zoom 75%', 'Zoom to 75 percent of image') submenu.AppendRadioItem(704, 'Zoom 100%', 'Zoom to 100 percent of image') submenu.AppendRadioItem(705, 'Zoom 125%', 'Zoom to 125 percent of image') submenu.AppendRadioItem(706, 'Zoom 150%', 'Zoom to 150 percent of image') submenu.AppendRadioItem(707, 'Zoom 175%', 'Zoom to 175 percent of image') submenu.AppendRadioItem(708, 'Zoom 200%', 'Zoom to 200 percent of image') #...
This is not a problem:
submenu=wxMenu() for id,percent in zip(range(701,999),"25 50 75 100 125 150 175 200".split()): submenu.AppendRadioItem(id, 'Zoom %s%%'%percent, 'Zoom to %s percent of image'%percent)
I could have even calculated the zooms with [x*25 for x in range(1,9)]? or something, but that hurts readability a little, especially for my purposes here. I don't support this UI (or its ridiculous help tips) anyway.
Here are some repetitive functions, like I've shown above:
def GetPanelBgColor(self):
return self._panelbgcolor
def SetPanelBgColor(self, val):
self._panelbgcolor = val
def ShowTime(self):
return self._displaytime
def SetShowTime(self, val):
self._displaytime = val
These should be python properties, certainly. But if your API really needed these silly accessors, you could use something like this:
# (untested)
from __future__ import nested_scopes
class Spam:
# interesting class body
for accessor,attr in (('PanelBgColor','_panelbgcolor'),('ShowTime','_displaytime'),):
def get(self):
return getattr(self,attr)
setattr(Spam,'Get%s'%accessor,get)
def set(self,val):
setattr(self,attr,val)
setattr(Spam,'Set%s'%accessor,set)
Yes, I could have used lambda. It's scarier to some, and I wanted to use lots of names. Also, this form illustrates that the functions didn't have to be one-liners for this approach to work.
<hr> Some (edited) talk on #python:
drewp> . . . i'm proposing that java may be easier than python because you can't do a lot of the cool things that allow us to code so quickly (and well) in python
drewp> dreadlock: stuff along the lines of "for a in 'attr1','attr2','attr3': setattr(o,a,"foo")". i'm sure java's comb code is easier for non-skilled people to handle (while it just slows the rest of us down)
dreadlock> drewp: yes. but i code fast in java also (not valid for an unskilled code, so)
dash> dreadlock: but people who are good at python code faster than people who are good at java
error27_> drewp: anyone who says java is either than python is high.
dash> as anybody who's done both for a long time will tell you :)
drewp> error27_: i'm mostly getting at the comb code, i think. it's harder to read, harder to edit, takes longer to write, but i'm proposing that it's simpler in concept, so it may be why unskilled prog'rs might do better in java
itamar> not really
dreadlock> dash: well, hard to say, i am new to python - hence i cant compare it to my java. i never saw any python programmer solving a prob i did in java. i cant say.
itamar> java is a lot less explicit
itamar> and has many more inconsistencies
drewp> itamar: those are both bad things of course, but i'm proposing that inconsistencies don't make java "harder to handle" for unskilled prog'rs. (i'm trying to find explanations for dreadlock's assertion about his experience with poor prog'rs; since i dont think the data-hiding is the right explanation)
<hr>
More talk on #python:
Deep7> class Application(Frame):
Deep7> def launchgame(self):
Deep7> #get_romselection(rom)
Deep7> print "Game Started"
Deep7> os.system("tuxnes %s, %s" % (options , rom))
Deep7> def get_romselection(self,rom):
Deep7> rom = self.romlist.curselection()
Deep7> def buildwidgets(self):
Deep7> self.Quit = Button(self)
Deep7> self.Quit["text"] = "Quit"
Deep7> self.Quit["fg"] = "red"
Deep7> self.Quit["command"] = self.quit
Deep7> self.Quit.pack({"side": "left"})
Deep7> self.romlist = Listbox(master=None);
Deep7> map(lambda x: romlist.insert(END, x), os.listdir('/home/jaysonh/roms/'))
Deep7> romlist.pack()
Deep7> self.Launch = Button(self)
Deep7> self.Launch["text"] = "Launch"
Deep7> self.Launch["fg"] = "green"
drewp_> dude-- self.Quit=Button(self,text="Quit",fg="red",etc)
Deep7> self.Launch["command"] = self.launchgame
Deep7> self.Launch.pack({"side": "left"})
Deep7> def __init__(self,master=None):
drewp_> say no to comb code
Deep7> Frame.__init__(self,master)
Deep7> self.pack()
Deep7> self.buildwidgets()
Deep7> tktuxnes = Application()
Deep7> tktuxnes.mainloop()
Deep7> comb code?
drewp_> comb code is (more or less) where the same thing appears in line after line, like your 'self.Quit[...]=...'
Deep7> ok
drewp_> tkinter offers a much better way, and even if it didnt, python'd let you write it without the comb
<hr> More talk on #python:
nFiniteFx@ircnet> def ib(s):
nFiniteFx@ircnet> a = s.split('.')[0]
nFiniteFx@ircnet> b = s.split('.')[1]
nFiniteFx@ircnet> c = s.split('.')[2]
nFiniteFx@ircnet> d = s.split('.')[3]
nFiniteFx@ircnet> so a - d are integers right?
drewp|home pukes
deltab> not yet
deltab> when you split a string, you get strings
nFiniteFx@ircnet> i have to use a = int(a) then
deltab> yes
nFiniteFx@ircnet> ok done
ameoba> nFiniteFX : try this:
ameoba> a,b,c,d = s.split('.')
drewp|home> nfinite: no comb code PLEASE. that's for C and java programmers
nFiniteFx@ircnet> what's no comb code?
ameoba> drewp : 'comb code' ?
dash> drewp: comb?
deltab> code with near-identical statements in parallel
dash> ah
drewp|home> code that's almost the same on every lline except for a little change, such that viewed at a distance, it looks like a comb
dash> heh :)
drewp|home> listen to deltab
dash> yes, that is a bad idea.
nFiniteFx@ircnet> oh
drewp|home> we just plain don't need it in python, and it's hard on the programmer and on readers of the code
ameoba> a,b,c,d = s.split('.')
<hr> More talk on #python:
exarkun> tee-hee :)
* drewp|home is offended by config.ConfigWindow.__init__
exarkun> Goo, how so?
drewp|home> where's your "for attrname,value in ('wrapVar',world.wrap),('scrollVar',str(world.scrollback)),...:"
* exarkun smirks.
drewp|home> setattr(attrname,Tkinter.Stringvar()); getattr(self,attrname).set(value)
drewp|home> ^ something like that
* drewp|home _hates_ the comb code