I have created a LinkedList in Ruby and was wondering if anyone had any input on efficiencies that I could add or deficiencies I could remove.

module LinkedList

class List
attr_accessor :node

def initialize
self.node = nil
end

if self.node.nil?
self.node = node
else
current_node = self.node
while ! current_node.getPointer.nil? do
current_node = current_node.getPointer
end
current_node.setPointer node
end
end

def get(node)
current_node = self.node
data_match = nil
while !current_node.getPointer.nil? and ! data_match.nil? do
data_match = current_node.getData if node.getData == current_node.getData
current_node = current_node.getPointer
end
return current_node
end

def remove(node)
previous_node = nil
current_node = self.node
next_node = current_node.getPointer
while ! current_node.nil? do
if current_node.getData == node.getData and current_node.getData == self.node.getData
self.node = next_node
return true
end
if current_node.getData == node.getData
previous_node.setPointer next_node
return true
end
previous_node = current_node
current_node = next_node
next_node = current_node.getPointer

end
return false
end

def print
current_node = self.node
while ! current_node.nil? do
pointerData = current_node.getPointer.nil? ? nil : current_node.getPointer.getData
puts "data=#{current_node.getData}, pointer=#{pointerData}"
current_node = current_node.getPointer
end
puts
end

end

class Node
attr_accessor :data
attr_accessor :pointer

def initialize(data = nil, pointer = nil)
self.data = data
self.pointer = pointer
end

def getData
return self.data
end

def getPointer
return self.pointer
end

def setData(data)
self.data = date
end

def setPointer(node)
self.pointer = node
end

end

end

• Isn't get going to return the node after the one with the match? – AShelly Mar 28 at 23:21

Some things that immediately hit me were not about the implementation of the algorithm but the look of the code, it's not idiomatic Ruby, it looks like Java.

1. Most Rubyists use 2 spaces instead of 4.
2. Why all the getters and setters? Setters are quite often a code smell, make sure you really need them (you may do but if you can draw a line under an object's period of mutability - hopefully by the end of initialization - you'll do yourself a favour).
3. Camel case is the Java-y bit, so no setPointer and friends. pointerData would be pointer_data in idiomatic Ruby.
4. You only need def pointer for the getter and def pointer= val for the setter, or to use the attr_… helpers.
5. … and you've used the attr_ helpers, so why is there even a setPointer at all? Just use previous_node.pointer = next_node (to give one example) and cut out the middle man. Same goes for all the others, why write a getter/setter to wrap another getter/setter?
6. self.node = nil in the intializer is unnecessary and if you were going to do it you'd probably use @node = nil.
7. self.node = node, again, why not access the instance variable directly if you know there's no extra processing going on? @node = node is more idiomatic.
8. No need to write return unless you want an early return, everything's an expression so the last expression in a method will become its return value.
9. pointer_data = current_node.pointer.nil? ? nil : current_node.pointer.data - why use a ternary where an && or || will do? pointer_data = current_node.pointer && current_node.pointer.data is terser.
10. To be even more up to date and terse you could use the new safe-navigation operator, &., e.g. pointer_data = current_node.pointer&.data
11. You're returning true and false from methods that are changing state. Return either self or the particular state changed (take a look at what happens when you do [] << 1 or [1,2,3].delete 2 in a REPL). Methods that return true or false are usually suffixed with ? and side effects will probably surprise the consumer.

Once you've cleaned up all of that then the algorithm will be clearer. Some more thoughts on the code that are stylistic but not about Ruby per se.

Firstly, I'd allow the head node to be set in the initialize method - why make a 1 line operation into 2? Secondly, I'd be clearer in my naming, node is a bit abstract, if you're really talking about the position of a node - the head, then call it head:

def initialize head=nil # <- this makes it optional
@head = head # <- this takes care of setting to nil anyway


while ! current_node.pointer.nil? is the same as while current_node.pointer but the latter is much easier to understand. No one likes a double negative.

def initialize(data = nil, pointer = nil) Optional arguments are nice until you start using them in multiple positions, something like def initialize(data = nil, pointer: nil) is better, or def initialize(data: nil, pointer: nil), it depends on what you think is most likely to be used / easier on the consumer of the library - think about the interface all the time.

This brings me to my last point - where are the examples for running it? Where are the tests? I've pasted my own version using the advice I've given but does it work? I don't know. Any refactoring can (will!) break things which is why you need examples/tests to fall back on and check your work. It's also where you I'd advise you should start in future. If you don't know how you want the code to be called then you won't do a good job writing it.

I hope that helps.

module LinkedList

class List
attr_accessor :node

end

else
while current_node.pointer do
current_node = current_node.pointer
end
current_node.pointer = node
end
self
end

def get(node)
data_match = nil
while current_node.pointer and data_match do
data_match = current_node.data if node.data == current_node.data
current_node = current_node.pointer
end
current_node
end

def remove(node)
previous_node = nil
next_node = current_node.pointer
while current_node do
if current_node.data == node.data and current_node.data == @head.data
return node
end
if current_node.data == node.data
# This looks like it will fail as previous_node is nil afaics
previous_node.pointer = next_node
return node
end
previous_node = current_node
current_node = next_node
next_node = current_node.pointer
end
nil
end

def print
while current_node do
pointer_data = current_node.pointer&.data
puts "data=#{current_node.data}, pointer=#{pointer_data}"
current_node = current_node.pointer
end
puts # puts *what*?
end

end

class Node
attr_accessor :data
attr_accessor :pointer

def initialize(data: nil, pointer: nil)
@data = data
@pointer = pointer
end
end

end