[erlang-questions] Feedback on inaka/erlang_standards

[Loïc Hoguin]

Hello,

I want to initiate a larger discussion that Twitter allows for the 
inaka/erlang_standards repository. I hope you don't mind that I do it 
here, Github issues is quite terrible for discussions and I feel that 
talking about these things with a larger number of people can bring 
better feedback.

I will take all points listed in the README at 
https://github.com/inaka/erlang_standards and comment if I have 
something (hopefully) interesting to say about it.

I know they are "Inaka's coding standards" but they are also used for 
the Elvis coding standard checker project which would be interesting to 
try and integrate as part of more people's workflow (regardless of 
projects using the same standards or not, simply checking that 
contributions respect it is already quite nice to have).

An example output can be seen there: 
https://gist.github.com/elbrujohalcon/332db8bc343f81235462

So let me start now.

 > Spaces over tabs, 2 space indentation.

Whatever floats your boat. I just want to point out here that 2-4 space 
vs tab vs mixed never lead anywhere. My problem with it though is that 
the Elvis tool has an option "no_tabs". That's very opinionated, instead 
you should have an option that allows you to define indentation. For 
example I want to know if people use spaces instead of tabs in my projects.

 > Surround operators and commas with spaces.

As much as I like having these with spaces around, I have an issue with 
that. I feel that the default should be "whatever Erlang is doing". For 
example maps, it does "#{a => b,c => d}". The default your tool should 
do is exactly what Erlang does, because you can then just copy paste 
everything and not have to worry. We could extend that logic to doing 
what erl_pp does but people really like their tabs/spaces/goats so 
that'd probably be taking it a bit too far.

 > Try to always separate PRIVATE and PUBLIC functions in groups, with 
the public ones first, unless it helps readability and code discovery.

I have to disagree with that. This is what I have done for a *long time* 
but I experimented and changed my mind recently.

What I do now is for *each* exported function (nice OOP language btw), 
start with the exported function, then the internal functions it calls, 
and then the unit tests (and performance tests). Then do the same for 
the next exported function and so on. It doesn't fit all kinds of 
modules (of course!) but for modules that aren't doing processes stuff 
(like gen_servers) it's very nice.

For example: 
https://github.com/ninenines/cowlib/blob/master/src/cow_qs.erl - if that 
module was doing the old public, private, tests, it would be pretty much 
unreadable. Having the unit tests right there with the code is very nice 
because you can easily see what the code takes and returns.

 > Don't design your system using god modules (modules that have a huge 
number of functions and/or deal with very unrelated things)

Well that's fine if your problem domain is simple, but not so much 
otherwise. The problem with your approach comes from being able to set a 
maximum number of functions project-wide. In Cowboy for example most 
modules are small, and cowboy_req is the exception. I know cowboy_req 
has a lot of exported functions, I don't want a tool to tell me that. 
It's meant to be this way.

 > Don't write the same code in many places, use functions and variables 
for that

I have for a long time been a proponent of applying DRY only when this 
code exists more than 2 times, and the code is significant enough. I 
care little about repeated code that's 2 lines long for example. I don't 
have hard limits there but it would be nice to analyze a few projects 
with different settings and find what's the most useful.

 > When having lots of modules, use subdirectories for them, named with 
a nice descriptive name for what that "package" does.

Sounds like you need to separate into different applications.

 > Don't write spaghetti code (A list comprehension with a case inside, 
or blocks with begin/end, and nested stuff)

Not what I call spaghetti code? I'm not even sure what this is about. 
List comprehensions with a case or begin/end inside can be immensely 
useful. Especially the begin/end ones.

 > Don't use  if .

I would say the opposite. ROK has already argued on that, no need to 
duplicate. :-)

 > Macros should be named in ALL_UPPER_CASE:

I would say "Don't use macros" instead. I would love the tool to tell me 
when it sees macros that aren't ?MODULE (and a way to whitelist a few 
small ones).

 > Write the -spec's for your public fun's, and for private fun's when 
it adds real value for documentation purposes. Define as many types as 
needed.

Honestly I'd remove the comment about internal functions (wow OOP man), 
they're generally not very useful, unless Dialyzer complains.

 > Module comments go with %%%, function comments with %%, and code 
comments with %.

Or you could stop worrying and enjoy life. Seriously that's some 
nitpicking you got there. :-)

Instead of saying that you should probably worry that people *actually* 
comment their code.

 > Try to write functions with a small number of lines. 12 lines per 
function except for integration tests is a good measure.

12, seriously? If I'm writing a game the init function or main loop 
function will hardly be less than 20 lines.

 > Help  dialyzer  and  xref  as much as you can, so that they can work 
for you

Like with most things in life, there's an optimal middle ground. Don't 
go out of your way to make tools happy. Tools are supposed to help you 
not hinder you.

 > When having many nested "include files", use -ifndef(HEADER_FILE_HRL) 
.... -endif so they can be included in any order without conflicts.

I'll need an option that complains when -include is used (again, with 
whitelist, probably never warn about file.hrl either). :-)

That's it. Hope to spark some (not too pointless) discussion.

Enjoy!

-- 
Loïc Hoguin
http://ninenines.eu