LinkedList Implementation

Question

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
 def add(node)
 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

• AShelly: Isn't get going to return the node after the one with the match?

Answer 1

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
 def initialize head=nil
 @head = head
 end
 def add(node)
 if @head.nil?
 @head = node
 else
 current_node = @head
 while current_node.pointer do
 current_node = current_node.pointer
 end
 current_node.pointer = node
 end
 self
 end
 def get(node)
 current_node = @head
 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
 current_node = @head
 next_node = current_node.pointer
 while current_node do
 if current_node.data == node.data and current_node.data == @head.data
 @head = next_node
 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
 current_node = @head
 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