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'
200_success
145k22 gold badges190 silver badges478 bronze badges
asked Nov 15, 2018 at 12:55
\$\endgroup\$

1 Answer 1

4
\$\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)
}

answered Nov 16, 2018 at 23:28
\$\endgroup\$
1
  • \$\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\$ Commented Nov 19, 2018 at 9:52

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.