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

다각형의 넓이 구하기 #22

Merged
merged 18 commits into from
Mar 26, 2018
Merged

Conversation

juchanhwang
Copy link

No description provided.

Copy link
Contributor

@crongro crongro left a comment

Choose a reason for hiding this comment

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

코딩컨벤션을 조금 수정해보죠.
리뷰남겼어요.

polygon.js Outdated
@@ -0,0 +1,18 @@
function circle(radius){
Copy link
Contributor

Choose a reason for hiding this comment

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

함수이름은 동사+명사로 무엇을 하는지를 표현하세요.

polygon.js Outdated
}

function square(base, height){
return base * height;
Copy link
Contributor

Choose a reason for hiding this comment

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

변수에 저장한 후, 그 변수값을 반환하는 것이 디버깅하기 좋아요.
변수를 좀더 활용하세요.

polygon.js Outdated
}

function Trapezoid(Tbase, Tuppbase, Theight){
if( Theight == undefined){
Copy link
Contributor

Choose a reason for hiding this comment

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

변수 이름은 대문자로 잘 시작안해요.
함수도요. (나중에 함수이름을 대문자로 시작하는 경우가 있는데 생성자라는 걸 만들때는 그렇게 해요)

polygon.js Outdated
if( Theight == undefined){
function getWidthTrapezoid(base1, uppbase1, height1){
let result3 = (base1 + uppbase1) * height1 * 0.5
if( base1 === undefined || uppbase1 === undefined || height1 === undefined){
Copy link
Contributor

Choose a reason for hiding this comment

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

혹시 getWidthTrapezoid(undefined, undefined, undefined) 라고 호출하면 ?

Copy link
Author

Choose a reason for hiding this comment

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

3개의 인자가 필요하다고 뜨네요오.. 근데 문제 자체가 넓이를 구하는 것이 문제인데, 인자 값에 숫자 말고 다른 값을 넣는 것 자체가 잘못 된게 아닌가요?!!

Copy link
Contributor

Choose a reason for hiding this comment

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

네 하지만, 코드가 오류없이 동작하고, 상황과 맞지 않는 메시지를 뱉게 되는거죠.
자바스크립트에서는 undefined이 어떤 오류라기보다, 하나의 정상적인 type이라고 보거든요.

Copy link
Author

Choose a reason for hiding this comment

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

아! 그렇군요 ㅎㅎ 수정해보겠습니다아아

polygon.js Outdated
function square(base, height){
return base * height;
function getWidthSquare(base, height){
let result2 = base * height;
Copy link
Contributor

Choose a reason for hiding this comment

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

result2 !!!!
지역함수이면 result1과 충돌날 일도 없습니다.
따라서 1,2이렇게 충돌을 걱정하는 것 같은 이름은 하지 마세요.

polygon.js Outdated
}

function Trapezoid(Tbase, Tuppbase, Theight){
if( Theight == undefined){
function getWidthTrapezoid(base1, uppbase1, height1){
Copy link
Contributor

Choose a reason for hiding this comment

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

여기도 base1.. 이런거 안됨.

Copy link
Contributor

@crongro crongro left a comment

Choose a reason for hiding this comment

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

네 여기까지는 일단 좋아요.

Copy link
Contributor

@crongro crongro left a comment

Choose a reason for hiding this comment

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

불필요한 for문을 제거해보죠.

return "2개의 인자값이 필요합니다";
}
let arr = [];
for(let n = 0; n < arguments.length; n++){
Copy link
Contributor

Choose a reason for hiding this comment

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

getWidthSquare함수는 arguments갯수가 2개라고 가정하는 것이죠.
그런데 굳이 for문을 쓸 필요가 없어보여요.
arr에 넣어둬서 아래에서 이렇게 비교도 의미 없고요.
if(arr.length === arguments.length){

이미 함수초반에 2개 인자체크를 끝냈으니, 간단히 인자가 2개니? 그러면 바로 base * height계산.
이렇게 쉽게 생각하고 단순하게 구현할 수 있을 거 같네요.

Copy link
Contributor

@crongro crongro left a comment

Choose a reason for hiding this comment

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

함수가 보기 좋아졌어요
좀더 수정해보죠.

getArea.js Outdated
}
}

function getArea(){
Copy link
Contributor

Choose a reason for hiding this comment

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

표현가능한 매개변수가 있다면 아래처럼 표현하는 게 좋겠어요.
두번째 매개변수 부터는 표현하기 어렵지만, es6에 있는 rest parameter라는 것으로 표현할 수 있어요.

function getArea(polygon, ...args) {

}

getArea.js Outdated

function getWidthCircle(radius){
let functionCheckLogic = checkLogic(radius);
if(functionCheckLogic == true){
Copy link
Contributor

Choose a reason for hiding this comment

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

if문은 true인지 아닌지를 알아서 판단해줍니다.

if(functionCheckLogic) 이렇게 가능.

getArea.js Outdated
return result;
}
else{
return functionCheckLogic;
Copy link
Contributor

Choose a reason for hiding this comment

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

변수이름이 이상해요. functionCheckLogic ??? 좀더 어울리는 이름으로 짓죠. (명사로)

Copy link
Contributor

@crongro crongro left a comment

Choose a reason for hiding this comment

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

함수의 반환값을 조금더 정리해보실래요?

return 타입을 일관되게 반환하는 것이,
더 쓰기 좋은 함수입니다.

getArea.js Outdated
let result = radius * radius * Math.PI;
return result;
}else{
return check;
Copy link
Contributor

Choose a reason for hiding this comment

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

이 함수는 return을 result를 하고 어떤 경우는 check를 하는데요.
check를 하는게 적당한 건가요? 그에 따라서 이함수를 부르는 쪽에서 어떤 처리를 하는게 있나요?

getArea.js Outdated
}


function getArea(polygon,...args){
Copy link
Contributor

Choose a reason for hiding this comment

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

getArea의 if/else 문이 중첩되어 있는데요, 이걸 좀 줄여보세요.
if-else -if -if... 이렇게 되지 않도록!

Copy link
Contributor

@crongro crongro left a comment

Choose a reason for hiding this comment

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

나아지고 있어요~

함수를 좀더 써서 if문을 단순하게 만들어보죠.

getArea.js Outdated
@@ -1,17 +1,17 @@
function checkLogic(){
function getcheckerror(){
Copy link
Contributor

Choose a reason for hiding this comment

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

좋은 시도고 괜찮아요. 에러처리를 이녀석에게 위임한거니까요.
다만, 인자를 받아서 처리하는 것이 아니고, 그냥 특정한상황만 (인자가 있는가, 숫자인가 정도) 처리하는 코드라서 아쉽네요.
아마 꼼꼼하게처리하려면 이녀석이 비대해질 거 같긴하고요.

아무튼 함수를 분리한 건 좋습니다.

getArea.js Outdated
return getWidthSquare(base, height);
}

if(polygon === 'circle'){
Copy link
Contributor

Choose a reason for hiding this comment

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

이곳에 보면 if/else if/else if 로 여러가지 도형을 확인해서 함수를 부르자나요?
이걸 아주 짧은 코드로 동작되게해보세요.

예를들어서,

else if(polygon ==='rect') return getWidthSquare(base, height);
else if(polygon ==='xxxx') return getWidthxxxxx(base, height);

이런식으로요.
즉 if문 안에 내용을 가능한 최소한으로 줄여보는 겁니다.

Copy link
Contributor

@crongro crongro left a comment

Choose a reason for hiding this comment

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

이부분도 함수로 분리해서 if문을 간단하게 만들어보죠.

if(polygon === 'circle'){
     let arr = [];
     for(let radius = args[0]; radius <= args[1]; radius ++){
       arr.push(getWidthCircle(radius));
     }
     if(typeof args[1] !== "number"){
       return getWidthCircle(args[0]);
     }
     return arr;
   }

Copy link
Contributor

@crongro crongro left a comment

Choose a reason for hiding this comment

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

잘하고 있어요.

circle 부분만 더 줄여보세요!

if(check !== true){
return check;
}
if(polygon === 'circle'){
Copy link
Contributor

Choose a reason for hiding this comment

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

이곳 circle 조건문 안의 내용도 줄일 수 있을까요? 아래 rect처럼요.

Copy link
Contributor

@crongro crongro left a comment

Choose a reason for hiding this comment

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

오 함수분리~~ 잘했네요.

그런데 아래 코드가 동작하나요?
console.log(getArea('circle',1));

그리고 소숫점 자리수를 2자리까지만 노출하도록 구현해보면좋겠어요.

Copy link
Contributor

@crongro crongro left a comment

Choose a reason for hiding this comment

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

늘 반환값을 변수에 담아둘 필요는 없지만, 디버깅을 쉽게 하는 방법이라 좋은 방법이기도해요.
당분간은 return 뒤에 너무 긴 내용은 변수에 담아두고 사용하세요.

@crongro crongro merged commit ae813a8 into code-squad:juchan96 Mar 26, 2018
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