[크리스마스 프로모션] 코드 리뷰 #3
Conversation
…Menu() 함수 동작 변경 없이 처리 과정 변경
| private const val ERROR_MESSAGE_PREFIX = "[ERROR] " | ||
| const val ERROR_MESSAGE_INPUT_VISIT_DATE = ERROR_MESSAGE_PREFIX + "유효하지 않은 날짜입니다. 다시 입력해 주세요." | ||
| const val ERROR_MESSAGE_INPUT_MENU = ERROR_MESSAGE_PREFIX + "유효하지 않은 주문입니다. 다시 입력해 주세요." | ||
|
|
There was a problem hiding this comment.
"ERROR"을 저렇게 따로 const val로 빼놓으니까 한결 가독성도 올라가고 혹시 모를 오타가 있어도 발견하기 쉬울 것 같네요(ex.ERROR을 Error로 썼다거나 하는 오류 들이요)
There was a problem hiding this comment.
오타 보다는 여러 가지가 파생될 수 있는 에러 메세지를 관리하기 위한 의도에 작성했었던 것 같아요. 다음 리뷰에 cbj0010님이 관련하여 제시해주신 Enum Class를 통해 관리하는 방법도 있어서 park-yina님께도 공유드립니다!
enum class ErrorMessage(private val message: String) {
ERROR_MENU_INPUT("유효하지 않은 주문입니다. 다시 입력해 주세요."),
ERROR_INPUT_DAY("유효하지 않은 날짜입니다. 다시 입력해 주세요.");
fun getMessage(): String = "[ERROR] $message"
}
| if (menuName == StoreMenu.T_BONE_STEAK.menuName || menuName == StoreMenu.BBQ_RIBS.menuName || | ||
| menuName == StoreMenu.SEAFOOD_PASTA.menuName || menuName == StoreMenu.CHRISTMAS_PASTA.menuName | ||
| ) |
There was a problem hiding this comment.
코틀린 코딩 컨벤션에서는 의무 사항은 아니지만, 3개 이상의 조건에 대해서는 when의 사용을 권장합니다. 만약 if를 사용하고 싶으시다면, 가독성 적인 측면을 위해서 각각 조건에 대해 여러 줄로 나누시는 등의 방법으로 가독성을 높이는 것이 좋을 것 같습니다.
There was a problem hiding this comment.
으악,,가독성을 많이 신경 쓴 미션이었는데 놓친 부분이 있었네요. 말씀해주신 방법들도 ELSE를 지양할 수 있다는 가정하에 고려해볼 수 있을 것 같아요. 지금 드는 생각은 메인 메뉴들을 맵이나 리스트로 그룹화하고 조건 검사를 진행했다면 한번의 검사로도 충분히 동일한 처리를 했을 수 있을 거 같아요..다른 클래스에서는 동일한 방법으로 리팩토링을 진행했기 때문에 아쉬움이 많이 남는 부분이네요..리뷰 감사합니다 :)
| validateVisitDate(visitDateInput) | ||
| visitDateInput.toInt() |
There was a problem hiding this comment.
validateVisitDate(visitDateInput) 예외 처리 과정에서 IllegalArgumentException이 발생하지 않는다면 입력이 유효한 것이기 때문에 따로 변수를 선언하여 불필요한 메모리 할당을 하지 않고 Int로만 변환하여 return 해도 될 것이라고 생각하고 작성했던 코드인 것 같습니다. validateVisitDate() 함수는 예외 처리에 관한 함수이기 때문에 역할과 책임에 대해서도 고민하고 작성했던 것 같긴 하지만 제시해주신 의견대로 validateVisitDate()에 return type을 지정하여서 return 값을 가지는 것도 하나의 방법이 될 수 있을 것 같아요! 감사합니다 :)
| when { | ||
| benefitAmount in STAR_MIN..STAR_MAX -> return BADGE_STAR | ||
| benefitAmount in TREE_MIN..TREE_MAX -> return BADGE_TREE | ||
| benefitAmount >= SANTA_MIN -> return BADGE_SANTA | ||
| } | ||
|
|
||
| return NONE |
There was a problem hiding this comment.
이거
return when{
어쩌고 저쩌고
else -> NONE
}
이렇게도 쓸 수 있는 걸루 알아요!
There was a problem hiding this comment.
미션의 요구 사항에 'else를 지양한다.'는 사항에 대해서 가독성은 유지함과 동시에 else를 사용하지 않는 방법에 대하여 고민하고 작성했던 코드인 것 같습니다. 제시해주신 return when도 구현 과정에서 고려했었지만 최종적으로는 when을 사용하게 되었던 것 같아요. else를 지양하면서 더 나은 가독성을 갖출 수 있는 방법을 더 고민해봐야 될 것 같습니다! 리뷰 감사합니다 :)
There was a problem hiding this comment.
헉 저도모르게 when문 안에 들어있는 else는 if-else랑 다르다고 생각을 하고 있었습니다..! 제가 놓친 부분을 짚어주셔서 감사하네요!
| WEEKEND_DISCOUNT("주말 할인"), | ||
| SPECIAL_DISCOUNT("특별 할인"); | ||
|
|
||
| companion object { |
There was a problem hiding this comment.
enum class 속 companion object는 어떤 역할을 하나요? 잘 몰라서 물어봅니다 😓
There was a problem hiding this comment.
답변을 드리고자 내용을 작성하였는데..코드 자체적으로도 단일 책임 원칙에 관해 더 고려할 부분이 있어 보이고 혹시나 제 답변이 사실과 다른 내용이 포함된다면 학습에 혼란을 드릴 수 있을 것 같아 잘 설명된 링크를 첨부합니다! 큰 도움을 드리지 못해 죄송합니다..
링크 1. https://kotlinlang.org/docs/object-declarations.html#companion-objects
링크 2. https://www.baeldung.com/kotlin/enum-static-method
| private const val ERROR_MESSAGE_PREFIX = "[ERROR] " | ||
| const val ERROR_MESSAGE_INPUT_VISIT_DATE = ERROR_MESSAGE_PREFIX + "유효하지 않은 날짜입니다. 다시 입력해 주세요." | ||
| const val ERROR_MESSAGE_INPUT_MENU = ERROR_MESSAGE_PREFIX + "유효하지 않은 주문입니다. 다시 입력해 주세요." |
There was a problem hiding this comment.
이렇게도 쓸 수 있군요!
저는 이런식으로 썼어요!
enum class ErrorMessage(private val message: String) {
ERROR_MENU_INPUT("유효하지 않은 주문입니다. 다시 입력해 주세요."),
ERROR_INPUT_DAY("유효하지 않은 날짜입니다. 다시 입력해 주세요.");
fun getMessage(): String = "[ERROR] $message"
}
There was a problem hiding this comment.
작성해주신 코드도 매우 깔끔하고 가독성이 높아 보여요! 에러 메세지들이 연관성이 있는 상수들이지만 Enum Class도 하나의 모델로 구성했기 때문에 객체를 증가시키기 보다는 다음과 같이 상수화 처리만 하였던 것인데 에러 메세지들을 더 명확하게 분류하는 방법이 아직 많이 부족한 것은 사실인 것 같습니다. 구성과 설계, 가독성을 높이는 방법에 대해 더 학습해보겠습니다. 리뷰 감사합니다 :)
| @@ -0,0 +1,59 @@ | |||
| package christmas.utils | |||
|
|
|||
| object Constant { | |||
There was a problem hiding this comment.
저는 개인적으로 이런 경우에는, Constants 파일 안에 object를 여러개 두는 편이에요!!
예시로
object Model {
const val FIRST_DAY_DISCOUNT_AMOUNT = 1_000
const val DAILY_DISCOUNT_AMOUNT = 100 ...
}
object Validate {
const val ORDER_ITEMS_SIZE = 2
const val VISIT_DATE_MIN = 1....
}
이런 식으로 하면 주석을 안 달아도 되어요!!
There was a problem hiding this comment.
와 이렇게 작성한다면 정말 깔끔해질 것 같아요! 저한테 꼭 필요한 학습 부분이었습니다. 다음 코드부터 적용해 확실히 개선해보겠습니다! 리뷰 감사드립니다 :)
| data class OrderItems( | ||
| private val menuName: String, | ||
| private val quantity: Int | ||
| ) { | ||
| fun getMenuName(): String = menuName | ||
|
|
||
| fun getQuantity(): Int = quantity | ||
| } |
There was a problem hiding this comment.
val 선언 때문에 따로 private로 제한하지 않아도 값 수정이 불가능할텐데, 혹시 private로 선언한 이유가 따로 있을까요..?!
There was a problem hiding this comment.
3주차 공통 피드백에도 포함되어 있는 내용인데 val은 값을 불변으로 제한하고자 하는 의도이고 private은 객체의 상태 접근을 제한하는 의도라고 생각하였습니다! 값의 수정을 막는 것도 중요하지만 private 접근 제한을 통해 객체를 확실하게 보호하고자 하는 의도에 작성하게 코드인 것 같아요. 리뷰 감사합니다 :)
| @Test | ||
| fun `배지 부여 테스트`() { | ||
| val noneBadge = decemberEvent.assignBadge(0) | ||
| val starBadge = decemberEvent.assignBadge(5000) | ||
| val treeBadge = decemberEvent.assignBadge(10000) | ||
| val santaBadge = decemberEvent.assignBadge(200000) | ||
|
|
||
| Assertions.assertTrue(noneBadge == "없음") | ||
| Assertions.assertTrue(starBadge == "별") | ||
| Assertions.assertTrue(treeBadge == "트리") | ||
| Assertions.assertTrue(santaBadge == "산타") | ||
| } |
There was a problem hiding this comment.
이 부분도 decemberEvent.assignBadge가 반복되기 때문에 @ParameterizedTest로 간단히 만들수 있을거 같아요!!
개인적으로 저는 저렇게 인자가 두개 필요한 경우에는 @MethodSource로 간단히 하는 편입니다!
여기 링크 참고하시면 될거 같아요!!
There was a problem hiding this comment.
테스트 코드에 대해서는 아직 어려움을 느끼고 있기 때문에 많은 학습이 필요한 것 같아요. 자세한 설명과 링크까지..리뷰 정말 감사합니다!!
| when { | ||
| freebie -> println(ITEMS_RESULT.format(StoreMenu.CHAMPAGNE.menuName, PRESENT_AMOUNT)) | ||
| !freebie -> println(NONE) | ||
| } |
There was a problem hiding this comment.
이 부분은 else를 지양해야한다는 요구사항때문에 고민이 많았었는데, 저는 그래도 코딩 컨벤션을 지키는게 좋을거 같아서 if else 문을 사용했습니다! 이 부분은 사람마다 관점이 다를거 같아서 그냥 참고만 해주세요!!
관련 컨벤션 링크입니다!!
There was a problem hiding this comment.
else를 지양하는 것이 구현 과정에서 여러번 고민에 빠지게 했던 것 같아요.. 가독성을 높이고 컨벤션을 지킬 수 있다면 더 주체적으로 판단하고 작성하는 것도 올바른 방향이라고 공감이 가는 것 같아요! 리뷰 감사합니다 :)
| for (item in orderMenu) { | ||
| val increaseCount = increaseDessertMenuCount(item.getMenuName(), item.getQuantity()) | ||
| dessertMenuCount += increaseCount | ||
| } |
There was a problem hiding this comment.
만약에 제가 kotlin api를 활용햇 해당 메서드를 구현한다면,
dessertMenuCount = orderMenu.filter { it.menuName == StoreMenu.CHOCO_CAKE.menuName || it.menuName == StoreMenu.ICE_CREAM.menuName }.sumOf { it.quantity }
이렇게 하면 메서드 하나로 디저트 메뉴 개수를 구할수도 있어요!!
There was a problem hiding this comment.
제시해주신 코드가 확실히 간결하고 코틀린스러워 보이네요! 앞으로의 과정에서는 코틀린에 대한 학습 시간을 일정 시간 지정해서 스스로 숙련도를 향상시킬 필요가 있을 것 같아요. 리뷰 감사합니다 :)
| if (menuName == StoreMenu.T_BONE_STEAK.menuName || menuName == StoreMenu.BBQ_RIBS.menuName || | ||
| menuName == StoreMenu.SEAFOOD_PASTA.menuName || menuName == StoreMenu.CHRISTMAS_PASTA.menuName | ||
| ) |
There was a problem hiding this comment.
이렇게 조건이 많을 경우에는,
val mainMenus = listOf(StoreMenu.T_BONE_STEAK.menuName....)
if (mainMenus.contains(menu)) {
return quantity
}
이렇게 하면 좀더 간단히 작성할 수 있어요!!
There was a problem hiding this comment.
제시해주신 코드가 확실히 개선된 코드인 거 같아요. 사실 방법을 알고도 반복되는 리팩토링 속에서 해당 부분을 놓쳣다는게 아쉬움을 많이 남겼습니다.. 깔끔한 코드와 리뷰 감사합니다 :)
| fun assignBadge(benefitAmount: Int): String { | ||
| when { | ||
| benefitAmount in STAR_MIN..STAR_MAX -> return BADGE_STAR | ||
| benefitAmount in TREE_MIN..TREE_MAX -> return BADGE_TREE | ||
| benefitAmount >= SANTA_MIN -> return BADGE_SANTA | ||
| } |
There was a problem hiding this comment.
아마 이렇게 작성하신 이유가, 그냥 >= 기호만 사용하면 STAR 배지만 return 되어서 그런거 같은데,
when {
benefitAmount >= 20000 -> retrun BADGE_SANTA
benefitAmount >= 10000 -> retrun BADGE_TREE
benefitAmount >= 5000 -> retrun BADGE_STAR
}
이렇게 거꾸로 작성하면 문제를 해결할 수 있습니다!
There was a problem hiding this comment.
아 동작 순서에 따라서 또 다르게 작성될 수 있네요. 제시 해주신 코드처럼 낮은 금액보다는 높은 금액부터 검사하는 것이 처리 과정을 확실히 줄일 수 있겠다는 생각이 들어요! 많은 부분에서 도움을 받게 된 것 같아요.. 정말 감사합니다!
|
4주간 고생 많으셨습니다! 이제야 리뷰를 남기네요 ㅠㅠ ㅡㅡㅡㅡㅡㅡㅡ 커밋 이력을 보니 오류 수정의 커밋 타입을 refactor 로 하셨는데 |
No description provided.