React breadcrumbs component implementation

Question

I've written a breadcrumbs component (RouteBreadcrumbs) that has the following features/behavior:

• Takes in an array of labels for each breadcrumb (ex: ['Settings', 'System settings', 'Display']. The component displays these labels in the order that they're placed in in the array.
• Assumes that the last label in the array represents the currently viewed page. For the rest of the labels that come before it, the user can click on one in order to navigate to that page.
• Gets the current path it is rendered in and parses it in order to assign the correct path for each breadcrumb (except the last one)
• If the current path does not contain enough "subpaths" to satisfy the number of labels passed in, I've chosen not to render the component. For example, the component would not render if the current path was only /system-settings/display when labels contained three items.

Ultimately, the component would look like this when rendered (I haven't yet implemented the arrows between each breadcrumb):

Settings > System settings > Display

In this scenario, "Display" would be the currently viewed page. And the user would be able to click on "Settings" or "System settings" in order to jump to those respective pages.

I'd love to get feedback on my implementation. I'm also interested in learning more about best coding practices and whether there are any strong code smells in my component (and if there is one, why it is considered a code smell).

Some areas that I think might be code smells but am not really sure if they are or why they are:

• Calling pathNameIdx-- both inside the while loop and then again once the loop is exited
• Having a return statement in multiple places in my component (in my case, in just two places)
• Using a regular for loop instead of other forms like for..of, for..in, or .forEach.

import React from "react";
import { useHistory, useLocation } from "react-router-dom";

const RouteBreadcrumbs = ({labels = []}) => {
    const location = useLocation();
    const history = useHistory();
    const pathName = location.pathname;

    const numBackslash = (pathName.match(/\//g) || []).length;
    if (numBackslash < labels.length) {
        return null;
    }

    let breadcrumbPaths = [];
    let pathNameIdx = pathName.length - 1;
    for (let i = labels.length - 1; i > 0 ; i--) {
        while ((pathName[pathNameIdx] !== "/")) {
            console.assert(pathNameIdx >= 0, "pathNameIdx is no longer a valid index");
            pathNameIdx--;
        }

        const newPath = pathName.substring(0, pathNameIdx);
        breadcrumbPaths = [newPath, ...breadcrumbPaths];
        pathNameIdx--;
    }
    
    return (
        <div>
            {labels.map((label, idx) => <h5 key={idx} onClick={idx < breadcrumbPaths.length ? () => history.push(breadcrumbPaths[idx]) : null} className={"label2"}>{label}</h5>)}
        </div>
    )
};

```

Answer 1

Your code is looking pretty good. Here's a handful of suggestions you could try:

This part

{labels.map((label, idx) => <h5 key={idx} onClick={idx < breadcrumbPaths.length ? () => history.push(breadcrumbPaths[idx]) : null} className={"label2"}>{label}</h5>)}

is a really long line of code. Try breaking it up, it makes it easier to read.

{labels.map((label, idx) => (
  <h5
    key={idx}
    onClick={
      idx < breadcrumbPaths.length
        ? () => history.push(breadcrumbPaths[idx])
        : null
    }
    className={"label2"}
  >
    {label}
  </h5>
))}

(I tend to prefer 2-space indentation with javascript and HTML as they both tend to be indentation heavy, and it seems to be a more common in the javascript community - so I'll use that in these examples)

Also, refrain from using an index for an element key when possible (the react docs talk about why). You could instead use the breadcrumbPath, as that would be unique to each element, and the keys would correctly change when the path changes.

{labels.map((label, idx) => (
  <h5
    key={breadcrumbPaths[idx]}
    onClick={...}
    className={"label2"}
  >
    {label}
  </h5>
))}

You have some logic in there to calculate the number of backslashes a URL contains, but if you just wait a little bit on that early return, you will already have easy access to that number, as its the same as breadcrumbsPath.length + 1

if (breadcrumbPaths.length + 1 < labels.length) {
  return null;
}

On that note, this function won't handle the case correctly when there's a leading slash on the URL (i.e. 'example.com/a/b/') - that'll need to be taken care of. If you use the above code snippet, then you will only need to fix this issue in one place instead of two - where you create the breadcrumbPaths array.

To address your concern of having multiple returns: there's nothing wrong with having multiple returns like that in a function - in fact, it can often lead to cleaner code. Often, the alternative means large if-thens or flag variables, both of which are much harder to follow. I've heard that once upon a time a function with a single exit used to be something encouraged, but that's not the case anymore.

Now onto the part that you probably care about most - what to do about that chunk of logic with the c-style for loop, and multiple pathNameIdx--s.

First things first, I would probably break the meat of that logic out into a helper function. There's a lot of low-level work going on there that gets in the way of reading and understanding your component.

Here's how your component might look after doing that

import React from "react";
import { useHistory, useLocation } from "react-router-dom";

const RouteBreadcrumbs = ({labels = []}) => {
  const location = useLocation();
  const history = useHistory();
  const pathName = location.pathname;

  const breadcrumbPaths = ancestorPaths(pathName);
  if (breadcrumbPaths.length + 1 < labels.length) {
    return null;
  }

  return (
    <div>
      {labels.map((label, idx) => (
        <h5
          key={breadcrumbPaths[idx]}
          onClick={
            idx < breadcrumbPaths.length
              ? () => history.push(breadcrumbPaths[idx])
              : null
          }
          className={"label2"}
        >
          {label}
        </h5>
      ))}
    </div>
  )
};

function ancestorPaths(pathName) {
  let breadcrumbPaths = [];
  let pathNameIdx = pathName.length - 1;
  while (true) {
    while (pathName[pathNameIdx] !== "/") {
      if (pathNameIdx === 1) {
        return breadcrumbPaths;
      }
      pathNameIdx--;
    }

    const newPath = pathName.slice(0, pathNameIdx);
    breadcrumbPaths = [newPath, ...breadcrumbPaths];
    pathNameIdx--;
  }
}

Notice how there's a lot less low-level logic going on in the actual RouteBreadcrumbs component? It's a lot easier to read through it and get a good idea of what's going on - except for maybe the actual jsx getting returned - it's a little too nested for my liking, but it'll get simplified in a bit.

A couple other things I'll note with your original code:

• Prefer string.slice() over string.substring() - it has fewer quarks
• Good use of console.assert() there. The one thing I dislike about console.assert() is that it doesn't actually throw an error, it just logs one. Sometimes this isn't a big deal, here it is. If that assertion happened to come true, it would be stuck in an infinite loop, logging forever. You can just do if (condition) throw new Error(...) instead, or make your own assert function that actually throws an error.

Now lets look at simplifying our new ancestorPaths() helper function. It is possible to use regex to gather the indicies of all the slashes, which would work similar to how you have it, but greatly simplify the logic (see here). I'm going to instead just split on '/', build a list, and use a generator function, as follows:

function* iterAncestorPaths(pathName) {
  const parts = pathName.split("/").filter(x => x !== "");
  for (let i = 1; i < parts.length; ++i) {
    yield "/" + parts.slice(0, i).join("/");
  }
}

Note that the .filter() at the beginning will take care of both the leading / and a potential trailing slash.

One final thing: If your breadcrumb segments are links, why not make them actual links? it'll simplify some things.

Here's the final solution, with a couple other misc tweaks.

import React from "react";
import { Link, useLocation } from "react-router-dom";

const RouteBreadcrumbs = ({labels = []}) => {
  const pathName = useLocation().pathname;

  const ancestorPaths = [...iterAncestorPaths(pathName)];
  if (ancestorPaths.length + 1 < labels.length) {
    return null;
  }

  return (
    <div>
      {ancestorPaths.map((path, idx) => (
        <Link key={path} to={path} className="label">
          {labels[idx]}
        </Link>
      ))}
      <span className="label">{labels[labels.length - 1]}</span>
    </div>
  );
};

function* iterAncestorPaths(pathName) {
  const parts = pathName.split("/").filter(x => x !== "");
  for (let i = 1; i < parts.length; ++i) {
    yield "/" + parts.slice(0, i).join("/");
  }
}