-
Notifications
You must be signed in to change notification settings - Fork 42
[HYEON] STEP 9 : 문서화 #58
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
Conversation
함수가 반환을 안할 수도 있는데 가급적 지정된 값을 반환하도록 구현하면, 테스트코드 구현하기가 쉬워요. |
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,0 +1,219 @@ | |||
> #### STEP 9 : 문서화 |
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.
step9의 시각화된 정보를 문서로 표현한 건 프로그램 구조를 쉽게 이해할 수 있어서 좋네요.
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.
더 노력하겠습니다 👍
아, 네.. 그 arrayParser와 objectParser 는 함수가 반환하는게 거의 없어서.. 테스트코드 작성이 조금 어렵더라구요 :( 나머지 부분에서 테스트 할 수 있는 함수들을 찾아서 다시 PR 하겠습니다 ! :) |
printManager 에서 계산을 담당하는 부분과 출력 부분 총 2가지에 대한 테스트 코드를 작성하였습니다.
반환값이 없어도 그 함수를 테스트 할 수는 있어요. |
아아, 바로 테스트 후 다시 commit 하겠습니다 |
return이 없는 메서드에 대한 테스트코드 1개를 추가하였습니다. 기존의 값을 수정하기 위해 startSquareBracket 변수에 대한 get/set 코드를 추가한 후, 정상적으로 바뀌는지 테스트하였습니다.
다양한 경우에 대해서 테스트코드를 추가하고 있습니다. 잠시만 기다려주세요 :) |
괄호 갯수를 조정하는 함수를 호출 후, 클래스 내에서 값이 정상적으로 바뀌는지 확인한다 : OK 2개 이상의 시작괄호 조건을 검사한다 : OK 시작한 괄호가 닫히는 문자인지 확인한다 : OK Type을 결정하고, resultObject에 추가되는 값이 정상적인지 확인한다 : OK 재귀가 동작하는지, 정상적인 결과값을 반환하는지 확인한다 : OK
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/arrayParser.test.js
Outdated
arrayParser.setMergeData("123"); | ||
arrayParser.determineType(); | ||
const testResult = arrayParser.getResultObject(); | ||
const answer = {"type":"Array","child":[{"type":"Number","value":"123","child":[]},{"type":"Array","child":[{"type":"Number","value":"22","child":[]}]},{"type":"Number","value":"33","child":[]},{"type":"Number","value":"123","child":[]}]}; |
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() 함수에서 arrayParser 객체를 share하면서 하고 있는데요.
이렇게 하기보다는,
각각의 test 함수안에서 새로운 객체를 만들어서 테스트 하는 것을 더 권장합니다.
즉 test함수끼리 서로 의존하는 경우, 테스트 케이스를 추가/삭제할때 영향을 받기도 하고요.
무슨 테스트 인지 알기가 어렵습니다.
지금의 경우만 보아도 123이 추가됐는데, answer보면 다른 값은 어디서 온 것들이지? 궁금하게 돼죠.
테스트코드 구현에서도 의존성있는 코드 구현은 좋지 않습니다.
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.
아 확인하였습니다 👍 수정해서 바로 Commit 할게요!
각각 독립된 arrayParser Class로 테스트한다.
저도 처음에 생각했던 부분인데, 정확히 꼬집어주셨네요 ㅠㅠ 감사합니다 👍 수정해서 Push 했습니다. |
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.
테스트코드를 어느정도 이해한 거 같네요.
이정도하고, 다음단계진행하는게 좋겠어요.
문서는 잘 정리해서 readme에 표현해서 포트폴리오로 활용하는 것도 좋겠습니다. 문서 잘 썼어요.
테스트코드는 앞으로 UI개발할때 계속 추가할 것이라
parser에서는 이쯤해도 될 듯 하네요.
수고했어요.
test("정확한 타입갯수를 분석하는지 확인한다", () => { | ||
const testData = {"array":2,"string":0,"null":0,"boolean":0,"object":0,"number":3}; | ||
const testResult = printManager.analyzeTypeData(testData); | ||
const answer = "타입 갯수를 분석하여 결과를 출력합니다\narray: 2\nstring: 0\nnull: 0\nboolean: 0\nobject: 0\nnumber: 3\n"; |
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 code이야기는 아니고,
analyzeTypeData 이 함수가 좀 안좋은구석이 있습니다. 저런 메시지를 포함해서 반환하고 있는데요.
저런 메시지는 변경이 될 소지가 큰데, 이렇게 반환값에 고정값으로 추가한 것이 어색해요.
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.
제 생각에는 analyzeTypeData는 분석한 결과만 반환하면 되는것이지,
출력을 위한 메시지 문자열까지 포함할 필요가 없어보여요.
analyzeTypeData 함수의 반환값을 받아서 필요하면 메시지를 더해서 화면에 출력하면 되는 것이겠죠.
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.
감사합니다. 메서드에는 불필요한 문자열까지 포함시키는쪽이 안좋은것이라는 것을 알았습니다 :)
README 가 항목으로 개편됨에 따라 문서화 내용은 STEP 9 항목에 링크로 대체하였습니다.
저번 피드백을 반영하기 위하여, crong 이 말씀해 주셨던 부분을 인지하고, 테스트코드를 작성하려고 했습니다.
하지만 arrayParser.js 및 objectParser.js 메서드들은 기본적으로 return 을 하지 않는 부분들이 많습니다.
해당 생성자변수의 값을 조작한다거나, result Object (결과로 반환하는 객체) 에 push 하는 작업들이 주를 이룹니다.
테스트 코드를 작성하는 입장에서 보면, 좋지 않은 (=일반적이지 않은) 메소드임을 느끼고 있습니다. 하지만 이외 테스트 할 수 있는 기능들은 lexer, util, printManager 에 구현되어 있습니다.
하지만, 테스트케이스에서 가장 오래 걸렸던 만큼, 언제 사용하는지, 어떻게 사용하는지에 대한 느낌은 가지고 있습니다.
혹시 부족한 부분이 있으면 바로 수정하겠습니다. 감사합니다 !