Found this deep in a group project. It looks bad and I'm sure there is a super elegant solution [closed]

Question

Pretty obvious what it's doing here, but It seems pretty silly. What's a better way?

    public bool DidHitBounds
{
    get {
        if (hitBounds == true)
        {
            hitBounds = false;
            return true;
        } else
        {
            return false;
        }
    }

    set
    {
        hitBounds = value;
    }
}

Answer 1

I agree that it looks silly but more about the design and less the code, I would generally consider it an abuse to modify an object’s state with a getter (second opinion from SO). An acceptable exception would be for cache fields, but this isn’t quite that. In this case there’s no documentation that this behavior will happen either. Such behavior would be clearer in a method name that specifies the effect.

There’s not too much to do in terms of making it more elegant. There’s no reason to have the else after the first return, saving a line. Then, you can eliminate another line and the if altogether knowing that the state is always going to be false in the end and just grab the value to return (unconditional set might be less efficient?)

I would scrap DidHitBounds as a property (considering the getter is confusing and the setter adds no value, HitBounds could be an auto-property) and instead name a method GetAndResetHitBoundsState. Then what’s going on is a least a bit more clear when you call that new method.

e.g.

public bool HitBounds { get; set; }

///<summary>Return the hit-bounds state, and reset that state to <c>false</c>.</summary>
///<returns>Pre-reset hit-bounds state.</returns>
public bool GetAndResetHitBoundsState()
{
    bool result = HitBounds;
    HitBounds = false;
    return result;
}

I suspect the true elegant solution involves understanding how your field is set and received, it smells like something that could be “more elegant” with events or threads but that’s obviously not a simple change.