2
\$\begingroup\$

I'm using SharePoint 2013 JSOM. My variable this.templates contains a collection of templates with many different names. The only way to iterate through this collection is by using an Enumerator. I can only get the name of each template by using get_title().

I want to get the templates whose names respect the following rules:

  • It contains "espace_projet"
  • It contains the letter "v" followed by any number, once

Then, out of the templates i got, I want to get the one with the highest value that was after the "v".

I'm not used to JavaScript, this code works fine, but feels like overkill. How could I make it shorter/more readable?

ApplyTemplate()
{
    var highestVersion = {number: -1, template: null};
    var templateEnum = this.templates.getEnumerator();

    while(templateEnum.moveNext())
    {
        //Gets the element name in my collection
        var templateName = templateEnum.get_current().get_title();
        if(templateName.includes('espace_projet'))
        {
            var version = templateName.match('v[0-9]+|v_[0-9]+');
            if(version){
                var versionNumber = version[0].match('[0-9]+');
                if(versionNumber[0] > highestVersion.number)
                    highestVersion = {number: versionNumber[0], template: templateEnum.get_current()};
            }
        }
    }

    return highestVersion;
}
\$\endgroup\$
  • \$\begingroup\$ what if there are two or more numbers following the v? should such a string still be considered? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Mar 16 '18 at 18:55
  • \$\begingroup\$ @SamOnela - Yes, version numbers can have multiple digits \$\endgroup\$ – Treycos Mar 16 '18 at 19:32
1
\$\begingroup\$

You have it almost perfect in your question. With a little improvement you can reduce memory and CPU load.

  1. Don't create a new object every time you find the next highest version. Set the properties of the existing object to the new highest.
  2. Only call templateEnum.get_current() once, you should not call it again when you find the highest version
  3. Don't pass strings to String.match it requires additional overhead to convert the string to a RegExp
  4. Don't test for the same thing twice. .match('v[0-9]+|v_[0-9]+') and .match('[0-9]+') can be done in one step.

And some style points

  1. The function is called ApplyTemplate (should not be capitalized) though to me it seams to find highest template. Maybe a better name?
  2. The function is about templates, is there a need to add template to templateEnum, templateName Good naming uses the context to infer. Keep names short by not adding redundant and inferred details.

Thus your function becomes

highestVersionTemplate() {
    const highest = { number: -1 };
    const enumer = this.templates.getEnumerator();
    while (enumer.moveNext()) {
        const current = enumer.get_current();
        const title = current.get_title();
        if (title.includes("espace_projet")) {
            const ver = title.match(/v_?([0-9]+)/);
            if (ver && ver[1] > highest.number) {
                highest.number = Number(ver[1]);
                highest.template = current;
            }
        }
    }
    return highest;
},
\$\endgroup\$
  • \$\begingroup\$ Two small additions: 1.) enum is forbidden as an identifier name as it is a reserved keyword for future use. 2.) ver[1] > highest.number is a string-number-comparison, I'd avoid comparison between different types wherever possible because of the non-trivial coercion rules. At least I don't know them by heart. \$\endgroup\$ – ComFreek Mar 17 '18 at 7:34
  • \$\begingroup\$ @ComFreek my bad was meant to be enumer \$\endgroup\$ – Blindman67 Mar 17 '18 at 11:10
1
\$\begingroup\$

Contentwise considerations

  • Use const wherever possible, otherwise let. Avoid var.
  • Match the version number only once (see code below) and employ the ? operator to save redundancy: v_?[0-9].
    (The latter might actually be disputable, I personally find both versions easy to read.)

Your code would be shorter and much easier to reason about if it was more functional. However, as JS is missing some functional concepts other languages take for granted (array comprehension, min/max in array by key function), this would require helper functions being a new, but onetime overhead. Are you using a third-party library like Lodash by accident? Then you already got many functional polyfills.

Which methods does the Enumerator you are using from SharePoint support apart from the iterative query interface with moveNext(), get_current()? If there is no built-in support, I'd suggest writing a generator function:

function *getTemplateIterator(templates) {
  const templateEnum = templates.getEnumerator();
  while (templateEnum.moveNext()) {
    yield templateEnum.get_current();
  }
}

function ApplyTemplate() {
  return [...getTemplateIterator(this.templates)]
    .filter(template => template.get_title().includes('espace_projet'))
    .map(template => {
      const version = /v_?([0-9]+)/.match(template.get_title());
      return {
        version: (version !== null) ? parseInt(version[1], 10) : -1
        template: template
      };
    })
    .reduce((highestVersionedTemplate, template) =>
      (template.version > highestVersionedTemplate) ? template : highestVersionedTemplate,
      { version: -1, template: null }
    );
    // In contrast with an array max/min function suporting a key extracting function
    // (e.g. like in Python: https://docs.python.org/3.8/library/functions.html#max)
    previousChainOfFunctions.maxOrDefault(template => template.version, { version: -1, template: null });
}

Stylistic considerations

  • Your variable names are self-explanatory and consistently written, that's fine!

  • Use consistent braces in blocks (function, if, many more), e.g. function { or function[newline]{, but do not mix both.

  • Always use braces around if, else, for, while.

\$\endgroup\$
  • \$\begingroup\$ @Treycos I've just fixed a critical bug: the version strings now get parsed as numbers. Intended numerical comparisons on strings do not work as expected: "2" > "10" is true. \$\endgroup\$ – ComFreek Mar 17 '18 at 7:38

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.