[Refactor] 아이템 수정사항 - 미소유 아이템 카테고리별 조회, 기본 아이템 회원가입 시 기본 장착

[Jinho622]

#️⃣연관된 이슈

#236

📝작업 내용

아이템 수정사항 - 미소유 아이템 카테고리별 조회, 기본 아이템 회원가입 시 기본 장착

🔎코드 설명

코드에 대한 설명을 작성해주세요.

💬고민사항 및 리뷰 요구사항 (Optional)

x

비고 (Optional)

x

[riadan710]

기능적으로는 문제없을것 같은데, 코드상의 의문이 있어 리뷰 남겼습니다!
확인 부탁드립니다 😄

[riadan710]

3개의 쿼리 형태가 동일하고 메소드명만 다른데 굳이 이렇게 구분한 이유가 있나요?

[riadan710]

방어적인 코드인건가요?
어차피 bodypart.get으로 구분이 된다면 그냥 getShopItemByBodyPart로 통일해서 거기다 파라미터로 넣으면 알아서 쿼리에서 구분해서 넣는 것 아닌가 궁금합니다.

[riadan710]

/gemini review

[gemini-code-assist]

Code Review

이번 PR은 아이템 관련 기능을 리팩토링하여, 미소유 아이템을 신체 부위별로 조회하는 기능과 회원가입 시 기본 아이템을 장착하는 기능을 추가했습니다. 전반적으로 기능 구현의 의도는 명확하지만, 코드의 중복, 안정성, 그리고 유지보수성 측면에서 개선할 점들이 보입니다.

주요 피드백은 다음과 같습니다:

• ItemRepository에 중복된 쿼리 메서드를 하나로 통합하여 코드 중복을 줄이고 유지보수성을 높이는 것을 제안합니다.
• ItemQueryServiceImpl의 로직을 단순화하고 불필요한 코드를 제거하여 가독성과 효율성을 개선합니다.
• UserCommandServiceImpl에서 신규 회원 가입 시 기본 아이템을 조회할 때 발생할 수 있는 예외를 처리하여 안정성을 높입니다.
• 또한, 코드에 하드코딩된 '매직 넘버'를 상수로 대체하여 가독성을 향상시키는 것을 권장합니다.

자세한 내용은 각 파일의 개별 코멘트를 참고해주세요.

[gemini-code-assist]

거의 동일한 쿼리를 사용하는 getShopItemByBodyPartHead, getShopItemByBodyPartEyes, getShopItemByBodyPartTorso 세 개의 메서드가 추가되었습니다. 이는 코드 중복이며 유지보수를 어렵게 만듭니다. bodyPart를 파라미터로 받는 단일 메서드로 통합하는 것이 좋습니다. 또한, 리포지토리 메서드의 파라미터로 Optional을 직접 넘기기보다, 서비스 레이어에서 Optional의 존재 여부를 확인하고 실제 값을 전달하는 것이 더 일반적이고 안전한 방법입니다.

Suggested change

	@Query("SELECT i FROM Items i " +
	"WHERE i.bodyPart = :bodyPart " +
	"AND NOT EXISTS (SELECT o FROM OwnedItems o WHERE o.users = :user AND o.items.id = i.id)")
	List<Items> getShopItemByBodyPartHead(@Param("user") Users user, @Param("bodyPart") Optional<BodyPart> bodyPart);
	@Query("SELECT i FROM Items i " +
	"WHERE i.bodyPart = :bodyPart " +
	"AND NOT EXISTS (SELECT o FROM OwnedItems o WHERE o.users = :user AND o.items.id = i.id)")
	List<Items> getShopItemByBodyPartEyes(@Param("user") Users user, @Param("bodyPart") Optional<BodyPart> bodyPart);
	@Query("SELECT i FROM Items i " +
	"WHERE i.bodyPart = :bodyPart " +
	"AND NOT EXISTS (SELECT o FROM OwnedItems o WHERE o.users = :user AND o.items.id = i.id)")
	List<Items> getShopItemByBodyPartTorso(@Param("user") Users user, @Param("bodyPart") Optional<BodyPart> bodyPart);
	@Query("SELECT i FROM Items i " +
	"WHERE i.bodyPart = :bodyPart " +
	"AND NOT EXISTS (SELECT o FROM OwnedItems o WHERE o.users = :user AND o.items.id = i.id)")
	List<Items> getShopItemByBodyPart(@Param("user") Users user, @Param("bodyPart") BodyPart bodyPart);

[gemini-code-assist]

ItemRepository의 메서드를 하나로 통합하는 변경에 맞춰 서비스 로직을 간소화할 수 있습니다. 현재의 if-else if 구조는 복잡하며, 새로운 BodyPart enum 값이 추가될 때마다 코드를 수정해야 하는 단점이 있습니다. 특히 else if (bodyPart.get() == null) 조건은 bodyPart.isPresent()가 참인 블록 내에 있어 절대 실행될 수 없는 코드(dead code)입니다. bodyPart가 존재할 경우, 제안된 단일 리포지토리 메서드를 호출하고, 그렇지 않은 경우 전체 미소유 아이템을 조회하도록 로직을 단순화하는 것을 권장합니다.

        // bodyPart에 따른 아이템 조회
        if (bodyPart.isPresent()) {
            return itemRepository.getShopItemByBodyPart(user, bodyPart.get());
        } else {
            return itemRepository.getShopItem(user);
        }
    }

[gemini-code-assist]

Optional.get()을 직접 호출하면 itemRepository.findById(itemId)가 비어있는 Optional을 반환할 때 NoSuchElementException이 발생하여 회원가입 전체가 실패할 수 있습니다. orElseThrow()를 사용하여 아이템이 없을 경우 ITEM_NOT_FOUND와 같은 명확한 예외를 발생시키는 것이 더 안전한 방법입니다. 또한, // 머리라는 주석은 첫 번째 아이템에만 해당되어 다른 아이템 ID에 대한 정보를 주지 못해 오해의 소지가 있습니다.

Suggested change

	.items(itemRepository.findById(itemId).get()) // 머리
	.items(itemRepository.findById(itemId).orElseThrow(() -> new ExceptionHandler(ITEM_NOT_FOUND)))

[gemini-code-assist]

기본 아이템 ID 1L, 4L, 7L, 9L이 하드코딩되어 있습니다. 이러한 '매직 넘버'는 코드의 의도를 파악하기 어렵게 만들고, 향후 ID가 변경될 경우 수정이 번거로워질 수 있습니다. private static final Long DEFAULT_HEAD_ITEM_ID = 1L;과 같이 의미 있는 이름의 상수로 정의하여 가독성과 유지보수성을 높이는 것을 권장합니다.