[3주차] 김영우 미션 제출합니다. #12

seank200 · 2021-04-29T10:02:48Z

페이스북 메신저 컨셉으로 디자인한 메신저입니다. 친구/채팅/설정 크게 3개의 탭으로 구분되어 있으며, 로그인/회원가입 > 친구 목록 및 프로필, 채팅(1:1 및 단체방), 환경설정 (계정 이름, ID, 상태메세지 변경)으로 구성되어 있습니다.

React.Context를 사용하여 많은 Component들이 사용하는 prop을 부모 > 자식1 > 자식2 > 자식3... 이렇게 매번 전달할 필요없이 한번에 불러올 수 있도록 하였습니다.
Context 파일 2개는 구조가 복잡하여 코드 리뷰어들을 위해 주석만 달면 자동으로 변수 및 함수에 대한 설명을 담은 웹사이트를 생성해주는 jsdoc auto API documentation generator tool을 사용하여 추가 documentation을 제공하였습니다. (코드리뷰어들은 제 리포지토리로 이동하여 소스 파일들을 다운받은 다음, Docs/ContactsProvider/index.html, Docs/RoomsProvider/index.html 파일 2개를 실행하시면 이해에 도움이 될 수 있습니다.)
React Custom Hooks를 사용하여 반복되는 작업인 localStorage 저장/불러오기를 state가 변함에 따라 자동으로 실행되도록 하였습니다.
어려웠던 점: JSON 파일 읽기/쓰기는 사용 경험이 없어 localStorage에 JSON을 사용하여 저장하고 불러오는 기능을 사용하였고,

https://react-messenger-13th-seank200.vercel.app

… is not in

…ting

sebastianrcnt

영우님, 미션 진행하느라 정말 많이 수고하셨습니다.

이렇게나 단일 PR에 코드 리뷰를 꼼꼼히 많이 달아본 적은 처음이네요.

작성해주신 코드 꼼꼼하게 읽어보면서 영우님께서 코드의 구조화와 품질에 정말 많은 고민과 시간을 쏟아부었다는 느낌이 들었습니다. 이러한 꼼꼼함과 성실성이 개발자에게는 반드시 필요한 부분이지 않을까 생각합니다. 이런 모습은 저도 정말 배우고 닮고 싶은 부분입니다.

특히나 JSDoc를 직접 코드에 일일히 작성해주신 것도 정말 대단하신 것 같습니다 ㅎㅎ.

제가 이 코드 리뷰를 통해서 가장 제안을 드리고 싶은 점은 바로 체계성과 유연성 사이의 균형이라는 것을 말씀드리고 싶습니다. 체계적이고 이론적으로 아름다운 코드를 작성하느냐, 아니면 좀 더 실용주의적이고 간결한 코드를 작성할 것이냐, 그 사이에서의 균형을 잡는 것이 가장 중요하다고 생각합니다.

이런 부분들만 조금 더 신경써보면 좋을 것 같습니다 ㅎㅎ (상세한 제안점은 리뷰에 자세히 남겨놓았습니다)

일관성 있고 정확한 네이밍
통일성 있는 코드 구조(styled-components vs. plain css)
함수의 길이 짧게 유지하기
불필요한 주석 빼기
따로 markdown으로 숲을 볼 수 있는 documentation 작성해보기

sebastianrcnt · 2021-05-08T17:39:45Z

src/App.js

-import ChatForm from './components/ChatForm';
+import Navbar from './components/Navbar';
+import Signup from './components/Signup';
+import RequireLogin from './components/RequireLogin';


Suggested change

import RequireLogin from './components/RequireLogin';

import LoginRequired from './components/LoginRequired';

Wrapper 컴포넌트는 형용사로 작성하는 경우가 많더라구요~!

sebastianrcnt · 2021-05-08T17:41:07Z

src/App.js

+                <Route path="/room/:id">
+                  <RequireLogin>
+                    <ChatRoom />
+                  </RequireLogin>
+                </Route>
+                <Route path="/rooms">
+                  <RequireLogin>
+                    <ChatRoomList />
+                  </RequireLogin>
+                </Route>
+                <Route path="/friend/:id">
+                  <FriendProfile />
+                </Route>
+                <Route path="/friends">
+                  <RequireLogin>
+                    <FriendList />
+                  </RequireLogin>
+                </Route>
+                <Route path="/settings">
+                  <RequireLogin>
+                    <Settings />
+                  </RequireLogin>
+                </Route>


<RequireLogin>인 route들을 한번에 감싸보는건 어떨까요?

sebastianrcnt · 2021-05-08T17:42:04Z

src/App.js

+                <Route path="*">
+                  <Page404 />
+                </Route>


Suggested change

<Route path="*">

<Page404 />

</Route>

<Route path="/">

<Page404 />

</Route>

요렇게 해도 괜찮을것같아요~!
그리고 <NotFoundPage>로 바꿔도 괜찮을듯?

sebastianrcnt · 2021-05-08T17:48:30Z

src/App.js

+    <ContactsProvider>
+      <RoomsProvider>


contacts, rooms등은 이제 곧 API로 쏘게 되겠죠?
그때부터는 이제 영우님이 하신 Context Provider 작업이 Redux, Apollo GraphQL, useSWR등 다양한 라이브러리들이 기본적으로 제공해준답니다 ㅎㅎ

이렇게 라이브러리를 쓰기 전 직접 손으로 Context, Provider와 State Mangement system을 구현하는 게 정말 좋은 공부인 것 같아요. 외부 라이브러리 독스를 볼때마다 아마 모든 기능 하나하나가 혁신이라고 생각하실 것 같습니다. ㅎㅎ

오...저는 이 부분이 무슨 말인지(코드인지) 몰라서 머릿속에 물음표만 가득했는데 시원님 코멘트 보고 배워갑니다.

sebastianrcnt · 2021-05-08T17:50:43Z

src/components/ChatContainer.js

  // Ref to access chatcontainer (div)
  const chatContainerRef = useRef(null);

  // Scroll div to bottom when new message arrives


정말 딱 필요한 만큼, 필요한 위치에 주석을 적어주셨네요.
별 거 아닌 것 처럼 느껴질 수 있지만 이런 작은 주석들이 가독성을 크게 올려주는 것 같아요~

sebastianrcnt · 2021-05-08T19:05:34Z

src/contexts/ContactsProvider.js

+  /**
+   * React State users. 모든 사용자들을 저장하고 있는 User object 배열
+   * @name users
+   * @type {Array.User}


영우님은 앞으로 TypeScript를 정말 좋아하실거에요. 제가 장담하죠 ㅎㅎ

sebastianrcnt · 2021-05-08T19:06:07Z

src/contexts/ContactsProvider.js

+    initialUsers.push(new User('sean', '김영우', '미션 수행 중'));
+    initialUsers.push(new User('ceos.fe', '프론트', '밤 새는 중'));
+    initialUsers.push(
+      new User('ceos.sinchon', '세오스', '우리 동아리 안힘들어요^^')
+    );
+    initialUsers.push(new User('test', '테스트', '시험용'));


밖으로 빼놓아도 될 것 같은 부분이긴 하네요~!

sebastianrcnt · 2021-05-08T19:12:23Z

src/contexts/ContactsProvider.js

+   * @method getCopy
+   * @returns {User}
+   */
+  getCopy() {


deep copy를 의도하신 것이라면 clone으로 네이밍을 하거나,
getCopy 대신 static 생성자를 만들어보는 것도 좋은 패턴인 것 같습니다. Flutter의 Dart가 이러한 패턴을 사용하고 있습니당.

static fromUser(user: User) { return new User(this.userId, this.userName, this.statusMsg, this.lastActive); }

usage

const user1 = new User(); const user1Clone = User.fromUser(user1);

sebastianrcnt · 2021-05-08T19:14:36Z

src/contexts/ContactsProvider.js

+    getUserById,
+    users,
+    setUsers,
+    createAccount,
+    currentUser,
+    selectUser,
+    deselectUser,
+    userActivity,
+    changeUserId,
+    changeUserName,
+    changeStatusMsg,
+    initializeLocalUsers,


워후.. 굉장하군요...

Hooks에서 이렇게 많은 내용을 내려보내는게 일반적인 것 같지는 않긴 하네요.
이런 방식보다는 User 모델의 구조를 좀 바꿔서 user 모델의 메소드를 이용해서 state를 업데이트 해보는것도 나쁘지 않은 것 같습니다. 다만 백엔드 통신 대신 자체 모델을 이용한 것이니 이렇게 해보는 것도 정말 좋은 경험일 것 같습니다.

프로그래밍을 하다 무엇이든 복잡하고 많다는 느낌이 들면, 계속해서 단순화하고 쉽게 만들려고 해보는 자세가 중요한 것 같아요. 물론 어렵고 복잡하고 난해한 코드와 모듈을 이해하고 작성하는 인내력과 끈기도 필요하지만, 간단하고 효율적이면서 쉬운 코드를 지향하면서 하는 고민들이 정말 큰 밑거름이 되는 것 같습니다.

sebastianrcnt · 2021-05-08T19:15:48Z

src/hooks/useLocalStorage.js

+    if (typeof initialValue === 'function') {
+      return initialValue();
+    } else {
+      return initialValue;
+    }
+  });


이건 어떤 뜻인가요?
initialValue가 function인 경우는 어떤 경우인지 주석으로 달아주시면 또 좋을 것 같습니다.

GeonHeeAhn

우와 영우님 코드 잘 봤습니다! 예전에 완성본을 봤을 때도 참 신경을 많이 썼구나 싶었는데 코드로 보니 더 잘 느껴지네요. 되게 사소한 부분까지도 다 신경을 쓰신것같아요. 코드 리뷰를 하며 와 이런것도 하셨군요!!라고 감탄하고 싶은 부분들이 많았는데 너무 감탄만 하고 가는 것 같아 조금 줄였습니다.ㅎㅎ 코드를 보며 제가 잘 모르는 태그/부분들이 있어서 그런 부분은 시원님이 작성해주신 코멘트 보면서 공부했습니다. 이런 갓벽한 코드를 제가 리뷰하기엔....네 더 덧붙일 말이 없어서요. 여기까지 하겠습니다. 이번 과제도 수고하셨습니다!

GeonHeeAhn · 2021-05-09T02:45:19Z

Docs/ContactsProvider/User.html

+<script> prettyPrint(); </script>
+<script src="scripts/linenumber.js"> </script>
+</body>
+</html>


html에서 바로 기본 틀을 짜준건가요? 중간중간에 있는 넓은 공백은 단순히 가독성을 위한 것인지 궁금합니다.

GeonHeeAhn · 2021-05-09T03:27:29Z

Docs/ContactsProvider/scripts/linenumber.js

+            lines[i].id = lineId;
+            if (lineId === anchorHash) {
+                lines[i].className += ' selected';
+            }


line마다 숫자를 부여한 이유가 다로 있을까요?

GeonHeeAhn · 2021-05-09T03:30:14Z

src/App.js

+    <ContactsProvider>
+      <RoomsProvider>


오...저는 이 부분이 무슨 말인지(코드인지) 몰라서 머릿속에 물음표만 가득했는데 시원님 코멘트 보고 배워갑니다.

GeonHeeAhn · 2021-05-09T03:34:27Z

src/components/ChatForm.js

-        value={ message }
-        onChange={ handleChange }
-        onKeyPress = { handleKeyPress }
+        placeholder={`${currentUser.userName} (으)로 메세지 전송`}


이렇게 설정해둔거 되게 좋네요

GeonHeeAhn · 2021-05-09T03:36:14Z

src/components/ChatMessage.js

+      <SystemMessage>
+        {`${chat.user.userName}님이 ${
+          chat.type === 'enter' ? '대화에 참여했습니다' : ''
+        }${chat.type === 'leave' ? '대화에서 나갔습니다.' : ''}`}
+      </SystemMessage>


이렇게 설정해둔 것도 너무 좋네요ㅎㅎ

GeonHeeAhn · 2021-05-09T03:38:22Z

src/components/ChatRoomList.js

+  const handleCreateChatButtonClick = () => {
+    if (searchQuery !== '' && searchQuery !== null) {
+      const query = searchQuery.split(' ');
+      let roomName = query[0];
+      let participants = [];
+      if (query.length > 1) {
+        // 검색 필드에 방이름 @사용자1 @사용자2.. 이런식으로 입력시 그 사용자가 초대된 상태로 새 방 생성
+        for (let i = 1; i < query.length; i++) {
+          if (query[i][0] === '@') {
+            participants.push(query[i].slice(1));
+          } else {
+            roomName += ' ' + query[i];
+          }
+        }
+        const newRoomId = createRoom(roomName, participants);
+        if (newRoomId) {
+          setSearchQuery('');
+          history.push(`/room/${newRoomId}`);
+        } else {
+          alert('새 채팅방 만들기에 실패하였습니다.');
+        }
+      } else {
+        // 검색 필드에 방이름 만 입력한 경우, 현재 사용자만 추가된 상태로 방 생성
+        const newRoomId = createRoom(searchQuery, [currentUser.userId]);
+        if (newRoomId) {
+          setSearchQuery('');
+          history.push(`/room/${newRoomId}`);
+        } else {
+          alert('새 채팅방 만들기에 실패하였습니다.');
+        }
+      }
+    } else {
+      // 검색 필드가 비어 있으면 새 방 이름을 입력할 수 있도록 prompt한 후 현재 사용자만 추가된 상태로 방 생성
+      let newRoomName = window.prompt('새 방 이름 입력', '');
+      if (newRoomName !== '' && newRoomName !== null) {
+        const newRoomId = createRoom(newRoomName, [currentUser.userId]);
+        if (newRoomId) {
+          history.push(`/room/${newRoomId}`);
+        } else {
+          alert('새 채팅방 만들기에 실패하였습니다.');
+        }
+      }
+    }


저도 새 채팅방 만들기를 구현해보고 싶었는데 막상 하려고 보니 감이 안잡혀서 하지 못했어요. 영우님처럼 하면 되는 거였군요. 배우고 갑니다!

GeonHeeAhn · 2021-05-09T03:41:21Z

src/components/Navbar.js

+  const handleInviteUserClick = () => {
+    let userIds = window.prompt(
+      '초대할 사람의 @아이디 를 공백으로 구분하여 입력하세요',
+      ''
+    );
+    const roomId = selectedRoom.roomId;
+    if (userIds !== '' && userIds !== null) {
+      userIds = userIds.split(' ');
+      userIds = userIds.map((userId) =>
+        userId.slice(userId[0] === '@' ? 1 : 0)
+      );
+      let failedUserIds = [];
+      userIds.forEach((userId) => {
+        if (getUserById(userId)) {
+          enterRoom(userId, roomId);
+        } else {
+          failedUserIds.push(userId);
+        }
+      });
+      if (failedUserIds.length > 0) {
+        let alertString = `${failedUserIds.length.toString()}명의 사용자를 찾지 못해 초대에 실패하였습니다. (`;
+        for (let i = 0; i < failedUserIds.length; i++) {
+          alertString += '@' + failedUserIds[i];
+          if (i < failedUserIds.length - 1) alertString += ', ';
+        }
+        alert(alertString + ')');
+      }
+      userActivity(currentUser.userId);
+    }
+  };


사용자 초대 기능도 되게 색다르고 좋은 것 같아요

seank200 added 15 commits April 14, 2021 00:59

chore: update to package json

2787819

chore: merge mission CEOS-Developers#3

c1d1568

refactor: remove unnecessary code

cbb4714

feat: implement friend profile view page

27227ab

chore: solve no-unused-vars warning in FriendProfile

4b64aca

fix: change error messages to Korean in signup, login

6c3fe23

chore: add initial user data on first run

07e098b

feat: improve room creation feature

402af0f

fix: fix issue of ChatRoomList displaying rooms that the current user…

a3bc8ce

… is not in

fix: fix layout issues in Settings init data button

c03be86

fix: fix no results message in ChatRoomList

eda57fb

chore: apply prettier formatting

f52f6f2

chore: add comments, documentation using jsdoc. apply prettier format…

6c1a329

…ting

feat: implement unreadcount counter in RoomsProvider

263705f

feat: implement unreadcount counter in RoomsProvider

c4013f1

seank200 changed the base branch from main to sean May 1, 2021 01:45

chore: fix git merge conflicts and jsdoc

ba687ae

sebastianrcnt reviewed May 8, 2021

View reviewed changes

seank200 changed the title ~~[4주차] 김영우 미션 제출합니다.~~ [3주차] 김영우 미션 제출합니다. May 8, 2021

GeonHeeAhn reviewed May 9, 2021

View reviewed changes

apply prettier formatting to docs

e61d0a1

	import RequireLogin from './components/RequireLogin';
	import LoginRequired from './components/LoginRequired';

[3주차] 김영우 미션 제출합니다. #12

Are you sure you want to change the base?

[3주차] 김영우 미션 제출합니다. #12

Uh oh!

Conversation

seank200 commented Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebastianrcnt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GeonHeeAhn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

seank200 commented Apr 29, 2021 •

edited

Loading

sebastianrcnt left a comment •

edited

Loading