【Swift・iOS】コードレビューからコーディングする際の注意点をまとめてみた。

こんにちは、@shogunzozoです。これまでは技術によった記事を書いてきました。

たまには違う系統の記事を書きたいなと思い何を書くか考えていました。

ふと、自分の成長のためにレビュー頂いた事を書き溜めたメモ㊙️を思い出しました。

恥ずかしい指摘も受けましたが、皆さんにも良いコードを書いて頂きたいと思ってシェアしようと思いました!!

今後も振り返りできるメモ的な感じで随時更新していこうと考えています。もし、興味ございましたら覗きにきて下さい。

お役に立てれば幸いです、是非最後まで読んで頂ければと思います!!

※前後の関係性などは薄くなったり似ている事項があるかもしれません。そちらはご了承下さい。

Scopeの絞り方を考える

必要な事は必要な所で宣言して、公開範囲を考える事です。

以下は極端な例ですが、ExampleTableViewCellのみで使用するExampleStateは以下の状態ですとグローバルに公開されています。
これは設計的な観点からも好ましくないです。

class ExampleTableViewCell: UITableViewCell {
~~~~ 省略　~~~~
}

struct ExampleState {
~~~~ 省略 ~~~~
}

この様にextensionを使って可読性を上げるためにも、ここで使うんだよという事を可視化してあげましょう。

class ExampleTableViewCell: UITableViewCell {
~~~~ 省略　~~~~
}

extension ExampleTableViewCell {
 struct ExampleState {
 ~~~~ 省略 ~~~~
 }
}

privateなどプロパティやメソッドの定義も外部からの安易な参照を防ぎ、不要なバグを防ぐ観点からも重要かと思います。

不必要な定数化をしていないか？

他で使われていないにも関わらず、メンバ変数などわざわざ宣言してしまうケースがあった。
意味も無いしコード量も増えてよろしくない。

@objcMembersの使い方

@objcMembersはclassの接頭につけるとclassの要素を全て開放する。

@objcMembers class ExampleClass: UIView {
var name: String
var family: String
var home: String
}

上記でSwiftファイルで使う要素がnameだけだった場合以下の様にしましょう。こちらも解放性を考慮してです。

@objc class ExampleClass: UIView {
 @objc var name: String
 var family: String
 var home: String
}

Xibファイルで完結できる事はコーディングしない

表題のままですが、backgroundColorやフォントのサイズなどが該当しますね。
バグのないコードはゼロコードという名言がある様に、コード量は極力減らす事を心がけましょう。

UIKit標準のオプショナルに関して

guard文やif let 文でオプショナル対策を行う時があると思います。
しかし時として行う必要がない事もあります。

それは例えばUILabelのtextとUIImageのimageです。これらはオプショナルなのでguardやif letで担保する必要はないです。

単純なif elseは三項演算子を使って見る(ワンラインで書くともいう)

以下極端な例ですがこんなコードがあったとします。

if (highlighted) {
backgroundColor = .white
} else {
backgroundColor = .red
}

以下の様にリファクタする
backgroundColor = highlighted ? .white : .red

おまけとして、nil or notでboolを判定したい場合は以下の様にもかけます。

isImageViewHidden = imageURL == nil

let isFollow = exampleState ?? falseという方法もある

またifのネストを避ける際は各内容ネスト前にreturnで処理を止める方法もある。

ファイル内プロパティの参照範囲の記述

そのファイルでしか参照しないものは必ずprivate,filePrivateなどをつける。また、外部から参照はして変更を不可にしたい場合はprivate(set)を使う事も。
基本的に依存性を考慮して外部ファイルから直接見ることは避ける。どうしても見たい場合は、プロパティをprivateにしてconfigureなどのメソッドを介して見ることを心がける。

classを作る時は、そのクラスが何を担っているのかなど意味を考えて作る。
ロジックもファイル内で定義すべきなのか否かを含めて、責務には注意。

Cellなどにデータを注入する際の注意事項

必要なものを必要なだけ持って来ることを心がける。

詳しくはこちらを参照

Extensionの共通化

もし、型変換など同ファイル内で行うためにextensionを使い定義するとする。
これは定義場所を考慮する。
なぜなら、その型変換を再度利用する事があった時に都度見にくる必要がある。これは依存性の観点から好ましくない。
なぜかというと、参照もとファイルに変更が加わると都度確認する必要が出てくる。つまり、変更に弱くなる。
型変換専用のファイルを定義する。

コーディングガイドラインについて

やりがちなコーディングミス。以下例

subjectImageUrl -> subjectImageURL
subjectId -> subjectID

参考

型が何を表しているのかを考える

structで型を定義をすることはしばしば。

その際に、クライアント側で使用するViewにまつわる型なのか？
サーバーからデータを受け取る際に必要な型なのか？
を考える必要がある。

なぜなら、責務の関係でViewを表現しているファイルにサーバーの型で状態を持たせると依存関係ができてしまうため。

責務というワードが出たが、ここは何をしているところなのか？は常に自問自答したいところ。
役割をはっきりと分けよう。

ObjCクロージャの循環参照に注意を

Swiftでは自動でweakを保管するが、ObjCは自動保管がないため忘れがち(Swiftを書き慣れている人は特に)
ObjCでクロージャを扱うときは意識を。weakSelf

プロパティ名のコツ(ケースバイケース)

よくある例

isViewHidden -> BoolでHiddenかどうかを判定
しかしこれはViewに依存している名前と言える

状態として保持して複数行える場合は別の命名もある
hasView -> これはviewを持っているか持っていないかと言う観点で実装を行うことができる

不要なバグは命名の意味が実装者に誤って伝わることも原因となる。

親クラスを確認すると言うこと

画面遷移で自分の想定していない挙動を確認してハマった事があった。自分の実装範囲を確認しても原因箇所が見つからない。
継承元の親クラスを確認。原因を特定。
対処法としては、子クラスで該当箇所をオーバライド。

動的にCellの高さを変える際のテクニック

こちらの記事参照

ラベルの高さ指定

ラベルに高さは基本的に指定しない。バグを踏む事になる。

クロージャのオプショナル

クロージャはオプショナルにしてデフォ値をnilにしてあげると、呼び出し元で表記しなくて済む。

空文字判定の扱い

空文字に関してはよく以下の様な記述を見かける

if name == "" {
// 行いたい処理
}

しかし,iOSエンジニアたるもの以下の様にスマートに書きたい

if name.isEmpty {
// 行いたい処理
}

Switch構文が威力を発揮する時

Switch case 構文をご存知の方はエンジニアをしていれば多いと思われます。
しかし、考えた事がありますか？？このSwitch caseが威力を発揮するのかをです。

それは状態によって処理を出しわけしたい時・決まった状態がありそれによりviewやbuttonなどの状態が変わる時です。

switch (stateA, stateB, stateC) {
case(true, false, false):
hasViewA = true
hasViewB = false
hasViewC = false

case(false, true, false):
hasViewA = false
hasViewB = true
hasViewC = false

case(false, false, true):
hasViewA = false
hasViewB = false
hasViewC = true

default:
hasViewA = false
hasViewB = false
hasViewC = false
}

同じコードは2度書かない

超基本すぎて書くのもお恥ずかしいくらいですが、自分への戒めとして書いておこうと思います。

一般的に否定系は読みづらいので肯定系で書く

以下の2つは意味している事は同じですがどちらがぱっと見でわかりますか？？
下ですよね？？あえて否定系で書く意味はないです。

label.text = !labelName.isEmpty ? brandName : "-"

label.text = labelName.isEmpty ? "-" : brandName

ワンラインの注意

以下の間違いはBoolを判定する上では不要

button.isHidden = status == .open ? false : true

以下の様にリファクタできる
button.isHidden = status != .open

同タイミングで2つの状態を持つのは良くない

実装を行う中で、状態を持たせたい事があった。それを生やしたプロパティに持たせる実装を行なった。
しかし、これは同タイミングで2つの状態を持っているストアードプロパティを持っている事になる。
これは予期せぬバグを踏む可能性がある。
そのため、状態を持たせてあげたい時はコンピューテッドプロパティで計算・算出した値を返す様にするべき。

失敗イニシャライザ

初期化が失敗するときにnilを返す。
具体例になるが、サーバー側からクライアント側が想定しないデータが返ってきた際、viewを非表示にするために使った。

xibでHidden要素の初期状態管理

hiddenになる可能性のある要素はxibの初期状態はhiddenにしておく

superClassを使う時の注意点

必ず、何を継承しているかを考え・確認する。それによっては小クラスに負の影響を与える事もある。その場合は、オーバーライドで対応したりする。

共通化した処理はメソッドとして切り出す

同じ処理や冗長の処理はメソッドとして切り出して呼ぶ様に心がける。

メソッド等の命名の考え方

それが何を意味しているのかを正確に書いてあげる。抽象的すぎるのか、具体性が必要なのか。それを意識して命名を行う。

メソッド名にVCはいらない

メソッド名にVCはいらない。これは各チームの運用ルールもあるかもしれないが注意点。

ObjCのプロパティnullableにするか否か問題

.hファイルに一つでもnullable指定をしていると他も求められる。

強制アンランプの可否

クライアント側で制御できるものとできないもの。サーバー側から返って来たのもはクライアントでは制御できない。そのため、強制アンラップは危険。だからしてはいけない。その代わり、クライアント側で制御できるものはOK。

NS_CLOSED_ENUMとNS_ENUMの話

レビューワーの気持ちになって考える

何を情報として事前に提供していれば良いかを考える。