feat: 과제2 계산기 구현 제출 #155

alstjr0183 · 2022-06-08T15:06:42Z

테스트랑 lint는 확인했는데 이번엔 저번 과제랑 달리 변수를 선언을 해서 구현을 했는데 이것두 잘못된 방향일까봐 걱정이되긴합니다..!
피드백 주시면 감사하겠습니다!!

테스트랑 lint는 확인했는데 이번엔 저번 과제랑 달리 변수를 선언을 해서 구현을 했는데 이것두 잘못된 방향일까봐 걱정이되긴합니다..! 피드백 주시면 감사하겠습니다!!

passwd10

계산기 로직이 복잡한데도 불구하고 테스트 요구사항을 잘 구현하고 계신 것 같아요 👍
다만 이 상태로 추가기능을 붙여야한다면, 금방 구현할 수 있을까요?

어떻게 하면 코드의 복잡도를 최소화할 수 있는지를 고민하면서 리팩토링을 진행해보시면 좋을 것 같아요!

passwd10 · 2022-06-08T15:30:01Z

src/index.js

+function render(
+  propObj = {
+    num1: '',
+    num2: '',
+    operator: '',
+    result: 0,
+    show: '0',
+  },
+) {
+  const obj = propObj;
+  const {
+    num1, num2, operator, result, show,
+  } = obj;


function render({ num1 = '', num2= '', operator= '', result= 0, show= '0', }) { }

다음과 같이 작성하면 훨씬 깔끔해질 것 같아요!

수정했습니다!

passwd10 · 2022-06-08T15:31:31Z

src/index.js

+    num1, num2, operator, result, show,
+  } = obj;
+  const numArray = ['1', '2', '3', '4', '5', '6', '7', '8', '9', '0'];
+  const operatorArray = ['+', '-', '*', '/', '='];


#154 (comment)

해당 커밋을 참고해서 변수명을 작성해주세요!

numbers 랑 operators 라고 변경해보았습니다!

passwd10 · 2022-06-08T15:33:42Z

src/index.js

+  const setNumInit = () => {
+    setState('num1', '');
+    setState('num2', '');
+  };
+
+  // num1 상태 셋팅
+  const setNum1 = (num) => {
+    const value = num1 + num;
+    setState('num1', value);
+    setState('show', value);
+  };
+
+  // num2 상태 셋팅
+  const setNum2 = (num) => {
+    const value = num2 + num;
+    setState('num2', value);
+    setState('show', value);
+  };


각각의 함수가 하는 역할이 비슷해보이지 않나요? 재사용 가능한 함수를 고민해보세요!

setNum 이라는 함수를 만들어서 중복을 줄여봤습니다!

passwd10 · 2022-06-08T15:36:31Z

src/index.js

+  const handleOperator = (propOperator) => {
+    if (operator) {
+      // 연산 버튼을 한번이상 누른 로직
+      if (result) {
+        // 한번이상 진행한 계산일때 로직
+        if (operator === '+') {
+          const value = result + Math.floor(num1);
+          setState('result', value);
+        }
+        if (operator === '-') {
+          const value = result - Math.floor(num1);
+          setState('result', value);
+        }
+        if (operator === '*') {
+          const value = result * Math.floor(num1);
+          setState('result', value);
+        }
+        if (operator === '/') {
+          const value = result / Math.floor(num1);
+          setState('result', value);
+        }
+        if (operator === '=') {
+          setState('result', result);
+        }
+      } else {
+        // 처음 계산할때
+        if (operator === '+') {
+          const value = Math.floor(num1) + Math.floor(num2);
+          setState('result', value);
+        }
+        if (operator === '-') {
+          const value = Math.floor(num1) - Math.floor(num2);
+          setState('result', value);
+        }
+        if (operator === '*') {
+          const value = Math.floor(num1) * Math.floor(num2);
+          setState('result', value);
+        }
+        if (operator === '/') {
+          const value = Math.floor(num1) / Math.floor(num2);
+          setState('result', value);
+        }
+        if (operator === '=') {
+          setState('result', result);
+        }
+      }
+      // 여기 부분도 setState 함수로 상태변경을 시키려했는데 이 로직안에서 이미 한번 사용해서 그런가싶었는데 밑에 부분에서는 또 써도 에러가안나네요..ㅠ
+      obj.show = obj.result;
+      setNumInit();
+    }
+    setState('operator', propOperator);
+    render(obj);
+  };


반복문이 중첩되게 되면 코드의 가독성이 안좋아져요.

if ~ else 를 사용하지 않고 리팩토링을 진행해보세요!

반복되는 로직이 보이는데, 함수로 분리할 수 있을 것 같아요

여기 부분이 어려웠었는데 구글링을해보니 if else 말고 Object Literals 이라는 방법이 있더라구요! 그걸 이용해서 적용해보았습니다
중간중간에 구분하기위한 if문들이 있는데 여기부분까지 없애보려했지만 방법이 잘 떠오르지 않더라구요 ㅠㅠ

passwd10 · 2022-06-08T15:38:48Z

src/index.js

+  };
+
+  // 숫자 버튼 클릭 이벤트
+  const handleCalculate = (num) => {


핸들링하는 대상이 각 숫자 버튼이기 때문에 handleClickNumberButton 과 같은 표현이 더 자연스러운 것 같아요!

선생님이 말씀해주신부분도 수정하구 그밑에 연사자 버튼 클릭했을때의 네이밍도 handleClickOperatorButton 이렇게 변경해보았습니다!

passwd10 · 2022-06-08T15:51:29Z

src/index.js

+      // 여기 부분도 setState 함수로 상태변경을 시키려했는데 이 로직안에서 이미 한번 사용해서 그런가싶었는데 밑에 부분에서는 또 써도 에러가안나네요..ㅠ
+      obj.show = obj.result;


= 을 클릭했을 때 setState 가 위에 조건문들을 통과하면서 불필요한 순간에 여러번 불리고, 원하는 값이 나오지 않는 것 같아요. console을 찍으면서 디버깅 해보세요!

여기부분에서 = 는 딱히 결과를 보여주는 기능말고 계산하는부분은 없어서 뺐더니 setState 함수도 작동하더라구요!

피드백 주신 내용들로 리팩토링을 진행해보았습니다

alstjr0183 · 2022-06-10T10:36:40Z

상세한 피드백 감사합니다! 피드백 주신 내용들로 리팩토링을 진행해보았습니다 ㅎ

passwd10 · 2022-06-10T13:25:35Z

src/index.js

+  const numbers = ['1', '2', '3', '4', '5', '6', '7', '8', '9', '0'];
+  const operators = ['+', '-', '*', '/', '='];


render 함수 밖에서 선언하셔도 될 것 같아요!

수정했습니다!

passwd10 · 2022-06-10T13:26:30Z

src/index.js

+function render(propObj = {
+  num1: '',
+  num2: '',
+  operator: '',
+  result: 0,
+  show: '0',
+}) {
  const obj = propObj;
  const {
    num1, num2, operator, result, show,
  } = obj;


바로 destructuring 해서 사용하시면 불필요한 코드들이 많이 줄어들 것 같아요!

render 함수밖에 initialState 선언후 render함수 사용할때 객체로 한번에 넘기고 싶어서 obj담아서 사용해보았습니다..!

passwd10 · 2022-06-10T13:27:19Z

src/index.js

    if (result || operator === '') {
-      setNum1(num);
+      setNum(num, 'num1');
    } else {
-      setNum2(num);
+      setNum(num, 'num2');
    }


이 부분도 if else 를 사용하지 않고 리팩토링해보세요!

if 를 사용하지않고 하는 방법이 떠오르질 않더라구요 ㅠㅠ 그래서 알려주신 GuardClause를 사용해보았습니다!

passwd10 · 2022-06-10T13:28:05Z

src/index.js

            onClick={() => {
-              handleCalculate(i);
+              handleClickNumberButton(i);
            }}


onClick={() => handleClickNumberButton(i);} 으로 바로 작성하셔도 될 것 같아요

수정했습니다!

passwd10 · 2022-06-10T13:29:56Z

src/index.js

+  const app = document.getElementById('app');
+  app.textContent = '';


반복되는 코드를 리팩토링 하셨네요 👍

선언하는곳과 사용하는곳을 공백으로 구분해주시면 가독성이 더 좋아질 것 같아요

수정했습니다!

passwd10 · 2022-06-10T13:32:02Z

src/index.js

+    if (operator) {
+      if (result) {
+        Calculate(result, Math.floor(num1));
+      } else {
+        Calculate(Math.floor(num1), Math.floor(num2));
+      }
+    }


GuardClause 패턴을 사용해서 조건문의 중첩을 최소화 해보세요!

알려주신 패턴을 사용해서 해보았는데 좀더 줄일수있는 방법이있을까요?!

@alstjr0183
저라면 먼저 중첩되는 반복문을 다음과 같이 함수로 분리할 것 같아요. 이렇게 하면 operator 가 있을 때 operator 와 관련된 어떤 연산을 하는구나? 라는걸 내부 구현을 확인하지 않고도 알 수 있겠죠?

const updateOperator = (newOperator) => { setState('operator', newOperator); render(obj); }; const calculateOperator = () => { if (result) { Calculate(result, Math.floor(num1)); return; } Calculate(Math.floor(num1), Math.floor(num2)); }; const handleClickOperatorButton = (propOperator) => { if (operator) { calculateOperator(); } updateOperator(propOperator); };

calculateOperator 함수에서 내부에서 반복되는 패턴이 보이기 때문에 다음과 같이 리팩토링을 진행할 수 있을 것 같아요.

const handleClickOperatorButton = (propOperator) => { if (operator) { Calculate(result || Math.floor(num1), Math.floor(result ? num1 : num2)); } updateOperator(propOperator); };

피드백 주신 내용들로 리팩토링을 해보았습니다..!

alstjr0183 · 2022-06-12T03:15:34Z

피드백 감사합니다..! 원래 어제 올렸어야했는데 if문 제거나 중첩최소화하는 방법이 떠오르질않아 로직자체를 수정해보고 고민고민하면서 결국 제자리지만 정리후 오늘에서야 올립니다 ㅠㅠ

passwd10 · 2022-06-12T13:03:16Z

첫주차 과제 진행하시느라 고생 많으셨어요! 👏👏
과제풀이 영상을 통해 아샬님이 어떻게 문제를 해결하셨는지, 어떤점이 다른지를 비교해보세요ㅎㅎ
주간회고도 잊지않고 작성해주시고요~ :)

feat: 과제2 계산기 구현 제출

524e5d6

테스트랑 lint는 확인했는데 이번엔 저번 과제랑 달리 변수를 선언을 해서 구현을 했는데 이것두 잘못된 방향일까봐 걱정이되긴합니다..! 피드백 주시면 감사하겠습니다!!

passwd10 reviewed Jun 8, 2022

View reviewed changes

fix: 리팩토링 진행

8ce7a61

피드백 주신 내용들로 리팩토링을 진행해보았습니다

passwd10 reviewed Jun 10, 2022

View reviewed changes

refactor : 리팩토링 작업

d1cea62

피드백 주신 내용들로 리팩토링을 해보았습니다..!

passwd10 merged commit 19ba960 into CodeSoom:alstjr0183 Jun 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: 과제2 계산기 구현 제출 #155

feat: 과제2 계산기 구현 제출 #155

alstjr0183 commented Jun 8, 2022

passwd10 left a comment

passwd10 Jun 8, 2022

alstjr0183 Jun 10, 2022

passwd10 Jun 8, 2022

alstjr0183 Jun 10, 2022

passwd10 Jun 8, 2022

alstjr0183 Jun 10, 2022

passwd10 Jun 8, 2022

alstjr0183 Jun 10, 2022

passwd10 Jun 8, 2022

alstjr0183 Jun 10, 2022

passwd10 Jun 8, 2022

alstjr0183 Jun 10, 2022

alstjr0183 commented Jun 10, 2022

passwd10 Jun 10, 2022

alstjr0183 Jun 12, 2022

passwd10 Jun 10, 2022

alstjr0183 Jun 12, 2022

passwd10 Jun 10, 2022

alstjr0183 Jun 12, 2022

passwd10 Jun 10, 2022

alstjr0183 Jun 12, 2022

passwd10 Jun 10, 2022

alstjr0183 Jun 12, 2022

passwd10 Jun 10, 2022

alstjr0183 Jun 12, 2022

passwd10 Jun 13, 2022

alstjr0183 commented Jun 12, 2022 •

edited

passwd10 commented Jun 12, 2022

		// 여기 부분도 setState 함수로 상태변경을 시키려했는데 이 로직안에서 이미 한번 사용해서 그런가싶었는데 밑에 부분에서는 또 써도 에러가안나네요..ㅠ
		obj.show = obj.result;

		const numbers = ['1', '2', '3', '4', '5', '6', '7', '8', '9', '0'];
		const operators = ['+', '-', '*', '/', '='];

		const app = document.getElementById('app');
		app.textContent = '';

feat: 과제2 계산기 구현 제출 #155

feat: 과제2 계산기 구현 제출 #155

Conversation

alstjr0183 commented Jun 8, 2022

passwd10 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

alstjr0183 commented Jun 10, 2022

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

alstjr0183 commented Jun 12, 2022 • edited

passwd10 commented Jun 12, 2022

alstjr0183 commented Jun 12, 2022 •

edited