Python オブジェクト指向 クラス設計について

Question

###前提・実現したいこと
ここ最近オブジェクト指向関連の稚拙な質問ばかりで申し訳ないです。
クラス設計についてどうすればいいのか分からないので教えて欲しいです。

##該当のソースコード
#####Report.py

from Dict import Dictonary

class Report:
    def __init__(self,name,comment):
        self._name = Name(name)
        self._comment = Comment(comment)

    def macthWords(self):
        dict = Dictionary()
        words = self._comment.parse()
        return [word for word in words if dict.isMatch(word)]


class Name:
    def __init__(self,name):
        self._name = name

    def isSame(self,name):
        return self._name == name

class Comment:
    def __init__(self,comment):
        self._comment = comment

    def parse(self):
        #parse処理
        return words

#####Dict.py

class Dictionary:
    def __init__(self):
        self._dict = self.__loadDictionary()

    def __loadDictionary(self):
        #ファイルを読み込んで辞書を作成
        return dict

    def isMatch(self,word):
        return word in self._dict

##この設計の問題点
今現状のクラス図はこのような感じでしょうか
NameとCommentはラップクラスです。

           |
           |
           |
[class Report:] ------------------------[class Dictionary:]
           |
           |--------------------┐
           |                            |
[class Name:]    [class Comment:]

問題が２つあります。
①ReportクラスのmatchWordsメソッドが呼ばれるたびに辞書を作成しまう。
②単一責任の原則に反しているのではないか？（commentでparseしたものをReportクラスで扱っている為）

②に関してはラップクラスとしてのCommentの責任からは離れているので問題ないのかなとは思っているのですが分からないので教えてください。
①は何か解決策がありましたら教えてください。

これ以外にも何かおかしかったり、こうした方がいいというのがあれば教えてください。

Answer 1

(1) ReportクラスのmatchWordsメソッドが呼ばれるたびに辞書を作成しまう。

これはオブジェクト指向としての話題か実装テクニックの話かよくわかりませんが・・・後者だとしてコメントしてみます。

この辞書をみると「何物にも依存しない辞書」に見えるので、インスタンスのライフサイクルをどうするか考えてみてはいかがでしょうか？辞書インスタンスのライフサイクルはメソッドの呼び出し期間、インスタンスの存在期間、クラスの存在期間、アプリケーションの存在期間のいずれにするかで設計に違いがでると思います。

またあまりよいことではないかも知れませんが、Dictionaryのインスタンスをいくつ作っても辞書本体の情報はDictionaryクラスが静的に保持しているという作り方もできなくはありません。利用者は毎回インスタンスを作るのですが、実際のロードはDictionaryクラスが「必要に応じて１回だけ」にするといったトリックです。

(2) 単一責任の原則

CommentやReportが「何であって何でないか」の設計次第だと思います。例えば「コメント」というクラス名からは「単語を分解する」とか「キーワードとそれ以外を取り出す」といった機能のイメージが(自分は)わいてきません。またReportが「何をリポートする目的のものか」いいかえれば「現実世界のどういった問題をモデル化したものか」イメージがわかないので「何をどのクラスに役割分担させようとしているか」がわかりません。それがイメージしにくいと各クラスが何を意味しているかがわからないのではっきりとした意見が持てない気がします。

Comments

[_Victorique__]

(2)に関しては確かにそうですね。
Reportに関しては、アンケートをイメージしてもらえると良いかもです。名前とコメントを書いてもらったものが複数人分あります。ですので、Commentクラスに関してはcommentを保持・操作する（commentを書き換えることはない）で、Reportクラスに関してはNameとCommentを用いた処理をします。
でもどうしてもcommentがメインになってしまっているのが現状です。

[KSwordOfHaste]

なるほど、アンケートに特定の単語が含まれているかどうかを調べるといったモデルなんですね。
だとするとCommentクラスではなくアンケートクラスとして興味のある単語の抜き出し機能を持っている設計もよい気がします。つまりReportクラスは不要という感じで。

[_Victorique__]

そうするとcommentとnameを結び付けられなくなってしまいます。実際はnameの他にもageやsex、dateなどの情報もアンケート１枚に含んでいるのでそれらをまとめる為にReportクラスを作った感じです。どのようにすれば良いでしょうか？

[_Victorique__]

実際には、ageやsex、dateを条件にcommentを操作する感じです。

[KSwordOfHaste]

Reportクラスは「単に関連するひとまとまりの情報を集約」するためだけのクラスとして考えてもいいのではないですか？情報がまとまっていて属性だけがあるＤＢアプリケーションでよく用いられる「データクラス」のようなものとして考えるわけです。

[KSwordOfHaste]

もし別のReportクラスを設計することを想定するとキーワードを抜き出すのがReportにあっていいのだろうか・・・という気がちょっとします。またキーワードを抜き出すのではなく、単に単語の集合を表現したいだけのクラスを利用したい場合はCommentにキーワードを抜き出す機能が含まれているとジャマに感じ「もっと単純な機能のクラスがほしかった」といいたくなる状況も想像できます。やはり「どう設計するか」にはバリエーションがあると思います。

[_Victorique__]

この日付のReportに対して処理したい時があるのでそれをReportに書く、という風に考えていましたが、ReportのDateにその日付か判定するメソッドを持たせて、それで処理するので十分ですね。これが正しいのならデータクラスとしてありだと思いますがどうですか？

[KSwordOfHaste]

そういう設計も充分アリと思います。

[_Victorique__]

ありがとうございます。
最後に、＞pashango2さんの回答コードをみてもらいたいのですが、Commentクラスのクラス変数としてDictionaryクラスのインスタンスを保持するのはOOP的に問題ないのでしょうか？

[KSwordOfHaste]

pashango2さんの考え方は
「辞書はReportに依存する情報」
「インスタンスごとに異なるものではなく共通のもの」
という前提での設計であり、その前提が質問者さんの意図にマッチするかどうかは別として、OOPとして自然な設計の一つだと自分は思います。

[_Victorique__]

ありがとうございます！全て解決しました！

Answer 2

①に関して、一番簡単な方法は以下の通りです。

class Report:
    _dict = Dictionary()

    def __init__(self,name,comment):
        self._name = Name(name)
        self._comment = Comment(comment)

    def macthWords(self):
        words = self._comment.parse()
        return [word for word in words if self._dict.isMatch(word)]

コードを読む限り、実ファイルからいつも同じ辞書を作成しているようなので、クラスメンバを使います。
C++で言うところのstaticメンバですね。

②に関して言えば、どこでもいいと思います。
というのも、責任の所在を議論するにはCommentクラスの機能が少なすぎます。

その他
設計には個々の好みがあるので、確実にこうした方がいい部分だけ列挙します。

Directoryクラスは実ファイルと密結合しているし、各々のクラス同士が少し結合度が高いかなと思います。
Dependency Injectionなどで少し結合度を下げた方が良いと思います。

Pythonじゃなくて「Javaっぽいな」というのが印象です、少なくともPEP8を守った方がよいかなと思います。dictなどのビルトインクラスの名前の上書きしている部分があるので、flake8などの静的解析ツールを導入したほうがよいですよ。

Comments

[_Victorique__]

これってスタティッククラスにするってことですか？
ちなみにReportのインスタンスを複数作った場合でも初期化は一度だけになります？インスタンスを作るたびに辞書を作ることはありませんか？

[pashango2]

いえ、スタティッククラスにする必要はありません。
普通にインスタンを生成してください、それでも_dictは1回しか初期化はされません。

[_Victorique__]

なるほどです。
すごくスッキリしていますが、これってOOP的に問題はないのですか？

[pashango2]

問題はありますよ、もしかしたらボロカスに言われるかもしれません。

しかし、この方法は「これ以上ないくらいシンプル」という何にも代えがたい力があります。
この方法で将来的に問題が出ないかもしれないのに、先回りしてシンプルさを崩す不可逆な変更をする事は愚策であると思います、不確定な未来に対しての過剰投資であり、早すぎる最適化です。
私なら問題が発覚してから対処します、対処しやすいようにある程度の軸足を考えて組む必要がありますが・・・

まぁ、設計は好みですので、この意見に納得できない場合はDictionaryを外部から注入してください。
Dictionaryの寿命管理責任はReportクラスよりも上のクラスにありますので。

[pashango2]

上のコメントはDirectory -> Dictionaryでした、修正しました。

Answer 3

最終的に以下のように使えるようになったらオブジェクト指向だと思います。

dictionary = Dictionary('file')
report = Report('name', 'comment')
words = dictionary.matchWords(report.comment.parse())