[김희선]w3_리뷰요청_server의 model,controller구현 #66
Conversation
bellaah
commented
Nov 21, 2019
•
bellaah
commented
- server와 client의 socket연결 구현했습니다.
- socket이 연결되면 server의 controller에서 random한 캐릭터 이미지와 랜덤한 x,y 좌표를 만들어서 클라이언트에게 넘겨줍니다. 그럼 client는 그 data를 바탕으로 canvas에 캐릭터를 그립니다.
- 또한 server쪽에서 model을 만들어 객체 지향 설계를 바탕으로 구현해봤습니다.
- 궁금한 부분은 주석으로 남겼습니다.
- 다른 부분도 개선될 부분이 있다면 남겨주시면 좋을 것 같습니다!
|
|
||
| class SocketContainer { | ||
| constructor() { | ||
| this.socket = undefined; |
There was a problem hiding this comment.
this.socket; 이렇게 할당하면 자동으로 undefined이 할당 될꺼에요!~
| class SocketContainer { | ||
| constructor() { | ||
| this.socket = undefined; | ||
| this.connect(); //this.socket = socketio.connect('http://localhost:3000');이 들어가는 것보다 이렇게 함수를 호출하는 것이 좋을까요? |
There was a problem hiding this comment.
제가 생각했을때에는 함수로 사용하는 이유는 재사용이 가장 큰 이유라 생각해요! 만약 한번만 사용하게 되면 굳이 함수로 빼지 않아도 된다고 생각해요!
| onEnterRoom(callback) { // on을 해주는 부분을 socket파일에 넣고 싶어서 callback을 받아서 실행하는데 이 구조는 괜찮나요? | ||
| this.socket.on(EVENT.ENTER_ROOM, (data) => callback(data)); | ||
| } |
There was a problem hiding this comment.
이렇게 Wrapping해서 많이 사용해요! 👍
콜백을 받아서 사용하고 싶다면 좋은 방식이에요! 호출하는 부분에서 socket객체도 사용할수 있어 좋은 방법이라 생각해요!
There was a problem hiding this comment.
이렇게 사용하려면 callback 파라메터가 함수인지 확인하는 방어 코드가 있으면 좋겠네요!
underscore, lodash를 사용하면 쉽게 사용할수 있을꺼에요!
https://lodash.com/
There was a problem hiding this comment.
네 그건 찾아봐서 적용하겠습니다. 감사합니다!
| } | ||
|
|
||
| emitStartGame() { | ||
| this.socket.emit('start_game'); // 보통 socket event name은 '_'를 많이 쓰는지 궁금합니다! |
There was a problem hiding this comment.
일반적으로 white space를 사용한다는데... 이 방법이 맞는지 모르겠네요! 나중에 알게되면 어떻게 사용하는지 멘션 달아주면 좋겠어요!
socketio/socket.io#1571
|
|
||
| app.use('/', indexRouter); | ||
|
|
||
|
|
| const otherCharacters = this.rooms[0].getExistCharacters(); //방은 하나라고 가정하고 rooms[0]번째 방을 이용합니다. | ||
| const character = await this.rooms[0].enterNewPlayer(); |
There was a problem hiding this comment.
this.rooms[0]는 변수로 할당하면 좋겠네요~
const firstRoom = this.rooms[0];
There was a problem hiding this comment.
이건 test로 쓴거고 rooms라는 배열에 많은 방들이 할당될 거라서 이건 로직에 맞게 이번주에 수정하겠습니다!
|
|
||
| bindPlayerEvents(socket) { | ||
| socket.on('start_game', () => { | ||
| // this.startRoomRound(0); // 문제 하나만 넘겨주는 logic |
| // this.startRoomRound(0); // 문제 하나만 넘겨주는 logic | ||
|
|
||
| this.players.forEach(async (player) => { // 문제 10개를 배열로 넘겨주는 logic | ||
| player.emit('get_quiz_list', await quizFinder.getQuizList()); |
There was a problem hiding this comment.
보통 get과 같은 이름은 객체에서 프로퍼티를 가져오는 등 가벼운 작업에서 많이 사용되는 네이밍 룰이에요! 데이터베이스에서 가져오는 거라면 fetch라는 네이밍을 사용하는게 더욱 명확한 이름이라 생각돼요!
There was a problem hiding this comment.
아 그렇군요. 네 알겠습니다 감사합니다!
| @@ -0,0 +1,8 @@ | |||
| import pool from './connection'; | |||
|
|
|||
| const getDataFromDB = async (sql) => { //이런식으로 db에서 pool.query하는 부분을 util로 빼서 사용하려고 합니다. | |||
There was a problem hiding this comment.
좋은 방식이라 생각해요! 👍 wrapping 함수를 만들어 더욱 편하게 사용하는 것은 지향하는 방법이에요! 만약 query를 하고 싶다면 네이밍은 query로 하는게 좋을것 같아요! 그리고 ORM을 참고해서 데이터베이스 유틸을 만들어보는것도 추천해요~
There was a problem hiding this comment.
네. 네이밍은 수정하고 ORM참고해서 자주 쓰일만한 것들은 util에 추가해볼게요!
| } | ||
|
|
||
| hasNoMoreQuiz() { | ||
| return this.quizs[this.currentRound] === undefined; |
There was a problem hiding this comment.
lodash, underscore등을 사용하면 명시적으로 비교할수 있을꺼에요! 예를들어 isUndefined와 같은 함수도 존재 해요~
There was a problem hiding this comment.
감사합니다. 코드리뷰로 알게 된 부분이 많은 것 같아요. 말씀하신대로 사용할만한 라이브러리도 살펴보고 더 좋은 방향내주신 부분은 수정해보려고 합니다. 늦은 시간에 코드리뷰해주셔서 감사합니다!
