3
\$\begingroup\$

I'm learning Erlang. It's my first time with a non-imperative programming language.

I've written a code and I want some thoughts about it:

  • Is my logic easy to understand?
  • Is my code idiomatic?
  • Is my code well organized?
  • Is my code using Erlang features correctly?

My goal here is improve readability and functional-thinking. I'm not concerned about performance.

-module(sb).
-export([start/0, barberShop/3, clientGenerator/2, barber/2]).
-define(MAX_BARBERS, 1).
-define(MAX_SITS, 5).

start() ->
  BarberShopPID = spawn(sb, barberShop, [[], 0, yes]),
  spawn(sb, clientGenerator, [BarberShopPID, 10]).

clientGenerator(BarberShopPID, ClientsToGenerate) ->
  case ClientsToGenerate of
    0 ->
      BarberShopPID ! clientGeneratorIsDone;
    _ ->
      timer:sleep(random(1000, 6000)),
      Hair = random(100, 600),
      BarberShopPID ! {client, Hair},
      clientGenerator(BarberShopPID, ClientsToGenerate-1)
  end.

barberShop(Clients, Barbers, IsCGRunning) ->
  receive
    {client, Hair} ->
      case length(Clients) of
        ?MAX_SITS ->
          write("A client entered, but there are no available sits."),
          barberShop(Clients, Barbers, IsCGRunning);
        _ ->
          write("A client entered and found a available sit."),
          {NewC, NewB} = tryAttend([Hair | Clients], Barbers, self()),
          barberShop(NewC, NewB, IsCGRunning)
      end;
    barberIsDone ->
      {NewC, NewB} = tryAttend(Clients, Barbers-1, self()),
      case shouldContinue(NewC, NewB, IsCGRunning) of
        yes ->
          barberShop(NewC, NewB, IsCGRunning);
        no ->
          write("Bye from the glorious barber shop.")
      end;
    clientGeneratorIsDone ->
      case shouldContinue(Clients, Barbers, no) of
        yes ->
          {NewC, NewB} = tryAttend(Clients, Barbers, self()),
            barberShop(NewC, NewB, no);
        no ->
          write("Bye from the glorious barber shop.")
      end
  end.

barber(Hair, BarberShopPID) ->
  write("Barber started his job."),
  timer:sleep(Hair * 10),
  write("Barber has done his job."),
  BarberShopPID ! barberIsDone.

tryAttend(Clients, ?MAX_BARBERS, _) ->
  {Clients, ?MAX_BARBERS};
tryAttend([], Barbers, _) ->
  {[], Barbers};
tryAttend(Clients, Barbers, BarberShopPID) ->
  [First | Rest] = Clients,
  spawn(sb, barber, [First, BarberShopPID]),
  {Rest, Barbers+1}.

shouldContinue([], 0, no) ->
  no;
shouldContinue(_, _, _) ->
  yes.

random(Min, Max) ->
  random:uniform(Max-Min+1)+Min.

write(Message) ->
  io:format(Message ++ "~n", []).
\$\endgroup\$
2
  • \$\begingroup\$ I don't know this language at all and I think I can mostly figure out what is happening - so that's good. Good question +1 \$\endgroup\$ Commented Apr 18, 2016 at 15:57
  • \$\begingroup\$ No one to help me. :( \$\endgroup\$
    – Gabriel
    Commented Apr 19, 2016 at 17:57

1 Answer 1

2
\$\begingroup\$

Overall it looks good, I would just have some minor comments. In any case please have a look at the official Ericsson guidance.

  1. Use macro ?MODULE instead of repeating sb.

  2. In Erlang function names are usually_with_underscores rather than camelCasing. There are some exceptions, even in OTP applications, but in general underscores help to distinguish functions from variable names, which are usually CamelCased. The same holds for atom names.

  3. I would usually put a code like this: [First | Rest] = Clients directly to the function argument to make it shorter, e.g.: tryAttend([First | Rest] = Clients, Barbers, BarberShopPID) ->

  4. NewC and NewB aren't very descriptive. I would rather use NClients if NewClients are too long for you.

  5. When accessing the head and tail of a list the names of variables are usually H and T, unless you want a specific name for the head, in which case the tail is just T or Tail, e.g. [H|T] = Clients or [First|T] = Clients. It's a convention which you will notice when reading someone else's code.

  6. When spawning process with a function that doesn't need to be exported for its own sake try to use fun, e.g.: spawn(fun() -> barber(First, BarberShopPID) end). It's better to not expose functions that are not meant to be called from outside. And if you do have to export a function like that, then it's better to put it into a separate export with an additional comment, e.g. %% Internal API.

  7. Didn't you want to write MAX_SEATS rather than MAX_SITS? :)

\$\endgroup\$
1
  • \$\begingroup\$ Thanks! Very helpful! And yes, it's "seats" , my bad :) \$\endgroup\$
    – Gabriel
    Commented Apr 20, 2016 at 13:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.