Critique – OO Design Flaws

Question

Recently, I received some negative feedback about the design of a class that I had originally thought to be innovative. After reflection, however, I agree it is clearly more flawed than innovative.

This post invited the community to join in the review and provide its thoughtful feedback on the design flaws and identify a fix.

Background and Details:

The class in question is a domain object that gets dual-use in the business layer and in the UI. (Sometimes it is necessary.)

The class includes a Display() Property that returns UI-friendly string. The rationale for including this Property is a convenience for class consumers. In my specific use, it was easy to bind a collection of this object to a UI grid control on the Display() Property.

Display is implemented through a delegate so consumers can swap in another delegate.

Implementation:

//Within the "Measure" class is a delegate for the Display()
public delegate string DisplayDelegate(IMeasure Measure);
private DisplayDelegate _displayDelegate;

...

//The class uses a Write-Only Property sets the Display() delegate.
//Within the constructor (not shown here), I set a default Display delegate by 
//concatenating a few properties together in a UI-friendly display string.
public DisplayDelegate Display {
    set { _displayDelegate = value; }
}
//Class consumers call "MeasureDisplay" for a 
public string MeasureDisplay {
    get { return _displayDelegate.Invoke(this); }
}

The responses below address the question of what is wrong with the design and what could fix it.

Answer 1

Well, for one I personally think there's never a valid reason to have a write-only property. Such properties should be refactored into a method.

Secondly, that code doesn't look very clear at all. In both Windows Forms and WPF there are much better ways of providing customizable formatting of an object or value that does not involve setting delegates. Anything UI related like that should not require anything that can't be expressed declaratively.

Some suggestions: TypeConverter, IFormattable, MVVM

Anyhow, I'm not trying to be a critic. I've written plenty of code through the years that I'm not proud of. But you asked for constructive criticism so I thought I'd offer my 2 cents.

Answer 2

To me it looks like you couple the layers to strong. Measure consumers could call the DisplayDelegate directly, there's no immediate need to hide it within the domain class Measure if it's only used in the UI layer. And if you don't want to burden the consumers with managing two separate objects, add a MeasureView that transforms Measure using DisplayDelegate.

Answer 3

Given the information you've presented I probably wouldn't use a delegate in that manner.

There's probably a default or simple implementation of the Display method that would let people wanting to use your class utilise without having to delve into the internals of it. If they subsequently find the Display method isn't suitable they could use inheritance to override the Display method.

If it was an internal class that wasn't intended for other people to use it wouldn't be so bad but if you're expecting other people to make use of it it's always nice to stick to the simplest and most common way of doing things.

Answer 4

Display is implemented through a delegate so consumers can swap in another delegate because I can't know too much about future UI display needs.

Well... if I were reviewing your code, I'd have brutalized you for that statement alone. You're greatly increasing the complexity of what you wrote and must work with today on the off-chance that it'll help you in some way later on. And you're doing so in a way that allows extremely limited customization, further reducing the chances that it'll ever actually be useful!

Write code that does what it's supposed to. If and when requirements change, then factor out common logic into a library, base class, etc. Otherwise, you're just adding complexity for the sake of complexity.

Answer 5

If you're on any team worth its salt, you should pose the question to them, not us.

Why did you get critique? What rules or conventions did you break? What was your negative feedback?

You should be asking them, not us.

Answer 6

I think I don't understand... When your consumer has a reference to the object, you would like him to set a property to point to a method of theirs which (when it is called) will be given a reference to Measure that they can then muck about with to get the output they want? I would just skip the delegate stuff and get my output from that original reference.

Answer 7

This isn't just about good design but how reviews should be conducted

If you honestly left that review feeling brutalized' then the review was poorly managed. It's entirely possible to sensitively provide feedback that doesn't leave a member of staff feeling beaten-up.

Yes there are problems with your design but they are indicative of a keen but junior engineer trying to impress others with their grasp of a programming language. Did you get any guidance as to what was wrong with your design? Do you have a mentor assigned?

Don't feel too bad, you've made a mistake but long after you've learned how to design and implement software pragmatically, the folks who conducted the 'review' will still be idiots.

Also learn try to focus on the positives - you have learned that your design failed in some areas. As many of us have said, just because some technique is possible with the language or framework at hand, it doesn't mean that you should use it. Don't over-engineer solutions; simple and quick is usually best.

Answer 8

If you can't handle being told that your code isn't good, then you can't handle getting better. Do you want to get better? Or do you just want everyone to run around telling you how you are the greatest coder since Alan Turing?

Your ego wouldn't even let you finish listening to the code review. Sad really.

You've left it so the folks on stack overflow have to guess what they said about your code.

First get a grip on your ego, then you can maybe be humble enough to become better.

Answer 9

If you didn't get the answer to your question during your review then either the review process is broken, or you don't function well and didn't listen.

One or both need to be fixed.

Only the reviewers would be able to answer this question - I suggest you go talk to them. If you don't you risk being "brutalized" again and again.

Answer 10

My guess would be what you've admitted in the first line - domain objects should only store the data (or perform operations too with the ActiveRecord pattern).

So passing in a delegate to format the data into a domain object may have been why.

A different way round of looking at it, is you probably should be passing the Domain object into a control and it works out what to do with the data, rather than the other round. I'm not going to paste a whole grid example, but for a simple label:

public class MyLabel : Label
{
    private Measure _measure;

    public MyLabel(Measure measure)
    {
        _measure = measure;
    }

    public override string Text
    {
        get
        {
            // Do something with the measure
        }
    }
}

Also a write only property probably didn't help.