timeconverter_01 #4
Conversation
| } | ||
| } | ||
|
|
||
| cvt(){ |
There was a problem hiding this comment.
이 함수가 어떤 역할을 하는지 이름만 보고도 알 수 있어야합니다.
| isType(){ | ||
| let type = ["d","h","m","s"] | ||
| for(let i=0;i<type.length;i++){ | ||
| if (this.data.split("").includes(type[i])){ | ||
| this.n_data[i] = parseInt(this.data.split(type[i])[0]); | ||
| this.data = this.data.split(type[i])[1]; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
이름에 is 혹은 has 가 들어가는 경우 보통 boolean 값을 리턴한다는 인상을 가지게 됩니다. 그런데 이 함수의 경우에는 어떤 역할을 하는지 이해하기 어렵고 리턴 값이 없어서 다른 사람이 코드를 읽을 때 어려워할 수 있습니다. 더 좋은 이름 그리고 더 좋은 함수로 구현하면 좋을 것 같습니다.
| printData(){ | ||
| let type = ["d","h","m","s"] | ||
| let message = ""; | ||
|
|
||
| for(let i=0;i<this.n_data.length;i++){ | ||
| if(this.n_data[i]!=0){ | ||
| message += this.n_data[i] | ||
| message += type[i]; | ||
| } | ||
| } | ||
| console.log(message); | ||
| } | ||
| } |
| this.ans_type = input.split(" ")[1]; | ||
| this.n_data = [0,0,0,0] // day, hour, minute, second | ||
| this.n_type = [24,60,60,1] // day_toHour, hour_toMinute, minute_toSecond, seocnd_toSecond |
There was a problem hiding this comment.
이러한 변수들의 이름을 다른 사람이 보았을 때 이해하기 어렵습니다. 때문에 이러한 변수를 설명하기 위하여 주석이 생긴 것 같습니다. 더 좋은 변수이름으로 짓는 다면 좋을 것 같습니다. 또 다른 함수명의 경우는 모두 카멜케이스를 사용하고 있는데 이 변수들은 언더스코어를 사용하는 부분이 많이 어색합니다. 혹이 이러한 부분을 처음 미리 정해 놓고하셨다면 계속 이렇게 진행하셔도 괜찮습니다. 다만 걱정하는 부분은 코드가 많아졌을 때 변수명을 짓는 방식이 모두 들쭉날쭉하게 되면 코드 가독성을 해칠 가능성이 있다는 것입니다. 👀
| } | ||
|
|
||
| isError(){ | ||
| // 서버 포멧하고 와서 추가할 예정 |
| if (cnt!=0){ | ||
| for(let i=len-1;i>cnt-1;i--){ | ||
| this.n_data[i-1] += parseInt(this.n_data[i]/this.n_type[i-1]); | ||
| this.n_data[i] = this.n_data[i]%this.n_type[i-1]; | ||
| } | ||
| } |
There was a problem hiding this comment.
이 부분을 더 개선할 수 있을 것 같습니다! 한 번 더 생각해보세요 👀
sprint-2/domain/array.js
Outdated
| Array.prototype.customReduce = function(callback) { | ||
| const array = Object(this); | ||
| const length = array.length; | ||
| let sum=0; | ||
| let value; | ||
| for(let i=0;i<length;i++){ | ||
| value = parseInt(array[i]); | ||
| sum = callback(sum, value); | ||
| } | ||
| return sum; | ||
| }; | ||
|
|
||
| module.exports = Array; |
There was a problem hiding this comment.
reduce는 이니셜벨류 등이 존재하는데, 이 함수는 단순 합을 구하는 함수 처럼 보입니다! MDN의 Reduce함수를 보고 재 구현 해주세요.
sprint-2/domain/commander.js
Outdated
| set(){ | ||
| if(this.command=='count') return this.count(this.array); | ||
| else if(this.command='add') return this.add(this.array); | ||
| else; | ||
| } |
There was a problem hiding this comment.
elseif에서 '=' 연산자가 사용되었는데 '=='로 바뀌어야할 것 같습니다.
There was a problem hiding this comment.
또 필요없는 else가 존재하는 것 같습니다. 그리고 되도록이면 else는 사용하지 않는 것이 좋습니다!
|
|
||
| function test(params){ | ||
| let domain = new Commander(params); | ||
| let util = new Log(domain.command, domain.set()); |
There was a problem hiding this comment.
이 변수의 이름이 부적절해보입니다. 그리고 Log 함수의 경우 어디서든지 쓸 수 있기때문에 이렇게 생성자를 이용하기 보다는 static 메서드를 정의한뒤 Log.print() 이런식으로 사용하는 것이 좋아 보입니다.
| this.employee_one; | ||
| this.employee_two; |
There was a problem hiding this comment.
두 대상 모두 같은 일을 하므로 하나의 class로 빼서 하는 것이 좋아 보입니다.
| this.employee_one = setInterval(()=>this.employee(),2000); | ||
| this.employee_two = setInterval(()=>this.employee(),2000); |
| @@ -0,0 +1,22 @@ | |||
| let syncTime = 0; | |||
There was a problem hiding this comment.
전역변수를 사용하여 구현하였네요, 사실 이런 방법이 좋은 방법이 아니지만 실질적으로 구현은 잘 되있어서 좋은 것 같습니다.
2시 서버 포멧하고 와서 에러 메세지 기능 넣겠습니다. 람쥐