Skip to content

Conversation

@nmbak
Copy link

@nmbak nmbak commented Nov 12, 2019

memo짠

@Jravvit Jravvit requested review from Jravvit and removed request for Jravvit December 4, 2019 14:07
//action creator
export function addTodo(todo,todos) {

const lastID = todos.length !== 0 ? todos[todos.length -1].id : 0;
Copy link

Choose a reason for hiding this comment

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

action은 단순히 명령과 어떤 데이터를 전달할것인지만 알려주는 부분입니다!
이 lastID를 알아내는 로직은 action보다는 action을 실행시키는 컴포넌트에서 계산해서 전달 해주시는게 좋습니다!

action 생성자는 action을 생성하는 역할만해야 합니다.
여기서 action을 생성할때는 view(container component)로 부터 전달받은 데이터만 사용해야 합니다.
그 이유는 역할을 명확하게 구분해서 복잡성을 줄이기 위함인데요.
지금 처럼 액션에 데이터를 변경하는 로직을 넣어 두고, 리듀서에서도 계산해주고, 컴포넌트에서도 값을 계산해서 전달해 주고 이러면 코드가 복잡해 집니다!
그래서 변화는 리듀서가, 어떤값이 변해야 하는지 계산과 전달은 컴포넌트에서 해주셔야 합니다.
위에 작성하신 코드는 컴포넌트에서 실행할때 배열의 마지막 녀석을 계산해서 action에 보내주시는게 좋은 방법입니다!

return {
type: ADD_TODO,
todo: todo,
id: lastID+1
Copy link

Choose a reason for hiding this comment

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

이 부분도 위와 마찬가지로 데이터를 8번째 줄에서 계산해 주시고 이번에 16번째줄에서도 계산을 해주시면,
비슷한 일을 하는 역할인데 관련된 일을 하는 코드들이 코드상 여기저기 존재하게 됩니다.
이렇게 되면 나중에 문제가 발생했을때 디버깅하기가 어려워지고, 버그가 생길 확률이 높아집니다!

});
case REMOVE_TODO:
return {
todos: state.todos.filter(v => v.id !== action.id)
Copy link

Choose a reason for hiding this comment

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

reducer는 store로 부터 state를 전달 받아서 새로운 state를 반환해주는 역할을 합니다.
이유는 나중에 디버깅 할때 변화전 state와 변화후 state를 비교하기 위함입니다.

여기서 말하는 새로운 state는 개발자가 만들어서 전달을 해줘야 하는데요!

15번째줄을 보시면 제가 Object.assign()이라는 함수를 사용한걸 보실수 있습니다.
이는 store로부터 전달받은 state를 복사해서 새로운 state를 만들어서 새로운 state에 변화를 주는 것입니다.

그래서 이부분도 마찬가지로 아래와 같이 구현해주시는게 좋습니다

return Object.assign({}, state, {
todos : [ ...state.todos, state.todos.filter(v=> v.id !== action.id()]
})

export default connect(
state => ({}),
dispatch => ({
globalActions: bindActionCreators(globalActions, dispatch),
Copy link

Choose a reason for hiding this comment

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

globalActions를 사용하신건 좋은 방법입니다!
다만 actions을 쪼갤때 이부분에서 액션들을 쪼개서 전달해주시는게 좋습니다.

dispatch -> ({
addTodo: bindActionCreators(globalActions.addTodo, dispatch),
removeTodo: bindActionCreators(globalActions.removeTodo.dispatch),
})
이렇게 해주시면 나중에 이 컴포넌트에서 어떤 액션들을 사용하는지 한눈에 보기도 쉽고, 불필요한 코드를 줄일수 있습니다!

}
}

export default connect(
Copy link

Choose a reason for hiding this comment

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

redux는 컨테이너 컴포넌트에서 사용하는게 좋습니다!
components폴더 밑에 있는 컴포넌트들은 프리젠테이셔널 컴포넌트로 컨테이너 컴포넌트에서 props를 전달받아 데이터를 보여주는 역할만 해야 합니다!

만약 Todo컴포넌트에 꼭 redux를 연결해서 사용하고 싶으시면 containers폴더로 컴포넌트를 이동시키셔서 사용하시면 됩니다!

이는 강제는 아니지만 개발할때 컴포넌트의 역할을 잘 구분해주셔야 프로젝트가 커졌을때 훨씬 깔끔하게 개발을 하실 수 있습니다

컨테이너 컨포넌트와 프리젠테이셔널 컴포넌트의 차이에 대한 글입니다!
https://blueshw.github.io/2017/06/26/presentaional-component-container-component/

handleSubmit = e => {
e.preventDefault();
const text = this.state.todo;
const todos = this.props.todos;
Copy link

Choose a reason for hiding this comment

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

const {todos } = this.props

로해주시면 좀더 범용적으로 사용 될수 있습니다!

return (
<div>
<form onSubmit={this.handleSubmit}>
<div onClick={this.handleDelete}>
Copy link

Choose a reason for hiding this comment

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

this.handleDelete는 안쓰시는것 같아요!

e.preventDefault();
const text = this.state.todo;
const todos = this.props.todos;
this.props.addTodo(text,todos);
Copy link

Choose a reason for hiding this comment

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

여기서 todos를 전달해주시는 이유가 있으신가요?!

const { removeTodo } = this.props.globalActions
return (
<li style={{ listStyle: "none" }}>
{this.props.todo.todo}
Copy link

Choose a reason for hiding this comment

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

8번째 줄에서

const { todo } = this.props

요렇게 해주셨으면

todo.id, todo.todo 이렇게 조금더 깔끔하게 쓸수 있을것 같습니다!

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