Erlang: simple refactoring

Question

Consider the code:

f(command1, UserId) ->
    case is_registered(UserId) of
        true ->
            %% do command1
            ok;
        false ->
            not_registered
    end;

f(command2, UserId) ->
    case is_registered(UserId) of
        true ->
            %% do command2
            ok;
        false ->
            not_registered
    end.

is_registered(UserId) ->
    %% some checks

Now imagine that there are a lot of commands and they are all call is_registered at first. Is there any way to generalize this behavior (refactor this code)? I mean that it's not a good idea to place the same case in all the commands.

Answer 1

I like the use of exceptions better. It would allow programming for the successful case, and it decrease the use of indentation levels.

Answer 2

I think ctulahoops' code reads better refactored like so:

run_command(Command, UserId) ->
    case is_registered(UserId) of
         true  ->
             Command();
         false ->
             not_registered
    end.

run_command(some_function_you_define);
run_command(some_other_function_you_define);
run_command(fun() -> do_some_ad_hoc_thing_here end).

This takes advantage of the fact that functions are first-class entities in Erlang. You can pass them into functions, even define them anonymously inline in the call. Each function can be named differently. cthulahoops' method requires that all your command functions be predefined and called run_command(), disambiguated by their first argument.

Answer 3

f(Command, UserId) ->
     Registered = is_registered(UserID),   
     case {Command, Registered} of
          {_, False} ->
               not_registered;
          {command1, True} ->
               %% do command1
               ok;
          {command2, True} ->
               %% do command2
               ok
     end.

is_registered(UserId) ->
     %% some checks

Also Wrangler, an interactive refactoring tool, might be your friend.

Answer 4

I prefer this way, it gives more flexibility:

f(Command, UserId) ->
    f(Command,is_registered(UserId),UserID).

f(command1,true,_UserID) -> do_command1();
% if command2 does not need to be registered but uses UserID, for registration for example
f(command2,_Reg,UserID) -> do_command2(User_ID);
f(_,false,_UserID) -> {error, not_registered}.

Answer 5

I'd go with

f(Command, UserId) ->
    case is_registered(UserId) of
         true  ->
             run_command(Command);
         false ->
             not_registered
    end.


run_command(command1) -> ok;   % do command1
run_command(command2) -> ok.   % do command2