Design Problem - Is Inheritance the right way to simplify this code?

Question

I have a design problem I'd like to solve. I have an interface, lets call it IProtocol, which is implemented by two separate classes. We're looking at over 600 lines of code here. The vast majority of the stuff they do is the same, except for some specific areas, like DiffStuff();

Current structure is something like this:

public class Protocol1 : IProtocol
{
  MyInterfaceMethod1()
  {
     Same1();
     DiffStuff();
     Same2();
  }
}

And

public class Protocol2 : IProtocol
{
  MyInterfaceMethod1()
  {
     Same1();
     Same2();
  }
}

I'm concerned with having copy-paste errors and the classic problem of code duplication if I keep the two protocols separate. We're talking about a full 600 lines of code each, not some simple methods.

I'm considering changing the implementation of Protocol1 to inherit from protocol2, like this (Protocol2 would mostly stay the same, except I'd have to wrap Same1() and Same2() into private methods.)

public class Protocol1 : Protocol2
{
  void Same1()
  {
     base.Same1();
  }

  void Same2()
  {
     base.Same2();
  }

  MyInterfaceMethod1()
  {
     Same1();
     DiffStuff();
     Same2();
  }
}  

Is this the right way to go about dealing with this problem?

Edit: Many people helped me with this question, thanks for the clear understanding. In my case, the two objects are not of the same type, even though much of their implementation is shared, so I went with Bobby's suggestion to use abstract base class, creating small methods to encapsulate changes between the classes. Additional Thanks to:

• jloubert
• Hans Passant
• Jeff Sternal

Answer 1

Not quite. There's no point in adding the Same1 and Same2 methods, you inherit them from ProtocolBase. And DiffStuff() should be a virtual method so that you can override it and give it different behavior.

Answer 2

I agree with MathEpic. I would use Template method pattern.

Answer 3

I better design (in my opinion)

public abstract class Protocol_common : IProtocol 
{ 
  MyInterfaceMethod1() 
  { 
     Same1(); 
     DiffStuff(); 
     Same2(); 
  } 

  abstract void DiffStuff();

}

public class Protocol1 : Protocol_common
{ 
  DiffStuff() 
  { 
      /// stuff here.
  } 
}

public class Protocol2 : Protocol_common
{ 
  DiffStuff() 
  { 
      /// nothing here.
  } 
}

(That's actually more pseudo-code than proper C#, but I hit the high points)

Answer 4

You're awfully close to describing the template method pattern, which has a long pedigree as an effective solution to problems like yours.

However, you should consider using composition instead of inheritance. The two approaches offer different advantages and disadvantages, but composition is often (usually?) better ^*:

 public class Protocol {

      private ISpecialHandler specialHandler;

      public Protocol() {}

      public Protocol(ISpecialHandler specialHandler) {
          this.specialHandler = specialHandler;
      }

      void Same1() {}
      void Same2() {}

      public void DoStuff() {
          this.Same1();
          if (this.specialHandler != null) {
              this.specialHandler.DoStuff();
          }
          this.Same2();
      }
 }

Callers can then pass in object instances (strategies) that provide specialized algorithms to handle whatever case is at hand:

 Protocol protocol1 = new Protocol(new DiffStuffHandler());
 protocol1.DoStuff();

^*See Patterns I Hate #2: Template Method for a detailed explanation of why composition is usually better than inheritance in your case.

Answer 5

You might want to consider whether this type of pattern works:

public class ProtocolHelper
{
    public void Same1() {}
    public void Same2() {}
}

public class Protocol1 : IProtocol
{
    private readonly ProtocolHelper _helper = new ProtocolHelper();

    void MyInterfaceMethod1()
    {
        _helper.Same1();
        DiffStuff();
        _helper.Same2();
    }
}

You can tell if this makes sense by seeing if you can come up with a good name for the 'ProtocolHelper' class. If a name naturally arises from your logic, then this is a good way to break up the class. You might need to pass in some dependencies (such as private fields) as parameters to the methods in order to make this work.

Answer 6

    /// <summary>
    /// IProtocol interface
    /// </summary>
    public interface IProtocol
    {
        void MyInterfaceMethod1();
        void Same1();
        void Same2();
    }

then...

public abstract class ProtocolBase : IProtocol
{
    #region IProtocol Members

    public void MyInterfaceMethod1()
    {
        // Implementation elided...
    }

    public void Same1()
    {
        // Implementation elided...
    }

    public void Same2()
    {
        // Implementation elided...
    }

    public abstract void DiffStuff();

    #endregion
}

finally...

public sealed class Protocol1 : ProtocolBase
{
    public override void DiffStuff()
    {
        // Implementation elided...
    }
}

public sealed class Protocol2 : ProtocolBase
{
    public override void DiffStuff()
    {
        // Implementation elided...
    }
}