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

1Week #1

Open
wants to merge 61 commits into
base: main
Choose a base branch
from
Open

1Week #1

wants to merge 61 commits into from

Conversation

seop-kim
Copy link
Owner

@seop-kim seop-kim commented Nov 8, 2023

[1Week] 김태섭

미션 요구사항 분석 & 체크리스트



체크리스트


1단계

  • 프로그램 실행 시 사용자 명령 받기
  • 종료 입력 시 프로그램 종료 구현

2단계

  • 명언 저장 객체 생성
  • 등록 입력 시 명언, 작가 값을 받아 명언 등록 구현

3단계

  • 명언 객체 내 고유번호 추가
  • 명언 등록 시 고유번호 출력

4단계

  • 명언 등록 시 고유번호 증가 구현
  • 등록된 명언 저장할 객체 생성

5단계

  • 목록 입력 시 등록된 명언 출력 기능 구현

6단계

  • 삭제?id={No} 입력 시 no 값의 고유번호를 가진 명언 삭제 기능 구현

7단계

  • 삭제?id={No} 입력 시 no 값의 고유번호를 가진 명언이 존재하지 않을 시 {no}번 명언은 존재하지 않습니다. 출력하는 예외 처리 구현

8단계

  • 수정?id={No} 입력 시 no 값의 고유번호를 가진 명언 수정, 수정은 명언과 작가 모두 새로운 입력 값을 받아 수정하도록 구현

9단계

  • 목록 데이터를 local에 저장하여 프로그램 재시작 시에도 데이터가 유지되도록 구현

10단계

  • 목록 데이터를 local에 JSON 형태로 저장되는 기능 구현

11단계

  • SimpleDbTest.java의 모든 테스트케이스를 만족하는 기능 구현

12단계

  • 파일 영속성에 관련된 기능 제거
  • 데이터를 DB에 저장

아쉬운점


  • View 를 제대로 나누지 못한 부분이 아쉽다. MVC와 비슷한 구조의 뷰를 구현하기 위해서는 View와 연관된 인터페이스를 생성하여 Controller에서 render를 해줘야하는건 알지만 코드가 설계되지 않는다..

  • Controller에서 path값과 id값을 map에 넣어 사용하기 위해 RequestConverter에서 분기가 2개로 나뉘어지는걸 하나의 분기로 수정하고 싶었으나 어디서부터 건드려야할지 잘 모르겠다.

  • 11 단계의 내용이 이해도가 떨어져 테스트 케이스를 완성하지 못함

  • 12단계에서 db 사용을 너무 구시대적인 방법으로한게 아닌가.. JPA를 썻다면.. 어땠을까.. (최초의 설계는 JPA에 맞춰 설계를 했다..)


Refactoring


9단계

  • 데이터 로드 후 신규 명언 등록 시 고유번호가 1부터 시작되는 문제 해결

모든 단계

  • Map 객체를 이용한 Controller 구현
  • Text 상수화
  • JPA를 사용하여 DB 처리
  • Controller 분기 1개로 수정
  • DB 연동 정보 properties로 분리
  • Obj저장소 싱글톤패턴 적용

Copy link

Choose a reason for hiding this comment

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

명령어와 자주 사용하는 출력 문구를 상수 클래스로 묶은 것 너무 좋은 선택입니다 😀
클래스 이름에 상수를 뜻하는 Const가 들어가는 것도 좋을 것 같아요!

Copy link

Choose a reason for hiding this comment

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

문구들을 상수화하여 사용한다는 것이 코드 작성자도, 코드를 보는 저에게도 가독성이 좋네요!

Copy link
Owner Author

Choose a reason for hiding this comment

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

감사합니다 :)

Comment on lines 25 to 31
private String getRequest() {
System.out.print(KoreaContent.REQUEST_MENU);
String request = Request.input();
model.put("request", request);
Request.setModel(model);
return request;
}
Copy link

Choose a reason for hiding this comment

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

getRequest() 를 MainController에서 담당하는 이유가 있을까요?

Copy link
Owner Author

Choose a reason for hiding this comment

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

음 이 부분은 확인을 안하고 넘어갔었네요
Request쪽에서 담당을 해도 괜찮을 것 같군요 !
수정해보도록 하겠습니다 !

Copy link

Choose a reason for hiding this comment

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

IMainControllable 구현체를 Enum에 담으셨군요! 너무 좋습니다.

public class Find implements IMainControllable {
@Override
public void process(Map<String, String> model) {
System.out.println(KoreaContent.LIST_GUIDE_MSG);
Copy link

@itonse itonse Nov 9, 2023

Choose a reason for hiding this comment

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

출력 될 문자열들을 KoreaContent 클래스에 상수로 저장해서 사용하는 방식으로 하셨는데, 현재 메서드에서 코드가 간결해지고, 나중에 재사용성, 유지보수 측면에서도 용이할 것 같아서 좋은 방법인 것 같습니다👍

import java.util.List;

public class LocalDataLoad {
private static final String USER_HOME_PATH = System.getProperty("user.home");
Copy link

@ChaChaMong ChaChaMong Nov 9, 2023

Choose a reason for hiding this comment

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

좋은점 : 경로 정보를 사용자의 PC정보로 가져와 사용자 편의성이 더 높아졌습니다

@@ -0,0 +1,11 @@
package com.likelion.wisesaying.controller;
Copy link

Choose a reason for hiding this comment

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

IMainControllable의 패키지가 controller가 아닌 구현체와 함께 function에 위치하는 것은 어떨까요?

Copy link

@ChaChaMong ChaChaMong left a comment

Choose a reason for hiding this comment

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

  • 좋은점
    • 코드에 의견 남겨놓았습니다
    • 전체적으로 Exception 처리를 잘 한 것으로 보입니다.
  • 의견제시
    • 코드에 의견 남겨놓았습니다.

private Long convertId(String request) {
Saying saying = DB_TYPE.findOne(TYPE_CONVERTER.strToLong(request));
if (saying == null) {
throw new IllegalArgumentException(request + KoreaContent.NONE_FIND_DATA);
Copy link

Choose a reason for hiding this comment

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

예외를 던질 때 request + KoreaContent.NONE_FIND_DATA 라는 명확한 에러 메시지를 제공하도록 하셔서, 예외가 발생했을 때 문제점 파악이 쉬울 것 같습니다!

Copy link

Choose a reason for hiding this comment

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

자주 변경하는 부분을 convertor 패키지에 일임하셨군요. 멋집니다

System.out.println(e.getMessage());
return;

} catch (CustomRequestException e) {
Copy link

@itonse itonse Nov 9, 2023

Choose a reason for hiding this comment

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

CustomRequestException 은 어떤 예외인지 궁금합니다👀

Copy link
Owner Author

Choose a reason for hiding this comment

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

리팩토링 과정 중에서 필요가 없어진 exception이네요 하하.
최초의 의도는 사용자로부터 request를 받을 때 split 을 통해 "?id="를 path와 id 값으로 나누는데 있어서 발생하는 ArrayIndexOutOfBoundsException를 처리하기 위함이였습니다 !

이제는 지워도 괜찮겠군요 :)

Copy link

Choose a reason for hiding this comment

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

이 클래스는 무엇을 담당하는 걸까요?

Copy link
Owner Author

Choose a reason for hiding this comment

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

삭제될 클래스였는데 삭제를 안하고 commit을 해버렸군요 감사합니다 :)

Copy link

Choose a reason for hiding this comment

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

메모리 DB와 DAO를 IRepoAdapter로 추상화하셨군요. 좋습니다!

Comment on lines 28 to 31
Saying saying = new Saying();
saying.setId(loadPostNum);
saying.setAuthor(loadAuthor);
saying.setContent(loadSentence);
Copy link

Choose a reason for hiding this comment

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

setter보다는 생성자를 활용하는 것은 어떨까요?

Copy link
Owner Author

Choose a reason for hiding this comment

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

좋은 선택인 것 같습니다 !

Copy link

@nnWon nnWon left a comment

Choose a reason for hiding this comment

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

[좋았던 점]

  • 자주 사용하는 코드를 상수 클래스, 유틸 클래스로 작성하는 부분이 인상 깊었습니다.
  • 저는 Map을 활용하는 부분을 Enum을 통해 풀어나가는 것을 보고 배워갑니다!

[개선하면 좋을 부분 && 궁금한 점]

  • 각 코드에 코멘트하였습니다!

코드 재사용과 리펙토링에 대해 많이 고민한 흔적이 엿보입니다.
고생하셨습니다~

import java.util.Map;
import java.util.Scanner;

public interface IMainControllable {
Copy link

@itonse itonse Nov 9, 2023

Choose a reason for hiding this comment

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

SayingService 에 대한 해당 인터페이스의 역할이 무엇이 있는지 궁금합니다 👀
(아직 제가 이 부분의 개념이 부족하여 살짝 여쭤봅니다)

Copy link

@itonse itonse left a comment

Choose a reason for hiding this comment

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

좋은점: 코드를 일관성 있게 짜신 것들이 눈에 많이 보이며, 특히 캡슐화가 잘 적용되어 있어 향후 유지보수나 확장성 측면에서 이점이 많을 것 같습니다.
많이 배워갑니다👍

의견제시: 궁금한 점들은 부분 리뷰로 남겨 놓았습니다

Copy link

@UJIN901 UJIN901 left a comment

Choose a reason for hiding this comment

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

  • 상수화를 하는 것 만으로도 코드의 가독성이 향상된다는 것을 새로 알았습니다.
  • 전체적으로 모듈화가 잘 되어있다는 느낌을 받았습니다. 프로젝트 관리에 많은 도움이 될 것 같습니다! 많이 배우고 갑니다.

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

5 participants