-
Notifications
You must be signed in to change notification settings - Fork 0
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
Network [#91] 투명도 낮추기 API 연결, 투명도 적용 토스트 메시지 분기 처리 완료, 답글 조회 API 연결 #94
Conversation
@@ -1136,6 +1155,7 @@ | |||
3CBCA3CA2B57212400D348D3 /* MyPageMemberCommentResponseDTO.swift in Sources */, | |||
3CD25D272B5466DB0075F80F /* Empty.swift in Sources */, | |||
2F1741882B500C270089FC4D /* UIApplication+.swift in Sources */, | |||
3CF4651B2B58398900997FCA /* PostTransparencyRequestDTO.swift in Sources */, | |||
3C2F544E2B50F65500E7BF01 /* DontBeBottomSheetView.swift in Sources */, | |||
2F05B1A72B5799B700AC368D /* PostDetailResponseDTO.swift in Sources */, | |||
3C61930C2B3A782100220CEB /* StringLiterals.swift in Sources */, |
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.
이 코드 패치는 주어진 파일들을 프로젝트에 추가하는 역할을 합니다. 패치 내용을 살펴보면, 3CF4651B2B58398900997FCA 파일과 관련된 빌드 설정과 파일 참조가 추가되었습니다.
개선 사항:
- 코드 패치에서 오류 위험이나 버그는 없어 보입니다.
- 단, 이 코드 패치만으로는 전체적인 컨텍스트 파악이 어렵습니다. 다른 부분의 코드와의 연관성을 알 수 있는 추가 정보가 있다면 도움이 될 것입니다.
@@ -58,6 +58,7 @@ enum ImageLiterals { | |||
|
|||
enum Home { | |||
static var textLogo: UIImage { .load(name: "Logo") } | |||
static var icnNotice: UIImage { .load(name: "icn_notice") } | |||
} | |||
|
|||
enum Posting { |
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.
아래는 코드 패치입니다. 해당 패치에 대한 간단한 코드 리뷰를 도와드리겠습니다. 버그 위험 및 개선 제안은 환영합니다.
- 이 코드 패치에는 심각한 버그나 위험이 보이지 않습니다.
- "Home" 열거형에 "icnNotice"라는 UIImage 속성이 추가되었습니다. 문제가 없어 보입니다.
개선 제안:
- 현재 코드 패치에서 큰 문제는 없어 보입니다. 하지만 몇 가지 개선 사항이 있을 수 있습니다. 이는 전체 코드 범위와 다른 파일/클래스에 따라 다를 수 있습니다.
- 네이밍 컨벤션을 확인하여 통일성 있는 네이밍을 사용하는 것이 좋습니다. 예를 들어 "icnNotice" 대신 "noticeIcon"과 같은 네이밍을 고려할 수 있습니다.
- Swift API 디자인 가이드라인을 따르면서,
UIImage
로딩에 대한 옵셔널 처리나 오류 처리를 추가하는 것도 고려할 수 있습니다.
좀 더 자세한 코드 분석이 필요하다면 전체 코드 범위와 다른 파일/클래스에 대한 정보를 제공해주시면 됩니다.
@@ -64,6 +64,7 @@ enum StringLiterals { | |||
enum Toast { | |||
static let uploading = "게시 중..." | |||
static let uploaded = "게시 완료!" | |||
static let alreadyTransparency = "이미 투명도를 적용한 유저예요." | |||
} | |||
|
|||
enum Notification { |
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.
주어진 코드 패치에는 별다른 버그나 위험은 발견되지 않았습니다. 그러나 몇 가지 개선 제안이 있습니다:
-
문자열 리터럴을 열거형으로 관리하는 기능은 좋습니다. 하지만 이 경우,
StringLiterals
와Toast
,Notification
은 독립된 두 개의 열거형으로 구성되어 있습니다. 열거형을 효율적으로 사용하려면 관련 있는 값들을 하나의 열거형으로 그룹화하는 것이 좋습니다. -
문자열을 저장하기 위해
enum
을 사용하는 대신 일반적인struct
를 사용해도 됩니다. 이 경우, 문자열 상수를static let
대신static var
로 선언할 수 있습니다. -
번역 가능한 문자열을 관리하는 방법도 고려해 볼 수 있습니다. 특히,
uploading
과uploaded
와 같은 내용은 다국어 지원을 위해 로컬라이즈해야 할 수도 있습니다.
필요에 따라 이러한 제안들을 참고하여 코드 패치를 수정하는 것이 좋습니다.
"author" : "xcode", | ||
"version" : 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.
위의 코드 패치를 간단히 검토해 드리겠습니다.
이 코드는 JSON 형식으로 되어 있으며, "images" 배열 내에 이미지 파일 정보가 포함되어 있습니다. 각 이미지는 "filename" (파일 이름), "idiom" (용도), "scale" (배율) 속성을 가지고 있습니다. 이 속성들은 각각 이미지 파일의 이름, 일반적인 용도, 배율을 나타냅니다.
코드 자체에 버그나 위험이 없으며, 개선 제안 사항도 별다른 것이 없습니다. 이는 단순히 이미지 파일 정보를 담고 있는 JSON 데이터이기 때문에 수정할 필요가 없을 수 있습니다.
크게 문제가 있다거나 개선이 필요한 부분이 있다면 추가로 알려주세요.
let alarmTriggerType: String | ||
let targetMemberId: Int | ||
let alarmTriggerId: Int | ||
} |
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.
이 코드 패치는 PostTransparencyRequestDTO
라는 구조체를 정의하고 있습니다. 이 구조체는 Encodable
프로토콜을 준수하며, alarmTriggerType
, targetMemberId
, alarmTriggerId
라는 세 가지 속성을 갖고 있습니다.
이 코드에 현재 확인할 수 있는 버그나 위험이 없어 보입니다. 그러나 몇 가지 개선점이 있을 수 있습니다:
- 주석을 더욱 상세하게 작성하는 것이 도움이 됩니다. 예를 들어, 각 속성이 어떤 역할을 하는지, 사용되는 데이터 형식은 어떤지 설명할 수 있습니다.
- 속성 이름과 변수 이름에서 더욱 명확한 이름으로 변경할 수 있습니다. 읽기 쉬운 코드는 유지보수와 협업을 용이하게 만듭니다.
- 필요에 따라 속성들에 대한 유효성 검사를 추가할 수 있습니다. 예를 들어,
targetMemberId
와alarmTriggerId
가 양수인지 확인하는 등의 체크를 할 수 있습니다. - 일반적으로
Codable
프로토콜을 사용할 때, 커스텀한 인코딩 및 디코딩 로직이 필요한 경우에는 특별한 조치가 필요할 수 있습니다. 이러한 경우에는CodingKey
프로토콜을 구현하거나, 커스텀 인코딩 및 디코딩 메서드를 추가할 수 있습니다.
그 외에는 현재 코드의 명료성과 안전성이 보존되며, 문제가 없어 보입니다.
@@ -187,7 +190,7 @@ extension HomeCollectionViewCell { | |||
|
|||
func setLayout() { | |||
backgroundUIView.snp.makeConstraints { | |||
$0.edges.equalToSuperview() | |||
$0.top.bottom.equalToSuperview() | |||
$0.width.equalTo(UIScreen.main.bounds.width - 32) | |||
} | |||
|
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.
아래는 코드 패치입니다. 버그 위험 또는 개선 제안이 있는지 간단한 코드 리뷰를 도와드리겠습니다.
alarmTriggerType
,targetMemberId
,alarmTriggerdId
라는 변수가 추가되었습니다. 해당 변수들의 초기화 값은 적절한지 확인해야 합니다.backgroundUIView.snp.makeConstraints
부분에서$0.edges.equalToSuperview()
대신$0.top.bottom.equalToSuperview()
로 수정되었습니다. 수정된 제약 조건이 의도한대로 동작하는지 확인해야 합니다.
let button = UIButton() | ||
button.setImage(ImageLiterals.Posting.btnTransparent, for: .normal) | ||
return button | ||
}() | ||
|
||
private let verticalTextBarView: UIView = { | ||
let verticalTextBarView: UIView = { | ||
let view = UIView() | ||
view.backgroundColor = .donPale | ||
return view |
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.
아래는 코드 패치입니다. 이 코드에 대한 간략한 코드 리뷰를 도와드리겠습니다. 버그 위험 또는 개선 제안이 있으면 알려주세요.
alarmTriggerType
,targetMemberId
,alarmTriggerdId
변수가 추가되었습니다.profileImageView
,nicknameLabel
,transparentLabel
,timeLabel
,kebabButton
,contentTextLabel
,likeButton
,likeNumLabel
,ghostButton
,verticalTextBarView
등의 UI 구성 요소가 private에서 let으로 변경되었습니다.
개선 제안:
- 변수 이름은 명확하고 의미 전달이 잘 되도록 작성해야 합니다.
alarmTriggerdId
와 같이 오타가 있는 변수 이름을 검토해보세요. - 기능과 목적에 따라서 여러 개의 함수로 분할하여 코드를 정리하는 것도 좋은 방법일 수 있습니다.
- 중복된 코드를 줄이고, 유지 관리 및 가독성을 향상시키기 위해 코드를 리팩토링해 볼 수 있습니다.
- 주석을 추가하여 코드의 의도와 동작 방식을 더 명확하게 설명할 수 있습니다.
} | ||
} | ||
} | ||
} |
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.
이 코드 패치는 몇 가지 버그와 개선 가능한 점이 있습니다:
버그:
viewDidLoad()
메서드에서self.navigationController?.navigationBar.isHidden
과textFieldView.isUserInteractionEnabled
값을 설정하는 부분이 주석 처리되어 있습니다. 이 값들을 적절히 설정해야 합니다.bindPostData(data:)
메서드에서self.postView.ghostButton.isHidden
와self.postView.verticalTextBarView.isHidden
의 초기화 상태를 올바르게 설정해야 합니다.getPostReplyData
구독에서self.postReplyCollectionView.reloadData()
를 호출하는 코드가 주석 처리되어 있습니다. 이 코드를 활성화하여 데이터 갱신 후 컬렉션 뷰를 다시 로드해야 합니다.
개선 사항:
showAlreadyTransparencyToast()
메서드가 비동기로 작동하고 있습니다. 이로 인해 투명도 토스트가 제대로 보여지지 않을 수 있습니다. 동기적으로 토스트를 표시하도록 변경해야 합니다.collectionView(_:numberOfItemsInSection:)
메서드에서viewModel.postReplyData
를 시간에 따라 내림차순으로 정렬하는 로직을 추가할 수 있습니다.collectionView(_:cellForItemAt:)
메서드에서 셀의 내용을 올바르게 설정하고 필요한 액션을 추가해야 합니다.
이 외에도 코드 전체를 살펴봐야하며, 사용되고 있는 다른 클래스나 메서드의 정의와 구현을 알아야 전체적인 확인이 가능합니다. 다만 여기서 언급된 사항들은 주요한 버그와 개선사항입니다.
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.
수고하셨슴다
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.
당신은 천재입니까? 감사합니다👻
self.alreadyTransparencyToastView?.snp.makeConstraints { | ||
$0.leading.trailing.equalToSuperview().inset(16.adjusted) | ||
$0.bottom.equalTo(self.tabBarHeight.adjusted).inset(6.adjusted) | ||
$0.height.equalTo(44) |
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.
P3
adjusted 추가 부탁드랴여!
} | ||
} | ||
} | ||
} |
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.
아래는 코드 패치에 대한 간략한 코드 리뷰입니다. 버그 위험 또는 개선 제안 사항을 환영합니다:
alarmTriggerType
,targetMemberId
,alarmTriggerdId
변수를 초기화하지 않았으며, 사용되기 전에 값을 할당해야 합니다.showAlreadyTransparencyToast()
메서드에서 애니메이션의 지연 시간과 다음 조치를 실행하기 전에 해당 ToastView를 삭제하는 대기 시간을 확인하십시오.setDelegate()
메서드에서transparentPopupVC.transparentButtonPopupView.delegate
를 설정하기 전에 해당 뷰가 초기화되었는지 확인하십시오.cell.KebabButtonAction
클로저와cell.TransparentButtonAction
클로저에서 동일한 코드가 중복됩니다. 코드 중복을 방지하고 유지 보수성을 향상시키기 위해 중복 코드를 추출하여 재사용할 수 있는 독립적인 함수로 만드는 것이 좋습니다.DontBePopupDelegate
프로토콜의confirmButtonTapped()
메서드에서 이스케이프 비동기 작업을 사용하고 있습니다. 이 경우await
키워드와 함께 사용받은 표현식을Task { }
블록 안으로 이동시켜야 합니다.
위의 사항들을 고려하여 코드를 개선할 수 있습니다.
👻 PULL REQUEST
💻 작업한 내용
📸 스크린샷
📟 관련 이슈