2
\$\begingroup\$

The scenario is the following. I get the string from an HTML element via outerHTML and I need to convert the 'rgb(r, g, b)' part into the hexadecimal representation. I tried this with regex and it seems to work but I have a feeling there are better ways to deal with this.

One particular thing I don't like is where I directly access color[1] from my regex match. I'm open to any feedback.

private cleanupRichText(richText: string) {    
    const rx = /rgb\((.+?)\)/ig
    const color = rx.exec(richText)
    if (color !== null) {
        const hexparts = color[1].split(',').map(str => parseInt(str.trim(), 10).toString(16))
        let hexString = '#'
        hexparts.forEach(element => {
        hexString += element.length === 1 ? '0' + element : element })
        text = text.replace(/rgb\(.+?\)/ig, hexString)
    }
}

To give an example input:

'rgb(234, 112, 30)'

Expected output:

'#ea701e'
\$\endgroup\$
3
\$\begingroup\$

While the given function does work for simple cases, it will not perform as expected if there are multiple colors within richText. The first color will override all other colors.

First, a look at your implementation:

  1. Why is richText used in some places and the (global?) text in others? It looks like these are expected to be equivalent, so you should just use richText since it is explicitly passed in.

  2. Instead of mutating the global text, you should return the replaced result. This makes testing the function easier and debugging much less painful.

  3. Why is this a class function? It seems to have well defined inputs & outputs that does not depend on the state of any object.

  4. Instead of manually padding numbers with element.length === 1 ? '0' + element : element, you should use element.padStart(2, '0') if possible. The polyfill isn't that large if it isn't supported everywhere you need it yet (IE), and it is considerably easier to read.

  5. parseInt ignores leading whitespace, so there's no need to do str.trim().

  6. String.prototype.replace accepts a replace function, which will be called with each match. Use this to extract the rgb values instead of doing re.exec yourself. In the vast majority of cases, you should not need re.exec.

  7. Should text inside the element really be converted? If you are manipulating HTML as text, you probably shouldn't be. If this rgb() text is in the style element of the element, you could be using element.getAttribute('style') instead.

  8. What should happen if I wrote rgb(1, 2, 3, 4, 5, 'asdf')?

Here is how I would write this function, keeping your assumption that all rgb strings are well formed.

function cleanupRichText(richText) {
  return richText.replace(/rgb\((.+?)\)/ig, (_, rgb) => {
    return '#' + rgb.split(',')
      .map(str => parseInt(str, 10).toString(16).padStart(2, '0'))
      .join('')
  })
}

const tests = [
  ['rgb(234, 112, 30)', '#ea701e'],
  ['rgb(0, 0, 0)', '#000000'],
  ['RGB(255, 255,255)', '#ffffff'],
  ['rgb(0, 0, 0) rgb(170, 170, 170)', '#000000 #aaaaaa']
]

for (const [text, expected] of tests) {
  const result = cleanupRichText(text)
  console.log(result === expected ? 'PASS' : 'FAIL', text, '->', result)
}

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for your feedback and suggestions. The reason why I'm doing this is that I have a richtext editor in html and I have to pass the richtext string to my application in order to render a representation of this richtext. Since the richtext parser only accepts hex strings I have to do this conversion. And yes, for the time being I assume that I only get well formed rgb strings. \$\endgroup\$ – Philipp Nov 19 '18 at 9:52

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.