[부나] 2단계 재화 미션 제출합니다. #26

tmdgh1592 · 2023-06-02T16:06:07Z

안녕하세요 벅벅!
벌써 우테코 마지막 미션이네요 🥹

재화 관련 요소는 포인트 하나만을 추가하였습니다.

Json Converter로 kotlinx-serialization 을 사용하였습니다.
기존에는 Gson을 사용하였는데, Gson은 Java 언어로 Retrofit을 사용할 때 최적화되어 있어서 아래와 같은 문제점이 있다고 판단하였습니다.

Not-Null 프로퍼티에 null이 들어갈 수 있는 문제점

Default 값을 설정할 수 없던 문제점

피드백 잘 부탁드립니다! 🙏

…er에서 RecyclerViewAdapter로 변경

…pter로 변경

BeokBeok

쇼핑 주문 2단계 미션하시느라 고생하셨습니다. 👍
고민해볼만한 의견들을 코맨트로 작성하였으니, 충분히 고민해보시고 도전해보세요. 💪

BeokBeok · 2023-06-03T14:11:00Z

app/src/main/java/woowacourse/shopping/data/dto/CartDto.kt

+
+@Serializable
+data class CartGetResponse(
+    @SerialName("id")


SerialName 어노테이션을 붙이는 이유는 무엇일까요?
만약 SerialName 어노테이션을 붙이지 않았을 경우에는 어떤 일이 발생할까요?

Gson의 @SerializedName 어노테이션과 동일하게,
@SerialName 어노테이션을 붙이지 않아도 property명과 response body 각각의 json key 이름이 동일하면 문제가 없는 것으로 알고 있습니다 : )

어노테이션을 사용한 이유는,
미션을 진행하면서 서버에서 제공하는 json의 key값과 클라이언트에서 사용하고자 하는 이름이 다른 경우가 있었습니다.

@SerialName("point") val usedPoint: Int

이 외에도, 클라이언트에서는 그대로 프로퍼티명을 유지하고 싶은데,
서버에서 제공해주는 key 이름만 변경되는 상황에는 @SerialName("...")만 변경하여 코드의 수정을 최소화하고자 하였습니다.

잘 알고 계시네요. 👍
추가로 "난독화" 관련해서도 알아보시면 도움이 될 거에요.

BeokBeok · 2023-06-03T14:13:17Z

app/src/main/java/woowacourse/shopping/data/repository/CartRepositoryImpl.kt

+        getAllCartProducts(
+            onSuccess = { cartProducts ->
+                val cartProduct = cartProducts.find { it.product.id == productId } ?: run {
+                    onFailed(IllegalStateException("해당 상품이 존재하지 않습니다."))


"해당 상품이 존재하지 않습니다." 의 문구는 View에서 보여주기 위한 메시지인데요.
CartRepository에서 이 메시지를 전달하는 것이 적합할까요?
CartRepository에서 에러를 어떻게 보내주고 이 것을 어떻게 View에서 보여주면 좋을 지 고민해보면 어떨까요?

확실히 위와 같은 코드는 관심사 분리가 되지 않아 보이기도 하고, 다국어 지원을 한다고 가정했을 때 변경이 없어야 하는 Repository까지 수정이 필요해 보입니다!
에러 코드에 따라 적절한 분기 처리를 하는 것이 좋아보이지만, 우선은 서버와 통신이 실패했다는 것을 presenter에게 알려주고 view에게 적절한 메시지를 보여주도록 구현해보겠습니다 😃

BeokBeok · 2023-06-03T14:18:46Z

app/src/main/java/woowacourse/shopping/data/repository/CartRepositoryImpl.kt

+        service.saveCartProduct(requestBody = CartAddRequest(productId))
+            .enqueue(object : Callback<Unit> {
+                override fun onResponse(call: Call<Unit>, response: Response<Unit>) {}
+                override fun onFailure(call: Call<Unit>, throwable: Throwable) {}


리턴 타입이 Unit인 경우에 onResponse와 onFailure의 구현부를 비워두셨네요.
만약 카트 상품을 저장하던 중 에러가 발생하게 되었을 때(가령, 네트워크 오류) 사용자에게 알려주려면 어떻게 해야 할까요? (이하 관련 내용 동일)

서버와 통신 과정에서 에러가 발생하면 좋지 않은 사용자 경험을 제공해줄 것 같습니다.
리턴 타입이 Unit인 경우에도, 오류가 발생하면 에러 메시지를 보여주도록 변경하겠습니다 😃

BeokBeok · 2023-06-03T14:22:01Z

app/src/main/java/woowacourse/shopping/data/repository/CartRepositoryImpl.kt

+            },
+            onFailed = {
+                saveCartProductByProductId(productId)
+                findCartProductByProductId(


increaseProductCountByProductId 함수 호출 시, 실패를 하게 되면 findCartProductByProductId 함수를 호출하도록 되어 있네요.
이렇게 재귀로 호출하게 되는 구조에서는, API 호출 실패 시 서버에 API를 계속적으로 호출하게 되면서 서버에 부하를 주는 구조가 됩니다.
서버에 부하가 걸리지 않도록 개선해보면 어떨까요?

하나의 통신에도 서버와 통신이 여러번 이루어지면서 부하가 발생할 수 있겠군요!
save 메서드의 결과로 cartItemId를 반환받는데, 그 값으로 카운트를 업데이트하는 구조로 만들어보겠습니다 : )

BeokBeok · 2023-06-03T14:24:53Z

app/src/main/java/woowacourse/shopping/data/util/RetrofitServiceCreater.kt

+    ignoreUnknownKeys = true
+    coerceInputValues = true
+    encodeDefaults = true
+    isLenient = true


각 옵션들이 반드시 필요한 옵션들일까요?

지금 당장은 필요없지만, 이후 상황을 가정하여 추가하였습니다.
현재 역/직렬화시 default값을 사용하기 위한 옵션인
encodeDefaults, coerceInputValues만 사용하도록 변경하겠습니다 : )

이후 상황을 가정하여 추가하는 것은 반드시 지양하셔야합니다.
이런 코드들이 레거시가 되서, 결국은 지우지도 못하고 방치되는 코드가 되기 쉽습니다. 🙏

BeokBeok · 2023-06-03T14:27:03Z

app/src/main/java/woowacourse/shopping/model/PageModel.kt

+    val hasPrevious: Boolean,
+    val hasNext: Boolean,
+) {
+    fun toText(): String = value.toString()


toText 함수를 잘 구현해주셨네요.
지금도 좋지만, toString 함수를 오버라이딩해서 구현할 수도 있을 것 같네요.

생각해보니 toString()을 사용하는 방법도 있었네요 😃
바로 반영하였습니다. 알려주셔서 감사합니다!

BeokBeok · 2023-06-03T14:31:00Z

앱을 실행하면 "후추 서버"에서는 컨텐츠가 제대로 나오지 않으며, "하마드 서버"에서는 Crash가 발생하고 있습니다.
앱을 삭제하고 설치해보시면 확인해보실 수 있을거에요.
앱이 실행이 되지 않아 코드 관점에서만 리뷰를 드렸습니다.
앱이 정상적으로 실행될 수 있도록 수정해주세요 🙏

tmdgh1592 · 2023-06-03T15:04:01Z

미션 제출 후에 서버 API 스펙이 변경된 점이 있어서 Crash가 발생하고 있습니다 🥲
다음 제출에 피드백 반영과 함께 Crash 해결 후 리뷰 요청 드리겠습니다 : )

- isLenient - ignoreUnknownKeys

BeokBeok

피드백 반영하시느라 고생하셨습니다. 👍
이전과 동일하게 crash가 발생하고 있습니다. 😭
해결하신 후에 다시 리뷰 요청주세요. 🙏

BeokBeok · 2023-06-05T14:36:12Z

Json Converter로 kotlinx-serialization 을 사용하였습니다.

사용하신 이유를 잘 설명해주셨네요. 👍
그렇다면, jackson과 moshi를 사용하지 않는 이유에 대해서 궁금해지는군요. 😄

- API 31 미만에서는 크래시 발생

tmdgh1592 · 2023-06-05T15:05:19Z

크래시 해결 후 다시 리뷰 요청하였습니다!
번거롭게 해드려 죄송합니다..! 🙏

BeokBeok

피드백 반영하시느라 고생하셨습니다. 👍
좋은 개발자가 되기 위한 소중한 경험이 되었길 바랍니다. 💪

BeokBeok · 2023-06-05T15:15:26Z

app/src/main/java/woowacourse/shopping/data/repository/CartRepositoryImpl.kt

@@ -37,17 +37,17 @@ class CartRepositoryImpl(private val service: CartService) : CartRepository {
    override fun findCartProductByProductId(
        productId: ProductId,
        onSuccess: (CartProduct) -> Unit,
-        onFailed: (Throwable) -> Unit,
+        onFailed: () -> Unit,


가급적이면 실패했을 경우에는 Throwable을 전달해주는 것이 좋습니다.
서버 통신 중에 네트워크 혹은 인증 에러와 같이 예상치 못한 에러가 전달되기 때문에, 이 부분에 대해서 대응을 해주는 것이 좋습니다.
특히 onFailed에서 Throwable을 전달하지 않는다면, 어떤 에러인지 알 수 없기 때문에 사용자에게 상황에 맞는 에러 메시지를 노출하기가 어려워집니다.

Callback 대신 Future와, Result를 사용하도록 리팩토링해보겠습니다!
그 과정에서 �피드백 해주신 것처럼, 에러 상황에 대한 처리를 할 수 있도록 구현해보겠습니다 : )

BeokBeok · 2023-06-05T15:16:49Z

app/src/main/java/woowacourse/shopping/data/repository/DefaultCartRepository.kt

@@ -14,7 +14,7 @@ import woowacourse.shopping.domain.repository.CartProductId
 import woowacourse.shopping.domain.repository.CartRepository
 import woowacourse.shopping.domain.repository.ProductId

-class CartRepositoryImpl(private val service: CartService) : CartRepository {
+class DefaultCartRepository(private val service: CartService) : CartRepository {


네이밍을 바꾸는 시도 좋습니다. 👍
다만, Default를 prefix로 넣게 됨으로써 오히려 매번 Default 문구를 추가해줘야하는 불편함이 있는지 고민해보셨으면 좋겠습니다.

Impl 라는 suffix를 사용하면 CartRepository를 구현하는 서브 클래스가 하나 더 추가되었을 때 네이밍을 결정하기 어려워질 것이라고 판단하였습니다.

A : CartRepository <- CartRepositoryImpl B : CartRepository <- CartRepositoryImpl2 (..?)

따라서, 기본적으로 사용하기 위한 Repository라고 의미를 부여하여 Default prefix를 사용하였고, 이후 CartRepository를 구현하는 또 다른 서브 클래스가 생긴다면 네이밍을 선택하기 더 유연할 것이라고 생각합니다 : )

Default보다 다른 네이밍을 고민해보시는 것을 권장해요.
결국 Default를 붙이더라도 결국은 Impl을 붙이던 것과 동일하게 상용구를 붙이기 때문이에요.

DefaultCartRepository같은 경우에는 ShoppingCartRepository라고 작성해도 괜찮을 것 같네요.

인터페이스 네이밍 관련 컨벤션 아티클을 공유해드릴게요.
ref. https://androiddev.blog/interface-naming-conventions/

BeokBeok · 2023-06-05T15:19:23Z

app/src/main/java/woowacourse/shopping/data/repository/DefaultCartRepository.kt

+            ) {
+                Log.d("buna", response.code().toString())
+                if (response.isSuccessful && response.body() != null) {
+                    onSuccess(response.body()!!.toDomain())


위의 if문에서 response.body가 널이 아님을 체크했는데도 non-null assertion(!!)을 왜 사용해야하는지 궁금하지 않으신가요? 😄

response.body() 메서드가 반환하는 타입을 살펴보면 원인을 알 수 있습니다.

public @Nullable T body() { return body; }

response.body() 메서드는 Nullable한 타입을 반환하고 있습니다.
body 필드가 final이기 때문에, response.body()에 대해 null 체크를 한 번만 해주면 개발자는 이 값이 null이 아님을 알 수 있습니다.
하지만 컴파일러는 response.body() 내부에서 어떤 값을 반환하는데, 이 값이 Nullable하다 라는 정보만으로 검사하기 때문에 !!을 사용해주어야 합니다.

위 방법을 피하고 싶다면, 아래와 같이 코드를 작성해볼 수도 있을 것 같습니다.

val body = response.body() if (response.isSuccessful && body!= null) { onSuccess(body.toDomain())

잘 이해하고 계시네요. 👍
이런 경우에는 scope function (apply, also, let, with, run 등)을 활용하기도 합니다.

BeokBeok · 2023-06-05T15:23:07Z

app/src/main/java/woowacourse/shopping/util/collection/DistinctList.kt

@@ -0,0 +1,21 @@
+package woowacourse.shopping.util.collection
+
+class DistinctList<E> : ArrayList<E>() {


중복으로 추가되지 않도록 하는 리스트를 구현해주셨네요.
잘 구현해주셨지만, 가급적이면 좀 더 검증된 함수 혹은 자료형을 활용해보면 어떨까요?

List의 함수 중에서는 distinct 함수를 제공하고 있어요.
이 함수를 사용하시거나, Set을 사용하셔도 됩니다.
실제로 distinct 함수 내부를 보면, Set으로 변환했다가 다시 List로 변환하고 있어요.

public fun <T> Iterable<T>.distinct(): List<T> { return this.toMutableSet().toList() }

매번 distinct를 사용한다는 것이 불편하게 느껴져서 DistinctList 자료구조를 임의로 만들어 보았습니다!
하지만 말씀하신 것처럼 이미 검증된 함수를 사용하는 것이 더 오류를 줄이는 데 도움이 될 것이라는 생각이 듭니다 😃
고민을 해보았는데, 일반적인 Set은 순서를 보장하지 않기 때문에 LinkedHastSet 자료구조를 사용하도록 하였습니다!

tmdgh1592 added 30 commits May 26, 2023 12:04

feat: 하마드 서버 base url 추가

a7a3ad9

feat(Page): 공통 코드를 Page 클래스로 이동

b100514

docs: 주문 기능 구현 기능 목록 작성

79a07c9

chore: Retrofit 라이브러리 추가

8b7cc29

feat: RetrofitServiceCreater 구현

4e05fce

refactor: OkHttpClient를 Retrofit으로 리팩터링

ffbf8c3

feat: 상품 주문하기 ui 구현

708d742

feat: 주문할 상품을 보여주는 화면 구현

9ad002d

feat: PointEditText 커스텀뷰 구현

491ff6a

feat: 장바구니 아이템 주문하는 기능 구현

b2de7d3

feat: 쇼핑 목록 화면으로 이동하는 기능 구현

902b7c9

feat: 주문 목록을 불러오는 기능 구현

4f6292c

feat: 상세 주문 내역을 보여주는 기능 구현

521ed80

fix: 상품 주문시, 상품 id와 수량을 서버에 보내는 대신 cartItemId를 전달하도록 변경

db6e20d

refactor: RecyclerViewBindingAdapter의 setAdapter 메서드의 인자를 ConcatAdapt…

cc52965

…er에서 RecyclerViewAdapter로 변경

refactor: ORDER_DETAIL과 ORDER 뷰타입의 value를 OrderProduct에서 관리하도록 변경

9b4c856

refactor: cartItemIds를 orderItems로 변경

c9babbe

chore: kotlinx.serialization 라이브러리 추가

4c4311b

refactor: Dto에 Serializable 어노테이션 추가

d7e8da7

feat: 각 화면별로 뒤로가기 기능 구현

4054061

refactor: 라인 포맷팅

558c694

refactor: isolatedViewTypeConcatAdapter을 notIsolatedViewTypeConcatAda…

4b6cfed

…pter로 변경

feat: 스낵바를 보여주는 확장함수 구현

8a05f31

refactor: 상품 주문 완료시 스낵바로 메시지를 보여주도록 변경

15ef714

refactor: 사용하지 않는 클래스, 메서드 제거

87709b2

refactor: ShoppingDatabase를 싱클톤으로 변경

9685128

refactor: inject 메서드명을 명시적으로 변경

c68c1f1

refactor: ui의 CartProduct typealias를 제거하고 Model suffix를 붙이도록 변경

471bf9a

refactor: ui의 OrderModel typealias를 제거하고 Model suffix를 붙이도록 변경

05e244c

refactor: ui의 OrderProductModel typealias를 제거하고 Model suffix를 붙이도록 변경

3f9f6e8

tmdgh1592 added 7 commits June 3, 2023 00:44

fix: gson 라이브러리 import 제거

ce80e22

fix: 주문 상세 리사이클러뷰에서 아이템이 보여지지 않는 버그 수정

ccf431b

feat: 주문 상세 아이템의 상품 이미지에 scaleType(centerCrop) 적용

a9a1cbf

fix: 결제 아이템 뷰의 높이를 wrap_content로 변경

02836bc

fix: CounterView의 숫자를 클릭하면 뒤의 배경이 클릭되는 버그 수정

e1fded7

refactor: Log 출력 제거

23e0295

refactor: 코드 포맷팅

6deb885

BeokBeok requested changes Jun 3, 2023

View reviewed changes

tmdgh1592 added 14 commits June 4, 2023 18:40

fix: 토큰 설정이 안 되었던 버그 수정

7b5f9a4

refactor: 주문 목록 가져오는 api body 형식 변경에 따른 dto 변경

a59cc1b

feat: 필요 없는 않는 json 변환 옵션 제거

5f2fb98

- isLenient - ignoreUnknownKeys

refactor: toText() 시그니처 대신 toString()을 오버라이드하여 사용하도록 변경

f2f6a9b

refactor: 제품 가져오기 페이지네이션 api 적용

4497d33

refactor: 주문 목록 api 변경에 따른 스펙 변경

a70f99e

refactor: findCartProductByProductId의 실패 콜백 람다의 인자 변경

0bc40f5

refactor: 장바구니에 담긴 제품 개수를 매번 서버에 요청하도록 변경

f2bd29c

refactor: Repository 네이밍의 Impl을 Default로 변경

a82f66f

feat: 주문 목록 페이징 구현

5c9cea7

fix: 쇼핑 리스트 페이지네이션이 동작하지 않는 버그 수정

1e09e7a

refactor: 주문 목록 더 보기 기능에서 take 함수를 호출하지 않도록 변경

1a46c16

fix: 최근 상품 목록에서 장바구니 개수가 담기지 않는 버그 수정

1a2b2ff

fix: 최근 상품 디테일 화면에서 상품 개수를 더해지지 않는 버그 수정

39f71be

BeokBeok requested changes Jun 5, 2023

View reviewed changes

fix: 커스텀뷰에서 TypedArray 인스턴스에 use 사용를 사용하는 대신 recycle() 메서드를 호출하도록 변경

d9c98c2

- API 31 미만에서는 크래시 발생

BeokBeok approved these changes Jun 5, 2023

View reviewed changes

BeokBeok merged commit c08795c into woowacourse:tmdgh1592 Jun 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[부나] 2단계 재화 미션 제출합니다. #26

[부나] 2단계 재화 미션 제출합니다. #26

tmdgh1592 commented Jun 2, 2023

BeokBeok left a comment

BeokBeok Jun 3, 2023

tmdgh1592 Jun 4, 2023

BeokBeok Jun 5, 2023

BeokBeok Jun 3, 2023

tmdgh1592 Jun 4, 2023

BeokBeok Jun 3, 2023

tmdgh1592 Jun 4, 2023

BeokBeok Jun 3, 2023

tmdgh1592 Jun 5, 2023

BeokBeok Jun 3, 2023

tmdgh1592 Jun 4, 2023

BeokBeok Jun 5, 2023

BeokBeok Jun 3, 2023

tmdgh1592 Jun 4, 2023

BeokBeok commented Jun 3, 2023 •

edited

Loading

tmdgh1592 commented Jun 3, 2023 •

edited

Loading

BeokBeok left a comment

BeokBeok commented Jun 5, 2023

tmdgh1592 commented Jun 5, 2023

BeokBeok left a comment

BeokBeok Jun 5, 2023

tmdgh1592 Jun 6, 2023

BeokBeok Jun 5, 2023

tmdgh1592 Jun 6, 2023

BeokBeok Jun 6, 2023

BeokBeok Jun 5, 2023

tmdgh1592 Jun 6, 2023 •

edited

Loading

BeokBeok Jun 6, 2023

BeokBeok Jun 5, 2023

tmdgh1592 Jun 6, 2023

		@@ -0,0 +1,21 @@
		package woowacourse.shopping.util.collection

		class DistinctList<E> : ArrayList<E>() {

[부나] 2단계 재화 미션 제출합니다. #26

[부나] 2단계 재화 미션 제출합니다. #26

Conversation

tmdgh1592 commented Jun 2, 2023

BeokBeok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BeokBeok commented Jun 3, 2023 • edited Loading

tmdgh1592 commented Jun 3, 2023 • edited Loading

BeokBeok left a comment

Choose a reason for hiding this comment

BeokBeok commented Jun 5, 2023

tmdgh1592 commented Jun 5, 2023

BeokBeok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmdgh1592 Jun 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BeokBeok commented Jun 3, 2023 •

edited

Loading

tmdgh1592 commented Jun 3, 2023 •

edited

Loading

tmdgh1592 Jun 6, 2023 •

edited

Loading