[백윤서]TS Todo Refactor 과제 #2

bgyoons · 2023-12-09T12:19:43Z

✅ PR 포인트 & 궁금한 점

함수에서 클래스로 변환했습니다.! 강의를 따라치는 것 외에 클래스로 코드를 작성해보는 건 처음인데 알맞게 했는지 리뷰 부탁 드립니다.
private 접근 제한자를 사용하는 것과 # prefix를 사용하는 것에 대해서 다들 어떻게 생각하시는지 궁금합니다.
접근 제한자를 조금 남발한 것 같은데 어디까지 쓰는게 좋은지 궁금합니다.
이 외에 타입스크립트를 잘 사용했는지 궁급합니다.

초라하지만 마이그레이션 하면서 느낀점 위주로 적은 회고입니다 😅

강의 들으며 따라 친 코드 입니다.

HoberMin

윤서님 갑자기 클래스로 바꿔서 과제하느라 고생하셨습니다.. 👍

실질적인 투두리스트의 동작보다는 그냥 타입스크립트만 집중해서 눈에보이는것들만 간단하게 리뷰남겼습니다

리뷰남기면서 저도 한번 더 생각하게되는 부분들이 있어서 좋았네요 :)

HoberMin · 2023-12-11T17:30:52Z

src/App.ts

+import { TodoCount as TodoCnt, TodoItem } from "./types/todo.js";
+
+export default class App {
+  private readonly todoList: TodoList;


일단 전 private, readonly를 잘 작성하지 않았는데.. TodoList같은경우는 클래스 자체를 의미할텐데 클래스 내부가 변경될 일이 없다. 여서 readonly 키워드를 포함시켜준건가요 !?

넵! 사실 클래스 자체에 접근 제한자를 쓰는 것이 맞을까?도 있었는데 우선 인스턴스에 접근을 하기 때문에 적기는 했습니다 ;)
맞는 방법인지는 잘 모르겠습니다 ..

HoberMin · 2023-12-11T17:33:14Z

src/App.ts

+    private readonly initialState: TodoItem[],
+    private readonly initialCount: TodoCnt
+  ) {
+    new Header(this.$target, "Todo List");


저는 props를 넘겨줄 때 객체상태로 잘 넘겨줍니다 .. ! 그렇게하면 interface하나만 선언해서 타입지정해주기가 편할 것 같아요 :)

그리고 용재님 코드리뷰 하면서 생각이 들었던건데 $app은 모든 하위컴포넌트(?) 들이 사용하니 필수키워드로 작성하고 interface의 확장을 통해서 intialState, onChange함수같은 여러 props가 있을 때 그것들을 제네릭으로 정의하는 방법이 있더라구요? << 요건 용재님코드보고 내일 한번 얘기해보면 좋을 것 같아요 :)

오호 저는 생성자 constructor가 매개변수로 받으면서 타입 지정해주면 자동으로 this에 바인딩해주기 때문에 객체로 하지 않은거긴 합니다! 객체로 지정했을 때도 되는지는 잘 모르겠네요 ㅎㅎ

HoberMin · 2023-12-11T17:38:12Z

src/App.ts

+      },
+    );
+
+    this.todoList = new TodoList(


저 사실 이부분 잘못..? 구현해서 시간을 엄청많이 날렸는데 지금 contructor에 모든 컴포넌트가 전부 렌더링 되게 구현이 되어있는데요 처음엔 각 메소드에서 ex) HeaderRender() 내에서 this.header = new Header() 가 되게 하려고했는데 인스턴스가 인식을 못해서 결국 contructor로 때려넣어버렸는데.. 혹시 이유를 아신다면 공유를 부탁드리겠습니다 말로 설명하려니 너무 어려워서 간단하게 코드남겨놓을게요

class App { todoList: TodoList; constructor() { this.todoListRender(); } todoListRender() { this.todoList = new TodoList(); } }

이런식으로 했는데 todoList : TodoLIst에서 타입인식을 못하더라구요...

아하 저런 방식으로 코드를 적어보니 Property 'todoList' has no initializer and is not definitely assigned in the constructor.로 에러가 뜨기는 하네요.
constructor 생성자 함수는 인스턴스를 생성하고 초기화하는 메서드라고 알고 있습니다. 따라서 실행 시점이 this.todoList에 초기화 되기 전에 this.todoListRender() 실행 > this.todoList 참조로 이어지기 때문에 에러가 뜨는 것 같습니다. this.todoList 초기화 > this.todoListRender() 실행 > this.todoList 참조 라면 모를까 ..! 저는 이렇게 생각합니다 :D

오 윤서님 말씀이 맞아요!

new App()을 호출할 때, App 클래스의 인스턴스가 생성되는데, 이때 App 클래스의 constructor가 호출됩니다.
이후에 constructor 내에서 this.todoListRender();가 호출되고 이 시점에서 todoList 속성은 아직 초기화되지 않았기 때문에, undefined 상태입니다.
this.todoListRender(); 호출로 todoListRender 메소드가 실행됩니다. 이 메소드 내에서 this.todoList = new TodoList();가 실행되어 todoList 속성에 TodoList 인스턴스가 할당됩니다.

에러의 원인은 todoList 속성이 생성자에서 todoListRender 메소드를 호출하기 전까지 초기화되지 않는다는 것입니다. TypeScript에서는 모든 속성이 클래스 인스턴스화 시점에 확실히 초기화되어야 하기 떄문에 문제가 발생하게 돼요!

그렇기 때문에 윤서님처럼 constructor에서 직접 초기화를 해주거나 옵셔널을 사용해줘야 할 것 같습니다.

초기화에 대한 생각을 못했네요.. 🤣 의견 감사합니다 !!

HoberMin · 2023-12-11T17:41:01Z

src/components/TodoForm.ts

+    this.render();
+  }
+
+  private render() {


대부분이 중복되는 내용이라 하나만 달아둘게요 !

대부분의 메소드가 반환값이 없는데 이런건 별도로 :void와 같은 타입으로 반환값지정을 해주는게 좋을까요..?
전 명시적으로 다해주긴했는데 실질적으로 메소드는 반환값이 없는경우가 더 많다고 생각해서.. 이럴땐 어떤게 좋다고 생각하시나요??

흠 저는 메소드나 변수에 마우스 호버를 해서 추론을 하고 있으면 명시적으로 적지 않고 있습니다!
any로 추론을 하고 있다면 저는 명시적으로 지정해주는 방식으로 하고 있습니다

HoberMin · 2023-12-11T17:42:11Z

src/components/TodoList.ts

+import { TodoItem } from "../types/todo.js";
+
+export default class TodoList {
+  state: TodoItem[] = [];


아래에 검증하는 코드가 있는것 같아서 별도의 초기값은 필드에서 선언해주지 않아도 괜찮을 것 같아요 ! 😀

HoberMin · 2023-12-11T17:46:33Z

src/utils/storage.ts

@@ -0,0 +1,20 @@
+const storage = window.localStorage;
+
+export const setItem = (key: string, value: string) => {


저도 구현안하긴 했는데.. setItem, getItem 함수타입을 별도로 선언해주는게 좋지않을까? 라는생각을 했었구요
(그렇게 했을 때 갖다쓰는데서 실수를 해도 예상치 않은 에러가 발생안하지않을까? )

그리고 key값이 잘못입력되는경우 => 개발자의 실수에 의해서 (오타와 같은) 이런경우에

저같은경우는 todoList만 관리하지만 윤서님은 cnt, list두개를 관리하니까 key의 타입에 string 보다는 리터럴 유니언타입으로 관리하면 약간 타입스크립트의 진짜 맛도리를 알지않을까.. 하는 의견입니다~

리터럴 유니언 타입 좋은 방법 같습니다!! 감사합니다 :D

HoberMin · 2023-12-11T17:47:48Z

src/components/TodoList.ts

+    });
+  }
+
+  setState(nextState: TodoItem[]) {


TodoItem[] 도 그냥 타입으로 만들어서 꺼내오면 더 깔끔하지않을까 싶습니다 .. !

저는 개인적으로 TodoList를 위한 타입을 하나 더 만드는걸 별로 선호하지 않는 편이긴해요!
TodoItem[]으로 작성하면 어떤 타입의 배열인지 한 눈에 볼 수 있는게 더 좋아서 타입을 새로 생성하지 않습니다. 이 부분은 개인의 취향인 것 같아요!

오오 이런 부분에서도 취향이 나뉘는군요 ㅎㅎ!! 감사합니다!!

HoberMin · 2023-12-11T17:48:57Z

src/App.ts

@@ -0,0 +1,43 @@
+import Header from "./components/Header.js";


이거 저도 결국 .js로 확장명선언해서 코드짰는데요.. 도대체 어떻게 컴파일할 때 .js가 붙게하는거죠 ㅠㅡㅠ

imssein · 2023-12-12T03:45:34Z

src/utils/storage.ts

+export const setItem = (key: string, value: string) => {
+  try {
+    storage.setItem(key, value);
+  } catch (error) {


저도 이번에 하면서 try-catch error 타입에 대해 알게되었는데요! 이렇게 지정하는건 어떨까요? 👀

catch (error: unknown) { if (error instanceof Error) { throw new Error(error.message) } }

imssein

윤서님! 과제하시느라 고생하셨습니다 :)
더기팀 이후로 오랜만에 코드리뷰해서 더 반갑고 그러네요..ㅎㅎ
역시 윤서님 코드는 깔끔해서 전체적으로 이해하기 쉬웠습니다!
저는 이번 과제하면서 TS에만 집중했었는데, 윤서님은 다른 부분까지 더 신경쓰신게 보여서 꼼꼼함에 한번 더 배워갑니다..ㅎㅎ

Kim-Jaemin420

윤서님 항상 스터디 열심히 하시더니 금방 typescript로 마이그레이션 했네요! 차근 차근 꾸준히 하시는 모습 너무 보기 좋습니다. 엄청 빡세게 리뷰는 못드려서 도움이 많이 될지는 모르겠네요ㅎㅎ
다음 프로젝트때도 기대하겠습니다 : )

Kim-Jaemin420 · 2023-12-15T04:37:51Z

src/main.ts

+const initialState = getItem("todo", []);
+const initialCount = getItem("count", { total: 0, done: 0 });


todo와 count에 대한 정보를 main 파일이 받아와 App에서 주입을 해주고 있는데, 제가 생각했을때 main.ts는 엔트리 포인트 역할을 하기 때문에 다른 것에 대한 의존성이 적을수록 좋다고 생각합니다. 혹시 스토리지의 데이터를 가져와서 초기화 해주는 역할을 App에서 해주는 건 어떻게 생각하세요?

오 좋다고 생각합니다! 👍
전역 상태 관리를 한 경험이 많지 않다보니 이 부분에 대한 의문이 안 들어서 강의 코드를 그대로 적었네요! 😅
재민님 의견 이해도 잘 됐고 동의합니다 :D

Kim-Jaemin420 · 2023-12-15T04:50:32Z

src/components/TodoForm.ts

+        if (text.length > 1) {
+          $todo.value = "";
+          this.onSubmit(text);
+        } else alert("두 글자 이상 입력해주세요");


이 부분은 모달을 커스텀해서 보여주고 싶은게 아니라면 input의 minLength 속성을 사용할 수 있을 것 같아요!

<input type="text" placeholder="할 일을 입력하세요" name="todo" minLength="2" />

minLength 속성이 있었군요!
속성 찾아보다가 autocomplete도 추가하면 좋을 것 같아서 추가했어요 😄
기존 HTML 속성들도 잘 활용해봐야겠어요, 감사합니다! :D

Kim-Jaemin420 · 2023-12-15T04:51:53Z

src/components/TodoForm.ts

+      <button>추가</button>
+    `;
+
+    if (!this.isInit) {


render 메서드 안에서 이벤트 바인딩을 해주기 때문에 isInit 이라는 플래그 변수가 필요하네요! 돔을 렌더 하는 함수와 이벤트 바인딩을 해주는 함수를 분리하는 건 어떨까요? contructor에서 this.render를 호출한 이후에 이벤트 바인딩 함수를 호출하고, 상태가 변경될때는 render 메서드만 호출하는 방법이요!
그렇다면 플래그 변수가 필요 없을 것 같고, 또 함수가 한 가지 일만 해도 될 것 같습니다.

넵 맞습니다!
이 것도 강의 코드를 그대로 쓴 것일텐데 이상하게 isInit 플래스 변수를 true로 만들어주는 코드도 없네요 ..?! (엥)
제가 강의를 놓친건지 뭔지는 모르겠지만 .. ㅎㅎ 놓쳤다고 해도 기존 코드를 리팩토링 해보겠다는 생각을 전혀 못하고 있었네요
그리고 함수가 한 가지 일을 해야한다는 것에 대해서도 공감합니다!! :D
감사합니다 👍

Kim-Jaemin420 · 2023-12-15T04:57:15Z

src/components/TodoList.ts

+  ) {
+    this.$target.appendChild(this.$todoList);
+
+    if (Array.isArray(this.initialState)) this.state = this.initialState;


파라미터로 받아오는 initialState의 타입은 어차피 항상 배열일텐데 Array.isArray로 한 번 더 검증할 필요가 있나요?

뭔가 로컬 스토리지에 저장을 했기 때문에 조작당할 가능성이 있다고 판단해서 적은 코드인데, 여러 경우에서 테스트를 해보니 검증할 필요가 없을 것 같네요! :D
유효성 검사에 대한 자신이 부족해서 적은건데 더 섬세하게 고려해보겠습니다!

Kim-Jaemin420 · 2023-12-15T05:05:06Z

src/components/TodoList.ts

+    });
+  }
+
+  setState(nextState: TodoItem[]) {


저는 개인적으로 TodoList를 위한 타입을 하나 더 만드는걸 별로 선호하지 않는 편이긴해요!
TodoItem[]으로 작성하면 어떤 타입의 배열인지 한 눈에 볼 수 있는게 더 좋아서 타입을 새로 생성하지 않습니다. 이 부분은 개인의 취향인 것 같아요!

Kim-Jaemin420 · 2023-12-15T05:11:16Z

src/components/TodoList.ts

+        if (target.className === "deleteBtn") {
+          newState.splice(index, 1);
+          this.setState(newState);
+        } else if (target.className.includes("todoList")) {
+          const isCompleted = target.className.includes("completed");
+          if (isCompleted) target.classList.remove("completed");
+          else target.classList.add("completed");
+          newState[index] = {
+            ...newState[index],
+            isCompleted: !isCompleted,
+          };
+          this.setState(newState);
+        }
+      }


분기 처리가 생기면서 코드 이해가 살짝 어려워지는 것 같아요. deleteButton을 클릭했을때 handleClickDeleteButton 같은 메서드를 생성해서 호출해주면 분기문 안의 로직이 줄어 이해하기 조금 편할 것 같아요! 시간이 되면 구현하고, 시간이 없다면 인지만 해두고 넘어가셔도 좋을 것 같습니다.

저도 이 부분이 제가 봐도 코드 가독성이 떨어진다고 생각했는데 딱 지적을 해주셨네요 ㅎㅎ,,,
한번 고민해보겠습니다! :D

Kim-Jaemin420 · 2023-12-15T05:14:42Z

src/components/TodoList.ts

+        const newState = [...this.state];
+        const index = +($li.dataset.index as string);
+
+        if (target.className === "deleteBtn") {
+          newState.splice(index, 1);
+          this.setState(newState);


저는 개인적으로 this.state를 스프레드해서 새로 newState를 생성하지 않는 편을 더 선호해서요! 아래처럼 filter 함수를 사용하면 더 간결해지지 않을까? 하는 생각도 드네요! 이것도 인지만 하고 넘어가셔도 좋습니다.

const handleClickDeleteButton = () => { const newTodos = this.state.filter(); this.setState(newTodos); };

Kim-Jaemin420 · 2023-12-15T05:15:45Z

src/components/TodoList.ts

+  }
+
+  setState(nextState: TodoItem[]) {
+    const newState = validation.state(nextState);


타입스크립트인데 validate가 필요할까요? 만약 추가로 validate가 필요하다면 타입 지정이 빠진 부분이 있을 것 같습니다.

엇 .. 생각해보니 그렇네요 🥲
validation 메서드들 다 날렸습니다 :D

Kim-Jaemin420 · 2023-12-15T05:18:45Z

tslint.json

tslint를 사용하시는군요. tslint는 이제 더 이상 활발하게 유지보수하지 않는다고 해요! 그래서 typescript 팀도 eslint 사용을 권장하고 있습니다. 여기 들어가 보시면, 2019년에 deprecated됐다고 해요. 한 번 참고해보세요!

헉! 그렇군요
개발 블로그 글 보면서 셋팅을 따라했더니 공식문서 없이 셋팅한게 들켰네요 ㅎㅎ😅
공유 감사드립니다!!

learn-programmers and others added 30 commits September 19, 2023 04:29

Initial commit

a711653

Update README.md

568585d

Initial code

39ddbc5

강의 들으며 따라 친 코드 입니다.

컴포넌트에 new를 붙이지 않고 쓸 경우 에러 처리

a24eea8

script 파일 module 속성으로 설정

879cc34

TodoList에 isCompleted 값, 토글 기능 추가

da761cc

TodoList text 옆에 삭제 버튼 및 기능 추가

8494326

TodoCount 컴포넌트 생성 및 개수 조회 기능 추가

7be0d9e

린트 추가

31729f4

스타일 추가

26433a4

두 글자 이상 미입력 시 알림창 팝업

0f5b5a8

반복되는 로직 함수 추가

71703be

초기값 유효성 검사

ca37ecd

new.target 유효성 메서드 추가

e337536

투두리스트 상태 유효성 메서드 추가

12d4512

투두리스트 카운트 초기값 할당 조건 변경

20050fd

반복되는 로직 setState에 추가

1d84f5d

투두리스트 삭선/삭제 로직 변경

ab66e5f

rename: 폴더 구조 변경

565fcd4

rename: .js > .ts 파일로 변경

93fde36

chore: typescript 환경 셋팅

616d1d3

rename: 폴더 이름 변경

28224b1

feat: main.ts 타입스크립트 변환

2d8a1b3

feat: App 컴포넌트 클래스 + ts 변환

3201d20

feat: Header 컴포넌트 클래스 + ts 변환

ebc2683

feat: TodoList 컴포넌트 클래스 + ts 변환

90adc4b

feat: TodoForm 컴포넌트 클래스 + ts 변환

adb0435

remove: class로 바꾸면서 new 연산자 유효성 검사 코드 제거

46bfbbc

feat: TodoCount 컴포넌트 클래스 + ts 변환

634a993

comment: updateCount 주석 제거

d7ad87b

bgyoons reopened this Dec 9, 2023

bgyoons changed the title ~~TS Todo Refactor 과제~~ [백윤서] TS Todo Refactor 과제 Dec 11, 2023

bgyoons changed the title ~~[백윤서] TS Todo Refactor 과제~~ [백윤서]TS Todo Refactor 과제 Dec 11, 2023

HoberMin reviewed Dec 11, 2023

View reviewed changes

imssein reviewed Dec 12, 2023

View reviewed changes

bgyoons added 3 commits December 12, 2023 15:42

refactor: 타입 단언에서 타입 가드로 변경

663c37a

refactor: 초기값 삭제

e249e6a

Merge remote-tracking branch 'todo-list/main' into yoonseo_working

50a2ee1

bgyoons force-pushed the yoonseo_working branch from 7d4819e to 50a2ee1 Compare December 12, 2023 06:58

bgyoons added 6 commits December 14, 2023 00:17

refactor: constructor 내부 상태값 this 접근자 삭제

4fa5d9f

feat: 에러 타입 추가

2802b9f

refactor: early return으로 변경

ec5b624

feat: storage key 타입 추가

9c19859

refactor: todo list 배열 타입 추가

30c3404

Merge remote-tracking branch 'todo-list/main' into yoonseo_working

803a2b6

bgyoons force-pushed the yoonseo_working branch from 8c26270 to 803a2b6 Compare December 13, 2023 15:21

Kim-Jaemin420 reviewed Dec 15, 2023

View reviewed changes

bgyoons added 9 commits December 19, 2023 01:16

refactor: 전역 상태 main > App에서 관리하도록 변경

1c062e5

refactor: 최소 문자 개수 HTML 속성으로 변경 및 자동완성 기능 해제

474710e

refactor: isInit 플래스 변수 삭제

4c198d1

refactor: 돔 렌더와 이벤트 바인딩 함수 분리

d141abb

refactor: 불필요한 유효성 검사 삭제

088d7f5

refactor: 이벤트 바인딩 함수 분리

7199c76

refactor: 유효성 검사 메서드 삭제

63c7448

remove: 유효성 검사 파일 삭제

775a2a4

Merge remote-tracking branch 'todo-list/main' into yoonseo_working

4498a57

bgyoons force-pushed the yoonseo_working branch from 710cef3 to 66b4144 Compare December 18, 2023 16:45

rename: TodoList 프로젝트 폴더로 이동

8a7e3ed

bgyoons force-pushed the yoonseo_working branch from 66b4144 to 8a7e3ed Compare December 18, 2023 16:47

		@@ -0,0 +1,20 @@
		const storage = window.localStorage;

		export const setItem = (key: string, value: string) => {

		@@ -0,0 +1,43 @@
		import Header from "./components/Header.js";

		const initialState = getItem("todo", []);
		const initialCount = getItem("count", { total: 0, done: 0 });

[백윤서]TS Todo Refactor 과제 #2

Are you sure you want to change the base?

[백윤서]TS Todo Refactor 과제 #2

Conversation

bgyoons commented Dec 9, 2023 • edited Loading

✅ PR 포인트 & 궁금한 점

HoberMin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imssein left a comment

Choose a reason for hiding this comment

Kim-Jaemin420 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgyoons commented Dec 9, 2023 •

edited

Loading