Skip to content

Instantly share code, notes, and snippets.

Show Gist options
  • Save philsof/452d8570f1610afefbcc to your computer and use it in GitHub Desktop.
Save philsof/452d8570f1610afefbcc to your computer and use it in GitHub Desktop.
Code review world-s-simplest-browser-challenge pair-dmandelb,plonsker 02-01-2016.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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment