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 fail requires you to know that it’s just an alias for raise with 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 disallowing if 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, not Foo = Class.new(RuntimeError). The later requires more knowledge about Rubys meta model and what the argument to Class.new does 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.
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)
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.
Note: This discussion was imported from Loomio. Click here to view the original discussion.
I don’t want assignments in if statements, you rarely need them directly and you could assign the result of the assignment to a variable and then check the variable … this way the reader can not confuse this easily with the tests in if statements.
For the rest: I have no opinion and trust you do some sane decisions.
About JS camel case, I like the PEP8 i.e : CamelCase for classes or objects or others, snake_case for functions with the minimum length for function name. I don’t like the idea of auto-documented code. The actual code (espacially JS) should be systematically documented using /** */ coments.
You mean self-documenting code? If so, any reason? What’s wrong with code that’s self explanatory? And what’s wrong with spending effort on making code clearer than spending that effort on explaining your incomprehensible code instead and making sure the comments stay up-to-date? Do you never experience comment rot?
You mean self-documenting code? If so, any reason? What’s wrong with code that’s self explanatory?
A code is never self-explanatory. It can be comprehensible by someone with sufficient knowledges in a particular language when it’s well-written, but that is not self-explanatory, that is just good programming.
But when someone not really comfortable with a language reads it, a code is not self explanatory. This is why we always continue to write documentations…
And what’s wrong with spending effort on making code clearer than spending that effort on explaining your incomprehensible code instead and making sure the comments stay up-to-date?
But our code is not to teach you the language, we’re writing a software here, not a book that explains the language or a sample application for someone learning the language.
Not more than I experience code rot.
Lucky you then! I can’t say that’s my experience, in fact I’ve seen a lot in diaspora itself.
Make comments when necessary, documenting things that may be solved differently and explain why it was done this way.
I think these styleguides set the “must have” part - everyone should still feel free to add a comment to newly written code if it is necessary but not demanding comments when they are not needed.
More important: If you change something, validate that comments are still accurate, so no one reads comments and thinks “okay, I understood what this does” - but the code was changed in between and does something completely different now.
But our code is not to teach you the language, we’re writing a software here, not a book that explains the language or a sample application for someone learning the language.
Common, you know as well as me that it’s not about the language. See, even Java devs who I don’t think they are bad document their code a lot.
Make comments when necessary, documenting things that may be solved differently and explain why it was done this way.
We all know that diaspora* is not documentaed enough and new devs will never come if this is so difficult to understand the code structure. There are sometimes legacy things that appear to be crazy, espacially in the JS code. Don’t forget that we don’t code for us, but for those that will come after us.
Lucky you then! I can’t say that’s my experience, in fact I’ve seen a lot in diaspora itself.
There should be a right balance. At leat every JS function should be documented with /** */ comments and two or three lines. Just see JSXC source. It is highly commented and thanks to this easy to modify and maintain.
I think, from now, at least every new JS contribution has to be commented.
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, like bar_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.
I think, from now, at least every new JS contribution has to be commented.
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).
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.
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.
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.
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.
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.
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?
