Giter VIP home page Giter VIP logo

bombman-cs-362's People

Contributors

drummyfish avatar gammarhea avatar jemisonf avatar manncr avatar richjoregonstate avatar

Watchers

avatar

bombman-cs-362's Issues

Player Class Code Review

start

Line 259
This is a great example of a large class code smell, in order to instantiate a Player class, 32 different fields need to be accounted for. This could be refactored or extracted to a subclass in order to make the code more readable.

def __init__(self):
    super(Player,self).__init__()
    self.number = 0                       ##< player's number
    self.team_number = 0                  ##< team number, determines player's color
    self.state = Player.STATE_IDLE_DOWN
    self.state_time = 0                   ##< how much time (in ms) has been spent in current state
    self.speed = Player.INITIAL_SPEED     ##< speed in tiles per second
    self.bombs_left = 1                   ##< how many more bombs the player can put at the time
    self.flame_length = 1                 ##< how long the flame is in tiles
    self.items = {}                       ##< which items and how many the player has, format: [item code]: count
    self.has_spring = False               ##< whether player's bombs have springs
    self.has_shoe = False                 ##< whether player has a kicking shoe
    self.disease_time_left = 0
    self.disease = Player.DISEASE_NONE
    self.has_multibomb = False
    self.has_boxing_glove = False
    self.has_throwing_glove = False
    self.boxing = False
    self.detonator_bombs_left = 0         ##< what number of following bombs will have detonators
    self.detonator_bombs = []             ##< references to bombs to be detonated
    self.wait_for_special_release = False ##< helper used to wait for special key release
    self.wait_for_bomb_release = False
    self.throwing_time_left = 0           ##< for how longer (in ms) the player will be in a state of throwing (only for visuals)
    self.state_backup = Player.STATE_IDLE_UP    ##< used to restore previous state, for example after jump
    self.jumping_to = (0,0)               ##< coordinates of a tile the player is jumping to
    self.teleporting_to = (0,0)
    self.wait_for_tile_transition = False ##< used to stop the destination teleport from teleporting the player back immediatelly
    self.invincible = False               ##< can be used to make the player immortal
    self.info_board_update_needed = True
    self.kills = 0
    self.wins = 0
  
    self.items[GameMap.ITEM_BOMB] = 1
    self.items[GameMap.ITEM_FLAME] = 1

Line 270
Oddly enough, the developer started to use comments to explain what each field was related to, but stopped halfway through before picking it back up again.

    self.has_shoe = False                 ##< whether player has a kicking shoe
    self.disease_time_left = 0
    self.disease = Player.DISEASE_NONE
    self.has_multibomb = False
    self.has_boxing_glove = False
    self.has_throwing_glove = False
    self.boxing = False

Line 361
When a player is killed, the object is not destroyed, meaning that for the duration of the game, this object is taking up space.

  def kill(self, game_map):
    if self.invincible:
      return
    
    self.info_board_update_needed = True
    
    self.state = Player.STATE_DEAD
    game_map.add_sound_event(SoundPlayer.SOUND_EVENT_DEATH)

Line 446 and 466
Code uses a static array instead of an iterator with range (in Python 2).

      result += [item for i in range(self.items[item])]
        
    return result
    for y in range (self.jumping_from[1] - 3,self.jumping_from[1] + 4):
      for x in range (self.jumping_from[0] - 3,self.jumping_from[0] + 4):
        tile = game_map.get_tile_at((x,y))

Line 534
This method has a lot of glaring issues, first and foremost, this method has the inappropriate intimacy code smell written all over it. In constantly calls the methods of the map class, meaning that any changes made to either class is going require major refactoring within the other.

Line 556
Utilizing a huge wall of switch statements like this makes the code much harder to read and will cause considerable trouble to refactor. Extracting this to sub-method that handles the logic would help considerably.

def give_item(self, item, game_map=None):
    self.items[item] = 1 if not item in self.items else self.items[item] + 1
      
    self.info_board_update_needed = True
      
    if item == GameMap.ITEM_RANDOM:
      item = random.choice((
        GameMap.ITEM_BOMB,
        GameMap.ITEM_FLAME,
        GameMap.ITEM_SUPERFLAME,
        GameMap.ITEM_MULTIBOMB,
        GameMap.ITEM_SPRING,
        GameMap.ITEM_SHOE,
        GameMap.ITEM_SPEEDUP,
        GameMap.ITEM_DISEASE,
        GameMap.ITEM_BOXING_GLOVE,
        GameMap.ITEM_DETONATOR,
        GameMap.ITEM_THROWING_GLOVE
        ))
      
    sound_to_make = SoundPlayer.SOUND_EVENT_CLICK
      
    if item == GameMap.ITEM_BOMB:
      self.bombs_left += 1
    elif item == GameMap.ITEM_FLAME:
      self.flame_length += 1
    elif item == GameMap.ITEM_SUPERFLAME:
      self.flame_length = max(GameMap.MAP_WIDTH,GameMap.MAP_HEIGHT)
    elif item == GameMap.ITEM_MULTIBOMB:
      self.has_multibomb = True
    elif item == GameMap.ITEM_DETONATOR:
      self.detonator_bombs_left = 3      
    elif item == GameMap.ITEM_SPRING:
      self.has_spring = True
      sound_to_make = SoundPlayer.SOUND_EVENT_SPRING
    elif item == GameMap.ITEM_SPEEDUP:
      self.speed = min(self.speed + Player.SPEEDUP_VALUE,Player.MAX_SPEED)
    elif item == GameMap.ITEM_SHOE:
      self.has_shoe = True
    elif item == GameMap.ITEM_BOXING_GLOVE:
      self.has_boxing_glove = True
    elif item == GameMap.ITEM_THROWING_GLOVE:
      self.has_throwing_glove = True
    elif item == GameMap.ITEM_DISEASE:
      chosen_disease = random.choice([
        (Player.DISEASE_SHORT_FLAME,SoundPlayer.SOUND_EVENT_DISEASE),     
        (Player.DISEASE_SLOW,SoundPlayer.SOUND_EVENT_SLOW),
        (Player.DISEASE_DIARRHEA,SoundPlayer.SOUND_EVENT_DIARRHEA),
        (Player.DISEASE_FAST_BOMB,SoundPlayer.SOUND_EVENT_DISEASE),
        (Player.DISEASE_REVERSE_CONTROLS,SoundPlayer.SOUND_EVENT_DISEASE),
        (Player.DISEASE_SWITCH_PLAYERS,SoundPlayer.SOUND_EVENT_DISEASE),
        (Player.DISEASE_NO_BOMB,SoundPlayer.SOUND_EVENT_DISEASE),
        (Player.DISEASE_EARTHQUAKE,SoundPlayer.SOUND_EVENT_EARTHQUAKE)
        ])
      
      if chosen_disease[0] == Player.DISEASE_SWITCH_PLAYERS:
        if game_map != None:
          players = filter(lambda p: not p.is_dead(), game_map.get_players())
          
          player_to_switch = self
          
          if len(players) > 1:     # should always be true
            while player_to_switch == self:
              player_to_switch = random.choice(players)
          
          my_position = self.get_position()
          self.set_position(player_to_switch.get_position())
          player_to_switch.set_position(my_position)
      elif chosen_disease[0] == Player.DISEASE_EARTHQUAKE:
        if game_map != None:
          game_map.start_earthquake()
      else:
        self.disease = chosen_disease[0]
        self.disease_time_left = Player.DISEASE_TIME
    
      sound_to_make = chosen_disease[1]
    
    if game_map != None and sound_to_make != None:
      game_map.add_sound_event(sound_to_make)

Line 672
The method for how_many_items is completely unused in the entirety of the code for the game, and is just dead code.

  def how_many_items(self, item):
    if not item in self.items:
      return 0
    
    return self.items[item]

Link 743
The __manage_input_actions function handles a large number of potential player actions using conditional statements, and the method is almost 100 lines long. This long method could be split into some smaller sub methods to improve readability and make the code more abstract, such as having a method to handle specific inputs, like placing bombs, or moving.

def __manage_input_actions(self, input_actions, game_map, distance_to_travel):
    moved = False         # to allow movement along only one axis at a time
    detonator_triggered = False
    special_was_pressed = False
    bomb_was_pressed = False

    for item in input_actions:
      if item[0] != self.number:
        continue                           # not an action for this player
      
      input_action = item[1]

      if self.disease == Player.DISEASE_REVERSE_CONTROLS:
        input_action = PlayerKeyMaps.get_opposite_action(input_action)
          
      if not moved:
        if input_action == PlayerKeyMaps.ACTION_UP:
          self.position[1] -= distance_to_travel
          self.state = Player.STATE_WALKING_UP
          moved = True
        elif input_action == PlayerKeyMaps.ACTION_DOWN:
          self.position[1] += distance_to_travel
          self.state = Player.STATE_WALKING_DOWN
          moved = True
        elif input_action == PlayerKeyMaps.ACTION_RIGHT:
          self.position[0] += distance_to_travel
          self.state = Player.STATE_WALKING_RIGHT
          moved = True
        elif input_action == PlayerKeyMaps.ACTION_LEFT:
          self.position[0] -= distance_to_travel
          self.state = Player.STATE_WALKING_LEFT
          moved = True
    
      if input_action == PlayerKeyMaps.ACTION_BOMB:
        bomb_was_pressed = True
        
        if not self.wait_for_bomb_release and self.bombs_left >= 1 and not game_map.tile_has_bomb(self.position) and not self.disease == Player.DISEASE_NO_BOMB:
          self.putting_bomb = True
    
      if input_action == PlayerKeyMaps.ACTION_BOMB_DOUBLE:  # check multibomb
        if self.has_throwing_glove:
          self.throwing = True
        elif self.has_multibomb: 
          self.putting_multibomb = True

      if input_action == PlayerKeyMaps.ACTION_SPECIAL:
        special_was_pressed = True
        
        if not self.wait_for_special_release:       
          while len(self.detonator_bombs) != 0:       # find a bomb to ddetonate (some may have exploded by themselves already)
            self.info_board_update_needed = True
            
            bomb_to_check = self.detonator_bombs.pop()
          
            if bomb_to_check.has_detonator() and not bomb_to_check.has_exploded and bomb_to_check.movement != Bomb.BOMB_FLYING:
              game_map.bomb_explodes(bomb_to_check)
              detonator_triggered = True
              self.wait_for_special_release = True    # to not detonate other bombs until the key is released and pressed again
              break
            
          if not detonator_triggered and self.has_boxing_glove:
            self.boxing = True
      
    if moved:
      game_map.add_sound_event(SoundPlayer.SOUND_EVENT_WALK)

    if not special_was_pressed:
      self.wait_for_special_release = False
      
    if not bomb_was_pressed:
      self.wait_for_bomb_release = False

Line 882
Another long method, this time for the bomb class that changes a lot of Player fields, reacts_to_inputs spans more than 100 lines which could be split into smaller methods such as checking to see if bombs are placed or if the players gets the disease power-down.

  def react_to_inputs(self, input_actions, dt, game_map):
    if self.state == Player.STATE_DEAD or game_map.get_state() == GameMap.STATE_WAITING_TO_PLAY:
      return
    
    if self.state in [Player.STATE_IN_AIR,Player.STATE_TELEPORTING]:
      self.state_time += dt

      if self.state_time >= (Player.JUMP_DURATION if self.state == Player.STATE_IN_AIR else Player.TELEPORT_DURATION):
        self.state = self.state_backup
        self.state_time = 0
        self.jumping_to = None
        self.teleporting_to = None
      else:
        return 

    current_speed = self.speed if self.disease != Player.DISEASE_SLOW else Player.SLOW_SPEED
    
    distance_to_travel = dt / 1000.0 * current_speed
    
    self.throwing_time_left = max(0,self.throwing_time_left - dt)

    self.position = list(self.position)    # in case position was tuple

    old_state = self.state
 
    if self.state in (Player.STATE_WALKING_UP,Player.STATE_IDLE_UP):
      self.state = Player.STATE_IDLE_UP
    elif self.state in (Player.STATE_WALKING_RIGHT,Player.STATE_IDLE_RIGHT):
      self.state = Player.STATE_IDLE_RIGHT
    elif self.state in (Player.STATE_WALKING_DOWN,Player.STATE_IDLE_DOWN):
      self.state = Player.STATE_IDLE_DOWN
    else:
      self.state = Player.STATE_IDLE_LEFT

    previous_position = copy.copy(self.position)    # in case of collision we save the previous position

    self.putting_bomb = False
    self.putting_multibomb = False
    self.throwing = False
    self.boxing = False
    
    if self.disease == Player.DISEASE_DIARRHEA:
      input_actions.append((self.number,PlayerKeyMaps.ACTION_BOMB))  # inject bomb put event

    self.__manage_input_actions(input_actions, game_map, distance_to_travel)
      
    # resolve collisions:
    check_collisions = True

    current_tile = self.get_tile_position()  
    previous_tile = Positionable.position_to_tile(previous_position)
    transitioning_tiles = current_tile != previous_tile
    
    if transitioning_tiles:
      self.wait_for_tile_transition = False
    
    if game_map.tile_has_bomb(current_tile):   # first check if the player is standing on a bomb
      if not transitioning_tiles:              
        check_collisions = False               # no transition between tiles -> let the player move

    collision_happened = False

    if check_collisions:
      collision_happened = self.__resolve_collisions(game_map, distance_to_travel, previous_position)
    
    if self.putting_bomb and not game_map.tile_has_bomb(self.get_tile_position()) and not game_map.tile_has_teleport(self.position):
      self.lay_bomb(game_map)
    
    # check if bomb kick or box happens
    self.__manage_kick_box(game_map, collision_happened)
    
    if self.throwing:
      bomb_thrown = game_map.bomb_on_tile(current_tile)
      game_map.add_sound_event(SoundPlayer.SOUND_EVENT_THROW)
    
      if bomb_thrown != None:
        forward_tile = self.get_forward_tile_position()
        direction_vector = self.get_direction_vector()
        destination_tile = (forward_tile[0] + direction_vector[0] * 3,forward_tile[1] + direction_vector[1] * 3)
        bomb_thrown.send_flying(destination_tile)
        self.wait_for_bomb_release = True
        self.throwing_time_left = 200
    
    elif self.putting_multibomb:                     # put multibomb
      current_tile = self.get_tile_position()
      
      if self.state in (Player.STATE_WALKING_UP,Player.STATE_IDLE_UP):
        tile_increment = (0,-1)
      elif self.state in (Player.STATE_WALKING_RIGHT,Player.STATE_IDLE_RIGHT):
        tile_increment = (1,0)
      elif self.state in (Player.STATE_WALKING_DOWN,Player.STATE_IDLE_DOWN):
        tile_increment = (0,1)
      else:     # left
        tile_increment = (-1,0)
  
      i = 1
  
      while self.bombs_left > 0:
        next_tile = (current_tile[0] + i * tile_increment[0],current_tile[1] + i * tile_increment[1])
        if not game_map.tile_is_walkable(next_tile) or game_map.tile_has_player(next_tile):
          break
        
        self.lay_bomb(game_map,next_tile)
        i += 1
  
    # check disease
    
    if self.disease != Player.DISEASE_NONE:
      self.disease_time_left = max(0,self.disease_time_left - dt)
      
      if self.disease_time_left == 0:
        self.disease = Player.DISEASE_NONE
        self.info_board_update_needed = True
    
    if old_state == self.state:
      self.state_time += dt
    else:
      self.state_time = 0                 # reset the state time

AI Class code review

AI class | lines 4547-4933

start

In the for loop they use a hardcoded set form 0-3 this set could be an iterator to improve memory usage.

line 4613

  def rate_bomb_escape_directions(self, tile_coordinates):
                    #          up       right   down   left 
    axis_directions =          ((0,-1), (1,0),  (0,1), (-1,0))
    perpendicular_directions = ((1,0),  (0,1),  (1,0), (0,1))

    result = [0,0,0,0]
    
    for direction in (0,1,2,3):#TODO: This could be a range iterator
      for i in range(1,self.player.get_flame_length() + 2):
        axis_tile = (tile_coordinates[0] + i * axis_directions[direction][0],tile_coordinates[1] + i * axis_directions[direction][1])
        
        if not self.tile_is_escapable(axis_tile):
          break
        
        perpendicular_tile1 = (axis_tile[0] + perpendicular_directions[direction][0],axis_tile[1] + perpendicular_directions[direction][1])
        perpendicular_tile2 = (axis_tile[0] - perpendicular_directions[direction][0],axis_tile[1] - perpendicular_directions[direction][1])

        if i > self.player.get_flame_length() and self.game_map.get_danger_value(axis_tile) >= GameMap.SAFE_DANGER_VALUE:
          result[direction] += 1
          
        if self.tile_is_escapable(perpendicular_tile1) and self.game_map.get_danger_value(perpendicular_tile1) >= GameMap.SAFE_DANGER_VALUE:
          result[direction] += 1
          
        if self.tile_is_escapable(perpendicular_tile2) and self.game_map.get_danger_value(perpendicular_tile2) >= GameMap.SAFE_DANGER_VALUE:  
          result[direction] += 1
    
    return tuple(result)

The code below is a good example of how this code is well documented but not well commented. There is no explanation as to how the danger score is being used. While I appreciate the headers in cases like this they don't help the reviewer understand why things are the way they are.

line 4613

## Returns an integer score in range 0 - 100 for given file (100 = good, 0 = bad).
    
  def rate_tile(self, tile_coordinates):
    danger = self.game_map.get_danger_value(tile_coordinates)
    
    if danger == 0:
      return 0
    
    score = 0
    
    if danger < 1000:
      score = 20
    elif danger < 2500:
      score = 40
    else:
      score = 60
    
    tile_item = self.game_map.get_tile_at(tile_coordinates).item
    
    if tile_item != None:
      if tile_item != GameMap.ITEM_DISEASE:
        score += 20
      else:
        score -= 10
        
    top = (tile_coordinates[0],tile_coordinates[1] - 1)
    right = (tile_coordinates[0] + 1,tile_coordinates[1])
    down = (tile_coordinates[0],tile_coordinates[1] + 1)
    left = (tile_coordinates[0] - 1,tile_coordinates[1])
    
    if self.game_map.tile_has_lava(top) or self.game_map.tile_has_lava(right) or self.game_map.tile_has_lava(down) or self.game_map.tile_has_lava(left):
      score -= 5    # don't go near lava
    
    if self.game_map.tile_has_bomb(tile_coordinates):
      if not self.player.can_box():
        score -= 5
    
    return score

In the below code a static mapping of cardinal directions appears again. Considering it's a static type and used in a number of locations it should be defined as other elements of the code are.

line 4698

  def number_of_blocks_next_to_tile(self, tile_coordinates):
    count = 0
    
    for tile_offset in ((0,-1),(1,0),(0,1),(-1,0)):  # for each neigbour file
      helper_tile = self.game_map.get_tile_at((tile_coordinates[0] + tile_offset[0],tile_coordinates[1] + tile_offset[1]))
      
      if helper_tile != None and helper_tile.kind == MapTile.TILE_BLOCK:
        count += 1
      
    return count

This is an odd choice rather than deleting the AI player object it continues to live and waste CPU cycles.
line 4738

  def play(self):
    if self.do_nothing or self.player.is_dead():
      return []
........

Again in the below an iterator could be used instead of a static array.

line 4797

      for direction in (0,1,2,3):
        score = self.rate_tile((current_tile[0] + tile_increment[direction][0],current_tile[1] + tile_increment[direction][1]))  
........

At this point, I should mention in the play function each action choice has a section of code associated with it. Each one of these sections could be split off into another function. This isn't necessary but rather a stylistic choice.

In the below code range should be xrange() as to not generate and store a set but to rather use an iterator.
line 4919

      for i in range(multibomb_count):
        if not self.game_map.tile_is_walkable(current_tile) or not self.game_map.tile_is_withing_map(current_tile):
          break
........

Reviewed with minor issues

  • AI class

Recommendations:

  • Refactor as noted in review
  • Move to a AI module to reduce size of main file
  • Replacing static sets with iterators
  • The replacement of range() with xrange()
  • Altering functionality so that AI's can be deleted when dead so they don't waste CPU cycles
  • Adding global definitions for the cardinal direction tuples.

Menu Classes Code Review

Menu Class

start

Line 2775:

  MENU_STATE_SELECTING = 0                ##< still selecting an item
  MENU_STATE_CONFIRM = 1                  ##< menu has been confirmed
  MENU_STATE_CANCEL = 2                   ##< menu has been cancelled
  MENU_STATE_CONFIRM_PROMPT = 3           ##< prompting an action
  
MENU_MAX_ITEMS_VISIBLE = 11

Use of constant variables to define numbers is good here

Line 2841

def process_inputs(self, input_list):
    if self.menu_left:
      self.menu_left = False
      self.state = Menu.MENU_STATE_SELECTING
      
      for action_code in self.action_keys_previous_state:
        self.action_keys_previous_state[action_code] = True
        
      return
    
    actions_processed = []
    actions_pressed = []
    
    for action in input_list:
      action_code = action[1]
      
      if not self.action_keys_previous_state[action_code]:
        # the following condition disallows ACTION_BOMB and ACTION_BOMB_DOUBLE to be in the list at the same time => causes trouble
        if (not (action_code in actions_pressed) and not(
          (action_code == PlayerKeyMaps.ACTION_BOMB and PlayerKeyMaps.ACTION_BOMB_DOUBLE in actions_pressed) or
          (action_code == PlayerKeyMaps.ACTION_BOMB_DOUBLE and PlayerKeyMaps.ACTION_BOMB in actions_pressed) )):
          actions_pressed.append(action_code)
    
      actions_processed.append(action_code)
    
    for action_code in self.action_keys_previous_state:
      self.action_keys_previous_state[action_code] = False
      
    for action_code in actions_processed:
      self.action_keys_previous_state[action_code] = True
    
    for action in actions_pressed:
self.action_pressed(action)

This is a long function that could be decomposed into smaller actions. process_inputs is not a very descriptive name.

Line 2923

    if self.state == Menu.MENU_STATE_CONFIRM and (self.confirm_prompt_result == None or self.confirm_prompt_result == False) and self.selected_item == menu_item_coordinates:

This if statement is too long and should be split into multiple lines.

Line 2941

else:
      if action == PlayerKeyMaps.ACTION_UP:
        self.selected_item = (max(0,self.selected_item[0] - 1),self.selected_item[1])
      elif action == PlayerKeyMaps.ACTION_DOWN:
        self.selected_item = (min(len(self.items[self.selected_item[1]]) - 1,self.selected_item[0] + 1),self.selected_item[1])
      elif action == PlayerKeyMaps.ACTION_LEFT:
        new_column = max(0,self.selected_item[1] - 1)
        self.selected_item = (min(len(self.items[new_column]) - 1,self.selected_item[0]),new_column)
      elif action == PlayerKeyMaps.ACTION_RIGHT:
        new_column = min(len(self.items) - 1,self.selected_item[1] + 1)
        self.selected_item = (min(len(self.items[new_column]) - 1,self.selected_item[0]),new_column)
      elif action == PlayerKeyMaps.ACTION_BOMB or action == PlayerKeyMaps.ACTION_BOMB_DOUBLE:
        self.state = Menu.MENU_STATE_CONFIRM
      elif action == PlayerKeyMaps.ACTION_SPECIAL:
self.state = Menu.MENU_STATE_CANCEL

This if statement is really wordy and has a lot of repetition. Would make sense to create a helper method for min(len(...)) and max(len(...)) with a better description of action.

ResultMenu class

start

Line 2999

Function set_results is really long.

Line 3019

for winner_number in winner_team_numbers:
        if first:
          first = False
        else:
          announcement_text += ", "
          
        announcement_text += Renderer.colored_color_name(winner_team_numbers[winner_number])
    
announcement_text += "!"

This block of code could probably be cut down with some combination of list comprehension, filter, map, join, etc.

SettingsMenu class

Start

Line 3099

 self.items = [(
      "sound volume: " + (SettingsMenu.COLOR_ON if self.settings.sound_is_on() else SettingsMenu.COLOR_OFF) + str(int(self.settings.sound_volume * 10) * 10) + " %",
      "music volume: " + (SettingsMenu.COLOR_ON if self.settings.music_is_on() > 0.0 else SettingsMenu.COLOR_OFF) + str(int(self.settings.music_volume * 10) * 10) + " %",
      "screen resolution: " + str(self.settings.screen_resolution[0]) + " x " + str(self.settings.screen_resolution[1]),
      "fullscreen: " + self.bool_to_str(self.settings.fullscreen),
      "allow control by mouse: " + self.bool_to_str(self.settings.control_by_mouse),
      "configure controls",
      "complete reset",
      "back"
)]

Some of these if statement don't fit on one line. Would be nice to split into multiple lines if possible.
Line 3122

if self.selected_item == (0,0):
          self.settings.sound_volume = min(1.0,self.settings.sound_volume + 0.1)
          self.game.apply_sound_settings()
          self.game.save_settings()
        elif self.selected_item == (1,0):
          self.settings.music_volume = min(1.0,self.settings.music_volume + 0.1)
          self.game.apply_sound_settings()
          self.game.save_settings()
        elif self.selected_item == (2,0):
          self.settings.screen_resolution = Settings.POSSIBLE_SCREEN_RESOLUTIONS[(self.settings.current_resolution_index() + 1) % len(Settings.POSSIBLE_SCREEN_RESOLUTIONS)]      
          self.game.apply_screen_settings()
          self.game.save_settings()
      elif action == PlayerKeyMaps.ACTION_LEFT:
        if self.selected_item == (0,0):
          self.settings.sound_volume = max(0.0,self.settings.sound_volume - 0.1)
          self.game.apply_sound_settings()
          self.game.save_settings()
        elif self.selected_item == (1,0):
          self.settings.music_volume = max(0.0,self.settings.music_volume - 0.1)
          self.game.apply_sound_settings()
          self.game.save_settings()
        elif self.selected_item == (2,0):
          self.settings.screen_resolution = Settings.POSSIBLE_SCREEN_RESOLUTIONS[(self.settings.current_resolution_index() - 1) % len(Settings.POSSIBLE_SCREEN_RESOLUTIONS)]
          self.game.apply_screen_settings()
self.game.save_settings()

This is really long and has a lot of redundant code. Would help to extract some stuff out to another function.

Reviewed without issues:

  • MainMenu class

Recommendations:

  • Refactor as noted in review
  • Move to a menus module to reduce size of main file

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.