Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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);
        }
    }
share|improve this question
up vote 26 down vote accepted

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.

share|improve this answer

This is pretty good-looking code, except for the fact that every argument is passed as an object. Let's look at what happens when I pass a custom type MyType declared in the namespace MyNamespace to ValidPin(object).

First, if (pin == null) succeeds, unless the value is null.

Next, we reach if (pin.ToString().Length != 4). What is the value of pin.ToString()? Do we really know? If I have not overridden ToString(), it will return MyNamespace.MyType. If I have overridden ToString(), it could be anything.

Finally, we reach return Convert.ToInt32(pin);. What happens here? We get an InvalidCastException() that crashes our code.

Because you are working with a value intended to be an int, you should take input as a string. Problems that could arise here are if the string "234r" is passed, which will cause Convert.ToInt32 to throw. You should probably use int.TryParse(string, out int) to ensure that the parse succeeds without crashing your code.

share|improve this answer
    
So do you mean that I should take int parameter in ValidPin method, or should I handle it in other way? For example, by taking int rather than object in all the methodes? – Verbum Feb 1 at 22:25
1  
Yes, you should take int rather than object in all your methods. It is very rare that you will need to explicitly use object anywhere. – Hosch250 Feb 1 at 22:30
    
Won't it brake my interface implementation? – Verbum Feb 1 at 22:33
    
You should update your interface implementation as well. And my code has a bug - you should take a string because of potential leading 0's, as Mat's Mug says. – Hosch250 Feb 2 at 0:04

I just saw that this was hard to unpack but:

You validation seems to be splitted. In the Verify-method() you are comparing the converted PIN to the given PIN. During change you omit this check. Together with the missing type safty this is all a little bit confusing.

So I suggest following:

Introduce type saftyness (change object to a more concrete type)

Consolidate all checks and throw proper exceptions.

The Verify()-method is publishing the similar information over two channels. First channel is the return value. Second channel is throwing exceptions through ValidPin()-method. I suggest one of these two possibilities:

  1. Do not return any value (void) from Verify() and only throw exceptions
  2. Catch exceptions in Verify and return false, true if everything is fine
share|improve this answer
    
You have a point with catching exceptions, but if I change type won't it brake my interface implementation? – Verbum Feb 1 at 22:58
    
@Verbum breaking the implementation would be the least of my concerns with an interface that exposes object in its API. – Mat's Mug Feb 1 at 23:05
1  
@Verbum you're being blinded by the principles. Exposing System.Object in a public API is bad practice, period. It's a last resort thing that you do when you're dealing with nasty COM interop code, not when you're designing a brand new system from scratch in 100% managed code. OCP principle has very little, if anything to do with what types you're using. – Mat's Mug Feb 1 at 23:13
1  
That's the thing: you don't change it. You make another class that implements the same interface. Sure, an object makes it easy for you as the framework designer... but makes an awful experience for your client code - it's a matter of balance. – Mat's Mug Feb 1 at 23:24
1  
The problem with object in a public API is if you are serious you only have the possibility to use it as a client payload support that is neutral to the framework as there is not at least one further assertion. The framework itself may have no clue what to do with it. If the signature says "Object" I expect to pass anything I want. But if you think twice you want to have at least the parameter connected to your framework domain by implementing something like a marker interface. – oopexpert Feb 2 at 0:54

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.