4
\$\begingroup\$

I'm in the unfortunate position to where I have to create files and folders programmatically that have to work with a legacy system. In this process, the file and folder names have to be heavily sanitized.

The POST requests that create the files and folders are sent from the client in javascript, while the actual creation happens on a server. For this, I wrote a function used to display to users what their input folder or file names will look like and to sanitize their input to work with the legacy system.

I've let this run over quite a few test cases including irregular file names that broke the legacy system prior and it seems to work fine. I'm curious about what could be improved though, especially with respect to readability and extensibility.

/**
 * @description sanitizes a folder or file name to work with #name of legacy system
 * @author me
 * @param {string} object_name
 * @param {string} mode     "file" / "folder"
 */

function sanitizeFolderOrFile(object_name, mode) {

    if (mode !== 'file' && mode !== 'folder') {
        return false;
    }

    let suffix = '';
    let retvar = '';

    if (mode === 'file') {
        suffix = object_name.split('.').pop();
        retvar = object_name.replace(new RegExp('(\.' + suffix + ')', 'g'), '');
    } else if (mode === 'folder') {
        retvar = object_name;
    }

    // values to replace
    const replace_values = [{
        value: '+\\&',
        replacement: 'u'
    }, {
        value: 'äÄ',
        replacement: 'ae'
    }, {
        value: 'öÖ',
        replacement: 'oe'
    }, {
        value: 'üÜ',
        replacement: 'ue'
    }, {
        value: 'ß',
        replacement: 'ss'
    }, {
        value: ' \\`\\´\\?\\(\\)\\[\\]\\{\\}\\/\\\\$\\§\\"\\\'\\!\\=\\-\\.\\,\\;\\:<>\\|\\^\\°\\*\\+\\~\\%',
        replacement: '_'
    }];

    replace_values.forEach(function(element) {
        retvar = retvar.replace(new RegExp('([' + element.value + '])', 'g'), element.replacement);
    });

    retvar = retvar.toLowerCase();

    if (mode === 'file') {
        retvar += '.' + suffix;
    }

    return retvar;
}
\$\endgroup\$
2
\$\begingroup\$

I think you need separate some parts of code.

  1. If you use ES5 with Decorators you can todo general determination Errors, and add for instance, function-condition in this Decorator.

  2. You can apply mechanismes Extract Methods, Simplifying Conditional Expressions and Replace Magic Number with Symbolic Constant from sanitizeFolderOrFile.

/* Decorator file */
 function determinationError(condition) {
   return function (target: any, propertyKey: string, descriptor: PropertyDescriptor) {
    const method = descriptor.value;
    descriptor.value = function () {
      try {
         condition.apply(this, arguments);
         method.apply(this, arguments);
      }
      catch(e) {
        console.error(e.message);
      }
    };
    return descriptor;
  };
}

//export determinationError;

/*enum MODE file */
 const MODE = {
     FILE: 'file',
     folder: 'folder'
  };

//export MODE;



/*file sanitizeFolderOrFile */


//determinationError = require('determinationError'); // require our decorator file
//MODE = require('MODE'); // require our MODE file

function isFileOrFolderCondition(mode) {
  if (mode !== MODE.FILE && mode !== MODE.FOLDER) {
    throw new Error('It is not file or folder') ;
  }
}

  const replaceValues = [{
        value: '+\\&',
        replacement: 'u'
    }, {
        value: 'äÄ',
        replacement: 'ae'
    }, {
        value: 'öÖ',
        replacement: 'oe'
    }, {
        value: 'üÜ',
        replacement: 'ue'
    }, {
        value: 'ß',
        replacement: 'ss'
    }, {
        value: ' \\`\\´\\?\\(\\)\\[\\]\\{\\}\\/\\\\$\\§\\"\\\'\\!\\=\\-\\.\\,\\;\\:<>\\|\\^\\°\\*\\+\\~\\%',
        replacement: '_'
    }];

function replacerAndToLowerCase(name) {
   replaceValues.forEach(function(element) {
        name = name.replace(new RegExp('([' + element.value + '])', 'g'), element.replacement);
    });

   return name.toLowerCase();
}

function sanitizeFile(fullName) {
   const ext = fullName.split('.').pop();
   let name = fullName.replace(new RegExp('(\.' + ext + ')', 'g'), '');
   name = replacerAndToLowerCase(name);
        
   return `${name}.${ext}`;
}


function sanitizeFolder(fullName) {
   return replacerAndToLowerCase(fullName);
}


//@determinationError(isFileOrFolderCondition)
function sanitize(mode, name) {
   if (mode === MODE.FILE) {
     return sanitizeFile(name);
   }
   
   return sanitizeFolder(name);
}


const fileName = sanitize(MODE.FILE, 'TeSäÄtTE.txt');
console.log('fileName', fileName);

const folderName = sanitize(MODE.FOLDER, 'SuperFolder???????');
console.log('folderName', folderName);


I added comment in Snippet when you can add Decorator and separate on files

\$\endgroup\$
  • \$\begingroup\$ is "file" still considered a magic number? Similar existing functions in our env use numbers like 1 for "file" and 2 for "folder" for modes, for instance. I assumed that naming them what they do would kill the magic number source of badness \$\endgroup\$ – Magisch Dec 19 '18 at 11:46
  • \$\begingroup\$ Everywhere where simple types are used is Magic Constant. You are comparing Something with Constant. mode === "file" - file you have to wrap in constant const FILE = 'file'. \$\endgroup\$ – Kostiantyn Okhotnyk Dec 19 '18 at 12:40
  • \$\begingroup\$ Magic Constant - is number, string, bool. Problems arise when you change mode for example "FILEs" , "FOLDERs" and you have another constants. And Everywhere when you use "file" and "folder" you must change to "FILEs" , "FOLDERs" \$\endgroup\$ – Kostiantyn Okhotnyk Dec 19 '18 at 12:45
  • \$\begingroup\$ And if your project has "file" and "folder" as string you will be have lot of places where you need change this. However, if you add constant FILE, FOLDER you will change only once place \$\endgroup\$ – Kostiantyn Okhotnyk Dec 19 '18 at 12:49
  • \$\begingroup\$ Please, confirm that you understand? \$\endgroup\$ – Kostiantyn Okhotnyk Dec 19 '18 at 12:51
1
\$\begingroup\$

Style

  • Javascript uses camelCase for naming. Don't use snake_case.
  • Use functions to reduce repetition.
  • Use the shortest or simplest form. Eg you define RegExp as strings and then create them when needed. It is simpler to define the RegExp directly, saving the need to add the extra control character delimiter \\.
  • Don't duplicate code. You test for mode not equal to "file" and "folder" then you test for mode equal to "file" then "folder" where a final else clause will do the first test without extra overhead. However I would wonder why the function is called with an unknown mode?
  • Use arrow functions when you can to reduce code size and thus increasing readability.
  • Don't assign variables with unused content that will be replaced further in the code. Eg let suffix = ""; should be let suffix;.
  • Learn to use the correct scope variable. In function scope you should use var not let and should be defined at the top of the function
  • In JavaScript for the browser an element is a very specific type of object, generally when iterating a nondescript array of items we call the item item, using element is misrepresenting what it is.

Code

  • You say legacy, but there is no clue as to which system you are converting to.
  • Your code comments indicate that mode is * @param {string} mode "file" / "folder" as a string yet no indication that it can be another value and such value would modify the return type.
  • The documentation comments give no return value and type. This is a particularly glaring omission as the return type can be two different types, a string or a boolean. Parcial, or incorrect documentation is worse than no documentation.
  • The differing return types based on mode is very problematic, and means that you need to add code to the calling function to test the return. This means that there is an indirect duplication of the test, in the function and the calling function. You may as well have the calling function determine if it should call the function in the first place and hive the function only return a fixed type.
  • Convert to the file/dir name to lowercase as first step.
  • Is the extension to keep its case?? I would think not (though this is a guess on my part)
  • There is no need to add the grouping to the regExp's \()\ as you don't use them.
  • There is no need to create a new variable to hold object_name, you can use it (the argument) rather than create a new one.
  • When you strip the extension (suffix) from the filename, you use a global regExp. It is possible for filenames to include many "." eg the filename "myfile.common.com" would be replaced with "myfilemon" rather than "myfile.common". You could store the split name and join it after popping the extension, or just trim the string.
  • Names.
    • object_name is a bit vague, its a filename or directory name, name is ma ybe a better name?
    • mode a little odd, maybe type is a better description?
    • suffix though not incorrect the more common name is fileExtension, fileExt, extension or ext.
    • retvar Should be retVar (camelCase) but that said it is a bad name. It is a var (well technically a let) so you have included a obvious implied attribute of the variable. Generally for non specific return variable we use result or in this case may be sanitizedName would be better? (or see example where name is used to hold the result)
    • element is a DOM object, can be confusing so maybe a better name is item?

The rewrite

Addressing most of the points above the rewrite only returns a string (mode now called type is "file" or it defaults as "folder"). Converts the extension to lowercase. Use RegExp literals rather than create them via instantiation. Use arrays to hold replacement arguments so that the spread operator can be used to call replace.

Please Note that I had to remove many / from the reg expressions, I did not test the edited regExps so the last one may have errors regarding what it replaces.

/**
 * @description Sanitizes a folder or file name to work with #name of legacy system
 *              Legacy system unknown
 * @author BM67
 * @param {String} name The string to sanitize
 * @param {String} type  Optional "file" or default is assumed "folder"
 * @return {String} The sanitized name
 */ 

function sanitizeFolderOrFile(name, type) { // type is "file" all else is "folder"
    var ext = "";  // adds the default at return if type is folder
    name = name.toLowerCase(); // move below following statement if case must be maintained
    if (type === "file") {
        const parts = name.split(".");
        ext = "." + parts.pop();
        name = parts.join(".");  // Could join with "_" as you replace it later
    }    
    [[/[+\&]/g, "u"], [/ä/g, "ae"], [/ö/g, "oe"], [/ü/g, "ue"], [/ß/g, "ss"],
        [/[ \`\´\?\(\)\[\]\{\}\/\\$\§\"\'\!\=\-\.\,\;\:<>\|\^\°\*\+\~\%]/g, "_"]
    ].forEach(item => name = name.replace(...item));
    return name + ext;
}

Or for those that like it spaced out (Note also keep extension case)

function sanitizeFolderOrFile(name, type) {
    var ext = "";
    if (type === "file") {
        const parts = name.split(".");
        ext = "." + parts.pop();
        name = parts.join("_"); 
    }
    name = name.toLowerCase();
    [
        [/[+\&]/g, "u"], 
        [/ä/g, "ae"], 
        [/ö/g, "oe"], 
        [/ü/g, "ue"], 
        [/ß/g, "ss"],
        [/[ \`\´\?\(\)\[\]\{\}\/\\$\§\"\'\!\=\-\.\,\;\:<>\|\^\°\*\+\~\%]/g, "_"]
    ].forEach(item => name = name.replace(...item));
    return name + ext;
}

And the ES5 version

function sanitizeFolderOrFile(name, type) {
    var parts, ext = "";
    if (type === "file") {
        parts = name.split(".");
        ext = "." + parts.pop();
        name = parts.join("_"); 
    }
    name = name.toLowerCase();
    [
        [/[+\&]/g, "u"], 
        [/ä/g, "ae"], 
        [/ö/g, "oe"], 
        [/ü/g, "ue"], 
        [/ß/g, "ss"],
        [/[ \`\´\?\(\)\[\]\{\}\/\\$\§\"\'\!\=\-\.\,\;\:<>\|\^\°\*\+\~\%]/g, "_"]
    ].forEach(item => name = name.replace(item[0], item[1]));
    return name + ext;
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for your review. Adressing your first comment about casing, I'm working within a project framework where snake_case is to be used for all variables while camelCase is reserved for functions. My project lead also told me to curb use of var and use let or const exclusively instead. As for the legacy system, it's a ms access application that is not extensible anymore but uses file and folder names in eval type statements (constructed command line calls) without escaping them at all, so any kind of special chars in names break it in one way or another. \$\endgroup\$ – Magisch Dec 19 '18 at 15:16
  • \$\begingroup\$ And yes, the extension needs to keep its case. The code could probably benefit from a comment stressing that \$\endgroup\$ – Magisch Dec 19 '18 at 15:19
  • 1
    \$\begingroup\$ @Magisch Well you must follow the team directives. Though snake_case in javascript is not idiomatic, and tenicaly function names are variables so the naming based on variable type is not a good thing (IMHO). Though there are arguments for \$\endgroup\$ – Blindman67 Dec 19 '18 at 15:25

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.