views:

230

answers:

3

In a system that accepts orders which have payments which have gateway transactions should the objects be like this:

class Order(object):
        ... Inside init ...
        self.total_in_dollars = <Dollar Amount>
        self.is_paid = <Boolean Value>

class Payment(object):
        ... Inside init ...
        self.order = order_instance
        self.amount = order.total_in_dollars

class GatewayTransaction(object):
        ... Inside init ...
        self.payment = payment_instance
        self.amount = <Dollar Amount>

It seems this would be the way to do it (obviously this is not real code with integer dollar amounts, etc but you get the picture). I did it this way because an order can exist without payment and a payment can exist before an actual PayPal transaction occurs. Does this lack in your opinion? Am I backwards in my thinking?

OR, Should it be more like this:

class GatewayTransaction(object):
    payment = payment_instance
    amount = <Dollar Amount>

class Payment(object):
    amount = <Dollar Amount>
    gateway_transaction = gateway_transaction_instance

class Order(object):
    amount_in_dollars = <Dollar Amount>
    payment = payment_instance
+2  A: 

Give the objects constructors that take the appropriate other object that you want it to refer to, then make the fields into properties that verify the type you assign to them.

Ignacio Vazquez-Abrams
What about the aspect of missing parts (ie, I create an Order with no Payment), and also the part where I don't know how to create the Payment (ie, the Order may have a payment_choice attribute and I create a Payment that references Order.payment_choice to determine how to process it).
orokusaki
Clearly there's no need to require a value that isn't required; if there's nothing wrong with having an attribute be None then allow that in the property and have that as the default argument in the constructor.
Ignacio Vazquez-Abrams
+5  A: 

You appear to be assigning what should obviously be instance variables as class variables, which is clearly a very wrong tack to take. IOW, the variables should be self.total_in_dollars (for instance of Order) and so forth, assigned in __init__, not class-variables, assigned in the class statement!

Simply creating an instance of Order without a corresponding one of Payment is fine (and should obviously set is_paid to False), based just on the total (and presumably some numeric ID so customers &c can in the future refer to a specific order).

Don't duplicate information needlessly! Since a Payment instance will always have a reference to an Order instance, it should not copy self.order.total_in_dollars over to self.amount -- better to have that information in one place (you can make a readonly property if you want to access it nicely); and even more so for the transaction instances.

If an Order instance carried further metadata that influences how the corresponding Payment instance is created and behaves, that's OK, but strongly suggests making the creation of the Payment instance the job of a factory-method of class Object (which can then also keep track of already-generated instances and ensure there will never be more than one Payment instance for a single given Order instance).

Edit: now that the OP has somewhat edited the A, I can confirm the opinion that the dependencies in the first version are roughly correct (except, again, the amount should not get copied all over the place) and those in the second versions are, prima facie, not correct (the presence of a mutual/circular dependency for example is always a design smell unless clearly and explicitly justified by special application needs -- even when the need to navigate back and forth exists, one of the two links should be a weak reference, at least).

Edit: as the OP explicitly asks for more detail on the factory method I suggested, what I had in mind was something like this:

import weakref

class Payment(object):
  def __init__(self, order):
    self.order = weakref.proxy(order, self.ordergone)
  def ordergone(self, *_):
    self.order = None
  @property
  def amount(self):
    if self.order is None: return None
    else: return self.order.total_in_dollars

class Order(object):
  def __init__(self, amount):
    self.total_in_dollars = amount
    self.is_paid = False
    self._payment = None
  @property
  def payment(self):
    if self._payment is None:
      self._payment = Payment(self)
    return self._payment
Alex Martelli
Oops, I just edited my typo (for the first example anyways). Thanks.
orokusaki
Thanks, that was really helpful. Can you give me an example of how you might implement the last paragraph. I'm interested (ie, I don't know much about factory pattern but have wanted to learn).
orokusaki
Thanks Alex for the example. I don't fully understand it because I don't understand what weakref does exactly (I just barely learned about weakref a few hours ago and plan to research it more now). I'm assuming that it just gives you a reference to an object without increasing the reference count of that object.
orokusaki
Also, what does the *_ do in the ordergone() method?
orokusaki
Having `*whatever` as the last parameter in a `def` mean: accept any further positional arguments. For the `whatever` I use `_` because I intend to ignore those arguments and it's a common convention to name `_` a variable that you intend to ignore. And yes, "weak references" (both refs and proxies, just minor differences) serve to access an object as long as it's alive but without _keeping_ it alive -- on building them you can also pass a callback that gets called right after the object disappears (if the object disappears before the ref or proxy does).
Alex Martelli
Oh, thanks. The _ through me off, so I didn't even think about *args.
orokusaki
+2  A: 

I would approach this differently - by writing the some code for dealing with Orders, Payments etc. This would clarify my design needs, e.g. it would turn out that Payment.amount could be greater that Order.total_in_dollars, because of some processing fees. But then, it could turn out that maybe those processing fees should be stored separately, or even they could/should have their own model. And yes, this is TDD.

Tomasz Zielinski
Thanks. That makes sense. In other words, the style that works best is determined based on needs along the way, vs an end-all-be-all approach to the perfect OO design.
orokusaki