이번에 같이 일하는 동료 개발자 분과 함께 리팩토링 2판 - 마틴 파울러 책을 읽어보면서 느낀 점을 공유하는 스터디를 진행하기로 하였습니다. 책은 JS기준으로 쓰여있어서, 틈틈이 swift로 생각해보고, 책 내용을 요약하면서 제 생각도 같이 써 내려가 보려 합니다.
저자는 실제 코드 예시를 가장 앞장인 1장에 배치하여 리팩토링을 이해시키려 합니다.
// plays.json 연극 정보
{
"hamlet": {"name": "Hamlet", "type": "tragedy"},
"as-like": {"name": "As You Like It", "type": "comdy"},
"othello": {"name: Othello", "type": "tragedy"}
}
// invoices.json 공연료 청구서 데이터
[
{
"cusotmer": "BigCo",
"performances": [
{
"playID": "hamlet",
"audience" 55
},
{
"playID": "as-like",
"audience" 35
},
{
"playID": "othello",
"audience" 40
},
]
}
]위는 책에서 이용하는 json데이터입니다
이제 이를 통해 책에 맨처음 공연료 청구서하는 코드를 스위프트로 치환해서 적어보겠습니다. 커맨드 라인툴을 이용했습니다
format쪽은 NumberFormatter과 locale을 사용하면 되지만 이 쪽은 우선 넘어가겠습니다
import Foundation
struct Performance {
let playID: String
let audience: Int
}
struct Play {
let name: String
let type: String
}
struct Invoice {
let customer: String
let performances: [Performance]
}
enum CustomError : Error {
case unknown
}
func statement(invoice: Invoice, plays: [String: Play]) throws -> String {
var totalAmount: Double = 0
var volumeCredits: Double = 0
var result = "청구내역 (고객명 :\(invoice.customer))\n"
for perf in invoice.performances {
let play = plays[perf.playID]
var thisAmount: Double = 0
switch play?.type {
case "tragedy":
thisAmount = 40000
if perf.audience > 30 {
thisAmount += Double(1000 * (perf.audience - 30))
}
case "comedy":
thisAmount = 30000
if perf.audience > 20 {
thisAmount += Double(10000 + 500 * (perf.audience - 20))
}
thisAmount += Double(300 * perf.audience)
default:
throw CustomError.unknown
}
volumeCredits += max(Double(perf.audience) - 30, 0)
if "comedy" == play?.type {
volumeCredits += floor(Double(perf.audience) / 5.0)
}
result += "\(play?.name ?? ""): $\(thisAmount/100) (\(perf.audience)석)\n"
totalAmount += thisAmount
}
result += "총액: $\(totalAmount/100.0)\n"
result += "적립 포인트: $\(volumeCredits)점\n"
return result
}
let plays = [
"hamlet": Play(name: "Hamlet", type: "tragedy"),
"as You Like It": Play(name: "As You Like It", type: "comedy"),
"othello": Play(name: "Othello", type: "tragedy")
]
let invoice = Invoice(customer: "BigCo", performances: [
Performance(playID: "hamlet", audience: 55),
Performance(playID: "as You Like It", audience: 35),
Performance(playID: "othello", audience: 40)
])
print(try statement(invoice: invoice, plays: plays))
매개변수 plays가 dictionary로 되어있어 key로 찾는게 좀 불편하지만.. json파일과 책에서 저렇게 되어있어 우선 이렇게 구현했습니다.
코드를 짜놓고 나니.. 당장은 괜찮겠지만 향후 유지보수가 쉽지 않아 보입니다.. 포인트 계산법이 바뀌거나, 장르가 다양해지거나, 요구사항이 조금더 많아지고 수정사항이 잦아진다면 금방 스파게티 코드가 될 것 같아 보입니다.
리팩토링에 하기 앞서서 무조건 테스트코드를 작성하라고 저자는 말합니다. 그 이유는 사람은 실수를 하기 때문에 리팩토링시 하는 실수를 방지해주기 때문입니다. 당장은 테스트 코드 작성에 시간이 걸릴지 모르지만 전체 작업시간은 오히려 단축됩니다.
테스트 코드를 작성해보겠습니다
import XCTest
class TestRefactoring: XCTestCase {
func test_statement() throws {
let plays = [
"hamlet": Play(name: "Hamlet", type: "tragedy"),
"as You Like It": Play(name: "As You Like It", type: "comedy"),
"othello": Play(name: "Othello", type: "tragedy")
]
let invoice = Invoice(customer: "BigCo", performances: [
Performance(playID: "hamlet", audience: 55),
Performance(playID: "as You Like It", audience: 35),
Performance(playID: "othello", audience: 40)
])
let givenString = "청구내역 (고객명 :BigCo)\nHamlet: $650.0 (55석)\nAs You Like It: $580.0 (35석)\nOthello: $500.0 (40석)\n총액: $1730.0\n적립 포인트: $47.0점\n"
let resultString = try statement(invoice:invoice , plays: plays)
XCTAssertEqual(givenString, resultString)
}
}사실 String format이 바뀔 수 있기 때문에 String 자체를 비교하는 테스트보다 result를 String이 아닌 Custom Type으로 하는게 좋아보입니다. 하지만 우선 리팩토링 책에 따라 순서를 맞춰보겠습니다
좋습니다
statement() 함수 쪼개기
switch문 분리, 책과 달리 swift에는 함수명이아닌 매개변수에 전치사를 붙일 수 있어서 이를 이용하였습니다.
func statement(invoice: Invoice, plays: [String: Play]) throws -> String {
var totalAmount: Double = 0
var volumeCredits: Double = 0
var result = "청구내역 (고객명 :\(invoice.customer))\n"
for perf in invoice.performances {
let play = plays[perf.playID]
let thisAmount = try amount(for: perf, for: play)
volumeCredits += max(Double(perf.audience) - 30, 0)
if "comedy" == play?.type {
volumeCredits += floor(Double(perf.audience) / 5.0)
}
result += "\(play?.name ?? ""): $\(thisAmount/100) (\(perf.audience)석)\n"
totalAmount += thisAmount
}
result += "총액: $\(totalAmount/100.0)\n"
result += "적립 포인트: $\(volumeCredits)점\n"
return result
}
func amount(for perf: Performance, for play: Play?) throws -> Double {
var thisAmount: Double = 0
switch play?.type {
case "tragedy":
thisAmount = 40000
if perf.audience > 30 {
thisAmount += Double(1000 * (perf.audience - 30))
}
case "comedy":
thisAmount = 30000
if perf.audience > 20 {
thisAmount += Double(10000 + 500 * (perf.audience - 20))
}
thisAmount += Double(300 * perf.audience)
default:
throw CustomError.unknown
}
return thisAmount
}이렇게 분리한 후엔 테스트를 바로 실행하여 올바르게 동작하는지 확인해봐야합니다. 테스트가 올바르게 동작한다면 커밋을 하여 중간에 문제가 생길경우 이전으로 돌아갈 수 있게 해줍니다
명확한 네이밍
책에서는 aPerformance라는 네이밍으로 하였지만 swift는 타입을 통해 추론이 가능하기 때문에 관사는 생략했습니다.
thisAmount-> result, perf-> performance
func statement(invoice: Invoice, plays: [String: Play]) throws -> String {
var totalAmount: Double = 0
var volumeCredits: Double = 0
var result = "청구내역 (고객명 :\(invoice.customer))\n"
for performance in invoice.performances {
let play = plays[performance.playID]
let thisAmount = try amount(for: performance, for: play)
volumeCredits += max(Double(performance.audience) - 30, 0)
if "comedy" == play?.type {
volumeCredits += floor(Double(performance.audience) / 5.0)
}
result += "\(play?.name ?? ""): $\(thisAmount/100) (\(performance.audience)석)\n"
totalAmount += thisAmount
}
result += "총액: $\(totalAmount/100.0)\n"
result += "적립 포인트: $\(volumeCredits)점\n"
return result
}
func amount(for performance: Performance, for play: Play?) throws -> Double {
var result: Double = 0
switch play?.type {
case "tragedy":
result = 40000
if performance.audience > 30 {
result += Double(1000 * (performance.audience - 30))
}
case "comedy":
result = 30000
if performance.audience > 20 {
result += Double(10000 + 500 * (performance.audience - 20))
}
result += Double(300 * performance.audience)
default:
throw CustomError.unknown
}
return result
}play 함수 추출
func statement(invoice: Invoice, plays: [String: Play]) throws -> String {
var totalAmount: Double = 0
var volumeCredits: Double = 0
var result = "청구내역 (고객명 :\(invoice.customer))\n"
for performance in invoice.performances {
let thisAmount = try amount(for: performance, for: play(for: performance))
volumeCredits += max(Double(performance.audience) - 30, 0)
if "comedy" == play(for: performance)?.type {
volumeCredits += floor(Double(performance.audience) / 5.0)
}
result += "\(play(for: performance)?.name ?? ""): $\(thisAmount/100) (\(performance.audience)석)\n"
totalAmount += thisAmount
}
result += "총액: $\(totalAmount/100.0)\n"
result += "적립 포인트: $\(volumeCredits)점\n"
return result
}
func amount(for performance: Performance, for play: Play?) throws -> Double {
var result: Double = 0
switch play?.type {
case "tragedy":
result = 40000
if performance.audience > 30 {
result += Double(1000 * (performance.audience - 30))
}
case "comedy":
result = 30000
if performance.audience > 20 {
result += Double(10000 + 500 * (performance.audience - 20))
}
result += Double(300 * performance.audience)
default:
throw CustomError.unknown
}
return result
}
func play(for performance: Performance) -> Play? {
return plays[performance.playID]
}여기가 좀 의아한데, 갑자기 plays를 전역변수로 받아버립니다.. 아마 중첩함수인것 같습니다.
play 매개변수 삭제, thisAmount 인라인
func statement(invoice: Invoice) throws -> String {
var totalAmount: Double = 0
var volumeCredits: Double = 0
var result = "청구내역 (고객명 :\(invoice.customer))\n"
for performance in invoice.performances {
volumeCredits += max(Double(performance.audience) - 30, 0)
if "comedy" == play(for: performance)?.type {
volumeCredits += floor(Double(performance.audience) / 5.0)
}
result += "\(play(for: performance)?.name ?? ""): $\(try amount(for: performance)/100) (\(performance.audience)석)\n"
totalAmount += try amount(for: performance)
}
result += "총액: $\(totalAmount/100.0)\n"
result += "적립 포인트: $\(volumeCredits)점\n"
return result
}
func amount(for performance: Performance) throws -> Double {
var result: Double = 0
switch play(for: performance)?.type {
case "tragedy":
result = 40000
if performance.audience > 30 {
result += Double(1000 * (performance.audience - 30))
}
case "comedy":
result = 30000
if performance.audience > 20 {
result += Double(10000 + 500 * (performance.audience - 20))
}
result += Double(300 * performance.audience)
default:
throw CustomError.unknown
}
return result
}
func play(for performance: Performance) -> Play? {
return plays[performance.playID]
}비록 딕셔너리 참조이긴하지만 plays를 참조하는 횟수가 많아져 성능이 조금 느려졌습니다. 책에서는 성능에 큰 영향이 없다면 가독성이 훨씬 중요하다고 주장합니다. 저도 그렇게 생각합니다.
책에선 지역변수를 없애고, 함수로 바로 넣는걸 인라인이라고 표현합니다.
포인트 계산 코드 추출하기 및 중간 점검
책에서 함수로 분리하면서 기존 매개변수로 받던것들을 전역변수로 바로 참조하여 class로 한번 감싸주었습니다 ( 끝까지 가보니 이는 중첩함수였습니다..)
import Foundation
class Refactoring {
struct Performance {
let playID: String
let audience: Int
}
struct Play {
let name: String
let type: String
}
struct Invoice {
let customer: String
let performances: [Performance]
}
enum CustomError : Error {
case unknown
}
func statement(invoice: Invoice) throws -> String {
var result = "청구내역 (고객명 :\(invoice.customer))\n"
for performance in invoice.performances {
result += "\(play(for: performance)?.name ?? ""): $\(try amount(for: performance)/100) (\(performance.audience)석)\n"
}
result += "총액: $\(try totalAmount() / 100.0)\n"
result += "적립 포인트: $\(totalVolumeCredits())점\n"
return result
}
func totalAmount() throws -> Double {
var result: Double = 0
for performance in invoice.performances {
result += try amount(for: performance)
}
return result
}
func totalVolumeCredits() -> Double {
var result: Double = 0
for performance in invoice.performances {
result += volumeCredits(for: performance)
}
return result
}
func volumeCredits(for performance: Performance) -> Double{
var result: Double = 0
result += max(Double(performance.audience) - 30, 0)
if "comedy" == play(for: performance)?.type {
result += floor(Double(performance.audience) / 5.0)
}
return result
}
func amount(for performance: Performance) throws -> Double {
var result: Double = 0
switch play(for: performance)?.type {
case "tragedy":
result = 40000
if performance.audience > 30 {
result += Double(1000 * (performance.audience - 30))
}
case "comedy":
result = 30000
if performance.audience > 20 {
result += Double(10000 + 500 * (performance.audience - 20))
}
result += Double(300 * performance.audience)
default:
throw CustomError.unknown
}
return result
}
func play(for performance: Performance) -> Play? {
return plays[performance.playID]
}
let plays = [
"hamlet": Play(name: "Hamlet", type: "tragedy"),
"as You Like It": Play(name: "As You Like It", type: "comedy"),
"othello": Play(name: "Othello", type: "tragedy")
]
let invoice = Invoice(customer: "BigCo", performances: [
Performance(playID: "hamlet", audience: 55),
Performance(playID: "as You Like It", audience: 35),
Performance(playID: "othello", audience: 40)
])
}
let refactoring = Refactoring()
print(try refactoring.statement(invoice: refactoring.invoice))
테스트 코드
class TestRefactoring: XCTestCase {
func test_statement() throws {
let refactoring = Refactoring()
let givenString = "청구내역 (고객명 :BigCo)\nHamlet: $650.0 (55석)\nAs You Like It: $580.0 (35석)\nOthello: $500.0 (40석)\n총액: $1730.0\n적립 포인트: $47.0점\n"
let resultString = try refactoring.statement(invoice: refactoring.invoice)
XCTAssertEqual(givenString, resultString)
}
}
코드의 길이가 길어지긴 했지만 가독성이 좋아졌습니다. 기존에 비해 for문이 많아져서 성능상 영향이 있을거라 생각하지만 책의 저자 마틴 파울러는 최신 컴파일러는 각종 최적화기법이 잘 되어있어, 실제 성능은 일단 리팩토링을 하고 느려지면 다시 수정하라고 합니다.. 흐으음..?
무슨 의미인지는 알것 같아 우선 넘어 가겠습니다
계산단계와 포맷팅 단계 분리하기
import Foundation
class Refactoring {
struct Performance {
let playID: String
let audience: Int
var play: Play?
var amount: Double?
var volumeCredits: Double?
}
struct Play {
let name: String
let type: String
}
struct Invoice {
let customer: String
let performances: [Performance]
}
enum CustomError : Error {
case unknown
}
struct StateMentData {
let customer: String
let performances: [Performance]
let totalAmount: Double
let totalVolumeCredits: Double
}
let plays = [
"hamlet": Play(name: "Hamlet", type: "tragedy"),
"as You Like It": Play(name: "As You Like It", type: "comedy"),
"othello": Play(name: "Othello", type: "tragedy")
]
let invoice = Invoice(customer: "BigCo", performances: [
Performance(playID: "hamlet", audience: 55),
Performance(playID: "as You Like It", audience: 35),
Performance(playID: "othello", audience: 40)
])
func createStateMentData(invoice: Invoice, plays: [String: Play]) throws -> StateMentData {
func enrichPerformance(performance: Performance) throws -> Performance {
var result = performance
result.play = play(for: performance)
result.amount = try amount(for: performance)
result.volumeCredits = volumeCredits(for: performance)
return result
}
func play(for performance: Performance) -> Play? {
return plays[performance.playID]
}
func totalAmount() throws -> Double {
var result: Double = 0
for performance in invoice.performances {
result += try amount(for: performance)
}
return result
}
func totalVolumeCredits() -> Double {
var result: Double = 0
for performance in invoice.performances {
result += volumeCredits(for: performance)
}
return result
}
func volumeCredits(for performance: Performance) -> Double{
var result: Double = 0
result += max(Double(performance.audience) - 30, 0)
if "comedy" == play(for: performance)?.type {
result += floor(Double(performance.audience) / 5.0)
}
return result
}
func amount(for performance: Performance) throws -> Double {
var result: Double = 0
switch play(for: performance)?.type {
case "tragedy":
result = 40000
if performance.audience > 30 {
result += Double(1000 * (performance.audience - 30))
}
case "comedy":
result = 30000
if performance.audience > 20 {
result += Double(10000 + 500 * (performance.audience - 20))
}
result += Double(300 * performance.audience)
default:
throw CustomError.unknown
}
return result
}
return StateMentData(customer: invoice.customer, performances: invoice.performances.compactMap { try? enrichPerformance(performance: $0) }, totalAmount: try totalAmount(), totalVolumeCredits: totalVolumeCredits())
}
func statement(invoice: Invoice, plays: [String: Play]) throws -> String {
return try renderPlainText(data: createStateMentData(invoice: invoice, plays: plays))
}
func renderPlainText(data: StateMentData) throws -> String {
var result = "청구내역 (고객명 :\(data.customer))\n"
for performance in data.performances {
result += "\(performance.play?.name ?? ""): $\((performance.amount ?? 0.0) / 100) (\(performance.audience)석)\n"
}
result += "총액: $\(data.totalAmount / 100.0)\n"
result += "적립 포인트: $\(data.totalVolumeCredits)점\n"
return result
}
}
let refactoring = Refactoring()
print(try refactoring.statement(invoice: refactoring.invoice, plays: refactoring.plays))처음과 비교해 코드량이 엄청 길어졌습니다. 다만 이제 로직에 대한 책임이 나눠져서 다른 rendering할 시에도 함수를 쉽게 추가할 수 있게 되었다고합니다..
이제 남은것은 조건부 로직을 다형성으로 바꾸기 입니다.
공연료 계산기 만들기 ( 최종)
에러 던지는것을 그냥 fatalError로 수정하였습니다.
import Foundation
class Refactoring {
class PerformanceCalculator {
let performance: Performance
let play: Play
init(performance: Performance, play: Play) {
self.performance = performance
self.play = play
}
func amount() -> Double {
fatalError("서브 클래스에서 구현되게 설계되었습니다")
}
func volumeCredits() -> Double{
return max(Double(performance.audience) - 30, 0)
}
}
class TragedyCalculator: PerformanceCalculator {
override func amount() -> Double {
var result: Double = 40000
if performance.audience > 30 {
result += Double(1000 * (performance.audience - 30))
}
return result
}
}
class ComedyCalculator: PerformanceCalculator {
override func amount() -> Double {
var result: Double = 30000
if performance.audience > 20 {
result += Double(10000 + 500 * (performance.audience - 20))
}
result += Double(300 * performance.audience)
return result
}
override func volumeCredits() -> Double {
return super.volumeCredits() + floor(Double(performance.audience) / 5.0)
}
}
struct Performance {
let playID: String
let audience: Int
var play: Play?
var amount: Double?
var volumeCredits: Double?
}
struct Play {
let name: String
let type: String
}
struct Invoice {
let customer: String
let performances: [Performance]
}
struct StateMentData {
let customer: String
var performances: [Performance]
var totalAmount: Double = 0
var totalVolumeCredits: Double = 0
}
let plays = [
"hamlet": Play(name: "Hamlet", type: "tragedy"),
"as You Like It": Play(name: "As You Like It", type: "comedy"),
"othello": Play(name: "Othello", type: "tragedy")
]
let invoice = Invoice(customer: "BigCo", performances: [
Performance(playID: "hamlet", audience: 55),
Performance(playID: "as You Like It", audience: 35),
Performance(playID: "othello", audience: 40)
])
func createPerformanceCalculator(performance: Performance, play: Play) -> PerformanceCalculator {
switch play.type {
case "tragedy":
return TragedyCalculator(performance: performance, play: play)
case "comedy":
return ComedyCalculator(performance: performance, play: play)
default:
fatalError(play.type)
}
}
func createStateMentData(invoice: Invoice, plays: [String: Play]) -> StateMentData {
func enrichPerformance(performance: Performance) -> Performance {
let calculator = createPerformanceCalculator(performance: performance, play: play(for: performance) ?? .init(name: "", type: ""))
var result = performance
result.play = calculator.play
result.amount = calculator.amount()
result.volumeCredits = calculator.volumeCredits()
return result
}
func play(for performance: Performance) -> Play? {
return plays[performance.playID]
}
func totalAmount(data: StateMentData) -> Double {
return data.performances.reduce(0) { total, performance in
total + (performance.amount ?? 0.0)
}
}
func totalVolumeCredits(data: StateMentData) -> Double {
return data.performances.reduce(0) { total, performance in
total + (performance.volumeCredits ?? 0.0)
}
}
func volumeCredits(for performance: Performance) -> Double{
var result: Double = 0
result += max(Double(performance.audience) - 30, 0)
if "comedy" == play(for: performance)?.type {
result += floor(Double(performance.audience) / 5.0)
}
return result
}
var result = StateMentData(customer: invoice.customer, performances: invoice.performances.compactMap { enrichPerformance(performance: $0) })
result.performances = invoice.performances.compactMap { enrichPerformance(performance: $0) }
result.totalAmount = totalAmount(data: result)
result.totalVolumeCredits = totalVolumeCredits(data: result)
return result
}
func statement(invoice: Invoice, plays: [String: Play]) -> String {
return renderPlainText(data: createStateMentData(invoice: invoice, plays: plays))
}
func renderPlainText(data: StateMentData) -> String {
var result = "청구내역 (고객명 :\(data.customer))\n"
for performance in data.performances {
result += "\(performance.play?.name ?? ""): $\((performance.amount ?? 0.0) / 100) (\(performance.audience)석)\n"
}
result += "총액: $\(data.totalAmount / 100.0)\n"
result += "적립 포인트: $\(data.totalVolumeCredits)점\n"
return result
}
}
let refactoring = Refactoring()
print(refactoring.statement(invoice: refactoring.invoice, plays: refactoring.plays))
테스트 코드
import XCTest
class TestRefactoring: XCTestCase {
func test_statement() throws {
let refactoring = Refactoring()
let givenString = "청구내역 (고객명 :BigCo)\nHamlet: $650.0 (55석)\nAs You Like It: $580.0 (35석)\nOthello: $500.0 (40석)\n총액: $1730.0\n적립 포인트: $47.0점\n"
let resultString = refactoring.statement(invoice: refactoring.invoice, plays: refactoring.plays)
XCTAssertEqual(givenString, resultString)
}
}
결론:
아무래도 JS코드에 맞춰가다 보니 코드자체가 swift스럽지 못한 부분도 있지만 마틴 파울러가 무엇을 말하려는지는 알 수 있었습니다. 몇몇 프로젝트와 실무를 하면서 경험적으로 머리속에 정돈되었던 지식과, 책에서 나타나는 모습이 얼추 맞아떨어지는 모습을 보면서 그래도 잘못 공부하진 않았구나 생각하면서 1장을 읽었습니다. 책에서 계속 강조하는 말은 단순 코드변경아닌, 그 사이에 잘게잘게 테스트코드와 커밋을 계속 해주는 "흐름"을 중요하게 보라고합니다 이렇게 해야 코드가 절대 깨지지 않으며, 결과적으로 더 빨리 처리할 수 있다고 합니다
좋은 코드를 가늠하는 확실한 방법은 '얼마나 수정하기 쉬운가'다.
1장에서 인상깊었던 구절로 글을 마무리 하겠습니다.
'Programming' 카테고리의 다른 글
| Sort에 관한 고찰 (2) | 2021.03.15 |
|---|
