これを Python でやってみます
元のコード
元の Ruby 版をできるだけ真似してみます。テストは 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 # 組み込み関数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
解説は要らないと思いますが、必要なら Ruby 版のものを読んでください。
その1 - 「イケてない」から「マシ」にする
まずorders_within_rangeに範囲内のorderを格納している処理を変更する。
これは Python だと filter か内包表記にするところです。 filter は条件がすでに関数になっているときには便利なのですが、条件式を書きたいときは filter と lambda を組み合わせるよりも内包表記が良いです。
ruby版
orders_within_range = @orders.select do |order|
order.placed_at >= @start_date && order.placed_at <= @end_date
end
python版
orders_within_range =
[o for o in self.orders
if self.start_date <= o.placed_at <= self.end_date]
次がeachで回して売上合計を出す処理。
sumの宣言などがあってやたらと行数が長い。基本的に１つのメソッドは５行以内にするべき。元のコードは１３行もある。当面はとにかく行数を短縮することだけを考えてリファクタリングしてもいい。ここで使えるのはinject。
Python の場合は inject の代わりに reduce があるのですが、あまり使いません。こういう場合は素直に sum を使います。 sum の引数には、内包表記を使ってジェネレーターを渡します。
Ruby 版:
orders_within_range.inject(0) do |sum, order|
sum + order.amount
end
Python 版:
return sum(o.amount for o in orders_within_range)
Ruby 版は inject を使ったことで return 文自体が省略できましたが、 Python では省略できず、計算結果を返すことを明示的に表現します。
その2 - 「マシ」から「いいね」にする
まずはorders_within_rangeをprivateメソッドにして外に出してしまうことにする。メソッド名はそのままorders_within_rangeとした。
この程度の長さの関数をさらに分割するのはあまり好きではありませんが、 orders_within_range を使った計算メソッドが他にも複数あるなら、共通化して良いと思います。 Python の場合 private はなく、慣習としてアンダースコアで始まる名前を使います。引数もないのでプロパティにしています。
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]
さて、次ですが、
ここは以前に本ブログで書いた「聞くな、言え」の法則に反している。
...
ここではselectの中でいちいちorder.placed_atが範囲内に入っているかどうかを「聞いている」からよくない。これを「言うだけ」に変更するとこうなる。
ということで、 Order クラスに placed_between? を追加してるんですが、この変更には反対です。
placed_before?, placed_after?, placed_at?, 必要になったらどんどん追加するんですか？必要なくなったときに呼び出し元がなくなったことを確認して削除するんですか？削除忘れて使ってないメソッドがプロジェクト内に大量に残ったりしませんか？
order.placed_at を非公開にする理由があるならそれでも良いのかもしれませんが、公開している以上、「日付の比較」という一般的な処理を Order の責務に含めるのには反対です。
。コード全体を見渡すとあちこちにstart_date, end_dateを引数として使われている。これらのstart_dateとend_dateは毎回入れなければならない。それで可読性と再利用性が下がってしまっている。
...
start_date, end_dateというものは常にペアで、どちらかひとつでは成り立たない。なのでそれらをひとまとめにしてしまうのが次にするべき変更。
これは正しい。 Order クラスだけなら微妙だけど、他にも日付の範囲を利用する箇所がたくさんあるなら実装しておきたい。
(余談だけど、 SQLAlchemy なら Composite Column Type という機能で、「日付の範囲を指定するクラス」をRDB上の２つのカラムにマッピングして、さらにSQLの生成もできる 参考)
ここまで実装したものが以下になる。 Ruby 版にあった include? は少し悩んだが、 __contains__ にしてしまった。範囲を表すときには Python だと (C++ などと同じように) 半開放区間 [start, end) で表すのが普通なので、その部分も Ruby とは別にした。
DateRange は実際には date に依存していないので単に Range 型として数値の範囲などを表せるようにしたい誘惑に駆られるが、組み込みの range と競合するし、行き過ぎた一般化の恐れが強いので自制する。
ここまでのコードをまとめると
#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]
その3 - 「いいね」から「スゲーいいね」にしない
元記事のその3では、次のような2段階のリファクタリングを行っていた。
リファクタ前:
# リファクタ前
def total_sales_within_date_range
orders_within_range.map(&:amount).inject(0) do |sum, amount|
sum + amount
end
end
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
そして total_sales の実装をイディオムで1行に:
def total_sales(orders)
orders.map(&:amount).inject(0, :+)
end
メソッド抽出をした理由は public メソッドの実装はひと目で分かるくらいに簡単にしたいというもので、確かにリファクタ前のコードは何をやっているのか把握するのに「えーっと、」と一息つく必要がある。イディオムで1行にした最後の実装でも、このイディオムをしらない人にとっては一目瞭然とはいえないので、名前をつけたメソッドで分けておく意味はあるかもしれません。
一方 Python の方は sum(o.amount for o in self._orders_within_range) の時点で十分に読みやすく、これをあえて self._total_sales(self._orders_within_range) と書き換えてもたいして可読性 (※) は向上しません。
それどころか、 _total_sales の実装が単に amount の合計を取っていることを確認するためだけにジャンプして戻ってくる手間の分、可読性が低下している。 なのでここでは何もしないことにします。
※ ここでいう「可読性」は、 "The Art of Readable Code" で説明されているような、デバッグしたり改修したりできるレベルに深く理解するのに掛かる時間です。なので、1行で書ける十分に簡潔で読むのに頭を捻る必要もない、呼び出し元も1箇所~数カ所しか無い処理をメソッドにわけてしまうと、「そのメソッドが具体的に何をしているのか」を確認するためのコードの行き来に余計に脳を使ってしまうことになるので、逆に可読性が落ちる事になります。
その4 - オブジェクト指向設計を捨てる
個人的には、 OrdersReport の責務が分かりにくいと思った。
名前の通り、 Order の配列から各種のレポートを計算する役割なら、他のレポート生成メソッドは date_range に依存しないかもしれないので、 date_range はコンストラクタに渡すのではなく total_sales_within_date_range の引数にするべきではないだろうか？
そうすると、クラスにする意味すらわからなくなってくる。 Python はファイル自体がモジュールなので、関数で十分だ。
def total_sales_within_date_range(orders, date_range):
return sum(o.amount for o in orders if o.placed_at in date_range)
まとめ
Ruby の人がオブジェクト指向的に美しいと思うコードと、 Python の人がシンプルで普通と思うコード、あなたはどちらが好みだろうか？
Python 版:
#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 版:
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