[erlang-questions] Please correct my leeway on my first Erlang/OTP application

[Ladislav Lenart]

Hello.

Disclaimer: I am by no means an Erlang expert, but here you
go...

I haven't study your application from the architectural POV
(i.e. process structure and what is done where) so I have no
remarks to the overall semantics. I only looked at a couple
of your modules. My mostly stylish and syntactic notes:
* It is an OTP convention to split files to the following
   folders:
   * /src for *.erl and internal *.hrl,
   * /include for public *.hrl; i.e. those used in -include_lib(),
   * /priv for various data files and e.g. C sources and
   * /ebin for compiled beam code.
   I suspect that many people adhere to these (or very similar
   ones).
* It is also desirable to prefix all your modules to lower
   conflict ratio with modules in other apps. Use e.g. full
   'clustmea_' or shorter 'clea_' or some such. Usage of the
   name gen_uploader is a classy antipattern example. The
   name suggests that the module belongs to the basic OTP
   libs which is not the case.
* I am not sure if it is feasible, but you should at least
   consider adding API functions to clustmea module so the
   user does not have to know unnecessary internal details
   of your code (at least not upfront). This would also allowed
   you to refactor it (change the process structure) later.
   These API functions would be simple wrappers calling the
   right function(s) of the right module(s) with the right
   arguments. Everything else would be private to your app.
* Overall I like your style. It is idiomatic, consistent and
   thus predictable and reads nicely :-)
* However, I wouldn't use records for messages encapsulated
   behind a functional API. So instead of e.g.

       -record(stop, {reason})

   I would simply write

       {stop, Reason}

   But this is just nitpicking (though up until now, I haven't
   seen this style).
* I wouldn't use records such as this

       -record(state, {tasks})

   I would simply use the value of tasks as the entire state
   and refactor the module later, if/when the need arises.
   But again, this is just me nitpicking.
* You can pattern match in the function clause head. Instead
   of

       handle_cast(run, State) ->
         TaskConf = State#state.config,
         UploaderConf = get_uploader_config(TaskConf),
         IterationPolicy = get_policy(iteration, TaskConf, _DefaultPolicy = soft),
         Module = State#state.module,
         run(Module, IterationPolicy, UploaderConf),
         {noreply, State};

   I would write

       handle_cast(run, #state{config=TaskConf, module=Module} = State) ->
         UploaderConf = get_uploader_config(TaskConf),
         IterationPolicy = get_policy(iteration, TaskConf, _DefaultPolicy = soft),
         run(Module, IterationPolicy, UploaderConf),
         {noreply, State};
* I don't think the following adds any value

       handle_call(_Request, _From, State) ->
         Reply = ok,
         {reply, Reply, State}.

   so I would write code like this directly

       handle_call(_Request, _From, State) ->
         {reply, ok, State}.
* I would rename is_policy/1 to something like check_policy/1
   which I think better conveys the possibility of throwing
   an error:

       get_policy(Name, TaskConf, DefaultPolicy) ->
         check_policy(Name),
         case lists:keyfind(Name, 1, TaskConf) of
           false -> DefaultPolicy;
           {Name, Policy} -> Policy
         end.

       check_policy(iteration) -> ok.
* I have seen several occasions where you access a record
   as a tuple, e.g.

       {state, Tasks1} = State,

   vs.

       #state{tasks=Tasks} = State,

   I am sure this is just an omission but it may
   hurt you badly one day nonetheless ;-)


HTH,

Ladislav Lenart


On 21.3.2012 08:05, Aleksandr Vinokurov wrote:
>
>
> Hello all,
>
> I'm quite a newbee in Erlang/OTP and will be very appreciated if you can point any wrong style or misunderstanding of the OTP in my first Erlang/OTP application.
>
> I've put it here https://github.com/aleksandr-vin/clustmea and will be glad to here any of your feedback.
>
> With best wishes,
> Aleksandr Vinokurov
> @aleksandrvin
>
>
>
> _______________________________________________
> erlang-questions mailing list
> erlang-questions@REDACTED
> http://erlang.org/mailman/listinfo/erlang-questions