Conversation
cross-spawn 7.0.0 - 7.0.4 Severity: high Regular Expression Denial of Service (ReDoS) in cross-spawn - GHSA-3xgq-45jj-v275 fix available via `npm audit fix` node_modules/cross-spawn
- 값이 비어있을 경우 에러 - 정규표현식을 통하여 입력된 값에 대한 규칙 검증 - 값을 배열로 만들어서 각 자동차 이름들이 5글자가 넘어가지 않는지 검증
jinnyjiinlee
left a comment
There was a problem hiding this comment.
보람님~ 일주일 동안 작성하신다고 수고하셨어요!
유효성 검사를 완전 세분화하여 파일을 나눠신 것이 인상적입니다!
개인적인 피드백 남겼습니다 :)
밑에 코멘트도 남겨놓았는데, double quotes 사용은 최대한 피하는게 좋을 것 같아요~!
우테코에서 지시한 요구사항을 잘 따르는 것을 중요하게 생각하더라고요!
이번 일주일은 로또 미션인데 화이팅이예요!
| const ERROR_MESSAGES = { | ||
| VALIDATE_EMPTY: "[ERROR] 입력값이 없습니다. 다시 입력해주세요.", | ||
| VALIDATE_NOT_CORRECT_REGEX: "[ERROR] 형식이 올바르지 않습니다.", | ||
| VALIDATE_FIVE_LENGTH: "[ERROR] 입력된 자동차 이름 중에서 5글자가 넘는 이름이 존재합니다.", | ||
| VALIDATE_LIMIT_NUMBER: "[ERROR] 너무 과도한 시도 횟수를 입력하셨습니다.", | ||
| }; | ||
|
|
||
| export default ERROR_MESSAGES; No newline at end of file |
There was a problem hiding this comment.
'[ERROR]' 문자가 반복되니 이 부분도 상수처리로 하면 어떨까요~?
예를 들어 ERROR_PREFIX: '[ERROR]' 로 하였을 때 개인적으로는 가독성이 좋아지더라고요!
| async racing() { | ||
| const carNamesObject = await this.getCarNamesObject(); | ||
| const tryNumber = await this.getTryNumber(); | ||
| const carRacingResult = this.startCarRacing(carNamesObject, tryNumber); |
There was a problem hiding this comment.
tryNumber네이밍이 경기 횟수 입력한 부분을 의미하는 것 같은데, 명확하지 않은 것 같아요~!
경기횟수라는 의미가 들어가는 네이밍으로 작성하시면 더 좋을 것 같습니다!
There was a problem hiding this comment.
엇 저같은 경우는 레이싱을 시도하는 횟수라고 생각해서 tryCount라는 네이밍을 사용한 기억이 있네요 ,,, ㅋㅋㅋㅋ
| } | ||
|
|
||
| async getCarNamesObject() { | ||
| const carNames = await this.readCarNames(); |
There was a problem hiding this comment.
개인적으로 고민이 되는 부분은,
입력 받은 값을 함수로 네이밍을 지을 때
read / get ~ input 중에서 어떤게 더 직관적인지 고민이 되더라고요 ㅎㅎ
보람님 생각도 궁금합니다 :) !!
There was a problem hiding this comment.
참고로 저는 get ~ input을 템플릿 처럼 쓰고 있는데, 이게 글자수도 길어져서 read 로 작성할지..? 고민이 되어요 ㅎㅎ
There was a problem hiding this comment.
잘 모르는 상태로 보통 뒤죽박죽 써왔지만,
지금까지의 저의 생각은요.
사용자로부터 뭔가 입력을 받을 때에는 input을 쓰려고 생각했었구요.
무언가 데이터를 가공해서 얻어내는 것은 get을 썼었습니다.
(원래 read는 잘 사용하지 않았다가 여러 메서드나 함수가 많아지면서 네이밍이 비슷해지는 것을 막기 위해서 read가 나왔던 것 같습니다.)
| const stringType = String(carNames); | ||
| const validatedCarNames = this.validationCarNames(stringType); | ||
| const carNamesArray = makeArrayFromString(validatedCarNames); | ||
| MissionUtils.Console.print(""); |
There was a problem hiding this comment.
import할 때, Console 자체를 import를 하면 어떨까요~?
그렇게 하면 매번 Console.print 앞에 MissionUtils. 적을 필요가 없으니 더 간결해 보일 것 같아요~!
There was a problem hiding this comment.
편의점 과제 구현할 때에 throw new Error() 로 종료시키지 않고 에러를 보내서 테스트 코드를 통과하려고 Console.print();로 해서 통과가 안되었는데, 다른 방법을 찾아보다가 MissionUtils.Console.print();로 테스트 통과가 되어서 다른 것으로 알고 있었습니다.
무슨 차이인지는 좀 더 알아보겠습니다.
There was a problem hiding this comment.
오호... 저도 항상 Console을 import해서 사용하던 터라 무슨 차이인지 궁금하네요 👀
There was a problem hiding this comment.
편의점 구현 때 무조건 테스트코드를 먼저 통과시키려고 노력할 때였습니다.
throw를 쓰면 로직이 끝나버리고, try{}catch{}도 안쓰고 무조건 테스트 조건에 맞추려고 하다가 Console.log, Console.print, 사용했는데 테스트통과가 안되다가 MissionUtils.Console.print 사용하고 테스트 통과가 되었던 적이 있었습니다.
if (item.quantity > product.quantity) {
// throw new Error(
// "[ERROR] 재고 수량을 초과하여 구매할 수 없습니다. 다시 입력해 주세요."
// );
MissionUtils.Console.print(
'[ERROR] 재고 수량을 초과하여 구매할 수 없습니다. 다시 입력해 주세요.'
);
}원리는 어떻게 알아내야할지 잘 모르겠습니다 ㅠ
테스트 코드 찾아보니 아래와 같이 되어있기는 한데요.
test("예외 테스트", async () => {
await runExceptions({
inputs: ["[컵라면-12]", "N", "N"],
inputsToTerminate: INPUTS_TO_TERMINATE,
expectedErrorMessage:
"[ERROR] 재고 수량을 초과하여 구매할 수 없습니다. 다시 입력해 주세요.",
});
});npm package module을 뜯어보는 방법이나 어떤 역할을 하는지 확인해보는 방법을 열심히 찾아봤는데요.
https://github.com/woowacourse-projects/javascript-mission-utils/blob/main/src/index.js
package 출처에서 확인해본 결과
MissionUtils.Console.print과 Console.print가 하는 역할에는 전혀 차이가 없는 것을 확인했습니다.
제가 테스트코드를 모르지만, 편의점 테스트코드를 다시 확인해보니 MissionUtils만 import가 되어있어서 MissionUtils.Console.print로 출력되는 것만 인식했던 것이 아닐까? 추측해봅니다...
import { MissionUtils } from "@woowacourse/mission-utils";결론적으로 문제가 없으셨다면, 역할에 전혀 차이가 없어보이니 간결하게 Console.print로 쓰면 될 것 같습니다!
| @@ -0,0 +1,11 @@ | |||
| import ERROR_MESSAGES from "../../constants/errorMessages.js"; | |||
There was a problem hiding this comment.
우테코에서 airbnb Javascript 컨벤션을 따르라고 권장을 하였는데, airbnb 컨벤션에서는 쌍따옴표를 권장하지 않더라고요!
single quotes로 작성하면 좋을 것 같습니다~!
참고로 prettier에 설정하면 자동으로 변환해주어서 편하게 저는 이용하고 있어요~!
| const validateFiveLength = (value) => { | ||
| const carNamesArray = makeArrayFromString(value); | ||
| carNamesArray.forEach((carName) => { | ||
| if (carName.length > 5) { |
There was a problem hiding this comment.
저도 자꾸 놓치는 부분인데, 5가 매직넘버가 될 수 있으니 상수처리해주면 좋을 것 같아요~
예를 들어 MIN_CARNAME_LENGTH와 같이 하면 어떨까요 ~?
| const inputCarNames = await MissionUtils.Console.readLineAsync( | ||
| "경주할 자동차 이름을 입력하세요. (이름은 쉼표(,) 기준으로 구분) \n => " |
There was a problem hiding this comment.
에러 메시지를 상수화 한 것 처럼, input 메시지도 상수처리해서 관리하면 어떨까요~? 유지보수 측면에서 더 좋을 것 같아서 피드백 드립니다 :) !!
rosielsh
left a comment
There was a problem hiding this comment.
제가 궁금한 부분 위주로 코멘트 남겨봤습니다 !
제안한 부분은 생각해보고 반영하면 더 좋은 코드가 될 수 있지 않을까 생각합니다.
그리고 정규식을 적극적으로 활용하신게 인상깊었습니다 :)
이번주차도 수고하셨어요 👍
| async run() {} | ||
| constructor () { | ||
| this.racingController = new RacingController | ||
| } |
There was a problem hiding this comment.
오 여기 RaceController뒤에 ()가 빠진것 같은데, 정상적으로 동작하는지 궁금하군요...? 👀
There was a problem hiding this comment.
정확한 원리는 잘 모르겠지만, 이 부분 제가 직접 해봤었는데 괄호가 없어도 작동이 됐었습니다!
아마도 괄호에 매개변수나 들어갈게 없다보니 되지 않았을까? 추측합니다.
기본적인 prettier만 적용해도 자동고침(괄호적용) 되는 부분입니다.
| VALIDATE_EMPTY: "[ERROR] 입력값이 없습니다. 다시 입력해주세요.", | ||
| VALIDATE_NOT_CORRECT_REGEX: "[ERROR] 형식이 올바르지 않습니다.", | ||
| VALIDATE_FIVE_LENGTH: "[ERROR] 입력된 자동차 이름 중에서 5글자가 넘는 이름이 존재합니다.", | ||
| VALIDATE_LIMIT_NUMBER: "[ERROR] 너무 과도한 시도 횟수를 입력하셨습니다.", |
There was a problem hiding this comment.
지인님께도 드렸던 피드백인데
ERROR_PREFIX와 텍스트를 인자로 에러 텍스트를 생성하는 로직을 재사용하는 방법도 있으니 참고하면 좋을 것 같아요!
ex)
const createMsg = (msg) => `${ERROR_PREFIX} ${msg}\n`;| async racing() { | ||
| const carNamesObject = await this.getCarNamesObject(); | ||
| const tryNumber = await this.getTryNumber(); | ||
| const carRacingResult = this.startCarRacing(carNamesObject, tryNumber); |
There was a problem hiding this comment.
엇 저같은 경우는 레이싱을 시도하는 횟수라고 생각해서 tryCount라는 네이밍을 사용한 기억이 있네요 ,,, ㅋㅋㅋㅋ
| const stringType = String(inputCarNames); | ||
| const removeSpace = stringType.replace(/ /g, ""); | ||
| return removeSpace; |
There was a problem hiding this comment.
stringType, removeSpace라는 네이밍보다 조금 더 변수의 의미를 드러내는 stringCar, removeSpaceCar이라는 네이밍은 어떨까요?
| let racingResult = carNamesObject; | ||
| for (let i = 0; i < tryNumber; i++) { | ||
| racingResult = racingResult.map((object) => { | ||
| if (this.getRandomNumber() > 4) object.forward += 1; |
There was a problem hiding this comment.
자동차는 랜덤 값이 4이상일 경우 전진한다.
4도 포함하도록 수정해야할 것 같습니다 :)
| if(numberValue > 20) | ||
| throw new Error(ERROR_MESSAGES.VALIDATE_LIMIT_NUMBER); | ||
|
|
There was a problem hiding this comment.
과도한 시도횟수도 validation 하자는 생각이었습니다.
|
|
||
| const validateCarNames = (validateTarget) => { | ||
| validateEmpty(validateTarget); | ||
| const regExPattern = /^(\w+)(,\w+)*$/; |
There was a problem hiding this comment.
정규식을 굉장히 잘사용하시는군요,,,!!
제가 개인적으로 정규식을 어려워해서 덕분에 좋은 패턴 알아갑니다!
| const stringType = String(carNames); | ||
| const validatedCarNames = this.validationCarNames(stringType); | ||
| const carNamesArray = makeArrayFromString(validatedCarNames); | ||
| MissionUtils.Console.print(""); |
There was a problem hiding this comment.
오호... 저도 항상 Console을 import해서 사용하던 터라 무슨 차이인지 궁금하네요 👀
kimyou1102
left a comment
There was a problem hiding this comment.
이번주차도 고생많으셨습니다!! 저는 gpt를 통해 정규식을 배워서 아직 많이 어려운데 정규식을 표현들 배워갑니다👍
그리고 3주차 피드백을 토대로 한다면 UI로직을 분리하기 위해 inputView, outputView를 만들어서 사용하면 더 좋을 거 같아요🙂
| for (let i = 0; i < number; i++) { | ||
| dash += "-"; | ||
| } | ||
| return dash; |
There was a problem hiding this comment.
자바스크립트 repeat 메서드를 활용해도 좋을 거 같아요!
return '-'.repeat(number);
| return MissionUtils.Random.pickNumberInRange(0, 9); | ||
| } | ||
|
|
||
| changeDashUtil(number) { |
There was a problem hiding this comment.
number보다는 진행 정도를 나타내게끔 forwardCount 같은 변수도 좋을 거 같아요!
| const inputCarNames = await MissionUtils.Console.readLineAsync( | ||
| "시도할 횟수는 몇 회인가요? \n => " | ||
| ); | ||
| const removeSpace = inputCarNames.replace(/ /g, ""); |
There was a problem hiding this comment.
문자열의 양 옆의 공백을 제거하는 용이라면 trim 메서드를 활용해도 좋을 거 같아요!
| .filter((object) => object.forward === maxForwardNumber) | ||
| .map((object) => object.name); | ||
| const winner = winnerArray.join(", "); | ||
| return MissionUtils.Console.print(`최종우승자 : ${winner}`); |
There was a problem hiding this comment.
functions내의 파일들의 함수들을 하나의 파일에서 관리해도 좋을 거 같아요. 공통적으로 사용되는 validation을 한파일에서 관리하면 더 편할 거 같아요!
There was a problem hiding this comment.
맞습니다 ㅠ 여기서 시간이 오래걸렸었고, 비효율적이네요~
validation 만드는 방법을 바꾸는 중입니다!
| return removeSpace; | ||
| } | ||
|
|
||
| validationTryNumber(validationTarget) { |
There was a problem hiding this comment.
validateTryNumber 함수를 바로 사용해도 되는 상황처럼 보이는데 클래스 내부 메서드로 한번 더 감싼 이유가 있을까요??
There was a problem hiding this comment.
네 validation 함수로 만들어놓았고, 그냥 그대로 가져다 쓰기만 하면되는데, 제가 왜 메서드로 한번 더 감싸게 되었는지 모르겠네요.. 별 이유가 있는 것은 아니였습니다 ㅎ
|
|
||
| const validateTryNumber = (validationTarget) => { | ||
| validateEmpty(validationTarget); | ||
| const regExPattern = /^\d{1,2}$/; |
xxziiko
left a comment
There was a problem hiding this comment.
안녕하세요 보람님!
개인적으로 정규식을 잘 활용하시는 것 같아서 저도 많이 배우고 갑니다!
테스트 코드도 작성하면서 진행 하면 조금 더 효율적으로 오류를 줄일 수 있어서 이부분도 고려를 해보시면 좋을 것 같아요!
이번 주 스터디도 화이팅입니다 :)
| printGameProgress(object) { | ||
| object.map((array) => { | ||
| const carName = array.name; | ||
| const dash = this.changeDashUtil(array.forward); | ||
| return MissionUtils.Console.print(`${carName} : ${dash}`); | ||
| }); | ||
| MissionUtils.Console.print(""); | ||
| } |
There was a problem hiding this comment.
매개변수명을 object라고 하신 특별한 이유가 있으실까요? 배열을 map 하는 것 같은데 네이밍 때문에 혼란이 오는 것 같습니다..!
그리고 return 값이 필요하지 않다면 forEach가 조금 더 의도에 맞을 것 같습니당
There was a problem hiding this comment.
-
매개변수명을 Object라고 한 이유는 일반 배열이 아닌 객체배열이라서 저렇게 썼던 것이었습니다. (그냥 저의 인식일 뿐이었습니다.)
object와 array를 섞어서 사용하니 혼란이 야기될 수 있는 것일까요??? 가르침을 주시면 감사하겠습니다 ㅎ -
map과 forEach 조금 더 공부해서 적절한 적용을 해볼 수 있도록 하겠습니다. 알려주셔서 감사합니다!!!
| const maxForwardNumber = Math.max( | ||
| ...carRacingResult.map((object) => object.forward) | ||
| ); |
There was a problem hiding this comment.
스프레드 연산자를 사용할 때는 ...변수명 이런식으로 하는게 조금 더 가독성에 좋다고 생각되는데, 이부분에 대해서는 어떻게 생각하시는지 궁금해요!
해당 함수가 정확히 어떤 요소를 return 하는지 네이밍함으로서 배열의 요소가 더 명확해 질 수 있을거라고 생각합니다!
const forwardSteps = carRacingResult.map((car) => car.forward)
const maxForwardNumber = Math.max(...forwardSteps);There was a problem hiding this comment.
너무 좋은 코드인데요?! 저는 제 머릿속으로 생각해내지 못한거라서요~ 배우고 가겠습니다
| async run() {} | ||
| constructor () { | ||
| this.racingController = new RacingController | ||
| } |
신경썼던 부분
어려웠던 부분
개인적인 회고
예기치 못한 평일 일정으로 인하여 많이 부족한 것들을 공부하지 않은 점 반성합니다.