見出し画像

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

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

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

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

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

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

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

内容としては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のスコープが必要なくなりました。
もう一度全体を見てみます。

この続きをみるには

この続き: 4,837文字

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

sakiyamaK

100円

この記事が気に入ったら、サポートをしてみませんか?
気軽にクリエイターの支援と、記事のオススメができます!
15
画像処理の工学博士/画像処理ソフトやアルゴリズム開発/iOSエンジニア/Flutterの可能性に感激しまくる日々/とくに何もないけどgithub http://github.com/sakiyamaK
コメントを投稿するには、 ログイン または 会員登録 をする必要があります。