【part 4】AppApp のリファクタリング過程紹介【コーディング規約編】

どうも。Reoです。

AppAppのお手入れを開始して日付的に2日目です。part4だ。今日も頑張っていこう。

part1〜3ではSwiftLintを導入して、そのErrorとWarningの解消を行いました。

今回は、SwiftLint が教えてくれないことについて触れていこうと思います。

環境

Xcode11.3.1
Swift 5.1.3
iOS13.3

SwiftLint編

リポジトリ→ uruly/AppApp

今回の目標

今回は全てを直せるとは思っていないので、実際の作業はあんまり行いません。

今後リファクタリングをしていく中で、意識して修正していくことについて触れていきます。

コーディング規約

あらゆる言語にはそれぞれ様々なコーディング規約があります。Swiftを用いたiOS開発でも同様です。

これは会社やプロジェクトにおいても異なる場合があります。

Swift コーディング規約で調べるとそれだけでもたくさんヒットします。それぞれで異なるけれど、でも大抵は同じようなことが書かれています。

https://github.com/cookpad/styleguide/blob/master/swift.ja.md

https://github.com/jarinosuke/swift-style-guide/blob/master/README_JP.md

Swiftコーディング規約@Life is Tech !

等々...

私はコーディング規約として意識して書いてきたというより、初めてiOSエンジニアとしてお仕事をさせて貰う時に、参画するプロジェクトのコードをめちゃくちゃ読み込んだんですよね。

改めてこういうコーディング規約を見てみると、ちゃんと沿ってるんだなぁと実感します。

あらゆるコーディング規約がありますが、自分が意識してやっているかつSwiftLintでは直されないことについて紹介していこうと思います。

アクセス修飾子

public, internal, private のお話です。

アクセス修飾子は最もスコープが狭くなるように設定しましょう。

例えば、AppAppのプロジェクトの一部を見てみます。酷いですよ。

class LabelListViewController: UIViewController {

    var list: [AppLabelData] = []
    var checkArray: [AppLabelData] = [] {
        didSet {
            if checkArray.count > 0 {
                if (self.naviBar.items?.count ?? 0) > 0 {
                    self.naviBar.items![0].rightBarButtonItem?.tintColor = nil
                }
            } else {
                if (self.naviBar.items?.count ?? 0) > 0 {
                    self.naviBar.items![0].rightBarButtonItem?.tintColor = UIColor.lightGray
                }
            }
        }
    }
    var tableView: UITableView!
    var appList: [ApplicationStruct] = []
    var collectionView: UICollectionView!
    var naviBar: CustomNavigationBar!
    var baseVC: BaseViewController!

    // ...以下略
}

はい、何も書いてないねえ！

関数部分もびっくりするくらい何も書いてないです。それ以外の問題も多すぎるんですがそれは追々...

一時期 fileprivate を書くみたいな時期があった気がしてて、その時にRobinを触っていたんですが、なんかしらんけど fileprivate っていっぱい書いてありました。

上記にアクセス修飾子だけ設定してあげます。

final class LabelListViewController: UIViewController {

    var appList: [ApplicationStruct] = []
    var baseVC: BaseViewController!

    private var list: [AppLabelData] = []
    private var checkArray: [AppLabelData] = [] {
        didSet {
            if checkArray.count > 0 {
                if (self.naviBar.items?.count ?? 0) > 0 {
                    self.naviBar.items![0].rightBarButtonItem?.tintColor = nil
                }
            } else {
                if (self.naviBar.items?.count ?? 0) > 0 {
                    self.naviBar.items![0].rightBarButtonItem?.tintColor = UIColor.lightGray
                }
            }
        }
    }
    private var tableView: UITableView!
    private var collectionView: UICollectionView!
    private var naviBar: CustomNavigationBar!

private と final をつけました。

外部から誤って使われることがないようにしてあげます。それ以外の実装が酷いなー。

selfの省略

当時、結構衝撃を受けた気がしました。

アクセス修飾子はハッキリ言って自分の怠慢なのがわかっていたので、仕事で意識して書くようになって慣れました。

selfって書かなくていいんだ。

// 修正前
override func awakeFromNib() {
    super.awakeFromNib()

    self.layer.masksToBounds = false
    self.layer.shadowColor = UIColor.darkGray.cgColor
    self.layer.shadowOffset = CGSize(width: 2, height: 2)
    self.layer.shadowRadius = 4
    self.layer.shadowOpacity = 0.5
}

// 修正後
override func awakeFromNib() {
    super.awakeFromNib()

    layer.masksToBounds = false
    layer.shadowColor = UIColor.darkGray.cgColor
    layer.shadowOffset = CGSize(width: 2, height: 2)
    layer.shadowRadius = 4
    layer.shadowOpacity = 0.5
}

プロジェクトによるかもしれませんが、self はできる限り省略します。self を使わないとコンパイラに怒られるところにのみ用いるようにします。

AppAppでは何も意識してないので、selfがあったりなかったりします。一番ｲｸﾅｲ。

クロージャの循環参照対応

[weak self] もしくは [unowned self] を書くことを、一体どこの誰から学べばよかったのか。と思って手持ちの詳解Swiftを見てみると書いてありました。ですよねぇ。

そもそもAppAppであんまりクロージャ使ってない気がしますね。どこにあるか探すのが大変なので見つけたら直します。

以下 OKAIMO たんから。

readLabel { [weak self] in
    self?.setupChildren()
}

迷ったらつけるようにしてますが、全てのクロージャに必須なわけではないようです。

You don’t (always) need [weak self] – Flawless iOS – Medium

クロージャって概念を理解するのにも結構かかったんですが、さらに非所有参照...？弱参照？なにそれ...って感じですよね。

つけなくてもエラーを起こすこともないので、ただ循環参照でメモリリークしてるかもなんて初心者はわからんがなですよ。

AppApp 実装当時は、どこにつけるべきなのかわからず、仕事始めてからは全部のクロージャにつけて、そうして今ようやく少し使い分けができるようになってきたレベルです。

説明は他の方にお任せします。いっぱい出てきます。（逃げ

いっぱい書いてると煩わしくなってくる[weak self] 、以下のような方法を取っている方もいて奥が深いですね...。

【swift】コールバックから[weak self]をなくす方法

ライフサイクル等のグルーピング

単純に自分が適当な人間なのがよくわかることなんですが、クラス内に適当に関数や変数を宣言するのはやめましょう...

もはや AppApp のコードを貼るのを諦めてOKAIMOのコードを貼りますけども。

final class PageViewController: UIPageViewController {

    weak var pageDelegate: PageViewControllerDelegate?

    var currentPage: Int = 0
    var isDuringAnimation: Bool = true

    // MARK: - Initializer

    init(first viewController: UIViewController) {
        super.init(transitionStyle: .scroll, navigationOrientation: .horizontal, options: nil)
        setViewControllers([viewController], direction: .forward, animated: false, completion: nil)
    }

    required init?(coder: NSCoder) {
        fatalError("init(coder:) has not been implemented")
    }

    // MARK: - Life cycle

    override func viewDidLoad() {
        super.viewDidLoad()
        setup()
    }

    // MARK: - Private method

    private func setup() {
        delegate = self
        dataSource = self
        let scrollView = view.subviews.compactMap { $0 as? UIScrollView }.first
        scrollView?.delegate = self
    }

}

// MARK: - UIPageViewControllerDelegate

extension PageViewController: UIPageViewControllerDelegate {}

Swift 3.0 Coding Standard - Qiita にあるものとは若干違うんですが。

ちゃんと仲間をまとめてあげるだけで見やすくなります。

これも viewDidLoad で一度しか使わないものをsetup()に切り出す必要がないのでは？とか考える余地はいっぱいあると思います。

UIViewControllerのライフサイクルをちゃんと呼び出される順に書くとか、あっちこっちに書かないとか、そういうことも大事です。大事だよって過去の自分に言いたい。あっちこっちに書くなー！

その他

多分キリがないくらいルールはあると思います。デファクトスタンダードになっている規約や独自ルールまで。

switch文のcaseの後は改行してから書こうとか、UIView等のUIKitのパーツの設定は didSet で書くとか。delegateはextensionで書こうぜとか。

final class ViewController {

    @IBOutlet private weak var tableView: UITableView! {
        didSet {
            tableView.register(R.nib.labelSettingColorTableViewCell)
            tableView.delegate = self
            tableView.dataSource = self
            tableView.addGestureRecognizer(tapGestureRecognizer)
        }
    }

    // なんか色々処理
}

// MARK: - UITableViewDelegate

extension ViewController: UITableViewDelegate {}

その他にも色々あるはず。

忘れてた！あと最後に大事なの。

使うかどうか迷っているコードをコメントアウトしたままコミットをして更にはマージまでするんじゃない！

AppApp見ると本当にひどいんですよ、いらないコードをコメントアウトしたまま放置してある。いるのかいらないのかもうわからない...

git管理していたら戻れるので使わないコードはちゃんと消しておきましょうね...

まとめ

丁寧にコードを書こう。それに尽きます。

今回あげたのは、昔の自分ができてなかったことばかりです。

入門本や文法本を読んで、アプリを何本か作ったけれど、結局意識をしてできるようになったのはiOSエンジニアとして仕事を始めてからでした。

仕事を始める時に、この人のコードめっちゃ綺麗だなって思ったのが始まりで、その後はとにかく見よう見まねでやってきました。

今回紹介したものも、プロジェクトによっては正解ではないかもしれません。

良いコードを見かけたらどんどん真似してみましょう。

次回

なにしよう。今回コードはほとんど触ってないですね。謎実装が多くて見てられなくなった。

あー。次回は、ディレクトリ構成について書いていきましょうかね。ちょっと整理しないと。

おわりに

なんか書きたいことかけたのかわからんけど。

少し自分語り。Swift遍歴を紹介します。

2015年に作って学ぶ系の本でSwiftを始めました。その後 pocket時間割というアプリを作成。

それから詳解Swiftを読みました。多分このタイミングのはず。

ideaDASH, Robin といったアプリを作成。チーム開発を始めるも、iOS担当が自分だけだったので動けばなんでもよかった。

2017年末にAppAppを作成。

そして2018年末から初めてレビューをしてもらいながらiOSエンジニアとして仕事を始めました。

最新作のお買い物メモアプリ「OKAIMO」を作ったのは2019年の夏頃です。

詳しくは、管理人について。

現在AppStoreで公開しているものはOKAIMOだけです。

独学で4年間ほど Swift を触っていましたが、結局その4年間なんだったんだってぐらい、人と同じプロジェクトを触ることで学んだことは大きいです。

仕事を始める前に作ったAppAppと、仕事を始めた後に作ったOKAIMO。

本来ならAppAppのコードを使いまわしてOKAIMOが出来上がってもいいはずのデザインなのに、そうできなかった理由は、コードのキレイさと再利用性のなさでした。

最近ではRxSwiftをやらせてもらっていたので、OKAIMOですら FatViewController に感じます。でもCocoa MVCでやるなら結構使えるところはあるかなぁ。

かれこれ5年はやっているはずなのに、いつまで経っても実装に自信は持てないし、知らないこともまだまだたくさんあります。それにプラス、仕様変更や新しい実装パターンもでてきます。

それでも、こうやって何かしらのきっかけで少しずつだけれど確実に以前よりは良くなってきました。人と比べてキレイなコードを書けているかわからないけれど、昔の自分よりはキレイに書けているのは間違いないです。

これからも、丁寧にコードを書いて、柔軟に良いと思ったものを取り入れていきたいです。

ではでは。次行ってみよう。