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