6

The code I've inherited has a design that I'm not familiar with (I'm still new to the world of programming).

It is a .net project and there are 3 classes in question here.

public Class1
{
    public void DoSomething()
    {
        Class2 class2 = new Class2();
        string myValue = class2.GetSomeValuePlease();

       //why would I not do class2.MyClass3.GetActualValue();
    }
}

public Class2
{
    public Class3 MyClass3 = new Class3();
    public string GetSomeValuePlease()
    {
        return this.MyClass3.TheActualValue();
    }
}

public Class3
{
    public string TheActualValue()
    {
        return "This is the value";
    }
}

As you can see, all 3 classes are public. I don't understand why, using the example above, I would use Class2 at all? I could understand if within the GetSomeValuePlease() method there was some logic which affected Class2, but there isn't.

Normally, I'd just go ahead and remove the Class2 method (GetSomeValuePlease) and call Class3 method (GetActualValue) direct from Class1, but, the developer I took over (who is not contactable) is wiser and more experienced than I. I have a feeling this is just over engineering and this is just extra code / extra maintenance.

Does any one have experience in designing this way that could explain the thought process or implications of having this "middle class" vs going direct?

6
  • 1
    It's already over-engineered with that many layers of concrete classes. But why not ask him?
    – itsbruce
    Nov 14, 2013 at 9:42
  • Good question @itsbruce. He no longer works here, company policy is to not provide ex-member contact details. Nice to know your feeling is it is over engineered and I will assume my suggestion of going direct didn't make you think I was making an incorrect decision
    – Dave
    Nov 14, 2013 at 9:43
  • 1
    The obfuscation you added does not help the question. If you can explain what each class actually does (or restore some of the original names), more specific answers can be given. It does look to be hinting towards a SOLID, testable design, despite the lack of interfaces. It seems the original developer did not want Class1 to know about Class3.
    – CodeCaster
    Nov 14, 2013 at 9:47
  • As an aside, if this is c# code, you don't need return = value;, rather just return value;.
    – Rotem
    Nov 14, 2013 at 9:53
  • Class3 MyClass3 = new Class did you mean to write Class3 MyClass3 = new Class3?
    – gnat
    Nov 14, 2013 at 10:05

3 Answers 3

19

Looks like the purpose of Class2 is to abstract away the implementation of Class3.

This allows Class3 to be changed in the future. Maybe the value will be grabbed from a database, maybe it will be read from a local file, maybe it will be read from the web. Maybe it won't even be read from Class3 at all, maybe it will be read from Class4.

The abstraction makes it possible to change the implementation of getting a correct value without Class1 having to change any code.

This is a valid design pattern (see Facade Pattern Bridge Pattern). To say whether or not it is necessary, you'd need to provide the actual classes.

9
  • Thanks, it makes sense about keeping class 1 and 3 separate (as you said, there is noway to know if necessary based upon the detail I provided). You are also right about there being a class 4, sadly also a class 5 and 6... Thank you.
    – Dave
    Nov 14, 2013 at 9:51
  • 1
    @CodeCaster, not at this stage, there is a lot of code to go through and sadly his naming convention is IMO poor (I would call a property PeriodWidthHours, where he is calling it pwhvd (period width hour value double), so since I'm updating and refactoring as I go and I thought I'd ask if this needed to be addressed as well.
    – Dave
    Nov 14, 2013 at 9:54
  • 3
    I make it a point not to modify program structure without a good reason on code produced by another programmer until I get to know it well enough. Most programmers have a reason for doing things the way they do, even if it is to cover up for bad design decisions made elsewhere.
    – Neil
    Nov 14, 2013 at 10:14
  • 1
    Is this really Facade pattern or is it perhaps more aptly described as Bridge. I consider Facade to be only applied to 'an entire subsystem' represented by a single object. Nov 14, 2013 at 16:55
  • 1
    the main idea in all these isn't abstraction its decoupling. An abstraction may be used to achieve that, but its not the goal. Nov 14, 2013 at 21:59
2

It depends. The good news is that if you understand the refactoring, you can reverse your decision later.

The advantage of the code as written is that you have more flexibility to change the internal structure of Class2 or change Class3 without affecting Class1.

The disadvantage is that every time you want to add a feature to Class3 that is used by Class1, you have to add a delegate method to Class2.

I suggest that you trust your instincts. You can use Remove the Middle Man to have the Class1 call Class3 directly.

If later you decide you should go back to the original design, you can use Hide Delegate.

I recommend the book Refactoring: Improving the Design of Existing Code by Martin Fowler.

2

This is the application of the "Law of Demeter" (which is not so much a law, but a good guide)

http://en.wikipedia.org/wiki/Law_of_Demeter

The basic idea is not to reach through objects to get access to other objects as that increases your coupling with an eco system of objects, meaning, changes are likely to ripple through entire systems.

However, I have seen people go too far with this and end up building facades to other objects into objects that have their own responsibility.

Most often if you find you want to apply "LoD" and find you are doing this kind of delegating / facade / Bridge type stuff, start having a good think about whether your design is right, because quite often a slight adjustment to the design / responsibilities will untangle the need to reach through one object to get to other objects.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.