OOP Design - In Python, is this a quality OO Design or an epic fail?
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 开发者_StackOverflow= <Dollar Amount>
payment = payment_instance
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
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.
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.
精彩评论