views:

32

answers:

2

In the method below the second constructor accepts a ForumThread object which the IncrementViewCount() method uses. There is a dependency between the method and that particular constructor.

Without extracting into a new private method the null check in IncrementViewCount() and LockForumThread() (plus other methods not shown) is there some simpler re-factoring I can do or the implementation of a better design practice for this method to guard against the use of the wrong constructor with these dependent methods?

Thank you for any suggestions in advance.

private readonly IThread _forumLogic;
    private readonly ForumThread _ft;

    public ThreadLogic(IThread forumLogic)
        : this(forumLogic, null)
    {
    }

    public ThreadLogic(IThread forumLogic, ForumThread ft)
    {
        _forumLogic = forumLogic;
        _ft = ft;
    }

    public void Create(ForumThread ft)
    {
        _forumLogic.SaveThread(ft);
    }

    public void IncrementViewCount()
    {
        if (_ft == null)
            throw new NoNullAllowedException("_ft ForumThread is null; this must be set in the constructor");

        lock (_ft)
        {
            _ft.ViewCount = _ft.ViewCount + 1;
            _forumLogic.SaveThread(_ft);
        }
    }

    public void LockForumThread()
    {
        if (_ft == null)
            throw new NoNullAllowedException("_ft ForumThread is null; this must be set in the constructor");

        _ft.ThreadLocked = true;
        _forumLogic.SaveThread(_ft);
    }
A: 

If you implement a property for the ForumThread then you could throw the exception from the get if it is null. This may create a method in the underlying IL but its not a new private method in your class (plus - whats wrong with having a new method?)

Leom Burke
The reason I didn't want a new private method is because a call to that method would be the first line in any subsequent public method created that needs to use the constructor set object, not to mention developers remembering to call the private method when adding the new public method.
N00b
I see. If you have a property then you can just call your method on it and it will throw from the getter. This should give you the correct behavior. You could always remove the constructor that accepts a single parameter and throw the error if null is passed in.
Leom Burke
The single param constructor is needed on occasions e.g. Create(). I do like the idea of just checking/throwing on the a Get property, it does make far more sense.
N00b
I've set your suggestion as the answer. It is simple but effective, sometimes over complication isn't a good thing either!
N00b
A: 

Why don't sub-class ThreadLogic and provide additional constructor and dependent methods only in the sub-class? Alternatively, if you already have some hierarchy, you can use delegation and decorator pattern to add new features. In both cases you can check null in single place: constructor

iPhone beginner