8/27/03

Critique of Bowling Simulator

(assignment∞)

main.cpp
The variable names s and f are terribly nondescript. The outer loop should be rewritten with iterators and possibly for_each, especially considering it depends on the vector's size. The inner loop (centered around j) should be spun into a function, probably a method of Game, or better yet a couple of fields simply tallied during scoring. The printing is good enough, but there's no reason it should be separated from the Game::Print; furthermore, it should print to an ostream specified via parameter or class field. To be very nit-picky, it's clearer to avoid postincrements when not strictly necessary (this goes for the whole program). In sum, the only part of Stats() I'd keep would be the high-score tally. A pre/post would have been nice when it was in charge of 3-4 rather random tasks, but if refactored as described it should be self-documenting.

PromptPlayer is ok design-wise, but should eliminate the "magic number" 15 and not use the "magic iostreams" cin/cout. I'm debating whether PlayMatch should use for_each -- the actual algorithm begs it, but writing a 1-line functor would probably just obfuscate things. Main is simple enough, but should probably have a "do you want to continue?" loop, as well as handle assigning the iostreams previously discussed. I also feel that calling Games "players" is confusing.

game.cpp / .h
I don't see the point of the default constructor; other classes don't have them. I like how this class includes Bowler and Frame by composition (vs., say, inheritance). However, as designed I think the class names are pretty confusing. I'm glad the first comment in TakeTurn is there, since the if block isn't exactly self-documenting. As discussed, Print should also print names, marks, and strikes. The functions encapsulate things well, but it's hard to follow the control flow.

bowler.cpp / .h
Having TakeTurn() do printing is poor coupling.


1. The comments were mostly helpful, but there were a few places where they were sorely missing. The ones in the .h files comprised most of the non-helpful ones -- they seemed like they were there because someone said "you must write one line about every method."

2. PlayMatch() is not long. Stats() has been discussed. Game::TakeTurn() should use a utility function to perform int shot1 = myBowler.Roll(pinsStanding); / pinsStanding -= shot1; / AddPinCount(shot1); each time.

3. Already commented on the obvious need for such functions in place of the logic being done after the fact in Stats().

4. The comments in Bowler::Roll() are sufficient to determine the purpose of skill_level, but to see its range you have to know a bit about Dice. I myself don't know much about it, but I infer that a skill_level can be from 0 (random rolls between 1-10) to virtually as high as you want, which would make numbers above ten (interpreted as strikes) very likely.

5. The program appears to work. Test cases should be easy to construct by feeding text files to stdin.


Back to SoftwareDesign