Rate my JS code

Code Feedback
1

I’m doing a simple program that speack the thing you write in a input, it works, but, there is a simple or more elegant way to do the JavaScript code?

Btw, any opinion about the name “robot-voice”? i think i need to change it, for a better one.
Thanks in advance!

Full proyect here, CodePen:
https://codepen.io/ricardonothing/pen/yLaMvad

const closeBtn = document.querySelector('.close-btn'); 
const alertHelp = document.querySelector('.alert');

document.querySelector("button").addEventListener("click", e=>{
  let voice = '';
  const voiceList = document.querySelector('select').value;
  switch (voiceList) {
    case 'UK Famale':
      voice = undefined;
      break;
    case 'UK Male':
      voice = 'UK English Male';
      break;
    case 'US Famale':
      // Output => Famale voice
      voice = 'US English Male'
      break
    case 'Latin Famale':
      // Output => Famale voice
      voice = 'Spanish Latin American Male'
      break;
    case 'Spanish Famale':
      // Output => Famale voice
      voice = 'Spanish Male'
      break;
  }

  responsiveVoice.speak(document.querySelector("input").value, voice);	
});

// Help | Alert
document.querySelector('.help').onclick = () => alertHelp.style.display='block';

closeBtn.onclick = () => closeBtn.parentElement.style.display='none';

HTML:

<div class="container">
  <h1>Robot-Voice</h1>
  
  <form>
    <select>
      <option value="UK Famale">UK English Famale</option>
      <option value="UK Male">UK English Male</option>
      <option value="US Famale">US English Famale</option>
      <option value="Latin Famale">Spanish Latin American Famale</option>
      <option value="Spanish Famale">Spanish Famale</option>
    </select>
  </form>
  
  <input type="text" placeholder="Write something!" value="Write something!"/>
  
  <button>Speak!</button>
</div>

<div class="alert">
  <span class="close-btn">&times;</span>
  <p class="alert__p">
    - Instructions: <br>
    1) Select a lenguage and voice. <br>
    2) Write something in the field. <br>
    3) Press the button. <br>
    4) Listen to the voice.
  </p>     
</div>

<span class="help">?</span>

<script src="https://code.responsivevoice.org/responsivevoice.js?key=auvTMQpf"></script>
2

It looks kind of cool If I’m going to be picky…

You misspelled “female”.

Your comments in the switch statement are incorrect. They’re also unnecessary - the code is clear enough that they aren’t needed.

<input type="text" placeholder="Write something!" value="Write something!"/>

You shouldn’t set the value prop here, just the placeholder - that defeats the purpose of the placeholder.

  <p class="alert__p">
    - Instructions: <br>
    1) Select a lenguage and voice. <br>
    2) Write something in the field. <br>
    3) Press the button. <br>
    4) Listen to the voice.
  </p>

This should be an ordered list.

Your switch statement should have a default. Why not have let voice be undefined when it is created and then your default could just be fore the UK Female. It would eliminate a little code. Actually I probably would have wrapped that switch logic in its own function so it could just return that value instead of setting a variable and breaking - but that’s more a stylistic choice than a criticism. Or actually, a dictionary would work well there too. But again, more stylistic.

... .addEventListener("click", e=>{

Why are you passing in e? I’d rather just have empty parentheses there, otherwise I assume that the function is doing something with that variable.

But all in all, it looks pretty good. I can’t find much to complain about.

1 Like
3

Sorry, I see why you did the comments now. I just might have made them inline and more succinct, like // female, but that’s a stylistic choice.

1 Like
4

This is breaking the switch into a helper function:

const getVoice = voiceList => {
  switch (voiceList) {
    case 'UK Male':
      return 'UK English Male';
    case 'US Famale':
      return 'US English Male';
    case 'Latin Famale':
      return 'Spanish Latin American Male';
    case 'Spanish Famale':
      return 'Spanish Male';
    case 'UK Famale':
    default:
      return undefined;
  }
}

document.querySelector("button").addEventListener("click", () => {
  const voiceList = document.querySelector('select').value;
  let voice = getVoice(voiceList);
  responsiveVoice.speak(document.querySelector("input").value, voice);	
});

This would be a dictionary solution:

**const voices = {
  'UK Famale': undefined,
  'UK Male': 'UK English Male',
  'US Famale': 'US English Male',
  'Latin Famale': 'Spanish Latin American Male',
  'Spanish Famale': 'Spanish Male',
}

document.querySelector("button").addEventListener("click", () => {
  const voiceList = document.querySelector('select').value;
  let voice = voices[voiceList];
  responsiveVoice.speak(document.querySelector("input").value, voice);	
});

I think they’re a little cleaner, but again, this is stylistic, not an actual JS problem.

1 Like
5

I would suggest you check out the FormData interface. If you just change the option values to the voice values you need it would be a lot easier.

<form>
  <select name="voice">
    <option value="UK English Female">UK English Female</option>
    <option value="UK English Male">UK English Male</option>
    <option value="US English Female">US English Female</option>
    <option value="Spanish Latin American Female">Spanish Latin American Female</option>
    <option value="Spanish Female">Spanish Female</option>
  </select>
</form>

const form = document.querySelector("form");

document.querySelector("button").addEventListener("click", () => {
  const formData = new FormData(form);
  const voice = formData.get("voice");
  responsiveVoice.speak(document.querySelector("input").value, voice);
});
2 Likes
6

Thanks a lot for time your time and give me feedback! Your solution are definitely more elegant (personal opinion) and that’s what I was looking for!
Thanks again! I will be careful with the english words, using correct tags and finding ways to write less and cleaner code.

7

Your solution is amazing! I didn’t know this way to do it, is a lot better than my Switch/Case. I’m going to learn more about FormData interface. Thanks!

1 Like