Code Review 4: Bowling Kata, Erlang edition
Yeah, yeah, I bet you thought that I had slipped into my familiar “I don’t have enough time for this” mumbo bugspit of the hiatus. “Oh, it’s been three weeks since the last time he posted any code, he must’ve stopped.”
False.
Simply put, I had to weather our organization’s usual two-pronged quarterly software release, which involved a fifteen-hour day and two 3 A.M. wake-up calls. On with the code.
I’m not sure where to start. I was going to try to order this code review by the changes I made, but there have been so many, so I’ll just do this by module.
-module(frame).
-export([roll/4, is_full/1, new/0, rescore/4, value_of/1]).
-include("frame.hrl").
new() -> {frame, #frame{}}.
roll(on_frame, #frame{first_roll=First} = Frame, that_knocked_down, Pins_Knocked_Down) when First =:= undefined ->
{frame, Frame#frame{first_roll = Pins_Knocked_Down, value = Pins_Knocked_Down}};
roll(on_frame, #frame{first_roll=First, second_roll=Second}, that_knocked_down, Pins_Knocked_Down) when (First =:= 10) and (Second =:= undefined) ->
{frame, #frame{first_roll = Pins_Knocked_Down, value = Pins_Knocked_Down}};
roll(on_frame, #frame{second_roll=Second, value=V} = Frame, that_knocked_down, Pins_Knocked_Down) when Second =:= undefined ->
{frame, Frame#frame{second_roll = Pins_Knocked_Down, value = V+Pins_Knocked_Down}};
roll(_, _, _, _) ->
{error, frame_already_has_two_rolls}.
is_spare(#frame{second_roll=Second}) when (Second =:= undefined) ->
false; %is_strike(Frame);
is_spare(#frame{first_roll=First}) when (First =:= undefined) ->
false;
is_spare(#frame{first_roll=First, second_roll=Second}) ->
(First + Second) =:= 10.
is_strike(#frame{first_roll=First, second_roll=Second}) ->
(First =:= 10) and (Second =:= undefined).
is_full(#frame{first_roll=First, second_roll=Second}) when ((First =:= 10) and (Second =:= undefined)) ->
true;
is_full(#frame{first_roll=First, second_roll=Second}) when ((First + Second) =:= 10) or ((First =/= undefined) and (Second =/= undefined)) ->
true;
is_full(F) when is_record(F, frame) ->
false.
... snip ...
rescore(frame, #frame{score_has_been_updated_due_to_spare = HasBeenUpdatedDueToSpare,
strike_update_count = StrikeUpdateCount} = Frame, for_a_roll_that_knocked_down, PinsKnockedDown) ->
IsSpare = is_spare(Frame),
IsStrike = is_strike(Frame),
if
IsSpare ->
rescore_spare(HasBeenUpdatedDueToSpare, Frame, PinsKnockedDown);
IsStrike ->
rescore_strike(StrikeUpdateCount, Frame, PinsKnockedDown);
true ->
{frame, Frame}
end.
rescore_spare(HasBeenUpdatedDueToSpare, Frame, PinsKnockedDown) ->
case (HasBeenUpdatedDueToSpare =:= false) of
true ->
{frame, spare(from, Frame, roll_knocked_down, PinsKnockedDown)};
false ->
{frame, Frame}
end.
rescore_strike(StrikeUpdateCount, Frame, PinsKnockedDown) ->
case (StrikeUpdateCount =< 1) of
true ->
{frame, strike(from, Frame, roll_knocked_down, PinsKnockedDown)};
false ->
{frame, Frame}
end.
value_of(#frame{value = V}) -> {value, V}.
A lot of things here:
- I introduced tagged return values en masse, but only to exported functions.
- I introduced named parameters, with which I experimented at the last review, to all exported functions.
- Acting on my pledge to read some Erlang code to try to get a feel for the idioms, I removed my use of try-catch-throw for rolling on frames with more than two rolls and replaced it with a tagged error return value.
- Notice the use of the undefined atom. That wasn’t there before. I had to use it in order to implement support for gutter balls. So now is as good a time as any to show you the latest frame record definition:
-record(frame, { first_roll, second_roll, value = 0, score_has_been_updated_due_to_spare = false, strike_update_count = 0 }). - Look at rescore/4. I wanted to refactor this function using guard expressions, but I kind of already knew that it wouldn’t work. I found a good explanation as to why it sucks so much.
- The clause of roll/4 that returns and error seems strange. It says that if any of the preceding clauses are not matched, then you have a problem. Should it be this way? Should I have a more specific clause for this condition, especially now that I have named parameters?
OK, on to the game module.
-module(game).
-export([ %factory functions
roller/10,
creator/10,
scorer/1,
%functions
new/0]).
-include("frame.hrl").
-include("game.hrl").
new() -> {game, #game{}}.
scorer(ValueOf) ->
{scorer, fun(#game{frames=Frames}) when (is_list(Frames)) and (Frames =/= []) ->
L = lists:map(ValueOf, Frames),
{score, lists:sum([V || {value, V} <- L])};
(#game{frames=Frames}) when (is_list(Frames)) and (Frames =:= []) ->
{score, 0}
end}.
roller(
frame_roll_function, Roll,
frame_is_full_function, IsFull,
frame_rescore_function, Rescore,
frame_constructor_function, NewFrame,
logger_function, Log) when
is_function(Roll) and
is_function(IsFull) and
is_function(Rescore) and
is_function(NewFrame) and
is_function(Log) ->
{roller, fun(on_game, #game{frames=Frames} = G, knocked_down, PinsKnockedDown) when Frames =:= undefined ->
{game, roll_with_no_frames(G, PinsKnockedDown, Roll, NewFrame, Log)};
(on_game, #game{frames=Frames} = G, knocked_down, PinsKnockedDown) when Frames =/= undefined ->
{game, roll_with_frames(G, PinsKnockedDown, Roll, IsFull, Rescore, NewFrame, Log)}
end}.
creator(
frame_roll_function, Roll,
frame_is_full_function, IsFull,
frame_rescore_function, Rescore,
frame_constructor_function, NewFrame,
logger_function, Log) when
is_function(Roll) and
is_function(IsFull) and
is_function(Rescore) and
is_function(NewFrame) and
is_function(Log) ->
{creator, fun(rolls_in_game, Frames) ->
{game, NewGame} = new(),
{game, aggregate_roller_loop(Frames, NewGame, Roll, IsFull, Rescore, NewFrame, Log)}
end}.
aggregate_roller_loop(Frames, Game, _, _, _, _, _) when Frames =:= [] -> Game;
aggregate_roller_loop(Frames, Game, Roll, IsFull, Rescore, NewFrame, Log) ->
{roller, RollGame} = roller(frame_roll_function, Roll,
frame_is_full_function, IsFull,
frame_rescore_function, Rescore,
frame_constructor_function, NewFrame,
logger_function, Log),
[FirstFrame|RestOfFrames] = Frames,
{game, RolledGame} = RollGame(on_game, Game, knocked_down, FirstFrame),
aggregate_roller_loop(RestOfFrames, RolledGame, Roll, IsFull, Rescore, NewFrame, Log).
roll_with_no_frames(#game{} = G, PinsKnockedDown, Roll, NewFrame, Log) ->
Log(trace, "trace 13", with_code, 1),
{frame, NewlyCreatedFrame} = NewFrame(),
{frame, Frame} = Roll(on_frame, NewlyCreatedFrame, that_knocked_down, PinsKnockedDown), % todo mock this
G#game{frames = [Frame]}.
roll_with_frames(#game{frames=Frames} = G, PinsKnockedDown, Roll, IsFull, Rescore, NewFrame, Log) ->
Log(trace, "trace 9", with_code, 1),
SizeOfFrames = size(list_to_tuple(Frames)),
ReversedFrames = lists:reverse(Frames),
[MostRecentFrame|_] = ReversedFrames,
FirstFrameIsFull = IsFull(MostRecentFrame),
if
((SizeOfFrames =:= 1) and (not FirstFrameIsFull)) ->
roll_with_one_non_full_frame(G, PinsKnockedDown, Roll);
((SizeOfFrames =:= 1) and (FirstFrameIsFull)) ->
roll_with_one_full_frame(G, PinsKnockedDown, Roll, Rescore, NewFrame);
true ->
roll_with_many_frames(G, PinsKnockedDown, Roll, IsFull, Rescore, NewFrame)
end.
... snip ...
roll_with_many_frames(#game{frames=Frames} = G, PinsKnockedDown, Roll, IsFull, Rescore, NewFrame) ->
ReversedFrames = lists:reverse(Frames),
[MostRecentFrame,PreviousFrame|RestOfFrames] = ReversedFrames,
{Type, X} = Roll(on_frame, MostRecentFrame, that_knocked_down, PinsKnockedDown),
{frame, FrameThatResultsFromRoll} = case Type of
error ->
{frame, Frame} = NewFrame(),
Roll(on_frame, Frame, that_knocked_down, PinsKnockedDown); % todo mock this
frame ->
{frame, X}
end,
NewPreviousFrame = determine_previous_frame(MostRecentFrame, IsFull, PreviousFrame),
{frame, SecondToLastFrame} = Rescore(frame, NewPreviousFrame, for_a_roll_that_knocked_down, PinsKnockedDown),
case IsFull(MostRecentFrame) of
true ->
{frame, ThirdToLastFrame} = Rescore(frame, PreviousFrame, for_a_roll_that_knocked_down, PinsKnockedDown),
G#game{frames = lists:reverse([FrameThatResultsFromRoll,SecondToLastFrame,ThirdToLastFrame|RestOfFrames])};
false ->
G#game{frames = lists:reverse([FrameThatResultsFromRoll,SecondToLastFrame|RestOfFrames])}
end.
... snip ...
- I commented on the -export statement, noting that I essentially have two types of functions: “factory” functions that create other functions, and regular plain old functions.
Notice that various functions have been renamed. This was to address the complaints I had at the last review about the awkward interface. Factory functions are actually nouns, like “creator” or “scorer”, and they produce functions. Now, I found that the most readable way to deal with these is to store the fruits of the factory functions into variables that themselves are verbs:
{scorer, ScoreOf} = game:scorer(fun frame:value_of/1), TheScore = ScoreOf(MyGame)It kind of flips everything on its head, and this makes total sense when you’re in a functional language. Steve Yegge, liberal moron he is, was correct in his now-classic rant on execution in the kingdom of nouns. Erlang is a kingdom of verbs, which is the flipside of something like Java. It makes sense that certain naming conventions make more sense when they too are flipped.
- I used my first list comprehension in order to keep the scoring algorithm working when I introduced tagged return values.
- You are correct sir: there is a new dependency, a home-brewed logger! I wrote it both in preparation for the parallelism I will inevitably introduce and for my current debugging efforts and making sure that coverage is up to par, though I scanned through the Armstrong book and I did see that Erlang might have something like that built in.
Here be the logger:
-module(logger). -export([ %factory function new/1]). new(Filename) -> {logger, fun(error, Message, with_code, Code) -> {ok, Handle} = file:open(Filename, [append]), write_log(error, Message, Code, na, na, Handle), file:close(Handle); (info, Message, with_code, Code) -> {ok, Handle} = file:open(Filename, [append]), write_log(info, Message, Code, na, na, Handle), file:close(Handle); (trace, Message, with_code, Code) -> {ok, Handle} = file:open(Filename, [append]), write_log(trace, Message, Code, na, na, Handle), file:close(Handle) end}. write_log(Type, Message, Code, ProcessID, ProcessName, Handle) -> {Hour, Minute, Second} = erlang:time(), {Year, Month, Day} = erlang:date(), io:format(Handle, "time=~p-~s-~s ~s:~s:~s|type=~p|code=~p|process_id=~p|process_name=~p|message=~s~n", [Year, formatted_to_two_digits(Month), formatted_to_two_digits(Day), formatted_to_two_digits(Hour), formatted_to_two_digits(Minute), formatted_to_two_digits(Second), Type, Code, ProcessID, ProcessName, Message]). formatted_to_two_digits(Number) -> case (Number < 10) of true -> string:concat("0", erlang:integer_to_list(Number)); false -> erlang:integer_to_list(Number) end.And it produces messages like this:
time=2010-08-23 21:55:33|type=trace|code=1|process_id=na|process_name=na|message=trace 9
- In creator/10, I broke my own rule about only using named parameters in functions with more than one parameter. I think this is probably an isolated incident. In all other instances of this, I push the atom into the name of the variable holding the function.
- Another notable change is roll_with_many_frames/6 and how it now handles the error of a frame already having two rolls. My mind was opened a little when I got the idea to inspect the tag value to determine what situation I was in. Interesting. I think I am getting fairly comfortable with Erlang’s unique pattern-matching mechanism, even though I screwed up the variable name capitalization rule once.
For a little while, I had dreams of a constructor interface like this for both games and frames:
frame:newWith([{first_roll, 4}, {second_roll, 6}, {value, 10}, {score_has_been_updated_due_to_spare}])
It didn’t seem worth the trouble, though. The only benefit would be to completely encapsulate the fact that records are being used, even from test cases.
Finally, lest we not forget: I moved rescore/4 to the frame module, and this ended up reducing the number of unit tests for both modules. Funny what a little cohesion will do for you.
Here is a cool tutorial written for C, C++, and Java people: tutorial. And should I be using this little project as an opportunity to learn some other version control system, like maybe github?
I think the next steps here are to possibly fool around with a UI front-end, though I never bothered to do that in Smalltalk, to finish adding logging to this little application, and then, finally, add parallelism in some way… I’m thinking of writing a server on top of the game module and clients that request the scores of either an entire game or a game in progress.
An alternative would be to somehow separate the game and frame modules into processes and have them talk to each other across process boundaries. I have no idea why one would do that other than for the academic thrill. And you know how I feel about academic thrills.
Wow. Suddenly, this choice is very easy. I’ll probably write the server on top of the game module.
 |
Announcer: You’re reading the EIP web-ring. |
