-
Notifications
You must be signed in to change notification settings - Fork 175
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
[Spring 경로 조회 - 2단계] 연로그(권시연) 미션 제출합니다. #258
Conversation
- data.sql 제거 - 필요한 데이터 @beforeeach를 통해 생성 - truncate를 이용한 데이터 제거 - 전체/일부 실행해도 통과할 수 있도록 리팩터링 - 하드코딩 제거 - 테스트 케이스 추가
- 변수명, 메서드명 변경 - 오류 발생하는 테스트 코드 수정
- value는 null이 될 수 없음 - 잘못된 테스트 데이터 삽입 수정
- station 개수가 아닌 section 개수로 검증 - 테스트용 데이터 삽입 시 dao 대신 service 활용
- 오류 메시지 구체화 - 오류 메시지 상수화
- 연령별 할인 정책 적용
- 테스트 케이스 추가 - PathAcceptanceTest에서 age를 할인 적용 안되는 나이로 변경
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.
연로그 몇 가지 피드백 남겼어요
수정해보시고 궁금한 점 생기면 코멘트 남기시거나 오프라인으로 찾아오셔도 좋습니다
|
||
@Before("logTarget()") | ||
public void printRequest(JoinPoint joinPoint) { | ||
logger.info("###### request ######"); |
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.
로그를 남기는 부분은 현재 두 군데가 있는데요
- API 호출 시 요청 및 응답 데이터
- API 실행 시간
2의 경우에는 생략해도 상관없지만 1의 경우에는 필요하다고 생각합니다!
Exception이 발생해 해당 내용이 로그에 찍히는 경우 어떤 데이터에 대한 요청을 받다가 에러가 발생했는지 등 로그 파일을 통한 문제 상황 파악이 쉬워진다고 생각해요~
다만 정책상 기록이 남으면 안되는 데이터가 일부 존재하는 경우에는 데이터를 암호화해서 전달하던가 AOP 적용 범위를 변경해서 따로 처리해야한다고 생각합니다
long t2 = System.currentTimeMillis(); | ||
String className = joinPoint.getSignature().getClass().getSimpleName(); | ||
String methodName = joinPoint.getSignature().getName(); | ||
logger.info(String.format("%s.%s() 실행 시간: %d mls", className, methodName, (t2 - t1))); |
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.
log로 메서드 실행 시간을 측정하는 방법도 있지만 모니터링툴을 사용하면 편리하고 미리 알림을 받게 설정할 수도 있습니다.
private static final int BASIC_FARE = 1250; | ||
private static final int EXTRA_FARE = 100; | ||
private static final int EXCLUDED_FARE = 350; | ||
private static final int DISTANCE_1 = 10; |
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.
의미 있게 구분하라
컴파일러나 인터프리터만 통과하려는 생각으로 코드를 구현하는 프로그래머는 스스로 문제를 일으킨다.
예를 들어, 동일한 범위 안에서는 다른 두 개념에 같은 이름을 사용하지 못한다.
그래서 프로그래머가 한쪽 이름을 마음대로 바꾸고픈 유혹에 빠진다.
어떤 프로그래머는 철자를 살짝 바꿨다가 나중에 철자 오류를 고치는 순간 컴파일이 불가능한 상황에 빠진다.(예를 들면 변수명을 klass로 짓는 경우)
컴파일러를 통과할지라도 연속된 숫자를 덧붙이거나 불용어(noise word)를 추가하는 방식은 적절하지 못하다.
이름이 달라야 한다면 의미도 달라져야 한다.
연속적인 숫자를 덧붙인 이름(a1, a2, ... aN)은 의도적인 이름과 정반대다.
이런 이름은 그릇된 정보를 제공하는 이름도 아니며, 아무런 정보를 제공하지 못하는 이름일 뿐이다.
저자 의도가 전혀 드러나지 않는다.
다음 코드를 살펴보자.
public static void copyChars(char a1[], char a2[]) {
for (int i = 0; i < a.length; i++) {
a2[i] = a1[i];
}
함수 인수 이름으로 source와 destination을 사용한다면 코드 읽기가 훨씬 더 쉬워진다.
불용어를 추가한 이름 역시 아무런 정보도 제공하지 못한다.
Product라는 클래스가 있다고 가정하자.
다른 클래스를 ProductInfo 혹은 ProductData라 부른다면 개념을 구분하지 않은 채 이름만 달리한 경우다.
Info나 Data는 a, an, the와 마찬가지로 의미가 불분명한 불용어다.
a나 the와 같은 접두어를 사용하지 말라는 소리가 아니다.
의미가 분명히 다르다면 사용해도 무방하다. 예를 들어, 모든 지역 변수는 a를 사용하고 모든 함수 인수는 the를 사용해도 되겠다.
요지는 zork라는 변수가 있다는 이유만으로 theZork라 이름 지어서는 안 된다는 말이다.
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.
현재 거리에 대한 조건이 더 추가될 수도 있고 제거될 수도 있어요.
5km는 어떠한 기준이고 8km는 어떤 기준이다, 딱히 설명될 수 있는 말이 없다고 생각했어요. (나이 같은 경우에는 몇 세 미만은 어린이, 몇 세 미만은 청소년 등으로 구분 가능)
1, 2, 3을 제거하자 생각하니 FIRST_DISTANCE, SECOND_DISTANCE 이런거밖에 생각 안나더라고요ㅠㅠ
아니면 Distance라는 enum을 만들어서 FIRST, SECOND 이런 값을 넣는 편이 더 좋을 것 같다는 생각도 들고요
구구라면 어떤 네이밍을 생각하셨을 것 같은지가 궁금해요!
|
||
public class FareCalculator { | ||
|
||
private static final int BASIC_FARE = 1250; |
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.
새로운 요금 체계가 추가될 때마다 상수 갯수가 점점 많아지면서 코드 관리하기가 어려워지겠네요
상수를 의미있게 나누기 위해 enum을 잘 활용해보면 어떨까요?
return calculateByAge(calculateByDistance() + extraFare); | ||
} | ||
|
||
private int calculateByAge(int fare) { |
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.
마찬가지로 새로운 요구사항이 생길때마다 if절이 점점 늘어나겠네요
변경에 유연한 구조로 바꿔보시기 바랍니다.
|
||
import java.util.List; | ||
|
||
public interface PathStrategy { |
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.
현재를 JGraphT를 사용해 구현하고 있는데 다른 라이브러리를 활용한다던가, 최단거리 말고 최소환승을 구한다던가 등 여러 방식으로 경로를 구할 수 있다고 생각했어요!
현재는 위와 같은 케이스가 없어서 오버 프로그래밍이라고 볼 수도 있을 것 같아요.
이렇게 해두면 나중에 편하지 않을까? 싶어서 미리 도전해보았습니다.
if (newSection.isSameDownStationId(section)) { | ||
throw new IllegalArgumentException(CREATE_CROSSROADS_LIST_ERROR); | ||
} | ||
validCrossPath(newSection, section); | ||
|
||
if (newSection.isSameUpStationId(section)) { |
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.
stream의 findAny를 활용하면 줄일 수 있겠네요
그전에 메서드명을 점검해보시기 바랍니다.
findUpdateWhenAdd라는 메서드명은 개발자의 언어로 쓰여있어서 무엇을 의미하는지 모르겠어요
지하철 세상에서는 이걸 뭐라고 부르나요?
그리고 주석을 제거하시고 메서드명으로 의미를 표현해보시기 바랍니다.
잘 구현된 메서드는 주석이 필요 없습니다.
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.
추가로 Sections 클래스에 모든 주석을 제거하시기 바랍니다.
throw new IllegalArgumentException(String.format(CREATE_CROSSROADS_LIST_ERROR, newSection)); | ||
} | ||
} | ||
|
||
private void validNewSection(Section section) { |
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줄이 넘어가지 않도록 구현해보시기 바랍니다.
유효성 검사는 메서드로 추출해서 어떤 작업인지 표현해주면 읽기 좋은 코드가 되겠네요
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줄이 넘는 메서드가 많이 보이네요.
어떻게하면 간결한 메서드로 만들 수 있을까요?
@@ -14,17 +14,17 @@ public class Section { | |||
private Long downStationId; |
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.
아이디 대신 station 객체로 연관관계를 만들어보면 어떨까요?
id는 Long 타입이라 변수명으로 객체 간의 관계를 유추해야되는 상황이네요
객체가 아닌 아이디로 객체를 연결하면 객체지향적이지 않은 코드입니다.
@@ -32,7 +32,7 @@ public LineDao(NamedParameterJdbcTemplate jdbcTemplate) { | |||
} | |||
|
|||
public Long save(LineCreateRequest line) { |
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 Long save(LineCreateRequest line) { | |
public Long save(Line line) { |
컨트롤러에서 받은 dto를 repository 영역까지 내리지 마세요.
repository에서 도메인 객체만 처리하도록 만들어보세요
- SQL을 N번 호출하는 대신 in 절을 활용해 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.
연로그 추가 코멘트 남겼어요
수정해보시고 궁금한 점 생기면 코멘트 남기거나 오프라인으로 질문주세요.
} | ||
|
||
public List<Section> findByLineId(Long id) { | ||
String sql = "select * from SECTION where line_id = :id"; | ||
String sql = selectSql + "and sc.line_id = :id"; |
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.
메서드마다 쿼리는 따로 작성하세요.
selectSql 변수를 수정 할 일이 생기는 순간 모든 find 메서드에 영향이 갑니다.
@@ -67,24 +79,24 @@ public Long save(Section section) { | |||
public void update(Section section) { | |||
String sql = "update SECTION set down_station_id = :downStationId, up_station_id = :upStationId, distance = :distance where id = :id"; |
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.
120칸이 넘지 않도록 작성해봅시다
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.
한 줄에 있는 코드가 너무 길어지면 sql문을 확인하는데 불편하다는 것은 인지했습니다!
그런데 120
칸인 이유가 있을까요 ?_?
현재는 where 절에서 자르는게 쿼리문 로직을 이해하기 더 쉽다 생각하여 where 부분에서 자르도록 개선했습니다!
(+)
120칸이 컨벤션이라고 다른 크루한테 들었습니다!
IntelliJ 화면의 선이 120 기준을 보여주기 위함인줄은 처음 알았네요😮
|
||
public enum Age { | ||
|
||
CHILD((age) -> age < 13, 50), |
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.
CHILD((age) -> age < 13, 50), | |
CHILD(age -> age < 13, 50), |
this.discountRate = discountRate; | ||
} | ||
|
||
public static Age findByAge(int age) { |
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 static Age findByAge(int age) { | |
public static Age valueOf(int age) { |
이펙티브 자바 참고할 것
private static final int BASIC_FARE = 1250; | ||
private static final int EXTRA_FARE = 100; | ||
|
||
private final int fareUnit; |
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.
fare, distance 라고 쓰면 충분하지 않나요?
unit이라는 단어를 사용한 이유가 뭔가요
클린코드
p32
클래스 이름
클래스 이름과 객체 이름은 명사나 명사구가 적합하다. Customer, WikiPage, Account, AddressParser 등이 좋은 예다. Manager, Processor, Data, 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.
Distance라는 클래스를 보면 '거리'와 관련된 데이터가 존재한다는 생각을 할 것 같아요
거리 안에 존재하는 데이터가 fare와 distance로 저장된다면
fare
: 해당 거리에 부과되는 요금distance
: 해당 거리의 길이
라고 생각할 것 같았어요
실제로는 요금 / 거리라는 의미가 아니라 각각 요금 계산할 때 사용되는 거리의 단위
/ 해당 요금이 적용되기 위한 거리의 최소 길이에 대한 기준
입니다
정리하고 보니 Distance
라는 이름 대신 다른 걸 지었어야 하나라는 생각이 드네요
운임 거리
를 영어로 번역하니 fare distance
라고 뜨는데 아래와 같이 변경하는 것은 어떻게 느껴지시나요?
- 클래스 이름
Distance
->FareDistance
- 필드 이름
fareUnit
->unit
- 필드 이름
distanceUnit
->standard
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.
fare 패키지 안에 존재하는 Distance니까 클래스명은 유지했습니다
리팩터링할때 추가로 필요한 필드를 생성하며 이름도 함께 변경해주었습니다
fareUnit
->unit
distanceUnit
->startPoint
- (새로 생성)
endPoint
@@ -10,26 +11,26 @@ public class Section { | |||
|
|||
private Long id; |
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.
이 부분에 대해서는 노란 전구 보고 고민하다가 그냥 뒀었는데요
일부만 final인 경우엔 나머지 final이 안붙은 필드는 변경해도 되는 값이다라고 생각할까봐 통일성을 줘야하나?라는 생각을 했어요
정리하면서 든 생각인데 일부만이라도 변경이 안된다라고 못박아두는게 더 좋은 것 같네요😅
validDistance(distance); | ||
validStations(upStationId, downStationId); | ||
validStations(upStation.getId(), downStation.getId()); |
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.
validStations 메서드는 객체지향적인가요?
@@ -45,42 +46,33 @@ private void validStations(Long upStationId, Long downStationId) { | |||
} | |||
|
|||
public void reduceDistance(Section other) { |
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.
distance - other.distance를 유효성 검사, 값을 변경하면서 두 번 계산하고 있네요.
중복 코드를 제거해보시기 바랍니다.
import java.util.List; | ||
import java.util.stream.Collectors; | ||
|
||
public enum Distance { |
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.
새로 추가한 도메인 객체들이 의도한대로 동작하는지 모두 테스트해보시기 바랍니다.
import org.junit.jupiter.params.provider.CsvSource; | ||
import wooteco.subway.domain.fare.FareCalculator; | ||
|
||
class FareCalculatorTest { |
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.
정상적인 케이스만 테스트하지 마시고 예외값을 입력하면 어떻게 동작할지 테스트하시기 바랍니다.
- 쿼리문을 상수로 두지 않고 각 메서드에서 선언 - 120칸 넘어가지 않도록 줄바꿈
- 거리가 0 이하인 경우 - 나이가 0 이하인 경우
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.
연로그 피드백 반영 잘 해주셨어요
추가로 남긴 코멘트는 다음 미션 진행하면서 적용해보시기 바랍니다
@@ -74,12 +72,15 @@ public List<Line> findAll() { | |||
return jdbcTemplate.query(sql, eventRowMapper); | |||
} | |||
|
|||
public void update(Long id, LineRequest lineRequest) { | |||
public List<Line> findByIds(List<Long> ids) { | |||
String sql = "select * from LINE where id in (:ids)"; |
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.
private Long downStationId; | ||
private final Long id; | ||
private final Long lineId; | ||
private Station upStation; |
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.
final로 만들 방법은 없을까요?
|
||
public List<Long> getSortedStationIds() { | ||
List<Long> ids = new LinkedList<>(); | ||
addUpStationIds(ids, value.get(0).getDownStation().getId()); |
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.
객체지향 생활체조
한 줄에 한 점만 찍는다.
return value.stream() | ||
.collect(Collectors.toMap(section -> section.getUpStation().getId(), | ||
section -> section.getDownStation().getId() | ||
, (section1, section2) -> section1)); |
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.
의미있는 변수명을 사용합시다.
this.value = value; | ||
} | ||
|
||
public List<Long> getSortedStationIds() { |
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.
id 기반으로 비즈니스 로직을 만들지 마시고 객체 기반으로 다시 만들어보세요
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 List<Station> getSortedStation() {
return value.stream()
.map(section -> List.of(section.getUpStation(), section.getDownStation()))
.flatMap(Collection::stream)
.collect(Collectors.toUnmodifiableList());
}
} | ||
|
||
private void addUpStationIds(List<Long> ids, Long nowId) { | ||
Map<Long, Long> sectionMap = initMapByUpStationKey(); |
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.
map을 쓰지 않고 처리할 방법은 없나요?
안녕하세요 핀!
새로운 기능 구현은 오래 걸리지 않았으나 테스트 케이스를 추가하며 많은 예외 상황이 발생해 오래 걸렸네요ㅠ.ㅠ 핀의 제안도 최대한 적용하려 했으나 어떻게 개선할지 의문인 부분은 그대로 두었습니다. (이 부분은 코멘트로 표시해두겠습니다)
큰 변경 사항
궁금한 점은 코멘트 남기겠습니다.
이번에도 잘 부탁드립니다 😊