Skip to content
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

[스프링 - 협업 미션] 다니(이다은) 미션 제출합니다. #22

Merged
merged 69 commits into from
Jun 11, 2021

Conversation

da-nyee
Copy link

@da-nyee da-nyee commented May 29, 2021

안녕하세요, 데이브! 다니입니다 🙌

이번 미션은 프론트엔드 크루들 @Puterism, @dudtjr913 과 협업해서 더 새롭고 재밌는 경험이었어요!
그리고 페어 @binghe819@Gomding, @thisisyoungbin 와 함께 4인으로 진행해서 많은 의견을 주고받고 배울 수 있었습니다 👍

미션을 시작하기 앞서 API 명세를 gist에 정리했고, 미션을 완료하고 API 문서를 Swagger로 만들었어요.
지하철 도메인은 여기서, 지하철 사이트는 여기서 확인하실 수 있습니다!

미션을 하면서 궁금한 점이 하나 생겼어요 !

궁금한 점

4단계 요구사항에 따라 경로 조회를 회원/비회원으로 분리해야 됐어요.
이때, 동일한 URI를 사용하기 때문에 어떻게 나눠야 할 지 고민이 많았는데요.
결국 LoginMember에 guest 더미 데이터를 만들어서 비회원이면 해당 데이터를 반환하게 구현했습니다.

이 방식이 좋은 방법인지 잘 모르겠어요..!
데이브는 이런 경우 어떻게 하시나요??

마지막 미션 코드 리뷰 잘 부탁드립니다 ✨

귀한 시간 내주셔서 감사합니다 🙇‍♀️❣️

da-nyee added 30 commits May 21, 2021 14:20
@da-nyee da-nyee changed the base branch from master to da-nyee May 29, 2021 08:10
Copy link

@dave915 dave915 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다니 안녕하세요! 리뷰가 늦었네요 🙇‍♂️
미션 구현 잘 해주셨네요!
질문에 관한 코멘트와 몇가지 피드백 남겼습니다.
궁금한점 있으면 언제든지 질문 주세요 :)

@@ -14,6 +14,10 @@ public LoginMember(Long id, String email, Integer age) {
this.age = age;
}

public static LoginMember guest() {
return new LoginMember(0L, "[email protected]", 20);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

더미 데이터를 넣기 보단.. null로 주거나 GeustMember 객체를 만들어서 isGuest() 메소드를 true로 주는 방법은 어떠신가요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 37 to 42
if (distance <= 10) {
return 0;
}
if (distance <= 50) {
return calculateAdditionalFareByDistanceUnderFifty(distance);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

거리당 금액 측정 정책이 늘어나면 if가 계속 늘어나지 않을까요?
enum을 이용하거나 정책을 추상화 해보면 좋을 것 같아요 :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분을 어떻게 리팩토링 할까 고민하다 책임 연쇄 패턴을 적용해봤어요!
처음 보는 디자인 패턴이어서 제대로 적용한 건지 모르겠습니다...!
거리별 요금을 잘 추상화한 게 맞나요?!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 질문이 하나 더 있습니다!
현재 FareTable이 FareChain을 의존하게 구현했는데요, 지금 구조가 괜찮은 것인지 궁금합니다...!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

음.. FareChain같은 경우엔 요금 정책이기 때문에 한번만 생성되도 될 것 같은데 다른 방법은 없을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@EnableSwagger2
public class SwaggerConfig {
@Bean
public Docket api() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DM에서 드렸던 피드백 기록차 남겨둘게요!

로그인 제한이 걸려있는 api 는 swagger로 어떻게 요청 하셨나요?
swagger에서 jwt토큰을 추가해서 요청하도록 할 수 있어요. 아래 링크 참고해보세요!
https://kimyhcj.tistory.com/363

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런 방법이 있는지 몰랐어요 !
알려주셔서 감사합니다 👍

}

private int calculateFareByDistanceAndLine(int distance, List<SectionEdge> sectionEdges) {
int fare = 1_250;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1_250 이 기본 요금인걸 표시해주면 좋을 것 같아요!

Comment on lines +7 to +10
ADULT("어른", 0, 1.0, (age) -> age >= 19),
TEENAGER("청소년", 350, 0.8, (age) -> age >= 13 && age < 19),
CHILD("어린이", 350, 0.5, (age) -> age >= 6 && age < 13),
INFANT("유아", 0, 0, (age) -> age < 6);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
거리 정책도 이와 비슷하게 할 수 있을 것 같아요!

@@ -0,0 +1,19 @@
spring:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다른 크루들은 빌드 스크립트 파일이 있어서 거기다 남겼는데, 다니는 없어서 여기다 남길게요!
혹시 jar파일 실행할때 로그파일을 만들어 주기 위해 1>log.out 2>&1 이런 식으로 실행 시켜주지 않았나요?
logback fileappender 한번 찾아보면 좋을 것 같아요 :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런식으로 logback 프레임워크를 사용하면 되는 건가요?!

@da-nyee
Copy link
Author

da-nyee commented Jun 3, 2021

📚 다니의 학습 로그

[Linux] Commands - 3

내용

태그

{Linux}, {netstat}, {pidof}, {Commands}

@da-nyee
Copy link
Author

da-nyee commented Jun 7, 2021

안녕하세요, 데이브! 다니입니다 🙌

이것저것 공부하다 보니 재 PR이 늦어졌습니다.. 💧

코드 리뷰 감사합니다 ~!
피드백 바탕으로 리팩토링 진행했습니다 !

리팩토링 하다 궁금해진 점들은 👀 이모지와 함께 코멘트로 남겨뒀어요.
확인해주시면 감사하겠습니다 !

이번에도 귀한 시간 내주셔서 감사합니다 🙇‍♀️❣️

@da-nyee
Copy link
Author

da-nyee commented Jun 7, 2021

📚 다니의 학습 로그

[Network] OSI Model: OSI 7 Layers - 4

내용

태그

{Network}, {OSI Model}, {OSI 7 Layers}, {L2}, {Data Link Layer}, {L4}, {Transport Layer}, {Error Handling}

@@ -5,4 +5,19 @@
</encoder>
</appender>
<appender-ref ref="STDOUT" />

<appender name="FILE" class="ch.qos.logback.core.rolling.RollingFileAppender">
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

적용 잘 해주셨네요 👍
logback-access.xml 은 access log 만 남고 보통 어플리케이션 로그도 따로 남겨줘야해요!
어플리케이션 로그는 logback.xml로 설정해요.
아래처럼 해보세요! logHome 은 수정해주세요!

<configuration debug="true">
    <property name="logHome" value="/atdd-subway-fare/log" />

    <appender name="STDOUT" class="ch.qos.logback.core.ConsoleAppender">
        <encoder class="ch.qos.logback.classic.encoder.PatternLayoutEncoder">
            <pattern>%d{yyyy-MM-dd HH:mm:ss.SSS} [%thread] %-5level %logger{50} - %msg%n</pattern>
        </encoder>
    </appender>

    <appender name="fileAppender" class="ch.qos.logback.core.rolling.RollingFileAppender">
        <filter class="ch.qos.logback.classic.filter.ThresholdFilter">
            <level>DEBUG</level>
        </filter>
        <rollingPolicy class="ch.qos.logback.core.rolling.TimeBasedRollingPolicy">
            <fileNamePattern>${logHome}/%d{yyyyMMdd}/subway.%d{yyyyMMdd}.%i.log</fileNamePattern>
            <timeBasedFileNamingAndTriggeringPolicy class="ch.qos.logback.core.rolling.SizeAndTimeBasedFNATP">
                <maxFileSize>15MB</maxFileSize>
            </timeBasedFileNamingAndTriggeringPolicy>
        </rollingPolicy>
        <encoder>
            <Pattern>
                %d{yyyy-MM-dd HH:mm:ss.SSS} %thread %-5level %logger - %m%n
            </Pattern>
        </encoder>
    </appender>

    <root level="info">
        <appender-ref ref="STDOUT" />
        <appender-ref ref="fileAppender" />
    </root>

</configuration>

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

어플리케이션 로그는 따로 설정해줘야 하는지 몰랐어요!
알려주셔서 감사합니다 ㅎㅎㅎ 👍

Comment on lines +29 to +30
.securitySchemes(Lists.newArrayList(apiKey()))
.securityContexts(Lists.newArrayList(securityContext()));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


@Override
public int calculate(int distance) {
return (int) ((Math.ceil(((distance - BOUND) - 1) / OVER_DISTANCE) + 1) * EXTRA_FARE);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

지금 같이 calculate에서 본인 계산만 하는걸 보면 ThirdBoundChain객체가 본인이 마지막 체인이라는 것을 알고 있는것 같네요 🤔
만약 네번째 조건이 추가되면 이 라인에도 코드 수정이 필요할 것 같아요.

개인적으로는 책임연쇄 패턴 보단 한눈에 여러 Bound 조건을 알수 있는 Enum을 적용하는게 더 좋을 것 같은데, 다니의 의견이 궁금하네요 !

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

새로운 조건이 추가될 수 있다는 걸 생각하지 못했어요...!

책임 연쇄 패턴을 적용하면 네번째 조건이 생겼을 때,
ThirdBoundChain 클래스를 수정하고 FourthBoundChain 클래스를 생성해야 할 것 같아요.
한편, enum을 사용하면 해당 클래스에만 코드 한 줄을 추가하면 될 거 같아요!

그래서 데이브가 제안해주신 것처럼 책임 연쇄 패턴 대신 enum을 활용하도록 변경했습니다 !

Comment on lines 37 to 42
if (distance <= 10) {
return 0;
}
if (distance <= 50) {
return calculateAdditionalFareByDistanceUnderFifty(distance);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

음.. FareChain같은 경우엔 요금 정책이기 때문에 한번만 생성되도 될 것 같은데 다른 방법은 없을까요?

Copy link

@dave915 dave915 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다니 안녕하세요!
피드백 반영한 부분 확인했어요 👍
몇가지 피드백 남겼어요! 확인 부탁드릴게요 :)
궁금한점 있으면 언제든 질문 주세요!

@da-nyee
Copy link
Author

da-nyee commented Jun 9, 2021

안녕하세요, 데이브!

남겨주신 피드백 바탕으로 리팩토링 진행했습니다 !
이번에도 잘 부탁드립니다 🙇‍♀️❣️

Copy link

@dave915 dave915 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다니 안녕하세요!
피드백 반영 확인했어요 👍
그동안 고생하셨습니다! 이만 머지할게요 :)
궁금한점 있으면 언제든 DM주세요!

import java.util.Arrays;
import java.util.function.Predicate;

public enum FarePolicy {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -0,0 +1,31 @@
<configuration debug="true">
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@dave915 dave915 merged commit e980371 into woowacourse:da-nyee Jun 11, 2021
@da-nyee
Copy link
Author

da-nyee commented Jun 21, 2021

📚 다니의 학습 로그

[Java] Gradle Dependency Configurations - 4

내용

태그

{Java}, {Gradle}, {Dependency}, {Configurations}, {compileOnly}, {runtimeOnly}, {implementation}, {api}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants