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

[코드리뷰] 변경할 부분, 추천하는 기능, 궁금한 사항 #5

Open
danieluhm2004 opened this issue Jul 28, 2022 · 3 comments

Comments

@danieluhm2004
Copy link

danieluhm2004 commented Jul 28, 2022

안녕하세요.

가볍게 코드리뷰를 한 번 해봤는데, Nestjs가 갖고 있는 되게 기본적인 기능이지만 적용하면 좋은 것들이나 코드를 이렇게 작성한 의도가 궁금하기도 하고 어떠한 부분은 수정하면 좋을지 가볍게 이야기해보려고 합니다.

변경할 부분

== 과 !=

굉장히 당연한 이야기지만 가끔 ==과 !=를 적용한 코드가 있어서 수정해주면 좋을 것 같습니다. 혹시라도 이에 대한 차이를 모르신다면 이 글을 참고해주세요.


board 안에 post와 comment

Nestjs 에서 모듈이라는 개념을 정의하는 것 자체가 한 가지의 기능(게시글, 댓글 등...)를 갖도록 하는 것이 원칙이기 때문에 한 폴더 안에 comment와 post가 같이 공존하는 것은 어떻게보면 기본적인 모듈의 개념에 위반하는 내용이기 때문에 분리해주는게 좋습니다.


App Module 에 있는 TypeORM과 Winston 파일 따로 빼기

라이브러리더라도 파일을 빼는 과정을 통해 순전히 어떤 모듈이 로드되고 있는지 바로 확인할 수 있도록 하는 것이 좋습니다.


@nestjs/swagger로 문서 정리

가끔식 급하게 작업하느라 필드명 또는 데이터 인풋 조건에 대한 정보가 문서에서 누락되는 경우가 발생합니다. 그러한 문제를 방지하기 위해서는 자동으로 문서를 생성해주는 라이브러리 사용을 권해드립니다.

물론 규모가 된다면 별도로 작성해도 되지만 문서를 일일이 수정하는 것은 생각보다 쉽지 않습니다. 문서는 클라이언트와의 약속이기 때문에 서버와 동일하게 유지해가는 것이 중요합니다. 뿐만 아니라 정규식과 같은 규칙에 대한 정보도 보여주기 때문에 간편합니다.


TypeORM 모듈 설정시 entities에 클래스로 선택

물론 경로로 설정하면 간편하지만 타입스크립트 컴파일러는 기본적으로 이름을 바꾸거나 삭제하지 않고 새로 생성하는 형태로 가기 때문에 엔티티가 지워지지 않을 수 있기 때문에 경로로 지정하는 것을 권하지 않습니다.


DTO 재활용

이 기능을 사용하면 데이터베이스의 DTO 정보를 그대로 가져올 수 있습니다. 복사/붙여넣기를 통해 발생하는 잔잔한 오류들은 예방할 수 있습니다.


로깅

raw 데이터를 직접 띄워주는 방법 대신 사용자가 어떠한 액션을 취했는지 간단하게 띄워주는 것을 추천합니다. 이 과정에서 영어와 한글은 상관없습니다. 또한 추후 개발 상황을 고려 컨트롤러 단보다 서비스 단에 로깅을 하여 추후 로그를 분석할 때, 어떠한 문제가 있었거나 어떠한 일이 발생했는지 프로세스를 확인할 수 있어, 정밀하게 파악이 가능합니다.


TypeORM Auto Sync 끄기

다소 자동으로 업데이트가 되지 않아 불편할 수 있지만 이후에 마음대로 스키마가 변경되는 일을 방지하기 위해서는 가능한 Auto Sync를 끄는 것을 추천드립니다.


DTO 파일 이름 통합

파일 중 간혹 abc.dto.ts가 아닌 abc-dto.ts로 되어 있는 파일이 있습니다. 변경하여 통일성을 맞춰주세요.

적용하면 좋은 기능

Nestjs 에서 제공하는 버전 관리

이 기능을 사용하면 추후 API 변경에 따른 클라이언트 오류가 발생하지 않도록 할 수 있습니다. 가장 직관적인 Path로 거는 것을 가장 권해드립니다.


Nestjs 에서 제공하는 압축 활성화

이 기능을 통하여 통신시 압축을 해주면 통신 속도를 어느정도 줄일 수 있습니다.


Nestjs 에서 제공하는 요청 제한 기능

가끔식 과도하게 요청을 하는 통신이 발생할 수 있습니다. 이는 서버와 데이터베이스에 무리를 줄 수 있으므로 이 기능을 사용하여 과도한 요청을 거절시키는 기능을 켜주는 것을 권해드립니다.


Nestjs 에서 제공하는 기본 보안 기능

이 기능을 사용하여 기본적인 취약점을 보안할 수 있습니다.

궁금한 사항

Auto Increment 사용하는 이유

Auto Increment가 숫자이기 때문에 인덱싱에 조금 더 유리한 면을 가지고 있지만 최고의 퍼포먼스가 필요한 것이 아니라면 UUID 또는 CUID와 같은 형태의 포멧으로 PK를 만들면 어떨까 합니다. 보안 취약점 발생시 Auto Increment는 PK 를 예측할 수 있어 위험할 수 있습니다.


Chat Room, Team Room 등에서 Buffer 사용

ID를 Buffer로 사용한 이유가 있을까요? 분명히 이렇게 하신 이유가 있으실 것으로 보이는데, 이렇게하신 이유가 개인적으로 궁금합니다. 만약 특별한 이유가 없으시다면 UUID 같은 형태로 변경하는 것이 추후 관리에 용이하지 않을까 싶습니다.


릴레이션에 관해서

TypeORM에서 OneToMany 또는 OneToOne, ManyToOne, ManyToMany 와 같은 릴레이션 기능을 이미 충분히 제공하고 있는데 별도의 릴레이션을 엮은 이유가 궁금합니다. 혹시 제가 알고 있지 못한 릴레이션 관계가 있을까요?

감사합니다.

@leehj050211
Copy link
Member

Auto Increment를 사용한 이유는 원래 부터 쓰던 것도 있고, 이전 채팅 목록을 불러올 때 해당 채팅의 id보다 작은 값을 불러와야 해서 그런 것 같은데 나중에 좀 더 생각 해 보겠습니다.

@leehj050211
Copy link
Member

leehj050211 commented Jul 29, 2022

Buffer는 인터넷에서 DB에 UUID 저장할 때 binary로 타입을 지정해서 저장하면 용량을 적게 먹는다고 해서 그렇게 했는데, 써보니 16진수 변환하는게 조금 귀찮아서 나중에 String으로 변경할 생각입니다.

@leehj050211
Copy link
Member

leehj050211 commented Jul 29, 2022

릴레이션은 그동안 DB에서만 외래키를 사용했었고 ORM에서 릴레이션을 맺은 것은 이번 프로젝트가 처음이었습니다.
사실 ManyToOne 같은 걸로 해도 됐었는데, 예를 들어 팀에 소속된 멤버 목록을 가져올 때 팀 id로 선택을 해서 가져오고 싶은데 팀 엔티티로 선택을 해야 되길래 인터넷으로 찾아보았습니다.

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

No branches or pull requests

2 participants