4
\$\begingroup\$

What do you think about quality of code in this method?

It should:

  • Generate new key based on keys in array
  • The key should start with "TG"
  • Code should find the max number in array keys with "TG-" and increment this number
  • It should have 7 digits
  • No need to check what if there is a key TG9999999, and what if array is empty or doesn't have keys "TG-"

private static string[] allKeys = new string[]
{
    "TG0000006",
    "TG0000026",
    "TG0000086",
    "TG0000796",
    "TG0023106",
    "LG0004406",
    "MS0000796",
    "TT0023106",
    "LK0004406",
};

static string GenerateKey()
{
    var keys = allKeys.Where(k => k.StartsWith("TG"));
    List<int> nums = new List<int>();
    foreach (var key in keys.Select(str => Regex.Match(str, @"\d+").Value))
    {
        int num;
        if (int.TryParse(key, out num))
            nums.Add(num);
    }
    string newNum = (nums.Max() + 1).ToString();
    return "TG" + new string('0', 7 - newNum.Length) + newNum;
}
\$\endgroup\$
5
\$\begingroup\$
  • If you want only the numbers 0..9 to be matched by the Regex and you don't care about for instance some Eastern Arabic numerals like ´٠١٢٣٤٥٦٧٨٩, then you should use an expression like [0-9]+.

  • because by using either the d+ or [0-9]+ expression you already made sure that the var key is a number. Because this number has at the maximum 7 digits, you can simply call Parse() instead of TryParse() which eleminates the usage of the int num.

  • please add always braces {} although they might be optional. This will make your code less error prone.

  • the return statement

    return "TG" + new string('0', 7 - newNum.Length) + newNum;  
    

    can be simplified by using the PadLeft() method like so

    return "TG" +  newNum.PadLeft(7,'0');  
    
  • personally I would just have a if clase for checking the highest number instead of the List<int> together with the call to Max().

Putting this all together leads to

static string GenerateKey()
{
    int max = int.MinValue;
    var keys = allKeys.Where(k => k.StartsWith("TG"));
    foreach (var key in keys.Select(str => Regex.Match(str, @"[0-9]+").Value))
    {
        int num = int.Parse(key);
        if (num > max)
        {
            max = num;
        }
    }

    return "TG" + (max + 1).ToString().PadLeft(7, '0');
}
\$\endgroup\$

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.