くそコードが無くなる日を目指して

実例はC++/Javaがメインです|良いコードは健康を増進します

Category: コーディング

ソースコードを見てて最近気になったこと。
public void someCalculation(long number1, long number2) {
...
    // 最大公約数を計算する
    long a = number1;
    long b = number2;
    while (b != 0) {
        a = b;
        b = a % b;
    }
    long gcd = a;
...
}
みたいなコメント。
コメント以下の部分の説明をしていて、丁寧!ってことなんだと思いますが、あまりいけてないと思います。
いけてないポイントたち
1)コメントの対象がどこまでなのかはっきりしない。
2)コードが正しいかどうか検証しにくい
3)一行コメント書くくらいなら、名前をつければいい

なので、 こういうなんか以下は〜です系のコメントは、
public void someCalculation(long number1, long number2) {
...
    long gcd = computeGreatestCommonDivisor(number1, number2);
...
}

public long computeGreatestCommonDivisor(long a, long b) {
    while (b != 0) {
        a = b;
        b = a % b;
    }
    return a;
}
みたいにしたほうが良いと思う 。
こうすれば、
1) どこまでが計算なのかがはっきりする
2) メソッド化したので、ユニットテストできる
3) 結局コメントと同じ効果が得られる
 
じゃあどういうコメントが良いのか。
いろいろな意見はあると思いますが、何しているのかではなく、なぜこう書かれているのかを説明するコメント。
さっきの例ではこんな感じ。
public void someCalculation(long number1, long number2) {
...
    long gcd = computeGreatestCommonDivisor(number1, number2);
...
}

public long computeGreatestCommonDivisor(long a, long b) {
    // ユークリッド互除法を使って計算を高速化
    while (b != 0) {
        a = b;
        b = a % b;
    }
    return a;
}

 

三項演算子って嫌われる風潮がありませんか?
なんか特に理由もなく三項演算子は醜い、読みづらい→悪!的な・・・

個人的には使いどころ次第だと思うんですよね。
三項演算子NG!と言ってる人はたぶんこういうのを気にしてるんですよね
int number = (high > low) ? highNumber : (middle > low) ? middleNumber : lowNumber;
三項演算子がひたすらつながっているようなもの

確かに三項演算子はつなげたら読みにくいので、良くないと思います。
でも繋げないときはむしろいいことのほうが多いので、個人的には(繋げないという条件付きで)三項演算子は使うのを推奨します。
例えば、初期化するときに
int number = 10;
if (condition) {
    number = 20;
}
int number;
if (condition) {
    number = 20;
} else {
    number = 10;
}
より
int number = (condition) ? 20 : 10;
のほうが良いと思いませんか?
だって、最初のは一度10を入れてるけど、条件によっては20がその後で代入されてて、それって初期化なの?初期化って最初に入れる値が一意に定まってるべきじゃない?って感じですし、2つ目のは初期化するのに5行も使うの????って感じじゃないですか。
まだ、上のは一つしかないから耐えられるかもしれないですけど、2つ以上初期化したかったらどうするんですか?
int number = 10;
if (condition) {
    number = 20;
}
int number2 = 20;
if (condition2) {
    number2 = 30;
}
int number3 = 30;
if (condition3) {
    number3 = 40;
}
...
みたいに書くんですか???初期化に10行も20行も費やして良いんですか???number5を見る頃にはnumberがいくつに初期化されてるかなんて忘れてます…
そう考えると、三項演算子で
int number = (condition) ? 20 : 10;
int number2 = (condition2) ? 30 : 20;
int number3 = (condition3) ? 40 : 30;
の方が絶対いいですよね。初期化の行数も変数の数分だし。

あと最近みたんですが、こういうのも良くないと思うので書き直すべきです。
if (condition) {
    functionA(argument1)
} else {
    functionA(argument2)
}
...
を、
functionA((condition) ? argument1 : argument2)
...
のように。
これはただ短くなるからという話ではありません。
functionAは共通して適用したい操作なのにもかかわらず、条件分岐されて、二箇所に書かれているというところがポイントです。
つまり、conditionで分かれているのは引数だけなのに、functionAを適用するという操作まで、条件に入ってしまっているところが問題です。
共通だからくくりだすとはよく言われることなんですが、そもそも、何が条件によって分かれているの?っていうのを考えたらfunctionAは一箇所にしか現れないはずなんですよね。
まぁ、これは三項演算子とは少し違う話ですが。

とりあえず、このブログは三項演算子は結構いけてると思っています
 

いつも他の人のコードを見ていて思うんですが、みんな名前略すの好きですよね
例えばこんな感じ
int pass;
passって何?ってなりませんか?
いやpasswordでしょ・・・intだけど。って短絡的に思うのは危険です。
もしかしたらこんなふうになってるかもしれません。
if (pass == 1) {
    // 認証できている
}
パスワードじゃなかったのかよ・・・と。
短い曖昧な名前は書いた本人しか結局わからないんです。
Dai語とかと変わりません。IHって言われてI’m Happyとかわかるわけありません。
クッキングヒーターでしょ。。。

もちろんその前後をきちんと読めばわかるんですが、それって名前が分かりやすかったら必要ない作業ですよね?

略すのはやめましょう

 

クラスを継承したときの初期化(コンストラクタ等)と対になる終了処理(デストラクタ等)の話。

初期化の順番ってどれほど気にしているでしょうか?例として、
public class InitializePattern1 extends Parent {
    public InitializePattern1() {
        initializePattern1Class();
        super();
    }
}


public class InitializePattern1 extends Parent {
    public InitializePattern1() {
        super();
        initializePattern1Class();
    }
}

だったら、どっちが良いと思いますか?どっちでも良いと思いますか? 
前者はコンパイルエラーになります(コンストラクタなので、たぶん)。

なぜか。

親を継承した子クラスは親クラスが存在して初めて成立するので、
親クラスが準備出来ていない時に子クラスが何かする(この場合は初期化する)と、
親クラスの初期化されていないメンバにアクセスしたりして、予期しない動作を起こす可能性がでてきてしまうからです。
→だから順番が強制されているのです。(終了処理は必然的に順番が逆になります)
ちなみに親が子のことを予め知ることはできないので、親→子の順番で初期化するのは何も問題おきません・・・。
(こういう理由でprivateなコンストラクタを持つクラスは継承できないのです。つまり、コンストラクタにつく「private」は継承を禁止にするキーワードでは全くなく、コンストラクタがprivateだと呼べないから結果的に継承できなくなってるだけなのです)

この例はコンストラクタなので、当たり前なところがありますが、最近流行り?のAndroid界隈では結構こんなコードを見かけます。
public class SampleActivity extends Activity {
    @Override
    public onCreate() {
        doInitialization(); // 先に初期化しちゃってる
        super.onCreate();
    }

    public onDestroy() {
        super.onDestroy(); 
        doClosing();// 後で終了処理しちゃってる
    }
}
これは、コンストラクタやデストラクタ(C++)以外は、特に順番関係なく呼べちゃうからできちゃうんですね。
でも上で言った話があるので、本当は逆にしたほうがいいです。

最後に関連して一つ追加すると、
初期化と終了は対になっているはずなので、A->B->Cの順番で初期化したら
C->B->Aの順番で終了されているのが正しいです。
(もちろん互いに関係なければ順番は関係ないですが、お作法的な配置の話です)
public class SampleActivity extends Activity {
    @Override
    public onCreate() {
        super.onCreate();
        doInitializationA(); // Aを初期化
        doInitializationB(); // Bを初期化
        doInitializationC(); // Cを初期化
    }

    public onDestroy() {
        doClosingC(); // Cを終了
        doClosingB(); // Bを終了
        doClosingA(); // Aを終了
        super.onDestroy();
    }
}

まとめると、初期化は親→子、終了処理は子→親の順番でやるのが正しくて、順番が逆なのはなにも気にしていないか、よほどの理由があるときしかありえない。

ぜひ気をつけましょう。

 

名前付けは重要。そんなことはわかってるという人はたくさんいると思います。
でも、中身/内容が同じだったら全部同じ名前でいいと思っていたりしないでしょうか?

細かいことを気にしなければ、確かに中身が同じコード=同じメソッド名、変数名でもいいかもしれません。でもこのブログは 、くそコードが無くなる日を目指している手前、そんな中途半端なことを、やってOK!とは口が裂けても言いません。

具体的にどういうことか、ちょっと例を示しながら説明したいと思います。

TVが付いているという状態を表現したいとします。
特に何も考えないのであれば、
 
public class XXX {
    ...
    boolean mIsTvOn = true;
}
のようにするわけです。良さそうに見えます。
でもこれで問題ないかどうかはXXX次第です。
XXXがLivingRoomだったとします。
 
public class LivingRoom {
    ...
    boolean mIsTvOn = true;
}
リビングのテレビがついている。至って自然です。
ではXXXがTv(テレビ自身)だったらどうでしょう?
 
public class Tv {
    ...
    boolean mIsTvOn = true; // Tv自身がついている?それとも別のTvがついている?
}
少し混乱します。「TVがついている」という状態は、文章でみると無意識的に想像して、表現が同じになりそうですが、実際は誰から見るかで、表現のされ方が変わらなければいけないとわかります。
つまりTvから見れば、自身がついているときは、自分自身がTvなことは自明なので、 
public class Tv {
    ...
    boolean mIsTurnedOn = true; // Tv自身がついている
}
とすべきといえます。

一言でまとめると、「視点の違いで適切な名前は変わる」ということです。
こういった名前付けはメソッドでも同じです。

是非気をつけてみてください。 

↑このページのトップヘ