Code review world-s-simplest-browser-challenge pair-dmandelb,plonsker 02-01-2016.md

code-review-world-s-simplest-browser-challenge-pair-dmandelb,plonsker.md

Good work! Per your desire to sharpen your MVC skills, here are some detailed notes on your code:

• You have a puts call in your Display class's initialize method. Instead, you are going to want your Controller to utilize your Display.look_at_this method to print this initial welcome text to the console:

  @viewer.look_at_this("Welcome to the World's Simplest Browser")

Or, you could write a specific Display method for this:

#Display class code:
def welcome
  puts "Welcome to the World's Simplest Browser"
end

#Controller code:
@viewer.welcome

• You have a gets.chomp assignment in your Display class. This should be in your Controller. So in your Controller you would have something like this:

  @viewer.look_at_this("What website would you like to visit today?")
  # or @viewer.site_prompt with only the 'puts' in the site_prompt method
  site = gets.chomp

• You have some of your Controller code in its initialize method, and some code outside of its initialize method. Better to just have one method that controls the program flow, and call that method in your Controller initialize method. So your Controller would end up looking like this:

  class Controller
    
    def initialize
      self.run
    end
    
    def run
      #controller code goes here
    end
  end

• You've got a lot of parsing happening in your Controller. This should be happening in your classes or modules. Specifically for this, it would be better to have a module that parses the data and delivers it to the Page model, so that all you would have in your Controller would be the calls to these methods, like this:

  # ParsingModule would receive the URL and return a hash 
  # from which Page.new could create a Page object.
  # So all of the parsing happens in the module
  @page = Page.new(ParsingModule.parse(@url)) 
  @viewer.look_at_this(@page)

• Nice work having a to_s method on your Page class. Even better: you realize that, when calling puts on an object with a to_s method, that Ruby will automatically call the to_s method on that object. (You realize you do not need to explicitly call to_s.)

• You don't need this send method. Just call your Display methods directly on your @viewer instance variable.

Any questions, let me know.

-Phil