The topic of code style guidelines has come up again just recently in the newsgroups. This isn’t the first time, and probably won’t be the last. Everyone has an opinion – some strongly held – about where control statement braces should go (K&R baby!), how you should wrap multi-line conditions (logical operators at EOL!), and how many spaces (or tabs! ew!) should be used for indentation (2!).
Usually the justification that fuels long debates about determining the One True Mozilla Code Style is that well-defined, strictly enforced, project-wide guidelines will help new contributors and make the code easier to maintain. Some people seem to also be suggesting that the guidelines be applied equally to all of Mozilla’s C++ and JavaScript code, since the two languages share much of their syntax. I don’t buy it.
I think that the benefits to a common style are often oversold. Consistency tends to be highly valued by us software developer types, sometimes to an unhealthy degree. I may find a particular bracing or indentation style unappealing when I approach new code, but I think it very rarely significantly impacts my ability to understand or contribute to it, assuming it’s at least internally consistent. I’m sympathetic to the idea that a common style can be beneficial at the file- (or maybe even module-) level, even if only for aesthetic reasons, but I think that trying to create and enforce more ambitious policies usually ends up being more trouble than it’s worth, especially for a project the size of Mozilla. The benefits to cross-module style guidelines just seem mostly theoretical to me, even more so for policies that attempt to span Mozilla’s loosely defined front-end/platform and JS/C++ boundaries.
Finding a single policy that covers all aspects of code style while satisfying even just a plurality of developers in a project this large can be costly just in terms of time spent debating. Add to that the time spent attempting to normalize the code base with large style-only cleanup patches, which would be necessary given our current state of affairs, and I think the scales are already tipped in favor of more tightly scoped code-style policies.
In practice, the project has settled into an fairly stable equilibrium already – we have a style guide, but the economics of making all of our code follow it, which would require both carefully auditing new code and fixing up old code, just haven’t worked out. I think we should stop kidding ourselves into thinking that they someday will, or that they should.
I agree with this completely. I often smile when I hear people talk about their own personal style, since I think it points to the fact that they haven’t worked on very much code they don’t own. I don’t have a style any more: my style is the file I’m in right now, since I almost exclusively work on code I don’t own, or over which I share joint ownership. I also think that people who can’t handle the variation of coding styles in something the size of Mozilla are going to have more trouble with the even wider variety of people, techniques, technologies, etc. Big code is diverse; big communities, too. I’m also aware that I’m not a pedant, and that programming attracts many people who are slavishly bound to rules and guidelines, and *need* others to be too. If this is who you are, I can understand how your approach above will be seen to be unnecessary chaos. I would argue that this sort of “chaos” is nothing more than learning to navigate the differences between people in a large community, and you’re better off learning to do this early and on something simple like tabs, spaces, and line breaks.
I completely agree. Having a consistent style across a huge codebase isn’t realistic. The benefits of various coding styles are mostly theoretical.
Couldn’t have said it better myself. (And by that, I mean +1 on the title pun!)
I think the common coding style is an important piece to help new developers, to help reviewers and to improve readability ( thus i don’t get how “} else {” can be more readable than “else {” :p ).
But that said, tules are not carved in stone, they are an awesome base to write maintainable code. We should just have some basic rule like “use the right type of comment (don’t use multiline for single lines)”, “use javadocs”, “be consistent across the module you are changing”, “ask the module owner if in doubt”, “try to make code readable but not sparse”, “write meaningful comments, that don’t repeat name of the function just below them”.
The issue I have with not having a common coding style, is with code reviews. What should i tell to a new dev approaching us and using a style that is suggested by the guide but not consistent with current module the patch applies on? Is this fault of the new dev, of the module, or mine?
Probably there is no fault, still I dunno what to say, thus i tend to suggest one side of the “rules”.
Another issue i have when creating patches is “what should i do now that i’m changing this browser code that is here from 5 years, should i follow old browser.js style or adhere to what Gavin prefers for new code?”.
I think nobody would ever want to fix the codebase to be consistent, nor write gigantic code style patches. The discussion came out about writing consistent NEW code and drive new developers.
I just wanted to point to this page that Adw wrote about Jetpack code style, i find it almost (damn for loop without braces!) perfect, and showing code examples, so that you can directly compare readability and get your idea: https://wiki.mozilla.org/Labs/Jetpack/Reboot/Style_Guide
Still, most of this is not what the “common” guide says, a clear example that this is a no-solutions problem.
The best thing is probably to try being consistent inside each module, unfortunately looks like code styles follow a trend, when a majority prefers something, and the year later is the opposite 🙂
Marco: I don’t think you need to tell new contributors anything about style, apart from maybe saying “follow local file- or module-style to avoid introducing style mixups”. As for what to do when changing old code, I’m somewhat ambivalent – I think it’s fine to attempt to “clean up” old style to reflect new common practices, even if it means introducing (perhaps temporary) inconsistency within the file. Code style inconsistency just doesn’t bother me very much.
I strongly disagree. Maybe it’s because I write patches to lots of different modules, including patches that affect multiple modules with different styles. I *hate* having to constantly check nearby code to see what the style is for file I’m modifying. There’s no muscle memory, and it really slows me down. I don’t understand how you can see this as a desirable state.
Also, the problem is just as bad for reviewers, if not worse. Checking that the style in a patch is correct for each file is super hard if you allow the style to vary file to file. But if you don’t check, then inevitably the result will be style that varies from function to function or from line to line. Does that sound OK to you?
Roc: I’m surprised that it slows you down. Generally a quick look at the file is enough to discern existing style, and taking that into account isn’t something that I find difficult when writing new code. I guess I probably don’t write code for differently-styled modules as frequently or regularly as you do.
I also don’t think I buy your “super hard”, re: reviewing. I’d buy “pain to remember to do”, perhaps, but most often when I’m reviewing, style variations tend to jump out (sometimes just from context, other times because I’m familiar with the file/module style already), and I generally look at the entire files being patched anyways, so I personally haven’t found that to be a problem.
I contribute to a lot of projects and they all have different coding styles. So I often use the wrong format. I’ve found an equilibrium there: Either you have a tool that automatically formats my code correctly (Webkit and GStreamer have that), you don’t care about correct formatting too much (glib, Gtk) or you fix the style issues when applying my patch.
But I’m grown-up enough to know that it’s not my fault if the formatting of my patch is wrong. It’s your fault for running a project that can’t autoformat its code.
gavin: constantly taking “a quick look at the file” does slow me down. Why would that be surprising?
I generally do not look at the whole file when reviewing — in many cases that would make reviews take enormously longer. 3-8 lines of context is often not enough to tell you what you need to know.
I’m not surprised that if you mostly work in modules that have a consistent style, you would not be worried about style inconsistency across modules.
Benjamin: I know Webkit has a style-checker script, but I was not aware it had a tool that actually reformats code for you. Got a link?
How are you editing files if not by loading them into an editor and looking at them? 🙂 As for reviewing, I almost never review patches based on context alone – I didn’t think I was anomalous in that regard.
What actually slows you down? Trying to maintain a consistent style, or dealing with inconsistent styling? Both? Would things be better if we gave up trying to be consistent entirely, even within files? I’d be fine with that too 🙂
I guess I still find it really surprising that style inconsistency would actually impact you as much as you say it does. I wonder how reflective your situation is of the broader development community’s…
“A foolish consistency is the hobgoblin of little minds”… I tend to agree. IMO indentation is the one thing you have to be consistent on.