どうも、ねこきち（@nekokichi1_yos2）です。
リーダブルコード改善シリーズ、第3弾、です。
（詳しくは下記をご参照ください）
[リーダブルコードで読みにくいコードを改善していく ~ 名前編]
(https://qiita.com/NEKOKICHI2/items/dc774bd79b1623032712)
リーダブルコードで読みにくいコードを改善していく ~ 準備編
今回は、コードを美しいコードに改善していきます。
リーダブルコードの教え
・共通する要素を持つコードをまとめる
・見た目の並びを改行やスペースで整列させる
->目を1,2方向に動かすだけで各コードを確認できる
->ただし、似たコードがあれば、手間が増える
・ブロック、グループに分ける
->複数のコードを一括して理解できる
・処理の手順を段落で分ける
・一貫性を持たせる
->コーディング規約など
->何人もの個人的な解釈や書き方が混在し、読みにくくなる
美しくないコードを探す
ViewController
override func viewWillAppear(_ animated: Bool) {
super.viewWillAppear(animated)
let realm = try! Realm()
attributedTextArray = [NSAttributedString]()
memoList = realm.objects(MemoModel.self)
if memoList.count > 0 {
for i in 0...memoList.count-1 {
let attributeText = try! NSKeyedUnarchiver.unarchiveTopLevelObjectWithData(memoList![i].data) as! NSAttributedString
attributedTextArray.append(attributeText)
}
}
memoTableView.reloadData()
}
・空行がないので、全てのコードが重要に見えてしまい、読みづらい
・Realmのデータに関する処理だと分かるが、コメントがあれば、細かく把握できる
・Realmの処理とTableViewの処理は空行で分けた方がいい
override func prepare(for segue: UIStoryboardSegue, sender: Any?) {
if segue.identifier == "display" {
let vc = segue.destination as! DisplayMemo
vc.selectedMemoObject = memoList[memoTableView.indexPathForSelectedRow!.row]
vc.selectedMemo_attributedText = attributedTextArray[memoTableView.indexPathForSelectedRow!.row]
vc.selectedIndexPathRow = memoTableView.indexPathForSelectedRow?.row
}
}
・インスタンスの取得、プロパティへの代入、は分けた方が見やすい
・プロパティへの代入が3度行われてるので、縦に揃えれば見やすい
AddMemo
@IBAction func addMemo(_ sender: Any) {
let realm = try! Realm()
let memoObject: MemoModel = MemoModel()
let archivedAttributedText = try! NSKeyedArchiver.archivedData(withRootObject: memoTextView.attributedText!, requiringSecureCoding: false)
memoObject.data = archivedAttributedText
memoObject.identifier = String().randomString()
try! realm.write{
realm.add(memoObject)
}
self.navigationController?.popViewController(animated: true)
}
・変数の宣言、値の代入、書き込み、は段落で分けた方がいい
・Realmの書き込み処理、ナビゲーションコントローラの戻る処理、をクロージャでまとめる方法もある
func imagePickerController(_ picker: UIImagePickerController, didFinishPickingMediaWithInfo info: [UIImagePickerController.InfoKey : Any]) {
if let pickerImage = info[.originalImage] as? UIImage {
let mutAttrMemoText = NSMutableAttributedString(attributedString: memoTextView.attributedText)
let resizedImage = pickerImage.resizeImage(withPercentage: 0.1)!
let width = pickerImage.size.width
let padding: CGFloat = self.view.frame.width / 2
let scaleRate = width / (memoTextView.frame.size.width - padding)
let imageAttachment = NSTextAttachment()
imageAttachment.image = UIImage(cgImage: resizedImage.cgImage!, scale: scaleRate, orientation: resizedImage.imageOrientation)
let imageAttributedString = NSAttributedString(attachment: imageAttachment)
mutAttrMemoText.append(imageAttributedString)
memoTextView.attributedText = mutAttrMemoText
}
dismiss(animated: true, completion: nil)
}
・変数宣言、代入・実行の処理、はそれぞれでまとめるべき
・変数宣言の行が多いので、何らかの順序で並べ方が良い
DisplayMemo
var selectedMemoObject: MemoModel = MemoModel()
var selectedMemo_attributedText = NSAttributedString()
var selectedIndexPathRow: Int!
・=を縦に揃えた方がいい
・型指定、初期化、どちらで型を決めるかを共通にするべき
EditMemo
let imagePicker = UIImagePickerController()
var memoList:Results<MemoModel>!
var selectedMemoObject: MemoModel = MemoModel()
var selectedMemo_attributedText = NSAttributedString()
var selectedIndexPathRow: Int!
・ジェネリクスの宣言、インスタンスの宣言、変数の宣言、に分けてまとめた方がいい
（扱うデータや存在感においても、変数よりインスタンスやジェネリクスの方が大きい）
・型の書き方は一貫して揃えた方がいい
（型の指定、インスタンスでの初期化、のどちらかにするべき）
override func viewDidLoad() {
super.viewDidLoad()
imagePicker.delegate = self
imagePicker.sourceType = .photoLibrary
let realm = try! Realm()
memoList = realm.objects(MemoModel.self)
memoTextView.attributedText = selectedMemo_attributedText
}
・UIImagePickerの処理、値の代入は分けるべき
・Realmのインスタンスは、Realmの処理にしか使用しないので、スコープを広げた方がいい
美しいコードに変える
ViewController
override func viewWillAppear(_ animated: Bool) {
super.viewWillAppear(animated)
//初期化
attributedTextArray = [NSAttributedString]()
//データ取得
memoList = realm.objects(MemoModel.self)
//データをNSAttributedStringに変換
if memoList.count > 0 {
for i in 0...memoList.count-1 {
let attributeText = try! NSKeyedUnarchiver.unarchiveTopLevelObjectWithData(memoList![i].data) as! NSAttributedString
attributedTextArray.append(attributeText)
}
}
//tableView更新
memoTableView.reloadData()
}
・初期化&代入、Realmのデータに関する処理、その他、の段落に分けた
・代入、複雑な処理、メソッドの実行、と区別できるように空行を追加
override func prepare(for segue: UIStoryboardSegue, sender: Any?) {
if segue.identifier == "display" {
//DisplayMemoのインスタンスを宣言
let vc = segue.destination as! DisplayMemo
//DisplayMemoのプロパティに代入
vc.selectedMemoObject = memoList[memoTableView.indexPathForSelectedRow!.row]
vc.selectedMemo_attributedText = attributedTextArray[memoTableView.indexPathForSelectedRow!.row]
vc.selectedIndexPathRow = memoTableView.indexPathForSelectedRow?.row
}
}
・一目で何の処理か理解できるが、中の全コードがDisplayMemoに関する処理なので、区別できるようコメントを追加
・代入のコードが連続してるので、=を縦に揃えた
AddMemo
let realm = try! Realm()
let imagePicker = UIImagePickerController()
・Realmは、理由はないけど、AddMemo内で使えるようにした
@IBAction func addMemo(_ sender: Any) {
//必要な変数を宣言
let memoObject = MemoModel()
let archivedAttributedText = try! NSKeyedArchiver.archivedData(withRootObject: memoTextView.attributedText!, requiringSecureCoding: false)
//入力値を代入
memoObject.data = archivedAttributedText
memoObject.identifier = String().randomString()
//データを追加
try! realm.write{
realm.add(memoObject)
}
//ViewControllerに戻る
self.navigationController?.popViewController(animated: true)
}
・宣言、代入、処理、に区別できるように空行を追加
・コメントで段落をつけ、段落の内容と範囲がわかるようにした
・クロージャでまとめるより、このままの方がわかりやすいので、クロージャは辞めた
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)
//圧縮した画像
let resizedImage = pickerImage.resizeImage(withPercentage: 0.1)!
//インスタンスの宣言
let imageAttachment = NSTextAttachment()
var imageAttributedString = NSAttributedString()
//memoTextViewのテキストをAttributedStringに変換
let mutAttrMemoText = NSMutableAttributedString(attributedString: memoTextView.attributedText)
//画像をNSAttributedStringに変換
imageAttachment.image = UIImage(cgImage: resizedImage.cgImage!, scale: scaleRate, orientation: resizedImage.imageOrientation)
imageAttributedString = NSAttributedString(attachment: imageAttachment)
mutAttrMemoText.append(imageAttributedString)
//画像を追加したAttributedStringをmemoTextViewに追加
memoTextView.attributedText = mutAttrMemoText
}
//imagePickerを閉じる
dismiss(animated: true, completion: nil)
}
・宣言や代入が多いので、=を縦に揃えた
・他変数と揃えるため、paddingの型指定を排除
・値の代入、インスタンス&イニシャライザ、他処理、に分類
・各段落の内容を理解できるようにコメントを追加
・全ての変数は宣言時、pickerImage以外に利用する値がないので、順序を変えても大丈夫
DisplayMemo
//ViewControllerから値を受け取る変数群
var selectedMemoObject = MemoModel()
var selectedMemo_attributedText = NSAttributedString()
var selectedIndexPathRow = Int()
・=を縦に揃えた
・初期化による型指定に変更
・コメントで、どのような役割を担うのかを説明
EditMemo
//Realmのインスタンス、Realmのデータを受け取る変数
var memoList:Results<MemoModel>!
let realm = try! Realm()
//imagePickerのインスタンス
let imagePicker = UIImagePickerController()
//DisplayMemoから値を受け取る変数群
var selectedMemoObject = MemoModel()
var selectedMemo_attributedText = NSAttributedString()
var selectedIndexPathRow = Int()
・=を縦に揃えた
・Realmの処理する度にインスタンスを生成するのは面倒なので、スコープをEditMemo全体に広げた
・ジェネリックはRealm指定のを使用しており、classとして定義していないので、初期化の書き方に変更できなかった
override func viewDidLoad() {
super.viewDidLoad()
//imagePickerの設定
imagePicker.delegate = self
imagePicker.sourceType = .photoLibrary
//Realmからデータを取得、
memoList = realm.objects(MemoModel.self)
//memoTextViewにDisplayMemoから渡されたデータを代入
memoTextView.attributedText = selectedMemo_attributedText
}
・処理別に分類
・どのコードも=を縦に揃えることもできたが、処理の内容も担当する役割も全く違うので、区別のために揃えなかった
まとめ
リーダブルコードを読むまで、コードの美しさとは
・少ないコード
・コメントが少ない
・短くてわかりやすい名前
・無駄な改行やスペースがない
を指すのだと思ってました。
ですが、
・=を基準に揃える
・不可欠でもないコメントや空行
・同じ形式のコードに一貫性を持たせる
を試してみて、地味で細かい作業も開発の1つだと気付きました。
オンラインサロンで自分のコードを見てもらう機会がありますし、これからもプログラミングに携わる身として、早いうちからコードを美しくする方法を知れてよかったです。
次回は、コードのロジック、を改善していきます。