質問をすることでしか得られない、回答やアドバイスがある。

15分調べてもわからないことは、質問しよう!

新規登録して質問してみよう
ただいま回答率
85.50%
JavaScript

JavaScriptは、プログラミング言語のひとつです。ネットスケープコミュニケーションズで開発されました。 開発当初はLiveScriptと呼ばれていましたが、業務提携していたサン・マイクロシステムズが開発したJavaが脚光を浴びていたことから、JavaScriptと改名されました。 動きのあるWebページを作ることを目的に開発されたもので、主要なWebブラウザのほとんどに搭載されています。

Q&A

解決済

4回答

3576閲覧

よくある範囲外判定コード、他の書き方を知りたいです。【たぶん基本】

himejiy3

総合スコア77

JavaScript

JavaScriptは、プログラミング言語のひとつです。ネットスケープコミュニケーションズで開発されました。 開発当初はLiveScriptと呼ばれていましたが、業務提携していたサン・マイクロシステムズが開発したJavaが脚光を浴びていたことから、JavaScriptと改名されました。 動きのあるWebページを作ることを目的に開発されたもので、主要なWebブラウザのほとんどに搭載されています。

0グッド

1クリップ

投稿2018/08/09 18:00

編集2018/08/12 10:34

座標(x, y)が(0,0)-(COL-1, ROW-1)の外にハミ出したら行き止まりだよ、という単純なコードなのですが、
こういうのって、もっといい感じの書き方があるのではないでしょうか。
(修正:
(記事タイトルの表現が今一つ良くなかったので修正させていただきました。
解決済である点に変更はありません。)

JavaScript

1 if (x < 0) { 2 x = 0; 3 } else if (x > COL - 1) { 4 x = COL - 1; 5 } 6 if (y < 0) { 7 y = 0; 8 } else if (y > ROW - 1) { 9 y = ROW - 1; 10 }

(修正:
(改行が多くて10行にも及んでいますが、だからと言って見づらいわけでもないようです。
しかしながら基礎中の基礎の文法で書いているため、これで良いのかという疑問がありました。)

当面はJavaScriptの答えを頂きたいのですけど、他言語でも結構です。
ご教授よろしくお願いします。

気になる質問をクリップする

クリップした質問は、後からいつでもMYページで確認できます。

またクリップした質問に回答があった際、通知やメールを受け取ることができます。

バッドをするには、ログインかつ

こちらの条件を満たす必要があります。

guest

回答4

0

読みづらいとは

JavaScript

1if (x < 0) { 2 x = 0; 3} else if (x > COL - 1) { 4 x = COL - 1; 5} 6if (y < 0) { 7 y = 0; 8} else if (y > ROW - 1) { 9 y = ROW - 1; 10}

私はこのコードは「無駄な処理がない」良いコードと思います。
読みづらいと感じる具体的なポイントはどこでしょうか。

短いコードが良いとは限らない

「コードは短い方が良い」といわれますが、これは正確ではなく、同じ処理で短く書けるならの前提付きと認識しています。
例えば、次のようにコードを書き換えたとして、

JavaScript

1x = x > COL - 1 && COL - 1 || x > 0 && x || 0; 2y = y > ROW - 1 && ROW - 1 || y > 0 && y || 0;

コードの行数は減りましたが、読む苦労は増えていると感じます。
なぜなら、x,yの初期値分の条件式/代入値は元のコードにはない「無駄な処理」で、それが頭の中でアルゴリズムを描くまての手間を大きくしているからです。
良いコードとはアルゴリズムが短いコードであって、実際に書くコードが短い必要はありません。

(2018/08/10 13:17追記)
短縮化したコードの 1,0 が期待通りに動作しない為、コードを修正しました。

Re: himejiy3 さん

投稿2018/08/10 03:59

編集2018/08/12 00:21
think49

総合スコア18156

バッドをするには、ログインかつ

こちらの条件を満たす必要があります。

think49

2018/08/10 04:11

> x = x < 0 && 1,0 || x > COL - 1 && COL - 1 || x; これ多分、 1,0 の箇所が期待通りに動かないですね。 今、検証出来る環境がないので、文脈で読み取って下さい。 他の方の回答をお借りすると、jun68yktさんが書かれたコードにも同じものを感じます。Math.min, Math.max は元のコードにない無駄な処理で、読む労力を増やす原因になっています。
think49

2018/08/10 04:21

短絡評価の最後に0を持って行けば良かったので、親記事のコードを修正しました(未検証、脳内シミュレーションのみ)。
himejiy3

2018/08/10 04:44

回答ありがとうございます。 >『私はこのコードは「無駄な処理がない」良いコードと思います』 なんと。そういうものですか。そう言われると何だかそんな風にも見えてきたような…? 後半の解説は「なるほど」と思いました。その例文コードは読むスピードがとても遅くなりますね。 今回は、if…else if…またif…else if…と、基礎中の基礎で固めたコードに「これでいいのだろうか」と感じたのですが、これでも問題は無く、関数化するかどうかさえもケースバイケースなのかもしれないと思いました。
himejiy3

2018/08/10 05:18

すみません、今回のベストアンサーはku__ra__geさんの回答にさせていただきましたが、 こちらのthink49さんの回答は目から鱗ものでした。 結局、元のコードはそのままにしておくと思います。
think49

2018/08/12 00:28

全体的にイメージに頼りすぎの感があるので、そこは改善ポイントだと思います。 「何だか見づらい」「もっといい感じの書き方」「何だかそんな風にも見えてきたような」 結局、読みづらいと感じる具体的なポイントはどこか、の質問に答えて頂けていないですよね。 問題が解決したから良しとするのではなく、「問題の解決手段はどうあるべきだったのか」を考えるべきではないでしょうか。 問題点は具体化した方が良いです。 私が過去に同様の疑問を持った時には「x= の代入文が2回あるのは冗長。1回の代入文に出来ないか。」という疑問を持ち、「出来ない(関数定義を除く)」の結論に至りました。 DRYを突き詰めると、同じようなコードは全て関数化する運命にありますが、呼び出し回数の少ない短いコードを関数化すると、呼び出し元関数まで目線を移動しなければならない関係で、かえって読みづらくなることもあります。 そこは個人のバランス感覚次第なので、絶対の指標はありませんが、私のこのコードなら関数化はしないスタンスです。 自分なりのバランスを設定して下さい。
guest

0

こんにちは。
ご質問に

もっといい感じの書き方があるのではないでしょうか。

とあります。何をもって いい感じ のコードとするかは、回答者の私見に委ねられているものと思いますが、私は 「ifelse を使わないで書いてみるとどうなりますか?」というお題と解釈したので、それに沿っての回答になります。

Math.minMath.maxを使って、以下でどうでしょう?

javascript

1x = Math.min(COL-1, Math.max(0, x)); 2y = Math.min(ROW-1, Math.max(0, y)); 3

上記はあくまでコード量の少なさ、見た目のコードの短さにこだわった書き方です。
参考までに、stackoverflow にも同様の質問(javascriptではないですが。)

が挙がっていて、こちらの回答の中にも、Math.MinMath.Maxを使う、以下の回答があります。

ですが、上記の回答に対するこのコメントで指摘されているように、一回余分な比較が発生する余地があります。

余分な比較を発生させず、かつ一行で書くのであれば、上記の投稿で、質問者さんが最初に

I would like to clamp a value x to a range [a, b]:

x = (x < a) ? a : ((x > b) ? b : x);

と書いているように、三項演算子の利用も検討されるとよいかと思います。


追記1

ご質問の冒頭に、

座標(x, y)が・・・

とありましたので、座標上の点を処理するコードのリファクタという観点で、以下を追記します。やることは以下です。

  • 座標上の点クラス Point を作成
  • Pointを拡張して、あらかじめ設定された最小値と最大値の範囲に収まるように各座標が調整されるRestrictedPointを作成
  • なお以下では、応用であることを明確にするため、Point インスタンスは、xyz軸を持つ3次元空間の点としています。

上記にそって、作ったコードが以下です。

javascript

1class Point { 2 constructor(x, y, z) { 3 this.x = x; 4 this.y = y; 5 this.z = z; 6 } 7} 8 9class RestrictedPoint extends Point { 10 constructor(x, y, z) { 11 super(x, y, z); 12 this.adjust(); 13 } 14 15 adjust() { 16 for ( const [axis, value] of Object.entries(this)) { 17 const config = this.constructor.config[axis]; 18 this[axis] = Math.min(config.max, Math.max(config.min, value)); 19 } 20 } 21} 22 23RestrictedPoint.config = { 24 x: { min: 0, max: 99 }, // X座標は0以上99以下 25 y: { min: 0, max: 199 }, // Y座標は0以上199以下 26 z: { min: 100, max: 399 } // Z座標は100以上399以下 27}; 28 29const p1 = new Point(-1, 230, 50); 30console.log(p1); // => Object {x=-1, y=230, z=50} 31 32const p2 = new RestrictedPoint(-1, 230, 50); 33console.log(p2); // => Object {x=0, y=199, z=100} 34

以下、上記を試行するサンプルです。
https://jsfiddle.net/jun68ykt/cs86vtj3/15/


追記2

もうひとつ蛇足ですが、上記のRestrictedPoint の書き方として、アクセサを使う手もありそうです。

javascript

1class Point { 2 constructor(x=0, y=0, z=0) { 3 this._x = x; 4 this._y = y; 5 this._z = z; 6 } 7 8 get x() { return this._x; } 9 set x(value) { this._x = value; } 10 11 get y() { return this._y; } 12 set y(value) { this._y = value; } 13 14 get z() { return this._z; } 15 set z(value) { this._z = value; } 16 17 toString() { 18 return `(x: ${this.x}, y: ${this.y}, z: ${this.z})`; 19 } 20} 21 22class RestrictedPoint extends Point { 23 24 static config() { 25 return { 26 x: { min: 0, max: 99 }, // X座標は0以上99以下 27 y: { min: 0, max: 199 }, // Y座標は0以上199以下 28 z: { min: 100, max: 399 } // Z座標は100以上399以下 29 }; 30 } 31 32 get(axis) { 33 const conf = this.constructor.config()[axis]; 34 return Math.min(conf.max, Math.max(conf.min, this[`_${axis}`])); 35 } 36 37 get x() { return this.get('x'); } 38 set x(value) { this._x = value; } 39 40 get y() { return this.get('y'); } 41 set y(value) { this._y = value; } 42 43 get z() { return this.get('z'); } 44 set z(value) { this._z = value; } 45 46} 47 48const p1 = new Point(-1, 230, 50); 49console.log(`${p1}`); // => (x: -1, y: 230, z: 50) 50 51p1.x = 1000; 52console.log(p1.x); // => 1000 53 54const p2 = new RestrictedPoint(-1, 230, 50); 55console.log(`${p2}`); // => (x: 0, y: 199, z: 100) 56 57p2.x = 1000; 58console.log(p2.x); // => 99 59

以下に上記のコードを上げました。

https://jsfiddle.net/jun68ykt/cs86vtj3/73/

アクセサを使う利点としては、上記のコードの最後の2行に書いてあるように、p2.x = 1000; と代入したつもりでも、 p2.x は X座標の上限値の99を返すようにできることです。

投稿2018/08/09 19:11

編集2018/08/11 00:42
jun68ykt

総合スコア9058

バッドをするには、ログインかつ

こちらの条件を満たす必要があります。

himejiy3

2018/08/10 04:00

回答ありがとうございます。 おー、これは後々でも使い道がありそうな書き方ですね。 追記2のほうは私でも読めます。今後はこういう書き方も増やしていこうと思います。 追記1のほうはadjust()の中がパッと見で分からずネット検索するレベルです。
jun68ykt

2018/08/10 05:02 編集

コメントありがとうございます。 Math.max と Math.min は覚えておいて損はありませんが、stackoverflow の以下の投稿(javascript ではありませんが同じテーマです。) Where can I find the “clamp” function in .NET? https://stackoverflow.com/questions/2683442/ では、関数Clampを作るという回答が一番得票数が多いです。if を使うかわりに、3項演算子を使った、 public static int Clamp(int value, int min, int max) {   return (value < min) ? min : (value > max) ? max : value; } もそれなりの得票数を取ってますね。
himejiy3

2018/08/10 05:11

英語ページは翻訳ソフトに頼らざるをえない英語力なので、なかなか大変です(苦笑) しかし例え趣味のプログラミングでも、どこかで必ず英語の壁に当たるので翻訳ソフト万歳。 3項演算子を使う方法は、飛ばし読みする程度のコードを書く場面で利用するかもです。
jun68ykt

2018/08/10 06:20

> 3項演算子を使う方法は、飛ばし読みする程度のコードを書く場面で利用するかもです。 はい。それでよろしいかと思います。 いくつかの書き方の選択肢があったときに、ご自身(および、職業プログラマーの場合は、自分の属す開発チームの人たち)が思うところの「読みやすさ」や「より良いコード」の基準に合わせて、採用するコードを選択していけばよろしいかと思います。
guest

0

こういう書き方もできますよ。

js

1x = Math.min( 2 Math.max(x, 0), 3 COL - 1 4) 5y = Math.min( 6 Math.max(y, 0), 7 ROW - 1 8)

追記

他の回答でも言われているように関数化することでコードの見通しが良くなります。

js

1const clamp = (n, min, max) => 2 Math.min( 3 Math.max(n, min), 4 max 5 ) 6x = clamp(x, 0, COL - 1) 7y = clamp(y, 0, ROW - 1)

投稿2018/08/09 19:10

編集2018/08/10 00:30
退会済みユーザー

退会済みユーザー

総合スコア0

バッドをするには、ログインかつ

こちらの条件を満たす必要があります。

himejiy3

2018/08/10 04:02

回答ありがとうございます。 Math.min、Math.maxを使う発想は全く無かったので、ひとつ勉強になりました。
guest

0

ベストアンサー

おとなしく関数化しましょう。

x = LimitRange(x, 0, COL-1); y = LimitRange(y, 0, ROW-1); function LimitRange(value, min, max) { if (value < min) { return min; } if (value > max) { return max; } return value; }

投稿2018/08/10 00:10

編集2018/08/10 02:49
ku__ra__ge

総合スコア4524

バッドをするには、ログインかつ

こちらの条件を満たす必要があります。

himejiy3

2018/08/10 03:48

回答ありがとうございます。 これは分かりやすいですね…! 最近はifやswitchを使わないコードが流行りとなっているようですが、私はまだまだ使わせてもらいたい段階なので、易しい回答があってホッとしました。 プログラミングの世界では「1年前の自分は他人」と言いますが、このコードなら1年後の自分でも理解できそうです。
guest

あなたの回答

tips

太字

斜体

打ち消し線

見出し

引用テキストの挿入

コードの挿入

リンクの挿入

リストの挿入

番号リストの挿入

表の挿入

水平線の挿入

プレビュー

15分調べてもわからないことは
teratailで質問しよう!

ただいまの回答率
85.50%

質問をまとめることで
思考を整理して素早く解決

テンプレート機能で
簡単に質問をまとめる

質問する

関連した質問