Let's do this in Python
Try to imitate the original Ruby version as much as possible. The test uses py.test.
#ordersreport.py
from collections import namedtuple
Order = namedtuple("Order", "amount placed_at")
class OrdersReport:
def __init__(self, orders, start_date, end_date):
self.orders = orders
self.start_date = start_date
self.end_date = end_date
def total_sales_within_date_range(self):
orders_within_range = []
for order in self.orders:
if self.start_date <= order.placed_at <= self.end_date:
orders_within_range.append(order)
sum_ = 0 #Underscore to avoid collision with built-in function sum
for order in orders_within_range:
sum_ += order.amount
return sum_
#ordersreport_test.py
from datetime import date
from ordersreport import Order, OrdersReport
def test_sales_within_date_range():
order_within_range1 = Order(
amount=5, placed_at=date(2016, 10, 10))
order_within_range2 = Order(
amount=10, placed_at=date(2016, 10, 15))
order_out_of_range = Order(
amount=6, placed_at=date(2016, 1, 1))
orders = [order_within_range1, order_within_range2, order_out_of_range]
start_date = date(2016, 10, 1)
end_date = date(2016, 10, 31)
report = OrdersReport(orders, start_date, end_date)
assert report.total_sales_within_date_range() == 15
I don't think you need any explanation, but if you need it, read the Ruby version.
First, change the process that stores orders within the range in orders_within_range.
This is where Python uses filter or comprehensions. filter is useful when the condition is already a function, but when you want to write a conditional expression, inclusion is better than combining filter and lambda.
ruby version
orders_within_range = @orders.select do |order|
order.placed_at >= @start_date && order.placed_at <= @end_date
end
python version
orders_within_range =
[o for o in self.orders
if self.start_date <= o.placed_at <= self.end_date]
Next is the process of turning at each to get the total sales. The number of lines is extremely long due to the declaration of sum. Basically, one method should be within 5 lines. The original code has 13 lines. For the time being, you can refactor just to reduce the number of lines. You can use inject here.
In Python, there is reduce instead of inject, but I don't use it much. In this case, use sum obediently. Pass the generator as an argument to sum using comprehension notation.
Ruby version:
orders_within_range.inject(0) do |sum, order|
sum + order.amount
end
Python version:
return sum(o.amount for o in orders_within_range)
In the Ruby version, the return statement itself could be omitted by using inject, but in Python it cannot be omitted, and it explicitly expresses that the calculation result is returned.
First, let's make orders_within_range a private method and let it go out. The method name is orders_within_range as it is.
I don't really like splitting functions of this length further, but if there are multiple other computational methods that use orders_within_range, I think they can be shared. In the case of Python, there is no private, and it is customary to use names that start with an underscore. Since there is no argument, it is a property.
def total_sales_within_date_range(self):
return sum(o.amount for o in self._orders_within_range)
@property
def _orders_within_range(self):
return [o for o in self.orders
if self.start_date <= o.placed_at <= self.end_date]
Well, next,
This violates the "don't listen, say" law I wrote earlier in this blog. ... Here, it is not good because I am "listening" whether order.placed_at is within the range in select. If you change this to "just say", it will be like this.
So I'm adding placed_between? to the Order class, but I'm against this change.
placed_before?, placed_after?, placed_at?, Do you add more and more when you need it? Do you want to confirm that the caller is gone and delete it when you no longer need it? Do you forget to delete and leave a lot of unused methods in your project?
ʻIf there is a reason to keep order.placed_at` private, that's fine, but as long as it's public, I'm against including the general process of "dating comparisons" in Order's responsibilities. ..
. Looking over the entire code, start_date and end_date are used as arguments here and there. These start_date and end_date must be entered each time. That reduces readability and reusability. ... start_date and end_date are always pairs, and neither one is valid. So the next change to make is to put them together.
Is this correct. The Order class is subtle, but if there are many other places that use the date range, I would like to implement it.
(As an aside, with SQLAlchemy, you can also generate SQL by mapping the "class that specifies the date range" to two columns on the RDB with a function called Composite Column Type [Reference](http: // qiita. com / methane / items / 57c4e4cf8fa7822bbceb)))
The ones implemented so far are as follows. I was a little worried about ʻinclude?That was in the Ruby version, but I changed it tocontains`. When expressing a range, in Python (similar to C ++ etc.), it is usually expressed by a semi-open interval [start, end), so that part is also separated from Ruby.
DateRange doesn't really depend on date, so I'm tempted to just want to be able to represent a range of numbers as a Range type, but it conflicts with the built-in range and there is a strong risk of over-generalization. ..
To summarize the code so far
#ordersreport.py
from collections import namedtuple
Order = namedtuple("Order", "amount placed_at")
class DateRange(namedtuple("DateRange", "start end")):
def __contains__(self, date):
return self.start <= date < self.end
class OrdersReport:
def __init__(self, orders, date_range):
self.orders = orders
self.date_range = date_range
def total_sales_within_date_range(self):
return sum(o.amount for o in self._orders_within_range)
@property
def _orders_within_range(self):
return [o for o in self.orders if o.placed_at in self.date_range]
In Part 3 of the original article, the following two-step refactoring was performed.
Before refactoring:
#Before refactoring
def total_sales_within_date_range
orders_within_range.map(&:amount).inject(0) do |sum, amount|
sum + amount
end
end
Method extraction of the part that takes the sum of ʻOrder.amount`:
def total_sales_within_date_range
total_sales(orders_within_range)
end
private
def total_sales(orders)
orders.map(&:amount).inject(0) do |sum, amount|
sum + amount
end
end
And the implementation of total_sales is idiomized on one line:
def total_sales(orders)
orders.map(&:amount).inject(0, :+)
end
The reason for extracting the method is that I want to make the implementation of the public method so easy that I can understand it at a glance, and it is certainly necessary to take a break to understand what the code before refactoring is doing. .. Even the last implementation on a single line in an idiom isn't obvious to anyone who doesn't know this idiom, so it might make sense to separate them by named methods.
On the other hand, Python is easy enough to read at the time of sum (o.amount for o in self._orders_within_range), and even if you dare to rewrite this as self._total_sales (self._orders_within_range), the readability (*) is very good. Does not improve.
On the contrary, the readability is reduced by the hassle of jumping back just to make sure that the implementation of _total_sales is summing the amount. So I will not do anything here.
Personally, I found it difficult to understand the responsibilities of OrdersReport.
As the name implies, date_range should not be an argument to total_sales_within_date_range instead of passing it to the constructor, as other report generation methods may not depend on date_range for the role of computing various reports from an array of Orders. I wonder?
Then you don't even know what it means to be a class. Functions are sufficient in Python because the files themselves are modules.
def total_sales_within_date_range(orders, date_range):
return sum(o.amount for o in orders if o.placed_at in date_range)
Which do you prefer, code that Ruby people find beautiful in an object-oriented way, or code that Python people find simple and normal?
Python version:
#ordersreport.py
from collections import namedtuple
Order = namedtuple("Order", "amount placed_at")
class DateRange(namedtuple("DateRange", "start end")):
def __contains__(self, date):
return self.start <= date < self.end
def total_sales_within_date_range(orders, date_range):
# (List[Order], DateRange) -> int
return sum(o.amount for o in orders if o.placed_at in date_range)
Ruby version:
class OrdersReport
def initialize(orders, date_range)
@orders = orders
@date_range = date_range
end
def total_sales_within_date_range
orders_within_range.map(&:amount).inject(0) do |sum, amount|
sum + amount
end
end
private
def orders_within_range
@orders.select do |order|
order.placed_between?(@date_range)
end
end
end
class DateRange < Struct.new(:start_date, :end_date)
def include?(date)
(start_date..end_date).cover? date
end
end
class Order < OpenStruct
def placed_between?(date_range)
date_range.include?(placed_at)
end
end
Recommended Posts