文字っぽいの。

文字を書いています。写真も混ざります。

Swiftのコードレビュー勘所

はじめに

  • Swift with iOSなコードをレビューする時に僕が気をつけて見ているところをざっくりまとめました
  • 今年からSwift始めます!って人が読めばクラッシュしやすいコードを書きにくくなるはず
  • ロジック面についてもちゃんと確認しましょう

勘所

AnyObject使ってる

不適切に使われてる(型を消すとか)場合には絶対に直させる。型を消したい場合は protocol + typealias とかgenerics でなんとかできる場合が多い。というか AnyObject を使ってるコードでは、型を消す必要がない場合が多い。

配列にindex指定してアクセスしている

let item = items[index]

とか。基本的にはindexなど使わずにアクセスするのが一番なので、書き直せないか確認する。 書き直せない場合はロジック自体の設計を変えられないか、もしくははみ出ない処理がちゃんと書かれているかを確認している。

Force Unwrapしてる

let width = optionalView!.frame.width

let dic = unknownAnyObject as! [String : String]

someFuction(optionalData!)

クラッシュ原因ほとんどはこれ。安易にForce Unwrapすべきではない。

if let width = optionalView?.frame.width { 
    // この中で `witdh` を使う
}

if let dic = unknownAnyObject as? [String : String] { 
    // この中で `dic` を使う
}

if let data = optionalData {
  someFunction(data)
}

ちなみに、StoryBoard系とかCell系とかの呼び出しには使っても良い。

let storyboard = UIStoryboard(name: "WelcomePage", bundle: nil)
let viewController = storyboard.instantiateInitialViewController() as! WelcomePageViewController

なぜなら、これらが無かった時にエラーハンドリングできない。できても画面が真っ白になるとかそういう感じなので、デバッグ中やQA中にクラッシュしてもらって素早く直した方が良い。

非同期処理の中で self 呼んでるのに weak or unowned になってない

let queue = dispatch_queue_create("com.example.task", DISPATCH_QUEUE_SERIAL)
dispatch_async(queue) {
    self.someFunction(self.someData)
}

非同期処理中では self が先に消えてることがあり、アクセスするとクラッシュする場合がある。

let queue = dispatch_queue_create("com.example.task", DISPATCH_QUEUE_SERIAL)
dispatch_async(queue) { [weak self]
    self?.someFunction(self?.someData)
}

weak にしておくと、block内のselfがOptionalとして扱われるので、安心して使える。

追記(7月5日21時)

これは誤りです。非同期処理中のselfは強参照で確保されているため、selfが先に消えることはないため、weakunownedにする必要はありません。ブコメ等でご指摘いただいた皆様、ありがとうございました。

delegateweak じゃない

protocol SomeDelegate: class {
    func updateItem()
}

class SomeClass {
    var delegate: SomeDelegate?
}

とか宣言されてる。ようこそ循環参照の村へという感じになる。 delegate には大体親の ViewController から self を渡すので、これを weak にしておかないと SomeClassインスタンスは永遠に開放されない。「なんで deinit()呼ばれないんだろ?」と思ったらだいたいこれが原因。

protocol SomeDelegate: class {
    func updateItem()
}

class SomeClass {
    weak var delegate: SomeDelegate?
}

ちゃんとweakにしようなっ!

nilチェックを != でしてる

if optionalValue != nil {
    awesomeMethod(optionalValue!)
}

Obj-Cっぽい。Swiftでこういう書き方をしなければいけない場面はほぼない。

if let value = optionalValue {
    awesomeMethod(value)
}

と書ける。もし、nilかどうかだけ見たくてblock内では利用しない場合は

if let _ = optionalValue {
    // do something
}

と書くのが良い。

guardによる早期リターンをしてない

if someFlag {
    // 10行ぐらいの処理
} else {
    // 1行の処理
}

10行の処理って書いてるけど、だんだん10行とかでは済まない長さになり、ネストが深くなり、可読性が下がる。可読性が下がるとエンバグしやすくなる。エンバグすると人が死ぬ。

guard someFlag else {
    // 1行の処理 
   return
}

// 10行の処理

と素早くreturnしてのが良い。

繰り返し処理の中で guardifreturn してる

for item in items {
    guard item.someFlag else {
        return
    }
}

guard慣れしてくるとうっかりミスる奴。returnではなくてbreak とか continue じゃないとダメな場合があるので、気をつけて見ている。

?? で書けるところを if で分岐している

let x: CGFloat
let y: CGFloat
if let view = optionalView {
    x = view.frame.x
    y = view.frame.y
} else {
    x = 0
    y = 0
}

大体の場合においてコードが長くなって可読性が下がる。optionalValuenil だった場合にエラーハンドリングしたい場合はこの書き方で良いが、値が欲しくてデフォルト値も決まっている場合は

let x: CGFloat = optionalView?.frame.x ?? 0
let y: CGFloat = optionalView?.frame.y ?? 0

と書くと良い。

min(x: T, y: Y) や max(x: T, y: Y) を使ってない

if view.frame.width > 540 {
    width = 540
} else {
    width = view.frame.width
}

view のサイズが540より大きかったら540にして、そうじゃないならview のサイズを使うというやつ。こういう処理をする時はだいたい高さや縦横比なんかの計算が混ざるので、上記みたいにシンプルに終わらず if のネストが始まってつらい。

let width = min(540, view.frame.width)

こうするとシュッと終わる。