Skip to content

Instantly share code, notes, and snippets.

@philsof
philsof / orange-trees-1-pair-san115,superboyblue.md
Last active January 26, 2016 06:18
Code review of Orange Tree 1 challenge for pair-san115,superboyblue branch

Good work! Here are my comments on your code:

Orange Tree class:

age! method: Right now, all of the oranges you create have a diameter of 3. If you want to keep it like this, you could make your code simpler by hard coding the diameter into your Orange class initialize method. Or (much better!), perhaps you could change this method so that each orange is created with a random diameter (like in real life). It would be cool if you did that! (And heads up: the runner.rb file assumes your oranges will have varying diameters. See my note on the runner.rb file below.)

Also heads up: Looks like line 25 will add oranges to a tree even if the tree is already dead (!). Uh oh! How could you rewrite your code to avoid this from happening?

Also you should utilize your attr_writer :dead method here. On line 19, instead of @dead = true you could utilize your attr method by writing this as self.dead=(true) (This samem concept also applies to all of your references to grown_oranges and `

@philsof
philsof / miniQuery-challenge-sasha-jeff.md
Last active January 26, 2016 06:18
Code review for pair-Sasha,Jeff for miniQuery-challenge

Good work on releases 0 and 1!

A few notes:

###It Works!###

  • Your code does everything that releases 0 and 1 ask for. Success!

###Formatting###

  • Your JavaScript indentation is perfect. That makes your code very easy to read, which is greatly appreciated. Readability is vital!
@philsof
philsof / pair-jesskrich-cookies-and-ovens-challenge.md
Last active January 26, 2016 06:18
Code review for pair-jesskrich on cookies-and-ovens-challenge

Good effort on this challenge! Here are some comments from me:

###Release 0: Move cookies into and out of the oven###

  • I can move cookies into your oven, but not out of it. Take a look at your ``remove_cookies``` method:

    def remove_cookies
      @contents.each do |cookie|
        if cookie.done == true
          @cooling_rack << cookie
@philsof
philsof / code-review-for-pair-kerryimai,yilu1021-scraping-hn-1-building-objects-challenge.md
Last active January 26, 2016 06:18
Code review for pair-kerryimai,yilu1021 branch: scraping-hn-1-building-objects-challenge

Your code works! Good job! Here are some notes:

  • Line 3 in your comment.rb file sets two attribute readers: attr_reader :comment_user_ID, :comment_text. These attribute names are redundant since they are attributes on your comment class. Removing the word 'comment' from the attribute names and simply naming them attr_reader :user_ID, :text would suffice.
  • Lines 12-14 in your post class can be refactored. Instead of this:
  def comments
    @collection_of_comments
  end

you could eliminate the need to write this method by naming your collection @comments and create an attr_reader for it: attr_reader :comments

@philsof
philsof / code-review-for-pair-dandersen2,edwardgemson-from-scraping-hn-1-building-objects-challenge.md
Last active January 26, 2016 06:18
Code review for pair-dandersen2,edwardgemson from scraping-hn-1-building-objects-challenge

This is really good stuff. I am going to get into nitty gritty to give you some items you could tweak:

  • Your Parsable module is statically referencing two instance variables that are being created outside this module: @nokogiri_document and @file. That is a no-no. You want your module to be written in such a way that it needs to know nothing about the rest of your code. Thus, a better way would be to pass the nokogiri document and the file to the module as arguments. As in other words, your Parsable module should be written in such a way that, if the names of these two instance variables were changed, there would be no need to change any of the code in your Parsable module.
  • The map_with_index on line 5 in your Parsable module is sweet. Nice work iterating over the DOM like this and delivering an array of comment objects to your post initialize method.
  • Heads up: Your to_s method on your post class only prints the state of the ```comment
@philsof
philsof / code-review-plonsker_and_kb-for-scraping-hn-1-building-objects-challenge.md
Last active January 26, 2016 06:18
code-review-plonsker_and_kb-for-scraping-hn-1-building-objects-challenge

Good effort on this! Here are my notes:

  • Your runner code (which appears to be in your parser.rb file) does not seem to be utilizing any of your post class code. It is not creating a post object or printing any post info.
  • There is code in your post initialize method that is most likely not doing what you think it is doing, right here. This is because of how your add_comment method is written (see next point).
  • In your post class you have a very interesting add_comment method, which is this:
def add_comment(comment)
  symbol_id = ("id" + comment.id.to_s).to_sym
  @comments[symbol_id] = comment
end
@philsof
philsof / code-review-pair-espezua,mattopps-scraping-hn-1-building-objects-challenge.md
Last active January 26, 2016 06:18
Code Review for pair-espezua,mattopps branch on scraping-hn-1-building-objects-challenge

Your code works! Good job!

Here are some notes from me:

  • I like the attribute naming on your post object, and the cleanness of your add_comments method, and the utilization of a to_s method on your post object, and the use of an args hash for your initialize arguments, and the cleanness of your encapsulating your comment objects inside your post object. Well done!
  • One of the attribute names on your comment class is a little redundant: comment.user_comment. Since we already know this is an attribute of a comment, we can name it something like comment.text or comment.content. (This attribute is the text or content of the comment. To say that it is the user_comment of the comment is too vague and redundant.)
  • [A lot of your runner code](https://github.com/nyc-chorus-frogs-2016/scraping-hn-1-building-objects-challenge/blob/pair-espezua%2Cmatto
@philsof
philsof / code-review-pair-mirascarvalone,superboyblue-scraping-hn-1-building-objects-challenge.md
Last active January 26, 2016 06:18
Code review for pair-mirascarvalone,superboyblue branch on scraping-hn-1-building-objects-challenge

Good effort! Here are my notes on your code:

  • I can't find any runner code in your branch. (There is a runner.rb file, but when I run it, nothing happens.) When I add my own runner code to your runner file (by adding the line puts post to the end of your runner file), and then run the file, I get this output:
A/B testing mistakes | Hacker News (Hacker News ID: 5003980)
    URL: http://visualwebsiteoptimizer.com/split-testing-blog/seven-ab-testing-mistakes-to-stop-in-2013/
    Author: ankneo
    Points: 53

    Comments:
@philsof
philsof / code-review-pair-jesskrich,san115-scraping-hn-1-building-objects-challenge.md
Last active January 26, 2016 06:18
Code review for pair-jesskrich,san115 branch on scraping-hn-1-building-objects-challenge

Good work!

##The good stuff:##

  • Very nice that you created separate to_s methods for your post class and comment class, and that your post class to_s utilizes the comment class to_s. Very sharp separation of responsibilities!
  • Very wise and clean to separate the parsing of the data and creating of the objects from the rest of your code. Also I like the names of your modules because they clearly convey what each module is doing: PostFactory and CommentFactory. These modules show you are grasping what it means to separate responsibilities.
  • Good use of map_with_index here

##Stuff to work on:##

@philsof
philsof / code-review-solokerry-roman-numerals-challenge.md
Last active January 26, 2016 06:18
code-review-solokerry-roman-numerals-challenge

Good work on this! Your code works, it successfully converts Arabic numbers into Roman numerals. Success!

Some notes:

  • This challenge is testing your ability to utilize hashes and the / and % operators, which you do well.

  • How could you better utilize these operators in your code? Here is a really short solution that further taps into the capabilities of / and %:

    def to_roman(num)
      roman_num = {1000 => "M", 900 => "CM", 500 => "D",400 => "CD",100 => "C",90 => "XC" ,50 => "L",40 => "XL", 10 => "X",9=>"IX", 5 => "V",4=>"IV",1 => "I" }

roman_num_string = ""