이슈 트래커 - 코드리뷰 정리

이슈트래커 리뷰 정리

1. 중복 로직 통합

현재 코드에는 Github Web과 iOS

@RestController
@RequestMapping("/login")
public class GitHubLoginController {

    private final GitHubLoginService loginService;

    public GitHubLoginController(GitHubLoginService loginService) {
        this.loginService = loginService;
    }


    @GetMapping("/github")
    public ApiResponse<GitHubUserResponse> githubLogin(@RequestParam String code) {
        GitHubUserResponse response = loginService.login(code);
        return ApiResponse.ok(response);
    }

    @GetMapping("/github/iOS")
    public ApiResponse<GitHubUserResponse> githubIOSLogin(@RequestParam String code) {
        GitHubUserResponse response = loginService.loginIOS(code);
        return ApiResponse.ok(response);
    }
}

• 'USER-AGENT' 혹은 'USER-ID' 등과 같이 사용자를 식별하는 커스텀 header를 사용해 인터셉터에서 iOS와 Web을 구분하는 로직을 만들고 컨트롤러 레이어부터는 로그인 시 공통 로직을 사용하게 하는 방식을 사용해도 좋을 것 같다는 생각이 든다.

💡 커스텀 헤더 네이밍?
[ 참고 : Custom HTTP headers : naming conventions ]

• The recommendation is was to start their name with "X-". E.g. X-Forwarded-For, X-Requested-With
• On June 2012, the deprecation of recommendation to use the "X-" prefix has become official as RFC 6648.
• 결론은, "X-" prefixed headers를 계속 사용해도 좋지만, 더 이상 공식적으로 권고하는 방식은 아니다.

2. 공통 API 응답 포맷

• 모든 응답에 대해서 클라이언트에게 보내기 전에 ApiResponse라는 클래스를 통해서 매핑하는 과정을 추가했었다.

public class ApiResponse<T> {
    private T data;
    private ApiResponse() { }
}

@GetMapping("/github")
public ApiResponse<GitHubUserResponse> githubLogin(@RequestParam String code) {
     GitHubUserResponse response = loginService.login(code);
     return ApiResponse.ok(response);
}

• 그런데 ApiResponse 를 사용하는 것이 레거시 느낌이 강하게 나는 패턴이라는 리뷰를 받았다. 이것을 사용했을 때와 사용하지 않았을 때를 구분해서 생각해보시면 좋을 것 같다고 말씀해주셨다.

• 흠.. 근데! 통신을 할 때 공통 포맷이 있다면 서버와 클라이언트가 좀 더 쉽게 소통할 수 있지 않을까라는 생각?

• 구글 json 가이드에서 YouTube JSON API를 보면 error를 리턴하는 형태가 다르다는 것만 제외하면, data 필드 아래에 실제 데이터를 래핑해서 응답을 보내는 형식은 같은데.. 훔 어떤게 맞는건지 잘 모르겠다;;😂 스터디에서 같이 이야기해봐야지

3. Profile 값 받기

• 기존에는 생성자 주입을 통해 Environment 빈을 받고, getProperty()메소드를 활용해서 profile에 있는 값을 가져왔었다
• 이번에 받은 리뷰에서는 @Value 어노테이션을 통해서 profile 값을 가져오는 방식을 추천해주셨는데 아직 이점에 대해서는 정확하게는 잘 모르겠다.
• 추측하기로는 Environment 객체에 의존적이지 않는 코드를 만들어 최대한 클래스간 결합도를 낮추기 위한 방식이기 때문이 아닐까

// Environment 빈을 사용해서 주입하는 방식
public LoginController(Environment environment) {
        this.CLIENT_ID = environment.getProperty("github.client.id");
        this.CLIENT_ID_IOS = environment.getProperty("github.client.id.ios");
    }
    
// @Value 어노테이션을 사용해 특정 값을 주입받는 형식
 public LoginController(@Value("github.client.id") String clientId, @Value("github.client.id.ios") String iOSClientId) {
        this.CLIENT_ID = environment.getProperty("github.client.id");
        this.CLIENT_ID_IOS = environment.getProperty("github.client.id.ios");
    }

4. 필드 인젝션 VS 생성자 인젝션

• 현재 코드에서는 필드 인젝션을 사용하고 있다
• 필드 인젝션을 사용할 경우 외부에서 변경이 불가능하므로 테스트가 힘들 수도 있다
• 물론 지금의 경우에는 상속관계에 놓여있는 클래스 타입이 아니라 String이라서 괜찮을 수도 있겠지만!! 그래도 다른 코드와 통일성을 지켜준다는 측면에서는
• 예를 들어 필드 인젝션의 대상이 Repsitory와 같이 DB에 종속적인 타입일 때, 개발자가 개발단계에서는 MainRepostory를 사용하고 테스트 환경에서는 MinorRepository를 사용하는 전략을 취할 수 없다.

@Configuration
public class AwsConfig {

    @Value("${cloud.aws.s3.access-key}")
    private String accessKey;

    @Value("${cloud.aws.s3.secret-key}")
    private String secretKey;

    @Value("${cloud.aws.s3.region}")
    private String region;

    @Value("${cloud.aws.s3.bucket}")
    private String bucket;

    @Bean
    public S3Client s3Client() {
        final AWSCredentials awsCredentials = new BasicAWSCredentials(accessKey, secretKey);
        final AmazonS3 amazonS3 = AmazonS3ClientBuilder
                .standard()
                .withRegion(region)
                .withCredentials(new AWSStaticCredentialsProvider(awsCredentials))
                .build();

        return S3Client.create(amazonS3, region, bucket);

    }
}