Styleguides
Jonne Haß
Public
Seen by 69
So looks like we want to establish coding styleguides and actually enforce them. Let's try to find one that we all like.
Tools
We recently enabled Hound for Javascript. That should settle JSHint as linter for Javascript. Hound supports Ruby and SCSS linting too, through Rubocop and and scss-lint respectively. Those seems to be flexible tools, so let's settle on these too and enable Hound for Ruby and SCSS too.
Ruby
Rubocop defaults to the bbatsov styleguide plus some additional checks not specified there. Hound changes some of Rubocops defaults, unless you specify your own config, to match the thoughtbot styleguide. I can not fully agree to either, however bbatsov's provides a good base, especially since Rubocops defaults are modeled after it, which makes it less work to configure. Therefore I started a Rubocop config that we can use as a base for discussion. I tried to add some reasoning to most entries. However let me point out the main choices and deviations from bbatsov here:
- Line length: 80 is a bit too short to enforce, especially since we have bigger screens these days. I'm working with 120 for years now and so far nobody complained.
- Different spacing rules for
{ ... }blocks and hash literals, in order to differentiate them better - Using
failrequires you to know that it's just an alias forraisewith no benefit, in other places the styleguide already discourages using aliases. Having more than one semantic meaning for exceptions is wrong in the first place, it suggests that you're using them for control flow. - No safe assignment, that is, do not allow
if (foo = bar)while disallowingif foo = bar. We should either allow assignments in if statements or not, this is just inconsistent. - Do not enforce block argument names for
inject. Enforcing variable names, especially single letter ones is just plain silly.
Additional rules and deviations that I couldn't make out a cop for:
- Define exceptions as
class Foo < RuntimeError; end, notFoo = Class.new(RuntimeError). The later requires more knowledge about Rubys meta model and what the argument toClass.newdoes for basically no benefit. - Disallow spaces inside string interpolation that is allow
"a #{b} c"but not"a #{ b } c". This is to settle to one style and keep it consistent. - Disallow doing control flow with
&&and||outside conditionals. The only valid uses of&&and||are in a condition and to compute a predicate that's returned from a method.
Please read bbatsov's styleguide and then my Rubocop config if you want to know more.
Javascript
(last edit by Steffen)
The proposed deviations for our settings are: (follow the links for more information about the options)
- camelcase :
true - latedef:
true - maxlen:
120 - nonew:
false. nonew would prohibit the use of constructor functions for side-effects. While testing that setting to me it looked like we need those side-effects for our backbone code. Does anyone have more information about that? (I am no javascript/backbone.js expert) - quotmark:
'single' - eqeqeq:
true - freeze:
true - indent:
2 - devel:
true. We usealertandconfirmin some places. - eqnull:
true. - lastsemic:
true - supernew:
true. We use that in some places. I don't know if we should get rid of it but because we are currently using it this is set totrue. - I also added some global variables we are using.
SCSS
(edited by Pablo Cúbico)
I'm suggesting using the styleguide from Bootstrap, which is a little leaner than things like BEM and OOCSS.
Some ideas from its maker:
http://markdotto.com/2014/07/23/githubs-css/
I'll try to match the configuration of SCSS Lint to it and post it here.
Porting the codebase
We won't do it. As reasoned excellently in Hound's faq, the way to adopt a styleguide is to gradually port all code you write and touch to it, not to do it in one go.
Deleted account Mon 23 Feb 2015 7:37AM
In both of your examples I see 40-60% of the comments as superfluous.
That's what I say : you see. But you don't code for yourself, you code for every other developer of diaspora*.
A code is never self-explanatory. It might be for you, but I'm sorry to say that, given a complex function, a name with two or three words is never enough. Especially in JS.
and I know enough developers who don’t like it
Oh ! This is an actual study from the institute of the wet finger !?
At work, I'm responsible for the documentation of all the projects. I see every day devs who were convinced their code was self-documented, no need for comments or actual document. And see, 5 years later, they are unable to say what their code do, what they did it for. For some project, we are even unable to say what they are used for and if they are stil up-to-date ! Documentation and comments are not luxury.
Giving an overview at the top of your class / file about its purpose can be good if it’s non-obvious
Who determines it is non-obvious ? We have to be clear : wether we do, wether we don't. Where you see superfluous comments, I see a sane way of coding where comment helps to see the logic and the structure of the code.
Faldrian Mon 23 Feb 2015 7:58AM
When you want to know how something specific works, you will always read the code.
But what we really would need (and I think this is more important than inline-comments or class-comments) is a top-down overview of the whole project, maybe with 2-3 different zoom-levels, explaining how everything works together (graphically with annotations). That would not be part of this styleguide-thing, but a new piece of works.
@augier I don't think we have to introduce mandatory comments here. If you think they are useful and want to add some - you can, but you don't have to.
Jonne Haß Mon 23 Feb 2015 12:21PM
I guess we have to put that point to vote then, let me answer to one more of your points though:
given a complex function, a name with two or three words is never enough.
The problem in that case is not that you have no comment on it, the problem is that you didn't break up your function. I only have a Ruby example at hand, but let's take it. This method calls a bunch of other methods that aren't used anywhere else, they're extracted not for reusability but for readability and to reduce the size of the method.
I can recommend this book, it has an excellent chapter on comments too.
Deleted account Mon 23 Feb 2015 2:58PM
But what we really would need (and I think this is more important than inline-comments or class-comments) is a top-down overview of the whole project, maybe with 2-3 different zoom-levels, explaining how everything works together (graphically with annotations). That would not be part of this styleguide-thing, but a new piece of works.
Agree.
@faldrian : I don't have strong opinion about Ruby, but concerning JS, and given how this language is unnecessarily verbose, I really think we should have document comments for each function more than 4 lines. And there is a lot of them.
The problem in that case is not that you have no comment on it, the problem is that you didn’t break up your function.
This is also a problem, for sure. The there are many complex function of that kind in the JS part. And we all know that they will never be refactored.
Your Ruby example is not fully relevant as Ruby is a very concise and easy to read, which is not the case of JS.
Please also notice that it has many doc comments and is not as difficult to read as you say.
Also, I think we should do an inventory of the SCSS files during the port to BS3.
Faldrian Mon 23 Feb 2015 3:05PM
as Ruby is a very concise and easy to read, which is not the case of JS.
I think your milage may vary. I find ruby very disturbing to read and feel like in wonderland when I can read JS. So this does not depend on the language.
Jonne Haß Mon 23 Feb 2015 3:11PM
Also the comment in my example is for the API docs, the extracted private API has no comments at all.
Deleted account Mon 23 Feb 2015 4:07PM
About quotmark, I've adopted the styleguide of Ruby mine for both JS and Ruby, i.e : ' ' by default and " " only when necessary.
Jonne Haß Mon 23 Feb 2015 6:12PM
In Ruby, I found myself repeatedly changing 's to "s since I wanted to use string interpolation or have an interpreted \n and so on, therefore I adopted to default to "s and only use ' if I need to their semantics. Any such reason for preferring ' in JS?
Faldrian Mon 23 Feb 2015 6:25PM
Of course! :D https://twitter.com/mathias/status/247708186696634368
(No, there is not even a performance difference ... http://jsperf.com/single-quote-vs-double-quote/3 )
Jonne Haß · Sun 22 Feb 2015 11:35PM
In both of your examples I see 40-60% of the comments as superfluous. I mean things like
/** Some constants */? Or/** My bar id */,bid: null,, here the time would've been better spend by using a self-explanatory name, likebar_id. Most of the comments of your Java example are for the API docs, not to explain the actual code or even the classes structure.Java styleguides often have such a requirement, and I know enough developers who don't like it. Therefore I highly disagree with such a requirement. Giving an overview at the top of your class / file about its purpose can be good if it's non-obvious, but it should be applied as necessary, not as necessity. The later only leads to superfluous noise in the code, like in the examples I pointed out (which I didn't even have to scroll down a page for).