-
Notifications
You must be signed in to change notification settings - Fork 3
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
base: main
Are you sure you want to change the base?
Conversation
…igit when calculate +
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
데이터속성을 이용한 값 체크와 정규표현식을 이용한 검증 로직이 인상깊었고, 과제 진행 중 해당 부분을 차용했습니다.
cypress에서 파트들을 describe로 나누고 세부적인 것들을 it로 나눠야 테스트 결과 보기가 더 좋다는걸 알았네요.
전반적으로 가독성 좋고 깔끔한 코드 덕에 많이 배웠습니다 :)
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; |
There was a problem hiding this comment.
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문이 속도에서 유의미한 차이는 없는 듯 합니다.
There was a problem hiding this comment.
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문이 더 효율적입니다. 물론, 가독성이 더 중요하겠지만요..! 아래는 최적화에 대한 좋은 글 있어서 링크올렸습니다.
if (Number(totalTarget.innerText) === 0) { | ||
return; | ||
} | ||
else if (!['/', '-', 'X', '+'].includes(totalTarget.innerText[totalTarget.innerText.length - 1])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분도 정규표현식을 이용할 수 있지 않을까요?
There was a problem hiding this comment.
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})') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
가운데 연산자를 추출하기 위한 정규표현식에서 =
는 필요 없어 보입니다.
(과제 진행하며 정규표현식 사용에 대해 많은 도움을 받았습니다 👍)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞네요! ㅎㅎ 오.. 감사합니다 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
잘 봤습니다. 테스트도 함수로 빼내어 깔끔하게 작성하신 것이 좋았습니다.
수고하셨습니다!
const testInputClickEvent = (first, result) => { | ||
for (let item of first) { | ||
cy.get('.digit') | ||
.contains(item) | ||
.click(); | ||
} | ||
cy.get('#total').should('have.text', result); | ||
}; |
There was a problem hiding this comment.
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" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dist 라는 이름의 의미는 뭔가요?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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로 분리시켰습니다!
if (operator === '%') | ||
result = total % num; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
프로그램에서 % 연산이 쓰이는 것 같진 않네요
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTMLDivElement로 바꿔도 될 것 같습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 동의합니다. 타입스크립트로 작성하는 만큼 조금 더 구체적 타입을 지정해야한다고 생각합니다.
const ACEvent = (): void => { | ||
document.getElementById('total')!.innerText = '0'; | ||
}; |
There was a problem hiding this comment.
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을 누르려고 하면 +가 나타나지 않습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 좋은 피드백이네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오.. 감사합니다!
There was a problem hiding this 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로 잘리고 + 를 보여주면 더 자연스러울 것 같아요 !
const eventTarget: HTMLElement = e.target as HTMLElement; | ||
const totalTarget: HTMLElement | null = document.getElementById('total'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eventTarget은 타입단언을 해줬는데 totalTarget은 하지 않은 이유가 혹시 있을까요?
There was a problem hiding this comment.
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;
지금은 위와같이 구체화 시키는게 좋다고 생각합니다.
if (!result) { | ||
return ''; | ||
} | ||
if (result[2] === undefined) { | ||
if (result[1] === undefined) { | ||
|
||
return ''; | ||
} | ||
|
||
return result[1]; | ||
} | ||
if (result[3] === undefined) { | ||
|
||
return result[1] + result[2]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 함수 깔끔하네요 !
const setCountOperationdataSet = (count: string): void => { | ||
const operations = document.getElementsByClassName( | ||
'operations' | ||
)[0]! as HTMLElement; | ||
operations.dataset['count'] = count; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기서 사용하는 플래그가 스트링 자료형이니까, '0', '1'보단 기왕이면 의미를 담은 문자열이면 더 좋을 것 같아요 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
동의합니다!
const ACEvent = (): void => { | ||
document.getElementById('total')!.innerText = '0'; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 좋은 피드백이네요
BDD를 작성하면서, given, when, then 형식에 맞춰 테스트 케이스를 작성하려고했고, 전체 큰틀을 먼저정하고 각 섹션마다 테스트 케이스를 BDD 주기를 돌리면서 점점 세분화시켜가면서 구현했습니다. 코드를 짜기에 앞서서 먼저 계획을 세우고, 알맞은 동작을 먼저 정의해서 정해놓은 범위 안에서 기능들을 구현하는데 요구사항을 고민할 필요가 없어서 수월했습니다.
또한, 테스트 코드도 행동 중심으로 기술했기 때문에 프로그래밍을 모르는 사람도 테스트 코드가 어떤 동작를 테스트하는 코드인지 알 수 있다는 점이 BDD의 강점이라고 생각합니다.