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

2주차 과제제출 #46

Merged
merged 37 commits into from
Aug 23, 2021
Merged

2주차 과제제출 #46

merged 37 commits into from
Aug 23, 2021

Conversation

jxmen
Copy link

@jxmen jxmen commented Aug 17, 2021

안녕하세요? 2주차 과제를 제출합니다.
현재는 강의를 수강한 부분까지 완료하였으며 계속 추가할 예정입니다.
기존 1주차에서 사용하였던 코드를 조금 비슷하게 재구성 하였습니다.

리뷰 잘 부탁드립니다 ^~^

Copy link
Collaborator

@johngrib johngrib left a comment

Choose a reason for hiding this comment

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

반갑습니다. 한 주간 열심히 학습해 봅시다.

app/build.gradle Show resolved Hide resolved
@jxmen
Copy link
Author

jxmen commented Aug 19, 2021

08.19

  • 리뷰 내용 반영
  • 할 일 삭제, 수정 view 추가 (e2e 테스트 통과)
  • javadoc 추가 - TaskManager, TaskMap

Comment on lines 51 to 54
private String getPreviousMethodName() {
int PREVIOUS_METHOD_INDEX = 2;
int PREVIOUS_METHOD_INDEX = 3;
return Thread.currentThread().getStackTrace()[PREVIOUS_METHOD_INDEX].getMethodName();
}
Copy link
Author

@jxmen jxmen Aug 19, 2021

Choose a reason for hiding this comment

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

어디서 ID를 찾을 수 있는지 나타내기 위해 메서드 이름을 가져오게 하였습니다. 그런데 이 부분이 stackTrace에 의존하고 있고, 만약 메서드 위치라도 바뀐다면 이 인덱스를 수정해야 할 것 같습니다. 어떻게 하면 더 좋은 방법을 찾을 수 있을지 궁금합니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

앗 이거 이렇게까지 할 필요는 없어요. 일단은 에러 메시지가 문제의 원인이 된 "데이터"에 집중하도록 작성해 주세요. 에러 메시지에서 "코드"에 집중하는 것은 훨씬 심각한 경우에 드물게 선택하게 되는 일입니다.

Comment on lines 9 to 12
* <p>
* {@link com.codesoom.assignment.controllers.TasksController TasksController}에서 요청을 받고,
* {@link TaskMap TaskMap}에 실제 할 작업을 요청하고 결과를 가져옵니다.
* </p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 주석은 좋지 않아요. TaskManagerTasksController에 의존하고 있지 않습니다. 그런데 이런 주석이 생기면 TaskManager의 주석이 TasksController에 의존하게 되어 커플링이 발생합니다.

만약 TasksControllerTaskManager를 사용하지 않게 되거나, TaskController의 이름이 바뀐다고 상상해 봅시다.
그러면 TaskManager는 코드를 수정하지도 않았고 수정할 일도 없는데 TaskManager의 주석은 잘못된 정보를 갖고 있게 되겠죠? 즉 TasksController의 변경이 TaskManager까지 전파되는 것입니다.

Suggested change
* <p>
* {@link com.codesoom.assignment.controllers.TasksController TasksController}에서 요청을 받고,
* {@link TaskMap TaskMap}에 실제 할 작업을 요청하고 결과를 져옵니다.
* </p>

Comment on lines 14 to 16
* @author pangnem
* @see com.codesoom.assignment.controllers.TasksController TasksController
* @see TaskMap
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 세 줄도 불필요합니다.

Suggested change
* @author pangnem
* @see com.codesoom.assignment.controllers.TasksController TasksController
* @see TaskMap
  • @author는 취향을 타기는 하는데요, 요즘은 git이 있어서 굳이 남기지 않아도 괜찮습니다.
  • @seeTaskController가 있는 것은 좋지 않습니다. TaskManager는 자신을 누가 사용하는지 몰라야 좋습니다.
  • @seeTaskMap이 있는 것은 내부 구현 유출입니다. javadoc 주석은 외부에 공개하는 정보라고 생각하고 작성하시는 것이 좋아요. 설령 실제로는 절대로 외부에 공개하지 않는다고 해도요.

Comment on lines 18 to 20
public class TaskManager {

private static final TaskManager uniqueInstance = new TaskManager();
Copy link
Collaborator

Choose a reason for hiding this comment

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

static finalTaskManager를 선언해 싱글톤으로 사용하는 것보다 Spring Bean으로 만들어 싱글톤으로 관리하는 것이 좀 더 편리할지도 모릅니다.

Copy link
Author

Choose a reason for hiding this comment

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

Spring Bean에 대해 조사해보고 적용해보겠습니다! :)

app/src/main/java/com/codesoom/assignment/TaskMap.java Outdated Show resolved Hide resolved
app/src/main/java/com/codesoom/assignment/TaskMap.java Outdated Show resolved Hide resolved
app/src/main/java/com/codesoom/assignment/TaskMap.java Outdated Show resolved Hide resolved
public class TaskIdNotFoundException extends RuntimeException {

public TaskIdNotFoundException(String methodName, Long id) {
super(String.format("%s 메서드에서 id %s 를 찾을 수 없습니다.", methodName, id));
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
Author

@jxmen jxmen Aug 20, 2021

Choose a reason for hiding this comment

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

id만 메시지에 나오도록 수정하였습니다.

말씀하신대로 문제의 원인을 생각해보면, 단일조회/수정/삭제 요청이든 간에 ID를 찾지 못했다 라는 것이 문제이지 어느 곳에서 발생했는지는 중요하지 않을 것 같네요.

Comment on lines +5 to +21
/**
* 할 일 번호 생성을 담당합니다.
*/
@Service
public class TaskIdGenerator {

private Long lastId = 0L;

public Long getLastId() {
increaseLastId();
return lastId;
}

private synchronized void increaseLastId() {
lastId++;
}
}
Copy link
Author

Choose a reason for hiding this comment

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

할일 번호 Id 발급을 해당 클래스가 책임지도록 따로 분리하였습니다.

Comment on lines +31 to +35
private final TaskManager taskManager;

public TasksController(TaskManager taskManager) {
this.taskManager = taskManager;
}
Copy link
Author

@jxmen jxmen Aug 20, 2021

Choose a reason for hiding this comment

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

생성자가 오직 하나만 있고 매개변수로 전달하는 값이 모두 빈으로 등록되어 있다면 생성자에는 @AutoWired 어노테이션을 붙이지 않고 빈으로 꺼내 쓸 수 있습니다.
TaskManager는 @Service 어노테이션으로 빈으로 등록이 되어있기 때문에, TasksController 생성자에서는 @AutoWired어노테이션을 붙이지 않아도 빈으로 주입이 됩니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

좋습니다 :-)

taskMap.put(id, task);
}

public Collection<Task> getValues() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

이렇게 수정하면 어떨까요?

Suggested change
public Collection<Task> getValues() {
public Collection<Task> getTasks() {

또는

Suggested change
public Collection<Task> getValues() {
public Collection<Task> getAllTasks() {

return findWith(id);
}

public Task update(Long id, String title) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 메소드에 javadoc 주석을 작성해 보세요. 이 메소드는 꼭 작성해야 합니다.

Copy link
Author

Choose a reason for hiding this comment

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

하는김에 TaskRepository 메서드 모두 추가해 보았습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

가급적이면 public 메소드에는 모두 작성하는 것이 좋다고 생각해요. private은 메소드가 한 눈에 이해하기 어려운 경우에만 작성하고요.

app/src/main/java/com/codesoom/assignment/models/Task.java Outdated Show resolved Hide resolved
Comment on lines 32 to 34
public boolean isMatchId(long taskId) {
return this.id.equals(taskId);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 메소드에 대해 javadoc을 작성해 보세요. boolean 메소드는 javadoc을 연습하기에 아주 좋은 대상입니다.

Comment on lines 37 to 43

/**
* 번호에 해당하는 할 일을 찾습니다.
*
* @param id 찾을 할 일 번호
* @return Task
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

리턴하는 것은 무엇인가요?

Comment on lines +5 to +7
/**
* 할 일 번호 생성을 담당합니다.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

번호라는 표현보다 더 추상화된 표현도 생각해 봅시다.

다음과 같은 방법은 어떤가요?

Suggested change
/**
* 할 일 번호 생성을 담당합니다.
*/
/**
* 할 일 식별자의 생성을 담당합니다.
*/

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
Collaborator

Choose a reason for hiding this comment

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

그렇습니다!

@jxmen
Copy link
Author

jxmen commented Aug 22, 2021

현재 Task 라는 도메인에 getter와 setter가 존재합니다. 객체지향 생활 체조 원칙 중 하나에서는 게터/세터/프로퍼티를 쓰지 않는다 였던 것이 기억이 납니다. 만약 이것들을 제거할려면 어떻게 해야할까요? DTO를 사용하는 경우는 getter와 setter가 허용되는 걸로 알고 있는데, 그렇다면 DTO를 어떤 식으로 만들면 좋을까요?

Comment on lines 28 to 35
/**
* id가 일치하면 true를 리턴하고, 아닐 경우 false를 리턴합니다.
* @param id 비교 대상
* @return id가 일치하는 경우 true, 그 외의 경우 false
*/
public boolean isMatchId(long id) {
return this.id.equals(id);
}
Copy link
Author

@jxmen jxmen Aug 22, 2021

Choose a reason for hiding this comment

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

리뷰해주신대로 javadoc을 추가하였습니다. 다만 isMatchId() 의 경우 현재는 더이상 사용하지 않는데, 이런 경우 제거하는게 좋을까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

안 쓰는 메소드라면 삭제하는 것이 좋겠죠.

Comment on lines 24 to 27
Long lastId = taskIdGenerator.getLastId();
task.setId(lastId);
Task task = new Task(lastId, source.getTitle());

taskRepository.save(lastId, task);
Copy link
Author

Choose a reason for hiding this comment

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

아이디 발급, 새로운 할 일 생성 등을 현재는 Service에서 하고 있는데, 각 작업들을 Service와 Repository 각각 어디에서 수행하는 게 좋은지 기준이 모호합니다. 예를 들면 taskRepository에서 아이디 발급과 새로운 할 일 생성 등의 작업을 위임할 수도 있을 것 같은데요, 각 레이어마다 어떤 작업까지 하고 넘겨주어야 할지가 궁금해졌습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

이렇게 애매할 때가 책임을 고민해볼 때라 할 수 있어요. 그런데 service라는 이름은 너무 포괄적이고 넓은 의미를 갖고 있지 않나요? 만약 service라는 단어가 클래스 이름에 금지된다면 우리는 어떤 방법을 생각할 수 있을까요?

Copy link
Author

@jxmen jxmen Aug 23, 2021

Choose a reason for hiding this comment

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

앗 제가 TaskManagerservice라고 표현했군요. 그렇다면 클래스 이름은 그 이름이 어디까지 책임을 지는가? 라고 생각한다면, 내 영역이 아니라면 위임하고, 내가 일을 그만큼 덜한다면 다른 곳에서 그 일을 가져와서 해도 되지 않을까? 라는 생각이 들었습니다. 예를 들면 새 할일을 만드는 부분을 TaskFactory 등의 새 클래스를 만드는 것입니다.

예전 글이 생각나서 가져왔는데, TaskManager도 좋은 이름은 아닌 것 같군요 🥲

혹은 controller에서 repository를 바로 호출하는 방식도 있을텐데, 이 글을 통해 좋은 방법인 것 같지는 않다고 판단이 되었습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

여기에서 생각해봐야 하는 것은 service를 여러개로 쪼개서 다양한 이름을 만들어 주는 것이라 할 수 있어요.

@johngrib
Copy link
Collaborator

johngrib commented Aug 22, 2021

현재 Task 라는 도메인에 getter와 setter가 존재합니다. 객체지향 생활 체조 원칙 중 하나에서는 게터/세터/프로퍼티를 쓰지 않는다 였던 것이 기억이 납니다. 만약 이것들을 제거할려면 어떻게 해야할까요? DTO를 사용하는 경우는 getter와 setter가 허용되는 걸로 알고 있는데, 그렇다면 DTO를 어떤 식으로 만들면 좋을까요?

setter를 없애는 것은 쉽습니다. 생성자를 통해서만 객체의 인스턴스를 생성하면 돼요. 또는 TaskTask를 감싸는 방법도 생각할 수 있습니다(decorator 패턴을 조사해 보세요). 한편 getter 를 없애는 것은 어렵습니다. 특히 jackson 같은 라이브러리를 사용하고 있다면 getter가 있어야 하니까요. 하지만 getter 메소드가 필수적으로 요구되는 환경이 아니라면 모든 필드 멤버를 final로 선언하고 외부에서 값을 사용할 때 getter로 접근하는 게 아니라 그냥 필드 멤버로 접근하는 방법을 고려할 수 있어요. 이렇게 해도 final이기 때문에 값이 바뀔 일도 없고, getter가 없기 때문에 코드도 깔끔하죠.

한편 getter라는 명명을 피하고 좀 더 의미를 갖는 이름을 사용하는 방법도 생각할 수 있어요(getter를 쓰지 말자는 것은 보통 이런 의미입니다). 예를 들어 bill.getPrice() * bill.getQuantity() 같은 코드가 있다면 이건 그냥 필드 접근과 별로 다를 것이 없으니 bill.totalPrice() 메소드를 제공하는 것이 낫다는 것입니다.

만약 여러 개의 클래스에 공통되는 getter가 필요한 경우가 있다면 이를 인터페이스로 분리해 정의하는 것도 고려할 수 있습니다. 이렇게 정의된 getter는 매우 특별한 것이 되고(supplier의 의미가 되니까요), 특정 레이어를 넘나드는 패스포트가 될 수 있습니다. 예를 들어 Http request에서 회원 아이디와 패스워드가 넘어왔다고 칩시다. 그러면 MemberLoginRequest 클래스에 있는 getId()getPassword() 같은 메소드를 사용하겠죠?

그런데 아이디와 패스워드를 필요로 하는 서비스 클래스가 또 있다고 합시다. 다만 문제는 MemberLoginRequest가 서비스 클래스에 공개되어 있지 않다는 거에요. 이런 경우엔 그냥 request.getId()하고, request.getPassword()하는 방식으로 두 값을 꺼내어 서비스에 제공해야 할 겁니다. 하지만 만약 두 getter 메소드만 정의한 인터페이스가 public으로 공개되어 있다면? MemberLoginRequest가 이 인터페이스를 구현하기만 하면 됩니다. 흔한 트릭은 아니지만 저는 좋아하는 방법입니다.

Copy link
Collaborator

@johngrib johngrib left a comment

Choose a reason for hiding this comment

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

일요일에는 한 주간의 회고를 작성해 슬랙에 공유해주세요. 회고를 확인하면 PR을 머지해드립니다.

Comment on lines +28 to +32
/**
* id가 일치하면 true를 리턴하고, 아닐 경우 false를 리턴합니다.
* @param id 비교 대상
* @return id가 일치하는 경우 true, 그 외의 경우 false
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

거의 완벽한 javadoc을 작성하셨어요. 훌륭합니다.

@johngrib johngrib merged commit 0c14868 into CodeSoom:pangnem Aug 23, 2021
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