Intermediate Algorithm Lesson Review

Hello, can someone review my code please, is it to verbose? I want to write readable and efficient code, so looking for some feedback :slight_smile: .

This snippet is for lesson :
Convert HTML Entities


function convertHTML(str) {
  let htmlStr = '';
  const htmlEntities = {
    '&': '&',
    '"': '"',
    '<': '&lt;',
    '>': '&gt;',
    "'": '&apos;'
  };
  for (let char of str) {
    if (htmlEntities[char]) {
      char = htmlEntities[char];
      htmlStr += char;
    } else {
      htmlStr += char;
    }
  }
  return htmlStr;
}

It could be cleaned a little. You’re concatenating to htmlStr regardless of whether the character is a key in your table, so why write it twice?

You can change these things:

  1. htmlStr += char; runs in both if/else cases, so you can put it to the outside.
  2. Because the else will be empty, you can delete it.
  3. The if case can be put in one line.
for (let char of str) {
    if (htmlEntities[char]) char = htmlEntities[char];
    htmlStr += char;
}

Hmm, I thought I was making it more readable, but it seems I was repeating myself (not DRY). Thank you guys for the feedback, I refactored it.

Do not worry about writing efficient code until you have a performance issue, and have run tests to find the bottleneck. Very rarely will the code you think is slow be the bottleneck.

Instead, you should focus on what you said here

This is the most important focus when writing code.

We don’t write code for ourselves. Instead, we write code that others can maintain.

And your code was already readable.

But since you asked, a more readable solution would involve multiple iterations, but let the computer do all the hard work.

Since you know that you’re looking for a very specific string to replace, you can use a simple regex and avoid all misdirection.

function convertHTML(str) {

  return str
          .replace(/&/g, '&amp;')
          .replace(/"/g, '&quot;')
          .replace(/</g, '&lt;')
          .replace(/>/g, '&gt;')
          .replace(/'/g, '&apos;')
}
1 Like

Hehe is nice to have some confirmation when you are learning by yourself, thanks. And I will focus more on readability and not worry about performance so much yet… :slight_smile: