1
\$\begingroup\$

I wrote this method in order to generate a sequence number based on a specific pattern:

public string LatestNumber(Guid id = default(Guid))
{
    var number = "";
    if (id == Guid.Empty)
    {
        var items = myList.ToList();
        var lastNumber = items.Any() ? items[items.Count - 1].Number : "";

        var intitialNumber = "Number#000001";
        if (!items.Any())
            number = intitialNumber;
        else
        {
            var nextNumber = int.Parse(lastNumber.Split('#')[1]) + 1;
            number = $"Number#{nextNumber:D6}";
        }
    }
    else
    {
        number = myList.FirstOrDefault(p => p.Id == id).Number;
    }

    return number;
}

This code works as expected in production but I wonder if there's a better way to make this code clean or more testable. What do you think?

\$\endgroup\$
  • \$\begingroup\$ I get this is code review but what is requirements here? This seems like an odd design? \$\endgroup\$ – paparazzo Jan 1 '17 at 21:22
2
\$\begingroup\$

There are a couple of things you could do differently.

  • number - this variable is not necessary, you can return anytime
  • myList - if this is already a list then you don't need to copy it
  • lastNumber - this is initialized too early. It should be placed inside else
  • if - don't ommit the {}
  • !items.Any() and items.Any() - you don't need to check it twice and even with different results
  • : "" - this value is never used because you don't enter the first else if myList.Any() - in this case you use .Number and the only place where you use the resulting lastNumber is the first else
  • [1] - you should name this index
  • intitialNumber - define this as a constant

number = myList.FirstOrDefault(p => p.Id == id).Number;

You know that this might blow? If the list is empty then the Number cannot be read. You need either C# 6 for this or one more line of code.


Putting everything together gives:

const string intitialNumber = "Number#000001";

if (id == Guid.Empty)
{
    if(myList.Any())
    {
        const int someMagicValue = 1;
        var lastNumber = myList.Last().Number; // Last is O(1) for IList
        var nextNumber = int.Parse(lastNumber.Split('#')[someMagicValue]) + 1;
        return = $"Number#{nextNumber:D6}";
    }
    else
    {
        return intitialNumber;
    }
}
else
{
    var first = myList.FirstOrDefault(p => p.Id == id);
    return first != null ? first.Number : intitialNumber;

    // C# 6
    // myList.FirstOrDefault(p => p.Id == id)?.Number ?? intitialNumber;
}
\$\endgroup\$
  • \$\begingroup\$ initialNumber is not defined \$\endgroup\$ – paparazzo Jan 1 '17 at 18:23
  • \$\begingroup\$ No it is not obvious to me. In the OP code it is var intitialNumber = "Number#000001"; No need for the snark. I will remember to just stay off your posts in the future. \$\endgroup\$ – paparazzo Jan 1 '17 at 18:31
  • \$\begingroup\$ @Paparazzi haha, my bad, I somehow overlooked that this one was missing \$\endgroup\$ – t3chb0t Jan 1 '17 at 18:33
  • \$\begingroup\$ Not the first time you have snapped at me when my comment was valid. \$\endgroup\$ – paparazzo Jan 1 '17 at 18:39
  • \$\begingroup\$ @Paparazzi that's true and I apologize for this. I'll try to make an effort so there is no next time ;-) \$\endgroup\$ – t3chb0t Jan 1 '17 at 18:43

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.