riad-binary / webrtc_p2p_basic_app

0 stars 0 forks source link

Code review #1

Open sandabu opened 2 years ago

sandabu commented 2 years ago

const is better here because this variable is not reassigned and redefined and we often use () => {} (arrow-functions) on arguments. https://github.com/riad-binary/webrtc_p2p_basic_app/blob/c80bc879e49e28c448c86e93c65f8a077c037e73/index.js#L7

sandabu commented 2 years ago

I think it's general to omit px and muted property is boolean type so you can also omit the value.

<video id="user-video" width="500" height="500" muted></video>

https://github.com/riad-binary/webrtc_p2p_basic_app/blob/c80bc879e49e28c448c86e93c65f8a077c037e73/public/index.html#L17

sandabu commented 2 years ago

These variables also should be declared with const. https://github.com/riad-binary/webrtc_p2p_basic_app/blob/c80bc879e49e28c448c86e93c65f8a077c037e73/public/chat.js#L1-L7

https://github.com/riad-binary/webrtc_p2p_basic_app/blob/c80bc879e49e28c448c86e93c65f8a077c037e73/public/chat.js#L13-L16

I prefer to declare DOM selector variable appended with prefix $ like jQuery.

const $lobby = document.getElementById("video-chat-lobby");

It's easier to understand this variable is DOM.

sandabu commented 2 years ago

Good validation.

I think it's better to check also empty string(" ") here. https://github.com/riad-binary/webrtc_p2p_basic_app/blob/c80bc879e49e28c448c86e93c65f8a077c037e73/public/chat.js#L30-L31

sandabu commented 2 years ago

We often name it pc https://github.com/riad-binary/webrtc_p2p_basic_app/blob/c80bc879e49e28c448c86e93c65f8a077c037e73/public/chat.js#L10

sandabu commented 2 years ago

There is no logic when creator is false so you can use Return Early Pattern

https://medium.com/swlh/return-early-pattern-3d18a41bba8

if(!creator) {
  return;
}

https://github.com/riad-binary/webrtc_p2p_basic_app/blob/c80bc879e49e28c448c86e93c65f8a077c037e73/public/chat.js#L146

sandabu commented 2 years ago

RTCPeerConnectrion.setLocalDescription(offer) returns Promise. It's better to complete setting local description before emitting.

 rtcPeerConnection 
   .createOffer() 
   .then((offer) => { 
     return rtcPeerConnection.setLocalDescription(offer); 
   })
   .then(() => {
     socket.emit("offer", offer, roomName);
   }) 

And async/await way can make it simple.


  try{
    const offer = await rtcPeerConnection.createOffer();
    await rtcPeerConnection.setLocalDescription(offer);
    socket.emit("offer", offer, roomName);
  }catch(e){
    console.error(e);
  }

https://github.com/riad-binary/webrtc_p2p_basic_app/blob/c80bc879e49e28c448c86e93c65f8a077c037e73/public/chat.js#L154-L163

sandabu commented 2 years ago

You did a good job!

On starting new web project next time, please install a linter such as eslint via npm.

https://eslint.org/