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}).
1 Answer 1
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.