Discussion on: How would you refactor this JS function?

[Spyros Argalias]

Solution to this question

For this particular function, I would just remove the unnecessary conditionals. I wouldn't turn it into a one-liner. I also wouldn't give it more complicated logic than simple if statements.

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

The real problem

However, passing a Boolean into a function is considered a code smell. Some downsides are that:

• it's a strong indication that the function is doing multiple things. This one is doing 4 things.
• it makes it more difficult to test (because of the number of code paths).
• it's more troublesome to reuse it. Some other code in the codebase may want to surround something with h1 tags, without caring about the other cases. But, instead of calling something like createHeading(text), it needs to bother with more parameters and call lineToHtml(text, true)
• it doesn't grow well. As you need more formatting functionality in the future, you'll modify this function, give it more logic, and more parameters.

The real solution

The solution is to refactor the parent of this function to something like this:

const parent = (textLines) => {
  return textLines.map((line, i) => {
    if (i === 0) {
      return toHtmlHeading(line);
    }
    return toHtmlParagraph(line);
  })
}

Then, make the lower-level functions only do one thing:

const toHtmlHeading = (text) => `<h1>${text}</h1>`;
const toHtmlParagraph = (text) => `<p>${text}</p>`;

[Kimmo Sääskilahti]

Excellent answer, I like that you got rid of the boolean argument.

Now I'm only waiting for someone to write unit tests, in my world those are written before refactoring :)

[Spyros Argalias]

Good suggestion :)

[RRKallan]

it("should handle parent", () => {
    const textLinesAsArray = ["line 1", "line 2", "", "line 4"];

    const expectedValue = ["<h1>line 1</h1>", "<p>line 2</p>", "<p></p>", "<p>line 2=4</p>"];
    const resultValue = parent(textLinesAsArray);

    expect(resultValue).toEqual(expect.arrayContaining(expectedValue));
    expect(expectedValue).toEqual(expect.arrayContaining(resultValue));
    expect(resultValue.length).toEqual(expectedValue.length);
});

it("should handle toHtmlHeading", () => {
    const value = "Heading title";

    const expectedValue = "<h1>Heading title</h1>";
    const resultValue = toHtmlHeading(value);

    expect(resultValue).toEqual(expectedValue);
});

it("should handle toHtmlParagraph", () => {
    const value = "Paragraph text";

    const expectedValue = "<p>Paragraph text</p>";
    const resultValue = toHtmlParagraph(value);

    expect(resultValue).toEqual(expectedValue);
});

didnt test these but i think this would be a start for testing.

And it's a choice TDD or BDD. It all depends on application. preferred working approach,
And in this case the question is to refactor the code. Basically there would be no need to change the tests. Still you will insert same input and would expect the same output

Except 2 or 3 solutions added new functions. So only on that case you would write new test

[RRKallan]

a H1 could be used multiple times, there fore it could be line 3, 9, 18 etc etc. So to check and set it only on first iteration, is from my perspective not correct

Also in the provided solution the <br /> is missing and would there placed a empty <h1> on first iteration and for all other <p>

[Spyros Argalias]

The solution was just an example of how you can remove the Boolean argument for better code overall. It's just a fictional example.

Depending on what you need, you could make changes. For example, you can add a function for the <br> just like toHtmlHeading, or you could have a function such as toHtmlElement(text, tag) instead of individual functions for each element, or you could have a completely different parent.

Edit: Replaced my answer to also answer the added second part.

[RRKallan]

so based on the original provided code there is no iteration.
And on that function variable name is isFirstLine as boolean. And then just be an ashole, but isFirstLine could refer to new section

[Spyros Argalias]

I know there's no iteration in the original example. In my answer I'm making a guess at what a caller of lineChecker might intend to do. It's not shown in the opening post. I'm just creating it myself. It's fictional, so it won't always apply.

Sure, isFirstLine could refer to whatever you want. Anyone can name anything what they want. If, in your application, that could refer to a new section, then my example solution wouldn't apply.

Sorry, I didn't quite catch that but did you mean to call me an "asshole" there, or am I misunderstanding?

[RRKallan]

Sorry, I didn't quite catch that but did you mean to call me an "asshole" there, or am I misunderstanding?

No i'm not mentioning you, it meant to me because of the question. There I totally can understand your approach

[Spyros Argalias]

I see, sorry for asking and for misunderstanding.

Good points :)

[RRKallan]

No sorry please. Always better to ask and to clarify.
Also good to see and hear different approaches.

Which also is good that you can defend and clarify your solution.