views:

201

answers:

2

Please educate me if I have code smell (violating SOLID principles, etc) Also how can I make use of IoC to make dependency injection better? Any help, tips, feedback is REALLY appreciated :)

EDIT: Should all dependencies be injected from the constructor in the form of strings or should they be interfaces, if possible? For example, Login concrete class. It COULD take IAuthentication object rather than bunch of strings.

public interface ILogin
{
    IUserSession GetUserSession();
}

public interface IAuthentication
{
    bool IsValidUser();
}

public interface IUserSession
{
    string GetUsername();
    bool IsValid();
}

internal class Login : ILogin
{
    private readonly string _username;
    private readonly string _password;
    private readonly IAuthentication _authentication;

    public Login(string username, string password)
    {
        _username = username;
        _password = password;
        _authentication = new Authentication(username, password);
    }

    public IUserSession GetUserSession()
    {
        return new UserSession(_username, _authentication.IsValidUser());
    }
}

internal class Authentication : IAuthentication
{
    private readonly string _username;
    private readonly string _password;

    public Authentication(string username, string password)
    {
        _username = username;
        _password = password;
    }

    public bool IsValidUser()
    {
 //authentication logic here
        return true;
    }
}

internal class UserSession : IUserSession
{
    private readonly string _username;
    private readonly bool _isValid;

    public UserSession(string username, bool isValid)
    {
        _username = username;
        _isValid = isValid;
    }

    public string GetUsername()
    {
        return _username;
    }

    public bool IsValid()
    {
        return _isValid;
    }
}
+3  A: 

If you want to be able to test your Login class, you'll likely need to pass in an IAuthentication to your Login constructor. you'd need this to fake the IsValidUser call.

However, as it stands, your Login class is so simple, there's nothing to test here. If your class really is that simple, leave it as is.

Judah Himango
Yep, thank you.
SP
+2  A: 

It's not clear if this is merely a simplified example of a larger pattern in your code, but, as written, the classes are probably too decoupled. There's a muddling of responsibilities here and the interfaces are extraneous as the classes are too simple to require mocking and there is not a use case presented for extensibility. You may be better served by starting more simply and postpone refactoring into smaller units until you've more clearly identified the necessary isolation boundaries.

Dan Bryant
Thanks, I did start to refactor way too early. The use-case is REALLY simple (for now), so why bother. Thank you.
SP