-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
은행창구 관리앱 [Step4] 미르, 희동 #99
base: d_Mireu
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
마지막 스텝까지 고생 많으셨습니다!
전체적으로 그래도 핵심들을 잘 완성하셨어요 👍👍
리뷰도 이에 조금 놓칠 수 있는 부분들 위주로 드려봤습니다ㅎㅎ
궁금하신 부분에 대한 저의 의견
- 이건 사실상 고객 추가가 되는 로직에서도 timer를 isRunning 상태면 건들지 않는 등 로직적으로 문제는 없어보입니다!
그리고, 실제로 빨라지는 느낌 자체이지 실제 시간이 빨라지진 않았다고 보여요.
그렇기에 이 부분은 뷰 렌더링 관점에서 보는것이 조금 더 적합하지 않을까 생각합니다 🙋🏻
고객 추가를 하는 로직을 거쳐 뷰에 들어온 고객들을 추가할때 뷰가 다시 렌더링 되면서 그 렌더링 되는 시간동안 타이머는 실제로 지났지만, 뭔가 버벅이고 빨라져 보이는 느낌이 들거라 생각합니다ㅎㅎ
요 부분을 가지고 한번 고민해봐도 좋을것 같아요!
다시 한번 수고 많으셨습니다ㅎㅎ
우선 기간이 끝나서 어프로브 해두겠습니다~
언제든 머지 요청 주세요ㅎㅎ
print("업무가 마감되었습니다. 오늘 업무를 처리한 고객은 총 \(customNum)명이며, 총 업무시간은 \(time)초 입니다.") | ||
} | ||
} | ||
//struct Bank { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
실제 사용되지 않는 코드들은 데드 코드로 남겨두기보단 없애는것이 추후 현업에서 협업 PR을 올릴 때 신경쓰셔야할 하나의 부분이라 남겨둡니다 🙋🏻
아니라면, MARK 주석으로 왜 주석을 처리하는지에 대해 남겨둬도 좋습니다ㅎㅎ
init(bankClerk: [Banking : BankClerk]) { | ||
self.bankClerk = bankClerk | ||
} | ||
private let semaphore2 = DispatchSemaphore(value: 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2와 같은 네이밍보다 어떤 세마포어인지 구분 될 수 있다면 네이밍을 조금 더 명확히 바꿔보는건 어떨까요~?
func quit(cusomer: Customer) | ||
} | ||
|
||
class ViewController: UIViewController { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추후 더 많은 VC가 생기면 해당 네이밍이 살아남기 어려울것 같아요ㅠ
더 명확한 네이밍이 필요해보입니다 🙋🏻
func quit(cusomer: Customer) | ||
} | ||
|
||
class ViewController: UIViewController { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추후 상속될 여지가 있는 VC가 아니라면 final을 활용해봐도 좋을것 같아요!
fatalError("init(coder:) has not been implemented") | ||
} | ||
|
||
func viewLabel() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메서드명이 조금 어색한것 같은데 updateLabel
나 setLabel
과 같은 네이밍은 어떤가요?
대개 자주쓰이는 네이밍이라 말씀드려봅니다 🙋🏻
private let waitLabel: UILabel = { | ||
let label = UILabel() | ||
label.text = "대기중" | ||
label.font = .preferredFont(forTextStyle: .largeTitle) | ||
label.textColor = .white | ||
label.textAlignment = .center | ||
label.backgroundColor = .systemGreen | ||
label.translatesAutoresizingMaskIntoConstraints = false | ||
return label | ||
}() | ||
|
||
private let taskLabel: UILabel = { | ||
let label = UILabel() | ||
label.text = "업무중" | ||
label.font = .preferredFont(forTextStyle: .largeTitle) | ||
label.textColor = .white | ||
label.textAlignment = .center | ||
label.backgroundColor = .systemIndigo | ||
label.translatesAutoresizingMaskIntoConstraints = false | ||
return label | ||
}() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다른 부분도 거의 비슷하겠지만, 이런 스타일을 적용하는 로직이 동일하다면 하나의 별도 메서드로 통일하여 구성하고 간결하게 사용해봐도 좋을것 같아요 🙋🏻
DispatchQueue.main.async { | ||
let cusomerLabel = self.bankView.waitStackView.arrangedSubviews.first { view in | ||
guard let cusomerLabelNumber = view as? CustomerLabel else { return false } | ||
return cusomerLabelNumber.customer.numOfPerson == cusomer.numOfPerson |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
별거아니지만 오탈자가 있는것 같습니다~
cusomer -> customer
let customerLabel = self.bankView.taskStackView.arrangedSubviews.first { view in | ||
guard let cusomerLabelNumber = view as? CustomerLabel else { return false } | ||
return cusomerLabelNumber.customer.numOfPerson == cusomer.numOfPerson | ||
} | ||
customerLabel?.removeFromSuperview() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위 turn과 공통적인 로직인 고객레이블을 제거하는 로직 자체를 별도 private 메서드로 분리하여 사용해도 좋을것 같습니다 🙋🏻
@GREENOVER 안녕하세요 그린!
완벽구현은아니지만 할수있는만큼 해봤습니다!
많이 늦었지만 STEP4도 잘부탁드립니다!🙇
Step 4 : UI구현하기
🤔 고민했던점
업무가 끝나면 타이머가 일시정지되는 부분에 대해 고민했습니다.
고객을 10명추가하는 버튼을 누르고 업무중인 업무가 종료되면 타이머가 일시정지되어야하는데 멈추지 않고, 타이머가 계속흘러가는 이슈를 발견하여 dispatchGroup에 종료시점을 알리는 notify메서드를 넣어줬습니다. 그래서 종료를 알리는 시점에 timer가 멈출수 있도록 invalidate를 클로저내에서 실행되도록 구현해줬더니 업무가 끝나면 타이머가 일시정지되었습니다.
UI에서 스크롤하면 업무가 타이머가 일시정지되지 않는 부분을 고민했습니다.
업무를 진행하는 중에 UI화면에서 스크롤을 하더라도 타이머가 계속 흘러가야하는데, 스크롤을 하면 타이머가 멈추는 이슈가 있었습니다. 그래서 timer가 nil이 아닌지 확인하고 guard문을 통과했을때 타이머를 메인 run loop( 메인 run loop은 많은 애플리케이션에서 사용자 인터페이스를 업데이트하는 역할을 담당)에 추가하여 타이머가 계속 작동하도록 수정해줬습니다.
고객추가를 2-3번 눌렀을때 업무가 끝나면 타이머가 일시정지되는 부분에 대해 고민했습니다.
고객추가를 한번누르면 업무가 끝났을때 타이머가 일시정지되는데, 2-3번 누르더라도 업무가 끝나면 타이머가 일시정지되어야하는데, 2-3번 누르면 업무가 다끝나도 타이머가 멈추지않고 계속 진행되는 이슈를 발견했습니다. 그래서 Bool타입으로 타이머 실행여부를 확인하고 그에 맞는 동작을 수행하도록 해줬습니다. false를 줘서 타이머가 아직 실행 중이지 않은 경우에 새로운 타이머를 생성하고 시작하고, true로 업데이트하여 타이머가 현재 실행 중임을 나타넀습니다.
비동기처리를 UI에 어떻게 동작할지에 대해 고민했습니다.
모델에서 구현한 비동기처리를 데이터전달을 통해 UI에서도 동작할수있도록 델리게이트패턴을 사용했습니다.
🧐 조언을 얻고 싶은 부분
고객추가버튼을 누르면 업무를 진행할때 타이머가 진행되는데, 2번이상 연속으로 누르게 되면 타이머가 더 빨라지는 느낌이 듭니다.. 이런 이슈가 왜 발생하는지에 대한 조언을 얻고 싶습니다.