Fix [#163] 릴리즈 QA 수정사항 2차 반영 #164
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -18,4 +18,5 @@ struct PostDetailResponseDTO: Decodable { | |||
| let likedNumber: Int | |||
| let commentNumber: Int | |||
| let contentText: String | |||
| let isDeleted: Bool | |||
| } | |||
There was a problem hiding this comment.
이 코드 패치는 꽤 안정적으로 보입니다. 몇 가지 주의 사항과 개선 제안은 다음과 같습니다:
isDeleted속성이 추가되었으나, 다른 속성들과의 일관성을 유지하기 위해PostDetailResponseDTO의 초기화나 서드파티 생성자에서 해당 필드에 대한 처리가 필요합니다.- 모든 속성이
let상수이므로 변경할 필요가 없다면var로 변경되어야 합니다. - 코딩 스타일을 통일하기 위해 현재 세로 공백이 한 줄이므로 파라미터 다음에 세로 공백을 두 줄로 바꾸는 것이 좋습니다.
위 내용을 고려하여 코드를 수정하면 더 강력하고 읽기 쉬운 코드가 될 것입니다.
| @@ -586,6 +595,7 @@ extension HomeViewController: UICollectionViewDataSource, UICollectionViewDelega | |||
| let destinationViewController = PostDetailViewController(viewModel: PostDetailViewModel(networkProvider: NetworkService())) | |||
| destinationViewController.contentId = homeViewModel.postDatas[indexPath.row].contentId | |||
| destinationViewController.memberId = homeViewModel.postDatas[indexPath.row].memberId | |||
| destinationViewController.userProfileURL = homeViewModel.postDatas[indexPath.row].memberProfileUrl | |||
| self.navigationController?.pushViewController(destinationViewController, animated: true) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
코드 패치의 코드 리뷰:
-
isDeleted속성을 사용하여 회원이 탈퇴한 경우 셀의 닉네임 텍스트 색상을 변경하고 프로필 이미지 뷰의 상호작용 여부를 설정합니다. 이 부분은 안전하게 보입니다. -
destinationViewController에 새로운 속성userProfileURL을 추가하여 해당 URL을 전달합니다. URL을 직접 전달하는 것은 좋은 방법입니다.
개선 제안:
- UI 변경으로 인해 닉네임 텍스트 색상 및 프로필 이미지 뷰의 상호작용 여부를 변경하는 로직은 View나 ViewModel에 있으면 더 좋을 수 있습니다.
- Magic string 대신에
.donGray12,.donBlack같은 색깔 값들을 상수나 enum 혹은 리소스 파일에서 가져오는 것이 유지보수 면에서 좋을 수 있습니다.
| @@ -339,7 +344,8 @@ extension MyPageContentViewController: UICollectionViewDataSource, UICollectionV | |||
|
|
|||
| func collectionView(_ collectionView: UICollectionView, didSelectItemAt indexPath: IndexPath) { | |||
| let contentId = contentDatas[indexPath.row].contentId | |||
| NotificationCenter.default.post(name: MyPageContentViewController.pushViewController, object: nil, userInfo: ["contentId": contentId]) | |||
| let profileImageURL = contentDatas[indexPath.row].memberProfileUrl | |||
| NotificationCenter.default.post(name: MyPageContentViewController.pushViewController, object: nil, userInfo: ["contentId": contentId, "profileImageURL": profileImageURL]) | |||
| } | |||
|
|
|||
| func scrollViewDidEndDragging(_ scrollView: UIScrollView, willDecelerate decelerate: Bool) { | |||
There was a problem hiding this comment.
코드 리뷰:
-
numberOfItemsInSection함수에서self.contentDatas.count가 0이면 처음 컨텐츠 버튼을 숨기고, 그렇지 않으면 숨기지 않도록 하였습니다. 이것은 버그를 일으킬 수 있습니다.firstContentButton.isHidden처리는willDisplay나reloadData등 다른 적절한 시점에 발생해야 합니다. 데이터 변경 시 바로 처리하도록 하는 것이 좋습니다. -
collectionView(_: didSelectItemAt:)함수에서contentId뿐만 아니라profileImageURL도 함께 전달하도록 변경되었습니다. 이 부분은 안전하지만, 해당 키("profileImageURL") 값을 잘 설명하는 주석을 추가하는 것이 도움이 됩니다. -
코드의 일관성을 위해 들여쓰기를 균일하게 유지하고 주석을 추가하여 각 메서드 및 확장의 목적을 명확히 할 수 있습니다.
-
코드에서 사용되는 메서드, 변수, 클래스의 이름이 명확하고 일관된지 확인하세요. 가독성을 높이는 데 도움이 됩니다.
-
각 기능이 정확히 무엇을 하는지 주석을 작성하여 코드를 이해하기 쉽게 만드세요.
-
에러 처리 및 예외 상황에 대한 고려가 충분히 되어 있는지 확인하세요.
-
코드 스타일 가이드에 따라 변수 및 함수의 네이밍을 개선할 수도 있습니다.
-
앱의 성능을 향상시키기 위해 필요한 경우 비동기 작업을 고려하세요.
-
클래스와 확장자에 대한 역할과 책임이 명확하게 나뉘어 있는지 확인하세요. 클래스가 너무 많은 책임을 가지지 않도록 구조화되어 있는지 검토해 보세요.
-
마지막으로, 유닛 테스트를 추가하여 각 메서드 및 기능이 의도한 대로 작동하는지 확인하세요.
| let destinationViewController = PostDetailViewController(viewModel: PostDetailViewModel(networkProvider: NetworkService())) | ||
| destinationViewController.contentId = contentId | ||
| destinationViewController.userProfileURL = profileImageURL | ||
| self.navigationController?.pushViewController(destinationViewController, animated: true) | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 리뷰를 해보겠습니다.
-
첫 번째 변경 사항:
self.rootView.myPageContentViewController.firstContentButton.isHidden = false해당 줄이 삭제되었습니다.- 변경 내용: 버튼에 대한 숨김 여부 설정이 삭제됨
- 위험성: 해당 버튼이 의도치 않게 표시될 수 있음
- 개선 제안: 필요에 따라 해당 버튼을 다른 방식으로 관리하거나, 왜 해당 코드가 제거되었는지 주석으로 설명
-
두 번째 변경 사항:
if let contentId = notification.userInfo?["contentId"] as? Int, let profileImageURL = notification.userInfo?["profileImageURL"] as? String {추가- 변경 내용: contentId와 profileImageURL 모두 확인 후 nil 체크
- 위험성: contentId와 profileImageURL 중 하나라도 nil 일 경우, 빈 값을 처리할 로직이 없음
- 개선 제안: nil 값일 경우의 처리 로직을 추가하여 예상치 못한 오류를 방지
-
일반적인 추천:
- 코드의 가독성을 높이기 위해 변수 및 기능에 설명적인 이름 부여
- 코드 주석 작성으로 다른 팀원들이 코드 이해를 돕기
- 옵셔널 바인딩보다, guard 문 사용 권장
위 리뷰를 통해 코드 개선과 잠재적인 버그 사항을 처리하여 코드 안정성을 향상시킬 수 있습니다.
| } | ||
| } | ||
| } | ||
|
|
||
| extension HomeViewController: DontBePopupReasonDelegate { | ||
| func reasonCancelButtonTapped() { | ||
| transparentReasonView.removeFromSuperview() |
There was a problem hiding this comment.
코드 리뷰 및 개선 제안:
-
버그 위험:
setDelegate()에서transparentPopupVC의 delegate 설정이 빠진 부분이 있습니다.
-
개선 제안:
transparentPopupVC인스턴스 생성을 제거한 것은 좋은 점입니다.- 코드 중복을 줄이기 위해 유사한 코드를 함수로 추출하여 재사용하는 방법을 고려해 볼 수 있습니다.
- 조금 더 읽기 쉽도록 변수 및 메소드명을 더 명확하게 지어주는 것이 도움이 될 수 있습니다.
- 레이아웃 구성하는 코드와 이미지 설정을 섞어놓은 부분이 있어 가독성이 떨어집니다. 이를 분리하여 관리할 수 있도록 고민해보세요.
- 코드 중복을 피하기 위해 삭제된
DontBePopupDelegate프로토콜을UICollectionViewDataSource와 연계하여 한번에 처리할 수 있는 방법을 고려해보시기 바랍니다.
-
일반적인 생각:
- 코드베이스를 더 간결하고 유지보수가 쉽도록 만들기위해 항상 주의 깊게 검토하고 수정하는 것이 좋습니다.
- Swift 디자인 가이드라인을 준수하여 일관된 스타일을 유지하는 것도 중요합니다.
| memberId: loadUserData()?.memberId ?? 0, | ||
| userProfileImage: loadUserData()?.userProfileImage ?? StringLiterals.Network.baseImageURL)) | ||
|
|
||
| OnboardingViewController.pushCount = 0 | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
코드 리뷰 및 개선 제안:
-
버그 위험:
- 코드 내에 명시적인 버그는 보이지 않습니다.
-
개선 제안:
- 초기화되고 사용되지 않는
transparentPopupVC변수를 삭제한 것은 좋은 점입니다. setUI()함수에서transparentPopupVC.modalPresentationStyle설정 줄을 삭제했습니다. 이것은 필요 없는 코드라서 삭제하는 것이 좋습니다.rootView속성을 갖는 함수들에서self.rootView사용을 지속적으로 최소화하고 있습니다. 일관성 유지를 위해 모든self.참조를 확인하십시오.- 코드 중복을 줄이기 위해
commentGhostButtonTapped()함수와ghostReason초기화 부분의 중복을 방지할 수 있는 방법을 고려해 보세요.
- 초기화되고 사용되지 않는
-
일반적인 피드백:
- 코딩 스타일을 일관되게 유지하십시오.
- 주석을 추가하여 복잡한 부분 또는 특정 설계 결정에 대한 이유를 설명할 수 있습니다.
코드는 보호 문제가 없어 보이나, 항상 코드 베이스를 테스트하여 예상대로 작동하는지 확인하십시오.
| } | ||
| } | ||
| } | ||
|
|
||
| extension PostDetailViewController: DontBePopupReasonDelegate { | ||
| func reasonCancelButtonTapped() { | ||
| transparentReasonView.removeFromSuperview() |
There was a problem hiding this comment.
코드 리뷰 및 개선 제안:
-
삭제 된
transparentPopupVC변수를 제거한 것은 올바른 조치입니다. -
transparentPopupVC를 구성하고 있던 코드를 댓글처리 하는 동작에 사용하는 새로운transparentReasonView로 대체했습니다. -
리팩토링 제안:
- 중복되는 코드 일부를 함수로 추출하여 재사용성을 높일 수 있습니다.
bindPostData(data:)와 같이 긴 메서드를 좀 더 작은 단위로 분할하여 가독성을 향상시킬 수 있습니다.- 콜렉션뷰의 셀 관련 설정들을 별도의 메서드로 추출하여 관리하면 좋을 것 같습니다.
-
유지보수 관점에서, 필요한 모든 UI 업데이트와 상태 변화는 설명 주석과 함께 문서화하는 것이 좋습니다.
-
클로져 내부에서 약한 참조를 사용하여 강한 순환 참조 문제를 방지할 수 있습니다.
-
확장(extension) 섹션을 기능적으로 묶어서 코드의 의도를 명확히 전달할 수 있습니다.
-
네트워크 호출 및 데이터 변경 시 에러 처리 및 예외 상황 처리를 추가해야 합니다.
-
UI 및 레이아웃 설정 관련하여 AutoLayout을 사용하면 디바이스 크기나 회전에 따른 레이아웃 이슈를 방지할 수 있습니다.
-
마지막으로 코드 리뷰가 진행된 부분은 주석을 통해 기록을 남겨 추후에 비교 및 개선 사항을 확인할 수 있도록 하세요.
위의 제안사항을 고려하여 코드를 개선하시면 좋을 것입니다.
| @@ -80,7 +109,7 @@ class MyPageSignOutConfirmView: UIView { | |||
|
|
|||
| private let info3Label: UILabel = { | |||
| let label = UILabel() | |||
| label.text = "탈퇴와 재가입을 통해 아이디를 교체하며 선량한 이용자들께 피해를 끼치는 행위를 방지하려는 조치 오니 넓은 양해 부탁드립니다." | |||
| label.setTextWithLineHeight(text: "탈퇴와 재가입을 통해 아이디를 교체하며 선량한 이용자들께 피해를 끼치는 행위를 방지하려는 조치 오니 넓은 양해 부탁드립니다.", lineHeight: 22.adjusted, alignment: .center) | |||
There was a problem hiding this comment.
P3
요 laebel들은 Literals로 따로 안 뺀 이유가 있을까요 ??
There was a problem hiding this comment.
P3
빠르게 진행하느라,,ㅎㅎ 리팩토링때 수정하겠슴당ㅎ
👻 PULL REQUEST
💻 작업한 내용
💡 참고사항
📟 관련 이슈