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

[클린코드 2기 소인성] 자동차 경주 미션 STEP1 ~ 3 #62

Merged
merged 47 commits into from
Apr 13, 2022

Conversation

InSeong-So
Copy link
Member

@InSeong-So InSeong-So commented Apr 2, 2022

안녕하세요, 클린코드 2기 소인성 STEP1 ~ 3 제출합니다.

피드백 적용

  • 오버 엔지니어링을 피하고 최대한 미션에 집중했습니다.

내용

  • 마우스 없이 키보드로도 입력 가능하게 focus, disabled, form 이벤트를 넣었습니다.
  • 소소하게 ease-in-out 애니메이션을 넣었습니다.
  • Promise, async/await로 setInterval을 대체했습니다.
  • pipe 함수를 활용했습니다.
  • 사용자가 편하게 작업할 수 있도록 disabled, focus, form-validation을 작업했습니다.

구현

실행화면 바로가기

웹 VSCode로 보기


- ESLint, Prettier, @babel/eslint-parser
- Cypress
- 테스트 케이스 정의
- 인터페이스를 Proxy로 구현
- 도메인 모델 정의에 대한 컨벤션 작성
- 함수형 파이프라인 추가
- 의존성 주입 방향 설정
- 이벤트 바인딩 엘리먼트 함수 rest로 받도록 수정
- carNames validation 추가
- 유효성 검사 로직 분리, 에러 메세지 추가
- diff 알고리즘 추가
- publish/subsribe 패턴 적용
- view를 담당하는 함수가 컨트롤러 인자를 받아 인스턴스(상태)에 따른 엘리먼트 변경이 되도록 수정
@InSeong-So InSeong-So changed the base branch from main to inseong-so April 2, 2022 08:36
Copy link

@roy-jung roy-jung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요, 리뷰어 정재남입니다.
구조에 많은 신경을 쓰신 것 같아요. 그런데 그러다보니 코드 복잡도가 너무 올라간 것 아닌가 싶습니다. 서비스 규모에 비해 코드를 분석하고 이해하는 데에 드는 노력이 많이 요구되어 리뷰하기가 참 어려웠네요.
여러가지 욕심을 내신걸로 보여요. virtual dom - real dom 비교, pub/sub, functional, prototype, proxy... 이런 것들이 이번 미션의 요구사항을 구현하는 데에 있어 필요했던 것인지, 혹은 그만한 가치가 있는지를 생각해보시고, 가능한 오버엔지니어링을 지양해 주시면 좋겠습니다.
제가 정신이 혼미하여 코드 전반에 대한 리뷰는 드리기가 어렵고, 몇가지 코멘트는 남겼어요.

@@ -0,0 +1,25 @@
export const isNil = value => value == null;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lodash의 영향을 받으신 걸까요? 그런데 lodash의 isNil 함수는 null뿐이 아닌 undefined도 체크를 합니다.
그렇지 않고 오직 null만 체크하는 목적이라면 isNull이 맞지 않나 싶어요.
나아가 == 보다는 ===로 비교하는게 형변환을 하지 않아 성능상 더 좋다고 합니다.

Copy link
Member Author

@InSeong-So InSeong-So Apr 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

미션 2를 수행하면서 isNull과 isNil을 어떻게 해당 앱에 녹일까를 고민하다가 포괄적인 isNil선택했었습니다.

리뷰어님 말씀대로 null만을 비교하기 위해선 isNull이 옳았지만 인자 존재 여부를 조건문의 !연산으로 체크하는 게 아니라 명시적으로 표현하고 싶었기 때문입니다. 사용 결과 최대한 억제하는 게 옳아 보입니다!

Comment on lines 3 to 15
export const diff = ($element, realNode, virtualNode) => {
if (isNil(realNode) && isNil(virtualNode)) return;
if (isOnlyExistRight(realNode, virtualNode)) return $element.append(virtualNode);
if (isOnlyExistLeft(realNode, virtualNode)) return realNode.remove();
if (isChangedNode(realNode, virtualNode)) return realNode.replaceWith(virtualNode);

const [realChildren, realLength] = getChildren(realNode);
const [virtualChildren, virtualLength] = getChildren(virtualNode);

for (let i = 0; i < Math.max(realLength, virtualLength); i++) {
diff(realNode, realChildren[i], virtualChildren[i]);
}
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다른 경우에는 괜찮지만, isOnlyExistRight의 경우에는 diff가 제대로 수행될지 의심이 듭니다.

$element real virtual result
[li.a, li.b, li.d] [li.a, li.b, li.d] [li.a, li.b, li.c, li.d] [li.a, li.b, li.d, li.c]
[li.b, li.c, li.d] [li.b, li.c, li.d] [li.a, li.b, li.c, li.d] [li.b, li.c, li.d, li.a]

이렇게 되는거 아닐런지..

Copy link
Member Author

@InSeong-So InSeong-So Apr 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트해보니 리뷰어님 말씀대로 노드 변화에 따른 diff 수행이 제대로 되고 있지 않네요. DOM 요소가 innerHTML 처럼 동작하고 있어서 의미 없는 비교가 되었습니다. 추후 많은 수정을 거쳐야 할 것 같아요.

diff를 넣은 것은 리뷰어님들의 리뷰를 받아보기 위함이었으니, 다음 피드백 때 해당 미션에 맞는 엔지니어링을 진행해볼게요.

Comment on lines 17 to 21
const getChildren = target => {
if (!target) return [[], 0];
const targetChildrens = Array.from(target.children);
return [targetChildrens, targetChildrens.length];
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

children과 length를 별도로 제공하는 이유가 궁금합니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드를 조금 더 깔끔하게 만들어볼 수 있지 않을까 하는 의도에서 탄생했던 형태입니다.


if ($realNodeAttributes.length !== $virtualNodeAttributes.length) return true;

for (let i = 0; i < $realNodeAttributes.length; i++) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

12번줄에서는 max로 기준을 잡으셨는데 여기서는 real만을 기준으로 잡은 이유는 뭔가요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

max Length를 기준으로 잡았어야 했는데 놓친 부분이 있었네요😭

for (let i = 0; i < $realNodeAttributes.length; i++) {
const $r = $realNodeAttributes[i].name;
const $v = $virtualNodeAttributes[i].name;
if (isEqualsAttribute(realNode, virtualNode, $r, $v)) return true;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이러면 하나라도 같은 attribute가 발견되면 early return되어서 나머지 attribute들이 같은지 다른지 여부를 체크하지 않게 되는거 아닌가요?
라고 적고 45번줄로 가보니, 변수명은 isEqual이라서 '같은 경우에 true, 다르면 false'일 걸로 예상했는데, 내용은 정반대네요..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혼란을 줄 수 있는 네이밍이네요. 수정했습니다☺

Comment on lines 13 to 15
export const isFirstChar = (target, char) => target.charAt(0) === char;

export const isLastChar = (target, char) => target.at(-1) === char;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DOM API 활용을 적극적으로 시도해보겠습니다🙏


constructor() {
this.#racingGame = new RacingGame();
this.#publisher.subscribe({ render: this.render.bind(this) });
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

render를 arrow function으로 바꿔주시면 bind(this)를 뺄 수 있고 동시에 성능상 이점도 얻을 수 있습니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

미처 생각하지 못하고 악습관대로 작성했네요. 수정하겠습니다!

@@ -0,0 +1,46 @@
import RacingGameService from '../services/racingGame.service.js';

export default class RacingGameController {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 클래스를 없애고 racingGameService만으로 운영하더라도 충분해 보이는데 어떻게 생각하시나요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

과한 래핑 중 하나라고 생각되며, 제거하는 것에 공감합니다.

moveCount: 0,
};

export default () => ({ __proto__: _interface_(CarModelProps) });
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/Object/proto
다른 방법 (setPrototypeOf 또는 Object.create)를 추천합니다.

그런데 정말 프로토타입이 필요한 상황인가요? 전체 서비스를 통틀어 인스턴스는 하나만 사용되는 것 같은데..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

proxy를 사용해서 타입스크립트의 타입 시스템을 흉내 내어 보았습니다. 도메인 모델을 interface로 작성해보려고 했던 시도였어요.

던더 프로토는 2주 전 세션 때 포코가 언급해주셔서 사용을 해봤습니다. setPrototypeOf나 Object.create를 시도해봐야겠네요.

Comment on lines 23 to 24
const winnedCount = Math.max(...this.#racingGame.cars.map(car => car.getMoveCount));
const winnedCars = this.#racingGame.cars.filter(car => car.getMoveCount === winnedCount);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

순회 한 번으로 찾아낼 수는 없을까요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고민해보겠습니다!

@InSeong-So
Copy link
Member Author

InSeong-So commented Apr 4, 2022

virtual dom - real dom 비교, pub/sub, functional, prototype, proxy... 이런 것들이 이번 미션의 요구사항을 구현하는 데에 있어 필요했던 것인지, 혹은 그만한 가치가 있는지를 생각해보시고, 가능한 오버엔지니어링을 지양해 주시면 좋겠습니다. 제가 정신이 혼미하여 코드 전반에 대한 리뷰는 드리기가 어렵고, 몇가지 코멘트는 남겼어요.

우선 정신이 혼미하셨다니 죄송스럽네요🥺

오버 엔지니어링이 맞았으나 좋은 리뷰어들께 리뷰를 받을 수 있는 유일한 시간이었으므로 스스로에겐 유의미한 시도였다고 생각합니다. 이번에 조금 무리하더라도 시도해보지 않았던 여러 기술들이나 구조들을 적용해보았고, 회사에서 사용하든 DDD나 헥사고날 아키텍처를 이해하기 위해 작은 규모의 앱임에도 최대한 고민하여 분리해보았습니다.

  • diff 의 경우 주니어 레벨 스터디를 리딩하면서 혼자 개발했던 것이므로 꼭 리뷰를 받고 싶어서 꾸겨 넣었습니다.
  • functional 은 Array의 고차함수로만 사용했을 뿐, 깊은 공부를 하지 못한 영역이라 합성에 초점을 맞춰 계속 작성을 해볼 생각입니다.
  • proxy와 prototype은 타입스크립트 인터페이스를 흉내 내기 위한 시도였을 뿐, 실제로 활용하진 않겠지만 어떤 리뷰를 받을지 너무 기대했어요.

다른 분들의 커밋된 파일 수의 2배, 많게는 3배에 육박하더군요... 얼마나 힘드셨을지 짐작이 갑니다. 그럼에도 불구하고 좋은 리뷰 남겨주셔서 감사합니다. 다음 리뷰 때는 현재 구조를 전혀 기용하지 않고 앱에 맞는 규모로 코드를 작성할게요.

- 커스텀 엘리먼트(웹 컴포넌트) 활용
- HTML DOM 요소를 적극 활용하기
- view와 events 사이에서 validation을 자동으로 체크하도록 설계하기
- 최대한 HTML DOM 요소를 활용하기
- validation을 브릿지처럼 연결하여 사용하기
- fieldset을 form으로 교체하여 UX 향상
- attributeChangedCallback를 활용하여 상태 관리... 수정해야 함
- setInterval을 사용했으나 requestAnimationFrame을 활용하도록 수정할 것
- disabled 추가
- 적용되지 않았던 css를 수정
- 파이프 라인을 구성하는 팩토리로 서비스 로직을 명명
- 불필요한 헬퍼 함수 삭제
- 공통된 로직에 대해 리팩토링이 추가로 필요
- 렌더링이 가장 문제
- requestAnimationFrame을 적용해야 함
@InSeong-So InSeong-So requested a review from roy-jung April 10, 2022 14:07
@InSeong-So InSeong-So changed the title [클린코드 2기 소인성] 자동차 경주 미션 STEP1 [클린코드 2기 소인성] 자동차 경주 미션 STEP1 ~ 3 Apr 10, 2022
- setInterval 대체
- async/await로 제어하며 내부 함수를 인자로 받는다.
- GameSection에 존재하던 복잡도를 제거한다.
- factory 폴더를 기능에 따라 분리한다.
- pipeline을 통해 프로미스로 이벤트 제어가 비동기되는 현상을 store로 해결한다.
Copy link

@roy-jung roy-jung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

인성님 안녕하세요! 전반적인 코드 가독성이 확 올라서 즐겁게 읽었습니다.
다만 service, factory, helper, template의 각 명칭들이 뭔가 딱 와닿지 않는 느낌이랄까, 애매하다는 느낌이 드는데, 콕집어 뭐라고 말해야 좋을지는 모르겠어요 ㅎㅎ 서비스가 내가 아는 서비스가 맞나? 무엇을 팩토리라고 하는거지? 템플릿이랬는데 템플릿이 아닌것 같은...?
위 내용 및 각 파일에 남긴 코멘트들은 개인적인 소감이나 의견일 뿐이라 크게 신경쓰지 않으셔도 될 것 같아요. 다만 테스트해보니 동작이 안되는 버그가 있는데 이건 해결해주시면 좋겠습니다.
일단은 승인하고, 버그만 해결해주시면 바로 머지하도록 하겠습니다. 고생하셨어요 :)

Comment on lines 2 to 4
constructor() {
super();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요건 없어도 돼요 :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

삭제할게요! 최상위 웹 컴포넌트에서 무엇을 정의하여 효율적으로 사용할 수 있을까를 고민하다가 지우질 않았네요🙇‍♂️

src/js/App.js Outdated
</fragment>`;

export default class App extends Template {
#handler = [];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

21번줄, 33번줄, 그리고 Template의 'bindEvent' 내용을 살펴보면

  • 13번줄의 초기값이 배열이면 안될 것 같아요. 나중에 덮어씌워지긴 하지만 제삼자 입장에선 혼란스러울 수 있으니까요.
  • bindEvent함수 호출의 반환값은 removeHandler가 의미상 더 맞지 않을까 싶어요.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

말씀대로 명명하는 게 최선으로 보입니다! 초기값을 배열로 할당한 건 최상위 웹 컴포넌트의 내부에 핸들러를 선언했을 때 오버라이딩을 하지 않으면 에러가 나길래 했었는데... 현재는 각 자식 컴포넌트에 저장해두고 있죠. 이 부분은 제가 놓쳤습니다! 감사합니다🤗

const attrs = [
{ attr: 'car-names', value: detail.carNames },
{ attr: 'try-count', value: detail.tryCount },
];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attr, value로만 구성된 것이라면 Map이나 이중배열로 충분하지 않을까 하는 의견입니다.

Copy link
Member Author

@InSeong-So InSeong-So Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2차원 배열로 작업해 볼게요! map은 결국 코드가 길어지거나 2차원 배열과 비슷한 형태로 전개될 것 같거든요🙏

Comment on lines 15 to 41
const executor = {
[CONTROLL_KEY.CAR_NAMES]: pipe(
trim,
trimComma,
split,
removeSpace,
checkValidations,
),
[CONTROLL_KEY.CAR_NAMES_AFTER]: pipe(
() => $show('#game-try-count-form'),
() => $disabled('#car-names'),
() => $disabled('#car-names-confirm'),
() => setTimeout(() => $focus('#game-try-count'), 100),
),
[CONTROLL_KEY.TRY_COUNT_AFTER]: pipe(
() => $disabled('#game-try-count'),
() => $disabled('#game-try-count-confirm'),
),
[CONTROLL_KEY.GAME_BEFORE]: pipe(
split,
carNames => carNames.map(carName => ({ name: carName, moveCount: 0 })),
),
[CONTROLL_KEY.GAME]: pipe(
racingWrapper,
window.requestAnimationFrame,
),
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

executor들을 한 데 모아놓고 key를 상수로 관리할 필요성이 있는지 의문이 들어요.
오히려 실제 호출하는 곳에서 직접 pipeline을 정의하는 편이 더 좋지 않을까 하는 의견입니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분은 아래 길게 코멘트에 대체했습니다.
최초 작성 시 재남님 의견대로 했으면 더 깔끔했을 수 있겠네요😥

각 컴포넌트가 factory에만 의존해서 서비스 로직을 알 필요 없게 만들고자 했습니다. factory에는 try-catch로 에러도 처리하고, 서비스 로직은 pipe를 통해만 호출하도록...
하지만 그 결과 의도가 정확히 전달되지 않고 개발자들 사이의 표준을 헤집어 놓은 느낌이네요...😇

if (!newValue) return this.firstElementChild.classList.add('hidden');

this.#cars = pipeline(CONTROLL_KEY.GAME_BEFORE, this.getAttribute('car-names'));
this.#tryCount = this.getAttribute('try-count');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

얘는 여기서 가져와야 하는건가요? 멤버에서 제외시키고 35번줄에서 직접 접근하면 문제가 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

핫 맞네요...! 의미 없이 할당한 뒤에 호출하고 있었던 코드니 말씀대로 수정하겠습니다🙇‍♂️

const winners = this.getAttribute('winners');
document.getElementById('winners').textContent = winners;
// prettier-ignore
setTimeout(() => alert(`이번 레이싱 게임의 승자는\n\n${winners} 입니다!\n\n✨축하해요✨`), RESULT_ALERT_DELAY);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delay 경과 전에 '다시시작하기'를 클릭하는 경우에는 얼럿이 뜨지 않도록 하는게 어떨까 해요.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좀 더 사용자 직관적이게 변경하겠습니다💪

Comment on lines +23 to +26
gameReset = event => {
if (!event.target.matches('#game-reset')) return;
this.dispatch('reset');
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리셋과 관련된 어딘가가 문제가 있는것 같아 제보드려요.
'다시시작하기'로 몇 번 테스트를 진행해보니 어느순간 경기종료 후에 '다시시작하기 버튼' 및 '경기종료 얼럿'이 노출되지 않는 케이스가 발생합니다. 재현방법은 잘 모르겠지만 자주 발생하네요

Copy link
Member Author

@InSeong-So InSeong-So Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

로직을 하나 빠뜨렸는데, store를 통해 관리하던 tryCount의 문제입니다. 자동차 이름들은 입력 시 재할당 되어 문제가 없지만... maxTryCount는 게임이 진행된 후에 체크하거든요.

시도 횟수가 최초 3회 > 다시 시작 후 2회 라면 maxTryCount와 같지 않으므로 winner 측정이 되지 않았고, 웹 컴포넌트의 attributeChangedCallback로 컴포넌트를 제어하고 있었는데 갱신되는 속성값이 없어 렌더링이 이루어지지 않았어요.

store의 상태를 초기화하도록 후다닥 수정했습니다!🤗

const maxMoveCount = store.getState('maxMoveCount');
// prettier-ignore
const winners = cars.reduce((result, { name, moveCount }) => {
if (maxMoveCount === moveCount) return [...result, name];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기서 새 배열을 만들어야 하나요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

배열을 만들고, 다시 join으로 문자열로 치환하고... 불필요한 행위네요. car-names가 많아지니 리소스도 낭비됩니다.
삼항식으로 짧게 만들고 문자열로 변경했습니다!

@@ -0,0 +1,58 @@
import { DICE_RANGE, ERROR_MESSAGE } from '../constants.js';

// prettier-ignore

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 주석이 종종 보이는데, 어떤 이유에서 무시하게끔 하신건지 궁금해요

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

개인적인 느낌이지만... prettier 규격에 정해 놓은 포맷은 가끔 사람이 읽기 난해하게 정렬하는 것 같아요.

  • 적용
    1

  • 무시
    2


그래서 좀 더 읽기 편하게 일관성을 지켜줄 필요가 있어보이는 코드에 적용하는 편입니다.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요건 아마도 prettier 설정에 'printWidth'를 현재 설정보다 작게 잡으시면 해결될 것으로 보여요. 물론 완전한 해결책은 아니긴 하지만 ;)

- Template 명칭을 ComponentHandler로 변경
- 멤버 변수 handler를 removeHandler로 명시적이도록 변경
- $setAttributes 함수의 인자를 2차원 배열로 수정
- store의 상태를 초기화 시키는 메서드를 추가
- 앞선 컴포넌트 변경 사항 적용
- 게임 결과를 출력하는 형태를 일관성있게 변경하고 사용자 친화적으로 수정
- delayLoop의 기본 값을 수정
- alert 후 다시 시작 버튼을 렌더링하도록 변경
@InSeong-So
Copy link
Member Author

InSeong-So commented Apr 13, 2022

안녕하세요, 재남님! 리뷰 감사합니다. 관련 부분을 수정하여 다시 PR 드립니다!
적어주셨던 몇몇 코멘트를 설명으로 대체하려고 해요.

다만 service, factory, helper, template의 각 명칭들이 뭔가 딱 와닿지 않는 느낌이랄까, 애매하다는 느낌이 드는데, 콕집어 뭐라고 말해야 좋을지는 모르겠어요 ㅎㅎ 서비스가 내가 아는 서비스가 맞나? 무엇을 팩토리라고 하는거지? 템플릿이랬는데 템플릿이 아닌것 같은...?


저도 이 부분은 많은 고민을 하고 있습니다!

최초 factory라는 폴더는 services로, 나머지 하위 파일은 각 컴포넌트의 도메인 로직에 맞게 inputSection.service 등을 사용했었어요. 그러다보니... 한 파일에 하나의 로직만 존재하고, 이를 export하는 너무 과도한 오버엔지니어링 아닌가라는 의문점이 들었죠. 어떻게 해야 규모에 맞고 제3자가 읽고, 이를 확장할 때 유연하게 대처할 수 있을까?에 대한 좋은 방법을 찾다보니 코드가 짜여진 형태로 그들의 생각을 일관되게 만들고자 했습니다.

index에서 export하는 함수명칭은 pipeline이죠. 공장은 어떤 재료가 들어가든 완성된 형태로 무언가 반환되므로, 파이프라인을 통해 어떤 input을 집어넣으면 이러한 역할을 한다의 취지로 접근했습니다. 파이프만으로는 결과물이 나오지 않으므로 이를 제어할 실제 기기들, 즉 service라는 명칭으로 분리하여 pipline에 service가 의존하도록 설계했습니다. 약간 facade 패턴을 혼용한 느낌이에요.

어제 세션 이후 다른 참여자분들과 4시간 넘게 이야기하면서, 그들이 어떻게 받아들이는지 물어본 결과 재남님과 동일한 느낌을 받았고, 저도 동일하게 생각했습니다. 의도는 이러한데 충분히 드러나지 않는 느낌이에요. 다만... 다음 미션을 위해 설계적인 부분을 건드리면 너무 과하게 작업을 해야할까 현재 상황을 유지하고자 합니다😭

혹시 재남님이라면 어떤 네이밍을 채택하셨을까요? 다음 미션 때는 적극적으로 생각해보고 코드를 작성하고 싶네요.
마지막으로 해당 미션은 종료가 되어 다른 리뷰어님으로 배정되었으니... 재남님 생각에는 프론트엔드 개발자로서 어떤 것을 더 심화적으로 공부하면 좋을 지 키워드를 알려주실 수 있을까요?

@InSeong-So InSeong-So requested a review from roy-jung April 13, 2022 03:55
Copy link

@roy-jung roy-jung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

깊이 고민하고 많은걸 개선하느라 너무 고생하셨습니다!! 승인 및 머지하곘습니다.

아래 내용은 이렇게 하는게 좋겠다 보다는 저라면 이렇게 해볼것 같다 정도이고, 제가 인성님 코드를 똑같이 짤 자신은 없으니..그냥 저사람은 저렇게 생각하는구나 정도로만 여겨주시길 바라며...

  • factory/index => executor들을 실제 호출하는 곳들로 옮기고 나면 pipeline만 남게 되니 pipeline을 utils로 옮깁니다.
  • service는... store를 끼얹은 renderer 뿐이므로, view(component)에 위치시키는 편이 좋을 것 같아요. store와 component의 의존성을 끊었다곤 하지만 그 대신 view에 직접 관여하는 service가 store와 결합했으니, 결과적으론 마찬가지 아닌가 싶거든요.

프론트엔드 개발자로서 뭘 더 심화적으로 공부하면 좋을지

이미 차고 넘치게 잘 하고 계신 것 같습니다. 생각의 깊이나 지식의 양은 저보다 월등히 뛰어나신 것 같다고 생각해요. 아마도 지금 느끼시는 갈증은 아마도 절대적 경험량의 부족에서 기인한 것이라 생각합니다. 이론을 많이 알고 있더라도 어떤 상황에서 어떤 이론을 적용할지 혹은 하지 않을지를 판단하는 데에는 경험을 통해서만 체득되는 암묵지 영역이 존재하니까요. 이러한 암묵지는 단기에 채울 수 없기에 그저 많이 고민하고 다양하게 시도해보며 시행착오를 겪는게 최고라 생각합니다. 스스로 부족하다 생각하지 마시고 그저 코딩을 즐기시길 바라요 :)

@roy-jung roy-jung merged commit 717ab42 into next-step:inseong-so Apr 13, 2022
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

Successfully merging this pull request may close these issues.

2 participants