ifのネストが深いので改善するべきか

Question

前提・実現したいこと

勉強でコードを書いています。
if文のネストが深くなってきたので可読性が悪いのでは無いかと書き直しています。
独学の為コードの可読性が低かったり汚かったりするのではないかと心配です。

どのように改善するべきなのか現場で働いている方の感想やアドバイスを頂きたいと思い投稿致しました。

-追記-
mts10806さんからご指摘を受けて今動かしてみているコードをそのまま追加してみました。
指定日から表示するコードは配布されていたのですが、毎週表示するコードは配布されていませんでした。
作成のついでに配布用のコードを作って私みたいに検索した人の手助けになればと作成しているのですが、恥ずかしくないコードにしたいなと思い今回の投稿に至っております。

-追記２-
think49さんのからご指摘を受けて気付きました。
上側コード
$(elem).foreach(function(){
の部分を修正致しました。

このサイトの使用方法のアドバイスも承りました。
他にも利用方法で誤った使用方法があれば、今後の利用に役立てたいと思いますのでご指摘お願い致します。

-追記３-
think49さんからご指摘がありましたので追記致します。

上のコードは下のコードを質問用にシンプルにした（つもり）ものでしたがうまく伝わらないようなので下のコードを掲載致しました。
以降、上のコードは今後この質問を検索で訪れた方様に残しておくべきか悩んでおります。
残すべきかどうかのアドバイスも頂ければ幸いです。

該当のソースコード

$(elem).each(function(){
  var aaa = "moji", bbb = "moji",
      aa = true,  bb = true,
      a = 1, b = 1, c = 1, d = 1, e = 1;
  if(aa || aaa.match("hoge")){
    if((a >= 0 && b >= 0) || (a <= 0 && b <= 0)){
      //処理１
    }else if(a <= 0 && b >= 0){
      //処理２
      if(bb && b <= c){
        if(bbb.match("fuga")){
          //処理３
          if(e%2 == 0){
            //処理４
          }
        }else{
          //処理１
          if(e%2 == 0){
            //処理２
          }
        }
      }
    }
  }else{
    //処理１
  }
});

/*------------------------------------設定------------------------------------*/
//クラス名
var class_name = ".dtimer";
//点滅色（CSS形式）例：red / #008000 / rgb(0,0,255)/
var color = "red";
//カラーチェンジもしくは点滅開始時間（終了何秒前からカラーチェンジもしくは点滅するか）
var flash_time = 600;
//更新間隔(秒)
var reload_time = 1;
/*----------------------------------------------------------------------------*/
reload_time *= 1000;
$(document).ready(function displayTimer(){
  $(class_name).each(function(){
    var sh = parseInt($(this).attr("sh")) || 0,
        sm = parseInt($(this).attr("sm")) || 0,
        eh = parseInt($(this).attr("eh")) || 24,
        em = parseInt($(this).attr("em")) || 0,
        flash = $(this).attr("flash"),
        wval  = $(this).attr("week"),
        elem  = $(this).prop("tagName"),
        stime = (sh * 60 + sm) * 60,
        etime = (eh * 60 + em) * 60,
        now   = new Date(),
        ntime = parseInt((now.getHours() * 60 + now.getMinutes()) * 60 + (now.getSeconds())),
        sjisa = stime - ntime,
        ejisa = etime - ntime,
        weekarry = ["日","月","火","水","木","金","土"],
        weekno = now.getDay(),
        week = weekarry[weekno];
    if(wval === undefined || wval.match(week)){
      if((sjisa >= 0 && ejisa >= 0) || (sjisa <= 0 && ejisa <= 0)){
        $(this).hide();
      }else if(sjisa <= 0 && ejisa >= 0){
        $(this).show();
        if(flash && ejisa <= flash_time){
          if(elem.match(/(p|a)/i)){
            $(this).css("color","");
            if(ejisa%2 == 0){
              $(this).css("color",color);
            }
          }else{
            $(this).hide();
            if(ejisa%2 == 0){
              $(this).show();
            }
          }
        }
      }
    }else{
      $(this).hide();
    }
  });
  setTimeout(function(){displayTimer()}, reload_time);//再起呼び出し
});

Comments

[kei344]

「処理２」は2回行われていますが、そういう「処理」なのでしょうか。また、「処理」の順序は固定でしょうか。

[m.ts10806]

そもそもどういう目的の処理なのかを書かれた方が良い意見をもらえそうに思います。

[退会済みユーザー]

kei344さん　処理１は処理２を取消し、処理2は処理１を取消す性質があります。

[退会済みユーザー]

mts10806さん　HTMLエレメントを消したり色を変えたりと何が出来るのかを動かしながら学んでいます。

[m.ts10806]

回答につながるための情報なので、質問本文に追記してください。ここはあくまで「質問への追記・修正依頼」の場です。

[think49]

jQueryにforeachはありませんが、$(elem).foreach は原文のままですか。

[think49]

(Q1) 処理1, 処理2が重複していますが、重複分は同じ処理ですか。(Q2)「処理１は処理２を取消し、処理2は処理１を取消す性質」とありますが、取り消すぐらいなら何もしないのが正解では？

Answer 1

ネストが深いと、テストもやりづらくバグの温床にもなりやすいです。

このような場合は早期リターンを使えばネストが下げられます。

例えば最初のif文の判定を反転させて、elseの処理を先に持って行きて処理１を実行後すぐに関数を抜けます。そうすると、elseが不要になるのでネストがひとつ減ります。

$(elem).foreach(function(){
  var aaa = "moji", bbb = "moji",
      aa = true,  bb = true,
      a = 1, b = 1, c = 1, d = 1, e = 1;

  if(!(aa || aaa.match("hoge"))){
    //処理１
    return;
  }

  if((a >= 0 && b >= 0) || (a <= 0 && b <= 0)){
    //処理１
  }else if(a <= 0 && b >= 0){
    //処理２
    if(bb && b <= c){
      if(bbb.match("fuga")){
        //処理３
        if(e%2 == 0){
          //処理４
        }
      }else{
        //処理１
        if(e%2 == 0){
          //処理２
        }
      }
    }
  }
});

以下同様に整理を続けると下記のようになります。

$(elem).foreach(function(){
  var aaa = "moji", bbb = "moji",
      aa = true,  bb = true,
      a = 1, b = 1, c = 1, d = 1, e = 1;

  if(!(aa || aaa.match("hoge"))){
    //処理１
    return;
  }

  if (a >= 0 && b >= 0) {
    //処理１
    return;
  }

　if (a <= 0 && b <= 0){
    //処理１
    return;
  }

  if(!(a <= 0 && b >= 0)){
    return;
  } 

  //処理２

  if (!(bb && b <= c)) return;
    
  if(!(bbb.match("fuga"))){
    //処理１
    if(!(e%2 == 0)) return;
    //処理２
    return;
  }
  
  //処理３
  if(!(e%2 == 0)) return;
  //処理４
});

判定の反転には、否定を用いていますが、本来は適切に修正する必要があります

他にも判定式を関数にする方法だとか、処理の中で場合分けする方法も用いればかなりスッキリかけると思いますが、関数に意味がある名前をつける必要があり、今回のソースコードではそれができませんでした。

Comments

[退会済みユーザー]

ご回答ありがとうございます。
このような書き方もあるのですね。
勉強になります！

[iwamoto_takaaki]

”早期リターン”というテクニックです。ググッてみてください。
経験的には、これくらいのネストがあるとどこかしらバグを作っています。

あと、おそらく条件式もっと分解してわかりやすく整理できると思います。

[k.tada]

私もこの案に1票です。
更に条件式の部分を分かりやすい名前のメソッドにしてしまうとベターです（これもリファクタリングの手法です。）

[退会済みユーザー]

早期リターンというのですね。
早速ググって記事を読み漁っています。
検索用語が増えると自力で答えにたどり着ける機会が増えるので助かります。

[iwamoto_takaaki]

> k.tadaさん
まさにですね。判定式をメソッドにして、enumを返して処理を分けるのが理想ですね。

Answer 2

You should use 'guard' solution. That is the way to reach the world of freaking geeks!

/*************************************************************
 *
 * main process
 *
 *************************************************************/
$(elem).each(function()
{
    const CASE = {'case1' : 1, 'case2' : 2, 'case_unmatch': 0};
    
    var aaa = "moji", bbb = "moji",
        aa  = true, bb = true,
        a   = 1, b = 1, c = 1, d = 1, e = 1;
    
    try
    {
        // When aa isn't true AND aaa isn't much "hoge", throw 'Illegal Case'
        if(!isRegular(aa, aaa)) { throw 'Illegal Case'; }

        // get a CASE CODE depending on conditions
        switch(getCase(CASE, a, b))
        {
            // When (a >= 0 && b >= 0) or (a <= 0 && b <= 0)
            case CASE.case1:

                //処理１
                break;

            // When (a <= 0 && b >= 0)
            case CASE.case2:

                // When bb isn't true OR  b > c, exit this scope at once.
                if (!canProcess2(bb, b, c)){ break; }

                // Information!
                // subProcess1(): create A and sum up total with c and d, then alert "Hello!"
                // subProcess2(): delete an element #input and submit all inputs to server.
                (bbb.match("fuga"))? subProcess1(e) : subProcess2(e);

                break;

            // other case as Unexpected conditions.
            default:
                throw 'Unknown Case';
        }
    }
    catch(e)
    {
        // UnExpected processing.
        
        // console.log("An error has occurred. " + e);
        // return false;    // to exit jQuery.each() loop.
    }
    
    return true;
});


/*************************************************************
 *
 * functions
 *
 *************************************************************/

/*
 * return true when the condition is satisfied.
 */
function isRegular(aa, aaa)
{
    if(aa)                  { return true; }
    if(aaa.match("hoge"))   { return true; }

    return false;
}

/*
 * return CASE CODE depending on conditions.
 */
function getCase(CASE, a, b)
{
    if(a >= 0 && b >= 0)    { return CASE.case1; }
    if(a <= 0 && b <= 0)    { return CASE.case1; }

    if(a <= 0 && b >= 0)    { return CASE.case2; }

    return CASE.case_unmatch;
}

/*
 * When bb is true AND b <= c, return true. otherwise return false
 */
function canProcess2(bb, b, c)
{
    if(bb && b <= c)    { return true; }
    
    return false;
}

/*
 * create A and sum up total with c and d, then alert "Hello!"
 */
function subProcess1(e)
{
    //処理３
    
    if (e % 2 != 0) { return; }

    //処理４
}

/*
 * delete an element #input and submit all inputs to server.
 */
function subProcess2(e)
{
    //処理１ ←　Duplicated?
    
    if (e % 2 != 0) { return; }
    
    //処理２
}

Comments

[iwamoto_takaaki]

お見事！
しかし、質問者には理解できないかも・・・

[退会済みユーザー]

Nausicaa said "I found out that no one spits some shit from their mouth in where code and environment are keeping clean. The pollution lies in the code itself.."

[退会済みユーザー]

Thank you for answering!
英語が得意でないのでGoogle翻訳でニュアンスでかみ砕いています。
最近trycatchの勉強を始めてコードで使用し始めたのでまだ理解出来ていませんが、1行ずつ解読して自分の力にしていきたいと思います。

[退会済みユーザー]

英語苦手なんで CASE.case の命名に時間かかっちゃうんですよねぇ。。。

[退会済みユーザー]

Don't worry. JavaScript can use an multiple bytes charactor.
You could wrote "CASE.りんごをかじったら歯茎から血が出た場合: ".
Take it easy.

[退会済みユーザー]

声を上げて笑ってしまいました！
変数名に日本語って使えるんですね。
Google翻訳で日本語を英語に変換して変数名にしてました。
日本人が作った日本語の記事のコードでも日本語の変数名を目にした事が無いのは英語で付けるのが暗黙のルールかなにかなのでしょうか？？

[退会済みユーザー]

If you use only alphanumeric charactors to write computer programming codes and use multiple bytes charactors to write any comments, You could recognize what is written in there at once.

If these are alphanumerics, It is a code.
If these are Japanese, It is a comment.

[退会済みユーザー]

英語だとコードとしてすぐ認識出来るけど、日本語だとバイト数が多い分処理に時間がかかるというニュアンスで受け取ったのですがどうでしょうか？？

[退会済みユーザー]

And, fucking SIer usually uses continual numbers solution! like below.

CASE.X01B003:
CASE.B213G335:
CASE.QJ22S35C:

fuuuuuuuuuuuuuuuucking SIer shall all die quickly, yeah.

[退会済みユーザー]

>HaRu_8609

no, no. processing times has no difference between those.
I had talked about WYSIWYG.
If you see Alphanumerics, It is a code.
If you see Japanese, It is a comment.
You could understand it at once.

[退会済みユーザー]

「人間が記事を読む時、日本語があるとコードと直ぐに理解出来ないけど、英語だけならコードと直ぐに認識しやすいからみんな英語で書いている」という感じのニュアンスで受け取ったのですがどうでしょう？？

[退会済みユーザー]

You’re almost right.

[退会済みユーザー]

And, SIer shall all die.

[退会済みユーザー]

SIerと呼ばれる人たちの作るコードは私には魔法の呪文のように見えるのだろうなとおもいます。
同じレベルになりたいと思いますが、まずはtrycatchの使い方を完璧に覚える事から：＿（
先は長そうですが、私の寿命よりはやく到達出来る事を目標にします！

[退会済みユーザー]

>SIerと呼ばれる人たちの作るコードは私には魔法の呪文のように見えるのだろうなとおもいます。同じレベルになりたいと思いますが

You shall quit it!

[退会済みユーザー]

They "are" all insane.

[退会済みユーザー]

F1ドライバーの様に頭のネジが何本か取れてないと到達出来ないような領域なのですね。
狂うギリギリの領域のプロフェッショナルを目標地点に修正して頑張ります: )
そろそろみなさんから頂いたコードの理解に集中します。
今日は本当にありがとうございました。
私が質問する方になると思いますが、また機会があればよろしくお願いします。

Answer 3

コードを読み, 大体こんなことがしたいのかなと忖度して試しに組んでみました.
あまり動作検証はしていないのと, jQueryを使っていないので参考程度に.

• ループ処理は別途名前付きの関数として呼ぶようにし, 処理の内容を明確化する.
• 複雑な条件判定処理を要する場合は, 適宜フラグ値やステータス値を求めた後で具体的な処理を行うようにする.
• JavaScriptだけで全てを賄おうとするのではなく, DOMやCSSがもっている仕組みをフル活用する.

<div class="dtimer" data-sh="14" data-sm="0" data-eh="16" data-em="0">あいうえお</div>
<div class="dtimer" data-sh="12" data-sm="0" data-eh="14" data-em="35">あいうえお</div>
<div class="dtimer" data-sh="14" data-sm="0" data-eh="16" data-em="0" data-days="木">あいうえお</div>
<div class="dtimer" data-sh="12" data-sm="0" data-eh="14" data-em="35" data-days="木">あいうえお</div>

.flash{
	color: red;
	animation: flash 1s steps(1) infinite;
}
@keyframes flash{
	 0%{visibility: visible;}
	50%{visibility: hidden;}
}

"use strict";
document.addEventListener("DOMContentLoaded", e => {
	//点灯・点滅対象のノード
	const clsNm = "dtimer";
	const elems = document.getElementsByClassName(clsNm);
	
	//点滅を開始する秒数
	const flash = 600;
	//時分秒を求める
	const getSeconds = (h, m, s = 0) => (h * 60 + m) * 60 + s;
	//ノード単体に対する点灯・点滅設定を行う
	function display(elem, ntime, nday){
		//ノードに設定されている時刻を取得する
		const dl = elem.dataset;
		const stime = getSeconds(dl.sh|0, dl.sm|0);//点灯開始時刻
		const etime = getSeconds(dl.eh|0, dl.em|0);//点灯終了時刻
		const days = elem.dataset["days"];//点灯対象曜日
		
		//点灯・点滅のフラグ値
		let needToShow = false;
		let needToFlash = false;
		if((!days || days.indexOf(nday) >= 0) && (stime <= ntime && ntime <= etime)){
			needToShow = true;
			needToFlash = etime - ntime < flash; 
		}
		
		//フラグ値によって設定するスタイルを変更する
		elem.style.visibility = needToShow ? "visible": "hidden";
		elem.classList[needToFlash ? "add": "remove"]("flash");
	}

	//ノードリストに対する点灯・点滅設定を行う
	(function displayAll(){
		const today = new Date();
		const ntime = getSeconds(today.getHours(), today.getMinutes(), today.getSeconds());
		const nday = "日月火水木金土".substr(today.getDay(), 1);
		[].forEach.call(elems, elem => display(elem, ntime, nday));
		setTimeout(displayAll, 1000);
	})();
});

Comments

[退会済みユーザー]

色々な属性で動かしてみました。
同じ動きをするコードでもこんなにも違う書き方があるのに驚いています。
まだ"use strict"とconst、letが何なのかを調べて今からコードの中身を見ていくところですが、まずはお礼まで。

[退会済みユーザー]

１．頂いたコードを元にdata属性を勉強中なのですが、私が作っていたHTMLは下記の様な「data-」無しの状態で記述しているのに元コードで動いていました。

<div class="dtimer" sh="14" sm="0" eh="16" em="0">あいうえお</div>

予想ではjQueryの.attrでカスタム属性を作ったことになっていた？ものだと思うのですが、data属性（「data-」を付けた状態）でカスタム属性は扱った方が良いのでしょうか？

２．「"use strict";」は先頭に付けると他のファイルも厳格モードで動くという記事を見かけたのですが、ホームページにアップする際は「addEventListener」内先頭に移動する事でそのファイル内のみに厳格モードを適用出来るという認識で合っていますでしょうか？

[defghi1977]

1. そのような機能があるという例示であって, 強制するものではありません.
2. 関数単位でのモードが適切であればそのように.(私は基本strictモードでコードを記述するようにしているだけです)
https://developer.mozilla.org/ja/docs/Web/JavaScript/Strict_mode

[退会済みユーザー]

ご回答ありがとうございます。
1.「カスタム属性」や「独自　属性」で検索をかけた際に「data-」を付けるという記事ばかりでしたので、私のコードはたまたたま動いていたコードでは無いかと心配です。私が使っていた属性はなんという名称なのでしょうか？もし名称をご存じであればご教授頂ければ幸いです。

２．頂いたURL先を熟読してみます。誘導感謝致します。

[defghi1977]

一般のDOM(XML,HTML等に共通の)APIを用いた(名前空間なしの)属性値アクセスですから, 何ら恥ずべきところはありません.
https://developer.mozilla.org/ja/docs/Web/API/Element/getAttribute
ただし, 独自に属性を定義した場合, HTMLで定義されている名称と被る危険性があるため, dataから始まるカスタム属性を使ったほうが良いとされています.

[退会済みユーザー]

回答ありがとうございます。
かれこれ5時間ほど自分が書いていたコードが何故動いていたのか調べてふわふわとしか理解出来ていなかったので安心致しました。
今回誘導して頂いたリンク先、先程のリンク先共に熟読して理解を深めたいと思います。
とても親切丁寧にご教授頂いて本当にありがとうございます。

Answer 4

私はjavascriptではなくperlでややこしい条件判定が必要なプログラムを書くことがありますが、そういう時は、処理をそれぞれ関数化して、対象データとの対応表をあらかじめ作っておき、それを参照させることによって条件判定をひとつにまとめてしまうということをよくやります。

万能ではないですがコードがすごくすっきりすることは確かです。参考までに…。

aの値	bの値	cの値	呼び出す関数
0	0	0	funcA()
0	0	1	funcC()
0	1	0	funcB()
0	1	1	funcA()
...

Comments

[退会済みユーザー]

ご回答ありがとうございます。
複雑なプログラムを作る時には対応表を書いてそれを見ながら追ってもらう事で綺麗にする事もあるのですね。
スッキリしたコードになるよう心掛けてはいるのですが、勉強を進めていくとどんどん複雑になって「現場ではこの何十倍も長いコードとかあるだろうし読むの大変そう」と思っていましたが対応表があると読み砕くのにかなり役に立ちそうです。

Answer 5

「リファクタリング　手法」でキーワード検索されることをお勧め致します。

本題から少し離れますが、手順のアドバイスを。

1. バージョン管理ツール(git)の導入

コードを変更するため、gitを使って管理することをオススメします。

1. テストロジックの作成

動いているコードを変更する時は、コード変更前と変更後で動作が変わらないように保証するためにテストロジックやassertがなければ、作成してくださいな。

1. 上記2点がクリアされた後にコードを変更する方が、変更規模によりますが、手戻りが少なくなります。

◇参考情報
リファクタリング » 不吉な匂い

Comments

[退会済みユーザー]

コードを整理する事をリファクタリングというのですね。
勉強になります。
gitむずかしそうですが、慣れれば便利そうなので導入を試みたいと思います。
２に関しては理解するのに時間がかかりそうでしたのでまずはお礼まで。

[umyu]

>HaRu_8609さんへ。
gitはWindowsならGithubとGitHub Desktopがおすすめです。

Answer 6

ご提示のソースくらいのネストはざらにあるなぁ、くらいの感想です。
それでもきれいにしたい場合は、ifの処理、elseの処理、でそれぞれ関数化して見通しを良くする、というのもよくやります。

Comments

[退会済みユーザー]

ご回答ありがとうございます。
どのくらいネストしたら見直した方が良いでしょうか？おおよその目安でかまいませんのでご教授頂ければ助かります。

[KojiDoi]

＞どのくらいネストしたら　「なんか見にくいなあ」と思ったら見直せばいいんじゃないでしょうか。「どのくらい」は結局のところ主観だと思います。現にkaztoさんとiwamoto_takaakiさんでは感覚の違いが見えてますよね。

[退会済みユーザー]

ご回答ありがとうございます。
あとは現場に入ってから周りをみて合わせていく感じで良さそうですね。
気が楽になりました。

Answer 7

document.addEventListener("DOMContentLoaded", function(){
  //ノード単体に対する表示・非表示の設定を行う
  function display(elem, ntime, nweek){
    //ノードに設定されている曜日・時刻を取得する
    const data  = elem.dataset;
    const week  = data.week;
    const stime = ((data.sh|0) * 60 + (data.sm|0)) * 60;
    const etime = ((data.eh||24) * 60 + (data.em|0)) *60;
    //表示時間判定
    nweek = new RegExp(nweek);
    if((!week || (nweek).test(week)) && (stime <= ntime && ntime <= etime)){
      //表示
      elem.style.display = "block";
    }else{
      //非表示
      elem.style.display = "none";
    }
  }
  const elems = document.getElementsByClassName("dtimer");
  //指定したノードリストを設定用関数(display)に送る
  (function displayAll(){
    const today = new Date();
    const ntime = (today.getHours() * 60 + today.getMinutes()) * 60 + today.getSeconds();
    const nweek = "日月火水木金土".substr(today.getDay(), 1);
    NodeList.prototype.forEach.call(elems, function(elem){
      return display(elem, ntime, nweek);
    });
    setTimeout(displayAll, 1000);
  }());
});

皆様のおかげで無事配布用スニペット、記事用コード、紹介記事を完成させる事が出来ました。
添付したコードは記事で使用したコードになります。
また、ホームページ内の既存のjsコードも見直し現在ネストが4層以上ある箇所（取り合えずの自分ルールですが）を早期リターンや関数化して手直しをしながら再実装に向けてテスト中です。
luckerさんからご提案頂いたコードは、現在立案中のFXのチャート分析ページでかなり複雑に条件分岐すると予想出来る事から、KojiDoiさんの案と兼用して作成に取り組もうと考えています。

いるか分かりませんが、念の為どんな記事になったのかご興味を抱いて頂いた方用に記事へのURLを添付します。
作成した記事

検索しても自力で解決出来なかった際はまた質問をする事もあるかと思いますが、その際はお時間あればまたよろしくお願い致します。
本当にありがとうございました！

Answer 8

誤りかと思いますが

if((a >= 0 && b >= 0) || (a <= 0 && b <= 0)){
  //処理１
}else if(a <= 0 && b >= 0){
  //処理２
  

正しくは

if((a >= 0 && b >= 0) || (a < 0 && b < 0)){
  //処理１
}else if(a < 0 && b > 0){
  //処理２

ではないでしょうか。

Comments

[退会済みユーザー]

その通りです。同じ比較をしていたのに気づきませんでした。
ご指摘ありがとうございます。