2
\$\begingroup\$

I have a list of files that conform to a template like this:

XXX_{project}_{variation}---XXX.html

and

XXX_{project}_{variation}.html

The variation is not always present, so there are also files like this: XXX_{project}---XXX.html and XXX_{project}.html

What I want to do is to check if all these files have the same project and variation then return {project}_{variation}, if not, then to check if they all have the same project and return {project} else return "Mixed".

I have the following code, which works, but it looks too long:

let mixed = [
      "V50_A_X---100.html",
      "V20_A_X---101.html",
      "V50_B_X---102.html",
      "V50_A_Y---103.html",
      "V20_B---104.html",
      "V50_A_X.html",
]

let sameProject = [
  "V50_A_X---100.html",
  "V20_A_X---101.html",
  "V50_A_Y---102.html",
  "V50_A_Y---103.html",
  "V20_A---104.html",
  "V50_A_X.html",
]

let sameProjectAndVariation = [
  "V50_A_X---100.html",
  "V20_A_X---101.html",
  "V50_A_X---102.html",
  "V50_A_X---103.html",
  "V50_A_X.html",
]

const getPackageName = function(files) {

  files = files.map(f => f.replace('.html', '').split('---')[0]);

  let filesProjectAndVariation = files.map((file) => {
    return {
      project: file.split('_')[1],
      variation: file.split('_')[2] || '',
    }
  })
  let packageName = null;
  let projectAndVariationAreSame = filesProjectAndVariation.every( (val, i, arr) => JSON.stringify(val) === JSON.stringify(arr[0]) );
  if (projectAndVariationAreSame) {
    packageName = filesProjectAndVariation[0].project + "_" + filesProjectAndVariation[0].variation
  } else {
    let projectAreSame = filesProjectAndVariation.every( (val, i, arr) => val.project === arr[0].project );
    if (projectAreSame) {
      packageName = filesProjectAndVariation[0].project;
    } else {
      packageName = "MIXED";
    }
  }
 return packageName;
}

getPackageName(mixed)
getPackageName(sameProject)
getPackageName(sameProjectAndVariation)
\$\endgroup\$
2
\$\begingroup\$

Personally I don't think it's too long. It was fairly easy to read, especially with the good names.

Here's a few things that could improve it:

  • Use early returns instead of nested if's. This makes it even easier to read. Also, you don't need the packageName variable
  • Use const instead of let. That way I don't have to wonder if the variable will be mutated later on.
  • Handle the case where the array of files is empty
  • If none of the files have a variation you will have an empty variation. E.g. A_

Here's my attempt:

const getPackageName = (files) => {
    files = files.map(f => f.replace('.html', '').split('---')[0])

    const fileInfos = files.map(file => ({
        project: file.split('_')[1],
        variation: file.split('_')[2],
    }))
    const projects = [...new Set(fileInfos.map(f => f.project))]
    const variations = [...new Set(fileInfos.map(f => f.variation).filter(v => !!v))]

    if (projects.length === 1 && variations.length === 1) {
        return projects[0] + '_' + variations[0]
    }
    if (projects.length === 1) {
        return projects[0]
    }
    return 'MIXED'
}

Changed it to check projects and variations separately, which resulted in a little bit less code (or actually just simpler models), and a bit easier to handle the edge cases.

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

Just some smaller remarks:

Personally I don't like declaring "top level" functions using a function expression (or arrow expressions). I prefer normal function declarations for two reasons: It makes them optically distinct from "normal" variable declarations and hoists the function allowing it to be used before the declaration.

file.split('_') shouldn't be repeated.

Using JOSN.stringify to compare objects leaves a bad taste and may even be dangerous. Historically neither JS(*) nor JSON properties have an defined order, so there is no guaranty that the JSON representations two objects (or for the matter of the fact two different JSON representations of one object) will have the same order of properties.

(*) This has changed for JS, but not for JSON.

\$\endgroup\$
0
\$\begingroup\$
  • You are iterating over the files (or same length) array at least three times, when one time would suffice.

  • You can avoid a lot of clutter with destructuring assignments.

  • The JSON.stringify’s are superfluous and expensive (as already mentioned).

  • A RegExp could fit better for splitting.

Below is some code illustrating the above points:

const separator = /[_.]|-{3}/

const sameName = (acc, fileName) => {
  const [_, project, variation] = fileName.split(separator)
  const [prevProject, prevVariation] = acc.split(separator)

  return (variation == prevVariation && project == prevProject)
    ? `${project}_${variation}`
    : project == prevProject
      ? project
      : "Mixed"
}

const getPackageName = ([first, ...rest]) =>
  rest.reduce(sameName, first.split(separator).slice(1, 3).join('_'))

(Using a for loop would be able to return earlier.)

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Just fyi: This breaks when files is an empty array. The variation can end up as "html" if you lack a real variation. The prevProject will be "Mixed" in case you had a mixed acc. Both project and variation will be set to undefined in multiple cases. What happens if a real project is called "Mixed"? \$\endgroup\$ Jul 15 at 10:18
  • \$\begingroup\$ @MagnusJeffsTovslid good point on empty array, thanks. Since the end result of the function is one of three strings, I wanted to highlight that (so it doesn't matter what part ends up as what, as long as they're different/same from each other). If a project is called "Mixed", it would be indistinguishable by definition. \$\endgroup\$
    – morbusg
    Jul 16 at 8:04
  • \$\begingroup\$ @MagnusJeffsTovslid the project doesn't get set as undefined on any of the given example cases. \$\endgroup\$
    – morbusg
    Jul 16 at 8:11

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.