Suggested reading material: http:https://www.erlang.se/doc/programming_rules.shtml
Table of Contents:
- Rules & Conventions
- Source Code Layout
- Spaces over tabs
- Use your spacebar
- 80-90 column per line
- Maintain existing style
- Avoid deep nesting
- More, smaller functions over case expressions
- Group functions logically
- No God modules
- Simple unit tests
- Honor DRY
- Avoid dynamic calls
- Group modules in subdirectories by functionality
- Don't write spaghetti code
- Syntax
- Naming
- Design
- Comments
- Exceptions
- Strings
- Macros
- Misc
- Tools
- Source Code Layout
- Suggestions & Great Ideas
"Things that may be used as reason to reject a Pull Request."
Spaces over tabs, 2 space indentation.
% bad (inconsistent)
handle_info(timeout, State=#state{}) ->
try
EpisodeOnAir = State#state.on_air,
OrgId = State#state.org#org.id,
NextEpisode = conflux_db:episode_on_air(OrgId),
{ok, StateEpisode, NextStopTime, NewEpiName} =
case NextEpisode of
EpisodeOnAir -> {
ok,
EpisodeOnAir,
State#state.next_stop_time,
State#state.on_air_name
};
undefined ->
% No next episode, so let's see if we should keep this one on the air.
Now = calendar:datetime_to_gregorian_seconds(calendar:universal_time()),
if
Now >= State#state.next_stop_time ->
handle_cast({stop_on_air}, State),
{ok, undefined, 0, undefined};
true ->
lager:debug(
"Keeping current episode for another: ~p seconds",
[State#state.next_stop_time - Now]
),
% better but still bad (consistent but 4 spaces)
handle_info({json, InitEvent}, State = #state{status = wait_for_init}) ->
lager:debug("New init message"),
NewVersion = State#state.version + 1,
VersionBin = conflux_utils:integer_to_binary(NewVersion),
VersionSuffix = integer_to_list(NewVersion) ++ ".json",
ChangesBin =
lists:foldr(
fun({json, Json}, _Acc) -> Json; %%NOTE: Init message renders everything before it invalid
(Change, Acc) ->
try conflux_utils:json_encode(Change) of
Json ->
case Acc of
<<>> -> Json;
Acc -> <<Acc/binary, ",\n\t", Json/binary>>
end
catch
_:Error ->
lager:error("Comet: Invalid Json: ~p / Error: ~p", [Change, Error]),
<<>>
end
end, <<>>, State#state.changes),
spawn_backup(State),
% good
stop(EpisodeName) ->
case on_air(EpisodeName) of
undefined -> ok;
Pid ->
gen_server:cast(Pid, stop),
catch erlang:unregister(EpisodeName),
ok
end.Reasoning: This is not intended to allow deep nesting levels in the code. 2 spaces are enough if the code is clean enough, and the code looks more concise, allowing more characters in the same line.
Surround operators and commas with spaces.
% bad
my_function(My,Space,Bar)->[is,not,working].
% good
my_function(Hey, Now, It) -> ["works" ++ "again" | [hooray]]].Reasoning: Again, easier to find / read / etc.
Stick to 80-90 chars per line, some of us still have to use vi sometimes, specially when editing code via ssh. Also, it allows showing more than one file simultaneously on a wide screen or laptop monitor.
% bad
function1([], Arg) ->
do_something(Arg).
function1(Foo = #rec{field1 = FF1, field2 = FF2, field3 = FF3}, Bar = #rec{field1 = BF1, field2 = BF2, field3 = BF3} | Rest], Arg2) ->
do_something(FF1, FF2, FF3, BF1, BF2, BF3, function1(Rest, Arg2)).
% good
function1([], Arg) ->
do_something(Arg).
function1([Foo, Bar | Rest], Arg2) ->
Foo = #rec{field1 = FF1, field2 = FF2, field3 = FF3},
Bar = #rec{field1 = BF1, field2 = BF2, field3 = BF3},
do_something(FF1, FF2, FF3, BF1, BF2, BF3, function1(Rest, Arg2)).When editing a module written by someone else, stick to the style in which it was written. If a project has an overall style, stick to that when writing new modules as well.
% bad
-exports([ function1
, i_like
, preceding_commas, i_dont, like_them
]).
% good
-exports([ function1
, i_like
, preceding_commas
, i_dont
, like_them
, but_i_respect_your_style
]).Reasoning: It's better to keep a module that just looks ugly to you than to have a module that looks half ugly to you, half ugly to somebody else.
Try not to nest more than 3 levels deep.
% bad
function1(Foo) ->
case Foo of
true ->
Bar = do_something(Foo),
case Bar of
{ok, WhatIReallyWanted} ->
try
InterimValue1 = this_might_blow_up(WhatIReallyWanted),
InterimValue2 = compute(InterimValue1),
get_final_value(InterimValue2)
catch
_:_ ->
% dont do this either, let it fail
io:format("something bad happened")
end;
undefined ->
error(my_application_error)
end;
false ->
io:format("Oh noes!")
end.
% good
function1(true) ->
Bar = do_something(Foo),
really_descriptive_name(Bar);
function1(false) ->
io:format("Oh noes!").
really_descriptive_name({ok, WhatIReallyWanted}) ->
try
InterimValue1 = this_might_blow_up(WhatIReallyWanted),
InterimValue2 = compute(InterimValue1),
get_final_value(InterimValue2)
catch
_:_ ->
% dont do this either, let it fail
io:format("something bad happened")
end;
really_descriptive_name(undefined) ->
error(my_application_error).Reasoning: Nested levels indicate deep logic in a function, too many decisions taken or things done in a single function. This hinders not only readability, but also maintainability (making changes) and debugging, and writing unit tests.
Use pattern-maching in clause functions rather than case's. Specially important if the case is:
- the top-level expression of the function
- huge
% bad
my_fun(Arg) ->
case Arg of
option1 -> process1();
option2 -> process2()
end.
my_other_fun(Arg) ->
…
case Something of
option1 ->
…multiple lines of code…;
option2 ->
_multiple lines of code…;
…many other options…
end,
….
% good
my_fun(option1) -> process1();
my_fun(option2) -> process2().
my_other_fun(Arg) ->
…
something_to_do_with(Something),
….Try to always separate PRIVATE and PUBLIC functions in groups, with the public ones first, unless it helps readability and code discovery.
% bad
-module(my_module).
-exports([public1/0, public2/0]).
public1() -> private3(atom1).
private1() -> atom2.
public2() -> private2(private1()).
private2(Atom) -> private3(Atom).
private3(Atom) -> Atom.
% not that bad
-module(my_module).
-exports([public1/0, public2/0]).
public1() ->
case application:get_env(atom_for_public_1) of
{ok, X} -> public1(X);
_ -> throw(cant_do)
end.
public1(X) -> private3(X). % This is a private function but it's obviously related just to the one before
public2() -> private2(private1()).
private1() -> atom2.
private2(Atom) -> private3(Atom).
private3(Atom) -> Atom.
% good
-module(my_module).
-exports([public1/0, public2/0]).
public1() ->
case application:get_env(atom_for_public_1) of
{ok, X} -> private3(X);
_ -> throw(cant_do)
end.
public2() -> private2(private1()).
%%% PRIVATE FUNCTIONS %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
private1() -> atom2.
private2(Atom) -> private3(Atom).
private3(Atom) -> Atom.Reasoning: Well structured code is easier to read/understand/modify.
Don't design your system using god modules (modules that have a huge number of functions and/or deal with very unrelated things)
Single responsibility applies to tests as well. When writing unit tests, keep them short and don't pur more than 1 or 2 asserts per test
This convention is specifically put in this list (instead of treat it as a great idea) so that reviewers can reject PRs that include the same code several times or PRs that re-implement something that they know it's already done somewhere else.
If there is no specific need for it, don't use dynamic function calling.
When having lots of modules, use subdirectories for them, named with a nice descriptive name for what that "package" does.
Remember to properly configure your Emakefile and rebar.config to handle that
Don't write spaghetti code (A list comprehension with a case inside, or blocks with begin/end, and nested stuff)
% bad
Organizations =
[binary_to_list(Org)
|| Org <- autocomplete_db:smembers(
case Product of
consumer -> <<"organizations">>;
spot -> <<"product:", (?SPOTMD_KEY)/binary, ":organizations">>;
whisper -> <<"product:", (?WHISPER_KEY)/binary, ":organizations">>
end)],
% good
Organizations =
[binary_to_list(Org) || <- Org <- product_organizations(Product)],
Erlang syntax is horrible amirite? So you might as well make the best of it, right? Right?
Don't use
if.
% bad
if
SomethingIsTrue -> do_something();
true -> default()
end
% good
case Something of
an_appropriately_named_thing -> good(not_a_boolean);
_ -> default()
endReasoning: Clarity of intention and using the right tool for the job.
Use the same variable name for the same concept everywhere (even in different modules).
% bad
…
my_function(OrganizationID) -> …
…
my_other_function(OrgID) -> …
…
% good
…
my_function(OrganizationID) -> …
…
my_other_function(OrganizationID) -> …
…Reasoning:
When trying to figure out all the places where an OrgID is needed (e.g. if we want to change it from string to binary), it's way easier if we can just grep for OrgID instead of having to check all possible names.
Name your state records
#stateand use-type state():: #state{}in all your OTP modules.
Variables beginning with _ are still variables, and are matched and bound, the _ just keeps the compiler from warning when you don't use them. If you add the _ to a variable's name, don't use it. Say what you mean, mean what you say.
% bad
function(_Var) ->
…
other_function(_Var),
…Don't use boolean parameters
% bad
square:draw(EdgeLength, true).
square:draw(EdgeLength, false).
% good
square:draw(EdgeLength, full).
square:draw(EdgeLength, empty).Reasoning: Clarity of intention and not requiring the reader to check the function definition.
Stick to one convention when naming modules (i.e: tt_something vs ttsomething vs something).
Use iolists instead of string concatenation whenever possible
% bad
"/users/" ++ UserId ++ "/events/" ++ EventsId
% good
["/users/", UserId, "/events/", EventsId]Reasoning: Performance :P
Macros should be named in ALL_UPPER_CASE:
% bad
-define(?my_macro, '...').
-define(?MYMACRO, '...').
-define(?My_Macro, '...').
-define(?_mY_L33t_M@Cr0, '...').
% good
-define(?MY_MACRO, '...').
-define(?YOUR_MACRO, '...').Reasoning: It makes it easier not to duplicate macro names, to find them through grep, etc.
Don't use macros for module or function names
% bad
function() ->
?SM:function(Args).
% good
function() ->
some_module:function(Args).Reasoning: Copying lines of code to the console for debugging (something that happens a lot) becomes a really hard task if we need to manually replace all the macros.
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.
-type option_id():: atom().
-type option_count():: non_neg_integer().
-type option_percnt():: non_neg_integer().
-type option():: {option_id(), option_count()}.
-type result():: {option_id(), option_percnt()}.
-spec calc([option()]) -> [result()].
calc(Options) ->
TotalCounts = [ X || {_,X} <- Options],
calc(Options, lists:sum(TotalCounts)).erReasoning: Dialyzer output is complicated as is, and is improved with good type names. In general, having semantically loaded type names for arguments makes reasoning about possible type failures easier, as well as the function's purpose.
Avoid using records in your specs, use types.
% bad
-record(state, {field1, field2}).
-spec function(#state{}) -> #state{}.
function(State) ->
% ... do something,
NewState.
% good
-record(state, {field1, field2}).
-type state() :: #state{}.
-spec function(state())) -> state()).
function(State) ->
% ... do something,
NewState.Unless you know your project will be compiled with R14 or lower, use
-callbackinstead ofbehavior_info/1for your behavior definitions. In general, avoid deprecated functionality.
In your rebar.config or Erlang.mk, specify a tag or commit, but not master.
Don't let errors and exceptions go unlogged. Even when you handle them, write a log line with the stack trace so that somebody watching it can understand what's happening
When using lager, use the different logging levels with the following meanings:
debug: Very low-level info, that may cover your screen and don't let you type in it :Pinfo: The system's life, in some detail. Things that happen usually, but not all the time. You should be able to use the console with acceptable interruptions in this level.notice: Meaningful things that are worth noticing, like the startup or termination of supervisors or important gen_servers, etc…warning: Handled errors, the system keeps working as usual, but something out of the ordinary happenederror: Something bad and unexpected happen, usually an exception or error (DO log the stack trace here)critical: The system (or a part of it) crashed and somebody should be informed and take action about italert:emergency: