Skip to content

todo program design & code#71

Merged
crongro merged 4 commits intocode-squad:mukeunzifrom
mukeunzi:step4-1
Apr 15, 2019
Merged

todo program design & code#71
crongro merged 4 commits intocode-squad:mukeunzifrom
mukeunzi:step4-1

Conversation

@mukeunzi
Copy link
Copy Markdown

@mukeunzi mukeunzi commented Apr 15, 2019

JIN & mukeunzi

step4-1 할일 관리 프로그램 design + code

Copy link
Copy Markdown
Contributor

@crongro crongro left a comment

Choose a reason for hiding this comment

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

프로그래밍 설계한 내용이 있나요?
그 결과물(종이에 작성한 것이나, 수도코드나 등등)이 있나요?
있다면 보내주세요.

Copy link
Copy Markdown
Contributor

@crongro crongro left a comment

Choose a reason for hiding this comment

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

커다란 이미지! 잘 봤어요 ㅎ

리뷰한 내용 조금 수정해보세요!

todos.js Outdated
let newTodoList;
const makeNewTodoList = function(){
todos.forEach(function(todo){
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

다음엔 arrow function도 사용해보세요. 표현이 더 간결해져요.

todos.js Outdated
const printAll = function(){
let result = [];
for(key in newTodoList){
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

newTodoList가 배열인줄 알았는데, for-in을 사용한 것을 보니 객체인가보네요.
객체라면 이름으로 list보다는 object라고 하는 것이 더 자연스럽습니다.

todos.js Outdated

const checkTags = function(tag){
let result = [];
todos.forEach(function(list){
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

result 에 push 를 해서 새로운 배열을 만드는 데 forEach말고 더 적당한 메서드도 있을까요?

todos.js Outdated
newTodoList = {'todo' : [], 'doing' : [], 'done' : []};
makeNewTodoList();

if (keyWord == 'status') {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

== 과 === 의 차이를 이해하시고 더 적당한 것을 사용해보세요.

Copy link
Copy Markdown
Contributor

@crongro crongro left a comment

Choose a reason for hiding this comment

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

잘 수정했어요.

함수역할이 모두 명확한 편이라 좋습니다.
이름도 잘 지으셨고요.

고차함수 활용은 좀더 할 수 있을 거 같긴해요.
앞으로 잘 사용해보세요~!


const printAll = () => {
let result = [];
for(key in newTodoObject){
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

이부분도 고차함수를 활용할 수 있을 거 같네요.

}
];

let newTodoObject;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

전역변수가 존재하는 게 아쉽네요. ^^;
다음에는 이런 부분을 없애보려고 해보세요.

@crongro crongro merged commit 95b9897 into code-squad:mukeunzi Apr 15, 2019
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