前置き
先日、ゆめみさんの就活生必修！ゆめみが送る超実践的コーディングレビューワークショップというイベントに参加させていただきました。
メンターさんのTwitterDMにて許可をいただきましたので、ワークショップ参加中にいただいたフィードバックを基に作成したコードを解説していきます。
以下の点にご留意いただけると幸いです。
- 普段はPythonを利用しない人が書きました
- 本記事を投稿する許可はいただきましたが、記事・コードの品質に関してのレビューはいただいていません。
- フィードバックを聞くことに集中しすぎて、アドバイスをメモするのを忘れてしまったので、断片的な記憶を基に随筆しています。ですので、もしかしたら虚偽の情報があるかもしれません。
- 本記事に記載されているコードは、私が記述したものであり、ゆめみさんが作成した模範解答ではありません。(回答例を参考にはしています。）
概要
前置きにもある通り、ゆめみさんの就活生必修！ゆめみが送る超実践的コーディングレビューワークショップというイベントに参加させていただきました。
解いたことがある方も居るかもしれませんが、こちらの模擬問題を時間内に解き、レビューをいただくという内容でした。
なんとテスト環境まで用意されているという豪華な回でした。(こちらのGitHubにて公開されています。)
自力で解いてみた
コードはこちらです。
フィードバックを受けて...
褒めていただいた点
- コメントを書いている
- 引数や戻り値の型を定義している
- 例外に対してのコードを書いている。
これらに関しては後述します。
指摘していただいた点
指摘された点は１つだけだったような気がします。。。
グローバル変数を利用している
(ここから先は私の見解です)
当初このプログラムは、グローバル変数playersに読み込んだプレイヤーを入れていましたが、グローバル変数を使うということは、関数の戻り値や引数を利用しないことに繋がります。
もしこれが実務であれば、テストの粒度が大幅に下がることになりますので、不適切かなと思いました。
また、次のコードは私が書いたものですが、readCsv()等をした際のデータの流れが分かりづらいという欠点もあります。
if __name__ == "__main__":
# コマンドライン引数の読み取り
args = get_option()
fileName = str(args.fileName)
# CSVを読み込む（エラーがあれば出力する。）
readCsv()
sortedList = dicSort() # ソートする
output(sortedList) # 出力する
改善するためには、使用するデータは引数や戻り値で受け渡ししましょう。
全体に向けてのフィードバック
覚えている範囲で書いています...
コメントは書こう
ゆめみさんの中の方が、「仕様書が常に手元にあるとは限らないのでコメントは書きましょう」のようなニュアンスで仰っていたような気がしますので、ここから先は個人的見解を書いていきます。
今回の課題を例に説明しますと、プレイヤー名は昇順で表示するといった仕様はコード内に記載しておくことで、仕様書が無くても他の人が見た時に伝わりやすくなります。
また、関数の動きについても書いておくと、関数が何を表しているのかが、さらに伝わりやすくなります！
型の定義をしよう
(ここから個人的見解)
Pythonは型の定義が不要な言語ですが、関数の引数の型や戻り値が変わってしまうと、それだけでプログラムが動かなくなってしまうリスクがあります。
PyCharmというエディタの場合、明示した型と違うものを利用すると警告を出してくれるので、型を間違えるヒューマンエラーが防止できます。
また、明示することにより、明示していない時に比べて、コードを見た際の理解のしやすさが大幅に向上します。
エラーハンドリングをしよう
(ここから個人の見解)
本課題以外でも、誰かに何かを提供した時に、想定した通りに利用されるとは限りません。
今回の場合ですと、コマンドライン引数からCSVのファイル名を受け取る仕様となっていますが、権限がなかったり、そもそもパスが違っていたりして開けない可能性があります。
エラーハンドリングをして、変な使い方をされても優しい対処ができるようにしましょう！！
メモリ使用量を抑えよう
(ここから個人の見解)
今回の課題の１番大きなテストケースのCSVファイルサイズは1.5GBを超えています。
これを１行ずつ配列に格納してしまったら...そのままメモリを消費することになってしまいます。
今回の課題を例に説明しますと、日付データなども入力に含まれていますが、実際に欲しいデータは「プレイヤーID・平均点」のみです。
それを算出するためには、合計点と件数さえあれば計算できます。私の場合は1プレイヤーごとにPlayerクラスを作成して合計点と件数を格納しています。
そうすることによって、メモリ使用量を12MB未満に抑えることに成功しました。
要件をしっかりと分析し、取捨選択をすることが大切です。
定数を活用しよう
(ここから個人の見解)
今回の場合、10位以下のプレイヤーを出力するとなっていますが、デバッグする際や、仕様変更などで値が変わるかもしれません。その「10」という値をネストしたプログラムの中に埋め込んだとしたら...変更したい時に探すのが手間となってしまいます。
定数として分かりやすい場所に定義しておくことで、保守性や可読性が向上・改善します。
フィードバックを受けて改善した成果物
import csv
import itertools
import sys
from argparse import ArgumentParser, Namespace
from typing import Dict, List
# CSVに対応するカラム名
PLAYER_ID_COLUMN = 'player_id'
SCORE_COLUMN = 'score'
# 出力するプレイヤー数
REPORT_PLAYERS = 10
class Player:
"""
プレイヤー型。
省メモリを実現するために、辞書と併用して利用することを想定してプレイヤーIDは保持していません。
"""
_game_count: int = 0 # プレイしたゲームの数
_sum_score: int = 0 # 合計スコア
def getAverage(self) -> int:
"""
プレイヤーの平均スコアを計算します。
:return: 四捨五入された平均スコアです。
"""
if self._game_count == 0:
# 0除算対策
return 0
# 平均を返す
return round(self._sum_score / self._game_count)
def addResult(self, score: int):
"""
ゲームの結果を追加します。
:param score: 加算するスコア
"""
# ゲームのカウント値とスコアを変動させる。
self._game_count += 1
self._sum_score += score
class RankingObject:
"""
ランキングに使用するオブジェクト。
同一スコアのプレイヤーをまとめるために使用します。
"""
score: int
player_ids: List[str]
def __init__(self, score: int, playerIds: List[str]):
self.score = score
self.player_ids = playerIds
def get_option() -> Namespace:
"""
プログラムに渡された引数を取得します。
:return: 引数から渡された値
"""
_arg_parser = ArgumentParser()
_arg_parser.add_argument('fileName', type=str, help="利用するCSVファイル名を指定します。")
return _arg_parser.parse_args()
def readCsv(fileName: str) -> Dict[str, Player]:
"""
CSVからプレイヤーのデータを読み込みます。
:param fileName: 読み込むCSVファイルのパス
:return: 読み込み結果をPlayerオブジェクトに入れた辞書型ファイル。
"""
records: Dict[str, Player] = {}
with open(fileName, 'r') as f:
reader = csv.DictReader(f, lineterminator="\n")
for row in reader:
# CSVの内容を読み込む
player_id = row[PLAYER_ID_COLUMN]
score = int(row[SCORE_COLUMN])
# レコードがないなら作成する
if player_id not in records:
records[player_id] = Player()
# レコードの内容を書き換える
record = records[player_id]
record.addResult(score)
return records
def mean_by_player(records: Dict[str, Player]) -> Dict[str, int]:
"""
プレイヤーの平均点を求めた辞書を作成します。
算出方法はPlayerクラスに依存します。
:param records: プレイヤー一覧のレコード
:return: プレイヤーごとの平均点の辞書
"""
means: Dict[str, int] = {}
for player_id, record in records.items():
means[player_id] = record.getAverage()
return means
def make_ranking(means: Dict[str, int]) -> Dict[int, RankingObject]:
"""
平均点によるランキングを作成します。
同一点数のものは、同一順位として扱います。
:param means: プレイヤーごとの平均点の辞書
:return: 順位ごとのRankingObjectの辞書
"""
# 点数降順でソートする。
sorted_means = sorted(means.items(), reverse=True, key=lambda x: x[1])
# 点数ごとにまとめる。
grouped_means = itertools.groupby(sorted_means, lambda x: x[1])
ranking_data: Dict[int, RankingObject] = {}
rank = 1
for score, player_id_and_score in grouped_means:
# groupByしたアイテムの中から、player_idを引っ張り出してくる。
player_ids = list(map(lambda x: x[0], list(player_id_and_score)))
# 反映させる。
ranking_data[rank] = RankingObject(score, player_ids)
rank += len(player_ids)
return ranking_data
def output(rankDict: Dict[int, RankingObject], report_players: int = REPORT_PLAYERS):
"""
ランキングを出力します。
同じ順位のプレイヤーが複数いる場合は、プレイヤー名昇順で出力します。
:param rankDict: ランキング形式の辞書
:param report_players: 発表するプレイヤー数
"""
print("rank,player_id,mean_score")
printed = 0
for (rank, ranking_obj) in rankDict.items():
rank: int
ranking_obj: RankingObject
# 出力した件数が出力する順位以上であれば抜ける。
if report_players <= printed:
break
# プレイヤー名昇順で出力する。
sorted_player_ids = sorted(ranking_obj.player_ids)
for player_id in sorted_player_ids:
print(f"{rank},{player_id},{ranking_obj.score}")
# 印刷件数を変える。
printed += len(sorted_player_ids)
def main():
# コマンドライン引数の読み取り
args = get_option()
file_name = str(args.fileName)
# CSVを読み込む（エラーがあれば出力する。）
error = True
records: Dict[str, Player] = {}
try:
records = readCsv(file_name)
except FileNotFoundError:
print(f'"{file_name}"というファイルは見つかりませんでした。', file=sys.stderr)
except IsADirectoryError:
print('ディレクトリが指定されています', file=sys.stderr)
except PermissionError as e:
print('権限がありません\n', e, file=sys.stderr)
except ValueError as e:
print('CSVの値に想定外のものがありました\n', e, file=sys.stderr)
except Exception as e:
print('CSVの読み込みに失敗しました\n', e, file=sys.stderr)
else:
error = False
if error:
# エラーがあったのであればプログラムを終了する。
sys.exit(1)
player_mean_scores = mean_by_player(records)
ranking_dict = make_ranking(player_mean_scores)
output(ranking_dict)
if __name__ == "__main__":
main()
ゆめみさんが用意してくださった回答例(模範解答)を参考に、クラスを導入するなどして個人的に100点の回答が出せるコードが完成しました。
コメントとかちゃんと他人に伝わるように書いている自信があるので、ぜひこのコードを読んでみてください。
（突っ込みなどあればコメントで教えていただけると幸いです。）
改善点
この辺は全て個人の見解です。
DictReaderを利用した
csvの読み込み時に、DictReaderを利用することにより、カラム名が記載されている１行目を飛ばして読むことが可能になります。
また、行を読み込む時にもカラム名が使用できるため、CSVファイルの仕様変更に対応しやすいです。
グローバル変数を使わないようにした
いままで一時的な値はグローバル変数に格納していましたが、先ほど解説した通りテストのしやすさなどが大きく損なわれるため、main関数内で変数として取り扱うようにしました。
関数の構成を変更した
- 改善前
- 引数読み取り
- (CSVの)フォーマットチェック☆
- 辞書からプレイヤーを取り出す☆
- CSVの行を受け取って処理する☆
- CSVを読み込む☆
- 辞書をソートする
- 結果を出力する
- 改善後
- 引数読み取り
- CSVの読み取り★
- 平均点演算
- ランキング作成
- 出力
さて、改善することによって関数ごとの責務が良い感じになったのではないかと思います。特に、改善前はデータの流れがごちゃついていましたが、改善後は「順番に実行していったらなんとかなりそう」という感じがすると思います。
そして、CSV系の処理が１つにまとまりました。(☆が★にまとまった。）
テストのしやすさが落ちるのではないか？？と疑問に思うかもしれませんが、CSVの読み取り自体がDictReaderを使うリファクタリングによってシンプルになったことから、一つの関数としてまとめることが可能になりました。
細かくしすぎるとコードが読みづらくなる反面、テストがしやすくなるので、トレードオフの関係といってもいいかもしれません。適切な粒度を見つけるスキルが大事なのかなと思います。
また、平均点演算やランキング作成といった具体的な責務を関数に割り振ることにより、より分かりやすいコードを実現できたのではないかと思います。
定数を使用した
先ほども説明しましたが、１０位以内とするために使用する10という値を定数にしました。
型アノテーションを厳格にした
改善前も型アノテーションをして、型を明示していましたが、dictの中身まで明示することにより、可読性・保守性を高めました。
groupbyを活用した
模範解答 からパクった を参考にしたのですが、groupbyを利用することによって、点数をキー、プレイヤー一覧を値とする辞書を作成できるようになり、同一点数は同じ順位とする仕様の実装がシンプルになりました。
まとめ
今回のワークショップ参加を通じて、実務向けのより良いコードの書き方を学べました。
反対の個人開発では、自分だけのコードなので自由に書くことができますが、実務向けコードでも可読性向上等といった観点があり、個人開発に取り入れるべきことがたくさんあると思います。
思い切って他人に読んでもらう・使ってもらう想定でコードを書いてみることで、自身の生産性・技術力・クオリティが上がるかもしれません。
ちなみに私はキノコの山派の犬派です。マックはエグチが好きです。彼女を愛しています。LGTMよろしくお願いします。