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;
}
TheCoffeeCup
9,5164 gold badges38 silver badges96 bronze badges
asked Oct 8, 2015 at 18:58
\$\endgroup\$

1 Answer 1

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');
}
janos
113k15 gold badges154 silver badges396 bronze badges
answered Oct 9, 2015 at 8:29
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.