CODEWITHSUNDEEP
oopsolid-principlesopen-closed-principle
egyamado
egyamado

Reputation: 1121

How such an important principle "OCP" will be the reason of massive code duplication practice?

OCP (Open/Closed Principle) is one of the SOLID principles. Which is says:

”Software Entities should be Open for Extension, but Closed for Modification.”

It take me while to understand the above sentence about OCP. And when I start read more about it, I found it make sense and so useful, but in the mean time I noticed it cause duplicated code.

How such an important principle "OCP" will be the reason of massive code duplication practice?

namespace SOLIDPrinciples
 {  
    public class ReportFormatter {
        public virtual void FormatReport() {
            Console.WriteLine("Formatting Report for 8-1/2 X 11 ....");
        }
    }

    public class TabloidReportFormatter : ReportFormatter {
        public override void FormatReport() {
            Console.WriteLine("Formatting Report for 11 X 17 ....");
        }   
    }
 }

Am I missing something here? Is there another way for OCP to be explained?

Upvotes: 0

Views: 843

Answers (5)

Derek Greer
Derek Greer

Reputation: 16262

Code duplication refers to the significant repeating of blocks, statements, or even groupings of common member declarations. It doesn't refer to the repeating of language keywords, identifiers, patterns, etc. You wouldn't be able to achieve polymorphism otherwise.

The example you provide doesn't really demonstrate the Open/Closed Principle because you haven't demonstrated how a given class's behavior can be extended without modification. The Open/Closed principle is about creating new classes each time variant behavior is desired. This can be achieved using inheritance along with the Template Method pattern (i.e. calling abstract or virtual methods in a base class which are overridden in subclasses to achieve the desired/new behavior), but it's more often demonstrated using composition using the Strategy pattern (i.e. encapsulating the variant behavior in class and passing it to the closed type to be used in determining the overall behavior achieved).

From your example, it appears you were thinking more along the lines of trying to achieving OCP through inheritance, but starting with a base report formatter to establish the interface with subtypes to indicate different types of formats for a given report is actually a good start toward showing OCP through composition. What's needed to demonstrate OCP with a composition-based approach is a class which consumes the formatters ... say a ReportWriter.

public class ReportWriter : IReportWriter
{
    Write(IReportData data, IReportFormatter reportFormatter)
    {
         // ...

         var formattedStream = reportFormatter.Format(data, stream);

        // ...
    }
}

Just for the sake of demonstration, I've made some assumptions about how our new formatter might behave, so don't focus too heavily on that part. The thing to focus on is that ReportWriter is open for extension by allowing new formatters to be passed in, thus affecting how reports are ultimately written, but closed for modification of code needed to achieve this new behavior (i.e. the presence of if statements, switch statements, etc. to achieve multiple ways to format).

Not only does the Open/Closed principle not cause code duplication, when achieved through the use of composition over inheritance it actually can help reduce duplication. If true code duplication occurs in the course of creating your inheritance hierarchy or strategy classes then that points to a factoring issue unrelated to the fact that it might exist in the context of you trying to achieve OCP.

Upvotes: 4

soru
soru

Reputation: 5526

Something like this should work.

namespace SOLIDPrinciples
 {  
    public class ReportFormatter {
        public virtual void FormatReport() = 0;
    }

    public class VariableSizeReportFormatter : ReportFormatter {
        public void SetSize(String size);
        public override void FormatReport() {
                Console.WriteLine("implement generic layout routines");
        }       
    }

    public class TabloidReportFormatter : VariableSizeReportFormatter {
        public override void FormatReport() {
                // currently, we just do a tweaked version of generic layout ..
                SetSize("11x7");
                Super.Format(); 
                Console.WriteLine("RemoveThatColumnWeDontWant");
        }       
    }
 }

That means:

  • clients of ReportFormatter don't need to change (assuming some mechanism elsewhere that handles setup).
  • an improvement to generic formatting capabilities can be added to improves all reports, so no duplicated code
  • you just need things to work better in some particular case, you can just make that change

It's the third point that is the key: you can say 'in principle, the smart layout routine should know to drop the year from the date if the comment makes the row too big to fit'. But implementing that change in practise can be a nightmare, if it means that then the other report comes out wrong, so you have to change the logic of how it all works then retest every report layout in the system...

Upvotes: 0

haffax
haffax

Reputation: 6008

Having implemented a custom format report system myself, I'd say the approach is wrong already.

Why have a formatter class for each format when you give the format as an argument to the constructor? The formatter class ctor (or even the static format method of the ReportFormatter class) should have a report and a format description as arguments and format the report accordingly.

There is just no need for a class for each format.

Upvotes: 0

Cuga
Cuga

Reputation: 17904

Something like this could be solved with better design of the methods. Consider the following instead:

namespace SOLIDPrincibles
 {  
    public class ReportFormatter {
        public virtual void FormatReport(String size) {
                Console.WriteLine("Formatting Report for " + size + " ....");
        }
    }

    public class TabloidReportFormatter : ReportFormatter {
        public override void FormatReport() {
                super.FormatReport("11 X 17");
        }       
    }
 }

I think design along these lines would be your best shot of eliminating duplicated code.

Upvotes: 0

Vadim
Vadim

Reputation: 21704

What you have looks perfectly fine to me. You didn't modify ReportFormatter, you extended it.

Upvotes: 0

Related Questions