patternrubyMinor
Path Finder Maze
Viewed 0 times
mazepathfinder
Problem
The goal of my code was to find all possible moves from each open position in the maze (
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
@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:
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.
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
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.
gem install reekreek maze.rbYou 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.