どうも、ねこきち（@nekokichi1_yos2）です。

リーダブルコード改善シリーズ、第4弾、です。

（詳しくは下記をご参照ください）
リーダブルコードで読みにくいコードを改善していく ~ 準備編
 リーダブルコードで読みにくいコードを改善していく ~ 名前編
 リーダブルコードで読みにくいコードを改善していく ~ 美しさ編
 リーダブルコードで読みにくいコードを改善していく ~ コメント編

今回は、コード全体を改善していきます。

リーダブルコードの教え

「条件式を揃える」
・条件は肯定形を使う
・分かりやすい条件を先に書く

「三項演算子は何も凄くない」
・コードを短くできるが、読みにくい

「関数は早めに返す」
・出口を複数用意する
・コードが冗長にならずに済む

「ネストは簡潔でシンプルに」
・ネストが深くなれば、読み手に負担がかかる
・条件式や変数を常に覚える必要があり、ネスト内部の理解に集中力を要する

「説明変数、要約変数」
・説明変数：変数名でどんな値かを示す
・要約変数：巨大な値や式を変数でまとめる

「短絡評価の悪用」
・単に少ないコードが良いわけではない
・同じ内容でも、理解しやすい方が望ましい

「巨大な式を分割する」
・同じ式が何度も登場する場合、要約変数を使用
・メリット：タイプミスの現象、コードが短くなる、修正しやすくなる

「変数を削除」
・変数を使う必要があるか？を考慮する
・変数に代入しなくても、楽に理解できるなら必要なし

「変数のスコープを縮める」
・スコープが広いと、どこで変更されたか追跡が難しくなる
・グローバル→ローカル、にスコープを縮めよ
・アクセス修飾子（private,public,static）を使用する

「定義の位置を下げる」
・定義と処理に区切ると、常に変数の値を把握する必要がある
・定義と処理をセットにすれば、必要な変数を見つけられ、読みやすくなる

「変数は一度だけ書き込む」
・変更される回数が多いと、現在値がわからなくなる
・letやprivateを使用し、永続的な変更を禁止にする
・説明変数などで変更される回数を減らす

巨大な式を分割する

memoTextViewのattributedTextをData()に変換していますが、UIの値をそのまま代入するのは危なく、プロパティ名が長いので、guardlet文での変数inputAttrTextに入れました。

AddMemo.swift

let attributedMemoData = try! NSKeyedArchiver.archivedData(withRootObject: memoTextView.attributedText!, requiringSecureCoding: false)

↓

AddMemo.swift

guard let inputAttrText = memoTextView.attributedText else { return }
let attributedMemoData = try! NSKeyedArchiver.archivedData(withRootObject: inputAttrText, requiringSecureCoding: false)

prepare()内で遷移先の変数に値を代入しています。

memoTableViewの選択されたrow番目の値を参照してますが、プロパティ名が長すぎて見づらいので、変数indexPathRowを用意し、代入する式の長さを短くしました。

ViewController.swift

let vc = segue.destination as! DisplayMemo
vc.selectedMemoObject   = memoListForRealm[memoTableView.indexPathForSelectedRow!.row]
vc.selectedMemoString   = memoList[memoTableView.indexPathForSelectedRow!.row]
vc.selectedIndexPathRow = memoTableView.indexPathForSelectedRow!.row

↓

ViewController.swift

let vc = segue.destination as! DisplayMemo
guard let indexPathRow = memoTableView.indexPathForSelectedRow?.row else {
    return
}
vc.selectedMemoObject   = memoListForRealm[indexPathRow]
vc.selectedMemoString   = memoList[indexPathRow]
vc.selectedIndexPathRow = indexPathRow

変数と読みやすさ

不要な変数を削除

変数memoObjectのプロパティに値を代入しています。

が、Data()に変換したinputAttrTextを変数attributedMemoDataに代入してますが、わざわざ変数を介して渡すより、直接渡した方がコードを1文減らせるので、変数attributedMemoDataを削除しました。

AddMemo.swift

@IBAction func addMemo(_ sender: Any) {
    guard let inputAttrText = memoTextView.attributedText else { return }
    let memoObject             = MemoModel()
    // memoTextView.attributedText -> Data()
    let attributedMemoData     = try! NSKeyedArchiver.archivedData(withRootObject: inputAttrText, requiringSecureCoding: false)
    memoObject.data            = attributedMemoData
    memoObject.identifier      = String().randomString()

↓

AddMemo.swift

@IBAction func addMemo(_ sender: Any) {
    guard let inputAttrText = memoTextView.attributedText else { return }
    let memoObject             = MemoModel()
    // memoTextView.attributedText -> Data()
    memoObject.data            = try! NSKeyedArchiver.archivedData(withRootObject: inputAttrText, requiringSecureCoding: false)
    memoObject.identifier      = String().randomString()

セルをスワイプで削除した時、RealmとtableViewに使用する配列のそれぞれの値を削除してます。

しかし、RealmはResults<モデル名>に指定した変数を操作すれば、その変更が反映されるので、無駄な処理を消しました。

ViewController.swift

func tableView(_ tableView: UITableView, commit editingStyle: UITableViewCell.EditingStyle, forRowAt indexPath: IndexPath) {
    let selectedMemo = memoListForRealm[indexPath.row]

    //Realm-Delete
    try! realm.write() {
        realm.delete(selectedMemo)
        realm.delete(memoListForRealm[indexPath.row])
    }

↓

ViewController.swift

func tableView(_ tableView: UITableView, commit editingStyle: UITableViewCell.EditingStyle, forRowAt indexPath: IndexPath) {

    //Realm-Delete
    try! realm.write() {
        realm.delete(memoListForRealm[indexPath.row])
    }

変数のスコープを縮める

privateは、参照(get)と変更(set)を制限するアクセス修飾子です。

privateを付与することで、他ファイルや関数から利用されるのを防ぎ、他ファイルでの変更や参照を気にすることなくなります。

また、privateで
（この変数は他ファイルでは使われない）
と明示的に示すことができ、コードの追跡する手間が無くなります。

なので、可能な限り、全ての変数と関数にprivateをつけました。

@IBActionにまでprivateをつける必要はありませんが、修飾子が無いと、どこかで実行されて無いかと思うので、敢えてつけました。

ViewController.swift

@IBOutlet weak var memoTableView: UITableView!

// Realmへ保存用のメモリスト
var memoListForRealm:Results<MemoModel>!
// Realm
let realm               = try! Realm()
// メモリスト
var memoList            = [NSAttributedString]()

↓

ViewController.swift

@IBOutlet private weak var memoTableView: UITableView!

// Realmへ保存用のメモリスト
private var memoListForRealm:Results<MemoModel>!
// Realm
private let realm               = try! Realm()
// メモリスト
private var memoList            = [NSAttributedString]()

AddMemo.swift

let realm       = try! Realm()
let imagePicker = UIImagePickerController()

@IBAction func addMemo(_ sender: Any) {

@IBAction func attachImageGesture(_ sender: UILongPressGestureRecognizer) {

↓

AddMemo.swift

private let realm       = try! Realm()
private let imagePicker = UIImagePickerController()

@IBAction private func addMemo(_ sender: Any) {

@IBAction private func attachImageGesture(_ sender: UILongPressGestureRecognizer) {

EditMemo.swift

@IBOutlet weak var memoTextView: UITextView!

// Realm
let realm                       = try! Realm()
let imagePicker                 = UIImagePickerController()

@IBAction func attachImageGesture(_ sender: UILongPressGestureRecognizer) {

@IBAction func updateMemo(_ sender: Any) {

↓

EditMemo.swift

@IBOutlet private weak var memoTextView: UITextView!

// Realm
private let realm                       = try! Realm()
private let imagePicker                 = UIImagePickerController()

@IBAction private func attachImageGesture(_ sender: UILongPressGestureRecognizer) {

@IBAction private func updateMemo(_ sender: Any) {

DisplayMemo.swift

@IBOutlet weak var memoTextView: UITextView!

↓

DisplayMemo.swift

@IBOutlet private weak var memoTextView: UITextView!

finalは継承とオーバーライドを制限する修飾子です。

明示的に、このクラスは継承もオーバーライドもできません、と示します。

class ViewController: UIViewController,UITableViewDelegate,UITableViewDataSource {
class AddMemo: UIViewController {
class DisplayMemo: UIViewController {
class EditMemo: UIViewController {

↓

final class ViewController:UIViewController,UITableViewDelegate,UITableViewDataSource {
final class AddMemo: UIViewController {
final class EditMemo: UIViewController {
final class DisplayMemo: UIViewController {

定義の位置を下げる

変数宣言と関数の実行を分けると、見やすくなりますが、各変数の存在を覚えておかないと、処理の順序がわからなくなります。

そこで、変数を定義する位置を下げることで、実行される処理の近くに必要とされる変数が定義されてるので、理解度が高まります。

下のコードでは、変数の定義、関数の処理、それぞれのコードが多いので分けています。

ですが、定義されている変数が多く、一度に全部を覚えながら、後々の処理を理解するのは困難でした。

そこで、関連する変数と処理をセットにして、整理しました。

ViewController.swift

func imagePickerController(_ picker: UIImagePickerController, didFinishPickingMediaWithInfo info: [UIImagePickerController.InfoKey : Any]) {
    if let pickerImage = info[.originalImage] as? UIImage {
        // NSAttributedStringへ変換に必要なパラメーター
        let width                 = pickerImage.size.width
        let padding               = self.view.frame.width / 2
        let scaleRate             = width / (memoTextView.frame.size.width - padding)
        // 10%に圧縮した画像
        let resizedImage          = pickerImage.resizeImage(withPercentage: 0.1)!
        let imageAttachment       = NSTextAttachment()
        var imageAttributedString = NSAttributedString()
        // memoTextView.attributedText -> NSMutableAttributedString
        let mutAttrMemoText       = NSMutableAttributedString(attributedString: memoTextView.attributedText)

        // resizedImage -> NSAttributedString()
        imageAttachment.image = UIImage(cgImage: resizedImage.cgImage!, scale: scaleRate, orientation: resizedImage.imageOrientation)
        imageAttributedString = NSAttributedString(attachment: imageAttachment)
        mutAttrMemoText.append(imageAttributedString)
        // 画像を追加後のテキスト -> memoTextView.attributedText
        memoTextView.attributedText = mutAttrMemoText
    }
    dismiss(animated: true, completion: nil)
}

↓

ViewController.swift

func imagePickerController(_ picker: UIImagePickerController, didFinishPickingMediaWithInfo info: [UIImagePickerController.InfoKey : Any]) {
    if let pickerImage = info[.originalImage] as? UIImage {
        let width                 = pickerImage.size.width
        let padding               = self.view.frame.width / 2
        let scaleRate             = width / (memoTextView.frame.size.width - padding)
        // 10%に圧縮した画像
        let resizedImage          = pickerImage.resizeImage(withPercentage: 0.1)!
        let imageAttachment       = NSTextAttachment()
        // resizedImage -> NSAttributedString()
        imageAttachment.image = UIImage(cgImage: resizedImage.cgImage!, scale: scaleRate, orientation: resizedImage.imageOrientation)

        var imageAttributedString = NSAttributedString()
        imageAttributedString = NSAttributedString(attachment: imageAttachment)

        // memoTextView.attributedText -> NSMutableAttributedString()
        let mutAttrMemoString     = NSMutableAttributedString(attributedString: memoTextView.attributedText)
        mutAttrMemoString.append(imageAttributedString)
        // 画像を追加後のテキスト -> memoTextView.attributedText
        memoTextView.attributedText = mutAttrMemoString
    }
    dismiss(animated: true, completion: nil)
}

UIAlertControllerに使用する、2つのUIAlertActionの定義と実装をセットにしました。

@IBAction private func attachImageGesture(_ sender: UILongPressGestureRecognizer) {
    let alert = UIAlertController(title: "画像を添付", message: nil, preferredStyle: .actionSheet)

    let okAction = UIAlertAction(title: "OK", style: .default) { (action) in
        self.present(self.imagePicker, animated: true, completion: nil)
    }
    let cancelAction = UIAlertAction(title: "キャンセル", style: .cancel, handler: nil)

    alert.addAction(okAction)
    alert.addAction(cancelAction)

    present(alert, animated: true, completion: nil)
}

↓

@IBAction private func attachImageGesture(_ sender: UILongPressGestureRecognizer) {
    let alert = UIAlertController(title: "画像を添付", message: nil, preferredStyle: .actionSheet)

    let okAction = UIAlertAction(title: "OK", style: .default) { (action) in
        self.present(self.imagePicker, animated: true, completion: nil)
    }
    alert.addAction(okAction)

    let cancelAction = UIAlertAction(title: "キャンセル", style: .cancel, handler: nil)
    alert.addAction(cancelAction)

    present(alert, animated: true, completion: nil)
}

まとめ

ここまでリーダブルコードを参考にコードを改善してきましたが、本書は王道で基本的なリファクタリングを学べる本だと分かりました。

劇的にコードの量を減らして、誰もが理解できる美しいコードを書ける方法など書いてありません。

単純に
・変数、関数の名前を変える
・分かりやすいコメントを加える
・処理の順番を変える
など、簡単な方法ばかりでした。

しかし、リファクタリング=コードを綺麗にする、とは、地道な作業ばかりです。

簡単な方法だからこそ、何時も忘れてはならない、当たり前の方法だと思います。

本書の内容は技術書の中では読みやすいですが、基本に忠実なリファクタリングの方法が載っています。

今後エンジニアとして生きていくなら、本書からリファクタリングを学ぶのがおすすめです。