Skip to content

과제 1-2 pgsshiho 과제제출 #1

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

과제 1-2 pgsshiho 과제제출 #1

wants to merge 5 commits into from

Conversation

pgsshiho
Copy link

@pgsshiho pgsshiho commented Mar 6, 2025

No description provided.

@snowykte0426 snowykte0426 changed the title task1-2 과제 1-2 pgsshiho 과제제출 Mar 8, 2025
snowykte0426
snowykte0426 requested changes Mar 11, 2025
View reviewed changes
Copy link
Member

@snowykte0426 snowykte0426 left a comment

Choose a reason for hiding this comment

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

우선 수고하셨습니다!

하나 궁금한 사항이 있는데 HTML 파일은 왜 존재하는건가요?

snowykte0426
snowykte0426 requested changes Mar 12, 2025
View reviewed changes
Copy link
Member

@snowykte0426 snowykte0426 left a comment

Choose a reason for hiding this comment

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

한번 확인해주세요!

snowykte0426
snowykte0426 requested changes Mar 12, 2025
View reviewed changes
Copy link
Member

@snowykte0426 snowykte0426 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

잘못 전달된거 같습니다ㅠ

컨트롤러 각 메서드를 전부 나누라는게 아니라

@RequestMapping("/api/v1/auth")
@RequireConstructor
@RestController
public class AuthController {
    @GetMapping...
    public ResponseEntity<...> signin { ... }

    @PostMapping...
    public ResponseEntity<...> signup { ... }
}

이런 형식으로 해달라는 거였습니다ㅠ
API명세서와 API 설계가(특히 PATH)가 일치하지 않아 리뷰한 것이었습니다

snowykte0426
snowykte0426 requested changes Mar 12, 2025
View reviewed changes
Copy link
Member

@snowykte0426 snowykte0426 left a comment

Choose a reason for hiding this comment

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

IllegalArgumentException를 단순히 반환하면 클라이언트에선 500 Internal Server Error만 반환하게 됩니다,예외 클래스를 생성하고 이를 @AdviceController 와 같은 클래스로 예외를 처리하도록 해주세요

GOMS,MindWay와 같은 교내 프로젝트들에서도 동일한 구조를 취하고 있으니 찾아보셔도 좋을 것 같습니다

snowykte0426
snowykte0426 requested changes Mar 14, 2025
View reviewed changes
Copy link
Member

@snowykte0426 snowykte0426 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

일단 패키지 네이밍 방법이 전부 잘못되어 있습니다
패키지명은 첫글자 소문자로 시작해야하는데 지금 auth 내부 패키지가 전부 대문자로 시작합니다

일부 클래스 네이밍역시 잘못 되어 있는데 확인요

또한 service 패키지 내부에도 인테페이스는 인터페이스 끼리,구현체는 구현체끼리 impl 패키지에 모아주세요

다른 프로젝트들(Dotori,GOMS,Hi,MindWay 등....)도 참고해보세요

...main/java/com/gsm/_8th/class4/backend/task12/domain/auth/Service/GlobalExceptionHandler.java
Comment on lines +1 to +16
package com.gsm._8th.class4.backend.task12.domain.auth.Service;


import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.ControllerAdvice;
import org.springframework.web.bind.annotation.ExceptionHandler;

@ControllerAdvice
public class GlobalExceptionHandler {

@ExceptionHandler(IllegalArgumentException.class)
public ResponseEntity<String> handleIllegalArgumentException(IllegalArgumentException e) {
return ResponseEntity.status(HttpStatus.BAD_REQUEST).body(e.getMessage());
}
}
Copy link
Member

Choose a reason for hiding this comment

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

GlobalExceptionHandler는 다른 패키지로 이동시켜주세요!

프로젝트 전역적으로 적용되는 클래스이니 만큼 domain보단 global 패키지 내부가 적절할 듯 합니다

src/main/java/com/gsm/_8th/class4/backend/task12/domain/auth/Entity/NewSign.java
Comment on lines +1 to +28
package com.gsm._8th.class4.backend.task12.domain.auth.Entity;

import jakarta.persistence.*;
import lombok.AllArgsConstructor;
import lombok.Builder;
import lombok.Getter;
import lombok.NoArgsConstructor;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

@Entity
@Getter
@AllArgsConstructor
@NoArgsConstructor
@Builder
@Table(name = "new_sign")
public class NewSign {

@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Long id;
private String username;
private String password;
private String role;
private String email;
}
Copy link
Member

Choose a reason for hiding this comment

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

Entity 이름이 new_sign인 것은 현재 비즈니스 요구사항에는 적합하지 않은 것 같습니다

만약 회원가입이 승인제로 이뤄지는 서비스여서 새 계정 정보를 승인 전까지 임시로 저장하는 테이블이라면 나름 적합할 수도 있겠지만...
현재 비즈니스 요구에서는 단순한 사용자 정보 저장이 목적이니만큼 User나 Member 라는 이름이 적합한 것일것 같습니다

...main/java/com/gsm/_8th/class4/backend/task12/domain/auth/Controller/newsignUpController.java
@PostMapping("/signup")
public ResponseEntity<String> signup(@RequestBody UserSignupRequest request) throws URISyntaxException {
authService.signup(request);
URI location = new URI("http://localhost:8081/api/v1/auth/signin");
Copy link
Member

Choose a reason for hiding this comment

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

혹시 이 코드는 왜 있는건가요?
API 주도형 개발에서는 이런 URI를 반환할 이유가 거의 없지 않나요?서버에 직접 정적파일을 보관하는 것이 아니라면 불필요해 보입니다

혹시 필요하다면 이유를 알려주세요
진짜로 모름

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

@snowykte0426 snowykte0426 snowykte0426 requested changes

Assignees

@pgsshiho pgsshiho

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pgsshiho @snowykte0426