0
\$\begingroup\$

I really do not like how bad this looks. It is not readable and I am not sure if I need the stacked map's. There are a lot of iterations going on here (map, join, replace) and performance is really important, please help :(

Input:

const alternatives = [
  "flex",
  "float",
  [
    "background-size",
    "background-image"
  ]
]

Expected Output:

'Consider using 'flex', 'float' or 'background-size' with 'background-image' instead.'

Working Code:

const result = `Consider using ${alternatives
                 .map((alternative) => {
                   return Array.isArray(alternative)
                     ? alternative.map((a) => `'${a}'`).join(' with ')
                     : `'${alternative}'`;
                 })
                 .join(', ')
                 .replace(/, ([^,]*)$/, ' or $1')} instead.`
\$\endgroup\$
  • 1
    \$\begingroup\$ I would use the Oxford comma in this situation since English doesn't define the operator precedence between or and with. \$\endgroup\$ – Roland Illig Apr 20 at 5:07
  • \$\begingroup\$ Why is performance so important for this particular piece of code? This is not the test whether to generate a diagnostic (which I agree should be really fast), it's the part generating the diagnostic (which shouldn't be called that often anyway). Did you measure which part of the code really needs the performance boost? \$\endgroup\$ – Roland Illig Apr 20 at 5:12
  • \$\begingroup\$ Are a and alternative always strings? If so, you can omit the quotes and the inner map. \$\endgroup\$ – Roland Illig Apr 20 at 5:14
1
\$\begingroup\$

Remove the last item

You are not going to avoid the stepping over each item at least once. However the replace and be avoided by removing the last item.

One iteration

Use a for loop, building the string as you step over each item. Before the loop pop the last item, then after the loop push it back to keep the array intact. You can then add the last item after the conjunction.

I will use options rather than alternatives as in the bottom example it is more semantically fitting.

function toHumanReadable(options) {
    const toStr = opt => `"${Array.isArray(opt) ? `${opt[0]}" with "${opt[1]}` : opt}"`;
    const last = options.pop(); 
    var res = 'Consider using ';
    for (const opt of options) { res += toStr(opt) + ', ' }
    options.push(last);
    return res + `or ${toStr(last)} instead.`; 
}
const alt1 = [ "flex", "float" , ["background-size", "background-image" ] ];
const readable = toHumanReadable(alt1);

or you can make options an argument array, and don't need to push the last item back onto the array. It does mean an extra iteration.

function toHumanReadable(...options) {
    const toStr = opt => `"${Array.isArray(opt) ? `${opt[0]}" with "${opt[1]}` : opt}"`;
    const last = options.pop(); 
    var res = 'Consider using ';
    for (const opt of options) { res += toStr(opt) + ', ' }
    return res + `or ${toStr(last)} instead.`; 
}

const alt1 = [ "flex", "float" , ["background-size", "background-image" ] ];
const readable = toHumanReadable(...alt1);

Reusable

I am guessing this is just one of a many similar human readable strings. To make the function reusable you can add some arguments

The next version requires two iterations, but does not need the replace. This time the last item is popped from a copy of the options array and the conjunction is added via joining the joined options and the last option.

function toHumanReadable(options, open, close, conjunction) {
    options = options.map(opt => Array.isArray(opt) ? opt.join('" with "') : opt);
    const last = options.pop();
    return `${open} "${[options.join('", "'), last].join(`", ${conjunction} "`)}" ${close}`;
}

const alt1 = [ "flex", "float" , ["background-size", "background-image" ] ];
const readable = toHumanReadable(alt1, "Consider using", "instead.", "or");

Example usage

setTimeout(()=> { // just for snippet to put relevant code at top

  const alt1 = [ "flex", "float" , ["background-size", "background-image" ] ];
  const alt2 = [ "foo", ["bar","poo"], ["min-width", "max-height" ] ];
  log(toHumanReadable(alt1, sentenceTypes.anyOf));
  log(toHumanReadable(alt2, sentenceTypes.oneOf));
  log(toHumanReadable(alt1, sentenceTypes.allOf));   
},0);

function toHumanReadable(options, {open, close, conjunction}) {
    options = options.map(opt => Array.isArray(opt) ? opt.join('" with "') : opt);
    const last = options.pop();
    return `${open} "${[options.join('", "'), last].join(`", ${conjunction} "`)}" ${close}`;
}

const sentenceTypes = {
   create(name, open, close, conjunction) { this[name] = {open, close, conjunction} }
};
sentenceTypes.create("anyOf", "Consider using", "instead.", "or");
sentenceTypes.create("oneOf", "Use any one of", "to stay consistent.", "or");
sentenceTypes.create("allOf", "All the following", "are required.", "and");
function log(t){txt.appendChild(Object.assign(document.createElement("div"),{textContent:t}))}
body {
font-family: arial;
font-size: small;
}
<div id="txt"></div>

\$\endgroup\$

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.