アンチパターンといわれたがどうすればいいかわかりません。

Question

以前、このteratailである質問の回答をしたら他の方にこのように言われました。

「ローカル変数が多いことが非常に気になります。ローカル変数はいわば『メモ』の働きをしますから、あまりに多いと本当に重要な情報が埋もれてしまうのです。

ローカル変数を減らすためには、メソッドチェーンやそれに準ずるもの(一時変数の積極的利用)、適切なサブルーチン化などが考えられます。
特にフラグを必要とする処理は、アーリーリターンを駆使すれば簡潔に書ける場合が多いです。

また、変数をスコープの頭でまとめて宣言するのも一般にはアンチパターンとして知られますので、避けておいた方が良いかと思います。」

javaでは変数をスコープの頭でまとめて宣言するのはなぜダメでしょうか?
これだと、コードを書くとき「この変数を使おう」と思ったときその行に変数を宣言することに
なるのですか?(つまり変数宣言とかつて宣言した変数の処理が混ざる)
C言語だと最初に変数を宣言するのですかJavaでは違うのでしょうか?
どなたがご教授お願い致します。

<追記>

int []data = {3,4,5};
int sum = 0;
for(int i = 0; i < data.length; i++){
   sum += data[i];
}
int average = sum / data.length;

int []data = {3,4,5};
int sum = 0;
int average;
for(int i = 0; i < data.length; i++){
   sum += data[i];
}
average = sum / data.length;

Comments

[退会済みユーザー]

元々の質問が分かりません。リンクの追加をお願いします。あと、すでに解決済みのようですが、元々の質問の回答コメントにも本件のリンクを貼り付けては？指摘した方からの回答がもらえるのが一番良いと思います。

[LouiS0616]

https://teratail.com/questions/119051 これです。

Answer 1

アンチパターンといわれたがどうすればいいかわかりません

アンチパターンであっても動いているのであればまずはいいのです.

ただ、「アンチパターンである」もしくは「〜に見える」と指摘された際は, リファクタリングの時期に来ているとお考え下さい.

つまり、

ローカル変数が多いことが非常に気になります

は現在のコードにおけるブロック粒度がそれなりに大きく、処理内容をより細かいものに分解できるのではないか？という提案であり,

変数をスコープの頭でまとめて宣言するのも

も, スコープ全体が冗長に見えている査証です.

結局kaztoさんがおっしゃっているとおり「単一機能のコードが短くなれば自ずとローカル変数の数は少なくなるし、変数の定義場所についても気にならなくなる」が真相です.

Comments

[退会済みユーザー]

回答ありがとうございます。

Answer 2

javaでは変数をスコープの頭でまとめて宣言するのはなぜダメでしょうか?

上記の疑問に対する答えです。恐らく、下記の事を言っているのだと思います。

public class Hoge {
    public static void main(String[] args) {
        int localCounter;
        //ここではlocalCounterを一切使っていない処理が100行あったとする。
        //・・・・・・・・・・・・・・・・・・・・
        //・・・・・・・・・・・・・・・・・・・・
        //・・・・・・・・・・・・・・・・・・・・
        //・・・・・・・・・・・・・・・・・・・・
        //処理終わり。
        //ここで初めて何らかの演算をする。
        localCounter = localCounter + 1;

        return localCounter;
    }
}

とすると、上記のプログラムを初めて読んだ人がlocalCounterを調査しようとした時、一切使っていない処理100行の中でもその変数が使われているかどうかを意識しなければいけません。(・・・・処理の中でもlocalCounterが使えるので)

一方で、下記の様にすると

public class Hoge {
    public static void main(String[] args) {
        //ここではlocalCounterを一切使っていない処理が100行あったとする。
        //・・・・・・・・・・・・・・・・・・・・
        //・・・・・・・・・・・・・・・・・・・・
        //・・・・・・・・・・・・・・・・・・・・
        //・・・・・・・・・・・・・・・・・・・・
        //処理終わり。
        //ここで初めて何らかの演算をする。
        int localCounter;
        localCounter = localCounter + 1;

        return localCounter;
    }
}

localCounterを宣言した後のみ意識すればよいので、プログラムを読むときに意識する行数を大幅に
減らすことが出来ます。この場合、中間の100行は何の意識もしなくて良いですね。

変数宣言とかつて宣言した変数の処理が混ざる

ここが上手く理解できなかったのですが、どの様なプログラムの書き方をイメージされての質問でしょうか？
もし、回答内容に齟齬があれば、質問への追記をお願いします。

Comments

[退会済みユーザー]

回答ありがとうございます。「変数宣言とかつて宣言した変数の処理が混ざる 」について
言いたいのは<追記>の1つ目のコードのように
1.3つの変数の合計を求めます。
2.変数averageを宣言する
このようにするとかつて宣言した変数[]dataの処理をするところと変数averageを宣言することで
処理と変数の宣言が混ざるという意味です。わかりにくい説明ですみません
(2つ目のコードは変数をスコープの頭で宣言する1つ目との比較のためのものです。)

[motuo]

なるほどですね。上記のサンプル程度の規模であればどちらでも良いと思います。
ただ、上記のintの宣言とforとavarageの計算までがそれぞれ離れる(途中に多くの処理がある)と、上記のパターンに当てはまってしまいます。人によっては可読性が低いと感じるかもしれませんね。
その場合は、他の回答者様の意見にもありましたがメソッドを分割したりする事で、おのずと解消される事があります。

[退会済みユーザー]

学校の授業でC言語を習ったとき変数を最初にまとめて宣言すると習ったのでその癖がついて
しまったと思います。わかりやすい回答ありがとうございます。おかげで無事解決できました。

Answer 3

結局、物事の善し悪しなんか、正当な理由があるかどうかです。
頭でまとめて宣言する理由って何でしょうか？
その理由が正当なものであるならば、全然問題ないと思いますし、
その理由が正当なものでなかったとしても、自分なりの理由があるということが重要です。
僕の場合は、頭でまとめて宣言するメリットは全くないと考えます。

maisumakunさんのコメントにある通り
かつてのC言語の制限やJavaScriptの巻き上げなどのような、特別な理由がJavaにはないので、
ローカル変数は使う時に宣言して、スコープはできるだけ小さくすべきだと思っています。
影響範囲を小さくしていきたいからです。

単純に読みにくい

変数が宣言されてから、その変数が使われるまでに何百行とあったりすると、
その変数の状態変化を気にしなければならない範囲が広すぎてうんざりします。
で、結局何も使われていないと分かるとドッと疲れます。
motuoさんの回答がとても分かりやすいです。

まだいらないのに宣言して、結局いらないまま終わる

ちょっといい例が思い浮かばなくて変なコードなんですが、例えば以下のような感じです。
単なるメモリーの無駄遣いです。
使う前に宣言と適切な初期化をすべきです。

public List<String> toList(String arg) {
	List<String> tempList = new ArrayList<>();
	List<String> resultList = new ArrayList<>();
	Map<String, String> tempMap = new HashMap<>();
	if (arg == null) {
		// tempList, resultList, tempMap が無駄になる
		return null;
	}
	/*
	 * tempListにゴニョゴニョする
	 */
	if (tempList.isEmpty()) {
		// resultList, tempMapが無駄になる
		return null;
	}
	/*
	 * tempMapにゴニョゴニョする
	 */
	if (tempMap.containsKey("error")) {
		// resultListが無駄になる
		return null;
	}
	/*
	 * ようやくresultListを構築し始める
	 */
	return resultList;
}

Answer 4

変数をスコープの頭でまとめて宣言するのも一般にはアンチパターンとして知られますので、避けておいた方が良いかと思います。

私はC言語で育った人間なので、このアンチパターンには懐疑的です。

ローカル変数を減らすためには、メソッドチェーンやそれに準ずるもの(一時変数の積極的利用)、適切なサブルーチン化などが考えられます。

が適切にできていれば、自然とローカルスコープの変数の数も減るだろう、と思っています。

Comments

[maisumakun]

C言語で「途中からの変数宣言」が公認されたのはC99からと後付けですし、JavaScriptの変数宣言は書いた場所より上でも使える「巻き上げ」が発生します。

そういう言語的な背景がなければ、特にJavaでいうfinalのような、一度代入してそれ以降変化しない変数については、宣言と代入を適切な位置で一気に済ますほうがいいと考えます。

[退会済みユーザー]

回答ありがとうございます。

[kazto]

概論と各論が入り混じってる気がしますが、私の言わんとしていることは
「ちっちぇぇことを気にすんな」です。大きな問題（＝関数化など）を先に潰すべき、ということです。

Answer 5

javaの標準入力された値をその後の入力された値と比べて結果を返すプログラムがうまく作れません。
この質問の回答だと思いますが。

ソースコードを見て気になった点を列挙します。

1. mainメソッドに全部記述されているのでテストがしずらい。

メソッドの分割しすぎもよくありませんが、制御の流れが変わっている時点(入力、出力）で該当の処理を関数化したほうが読む人の負担が減ります。
0. flag 変数の命名と宣言場所が適切ではないかと、勝敗の判定を表すjudgeが適切ではないでしょうか。
0. dataとsampleの比較部分はforループで回すか、LouiS0616さんの回答の通り基数を掛け算して比較するという手も使えます。

見やすさと初学者さんへのわかりやすさもソースコードの一つの指標ですが、
個人的には**コードがテストしやすいかどうか（テスト容易性）**も重要なポイントではと個人的に思います。

---

2018/04/05追記
質問文の追記ソースコードは変数:averageと変数:sumをint型で宣言しているため、dataが{1,2}の時に算術平均が1となり正しく算出されません。
変数をdouble型もしくはjava.math.BigDecimalクラスで宣言するか以下のコードでも可能です。

import java.util.stream.IntStream;

public class A120426 {
	public static void main(String[] args) {
		int[] data = { 1, 2 };
		double average = IntStream.of(data).average().orElseThrow(null);
		System.out.println(average);
	}
}

Answer 6

宣言の位置もそうですが、その意見の元となった回答を見ると line line2 line3 が宣言されています。

多すぎます。
もっとどうにかなるはずです。

Comments

[Zuishin]

あと sample という配列と data という配列があり、無意味な名前の不必要なローカル変数の量産が可読性を落としているのは事実です。

変数の宣言位置の一般論ではなく該当ソースの複合的な問題です。

Answer 7

アンチパターンといわれたがどうすればいいかわかりません

まだまだ勉強中かと思いますので、それほど気にしなくてよいです。
多くのコードをみて勉強すれば、そのうち意識しなくても対処できます。

書籍だと、やはりリファクタリングが良いかと思います。
あと、webでおすすめは、間違ったコードは間違って見えるようにするは、いろいろためになると思います。

あなたが初心者プログラマか、あるいは初めて使う言語のコードを読もうとするとき、どの部分も同じように不可解に見える。そのプログラミング言語を理解するようになるまでは、簡単なシンタックスエラーにさえ気がつかない。

学習の最初のフェーズにおいて、私たちが「コーディングスタイル」と呼んでいるものを認識するようになる。

ハンガリアン記法についても、もしかしたら正しい使い方がわかるかもしれません。（すでに知ってるかもしれませんが）

ローカル変数が多いことが非常に気になります

こちらは、オブジェクト指向エクササイズが、オススメです。

コードを分割する時期になったら気にしてみては、どうでしょうか。

Comments

[退会済みユーザー]

回答ありがとうございます。

Answer 8

変数をスコープの頭でまとめて宣言するのも一般にはアンチパターンとして知られますので、

文字通りの意味であれば、「そんな事はないだろう」と思いますが、「ローカル変数のスコープに、ブロックスコープのある言語では、関数スコープだけでなく積極的にブロックを使って、ローカル変数のスコープを小さくしよう」ということなら、賛成です（限度はあると思いますが）。

また、該当プログラムでは、冒頭での宣言と同時に値の代入を行っていますね。プログラムの中身を読んでないのですが、「その代入は、そのタイミングが最適か？（プログラムを読む人の観点で）」という検討をされていないのであれば、検討すべきでしょう。

「代入タイミングを考慮した上で、初回代入時点で変数宣言すべき（宣言と初回代入を分けるべきでない）」という意図のコメントであれば、それはそれで有りだろうと思います。

Comments

[退会済みユーザー]

回答ありがとうございます。

Answer 9

C言語だと最初に変数を宣言するのですかJavaでは違うのでしょうか?

スコープの違いではないでしょうか。
C言語にはブロックスコープがありません。
コメントより指摘頂きました。
比較的新しいC言語の仕様では、ブロックスコープが存在するそうです。

変数の寿命は出来るだけ短くするのがベターです。
従いまして、C言語でもブロック毎に変数を宣言したほうが良いでしょう。

Comments

[maisumakun]

C言語も、{}でブロックスコープを作れる言語です。

[tekka]

え、マジですか。
時代は変わったんですね。。
だとしたら、先頭に宣言をまとめるのは
過去の習慣以上の意味は無いですね。

[dodox86]

{]ブロックの先頭で宣言すると言う意味ではC99よりもさらに前、ANSI-Cの時代からブロックスコープは実現できています。https://www.buildinsider.net/language/clang/01

[tekka]

そうでしたか。
曖昧な記憶で間違った回答をしました。すいません。

[iwamoto_takaaki]

この件については、誤解がある方も多いようです。（http://futurismo.biz/archives/1897）
提案なのですが、”回答に含めるべき内容は回答に追記しましょう”とありますので回答に勘違いだったことを追記するのはどうでしょうか？
間違いはマイナス評価に値しますが、よくある間違いの情報はプラス評価の対象だと思います。低評価を付けた方（見てくれてますか？）、回答が修正されたらマイナス評価を外してくれませんでしょうか？
よく間違える一人としてのお願いです。

[dodox86]

iwamoto_takaakiさんのご提案の主旨におおむね賛成します。当初は間違った回答だったとしても、補足／修正することで有益な回答群のひとつになると信じます。低評価のままですと埋もれてしまいますので。ただ、質問は本来 Javaに関するものですので、追記するのであれば混乱が無いように注意する必要がありそうですね。
（ちなみに私も時折間違えますし、本回答の低評価を付けた者ではありませんｗ）

[tekka]

ご指摘ありがとうございます。
修正しました！