文字っぽいの。

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

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)

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

ターミナルから音速でXcodeプロジェクトを開く

iOS開発をしていると、*.xcworkspace*.xcodeprojをよく開きます。Xcodeはよく落ちるし調子が悪くなると再起動しないといけないので、1日に何度も開いたりします。

最弱

$ cd ios-project
$ open .
# おもむろにクリック

弱い

$ cd ios-project
$ ls 
# ディレクトリ内の様子みる
$ open awesome-app.xcworkspace

たぶんこれが一番早いと思います

function open-xcode-project() {
    open *.xcworkspace || open *.xcodeproj || echo 'fatal: Not a Xcode repository'
}
alias xc='open-xcode-project'

これを設定しておけば

$ cd ios-project
$ xc

だけで開く

もっと早くする

ディレクトリに移動するのもxcを打つのも面倒なので、pecoと組み合わせます。

function open-xcode-project() {
    open *.xcworkspace || open *.xcodeproj || echo 'fatal: Not a Xcode repository'
}

function peco-cdr-and-open-xcode () {
    local selected_dir=$(cdr -l | awk '{ print $2 }' | peco)
    if [ -n "$selected_dir" ]; then
        BUFFER="cd ${selected_dir} && open-xcode-project"
        zle accept-line
    fi
    zle clear-screen
}
zle -N peco-cdr-and-open-xcode

bindkey "^x^o" peco-cdr-and-open-xcode

こうしておくと Ctrl-x Ctrl-o と押すだけでpecoで最近開いたディレクトリを検索でき、ディレクトリを選ぶと、その中のXcodeプロジェクトが開かれます。

『ライアン・マッギンレー BODY LOUD!』を観に行った。

オペラシティで開催されている、『ライアン・マッギンレー BODY LOUD !|東京オペラシティアートギャラリー』を観に行ってきた。

写真撮影可能だったので、気に入ったのを撮った。

f:id:FromAtom:20160515162639j:plain f:id:FromAtom:20160515162846j:plain f:id:FromAtom:20160515163008j:plain

GW後の土日ということもあり、あまり人もいなくて良かった。特にこのインスタレーションがすごかった。

f:id:FromAtom:20160515161736j:plain

ヌードというよりも、動物としてのヒトを撮影した感じで良さがあった。こういう生き物がいるんだなぁという感じ。ちょうど、『家畜人ヤプー』を読んでいる所だったので、「人間よりも高位の存在がいたら。」とか考えながら見てた。

新宿から1駅なので、暇があったら行ってみると良いと思います。7月10日までとのこと。

そういえば、ふともも写真の世界展に行きそびれたので、僕はもうダメです。

健康を確認した。

健康診断の結果が返ってきた。

f:id:FromAtom:20160509115840j:plain

オールAで圧倒的健康。 社会人になって美味いもの食いまくって、ビール飲みまくったので、痛風になるんじゃないかとヒヤヒヤしていたけど尿酸値も正常だった。これからもジャブジャブビール飲むぞ!!!

「これバグじゃね?」で始めるコミュニケーションはやめよう。

バグを見つけて教えてくれるのは嬉しいしありがたい。ユーザの体験損失や脆弱性を防ぐことができて良い。これは何にも勝る絶対的な真理である。

しかし、なにか変な挙動を見つけた時に脊髄反射的に「これバグじゃね?」と言うのは良くない。この発言はただの いちユーザの発言 であり、サービスやアプリを提供する側の人間としては、恥ずべき発言だと思う。

そもそも、その挙動の「バグか、仕様か」を判断するのは開発者であって、ユーザではない。「これバグじゃね?」は思考停止した愚かな発言であり、

  • この文脈上は正しい
  • あるプラットフォームの制約や文化の上では正しい
  • 人月や負債の関係でこれが限界だった
  • メインのスコープじゃないし致命的じゃないから後回しにしてる

という背景がある(かもしれない)挙動を十把一絡げに「バグじゃね?」と言うのは良くない。

「これバグじゃね?」と言われた時の感覚は「なんかお前臭くね?」と言われたのと同じ感じがある。「えっ、マジで?なんか変なの食べたっけ?にんにく?ニンニク食べたっけ?昨日風呂入ったよな?入ったよ。入浴剤も入れて入ったね。もしかして加齢臭?これが噂の加齢臭なのか?もしかしてワキガ?ワキガなのか?手術とか受けたほうが良いのかな?つら。」ぐらいに脳みそが回転してバグに構える。それだけエンジニアはバグに対して敏感に生きている。

もし「バグじゃね?」と言われた挙動がバグだった場合は「バグだわ。死にたい。」という気持ちになるし、バグじゃなくて意図した挙動だった場合は「なんやお前殺すぞ。」という気持ちになる。上記の例では「あぁ、お前の匂いじゃないわ。なんかエアコンが臭いわ。お前臭くねーわwwがははww」とか言われた感じですね。殺してやりたくなりますよ。

じゃあどうすればいいの?

明らかなバグを見つけた時

アプリを触ってたらクラッシュしたり、ふぁぼったのに反映されてなかったり、500エラーが出てたり。明らかなバグなら「バグっぽいです。」と教えてあげればよいです。

また、それを伝える際に どういう手順で動かすとバグる を一緒に伝えてあげると大変助かると思います。「この画面でボタン押したらバグったけど、今は大丈夫。」って言われても「何もしてないのにパソコンが壊れた。」と言われるのと同じくらい「どうしろってんだ」感があります。

なんか変な挙動を見つけた時

「この画面で、こうしたらAになったんですが。なんか私的には、Bになる気がするんですが、なにか理由がありますか?」とか聞いてもらえると、意匠がある場合はそれを教えることができますし、ただのバグなら「殺してくれ。」って良いながらIssueを立てます。

まとめ

最初から殴る気満々でコミュニケーションを始めるのはやめよう。

自転車パクられたけどチームメンバーにプレゼントしてもらった。

2015年10月、僕は自転車をパクられた。スーパーで買い物している時に、鍵をかけてなかったので窃盗犯にとってはとてもありがたい状態だったであろう。「鍵かけてないのが悪い」という意見もあると思うが、自転車をパクった奴が悪いのである。「痴漢されるのは扇情的な格好をしていた被害者も悪い」みたいな言説は本当にしょうもない。とにかく毎日スーパーに立ち寄るたびに犯人を呪っていたし、いい感じに車に轢き殺されてギリギリ死なない重症をキメて欲しいと今でも思ってる。

そんなわけで自転車がなくなり、毎日片道50分かけて徒歩で通勤することになった。案外50分歩くというのは簡単で、満員電車に押し込められておっさんの芳香に包まれるより全然良い。「自転車が無くなったくらいで遅刻などしない。窃盗犯には屈しない。」という謎の使命感を持って通勤していた。ただ、毎日2時間が移動によって消費されると結構不便である。可処分時間が減ると体調管理も甘くなるし、なにより趣味のコーディングやゲームをする時間が無くなる。料理をする時間ももったいないし、徒歩で帰宅中に腹が減ってしょうがないので、コンビニ飯や外食が増えた。

そんな僕を憂いて、誕生日プレゼントとして、チームメンバーが自転車をプレゼントしてくれた。圧倒的感謝。自転車はAmazonサンタがオフィスに届けてくれた。Amazonで自転車買えるんですね。すごい。

f:id:FromAtom:20160413192518j:plain

新しい自転車はデフォルトで鍵も変速機もライトもついているし、オレンジ色でかわいくて最高という感じ。ちなみにAmazonで買うと防犯登録はされてないので、自分で自転車屋に持って行って登録してもらう必要があるので注意。

チームメンバーのおかげで今日も元気に出社できました。ありがとうございます!問題は今日の天気予報が夕方から雨ってことです。