HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyMinor

Path Finder Maze

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
mazepathfinder

Problem

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

Solution

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.

Code Snippets

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]

Context

StackExchange Code Review Q#155813, answer score: 2

Revisions (0)

No revisions yet.