Why do I get NullReferenceException in my factory pattern implementation, c#?

Question

Main class:

public class ClP_Login
{
    private Form vrcView;
    private I_Repository<I_Identifiable> vrcRepository = null;

    public ClP_Login(Form vrpView)
    {
        vrcView = vrpView;
        SetTheme();
    }

    private void SetTheme()
    {
        if(vrcView !=null)
        vrcView.BackColor = Cl_BaseColor.StandardBackground;
    }

    public void CreateNewUser()
    {
        ClE_User test = new ClE_User();
        test.Name = "test name";
        test.Password = "";
        Cl_RepositoryFactory vrlFactory = new Cl_RepositoryFactory();
        vrcRepository = vrlFactory.CreateRepository(E_Repositories.User);
        vrcRepository.Add(test);
    }
}

Cl_RepositoryFactory class:

public class Cl_RepositoryFactory
{
    public virtual I_Repository<I_Identifiable> CreateRepository(E_Repositories vrpRepository)
    {
        I_Repository<I_Identifiable> vrlRepository = null;
        switch (vrpRepository)
        {
            case E_Repositories.User:
                vrlRepository = new Cl_UserRepository() as I_Repository<I_Identifiable>;
                break;
        }
        return vrlRepository;
    }
}

Enum E_Repositories:

public enum E_Repositories
{
    User
}

I_Identifiable Interface:

public interface I_Identifiable
{
    int Id { get; set; }
}

I_Repository Interface:

public interface I_Repository<T>
{
    T GetById(Guid id);
    T GetByQuery(Queue query);
    void Add(T item);
    void Remove(T item);
    void Update(T item);
}

Cl_UserRepository class:

public class Cl_UserRepository : I_Repository<ClE_User>
{
    public void Add(ClE_User item)
    {
        MessageBox.Show("Created new User");
    }

    public ClE_User GetById(Guid id)
    {
        throw new NotImplementedException();
    }

    public ClE_User GetByQuery(Queue query)
    {
        throw new NotImplementedException();
    }

    public void Remove(ClE_User item)
    {
        throw new NotImplementedException();
    }

    public void Update(ClE_User item)
    {
        throw new NotImplementedException();
    }
}

And ClE_User class:

public class ClE_User : I_Identifiable
{
    public int Id { get; set; }
    public string Name { get; set; }
    public string Password { get; set; }
}

The question is, why do I get null reference exception using vrcRepository? vrlFactory.CreateRepository(E_Repositories.User); return null and I don't have any idea why, please help

Because you are trying to cast `Cl_UserRepository` to be a `I_Repository` and it's not the same thing so the `... as ...` bit returns `null`. — DavidG, Jul 22 '17 at 19:04
Also, all the type prefixes you have here (`Cl_`, `I_` etc.) make your code horribly unreadable. — DavidG, Jul 22 '17 at 19:04
It would work if the generic type `T` on `I_Repository` where co-variant, but it isn't and cannot be because `T` is used as input to the interface. If that cast did work you could pass something that wasn't a `ClE_User` to any of the methods `Add`, `Remove`, and `Update`. — juharr, Jul 22 '17 at 19:07
@DavidG sorry for unreadable code, I just try to apply to the standards adopted in my company, can't do anything with that. — mietghar, Jul 22 '17 at 19:14
Oh wow, then your company standards really suck! Feel free to tell them I said that :) — DavidG, Jul 22 '17 at 19:16
juharr , DavidG and opewix were right, I've changed constructon of dependencies in interfaces and now it works, many thanks for help lads — mietghar, Jul 22 '17 at 19:38

score 0 · Answer 1 · answered Jul 22 '17 at 19:05

In CreateRepository method try to remove casting statement as I_Repository<I_Identifiable>. If your code will not compile, that will mean Cl_UserRepository is not compatible with I_Repository<I_Identifiable>.

Otherwise everyting is correct with CreateRepository method

Dialecticus · Answer 2 · 2017-07-22T19:39:38.407

ClE_User inherits from I_Identifiable, but I_Repository<ClE_User> does not inherit from I_Repository<I_Identifiable>. Those are different interfaces as far as C# is concerned.

To elaborate more, you have I_Repository<I_Identifiable> vrcRepository which should in theory take I_Repository of any I_Identifiable kind. So let's say you initialize this member to some other, for instance I_Repository<ClE_SomethingOtherThanUser>. But then you call vrcRepository.Add(test). That's not going to work, with test being ClE_User.

Now, first remove the as I_Repository<I_Identifiable> part, and then to make it compile make I_Repository just a plain dumb non-generic interface, whose methods take I_Identifiable parameter or return I_Identifiable value. This may not be what you wanted, but it will compile.

Morten Bork · Answer 3 · 2017-07-22T20:13:37.563

-1

EDIT

I realize that the enum will trigger. You are right

new Cl_UserRepository() as I_Repository

CL_UserRepository has to implement the interface you are trying to return, and then you don't need to type cast it at all. Sorry! I owe you a case of beer.

edited Jul 22 '17 at 20:13

answered Jul 22 '17 at 19:05

Morten Bork

1,413
11
23

1

No, OP is passing in `E_Repositories.User` which is one of the enum values in the switch. The problem is the `as ...` cast. – DavidG Jul 22 '17 at 19:06
The rest of your answer is also a bad recommendation - you shouldn't *always* have a default case, there's many good reason not to do that. And you owe OP a case of beer. – DavidG Jul 22 '17 at 19:09
No there is not. Unless the specific switch case statement sets a boolean, in which case, you should just use if else, you should always send a default handle to return an error, unless you can set a default value for your return. Closing of a switch without a default is bad practice in every single example I have ever seen. As a minimum, you would always have a better log of what happened, because your forced yourself to consider what happens, if, going towards inifinity and everything can happen, what should happen. – Morten Bork Jul 22 '17 at 19:12
Not going to argue with you about the default case, but take a read of [this](https://stackoverflow.com/questions/4649423/should-switch-statements-always-contain-a-default-clause) for a longer discussion. It's absolutely *not* bad practice. As for the rest of your comment, I really don't understand what you're saying. – DavidG Jul 22 '17 at 19:21
I took a look, and near all advice there says, Always use a default. Because... its good practice.... – Morten Bork Jul 22 '17 at 20:13
"Always" is a dangerous word to use, you have conveniently ignored all the advice on there that suggests when it is bad to do have a default. Like I said, there are many occasions when it is OK. – DavidG Jul 23 '17 at 01:03
And Like I said, no, there isn't. provide is with some examples, where it is a good idea, not to have a default, and specifically, where it would be bad to have a default. – Morten Bork Jul 23 '17 at 09:03
Did you even read all the answer there? [This](https://stackoverflow.com/a/5269018/1663001) and [this](https://stackoverflow.com/a/30944687/1663001). If you force a default it means that a static analyser would have trouble figuring out if you had missed an enum case. All I'm saying is that you cannot, with absolute certainty say **always**. – DavidG Jul 23 '17 at 10:28
I read them all, and they are all refuted. And allowing the compiler to catch an enum bug, does not cancel out that if your default is set, you still catch it at runtime, where it is more important to catch it. You simply should always use a default value, because it forces you to handle the error cases as well – Morten Bork Jul 23 '17 at 18:00
You're starting to sound like a flat earther, good luck with your ideologies there, I'll stick to the real world. – DavidG Jul 23 '17 at 20:24
Wow. You are the ignoring evidence, statement, and examples. And im the fact earther. Have fun learning. – Morten Bork Jul 24 '17 at 04:49

Why do I get NullReferenceException in my factory pattern implementation, c#?

3 Answers3