2
\$\begingroup\$

I made a simple, semi-playable command line Chess game for my class exercise.

At this point, this game can do the following things:

  1. Print out the updated board representation after every move
  2. Players can take turns
  3. Players can actually move pieces

The code is pretty long so I don't really expect people to read all of it. However, if you can somehow skim through them and provide one quick feedback, I would be really grateful! I just started learning Ruby and really want to be conscious of how I arrange objects and follow the SOLID principle.

Besides that, I also have one quick question: Is it recommended to put a lot of functions within one function? Sometimes I put 5 or 6 functions within the initialization function of a class. I am worried that might cause some trouble in the future. However, if I don't do that, I will have to call those functions one by one in the main app.rb, which would make the code longer in the main app.rb. Which is generally better?

App.rb

require_relative("lib/input.rb")
require_relative("lib/Board.rb")
require_relative("lib/Chess.rb")
require_relative("lib/Rook.rb")
require_relative("lib/Pawn.rb")
require_relative("lib/King.rb")
require_relative("lib/Bishop.rb")
require_relative("lib/Knight.rb")
require_relative("lib/Queen.rb")




puts "Welcome to this boring Chess Game!"
puts "It is so boring. U sure u want to play? y/n "
enter_game = gets.chomp.upcase

puts "you don't have a choice" if enter_game == "N"
puts "-----------------------"
puts "-----------------------"
puts "X: 1,2,3,4,5,6,7,8"
puts "Y: 1,2,3,4,5,6,7,8"
puts "Sample input: 58 would be Black King(bKing)"
puts "-----------------------"
puts "-----------------------"



board1 = Board.new
board1.show



play_condition = true

while play_condition
    ori_cor = board1.ask_for_ori
    board_move_validity_ori = board1.check_board_valid(ori_cor)

    # a convert function might be more useful here because we are reusing it again and again and also it's pretty simple to set up

    # puts "value 0 #{index_x}"
    # puts index_y

    if !board_move_validity_ori
    #   puts "false condition"
        board1.prompt_valid
        next
    end
    #puts index_start_x
    #puts index_start_y
    des_cor = board1.ask_for_des
    board_move_validity_des = board1.check_board_valid(des_cor,1)

    if !board_move_validity_des
        board1.prompt_valid
        next
    end

    array_ori = board1.convert(ori_cor)
    array_des = board1.convert(des_cor)

    start_x = array_ori[0]
    start_y = array_ori[1]

    # obtaining x,y index from the string input
    index_start_x = array_ori[2]
    index_start_y = array_ori[3]

    final_x = array_des[0]
    final_y = array_des[1]

    index_final_x = array_des[2]
    index_final_y = array_des[3]

    #p array_ori
    #p array_des


    intended_piece = board1.grid[index_start_x][index_start_y]
    #puts "can move" if intended_piece.can_move?(final_x,final_y)
    #puts piece_name   # pice name obtained
    #puts intended_piece
    piece_move_validity = intended_piece.can_move?(final_x,final_y)
        #puts piece_move_validity

    if piece_move_validity == true
        puts "can move"
#       des_status = board1.check_empty(index_final_x,index_final_y)
#       if des_status == "empty"
        #puts "start moving"
        board1.move(intended_piece,index_start_x,index_start_y,index_final_x,index_final_y)
#       puts "call move function"
#       else
# #             compare
#           puts "call compare function"
#       end
    else
        board1.prompt_valid
    end

    board1.show

end

Board.rb

class Board
    include Input

    attr_reader :grid
    def initialize

        @turn = 0

        bR = Rook.new(1,8,"black","B5")
        bK = Knight.new(2,8,"black","B4")
        bB = Bishop.new(3,8,"black","B3")
        bQ = Queen.new(4,8,"black","B2")
        bKing = King.new(5,8,"black","B1")
        bB2 = Bishop.new(6,8,"black","B3")
        bK2 = Knight.new(7,8,"black","B4")
        bR2 = Rook.new(8,8,"black","B5")

        wR = Rook.new(1,1,"white","W5")
        wK = Knight.new(2,1,"white","W4")
        wB = Bishop.new(3,1,"white","W3")
        wQ = Queen.new(4,1,"white","W2")
        wKing = King.new(5,1,"white","W1")
        wB2 = Bishop.new(6,1,"white","W3")
        wK2 = Knight.new(7,1,"white","W4")
        wR2 = Rook.new(8,1,"white","W5")

        bP1 = Pawn.new(1,7,"black","BP")
        bP2 = Pawn.new(2,7,"black","BP")
        bP3 = Pawn.new(3,7,"black","BP")
        bP4 = Pawn.new(4,7,"black","BP")
        bP5 = Pawn.new(5,7,"black","BP")
        bP6 = Pawn.new(6,7,"black","BP")
        bP7 = Pawn.new(7,7,"black","BP")
        bP8 = Pawn.new(8,7,"black","BP")

        wP1 = Pawn.new(1,2,"white","WP")
        wP2 = Pawn.new(2,2,"white","WP")
        wP3 = Pawn.new(3,2,"white","WP")
        wP4 = Pawn.new(4,2,"white","WP")
        wP5 = Pawn.new(5,2,"white","WP")
        wP6 = Pawn.new(6,2,"white","WP")
        wP7 = Pawn.new(7,2,"white","WP")
        wP8 = Pawn.new(8,2,"white","WP")

        @grid = [
            [bR,bK,bB,bQ,bKing,bB2,bK2,bR2],
            [bP1,bP2,bP3,bP4,bP5,bP6,bP7,bP8],
            ["00","00","00","00","00","00","00","00"],
            ["00","00","00","00","00","00","00","00"],
            ["00","00","00","00","00","00","00","00"],
            ["00","00","00","00","00","00","00","00"],
            [wP1,wP2,wP3,wP4,wP5,wP6,wP7,wP8],
            [wR,wK,wB,wQ,wKing,wB2,wK2,wR2],    
        ]


    end

    def check_empty(final_x,final_y)
        if @grid[final_x][final_y] == "00"
            "empty"
        else
            @grid[final_x][final_y]
        end
    end


    def show
        puts "Current Board Status:"
        @grid.each do |row|
            row.each do |element|
                if element == "00"
                    print "|#{element}|"
                else
                    print "|#{element.name}|"
                end
            end
            print "\n"
        end
    end
    def prompt_valid
        puts "Plz input valid coordinates for the piece"
        puts "-----------------------"
        puts "-----------------------"
        puts "X: 1,2,3,4,5,6,7,8"
        puts "Y: 1,2,3,4,5,6,7,8"
        puts "Sample input: 58 would be B1 / Black King(bKing)"
        puts "-----------------------"
        puts "-----------------------"
    end

    def move piece, index_start_x, index_start_y, index_des_x, index_des_y
        #puts index_start_x,index_start_y,index_des_x,index_des_y
        #puts "destination original value #{@grid[index_des_x][index_des_y]}"
        #puts "destination become #{@grid[index_des_x][index_des_y].name}"
        #puts "original is #{@grid[index_des_x][index_des_y].name}"
        @grid[index_des_x][index_des_y] = piece.clone
        @grid[index_des_x][index_des_y].start_x = index_start_y + 1
        @grid[index_des_x][index_des_y].start_y = 8 - index_start_x
        @grid[index_start_x][index_start_y] = "00"
        if @turn ==1
            @turn = 0
        elsif @turn == 0
            @turn = 1
        end

    end

    def compare

    end
end

Input.rb

module Input
    def ask_for_ori

        puts "-------------------------"
        puts "-------------------------"

        if @turn ==0
            puts "White chess turn"
        else 
            puts "Black chess turn"
        end

        puts "Input the coordinates of the piece you want to move"
        puts "-------------------------"
        puts "------------------------"
        input_cor = gets.chomp
    end

    def ask_for_des
        puts "-------------"
        puts "Input the coordinates of the destination"
        input_des = gets.chomp
    end


    def check_board_valid(input_cor, des = 0) # whether input is within the range
        array1 = input_cor.split("") 

        if des == 0
            if (@grid[8-(array1[1].to_i)][(array1[0].to_i)-1].name =~ /[W]/ ) && @turn == 0
                puts "first con"
                true
            elsif (@grid[8-(array1[1].to_i)][(array1[0].to_i)-1].name =~ /[B]/) && @turn ==1
                puts "second con"
                true
            else
                puts "It's not your turn yet piece of shit"
                return false
            end
        end


        #puts @grid[8-(array1[1].to_i)][(array1[0].to_i)-1]
         if (1..8).include?(array1[0].to_i) && (1..8).include?(array1[1].to_i) && array1.length ==2
            if @grid[8-(array1[1].to_i)][(array1[0].to_i)-1] == "00" && des ==0
                puts "This slot has an empty piece.pls try again"
                return false
            end
            return true

         else
            return false
         end

    end

    def convert input_cor
        start_x = input_cor.split("")[0].to_i
        start_y = input_cor.split("")[1].to_i

        # obtaining x,y index from the string input
        index_start_x = 8 - start_y
        #puts "after math equal #{index_start_x}"

        index_start_y = start_x -1
        array1 = start_x,start_y,index_start_x,index_start_y
        #p array1
        #array1
    end

end

For the sake of not making this page looks too horrifying, other source code is included in my git. Please check it out if you have the time.

\$\endgroup\$
2
\$\begingroup\$

First of all, some variable names can be more descriptive.

It looks like you're using tabs for indentation. It's convenient to use 2 spaces. It's convenience to use single quotes if there is no interpolation in the string.

Constructions like that one:

if @x_diff <= 1 && @y_diff <= 1
  true
else
  false
end

Can be simplified to:

@x_diff <= 1 && @y_diff <= 1

You're using @turn for some kind of "flag" with two possible states. Why not to use true/false? That piece of code can be rewritten in more concise way:

if @turn == 1
  @turn = 0
elsif @turn == 0
  @turn = 1
end

Like that:

@turn = false
# ...
@turn = !@turn # now 1 is true and false is 0.

"00" is used for "placeholder" of empty square. It will be easier to use false or nil. And use "00" only for rendered part. Updated @grid and Board#show can be rewritten in that way:

@grid = [
  [bR,bK,bB,bQ,bKing,bB2,bK2,bR2],
  [bP1,bP2,bP3,bP4,bP5,bP6,bP7,bP8],
  [nil] * 8,
  [nil] * 8,
  [nil] * 8,
  [nil] * 8,
  [wP1,wP2,wP3,wP4,wP5,wP6,wP7,wP8],
  [wR,wK,wB,wQ,wKing,wB2,wK2,wR2],  
]
#...

def show
  puts "Current Board Status:"
  @grid.each do |row|
    row.each { |element| print "|#{element ? element.name : '00'}|" }
    print "\n"
  end
end

Board#check_board_valid have bunch of responsibilities. It looks more like "functional-styled" code. Can be separated into simple methods:

def check_board_valid(input_cor, des = 0) # whether input is within the range
  input_cor = input_cor.split('').map(&:to_i)

  return unless coord_valid?(input_cor) # maybe add some message here

  figure = @grid[8 - input_cor.last)][input_cor.first - 1]

  return puts('This slot has an empty piece.pls try again') unless figure

  if des == 0 # this condition still can be simplified
    if whites_move?(figure)
      puts 'first con'
      true # probably, that line can be removed after overall refactoring
    elsif blacks_move?(figure)
      puts 'second con'
      true # probably, that line can be removed after overall refactoring
    else
      puts 'It\'s not your turn yet piece of shit'
      return false
    end
  end
end

private

def coord_valid?(input_cor)
  input_cor.size == 2 && input_cor.all? { |coord| (1..8).include?(coord) }
end

def whites_move?(figure)
  figure.is_white? && !@turn
end

def blacks_move?(input_cor)
  figure.is_black? && @turn
end

# at "lib/Chess.rb"
class Chess
#...
  def is_black?
    self.name =~ /[B]/
  end

  def is_white?
    self.name =~ /[W]/
  end
end

That's it for now. I hope, that you get the main idea.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you. This is immensely helpful! I was worried about calling too many methods inside one method before. That's why I give the function so many responsibilities so it doesn't have to depend on other things. But I just read that it's okay to call other methods in your class directly because it won't violate the principle of least knowledge. \$\endgroup\$ – whales Jun 20 '16 at 14:04
  • \$\begingroup\$ Glad I could help, @whales! \$\endgroup\$ – Sergii K Jun 20 '16 at 14:19

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.