退職エントリ(2/3)：出会わなかったプログラマーに、祈りを
酒の提供をめぐって一転二転、万物流転している世の中でありますが、それは客先常駐エンジニアも例外はないのであります。いくら良い仕事をしても契約期間が過ぎれば現場からは引き離されてしまい、その後を見届けることはできません。
しょっちゅう人が入れ替わるタイプの現場では技術水準を保つことも難しく、後続の人員がせっかく作ったものを台無しにしてしまうこともあります。
僕の入っていた現場ではベースとなる機能をインターフェースとして用意し、必要な処理を実装するという後々の変化を見越した柔軟なつくりをしていました。
実装部分のコードについても、最初につくった人の書き方はこなれていてスッキリとしたコードだったのです。
しかし改修の時期になって別の人が手がけた部分については見るも無残なありさま。異様にネストの深い処理ブロック、可読性の悪い条件式、不必要なtry-catchなど、最初に書いた人とは明らかに別人、素人が書いたなと一目でわかるひどさで、ベース部分の洗練された記述からのあまりの落差、目を覆いたくなるような乱雑さに、名画がズタズタに引き裂かれたような沈痛な心持ちになりました。
一例を挙げましょう。文字列が空・NULLでなければ処理を行うというよくある条件式です。
if(str != "" && str != null) {
正常処理
}素直といえば素直な書き方ですが、これは以下のように書いたほうがすっきりします。
<JavaScriptの場合>
if(!str) {
正常処理
}
<Javaの場合>
if(!StringUtils.isBlank(str)) {
正常会処理
}元の書き方でも別にいいじゃん何が問題なの？という方のために申し上げますと、この判定式、ソースの中でしょっちゅう出てくるんです。なので非常に見づらい。以下のような状態を想像してみてください。
if(str1 != "" && str1 != null) {
正常系処理
}
if(str2 != "" && str2 != null) {
正常系処理
}
if(str3 != "" && str3 != null) {
正常系処理
}
if(str4 == "" && str4 != null) {
正常系処理
}
if(str5 != "" && str5 != null) {
正常系処理
} 記述量が多くて読むのが大変ですね。そしてお気づきでしょうか。str4の行の記述が他の行と微妙に違うことに。
これ、書いた本人以外の人が読んだら単なる書き間違いなのか仕様通りの記述なのか一見しただけでは判断がつきかねると思います。つきかねました。
こういった判断の微妙な部分の確認に時間をとられて改修が遅々として進まないなんてこともありうるのです。いわゆる保守性が悪い状態です。
ただ、これは空・NULL判定の簡潔な書き方を知らなかっただけで一度身につけてしまえば以後間違えることはないでしょう。しかし次の書き方は擁護不可能です。
!"1".equals(str) && str.equals(NULL){
正常系処理
} コーディングでフリースタイルはやめろ。
否定を文頭に持ってくるのもステータスのハードコーディングも百歩譲って許すから、せめて変数の記述位置は左右どちらかに統一してたもう……。
極めつけはユーザー登録ツールです。CSVファイルに記述されたユーザー情報をデータベースに登録するツールなのですが、これがなんとJavaのmainメソッドに処理がすべてベタ書きしてあったのです。
public static void main (String[] args){
変数宣言
ひたすら処理が続いている
}サブルーチンという概念に真っ向からケンカを売るパンクなコーディングスタイルには目が飛び出そうになりました。ところでこのツール、1,000件程度のデータ登録に3分もかかるんですけどどういうアルゴリズム組んだらこんなに時間のかかる代物*1になるんですかね。
*1 スペックの低いPCでデータの暗号化処理をしているほか、もともとのデータベース設計が拙劣であるという可能性も高そう
以上のように開発経験の少ない僕でも一目でわかるクソコードの一部を紹介してきました。保守性・可読性を考えない、動けばいいやとのやっつけコードの数々に悲憤慷慨し、このコードを書いたのは誰だあ！と海原雄山もかくやといった気持ちになりましたが、いくつかのソースコードを読むうちに(とても苦痛でした)ある疑念が湧いてきました。
どうしてこんな明らかなクソコードが放置されているのだろう。
気になって先輩エンジニアに尋ねてみたところ、なんとこの現場ではコードレビューの類いを一切やっていないというのです。驚きました。そしてあのコードを書いたエンジニアが気の毒にもなりました。
確かにあれはまごうことなきクソコードであるのは間違いないのですが、曲がりなりにも動くものは作れているのです。手を動かす馬力は間違いなくあるのです。
それによくよく見ると微笑ましい間違え方をしている箇所もありました。例えば拡張for文を使った以下のような処理ブロック。
int loopCount = 0;
for (object item : items) {
loopCount++;
処理
} 拡張for文なのになぜかループカウンターが用意されています。処理の中で条件式の判定に使ったり配列の添え字として使っているわけでもないようでした。一見すると意味不明です。
しかし僕は初心者に近いので実装者の気持ちか推察できます。きっとこの人は、多少はプログラミングを扱えるようになってきて、ちょっと気取った書き方をしてみたくなったのでしょう。
プログラミングの手ほどきを受けたことのある方なら覚えがあると思います。"Hellow World !"を表示させ、変数と四則演算を覚え、if文を覚え、ループ処理を覚えてプログラムがだんだんサマになってくると楽しくなって、次も次もと新しいことを身につけたくなってきます。
その意欲的な心理状態で、ループ処理には通常のfor文のほかに拡張for文があるということを知って、実際に使ってみたくなったのではないでしょうか。新しいおもちゃを手に入れたらそれで遊んでみたくなる。気持ちはよくわかります。それに拡張for文、なんか書き方がスマートに見えますもんね。イケてるエンジニア感を出すために使ってみたくなるのもよくわかりますよ。
でもちゃんと理解しないまま使ってしまった。通常のfor文の単なる別表現にすぎないと誤って理解しているのでループカウンタが必要だと考えたのでしょう。
同様のことが例外処理の実装でも行われています。
public Object method(String str) throws IOException {
try {
正常処理
} catch (IOException ex) {
例外処理
} メリッド宣言の際に例外のスロ一宣言をしているにもかかわらず、処理ブロック内で同じ例外を処理する記述をしています。
スロー宣言をするということは、例外が発生した際に指定した例外クラスに処理を任せるということですから、自クラスのメソッドでスロー宣言した例外クラスの処理を書く必要はありません。
この記述からも、実装者がなまなかな知識でコーディングしていることがうかがえます。
ただ、「よくわからけどとりあえず使ってみる」という姿勢はそれほど責められる態度ではないと思うのです(少なくとも新人～若手レベルなら)。どれだけ机上で泳ぎ方を学ぼうが実際に水に入ってみなければ泳ぎが身につかないのと同じように、実際に書いてみないと機能や仕様がわからないことも多々あるでしょう。
特に初学者のうちは手持ちの知識から起こりうる現象を推量するのが難しいため、とりあえず手を動かす能力は貴重だと思うのです。
この実装者も、経験が浅いなりに手を動かして、なんとか動くものを作ろうという必死さは感じられるし、実際にとりあえず動くものは作れています。
しかしコンパイルが通り、機能的には仕様通りの動きをしているために、かえって自分ではコードの拙劣な部分には気づきにくいようになっている。なんかイケてないなと感じられればむしろ上出来ではないでしょうか。
そこで改良に重要になってくるのか先達からのレビューや指導のはずですが、先述したようにこの現場では一切コードレビューを行っていません。品質管理を放棄しているのです(一部上場の大企業がです)。
誰かが指導役としてついていれば多少は改善されていたでしょうから、実装者の所属していた会社は、促成栽培のエンジニアを単騎で最前線に送るような限界ソフトハウスであっただろうと推察できます。
適切な指導者さえいれば腕のいいプログラマーになれただろうに、フィードバックの機会が与えられないまま、自分のなにが良くてなにがダメなのかわからず五里霧中で作業に従事されられた彼(or彼女)のことを考えると気の毒に思わずにはいられません。
この人がまだコードを書いてるかはわかりませんが、もう少し人権のととのった環境で気持ちよく作業ができていることを祈ります。
おまけ
読みやすいコードを考えるにあたって『リータブルコード』という書籍を参考にしました。
初心者から上級者までどのレベルのエンジニアが読んだとしても収穫があると思いますのでおすすめです。開発現場に一冊備えておくと嬉しい。
この記事が気に入ったらサポートをしてみませんか？