Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: small simplification #1657

Open
wants to merge 1 commit into
base: gh-pages
Choose a base branch
from
Open

Conversation

Keuklar
Copy link

@Keuklar Keuklar commented Apr 29, 2024

Description
small refactoring

Purpose
code factorisation / simplification / concision

@alvestrand
Copy link
Contributor

The purpose of samples is to have code that is easy to understand.
While the suggestion uses many more Javascript constructs, I have trouble seeing how it is easier to understand than the old code.

@Keuklar
Copy link
Author

Keuklar commented Apr 29, 2024

ok I see, sorry for that, the main goal was to factor this code :

if (readyState === 'open') {
    dataChannelSend.disabled = false;
    dataChannelSend.focus();
    sendButton.disabled = false;
    closeButton.disabled = false;
  } else {
    dataChannelSend.disabled = true;
    sendButton.disabled = true;
    closeButton.disabled = true;
  }

which is a bit verbose, repetitive.
But I got a little carried away, I could just focus on the 'if...else' if you wish.
Something like that:

  const closed = readyState !== 'open'
  dataChannelSend.disabled = closed;
  sendButton.disabled = closed;
  closeButton.disabled = closed;
  if (!closed) {
    dataChannelSend.focus();
  }

@alvestrand
Copy link
Contributor

That seems more readable, yes. (I would call the variable "isOpen" just because that's what the ready state is named. But that's taste.

@Keuklar
Copy link
Author

Keuklar commented Apr 30, 2024

So, I've made a new proposal, I let closed const, as you said it's a matter of taste, since I find it more straightforward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants