作っているソフトのソースコードのレビューをしてもらった。

KIMATA RobertHisasi @robert_KIMATA

https://t.co/X9fBEqW0nZ 書いてる最中は「これ共通化できる」と思ってどんどん共通化して、後日あまりに汎用なパーツを前に脳内デバグに失敗することが多々

ありさん@Web系かTesterで求職中 @arigayas

@arigayas @d_toybox 1番目から複数個NGコメントになってるというのはニコ生で放送が途絶えた場合にたまにXMLファイルが別ファイルになる場合があってそのために対応しないといけないんです

なかのんの旅々(Masayuki Nakano) @d_toybox

@arigayas NGコメント達の次のコメント(つまり、見つかったコメント)って、正常時の処理、行われてなくない?

なかのんの旅々(Masayuki Nakano) @d_toybox

@arigayas 123..6って入力されてた場合の、6の時、ぱっと見、4と5にNGコメントって突っ込むけど、6を処理処理せず、ループまわしてない?

なかのんの旅々(Masayuki Nakano) @d_toybox

@arigayas Cっぽく書くと、 if (IDがスキップしてた) { FillListWithNGComment(&List); } else if (IDが巻き戻ってた) { continue; } 現在のコメントを処理; って感じじゃないとおかしくない?

ありさん@Web系かTesterで求職中 @arigayas

@d_toybox そうかもです・・・。　なので書き直したほうが良い気がしてるんです・・・。

なかのんの旅々(Masayuki Nakano) @d_toybox

@arigayas 典型的に、べたっと書くから出てくるバグやね。内容を小分けにしてたら、こういうミスはすぐに気付くし、メソッドの呼び出し元を訂正するだけで修正できる。

ありさん@Web系かTesterで求職中 @arigayas

@d_toybox とある放送のコメントデータをダウンロードして1,2がNGだった場合の処理を考えていないことに気がついてそれに対応するために強引に突っ込んだのでバグを生んだようです＿|￣|○

なかのんの旅々(Masayuki Nakano) @d_toybox

@arigayas Delphiって汚く書くのが苦手な設計になってる。例えば、ブロックスコープの変数を作れないとか。だから、ブロックをできるだけ作らず、ブロックの中身を他のメソッドに追い出すと綺麗になるよ。あとは、early returnした方がすっきりする。

なかのんの旅々(Masayuki Nakano) @d_toybox

@arigayas 最初の if FileExists(Filename) then なんて、逆にして、即メソッドから出てしまえば良い。

なかのんの旅々(Masayuki Nakano) @d_toybox

@arigayas このループ内で、何するか判断するのに必要な変数って、「現在のコメントID」「記録済みコメント数」の二つだけよね。そう考えると色々とおかしい。

ありさん@Web系かTesterで求職中 @arigayas

@d_toybox どういうことですか？

ありさん@Web系かTesterで求職中 @arigayas

@d_toybox たしかにｗ

なかのんの旅々(Masayuki Nakano) @d_toybox

@arigayas if not FileExtists(FileName) then exit; でよくない?

ありさん@Web系かTesterで求職中 @arigayas

@d_toybox exit; ってソフト終了ですか？

なかのんの旅々(Masayuki Nakano) @d_toybox

@arigayas curID, lastID if (curID <= lastID) continue; if (curID > lastID) FillListWithNGComment(...); AddComment(...); こうなりそう。

なかのんの旅々(Masayuki Nakano) @d_toybox

@arigayas return だっけ。Delphi忘れた(￣△￣；)

なかのんの旅々(Masayuki Nakano) @d_toybox

@arigayas まあ、異常事態からはとっとと、メソッドから出るようにしておけば、正常時の処理に集中できるやろ?

ありさん@Web系かTesterで求職中 @arigayas

@d_toybox ですね。

なかのんの旅々(Masayuki Nakano) @d_toybox

@arigayas しかも見にくいインデントを削減できる

ありさん@Web系かTesterで求職中 @arigayas

@d_toybox ｆｍｆｍ

なかのんの旅々(Masayuki Nakano) @d_toybox

@arigayas まあコツとして、最初に書く時って、あれせなあかん、これせなあかんって考えるやん? そのひとつひとつを最低、一つのメソッドで表現することにして、そいつらの呼び出し元の、骨組みから書いてしまえば良い。

なかのんの旅々(Masayuki Nakano) @d_toybox

@arigayas その、各やらなあかんことが複雑なら、さらに、そのためにあれやらなあかん、これやらなあかん、と考えて、同じ手法で分割していく。

ありさん@Web系かTesterで求職中 @arigayas

@d_toybox 今回の場合、後から徐々に増えたパターンです・・・。

なかのんの旅々(Masayuki Nakano) @d_toybox

@arigayas メンテナンスで機能追加なら、単純に元に組み込むんじゃなくて、新しいメソッドしにて、元のコードは呼び出すだけ、結果によって処理を継続するかどうか判断するだけ、みたいにすれば良い。そしたら、元々あるコードはバグらない。