0
\$\begingroup\$

I've a list of strings and a list of regular expressions. I need to categorize each string to get what type of string it is based on regex.

This is my code:

const caseARegex = /^[\s]*[0-9]{4}[\s]*-[\s]*[0-9]{4}[\s]*$/gi;
const caseBRegex = /^[\s]*[0-9]{4}[\s]*-[\s]*$/gi;
const caseCRegex_1 = /^[\s]*-[\s]*[0-9]{4}[\s]*$/gi;
const caseCRegex_2 = /^(until)[\s]*[0-9]{4}[\s]*$/gi;
const caseDRegex_1 = /^(by|before)[\s]*[0-9]{4}[\s]*[-]*[\s]*$/gi;
const caseDRegex_2 = /^[\s]*-[\s]*still[\s]*in[\s]*[0-9]{4}[\s]*$/gi;
const caseERegex = /^(by|probably)[\s]*[0-9]{4}[\s]*-[\s]*[0-9]{4}[\s]*$/gi;
const caseFRegex = /^[\s]*[0-9]{4}[\s]*-[\s]*(still in|probably)[\s]*[0-9]{4}[\s]*$/gi;

const labelsWithCase = [
  "1968 - 1970",
  "1868 -",
  "- 1868",
  "until 1868",
  "by 1968",
  "by 1968 -",
  "before 1868",
  "- still in 1868",
  "by 1868 - 1870",
  "probably 1868 - 1870",
  "1868 - still in 1870",
  "1868 - probably 1870"
].map((l) => computeInfo(l));
console.log(labelsWithCase);

function computeInfo(label) {
  if (caseARegex.test(label)) {
    return { case: "a", start: 0, end: 0 };
  } else if (caseBRegex.test(label)) {
    return { case: "b", start: 0, end: 0 };
  } else if (caseCRegex_1.test(label) || caseCRegex_2.test(label)) {
    return { case: "c", start: 0, end: 0 };
  } else if (caseDRegex_1.test(label) || caseDRegex_2.test(label)) {
    return { case: "d", start: 0, end: 0 };
  } else if (caseERegex.test(label)) {
    return { case: "e", start: 0, end: 0 };
  } else if (caseFRegex.test(label)) {
    return { case: "f", start: 0, end: 0 };
  } else {
    console.error(`Something goes wrong.`);
    return {};
  }
}

Is there a better way to do that? Consider that I could have more and more strings and regex.

\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

What I think is wrong in your code :

  • Instead of assigning one variable name per Regex, put them all in an Array, or an Object. An Object makes sense here, because you want to associate regexes to letters.
  • if() else if () else if() is cumbersome to read and maintain. Instead, it's easier to loop over your regexes, and return the result whenever you find a match. Listing the regexes one by one require 2N lines of code (50 lines for 25 regexes), whereas looping is only the same two or three lines, no matter how many regexes you're going through.

I'd do it this way. Create a regex store, in which each letter can be associated with one or multiple regexes. Then, apply array.some() to detect if one of the regexes is a match, and return the corresponding letter.

const regexesStore = {
  a: [/^[\s]*[0-9]{4}[\s]*-[\s]*[0-9]{4}[\s]*$/gi],
  b: [/^[\s]*[0-9]{4}[\s]*-[\s]*$/gi],
  c: [/^[\s]*-[\s]*[0-9]{4}[\s]*$/gi, /^(until)[\s]*[0-9]{4}[\s]*$/gi],
  d: [/^(by|before)[\s]*[0-9]{4}[\s]*[-]*[\s]*$/gi, /^[\s]*-[\s]*still[\s]*in[\s]*[0-9]{4}[\s]*$/gi],
  e: [/^(by|probably)[\s]*[0-9]{4}[\s]*-[\s]*[0-9]{4}[\s]*$/gi],
  f: [/^[\s]*[0-9]{4}[\s]*-[\s]*(still in|probably)[\s]*[0-9]{4}[\s]*$/gi],
}

const computeInfo = label => {
  for (let [letter, regexes] of Object.entries(regexesStore)) {
    if (regexes.some(regex => regex.test(label))) return { case: letter, start: 0, end: 0 };
  }
  return {};
}

const labelsWithCase = ["1968 - 1970", "1868 -", "- 1868", "until 1868", "by 1968", "by 1968 -", "before 1868", "- still in 1868", "by 1868 - 1870", "probably 1868 - 1870", "1868 - still in 1870", "1868 - probably 1870"].map(computeInfo);

console.log(labelsWithCase);

\$\endgroup\$
4
  • \$\begingroup\$ You have presented an alternative solution, but haven't reviewed the code. Please edit to show what aspects of the question code prompted you to write this version, and in what ways it's an improvement over the original. It may be worth (re-)reading How to Answer. \$\endgroup\$ Oct 12, 2021 at 7:57
  • \$\begingroup\$ OK thanks, will read and try to improve. I'm very much used to Stackoverflow, not yet to Codereview. \$\endgroup\$ Oct 12, 2021 at 8:02
  • \$\begingroup\$ That means that you might benefit from A guide to Code Review for Stack Overflow users. That's quite a good summary of the differences between the sites. Your answer already looks much better - thank you for improving it. \$\endgroup\$ Oct 12, 2021 at 8:42
  • \$\begingroup\$ You're full of resources :) \$\endgroup\$ Oct 12, 2021 at 8:43
0
\$\begingroup\$

You want a code that can scale as the incoming data increases... First of all, don't make a mess of if and else statements. It is painful to read and to code.

There are few browsers which don't support the let and of keywords, it really is a pain but one such example is Opera Mini.

Who uses Opera Mini? First of all, even if there are 2 users of Opera Mini in the world, we as web-developers need make the web accessible to them. But there are still 260 million users present for Opera Mini, as per Google and Opera themselves.

And to take it even a step further, people still use internet explorer... I am stupefied too.

So lets make web accessible for them... I hate this, but we have no choice its about moral code.

What's wrong with your code

First: Too many if and else statements.

Second: Non scalable structure... You must not declare so many regex expressions globally.

Finally: This is not a malpractice but please don't return empty objects.

Also please try not to map a list by overwriting its contents, you won't be able to recover it once it is gone. This might lead to few unknown bugs later along the lines.

What you should do?

Using your logic you need a list of regex expressions and a list of labels... Then you want to map the labels with a matching regex case.

Here is what I came up with...

// --- This is your regex list which follows the format given below --- //
// -------- regexStockList = { case: name, regex: expression } -------- //

const regexStockList = [];

// ----------- This is your label list ------------ //
// --- Here you can add labels you want to test --- //
const labels = [];

// --- Here is a function to add new regex expressions --- //
function addNewRegexStock(caseLabel, expression) {
  regexStockList.push({
    case: caseLabel,
    regex: expression
  });
}

// --- This function can be used to add new labels --- //
function addNewLabel(label) {
  labels.push(label);
}

// --- Your original mapping function --- //
function computeInfo(label) {
  for (var stockNumber in regexStockList) {
    if (regexStockList[stockNumber].regex.test(label)) {
      return {
        case: regexStockList[stockNumber].case,
        label: label,
        start: 0,
        end: 0,
      };
    }
  }
  return {
    case: "no match",
    label: label,
    start: 0,
    end: 0,
  };
}

// --- Adding your regex expression here --- //
// ---- You can use a for loop for this ---- //
addNewRegexStock("a", /^[\s]*[0-9]{4}[\s]*-[\s]*[0-9]{4}[\s]*$/gi);
addNewRegexStock("b", /^[\s]*[0-9]{4}[\s]*-[\s]*$/gi);
addNewRegexStock("c-1", /^[\s]*-[\s]*[0-9]{4}[\s]*$/gi);
addNewRegexStock("c-2", /^(until)[\s]*[0-9]{4}[\s]*$/gi);
addNewRegexStock("d-1", /^(by|before)[\s]*[0-9]{4}[\s]*[-]*[\s]*$/gi);
addNewRegexStock("d-2", /^[\s]*-[\s]*still[\s]*in[\s]*[0-9]{4}[\s]*$/gi);
addNewRegexStock("e", /^(by|probably)[\s]*[0-9]{4}[\s]*-[\s]*[0-9]{4}[\s]*$/gi);
addNewRegexStock("f", /^[\s]*[0-9]{4}[\s]*-[\s]*(still in|probably)[\s]*[0-9]{4}[\s]*$/gi);


// ----- Adding the label ----- //
// --- Use a for loop later --- //
addNewLabel("1968 - 1970");
addNewLabel("1868 -");
addNewLabel("- 1868");
addNewLabel("until 1868");
addNewLabel("by 1968");
addNewLabel("by 1968 -");
addNewLabel("before 1868");
addNewLabel("- still in 1868");
addNewLabel("by 1868 - 1870");
addNewLabel("probably 1868 - 1870");
addNewLabel("1868 - still in 1870");
addNewLabel("1868 - probably 1870");


// --- With this function you can get a map of your label cases --- //
function getMappedLabelsWithCases() {
  var labelWithCases = []
  for (var labelNumber in labels) {
    labelWithCases.push(computeInfo(labels[labelNumber]));
  }
  return labelWithCases;
}

console.log(getMappedLabelsWithCases())

As you can see... This code will scale pretty well for your case

I also think you got support for Opera Mini and internet explorer with this code.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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