3
\$\begingroup\$

I'm creating a battleship game in Ruby. Let me explain how it works:

  • The game is played on 5x5 grids (one per player).
  • Players have 2 ships each to place on their grid. A small ship (3x1 side) and a large one (4x1 size).
  • Players are asked in turn to place their ships on their board. A ship can't be placed out of bounds nor on the same space as another ship.
  • Players take turns to shoot at the opponent grid one after the other by selecting coordinates.
  • The first player to reach 7 points (all boats sinked) wins the game.

As you can see, some parts of my code are very repetitive:

  • I ask to each player to place its 2 ships so I have the "same" code 4 times (--- PLACEMENT OF THE SIZE 3 SHIP OF #{name_p1.upcase} ---).
  • I ask players to take turns to shoot at the opponent grid one after the other (this is done twice).
  • Same problem when I print the result of a shot.

I'm very struggling to refactor my code to make it simpler and shorter. How can it be simplified?

# Board for player 1
board1 = []
for i in 0..4
  board1[i] = []
  (0..4).each do
    board1[i].append('O')
  end
end

# Board for player 2
board2 = []
for i in 0..4
  board2[i] = []
  (0..4).each do
    board2[i].append('O')
  end
end

# Display the boards
def display_board(board)
  for row in board
    puts row.map { |k| "#{k}" }.join('  ')
  end
end

def check_obstacle(ship_size, player_array, posX, posY, direction)
  check = 0
  @ship_present = 0
  @space_available = 0

  @ship_present += 1 if player_array[posX][posY] == 1
  @space_available += 1 if player_array[posX][posY] == 0
  while check < ship_size && @space_available < ship_size
    case direction
    when 'north' then posX -= 1
    when 'east' then posY += 1
    when 'south' then posX += 1
    when 'west' then posY -= 1
    end
    
    @space_available += 1 if posX.between?(0, 4) && posY.between?(0, 4)
    @ship_present += 1 if posX.between?(0, 4) && posY.between?(0, 4) && player_array[posX][posY] == 1
    check += 1
  end
  @space_available == ship_size && @ship_present == 0
end

def ship_placement(ship_size, player_array, posX, posY, direction)
  steps = 0

  while steps < ship_size && @ship_present == 0 && @space_available == ship_size
    player_array[posX][posY] = 1
    case direction
    when 'north' then posX -= 1
    when 'east' then posY += 1
    when 'south' then posX += 1
    when 'west' then posY -= 1
    end
    steps += 1
  end
  puts "The ship of size #{ship_size} is placed."
end

# Generation of the player boards
array1 = [[0, 0, 0, 0, 0], [0, 0, 0, 0, 0], [0, 0, 0, 0, 0], [0, 0, 0, 0, 0], [0, 0, 0, 0, 0]]
array2 = [[0, 0, 0, 0, 0], [0, 0, 0, 0, 0], [0, 0, 0, 0, 0], [0, 0, 0, 0, 0], [0, 0, 0, 0, 0]]

# A player wins when he/she reaches 7 points (2 ships each to place on their grid : a small ship (3x1 side) and a large one (4x1 size))
solution = 7

# We set the number of points at 0 at the start of the game
points_p1 = 0
points_p2 = 0

#Starting of the game and printing the board 
while true do
  puts 'Welcome soldiers!'
  puts 'To start the game, enter "start". To quit, enter "stop":'
  starting = gets.chomp
  puts "\n"

  case starting.downcase
  when 'start'
    puts 'Enter the first name of the first player:'
    name_p1 = gets.chomp.to_s
    puts "\n"
    puts 'Enter the first name of the second player:'
    name_p2 = gets.chomp.to_s
    puts "\n"

    puts "--- PLACEMENT OF THE SIZE 3 SHIP OF #{name_p1.upcase} ---"
    while true
      while true
        puts 'Enter LINE number (between 1 and 5):'
        placement_row = gets.chomp.to_i-1
        break if placement_row.between?(0, 4)
      end

      while true
        puts 'Enter COLUMN number (between 1 and 5):'
        placement_column = gets.chomp.to_i-1
        break if placement_column.between?(0, 4)
      end

      while true
        puts "Enter ship direction (north, east, south ou west)"
        orientation = gets.chomp.downcase
        break if %w(north east south west).include? orientation
      end

      if check_obstacle(3, array1, placement_row, placement_column, orientation)
        ship_placement(3, array1, placement_row, placement_column, orientation)
        break
      else
        puts "Unable to place. Please try again."
      end
    end

    puts "--- PLACEMENT OF THE SIZE 4 SHIP OF #{name_p1.upcase} ---"
    while true
      while true
        puts 'Enter LINE number (between 1 and 5):'
        placement_row = gets.chomp.to_i-1
        break if placement_row.between?(0, 4)
      end

      while true
        puts 'Enter COLUMN number (between 1 and 5):'
        placement_column = gets.chomp.to_i-1
        break if placement_column.between?(0, 4)
      end

      while true
        puts "Enter ship direction (north, east, south ou west)"
        orientation = gets.chomp.downcase
        break if %w(north east south west).include? orientation
      end

      if check_obstacle(4, array1, placement_row, placement_column, orientation)
        ship_placement(4, array1, placement_row, placement_column, orientation)
        break
      else
        puts "Unable to place. Please try again."
      end
    end
###############################################################
    puts "--- PLACEMENT OF THE SIZE 3 SHIP OF #{name_p2.upcase} ---"
    while true
      while true
        puts 'Enter LINE number (between 1 and 5):'
        placement_row = gets.chomp.to_i-1
        break if placement_row.between?(0, 4)
      end

      while true
        puts 'Enter COLUMN number (between 1 and 5):'
        placement_column = gets.chomp.to_i-1
        break if placement_column.between?(0, 4)
      end

      while true
        puts "Enter ship direction (north, east, south ou west)"
        orientation = gets.chomp.downcase
        break if %w(north east south west).include? orientation
      end

      if check_obstacle(3, array2, placement_row, placement_column, orientation)
        ship_placement(3, array2, placement_row, placement_column, orientation)
        break
      else
        puts "Unable to place. Please try again."
      end
    end

    puts "--- PLACEMENT OF THE SIZE 4 SHIP OF #{name_p2.upcase} ---"
    while true
      while true
        puts 'Enter LINE number (between 1 and 5):'
        placement_row = gets.chomp.to_i-1
        break if placement_row.between?(0, 4)
      end

      while true
        puts 'Enter COLUMN number (between 1 and 5):'
        placement_column = gets.chomp.to_i-1
        break if placement_column.between?(0, 4)
      end

      while true
        puts "Enter ship direction (north, east, south ou west)"
        orientation = gets.chomp.downcase
        break if %w(north east south west).include? orientation
      end

      if check_obstacle(4, array2, placement_row, placement_column, orientation)
        ship_placement(4, array2, placement_row, placement_column, orientation)
        break
      else
        puts "Unable to place. Please try again."
      end
    end

    while points_p1 < solution || points_p2 < solution
      puts "--- #{name_p1.upcase}'S TURN ---"
      while true
        puts 'Enter LINE number (between 1 and 5):'
        row_p1 = gets.chomp.to_i-1
        break if row_p1.between?(0, 4)
      end
      while true
        puts 'Enter COLUMN number (between 1 and 5):'
        column_p1 = gets.chomp.to_i-1
        break if column_p1.between?(0, 4)
      end
      
      # Shot fired!
      case array1[row_p1][column_p1]
      when 1
        board1[row_p1][column_p1] = 'X'
        array1[row_p1][column_p1] = 'X'
        points_p1 += 1
      when 0
        board1[row_p1][column_p1] = '-'
        array1[row_p1][column_p1] = '-'
      when 'X', '-'
        puts 'Square already played.'
        next
      end

      puts "\n"
      puts '--------------'
      display_board(board1)
      puts '--------------'
      puts '----------------------'
      puts "#{name_p1} has #{points_p1} point#{"s" if points_p1 > 1}."
      puts '----------------------'
      puts "\n"

    break if points_p1 == solution

      puts "--- #{name_p2.upcase}'S TURN ---"
      while true
        puts 'Enter LINE number (between 1 and 5):'
        row_p2 = gets.chomp.to_i-1
        break if row_p2.between?(0, 4)
      end
      while true
        puts 'Enter COLUMN number (between 1 and 5):'
        column_p2 = gets.chomp.to_i-1
        break if column_p2.between?(0, 4)
      end

      # Shot fired!
      case array2[row_p2][column_p2]
      when 1
        board2[row_p2][column_p2] = 'X'
        array2[row_p1][column_p1] = 'X'
        points_p2 += 1
      when 0
        board2[row_p2][column_p2] = '-'
      when 'X', '-'
        next
      end

      puts "\n"
      puts '--------------'
      display_board(board2)
      puts '--------------'
      puts '----------------------'
      puts "#{name_p2} has #{points_p2} point#{"s" if points_p2 > 1}."
      puts '----------------------'
      puts "\n"
      break if points_p2 == solution
    end

    puts "Congratulations #{name_p1}, you have destroyed all the enemy ships!" if points_p1 == solution
    puts "Congratulations #{name_p2}, you have destroyed all the enemy ships!" if points_p2 == solution
    puts "\n"
    break

  when 'stop'
    puts 'See you soon!'
    break
  else
    puts 'Please make a choice between "start". To exit, enter "stop".'
    puts "\n"
  end
end
\$\endgroup\$
1
  • \$\begingroup\$ the "same" code 4 times => Make a class. Then it's 4 calls to the same code. \$\endgroup\$
    – radarbob
    Mar 18 at 4:41

2 Answers 2

1
\$\begingroup\$

I think an object oriented approach would help a lot to organize your code. Something like:

class Player
  attr_reader :name, :number
  attr_accessor :board, :points

  def initialize(number)
    @number = number
    # These nested Array.new calls will do pretty much the same as that 
    # `for i in 0..4` code you have at the beginning
    @board = Array.new(5) { Array.new(5, 'O') }
    @points = 0
  end

  def input_name
    puts 'Enter the first name of the first player:'
    self.name = gets.chomp.to_s
    puts "\n"
  end

  # ...
end

class Battleship
  attr_reader :player_1, :player_2
  # The solution value is never going to change, so it's a constant. Not
  # really sure if belongs here or Player, but it's up to you where you'll
  # use it more often.
  SOLUTION = 7

  def initialize
    @player_1 = Player.new(1)
    @player_2 = Player.new(2)
  end

  def play
    while true do
      puts 'Welcome soldiers!'
      # ...
  end
end

Battleship.new.play

then start moving some of your methods to these classes. To me looks like check_obstacle and ship_placement belong more to a Player instance. Also you could refactor the inputs and add some methods like:

def number_input(type = :line)
  while true
    puts "Enter #{type} number (between 1 and 5):"
    input = gets.chomp.to_i - 1
    return input if input.between?(0, 4)
  end
end

def direction_input
  while true
    puts "Enter ship direction (north, east, south ou west)"
    orientation = gets.chomp.downcase
    return orientation if %w(north east south west).include?(orientation)
  end
end

then call them like:

  placement_row = number_input
  placement_column = number_input(:column)
  orientation = direction_input

indeed, all that puts "--- PLACEMENT OF THE SIZE 3 SHIP OF... code looks like it can be moved into a single method.
I can see there's a lot of space to refactor in your code, but I hope this gives you some hints about where to point from here.

\$\endgroup\$
1
\$\begingroup\$

struggling to refactor

There is no structure to refactor into so create foundational structure. A class is structure. Make classes.


The Board

Coding board functionality is complex no matter what you do. One or more Board related classes will make complexity more manageable. I suggest hard coding some things now and replacing it with code later.

  • Declare a literal 2d array, don't construct it with loops.
  • Hard code the ship positions
  • Come up with a concept for coordinates, mapping ship locations and tracking hits

Basic Classes (Structure)

In the beginning You cannot know every class and every method you will eventually need. As you build the game you'll realize things to add, change, and delete.

Start with the simple and obvious. Write enough of each class to construct it and output its details. Build simple first, make sure it works, then go on to more complex. You will be working on several classes at once when building interactions so the basic stuff must be working first.

  • Ship, Fleet, Player
  • Board, Coordinates
  • Battleship - the game "driver"

Ship, Fleet, Player

  • A Player has a Fleet that has Ship(s)
  • Use "battleship" or naval terminology.
  • initialize with needed objects, no public individual attribute setting allowed.
  • Your "solution" I call "strength"
  • classes override to_s
  • default parameters
    • initialize certain properties
    • I think Player will contain a Board which is not built yet
  • see test code at end of this section

make it simpler and shorter. How can it be simplified?

This code looks very long, but it is all dead simple. This is a characteristic of good class design. Take out all the 'def', 'class', 'end', blank lines and you might think "that can't be enough code!" considering what we started from. Simple IS 'single responsibility', class specific code. "Everything all at once" coding is anti-matter to 'simple and short'. Notice methods sunk? & to_s. Building up composite behavior across Ship, Fleet, Player stays simple even as class composition grows. I call this the invisible hand of Object Oriented design.

class Ship
  # strength is how many hits required to sink a new ship
  attr_reader :type, :strength, :hits
  
  def initialize (type = "GarbageScow", strength = 0, hits = 0)
    @type     = type
    @strength = strength
    @hits     = hits
  end
  
  # remaining hits needed to sink
  def firepower
    strength - hits
  end
  
  def hit
    @hits += 1 unless sunk?
  end
  
  def sunk?
    firepower <= 0
  end
  
  def to_s
    "type | strength | hits | sunk? => #{type} | #{strength} | #{hits} | #{sunk?}"
  end

end #Ship


class Fleet
  attr_reader  :ships
  
  def initialize (ships = [])
    @ships = ships
  end
  
  def add (ship)
    @ships << ship unless ship.nil?
  end
  
  # hits needed to sink entire fleet, 
  # minus current hits
  def firepower
    totalPower = 0
    ships.each { |ship| totalPower += ship.firepower }
    return totalPower
  end
  
  def sunk?
    ships.all? { |ship| ship.sunk? }
  end
  
  def to_s
    passInReview = "Ships: #{ships.length}  Firepower: #{firepower} \n"
    
    ships.each { |ship| passInReview << ship.to_s + "\n" }
    
    passInReview
  end
end #Fleet


class Player
  attr_reader :name, :fleet, :ocean
  
  def initialize( name = "Popeye", fleet = [], board = nil )
    @name = name
    @ocean = board
    @fleet = Fleet.new(fleet)
  end
  
  def sunk?
    fleet.sunk?
  end
  
  # override to_s for debugging!
  def to_s
    me = "Admrial #{name} : \n"
    me << fleet.to_s
    return me
  end

end #Player

# ========= Let's Test ====================

p1s1 = Ship.new("destroyer", 3)
p1s2 = Ship.new("patrol", 2, 0)

p3s1 = Ship.new("destroyer", 3)
p3s2 = Ship.new("battleship", 5)
p3s3 = Ship.new("cruiser", 4)
p3s4 = Ship.new("patrol", 2)
p3s5 = Ship.new()

player1 = Player.new("Nelson", [p1s1,p1s2])
player3 = Player.new("Jellicoe", [p3s1, p3s2, p3s3, p3s4, p3s5])

puts Player.new; puts
puts player1; puts
puts player3; puts "#{player3.name} fleet sunk? #{player3.sunk?}"

Board class

I decided I want coordinates like the original game: A2, B7, etc. This class is can be modified for a 2d array easily enough. I don't physically make a 2d array out of it, just a Hash. But note the to_s method.

Notice there is no console stuff in any classes so far. The Battleship class will talk to the user, call the classes, and output to console as needed. When we first focus on the basic class and what they do the interactive stuff is much easier.

class Board
  attr_reader :board

  # coordinate is a string of "rowcolumn"
  @@rows    = ("A".."J")
  @@columns = (1..10)
  
  
  @@hit  = 'H'
  @@miss = 'M'
  @@ship = 'S'  #occupied by a ship. Maybe later- (d)stroyer, (c)arrier, etc.
  
  def initialize
    @board = {}
    @@rows.each{ |letter| @@columns.each{ |col| @board[letter + col.to_s] = letter + col.to_s } }
  end

  def to_s
    theOcean = "\n\ninitialized with position for development debugging"
    @@rows.each{ |row| theOcean << "\n\n"; @@columns.each{ |col| theOcean << board[row+col.to_s] << "   "  } }
    return theOcean
  end
  
  def self.on_map?(coordinate)
    return false if coordinate.nil?
    return false unless coordinate.is_a? String
    
    in_play = true
    in_play = @@rows.include?(coordinate[0]) ? in_play : false     #does not match lowercase
    in_play = @@columns.include?(coordinate[1..-1].to_i) ? in_play : false
    return in_play
  end
  
end  #Board

#=========== let's test ================

ocean = Board.new 
puts ocean
puts "B4 on map? #{Board.on_map? 'B4'}"
puts "b4 on map? #{Board.on_map? 'b4'}"

puts "K4 on map? #{Board.on_map? 'K4'}"
puts "B12 on map? #{Board.on_map? 'B12'}"
puts "J** on map? #{Board.on_map? 'J  '}"
puts "*9 on map? #{Board.on_map? ' 9 '}"
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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