5
\$\begingroup\$

I'm required to perform a slight conversion on a string which contains html.

The conversion should transform a elements such that they become span elements. The conversion should also place the href attribute from the original a element into the span.

I have what I think is a working solution, but the code feels overcomplicated. I had hoped this may be possible with relatively little code, so I was wondering if anyone could suggest ways to make this code neater.

Browser compatibility is not an issue, as this will be transpiled by babel.

Here's what I have:

const testHtml = `
  <p>This is a test paragraph</p>
  <a style="font-weight: bold;" href="http://www.google.co.uk">Google is here</a>
  <a style="font-weight: bold;" href="http://www.msn.co.uk">Msn is here</a>
  <h2>This is a heading</h2>
`

const splitByLinkEnds = html => {
  const split = html.split('</a>')
  const allButLast = split.slice(0, -1)
  const last = split.slice(-1)[0]

  return [
    ...allButLast.map(x => `${x}</a>`),
    last
  ]
}

const splitByLinks = html => {
  const split = html.split('<a');
  const [
    first,
    ...result
  ] = split

  const withReaddedOpeningTags = [
    first,
    ...result.map(x => `<a${x}`)
  ]

  return withReaddedOpeningTags
    .reduce((prev, curr) => [
      ...prev,
      curr.indexOf('<a') > -1 ?
        splitByLinkEnds(curr) : curr
    ], []).flat()
}

const transformLinkToTextOnly = linkHtml => {
  const linkHref = linkHtml
    .match('href="[^"]*"')[0]
    .replace('href="', '')
    .replace('"', '')

  const htmlWithSpan = linkHtml
    .replace('<a', '<span')
    .replace('</a>', '')

  return `${htmlWithSpan} (${linkHref})</span>`
}

const transformLinksToTextOnly = html =>
  splitByLinks(html)
    .map(x => x.indexOf('<a') > - 1 ?
      transformLinkToTextOnly(x) : x
    ).join('')

const result = transformLinksToTextOnly(testHtml)

document.body.innerHTML += testHtml
document.body.innerHTML += '<hr />'
document.body.innerHTML += result


Edit I should note that the solution ought to work in node, ideally. I appreciate that the fact I include dom manipulation in the question is quite misleading, I included this more to attempt to create an easy working example that to demonstrate how the code might actually be used.

A further note, the performance of this code is not particularly important. It's only for relatively small amounts of html and gets run once when the application loads.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for this great question - I hope you get some good reviews, and I hope to see more of your contributions here in future! \$\endgroup\$ – Toby Speight Dec 13 '18 at 13:03
4
\$\begingroup\$

One thing to consider is the whole working with HTML as string. In the answer I'll assume you work within the web-browser. The web-browser can parse the HTML for you, this way you'll only have to work with nodes.

Here is an example:

const testHtml = `
  <p>This is a test paragraph</p>
  <a style="font-weight: bold;" href="http://www.google.co.uk">Google is here</a>
  <a style="font-weight: bold;" href="http://www.msn.co.uk">Msn is here</a>
  <h2>This is a heading</h2>
`;

function transformLinksToTextOnly(htmlString) {
  const root     = document.createElement('root');
  root.innerHTML = htmlString;
  const anchors  = root.querySelectorAll('a');

  anchors.forEach(anchor => {
    const span = document.createElement('span');

    span.innerHTML = anchor.innerHTML;
    if (anchor.href) 
      span.innerHTML += ` (${anchor.href})`;

    anchor
      .getAttributeNames()
      .forEach(attrName => {
        const attrValue = anchor.getAttribute(attrName);
        span.setAttribute(attrName, attrValue);
      });

    anchor.parentNode.replaceChild(span, anchor);
  });

  return root.innerHTML;
}

const result = transformLinksToTextOnly(testHtml);

document.body.innerHTML += testHtml;
document.body.innerHTML += '<hr />';
document.body.innerHTML += result;

\$\endgroup\$
  • \$\begingroup\$ Thanks for this; I was actually thinking of using this in node, which I should have probably mentioned in the question, but it may be that I can take a similar approach using a library \$\endgroup\$ – OliverRadini Dec 13 '18 at 14:58
0
\$\begingroup\$

Highly inefficient

The code is highly inefficient, wasting CPU cycles and memory due to poor use of Javascript API's. It is also badly formatted and thus hard to read and follow and poor naming also made working out what you were doing difficult.

Code Points.

  • Better naming alround. Example, you have two functions transformLinkToTextOnly and transformLinksToTextOnly Now I might be a bit blind but without spaces its not at all clear that these are two separate functions. If you consider the context of the functions and encapsulate them you can reduce the complex names to transformLink and transformLinks making the plural stand out, the rest is inferred due to context.
  • Indent chained functions.
  • Use semicolons to end lines. There are many arguments for and against using semicolons. Semicolons are required by the language, and are inserted automatically when javascript is parsed, however there are many edge cases that most people are unaware of and can catch you out. The rule of thumb is "Can you list all edge cases where semicolon insertion can cause problems?" if not use them. It is even more important that you are consistent. Even if you know all the rules, using semicolons in some places and not in others breaks the style rule "Be consistent".
  • Avoid single use variable declarations.
  • If you use terneries in multi expression lines, put them inside (...) for clarity. See example A
  • Know your javascript. It is important that you are thoroughly familiar with the language you write in. You need to regularly review the JS reference and know the most concise and performant forms to achieve what you desire. Your code is very GC (garbage collection) unfriendly creating many arrays and thrashing memory and slowuing down the code, all of which can be avoided . See example B

Example A

  return withReaddedOpeningTags
    .reduce((prev, curr) => [
        ...prev,
      curr.indexOf('<a') > -1 ?
            splitByLinkEnds(curr) : curr
    ], []).flat()

Better syntax written as

return withReaddedOpeningTags
    .reduce((prev, curr) =>
        [...prev, (curr.indexOf('<a') > -1 ? splitByLinkEnds(curr) : curr)]
        , [])
    .flat();

As I am at this function I will point out that for every iteration you make a new copy of the array, add it as another array, and in the end you are forced to flatten the result to get what you want.

The same can be done with.

return withReaddedOpeningTags
    .map(item =>  item.indexOf('<a') > -1 ? splitByLinkEnds(item) : item);

or

return withReaddedOpeningTags
    .map(item => item.includes('<a') ? splitByLinkEnds(item) : item);

Example B

An example function splitByLinkEnds create 5 arrays

const splitByLinkEnds = html => {
    const split = html.split('</a>');       // array 1

    const allButLast = split.slice(0, -1);  // array 2
    const last = split.slice(-1)[0];        // creates array 3 an uses first item

    return [
        ...allButLast.map(x => `${x}</a>`),  // map creates an array 4 that is then 
                                             // copied to the return array 5
        last 
    ];
}    

It can be done using only one new array.

const splitEnds = markup => {
    const parts = markup.split("</a>");
    const end = parts.pop();
    parts.forEach((x, i) => parts[i] = x + "</a>");
    parts.push(end);
    return parts;
}

or as

const splitEnds = markup => {
    return markup.split("</a>").forEach((x, i, a) => a[i] = x + (i < a.length - 1 ? "</a>" : ""));
}

Rewrites

I have include 4 ways of achieving the same result. Javascript is very expressive so there is no single right way to do anything.

Rewrite using only strings and arrays

This method does not require the DOM and splits the markup at anchor boundaries creating an array of anchor descriptions which is then used to replace the anchors in the original markup.

function replaceAnchors(markup) {
    function findLinks(markup) {
        return markup.substring(markup.indexOf("<a") + 2).split("<a")
            .map(markup => ({
                markup : "<a" + markup.split("</a>")[0] + "</a>",
                anchor : markup.split(">")[0],
                href : markup.split("href=\"")[1].split("\"")[0],
                text : markup.split(">")[1].split("</a")[0],
            }));
    }        
    findLinks(markup).forEach(link => {
        markup = markup.replace(link.markup,`<span${link.anchor}>${link.text}(${link.href})</span>`);
    });
    return markup;
}

Second version does away with the intermediate array and does it in one pass.

function replaceAnchors(markup) {
    markup.substring(markup.indexOf("<a") + 2).split("<a")
        .forEach(a => {
            const href = a.split("href=\"")[1].split("\"")[0];
            const text = a.split(">")[1].split("</a")[0];

            markup = markup.replace(
                `<a${a.split("</a>")[0]}</a>`,
                `<span${a.split(">")[0]}>${text}(${href})</span>`
                );
        });
    return markup;
}

Rewrite with memory and performance in mind.

This uses named properties to determine what to do with each character in turn. If a match[current] is found then actions[current].add and actions[current].action are performed, each action sets up the next expected match and actions by setting the string current to the correct name. It is very fast and has a little room to go even faster. It is also very memory efficient

function replaceAnchors(markup) {
    const match = {
        get anchorStart() { return markup[idx] === "<" && markup[idx+1] === "a" },
        get hrefStart() { return markup.substring(idx,idx + 6) === "href=\"" },
        get hrefEnd() { return markup[idx] === "\"" },
        get tagClose() { return markup[idx]  === "<" && markup[idx+1]  === "/" && markup[idx+2]  === "a" },
    };
    const actions = {
        anchorStart: {
            add() { idx += 2; return "<span" },
            action() { current = "hrefStart" }
        },
        hrefStart: {
            add() { idx += 6; return "href=\""}, 
            action() { hrefStart = idx; current = "hrefEnd" }
        },
        hrefEnd: {
            add() { idx += 1; return "\""}, 
            action() { href = markup.substring(hrefStart, idx -1); current = "tagClose" }
        },
        tagClose: {
            add() { idx += 4; return "(" + href + ")</span>" },
            action() { current = "anchorStart" }
        },
    }
    var result = "", idx = 0, hrefStart, href, current = "anchorStart";
    while (idx < markup.length) {
        if (match[current]) {
            result += searchs[current].add();
            searchs[current].action();
        } else {
            result += markup[idx++]; 
        }
    }
    return result;
}

Rewrite using the DOM

This method uses the Markup parser to help locate anchor tags and then just replaces the start and end tag with span and the additional href

function replaceAnchors(markup) {
    const nodes = document.createElement("span");
    nodes.innerHTML = markup;
    for (const a of nodes.querySelectorAll("a")) {
        const anchor = a.outerHTML;
        markup = markup.replace(anchor, anchor
            .replace("</a>",`(${a.getAttribute("href")})</span>`)
            .replace("<a","<span")
        );

    }
    return markup;
}   
\$\endgroup\$
  • \$\begingroup\$ Thanks for your detailed response. I completely agree about the indenting: I put the code together in jsfiddle and must have been using a mixture of tabs and spaces, sorry about that. It's corrected now. I like your point about wrapping the ternary statement in parentheses; I think that'd make things quite a bit more readable. \$\endgroup\$ – OliverRadini Dec 14 '18 at 8:57
  • \$\begingroup\$ Some of the suggestions you make seem to go against the direction in which javascript is heading these days. Do you think that your style (for loops, while loops, manipulating state, avoiding single use variables etc.) is the better way of doing things? It's how I used to write javascript, but the trend of the language is very much against that these days. \$\endgroup\$ – OliverRadini Dec 14 '18 at 9:01
  • \$\begingroup\$ I also agree with your comments on semicolons; I see some style recommendations against them, but it seems like they're a useful part of the language which both prevent bugs and improve readability. \$\endgroup\$ – OliverRadini Dec 14 '18 at 9:04
  • \$\begingroup\$ @OliverRadini You asked "Do you think that your style..." "...is the better way of doing things?" The aim is to reduce the code size. The number of lines of code is by far the most reliable metric used to determine code quality. Each coder will have an error rate measured in bugs per lines of code, reduce the number of lines to do a task and you reduce the number of bugs. This is consistent across languages and paradigms. \$\endgroup\$ – Blindman67 Dec 14 '18 at 9:52
  • \$\begingroup\$ That's interesting; it's new to me to have the reduction of the number of lines of code as a primary aim. It seems like, if you're looking for a quantifiable metric then it's quite useful, but this leads me to think that quantifiable metrics are unlikely to give a good indication of the quality of the code. \$\endgroup\$ – OliverRadini Dec 14 '18 at 10:00

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.