Writing Beautiful Code

Erlang Factory San Fran Bay Area 2013

Garrett Smith, CloudBees

@gar1t

Presenter Notes

"Very Difficult"

  • "appears to work" - easy
  • "correct" - far more difficult
  • "can be understood by others" - very difficult
  • "can be modified in the future" - very very difficult
  • "efficient" - very very very difficult

- Joe Armstrong

Presenter Notes

"Scruples"

"it doesn't even work. this is no time for scruples"

"now it works." "now it's time for scruples"

- Kent Beck? (Twitter)

Presenter Notes

A Whopper - Part 1

handle_changes(Args1, Req, Db0) ->
    #changes_args{
        style = Style,
        filter = FilterName,
        feed = Feed,
        dir = Dir,
        since = Since
    } = Args1,
    {FilterFun, FilterArgs} = make_filter_fun(FilterName, Style, Req, Db0),
    Args = Args1#changes_args{filter_fun = FilterFun, filter_args = FilterArgs,
    Start = fun() ->
        {ok, Db} = couch_db:reopen(Db0),
        StartSeq = case Dir of
        rev ->
        couch_db:get_update_seq(Db);
        fwd ->
        Since
        end,
        {Db, StartSeq}
    end,
    % begin timer to deal with heartbeat when filter function fails
    case Args#changes_args.heartbeat of
    undefined ->
        erlang:erase(last_changes_heartbeat);
    Val when is_integer(Val); Val =:= true ->
        put(last_changes_heartbeat, now())
    end,

Presenter Notes

A Whopper - Part 2

    if Feed == "continuous" orelse Feed == "longpoll" ->
        fun(CallbackAcc) ->
        {Callback, UserAcc} = get_callback_acc(CallbackAcc),
        Self = self(),
        {ok, Notify} = couch_db_update_notifier:start_link(
            fun({_, DbName}) when  Db0#db.name == DbName ->
            Self ! db_updated;
            (_) ->
            ok
            end
        ),
        {Db, StartSeq} = Start(),
        UserAcc2 = start_sending_changes(Callback, UserAcc, Feed),
        {Timeout, TimeoutFun} = get_changes_timeout(Args, Callback),
        Acc0 = build_acc(Args, Callback, UserAcc2, Db, StartSeq,
                 <<"">>, Timeout, TimeoutFun),
        try
            keep_sending_changes(
            Args#changes_args{dir=fwd},
            Acc0,
            true)
        after
            couch_db_update_notifier:stop(Notify),
            get_rest_db_updated(ok) % clean out any remaining update messages
        end
    end;

Presenter Notes

A Whopper - Part 3

    true ->
        fun(CallbackAcc) ->
        {Callback, UserAcc} = get_callback_acc(CallbackAcc),
        UserAcc2 = start_sending_changes(Callback, UserAcc, Feed),
        {Timeout, TimeoutFun} = get_changes_timeout(Args, Callback),
        {Db, StartSeq} = Start(),
        Acc0 = build_acc(Args#changes_args{feed="normal"}, Callback,
                 UserAcc2, Db, StartSeq, <<>>, Timeout, TimeoutFun),
        {ok, #changes_acc{seq = LastSeq, user_acc = UserAcc3}} =
            send_changes(
            Args#changes_args{feed="normal"},
            Acc0,
            true),
        end_sending_changes(Callback, UserAcc3, LastSeq, Feed)
        end
    end.

Presenter Notes

Topics

  • Function names
  • Argument and variable names
  • Function scoping
  • Function types
  • Case and if expressions
  • Comments
  • Misc

Presenter Notes

Non Metaphysics

Presenter Notes

Function Names

  • avoidCamelCase, this_is_better
  • Canonical when applicable
    • new
    • start, start_link
    • init
    • handle_xxx
  • Rename after it works
  • Let common sense prevail!

Presenter Notes

Argument and Variable Names

  • AlwaysUseCamelCase
  • Short, but not too short
  • Let common sense prevail!
  • Avoid type naming
    • AgeNum -> Age
    • UserList -> Users
    • CheckFlag -> Check

Presenter Notes

Required and Optional Arguments

  • Provide a the simplest possible case
  • Add arguments to cover other common cases
  • Use a single Options argument to capture non-essential args

Presenter Notes

Required and Optional Args

replicate_master(Master, Filename, Position) ->
    case mysql_admin:replicate_master(Master, Filename, Position) of
        ok -> ok;
        {error, timeout} -> error(timeout)
    end.

replicate_master(Master, Filename, Position, Options) ->
    case proplists:get_bool(dont_check, Options) of
        true ->
            start_slave(Master, Filename, Position);
        false ->
            replicate_master(Master, Filename, Position)
    end.

Presenter Notes

Function Scoping

  • Dynamic / iterative - get it working, then improve
  • Do one thing
  • Explain to your rubber duck
  • Explain to your therapist
  • Hard, hard, hard work

Presenter Notes

Function Types

  • Dispatch
  • Translation
  • Maybe
  • Result Handler
  • Accumulator
  • List Processor
  • Phase Processor
  • State Init

Presenter Notes

Dispatch

handle_msg({"user.create", Details}, _From, State) ->
    handle_user_create(Details, State);
handle_msg({"user.delete", Id}, _From, State) ->
    handle_user_delete(Id, State);
handle_msg(Msg, _From, State) ->
    log({unhandled_msg, Msg}),
    {noreply, State}.

Or:

ping(Timeout) ->
    gen_server:call(?SERVER, ping, Timeout).

Presenter Notes

Translation

param_to_action(undefined, _Params) -> undefined;
param_to_action("clear", _Params) -> clear;
param_to_action("ignore", Params) -> ignore_timespec(Params);
param_to_action(Other, _Params) -> {other, Other}.

Alternative to:

case Action of
    undefined -> undefined;
    "clear" -> clear;
    "ignore" -> ignore_timespec(Params);
    Other -> {other, Other}
end.

Presenter Notes

Maybe

delete_expired_user(User) ->
    maybe_delete_user(user_expired(User), User).

maybe_delete_user(false, _User) -> ok;
maybe_delete_user(true, User) ->
    delete_user(User).

Alternative to:

delete_expired_user(User) ->
    case user_expired(User) of
        true -> delete_user(User);
        false -> ok
    end.

Presenter Notes

Result Handler

delete_user(User) ->
    handle_delete_user(users:delete(User)).

handle_delete_user(ok) -> ok;
handle_delete_user({error, Err}) ->
    error({user_delete, Err}).

Alternative to:

delete_user(User) ->
    case users:delete(User) of
        ok -> ok;
        {error, Error} -> error(Error)
    end.

Presenter Notes

Accumulator

format_users(Users) -> format_users_acc(Users, []).

format_users_acc([], Acc) -> lists:reverse(Acc);
format_users_acc([User|Rest], Acc) ->
    Formatted = format_user(User),
    format_users_acc(Rest, [Formatted|Acc]).

Alternative to:

format_users(Users) ->
    [format_user(User) || User <- Users].
format_users(Users) ->
    Folder = fun(User, Acc) -> [format_user(User)|Acc] end,
    lists:reverse(lists:foldl(Folder, [], Users)).

Presenter Notes

List Processor

print_users([]) -> ok;
print_users([User|Rest]) ->
    print_user(User),
    print_users(Rest).

Alternative to:

print_users(Users) ->
    lists:foreach(fun print_user/1, Users).

Presenter Notes

Phase Processor

init_server() ->
    apply_phases([start, init, verify, notify]).

apply_phases([Phase|Rest]) ->
    handle_phase_result(apply_phase(Phase), Phase, Rest).

handle_phase_result(ok, _Phase, Rest) ->
    apply_phases(Rest);
handle_phase_result({error, Err}, Phase, _Rest) ->
    handle_phase_error(Phase, Err).

Presenter Notes

State Init

new_user(Id, Name, Email) ->
    #user{id=Id, name=Name, email=Email}.

new_user(Id) ->
    case users:lookup(Id) of
        {ok, [Name, Email]} ->
            #user{id=Id, name=Name, email=Email};
        error ->
            error({no_such_user, Id})
    end.

Presenter Notes

Lines Per Function

  • Possible to keep most to < 4 lines
  • Not a rule, a guide
  • Long might be doing too much
  • Exceptions: dispatch and translations

Presenter Notes

Case / If Expressions

  • Moment to pause and think
  • Write it, read it, rewrite it
  • if expressions, unicorn rare?

Presenter Notes

The Imperative Trap

Before:

single_index_bounds(S) ->
    if
       S >= 0 -> LowerBound = S;
       S < 0 -> LowerBound = 0
    end,
    LowerBound.

After:

single_index_bounds(S) when S >= 0 -> S;
single_index_sounds(_) -> 0.

Presenter Notes

Imperative Trap - part 2

Before:

index_defaults(ListLength, Start, End, Step) ->
    case Start==[] of
        true -> Start1 = 0;
        false -> Start1 = Start
    end,
    case End==[] of
        true -> End1 = ListLength;
        false -> End1 = End
    end,
    case Step==[] of
        true -> Step1 = 1;
        false -> Step1 = Step
    end,
    {Start1, End1, Step1}.

Presenter Notes

Imperative Trap - part 2

After:

index_defaults(ListLen, Start, End, Step) ->
    {default_start(Start),
     default_end(End, Len),
     default_step(Step)}.

default_start([]) -> 0;
default_start(N) -> N.

default_end([], ListLen) -> ListLen;
default_end(N, _ListLen) -> N.

default_step([]) -> 1;
default_step(N) -> N.

Presenter Notes

Comments

  • Apart from end-user docs (libraries) I avoid them
  • You may want to avoid them, not sure
  • I also tend to avoid specs

Presenter Notes

Miscellaneous

Presenter Notes

Max Line Length

79

Presenter Notes

Handler Function vs Case

Before:

systemd_service_exists() ->
    cmd_success(
      catch(stax_cmd:run("systemctl", ["is-enabled", "mysqld.service"]))).

cmd_success({0, _}) -> true;
cmd_success(_) -> false.

After:

systemd_service_exists() ->
    case try_systemd_service_enabled() of
        {0, _} -> true;
        _Err -> false
    end.

try_systemd_service_enabled() ->
    catch(stax_cmd:run("systemctl", ["is-enabled", "mysqld.service"])).

Presenter Notes

Extra Variable

Before:

set_mysql_start_timeout({service, Service}) ->
     SedExpr = io_lib:format("/\\[Install\\]/ i TimeoutSec=%b\\n",
                             [?MYSQL_START_TIMEOUT]),
     stax_cmd:quiet("sed", ["-i", SedExpr, Service]).

After:

set_mysql_start_timeout({service, Service}) ->
    Pattern = "/\\[Install\\]/ i TimeoutSec=%b\\n",
    SedExpr = io_lib:format(Pattern, [?MYSQL_START_TIMEOUT]),
    stax_cmd:quiet("sed", ["-i", SedExpr, Service]).

Presenter Notes

Left Edge Alignment

Before:

set_mysql_start_timeout({service, Service}) ->
     SedExpr = io_lib:format("/\\[Install\\]/ i TimeoutSec=%b\\n",
                             [?MYSQL_START_TIMEOUT]),
     stax_cmd:quiet("sed", ["-i", SedExpr, Service]).

After:

set_mysql_start_timeout({service, Service}) ->
     SedExpr =
        io_lib:format(
          "/\\[Install\\]/ i TimeoutSec=%b\\n", [?MYSQL_START_TIMEOUT]),
     stax_cmd:quiet("sed", ["-i", SedExpr, Service]).

Presenter Notes

Extremely Random Snippets

Presenter Notes

gproc - part 1

add_local_property(Name , Value) ->
    ?CATCH_GPROC_ERROR(reg1({p,l,Name}, Value), [Name, Value]).

reg1({_,g,_} = Key, Value) ->
    %% anything global
    ?CHK_DIST,
    gproc_dist:reg(Key, Value);
reg1({p,l,_} = Key, Value) ->
    local_reg(Key, Value);
reg1({a,l,_} = Key, undefined) ->
    call({reg, Key, undefined});
reg1({c,l,_} = Key, Value) when is_integer(Value) ->
    call({reg, Key, Value});
reg1({n,l,_} = Key, Value) ->
    call({reg, Key, Value});
reg1(_, _) ->
    ?THROW_GPROC_ERROR(badarg).

Presenter Notes

gproc - part 2

local_reg(Key, Value) ->
    case gproc_lib:insert_reg(Key, Value, self(), l) of
        false -> ?THROW_GPROC_ERROR(badarg);
        true  -> monitor_me()
    end.

-define(CATCH_GPROC_ERROR(Expr, Args),
                try Expr
                catch
                    throw:{gproc_error, GprocError} ->
                                erlang:error(GprocError, Args)
                            end).

-define(THROW_GPROC_ERROR(E), throw({gproc_error, E})).

Presenter Notes

gproc - part 3

call(Req) ->
    call(Req, l).

call(Req, l) ->
    chk_reply(gen_server:call(?MODULE, Req));
call(Req, g) ->
    chk_reply(gproc_dist:leader_call(Req)).

call(N, Req, l) ->
    chk_reply(gen_server:call({?MODULE, N}, Req));
call(undefined, Req, g) ->
    %% we always call the leader
    chk_reply(gproc_dist:leader_call(Req)).

Presenter Notes

Arrow Alignment

From Joe Armstrong / chandler.

classify_extension(".gif") -> gif;
classify_extension(".jpg") -> jpg;
classify_extension(".png") -> png;
classify_extension(".js")  -> js;
classify_extension(".css") -> css;
classify_extension(".swf") -> swf;
classify_extension(_)      -> html.

Presenter Notes

Embedded Funs

From Robert Virding / luaerl:

encode(B, St) when is_binary(B) -> {B,St};
encode(A, St) when is_atom(A) -> {atom_to_binary(A, latin1),St};
encode(I, St) when is_integer(I) -> {float(I),St};
encode(F, St) when is_float(F) -> {F,St};
encode(B, St) when is_boolean(B) -> {B,St};
encode(nil, St) -> {nil,St};
encode(L, St0) when is_list(L) ->
    {Es,{_,St1}} = lists:mapfoldl(fun ({K0,V0}, {I,S0}) ->
                                          {K1,S1} = encode(K0, S0),
                                          {V1,S2} = encode(V0, S1),
                                          {{K1,V1},{I,S2}};
                                      (V0, {I,S0}) ->
                                          {V1,S1} = encode(V0, S0),
                                          {{I,V1},{I+1,S1}}
                                   end, {1.0,St0}, L),
    {T,St2} = luerl_eval:alloc_table(Es, St1),
    {T,St2};                            %No more to do for now
encode(_, _) -> error(badarg).          %Can't encode anything else

Presenter Notes

Fun -> Function

encode(L, St0) when is_list(L) ->
    {Es,{_,St1}} = lists:mapfoldl(fun what_is_this/1, {1.0,St0}, L),
    ...

what_is_this({K0,V0}, {I,S0}) ->
    {K1,S1} = encode(K0, S0),
    {V1,S2} = encode(V0, S1),
    {{K1,V1},{I,S2}};
what_is_this(V0, {I,S0}) ->
    {V1,S1} = encode(V0, S0),
    {{I,V1},{I+1,S1}}.

Presenter Notes

The Amazing Nested do Function!

From OTP mod_auth.erl
do(Info) ->
    ?hdrt("do", [{info, Info}]),
    case proplists:get_value(status,Info#mod.data) of
        %% A status code has been generated!
        {_StatusCode, _PhraseArgs, _Reason} ->
            {proceed, Info#mod.data};
        %% No status code has been generated!
        undefined ->
            case proplists:get_value(response, Info#mod.data) of
                %% No response has been generated!
                undefined ->
                    Path = mod_alias:path(Info#mod.data,Info#mod.config_db,
                                          Info#mod.request_uri),
                    %% Is it a secret area?
                    case secretp(Path,Info#mod.config_db) of
                        {yes, {Directory, DirectoryData}} ->
                            ?hdrt("secret area",
                                  [{directory, Directory},
                                   {directory_data, DirectoryData}]),

                            %% Authenticate (allow)
                            case allow((Info#mod.init_data)#init_data.peername,
                                       Info#mod.socket_type,Info#mod.socket,
                                       DirectoryData) of
                                allowed ->
                                    ?hdrt("allowed", []),
                                    case deny((Info#mod.init_data)#init_data.peername,
                                              Info#mod.socket_type,
                                              Info#mod.socket,
                                              DirectoryData) of
                                        ...

Presenter Notes

Embedded Case Expression

start_port() ->
    ...
    {Pid, Mon} = case whereis(?PORT_CREATOR_NAME) of
                     undefined ->
                         spawn_monitor(fun() ->
                                               start_port_srv(Request)
                                       end);
                     P ->
                         P ! Request,
                         M = erlang:monitor(process, P),
                         {P, M}
                 end,
     ...

Presenter Notes

Not Embedded Case Expression

start_port() ->
    ...
    {Pid, Mon} = port_creator(),
    ...

port_creator() ->
    case whereis(?PORT_CREATOR_NAME) of
        undefined -> spawn_monitor();
        P ->
            P ! Request,
            M = erlang:monitor(process, P),
            {P, M}
    end.

spawn_monitor() ->
    spawn_monitor(fun() -> start_port_srv(Request) end).

Presenter Notes

Finally, Done!

Presenter Notes

Contact

CloudBees: http://www.cloudbees.com
Twitter: @gar1t
Blog: http://gar1t.com
github: http://github.com/gar1t

Presenter Notes