1

I have an abstract base class "UserFeedback", and a couple of subclasses "A" and "B".

UserFeedback has a static "CreateInstance()" method.

I'd like to PREVENT callers from instantiating a new UserFeedback object directly:

public abstract class UserFeedback
{
    public string ComponentType { get; set; }
    public string UserName { get; set; }
    public string EMail { get; set; }

    protected UserFeedback(string componentType)
    {
        ComponentType = componentType;
    }

    public static UserFeedback CreateInstance(string componentType)
    {
        switch (componentType)
        {
            case "A":
                return new A(componentType);  // Line 32
            case "B":
                return new B(componentType);  // Line 34
            default:
                throw new ArgumentException(String.Format("User Feedback Type: {0}", componentType));
        }
    }

    public abstract string GetTitle(string operation);
}

public class A : UserFeedback
{
    protected A(string componentType) : base(componentType)
    { }

    public override string GetTitle(string operation)
    {
        return "A Feedback";
    }
}

public class B : UserFeedback
{
    protected B(string componentType) : base(componentType)
    { }

    public override string GetTitle(string operation)
    {
        return "B Feedback";
    }
}

This gives me compile errors on Line 32 and 34:

Error   CS0122  'A.A(string)' is inaccessible due to its protection level

Is there any "simple" way to accomplish this in newer versions of C#/.NET?

Or should I just punt, and make the constructors for A() and B() "public"?

paulsm4
  • 114,292
  • 17
  • 138
  • 190
  • 1
    What do you mean you want to prevent callers from being able to create `UserFeedback` directly? That already exists by nature of the class being abstract. – David L Jun 15 '23 at 23:52
  • 1
    I would avoid using a `string` to specify a `Type` parameter and consider generics instead. C#7? You can define a generic interface with an abstract static factory method ... – Jeremy Lakeman Jun 16 '23 at 00:56
  • 1
    Why would you want someone to be able to make the derived type but not instantiate it? That's honestly the real question. – Erik Philips Jun 16 '23 at 01:13

4 Answers4

2

You could simply make A and B private Nested Types

public abstract class UserFeedback
{
    public string ComponentType { get; set; }
    public string UserName { get; set; }
    public string EMail { get; set; }

    protected UserFeedback(string componentType)
    {
        ComponentType = componentType;
    }

    public static UserFeedback CreateInstance(string componentType)
    {
        switch (componentType)
        {
            case "A":
                return new A(componentType);  // Line 32
            case "B":
                return new B(componentType);  // Line 34
            default:
                throw new ArgumentException(String.Format("User Feedback Type: {0}", componentType));
        }
    }

    public abstract string GetTitle(string operation);

    private class A : UserFeedback
    {
        public A(string componentType) : base(componentType)
        { }

        public override string GetTitle(string operation)
        {
            return "A Feedback";
        }
    }

    private class B : UserFeedback
    {
        public B(string componentType) : base(componentType)
        { }

        public override string GetTitle(string operation)
        {
            return "B Feedback";
        }
    }
}
David Browne - Microsoft
  • 80,331
  • 6
  • 39
  • 67
2

If you really don't want clients to create instances of the derived classes and only want them to go through the static CreateInstance method, a possible way is to make your derived classes private and have them in the same class as Userfeedback. If you do go this route, you won't be able to cast to those instances as they won't be visible

public abstract class UserFeedback
{
    public string ComponentType { get; set; }
    public string UserName { get; set; }
    public string EMail { get; set; }

    protected UserFeedback(string componentType)
    {
        ComponentType = componentType;
    }

    public static UserFeedback CreateInstance(string componentType)
    {
        switch (componentType)
        {
            case "A":
                return new A(componentType);  // Line 32
            case "B":
                return new B(componentType);  // Line 34
            default:
                throw new ArgumentException(String.Format("User Feedback Type: {0}", componentType));
        }
    }
    public abstract string GetTitle(string operation);
    private class A : UserFeedback
    {
        public A(string componentType) : base(componentType)
        { }

        public override string GetTitle(string operation)
        {
            return "A Feedback";
        }
    }

    private class B : UserFeedback
    {
        public B(string componentType) : base(componentType)
        { }

        public override string GetTitle(string operation)
        {
            return "B Feedback";
        }
    }
}
JohanP
  • 5,252
  • 2
  • 24
  • 34
1

In your code, it doesn't seem 'correct' that the sub-class A can receive any value even though only A is valid. Why is the constructor parameter even needed in the sub-classes? If it's not needed then why not make it public and let callers instantiate it?

If the above it true, let callers instantiate the A and B implementations directly. These objects have the information they need to instantiate themselves and shouldn't need a constructor parameter to tell them.

First let's remove the static CreateInstance method from the abstract class.

public abstract class UserFeedback
{
    public string ComponentType { get; set; }
    public string UserName { get; set; }
    public string EMail { get; set; }

    protected UserFeedback(string componentType)
    {
        ComponentType = componentType;
    }

    public abstract string GetTitle(string operation);
}

Next, in the sub-classes, make the constructor a public parameter-less constructor as we don't need anything outside to tell us what A or B is. They already know what they are, so we can just hard code that to the base constructor call.

This also has the added benefit of not having to update the base UserFeedback class when new sub-classes are implemented.

I've used nameof() in my example to get rid of the strings.

public class A : UserFeedback
{
    public A() : base(nameof(A))
    { }

    public override string GetTitle(string operation)
    {
        return "A Feedback";
    }
}

public class B : UserFeedback
{
    public B() : base(nameof(B))
    { }

    public override string GetTitle(string operation)
    {
        return "B Feedback";
    }
}
quaabaam
  • 1,808
  • 1
  • 7
  • 15
0

There is a different option. Sometimes when this comes up, nested classes is undesirable or otherwise awkward to achieve.

When it happens, protected internal constructor is your friend. Only your own assembly can call the base constructor, so only your assembly can instantiate it.

I'm used to seeing CreateInstance() do nice things like access one of several implementations based on #if or other exotic things. There's also a mechanism I don't fully understand by which a specific assembly can be loaded upon request that can access your protected external constructor and that assembly is blessed by yours at compile time so nobody else can do it.

Joshua
  • 40,822
  • 8
  • 72
  • 132