Skip to content

Conversation

@PParkJy
Copy link

@PParkJy PParkJy commented Jan 16, 2020

pull request 에러가 계속 나길래 해결 방법을 찾다가 저장소를 지우고 새로 만들어버렸습니다.
그래서 이전 commit 내용들이 다 날아가버렸습니다....껄껄....
저처럼 멍청하게 하지마세요 흑흑

@PParkJy PParkJy changed the title [tracy] "react-calculator" 구현 완료입니다. [tracy] react-calculator 구현 완료입니다. Jan 16, 2020
@leaf-upper
Copy link

고생하셨습니다!!

@Tetramad
Copy link

으앗.. gitignore파일 지정해 주세요:sob: 모듈 파일때문에 코드 쓰신게 하나도 안떠요

@PParkJy
Copy link
Author

PParkJy commented Jan 16, 2020

gitignore 추가했습니다!

Copy link

@Tetramad Tetramad left a comment

Choose a reason for hiding this comment

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

굿:+1:

Comment on lines +3 to +4
const preOperand = parseFloat(state.displayValue || "0")
const backOperand = parseFloat(state.nextValue || (operator === "/" || operator === '*' ? "1": "0"))

Choose a reason for hiding this comment

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

preOperand, backOperand라는 변수명보다 lhs, rhs 또는 lhsOperand, rhsOperand를 사용하는 것을 추천합니다. left-hand side, right-hand side의 약자입니다.
state.displayValue || "0"null 처리하는 것은 좋아요! 👍

Comment on lines +2 to +8
if (next) {
if (next.includes(".")) {
return {};
}
return { nextValue: next + "." };
}
return { nextValue: "0." };

Choose a reason for hiding this comment

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

들여쓰기 간격 맞춰주세요!

@HodaeSsi
Copy link
Member

고생하셨습니다!

Copy link
Contributor

@Parkhyunseo Parkhyunseo left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!! 전체적으로 코드가 이전에 비해 많이 깔끔해졌습니다. 그러나 아직 null과 빈 객체를 많이 사용하고 명확하지 않은 코드들이 보이네요. 그리고 React 컴포넌트를 조금 더 활용했으면 좋겠습니다!

@@ -0,0 +1,11 @@
function decimalPoint(next){
Copy link
Contributor

Choose a reason for hiding this comment

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

함수명은 Naming Convention에 의거하여 동사를 사용해야 합니다.

@@ -0,0 +1,11 @@
function decimalPoint(next){
if (next) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if(next)가 의미하는게 명확하지 않습니다.
next === NaN, next === undefined 이런식으로 정확히 명시해주세요.

const operator = state.operatorValue
const preOperand = parseFloat(state.displayValue || "0")
const backOperand = parseFloat(state.nextValue || (operator === "/" || operator === '*' ? "1": "0"))
if (operator === "+") {
Copy link
Contributor

Choose a reason for hiding this comment

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

여유가 된다면 string연산자와 대응하는 함수를 객체로 묶어서 개발해 보는걸 추천합니다.

return (preOperand/backOperand).toString();
}
}
console.log('error');
Copy link
Contributor

Choose a reason for hiding this comment

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

이번 요구사항에 console 사용은 없습니다! 지워주세요.

render(){
return(
<Fragment>
<button name="C" onClick={event => this.props.click(event.target.name)}>C</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

버튼도 모두 React Component로 만들어보는 걸 추천합니다.

import DisplayLayout from "./displayLayout"
import ButtonLayout from "./buttonLayout"

class reactCalculator extends Component{
Copy link
Contributor

Choose a reason for hiding this comment

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

클래스명은 대문자로 시작합니다

@@ -0,0 +1,12 @@
import React, {Component } from 'react';

class displayLayout extends Component{
Copy link
Contributor

Choose a reason for hiding this comment

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

클래스명은 대문자로 시작합니다.

return {};
}

if (operator) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Indent가 맞지 않네요

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.

5 participants