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

[holee] typescript-calculator #2

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

hochan222
Copy link
Member

@hochan222 hochan222 commented Jun 1, 2021

BDD를 작성하면서, given, when, then 형식에 맞춰 테스트 케이스를 작성하려고했고, 전체 큰틀을 먼저정하고 각 섹션마다 테스트 케이스를 BDD 주기를 돌리면서 점점 세분화시켜가면서 구현했습니다. 코드를 짜기에 앞서서 먼저 계획을 세우고, 알맞은 동작을 먼저 정의해서 정해놓은 범위 안에서 기능들을 구현하는데 요구사항을 고민할 필요가 없어서 수월했습니다.

또한, 테스트 코드도 행동 중심으로 기술했기 때문에 프로그래밍을 모르는 사람도 테스트 코드가 어떤 동작를 테스트하는 코드인지 알 수 있다는 점이 BDD의 강점이라고 생각합니다.

  • 그 외
    • 타입스크립트를 처음 사용해보았는데 null과 any를 없애려고 노력했습니다.
    • 정규표현식을 사용해서 전체 코드량을 줄였습니다.

hochan222 added 25 commits June 1, 2021 15:17
Copy link
Member

@jwon42 jwon42 left a comment

Choose a reason for hiding this comment

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

데이터속성을 이용한 값 체크와 정규표현식을 이용한 검증 로직이 인상깊었고, 과제 진행 중 해당 부분을 차용했습니다.
cypress에서 파트들을 describe로 나누고 세부적인 것들을 it로 나눠야 테스트 결과 보기가 더 좋다는걸 알았네요.
전반적으로 가독성 좋고 깔끔한 코드 덕에 많이 배웠습니다 :)

Comment on lines +8 to +12
if (operator === '+') result = total + num;
if (operator === '/') result = total / num;
if (operator === '%') result = total % num;
if (operator === '-') result = total - num;
if (operator === 'X') result = total * num;
Copy link
Member

Choose a reason for hiding this comment

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

다양한 방법을 찾아보고 고민하다가 웹서버 메소드 나눌 때 생각이 나서 switch문을 사용해봤습니다.
https://github.com/transcendence42/javascript-calculator/pull/4/files#diff-2a93db0cc8b6388990eb7aa5c3a7ac60f207c94775e82ade82ec3503d0507c77

궁금해서 검색해보니 if문과 switch문이 속도에서 유의미한 차이는 없는 듯 합니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

저도 어셈블리 생각나서 조금 찾아보았습니다.

int main()
{
	int x=0;
	int y;

	if(x==0)  y = 0;
	else if(x==1) y = 1;
	else if(x==2) y = 2;
	else if(x==5) y = 5;
	else if(x==6) y = 6;


	switch(x)
	{
	case 0 : y = 0; break;
	case 1 : y = 1; break;
	case 4 : y = 4; break;
	case 5 : y = 5; break;
	case 6 : y = 6; break;
		
	}

	return 0;
}

을 어셈블리로 바꾸면

7:        if(x==0)  y = 0;
0040D4BF   cmp         dword ptr [ebp-4],0
0040D4C3   jne         main+2Eh (0040d4ce)
0040D4C5   mov         dword ptr [ebp-8],0
8:        else if(x==1) y = 1;
0040D4CC   jmp         main+68h (0040d508)
0040D4CE   cmp         dword ptr [ebp-4],1
0040D4D2   jne         main+3Dh (0040d4dd)
0040D4D4   mov         dword ptr [ebp-8],1
9:        else if(x==2) y = 2;
0040D4DB   jmp         main+68h (0040d508)
0040D4DD   cmp         dword ptr [ebp-4],2
0040D4E1   jne         main+4Ch (0040d4ec)
0040D4E3   mov         dword ptr [ebp-8],2
10:       else if(x==5) y = 5;
0040D4EA   jmp         main+68h (0040d508)
0040D4EC   cmp         dword ptr [ebp-4],5
0040D4F0   jne         main+5Bh (0040d4fb)
0040D4F2   mov         dword ptr [ebp-8],5
11:       else if(x==6) y = 6;
0040D4F9   jmp         main+68h (0040d508)
0040D4FB   cmp         dword ptr [ebp-4],6
0040D4FF   jne         main+68h (0040d508)
0040D501   mov         dword ptr [ebp-8],6
12:
13:
14:       switch(x)
15:       {
0040D508   mov         eax,dword ptr [ebp-4]
0040D50B   mov         dword ptr [ebp-0Ch],eax
0040D50E   cmp         dword ptr [ebp-0Ch],6
0040D512   ja          $L286+7 (0040d549)
0040D514   mov         ecx,dword ptr [ebp-0Ch]
0040D517   jmp         dword ptr [ecx*4+40D552h]
16:       case 0 : y = 0; break;
0040D51E   mov         dword ptr [ebp-8],0
0040D525   jmp         $L286+7 (0040d549)
17:       case 1 : y = 1; break;
0040D527   mov         dword ptr [ebp-8],1
0040D52E   jmp         $L286+7 (0040d549)
18:       case 4 : y = 4; break;
0040D530   mov         dword ptr [ebp-8],4
0040D537   jmp         $L286+7 (0040d549)
19:       case 5 : y = 5; break;
0040D539   mov         dword ptr [ebp-8],5
0040D540   jmp         $L286+7 (0040d549)
20:       case 6 : y = 6; break;
0040D542   mov         dword ptr [ebp-8],6
21:
22:       }

라고 하는데요. 다르게 변환되는걸 볼 수 있습니다. switch문의 경우 hash table 혹은 배열로 만들어진 jump table로 구현이 된다고 합니다.
조건이 적을때는 if문이 더 좋을 수 있지만, 조건이 많아지면 switch문이 더 효율적입니다. 물론, 가독성이 더 중요하겠지만요..! 아래는 최적화에 대한 좋은 글 있어서 링크올렸습니다.

https://12bme.tistory.com/134

if (Number(totalTarget.innerText) === 0) {
return;
}
else if (!['/', '-', 'X', '+'].includes(totalTarget.innerText[totalTarget.innerText.length - 1])) {
Copy link
Member

Choose a reason for hiding this comment

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

이 부분도 정규표현식을 이용할 수 있지 않을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

오.. 맞아요 test 문법을 사용하면 간단하게 표현가능할 수 있을것 같아요!


const calculateResult = (total: string): number => {
const result: RegExpMatchArray | null = total.match(
new RegExp('(\\-?[\\d]{1,3})(X|\\-|\\+|\\/|\\=)(\\-?[\\d]{1,3})')
Copy link
Member

Choose a reason for hiding this comment

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

가운데 연산자를 추출하기 위한 정규표현식에서 =는 필요 없어 보입니다.

(과제 진행하며 정규표현식 사용에 대해 많은 도움을 받았습니다 👍)

Copy link
Member Author

Choose a reason for hiding this comment

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

맞네요! ㅎㅎ 오.. 감사합니다 :)

Copy link

@yhshin0 yhshin0 left a comment

Choose a reason for hiding this comment

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

잘 봤습니다. 테스트도 함수로 빼내어 깔끔하게 작성하신 것이 좋았습니다.

수고하셨습니다!

Comment on lines +1 to +8
const testInputClickEvent = (first, result) => {
for (let item of first) {
cy.get('.digit')
.contains(item)
.click();
}
cy.get('#total').should('have.text', result);
};
Copy link

Choose a reason for hiding this comment

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

함수로 분리하니까 훨씬 깔끔하고 좋네요

@@ -4,7 +4,7 @@
<meta charset="UTF-8" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>Calculator</title>
<link rel="stylesheet" type="text/css" href="src/css/index.css" />
<link rel="stylesheet" type="text/css" href="dist/css/index.css" />
Copy link

Choose a reason for hiding this comment

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

dist 라는 이름의 의미는 뭔가요?

Copy link
Member

Choose a reason for hiding this comment

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

타입스크립트로 작성한 파일이 js로 변환되어 저장되는 폴더입니다.
tsconfig.json파일의 "outDir" 에서 확인 가능합니다.
https://github.com/transcendence42/javascript-calculator/pull/2/files#diff-b55cdbef4907b7045f32cc5360d48d262cca5f94062e353089f189f4460039e0

Copy link
Member Author

Choose a reason for hiding this comment

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

dist는 distribute의 약자로 배포의 의미를 가지고 있습니다. src에는 코드 작성부인 ts 파일들이있고 js파일들을 tsconfig를 통해서 dist로 분리시켰습니다!

Comment on lines +7 to +8
if (operator === '%')
result = total % num;
Copy link

Choose a reason for hiding this comment

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

프로그램에서 % 연산이 쓰이는 것 같진 않네요

Copy link
Member Author

Choose a reason for hiding this comment

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

앗앗 맞아요 ㅎㅎ 예전에 쓰던 함수를 그대로 긁어와서 레거시가 생겼네요..!

const getCountOperationdataSet = (): string => {
const operations = document.getElementsByClassName(
'operations'
)[0]! as HTMLElement;
Copy link

Choose a reason for hiding this comment

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

HTMLDivElement로 바꿔도 될 것 같습니다

Copy link
Member Author

Choose a reason for hiding this comment

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

저도 동의합니다. 타입스크립트로 작성하는 만큼 조금 더 구체적 타입을 지정해야한다고 생각합니다.

Comment on lines +55 to +57
const ACEvent = (): void => {
document.getElementById('total')!.innerText = '0';
};
Copy link

Choose a reason for hiding this comment

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

AC를 클릭할 때 operations data-count도 초기화를 하는 것이 좋을것 같아요(setCountOperationdataSet('0');)

예를 들어, 6+4를 누르고 AC를 누른 다음, 5+3을 누르려고 하면 +가 나타나지 않습니다

Copy link
Member

Choose a reason for hiding this comment

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

오 좋은 피드백이네요

Copy link
Member Author

Choose a reason for hiding this comment

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

오.. 감사합니다!

Copy link
Member

@yechoi42 yechoi42 left a comment

Choose a reason for hiding this comment

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

정규표현식을 사용해서 코드를 짠 부분이 깔끔했어요! 테스트 코드도 기능별로 잘 쪼갠 것 같고요.
하나 제안 사항은 결과값이 3자리를 넘어서면, 3자리로 잘라주는데 그 타이밍을 좀 일찍 잡아주면 더 좋을 것 같아요.

예를 들면 123*123 = 15129 결과값을 받고 나서
+ 를 입력하면 여기까지 그대로 잘 입력되고
그다음에 숫자를 입력하면 151로 잘리는데,

+를 입력할때 15129가 151로 잘리고 + 를 보여주면 더 자연스러울 것 같아요 !

Comment on lines +9 to +10
const eventTarget: HTMLElement = e.target as HTMLElement;
const totalTarget: HTMLElement | null = document.getElementById('total');
Copy link
Member

Choose a reason for hiding this comment

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

eventTarget은 타입단언을 해줬는데 totalTarget은 하지 않은 이유가 혹시 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

처음 쓰다보니 에러로 안잡아줘서 잘 몰랐습니다.

const totalTarget: HTMLHeadingElement | null = document.getElementById('total') as HTMLHeadingElement;

지금은 위와같이 구체화 시키는게 좋다고 생각합니다.

Comment on lines +5 to +19
if (!result) {
return '';
}
if (result[2] === undefined) {
if (result[1] === undefined) {

return '';
}

return result[1];
}
if (result[3] === undefined) {

return result[1] + result[2];
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!result) {
return '';
}
if (result[2] === undefined) {
if (result[1] === undefined) {
return '';
}
return result[1];
}
if (result[3] === undefined) {
return result[1] + result[2];
}
if (!result) {
return '';
}
else if (result[2] === undefined) {
if (result[1] === undefined) {
return '';
}
return result[1];
}
else if (result[3] === undefined) {
return result[1] + result[2];
}

@@ -0,0 +1,22 @@
export const checkValidInput = (total: string): string => {
Copy link
Member

Choose a reason for hiding this comment

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

이 함수 깔끔하네요 !

Comment on lines +9 to +14
const setCountOperationdataSet = (count: string): void => {
const operations = document.getElementsByClassName(
'operations'
)[0]! as HTMLElement;
operations.dataset['count'] = count;
};
Copy link
Member

Choose a reason for hiding this comment

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

여기서 사용하는 플래그가 스트링 자료형이니까, '0', '1'보단 기왕이면 의미를 담은 문자열이면 더 좋을 것 같아요 !

Copy link
Member Author

Choose a reason for hiding this comment

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

동의합니다!

Comment on lines +55 to +57
const ACEvent = (): void => {
document.getElementById('total')!.innerText = '0';
};
Copy link
Member

Choose a reason for hiding this comment

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

오 좋은 피드백이네요

@hochan222 hochan222 changed the title [holee] javascript-calculator [holee] typescript-calculator Jun 5, 2021
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.

4 participants