[yshin] javascript-calculator by yhshin0 · Pull Request #3 · transcendence42/javascript-calculator

yhshin0 · 2021-06-03T04:13:03Z

테스트 케이스에 대해 많이 고민하고 프로그램을 설계해야 기능을 추가하거나 수정할 때 어려움이 적은 것 같습니다.

hochan222

우와... 엄청 깔끔하네요.. 무엇보다 기능별로 잘 나뉘어져 있어서 좋았습니다. 저도 다음에 짤때는 부분부터 먼저 나누고 짜야겠습니다. 고생하셨습니다 :)

hochan222 · 2021-06-04T03:52:53Z

.gitignore

 .vscode-test

+# javascript source file
+src/**/*.js


LGTM :) 👍👍

hochan222 · 2021-06-04T04:07:58Z

src/ts/equation.ts

@@ -0,0 +1,39 @@
+interface IEquation {


오... 인터페이스 너무 좋아요..!

저도 다음번엔 인터페이스를 활용해봐야겠어요 !
단순 멤버변수로 뒀을 수도 있을 텐데, 인터페이스 상속해온 이유가 있나요?

사실 여기서는 큰 의미가 없습니다. 인터페이스를 구현하여 속성을 강제한다거나, 확장을 위해 클래스에 인터페이스를 구현한다고 하는데요. 우선은 조금이나마 익숙해지기 위해 인터페이스를 사용해보았습니다.

hochan222 · 2021-06-04T04:11:02Z

src/ts/app.ts

+    }
+  }
+  clickOperations(target: HTMLButtonElement, equation: Equation) {
+    if (target.className !== "operation") {


아.. 이렇게 간단하게 처리를 할 수 있었네요!

hochan222 · 2021-06-04T04:14:05Z

src/ts/app.ts

+    document
+      .querySelector("div.modifiers.subgrid")!


Suggested change

document

.querySelector("div.modifiers.subgrid")!

$("div.modifiers.subgrid")!

https://github.com/transcendence42/javascript-calculator/pull/4/files#r645262877

hochan222 · 2021-06-04T04:15:08Z

src/ts/app.ts

+      return;
+    }
+    const operation: string = target.innerText;
+    const num = Number(this.totalDiv.innerText);


Suggested change

const num = Number(this.totalDiv.innerText);

const num: number = Number(this.totalDiv.innerText);

저는 적는게 좋다고 생각하는데 혹시 어떻게 생각하시나요?

그게 맞는 것 같습니다. 타입스크립트 쓰는 김에 타입을 적을 수 있는 부분은 모두 적는게 좋은 것 같아요.

jwon42

코드가 간결하고 가독성이 좋아 전체 코드 중 가장 읽기 편했습니다.
클래스를 사용하면 클래스 내부에서 공통적으로 사용되는 변수들에 대해 정의하고 사용할 수 있기 때문에 마치 전역변수를 사용하는 것 같은 효과가 있군요.
수고하셨습니다 :)

jwon42 · 2021-06-04T04:43:40Z

src/ts/app.ts

+  private plus(equation: Equation): number {
+    return equation.firstNum + equation.secondNum;
+  }
+  private minus(equation: Equation): number {
+    return equation.firstNum - equation.secondNum;
+  }
+  private multiply(equation: Equation): number {
+    return equation.firstNum * equation.secondNum;
+  }
+  private divide(equation: Equation): number {
+    return Math.floor(equation.firstNum / equation.secondNum);
+  }


연산 부분을 함수로 분리해두어서 가독성이 굉장히 좋습니다.
다만 한 줄로 표현될 수 있는 내용이 함수로 분리되어 비용 지출이 있는건 아닐지 고민해보아야 할 듯 합니다.

jwon42 · 2021-06-04T04:45:31Z

src/ts/equation.ts

+interface IEquation {
+  firstNum: number;
+  secondNum: number;
+  operation: string;
+}


typedef 하는 느낌으로 interface를 사용하셨다고 이해하면 될까요?

약간 다릅니다. 큰 의미는 없지만 클래스에서 해당 변수를 사용하도록 강제하기 위해 인터페이스를 사용하였습니다.

jwon42 · 2021-06-04T04:49:38Z

src/ts/app.ts

+    if (target.className !== "digit") {
+      return;
+    }
+    if (isNaN(Number(this.totalDiv.innerText))) {


inNaN의 경우 문자 가장 앞에 0x가 오는 경우 16진수로 이해하기 때문에 0을 대상으로 한 연산도 진행하기 위해서 정규표현식을 이용한 별도의 숫자여부 체크 함수를 만들어 사용했었습니다.

yechoi42

제일 좋았던 건 equeation이라는 클래스에서 firstNum, secondNum, operation 정보를 저장해두는 부분이었습니다. 이 자료 형태가 있어서 코드가 전반적으로 깔끔했던 것 같아요. 별도의 파싱 과정도 필요 없었고요. 잘 보고 갑니다 ~

yechoi42 · 2021-06-04T05:55:38Z

src/ts/equation.ts

@@ -0,0 +1,39 @@
+interface IEquation {


저도 다음번엔 인터페이스를 활용해봐야겠어요 !
단순 멤버변수로 뒀을 수도 있을 텐데, 인터페이스 상속해온 이유가 있나요?

yechoi42 · 2021-06-04T06:02:13Z

src/ts/equation.ts

+  setFirstNum(num: number) {
+    this.firstNum = num;
+  }


객체 리터럴 안에서 getter와 setter 메서드는 get과 set으로 나타낼 수 있습니다.

아래처럼 사용하는데요

let user = { get name() { return this._name; }, set name(value) { this._name = value; } }; user.name = "Pete"; alert(user.name); // Pete

이것도 set FirstNum(num: number)로 만들어두면 Equation.firstNum = number 이런 식으로 사용해볼 수 있을 것 같습니다.

yechoi42 · 2021-06-04T06:06:04Z

src/ts/app.ts

+    this.totalDiv = document.querySelector("#total") as HTMLDivElement;
+    document


이렇게 멤버변수로 만들어 두는 거 유용하네요

yechoi42 · 2021-06-04T06:15:15Z

src/ts/app.ts

+    if (target.className !== "digit") {
+      return;


querySelector("div.digits.flex")에 더해진 이벤트 리스너인데 target.className이 digit이 아닐 때도 있을까요?

버튼 사이 경계 부근을 클릭하면 가끔 잘못 인식해서 숫자가 아닌 이상한 값이 입력되는 경우가 있습니다.

예를 들면, 9랑 8 사이를 클릭했는데 9부터 0까지 출력되는 경우가 있었어요

이러한 경우를 방지하기 위해 상기 코드를 작성하였고, 이후 테스트에서 같은 현상은 발견되지 않았습니다.

만약 같은 현상이 재발한다면 해당 버그를 해결하기 위한 코드를 생각해봐야 할 것 같습니다.

yechoi42 · 2021-06-04T06:21:24Z

src/ts/app.ts

+    }
+    if (this.totalDiv.innerText.length < 3) {
+      this.totalDiv.insertAdjacentText("beforeend", target.innerText);
+      this.totalDiv.innerText = String(Number(this.totalDiv.innerText));


이미 innerText는 string일텐데 한번더 형변환 처리해주는 이유가 뭔가요??

숫자로 변환한 값을 innerText에 넣기 위해 다시 string으로 형변환을 합니다.

yhshin0 · 2021-06-04T07:10:48Z

퇴고가 필요한 것 같습니다...

부끄럽게도 어제 완성한 코드인데 if문이 어떤 케이스 때문에 사용되는지 바로 이해가 되지 않았네요...

Younghak Shin added 16 commits June 3, 2021 10:48

docs: add test, env settings to README

823a98e

chore: eslint, prettier, cypress

2c7ed01

feat: add test case by cypress

ad9bc89

feat: add class App

c124ab2

feat: add class Equation

128e23a

feat: add click num

7f72ab5

chore: modify gitignore to exclude js

e517c81

feat: add calculate

24f6ffa

feat: add AC

60695f6

refactor: separate AC from constructor

24a0e8d

refactor: separate typeNumber from constructor

17dce6d

refactor: separate clickOperation from constructor

fffa44a

fix: click operation

14c710e

fix: click number after calculating

b7e8393

feat: remove click number test

af548d5

delete: index.js

c1a19a0

hochan222 approved these changes Jun 4, 2021

View reviewed changes

jwon42 approved these changes Jun 4, 2021

View reviewed changes

yechoi42 approved these changes Jun 4, 2021

View reviewed changes

	document
	.querySelector("div.modifiers.subgrid")!
	$("div.modifiers.subgrid")!

	const num = Number(this.totalDiv.innerText);
	const num: number = Number(this.totalDiv.innerText);

		this.totalDiv = document.querySelector("#total") as HTMLDivElement;
		document

Conversation

yhshin0 commented Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hochan222 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwon42 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yechoi42 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yhshin0 commented Jun 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yhshin0 commented Jun 3, 2021 •

edited

Loading