4
\$\begingroup\$

The goal of my code was to find all possible moves from each open position in the maze (@moves_hash), assign it a value for how many moves it would take to get to that position (@distance_hash), then going backwards from the end_position pick moves that have the lowest distance back to the start point.

How can I make my code more readable and better formatted? I'm currently reading through Practical Object Oriented Design in Ruby, but am unsure how to apply certain principles from the book. Should I have created another class or are there other methods I should breakdown to have less dependency? What other types of ways could I make my code easier to understand, be reusable, and have less risk of bugs?

class Maze
  attr_reader :maze

  def initialize(maze)
    @maze = maze
    @current_position = find_start_point
    @start_position = find_start_point
    @end_position = find_end_point
    @moves_hash = Hash.new
    @distance_hash = Hash.new
    @moves = [method(:move_up),method(:move_right),method(:move_down),method(:move_left)]
    @allpossiblemoves = allpossiblemoves
  end

  def update_moves_hash(parent)
    possiblemoves = Array.new
    @moves.each do |x|
      nextmove = x.call(parent)
      possiblemoves << nextmove if possible_move?(nextmove)
    @moves_hash[parent] = possiblemoves
    end
  end

  def fill_moves_hash
    count = 0
    until @allpossiblemoves.count+1 == @moves_hash.keys.count
      @moves_hash.values.each do |y| 
        y.each {|x| update_moves_hash(x) if !@moves_hash.keys.include? y}
        y.each {|x| update_distance(x,count) }
      end
    count += 1
    end
  end

  def update_distance(pos,count)
    @distance_hash[pos] = count if @distance_hash[pos] == nil
  end

  def allpossiblemoves
    list = Array.new
    @maze.each_index do |x|
     @maze[x].each_index { |y| list << [x,y] if @maze[x][y] == " " }
    end
    list
  end

  def [](pos)
    row,col = pos
    @maze[row][col]
  end

  def []=(pos,val)
    row,col = pos
    @maze[row][col] = val
  end

  def find_end_point
    @maze.each_index do |x|
      @maze[x].each_index { |y| return [x,y] if @maze[x][y] == "E" }
    end
  end

  def find_start_point
    @maze.each_index do |x|
      @maze[x].each_index { |y| return [x,y] if @maze[x][y] == "S" }
    end
  end

  def possible_move?(pos)
    self[pos] == " " or self[pos] == "E" or self[pos] == "S"
  end

  def place_mark(pos,val)
    self[pos] = val
  end

  def move_up(pos)
    [pos[0]-1,pos[1]]
  end

  def move_down(pos)
    [pos[0]+1,pos[1]]
  end

  def move_right(pos)
    [pos[0],pos[1]+1]
  end    

  def move_left(pos)
    [pos[0],pos[1]-1]
  end

  def game_over?
    @moves.each { |x| return true if self[x.call(@start_position)] == "X" } 
    false
  end

  def display
    y = @maze.map {|x| x.join("")}
    y.each.with_index {|a,b| p "#{b} #{a}"}
  end

  def find_shortest(arr)
    distance = 999
    move = Array.new
    arr.each {|x| move,distance = x, @distance_hash[x] if @distance_hash[x] < distance}
    move
  end

  def find_next_to_end(pos)
    @moves.each do |x|
      return x.call(pos) if possible_move?(x.call(pos))
    end
  end

  def find_move(parent)
    moves= Array.new
    @moves_hash.each { |x,y| moves << x if y.include? parent} 
    find_shortest(moves)
  end

  def play_turn
    nextmove = find_move(@current_position)
    place_mark(nextmove,"X")
    @current_position = nextmove
  end

  def setup
    update_moves_hash(@current_position)
    fill_moves_hash
    @current_position = @end_position
    until game_over?
      play_turn
      display
    end
  end

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

My first recommendation is to start using reek. If you use atom as an editor, then integrate them via it's linter package. if not, it has a command line tool:

gem install reek

reek maze.rb

You will then see 39 issues with your code. Below is a sample of the output. each item has the line numbers associated and links to pretty good documentation about why it's bad and how to fix it. Fix all of these issues first, and repost. Then we can start looking at possible abstractions.

maze.rb -- 39 warnings:
 [41]:UncommunicativeVariableName: Maze#allpossiblemoves has the variable name 'x' [https://github.com/troessner/reek/blob/master/docs/Uncommunicative-Variable-Name.md]
 [42, 42]:DuplicateMethodCall: Maze#allpossiblemoves calls '@maze[x]' 2 times [https://github.com/troessner/reek/blob/master/docs/Duplicate-Method-Call.md]
 [1]:IrresponsibleModule: Maze has no descriptive comment [https://github.com/troessner/reek/blob/master/docs/Irresponsible-Module.md]
 [42]:NestedIterators: Maze#allpossiblemoves contains iterators nested 2 deep [https://github.com/troessner/reek/blob/master/docs/Nested-Iterators.md]
 [59]:NestedIterators: Maze#find_end_point contains iterators nested 2 deep [https://github.com/troessner/reek/blob/master/docs/Nested-Iterators.md]
 [65]:NestedIterators: Maze#find_start_point contains iterators nested 2 deep [https://github.com/troessner/reek/blob/master/docs/Nested-Iterators.md]
 [36]:NilCheck: Maze#update_distance performs a nil-check [https://github.com/troessner/reek/blob/master/docs/Nil-Check.md]
 [1]:TooManyInstanceVariables: Maze has at least 8 instance variables [https://github.com/troessner/reek/blob/master/docs/Too-Many-Instance-Variables.md]
 [1]:TooManyMethods: Maze has at least 22 methods [https://github.com/troessner/reek/blob/master/docs/Too-Many-Methods.md]
 [24]:TooManyStatements: Maze#fill_moves_hash has approx 7 statements [https://github.com/troessner/reek/blob/master/docs/Too-Many-Statements.md]
 [77]:UtilityFunction: Maze#move_up doesn't depend on instance state (maybe move it to another class?)     [https://github.com/troessner/reek/blob/master/docs/Utility-Function.md]

Make a rule for yourself that you don't check in anything that doesn't pass reek 100%. If there's a rule you disagree with, disable it in the .reek file in the project root.

After reek, do the exact same thing with Rubocop. These are more repetitive and easier to fix than the reek ones. with both of these passing 100%, you'll be surprised how much cleaner your code looks.

\$\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.