4
\$\begingroup\$

I'm very new to Erlang and I'm specifically interested in making my Erlang idiomatic (in other words, can you tell I don't know Erlang from this code?) and removing code duplication. I am not as interested in performance or the UI of the "game."

This is a simple interactive Rock, Paper, Scissors game and I've been using erlc rps.erl && ./rps-main.erl to run it.

rps.erl

-module(rps).
-export([play_game/1, get_random_symbol/0]).

get_symbol(1) -> rock;
get_symbol(2) -> paper;
get_symbol(3) -> scissors.

get_random_symbol() ->
    get_symbol(rand:uniform(3)).

play_game(InputFunctions) ->
    Score = [{win, 0}, {loss, 0}, {tie, 0}],
    play_game(Score, InputFunctions).

play_game(ok, _) ->
    ok;
play_game(Score, {Player1Input, Player2Input}) ->
    display_score(Score),
    Input1 = Player1Input(),
    Continue = Input1 =/= quit,
    Input2 = Player2Input(Continue),
    NewScore = compare_results(Input1, Input2, Score),
    play_game(NewScore, {Player1Input, Player2Input}).

compare_results(quit, _, _) ->
    ok;
compare_results(_, quit, _) ->
    ok;
compare_results(Same, Same, [Wins, Losses, {tie, Ties}]) ->
    [Wins, Losses, {tie, Ties+1}];
compare_results(rock, paper, [Wins, {loss, Losses}, Ties]) ->
    [Wins, {loss, Losses+1}, Ties];
compare_results(paper, scissors, [Wins, {loss, Losses}, Ties]) ->
    [Wins, {loss, Losses+1}, Ties];
compare_results(scissors, rock, [Wins, {loss, Losses}, Ties]) ->
    [Wins, {loss, Losses+1}, Ties];
compare_results(rock, scissors, [{win, Wins}, Losses, Ties]) ->
    [{win, Wins+1}, Losses, Ties];
compare_results(scissors, paper, [{win, Wins}, Losses, Ties]) ->
    [{win, Wins+1}, Losses, Ties];
compare_results(paper, rock, [{win, Wins}, Losses, Ties]) ->
    [{win, Wins+1}, Losses, Ties].

display_score([{win, Wins}, {loss, Losses}, {tie, Ties}]) ->
    io:format("Player1 Wins: ~B~nPlayer2 Wins: ~B~nTies: ~B~n", [Wins, Losses, Ties]).

rpsgame.erl

#!/usr/bin/env escript
%% -*- erlang -*-
%%! -pa 'ebin/'
-import(rps, [play_game/1, get_random_symbol/0]).

main(_) -> 
    GetPlayerInput = fun() ->
        io:format("Enter rock, paper, scissors, or quit~n"),
        case io:get_line("> ") of
            "rock\n" -> rock;
            "paper\n" -> paper;
            "scissors\n" -> scissors;
            _ -> quit
        end
    end,
    GetCompInput = fun(Continue) when Continue =:= false ->
                        ok;
                    (_) ->
                        Input = rps:get_random_symbol(),
                        io:format("Computer threw ~p~n", [Input]),
                        Input
                    end,
    play_game({GetPlayerInput, GetCompInput}).
\$\endgroup\$
2
\$\begingroup\$

In whole not bad, but something can be simplified.

1. In short functions don't need to create separate variables, especially if it is just a wrapper over the call of inner function. So play game/1 from rps can be rewritten as:

play_game(InputFunctions) ->
  play_game([{win, 0}, {loss, 0}, {tie, 0}], InputFunctions).

2. Behavior of function for get result from player2 must not depend on result first player. Together with hint from 1 and giving an alias to the name of argument get

play_game(Score, {Player1Input, Player2Input} = P) ->
  display_score(Score),
  case Player1Input() of
    quit -> ok;
    Input -> play_game(new_score(Input, Player2Input(), Score), P)
  end.

where Player2Input/0:

  GetCompInput = fun() ->
    Input = rps:get_random_symbol(),
    io:format("Computer threw ~p~n", [Input]),
    Input
                 end,

new_score in my opinion, better captures the essence of the function than compare_results. This also allow delete first clause in new_score(compare_results)/3.

3. Usage the symbol - in the name of the module is not very convenient, because you must wrap the name in ' ' when call function. Better use _ as delimiter.

4. Not should use guard,when can use pattern matching. Compare

fun(Continue) when Continue =:= false ->

and

fun(false) ->

5. Do not create an anonymous argument for a single function clause, in this case it is just an excess parameter.

 main(_) ->

6. Do not use the directive import since it can will only confuse. If you know C++, it is possible to draw an analogy with an indicate explicit namespace instead of using namespace.

In addition I did not delete lines

play_game(ok, _) ->
  ok;

and

new_score(_, quit, _) ->
  ok;

although when playing with the computer this function clause does not get called.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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