Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Panda Assignment submission. #20

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

codeblahblah
Copy link

Created a panda.rb file in the Models directory.

@@ -0,0 +1,13 @@
class Automobile
attr_accessor :color, :make, :model, :year
def self.wheels?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a method that ends in "?" should return a true/false

@jwo
Copy link
Member

jwo commented Feb 4, 2014

Good first step at a Panda submission -- let me know if you have any questions on it, and be sure to ping me "@jwo" when you push new code for this pull request.

@codeblahblah
Copy link
Author

Gotcha.
Will re-attempt it.
How does pinging on Github work?

On 4 February 2014 17:19, Jesse Wolgamott notifications@github.com wrote:

Good first step at a Panda submission -- let me know if you have any
questions on it, and be sure to ping me "@jwo https://github.com/jwo"
when you push new code for this pull request.

Reply to this email directly or view it on GitHubhttps://github.com//pull/20#issuecomment-34069375
.

"When you are tired of being yourself then you can be ordinary." - dudu

@jwo
Copy link
Member

jwo commented Feb 4, 2014

You should type “@jwo"

On Tuesday, February 4, 2014 at 10:21 AM, drammopo wrote:

Gotcha.
Will re-attempt it.
How does pinging on Github work?

On 4 February 2014 17:19, Jesse Wolgamott <notifications@github.com (mailto:notifications@github.com)> wrote:

Good first step at a Panda submission -- let me know if you have any
questions on it, and be sure to ping me "@jwo https://github.com/jwo"
when you push new code for this pull request.

Reply to this email directly or view it on GitHubhttps://github.com//pull/20#issuecomment-34069375
.

"When you are tired of being yourself then you can be ordinary." - dudu


Reply to this email directly or view it on GitHub (#20 (comment)).

@codeblahblah
Copy link
Author

Corrections made.

@jwo
Copy link
Member

jwo commented Feb 4, 2014

Looks good. The only thing missing is:

I should be able to pass in a hash of color, make, model, and year to the class to update its variables

So, I should be able to:

auto = Automobile.new(x,y,z)
auto.update(a,b,c)

@codeblahblah
Copy link
Author

Added update method to the class.

p auto
auto.update(color: 'Blue', year: 2012)
puts auto.color
puts auto.year
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does purs auto.model output? Is it what you expect?

@codeblahblah
Copy link
Author

puts auto.model returns nil.
Not what I expected.
Changed code to @model ||= new_args[:model]
Alternatively could have used @model = new_args[:model] unless @model.
Which is 'better' syntax?

@jwo
Copy link
Member

jwo commented Feb 4, 2014

@model ||= new_args[:model] is dependent on the state of @model
@model = new_args[:model] unless @model will never override @model

What you want to do is overwrite, if the arg is there, otherwise you leave it. This is the most common form of that expression

@model = new_args[:model] if new_args[:model]

You could also use fetch with a default to achieve a cleaner, but possibly harder to understand look:

@model = new_args.fetch(:model, @model)

@codeblahblah
Copy link
Author

For now I'll use:

@model = new_args[:model] if new_args[:model]

The correction has been made.


def update(new_args)
@color = new_args[:color] if new_args[:color]
@make ||= new_args[:make] if new_args[:make]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why use "||=" ?

@codeblahblah
Copy link
Author

Well spotted.
Fixed.
Thanks.

@jwo
Copy link
Member

jwo commented Feb 4, 2014

Awesome 🎉 !

@codeblahblah
Copy link
Author

@jwo Automobiles are not counted when created? All other types are. Why is that?

@jwo
Copy link
Member

jwo commented Feb 6, 2014

Oh, should have caught that. Call "super" in your subclass, and it'll call the initialize in your superclass. So when auto is initialized, if you call super, it will call initialize of Vehicle as well.

On Thu, Feb 6, 2014 at 11:39 AM, drammopo notifications@github.com
wrote:

@jwo Automobiles are not counted when created? All other types are. Why is that?

Reply to this email directly or view it on GitHub:
#20 (comment)

@codeblahblah
Copy link
Author

I had that in my original code and but got:
eagle.rb:3:in `initialize': wrong number of arguments (1 for 0) (ArgumentError)

See snippet below

  def initialize(args)
    @color = args[:color]
    @make = args[:make]
    @model = args[:model]
    @year = args[:year]
    super
  end

@jwo
Copy link
Member

jwo commented Feb 6, 2014

Ahh try 

super()

That's tell ruby to only call it with 0 arguments.

On Thu, Feb 6, 2014 at 11:55 AM, drammopo notifications@github.com
wrote:

I had that in my original code and but got:
eagle.rb:3:in `initialize': wrong number of arguments (1 for 0) (ArgumentError)
See snippet below

  def initialize(args)
    @color = args[:color]
    @make = args[:make]
    @model = args[:model]
    @year = args[:year]
    super
  end

Reply to this email directly or view it on GitHub:
#20 (comment)

@codeblahblah
Copy link
Author

Working now.
Thanks.

@jwo
Copy link
Member

jwo commented Feb 7, 2014

It would seem that the following are not needed

 Vehicle.register(self)
 Automobile.build_car(args)

And you don't need both a build and a build_car method, right?

@jwo
Copy link
Member

jwo commented Feb 7, 2014

To clarify, this is what I'm after:

class Automobile
 def initialize(args)
    @color = args[:color]
    @make = args[:make]
    @model = args[:model]
    @year = args[:year]
    Vehicle.register(self)
    Automobile.build_car(args)
    super()
  end

  def self.build(args)
    auto = Automobile.build(args)
    @@vehicles << auto
    auto
  end

end

describe Automobile do
  it "records the auto when I build it" do
    auto = Automobile.build
    expect(Automobile.all_autos).to include?(auto)
  end
end

@codeblahblah
Copy link
Author

@jwo Please expand on the Autobile.all_autos method.
Is it similar to the Vehicle.count in the sense that each Auto created will be sent to a @@atuos variable?
In which case you are checking that Array via the include?(auto) statement

Also can you call self.build on itself as per your example above?

  def self.build(args)
    auto = Automobile.build(args)
    @@vehicles << auto
    auto
  end

@jwo
Copy link
Member

jwo commented Feb 7, 2014

Yes it's similar. 

 Also can you call self.build on itself as per your example above?

Yes, why do you think you couldn't?

On Fri, Feb 7, 2014 at 1:37 PM, drammopo notifications@github.com wrote:

@jwo Please expand on the Autobile.all_autos method.
Is it similar to the Vehicle.count in the sense that each Auto created will be sent to a @@atuos variable?
In which case you are checking that Array via the include?(auto) statement
Also can you call self.build on itself as per your example above?

  def self.build(args)
    auto = Automobile.build(args)
    @@vehicles << auto
    auto
  end

Reply to this email directly or view it on GitHub:
#20 (comment)

@codeblahblah
Copy link
Author

@jwo Hmmmm. I see the error in my thinking.
As per the code below, there's no need for a #register as we do so via @@vehicles << auto
I'm still stuck on #build. Does it take the current instance and send it forward? i.e the self that was created during the Automobile.new?
In which case you cannot call Automobile.build before Automobile.new? Which kinda makes sense in the real world...

class Automobile
 def initialize(args)
    @color = args[:color]
    @make = args[:make]
    @model = args[:model]
    @year = args[:year]
    Automobile.build(args)
    super()
  end

  def self.build(args)
    auto = Automobile.build(args)
    @@vehicles << auto
    auto
  end
end

describe Automobile do
  it "records the auto when I build it" do
    auto = Automobile.build
    expect(Automobile.all_autos).to include?(auto)
  end
end

@jwo
Copy link
Member

jwo commented Feb 7, 2014

Here is what I want in this discussion. You don't register when you call Automobile.new

class Automobile
 def initialize(args)
    @color = args[:color]
    @make = args[:make]
    @model = args[:model]
    @year = args[:year]
    super()
  end

You register if someone calls Automobile.build

That makes Automobile.build your API endpoint, and leaves Automobile.new for your internal stuff.

In the same way, in Rails, you can call User.create if you'd like to, which would "register" (save to the database) the user being created.

@codeblahblah
Copy link
Author

@jwo
Gotcha.
The difference comes in that you can call User.count immediately after User.create because a save to the database has occurred.
In our case you'd have to:
Automobile.new(model: 'Mustang', color: 'Red', make: 'Ford', year: 2007) then Automobile.build because there's no middle-man for storage.

The latest code is up.
Please have a look at automobile_spec.rb as it fails on account of it expecting arguments whereas you examples above do not.

@jwo
Copy link
Member

jwo commented Feb 7, 2014

here's what I'd love to see

Automobile.count
=> 0
Automobile.build(model: 'Mustang', color: 'Red', make: 'Ford', year: 2007)
Automobile.count
=> 1

@codeblahblah
Copy link
Author

@jwo
Aside from the obvious, how is Automobile.build(model: 'Mustang', color: 'Red', make: 'Ford', year: 2007) different from Automobile.new(model: 'Mustang', color: 'Red', make: 'Ford', year: 2007)

@jwo
Copy link
Member

jwo commented Feb 7, 2014

Well, this is an exercise in showing the difference between class methods and initialization methods. 

In a production system, you don't want an initializer to have side effects. You should be able to create objects without effects. Then, you can call different methods to persist or otherwise affect them.

On Fri, Feb 7, 2014 at 3:41 PM, drammopo notifications@github.com wrote:

@jwo

Aside from the obvious, how is Automobile.build(model: 'Mustang', color: 'Red', make: 'Ford', year: 2007) different from Automobile.new(model: 'Mustang', color: 'Red', make: 'Ford', year: 2007)

Reply to this email directly or view it on GitHub:
#20 (comment)

@codeblahblah
Copy link
Author

@jwo
Made this change and it works now

  def self.build(args)
    auto = Automobile.new(args)
    @@vehicles << auto
    @@auto << auto
    auto
  end

@jwo
Copy link
Member

jwo commented Feb 7, 2014

awesome!

On Fri, Feb 7, 2014 at 3:47 PM, drammopo notifications@github.com wrote:

@jwo
Made this change and it works now

  def self.build(args)
    auto = Automobile.new(args)
    @@vehicles << auto
    @@auto << auto
    auto
  end

Reply to this email directly or view it on GitHub:
#20 (comment)

@codeblahblah
Copy link
Author

@jwo Used the Rails operation(s) all the time without understanding what was going on under the hood. Thanks for this!

@jwo
Copy link
Member

jwo commented Feb 7, 2014

Used the Rails operation(s) all the time without understanding what was going on under the hood. Thanks for this!

That is literally the coolest, best compliment you can give me for this course 🎉 😄

Do you approve for me to use it in a testimonial?

@codeblahblah
Copy link
Author

Definitely.
Provided I can see where it's being posted first.

Midnight all ready. Gotta catch some Zzzz.
Cheers

@codeblahblah
Copy link
Author

@jwo I made a pun related to cars without realising.
I've created my first test suite.
What do you think?



before :each do
@auto = Automobile.new(model: 'Mustang', color: 'Red', make: 'Ford', year: 2007)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather you use "let" instead of @auto.

Instead of

before :each do
  @auto = Automobile.new(model: 'Mustang', color: 'Red', make: 'Ford', year: 2007)
end

I would

let(:auto) { Automobile.new(model: 'Mustang', color: 'Red', make: 'Ford', year: 2007) }

Then later,

auto.should be_an_instance_of Automobile

@jwo
Copy link
Member

jwo commented Feb 11, 2014

Test suite looks most excellent!

I think you have this down (and also the longest pull request ever :) )

@codeblahblah
Copy link
Author

@jwo Awesome!
Had my first Ah-ha moment during the exercise which lead me to http://betterspecs.org/
Will make the changes as suggested.

@codeblahblah
Copy link
Author

@jwo Am I testing the Vehicle#search correctly?

@jwo
Copy link
Member

jwo commented Feb 11, 2014

Can you point me to where you are testing it?

@codeblahblah
Copy link
Author

@jwo /spec/vehicle_spec.rb

@jwo
Copy link
Member

jwo commented Feb 11, 2014

Going to say no, vehicle_spec looks extremely bare.

@codeblahblah
Copy link
Author

@jwo Apologies.
Had a commit issue.
Please have a look now.

Automobile.build model: :toyota, color: :red, make: 'Corolla', year: 2010
auto = Automobile.build model: :honda, color: :blue, make: 'Civic', year: 2013
Automobile.build model: :mazda, color: :blue, make: 'Accord', year: 2003
Vehicle.search(color: "blue", model: "honda").first.should eq(auto)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a fine search. It doesn't cover all functionality though.

I might like it if it were the following, where you are more assertive about the results.

Vehicle.search(color: "blue", model: "honda").should eq([auto])

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants