클린코드 적용하기

Javascript의 Clean code 적용기

이 글을 clean code 한글판을 보고 나서 개인적으로 느낀 점 혹은 지금까지의 코드 중 개선이 필요하다고 느끼는 부분에 대해서 쓴 글입니다. 혹시나 해당 원글을 보고 싶으신 분은 여기 를 클릭하시면 됩니다.

항상 읽기 좋은 코드에 대해서 이야기를 하다 최근 회사에서의 코드 리펙토링을 하며, 아직 많은 부족한 점을 느껴, clear code에 대해 한글로 번역해둔 글을 읽고 리뷰를 시작해보았다. 위의 글을 보고 개인적으로 느꼈던 점에 대해서만 쓰고 있으므로, 어디까지나 개인적인 스타일 혹은 주관적임을 미리 밝힙니다.

문맥상 필요없는 것들을 쓰지 마세요

1
2
3
4
5
6
7
8
9

const Car = {
  carMake: 'BMW',
  carModel: 'M3',
  carColor: '파란색'
};

function paintCar(car) {
  car.carColor = '빨간색';
}

위의 코드를 보고 개선점에 대해서 보았을 때, 사실 나는 위와 같이 불필요한 것들도 같이 사용해주었다. 개인적으로는 변수나 함수 명을 길게 쓰는데에 대한 거부감이 없었고, 대략적으로나마 변수명의 프로퍼티 명만 보고도 어느 정도 이해가 되어야 한다고 생각했다. 하지만 위의 코드가 아래와 같이 개선되었다고 해도, 문맥상 어느 정도 이해가 된다는 것을 깨달았다.

1
2
3
4
5
6
7
8
9

const Car = {
  make: 'BMW',
  model: 'M3',
  color: '파란색'
};

function paintCar(car) {
  car.color = '빨간색';
}

함수는 하나의 행동만 해야합니다

이 부분에 대해서는 과거 SOLID 원칙에 대해서 설명을 하면서도 항상 관심사 분리 측면에서 지키려 하되 항상 제대로 지킨다는 것이 쉽지 않다. 하지만 클린 코드에서도 설명하듯이 소프트웨어 엔지니어링에서 가장 중요한 규칙 이라고 할 정도로 기본이자 중요한 것이지미나, 개인적으로 생각하기엔 제일 지켜지지 쉽지 않은 규칙이지 않을까 라는 생각을 한다.

1
2
3
4
5
6
7
8

function emailClients(clients) {
  clients.forEach(client => {
    const clientRecord = database.lookup(client);
    if (clientRecord.isActive()) {
      email(client);
    }
  });
}

사실 단순하게 위쪽의 코드만 본다면 그냥 가독성이 좋지 않구나 뿐, 관심사가 제대로 분리가 안되었구나 라는 생각은 잘 들지 않는다. 하지만 자세히 보면 client를 database에서 찾고, 찾은 후 그 유저가 active한 유저라면 이메일을 보낸다 라고 되어 있다. 하나의 함수에서 database에서 유저를 찾고, 메일도 보내고 있다. 이렇게 뜯어 놓고 본다면 위에서 이야기하는 함수는 하나의 행동만 해야한다 라는 원칙을 위반했다는 것을 알 수 있다.

1
2
3
4
5
6
7
8
9
10

function emailClients(clients) {
  clients
    .filter(isClientActive)
    .forEach(email);
}

function isClientActive(client) {
  const clientRecord = database.lookup(client);
  return clientRecord.isActive();
}

함수는 단일 행동을 추상화 해야합니다

OOP를 주장하며 꼭 빠지지 않고 나오는 추상화 혹은 캡슐화에 연관 관계가 가장 깊은 원칙이지 않을까 라는 생각을 한다. 또한 위의 링크에서 역시 강조하는 내용 중 가장 큰 공감을 하는 것은 함수들을 나누어서 재사용가능하고 테스트 가능하도록 이라는 말이 가장 공감이 가는 내용이다. 테스트가 가능한 코드를 만드는 것이 내포하는 그 중요성에 대해 공감을 한다.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22

function parseBetterJSAlternative(code) {
  const REGEXES = [
    // ...
  ];

  const statements = code.split(' ');
  const tokens = [];
  REGEXES.forEach(REGEX => {
    statements.forEach(statement => {
      // ...
    });
  });

  const ast = [];
  tokens.forEach(token => {
    // lex...
  });

  ast.forEach(node => {
    // parse...
  });
}

개선 전의 위에서의 코드를 본다면, 하나의 인자를 받아 하나의 함수에서 모든 것을 다 하고 있다. 코드가 돌아가기 전까지는 정확히 무슨 역할을 하는 함수인지 바로 보고 파악하기는 힘들지만 대략적으로 함수명을 보고 판단한다면 js파일을 뭔가 더 좋아보이도록 파싱을 해주는 함수일 것 이다.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32

function tokenize(code) {
  const REGEXES = [
    // ...
  ];

  const statements = code.split(' ');
  const tokens = [];
  REGEXES.forEach(REGEX => {
    statements.forEach(statement => {
      tokens.push( /* ... */ );
    });
  });

  return tokens;
}

function lexer(tokens) {
  const ast = [];
  tokens.forEach(token => {
    ast.push( /* ... */ );
  });

  return ast;
}

function parseBetterJSAlternative(code) {
  const tokens = tokenize(code);
  const ast = lexer(tokens);
  ast.forEach(node => {
    // parse...
  });
}

중복된 코드를 작성하지 마세요

중복 코드에 대한 이야기는 개발자마다 보기에 따라 다른 성향을 가지는 듯 하다. 물론 이 또한 리팩토링의 시기를 어떻게 보느냐에 따른 관점도 있겠지만, 모든 중복 코드가 잘 못 되었다 라고 생각하지는 않는다. 물론 완전히 동일한 로직을 처리하는 중복 코드라면 잘 못 되었겠지만, 부분적으로 다른 후처리를 하는 로직도 존재할 것이다. 그래서 그러한 부분들은 리팩토링 시기에 고쳐져야 한다고 생각하며, 평균적으로 중복 코드가 3번 정도 반복되어 진다면 그 때가 리팩토링 시기이지 않을까 싶다. 물론 이 또한 개인적인 견해일 뿐, 객관적인 사실은 아니다.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29

function showDeveloperList(developers) {
  developers.forEach(developers => {
    const expectedSalary = developer.calculateExpectedSalary();
    const experience = developer.getExperience();
    const githubLink = developer.getGithubLink();
    const data = {
      expectedSalary,
      experience,
      githubLink
    };

    render(data);
  });
}

function showManagerList(managers) {
  managers.forEach(manager => {
    const expectedSalary = manager.calculateExpectedSalary();
    const experience = manager.getExperience();
    const portfolio = manager.getMBAProjects();
    const data = {
      expectedSalary,
      experience,
      portfolio
    };

    render(data);
  });
}

위와 같이 두 개의 함수가 있다. 하나는 개발자의 리스트를 보여주는 함수이고, 다른 하나는 매니저의 리스트를 보여주는 함수이다. 두 함수를 봤을 때, 거의 대부분이 중복 코드인데, 딱 한가지만 서로 다르다. 개발자 리스트 함수는 githubList를 가져와야 하고, 매니저 리스트 함수에서는 포트폴리오를 가져와야 한다. 위의 부분만 봐도 일부 중복 코드가 존재한다.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20

function showEmployeeList(employees) {
   employees.forEach((employee) => {
     const expectedSalary = employee.calculateExpectedSalary();
     const experience = employee.getExperience();
 
     let portfolio = employee.getGithubLink();
 
     if (employee.type === 'manager') {
       portfolio = employee.getMBAProjects();
     }
 
     const data = {
       expectedSalary,
       experience,
       portfolio
     };
 
     render(data);
   });
 }

사실 위의 코드 역시 그렇게 확정성이 있어 보이는 코드는 아니다. 만약 또 다른 타입의 Employee가 추가된다고 하면 아마도 분기를 또 태워야 할 것이다.

매개변수로 플래그를 사용하지 마세요

직접 만든 코드를 보면 종종 이러한 매개변수를 플래그를 이용해서 사용하는 경우가 있고 때로는 분기를 태우기 위한 인자로 던질 때가 있다. 그런데 플래그로 사용하는 경우 클린 코드에서 말하듯 함수 자체가 한가지 이상의 일을 한다는 것을 의미한다고 한다.

1
2
3
4
5
6
7

function createFile(name, temp) {
  if (temp) {
    fs.create(`./temp/${name}`);
  } else {
    fs.create(name);
  }
}

위와 같이 temp의 값 여부에 따라 피일을 생성하는 함수가 있다면 위에 처럼 분기를 태우지 말라는 의미이다.

1
2
3
4
5
6
7

function createFile(name) {
  fs.create(name);
}

function createTempFile(name) {
  createFile(`./temp/${name}`);
}

차라리 위와 같이 별개의 함수를 만들어서 관리를 하게 되면 말 그대로 하나의 함수에서 하나의 역할만 하고 있는 것을 볼 수 있다. 이후의 확장성을 생각했을 경우에도 위와 같이 별개의 함수로 빼는 것이 훨씬더 효율적일 수 있음을 깨달을 수 있다.

사이드 이펙트를 피하세요

일단 사이드 이펙트 라는 의미를 WIKI 백과에 찾아보면 부작용, 역효과 라는 의미를 가진다. 하지만 프로그래밍에서의 사이드 이펙트란 꼭 부정적인 의미는 아니다. 프로그래밍 언에서의 사이드 이펙트는 다음과 정의하고 있다.

Accessing an object designated by a volatile lvalue, modifying an object, calling a library I/O function, or calling a function that does any of those operations are all size effects, which are changes in the state of the execution environment. 즉, 라이브러리 I/O 함수를 호출하거나 객체 변경 등 이러한 행위들과 같이 실행 중에 어떤 객체를 접근해서 변화가 일어나는 행위를 말한다.

그래서 함수와 같은 경우에는 값을 받아서 어떠한 일을 하거나 혹은 리턴을 시킬 때 사이드 이펙트를 만든다라고 볼 수 있다. 하지만 그 함수가 개발자가 예측하지 못하는 사이드 이펙트를 만든다고 하면 아마 문제가 커질 것이다.

1
2
3
4
5
6
7
8
9

let name = 'Ryan McDermott';

function splitIntoFirstAndLastName() {
  name = name.split(' ');
}

splitIntoFirstAndLastName();

console.log(name);

위의 예제를 보면 name 이라는 변수에 한 사람의 이름이 전역적으로 선언되어 있으며, 함수 splitIntoFirstAndLastName 에서는 함수명을 통해 알 수 있듯이
성과 이름을 분리 시켜주는 역할을 하는 함수이다. 그 안에서 처리되는 부분을 보면 전역적으로 선언되어 있는 이름을 바꿔서 다시 전역 변수에 저장을 시켜버린다. 만약 전역적으로 사용하는 변수가 한 곳뿐만 아니라 다른 쪽에서도 같은 변수를 바라보고 작업을 한다고 하면 분명 서로 원인 파악하기조차 하기가 힘들어 질 것이다.

1
2
3
4
5
6

function splitIntoFirstAndLastName(name) {
  return name.split(' ');
}

const name = 'Ryan McDermott';
const newName = splitIntoFirstAndLastName(name);

위와 같이 개선이 된다고 한다면 전역 변수가 오염될 일도 없을 것이다. 물론 전역변수 자체를 사용하는 것 자체가 위험한 행동이기는 해서 전역변수를 모듈에서 바라보면서 작업할 일은 없을 것이다.

두번째는 위와 같은 전역 변수의 이야기가 아니라 객체와 배열과 같은 참조형 변수와 같은 케이스이다. 변수와 배열은 항상 참조 주소를 공유한다. 이 글을 참조한 Github 블로그에서는 상품 구매에 대한 예제를 들었다.

1. 구매 버튼을 누르면 장바구니 배열을 서버에 보내며 결제를 유도한다.
2. 장바구니 추가 버튼을 누르면 장바구니 배열에 아이템을 추가한다.

둘 사이의 공통점을 보면 장바구니 라는 것이 있다. 낙관적으로 항상 최상의 환경에서는 문제가 안되겠지만, 항상 낙관적인 상황만 있을 순 없다. 만약 사용자가 구매 버튼을 누르자마자 실수로 장바구니 추가를 눌렀다. 그런데 구매 버튼을 눌렀을 때, 네트워크 환경이 좋지 않아 통신이 실패했다. 그래서 다시 구매 함수에 대해 서버에 요청해야할 일이 생겼다. 그렇다면 실수로 추가된 장바구니의 배열을 서버에 보내게 된다. 이러한 문제를 피하기 위해 깊은 복사를 해야하며, 이러한 부분에 대한 좋은 라이브러리도 있다.

1
2
3

const addItemToCart = (cart, item) => {
  cart.push({ item, date: Date.now() });
};

위와 같이 함수를 만든다면 원본의 배열에도 동일한 반영이 될 것이다.

1
2
3

const addItemToCart = (cart, item) => {
  return [...cart, { item, date : Date.now() }];
};

출처

로버트C.마틴 저자 - Clean code
Clean Code in Javascript Github