Salesperson assignment submission

[codeblahblah]

@jwo Completed the Panda Level.
Had to comment out the tests below as they used stubs:
Pending:
SalesPerson should have many cities
# Temporarily disabled with xit
# ./spec/sales_person_spec.rb:7
SalesPerson should keep the cities only scheduled once
# Temporarily disabled with xit
# ./spec/sales_person_spec.rb:13
Is this approach correct?
New code:

  def schedule_city(city)
    @cities << city unless @cities.include?(city)
    @cities.each_with_index do |city, index|
      if city.starting_point == true
      @cities.insert(0, cities.delete_at(index))
      end
    end
  end

[codeblahblah]

@jwo Created a separate file for the Benchmarking named salesperson_bm.rb

[codeblahblah]

@jwo The route distance calculation is working however I am "losing" the original city schedule after the CalculateRoute#route invocation.
I couldn't figure out why. Help!
In summary:

#puts phil.cities.inspect => [#<Place:0x007fd3e2bbf838 @name="Austin, TX", @coordinates=[30.267153, -97.7430608], @starting_point=true>, #<Place:0x007fd3e2bcee78 @name="Dallas, TX", @coordinates=[32.7801399, -96.80045109999999], @starting_point=false>, #<Place:0x007fd3e2bc7880 @name="El Paso, TX", @coordinates=[31.7699559, -106.4968055], @starting_point=false>, #<Place:0x007fd3e2bb66e8 @name="Lubbock, TX", @coordinates=[33.5778631, -101.8551665], @starting_point=false>]
puts phil.route
puts phil.distance_of_route
#puts phil.cities.inspect => []

[codeblahblah]

@jwo It seems the route << remaining_points.slice!(remaining_points.index(next_point)) mutates the original obj. due to bang call.
I'm using remaining_points = points.clone in order to keep the original routes untouched.

  def self.calculate(points)
    remaining_points = points.clone
    route = []
    route << remaining_points.slice!(0)
    until remaining_points == [] do
      next_point = shortest_distance(route.last, remaining_points)
      route << remaining_points.slice!(remaining_points.index(next_point))
    end
    route
  end

[jwo]

I haven't looked into this fully, but yes, slice! will "mutate" (change) the object it's called on. This convention in ruby tells you that something is about to happen.

Witness another example:

word = "Sheep"
word.gsub!("ee", "o")
=> "Shop"
puts word
"Shop"

word = "Sheep"
word.gsub("ee", "o")
=> "Shop"
puts word
"Sheep"

You'll see this all over ruby.

[codeblahblah]

@jwo Gotcha!

[codeblahblah]

@jwo Some questions...

Can the starting point method be refactored or simplified?

  def schedule_city(city)
    cities << city unless cities.include?(city)
    cities.each_index do |index|
      cities.insert(0, cities.delete_at(index)) if cities[index].starting_point == true   
    end
    cities
  end

Anytime I see an IF stament in a 'each' loop, I perceive it to be a good smell?

When would you use variables versus a Hash for object initialization e.g.

class Place
  def self.build(name, starting_point = false)
    @starting_point ||= starting_point
  end
end

class Place
  def self.build(args)end
  @starting_point = args.fetch(starting_point, false)
end

Benchmarking code:
Can this be bettered?

def run_route(n)
  salesperson = SalesPerson.new

  all_cities = ["Abilene", "Alamo", "Alamo Heights", "Alice", "Allen", "Alpine", "Alvarado", "Alvin",.....]

  all_cities.shuffle.take(n).each do |city|
      salesperson.schedule_city(Place.build("#{city}, TX"))
  end
end

Benchmark.bm do |x|
  x.report("Benchmarking 2 city route:"){ run_route(2)}
  x.report("Benchmarking 10 city route:"){ run_route(10)}
  x.report("Benchmarking 50 city route:"){ run_route(50)}
  x.report("Benchmarking 200 city route:"){ run_route(100)}
end

RSpec:
Why do these specs fail?

  xit "should have many cities" do
    city = stub
    subject.schedule_city(city)
    subject.cities.should include(city)
  end

  xit "should keep the cities only scheduled once" do
    city = double
    expect{
      subject.schedule_city(city)
      subject.schedule_city(city)
    }.to change(subject.cities,:count).by(1)
  end

How would you convert 28.83 hours to a more readable human format? Are there any Gems for such? When does one decide to go use a Gem versus hack their own solution?

[jwo]

This does seem very complicated. I recommend you rewrite into english, and have the methods do those things.

cities.each_index do |index|
  cities.insert(0, cities.delete_at(index)) if cities[index].starting_point == true   
end

If you are looking to have the cities without the starting point, this could be useful:

cites.reject{|c| c.starting_point}

[jwo]

Anytime I see an IF stament in a 'each' loop, I perceive it to be a good smell?

You probably mean "code smell", which is bad. I agree.

[jwo]

When would you use variables versus a Hash for object initialization e.g.

Good Guidline to follow: You can send 0, 1, or 2 variables to any method, including initialization. Any more than that, use a hash.

[jwo]

For converting 28.83 hours -- check out these:

1. http://www.ruby-doc.org/stdlib-2.1.1/libdoc/time/rdoc/Time.html (specifically Additional Notes)
2. http://stackoverflow.com/questions/2310197/how-to-convert-270921sec-into-days-hours-minutes-sec-ruby

[jwo]

BTW, code overall is really good

[jwo]

This will make your tests pass:

-  xit "should have many cities" do
-    city = stub
+  it "should have many cities" do
+    city = double(:city, starting_point: false)

You could tell this because the failure message told you:

     Failure/Error: subject.schedule_city(city)
       Double received unexpected message :starting_point with (no args)

[codeblahblah]

@jwo Thanks for the feedback and encouragement.
I will work on the refactoring.

Side note: I'm working on the Gem assignment and get a load path error when
I define my gem as about-drammopo but don't get the same error when I
define it as about_drammopo. Why is that? I've seen gems with
dashes/hyphens in their naming? Also, Jewelers structuring and naming of
gems seems to be opposite that which is advised at Rubygems?

On Wednesday, March 5, 2014, Jesse Wolgamott notifications@github.com
wrote:

This will make your tests pass:

• xit "should have many cities" do
• city = stub
• it "should have many cities" do
• city = double(:city, starting_point: false)

You could tell this because the failure message tnold you:

 Failure/Error: subject.schedule_city(city)
   Double received unexpected message :starting_point with (no args)

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

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

[jwo]

"about-drammopo" -- means drammopo is a gem that is a plugin to the about gem
"about_drammopo" -- mean there is a gem AboutDrammopo.

You want the second.

As far as "jeweler" -- not used much anymore.

More: http://guides.rubygems.org/name-your-gem/

[codeblahblah]

@ jwo I'm stuck here:

cities.each_index do |index|
  cities.insert(0, cities.delete_at(index)) if cities[index].starting_point == true   
end

Using your cites.reject{|c| c.starting_point} clue, I think I can reject the starting point and #unshift it to the front of the Array?

This is the code:

  def schedule_city(city)
    cities << city unless cities.include?(city)
    starting_city = city if city.starting_point == true
    cities.reject!{|c| c.starting_point}
    cities.unshift(starting_city)
    cities
  end

But I get a NilClass error below:
sales_person.rb:17:in block in schedule_city': undefined methodstarting_point' for nil:NilClass (NoMethodError)

[jwo]

It's probably best to isolate the problem; it can illuminate issues that seem hard to find. 

The error you entered looks like it has a nil in the array - can you prove/disprove this?

On Mon, Mar 10, 2014 at 2:35 PM, drammopo notifications@github.com
wrote:

@ jwo I'm stuck here:

cities.each_index do |index|
  cities.insert(0, cities.delete_at(index)) if cities[index].starting_point == true   
end

Using your cites.reject{|c| c.starting_point} clue, I think I can reject the starting point and #unshift it to the front of the Array?
This is the code:

  def schedule_city(city)
    cities << city unless cities.include?(city)
    starting_city = city if city.starting_point == true
    cities.reject!{|c| c.starting_point}
    cities.unshift(starting_city)
    cities
  end

But I get a NilClass error below:

sales_person.rb:17:in block in schedule_city': undefined methodstarting_point' for nil:NilClass (NoMethodError)

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

[codeblahblah]

@jwo Isolating the problem worked! I wrote it down in simplified English and it became clearer.
Note: I'm using the unrouted variable to keep track of the original "route".

  def schedule_city(city)
    unrouted_cities << city unless unrouted_cities.include?(city)
    cities << city unless cities.include?(city)
    starting_city = city if city.starting_point
    cities.reject{|c| c.starting_point}
    cities.unshift(starting_city) unless (cities.include?(starting_city) || starting_city.nil?)
  end

Pity you cannot multiple assign arrays thus my:

    unrouted_cities << city unless unrouted_cities.include?(city)
    cities << city unless cities.include?(city)

[jwo]

AWESOME!

    unrouted_cities << city unless unrouted_cities.include?(city)
    cities << city unless cities.include?(city)

Well, let's see what we can do. If we want to send an item into two arrays, maybe we could:

[unrouted_cities, cities].each{|a| a << city} unless cities.include?(city)

That might read better... what do you think?

[codeblahblah]

@jwo GREAT!
Reads much, much better. I really like it. Feels more like Ruby :)