3
\$\begingroup\$

I recently got a prompt when looking for a job. To spare any details, the company decided not to get back to me, so because I did not get a code-review/feedback from them, I was wondering if someone would review this and tell me how i could improve it.

Here is a link to the full prompt (pretty long) with sample input and expected output: https://docs.google.com/document/d/1QlJJcf7mr81602QPM_zgz15Ud5kalXSGxkiMjqO74os/edit?usp=sharing

The gist of it, is to write a script that manages a system's dependencies for handling the installing/uninstalling of various packages inside a system. In doing some digging around (after I turned it in), I found that this is not only a common interview question, but also is part of Cornell's CS program

I was pretty proud of myself for completing this, and right now, based on the prompt there are only 2 problems.

  1. I was unable to figure out the proper rendering (in order) of the (first) LIST command. However, in both spots where called, LIST returns all the correct elements

  2. The HTML component does not output in the REMOVE BROWSER call towards the end, but it does not list in the final LIST command

This is exactly the code I turned in with no modifications. Here is my code

class DependencyManager 
  attr_reader :name
  attr_accessor :dependencies
  def initialize(text_input)
    @text_input = text_input
    @dependencies = DependencyList.new
  end 

  def call
    @text_input.each_line do |line|
      line = line.gsub(/[^a-z0-9\s]/i, '')
      line_list = line.split(' ')
      if line_list[0] == 'DEPEND'
        puts "#{line_list.join(' ')}"
        depend(line_list)
      elsif line_list[0] == 'INSTALL'
        puts "INSTALL #{line_list[1]}"
        install(line_list)
      elsif line_list[0] == 'REMOVE'
        puts "REMOVE #{line_list[1]}"
        remove(line_list)
      elsif line_list[0] == 'LIST'
        puts "LIST"
        list(line_list)
      elsif line_list[0] == 'END'
        handle_end
      else
        print 'INVALID INPUT'
      end
    end
  end

  def depend(line_list)
    first = line_list.slice(0, 2)
    second = line_list.slice(2, line_list.length)
    dependency = Dependency.new(first[1])
    second.each do |depend|
      dep = Dependency.find_by_name(depend)
      if dep 
        dep.depends_on.list.push(dependency)
      else
        dep = Dependency.new(depend)
        dep.depends_on.list.push(dependency)
      end
    end 
    dependency.dependencies.list.concat(second)

    @dependencies.list.push(dependency)
  end

  def install(line_list)
    dep_name = line_list[1]
    elem = Dependency.find_by_name(dep_name)

    if elem 
      # already exists, just install item and all dependencies
      elem.dependencies.list.select { |dep| 
        Dependency.find_by_name(dep).installed == false 
      }.each { |dep| 
        Dependency.find_by_name(dep).installed = true ; 
        p "Installing #{dep}" 
      }
      if elem.installed == true 
        p "#{elem.name} is already installed."
      else 
        elem.installed = true 
        p "Installing #{elem.name}"
      end
    else
      dep = Dependency.new(dep_name, true)
      p "Installing #{dep.name}"
      elem = dep
    end 
    @dependencies.list.push(elem)
  end

  def remove(line_list)
    dep_name = line_list[1]
    elem = Dependency.find_by_name(dep_name)

    if elem.installed == false 
      return p "#{elem.name} is not installed."
    end

    installed_components = @dependencies.list.select { |dep| dep.installed == true }
    installed_components.each do |component|
      if component.dependencies.list.include?(elem.name)
        return p "#{elem.name} is still needed."
      end
    end

    # multi-level removals
    elem.installed = false 
    p "Removing #{elem.name}"
    elem.depends_on.list.each do |dep|
      dep = Dependency.find_by_name(dep)
      if dep
        dep.installed = false
        p "Removing #{dep}"
      end
    end
  end

  def list(line_list)
    new_list = []
    @dependencies.list.select { |dep| dep.installed == true }.each do |dep|
      new_list.push(dep.name)
      dep.dependencies.list.each do |inner_dep|
        new_list.push(inner_dep)
      end 
    end
    # output better?
    new_list = new_list.uniq
    new_list.each do |item|
      p "#{item}"
    end 
  end

  def handle_end
    puts "END"
  end
end 

class Dependency
  attr_reader :name, :id
  attr_accessor :installed, :dependencies, :depends_on

  @@id = 1
  @@all = []

  def initialize(name, installed = false)
    @name = name 
    @installed = installed
    # child
    @dependencies = DependencyList.new
    # parent
    @depends_on = DependencyList.new

    @id = @@id 
    @@id += 1

    @@all << self
  end

  # input: string
  # output: dependency (if exists) or false (if not exists)
  def self.find_by_name(name)
    elem = @@all.select { |dep| dep.name == name}
    if elem.empty?
      return false 
    else 
      return elem.pop
    end
  end
end 

class DependencyList
  attr_accessor :list

  def initialize
    @list = []
  end 
end

sample_input = <<-EOT
DEPEND   TELNET TCPIP NETCARD
DEPEND TCPIP NETCARD
DEPEND DNS TCPIP NETCARD
DEPEND  BROWSER   TCPIP  HTML
INSTALL NETCARD
INSTALL TELNET
INSTALL foo
REMOVE NETCARD
INSTALL BROWSER
INSTALL DNS
LIST
REMOVE TELNET
REMOVE NETCARD
REMOVE DNS
REMOVE NETCARD
INSTALL NETCARD
REMOVE TCPIP
REMOVE BROWSER
REMOVE TCPIP
LIST
END
EOT

dependency_manager = DependencyManager.new(sample_input)
dependency_manager.call 
\$\endgroup\$
  • \$\begingroup\$ The problem maps a graph data structure and a topological sort of graph would tell you which components to install in what order, with a dictionary to tell you whether it has been installed already or not. Your solution does not use a proper data structure suitable for the problem. \$\endgroup\$ – Satyendra Kumar Jul 25 at 12:22
1
\$\begingroup\$
  1. In call why do you reassemble string from parts instead just of echoing exact line from input?
  2. You should use Sets and Hashes, there should be no need to write find_by_name method. LIST should just list items of some installed Set, there should be no need for additional processing.
  3. Both install and remove probably should use recursion, it is natural way to process dependency graphs.
  4. I would expect handle_end to stop processing input, currently it does nothing.
\$\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.