0

I'm not sure whether this qualifies as code smell, or if there may be some better way of performing the same task, but, basically, I want to format a 10 digit string, using C#, and have the following options...

Option 1

public string FormatNumber(string value)
{
    int i;
    if(int.TryParse(value, out i)) return i.ToString("000 000 0000");
    else throw new InvalidArgumentException("'" + value + "' is not a valid argument.");
}

Option 2

public string FormatNumber(string value)
{
    int i;
    if(int.TryParse(value, out i)) return value.Insert(6," ").Insert(3," ");
    else throw new InvalidArgumentException("'" + value + "' is not a valid argument.");
}

Both achieve the correct result, but they both feel a little ... smelly.

Am I being too pedantic, or is their a clear winner?

  • 1
    Is this 10 digit string really a number, or is it a US phone number? – Dan Pichelman Aug 21 '18 at 18:34
  • 2
    What is the point of using TryParse if you're just going to throw on the parse error anyway? – Robert Harvey Aug 21 '18 at 18:43
  • @DanPichelman - No, the numbers I'll be processing are basically already strings. What I was after was the quickest and most obvious way of ensuring that they are a number and have no rogue data in there. The strings themselves are modulo 11 numbers (i.e. a 9 digit number with a checksum on the end). – Paul Aug 22 '18 at 7:25
  • @RobertHarvey - The numbers being checked (as per the comment to Dan, above) are all modulo 11 numbers, and this is just a phase in the parsing. The exception is nothing more than filler - I'm using it for an alternative else to shorten (in SE) what it does in the real world. – Paul Aug 22 '18 at 7:28
  • So you've created artificial conditions for which you want natural opinions. – Robert Harvey Aug 22 '18 at 14:53
8

Personally I think the first one looks more readable. It make really clear what's the expected output. The second one may take some more effort.

By the way, what happens if the number does not fit the 10 digits ? And what if the number is negative ?

|improve this answer|||||
  • Hi Matteo, thanks for your reply. That's what I suspected myself. My main issue is down to speed - I may have many numbers to process using this function, so I want it as quick as possible. I haven't benchmarked them yet, but that will be on the cards. As for negative numbers, I need to test that, and numbers over 10 digits isn't a problem - the input method caps the number of characters. – Paul Aug 22 '18 at 7:33
1

One trick you may not have thought of is to give the special number its own class, with implicit conversion, which makes it easier to use.

The special class provides a natural extension point if in the future you need to add comparison operations, custom hash code, custom serialization, etc.

Also note-- an int can't hold all ten digit numbers, you'd need a long. Also, you should validate against negative numbers.

public class MyNumber
{
    protected readonly long _value;

    public MyNumber(string source)
    {
        if (!long.TryParse(source, out _value)) throw new ArgumentException("String cannot be converted to a MyNumber");
        if (_value <=0) throw new ArgumentException("MyNumber must be at least 1.");
        if (_value > 9999999999) throw new ArgumentException("MyNumber must be ten digits or fewer.");
    }

    static public implicit operator long(MyNumber source)
    {
        return source.Value;
    }

    static public implicit operator MyNumber(string source)
    {
        return new MyNumber(source);
    }

    public long Value { get { return _value; } }

    public override string ToString()
    {
        return _value.ToString("000 000 0000");
    }
}

Some ways you could use it:

MyNumber n = "12345678";
Console.WriteLine("Your magic number is {0}", n);

Console.WriteLine("Another number is {0}", (MyNumber)"9876543");

Output:

Your magic number is 001 234 5678
Another number is 000 987 6543

Link to DotNetFiddle

|improve this answer|||||
  • Thanks for your post, John. I have, indeed, looked at such a class before, based on what the number represents. However, the (how can I describe it?...) 'policy' behind the number changes too frequently, so it simply remains a (set of) data item(s) [sometimes the number may have attributes attached to it depending on who makes decisions considerably further up the tree]. – Paul Aug 22 '18 at 7:39
  • Nope, downvote, why so eager to conceal the parsing logic as some 'magic'? – JᴀʏMᴇᴇ Aug 22 '18 at 8:34

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.