저번에 시도했던 투두 리스트 리팩토링을 시도해봤다.
원래는 자바스크립트 조금 더 공부하고, 오늘은 CS 공부를 하는 날이였으나
유튜브에서 [우아한테크세미나] 190425 TDD 리팩토링 by 자바지기 박재성님 를 보고 아주 리팩토링 뽕에 차서 시도해봤다.
시도하기 전
나는 디자인 패턴이 뭔지도 모른다.
그리고 좋은 코드란게 뭔지도 모른다.
그냥 . . 계란으로 바위깨기 형식으로 시도해봤으나
역시나 대차게 깨졌다.
아무것도 모르던 내가 생각하던 리팩토링
리팩토링의 정의를 위키 백과에서 검색해서 읽어보고 지피한테도 물어봤다.
아 ~ 가독성 좋고 코드의 불필요한 반복을 제거해야겠군
생각했다.
그리고 리팩토링 전 객체지향 프로그래밍 에 대해서도 조금 읽어봤다. 아주 조금 ..
아 ~ 나는 객체 지향 프로그래밍을 하니까 하나의 객체로 다 때려부셔야지 ㅋㅋ
이러고 생각했다. .. 그러지 말았어야했다.
아니 적어도 더 공부하고 했어야 했다.
대차게 실패하는 과정
문제의 발단
1. 동일한 것을 반복하는게 너무 불편하다고 느꼈다.
이전 코드에서
$typedGoal.classList.add('typed-goal');
$typedGoal.id = 'uncompleted';
$typedGoal.textContent = $input.value;
$innerWrapper.appendChild($typedGoal);와 같이, 새로운 목록이 추가 될 때 마다 부모 노드에 추가학기 전 클래스 명, 아이디명, 콘텐츠 값 등을 하나하나 반복적으로 적어줘야 했다.
그래서 , 그럴 필요 없이 객체로 만들고 인수로 넘겨버리자고 생각했다.
class Node {
constructor(
tag,
value = null,
idName = null,
className = null,
existed = false,
) {
this.node = existed
? this.selectExistedNode(idName, className)
: this.createNewNode(tag, idName, className, value);
}
selectExistedNode(idName, className) {
const identifer = idName ? `#${idName}` : `.${className}`;
return document.querySelector(identifer);
}
createNewNode(tag, idName, className, value) {
const node = document.createElement(tag);
if (idName) node.id = idName;
if (className) node.classList.add(className);
if (value) node.textContent = value;
return node;
}
appendNode(obj) {
this.node.appendChild(obj.node);
}
removeNode(obj) {
this.node.removeChild(obj.node);
}
editValue(text) {
this.node.textContent = text;
}
changeStyle(key, style) {
this.node.style[key] = style;
}
getText() {
return this.node.value;
}
}그래서 인수만 넘기면 알아서 슈슈슉 아이디명, 클래스명, 콘텐츠 값 등을 설정하고, 만약 존재하는 노드일 경우엔 새로 만들지 말고 찾아오라고 했다.
여기서부터가 문제였다. 추상화를 해도 너무나도 추상화했다.
타임머신이 있다면 타고 날라가서 나한테 싸커킥을 후려 떄리고 싶다.
2. 전역 변수를 너무 쓰는건 오바참치 아닌가
기능이 매우 단순한 투 두 리스트임에도 불구하고 태그들을 전역에서 선언해서 쓰는 것이
마치 옷을 발가벗고 동네를 돌아댕기는 꼬마마냥 부끄럽게 느껴졌다.
그래서 클래스 내부의 인스턴스로 만들었다.
리팩토링 시작
class ToDoList {
constructor() {
this.progressBar = new Node('div', null, 'progress-bar', null, true);
this.progressText = new Node('span', null, 'progress-text', null, true);
this.input = new Node('input', null, 'input', null, true);
this.textSubmit = new Node('button', null, 'text-submit', null, true);
this.content = new Node('div', null, 'content', null, true);
this.totalGoals = 0;
this.completedGoals = 0;
this.timer;
this.updateProgressText();
this.updateProgressBar();
}사실 여기까지만 했을 때, 내가 잘하고 있는줄 알았다.
어쩌면 나는 추상화의 귀재일지도 .. 이러고 있었다.
그 다음 이전 코드들의 메소드 방식을 참조해서 하나하나 만들었다.
여기까지는 리팩토링 이전과 차이가 없다.
// header 부분의 progress-text 관련 메소드
updateProgressText() {
this.progressText.editValue(`${this.completedGoals}/${this.totalGoals}`);
}
// header 부분의 progress-bar 관련 메소드
updateProgressBar() {
const accomplish = Math.round(
(this.completedGoals / this.totalGoals) * 100,
);
this.progressBar.changeStyle('width', `${!accomplish ? 0 : accomplish}%`);
this.progressBar.editValue(accomplish ? accomplish : '');
if (accomplish === 100) {
this.progressBar.changeStyle('box-shadow', '0 0 10px red');
return;
}
this.progressBar.changeStyle('box-shadow', '');
}
// input 의 Enter키가 눌리면 textSubmit 의 click 이벤트를 발생시키는 메소드
enterToSubmit(event) {
if (event.key !== 'Enter') return;
this.textSubmit.node.click();
}그냥 전역에서 선언된 함수들을 클래스 내부에서 호출하는 것과 다른 점이 없다.
좀 차이가 있다면 괜히 추상화 해보겠다고 DOM 의 프로토타입 메소드로 하면 되는 것을
내가 추상화 한 Node 객체의 프로토타입 메소드로 변경해서 적었다.
사실 여기까지만 해도 ,
ㅋㅋ 어 ~ 그럴 수 있어 ~ 그게 객체 지향 프로그래밍이야 ㅋㅋ ~이랬다.
여기서부터 문제가 심각해짐을 느꼈다.
addGoal() {
// 만약 input 에 적힌 값이 없다면 시행하지 않기
if (!this.input.getText()) return;
// content에 innerWrapper 추가하기
const innerWrapper = new Node(
'div',
null,
`inner-wrapper${this.totalGoals + 1}`, // 목표를 추가 할 때 마다 id 값 생성
null,
);
this.content.appendNode(innerWrapper);
requestAnimationFrame(() => {
innerWrapper.changeStyle('width', '90%');
innerWrapper.changeStyle('height', '30px');
});
// buttonWrapper에 button 넣기
const buttonWrapper = new Node('div', null, null, 'button-wrapper');
const completeButton = new Node(
'button',
'⭕',
`${this.totalGoals + 1}`,
'complete',
);
const deleteButton = new Node(
'button',
'❌️',
`${this.totalGoals + 1}`,
'delete',
);
[completeButton, deleteButton].forEach((button) =>
buttonWrapper.appendNode(button),
);
// innerWrapper에 input에 적힌 값 넣기
const typedGoal = new Node(
'div',
this.input.getText(),
`typed-goal${this.totalGoals + 1}`,
'uncompleted',
);
innerWrapper.appendNode(typedGoal);
requestAnimationFrame(() => {
typedGoal.changeStyle('opacity', '1');
});
// innerWrappe에 button 넣기
innerWrapper.appendNode(buttonWrapper);
requestAnimationFrame(() => {
buttonWrapper.changeStyle('opacity', '1');
});
// 목표 수와 progress bar 변경하기
this.totalGoals += 1;
this.updateProgressBar();
this.updateProgressText();
// 목표를 정하면 input 값 비우기
this.input.node.value = '';
}해당 코드가 뭘 하는지 알 것 같은가 ?
인풋에서 목표를 적고 버튼을 누르면 적힌 버튼이 투두 리스트에 나오는 코드이다.
예전에 전역에서 함수들을 선언 할 때는, 함수들이 한 기능만 하도록 잘 만들었으나
괜히 클래스 내부에서 선언하려고 하니 ,
함수들을 쪼개서 객체의 프로토타입으로 선언하면 해당 객체의 프로토타입으로 보기 힘들지 않을까? 이런 생각을 했다.
그러니까 무슨 말이냐면 리팩토링 이전에는 전역에서
// inner-wrapper 를 생성하여 천천히 content 에 띄우는 함수
const createInnerWrapper = () => {
const $innerWrapper = document.createElement('div');
$innerWrapper.classList.add('inner-wrapper');
$content.appendChild($innerWrapper);
requestAnimationFrame(() => {
$innerWrapper.style.width = '90%';
$innerWrapper.style.height = '30px';
});
return $innerWrapper;
};
// 목표를 설정하면 설정되어 있는 목표가 typed-goal 에 적히는 함수
const createTypedGoal = (innerWrapper) => {
const $innerWrapper = innerWrapper;
const $typedGoal = document.createElement('div');
$typedGoal.classList.add('typed-goal');
$typedGoal.id = 'uncompleted';
$typedGoal.textContent = $input.value;
$innerWrapper.appendChild($typedGoal);
requestAnimationFrame(() => {
$typedGoal.style.opacity = '1';
});
};
// button-wrapper 를 생성하여 천천히 inner-wrapper에 띄우는 함수
const createButtonWrapper = (innerWrapper) => {
const $buttonWrapper = document.createElement('div');
$buttonWrapper.classList.add('button-wrapper');
innerWrapper.appendChild($buttonWrapper);
const $complete = document.createElement('button');
const $delete = document.createElement('button');
[$complete.textContent, $delete.textContent] = ['⭕', '❌️'];
[$complete.id, $delete.id] = ['complete', 'delete'];
[$complete, $delete].forEach((button) => {
const $button = button;
$buttonWrapper.appendChild($button);
requestAnimationFrame(() => {
$buttonWrapper.style.opacity = '1';
});
});
};
// submit 버튼이 click 되면 input 의 글을 가지고 content의 자식 노드를 생성하는 함수
// 목표가 설정되면 전체 목표 개수가 1 올라가고 ProgressText 의 변화를 주기
const setGoal = () => {
if ($input.value === '') return; // 적힌 값이 없으면 early return
const $newGoal = createInnerWrapper();
createTypedGoal($newGoal);
createButtonWrapper($newGoal);
totalGoals += 1;
updateProgressText();
updateProgressBar();
$input.value = ''; // 태그를 추가한 후에는 input 값 초기화
};이런식으로 함수가 한 기능을 하도록 잘게 쪼개고 setGoal 이란 함수 내부에 총 집합 시켜주었었는데
ToDoList 라는 클래스에서 저 잘개 쪼개진 메소드들을 넣으면, 그건 ToDoList 객체가 가지기엔, 너무나도 작은 메소드들이라고 생각했다.
그랬드니 이런 똥코드가 나타났다.
그래도 다행인점은 아무런 문제 없이 기능은 하드라
그래서 사실 여기까지도 문제 의식을 못느꼈다.
잘 날라가고 있는줄 알았는데 추락하는 중이였음요
// ⭕ 버튼이 눌리면 성취된 목표로 변경하는 함수
completeGoal(id) {
const typedGoal = document.querySelector(`#typed-goal${id}`);
const innerWapper = document.querySelector(`#inner-wrapper${id}`);
typedGoal.className = 'completed';
typedGoal.style.textDecoration = 'line-through';
typedGoal.style.color = '#aaa';
innerWapper.style.boxShadow = '0px 0px 5px blue';
}
// ❌️ 버튼이 눌리면 삭제된 목표로 변경하는 함수
deleteGoal(id) {
const innerWrapper = document.querySelector(`#inner-wrapper${id}`);
console.log(innerWrapper);
[...innerWrapper.children].forEach((tag) => innerWrapper.removeChild(tag));
setTimeout(() => {
this.content.removeNode(innerWrapper);
}, 3000);
innerWrapper.style.padding = '0px';
requestAnimationFrame(() => {
innerWrapper.style.width = '0%';
innerWrapper.style.height = '0%';
});
}
changeGoal(event) {
if (event.target.tagName !== 'BUTTON') return;
const button = event.target;
const id = button.id;
const status = button.className;
const typedGoals = document.querySelector(`#typed-goal${id}`);
if (status[0] === 'c') {
this.completeGoal(id);
this.completedGoals += 1;
button.id = '';
} else {
this.deleteGoal(id);
if (typedGoals.className === 'completed') this.completedGoals -= 1;
this.totalGoals -= 1;
}
this.updateProgressText();
this.updateProgressBar();
}
}여기서부터가 문제의 서막이였다.
버튼을 눌렀을 때 목표가 성취되거나 삭제되는 이벤트 핸들러를 content 영역에서 위임한 후, 눌린 버튼의 부모 노드, 형제 노드를 참조해서 노드들을 동적으로 제거해줬어야 했다.
리팩토링 하기 전에는 문제가 없었던 것이 Node 객체로 추상화 했을 때 문제가 발생했다.
내 Node 객체에서는 부모 노드, 형제 노드의 개념이 존재하지 않는다.
그렇기 때문에 Node 객체를 수정하느라 1시간을 썼다.
1시간을 수정해도 부모 노드 , 형제 노드의 개념을
Node객체에서 구현하는데 실패했다.
그래서 결국 Node 객체를 만들어 놓고도 리팩토링 이전과 거의 동일한 타입으로 코드들을 생성했다.
이 때부터 리팩토링이 실패를 향해 달려가고 있단걸 느끼고 집중력이 많이 떨어졌다.
그래서 변수명도 걍 댕같이 짓고, 변수명과 비슷한 메소드명도 짓고 아주 난리였다.
그래서 그냥 이때부터는 구현에만 힘쓰고 아주 대애애애애애ㅐ애애애앵ㅇ애애박 똥 코드를 싸질렀다.
회고
내 코드가 안읽힌 가장 큰 문제점은
class Node {
constructor(
tag,
value = null,
idName = null,
className = null,
existed = false,
) {
this.node = existed
? this.selectExistedNode(idName, className)
: this.createNewNode(tag, idName, className, value);
}
selectExistedNode(idName, className) {
const identifer = idName ? `#${idName}` : `.${className}`;
return document.querySelector(identifer);
}
createNewNode(tag, idName, className, value) {
const node = document.createElement(tag);
if (idName) node.id = idName;
if (className) node.classList.add(className);
if (value) node.textContent = value;
return node;
}바로 이 쓸데없이 새로운 객체를 만들어버리려고 했던 것이 문제였던 것 같다.
이놈을 시작으로 아주 코드들이 똥같이 나왔다.
챗지피티한테 추상화의 함정 에 대해 설명해줘 .. 하니까
그냥 내 코드 그 잡채 ..
슬퍼요
느낀점
그냥 아무것도 모르면서 해보겠다고 까불다가 시간만 버렸다.
바로 책 찾아서 구매했다.
선생님 저에게 힘을 주세요
자바스크립트 공부부터 우선 끝내고 읽어봐야겠다. 아직 자바스크립트로 API 다루는 법, HTTP 요청 보내고 그러는 것도 모르면서 괜히 코드 질만 욕심내고 있다.
지금 나는 구현부터 열심히 생각 할 때니까 ..
저거는 틈틈히 이동 할 때 마다 읽고 나중에 각잡고 읽어봐야겠다.
그래도 내가 망가뜨린 장난감 수 만큼 성장한다고 했다. 오늘 똥 싼만큼 성장 했기를 ..
