4
\$\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
  • 1
    \$\begingroup\$ the "same" code 4 times => Make a class. Then it's 4 calls to the same code. \$\endgroup\$
    – radarbob
    Commented Mar 18, 2022 at 4:41

2 Answers 2

2
\$\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\$
2
\$\begingroup\$

struggling to refactor

Make classes. This will organize the functionality which in turn simplifies methods because it is not doing everything all at once.

Start with simple things and build on that. Classes are essential in this approach.

Identify concepts and think about potential (class) structure needed to capture and pass information/data/arguments/etc. For example a Shot is more than a target grid-coordinate. I think who fired and end result are also needed. A Fleet class will help with things like checking for all ships sunk in that fleet.

Put off complex stuff like entering ship positions with deconfliction/validation. Just hard code the ship positions for now; refactoring will be 10x easier.


Basic Classes

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.


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

If the below code looks like an awful lot of code that is doing nothing, that misses the point totally. The individual methods are very short because function is focused; this in turn makes code dead simple. Short and simple methods is characteristic of good class design. Then, notice methods sunk? & to_s. Building up composite behavior across Ship, Fleet, Player stays simple even as class code grows. I call this the invisible hand of Object Oriented design.

API eliminates Contrivances

The POINTS (7 points to win) idea is gone. It existed because there was no way to easily determine player_arrayShip state - points_p1 == solution sunk? or not, then for the whole undefined Fleet.

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?}"

Battleship Class

This 'driver' will contain all code to build and play a game. Therefore you can say that all code is in the Battleship namespace. Nothing is in the Ruby interpreter global (name)space. Always a good thing.

class Battleship
  #working on it

end #Battleship

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 and acknowledge you have read our privacy policy.

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