MichaelSid / Week4-Test

A test on reopening classes, the inject method, and gems
0 stars 0 forks source link

PASS #1

Open antonydenyer opened 10 years ago

antonydenyer commented 10 years ago

Filenames do not match class names

I think

result == self.first ? (self[1..-1].each {|x| result=yield(result, x)}) : (self.each {|x| result=yield(result, x)})

can be written as, make sure you fully understand what's going on here.

self.each {|x| result=yield(result, x)}

Fix your editor, be consistent with either tabs or space do not mix the two.

Good use of separation between text message and takeaway. Try and make the instance generic and the implementation specific. Imagine switching to a different text message provider would the variable name twilioClient make sense in the Takeaway class?

It's a small thing but naming is really important

Takeaway.new(TwilioClient.new)

class Takeaway
  def initialize(textMessageClient)
    ...
  end
end

Notice the constructor for the Takeaway class can take any kind of textMessageClient. In this case we provide it with an instance of TwillioClient.

Have a read of http://www.objectmentor.com/resources/articles/srp.pdf to reinforce understanding.

MichaelSid commented 10 years ago

Thank you! I don't really get your first point about re-writing this:

self.each {|x| result=yield(result, x)}

Would be great to talk about it later. Thanks

On Wed, Mar 19, 2014 at 9:11 AM, Antony Denyer notifications@github.comwrote:

Filenames do not match class names

I think

result == self.first ? (self[1..-1].each {|x| result=yield(result, x)}) : (self.each {|x| result=yield(result, x)})

can be written as, make sure you fully understand what's going on here.

self.each {|x| result=yield(result, x)}

Fix your editor, be consistent with either tabs or space do not mix the two.

Good use of separation between text message and takeaway. Try and make the instance generic and the implementation specific. Imagine switching to a different text message provider would the variable name twilioClient make sense in the Takeaway class?

It's a small thing but naming is really important

Takeaway.new(TwilioClient.new)

class Takeaway def initialize(textMessageClient) ... end end

Notice the constructor for the Takeaway class can take any kind of textMessageClient. In this case we provide it with an instance of TwillioClient.

Have a read of http://www.objectmentor.com/resources/articles/srp.pdf to reinforce understanding.

Reply to this email directly or view it on GitHubhttps://github.com/MichaelSid/Week4-Test/issues/1 .