[WIP] 中3の頃のコードを高2目線でリファクタリングする
はじめに
この記事はLife is Tech！Members Advent Calendar 2020の3日目の記事です！3日目は僕(Twitter)が担当させていただきます！！
普段はiOS Swiftだったり、React, ReactNativeを書いてたりします。
1日目, 2日目の記事のクオリティの高さにビビり散らかしてるのですが頑張っていこうと思います！
ちなみにこれは余談ですがわくわくさんとノリで始めたこのアドベントカレンダー、残り枠が13日分ほどありこのままだとわくわくさんが合計14記事書く(理不尽)ことになってしまうのでLiT! Memberの方は時間があればどんどん参加して欲しかったりします（👀👀）
今回はタイトル通り、中学3年生のころの僕が作ったカラーピッカーアプリを高校2年生になって少し成長した僕が今の目線でリファクタリング、レビューしていこうと思います。但し、あえてライブラリの導入だったり元の実装を大幅に壊すことはせずにいきたいと思います！ただ、「このアプリを1から実装するならこういう実装にする」みたいな考えがあったりするのでそちらも最後の方に載せようと思います！！また、Swift前提での話にはなるのでSwiftまたはSwiftと似た言語であることが前提のレビューが多いですがプログラミング言語全体について言える命名等にも触れているので興味があればご一読ください！！
まだまだげきよわエンジニアなので生暖かい目で見守っていただけると幸いです☀️☀️☀️☀️
どんなアプリを作るのか
冒頭でも述べたとおり、今回作るアプリはカラーピッカーアプリです。
Red, Green, Blueの光の三原色のスライダーを動かすとそれぞれの比率に応じて該当する色とその補色、反転色が表示されるようなアプリになっています。
中3の時のコード
こちらが今回リファクタリングしていく中3のころのコードなのですが、なかなかに悲しい気持ちになるものですね.............ツッコミどころがめちゃめちゃ多いです。中3なら耐えるか(知らんけど
ただ、細かいエラーハンドリングはしっかりしているのでそこはすごく偉いなと思います
//
// ViewController.swift
// ColorCode
//
// Created by Ren Matsushita on 2019/01/25.
// Copyright © 2019 Ren Matsushita. All rights reserved.
//
import UIKit
class ViewController: UIViewController, UITextFieldDelegate {
var redSlider = UISlider()
var greenSlider = UISlider()
var blueSlider = UISlider()
var redValue: Int = 255
var greenValue: Int = 255
var blueValue: Int = 255
var textfield = UITextField()
var colorLabel = UILabel()
var reverseColorLabel = UILabel()
var comColorLabel = UILabel()
let pasteboard: UIPasteboard = UIPasteboard.general
let screenHeight = UIScreen.main.bounds.height
let screenWidth = UIScreen.main.bounds.width
override func viewDidLoad() {
super.viewDidLoad()
createView()
}
@objc func moveReverseColor() {
guard let reverseColorHex = reverseColorLabel.text else { return }
makeColor(reverseColorHex)
}
@objc func moveComColor() {
guard let comColorHex = comColorLabel.text else { return }
makeColor(comColorHex)
}
@objc func copyColor() {
pasteboard.string = colorLabel.text
let alert = UIAlertController(title: "Done", message: "背景色をコピーしました!!", preferredStyle: .alert)
alert.addAction(
UIAlertAction(title: "OK",
style: .default,
handler: { action in })
)
present(alert, animated: true, completion: nil)
}
@objc func copyComColor() {
pasteboard.string = reverseColorLabel.text
let alert = UIAlertController(title: "Done", message: "反転色をコピーしました!!", preferredStyle: .alert)
alert.addAction(
UIAlertAction(title: "OK",
style: .default,
handler: { action in })
)
present(alert, animated: true, completion: nil)
}
@objc func copyReverseColor() {
pasteboard.string = comColorLabel.text
let alert = UIAlertController(title: "Done", message: "補色をコピーしました!!", preferredStyle: .alert)
alert.addAction(
UIAlertAction(title: "OK",
style: .default,
handler: { action in })
)
present(alert, animated: true, completion: nil)
}
@objc func redSliderChanged(_ sender: UISlider) {
redValue = Int(sender.value)
toHex()
}
@objc func greenSliderChanged(_ sender: UISlider) {
greenValue = Int(sender.value)
toHex()
}
@objc func blueSliderChanged(_ sender: UISlider) {
blueValue = Int(sender.value)
toHex()
}
func toHex() {
let color = UIColor.rgba(red: redValue, green: greenValue, blue: blueValue, alpha: 1)
let invertedColor = UIColor.rgba(red: (255-redValue), green: (255-greenValue), blue: (255-blueValue), alpha: 1)
self.view.backgroundColor = color
colorLabel.textColor = color
let wh = UIColor.white
let bl = UIColor.black
if redValue + greenValue + blueValue <= 382 {
textfield.textColor = wh
colorLabel.textColor = wh
reverseColorLabel.textColor = bl
comColorLabel.textColor = wh
textfield.layer.borderColor = wh.cgColor
} else if redValue + greenValue + blueValue >= 382 {
textfield.textColor = bl
colorLabel.textColor = bl
reverseColorLabel.textColor = wh
comColorLabel.textColor = bl
textfield.layer.borderColor = bl.cgColor
}
let com = max(redValue, greenValue, blueValue) + min(redValue, greenValue, blueValue)
reverseColorLabel.backgroundColor = invertedColor
comColorLabel.backgroundColor = UIColor.rgba(red: (com - redValue), green: (com - greenValue), blue: (com - blueValue), alpha: 1)
colorLabel.text = hexValue(red: redValue, green: greenValue, blue: blueValue)
textfield.text! = hexValue(red: redValue, green: greenValue, blue: blueValue)
reverseColorLabel.text = hexValue(red: (255-redValue), green: (255-greenValue), blue: (255-blueValue))
comColorLabel.text = hexValue(red: (com - redValue), green: (com - greenValue), blue: (com - blueValue))
}
func hexValue(red: Int, green: Int, blue: Int) -> String {
var r: String = String(red, radix: 16)
if red < 16 {
r = "0" + r
}
r = r.uppercased()
var g: String = String(green, radix: 16)
if green < 16 {
g = "0" + g
}
g = g.uppercased()
var b: String = String(blue, radix: 16)
if blue < 16 {
b = "0" + b
}
b = b.uppercased()
return r + g + b
}
func makeColor(_ hex: String) {
let rgbArray = hex.splitInto(2)
redValue = Int(rgbArray[0], radix: 16)!
greenValue = Int(rgbArray[1], radix: 16)!
blueValue = Int(rgbArray[2], radix: 16)!
redSlider.value = Float(redValue)
greenSlider.value = Float(greenValue)
blueSlider.value = Float(blueValue)
toHex()
}
func textFieldShouldReturn(_ textField: UITextField) -> Bool {
if textfield.text!.count != 6 {
textfield.text! = colorLabel.text!
let alert = UIAlertController(title: "Error", message: "6文字で入力してください", preferredStyle: .alert)
alert.addAction(
UIAlertAction(title: "OK",
style: .default,
handler: { action in })
)
present(alert, animated: true, completion: nil)
return true
} else {
var result = 0
for i in 0...5 {
result += Judgment(index: i)
}
if result != 0 {
textfield.text! = colorLabel.text!
let alert = UIAlertController(title: "Error", message: "その文字を使うことはできません。", preferredStyle: .alert)
alert.addAction(
UIAlertAction(title: "OK",
style: .default,
handler: { action in })
)
present(alert, animated: true, completion: nil)
return false
}
}
var rgbArray = textfield.text!.splitInto(2)
redValue = Int(rgbArray[0], radix: 16)!
greenValue = Int(rgbArray[1], radix: 16)!
blueValue = Int(rgbArray[2], radix: 16)!
redSlider.value = Float(redValue)
greenSlider.value = Float(greenValue)
blueSlider.value = Float(blueValue)
toHex()
textfield.resignFirstResponder()
return true
}
func Judgment(index: Int) -> Int {
let text = textfield.text!
let goot = ["0","1","2","3","4","5","6","7","8","9","a","b","c","d","e","f","A","B","C","D","E","F"]
let textArray = text.splitInto(1)
var result = 0
var y = 0
for i in 0..<goot.count {
if !(textArray[index].contains(goot[i])) {
y += 1
if y >= 22 {
result += 1
}
}
}
return result
}
func createView() {
textfield.font = UIFont.systemFont(ofSize: screenWidth/16.3043478)
colorLabel.font = UIFont.systemFont(ofSize: screenWidth/4.6875)
reverseColorLabel.font = UIFont.systemFont(ofSize: screenWidth/7.5)
comColorLabel.font = UIFont.systemFont(ofSize: screenWidth/7.5)
if screenWidth > 414 {
textfield.font = UIFont.systemFont(ofSize: (screenWidth/16.3043478)-7)
colorLabel.font = UIFont.systemFont(ofSize: (screenWidth/4.6875)-15)
reverseColorLabel.font = UIFont.systemFont(ofSize: (screenWidth/7.5)-5)
comColorLabel.font = UIFont.systemFont(ofSize: (screenWidth/7.5)-5)
}
colorLabel.frame = CGRect(x: screenWidth/16.3043478, y: screenHeight/9.44186047, width: screenWidth/1.13981763, height: screenHeight/9.12359551)
colorLabel.text = "FFFFFF"
colorLabel.textColor = UIColor.black
colorLabel.font = UIFont.systemFont(ofSize: screenWidth/4.6875)
colorLabel.textAlignment = .center
colorLabel.isUserInteractionEnabled = true
colorLabel.tag = 1
let longtapColorLabel = UILongPressGestureRecognizer()
longtapColorLabel.addTarget(self, action: #selector(self.copyColor))
longtapColorLabel.minimumPressDuration = 0.5
colorLabel.addGestureRecognizer(longtapColorLabel)
self.view.addSubview(colorLabel)
textfield.frame = CGRect(x: screenWidth/2.88461538, y: screenHeight/4.27368421, width: screenWidth/3.28947368, height: screenHeight/27.06666666)
textfield.borderStyle = .line
textfield.textAlignment = .center
textfield.delegate = self
textfield.text! = "FFFFFF"
textfield.clipsToBounds = true
textfield.keyboardType = .namePhonePad
self.view.addSubview(textfield)
reverseColorLabel.frame = CGRect(x: screenWidth/7.8125, y: screenHeight/2.75254237, width: screenWidth/1.34892086, height: screenHeight/11.2777777778)
reverseColorLabel.text = "000000"
reverseColorLabel.textColor = UIColor.white
reverseColorLabel.textAlignment = .center
reverseColorLabel.backgroundColor = UIColor.black
reverseColorLabel.layer.cornerRadius = 20
reverseColorLabel.clipsToBounds = true
reverseColorLabel.isUserInteractionEnabled = true
reverseColorLabel.tag = 1
let tapReverseColorLabel = UITapGestureRecognizer()
tapReverseColorLabel.addTarget(self, action: #selector(self.moveReverseColor))
reverseColorLabel.addGestureRecognizer(tapReverseColorLabel)
let longtapReverseColorLabel = UILongPressGestureRecognizer()
longtapReverseColorLabel.addTarget(self, action: #selector(self.copyComColor))
longtapReverseColorLabel.minimumPressDuration = 0.5
reverseColorLabel.addGestureRecognizer(longtapReverseColorLabel)
self.view.addSubview(reverseColorLabel)
comColorLabel.frame = CGRect(x:screenWidth/7.8125, y: screenHeight/2.00493827, width: screenWidth/1.34892086, height: screenHeight/11.27777777778)
comColorLabel.text = "FFFFFF"
comColorLabel.textColor = UIColor.black
comColorLabel.textAlignment = .center
comColorLabel.backgroundColor = UIColor.white
comColorLabel.layer.cornerRadius = 20
comColorLabel.clipsToBounds = true
comColorLabel.isUserInteractionEnabled = true
comColorLabel.tag = 1
let tapComColorLabel = UITapGestureRecognizer()
tapComColorLabel.addTarget(self, action: #selector(self.moveComColor))
comColorLabel.addGestureRecognizer(tapComColorLabel)
let longtapComColorLabel = UILongPressGestureRecognizer()
longtapComColorLabel.addTarget(self, action: #selector(self.copyReverseColor))
longtapComColorLabel.minimumPressDuration = 0.5
comColorLabel.addGestureRecognizer(longtapComColorLabel)
self.view.addSubview(comColorLabel)
redSlider.frame = CGRect(x: screenWidth/11.71875, y: screenHeight/1.38803419, width: screenWidth/1.20967742, height: 29)
redSlider.minimumValue = 0
redSlider.maximumValue = 255
redSlider.value = 255
redSlider.tintColor = UIColor.rgba(red: 255, green: 0, blue: 0, alpha: 1)
redSlider.isUserInteractionEnabled = true
redSlider.addTarget(self, action: #selector(self.redSliderChanged(_:)), for: UIControl.Event.valueChanged)
self.view.addSubview(redSlider)
greenSlider.frame = CGRect(x: screenWidth/11.71875, y: screenHeight/1.24923077, width: screenWidth/1.20967742, height: 29)
greenSlider.minimumValue = 0
greenSlider.maximumValue = 255
greenSlider.value = 255
greenSlider.tintColor = UIColor.rgba(red: 0, green: 255, blue: 0, alpha: 1)
greenSlider.isUserInteractionEnabled = true
greenSlider.addTarget(self, action: #selector(self.greenSliderChanged(_:)), for: UIControl.Event.valueChanged)
self.view.addSubview(greenSlider)
blueSlider.frame = CGRect(x: screenWidth/11.71875, y: screenHeight/1.13566434, width: screenWidth/1.20967742, height: 29)
blueSlider.minimumValue = 0
blueSlider.maximumValue = 255
blueSlider.value = 255
blueSlider.tintColor = UIColor.rgba(red: 0, green: 0, blue: 255, alpha: 1)
blueSlider.addTarget(self, action: #selector(self.blueSliderChanged(_:)), for: UIControl.Event.valueChanged)
self.view.addSubview(blueSlider)
}
}
extension String {
func splitInto(_ length: Int) -> [String] {
var str = self
for i in 0 ..< (str.count - 1) / max(length, 1) {
str.insert(",", at: str.index(str.startIndex, offsetBy: (i + 1) * max(length, 1) + i))
}
return str.components(separatedBy: ",")
}
}
extension UIColor {
class func rgba(red: Int, green: Int, blue: Int, alpha: CGFloat) -> UIColor{
return UIColor(red: CGFloat(red) / 255.0, green: CGFloat(green) / 255.0, blue: CGFloat(blue) / 255.0, alpha: alpha)
}
}
問題点
func createView()に100行使っててウケる
まあそれは置いといて具体的に問題点をnつのセクションに分けて話していきたいと思います。
命名が分かりづらい
これはプログラミングで初心者の方が一番やってしまいがちといっても過言ではないミスだと思うのですが、全体的に変数名、関数名の命名規則に一貫性がないです。一般的に変数名、関数名の付け方が特殊なだけであればこの規模のプロジェクトだと、1人でフロントエンドを書くことが多いので可読性の面で特に問題になることはありません。(もちろん決していいコードではない)しかし、今回のコードの場合は筆者の僕本人ですら書いてから2年経った今読むのにかなり苦労しています。実際に2度目に読んだのが3ヶ月でも同じように読むのに苦労していたと思います。ここからは実際にこのコードの変数名、関数名で分かりにくい部分や一般的にアンチパターンとされる部分を説明していきたいと思います！！
省略された変数名
よくない部分
var comColorLabel = UILabel()
どうしてよくないのか
まずはこの部分、comColorLabelという命名の変数がありますが、皆さんはパッとこの変数名を見てこのアプリの仕様からこの変数の役割を推測することはできますか？多くの方はできないかなと思われます。
何故推測できないかというとこの変数名のcomの部分が省略されているからです。これは本来はcomplementary(Colorと合わせると英語で補色の意)という単語なのですが当時の僕は長い変数名をタイピングするのがめんどくさかったのかcomに省略してしまっています。
「省略して何がいけないの？タイピングする範囲が狭くなって効率的じゃね？」と思った方もいらっしゃるかもしれません。Golangなんかは「推測が容易にできる範囲でその単語であると確定できる一番短い変数名をつける」みたいな慣習があるらしいですし、確かに略しても意味が伝わるのであれば僕も変数の省略はしてもいいと思っています。
ただ、今回のコードの場合だとcomという略された後の文字から推測できる単語がたくさん出てきてしまいます。英語が苦手な僕でもcompany, complete, compact等々の単語が推測できてしまいます。これでは推測ができていても略語から本来の単語が確定できません。他の人が見たら辛そうですね.....
どうすればよかったのか
今回の場合だと、今の僕ならcomplementaryColorLabelと少し長くなりますが単語をフルに使うと思います。先ほども言ったとおり僕は命名の省略について否定的ではないのですが今回の場合だとcomplementary colorという単語が「補色」という意味になることが一般的ではないので次に見た人が検索しても引っかかるような命名が適切かと思われます。逆に、有名だったりコーディングでよく使用される単語(info, id, auth)等であれば省略しても意味が伝わるので大丈夫かなと思います。
これは今回のコードからは少し外れるのですが、命名で一番よくないのはlabel1, label2のように意味を持たない数字(マジックナンバーと呼ばれたりする）を使うことで、今回のコードだとまだ個々のlabelが違う物であることが伝わるので最低限のラインは担保できているのですが、マジックナンバーを使用してしまうとコードを書いている張本人でも本気で何が何かわからなくなってしまうのでやめましょう()
本気で2つの命名に意味の差がない場合、フロントエンドだとtopLabelやbottomLabel等表示する場所で変数名を変えるだけでも相当分かり易くなると思います。
強制アンラップの多用
// TODO: description
Viewの生成部分のコードが非直感的
// TODO: description
FatViewController
// TODO: description
高2の僕ならこんな感じで書く
// TODO: description
ついでに
僕が今からこのアプリを実装するならこんなコードを書きます。(リポジトリ自体は1年ほど前のものですが割と物がいいのでそのまま採用)
GitHub
RxSwift＋MVVMの1年前にめちゃめちゃ流行っていた構成になっています。viewから完全にロジックを剥がして、viewModel以降の層に丸投げできているので割といいコードかなと個人的には思っています！
おわりに
最後までお読みいただきありがとうございます。
今回は中学2年生のころのコードをレビューするというなかなか普段しないことをしてみました！
実際にこの記事を書いてみて気づいたことですが、過去の自分の実力と向き合いながら今の自分の実力でアウトプットをすることで普段自分が意識せず考えていることだったり、過去の自分と比べての成長を感じることができていい機会になりました。皆さんも是非、2,3年前の自分のコードをレビューするということをしてみてはいかがでしょうか！！！！！
Life is Tech！Members Advent Calendar 2020の3日目、れんがお送りしました！！続いてはげきつよiOS SwiftエンジニアのまさしがSwiftについて書いてくれるようです！！楽しみだ....
Discussion