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

Practice study #4

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

Conversation

jinnyjiinlee
Copy link

No description provided.

Copy link

@dlsxjzld dlsxjzld 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 +56 to +65
const onlyNumberInArray = [];
for (let i = 5; i < this.inputString.length; i += 2) {
onlyNumberInArray.push(this.inputString[i]);
}

onlyNumberInArray.forEach((number) => {
if (Number.isNaN(Number(number))) {
throw new Error(ERROR_MESSAGES.WRONG_CUSTOM_INPUT);
}
});

Choose a reason for hiding this comment

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

이 부분이 위에 validateNormalCase()와 로직이 겹치는 부분이 많아서 다음과 같이 재사용 할 수도 있을 것 같아요! 🤔
this.inputString에서 앞에 5글자를 지우고, 이 값을 validateNormalCase에 넘기는 형식으로 해도 될 것 같아요! 👍

Comment on lines +15 to +16
this.inputString[1] === DELIMITER.COMMA ||
this.inputString[1] === DELIMITER.COLON;

Choose a reason for hiding this comment

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

현재 코드를 보면 1,2;3 이 통과가 안 되어야 하는데 통과가 될 것 같아요! 🤔
구분자를 1개만 확인하는 게 아니라 뒤에 모두 확인하는 방식은 어떨까요??

Choose a reason for hiding this comment

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

상수 파일들을 에러 메시지와 일반 메시지까지 꼼꼼하게 잘 나누신 것 같아요 👍
너무 잘 나눠서 가독성이 좋은 것 같습니다 👍

import { OUTPUT_MESSAGES } from '../Constant/messages.js';

export class OutputHandler {
async printResultOfSum(resultAddedNumbers) {

Choose a reason for hiding this comment

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

printResultOfSum 함수는 비동기를 처리하지 않아서 async가 안 붙어도 될 것 같아요! ㅎㅎ

async printResultOfSum(resultAddedNumbers) {
const { RESULT } = OUTPUT_MESSAGES;

return Console.print(`${RESULT} : ${resultAddedNumbers}`);

Choose a reason for hiding this comment

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

Console.print()는 단순히 console.log(message)라 반환하는 값이 없어서 return 문을 빼도 괜찮을 것 같아요! ㅎㅎ 👍
console.log() 리퍼런스도 남깁니다!

Comment on lines +23 to +26
this.inputString[0] === DELIMITER_MAKER.STRING_FIRST &&
this.inputString[1] === DELIMITER_MAKER.STRING_SECOND &&
this.inputString[3] === DELIMITER_MAKER.STRING_THIRD &&
this.inputString[4] === DELIMITER_MAKER.STRING_FOURTH;

Choose a reason for hiding this comment

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

구분자가 1자리가 아니거나 위치가 무작위일 경우에는
indexOflastIndexOf 혹은 정규표현식을 활용해도 좋을 것 같아요 ㅎㅎ 👍

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.

2 participants