プログラミング初心者のコードをレビューした時の内容を教えます

前回のノートに経歴や経緯などを書いていますが、4月からプログラミングを個人で教えており、コードレビューをたくさんやりました。その経験から、同じようなことを何人にも指摘しましたので、その内容をまとめてみました。

初学者の方に多いのは「functionは何をするか分かる」「if文は何をするか分かる」と、それぞれについては理解しているのですが

じゃあ、それをどう使うの？

というのが分からないかと思います。

もちろん「if文とはこう使うものだ」と１パターンしかないのであればいいのですが、プログラミング言語はあくまで言葉であるため、状況によって使い方は違います。

今回はその１例を示します。

内容としてはSwift言語ですが、functionとif文というものは今時あらゆる言語に備わっており、どの言語の初学者でも参考になるかと思います。

作りたいメソッドの仕様

ユーザが入力した文字列をもらって名前のラベルに表示するメソッドを例にします。
文字数は3文字以上10文字以下で数値は入れられないという仕様です。

何人かの初学者の方達は最初このように書きました。

//ユーザ ネームラベルを更新するメソッド
//ユーザ名に数値は入れられない
func updateUserName(_ name: String?) {
 if let _name = name {
   for i in _name {
     let c = "\(i)"
     if Int(c) != nil {
       //本来は画面にアラートを出す
       print("\(_name) 数値は入れられません")
       return
     }
   }

   if _name.count >= 3 {
     if _name.count <= 10 {
       //本来はlabelを更新する
       print("\(_name)")
       return
     }
     //本来は画面にアラートを出す
     print("\(_name) 10以下の文字列を入力してください")
   } else {
     //本来は画面にアラートを出す
     print("\(_name) 3以上の文字列を入力してください")
   }
 } else {
   //本来は画面にアラートを出す
   print("名前を入力してください")
 }
}

これの動作確認をすると確かに正しく動作します。

for str in ["abcdefghi", "abcd123efg", "abcdefghijklmn", "ab", nil] {
 updateUserName(str)
}

/* 出力結果
>> abcdefghi
>> abcd123efg 数値は入れられません
>> abcdefghijklmn 10以下の文字列を入力してください
>> ab 3以上の文字列を入力してください
>> 名前を入力してください
*/

ただし、このコードを見せられたらまず面接はしません。
私自身も、いくつかの企業で採用担当をしましたが、少なくとも私がこれまで働いてきたITベンチャー企業だと無理です。

では、これだと何が悪いのかひとつひとつ指摘していきながらコードを書き換えていきたいと思います。

メソッドが本来やりたいことがどこか分かりにくい

さきほどのメソッドは本来「ユーザが入力した文字列を画面に表示する」メソッドです。

ただし、ユーザがこちらの意図通りに入力してくれるとは限らないのであちこちに「不正な入力がきた場合はアラートを表示する」処理が書かれています。

さきほどのメソッドに

//○○○○○ 正しい入力がきた場合 ○○○○○//
//XXXXX 不正な入力がきた場合 XXXXX//

というコメントを付け足してみます。

func updateUserName(_ name: String?) {
 if let _name = name {
   for i in _name {
     let c = "\(i)"
     if Int(c) != nil {
       //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
       print("\(_name) 数値は入れられません")
       return
     }
   }

   if _name.count >= 3 {
     if _name.count <= 10 {
      //○○○○○ 正しい入力がきた場合 ○○○○○○//
       print("\(_name)")
       return
     }
     //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
     print("\(_name) 10以下の文字列を入力してください")
   } else {
     //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
     print("\(_name) 3以上の文字列を入力してください")
   }
 } else {
   //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
   print("名前を入力してください")
 }
}

//○○○○○ 正しい入力がきた場合 ○○○○○//に、このメソッドが本来やりたいことである「ユーザが入力した文字列を画面に表示する」処理が書かれています。

どうでしょうか？これだと、このメソッドが本来やりたい処理が三重のif文の中にあってパッと見て何をするメソッドか分かりづらくないでしょうか？

メソッドの構成

メソッドの基本的な構成というのはだいたいこうあるべきです 。(もちろん例外はあります)

func hogehoge(input: String?) {
 ifなりguardなりで不正な入力か判定 {
   return
 }
 ifなりguardなりで不正な入力か判定 {
   return
 }
 ifなりguardなりで不正な入力か判定 {
   return
 }

 本来このメソッドがやるべき処理
}

最初に不正な入力を判定して、不正な入力用の処理を書きメソッドを抜けます。
全ての不正な入力の判定をして問題がなければ最後にこのメソッドがやるべき処理を書きます。

この考え方で、さきほどのメソッドを書き換えてみます。

ネストを減らす

そのためにまず最初に一番深いところのif文のネストを減らしていきます。

func updateUserName(_ name: String?) {
 if let _name = name {
   for i in _name {
     let c = "\(i)"
     if Int(c) != nil {
       //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
       print("\(_name) 数値は入れられません")
       return
     }
   }

   /********************************/
   /***********  ここから ************/
   /********************************/
   if _name.count >= 3 {
     if _name.count <= 10 {
      //○○○○○ 正しい入力がきた場合 ○○○○○○//
       print("\(_name)")
       return
     }
     //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
     print("\(_name) 10以下の文字列を入力してください")
   } else {
     //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
     print("\(_name) 3以上の文字列を入力してください")
   }
   /********************************/
   /***********  ここまで ************/
   /********************************/
 } else {
   //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
   print("名前を入力してください")
 }
}

それ以外は今は気にしなくて良いので抜き出します。

if _name.count >= 3 {
    if _name.count <= 10 {
      //○○○○○ 正しい入力がきた場合 ○○○○○○//
       print("\(_name)")
       return
     }
     //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
     print("\(_name) 10以下の文字列を入力してください")
} else {
    //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
    print("\(_name) 3以上の文字列を入力してください")
}

まず、一番奥にある  ​
if n.count <= 10 { ~~~~ }
の判定を逆にして正しい処理がきた場合と入れ替えます。

if _name.count >= 3 {
    if _name.count > 10 {
        //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
        print("\(_name) 10以下の文字列を入力してください")
        return
     }
     //○○○○○ 正しい入力がきた場合 ○○○○○○//
     print("\(_name)")
} else {
    //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
    print("\(_name) 3以上の文字列を入力してください")
}

次に外側のif文も同じように入れ替えます

if _name.count < 3 {
    //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
    print("\(_name) 3以上の文字列を入力してください")
} else {
    if _name.count > 10 {
        //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
        print("\(_name) 10以下の文字列を入力してください")
        return
     }
     //○○○○○ 正しい入力がきた場合 ○○○○○○//
     print("\(_name)")
}

if { ~ } else { ~ } の構文ですが、不正な入力がきた場合はそれ以上処理する必要がないためreturnに変えます。

if _name.count < 3 {
    //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
    print("\(_name) 3以上の文字列を入力してください")
    return
}

if _name.count > 10 {
    //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
    print("\(_name) 10以下の文字列を入力してください")
    return
}

//○○○○○ 正しい入力がきた場合 ○○○○○○//
print("\(_name)")

こうすることでelseのスコープが必要なくなりました。
もう一度全体を見てみます。

func updateUserName(_ name: String?) {
 if let _name = name {
   for i in _name {
     let c = "\(i)"
     if Int(c) != nil {
       //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
       print("\(_name) 数値は入れられません")
       return
     }
   }

   /********************************/
   /***********  ここから ************/
   /********************************/
   if _name.count < 3 {
     //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
     print("\(_name) 3以上の文字列を入力してください")
     return
   }

   if _name.count > 10 {
     //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
     print("\(_name) 10以下の文字列を入力してください")
     return
   }

   //○○○○○ 正しい入力がきた場合 ○○○○○○//
   print("\(_name)")
   /********************************/
   /***********  ここまで ************/
   /********************************/
 } else {
   //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
   print("名前を入力してください")
 }
}

三重のif文がなくなり少しすっきりしました。
同様に外側のif文も書き換えますがSwiftにはこう言う時に便利に使えるguard文というものがあるのでそれを利用します。

guard文を利用する

guardは簡単に言ってしまうとif文の逆です。
簡単な例を示します。

まずif文です

if let _a = a {
  //aがnilじゃなく_aはoptionalじゃないものになる(1)
  return
} 
// aがnilなので_aは使えない(2)

これはaという入力があった時に、それがnilじゃない場合に本来やりたい処理になることが多いのですが、その処理をする(1)はネストが深い位置にいます。

これをguard文で書くとこうなります。

guard let _a = a else {
  // aがnilなので_aは使えない(2)
  return
}
//aがnilじゃなく_aはoptionalじゃないものになる(1)

if文と違い(1)がネストが浅くなっていることが分かります。

これと同じようにupdateUserNameも書き換えます。

func updateUserName(_ name: String?) {
 guard let _name = name else {
   //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
   print("名前を入力してください")
   return
 }

 for i in _name {
   let c = "\(i)"
   if Int(c) != nil {
     //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
     print("\(_name) 数値は入れられません")
     return
   }
 }

 if _name.count < 3 {
   //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
   print("\(_name) 3以上の文字列を入力してください")
   return
 }

 if _name.count > 10 {
   //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
   print("\(_name) 10以下の文字列を入力してください")
   return
 }

 //○○○○○ 正しい入力がきた場合 ○○○○○○//
 print("\(_name)")
}

二重のif文も消えて「不正な入力がきた場合」を先にチェックして処理を終えられるようになりました。

ですが、これでまだ終わりません。

文字列の処理は専用のメソッドがあるか確認する

文字列とは"あいうえお", "今日は良い天気"などのいわゆるString型です。

文字列は今時の言語だとだいたい存在する型です。
また、文字列を操作したりチェックしたりすることはどのプログラミング言語でも必要な処理で、かつやることも似たり寄ったりです。

そのため、文字列を操作する専用のメソッドがたくさんあるので、何か文字列を操作する場合はまず専用のメソッドがないか調べることをお勧めします。

今回の場合はここの部分にあたります。

func updateUserName(_ name: String?) {
 guard let _name = name else {
   //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
   print("名前を入力してください")
   return
 }

 /********************************/
 /***********  ここから *************/
 /********************************/
 for i in _name {
   let c = "\(i)"
   if Int(c) != nil {
     //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
     print("\(_name) 数値は入れられません")
     return
   }
 }
 /********************************/
 /**********  ここまで  *************/
 /********************************/

 if _name.count < 3 {
   //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
   print("\(_name) 3以上の文字列を入力してください")
   return
 }

 if _name.count > 10 {
   //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
   print("\(_name) 10以下の文字列を入力してください")
   return
 }

 //○○○○○ 正しい入力がきた場合 ○○○○○○//
 print("\(_name)")
}

何をしているかというと、文字列の中に数が含まれていないかチェックするために
１文字ずつfor文で取得し、Intでキャストして、それがnilじゃない、つまりIntになったならその文字は数値であると判断して、最後にアラートを出すという処理をしています。

これを修正していきます。

と、言いつつ私も大量の文字列操作関数の全てを覚えているわけではないので、まず「swift 文字列 含む」と検索をしてみました。

そうするとcontainsというメソッドがあることが分かりました。
これは文字列の中に指定した文字が含まれていたらtrueになるメソッドです。
それを使って書き換えるとこうなります。

/********************************/
/***********  ここから *************/
/********************************/
 for numStr in "0123456789" {
   if _name.contains(numStr) {
     //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
     print("\(_name) 数値は入れられません")
     return
   }
}
/********************************/
/**********  ここまで  *************/
/********************************/

何をしているかというと0~9という文字をひとつずつforで回して_nameに含まれているかどうか確認しています。
さきほどは_nameの中に数値があるか確認していましたが、発想が逆になっていますね。
これだとInt型にキャストしたりさせる必要がないのですっきりしました。

全体を書き直すとこうなります。

func updateUserName(_ name: String?) {
 guard let _name = name else {
   //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
   print("名前を入力してください")
   return
 }

 for numStr in "0123456789" {
   if _name.contains(numStr) {
     //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
     print("\(_name) 数値は入れられません")
     return
   }
 }

 if _name.count < 3 {
   //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
   print("\(_name) 3以上の文字列を入力してください")
   return
 }

 if _name.count > 10 {
   //XXXXXXXXXX 不正な入力がきた場合　XXXXXXXX//
   print("\(_name) 10以下の文字列を入力してください")
   return
 }

 //○○○○○ 正しい入力がきた場合 ○○○○○○//
 print("\(_name)")
}

最初のソースコードと見比べてずいぶんすっきりしたのが分かるかと思います。
実務ではそもそもエラーチェック用のenumを用意したりしてもっとすっきり書いたりもしますが、ひとまずこれぐらいになっていれば「コードが綺麗にまとまってるな」と判断してもらえるかと思います。

まとめ

・メソッドが本来やりたいことを深いネストに書かないようにしよう

・入力チェックは最初に全部終わらせよう

・文字列を操作する時は自作せずメソッドがあるか調べよう

おまけ

文字列に数値が含まれるか判定する部分ですが、もう少し調べると以下のような書き方もあります。
このあたりが使いこなせるとより良いですね。

let hasNumber = _name.filter { "0123456789".contains($0) } != _name

let hasNumber = _name.components(separatedBy: .decimalDigits).joined() != _name