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

[Keff]

Interesting function. I would need to have a closer look at the rest of the code to see whether refactoring this specific function is really the best option. It's seems like a weird function to me, and it does a few things.

The most straight forwards refactor would be:

const lineChecker = (line = '', isFirstLine = false) => {
    const hasLine = line !== '';
    if (!hasLine) return '<br />';

    return isFirstLine
        ? `<h1>${line}</h1>`
        : `<p>${line}</p>`;
};

But I don't know if line could be null or other falsy values as well, and if those would also return <br /> or not. In case we want to check for null values too:

const lineChecker = (line = '', isFirstLine = false) => {
    const hasLine = line !== '' && line != null;
    if (!hasLine) return '<br />';

    return isFirstLine
        ? `<h1>${line}</h1>`
        : `<p>${line}</p>`;
};

If it was me, I would prefer separating logic into separate functions to separate concerns. A function to check whether the line is empty or not, another one to get the line, one to generate the header, etc... something like that.

This is the initial idea that comes to mind:

const isEmpty = (line) => line !== '' && line != null;
const isFirstLine = (index) => index === 0;
const header = (line) => `<h1>${line}</h1>`;
const paragraph = (line) => `<p>${line}</p>`;
const br = () => `<br/>`;

const lineChecker = (line = '', index) => {
    if (isEmpty(line)) return br();

    return isFirstLine(index) 
        ? header(line) 
        : paragraph(line);
}

const checkLines = (lines = []) => lines.map(lineChecker);

Note that I have separated it into smaller more composable functions for better reusability. It's a longer solution, but I feel like it makes it quite a lot more extendable and reusable across the rest of the codebase.

This could be taken in multiple ways though, so this is just my take!

[Lars Grammel]

Love it!

I'm a fan of simplicity so I ended up with the first solution you proposed (plus inlined hasLine), but I very much agree on your thoughts regarding null / undefined being passed in.

For your 3rd solution I was wondering where the line towards overengineering is, I think it depends very much on the project and it's context.

[Keff]

Great point on overengineering, I must admit the last solution could be a bit overengineered. That's the reason I would need to check the full code base and see the real use of this function.

I too prefer simplicity in general, so I would probably go for the first example.

[Keff]

On the point of the hasLine variable, it's just my personal preference, I really like how readable it makes the conditions.

[Lars Grammel]

It definitely makes the code more understandable.

[Full Name]

"It depends" definitely strikes me as the answer to the overengineering question.

Extracting predicates feels to me like an easy sell. Their reusability and the readability improvement approaches self evident. How many general purpose js libraries come without an isEmpty function?

The html strings would require more context (and pose more questions - what situation are we in that this is preferable to a robust templating solution?). Either Keff didn't mention it or I overlooked it, but the separate functions also permit independent testing.

[Lars Grammel]

In this particular example, I would consider extracting predicates premature abstraction. You might never need them, and you add unnecessary cognitive complexity. When it comes to testing, the function can easily be unit tested as is.

That being said, I am in favor of extracting predicates etc. when there is reuse and value in doing so, i.e. following the 3 strikes rule for refactoring.

[Full Name]

What added cognitive complexity is there in reading isEmpty rather than parsing the expression line !== '' && line != null?

WRT to testing, a mistake in a predicate would cause a test of that branch's eventual value to fail. Not because the code that produces that value is wrong, but because the predicate is wrong. Yes, you can still test every one of the diverse things this function is doing, but conflating multiple behaviors just makes finding the specific problem harder. It's changing 'the problem is here' to 'the problem is in this general area'.

[Lars Grammel]

The complexity is in the number of lines to read and in the links that you need to follow. The individual pieces might be simpler, but their sum is not due to their links. E.g. if you want to understand what paragraph does, you need to find and read the definition, keep it in your head, go back to the place where it was used, and then put it together.

Re testing, I agree that it's good to tear things apart (and introduce e.g. predicates), but only when the code is complex enought. I consider this method way too trivial to do that - it's far easier to test as one.

[Full Name]

Okay, I follow what you mean by complexity. Thank you for clarifying. I don't wish to say I disagree, but I don't think I share your perspective.

I think there's an argument to be made in the very particular case that a function (eg isEmpty) has a name (or function signature, or instantly visible documentation if one has the convenience of IDE like features) that is clear to the reader, and that function is separately tested. That it's separately tested allows me to trust that the implementation does what it should. That the name or signature (particularly the signature, tbh) accurately describe behavior allows me ignore implementation details when I'm reading lineChecker.

If my units (ie functions as the unit of composition) are well named and tested, I should be able to compose them without repeatedly diving into every implementation of every unit. If one can't do that, I think there is a massive problem in one's approach to writing and testing software.

Diving into every implementation detail of every function while reading to understand the behavior of a function/module/unit/etc strikes me as a premature step in a manner similar to what you've expressed about extracting things before there are demonstrated instances of reuse.

Edit to add: If one has to jump back to the implementation of every unit within their own code, Im not sure I see how one can logically avoid doing the same thing with library code. Does the average React user feel the need to read the implementation of useState, for example? If not, why is that function call privileged?

[Lars Grammel]

Overall I think I agree with you and would have made similar arguments in the past. I often split code along similar lines in my projects, maybe with slightly different thresholds to method/function extraction.

However it seems to assume a perfect world with perfect software development projects, or that you and only you own the code, and you never need to hand it over to someone else.

In the reality that I've encountered in professional life, time pressure, employee turnover, mistakes, and other forces lead to source code being far from perfect, sadly. Trusting the function name or that a function is sufficiently tested is a bet in that world. I would not call that "a massive problem in one's approach" - for one-person projects, it's not an issue at all. It happens when shifting teams of developers work on moving requirements over multiple years, and it's non-trivial to avoid.

In that world, I prefer going for code that's as simple and predictable as possible, and that's why I prefer not extracting functions or predicates unless it is genuinely needed (either for testing or because it reduces code complexity, e.g., by removing duplication).

Thanks for the long reply; I enjoyed hearing your perspective!

[Full Name]

Fair enough.

Professionally I've encountered sprawling monoliths with no tests at all and team leads who felt it was okay because it had been in production longer than I had been with the company. Your quite right, none of us can avoid reality. It does not, however, change what I would aim for or advise to others.

Likewise, thank you for the civil discussion!

[Siddharth]

To check for non-nullish I'd suggest using ??.

[Calin Baenen]

I love your refactor. So simple!

[RRKallan]

There is a small bug in your last solution

const isEmpty = (line) => line !== '' && line != null;

returns false when line = "" and true when line = "test"

possible solution

const isEmpty = (line) => !line && line !== 0;
const isFirstLine = (index) => !index;
const getElementTag = (index) => (isFirstLine(index) ? "h1" : "p");
const getText = (line, index) => {
    const elementTag = getElementTag(index);
    const text = `<${elementTag}>${line}</${elementTag}>`;
    return text;
};
const br = () => "<br />";
const lineChecker = (line = "", index) => {
    if (isEmpty(line)) return br();

    return getText(line, index);
};

const checkLines = (lines = []) => lines.map(lineChecker);

[Keff]

Ohh my bad. Thanks for pointing it out! I did not run the code after the refactor (big mistake when refactoring)

[RRKallan]

haha can happen :)

[Keff]

It definitely does, I've been away from javascript for a while, mostly working with dart/flutter...

[Hanan Hamza]

the easiest way to check for all falsy values would be if(!!line === false)

[RRKallan]

What do you think about?

If(!line)

[rinahs]

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

[Austin Shelby]

I would first start thinking about what this function actually does.

For me the name lineChecker is a little bit misleading because it looks like the function actually parses strings to html.

This is what I came up with:

const parseStringToHtml = (
  isFirstLine: boolean,
  line?: string
): string => {
  if (!line) {
    return "<br />";
  } else if (isFirstLine) {
    return `<h1>${line}</h1>`;
  } else {
    return `<p>${line}</p>`;
  }
};

For a function this small there isn't really much to refactor. Parsing markdown has been done thousands of times already and there are libraries that do this perfectly.

Cool puzzle nonetheless.

[Lars Grammel]

The renaming idea + introducing types is great!

To me, the method seems to be more of a print than a parse method (but I don't have the full context, just saw it in isolation).

[RRKallan]

Adding the types makes sense.

Only the else if else statement could be replaced. My suggestion based on your example

const parseStringToHtml = (line = "", isFirstLine = false): string => {
    if (!line) return "<br />";

    if (isFirstLine) return `<h1>${line}</h1>`;

    return `<p>${line}</p>`;
};

edited

[Siddharth]

I'd suggest putting the line argument in the function first, as line is more important and isFirstLine can have a default value

[RRKallan]

true i just copy paste it and didn't read the arguments.

[James Thomson]

Extra bonus points for renaming the function. This would be my first improvement. Even if you left the function as is and just renamed it to describe its purpose that's a huge improvement in my book.

[gustavoalias]

Another approach

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

[RRKallan]

Assumption:

• line must is a string and length >= 1 return line
• isFirstLine is a boolean true htmlTag = h1 else htmlTag = p

const lineChecker = (line = "", isFirstLine = false) => {
    if (typeof line !== "string" || !line.length) return "<br />";

    const elementTag = isFirstLine === true ? "h1" : "p";
    return `<${elementTag}>${line}</${elementTag}>`;
};

[Lars Grammel]

Nice refactoring, I like the tag extraction and the input type checking!

[RRKallan]

THX.

[ReggieLee]

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

I like the shortest way, but it seems is not readability

[Siddharth]

Simplified one liner, with default props

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

Note that this isn't a very scalable solution. Scaling this will need a complete rewrite.

[Siddharth]

The bigger answer is "it depends". If this is it (which probably isn't), you can use my solution. If not there are many ways of making this extensible, like mapping conditions and tags in an array so we can simply add an item to the array to add more elements. Or we could refactor the wrapping in elements to a custom function. There are many more ways we can do this, I'm just mentioning the ones which come to mind

[Vincent A. Cicirello]

I didn't read through all of the other comments so someone else may have already suggested this one, but if the objective is to minimize conditions that are explicitly checked, start with the last case like this:

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

I also got rid of the concats with the unnecessary local variable.

[James Thomson]

When using the early return method, you don't need else if. Just use if.

[Calin Baenen]

Here's a good one:

const lineChecker = (line, isLn1) => {
    isLn1 = isLn1 ?? false; line = line ?? "";
    const nEmpty = line.length > 0;
    let doc = "";

    if(nEmpty && isLn1) doc += `<h1>${line}</h1>`;
    if(nEmpty && !isLn1) doc += `<p>${line}</p>`;
    if(!nEmpty) doc += "<br/>";

    return doc;
};

[Calin Baenen]

Rerefactor based on Keff's answer.

const lineChecker = (ln="", isLn1=false) => {
    ln = ln ?? ""; isLn1 = isLn1 ?? false;

    const empty = ln.length == 0;
    if(empty) return "<br/>";
    return isLn1 ? `<h1>${ln}</h1>` : `<p>${ln}</p>`;
};

Theoretically, this is the most optimized you can get.
BUT, if we use TypeScript, and go the mathematical route of only using one letter per var-name, we get this:

const c:Function = (l:string="", f:boolean=false) => {
    const e:boolean = ln.length == 0;
    if(e) return "<br/>";
    return f ? `<h1>${l}</h1>` : `<p>${l}</p>`;
};

Assuming people use the correct types in JS, as if we we're in a strongly typed language, we get the final result of:

const c = (l="", f=false) => {
    const e = l.length == 0;
    if(e) return "<br/>";
    return f ? `<h1>${l}</h1>` : `<p>${l}</p>`;
};

But, we can make e a one-time expression:

const c = (l="", f=false) => {
    if(l.length == 0) return "<br/>";
    return f ? `<h1>${l}</h1>` : `<p>${l}</p>`;
};

LOOK! It's a 2-liner! :O

However, (to me) this looks like the end of the road for refactorization, I don't think we can make this any smaller,- and if you can, it's because of a very niche detail.
But I think this is the best optimization from the original function.

[RRKallan]

understandable and readable is very hard in your solution.
Refactor is not to get code smaller.

I prefer === checks instead ==
I prefer !?.length instead l.length == 0

According to w3 guidelines, a space should be included before the trailing / and > of empty elements, for example, <br />. These guidelines are for XHTML documents to render on existing HTML user agents.

edited

[Calin Baenen]

According to w3 guidelines, a space should be included before the trailing / and > of empty elements, for example,

I don't think your example works as intended.

Also, why is that? Does the space do something to make the HTML more understandable to some interpreters?

[RRKallan]

edited my comment

According to w3 guidelines, a space should be included before the trailing / and > of empty elements, for example, <br />. These guidelines are for XHTML documents to render on existing HTML user agents.

[Calin Baenen]

understandable and readable is very hard in your solution.
Refactor is not to get code smaller.

Then I think the people who submitted those one-liners (Which are even harder to understand than my code!) misunderstood the point. :p

[RRKallan]

the one line solutions with nested ternary expressions are also difficult to understand.

[RRKallan]

Goals for refactoring code: readability, understandability, maintainability, performance, usage of latest code style / programming / functions / methods

[Timothy Foster]

The first thing I would do is change the name, perhaps to parseLineToHtml, which is more in line with what it does.

The second thing unfortunately depends on context, but the isFirstLine parameter bothers me. Probably because this function is very close to being able to operate on single lines independent of additional context, if not for that parameter.

The third thing is probably shuffle the br condition to be first so we don't have to repeatedly ask if the line isn't empty. Personally, I don't mind small, non-nested if-ladders as they can sometimes be more readable than ternaries.

And finally, I wouldn't do any of that without tests ( :

[Utsav Ladani]

If line is plain text, then you should use object to add HTML in document instead of document += ....

const header = document.createElement("h1")
header.textContent = line
document.append(header)

It makes function faster

[𒎏Wii 🏳️‍⚧️]

Other than shortening it a bit, I'd change the second parameter:

const lineChecker = (ilne, followup) =>   ​line
  ​? followup
      ? `<p>${line}</p>`
      : `<h1>${line}</h1>`
   : `<br>`

Why switch the boolean around? Because now you can do

let liens = ["Title", "", "paragraph"].map(lineChecker)

[Keff]

Great addition, I always like your functional approaches!!

[lynn]

I'd just rename it and call it lineFormatter, and add comments about sample inputs and outputs.