こんにちは。船井総合研究所のいっちーです。
はじめに
本記事ではGeeklyさんの広告を題材にしていますが、同社様に対する批評的なことを目的とした記事ではありません。
クイズ広告
SNSなどでよく流れてくる広告に、このようなものがあります。
このコードスニペットのまずいところを指摘してください、というGeeklyさんのクイズ広告です。実際の動画ではこの画面の直後に答え(に近いヒント)が出てきます。おそらく本記事を読まれている方は、すぐに正解にたどり着ける方だと思いますので、その前提で考察していきます。
深堀りしてみよう
Geeklyさんの題意はおそらく「金額計算はDecimal型を使うべき」というものだと思われますが、品質の観点からさらに深堀りしていきたいと思います。
その1 ヘッダコメントは？
この動画はエンジニアの採用広告の一部なので、このコードも「何らかの納品物という想定である」という前提で扱います。
すると、「ヘッダコメントがない」という指摘が真っ先に来ます。一時的な使い捨てツールでもない限り、ヘッダコメントにその関数の仕様をきちんと書くべきで、この関数であれば「請求金額を合計し、小数点以下2桁で返す。端数は四捨五入する」といったことを書いておくべきです。そうしないと、この処理がそもそも正しいのか間違っているのか、どの程度間違っているのかも分かりません。
そもそも「請求金額の合計」という処理名自体が間違いだったり(実際、私も、最初に小数が出てくるので、一瞬、消費税計算かと勘違いしました)、仮に金額計算でも「浮動小数点の誤差程度であれば許容する」ような雑な要件であれば、「何も問題ない」という答えも正解になってしまいます。
私なら、ビジネスロジックのコードにヘッダコメントが無い時点で、そこから先は読むことなく差し戻すと思います。
その2 itemsのデータ構造は大丈夫？
totalが小数になっているのは、もしかしたらドルのような、小数を前提とした通貨だからかも知れません。すると、itemsで渡されてくる請求データのデータ構造にも問題がないか？という心配が出てきます。この関数の実装自体にも問題がありますが、問題の源泉は引数で渡されるデータの型にあるのかも知れません。
その3 未使用変数を残さないで！
itemsからnameという変数をアンパックしていますが、これがその後の処理で使われていません。実際にはIDEが何らかの警告を出すと思いますが、使わない変数は削除する(たぶん請求データのレコード群から必要なカラムだけを切り出したタプルとして渡される前提の処理だと思うので、タプルの項目から除外するか、_で使わないことを明示する)べきです。
そもそも、itemsの要素が「name price quantity」という値を纏めたデータ構造なら、タプルではなく辞書型で定義するべきですね。
その4 バグを隠蔽？
計算したtotalを小数点以下2桁で丸めて処理結果としていますが、そもそも浮動小数点型で計算しているので、その誤差を覆い隠してしまう、という処理になってしまっています。これは、実質的にバグの隠蔽であり、傷口を広げる行為に他なりません。
あれ？こんなコメントが透けて見えてきたぞ？？？
# 時々ズレるので、四捨五入する。
return round (total, 2)
繰り返しになりますが、金額項目は10進数型(通貨型が提供されていれば、通貨型)で扱わないといけません。また、今回は掛け算でしたが、割り算を含む処理は、「どのタイミングで割り算をする」「端数はどのタイミングで、何桁目をどうやって丸める」を要件として明文化しておかないといけません。そうしないと、未来の誰か(もしかしたら、自分自身かも)から未来永劫恨まれることになります。
品質には気をつけよう
この例だと数行のコードなので簡単に読み切れますが、仕事で作る実際のコードはこの10倍以上長いので、こういった基本ができていないと、それが積もり積もってとんでもない技術負債を後世に残し、多額の保守費用を無駄遣いすることになります。どんな小さなことであっても、基本は大事にしたいものです。
まとめ
GeeklyさんのSNS広告から、品質について考えてみました、というお話でした。Geeklyさん、記事のネタをありがとうございました。
それではまた。