-
Notifications
You must be signed in to change notification settings - Fork 1
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
[FEAT] user stat 결과 경험치를 %로 치환해서 응답 #90
[FEAT] user stat 결과 경험치를 %로 치환해서 응답 #90
Conversation
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.
이번주에 클린코드 읽었더니 변수이름에 초점이 많이 맞춰지네요 ㅋㅋㅋ
고생하셨습니다. 👍
@@ -56,7 +57,7 @@ public LoginRes socialLogin(SocialLoginReq socialLoginReq) { | |||
.email(socialUserRes.getEmail()) | |||
.accessToken(accessToken) | |||
.avatarId(userAvatarService.getUserAvatarId(user.getId())) | |||
.userStat(userService.getUserStat(user)) | |||
.userStat(UserStat.from(user)) |
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.
함수이름을 from이라고 지은 이유가 궁금해요!
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.
정적 팩토리 메소드로 인수인 user에서 정보를 가져와 생성한다는 의미를 담았습니다.
from은 주로 하나의 매개변수를 받을 때 사용하는 네이밍 입니다.
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.
그렇군요 👍
UserStat userStat = missionService.afterMissionComplete(userId, missionLogCreateReq); | ||
notificationService.sendNotificationToUsersInRoom(missionLogCreateReq.getAlarmId(), userId); | ||
// TODO: 비즈니스 로직 제거하기 | ||
if (Objects.nonNull(missionLogCreateReq.getRoomId())) { | ||
notificationService.sendNotificationToUsersInRoom(missionLogCreateReq.getAlarmId(), userId); | ||
} |
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.
// TODO: 비즈니스 로직 제거하기
->
저번 코드 리뷰에서 오해가 있어서 컨트롤러에 옮겼었는데
노티를 보내는 부분을 다시 afterMissionComplete에 옮기면 어떨까요? (@transactional이 걸리긴 해요)
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.
이 부분은 조금 더 알아본 후에 답장을 달겠습니다! 📌
조언 받아보고 시일 안에 업데이트 하겠습니다!!
@Transactional | ||
public UserStat afterMissionComplete(int userId, MissionLogCreateReq req) { | ||
createMissionLog(userId, req); | ||
return userService.userProgress(userId); | ||
} | ||
|
||
// TODO : 미션 수행 기록 추가 + REQUEST 수정하기 | ||
public void createMissionLog(int userId, MissionLogCreateReq req) { | ||
User user = userRepository.findById(userId) | ||
.orElseThrow(() -> new BaseException(Status.USER_NOT_FOUND)); | ||
|
||
createMissionLog(user, req); | ||
userService.userProgress(user); | ||
|
||
return UserStat.from(user); | ||
} | ||
|
||
// TODO : 미션 수행 기록 추가 + REQUEST 수정하기 | ||
public void createMissionLog(User user, MissionLogCreateReq req) { | ||
Alarm alarm = alarmRepository.findById(req.getAlarmId()) | ||
.orElseThrow(() -> new BaseException(Status.ALARM_NOT_FOUND)); | ||
|
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.
createMissionLog에 userId를 넘기던 것을 왜 user를 넘기게 수정한 것인지 궁금해요
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.
afterMissionComplete
에 user를 조회하는 코드가 이미 존재해서 코드 중복을 줄였습니다!
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.
아하 근데 이전 afterMissionComplete에 user를 조회하는 코드가 없어보여요!
(이전 코드가 더 좋다는게 아니라 궁금해서 물어보는 겁니다!)
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.
오잉 저번부터 왜 이전 코드가 전부 안 보이는지 모르겠지만 원래는 아래 코드였습니다.
// TODO : 미션 수행 기록 추가 + REQUEST 수정하기
public void createMissionLog(int userId, MissionLogCreateReq req) {
User user = userRepository.findById(userId)
.orElseThrow(() -> new BaseException(Status.USER_NOT_FOUND));
Alarm alarm = alarmRepository.findById(req.getAlarmId())
.orElseThrow(() -> new BaseException(Status.ALARM_NOT_FOUND));
Room room = Objects.isNull(req.getRoomId())
? null
: roomRepository.findById(req.getRoomId())
.orElseThrow(() -> new BaseException(Status.ROOM_NOT_FOUND));
MissionLog missionLog = MissionLog.builder()
.alarmName(alarm.getName())
.missionPicLink(req.getMissionPicLink())
.user(user)
.room(room)
.build();
missionLogRepository.save(missionLog);
}
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.
아하 근데 createMissionLog 말고 afterMissionComplete 말씀하신거
아니였나요??
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.
createMissionLog()
에도 user를 조회하는 로직이 있었는데 afterMissionComplte()
에서 공통으로 조회하고 createMissionLog()
에서 중복되는 로직이 빠졌습니다.
@@ -67,7 +67,7 @@ public class User { | |||
private Integer level; | |||
|
|||
@Column(name = "experience") | |||
private Integer experience; | |||
private Integer expPoint; |
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 : 아래도 포인트가 있어서 헷갈릴 것같아요!
혹시 다른 후보가 뭐가 있었나요??
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.
아하 int로 저장한거 좋은것같아요!
근데 point가 이미 있어서 exPoint하고 좀 헷갈리는것 같기도 하네요!
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.
오타이실거라고 생각하지만, exPoint는 아니고 expPoint입니다.
exp는 게임에서 주로 사용하는 줄임말로 experience의 약자입니다.
많이 헷갈리신다면 point를 골드나 투게덥만의 화폐 이름으로 바꿔도 괜찮을 것 같습니다.
만약 생각하시는 변수명 예시가 있다면 추천해주시면 감사하겠습니다.
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.
아 네네 잘못적었네요
그냥 경험옆에 양을 의미하는 변수를 추가하는거 좋은 것같아요👍
포인트를 말씀하신대로 바꿔도 좋고 (디자인에 포인트 부분에 코인그림이 있으니까 골드도 좋은것같아요!) experienceAmount 도 생각나네요!
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.
https://en.wikipedia.org/wiki/Experience_point
exp Point 는 보통 게임에서 자주 사용되는 도메인 용어로 해당 코드에서도 사용되는게 적합한 것 같습니다.
대신 추후에 point를 다른 이름으로 변경하겠습니다!
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.
좋아요 👍 👍
public boolean checkUserLevelUpAvailable(int threshold) { | ||
return this.getExperience() >= threshold; | ||
return this.getExpPoint() >= threshold; | ||
} |
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.
함수명처럼 user의 levelup이 가능한지 판단하는 함수입니다!
혹시 어떤 부분이 이해하기 어려웠는지 알려주실 수 있나요?
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.
available 뜻때문에 헷갈렸어요!
private final int point; | ||
|
||
private UserStat(User user) { | ||
this.level = user.getLevel(); | ||
this.expPercentage = user.calculateExpPercentage(); | ||
this.point = user.getPoint(); | ||
} | ||
|
||
public static UserStat from(User user) { | ||
return new UserStat(user); | ||
} |
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.
👍
public int calculateLevelUpThreshold() { | ||
return 10 + 16 * (level - 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.
지금 위에 코드보니까 경험치가 10씩 증가하는데
임계치는 10 , 26, 42, 58 ,,, 식으로 증가하네요
100,150,200 이런식으로 딱 떨어지게 할 수도 있는데
임계치를 이렇게 설계하신 이유가 있는지 궁금해요!
(혹은 나중에 정책이 바뀔수도 있으니까 일반적인 경우로 설정하신건지 궁금하네요!)
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.
딱 떨어지면 경험치 바가 항상 몇 등분으로 나눠지는게 흥미롭지 않아 보여서 일부러 16을 더했습니다.
threshold와 획득하는 경험치는 몇 번의 테스트를 해본 결과 적당하다고 생각하는 수치로 정했습니다.
ex) 1레벨은 1번하면 레벨업
2레벨은 2번하면 레벨업
...
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.
아하 그랬군요
이 정책이 최종발표때 급하게 만들어져서 공유가 안된 것 같아요.
(혹시 다른 팀원하고이야기 했었었나요??)
이번주 회의때 같이 이야기해 보면 좋을 것 같습니다!
예시처럼 빨리 레벨업 되는게 적당해서 좋은 것 같아요 👍
- experience -> exp_point - point -> coin
- 메서드, DTO 이름 수정 - 알림 전송 메서드를 `afterMissionComplete` 내부로 이동 - try-catch 문으로 알림 전송에 실패했을 때 예외처리
public AlarmType determineAlarmType() { | ||
return Objects.isNull(room) ? AlarmType.PERSONAL : AlarmType.GROUP; | ||
return isRoomAlarm() ? AlarmType.GROUP : AlarmType.PERSONAL; | ||
} |
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.
👍 👍
// TODO: 디버그용 삭제 | ||
System.out.println(); | ||
System.out.println("탐지할 객체 = " + object); | ||
System.out.println("[감지된 객체]"); | ||
log.info("탐지할 객체 = " + object); | ||
log.info("[감지된 객체]"); |
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.
훨씬 좋네요!
.experience(DEFAULT_USER_EXPERIENCE) | ||
.point(DEFAULT_USER_POINT) | ||
.expPoint(DEFAULT_USER_EXPERIENCE) | ||
.coin(DEFAULT_USER_POINT) |
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.
DEFAULT_USER_POINT -> DEFAULT_USER_COIN 부탁해요!
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.
네 고치고 머지하겠습니다!
☀️ 작업 사항
☀️ 참고 사항