これはSwift歴4ヶ月の自分が、ベテランiOSエンジニアの方にがっつりコードレビューしていただいた際の備忘録です。今後、良くないコード書かないために、また自分がレビューする際に適切に指摘できるようになるために、内容をまとめることにしました。
コーディングルールは、組織によって違いがあると思います。これから紹介するレビューでのルールが絶対というわけではありません。この内容を参考にしていただき、より良いコーディングルール、レビューの行ってくれる方々が少しでもいてくれれば幸いです。
アクセス修飾子を適切に設定する
Swiftではアクセス修飾子(public、internal、private、fileprivate、open)を省略した場合、internalを同じ意味になります。internalは「同一モジュール内であればアクセス可能」です。internalだとアクセス範囲が広すぎる場合があるので、必要に応じて適切に設定します。
以下、例です。
UIViewControllerのUIのOutletにprivateをつける
class StartViewController: UIViewController {
/// タイトルのラベル
@IBOutlet private weak var titleLabel: UILabel!
/// スタートボタン
@IBOutlet private weak var startButton: UIButton!
}
titleLabel、startButtonそれぞれに「private」が設定されています。privateがない場合、ViewControllerの外からtitleLabel、startButtonの値が書き換えることが可能になっています。そういった要件が必要でない場合は「private」などの修飾子を設定し、値を書き換えられないようにします。
継承してほしくないclassにはfinalをつける
final修飾子をclassに設定すると、設定されたclassは継承が不可になります。継承を前提としないclassにfinalを付けないと、意図しないところで継承が行われることで、結果コードの可読性、保守の難易度が不必要に上がります。そういった事態を防ぐため、継承しないclassにはfinal修飾子を設定します。
// NextViewControllerは継承不可
final class NextViewController: UIViewController {
}
不要なコメントアウトされたコードは削除する
実装を進めていく中で、不必要になったコードをコメントアウトすることがあるかと思います。しかしコメントアウトされたコードが残っていると、時間が経つうちに、そのコードがアプリケーションにとって本当に必要なのか、コードを見ても判断できなくなっていきます。結果コードの可読性が落ち、保守が難しいプログラムになってしまいますので、不要なコメントアウトされたコードは積極的に削除します。
変数宣言の際、letとvarでは基本letを使う
letで宣言された変数は、一度値を設定すると後から書き換えることができない変数です。
let a = 1
a = 2 // error
反対にvarで宣言された変数は、後でも値を書き換えることができます。
var b = 2
b = 10 // ok!
値の再代入の必要がないにもかかわらずvarを使ってしまうと、コードを読む際「この変数は値が変わる可能性があるから、値の変化を追いかける必要がある」と認識させてしまう可能性があり、不必要にコードの可読性が下がります。変数宣言の際には、基本的にはletを使うようにします。
デコード処理で「!」は使わない
jsonのデコード処理の際、値のキャストが必要になったことがあるのですが、キャストに失敗したときにアプリが落ちるような実装を行ってました。
extension Coordinate {
public init(from decoder: Decoder) throws {
let container = try decoder.container(keyedBy: CodingKeys.self)
// とりあえずDoubleで変換できない値がAPIから来た場合はクラッシュしてもらう
x = Double(try container.decode(String.self, forKey: .x))!
y = Double(try container.decode(String.self, forKey: .y))!
}
}
しかしデコード処理のエラー処理の適切な書き方をレビューで教えて頂きましたので、今後はこの書き方で実装を行います。
extension Coordinate {
public init(from decoder: Decoder) throws {
let container = try decoder.container(keyedBy: CodingKeys.self)
// キャストに失敗した場合はDecodingError.typeMismatchの例外を投げる
// エラーのハンドリングが丁寧でわかりやすい
let rawX = try container.decode(String.self, forKey: .x)
guard let _x = Double(rawX) else {
throw DecodingError.typeMismatch(
String.self,
DecodingError.Context(codingPath: decoder.codingPath, debugDescription: "Can not convert to Double type."))
}
x = _x
let rawY = try container.decode(String.self, forKey: .y)
guard let _y = Double(rawY) else {
throw DecodingError.typeMismatch(
String.self,
DecodingError.Context(codingPath: decoder.codingPath, debugDescription: "Can not convert to Double type."))
}
y = _y
}
}
Binderを積極的に利用する
Binderを利用しない場合ViewModelからデータを取得しUIに渡すコードは、一例ですが以下のような書き方になります。複雑になればなるほど、viewDidLoadが肥大化してしまい、可読性が落ちます。
override func viewDidLoad() {
super.viewDidLoad()
let output = viewModel.transform()
// 処理が長い...
output.response.drive(onNext: { [unowned self] res in
self.dateLabel.text = res.date
self.dayOfTheWeekLabel.text = res.dayOfTheWeek
self.hourLabel.text = res.hour
self.salaryLabel.text = res.salary
}).disposed(by: disposeBag)
}
Binderを利用することで、viewDidLoadのコードの見通しが良くなり、可読性が上がります。
override func viewDidLoad() {
super.viewDidLoad()
let output = viewModel.transform()
// bindingの記述がシンプルに
output.response.drive(binding)
.disposed(by: disposeBag)
}
// binderの処理を別関数で実装
private var binding: Binder<Response> {
return Binder(self, binding: { (vc, res) in
vc.dateLabel.text = res.date
vc.dayOfTheWeekLabel.text = res.dayOfTheWeek
vc.hourLabel.text = res.hour
vc.salaryLabel.text = res.salary
})
}
さいごに
エンジニア4年目にして初めてコードレビューしていただける機会を頂けたのですが、本当に勉強になりましたし、レビューして頂いたエンジニアの方には感謝しかありません。この場をお借りして御礼申し上げます。