Skip to content

[1주차 과제 제출]#3

Open
jjinwo0 wants to merge 11 commits intoTODO-List-Study:mainfrom
jjinwo0:week1-jjinwo0
Open

[1주차 과제 제출]#3
jjinwo0 wants to merge 11 commits intoTODO-List-Study:mainfrom
jjinwo0:week1-jjinwo0

Conversation

@jjinwo0
Copy link
Collaborator

@jjinwo0 jjinwo0 commented Dec 16, 2023

구현 내용

  • in-memory 기반 DB
  • TodoList 생성, 조회, 수정, 삭제 API 구현

변경 사항

비고

  • 테스트 코드 추가 예정
  • 커밋 내역 관리 실수를 뒤늦게 알아 잘 대처하지 못한 부분 양해 부탁드립니다. 🥺

@jjinwo0 jjinwo0 marked this pull request as draft December 17, 2023 12:05
@jjinwo0 jjinwo0 marked this pull request as ready for review December 17, 2023 12:16
@jjinwo0 jjinwo0 marked this pull request as draft December 17, 2023 12:17
@jjinwo0 jjinwo0 marked this pull request as ready for review December 17, 2023 12:23
@Override
public void delete(Long id) {

DB.removeIf(todo -> todo.getId().equals(id));
Copy link
Collaborator

Choose a reason for hiding this comment

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

오 저는 우선 remove만 알고 있었는데 removeIf에 대해 배워갑니다 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

저도 이번에 작성하면서 배워갔어요! 앞으로 종종 사용할 일이 있을 것 같네요 😃


private Long id;
private String title;
private boolean checked;
Copy link
Collaborator

Choose a reason for hiding this comment

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

다른 필드와 다르게 원시형 타입으로 쓰신 이유가 있을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

형변환 가능성이 없다고 판단한 부분과 null을 허용하지 않고자 했으며 이외에는 특별하게 고려한 이유는 없었습니다!
하지만 애초에 Builder패턴으로 엔티티 생성 시 기본값으로 false를 지정해주기때문에 큰 의미가 없다고 느끼고 있어, 스터디 시간에 말한 바 있듯이 enum으로 처리하는 방식을 고려하여 수정 예정입니다 🤗

Copy link
Collaborator

Choose a reason for hiding this comment

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

한 클래스 안에 Rq, Rs를 두신 이유가 있을까요?
저는 Dto를 분리해서 TodoRequest, TodoResponse로 각각 반환해줍니다 ㅎㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

저 또한 엔티티의 수가 많지 않다면 가시성있게 Request, Response를 분리하는 것이 유리한 점을 갖는다고 생각합니다!
반면 프로젝트를 진행하면서 그 수가 많아지게 된다면 Dto 클래스의 수가 매우 많아지고, 이를 분리하기 위한 패키지도 많아져 개인적으로 불편함을 느꼈던 경험이 있습니다.
당시 고민 후에 엔티티에 대한 Dto를 하나만 작성하고, 그 안에 static class로 Request, Response를 분기하는 방식을 사용하였으며 해당 방식이 개인적으로 마음에 들어 지속적으로 사용하고 있습니다!
다른 좋은 방법이 있다면 또 얘기 나누어보고 싶네요 ㅎㅎ🥹

Copy link
Collaborator

Choose a reason for hiding this comment

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

toEnvelope()가 현재 build를 감싸기 역할만 하고 있는 것처럼 보이는데 맞을까요 ??
메서드를 추가로 작성하는 대신에 직접 build 메서드를 호출하는 것이 더 간단하고 명확할 것 같은데 어떻게 생각하시나요 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

맞습니다! 봉투 패턴을 사용하는 부분이 익숙치 않다보니 말씀해주신 부분 이외에도 많이 미숙하네요 ㅎ...
일단 Response되는 데이터가 원하는대로 잘 나오는지에 대한 부분만 집중하다보니 이런 문제가...😱
말씀해주신 부분과 같이 직접 build를 호출하여 작성해보도록 하겠습니다!
좋은 말씀 감사드려요 😊


sb.append("[");
sb.append(error.getField());
sb.append("]");
Copy link
Collaborator

@jhnyuk jhnyuk Dec 17, 2023

Choose a reason for hiding this comment

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

StringBuilder로 문자열 빌드하고 계시는데 꼭 사용해야하는 이유가 아니라면,,
stream + String.join()을 사용해서 더 간결하게 처리할 수 있을 것 같아요 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

앗 join이 있었군요!
코딩테스트 연습에서 StringBuilder에 익숙해져 사용했고 특별한 이유는 없었습니다 🥺
join은 평소에 비교적 사용하지 않아 놓친 것 같네요! 개선해보도록 하겠습니다!
좋은 말씀 감사드려요 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants