ベトナム人のソースコードレビュー

今日は久々にソースコードレビューをしました。
日本側チームが相変わらず現場をわかってないので、レビュー用ベトナム人追加の許可が下りず私がやることに・・。
私がレビューする事にどれだけオーバーヘッドがかかると思っているのか、そろそろ4時間くらい説教しようと思います。

さて、そんなこんなで久々にレビューをしたんですが、なんか6000行もソースコードがあります。
J2MEで6000行って結構だなぁと思いながら見ていったんですが、
ぱらっと50項目くらい指摘出来そうでした。ただ、先述のオーバーヘッド問題から50個も指摘すると一日かかっても終わらないので、出来る範囲でということで10個くらいレビューする事にしました。

以下。

命名規則は合わせましょう

int menu_ID;
char tiengViet;

私自身変数名なんてどうでもいいじゃんって思ってる人間なので、実はあんまり気が進まないんですが、レビューといえば定番なので単語の区切りでアンダーバー使うのかキャメルケースにするのか統一しましょうねという話が一つ目。

1とか2とかやめましょう

menu15Canvas
catMenu11
img1
img2

こういう名前がたくさんあるんで、これやめてちゃんと名前つけるかせめてなんかArrayっぽくするかしましょうというのが二つ目。

ラップするならちゃんとしましょう

static void log(Object value) {
  System.out.println(value);
}

こういうラップ関数作っているのに、時々System.out.printlnが直接呼ばれてるんで、ラップするのかしないのかはっきりしましょうという話が三つ目。

メンバー変数の位置はそろえましょう

class A {
 int age;
 public void init() {
 }
 String name;
}

こういうのが多いんで、メンバはメンバで固めましょうという話が四つ目。

無駄なコメントは消しましょう

// if (a == 0) {
if (a == 0 && b == 1) {

こういうコメント多いので、コメント残す場合はそのコメントがなぜ残っているか書くか、いらないんなら消しましょうという話が五つ目。
この方針は多分賛否あります。ちなみにベトナム人PGはコメント残しまくります。彼らはSVNやCVSを使ってても、毎日作業コピーのバックアップをとる人が多いとか、ちょっと効率の悪いマメさがありますね。

日本でも大手さんはとりあえず古いのはコメント残せってとこありますよね。私は嫌いです。
ちなみにこういうのはありです。

// debug mode: if (a == 0) {
if (a == 0 && b == 1) {

Exceptionの種類くらい出しましょう

} catch (Exception ex) {
log("loi exception");

Exceptionキャッチするのはまあ止めないんですけど、せっかく例外出してくてるんだからgetMessageするとか、せめてクラス名くらい出した方がいいと思いますよという話が六つ目。

PascalCaseの変数名はやめましょう

int StringMenu = 0;
String G2;

こういうのあったんですが、ちゃんと合わせましょうという話が七つ目。

if連発はやめましょう

if (a == 0) {
}
if (a == 1) {
}

switchかせめてelse ifにしましょうという話が八つ目。

createImageを毎回やるのはやめましょう

void paint(Graphics g) {
 Image img = Image.createImage("/Footer.png")
 g.drawImage...
}

遅いんでやめましょうというのが9つ目。
ちなみに上記は描画命令ごとに毎回画像生成からやっているんですが、このアプリにはシーン管理があるのでシーン切り替わりのタイミングで画像作っておけばいいのです。が、ここまでは説明出来ませんでした。

関数化しましょう

if (a == 0) {
 img1 = Image.createImage("1-1.png");
 img2 = Image.createImage("1-2.png");
} else if (a == 1) {
 img1 = Image.createImage("2-1.png");
 img2 = Image.createImage("2-2.png");
} else if (a == 2) {
 img1 = Image.createImage("3-1.png");
 img2 = Image.createImage("3-2.png");
}

もうちょっと関数化しましょうという話が最後です。

void initImage(String prefix) {
 img1 = Image.createImage(prefix+"-1.png");
 img2 = Image.createImage(prefix+"-2.png");
}

void run() {
 initImage(new String(a + 1));
}

javaしばらく触ってないんでnew String違うかもですが。

まとめ

今社会人一年生も指導しているので、こういう初歩的なところまで私が通訳介して話さないといけないんだと、結構しんどいですよという話でした。
海を挟むとこのレベルの問題提起が伝わらないので、なかなかのわびしさがありますね。

今に始まった事ではないんですけども。