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

4-1 과제 To-do 리스트 Redux를 사용하여 리팩터링 하기 #121

Merged
merged 44 commits into from
Jul 5, 2022

Conversation

alstjr0183
Copy link

안녕하세요 선생님..!
처음부터 커밋하면서 다시 진행했습니다!
아직 완성을 못했지만 제출하겠습니다
감사합니다!

redux, react-redux를 설치했습니다
actions , reducer , store 를 생성해 redux 구조를 만들었습니다
APP을 Provider로 추가해준뒤 store를 넘겨주었습니다
app.jsx 쪽에 provider를 index쪽으로 셋팅했습니다
app test에서 provider를 사용하는대신 저렇게 셋팅을해주었습니다
input value를 업데이트하는 로직을 redux를 사용하여 분리했습니다
할일을 추가하는 로직을 redux를 사용하여 분리했습니다
테스트를 위해 mock 폴더의 seletor에게 tasks값을 넣어주었습니다
dispatch 함수를 그대로사용하였는데 addTask로 변경했습니다
app test에 useSelector 를 이용해 테스트를 했습니다
useSelector를 이용해 작성한후 확인할수있는 테스트 케이스를 추가했습니다
reducer test 에 updateTaskTitle을 테스트하는 테스트 케이스를 추가했습니다
reducer test에 addTask가 작동하는 테스트 케이스를 추가했습니다
reducer 테스트의 deleteTask 케이스를 추가했습니다
할일 추가후 input value 가 초기화 되는 테스트를 추가해보았습니다
@daadaadaah
Copy link

daadaadaah commented Jun 29, 2022

  1. 지금 계속 작업하고 계신거예요? 🙂
  2. 현재 과제 어떻게 진행하고 계신지 궁금합니다!
    강의보고 따라하고 계신건가용? 아니면 혼자 해보고 계신건가용?

taskTitle에 값이 있는경우와 없는경우를 context를 사용해 나눠주고 없는경우의 테스트 코드를 추가해주었습니다
@alstjr0183
Copy link
Author

alstjr0183 commented Jun 29, 2022

  1. 앗 넵 지금 작업하고있습니다..!
  2. 현재는 강의 보고 따라하고있는데 테스트 케이스같은부분들은 먼저 작업해본뒤 강의 확인후 다른점을 수정하고있습니다!!

리뷰 진행해주셔두 괜찮아요!!!

@daadaadaah
Copy link

  1. 앗 넵 지금 작업하고있습니다..!
  • 현재 작업중이시니, 내일 오전에 리뷰하는게 좋을 것 같아요!ㅎㅎ계속 작업 진행해주세용! 💪

addTask 실행후 id 가 undefined가 아니라는 테스트를 추가했습니다
반복적으로 수행하는 로직을 함수로 처리하였습니다
관심사 분리를 위해 listcontainer 테스트 코드를 작성하였습니다
listcontainer로 인해 app에 사용안하는 부분을 제거하고
page쪽에는 만들어준 listcontainer를 사용합니다
list를 분리해주며 나온 테스트 오류를 useseletor를 이용하여 수정해주었습니다
inputContainer의 테스트케이스를 작성했습니다
list의 prop명을 잘못입력해준 부분을 수정했습니다
inputContainer를 만들었습니다
app , page쪽의 안쓰는 코드를 정리했습니다
page쪽에서 큰 역할이없어서 app으로 병합했습니다
describe와 it을 이용하여 리팩토링해주었습니다
describe , it 을 이용하여 테스트 케이스를 분리해주었습니다
describe, it 을 이용해 구분해주었습니다
영어로 되어있던 설명을 통일감있게 수정해주었습니다
describe , it 을 이용해 수정해주었습니다
describe , it 을 이용해 수정했습니다
@alstjr0183
Copy link
Author

분리하는 부분에서 테스트 케이스들이 너무 헷갈려서 헤맸네요 ㅠㅠ
강의 까지의 진도는 나간후 test케이스들 describe , context , it 을 이용하여 변경해주었습니다
리뷰 부탁드리겠습니다!!

inputContainer test change 이벤트 케이스 추가
it 설명부분을 수정했습니다
click 이벤트를 테스트하는 과정에서 type:'addTask'  넘겨주었는데 action에
있는 addTask 함수로 변경해주었습니다
listContainer의 완료버튼 눌렀을때를 테스트 하는 코드를 추가했습니다
@alstjr0183
Copy link
Author

npm test 돌렸더니 inputContainer , listContainer 에서 테스트 케이스가 빠져서 커버리지가 모자라더라구요..! 추가해주었습니다
reducer 쪽은 state가 없는경우 return 되는 테스트를 작성하면되는데 아직 고민중이라서 해결을 못했습니다..!

Copy link

@daadaadaah daadaadaah left a comment

Choose a reason for hiding this comment

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

늦은 시간까지 과제하느라 수고 많으셨습니다! 👏
남겨드린 코멘트 참고해서 PR 부탁드려용!😎

그리고, 과제 2에서는 commit 메세지를 TDD 사이클로 의도적으로 작성하는 연습을 해보는게 좋을 것 같아요!
지금 작업내역을 보면, 구현테스트 코드보다 앞서 있는 경우가 있어서요!

reducer 쪽은 state가 없는경우 return 되는 테스트를 작성하면되는데 아직 고민중이라서 해결을 못했습니다..!

고민해보시다가 도움이 필요하면 문의주세용👊

Comment on lines 15 to 17
const tasks = [
{ id: 1, title: '할일 1' }, { id: 2, title: '할일 2' },
];

Choose a reason for hiding this comment

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

tasks 다른 테스트에서도 자주 사용하는데, fixtures 폴더 하나 만들어서 따로 관리해볼까요?
이렇게 여러 곳에서 사용되는 테스트 데이터를 fixture라고 이야기하거든용!

Copy link
Author

Choose a reason for hiding this comment

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

수정했습니다!

아하 테스트에 있는 변수들을 줄일생각을 전혀안해봤었네요..!
폴더로 나눠서 관리하는 방법이 좋은거같습니다!!

Comment on lines 18 to 19
const { getByDisplayValue } = renderInput();
expect(getByDisplayValue('기존 할 일')).not.toBeNull();

Choose a reason for hiding this comment

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

맥락 구분을 위해 한줄 띄어줍시당!

Copy link
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 24
const { getByLabelText } = renderInput();
fireEvent.change(getByLabelText('할 일'), {

Choose a reason for hiding this comment

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

맥락 구분을 위해 한줄 띄어줍시당!

Copy link
Author

Choose a reason for hiding this comment

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

수정했습니다!

Comment on lines 3 to 4
import { useDispatch, useSelector } from 'react-redux';
import { addTask } from './actions';

Choose a reason for hiding this comment

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

맥락 구분을 위해 한줄 띄어줍시당!

Copy link
Author

Choose a reason for hiding this comment

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

수정했습니다!

Comment on lines 3 to 4
import { useDispatch, useSelector } from 'react-redux';
import { deleteTask } from './actions';

Choose a reason for hiding this comment

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

맥락 구분을 위해 한줄 띄어줍시당!

Copy link
Author

Choose a reason for hiding this comment

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

수정했습니다!

import reducer from './reducer';

describe('reducer', () => {
describe('taskTitle 변경을 시도한다', () => {

Choose a reason for hiding this comment

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

describe(updateTaskTitle, ~) 이렇게 간단하게 표현해도 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

아하 addTask 처럼 간결하게 통일해주는게 좋은거같네요!!
수정했습니다!

});
});

describe('tasks-title 지우기를 시도한다', () => {

Choose a reason for hiding this comment

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

describe(deleteTask, ~) 이렇게 간단하게 표현해도 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

수정했습니다!

src/reducer.js Outdated
Comment on lines 8 to 37
if (action.type === 'updateTaskTitle') {
return {
...state,
taskTitle: action.payload.taskTitle,
};
}

if (action.type === 'addTask') {
const { newId, taskTitle, tasks } = state;

if (!taskTitle) {
return state;
}

return {
...state,
newId: newId + 1,
taskTitle: '',
tasks: [...tasks, { id: newId, title: taskTitle }],
};
}

if (action.type === 'deleteTask') {
const { tasks } = state;

return {
...state,
tasks: tasks.filter((task) => task.id !== action.payload.id),
};
}

Choose a reason for hiding this comment

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

reducer 안에 if이 점점 많아지고 있어요~
점점 많아지게 되면, 읽기 어려울 것 같은데, if나 switch를 사용하지 않고, 어떻게 이 문제를 해결할 수 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

수정했습니다

if문 대신에 코드숨 1주차 프로젝트에서 사용했던 object Literals 를 사용해보았습니다!!

맥락구분을 하기위해 띄어쓰기를 해주었습니다
더 간결한 설명으로 수정했습니다!
테스트중 자주사용하는 변수를 fixtures폴더를 만들어 관리해주었습니다
state가 있는경우 없는경우 context를 이용해 분리해준뒤 state가 없는경우의 테스트를 추가했습니다
가독성이 안좋은 if문을 대신하여 object Literals를 사용했습니다
@alstjr0183
Copy link
Author

alstjr0183 commented Jun 30, 2022

수정했습니다!

9시 거의 다되서 겨우 제출했네요..!

질문이있습니다

현재 작업되어있는부분에는 reducer함수내에 type이 있는지를 체크해서 그에따른 상황을 반환해주고있었는데요
임시로 작성한 밑에 코드로 해도 테스트는 성공되던데 이방법은 별루일까요??
undefined일때는 return state를 해주게끔 해봤어요!
image

Copy link

@daadaadaah daadaadaah left a comment

Choose a reason for hiding this comment

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

잘 하셨습니다👏 몇가지만 수정하시고 과제2 진행해주세용!!🚀

현재 작업되어있는부분에는 reducer함수내에 type이 있는지를 체크해서 그에따른 상황을 반환해주고있었는데요
임시로 작성한 밑에 코드로 해도 테스트는 성공되던데 이방법은 별루일까요??
undefined일때는 return state를 해주게끔 해봤어요!

undefined 같은 예약어가 변수명, 매개변수, 함수명, 객체의 속성명 등에 쓰이는 건 안 좋은 패턴이예요!

src/App.test.jsx Outdated
@@ -4,14 +4,14 @@ import { useSelector } from 'react-redux';

import App from './App';

import { tasksDummy } from './fixtures/task-dummy';

Choose a reason for hiding this comment

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

dummy라는 용어 빼셔도 될 것 같아요!ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

수정했습니다!

dummy를 제거해서 바꿔줬는데 tasks 라는 변수명을 test코드안에 다른곳에서 쓰는 경우가있어서 그부분엔 다른 변수명들을 바꿔주었습니다!

src/reducer.js Outdated
taskTitle: action.payload.taskTitle,
};
}
const reducerObject = {

Choose a reason for hiding this comment

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

Object 라는 용어는 빼셔도 될 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

수정했습니다!
이미 reducer쪽에서 reducer 네이밍은 사용중이어서 reducerAction 이라고 변경해봤는데 좀더 나은 네이밍이있을까요?!

Comment on lines 1 to 5
export const taskTitleDummy = '할일';

export const tasksDummy = [
{ id: 1, title: '할일 1' }, { id: 2, title: '할일 2' },
];

Choose a reason for hiding this comment

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

Dummy라는 용어는 빼도 의도가 전달될 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

수정했습니다!

src/reducer.js Outdated
Comment on lines 37 to 41
export default function reducer(state = initialState, action) {
if (reducerObject[action.type]) { return reducerObject[action.type]?.(state, action); }

return state;
}
Copy link

Choose a reason for hiding this comment

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

  1. reducerObject[action.type]?.(state, action)이 아니라 reducerObject[action.type](state, action)이 맞는 것 같아요! 함수이니까요!
  2. reducer라는 함수가 본래는 reducerObject[action.type](state, action)을 리턴하고, 예외적인 상황에서 state를 반환하므로, 그 의도가 드러나도록 작성해주시는게 좋아요!

Copy link
Author

Choose a reason for hiding this comment

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

수정했습니다

감사합니다!

dummy 를 제거하여 taskTitle , tasks 로 변경해주었습니다
reducer를 그대로 쓰고싶은데 이미 reducer쪽에서 사용중이여서
reducerAction이라는 네이밍을 사용해봤습니다
@alstjr0183
Copy link
Author

alstjr0183 commented Jun 30, 2022

수정사항 모두 수정했습니다!
과제2 진행할게요!!
감사합니다~~

undefined 같은 예약어가 변수명, 매개변수, 함수명, 객체의 속성명 등에 쓰이는 건 안 좋은 패턴이예요!

생각해보면 예약어를 사용하는것은 생각지못한 버그들이 생길수있겠네요!
답변감사합니다!

@daadaadaah daadaadaah merged commit f4b4e78 into CodeSoom:alstjr0183 Jul 5, 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.

None yet

2 participants