Add shops and shop_inventory_items #6

Open
wants to merge 39 commits into
from

4 participants

@mlapeter

Hey Mark,

We've added shops and shop_inventory_items. Next we're planning to clean up the views so a player can view shops and items in shops. We left out quantity because you didn't mention it, if that's something you want us to tackle at this stage let us know. We're also planning to add a materials table in the future to deal with normalizing materials, does that make sense?

Thanks!

@mlapeter

We've added wrapper methods to shop for the shop_inventory_items join table, should be ready for review and merge. Thanks!

@mlapeter

We've also now added buy and sell actions to the shop model and on the view.

@mlapeter

We finished the buy and sell methods for shops. Had a philosophical question about whether to use exceptions or soft errors for the buy and sell methods. We had one case where it was returning false at the end of the method but still silently updating records.

@duncanjmiller

Hi Mark - this branch is ready to be reviewed for merge when you have a chance. Thanks!

@marktabler marktabler was assigned May 16, 2013
@marktabler marktabler commented on an outdated diff May 16, 2013
app/models/shop.rb
@@ -0,0 +1,73 @@
+class Shop < ActiveRecord::Base
+ attr_accessible :location_id, :name
+
+ belongs_to :location, :dependent => :destroy
+
+ has_many :shop_inventory_items
+ has_many :goods, :through => :shop_inventory_items
+
+ def find_or_create(good)
@marktabler
marktabler added a line comment May 16, 2013

I see what we're doing here, but the name is close to some existing Rails Magic and might cause confusion. How about def ensure_stocked(good) ? Or something like that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@marktabler marktabler commented on an outdated diff May 16, 2013
app/models/shop.rb
+ def find_or_create(good)
+ if item = shop_inventory_items.find_by_good_id(good.id)
+ item
+ else
+ ShopInventoryItem.create!(shop_id: self.id, good_id: good.id, quantity: 0)
+ #mark review - we have to set quantity at 0 so its not nil, should we set the default to 0 in the migration?
+ end
+ end
+
+ def stock(good, buy_price = nil, sell_price = nil, quantity)
+ item = find_or_create(good)
+ updated_quantity = item.quantity + quantity
+ item.update_attribute(:buy_price, buy_price) if buy_price
+ item.update_attribute(:sell_price, sell_price) if sell_price
+ item.update_attribute(:quantity, updated_quantity)
+ #mark review - the idea here is we may want to restock the item without setting a new buy and sell price - does this make sense?
@marktabler
marktabler added a line comment May 16, 2013

The idea is definitely sound, yeah. Couple tweaks to execution: we don't want to default arguments in the middle of a list, we have to default arguments at the end. To illustrate what I mean, consider "stock(:fuel, 40)" to stock 40 units of fuel, versus "stock(:fuel, 40, 80, 120)", which stocks not 40 but 120 units of fuel. I think the Ruby parser will handle this OK, but it's definitely hard to follow.

I think it might be worth breaking this into two methods:

def stock(good, quantity, buy_price, sell_price)
  item = find_or_create(good)
  item.update_attributes(buy_price: buy_price, sell_price: sell_price, quantity: quantity)
end

def restock(good, additional_quantity)
  item = find_or_create(good)
  item.update_attributes(quantity: item.quantity + additional_quantity)
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@marktabler marktabler commented on an outdated diff May 16, 2013
app/models/shop.rb
+ end
+
+ def trades_in?(good)
+ shop_inventory_items.find_by_good_id(good.id)
+ end
+
+ def has_stock_of?(good)
+ shop_inventory_items.find_by_good_id(good.id) && shop_inventory_items.find_by_good_id(good.id).quantity > 0
+ end
+
+ def valid_quantity?(good, quantity)
+ shop_inventory_items.find_by_good_id(good.id) && quantity_of(good) >= quantity
+ end
+
+ # mark review - we can't decide on raising errors or soft fail for these kinds of methods.
+ def sell(character, good, quantity)
@marktabler
marktabler added a line comment May 16, 2013

Depends on your use case, but this is probably a hard error. Consider the process by which a character might buy a good from a shop: the interface will probably be doing a bunch of checks beforehand, right? Like, I shouldn't be able to select a good for purchase that's not in the shop list. I shouldn't be able to select more of a good than they have for sale. I probably shouldn't even be able to select more of the good than I can afford. By the time I'm ready to invoke the sell method, I'm probably pretty confident in what I'm doing - it should Go Forth, and bitch loudly if it doesn't work. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@marktabler marktabler commented on an outdated diff May 16, 2013
app/models/character.rb
+ item.save!
+ end
+
+ def quantity_of(good)
+ character_inventory_items.find_by_good_id(good.id) ? character_inventory_items.find_by_good_id(good.id).quantity : 0
+ end
+
+ def buy_good(good, quantity)
+ stock(good, quantity)
+ end
+
+ def valid_quantity?(good, quantity)
+ character_inventory_items.find_by_good_id(good.id) && quantity_of(good) >= quantity
+ end
+
+ def sell_good(good, quantity)
@marktabler
marktabler added a line comment May 16, 2013

We're starting to get a little wet in the code here (which isn't a pithy acronym or anything, it's just the logical opposite of "DRY" code). Shops know about buying and selling goods to characters, and characters know about buying and selling goods as well. I'm not sure whether a given transaction should be called as @character.sell or @shop.buy. This is a common problem, and there's a few ways to approach it, but I think in this case I'd advocate for a module.

The way this would work is that we'd have a module called something like "transactionable" (I'm not fond of that name, actually, so maybe it gets a pithy name like "quartermaster" or somesuch). It would define a few methods: sell_to(customer, good, quantity), buy_from(vendor, good, quantity), and some supporting methods. Then, on everything that has the capacity to buy and sell (currently, shops and characters), we include Quartermaster and it defines all of our supporting methods.

@marktabler
marktabler added a line comment May 16, 2013

I think I'd like to take a stab at writing this one before I make any sweeping pronouncements here. It's a complex enough space that I don't think I can wrap my head around the whole thing without starting a little bit of code. :)

@marktabler
marktabler added a line comment May 16, 2013

OK, I think I've got it. We want both a model and a module. Here's how this is gonna work: we're gonna have a Quartermaster model, and it knows things like how to ask whether a buyer can buy a thing, a seller can sell a thing, and handle the transaction itself. Then we're gonna have a Transactionable model that implements the methods Quartermaster needs to use in order to handle its logic. I'll write up an example of this later this evening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@marktabler marktabler commented on the diff May 16, 2013
app/controllers/shops_controller.rb
+
+ def sell_to_character
+ shop = Shop.find(params[:id])
+ character = Character.first
+ item = ShopInventoryItem.find(params[:item_id])
+ quantity = params[:quantity].to_i
+ respond_to do |format|
+ if shop.sell(character, item, quantity)
+ format.html { redirect_to shop, notice: 'Item sold to character!' }
+ else
+ format.html { redirect_to shop, notice: 'Item not sold!' }
+ end
+ end
+ end
+
+ def buy_from_character
@marktabler
marktabler added a line comment May 16, 2013

Nope nope nope nope. :)

This is too much logic for a controller. We will definitely need an endpoint to handle this kind of thing, but it might not even live in the Shops controller. Here are a couple of possibilities:

  1. We make a Transactions controller - we can do this without creating a Transactions model if we want to. Transaction#create takes a Seller, a Buyer, and a Good, and delegates the underlying logic to a single selected model (i.e., all transactions are handled as a .buy method on the buyer or as a .sell method on the seller, but it's only one and it's the same every time)

  2. We make a "transact" action in the Shops controller. It looks up a Shop and a Customer, then passes relevant params (good, quantity, buy/sell instructions) and delegates the logic to the Shop model specifically.

  3. We make a "transfer" action in a Goods controller. It looks up a Good, then passes relevant params (buyer, seller, quantity) and delegates the logic to a single underlying model as above.

The important pattern is the "delegates the logic" bit. A controller should only support the channel over which a conversation is happening.

Have you ever had a relay call? It's where a person using a TDD is calling a person using a regular telephone. A TDD is text-based, of course, so there needs to be an adapter of some kind in the middle. That adapter is a Relay Operator, whose job is to read the text coming from the TDD out loud to the person on the phone, then transcribe the audio from the phone as text to the person using the TDD.

The Relay Operator is kind of like a controller. They can't actually participate in the conversation - they can only facilitate it. They are limited to helping with the actual channel of the conversation - you can ask them to re-read the last sentence, and you can ask them to spell a word as it was typed, but nothing else. I remember asking once whether a relay service as billable per minute, per call, or free, because it made a difference as to how I was going to handle the call; the operator duly typed the question to the customer to let them explain (it's a free, nationally-provided service).

So, in this buy/sell situation, our Controller should pass along the message between the right parties, but only that - incoming parameters, outgoing data. When asked questions like "What does buying a thing actually mean" or "how many parties are involved," the controller shouldn't know. It should know "Shop#buy: I send a good, a buyer, and a quantity to the correct shop. If successful, I send this message back; if not successful I send this other message back."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@marktabler marktabler commented on an outdated diff May 16, 2013
app/controllers/shop_inventory_items_controller.rb
@@ -0,0 +1,83 @@
+class ShopInventoryItemsController < ApplicationController
@marktabler
marktabler added a line comment May 16, 2013

This probably doesn't need a controller of its own. I'm thinking that, as a user, I'm interested in a Shop. A Shop's inventory items are viewed through the Shop context, which means Shop controller.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@marktabler marktabler commented on the diff May 16, 2013
app/views/shops/show.html.erb
@@ -0,0 +1,32 @@
+<p id="notice"><%= notice %></p>
+
+<p>
+ <b>Location:</b>
+ <%= @shop.location_id %>
+</p>
+
+<p>
+ <b>Name:</b>
+ <%= @shop.name %>
+</p>
+
+
+<h3>Shop Items:</h3>
@marktabler
marktabler added a line comment May 16, 2013

This right here is why we won't need a ShopInventoryItems Controller. :)

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

Hi Mark,

We've added the second polymorphic join table called wares and a module called vending. There were a number of methods in the vending module (quantity_of(good), has_enough(good) etc) that we duplicated from the owning module. We were wondering what you thought about the best way to solve this. Thanks!

@marktabler

Good question! On the surface, the code looks wet - we're talking about the same quantity / existence concepts in two different places. But, they're working explicitly with two different pools of data - even if the logic is the same, we either need to call separate methods or genericize some of them to make this work. I don't anticipate going to more than two sets of goods a thing will need to manage, so leaving them as separate modules is OK -- though I might change the name of some of the Vending methods - e.g., @shop.ware_has_any?(good) doesn't read as smoothly as @shop.has_any_wares?(good).

But, there is a way we could reasonably refactor - and if we started getting in to more than two sets of goods per owner, we would almost need to. Here's an example of the way this refactor would work:

def has_enough?(source, good, quantity)
  subject = collection(source).find_by_good_id(good.id)
  subject && subject.quantity >= quantity
end

def collection(source)
  self.send(source)
end

Now, has_enough? takes three arguments: the familiar good and quantity, as well as a source. Source is going to be a symbol representing one of the collections we might have; currently, either :wares or :possessions. So, what's happening is that when we say @shop.has_enough?(:wares, Good.first, 50), the collection(source) method will run :wares as if it were a method - that is, just like @shop.wares. Just make sure you're never letting "source" be user input - this should come from things that are hard-coded in our app, not from passing params.

As you're doubtless noticed, this pretty much boils down to the same idea either way - we're going to have to specify whether we're talking about wares or possessions when we run methods like has_enough?. We'll also need to keep track of which collections respond to which methods (e.g., we have to know we can't call sell_price on a possession). So, I think that if we wanted to leave the two modules as they are for now, I wouldn't argue. If we wanted to refactor this into a generalized solution, I wouldn't argue with that either. If we go down the refactoring path, we'd put all the methods into one module (I'd suggest that Owning is still a valid name for a module that handles all kinds of belongings), and still manage transactions through intermediary class(es), such as Quartermaster.

It's your call as to what you think feels better. I don't think we currently have any requirements that would guide us especially one way or the other. I do think it's worth pointing out that if we were to get a requirement that made the decision for us, it would almost certainly be in the direction of dynamic source selection, but there's also no indication we will be getting any such requirement.

@marktabler

Also, I had to take the nil fixes out of the migration file after all. For some reason, our model names were not recognized during runtime - this might have something to do with database state & environment settings. In any event, it's in a separate rake task in case we need it for maintenance later but we can probably delete it entirely pretty soon (if nobody has broken data, and the migrations prevent it from breaking, then the repair task is obsolete.)

Still not sure why it doesn't work in the migration - the Rails docs themselves suggest it ought to - but at least now we have reasons for an opinion about where that code should live. :)

@duncanjmiller

I think I know why... Mike and I created another migration today to delete those tables and also deleted the models, since those were for the old shop_inventory_items and character_inventory_items.

@mlapeter

Ok, we believe we're done with transactionable and Quartermaster, and we've added tests for all the cases we can think of. Please review and merge if it's ready and let us know what you think!

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