What's wrong with this code?

Question

3.00/5 (1 vote)

See more:

C#

internal class Base
{
   protected readonly string _field;

   public Base(string field)
   {
     _field=field;
   }

   public string Field
   {
     get {return _field;}
   }
}

internal class Derived : Base, IPublicInterface
{
   public Derived():base("I am derived")
   {
   }
}

public class CreatorFactory
{
   IPublicInterface CreatePublicInterface()
   {
    return new Derived();
   }
}

public interface IPublicInterface
{
  string Field{get;}
}

In this example, is the protected "_field" of Base, a bad usage, given that both Base and Derived are internal and would never be used outside of the assembly?

To add to complexity, what if protected field is complex data structure?

C#

public interface ICompositeDataType
{
  ICompositeDataType {get;}
  string Value{get;}
} 

internal class Base
{
   protected readonly ICompositeDataType _field;

   public Base(ICompositeDataType field)
   {
     _field=field;
   }

   public ICompositeDataType Field
   {
     get {return _field;} 
   }
}

internal class Derived : Base, IPublicInterface
{
   public Derived():base(new SomeCompositeDataTypeImpl())
   {
   }
}

public class CreatorFactory
{
   IPublicInterface CreatePublicInterface()
   {
    return new Derived();
   }
}

public interface IPublicInterface
{
  ICompositeDataType Field{get;}
}

Posted 9-Sep-12 2:54am

Ankush Bansal

Updated 9-Sep-12 6:49am

v2

Add a Solution

3 solutions

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

jschell · Answer 1 · 2012-09-09T04:57:00

Solution 1

internal class Derived : IPublicInterface

First thing wrong I would suppose is that that line is suppose to include 'Base'
But other than that nothing wrong with the first example.

Second example looks like it is getting overly complicated. But that depends on the needs of the application.

Posted 9-Sep-12 4:57am

jschell

Comments

Ankush Bansal 9-Sep-12 12:51pm

thanks for highlighting the mistake!! I've corrected it now.

Ankush Bansal 9-Sep-12 13:09pm

Hi,

So, do you mean to say that a protected field of primitive type is fine, however a protected field of complex data type is not recommended? If yes, can you please suggest some examples of why you think so?

Thanks
ankush

jschell 10-Sep-12 14:42pm

Nope - I posted what I meant.

Your second example, overall, seems overly complex. That statement has nothing to do with your use of protected.

Mattias Högström · Answer 2 · 2012-09-09T05:40:00

When I started learning object-orientation in the mid 90s. They forced us to think abstraction, abstraction, abstraction. Everything that could be put higher up in the hierarchy was better, even member variables.

Nowadays, some people say that one should focus more on interface reuse rather than object reuse. In this toy example, the _field looks very innocent. But if you add a few extra parent classes and a couple of more member variables on each level, the complexity increases very fast. The risk that future changes will mess things up is high.
It is also generally considered unsafe to change base class implementation,
since it is hard to foresee how derived classes use the implementation.
If one inherits just the interface, it is more safe to do changes.

I would like to recommend the book "Refactoring improving the design of exisisting code", by Martin Fowler, Kent Beck, etc. It is a very old book, but is full of refactoring tricks. I have learned a lot from it.

I like unit-tests. Therefore Dependency Injection is very important. Meaning that from my test I can plug in test classes easily. This is not possible when you create objects directly from classes as you do with

public Derived():base(new SomeCompositeDataTypeImpl())

I would probably do like this

new Derived(new SomeCompositeDataTypeImpl())

in order to make it more testable.

I like interfaces inheritance and object delegation a lot,
and avoid object inheritance as much as I can, but that is me.

CodeHawkz · Answer 3 · 2012-09-09T05:09:00

Solution 2

Hi there,

Your first example does not work since the Base class is not derived from IPublicInterface, therefore even though the class contains a Test property, when casted to the interface, it will give you an error.

Furthermore, are you trying to build a data structure like a tree? or just a menu? or are you just trying to learn OOP? :) Let us know that too, then our advice will be more inline with that.

Hope this helps, Regards

Posted 9-Sep-12 5:09am

CodeHawkz

Comments

Ankush Bansal 9-Sep-12 12:58pm

Hi,

Recently in code review I got a feedback that initializing a protected field as shown in example 1 is dangerous and highly discouraged.
To be very honest, through this question, I am trying to convince myself that use of protected property is not as bad as its portrayed.

Thanks,
ankush

jschell 10-Sep-12 14:41pm

You could always ask them to cite a source that demonstrates why it is dangerous.

But I suspect the reviewer didn't know what they are talking about.

The point of a "protected" is to allow access by the child.
The point of inheritence is that the child had better understand the contract of the parent.

As one specific case one can allow for a 'default' value which is set by the parent and the child is specifically supposed to override it if they want.

CodeHawkz 10-Sep-12 0:21am

Hi there,

Well, you've spotted the problem of that kind of initialization already.

First and foremost thing; It is hardcoded and you will not be able to pass a complex type like that. For instance, to initialize a Student class and set the first name and last name, and assign it to the property.

Secondly, if it's a value that is set in the initialization it's probably a value that do not change over the span of object life cycle. It's best to set these fields as readonly.

Thirdly, you've gone a factory pattern. The whole concept of it is to have dumb clients. As in when you've set the data in the derived class, the structure of the factory pattern is lost. One thing to remember is that all these count when in terms of future updates, scalability and testing.