4年前に作ったWindowsアプリのコードをセルフレビューしてみた

15578 ワード
C# クソアプリ WindowsForm C# テキストリンク

クソアプリ2 Advent Calendar 2018の6日目です。

はじめに

4年前に作成したNicoTagCleanというWindows向けアプリケーションのコードを久々に見る機会が出来てしまったので自分で自分のコードにセルフツッコミを入れていきます。
平成最後にクソ”アプリ”というよりクソ”コード”を置いていこうという魂胆です。

「NicoTagClean」 is 何？

ニコニコ動画への動画投稿者向けの自動タグ消しアプリです。
それしか出来ません。
作成したきっかけの概略としてはニコナレに投稿した「動画のタグ管理ツールを作ったときの話」にお任せするとして、相手がツールを使用して動画にタグを付けるならこちらもツールで対抗するという超短絡的発想で生まれたものです。
大多数の人には縁の無いという意味ではある意味クソアプリと言える筈。

クソコードを眺めてみる

前置きはここまでにして早速いくつか眺めていきましょう。

オブジェクト指向そんなもの無かった

サブフォームにある動画に付いたタグを取得するメソッド
private void GetVideoTag()
{
    Invoke(new DelegateClass.DelegateVideoTagStatuLabel(setStatusLabel), "読み込み中・・・");
    Invoke(new DelegateClass.DelegateVideoTagProgressBar(setStatusProgressBar), true);
    string s;
    s = StringInput("watch/", "\r\n", _videoId + "\r\n");
    var webRequest = (HttpWebRequest)WebRequest.Create(@"http://www.nicovideo.jp/tag_edit/" + s + @"/?res_type=json");
    webRequest.CookieContainer = _cookie;
    webRequest.Method = "GET";
    webRequest.DownloadStringAsync().Subscribe(SetVideoTagtoList);
}

もうこの時点でわかるクソ具合？その事実に気がついてしまったあなたは5D100でSANチェックです。
このコードはアプリケーション内で動画を選択すると、サブフォームが起動する際に動画についているタグを取得するためのコードなのですが、問題は・・・・

メインフォーム側のコード
private void GetVideoTag(string videoID)
{
    string s;
    s = StringInput("watch/", "\r\n", videoID + "\r\n");
    string url = @"http://www.nicovideo.jp/tag_edit/" + s + @"/?res_type=json";
    var webRequest = (HttpWebRequest)WebRequest.Create(url);
    webRequest.CookieContainer = _cookie;
    webRequest.Method = "GET";
    webRequest.DownloadStringAsync().Subscribe(json => SetVideoTagtoList(json, url), exception => showCommunicationErrorMessage(exception.Message));
}

いやお前なんでメインフォーム側にも同じコード書いているんだよ、しかも微妙にコード違うし。
言い訳すると当時オブジェクト指向な考え方なんてうるせーーーしらねーーーFINAL FANTASYだったものでこういう時はクラスするとかそんな考えは全く無かったのです。若気の過ちって奴ですきっと。
ちなみにこのコード、仕様変更があり今は動きません。（気が向いたら直します）

コメントがクソ

先ほどのコードのメソッド GetVideoTag なのですが、当然？このメソッドにも一応コメントが添えられているのですが

/// <summary>
/// 動画情報取得
/// </summary>
private void GetVideoTag()
{
   省略
}

oh...
もはや処理内容と全く合ってません、罠です。これならいっそコメントを削除するか、トートロジーコメントとなりますが

/// <summary>
/// 動画のタグ一覧取得
/// </summary>
private void GetVideoTag()
{
   省略
}

と書いた方が5000兆倍マシですね・・・
一体誰だこんな酷いコードを書いた人物は、親の顔が見たいわ。（巨大ブーメランが刺さる音）

（今思えば）逆に難しいことしてないか？

どのご家庭でも見かけるHTMLタグにもついている

<div id="hoge" data-hoge="fuga">テスト</div>

と言う風に data- から始まるdata属性ですが、これに欲しい情報がJSONで帰ってくるのでこれまたどのご家庭にもあるHTMLパーサーとどのご家庭にもあるJSONパーサー（場合によってはエスケープ処理が必要）でやってしまうのが 普通 ですが、当時data属性？JSON？HTNLのパーサーもまともに扱えないのに？という酷い状態だったので、次のようなメソッドを作成して、ろくに前処理もしていないHTMLを食わせて筋肉式で解決してました。

/// <summary>
/// 文字列と文字列の間を抽出して結果を文字列で返す
/// </summary>
/// <param name="Strart"></param>
/// <param name="End"></param>
/// <param name="TextMain"></param>
/// <returns></returns>
protected string StringInput(string Strart, string End, string TextMain)
{
    try
    {
        return TextMain.Substring(TextMain.IndexOf(Strart) + Strart.Length, TextMain.IndexOf(End) - TextMain.IndexOf(Strart) - Strart.Length);
    }
    catch
    {
        return "Error";
    }
}

仮にパーサーを使わない縛りだったとしても正規表現を使用した方がもう少しスマートだったかもしれません。
（なおこの記事を書くために改めてコードと解析対象のHTMLを眺めてようやくデータ形式がJSONであることに気がついた模様）

雑なエラー処理

埋め込まれた認証情報を取り出そうと先ほどのStringInputメソッドを使用している部分が以下の様になってました。

private void getNicoKey(string str)
{
    NicoToken = StringInput("csrfToken&quot;:&quot;", "&quot;,&quot;isPeakTime", str);
    if (NicoToken == "Error")
    {
        NicoToken = StringInput("csrfToken&quot;:&quot;", "&quot;,&quot;v&quot;:&quot;", str);
    }
    NicoAuthKey = StringInput("watchAuthKey&quot;:&quot;", "&quot;,&quot;", str);
    GetVideoTag();
}

処理からして勘の良い方ならわかると思いますが、このエラー処理雑です。
失敗する可能性を秘めているのに失敗しても処理が続行します、アホです、誰に向かってこんなクソコードを見せているんだ。
そのため最終的に取得失敗しても気にせず処理を続けるためこの先の処理で認証トークンが不正でコケます。
失敗しているならとっとと適切なエラーハンドリングを行ってあげるべきですね・・・・

そいつはそのタイミングの処理で良いのか？

サブフォームにある動画に付いたタグを取得するメソッド
private void GetVideoTag()
{
    Invoke(new DelegateClass.DelegateVideoTagStatuLabel(setStatusLabel), "読み込み中・・・");
    Invoke(new DelegateClass.DelegateVideoTagProgressBar(setStatusProgressBar), true);
    string s;
    s = StringInput("watch/", "\r\n", _videoId + "\r\n");
    var webRequest = (HttpWebRequest)WebRequest.Create(@"http://www.nicovideo.jp/tag_edit/" + s + @"/?res_type=json");
    webRequest.CookieContainer = _cookie;
    webRequest.Method = "GET";
    webRequest.DownloadStringAsync().Subscribe(SetVideoTagtoList);
}

再びこのコードです、このメソッド冒頭でInvoke経由でUIを操作しているので別スレッドで動作するものであることがわかると思いますが、ちょっと待ってください。
ここで行っている処理としてはUIに配置されたLabelに文字の代入と、UIに配置されたProgressbarの表示です。
つまり 別スレッドでの呼び出し前に処理してしまえば不要な処理 であると言えると思います。
どうしてこうなった
とはいえそこまでパフォーマンス等を重視するアプリでも無かったため、なんだかんだで放置されてしまうのであった。

おわりに

クソアプリが生み出されるのと同じぐらいもしくはそれ以上にクソコードも生まれてると思うのです。
そんなクソコードでももし残っているなら閲覧してみると、自分の成長を実感できるかもしれません。