HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMajor

PIN class with SOLID principles

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
principleswithsolidclasspin

Problem

I wrote this Pin class as a part of a project to learn SOLID. What do you think about it and what can be made better?

Interface

public interface IAuthorization
    {
        bool Verify(object obj);
        void Change(object obj);
    }


Class

public class Pin : IAuthorization
    {
        private int _pin;

        public Pin(object pin)
        {
            _pin = ValidPin(pin);
        }

        public void Change (object pin)
        {
            _pin = ValidPin(pin);
        }

        public bool Verify (object pin)
        {
            return (_pin == ValidPin(pin));
        }

        private int ValidPin(object pin)
        {
            if (pin == null)
            {
                throw new ArgumentNullException("pin", "Pin must be exactly 4 digits long integer");
            }

            if (pin.ToString().Length != 4)
            {
                throw new ArgumentException("Pin must be exactly 4 digits long integer", "pin");
            }

            return Convert.ToInt32(pin);
        }
    }

Solution

The D says "depend on abstractions, not concrete types" - that doesn't mean to box your value types into object!

In fact, this:

private int _pin;


Contradicts this:

if (pin.ToString().Length != 4)
{
    throw new ArgumentException("Pin must be exactly 4 digits long integer", "pin");
}


If your pin is meant to work like a string, then it should be stored as a string. If my pin is 0072, I have a 4-digit pin, but you're storing it as 72 and throwing an ArgumentException at me.

Exposing object in your public API is horrible practice - nobody knows what you're expecting to receive! Worse, there's no XML documentation on your public members, so the only way to find out, is to dig into your code.

Come to think about it, with respect to the I of SOLID, I think your interface should look like this:

public interface IPinAuthorization
{
    bool Validate(SecureString pin);
}


And a PIN shouldn't be that easy to modify. Consider making the type immutable, and asking for the current PIN before accepting a new one:

public interface IModifiablePin
{
    IPinAuthorization Modify(SecureString current, SecureString @new);
}


You could have a more secure PinAuthorization implementation like this:

public class PinAuthorization : IPinAuthorization, IModifiablePin, IDisposable
{
    private readonly SecureString _pin;

    public PinAuthorization(SecureString pin)
    {
        ValidateInternal(pin); // see comments below
        _pin = pin;
    }

    private void ValidateInternal(SecureString pin)
    {
        if (pin.Length != 4)
        {
            throw new ArgumentException("PIN must be 4 digits.", pin);
        }

        // further validation logic would break the security of the string.
        // this is an indication that it's not this type's responsibility
        // to know how to validate itself.
    }

    public bool Validate(SecureString pin)
    {
        return pin == _pin;
    }

    public IPinAuthorization Change(SecureString current, SecureString @new)
    {
        if (_pin != current)
        {
            throw new ArgumentException("Invalid PIN.", current);
        }

        return new PinAuthorization(@new);
    }

    public void Dispose()
    {
        _pin.Dispose();
    }
}


Now, if you want to implement some FingerprintAuthorization, you can make it implement IPinAuthorization and validate against a SecureString-encoded finger print (however you want to do that), and the type will not be implementing IModifiablePin, since you don't change your finger prints. Or do you? Well, at least you have the flexibility of doing either. At a glance though, it seems the class has so little responsibility, that it possibly wouldn't even need to be reimplemented or adapted in any way.

Code Snippets

private int _pin;
if (pin.ToString().Length != 4)
{
    throw new ArgumentException("Pin must be exactly 4 digits long integer", "pin");
}
public interface IPinAuthorization
{
    bool Validate(SecureString pin);
}
public interface IModifiablePin
{
    IPinAuthorization Modify(SecureString current, SecureString @new);
}
public class PinAuthorization : IPinAuthorization, IModifiablePin, IDisposable
{
    private readonly SecureString _pin;

    public PinAuthorization(SecureString pin)
    {
        ValidateInternal(pin); // see comments below
        _pin = pin;
    }

    private void ValidateInternal(SecureString pin)
    {
        if (pin.Length != 4)
        {
            throw new ArgumentException("PIN must be 4 digits.", pin);
        }

        // further validation logic would break the security of the string.
        // this is an indication that it's not this type's responsibility
        // to know how to validate itself.
    }

    public bool Validate(SecureString pin)
    {
        return pin == _pin;
    }

    public IPinAuthorization Change(SecureString current, SecureString @new)
    {
        if (_pin != current)
        {
            throw new ArgumentException("Invalid PIN.", current);
        }

        return new PinAuthorization(@new);
    }

    public void Dispose()
    {
        _pin.Dispose();
    }
}

Context

StackExchange Code Review Q#118580, answer score: 27

Revisions (0)

No revisions yet.