# Path Finder Maze

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

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


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.