6
\$\begingroup\$

In my program, I have special id numbers. They have a fixed length of 10 characters and may only contain digits and lowercase letter. I created my own data type for this. I want to be able to assign string values to it (with the implicit operator). When I do so, there should be automatically a verification if all conditions are met (the length and the constraints at the characters).

public class IdNumber
{
    private string idnumber;

    public static implicit operator IdNumber(string value)
    {
        if (value.Length == 10 && value.All(c => IsDigit(c) || IsLowercaseLetter(c)))
        {
            IdNumber m = new IdNumber();
            m.idnumber = value;
            return m;
        }
        else
        {
            throw new ArgumentException("id numer must have length 10 and must only have digits and lower case-letters");
        }
    }

    public override string ToString()
    {
        return idnumber;
    }

    public override int GetHashCode()
    {
        return idnumber.GetHashCode();
    }

    public override bool Equals(object obj)
    {
        if(obj is IdNumber other)
        {
            return this.idnumber == other.idnumber;
        }
        else
        {
            return false;
        }
    }

    public static bool operator ==(IdNumber m1, IdNumber m2)
    {
        if(m1 is null)
        {
            return m2 is null;
        }
        return m1.Equals(m2);
    }

    public static bool operator !=(IdNumber m1, IdNumber m2)
    {
        return !(m1 == m2);
    }

    private static bool IsDigit(char c)
    {
        return c >= '0' && c <= '9';
    }

    private static bool IsLowercaseLetter(char c)
    {
        return c >= 'a' && c <= 'z';
    }
}

Now I can use it via

IdNumber myId = "ab45jk23zz";
IdNumber otherId = "asfd" // throws exception

I have never used the implicit operator and I am also not sure about overriding the GetHashCode method (I want to use them as a key in a Dictionary, so it is important this works correctly). Hence I want to be sure I did not overlook anything. Of course, any other suggestions for improvement are welcome.

\$\endgroup\$
7
\$\begingroup\$

I will review your code from top to bottom.

private string idnumber;  

this should be named using camelCase casing and because you don't change its value you should make it readonly which means you can only assign a value to it inside the constructor or by initialising like e.g private string idnumber = "somevalue";. Because you set this value only inside the implicit operator you should add a private constructor which has a string as argument. This would then look like this

public class IdNumber
{
    private readonly string idNumber;
    private IdNumber(string idNumber)
    {
        this.idNumber = idNumber;
    }

Because the implicit operator is public you should validate the argument value wether it is null. It just looks better to throw an ArgumentNullException than to receive an NullReferenceException.

Instead of using value.All(c => IsDigit(c) || IsLowercaseLetter(c)) you could use a simple Regex to do the validation for you. This will shorten your code because the two methods IsDigit() and IsLowercaseLetter() won't be needed anymore.

The implicit operator would then look like this

public static implicit operator IdNumber(string value)
{
    if (value is null) { throw new ArgumentNullException(nameof(value)); }

    if (Regex.IsMatch(value, "[0-9a-z]{10}"))
    {
        return new IdNumber(value);
    }
    throw new ArgumentException("id numer must have length 10 and must only have digits and lower case-letters");
}

ToString() and GetHashCode() are both fine but in the Equals() method you can either just remove the else like so

public override bool Equals(object obj)
{
    if(obj is IdNumber other)
    {
        return this.idNumber == other.idNumber;
    }

    return false;
}  

or combine the if condition with the idNumber comparision like so

public override bool Equals(object obj)
{
    return (obj is IdNumber other) 
        && this.idNumber == other.idNumber;
}
|improve this answer|||||
\$\endgroup\$
4
\$\begingroup\$

Along with what @Heslacher mentioned.

There are some areas also can be improved.

The IsDigit and IsLowercaseLetter methods can be improved, as you're not handling any exceptions inside them, while there is an existing char.IsDigit and char.IsLower extensions that can be used instead, which is better handled. I'm sure using the .NET built-in extensions would be better option than recreating your own.

Another important point is that you're validating the input implicitly only! So, you either will have to copy & paste same condition on the constructor or any new method that will take an input to convert it to IdNumber or you'll stick with the implicit operator as a way of conversion. Both are not good practice.

What you want to do is to create a method to validate the input, which will help you to reuse it whenever needed.

example:

// Validate Id
public bool IsValid(string idNumber)
{
    return
        string.IsNullOrEmpty(idNumber) ? throw new ArgumentNullException(nameof(idNumber))
      : idNumber.Length == 10 && idNumber.All(x => char.IsDigit(x) || char.IsLower(x));
}

if the string is empty or null will throw ArgumentNullException otherwise will validate the input then return either true or false. you can see I used char.IsDigit(x) and char.IsLower(x) and that's because it has more validations at the back-scene, but most importantly, it checks the character Unicode which is important in your case as you always need a Numeric & a lower-case Latin (English) Letter which it's already covered in them.

With this method, you're eliminating the need of IsLowercaseLetter(char c) && IsDigit(char c) custom methods.

Now, you'll need to design your constructor first, before even thinking of your implicit operators (these can be saved for last).

we can define two constructors:

// store Id as string
private readonly string _idNumber;

// Auto Generate new id from Guid (just an example).
public IdNumber()
{
    _idNumber = Guid.NewGuid().ToString().Replace("-", "").Substring(0, 10).ToLower();
}

// Initiate the Id from the constructor.                
public IdNumber(string idNumber)
{
    if (string.IsNullOrEmpty(idNumber)) { throw new ArgumentNullException(nameof(idNumber)); }

    if (IsValid(idNumber))
    {
        _idNumber = idNumber;
    }
    else
    {
        throw new ArgumentException("id numer must have length 10 and must only have digits and lower case-letters");
    }
}

You added the ability to initiate a new Id via the constructors, either an auto generated Id, or a specific one, these two constructors will come in handy.

The ToString() should have null check, why ? it's a procedure were you expect the unexpected. (welcome to my world ;) ).

// Override the ToString(), and validate the string with string.IsNullOrEmpty()
public override string ToString() => string.IsNullOrEmpty(_idNumber) ? throw new ArgumentNullException(nameof(_idNumber)) : _idNumber;

Now, the equality operations in Equals() and == and also != in your code are treated the same (you're compare between objects values). Because of that, you can make Equals() main method of equality check, then just recall it in your other operators.

// Include conditions to cover null and string objects and use switch statement for better readability. (also easier to extend).
public override bool Equals(object obj)
{
        switch (obj)
        {
            case IdNumber other:
                return _idNumber == other._idNumber;
            case string other: 
                return string.IsNullOrEmpty(other) ? throw new ArgumentNullException(nameof(other)) : _idNumber == other;
            case null:
                throw new ArgumentNullException(nameof(obj));
            default:
                return false;
        }

}

// Call Equals()
public static bool operator ==(IdNumber m1, IdNumber m2) => m1.Equals(m2);

// Call Equals()
public static bool operator !=(IdNumber m1, IdNumber m2) => !m1.Equals(m2);

For the implicit operator, since we have already implemented our constructors and validation,(i told you it'll come in handy) we can do this:

// Cast string to IdNumber
public static implicit operator IdNumber(string value) => new IdNumber(value);

now, we can also add another implicit operator to do the reverse casting (from IdNumber to string), so we don't need to call ToString()

public static implicit operator string(IdNumber value) => value.ToString();

now you can cast back and forth between string and IdNumber directly.

we can now test them out :

// test 1 : initiate IdNumber with a string (without using the constructor)
IdNumber id = "ab45jk23zz";

// test 2 : Initiate a new IdNumber via constructor
var id2 = new IdNumber("ab45jk23zz");

// test 3 : Initiate a new random IdNumber
var id3 = new IdNumber();

// test 4 : convert IdNumber ToString()
var test = id.ToString();

// test 5 : cast IdNumber to string
var test2 = (string)id;

// test 6 : cast string to IdNumber
var test3 = (IdNumber)test;

// Between IdNumber objects 
var equalityTest = id.Equals(id2);

// Between IdNumber objects 
var equalityTest2 = id == id2;

// Between IdNumber & string 
var equalityTest3 = id.Equals("ab45jk23zz");

// Between IdNumber & string
var equalityTest4 = id == "ab45jk23zz";
|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ a string is an IEnumerable<char>, so this ToCharArray is not necessary. \$\endgroup\$ – Holger Dec 12 '19 at 21:33
  • \$\begingroup\$ @Holger thanks for mention that, I didn't noticed it. \$\endgroup\$ – iSR5 Dec 12 '19 at 23:14
  • \$\begingroup\$ Thanks a lot for your feedback. I have a question about overloading the == and != operators: When I do null == someIdNumber, this will throw a NullReferenceException because m1 will be null, won't it? \$\endgroup\$ – SomeBody Dec 13 '19 at 6:10
  • \$\begingroup\$ @SomeBody yes, you'll have to handle m1 && m2 nullity, or you can adjust it to m1 is null || m2 is null ? false : m1.Equals(m2); this way you'll get false when you compare it with nulls, and it won't throw NullReferenceException. \$\endgroup\$ – iSR5 Dec 13 '19 at 10:12

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.