趣味プログラマの開発を手伝ったときに最初に何をしたか

背景

英語を母語とする米国人の趣味プログラマの友人から「自分で開発しているソースコードの改修をしたいが難しいので、手伝ってほしい」と頼まれた。
言葉の壁はあるが、コードを書く分には問題ないだろうと思い引き受けた

方針：趣味者でもメンテできるコード＋環境

友人は趣味プログラマで他のことで多忙なので、我々職業プログラマが割けるほど開発手法の習得に時間は割けない。なので「ＩＴエンジニア同士で開発する際の最適な手法」ではなく、「学習にコストがかからず、かつひとりでも管理しやすい」を目標にした

コードの解析

ファイルを共有してもらった。1500行ほどのpythonコードと、アプリが用いるデータのようであった。 1500行も書くのは根性あるなあ。
pythonコードをざっと眺め、危険そうな箇所はないことをなんとなく確認した。幾つか、職業プログラマがあまり用いない方法でコードが書かれている点があることに気づいた。
まず、ファイルには幾つか関数が定義してあるものの、メインの処理が700行程度トップレベルに書かれている。これをまともに追うのは難しそうだ。それから、コードのあらゆる個所に文字列や数字がハードコードされている。先頭の方にクラスの定義がしてある。友人はクラスが使えるらしい。
文字列は大方ファイルパスのようだったが、一部は何かのデータの文字列表現のようだった。
数字は...? マジックナンバーが多い。4や8が多いが、これは事前に聞いていた使い方からするに、４方位と８方位だろう。
メインの処理は

while True:
    Input = input('\n > ')
    x = Input.split(' ')

    if Input.split(" ")[0] == ...

といった調子で始まっている。恐らくこれは CLI の repl アプリでサブコマンドが幾つかある。
エッ。完全な趣味プログラマだと思ってたけどレベル高くない...?

ちょっと襟を正しつつ読み進める。関数名とかを見るに、結構やりたいこと自体に対するドメイン知識が要りそうだ。僕は手伝いづらいかもなあ...

全体的に、文字列でデータを取りまわして、文字列処理(replaceなど)で処理を進めているところがある。バグらせそうだからなるべくなくしたい
for 文が何重かになっていて、細かくif文で変数に boolean をセットしている箇所がある。多分 early return で書き直せそう。
型が欲しいので自分の開発だったら mypy を導入するところだけど、友人にとってはめんどくさいことが増えるだけかもしれない。

とりあえずこんなもんでいいかな。

コードを眺めてみた感想

友人は思った以上にコードが書ける人間だった。職業プログラマが実践しているプラクティスに対する知識はそれほどないようだけれども、目の前の問題に対処するそれなりに込み入ったコードが出来上がっている。大したものだ。

動かしてみようとする

VSCode の remote container 機能を用いて python が動かせる環境を用意し、python コードを実行してみた。
が、例外を吐いて落ちる。どうやら、windows 固有のファイルパスがハードコードされているせいのようだった。

なんとか python の動く windows 環境を用意して、実行してみる。REPLが立ち上がる。
ソースコード中に見えるサブコマンドをたたいてみる。しかし動かない。
どうやらサブコマンドには適切な引数を与えないといけないらしい。

友人にサブコマンドの引数を尋ねる。
その通りに叩いても動かない。どうやらサブコマンド同士に実行順序があるらしく、REPLの現在の状態により使えるコマンドが違うらしい。
使い始めてから使い終わるまでの、使い方の例を尋ねてみる。いくつかユースケースを教わり、その通りにサブコマンドを実行する。無事に動いた。

共同開発体制づくり

Git + GitHub 体制へ

バージョン管理を適切に行うために、GitHub にプライベートリポジトリを作ってもらい、そこにpushしてもらった。
開発は僕がそこへ PR を送る形で行うことにした。

Windows 以外で動かない問題の対処

Windowsでの開発は慣れていないので、 linux でも動作検証できるようにしたい。
しかし、いきなりソースコードを変更してデグレを起こすのが怖い。そこで、少なくともヒアリングしたユースケースたちが正しく動いていることを確かめるために、本体のコードに手を入れる前に統合テストを実装した。

統合テストの実装

統合テストは powershell script で書かれた簡便なものだ。REPLを起動し、ユースケース通りにコマンドを打ち、標準出力が期待通りか確かめるものだ。
powershell という慣れない script を書くのは苦労したし、そのうえでREPLに複数回入力を渡したり出力を確認したりするのは大変だったが、なんとかテストが書けた。
テストを実行する。 pass する。やったあ。
なんとなく実行する。今度は fail する。うん？

git 化していたおかげで原因がつかめた。このRepl はデータファイルを読み込んで動作するが、同時にリアルタイムで書き込みもするのだ。動作の一回目と二回目でデータファイルが書き換わっているため、出力が異なり、２回目はテストに fail した。 git 化のおかげで気づきにくそうな箇所を素早く発見できた。

データファイルディレクトリを統合テスト用にコピーし、都度そこからコピーして実行するようにした。今度は何度走らせても成功する。OK。

CI

リポジトリは僕だけではなく本人も更新する。そこで、 main ブランチにもCIがあり、破壊的変更に気づけるようにしたかった。
main ブランチ更新時にも先ほど作った統合テストが走るようにした。一度安全策を作ると、それをもとに大きな安全策が組み立てられるので便利だ。

ファイルパスの文字列リテラルが使われている箇所の切り出し

これで安心して本体の改修に入れる。
ファイルパスの文字列リテラルは、あちこちで使われており、しかもバリエーションがあった。
使われ方を洗い出し、ファイルパス組み立て関数に切り出した。Windows / Linux の違いを吸収できるように、ファイルパス結合の標準関数を用いるようにした。
組み立て関数に引数として渡されるファイル名も、即値で渡さずに変数として先頭に纏めて定義するようにした。タイポによるバグを防ぐためである。
実際共通化と変数化の過程で、タイポによるバグを見つけることができた。

ここまでの作業で Linux 上でも動作確認ができるようになった。ありがたい...

丁寧なPR

それぞれのステップで、今回の変更が何をしたか、どういうメリットがあるかをしっかり解説したPRを作った。友人は根気強く説明を読んでくれた。友人の親切さと辛抱強さに感謝する。

main 部分の改修

700行の主要部分がべた書きされているのを読み書きする根気は僕にはなかった（友人にはあるのに！）そこでこの部分を次になんとかした。

きちんとした引数パーサーを書いてもよいのだろうけれど、友人にとってブラックボックスを増やしたくないので今回は使わないことにした。

ただ使われ方からして、REPLで入力された標準入力を空白で分けて、一つ目がサブコマンド、残りが引数であるのは間違いないようだった。
そこでサブコマンドごとに関数に切り出し、 主要部分はサブコマンドを理解してそれぞれの関数を呼び出すだけにした。
これで 状態管理をだいぶシンプルにすることができた。

ここから細かいドメインロジックの改修に入って行った。

おわりに

以上が趣味プログラマの友人のコーディングを手伝ったときに最初に行ったことである。
職業プログラマではない相手のとれる勉強時間に配慮しつつ快適な開発体制を整える作業は楽しかった。こういう土台作りの部分の楽しさってあるよね..