[hiro & bongf] Mission6 페이지나누기 #134

bong6981 · 2021-03-30T13:33:36Z

안녕하세요
백엔드반 히로와 봉프입니다.
미션6까지 구현했습니다.

1-6까지 한 번 더 미션을 돌리기 위해 빨리 하려다보니 고민이 부족했던 점이 있습니다.
커밋 메시지가 기능 단위별로 작성되지 않았습니다. 죄송합니다.

미션 5에서 주신 피드백에 대해 예외 메시지를 그대로 노출시키기 보다는 새로운 메시지로 처리했습니다.
여전히 예외처리를 하고 있어 피드백 반영이 잘 된 것 같지 않습니다 ㅠㅠ
미션 5에서 주신 피드백은 아래와 같습니다

   @ExceptionHandler(IllegalEntityIdException.class)
    public String handleIllegalEntityIdException(IllegalEntityIdException e) {
        return e.getMessage();
    }

    @ExceptionHandler(NotLoginException.class)
    public String handleNotLoginException(NotLoginException e) {
        return e.getMessage();
    }

    @ExceptionHandler(IllegalAccessException.class)
    public String handleIllegalAccessException(IllegalAccessException e) {
        return e.getMessage();
    }

사용자에게 예외의 메세지를 직접 전달하는 것을 의도하신 건가요?
사용자가 이 메세지를 보고 어떤 의미를 도출할 수 있을까요?
단순히 String 의 형태로 예외 메세지를 직접 전달하는 건 사용자 경험 측면에서 악영향일 뿐만 아니라, 잠재적인 보안 위험일 수도 있습니다.
아직 scipt.js에서 error 로 이동하여 alert(xhr.responseText); 를 보여주는 것과
Rusult에 메시지를 담아 success에서 alert로 Result.valid로 체크하여 false일시 alert로 해당 메시지를 보여주는 것의 차이를 모르겠습니다. ㅠㅠ

코드 리뷰 감사하다는 말씀 드립니다!

배포링크

https://bong-spring-boot-qna.herokuapp.com/

생성된 계정 정보

아이디 : javajigi / 비밀번호 : test
아이디 : sanjigi / 비밀번호 : test
아이디 : hi / 비밀번호 : hi

고민한 부분

QuestionPage 클래스를 별도로 만들어 페이지 관련 정보들을 담아 멤버변수 위치와 상수 활용이 맞을지 자신이 없습니다.
(아래 코드와 같이) 페이지 클래스를 만들지 않고 서비스에서 메소드로 페이징 처리 하는 방법도 고민해 봤는데 이 방법도 괜찮을지 문의드립니다.

public List<Integer> createPageNumbers(Pageable pageable) {
    List<Integer> pageNumbers = new ArrayList<>();
    Page question = findQuestions(pageable);
    int pagesRange = 5;
    int totalPage = question.getTotalPages();
    int startPage = (pageable.getPageNumber() / pagesRange) * pagesRange;
    for(int i = startPage; i < startPage + pagesRange; i++) {
        if(i >= totalPage) {
            break;
        }
        pageNumbers.add(i);
    }
    return pageNumbers;
}

@GetMapping
public String getQuestions(Model model, @PageableDefault(size = 15) Pageable pageable) {
    Page<Question> questionPage = questionService.findQuestions(pageable);
    model.addAttribute("questions", questionPage);
    model.addAttribute("check", questionService.getListCheck(pageable));
    model.addAttribute("prev", pageable.previousOrFirst().getPageNumber());
    model.addAttribute("next", pageable.next().getPageNumber());
    model.addAttribute("pages", questionService.createPageNumbers(pageable));
    return "/index";
}

저희가 생각했을 때는 QuestionPage로 만들면 관련 정보들을 묶어서 한번에 model에 넣을 수 있다는 장점이 있지만
QuestionPage 클래스를 보았을 때에 이 클래스 안에서 조차 클래스 멤버 변수로 선언할지 지역변수로 선언할지의 기준이 잡히지 않은 것으로 보아 애초에 클래스로 묶어주지 않는 범위가 아니었을까 고민했습니다.

미해결

더미데이터를 만들 때 for문으로 만들어 복사 붙여넣기를 했습니다. 어떤 키워드로 학습할 수 있을지 몰라 우선 말씀드린 방식으로 데이터를 넣었습니다. CS수업 때 파이썬으로 더미 데이터를 만드는 법은 배웠지만 그것은 데이터베이스에 직접 넣는 방식이라 이번 프로젝트와 같이 data.sql에 작성할 때는 방법을 찾지 못했습니다.

- 페이징 구현 완료

- 지역변수로 선언된 부분을 QustionPage 멤버 변수로 변경

- 날짜 내림 차순으로 구현 - 그를 테스트하기 위한 더미데이터 변경

ksundong

안녕하세요! Hiro & Bongf! 리뷰어 Dion입니다.
여러 고민들은 많이 해볼 수록 좋습니다. 코딩에 정답은 없으니, 최선을 지향하는 수밖에는 없다고 생각해요.
클래스에 꼭 상태값을 모조리 담아줄 필요는 없습니다. 필요한 시점에 그냥 그 값을 호출하는 방법도 있죠.
또한, 스프링에서 스프링이 실행되고나서 필요한 일을 시키는 방법이 있는데요. 그냥 알려드리면 재미가 없으니, 검색해보세요~ 힌트는 충분히 드린 것 같습니다.

사실 코드는 Paging이 제일 중요한데, Paging이 아직 깔끔하지 않다고 판단되어 변경요청 드립니다.

웹 애플리케이션 관련 리뷰

;jsessionid=09008FA332450BC65BE8B883C688A096 이거 주소창에 안나오게하는 설정 요구사항에 있었는데, 한 번 찾아보세요~
페이지가 0부터 시작하는 것은 뭔가 이상하지 않나요?
이전, 다음 버튼은 유동적으로 노출되어야하고, 눌렀을 때, 다음 단계로 이동해야 한다고 생각되네요.

고생하셨습니다!
궁금하시거나, 도움이 필요한 부분, 이해가 가지 않는 부분, 토론하고 싶은 부분은 코멘트 남겨주세요!
DM으로 여쭤보지 마시구요 ㅋㅋ!!

ksundong · 2021-03-31T15:25:18Z

src/main/java/com/codessquad/qna/web/QuestionPage.java

+import java.util.List;
+
+public class QuestionPage {
+    private final int COUNT_NUMBERS_TO_SHOW = 5;


Suggested change

private final int COUNT_NUMBERS_TO_SHOW = 5;

private final static int COUNT_NUMBERS_TO_SHOW = 5;

ksundong · 2021-03-31T15:25:40Z

src/main/java/com/codessquad/qna/web/QuestionPage.java

+
+public class QuestionPage {
+    private final int COUNT_NUMBERS_TO_SHOW = 5;
+    private final Pageable pageable;


얘는 final이고 나머지는 final이 아닌 이유가 있을까요?

ksundong · 2021-03-31T15:26:48Z

src/main/java/com/codessquad/qna/web/QuestionPage.java

+    private int previous;
+    private int next;
+    private Page<Question> questions;
+    private List<Integer> pageNumbers = new ArrayList<>(COUNT_NUMBERS_TO_SHOW);


여기서 상태로 관리되는 값과 아닌 값을 구분하는 시야를 기르셨으면 좋겠네요.
단순히 데이터를 저장하는 목적으로 필드를 사용하는 것은 추천드리지 않습니다.

ksundong · 2021-03-31T15:28:16Z

src/main/java/com/codessquad/qna/web/controller/ApiAnswerController.java

@@ -23,7 +23,7 @@ public ApiAnswerController(AnswerService answerService) {

    @PostMapping
    public Answer createAnswer(@PathVariable long questionId, String contents, HttpSession session) {
-        User user = HttpSessionUtils.getSessionedUser(session).orElseThrow(NotLoginException::new);
+         User user = HttpSessionUtils.getSessionedUser(session).orElseThrow(NotLoginException::new);


Suggested change

User user = HttpSessionUtils.getSessionedUser(session).orElseThrow(NotLoginException::new);

User user = HttpSessionUtils.getSessionedUser(session).orElseThrow(NotLoginException::new);

메서드 오퍼레이터의 사용은 좋네요!

ksundong · 2021-03-31T15:35:19Z

src/main/java/com/codessquad/qna/web/controller/ApiAnswerController.java

    @ExceptionHandler(IllegalEntityIdException.class)
-    public String handleIllegalEntityIdException(IllegalEntityIdException e) {
-        return e.getMessage();
+    public String handleIllegalEntityIdException() {
+        return "유효하지 않은 접근입니다";
    }

    @ExceptionHandler(NotLoginException.class)
-    public String handleNotLoginException(NotLoginException e) {
-        return e.getMessage();
+    public String handleNotLoginException() {
+        return "로그인이 필요합니다";
    }

    @ExceptionHandler(IllegalAccessException.class)
-    public String handleIllegalAccessException(IllegalAccessException e) {
-        return e.getMessage();
+    public String handleIllegalAccessException() {
+        return "자신의 답변만 삭제할 수 있습니다";
    }


이전 리뷰어분이 이런 형식의 예외 핸들링을 바라신 것은 아닌 것 같아요.
저는 솔직히 아직 이정도 수준의 예외 처리를 바라진 않는데, 에러를 반환하는 객체를 정의해서 일정한 형식으로 주는것을 고민해보세요!

ksundong · 2021-03-31T15:38:59Z

src/main/java/com/codessquad/qna/web/QuestionPage.java

+import java.util.ArrayList;
+import java.util.List;
+
+public class QuestionPage {


객체로 포장하려는건 좋은 시도였어요~

ksundong · 2021-03-31T15:39:21Z

src/main/java/com/codessquad/qna/web/QuestionPage.java

+        this.questions = questionsByPage;
+        this.previous = questionsByPage.previousOrFirstPageable().getPageNumber();
+        if (questionsByPage.hasNext()) {
+            this.next = questionsByPage.nextPageable().getPageNumber();
+        } else {
+            this.next = questionsByPage.getNumber();
+
+        }
+        this.pageNumbers = createPageNumbers();


여기서 유도된 친구들은 그냥 메서드 호출하면 그 때 반환해주는건 어때요?

오! 그런 방식은 생각해보지 않았네요. 그렇게 되면 멤버변수에 대한 고민도 해결되는 부분이 있는 것 같습니다! 감사합니다. 위에 final 키워드와 함께 수정해보겠습니다!

ksundong · 2021-03-31T15:40:22Z

src/main/java/temp.java

+        for(int i=1; i<=200; i++) {
+            System.out.println(
+                    "INSERT INTO QUESTION (ID, WRITER_ID, TITLE, CONTENTS, CREATED_DATE_TIME, COUNT_OF_ANSWERS) VALUES ('"+ i + "', '1', '6', 'dui', '2021-03-20 16:03:03.106461', '1')"
+            );
+        }
+
+    }


음 이건 아닌데요~
스프링에서는 스프링 컨테이너 시작시점에 동작을 수행하는 기능도 존재합니다. 이를 이용해보세요.

ksundong · 2021-03-31T15:40:55Z

src/main/java/com/codessquad/qna/web/QuestionPage.java

+
+    public QuestionPage(Pageable pageable, Page<Question> questionsByPage) {
+        this.pageable = pageable;
+        this.questions = questionsByPage;


가급적이면 일치시켜주세요~

넵 일치시키도록 하겠습니다!

ksundong · 2021-03-31T15:41:26Z

src/main/resources/static/js/scripts.js

+
+
+


빈 라인이 이렇게 추가된 이유는 뭔가용?

코드 정리를 하면서 잘못 추가된 것 같습니다. 삭제하겠습니다!

bong6981 · 2021-04-01T15:18:54Z

상세한 리뷰 감사합니다!
다만 미션4부터 제 부분의 코드가 반영이 많이 되어
새로 미션 1부터 시작 할 때 히로가 주도를 하기로 했습니다.
히로 부분에 대해서 피드백을 받는 것도 중요할 것 같고, 또 이번에 주신 피드백에 대해 학습할 내용도 많아 보여
피드백 모두 반영한 후 다시 PR요청드려도 될까요?
피드백 반영이 늦어질 것 같아 이렇게 글 남깁니다!

bong6981 added 8 commits March 30, 2021 18:27

feat : Pagenation

214f27f

feat : Rename

af0712f

feat : 페이징구현

f6ad700

- 페이징 구현 완료

feat : 예외처리보완

fe37215

feat : 변하지 않는 부분에 대해 상수 처리

3abe381

feat : QuestionPage 수정

f89d005

- 지역변수로 선언된 부분을 QustionPage 멤버 변수로 변경

feat : 날짜 내림차순 구현

9d3ce12

- 날짜 내림 차순으로 구현 - 그를 테스트하기 위한 더미데이터 변경

feat : Delete unused class

0c397ba

ksundong self-requested a review March 31, 2021 14:34

ksundong self-assigned this Mar 31, 2021

ksundong requested changes Mar 31, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[hiro & bongf] Mission6 페이지나누기 #134

[hiro & bongf] Mission6 페이지나누기 #134

bong6981 commented Mar 30, 2021 •

edited

Loading

ksundong left a comment

ksundong Mar 31, 2021

ksundong Mar 31, 2021

ksundong Mar 31, 2021

ksundong Mar 31, 2021

ksundong Mar 31, 2021

ksundong Mar 31, 2021

ksundong Mar 31, 2021

bong6981 Apr 1, 2021

ksundong Mar 31, 2021

ksundong Mar 31, 2021

bong6981 Apr 1, 2021

ksundong Mar 31, 2021

bong6981 Apr 1, 2021

bong6981 commented Apr 1, 2021 •

edited

Loading

	private final int COUNT_NUMBERS_TO_SHOW = 5;
	private final static int COUNT_NUMBERS_TO_SHOW = 5;

	User user = HttpSessionUtils.getSessionedUser(session).orElseThrow(NotLoginException::new);
	User user = HttpSessionUtils.getSessionedUser(session).orElseThrow(NotLoginException::new);

[hiro & bongf] Mission6 페이지나누기 #134

Are you sure you want to change the base?

[hiro & bongf] Mission6 페이지나누기 #134

Conversation

bong6981 commented Mar 30, 2021 • edited Loading

배포링크

생성된 계정 정보

고민한 부분

미해결

ksundong left a comment

Choose a reason for hiding this comment

웹 애플리케이션 관련 리뷰

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bong6981 commented Apr 1, 2021 • edited Loading

bong6981 commented Mar 30, 2021 •

edited

Loading

bong6981 commented Apr 1, 2021 •

edited

Loading