[Cosmo-dev] Commit 5847

[Matthew Eernisse]

Travis,

Thanks for fixing this annoying bug I introduced in one of my commits. 
Sorry for the frustration it caused you.

Thanks also for you thoughts about coding style and refactoring. While I 
agree in spirit with most of what you wrote, I have some additional 
input I'll throw out inline below.

Travis Vachon wrote:
> 1) Please try to avoid refactoring longer descriptive variable names 
> into terse, difficult to read variable names. Code is more often read 
> than written and terse variable names make reading code much harder. In 
> addition, since there is a significant dearth of good re-factoring tools 
> for Javascript the method I (and others) use most often is good old 
> emacs incremental search. Variables like "ev" and "reg" make this 
> process much more tedious and sometimes downright impossible. This 
> particular commit included one change from ev -> item which was an 
> improvement, but also a change from eventRegistry -> reg, a significant 
> decrease in readability and regex-ability.

The argument from readability is a good one -- although the variable you 
mention here looks like a local, temp variable. I personally find 
lengthy names for temp variables in a function distracting, and it 
actually makes the code harder for me to read. At some level, I think 
it's a matter of taste -- one man's "longer and descriptive" might be 
another's "needlessly verbose and distracting."

Another good example of this would be a commit to the layout code I 
remember seeing recently -- with a commit comment decrying terse 
variable names -- which involved laboriously renaming a scratch 
variable, used as a container node for each layout item, to have the 
name of whatever item it was being used with.

I guess to someone who really prefers longer, descriptive variable names 
it makes good sense, but to me it seemed only to create more typing, and 
actually made it harder to understand. It was easier for me to read 
before the refactoring, to see that "d" represented in each case "the 
DOM node container for the current layout item."

The searchability/regex argument also makes a fair bit of sense, because 
regexes can be pretty irritating and tweaky.

In this case though, it doesn't really seem like a trip through the 
Swamps of Sadness. Unless I'm missing something totally obvious, I think 
it's actually pretty easy to search for even a terse variable name like 
"reg," at least in Vim or grep:

Vim:

/reg\W

Grep:

grep -r "reg\W"

(I don't know that much about Emacs, but I'm assuming since it can cook 
your breakfast and everything else, it has even more uber regex fu.)

Having said all that on the other side of the argument, I still agree 
with the spirit of what you said, particularly since we want to recruit 
outside contributors. I would never advocate terse variable names for 
things used throughout the system -- that's why we have all that nice 
namespacing everywhere.

I *do* think you can go overboard on the "descriptive variable name" 
thing (as with the commit to the layout code I mentioned above), and 
clearly, opinions differ as to where the line is. I'm also aware that 
I'm particularly bad about being in "blow and go" mode, and not thinking 
ahead as much about the next guy (oftentimes my own self) who has to 
come and read the code afterward.

I'll try to triangulate a bit better.

> 2) As much as possible, please avoid combining re-factoring and 
> functionality changes in one commit. Doing this obfuscates the actual 
> changes, and this particular bug was a textbook example of why this is a 
> bad idea.

I think this policy is good in theory, but might be problematic for 
reasons I'll cite below.

> 3) In bug fix releases I think it would be a good policy to avoid any 
> and all re-factoring unless it is necessary for a bug fix. This will 
> help us avoid silly mistakes like the one that caused this bug.

Again, a good idea in theory, but given all the time pressures we have 
to build new features and fix the next bug, I have a feeling that would 
mean that a significant amount of refactoring would just not ever get done.

I think it's way more likely that local refactoring like that will get 
done when you're sitting there looking at the code, than that 1. you'll 
remember to go refactor that one piece of code you saw a week ago, or 2. 
that you'll add to your bug pile with a bug for it, or 3. that we'll 
schedule time specifically to refactor it.

I'm not saying it's an untenable policy, just that it would add 
significant friction against ongoing refactoring. My experience with 
similar policies at my last company was not good (one of the two apps 
also required a bunch of validation processes, so it was even worse), 
but we have a better team here and better tools, so it might not be as bad.

It might also be worth mentioning the idea that improved test coverage 
is supposed to make us more comfortable doing continuous refactoring to 
keep the codebase healthy. I know it's not entirely realistic to think 
it will catch every regression -- and you still have to fix the 
regressions after you catch them.

Seems like this issue could use some discussion.

Again, thanks for fixing my screwup, and thanks for thinking about how 
we can keep improving stuff.


Matthew