4
\$\begingroup\$

I try to split URLs into segments. These segments got two information, their name and their own URL.

The last segment in the segments list should not have the URL property because it's the current URL from the browser. There is no need for it.

I created this code which works really fine

const directoryPath = 'rootFolder/folder1/exampleFolder1/folder3/folder3/subFolder123/file2.md';

const segments = directoryPath.split('/');

const info = segments.map((segment, index) => {

  const item = {
    name: segment
  };

  if (index < segments.length - 1) {
    let route = '';

    for (let i = 0; i <= index; i++) {
      route += `/${segments[i]}`;
    }

    item.url = route;
  }

  return item;
});

console.log(info);

I don't know if this is the only way doing this, maybe there is a more optimized way. I don't know if I really need the for-loop because at the top of the code I split the URL into segments and after that I create smaller URLs out of it.

Is there a better / easier way?

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! You're asking for a better way. Better how? Faster? Easier to read? More reusable? \$\endgroup\$ – Mast Aug 20 '18 at 7:38
2
\$\begingroup\$

Defect per line of code.

Code defects AKA bugs have a strong correlation to the number of lines of code (*1). Across all languages, coding styles, and domains there is a consistent defect rate. The industry average is about 20 per 1000 lines of code.

The metric is simple.

"... a count of lines in the text of the program's source code excluding comment lines." (*2)

One of the simplest ways to reduce defects is to reduce the number of lines. This dose not mean putting everything in one line is a good idea, nor does it mean that sprawling out what would be a single line, to many lines, does not add to the line count.

Reducing your code

So with that in mind your code is a little low density, meaning that there are more lines than are needed to do what you want done.

Reducing your code while still using the same logic yields the following

const path = 'rootFolder/folder1/exampleFolder1/folder3/folder3/subFolder123/file2.md';

const info = path.split('/').map((name, index, dirs) => {
    const item = {name};
    if (index < dirs.length - 1) {
        item.url = '';
        for (let i = 0; i <= index; i++) {
            item.url += `/${dirs[i]}`;
        }
    }
    return item;
});

console.log(info);

Ignoring the top and bottom lines we now have 10 lines.

This is still a little long as the inner loop is just a Array.join but limited to the current index.

If we look for ways to reduce the size we can see that the need to exclude the URL from the file name is adding complexity. Unless there is a good reason to not have the URL we can just treat all items equally.

const path = 'rootFolder/folder1/exampleFolder1/folder3/folder3/subFolder123/file2.md';

const dirs = path.split('/');
const info = [];
while (dirs.length) { 
    info.unshift({ url : "/" + dirs.join("/"), name : dirs.pop() });
}

console.log(info);

We are now down to 5 lines. And personally info can share the declaration with dirs

const path = 'rootFolder/folder1/exampleFolder1/folder3/folder3/subFolder123/file2.md';

const info = [], dirs = path.split('/');
while (dirs.length) { 
    info.unshift({ url : "/" + dirs.join("/"), name : dirs.pop() });
}

console.log(info);

We have reduced the number of lines from 12 to 4, reducing the chance of defect by over half.


- (*1) Steve McConnell, Code Complete

- (*2) wiki, Source lines of code

\$\endgroup\$
1
\$\begingroup\$

Have some consideration about the naming you're using.

You speack about url, but in your code you're handling paths and then they become routes.

It' would be better if you focus on what is the main idea here and choose the names accordingly.

Naming is always hard, but change variables names just to have different variables is not a good way to address the issue.

The use of Array.map() is a good choice, the bad part is that you need the index argument.

About the for loop, I think you can go ahead with the javascript arrays functions and get rid of it.

Here is a rewrite of your code in that way:

const directoryPath = 'rootFolder/folder1/exampleFolder1/folder3/folder3/subFolder123/file2.md'

// helper function to remove the check and add of / for a path segment
function addFolderSeparator (folderSeparator, element) {
    return ((element.startsWith(folderSeparator)
                ? '' : folderSeparator) + element)
}

// verify if it is the last element for a given length
const isTheLastElementFor = length =>
    current => (current === length-1)

// if you use an arrow function you can return directly an object
// the for loop could be replaced by the array method slice
// and then a reduce operation to work on the array portion 
function routesFromPath (folderSeparator, path) {
    const segments = path.split(folderSeparator)
    const isTheLastElement = isTheLastElementFor(segments.length)

    return segments.map((segment, index) => (
        isTheLastElement(index)
        ? { name: segment }
        : {
           name: segment,
           url: index
                ? segments.slice(0, index + 1)
                   .reduce((acc, actual) => {
                       acc += addFolderSeparator(folderSeparator, actual)
                       return acc
                  }, '')
               : addFolderSeparator(folderSeparator, segment)
        }))
}

console.log(routesFromPath('/', directoryPath))

In the code I use ternary operators to keep the code small, if it became more complex you should introduce functions to handle the more code instead of just adding lines of code.

I get rid of the if condition you used to skip the last element, as I thought is was hard to understand the meaning, and someone could think of a bug, as the last element is:

{
    "name": "file2.md"
}

Without the url property.

Hope this is intetional, now I think is much clear anyway.

\$\endgroup\$
  • \$\begingroup\$ The third argument to the callback of Array.prototype.map() is "The array map was called upon." - that could be used instead of isTheLastElement()... \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Oct 4 '18 at 20:24
  • \$\begingroup\$ Do you mean instead of using segments.slice you you can name the arrow function and get rid of the need to stay in scope? \$\endgroup\$ – Mario Santini Oct 4 '18 at 20:34
  • \$\begingroup\$ I mean the map callback inside routesFromPath() could be updated like: return segments.map((segment, index, segments) => ( and then instead of using isTheLastElement(index) as the ternary condition, check if index == segments.length - 1 \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Oct 4 '18 at 20:38
  • \$\begingroup\$ Ok, right. The point to have the function is just to express meaning. \$\endgroup\$ – Mario Santini Oct 4 '18 at 20:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.