1
\$\begingroup\$

I'm trying to make a simple and straightforward example for WebRTC without a signaling server. The code works (whole HTML file below), but it feels like I'm overshadowing something in the logic that I can't pinpoint. I need help simplifying this code for easier readability. There are little limitations in regards to legacy.

<!DOCTYPE html>
<meta charset="utf-8">
<title>miniWebRTC</title>

<div id="createRoom">
  <h3>Create or join a room?</h3>
  <button id="createBtn">BOB: Create</button>
  <button id="joinBtn">ALICE: Join</button>
</div>

<div id="bobPrompt">
  <h3>BOB: Send your local offer to ALICE</h3>
  <input id="localOffer">
  <h3>Then, paste the "answer" you received</h3>
  <input id="remoteAnswer">
  <br>
  <br>
  <button id="answerRecdBtn">Okay, I pasted it.</button>
</div>

<div id="alicePrompt">
  <h3>ALICE: Paste the "offer" you received</h3>
  <input id="remoteOffer">
  <br>
  <br>
  <button id="offerRecdBtn">Okay, I pasted it.</button>
  <h3>Then, send your local answer to BOB</h3>
  <input id="localAnswer">
</div>

<div id="chatPrompt">
  <h1>Chat</h1>
  <br>
  <div id="chatlog" style="height:200px; overflow:auto; border:1px solid"></div>
  <br>
  <input type="text" id="messageTextBox" placeholder="Type your message here">
  <button onclick="sendMessage()">Send message</button>
</div>

<script>
  // @ts-check
  const config = { iceServers: [{ urls: 'stun:stun.l.google.com:19302' }] };

  // The local browser's RTCPeerConnection
  const peerConnection = new RTCPeerConnection(config)

  /** @type {RTCDataChannel | null} data channel */
  let activeDataChannel = null;

  function sendMessage() {
    if (messageTextBox.value) {
      activeDataChannel.send(JSON.stringify({ message: messageTextBox.value }));
      chatlog.innerHTML += `<span>ME: ${messageTextBox.value}</span><br/>`;
      messageTextBox.value = "";
    }
  }
  
  /**
   * Shows one of the 4 div elements in the webpage, and hides the rest.
   * 
   * @param {part} part The part to show.
   */
  function showPart(part) {
    const divList = [createRoom, bobPrompt, alicePrompt, chatPrompt]

    // Hide all elements that are not part
    divList.forEach(div => div.style.display = (part == div ? "block" : "none"));
  }

  showPart(createRoom);

  // BOB: create room
  createBtn.addEventListener("click", async () => {

    showPart(bobPrompt);

    const dataChannel = peerConnection.createDataChannel('chat');
    
    activeDataChannel = dataChannel;

    peerConnection.setLocalDescription(await peerConnection.createOffer())

    // Once all ice candidates are collected, set the value
    peerConnection.addEventListener("icecandidate", ({ candidate }) => {
      if (candidate == null) {
        localOffer.value = JSON.stringify(peerConnection.localDescription);
      }
    })

    // BOB: pasted Alice's answer
    answerRecdBtn.addEventListener("click", async () => {
      const answer = remoteAnswer.value;
      const answerDesc = new RTCSessionDescription(JSON.parse(answer))
      await peerConnection.setRemoteDescription(answerDesc);
    }, { once: true });

    // ALICE sent a message to BOB
    dataChannel.addEventListener("message", e => {
      const data = JSON.parse(e.data)
      chatlog.innerHTML += `<span>THEM: ${data.message}</span><br/>`;
      chatlog.scrollTop = chatlog.scrollHeight
    })

    peerConnection.addEventListener("connectionstatechange", e => {
      if (peerConnection.connectionState === "connected") {
        // Connected!
        showPart(chatPrompt);
      }
    })
  }, { once: true });

  // ALICE: listen to room
  joinBtn.addEventListener("click", () => {

    showPart(alicePrompt);

    // ALICE: pasted Bob's answer
    offerRecdBtn.addEventListener("click", async () => {
      const offer = remoteOffer.value;
      const offerDesc = new RTCSessionDescription(JSON.parse(offer))
      await peerConnection.setRemoteDescription(offerDesc)
      await peerConnection.setLocalDescription(await peerConnection.createAnswer())
      peerConnection.addEventListener("icecandidate", e => {
        if (e.candidate == null) {
          localAnswer.value = JSON.stringify(peerConnection.localDescription);
        }
      })
    }, { once: true });
    
    peerConnection.addEventListener("datachannel", ({ channel }) => {
      // Connected!
      activeDataChannel = channel;
      showPart(chatPrompt);
      activeDataChannel.addEventListener("message", e => {
        const data = JSON.parse(e.data)
        chatlog.innerHTML += `<span>THEM: ${data.message}</span><br/>`;
        chatlog.scrollTop = chatlog.scrollHeight
      })
    })
  }, { once: true });
</script>
\$\endgroup\$
3
  • 2
    \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$
    – BCdotWEB
    Jun 1 at 9:20
  • 1
    \$\begingroup\$ Moving the inline functions out of the main body of the script might help. The main body will then more clearly show the components in play and which events are important, and the functions can then be named so its clear what the intention of the function is. \$\endgroup\$ Jun 1 at 12:55
  • 1
    \$\begingroup\$ @DaveMeehan Please write all suggestions for improvements as answers. Posting advice as comments prevents us from voting and commenting on them properly. \$\endgroup\$ Jun 1 at 16:48

1 Answer 1

2
\$\begingroup\$

It might be a lot clearer if you move the inline functions out of the main body. This allow the main body to concentrate on how the parts are used, gives the parts distinct and useful names, and will likely be more readible in the future when your working model has degraded (or fresh eyes come to it).

I like to think of it in terms of a 'conductor' of an 'orchestra'. First define the players and their instruments, and the guide them through the process.

Instead of:

createBtn.addEventListener("click", async () => {

    showPart(bobPrompt);

    const dataChannel = peerConnection.createDataChannel('chat');
    
    activeDataChannel = dataChannel;

    // ...
}, { once: true })

try:

function onCreateButtonClick() {
    // ...
}

function onJoinButtonClick() {
    // ...
}

// main body (IIFE to indicate discreet work)
(() => {

    createBtn.addEventListener('click', onCreateButtonClick, { once: true })
    joinBtn.addEventListener('click', onJoinButtonClick, { once: true })

})()

Obviously I'm paraphrasing in the example, but this approach can be used for all of the inline functions used as event listener callbacks.

I'd also recommend being more explicit with method names such as sendMessage and showPart so that its clear from the naming what its doing. Having removed clutter from the main body, its easier to use verbosity to aid understanding without it adding more 'noise'.

I used an IIFE to indicate what was 'main', there are several alternatives:

function main() {
    // ...
}
main()

Or:

(function main() {
    // ...
})()

The purpose of encapsulating, rather than just having more declarations in the at script level scope is that it shows that its a discreet piece of work. The IIFE signature is quote common so becomes easy to spot without having to be pedantic about naming.

Whilst your application is quite simple, you're realising that as things get more complex there is a need to add modularity to help clarify what things are doing. It would be worth reading up on the Single Responsibility Principle to help inform just how much any discrete piece of code should be doing.

As the application becomes more complex, you'll likely want to start using discreet source modules. This will then give you a new challenge, as at present you are relying on globals/module scope to allow access to your peer connection instance. You might well need to think about how to inject dependent variables into those helper functions, for example, passing dependencies as parameters, something like:

function onCreateButtonClick(peerConnection) {
    // ...
}

(() => {

    const peerConnection = new RTCPeerConnection(/*config*/)

    createBtn.addEventListener(
        'click', 
        () => onCreateButtonClick(peerConnection), 
        { once: true }
    )
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.