은행 창구 매니저 [STEP 3] 아리, 허황 #128

leeari95 · 2021-12-28T07:09:52Z

안녕하세요 델마 @delmaSong 🙌🏻
아리 @leeari95 , 허황 @hwangjeha입니다.
은행 창구 매니저 프로젝트 STEP 3 구현 완료해서 PR 보냅니다.
저번에 와주셔서 직접 코드리뷰해주셔서 덕분에 많은 도움이 되었어요. 감사했습니다! 😆
궁금했던 점, 조언이 필요한 부분 이 외에 저희가 놓친 부분이나 부족한 부분이 있다면 번거롭더라도 한번 더 짚어주시면 감사하겠습니다.🙏🏻

고민했던 점

1. 다중 처리

여러 은행원이 동시에 고객의 일을 처리하는 로직을 고민해봤습니다.

global() 큐에 은행원의 수 만큼 task를 만들고 각각의 task에서 고객을 dequeue해서 처리하는 방식
하나의 while 구문에서 dequeue 해주고 고객의 은행 업무에 따라 예금 고객은 DispatchQueue.global()에 세마포어를 이용해 은행원 수 == 접근 가능한 스레드 수 만드는 방식
- DispatchSemaphore의 value를 1이 아닌 2나 3으로 줄 경우 Race Condition이 발생할 수 있다는 가능성이 존재하기 때문에 해당 방법은 적절하지 못하다는 생각이 들었습니다.
- Banker가 가지고 있는 work() 메소드는 현재 공유자원에 접근하고 있지 않지만, 추후 해당 메소드가 공유자원에 접근하지 않을거라는 보장이 없다라는 생각이 들었습니다.

추후 은행원의 수가 변경되더라도 대응할 수 있도록 은행원 수 만큼 DispatchQueue.global()에 task를 만드는 1번 방식으로 구현했습니다.
- 은행원 수 == DispatchQueue의 수

2. 대기번호 오름차순으로 업무를 할 수 있도록 처리

프로젝트 실행예시에는 대기번호 순으로 실행되고 있지 않은 점을 발견하게 되었습니다.
- 이 부분을 야곰에게 질문해보았고, 순차적으로 업무를 할 수 있도록 구현해보라고 하셔서 고민하게 되었습니다.
저희는 대기번호 순으로 차례대로 업무가 처리되야 된다고 생각했기 때문에 대출업무와 예금업무를 보는 고객들을 두개의 고객큐로 나누어 관리하도록 구현했습니다.

3. CustomStringConvertible을 사용하지 않고 연산프로퍼티를 사용

Banking의 경우 rawValue를 String으로 가지고 있는 형태인데, 이 부분을 은행원이 어떤 업무를 처리했는지 알려주기 위해 사용하려는 의도로 구현하게 되었습니다. 프로토콜 채택과 연산프로퍼티 중에 어떤 방식으로 활용할 지 고민해보았습니다.
- CustomStringConvertible의 경우 인스턴스를 원하는 형태의 문자열로 반환하고 싶을 때 채택하여 사용하는 프로토콜로 저희가 활용하고자 하였던 부분이랑은 적합하지 않다는 생각이 들었습니다.
- 따라서 description이라는 연산프로퍼티를 활용하여 rawValue를 반환하도록 구현해주었습니다.

궁금했던 점 / 조언이 필요한 부분

들여쓰기 vs 함수 호출 단계

Bank 타입 workBanker() 메소드에서 들여쓰기가 가독성을 해치는것 같아 따로 Dispatch.global().async 클로져 부분을 따로 함수로 분리해주면 어떨까라는 고민을 했습니다.

현재 코드 구조는 openBank()내부에서 workBankers()를 호출하고 workBankers()내부에서 workBanker()를 호출하는 3단계의 구조를 가지고 있어 함수를 또 분리할 경우 오히려 가독성을 해치는게 아닌가 라는 고민을 했습니다.

// 현재 코드
private func workBanker(customers: Queue<Customer>, group: DispatchGroup) {
    let banker = Banker()
    DispatchQueue.global().async(group: group) {
        while customers.isEmpty == false {
            guard let customer = customers.dequeue() else {
                fatalError()
            }
            self.numberOfCustomers += 1
            banker.work(for: customer)
        }
    }
}

// 들여쓰기 개선하려고 별도의 함수로 분리
// 함수를 분리할 경우 함수 호출 구조가 4단계가 됨.
private func workBanker(customers: Queue<Customer>, group: DispatchGroup) {
    DispatchQueue.global().async(group: group) {
        self.createBankerTask(customers: customers)
    }
}
    
private func createBankerTask(customers: Queue<Customer>) {
    let banker = Banker()
        
    while customers.isEmpty == false {
        guard let customer = customers.dequeue() else {
            fatalError()
        }
        self.numberOfCustomers += 1
        banker.work(for: customer)
    }
}

해결이 되지 않은 점

구조체 프로퍼티는 클로저 내부에서 변경할 수 없음

// 간단한 코드 예시
struct SomeStruct {
    var num = 0
    
    private mutating func test() {
        let closure = { // Escaping closure captures mutating 'self' parameter
            self.num += 1
        }
        closure()
    }
}

Escaping closure의 경우 구조체에서는 캡쳐가 불가능하기 때문에, 프로퍼티를 변경하려고 하면 위와 같은 에러가 발생하게 됩니다.

class와 같은 참조 타입이 아닌 Struct, enum과 같은 값타입에서는 mutating reference의 캡쳐를 허용하지 않기 때문에 self 사용이 불가능 하다.
저희가 Bank가 구조체였을 때 은행원이 일을 할 때, 처리한 고객의 수를 Dispatch.global().async 클로저 내에서 카운트(numberOfCustomers) 해주도록 하려고 했으나 위와 같은 에러가 발생하여 구조체일 때 개선해보고자 하였지만 방법을 찾지 못해서 결국엔 클래스로 구현을 해주었는데요.
이러한 상황일 경우 클래스 말고 다른 방법으로 개선할 수 있는 방법은 없을까요?

이번 피드백도 잘 부탁드립니다! 😆 🙌🏻

- 랜덤한 케이스를 반환하는 타입 메소드 random() 구현 - 케이스 별 업무 시간을 반환하는 연산 프로퍼티 speed 구현

- 은행원의 타입이름 BankClerk를 Banker로 수정 - 프로퍼티로 가지고 있던 workSpeed를 메소드로 수정 - 메소드로 Banking 타입을 전달받고 전달받은 case에 따라 speed를 반환하도록 구현 - 더이상 참조가 필요하지 않으므로 구조체로 수정

- 원시값을 반환하는 연산 프로퍼티 description 구현

- 은행 업무에 따라 고객대기열을 분리하여 생성 - 은행원의 수를 초기화 시 설정할 수 있도록 이니셜라이저 전반적으로 수정 - 대기표를 나눠줄 때 업무에 맞는 큐에 enqueue 될 수 있도록 수정 - 은행을 open 했을 때 대출과 예금업무 동시에 처리되도록 구현 - 은행원의 수 만큼 일을 나눠줄 수 있도록 workBankers 메소드 추가 - 은행원이 전달받은 고객의 대기열이 없어질 때까지 일을 하는 workBanker 메소드 추가 - 은행이 문을 닫을 때 numberOfCustomers에 0을 대입하도록 수정

- continue -> fatalError()

delmaSong

수고하셨습니다!
개선 사항과 질문주신 사항에 대한 답변은 코멘트로 남겨두었어요!
두분이 워낙 잘 구현해주셔서 추가적인 질문 드린 게 있는데 그건 스텝4 이후에 고민해보시면 좋을 것 같아요 😉

delmaSong · 2021-12-28T15:10:56Z

BankManagerConsoleApp/BankManagerConsoleApp/Model/Bank.swift

+ private let loanBankersNumber: Int
+ private let depositBankersNumber: Int


단순히 숫자라고 하면 무슨 숫자인지 애매한 부분이 있어서 갯수를 나타내는 count는 어떨까요?

delmaSong · 2021-12-28T15:13:05Z

BankManagerConsoleApp/BankManagerConsoleApp/Model/Bank.swift

+ init(loanBankersCount: Int = 1, depositBankersCount: Int = 2) {
+ self.loanBankersNumber = loanBankersCount
+ self.depositBankersNumber = depositBankersCount


여기에는 count를 사용하셨네요 ㅎㅎㅎ

안그래도 이부분 고민했던 부분인데 실수가 있었네요.
짚어주셔서 감사합니다! 반영하여 수정해보았습니다! 😁

delmaSong · 2021-12-28T15:15:20Z

BankManagerConsoleApp/BankManagerConsoleApp/Model/Bank.swift

+ while customers.isEmpty == false {
+ guard let customer = customers.dequeue() else {
+ fatalError()
+ }
+ self.numberOfCustomers += 1


저희가 Bank가 구조체였을 때 은행원이 일을 할 때, 처리한 고객의 수를 Dispatch.global().async 클로저 내에서 카운트(numberOfCustomers) 해주도록 하려고 했으나 위와 같은 에러가 발생하여 구조체일 때 개선해보고자 하였지만 방법을 찾지 못해서 결국엔 클래스로 구현을 해주었는데요.

Suggested change

while customers.isEmpty == false {

guard let customer = customers.dequeue() else {

fatalError()

}

self.numberOfCustomers += 1

while let customer = customers.dequeue() {

매번 numberOfCustomers의 숫자를 갱신해주려는 이유가 중간에 fatalError()로 종료될 상황을 고려해 정확한 작업 횟수(고객 수)를 파악하기 위함인가요?
조건을 위처럼 수정한다면 customers.count만큼 수행되므로(사실 수정 전도 마찬가지지만) 매번 numberOfCustomers 를 갱신해주지 않아도 될 것 같아요. handOutWaitingNumber(from:) 메서드에서 전달받은 숫자 그대로를 numberOfCustomers 사용해도 될 것 같은데 어떻게 생각하시나요?
구조체를 그대로 사용하고자 한다면 이런식으로 내부 코드를 수정하는 방법이 있을 수 있을 것 같아요.

handOutWaitingNumber(from:) 메서드에서 전달받은 숫자 그대로를 numberOfCustomers 넣어주는 방식으로 수정하고 Bank 타입을 클래스에서 구조체로 변경했습니다😊 좋은 의견 감사합니다😁

delmaSong · 2021-12-28T15:33:56Z

BankManagerConsoleApp/BankManagerConsoleApp/Model/Banker.swift

+ private func workSpeed(_ banking: Banking) -> TimeInterval {
+ return banking.speed
+ }
+
+ func work(for customer: Customer) {
+ let banking = customer.banking
+ print("\(customer.waitingNumber)번 고객 \(banking.description)업무 시작")
+ Thread.sleep(forTimeInterval: workSpeed(customer.banking))


그대로 banking.speed를 쓰면 되지 않나요?
메서드가 별도로 있는 이유를 잘 모르겠습니다.

처음 workSpeed() 메서드를 구현하고 나중에 코드를 수정하면서 Banking 열거형 타입에 speed를 추가만 해주고 사용하지 않고 있었습니다.
banking.speed를 사용하도록 수정했습니다😊

delmaSong · 2021-12-28T15:34:48Z

BankManagerConsoleApp/BankManagerConsoleApp/Model/Bank.swift

+ workBankers(loanBankersNumber, customers: loanCustomerQueue, group: bankGroup)
+ workBankers(depositBankersNumber, customers: depositCustomerQueue, group: bankGroup)
+


🙌🏼잘 구현하셔서 추가적으로 드리는 질문이라 이건 스텝4까지 다 진행한 후에 고민해보세요

대기번호가 오름차순으로 실행되도록 잘 구현해주셨는데요,
지금은 무조건 대출 업무부터 수행이되는데 1번 손님부터 업무가 처리되도록 하려면 어떻게 해야할까요? 🤔

오... 디테일적인 부분이네요. 이 부분은 STEP 4 구현을 마치고 나서 다시한번 고민해보겠습니다.
좋은 의견 감사합니다! 🙇🏻‍♀️

delmaSong · 2021-12-28T15:39:54Z

BankManagerConsoleApp/BankManagerConsoleApp/Model/Customer.swift

+ static var random: Banking {
+ return .allCases.randomElement() ?? .loan
+ }
+
+ var speed: TimeInterval {
+ switch self {
+ case .loan:
+ return 1.1
+ case .deposit:
+ return 0.7
+ }
+ }
+
+ var description: String {
+ return self.rawValue
+ }
+}
+
 struct Customer {
+ let banking: Banking
 let waitingNumber: Int
+
+ init(banking: Banking = .random, waitingNumber: Int) {
+ self.banking = banking
+ self.waitingNumber = waitingNumber


Banking이라는 업무가 random이라는 프로퍼티가 있어서 스스로 무엇인지 정하기보다는
고객이 자기가 보려는 업무를 스스로 정하는 게 좀 더 적절하지 않을까 싶어요

저희 생각은 Banking 타입 스스로 random한 케이스를 만들어야한다고 생각했습니다.
고객은 Banking 타입이 반환하는 랜덤한 케이스를 이용할 뿐이지
고객이 랜던함 케이스를 만드는건 Banking 타입의 일을 고객이 대신한다고 생각합니다.
이에 대한 의견에 델마의 생각은 어떠신가 궁금합니다🧐

고객이 예금이나 대출이라는 자신의 볼일이 있어서 은행에 방문하는 거 아닐까요? 그렇게 되면 고객단에서 랜덤하게 어떤 일을 할지 생성해서 은행에 찾아가는 방식이 적절해보여서요!

delmaSong · 2021-12-28T16:24:48Z

BankManagerConsoleApp/BankManagerConsoleApp/Model/Bank.swift

+ private func workBanker(customers: Queue<Customer>, group: DispatchGroup) {
+ let banker = Banker()
+ DispatchQueue.global().async(group: group) {
+ while customers.isEmpty == false {
+ guard let customer = customers.dequeue() else {
+ fatalError()
+ }
+ self.numberOfCustomers += 1
+ banker.work(for: customer)
+ }
+ }
+ }


Suggested change

private func workBanker(customers: Queue<Customer>, group: DispatchGroup) {

let banker = Banker()

DispatchQueue.global().async(group: group) {

while customers.isEmpty == false {

guard let customer = customers.dequeue() else {

fatalError()

}

self.numberOfCustomers += 1

banker.work(for: customer)

}

}

}

private func workBanker(customers: Queue<Customer>, group: DispatchGroup) {

let banker = Banker()

DispatchQueue.global().async(group: group) {

while let customer = customers.dequeue() {

banker.work(for: customer)

}

}

}

들여쓰기 vs 가독성
관련해서는 위처럼 코드를 개선하는 방향으로도 수정할 수 있을 것 같아요!
메서드가 가능한 하나의 일만 하도록 잘 쪼개져 있는지 판단하는게 먼저일 것 같구요. 그게 어느정도 잘 되어있고 (중복을 줄이고 네이밍을 신경쓰는 등의) 코드 개선이 어느정도 된 상태라면 두가지 다 크게 문제될 건 없을 것 같습니다.

오.. 왜 while let이 생각이 안났나 모르겠어요. 😂
피드백 반영해서 코드 수정 해보았습니다. 좋은 의견 주셔서 감사합니다!!

- number -> count

- 은행원이 일을 할 때 카운트를 해주었던 로직을 고객에게 대기표를 나눠줄 때 카운트 하도록 수정 - 클래스에서 구조체로 변경되면서 mutating키워드 추가

- Banking 연산 프로터피 speed 사용

- 기존에 guard문을 제거하고 while let으로 변경

leeari95 · 2021-12-29T04:55:00Z

델마! 이번에도 좋은 피드백 감사드립니다! 🤩 👍

아래와 같이 수정해보았습니다.
코멘트로 질문도 남겨드렸는데 확인 부탁드립니다~ 🙏🏻

수정사항

Bank 타입 리팩토링
- 은행원 수를 나타내는 프로퍼티명 number를 count로 수정
- Bank타입을 class가 아닌 struct로 수정
- workBanker() 메소드 내부에 while문을 while let으로 수정
Banker 타입의 workSpeed() 메소드를 제거
LinketList의 removeFirst() 메소드 내부를 Thread-Safe하게 개선
- Thread Safe
  - 기존 로직은 은행원 수만큼 global() 큐에 Task를 만드는 방식이여서 하나의 고객 큐에 여러 스레드가 접근해 Race Condition이 발생할 가능성이 있음.
  - 여러 스레드가 하나의 고객 큐에 접근해도 LinkedList에서 첫 번째 요소를 반환하는 메서드를 하나의 스레드에서만 접근할 수 있도록 SerialQueue.sync를 활용하여 Race Condition을 해결

코멘트 확인해주시고 문제없다면 merge 부탁드립니다!
감사합니다~ 😊

- 기존 로직은 하나의 고객 큐에 여러 스레드가 접근해서 race condition이 발생함.

delmaSong

수고하셨습니다 😃 경쟁상태를 고려한 코드 좋네요 ㅎㅎ
다음 스텝에서 뵈어요!

hwangjeha and others added 9 commits December 28, 2021 14:18

feat: Banking 열거형 타입 구현 #8

881e983

- 랜덤한 케이스를 반환하는 타입 메소드 random() 구현 - 케이스 별 업무 시간을 반환하는 연산 프로퍼티 speed 구현

feat: Banking 열거형 타입 원시값 추가 #8

adb2d27

- 원시값을 반환하는 연산 프로퍼티 description 구현

refactor: Bank 타입 workBanker() 메서드 개선 #9

a67d2a5

refactor: BankManager 이니셜라이저에 기본값 추가하여 리팩토링 #9

d153947

refactor: Customer Queue 옵셔널 바인딩 리팩터링 #9

d0c6cc6

- continue -> fatalError()

style: Banker 타입 work() 내부 줄바꿈 개선

6e4cc7c

docs: README.md STEP 3 내용 추가

e2d2679

delmaSong self-requested a review December 28, 2021 14:46

delmaSong reviewed Dec 28, 2021

View reviewed changes

hwangjeha and others added 4 commits December 29, 2021 13:03

refactor: Bank 타입 프로퍼티 명 개선 #10

f92daf5

- number -> count

refactor: Bank 타입을 class가 아닌 struct로 수정 #10

9a952a0

- 은행원이 일을 할 때 카운트를 해주었던 로직을 고객에게 대기표를 나눠줄 때 카운트 하도록 수정 - 클래스에서 구조체로 변경되면서 mutating키워드 추가

refactor: Banker 타입 workSpeed 메소드 제거 #10

b358f12

- Banking 연산 프로터피 speed 사용

refactor: workBanker 메소드 내부에 while 조건문을 while let으로 수정 #10

6627e57

- 기존에 guard문을 제거하고 while let으로 변경

leeari95 requested a review from delmaSong December 29, 2021 04:54

refactor: LinkedList removeFirst() thread safe 하게 개선 #10

45a164a

- 기존 로직은 하나의 고객 큐에 여러 스레드가 접근해서 race condition이 발생함.

hwangjeha force-pushed the step3 branch from f39e506 to 45a164a Compare December 29, 2021 05:35

delmaSong approved these changes Dec 29, 2021

View reviewed changes

delmaSong merged commit d2dbeb9 into yagom-academy:4-leeari95 Dec 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

은행 창구 매니저 [STEP 3] 아리, 허황 #128

은행 창구 매니저 [STEP 3] 아리, 허황 #128

leeari95 commented Dec 28, 2021

delmaSong left a comment •

edited

Loading

delmaSong Dec 28, 2021

delmaSong Dec 28, 2021

leeari95 Dec 29, 2021

delmaSong Dec 28, 2021

hwangjeha Dec 29, 2021

delmaSong Dec 28, 2021

hwangjeha Dec 29, 2021

delmaSong Dec 28, 2021

leeari95 Dec 29, 2021

delmaSong Dec 28, 2021

hwangjeha Dec 29, 2021

delmaSong Dec 29, 2021

delmaSong Dec 28, 2021

leeari95 Dec 29, 2021

leeari95 commented Dec 29, 2021

delmaSong left a comment

		private let loanBankersNumber: Int
		private let depositBankersNumber: Int

		workBankers(loanBankersNumber, customers: loanCustomerQueue, group: bankGroup)
		workBankers(depositBankersNumber, customers: depositCustomerQueue, group: bankGroup)

은행 창구 매니저 [STEP 3] 아리, 허황 #128

은행 창구 매니저 [STEP 3] 아리, 허황 #128

Conversation

leeari95 commented Dec 28, 2021

고민했던 점

1. 다중 처리

2. 대기번호 오름차순으로 업무를 할 수 있도록 처리

3. CustomStringConvertible을 사용하지 않고 연산프로퍼티를 사용

궁금했던 점 / 조언이 필요한 부분

들여쓰기 vs 함수 호출 단계

해결이 되지 않은 점

구조체 프로퍼티는 클로저 내부에서 변경할 수 없음

delmaSong left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leeari95 commented Dec 29, 2021

수정사항

delmaSong left a comment

Choose a reason for hiding this comment

delmaSong left a comment •

edited

Loading