ゲッターを使わないで変数にアクセスする(java)

質問

経験10年以上のベテランの人が
privateフィールドに対してゲッターを使わないで、if (!checkExist(this.productCode))としていました。
自分は未経験ですが、privateフィールドのアクセスはゲッターを使うべきだと思いましたし、“!”を使うのは分かりにくいのでif (checkExist(getProductCode()) == false)としました。

どちらの方が良いコードですか？

回答No.2

hanabutako · ベストアンサー

ベテランの書き方のほうが自分はいいと思います。

何かを金科玉条のごとく信じるほうが簡単で楽ですが、時間が経つとそういう根拠がわからない金科玉条は有害でしかありません。getter / setterを常に使うという金科玉条があるようですが、その盲信を自分は有害だと思います。

そもそも、getter / setterって何のためにあるのでしょうか？
自分は内部のデータ構造を外部に見せないためのカプセル化やData Abstractionの実現のためにあると思います。
ありがちな例で恐縮ですが、座標を表すPointクラスを実装することを考えましょう。
直感的にもわかりやすく、画面へのプロットもしやすいので最初は直交座標系 (x,y) で作るでしょう。
そして、しばらくした後、回転をサポートしないといけなくなりました。今後は回転をよく使うプログラムになりそうなので、極座標系 (r,θ)で作ったほうがよさそうです。
getter / setterがある場合、このクラスの書き換えだけで内部データを極座標系に変更できます。しかし、getter / setterを使わず、クラス外から直接xやyを参照、代入されている場合はxやyを使っているプログラム全部を極座標系対応のコードに書き換える必要があり、大規模修繕が必要になります。
自分はこの状況を避けるためにgetter / setterがあると思います。

同一クラス内の場合、そのメンバーや実装に変更をする場合は必然的にクラス全体の変更を余儀なくされるでしょうからgetterに拘る必要はないと思います。this.productCodeがgetProductCode()に単純に置き換わるなら、なおさらです。それに、そこが壊れた場合ユニットテストで簡単に見つかりそうです。

"!"をboolean型に対して使うのはいいと思います。if文が条件が満たされない時に発行されると見た瞬間に理解できますので。
余談ですが、プログラムを読むときは段々英文を読む感覚で読むようになってきますし、普通checkは条件が成立する場合はtrueを返すので、if (!checkExist(this.productCode))だと、if not exist productCode と読めます。これが、if (checkExist(getProductCode()) == false)だと、if exist check for gotten productCode is falseとなります。まず、余計なgetが読みづらいですし、否定があとに出てくるので更に読みにくいです。

というわけで、ベテランになればなるほどベテランの書き方になってくると思います。また、そちらのほうが可読性が高いと思います。

回答No.8

中村 拓男（@tknakamuri）

No.７ の補足。

私が使っているコーディング標準ではベテラン氏の書き方でないとダメですね。

それと checkExist は私が使っているコーディング標準の命名規約では違反になってしまいます。
exits か productCodeExists　でないとぺけです。

理由は英語として不自然だからです。

この規則はメソッド名が名刺から始まる得る例のひとつになっています。

回答No.7

中村 拓男（@tknakamuri）

>if (!checkExist(this.productCode))

普通だと思います。private のフィールドが属すクラス内でまで
setter/getter にこだわるのは変。目的を振り返りましょう。
必要に応じて使い分けましょう。

>if (checkExist(getProductCode()) == false)

見慣れないので見にくいですね。! の方が好きです。
でも Pascal 系の not はもっと好きです(^^;

回答No.6

root139

この質問には、下記の2つの問題が含まれているかと思います。

a. private フィールドに対するアクセッサ(getter, setter)使用の是非
b. if文においての boolean メソッド・フィールド・変数の扱い

まず、aのアクセッサの件ですが、個人的な意見としては、使う必要性が特に無い場合は直接アクセスでも問題無いと思います。もちろん遅延初期化／キャッシュなどを行っている場合はアクセッサを使用する必要が出てきますが、必要になった段階で変更すれば良いでしょう。
アクセッサを使う事でプロパティの実装に関するコードを局所化できるというメリットは有りますが、private フィールドの変更が負担になるほどクラスが肥大化しているなら、そっちの方が問題かと思います。

著名なプログラミングに関する本でも杓子定規にアクセッサを使用することは無いという意見がが多い様に感じますが、スコット・W・アンブラーはアクセッサ使用派ですね。
http://www.alles.or.jp/~torutk/oojava/codingStandard/writingrobustjavacode_pidid93_c3.html#doc1_id785

次に、bのif文やwhichなどにおいての boolean メソッド・フィールド・変数の扱いですが、こちらも「== true」「== false」は付けない方が分かりやすいと思います。
下記の様なコードを考えてみたとき・・・

----------------------------------------
List<?> bookList = new ArrayList<Book>();

if (bookList.isEmpty()) { // (1)
　// ・・・
}

if (bookList.isEmpty() == true) { // (2)
　// ・・・
}
----------------------------------------

(1) は「If bookList is empty ・・・」(もしブックリストが空ならば、・・・)と素直に読めますが、(2)は「If bookList is empty equal true ・・・」(もし『ブックリストが空』が真と同じならば、・・・)の様な回りくどい表現に感じます。確かに「!」が入ると素直には読めなくなりますが、それでも「== false」を付けるよりはストレートに条件を表現していると思います。
否定形よりも肯定系の方が分かりやすいという面はありますが、文全体を見て判断する必要が有ります。日本語で考えても「今日でなければ」という表現を「『今日であれば』が違っていたら」と否定形を使わずに言い直しても、かえって分かりづらいでしょう。

また、間違えて「=」にした際もコンパイルエラーにならない場合がある点も嫌ですね。

例) ------------------------------------
if (hoge = false) { // コンパイルエラーにならず、このブロックは絶対に実行されない
　// ・・・
}
----------------------------------------

確かに、if文やwhich文の条件式に比較演算子が入らないと分かりづらいと感じる人は少なくないようですが、Javaのコーディング標準に良く見られる「bool値を返すメソッドは、is + 形容詞、もしくは三単元動詞とする」という形式は、明らかに比較演算子を使わない書き方を意識しているものと思われますので、Javaのコーディングに限ってはその様な形式に慣れてもらった方が良いでしょう。

長々とコードの形式の是非を書いた後で恐縮ですが、この様な書き方に関する問題はコーディング標準やプロジェクトの慣習に従っていることの方が重要で、もしベテランの方が1人だけコーディング標準から外れる書き方をしているなら、そちらの方が悪いコードという事になります。

回答No.5

Tacosan

個人的には「ベテランの人」の方が好みだけど....

どっちにしても「checkExist」ってメソッド名が気にいらない. #2 では「普通checkは条件が成立する場合はtrueを返す」って書いてあるけど, 私はそこまで他人 (昨日以前や明日以降の自分を含む) を信用できない. むしろ
if (! existProductCode())
とかしちゃうなぁ. ただ, exist なのか have なのか has なのかで困るかも.

回答No.4

notnot

「なぜ、privateフィールドにgetterを使わないでアクセスできるの？？」と思ったら、同クラス内の話ですか。
他の方の書いているように、ベテランの方の書き方が正しいです。
＞if (checkExist(getProductCode()) == false)
は、間違いとまでは言いませんが、変。

また、そもそも、getter/setterと言っている時点でオブジェクト指向での設計が足りない（クラスを構造体のように思っている）です。

回答No.3

mintia007

外部クラスからフィールドをアクセスする時にサクセッサメソッドを持たせるのでは？

＞if (checkExist(getProductCode()) == false)
だとパッと見で外部クラスのgetProductCode()を呼び出していると思ってしまうのは自分だけかな？
（「オブジェクト名.」が指定してないので自クラスって分かるけどね）

thisの方が自クラスのフィールドって速攻で分かるので。

但し、ゲッターでフィールド値の取得だけではなく、何かしら判断文などの処理が入るようなら別ですが。

会議とかでコーディング規約について論議しても良いですし、品証・品管部門とかがあったら相談してみては？

回答No.1

wormhole

＞privateフィールドに対してゲッターを使わないで、if (!checkExist(this.productCode))としていました。

それだけではなんともいえないと思います。
getterがprivateフィールドの値をそのまま返すとも限りませんし。
ケースバイケースじゃないかと。

＞“!”を使うのは分かりにくいのでif (checkExist(getProductCode()) == false)としました。

別にわかりにくくないと思いますけど。
むしろ
if (checkExist(getProductCode()) == false)
と書く人は
if (checkExist(getProductCode()) == true)
と書きそうなのがいや。