Skip to content
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

send message #5

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

send message #5

wants to merge 10 commits into from

Conversation

gracekim027
Copy link
Collaborator

No description provided.

Copy link
Owner

@peng-u-0807 peng-u-0807 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

실제 디바이스를 이용한 테스트가 불가능하다보니 기울기/가속 감지 로직에 대해서는 깊게 리뷰를 남기기 어렵네요. 그 외의 부분들 위주로 리뷰 남깁니다. 코멘트 달지 않은 부분에도 전반적으로 Swift 스타일에 맞지 않는 코드가 많이 보여서, 한번 쭉 보면서 수정해주시면 좋을 것 같아요 수고하셨습니다~

Comment on lines 138 to 146
if (!directionFiltered.isEmpty){
let directionSorted = directionFiltered.sorted(by: {
norm_one($0.direction!) < norm_one($1.direction!)
})
return directionSorted.first!.discoveryToken
}else{
let distanceSorted = nearbyObjects.sorted { $0.distance ?? .zero < $1.distance ?? .zero }
return distanceSorted[0].discoveryToken
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사소하지만..Swift 스타일로 수정해주시면 좋을 것 같아요

Comment on lines 31 to 33
//MARK: - 뷰 관련 값
@Published var emoji : String = ""
@Published var content : String = ""
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기 emojicontent를 뷰모델로 뺀 이유가 있나요? 뷰모델에서 뷰로 publish할 필요가 있을지 궁금하네요

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

메세지를 보내는 로직이 뷰쪽에 있으면 mvvm 구조랑 맞지 앚는 것 같아서 뷰모델으로 빼놓았습니다. 뷰에서는 뷰모델의 viewstate 값에 따라서 중간 레이블 값만 (타이머, 전송 성공, 전송 실패, 모션 감지 실패 등) 바꿀 수 있도록 했고, 뷰에서 모션 감지나 보내는 로직이 들어가 있으면 복잡할 것 같아서요. 어떻게 생각하시나요?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 메시지를 보내는 로직이 뷰모델이 있는건 맞는데, emojicontent 자체는 뷰모델의 지시에 따라 바뀌어야하는 값이 아니므로 뷰가 독립적으로 들고 있다가 뷰모델쪽으로 그때그때 넘겨주는 방식이 맞아보여요

Comment on lines 35 to 44
//MARK: - 메세지 전송 관련 값
let motionManager = CMMotionManager()
@Published var sendTimer: Timer?
@Published var currentState: viewState = .none
@Published var isSending: Bool = false
@Published var isAccelerating: Bool = false
@Published var accelerationRate: Double = 0.0
@Published var counter: Int = 0
@Published var isReadyForSending: Bool = false
var isDetected: Bool = false
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 변수들 모두 view 내부에서만 사용되고 뷰모델을 통한 세션과의 연결은 필요하지 않으므로 특별한 이유가 없다면 뷰에 있는 것이 맞아보이는데 혹시 어떻게 생각하시나요

Comment on lines 81 to 86
.onChange(of: viewModel.content) { text in
if text.last == "\n" {
viewModel.content = String(text.dropLast())
UIApplication.shared.sendAction(#selector(UIResponder.resignFirstResponder), to:nil, from:nil, for:nil)
}
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분은 입력값의 마지막이 개행 문자인지 검사하는 것보다는 .onSubmit 등을 이용하는 것이 어떨까요? 그리고 @FocusState를 이용 중이므로 UIApplication ~ 한 줄을 위해 UIKit을 import하는 것보다는 SwiftUI스럽게 isTextFieldFocused = false로 수정하면 어떨지 제안 드립니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분은 지호님께서 수정하신 부분인것 같은데 깃에서 제가 추가한걸로 인식한 것 같네요!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants