DEV Community

Cover image for How would you refactor this JS function?
Lars Grammel for P42

Posted on

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;

};
Enter fullscreen mode Exit fullscreen mode

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

How would you refactor it?

Discussion (94)

Collapse
nombrekeff profile image
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>`;
};
Enter fullscreen mode Exit fullscreen mode

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>`;
};
Enter fullscreen mode Exit fullscreen mode

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);
Enter fullscreen mode Exit fullscreen mode

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!

Collapse
lgrammel profile image
Lars Grammel Author

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.

Collapse
ashoutinthevoid profile image
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.

Thread Thread
lgrammel profile image
Lars Grammel Author • Edited on

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.

Thread Thread
ashoutinthevoid profile image
Full Name • Edited on

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'.

Thread Thread
lgrammel profile image
Lars Grammel Author • Edited on

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.

Thread Thread
ashoutinthevoid profile image
Full Name • Edited on

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?

Thread Thread
lgrammel profile image
Lars Grammel Author

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!

Thread Thread
ashoutinthevoid profile image
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!

Collapse
nombrekeff profile image
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.

Collapse
nombrekeff profile image
Keff

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

Thread Thread
lgrammel profile image
Lars Grammel Author

It definitely makes the code more understandable.

Collapse
rkallan profile image
RRKallan

There is a small bug in your last solution

const isEmpty = (line) => line !== '' && line != null;
Enter fullscreen mode Exit fullscreen mode

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);
Enter fullscreen mode Exit fullscreen mode
Collapse
whatthehanan profile image
Hanan Hamza

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

Thread Thread
rkallan profile image
RRKallan

What do you think about?

If(!line)
Enter fullscreen mode Exit fullscreen mode
Collapse
nombrekeff profile image
Keff

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

Thread Thread
rkallan profile image
RRKallan

haha can happen :)

Thread Thread
nombrekeff profile image
Keff

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

Collapse
siddharthshyniben profile image
Siddharth

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

Collapse
baenencalin profile image
Calin Baenen

I love your refactor. So simple!

Collapse
sargalias profile image
Spyros Argalias • Edited on

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>`;
};
Enter fullscreen mode Exit fullscreen mode

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);
  })
}
Enter fullscreen mode Exit fullscreen mode

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

const toHtmlHeading = (text) => `<h1>${text}</h1>`;
const toHtmlParagraph = (text) => `<p>${text}</p>`;
Enter fullscreen mode Exit fullscreen mode
Collapse
rkallan profile image
RRKallan • Edited on

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>

Collapse
sargalias profile image
Spyros Argalias • Edited on

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.

Thread Thread
rkallan profile image
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

Thread Thread
sargalias profile image
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?

Thread Thread
rkallan profile image
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

Thread Thread
sargalias profile image
Spyros Argalias

I see, sorry for asking and for misunderstanding.

Good points :)

Thread Thread
rkallan profile image
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.

Collapse
ksaaskil profile image
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 :)

Collapse
rkallan profile image
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);
});
Enter fullscreen mode Exit fullscreen mode

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

Collapse
sargalias profile image
Spyros Argalias

Good suggestion :)

Collapse
austinshelby profile image
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>`;
  }
};
Enter fullscreen mode Exit fullscreen mode

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.

Collapse
rkallan profile image
RRKallan • Edited on

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>`;
};
Enter fullscreen mode Exit fullscreen mode

edited

Collapse
siddharthshyniben profile image
Siddharth

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

Thread Thread
rkallan profile image
RRKallan

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

Collapse
lgrammel profile image
Lars Grammel Author • Edited on

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).

Collapse
jamesthomson profile image
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.

Collapse
rkallan profile image
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}>`;
};
Enter fullscreen mode Exit fullscreen mode
Collapse
lgrammel profile image
Lars Grammel Author

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

Collapse
rkallan profile image
RRKallan

THX.

Collapse
moopet profile image
Ben Sinclair • Edited on

I'd address two problems:

I don't know what this function does

lineChecker doesn't tell me what it's checking, or what a "line" is, for that matter. I can guess it's a single line from some text based on the contents of the function, but...

This function doesn't do what it says

This function returns a formatted string based on the current line. It's producing output, not checking something. I'd expect a "check" function to return some meta information or a boolean regarding the passed data's validity.

As far as refactoring goes, there's no point in defining the document variable at all, and its name is misleading - it's a single HTML element as a string. "Document" is a word we associate with the DOM or, well, entire documents.

The last else if is redundant, it's the same as an else there.

I'd probably do this, but I'm not overjoyed about it:


javascript
formatLineAsHtmlFragment(line, isFirstLine) {
  if (line === "") {
    return "<br>";
  }

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

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

EDIT: why is my code not formatted as javascript when I did the backtick dance?
Enter fullscreen mode Exit fullscreen mode
Collapse
lgrammel profile image
Lars Grammel Author

Triple-backtick javascript worked for me to get JS formatting.

Collapse
moopet profile image
Ben Sinclair

I just changed it to that and it still breaketh!

Thread Thread
lgrammel profile image
Lars Grammel Author

javascript needs to be immediately after the 3 backticks, and needs to be followed by a newline.

Thread Thread
moopet profile image
Ben Sinclair

It is, and it's exactly how I've done it in all my other posts where it works fine. I'm not sure what's wrong with this one. Anyway, people can imagine what it's like with colours :)

Collapse
siddharthshyniben profile image
Siddharth

Simplified one liner, with default props

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

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

Collapse
siddharthshyniben profile image
Siddharth • Edited on

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

Collapse
baenencalin profile image
Calin Baenen • Edited on

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>`;
};
Enter fullscreen mode Exit fullscreen mode

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>`;
};
Enter fullscreen mode Exit fullscreen mode

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>`;
};
Enter fullscreen mode Exit fullscreen mode

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>`;
};
Enter fullscreen mode Exit fullscreen mode

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.

Collapse
rkallan profile image
RRKallan • Edited on

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

Collapse
baenencalin profile image
Calin Baenen • Edited on

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

Thread Thread
rkallan profile image
RRKallan

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

Thread Thread
rkallan profile image
RRKallan

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

Collapse
baenencalin profile image
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?

Thread Thread
rkallan profile image
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.

Collapse
reggielee7 profile image
ReggieLee
const lineChecker = (line, isFirstLine) => (`${line === "" ? <br /> : (isFirstLine ? `<h1>${line}</h1>` : `<p>${line}</p>`)}`)
Enter fullscreen mode Exit fullscreen mode

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

Collapse
nemesarial profile image
Mark Holtzhausen

I came up with two revisions. The first is what I would actually prefer because of its readability. The second was just for fun.

const lineCheck = (line, isFirstLine) => {
  if(line==='') return '<br />'
  if(isFirstLine){
    return `<h1>${line}</h1>`
  }else{
    return `<p>${line}</p>`
  }
}
Enter fullscreen mode Exit fullscreen mode

This is nice and readable -- and a trick I learned early on was to exit early if you have an simple condition that determines the outcome.

This one does the same, but more concise.

const lineCheckConcise = (line, isFirstLine)=>
  line !== ''
    ? (isFirstLine ? `<h1>${line}</h1>` :  `<p>${line}</p>`) 
    : '<br />'
Enter fullscreen mode Exit fullscreen mode

The parentheses are not necessary, but makes it more readable. Also, I broke this into multiple lines to avoid horizontal scroll, but this is actually a single line function.

Collapse
lukeshiru profile image
LUKESHIRU • Edited on

You could go full functional, using currying and having more reusable/generic utils:

const SELF_CLOSING_TAGS = [
    "area",
    "base",
    "br",
    "col",
    "embed",
    "hr",
    "img",
    "input",
    "link",
    "meta",
    "param",
    "source",
    "track",
    "wbr"
];

/** @param {string} string */
const isEmpty = string => string === "";

/** @param {keyof HTMLElementTagNameMap} tag */
const html =
    tag =>
    /** @param {string} [content] */
    content =>
        SELF_CLOSING_TAGS.includes(tag)
            ? `<${tag} />`
            : `<${tag}>${content ?? ""}</${tag}>`;

/**
 * @template Value
 * @param {(value: Value) => boolean} condition
 */
const when =
    condition =>
    /**
     * @template Output
     * @param {(value: Value) => Output} whenTrue
     */
    whenTrue =>
    /** @param {(value: Value) => Output} whenFalse */
    whenFalse =>
    /** @param {Value} value */
    value =>
        condition(value) ? whenTrue(value) : whenFalse(value);

/* END OF UTILS, THE IMPLEMENTATION FROM HERE: */

const heading1 = html("h1");
const paragraph = html("p");
const br = html("br");

/** @param {false} [isFirstLine=false] */
const lineChecker = (isFirstLine = false) =>
    when(isEmpty)(br)(isFirstLine ? heading1 : paragraph);
Enter fullscreen mode Exit fullscreen mode

You might noticed that I used JSDocs extensively to have a better DX (autocompletion, "type-checking" and so on).

One thing that concerns me is mainly that you're doing HTML with strings. Ideally if you're working on the DOM, you could keep the implementation as above, but the html function should use something like document.createElement and then append the output instead of using something nasty as innerHTML. You could also make the html function be aware of if is running on the backend or the frontend, and depending on that it renders as a plain string or as a DOM element.

Either way, this was fun, hope this approach is interesting for y'all.

Cheers!

Collapse
nombrekeff profile image
Keff

Great addition, I always like your functional approaches!!

Collapse
darkwiiplayer profile image
DarkWiiPlayer

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>`
Enter fullscreen mode Exit fullscreen mode

Why switch the boolean around? Because now you can do

let liens = ["Title", "", "paragraph"].map(lineChecker)
Enter fullscreen mode Exit fullscreen mode
Collapse
cicirello profile image
Vincent A. Cicirello • Edited on

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>`;
  } 
};
Enter fullscreen mode Exit fullscreen mode

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

Collapse
jamesthomson profile image
James Thomson

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

Collapse
andrehtissot profile image
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>`;
};
Enter fullscreen mode Exit fullscreen mode

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

Collapse
valeriavg profile image
Valeria

What about this one?

const lineChecker = (line, isFirstLine) => {
  if(!line?.trim()) return '<br/>'
  const tag = isFirstLine? 'h1':'p'
  return `<${tag}>${line}</${tag}>`
};
// Trim is needed to handle newlines & whitespaces
lineChecker('   \n   ',false) // <br/>
Enter fullscreen mode Exit fullscreen mode
Collapse
pawelmiczka profile image
Paweł Miczka
function lineChecker(line: string, isFirstLine: boolean) {
    if (line === '') return '<br />';

    return {
        [0]: `<p>${line}</p>`,
        [1]: `<h1>${line}</h1>`
    }[Number(isFirstLine)]
}
Enter fullscreen mode Exit fullscreen mode

but for that I would probably try to style it with CSS using ::first-line

Collapse
pawelmiczka profile image
Paweł Miczka

it is possible to use array too

function lineChecker(line: string, isFirstLine: boolean) {
    if (line === '') return '<br />';

    return [
        `<p>${line}</p>`, 
        `<h1>${line}</h1>`
    ][Number(isFirstLine)];
}
Enter fullscreen mode Exit fullscreen mode
Collapse
pawelmiczka profile image
Paweł Miczka

and yet another idea - use markdown cause why not?

Collapse
derekenos profile image
derekenos

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")
Enter fullscreen mode Exit fullscreen mode
Collapse
wrthfl profile image
wrthfl

My, maybe naive solution would be this:

const checkLines = (line = "", isFirstline = false) => {
  if (typeof line !== "string") return false;
  if (line === "") return "<br />";
  if (isFirstline) return `<h1>${line}</h1>`;
  else return `<p>${line}</p>`;
}
Enter fullscreen mode Exit fullscreen mode

Not a fan of ternary operators, so i put it like this.
Would love to get feedback on this too since i am most certainly not a pro.

Collapse
rkallan profile image
RRKallan

Assumption when line is not a string return empty string or nothing. In that case there would be no need for a check to print the line. But as I said, it’s a assumption.

The else is redundant and could be replaced with the return statement.

Collapse
panthablack profile image
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.)

Collapse
rkallan profile image
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?

Collapse
tqbit profile image
tq-bit • Edited on

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/>';
};
Enter fullscreen mode Exit fullscreen mode

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.

Collapse
lucus30 profile image
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")
)
Enter fullscreen mode Exit fullscreen mode
Collapse
jameslongstaff profile image
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;
};
Enter fullscreen mode Exit fullscreen mode
Collapse
radotooo2 profile image
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;
};
Enter fullscreen mode Exit fullscreen mode
Collapse
aisirachcha21 profile image
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 />";
}
Enter fullscreen mode Exit fullscreen mode

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

Collapse
machinesrental profile image
machinesrental • Edited on

"fat" version:

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

minimal:

const lineChecker = (line, isFirstLine) => (typeof line !== 'string' || line === '') ? '<br />' : isFirstLine ? `<h1>${line}</h1>` : `<p>${line}</p>`
Enter fullscreen mode Exit fullscreen mode
Collapse
merri profile image
Vesa Piittinen • Edited on

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')
Enter fullscreen mode Exit fullscreen mode

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>
Enter fullscreen mode Exit fullscreen mode

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')
Enter fullscreen mode Exit fullscreen mode

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>
Enter fullscreen mode Exit fullscreen mode

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.

Collapse
andi23rosca profile image
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>`;
};
Enter fullscreen mode Exit fullscreen mode
Collapse
eecolor profile image
EECOLOR

I don't like boolean parameters. Removing the boolean by splitting it into two functions makes is a lot more readable. At the call site you will know if it is the first line or not.

function formatFirstLine(line) {
  return line && `<h1>${line}</h1>`
}

function formatLine(line) {
  return line
    ? `<p>${line}</p>`
    : `<br />`
}
Enter fullscreen mode Exit fullscreen mode

Please note that formatFirstLine assumes line is a string.

Collapse
juliogc profile image
Júlio Gori Corradi • Edited on

I was looking below and there are good examples very similar to what came to my mind but what I'm thinking right now is why anyone doesn't get bothered by the function name?!

The function is called lineChecker but the main responsibility is to return a well-formatted line.

So the main change that I can purpose here is to call it lineFormatter!

Collapse
pengeszikra profile image
Peter Vivo

I like ternary operator in arrow function, even multiple ternary. So I use this tabulation for clear reading. Work even lineChecker() , lineChecker([42],1) use cases too.

const lineChecker = (line, isHeader) => !line
  ? line === "" 
    ? "<br />"
    : ""
  :  isHeader
    ? `<h1>${line}</h1>` 
    : `<p>${line}</p>`
;
Enter fullscreen mode Exit fullscreen mode
Collapse
pengeszikra profile image
Peter Vivo

Additional test cases, figure out without try it.

lineChecker([])
lineChecker(+[])
lineChecker(Infinity)
Enter fullscreen mode Exit fullscreen mode
Collapse
utsavladani profile image
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)
Enter fullscreen mode Exit fullscreen mode

It makes function faster

Collapse
qwertydude profile image
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 />`
}
Enter fullscreen mode Exit fullscreen mode
Collapse
auroratide profile image
Timothy Foster • Edited on

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 ( :

Collapse
baenencalin profile image
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;
};
Enter fullscreen mode Exit fullscreen mode
Collapse
gustavoalias profile image
gustavoalias

Another approach


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

Enter fullscreen mode Exit fullscreen mode
Collapse
rinah profile image
rinahs • Edited on

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

Collapse
senthilkumaranc profile image
SenthilKumaranC • Edited on

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 });
}

Enter fullscreen mode