Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm trying to think of ways to separate things out. I'm open to ideas, or if you see anything blatantly wrong, I'd like to know that too.

Generally, I'm happy with this, but the sheer size of the class seems like a code smell.

Original Source

'''The Game class.'''
import game_exceptions
from bases import Bases
from count import Count
from inning import Inning
from team import Team

# pylint: disable=R0902
# pylint complains about the number of instance attributes.
class Game(object):
    '''Maintain the state of the game.

    Includes methods to carry out various game
    related events.

    '''
    def __init__(self, home, away, league="league.db", innings_per_game=9):
        '''Set up a new game.

        home and away are the team_id pointing to a team in the league.

        The league variable is used to play games in different leagues.

        innings_per_game is the number of innings in a standard
        game, it can be changed if you want to represent a
        little league game, or some type of exhibition game.

        '''
        self.count = Count()
        self.home = Team(league=league, team_id=home)
        self.away = Team(league=league, team_id=away)
        self.inning = Inning(innings_per_game)
        self.bases = Bases()
        self.winner = None
        self.game_over = False
        self.bat_team = self.away
        self.pitch_team = self.home
        self.pitch_team_stats = self.pitch_team.stats.pitching
        self.current_pitcher = self.pitch_team.pitcher().stats.pitching
        self.bat_team_stats = self.bat_team.stats.batting
        self.current_batter = self.bat_team.current_batter().stats.batting

    def __str__(self):
        '''Output the important game information as a string.'''
        if self.game_over:
            return self.__str_finished_game()
        else:
            return self.__str_game_in_progress()

    def __str_finished_game(self):
        '''Output a finished game as a string.'''
        output = 'The game is over.\n'
        output += 'The %s team won.\n' % (self.winner)
        output += 'The final score was:\n'
        output += '%s to %s' % (self.home.score, self.away.score)
        output += '\n'
        if self.inning.number != self.inning.innings_per_game:
            output += 'The game took %s innings.\n' % (self.inning.number)
        return output

    def __str_game_in_progress(self):
        '''Output a game in progress as a string.'''
        if self.inning.top:
            half = "top"
        else:
            half = "bottom"
        output = "It's the %s of %s\n" % (half, self.inning.number)
        output += 'The score is:\n'
        output += '%s to %s' % (self.home.score, self.away.score)
        output += '\n'
        output += "%s is at bat." % (self.bat_team.current_batter())
        output += '\nThe count is:\n'
        output += str(self.count)
        output += '\nWith %s outs.\n' % (self.inning.outs)
        output += str(self.bases) + '\n'
        return output

    def strike(self):
        '''Pitcher throws a strike.

        If there are 2 strikes, call the __strike_out method.
        Otherwise, just increment the strike count.

        '''
        if self.count.strikes == 2:
            self.__strike_out()
        else:
            self.count.strike()

    def ball(self):
        '''Pitcher throws a ball.

        If there are 3 balls, call the __walk method, otherwise
        just increment the ball count.

        '''
        if self.count.balls == 3:
            self.__walk()
        else:
            self.count.ball()

    def foul_ball(self):
        '''Batter hits a foul.

        If there are less than 2 strikes, increment the strike count.

        '''
        if self.count.strikes != 2:
            self.count.strike()

    def hit_by_pitch(self):
        '''Batter is hit by pitch.

        Call the __stats_hbp method to update the stats.  Then call
        the __walk method with hbp set to True, so it knows not to
        also call the __stats_walk method.  We don't want to track it
        as a walk, we just want to perform the same game functions.

        '''
        self.__stats_hbp()
        self.__walk(hbp=True)

    def steal(self, base):
        '''Runner steals a base.

        base - the base that is stolen (second, third, home)

        If the base to be stolen is not empty, or the base 
        they are stealing from is not occupied, an error is
        assumed to have occured, and an exception will raise.

        '''
        if base == "home":
            if self.bases[2] != 0:
                self.__score_run(self.bases[2])
                self.__stats_steal(self.bases[2])
                self.bases.clear(2) 
                return
        base_num = self.__base_number(base)
        if self.bases[base_num - 1] != 0 and self.bases[base_num] == 0:
            self.__stats_steal(self.bases[base_num - 1])
            self.bases.advance(base_num - 1)
            return
        raise game_exceptions.InvalidSteal()

    def caught_stealing(self, base):
        '''Runner caught stealing a base.

        base - the base that is attempted to be stolen 
        (second, third, home)

        If the base to be stolen is not empty, or the base 
        they are stealing from is not occupied, an error is
        assumed to have occured, and an exception will raise.

        '''
        b_num = self.__base_number(base)
        if b_num == "home":
            if self.bases[2] != 0:
                self.__stats_caught_stealing(self.bases[2])
                self.bases.clear([2])
                self.__put_out()
            else:
                raise game_exceptions.InvalidSteal()
        elif b_num in [0, 1, 2]:
            if self.bases[b_num - 1] != 0 and self.bases[b_num] == 0:
                self.__stats_caught_stealing(self.bases[b_num - 1])
                self.bases.clear([b_num - 1])
                self.__put_out()
            else:
                raise game_exceptions.InvalidSteal()
        else:
            raise game_exceptions.InvalidSteal()

    def picked_off(self, base):
        '''Runner picked off from a base.

        base - the base that the runner was on 
        (first, second, third) or (0, 1, 2)

        If base comes as a string, the __base_number function will
        translate it to an int.

        If the base is empty, an error is assumed to have occured, 
        and an exception will raise.

        '''
        base_number = self.__base_number(base)
        if self.bases[base_number] != 0:
            self.bases.clear([0])
            self.__stats_pick_off()
            self.__put_out()
        else:
            raise game_exceptions.InvalidPickOff()

    def ground_out(self):
        '''Batter grounds out.'''
        self.__stats_atbat()
        self.__next_batter()
        self.__put_out()

    def fly_out(self):
        '''Batter flies out.'''
        self.__stats_atbat()
        self.__next_batter()
        self.__put_out()

    def balk(self):
        '''Pitcher performs a balk.

        If no baserunners are on, a balk has no effect.  If runners
        are on base, they will all be advanced one base, and possibly
        scored, if there is a runner on third.

        '''
        if not self.bases.men_on():
            return
        scorer = self.bases.advance_all()
        if scorer:
            self.__score_run(scorer)

    def hit(self, num=1, advances="default"):
        '''Batter gets a base hit.

        1 = single
        2 = double
        3 = triple
        4 = home run

        By default, the hit is a single, and all runners advance
        one base.

        The advances are a list of tuples, there should be one
        tuple for each runner who advances on the play.  It
        should specify the from and to bases that the runner
        moved from and to.

        0 = first
        1 = second
        2 = third
        3+ = home

        In the case where the baserunners didn't advance, advances
        should be passed in as None.

        '''
        scorers = []
        if advances == "default":
            advances = []
            for abase in range(3):
                if not self.bases.empty_base(abase):
                    advances.append((abase, abase + num))
        if advances:
            if self.bases.men_on():
                scorers = self.bases.advances(advances)
        for scorer in scorers:
            self.__stats_rbi()
            self.__score_run(scorer)
        if num == 4:
            self.__stats_rbi()
            self.__stats_home_run()
            self.__score_run(self.bat_team.batter)
        else:
            if num == 2:
                self.__stats_double()
            elif num == 3:
                self.__stats_triple()
            self.bases[num - 1] = self.bat_team.batter # put the batter on base.
        self.__stats_atbat()
        self.__stats_hit()
        self.__next_batter()

    def double_play(self, base1="second", base2="first", gdp=True):
        '''Batter hits into a double play.

        base1 - base where first out is made, assume force out.
        base2 - base where second out is made.
        gdp - True if it was a grounder into a double.

        '''
        if self.bases.men_on() == 0:
            raise game_exceptions.NotEnoughRunnersOnBase()
        if self.inning.outs == 2:
            raise game_exceptions.TooManyOuts()
        self.__stats_atbat()
        if gdp:
            self.__stats_gdp()
        self.__next_batter()
        self.bases[base1] = 0
        self.__put_out()
        self.bases[base2] = 0
        self.__put_out()

    def triple_play(self, gtp=True):
        '''Batter hits into a triple play.

        gtp should be set to True if it was a grounder into a triple
        play.

        '''
        if self.bases.men_on() < 2:
            raise game_exceptions.NotEnoughRunnersOnBase()
        if self.inning.outs != 0:
            raise game_exceptions.TooManyOuts()
        self.__stats_atbat()
        if gtp:
            self.__stats_gtp()
        self.pitch_team_stats.out(3)
        self.current_pitcher.out(3)
        self.__next_batter()
        self.__side_retired()

    def fielders_choice(self, base="second"):
        '''Batter hits into a fielder's choice.

        A fielder's choice is when a runner is on base, the
        batter hits the ball such that the runner is put out,
        but the batter is then able to get on base.

        base - base where the out is made, assume force out.
        It defaults to second, as that is the most common fielder's
        choice.

        '''
        self.__stats_atbat()
        self.__next_batter()
        self.bases[base] = 0
        self.__put_out()

    def error(self, batter_on_base=False, advances=None):
        '''Fielder makes an error.  By default nothing happens
        other than to take note of the error.

        batter_on_base determines if the batter got on base during
        this error.  If True, the batter will occupy first base.

        If the batter were to get multiple bases, the batter_on_base can
        be set to 1 for "second", 2 for "third", and in some really weird
        circumstances, 3 for "home" for an error based home run.

        advances is a list of tuples to advance runners from and to.  

        '''
        scorers = []
        if advances:
            if self.bases.men_on():
                scorers = self.bases.advances(advances)
            else:
                raise game_exceptions.NoRunnerToAdvance()
        for scorer in scorers:
            self.__score_run(scorer)
        if batter_on_base:
            if batter_on_base == True:
                batter_base = 0
            elif isinstance(batter_on_base, str):
                batter_base = self.__base_number(batter_on_base)
            if not self.bases.empty_base(batter_base):
                raise game_exceptions.InvalidAdvance()
            self.bases[batter_base] = self.bat_team.batter
            self.__stats_atbat()
            self.__next_batter()

    def sacrifice(self, advances=None):
        '''Batter performs a sacrifice bunt or fly out.  By 
        default all baserunners advance one base.  It wouldn't
        make sense to have a sacrifice and have no one advance.

        advances is a list of tuples to advance runners from
        and to.

        '''
        if self.bases.men_on() == 0:
            raise game_exceptions.NotEnoughRunnersOnBase()
        if self.inning.outs == 2:
            raise game_exceptions.BadSacrifice()
        if not advances:
            scored = self.bases.advance_all()
            if scored:
                self.__stats_rbi()
                self.__score_run(scored)
        else:
            if isinstance(advances, tuple):
                advances = [advances]
            for adv in advances:
                if self.bases[adv[0]] == 0:
                    raise game_exceptions.NoRunnerToAdvance()
            scorers = self.bases.advances(advances)
            for scorer in scorers:
                self.__stats_rbi()
                self.__score_run(scorer)
        self.__stats_sacrifice()
        self.pitch_team_stats.out()
        self.current_pitcher.out()
        self.inning.out()
        self.__next_batter()

    def end_game(self):
        '''Game is over, winner is declared, unless tied.

        When would we ever have a tie?

        Apparently, this can happen even in MLB, and we
        may want to simulate other levels of play as well.

        '''
        if self.home.score > self.away.score:
            self.winner = "home"
            self.home.wins += 1
            self.away.losses += 1
        elif self.away.score > self.home.score:
            self.winner = "away"
            self.away.wins += 1
            self.home.losses += 1
        else:
            # Tie
            self.winner = None
            self.home.ties += 1
            self.away.ties += 1
        self.game_over = True
        self.home.save()
        self.away.save()
        return

    @staticmethod
    def __base_number(base):
        '''Returns the base number, which is an index to the bases list.'''
        if base == "first":
            return 0
        if base == "second":
            return 1
        if base == "third":
            return 2
        return base

    def __new_inning(self):
        '''Start a fresh inning.

        Clear the bases, reset the count, change sides.

        TODO: Append to the inning scores list.  So we can track runs by
        inning.

        '''
        self.bases.clear()
        self.count.reset_count()
        self.inning.reset_outs()
        if self.bat_team == self.away:
            self.bat_team = self.home
            self.pitch_team = self.away
        else:
            self.bat_team = self.away
            self.pitch_team = self.home
        self.bat_team_stats = self.bat_team.stats.batting
        self.current_batter = self.bat_team.current_batter().stats.batting
        self.pitch_team_stats = self.pitch_team.stats.pitching
        self.current_pitcher = self.pitch_team.pitcher().stats.pitching

    def __side_retired(self):
        '''Three outs, time to switch sides, or end the game.'''
        if self.inning.number >= self.inning.innings_per_game:
            if self.inning.top:
                if self.home.score > self.away.score:
                    self.end_game()
                    self.__new_inning()
                    return
            else: # top
                if self.home.score != self.away.score:
                    self.end_game()
                    self.__new_inning()
                    return
        self.inning.increment()   
        self.__new_inning()

    def __strike_out(self):
        '''Batter strikes out.'''
        self.__stats_atbat()
        self.__stats_strike_out()
        self.__next_batter()
        self.__put_out()

    def __put_out(self):
        '''Runner is put out.'''
        self.current_pitcher.out()
        self.pitch_team_stats.out()
        if self.inning.outs == 2:
            self.__side_retired()
        else:
            self.inning.out()

    def __next_batter(self):
        '''Next batter is up in the lineup.'''
        self.bat_team.next_batter()
        self.current_batter = \
            self.bat_team.current_batter().stats.batting
        self.count.reset_count()

    def __score_run(self, scorer):
        '''Score a run.

        TODO: We currently count all runs as earned.  Needs to be fixed.
        TODO: We should add to the inning scores for home or away.

        '''
        self.__stats_run(scorer)
        self.current_pitcher.earned_run()
        self.pitch_team_stats.earned_run()
        self.bat_team.score_run()
        if self.inning.number >= self.inning.innings_per_game:
            if not self.inning.top:
                if self.home.score > self.away.score:
                    self.end_game()

    def __walk(self, hbp=False):
        '''Walk the batter.

        If hbp is True, the batter was hit by pitch, so we don't
        call the __stats_walk method.

        '''
        if not hbp:
            self.__stats_walk()
        if self.bases[0] != 0:
            if self.bases[1] != 0:
                if self.bases[2] != 0:
                    self.__score_run(self.bases[2])
                    self.__stats_rbi()
                else:
                    self.bases[2] = self.bases[1]
            else:
                self.bases[1] = self.bases[0]
        else:
            self.bases[0] = self.bat_team.batter
        self.__next_batter()

    def __stats_plate_app(self):
        '''Batter gets a plate Appearance.'''
        self.bat_team_stats.plate_app()
        self.current_batter.plate_app()

    def __stats_hbp(self):
        '''Hit by pitch.'''
        self.bat_team_stats.hbp()
        self.current_batter.hbp()
        self.__stats_plate_app()
        self.current_pitcher.hit_batter()
        self.pitch_team_stats.hit_batter()

    def __stats_steal(self, runner):
        '''Steal.'''
        self.bat_team_stats.steal()
        self.bat_team.lineup[runner].stats.batting.steal()

    def __stats_caught_stealing(self, runner):
        '''Caught stealing.'''
        self.bat_team_stats.caught_stealing()
        self.bat_team.lineup[runner].stats.batting.caught_stealing()

    def __stats_atbat(self):
        '''Batter gets an at bat.'''
        self.bat_team_stats.atbat()
        self.current_batter.atbat()
        self.__stats_plate_app()
        self.current_pitcher.atbat()
        self.pitch_team_stats.atbat()

    def __stats_pick_off(self):
        '''Pitcher picks off a baserunner.'''
        self.current_pitcher.pick_off()
        self.pitch_team_stats.pick_off()

    def __stats_walk(self):
        '''Walk.'''
        self.bat_team_stats.walk()
        self.current_batter.walk()
        self.__stats_plate_app()
        self.current_pitcher.walk()
        self.pitch_team_stats.walk()

    def __stats_rbi(self):
        '''Run batted in.'''
        self.bat_team_stats.rbi()
        self.current_batter.rbi()

    def __stats_home_run(self):
        '''Home Run.'''
        self.bat_team_stats.home_run()
        self.current_batter.home_run()
        self.current_pitcher.home_run()
        self.pitch_team_stats.home_run()

    def __stats_double(self):
        '''Double.'''
        self.bat_team_stats.double()
        self.current_batter.double()

    def __stats_triple(self):
        '''Triple.'''
        self.bat_team_stats.triple()
        self.current_batter.triple()

    def __stats_hit(self):
        '''Hit.'''
        self.bat_team_stats.hit()
        self.current_batter.hit()
        self.current_pitcher.hit()
        self.pitch_team_stats.hit()

    def __stats_gdp(self):
        '''Ground Double Play.'''
        self.bat_team_stats.gdp()
        self.current_batter.gdp()

    def __stats_gtp(self):
        '''Ground Triple Play.'''
        self.bat_team_stats.gtp()
        self.current_batter.gtp()

    def __stats_sacrifice(self):
        '''Sacrifice.'''
        self.bat_team_stats.sacrifice()
        self.current_batter.sacrifice()
        self.__stats_plate_app()

    def __stats_strike_out(self):
        '''Strike Out.'''
        self.bat_team_stats.strike_out()
        self.current_batter.strike_out()
        self.current_pitcher.strike_out()
        self.pitch_team_stats.strike_out()

    def __stats_run(self, scorer):
        '''Run.'''
        self.bat_team.player(scorer).stats.batting.run()
        self.current_pitcher.run()
        self.pitch_team_stats.run()
        self.bat_team_stats.run()
share|improve this question
    
don't use names like __foo. it doesn't do anything useful, and it certainly doesn't keep anyone else from mucking around in your class; it's just annoying magic. –  Eevee Jan 15 '13 at 4:17
    
It's intended to differentiate methods from those that are meant to be called by a user/consumer of this class. –  jgritty Jan 15 '13 at 17:27
    
no, that's what _foo is for. __foo does name mangling, and at a glance looks like it's supposed to be __foo__ –  Eevee Jan 15 '13 at 19:48
    
Yes, you're right, I think they should be single underscores. Not sure how I misread the docs the first time. –  jgritty Jan 15 '13 at 20:39

2 Answers 2

up vote 3 down vote accepted

Don't pass in the name of your database file. You should pass in a database connection object. As it stands you reopen the database in many different places across your code. You should really only open the database once and pass the connection around. If you do this:

  1. Your code will be simplified
  2. Your code will be more efficient
  3. You'll be able to run your unittests on the :memory: database which will make them faster.

Extract a GameStats() object. All those __stats_* methods should be methods on that object. The fact that you have a bunch of methods with those prefixes is a big hint. A sizable chunk of the Game class seems to be concerned with those stats, and so its a good candidate for extraction.

Don't use __str__ for output. It's supposed to be a string representation of your object, not your user output. I'd actually pull all user output out of this (and your other classes). Its better if you have some objects purely dedicated to the logic of he game. Have completely separately classes to do the user output.

Much of logic in Game should really be on Inning or Count. For example, the strike method.

def strike(self):
    '''Pitcher throws a strike.

    If there are 2 strikes, call the __strike_out method.
    Otherwise, just increment the strike count.

    '''
    if self.count.strikes == 2:
        self.__strike_out()
    else:
        self.count.strike()

Much of the logic here is concerned with the number of strikes which is Count's domain. This function is thus calling out to be moved to Count. But you can't because you need to call __strike_out. You've got the relationship between Count and Game backwards. Count should have a reference to and call Game when there is a strike out.

share|improve this answer

Your __walk() method is a good place to start.

def __walk(self, hbp=False):
    if not hbp:
        self.__stats_walk()
    if self.bases[0] != 0:
        if self.bases[1] != 0:
            if self.bases[2] != 0:
                self.__score_run(self.bases[2])
                self.__stats_rbi()
            else:
                self.bases[2] = self.bases[1]
        else:
            self.bases[1] = self.bases[0]
    else:
        self.bases[0] = self.bat_team.batter
    self.__next_batter()

Some of the problems I see there are:

  • It has hard-coded logic for each of the four bases. A maxim of programming is that there are either none, one, two, or many. If there are four bases, then you should write your program as if there were a dozen bases.
  • I don't think you have sufficiently modelled the objects involved in the game.
  • It would be nice to separate the scorekeeping logic from the player movement logic.

What is the definition of walking? Let's distill it down to its essence, and make it our goal to write the code as such:

def walk(self):
    self.bases[0].runner.advance(cascade=True)

Then let's see what kind of object modelling it takes to make that happen! Here's my proof-of-concept…

class Base:
    @staticmethod
    def make_diamond(game):
        bases = [
            Base(game, "at bat"),
            Base(game, "first"),
            Base(game, "second"),
            Base(game, "third"),
            HomeBase(game, "home"),
        ]
        for i in range(4):
            bases[i].next = bases[i + 1]
            bases[i + 1].prev = bases[i]
        return bases

    def __init__(self, game, name):
        self.game = game
        self.name = name
        self.runner = None

    def vacate(self):
        if self.runner:
            self.game.fire_event('left', self.runner, self)
        self.runner = None

    def accept(self, runner, cascade):
        self.game.fire_event('reached', runner, self)
        if self.runner and cascade:
            r = self.runner
            self.vacate()
            self.next.accept(r, True)
            self.runner = runner
        elif self.runner:
            runner.out()
        else:
            self.runner = runner

    def has_runner(self, runner):
        return self.runner == runner

    def __str__(self):
        return self.name

######################################################################

class HomeBase(Base):
    def accept(self, runner, cascade):
        self.game.fire_event('scored', runner)

    def has_runner(self, runner):
        return False

######################################################################

class Runner:
    number = 0

    def __init__(self, game):
        self.game = game
        self.name = "Player %d" % Runner.number
        Runner.number += 1

    def advance(self, cascade, num_bases=1):
        for i in range(num_bases):
            old_base = self.game.base_with_runner(self)
            if old_base:
                new_base = old_base.next
                old_base.vacate()
                new_base.accept(self, cascade)

    def out(self):
        self.game.fire_event('out', self)

    def __str__(self):
        return "Runner %s" % self.name

######################################################################

class Play:
    def __init__(self, game, batter):
        self.game = game
        self.batter = batter

######################################################################

class Game:
    def __init__(self):
        self.bases = Base.make_diamond(self)
        self.current_play = None
        self.event_listeners = []

    def next_batter(self):
        batter = Runner(self) # lineup.next_batter()
        self.bases[0].accept(batter, False)
        self.fire_event('at bat', batter)
        self.current_play = Play(self, batter)

    def hit_by_pitch(self):
        self.fire_event('hit by pitch', self.current_play.batter)
        self.bases[0].runner.advance(cascade=True)

    def walk(self):
        self.fire_event('walked', self.current_play.batter)
        self.bases[0].runner.advance(cascade=True)

    def base_with_runner(self, runner):
        return next(base for base in self.bases if base.has_runner(runner))


    def add_event_listener(self, listener):
        self.event_listeners.append(listener)

    def fire_event(self, event, *info):
        for listener in self.event_listeners:
            listener.handle(event, *info)

######################################################################

class Statistician:
    def __init__(self, game):
        self.game = game
        game.add_event_listener(self)

        self.hit_by_pitch = 0
        self.runs = 0
        self.walks = 0

    def handle(self, event, *info):
        print ("Event: %s %s " % (info[0], event)) + ' '.join(map(str, info[1:]))
        if event == 'hit by pitch':
            self.hit_by_pitch += 1
        if event == 'scored':
            self.runs += 1
        if event == 'walked':
            self.walks += 1

    def __str__(self):
        return "%d runs, %d HBP, %d walks" % (self.runs, self.hit_by_pitch, self.walks)

######################################################################

def test():
    g = Game()
    s = Statistician(g)

    g.next_batter()
    g.walk()
    g.next_batter()
    g.walk()
    g.next_batter()
    g.walk()
    g.next_batter()
    g.hit_by_pitch()

    print s

test()
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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