How would you refactor this JS function?

During the weekend, I found the following small JS function in a blog post:

const lineChecker = (line, isFirstLine) => {
  let document = ``;

  if (line !== "" && isFirstLine) {
    document += `<h1>${line}</h1>`;
  } else if (line !== "" && !isFirstLine) {
    document += `<p>${line}</p>`;
  } else if (line === "") {
    document += "<br />";
  }

  return document;

};

I refactored it and was thinking that it could be an excellent beginner-level refactoring kata.

How would you refactor it?

Comments

[jameslongstaff]

Personally I'd do..

const lineChecker = (line, isFirstLine) => {
  let document = "<br />";

  if (line.length) {
    const tag = isFirstLine ? 'h1' : 'p';
    document = `<${tag}>${line}</${tag}>`;
  }

  return document;
};

[tq-bit]

Leaving out all further context, this might be one of the rare occasions where to make use !! (double ! operator)

const lineChecker = (line = '', isFirstLine = false) => {
  if (!!line) {
    return isFirstLine ? `<h1>${line}</h1>` : `<p>${line}</p>`;
  }
  return '<br/>';
};

Note: After looking through some other solutions, I figured using !! introduces (or solves? I wouldn't know) a problem when passing unexpected falsy - values such as null, false or undefined.

I'd still say it's a pretty good example for refactoring. Even though it seems fairly easy to overengineer if you adhere to the 'functions do a single thing' mantra.

[SenthilKumaranC]

I have refactored like below. Divided all conditions to Spec Units and Grouped Spec Units. Check my Github Link to see my code.

const specApis = specificationGroup();

specApis.addSpecObject(headerSpecGroup, headerSpecGroupCallback);
specApis.addSpecObject(paragraphSpecGroup, paragraphSpecGroupCallback);
specApis.addSpecObject(isEmpty, breakSpecGroupCallback);

export const lineChecker = (line, isFirstLine) => {
    let initialDocument = "";
    return specApis.validateSpecs({ initialDocument, line, isFirstLine });
}

github.com/SenthilKumaranC/How-To-...

[npc pratt]

One-liner:

const lineChecker = (line, isFirstLine) => line ? isFirstLine ? `<h1>${line}</h1>` : `<p>${line}</p>` : "<br />"

[lucus-30]

functional implementation with ramda

import { always, curry, ifElse, isEmpty, pipe, split, join, map }  from "ramda"

const html = curry((tagName, str) => `<${tagName}>${str}</${tagName}>`)
const h1 = html("h1")
const p = html("p")
const br = always("<br>")

const formatLines = map(ifElse(isEmpty, br, p))
const formatText = ([title, ...lines]) => [h1(title), ...formatLines(lines)]

const textToHTML = pipe(
  split("\n"),
  formatText,
  join("\n")
)

[Derek Enos]

Here's my one-liner that includes a hilarious immediately-invoked arrow function:

const lineChecker = (line, isFirstLine) =>
      line === ""
      ? "<br />"
      : (tag => `<${tag}>${line}</${tag}>`)(isFirstLine ? "h1" : "p")

[Sebastien Rodrigues]

const lineChecker = (Line,isFirstLine) =>
return line === « » ? « 
 » :
isFirsLine ? « H1 … » :

;

[Chris Howard]

This is a simple as I could get it while retaining readability

const lineChecker = (line = "", isFirstLine = false ) => {
  let document = isFirstLine ? `<h1>${line}</h1>` : `<p>${line}</p>`
  return line ? document : `<br />`
}

[radotooo2]

const lineChecker = (line, isFirstLine) => {
  const document = ``;

  if (line === "") return document.concat("<br />");

  isFirstLine ? document.concat(`<h1>${line}</h1>`) : document.concat(`<p>${line}</p>`);

  return document;
};

[Ryan Kuruppu]

I'd have to see the rest of the code to decide whether or not this would be the best case but for now

const lineChecker = (line, isFirstLine) => {
    if(line !== "" && line != null)
        return isFirstLine ? `<h1>${line}</h1>` :  `<p>${line}</p>`;

    return "<br />";
}

Note that I've used != instead of !== in order to check for undefined. If you were going the strict approach you'd have used !== instead but because we're unaware of what will be coming in line. We're using a non-strict check here. I think this is a fairly simple solution in this case

[machinesrental]

"fat" version:

const lineChecker = (line, isFirstLine) => {
  if(typeof line !== 'string' || line === '') return '<br />';
  const tag = isFirstLine ? 'h1' : 'p';
  return `<${tag}>${line}</${tag}>;
}

minimal:

const lineChecker = (line, isFirstLine) => (typeof line !== 'string' || line === '') ? '<br />' : isFirstLine ? `<h1>${line}</h1>` : `<p>${line}</p>`

[Vesa Piittinen]

Well there are certainly more problems with this than just this function. For example we don't know if the line data has been escaped properly.

So if we go a bit outside the box of the function, and we keep the logic we could generate the document like this:

const document = text
  .trim()
  .split('\n')
  .map((line, index) =>
    (line === '' && '<br />') ||
    (index === 0 && `<h1>${line}</h1>`) ||
    `<p>${line}</p>`
  )
  .join('\n')

However this smells from HTML / CSS perspective because we're generating HTML like this:

<h1>Hello World</h1>
<br />
<p>Why there is a line break between paragraphs?</p>
<br />
<p>Did someone set paragraph margins to zero? That is evil business!</p>
<br />
<br />
<br />
<br />
<br />
<p>OMG I can do formatting with line breaks. Woohoo.</p>

While it works it is "ugly", and giving users the power to do as many line breaks as possible isn't often really desired. So we could only separate paragraphs when we have multiple line breaks in the source text.

const document = text
  .trim()
  // try this out at https://regex101.com
  .split(/\n\s*\n/)
  .map((line) => line.replace(/\n/g, '<br />'))
  .map((line, index) => index === 0 ? `<h1>${line}</h1>` : `<p>${line}</p>`)
  .join('\n')

And now we get "nicer" HTML output:

<h1>Hello world!</h1>
<p>I can no longer have multiple line breaks between paragraphs.</p>
<p>However I can...<br />now...<br />have...<br />line breaks...<br />within paragraphs! (And the header too.)</p>
<p>And paragraphs can retain their margins in CSS like they should when normal text is concerned.</p>

But this isn't good either, because this is still a non-standard way of parsing text to HTML. To have confidence you should make extensive tests to cover possible edge cases, and you have to maintain it yourself. So what to do?

Just use Markdown.

It might be worth the pain to refactor to using some Markdown library. Don't write your own, by now there are plenty of options that fit about any use case imaginable.

[André Tissot]

I tend to prefer a balance between smaller and readable code:

const lineChecker = (line, isFirstLine) => {
  if(line === "") return "<br />";
  if(isFirstLine) return `<h1>${line}</h1>`;
  return `<p>${line}</p>`;
};

But as many things, this is just a personal preference, and the team conventions should always supersede.

[panthablack]

The most pressing concern here (imo) is the function name - I don't know what it is meant to do, and certainly doesn't just 'check' lines - it constructs lines of HTML based on the arguments passed in and returns a string.

To refactor I would first look at the function in context and rename it something appropriate based on its intended use - the next steps would likely be as much to do with preferred style as anything else after that point. (Personally I lean towards declarative so I liked the solutions I have seen in the comments where the code was DRYed up using clearly named, reusable functions, but this is just personal preference.)

[RRKallan]

you touching a good point about the function name.
But the case is refactor a peace of code.
Would you rename the function, or would you write a new function with the correct understandable name also for the param?

[Andi Rosca]

My version:
I like to leave the happy path as the default return, and early return for any
exceptions

const lineChecker = (line, isFirstLine) => {
  if (!line) return "<br />";
  if (!isFirstLine) return `<p>${line}</p>`;
  return `<h1>${line}</h1>`;
};