4
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ Isn't get going to return the node after the one with the match? \$\endgroup\$ – AShelly Mar 28 at 23:21
4
\$\begingroup\$

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
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.