nomik94/gyeonggi

gyeonggi's Issues

로직 이동

gyeonggi/controllers/signup.controller.js

Line 12 in 56711c7

const re_email = /^[a-z0-9]+@[a-z]+\.[a-z]{2,3}$/;

유효성 검사에 대한 로직은 service 쪽으로 함수를 따로 빼는것이 좋아 보입니다.

검증하는 함수에서는 통과하지 못할시 error 를 throw 하고 controller 에서는 error 를 catch 해서 res 를 응답하면 좋을듯 합니다.

redirect 는 백엔드 에서 하지 않습니다.

gyeonggi/middlewares/auth-middleware.js

Line 17 in 56711c7

.redirect('/api/login');

백엔드와 프론트엔드가 분리된 상황에서 백엔드가 redirect 를 사용한다면 이후 모바일 앱등에 경우에서는 사용할수 없습니다.

findStartWork

gyeonggi/services/bossPage.service.js

Line 8 in 56711c7

const allStartWork = await this.bossPageRepository.findAllStartWork(userId);

status

• 상태 (status) 가 숫자가 아닌 의미가 있는것으로 넣어도 좋을듯 합니다. 의미가 필요한 상태에 숫자로 상태를 넣는것은, 용량을 줄일수 있지만, 그 효과는 미비 하며 오히려 유지보수할때 의미있는 값이 도움이 됩니다.

코드 리팩토링

findStartWork = async (userId) => {
    const allStartWork = await this.bossPageRepository.findAllStartWork(userId);
    const status = [
      '수거 중',
      '수거 완료',
      '배송 중',
      '배송 완료',
      '리뷰 확인',
    ]

    return allStartWork.map(StartWorks => ({
        ...StartWorks,
      status: status[StartWorks.status - 1] || "알수 없는 상태",
    })).sort((prev, next) => prev.createdAt - next.createdAt);
  };

• for / map 으로 의미없이 루프를 2번 돌릴 필요없습니다.
• a , b 와 같은 의미없는 변수는 지양합니다.
• StartWorks 같을 그대로 넘길거면 ...StartWorks 를 사용해도 됩니다.
• 의도지 않은 에러방지를 위해 알수 없는 상태 를 넣습니다.

간단한 로직 return 함수는 변수 할당 하지 않아도 됩니다.

gyeonggi/repositories/users.repository.js

Line 5 in 56711c7

class UserRepository {

createUser = async(userType, email, phoneNumber, hashPassword, name, address, point) => {
        return User.create({ userType, email, phoneNumber, password:hashPassword, name, address, point });
    }

    findOne = async(email, hashPassword) => {
        return User.findOne({
            where: {email, password:hashPassword}
        });
    }

    findAllUser = async(name) => {
        return User.findAll({
            where: { name }
        });
        
    }

return 에서 await 는 생략 가능합니다 (단 특별한 에러 처리할때 약간 다른 처리가 되는데, 이 부분은 이후에 학습 한번 해보세요)

알수 없는 에러

gyeonggi/controllers/signup.controller.js

Line 68 in 56711c7

res.status(400).json({

이미 데이터 유효성은 위에서 검증했고, 또한 createUserData 에서 에러가 나는 경우엔 데이터 형식에 대한 에러가 아닐수 있기때문에 다른 에러값을 정의하는것이 좋아보입니다.

findAllStartWork

gyeonggi/repositories/bossPage.repository.js

Line 10 in 56711c7

const user = await this.BossModel.findOne({

user 를 가져오는 부분과 works 를 가져오는 부분은 단일책임원칙에 의해 함수로 분리 되는것이 좋습니다.

아래 부분에 for 문으로 user / works 를 합치는데, hasMany 를 통해 처음부터 join 을 해서 select 하면 좀더 깔끔할듯 합니다.

통합테스트 코드 범위

gyeonggi/__tests__/integration/boss.review.integration.spec.js

Line 16 in 8affc49

expect(response.body).toEqual({

해당 테스트의 주석처럼, "계속 변경되는값"을 테스트로 검증하게 되면, 에러가 아닌 부분이 지속적으로 에러가 나는 가짜 테스트 실패가 나오게 됩니다.

이런 검증은 하지 않는게 좋습니다.

테스트 종합

• 통합 테스트 (e2e) 에서 응답값들만 제대로 내려주는 지 확인하는 테스트만 추가 되어도 안정성이 올라갑니다.

심화

mocking 하는 범주를 지정하는것도 노하우 입니다.
런던파, 고전파 로 테스트 mocking 하는 범주를 공부 해 보시면 좋은 경험이 될수 있습니다.

전체 공통

• readme 를 작성 해보시면 좋겠습니다.
• 사용하지 않는 import require 변수는 모두 삭제 해 주세요.
• backend api 에서는 redirect 를 사용하지 않습니다. (render 또한 사용하지 않습니다)

Repository 종합

for 문보다 map 을 이용

map을 이용하여 좀더 함수형 프로그래밍에 가깝고 간결한 코딩을 할수 있습니다.

하나의 함수는 한 가지 역할만 하도록 하면 좋습니다

하나의 함수에서 여러 역할을 담당하면 유지 보수가 어려우며, 코드가 복잡해지고, 테스트코드를 짜기 어렵습니다.

관계형 DB 에서는 hasMany, hasOne 을 사용하세요

n+1 problem 에 대해 공부해보시고, eager 로딩을 통해 관계형 데이터 함수를 이용해보세요.

동일 로직 중복 작성 지양

gyeonggi/services/updateWork.service.js

Line 8 in 56711c7

updateWork = async (workId, status, userId) => {

해당 로직은 다른 repository 파일에도 있습니다.
이럴경우 상태값 정의가 변경되면 모두 찾아서 변경해야하는 유지보수의 어려움이 발생하고 그로인해 버그의 원인이 됩니다.

같은 로직은 공통함수로 빼서 사용하는것이 좋습니다

상태값 명시

gyeonggi/repositories/updateWork.repository.js

Line 10 in 56711c7

if (status === 3) {

status === 3 이후 보면 3이란 값이 무슨값인지 몰라서 해당 로직이 이해가 가지 않을수 있습니다. (처음 보는 저도 물로 코드만 보고 왜 10000을 더해주는지 몰라요)

error 처리

gyeonggi/repositories/bossReview.repository.js

Line 8 in 56711c7

try {

try catch 로 발생한 에러처리를 아무것도 하지 않고 상위에서는 하지 않는데, 굳이 try catch 로 할 필요가 없어 보입니다.

스테이크 케이스 혼용

gyeonggi/repositories/workshow.repository.js

Line 19 in 56711c7

'user_id',

snake case (user_id) 와 camel case (userWanted)를 혼용하는건 좋지 않습니다.

로직 이동

gyeonggi/controllers/login.controller.js

Line 11 in 56711c7

const hashPassword = crypto.createHash('sha512').update(req.body.password + 10).digest('hex');

해당 로직은 loginService.findOne에 들어가있는게 어떨까 합니다.

사용하지 않는 변수는 제거 해주세요

gyeonggi/repositories/userReviewRepository.js

Line 14 in 56711c7

const work = await this.workModel.findByPk(workId);

사용하지 않을 변수인데 DB 에서 데이터를 가져오고 있습니다.
클린코드 관점에서도 퍼포먼스상으로도 좋지 않습니다.

응답과 요청

gyeonggi/middlewares/auth-middleware.js

Line 27 in 56711c7

res.locals.user = user;

user 정보를 넣는곳은 의미상 요청에 들어가는게 맞습니다.
즉 req.locals.user = user 로 해야합니다.

클린 코드

gyeonggi/controllers/signup.controller.js

Line 57 in 56711c7

const createUserData = await this.signupService.createUser(

인자가 4개를 초과 하는 경우 object 로 인자를 넘기는게 좋습니다.