Help us understand the problem. What is going on with this article?

コードレビューでよく指摘する箇所まとめ

More than 1 year has passed since last update.

はじめに

社会人6年目にもなると、後輩もできてコードレビューでPRにコメントをする回数も増えてきました。そこで、よく指摘する箇所をまとめてみようと思います。

弊社では、言語はPython、アーキテクチャにオニオンアーキテクチャを用いています。そのため、一部、言語やアーキテクチャに依存する箇所もあるかもしれませんが、多くの場合は他でも共通に使えるものだと思います。サンプルコードはPythonで書いてあります。

間違った継承関係

import json


class BaseRepository:
    def save(self, key: str, value: str) -> None:
        # 永続化領域へデータを保存する処理

    def fetch(self, key: str) -> str:
        # 永続化領域からデータを取得する処理


class UserRepository(BaseRepository):
    def save_user(self, user: Usesr) -> None:
        self.save(user.id, json.dumps(
            {"name": user.name, "address": user.address}))

一般的に継承関係は、BがAを継承する場合、「B is a A」という意味的な関係が成り立つとされています。上記のサンプルは UserRepositoryBaseRepository を継承しています。 「UserRepositoryBaseRepository である」というのはクラス名だけを見ると一見正しい継承関係に見えるかもしれません。しかし、それぞれの役割をよく見ると、UserRepositoryBaseRepository使う 関係です。例えば、以下のように修正してはいかがでしょうか?

import json


class XxxxClient:
    def save(self, key: str, value: str) -> None:
        # 永続化領域へデータを保存する処理

    def fetch(self, key: str) -> str:
        # 永続化領域からデータを取得する処理


class UserRepository:
    def __init__(self, client: XxxClient) -> None:
        self._client = client

    def save(self, user: Usesr) -> None:
        self._client.save(user.id, json.dumps(
            {"name": user.name, "country": user.country}))

    def fetch(self, user_id: int) -> User:
        user_dict = json.loads(self._client.fetch(user.id))
        return User(user_id, user_dict["name"], user_dict["country"])

上記のように 使う 関係にすることで、クラス間のつながりがゆるくなりました。これで、永続化領域に依存することなく UserRepository クラスのユニットテストを書くことも可能です。

import sure
from mock import MagicMock


dummy_client = MagicMock()
dummy_client.fetch.return_value = '{"name":"user1","country":"japan"}'
repository = UserRepository(dummy_client)
repository.fetch(1).should.be.a(User)

継承関係が正しいかどうかは、リスコフの置換原則を守っているかどうかを判断の材料にするのが良いかと思います。今回の例でいうと、 UserRepository オブジェクトを BaseRepository オブジェクトの代わりに用いて理にかなっているか?これは明らかにおかしいことが分かります。

アプリケーション(ユースケース)レイヤーの役割

エリック・エヴァンスのドメイン駆動設計 が紹介している4層レイヤードアーキテクチャ、オニオンアーキテクチャではアプリケーションレイヤー。クリーンアーキテクチャではユースケースと呼ばれるレイヤーの役割についてです。

アプリケーションレイヤーは短く言うと、「ソフトウェアが行う仕事を定義し、ドメインオブジェクトを組み合わせることによって実現するレイヤー」です。ドメインロジックはここには含めず、実際の処理はドメインレイヤーに任せます。
上記の話をレビューで指摘しても中々分かってもらえず、「ディレクトリから特定のファイルを探して開いて...」、みたいな処理が毎回アプリケーションレイヤーに書かれたプルリクエストが飛んできます。そういうときは、「このプログラムが何をするプログラムだか簡単に教えて?」と質問しています。「画像をファイルから読み込んで、分類器を用いて分類して、、、」そう、まさにその一つ一つがアプリケーションレイヤーの1行1行に該当します。

from typing import List


class ImageClassificationService:
    def classify(self) -> List[Category]:
        images = ImageRepository().fetch()
        categories = [ImageClassifier().classify(image) for image in images]
        return categories

アプリケーションレイヤーのコードを見るとそのプログラムの全体像が分かるのが理想です。
「ディレクトリから特定のファイルを探して開いて...」のような詳細部分はアプリケーションレイヤーにはふさわしくないです。アプリケーションレイヤーが長くなってしまっているときは、何かが間違っている可能性があるので、気を付けたほうが良いと思います。

逆に、必要な処理がスキップされている場合もよく見かけます。例えば、何かを計算する処理と、その結果を保存する処理をまとめて一つのクラスに任せるケースです。

class VideoGenerationService:
    def generate(self, document: Document, images: List[Image]) -> None:
        storyboard = StoryboardService().create(document, images)
        VideoService().generate(storyboard) # videoを作成して、永続化領域に保存

ビデオ生成の処理、永続化領域に保存する処理を一緒にすると、VideoSerivceのテストがしにくくなります。また、このコードを見たときに、コードを読む人はビデオがどこかに保存されていることに気づくことができません。副作用のあるコードはコードの可読性、テスタビリティを下げるので避けた方が良いです。

class VideoGenerationService:
    def generate(self, document: Document, images: List[Image]) -> None:
        storyboard = StoryboardService().create(document, images)
        video = VideoService().generate(storyboard)
        VideoRepository().save(video)

上記のようにすることで、ビデオをどこかに保存していることが明確に分かります。また、ビデオの生成、保存を別のクラスに分離したことによって、それぞれのテストに集中出来ます。

一意な変数名

例として、以下のような Token クラス、 Sentence クラスが存在するとします。

from typing import List


class Token:
    def __init__(self, surface: str, part_of_speech: str) -> None:
        self._surface = surface
        self._part_of_speech

    @property
    def surface(self): str
        return self._surface

    @property
    def part_of_speech(self): str
        return self._part_of_speech


class Sentence:
    def __init__(self, tokens: List[Token]) -> None:
        self._tokens = tokens

    @property
    def tokens(self) -> List[Token]:
        return self._tokens

以下のように色々な文脈において、色々な種類の型をもつ sentence 変数をよく見かけます。

# str型
sentence = "今日は晴れです。"

# List[str]型
sentence = ["今日", "は", "晴れ", "です", "。"]

# List[Token]型 
sentence = [Token("今日", "名詞"), Token("は", "助詞"), Token("晴れ", "名詞"), Token("です", "助詞"), Token("。", "記号")]

# Sentence 型
sentence = Sentence([Token("今日", "名詞"), Token("は", "助詞"), Token("晴れ", "名詞"), Token("です", "助詞"), Token("。", "記号")])

これは一見当たり前のように見えますが、非常によく見かけます。このようなコードは可読性を下げ、バグが注入しやすくなるので避けたほうが良いです。特にスクリプト言語だとコンパイラによる型チェックが無いため、 Sentence オブジェクトを返したつもりがstr型を返していたというバグを何度も見かけました。以下のように一意に型が推測できる名前にするべきです。

# str型
text = "今日は晴れです。"

# List[str]型
surfaces = ["今日", "は", "晴れ", "です", "。"]

# List[Token]型 
tokens = [Token("今日", "名詞"), Token("は", "助詞"), Token("晴れ", "名詞"), Token("です", "助詞"), Token("。", "記号")]

# Sentence 型
sentence = Sentence([Token("今日", "名詞"), Token("は", "助詞"), Token("晴れ", "名詞"), Token("です", "助詞"), Token("。", "記号")])

まとめ

他にも色々と書こうと思ったのですが、思ったより時間がかかってしまったので、時間があるときに適宜追加していきます。

Why not register and get more from Qiita?
  1. We will deliver articles that match you
    By following users and tags, you can catch up information on technical fields that you are interested in as a whole
  2. you can read useful information later efficiently
    By "stocking" the articles you like, you can search right away