1

I came upon a situation where multiple classes manifest duplicated logic. They all delegate to a worker object, but when interrupted they have to stop the worker.

class A {
    Worker w;
    public void OnSleep() { w.Stop(); }
}

class B {
    Worker e;
    public void OnSleep() { w.Stop(); }
}

class C { … }

The issue gets harder if I am to reuse the classes.

A solution with inheritance is to introduce a base class with only protected methods…

class Base {
    protected Worker TheWorker;

    protected virtual void OnSleep() { TheWorker.Stop(); }
}

…and make A, B etc. inherit the base.

However, the worker is totally an implementation detail. By all design patterns it seems like a bad smell.

I can think of a solution that makes the composition guys happy, by making the worker listen to OnSleepEvent. It could be done using a factory method:

public class Worker {
    public static Worker StopOnSleep(ICanSleep s) {
        Worker result = new();
        s.OnSleepEvent += _ => result.Stop();
        return result;
    }
}

While this seems DRY and composite, my intuition tells me that this feels weird, and anyways I have to add an event, an interface and a base implementation there. I think it is a big overhead which I don't want to pay.

So, how should I eliminate this duplication? Could you give me some examples?

P.S. real-world examples: threads, sockets, tweening and coroutines.

EDIT:

Sorry for the incompleteness of my previous question. Here is some context.

In a videogame I have a character who can run & jump, dash, and cling to walls. He can only be in one of the 3 states at the same time. In my implementation, I used coroutines for updating the physical status (velocity etc.) of the character.

class RunState {
    public void Update() {
        UpdateRunVelocity();
    }
}

class DashState {
    Coroutine _dashCoroutine;
}

class ClingState {
    Coroutine _clingCoroutine;
}

For the coroutines, I need to pre-stop them if I am to change the state, For example, when Player wants to dash when clinging, I will instantly kill the cling coroutine.

I'm thinking of deduplicating the logic. As you can see, I can also implement the states with explicit update, so the implementation becomes proactive instead of reactive. It solves the "stop-it" problem, but I think either way it is an implementation detail and I doubt if simply falling back to polling would do good in general.

Here is the real code:

public abstract class TweenMoveBehavior: MoveBehavior {
    Tween _tween;

    protected Tween Tween {
        get => _tween;
        set {
            StopTween();
            _tween = value;
            _tween.SetUpdate(UpdateType.Fixed);
        }
    }

    protected void StopTween() => _tween?.Kill();

    protected bool TweenRunning => Tween.Running();

    // HERE: I'm utilizing inheritance to unify the boilderplate of
    // having to stop the tween on disable. Is this correct?
    protected virtual void OnDisable() => StopTween();
}

public class JumpCommand: TweenMoveBehavior {
    [Range(0, 9)] public float jumpHeight = 2;
    [Range(0, 5)] public float powerJumpExtraHeight = 4;
    [Range(0, 1)] public float powerJumpChargingTime = 0.4f;
    public Ease powerJumpEase = Ease.OutQuad;

    // Conservation of mechanical energy: mv^2/2 = mgsh, where s = gravityScale.
    public static float CalculateElevatingSpeed(Rigidbody2D body, float deltaHeight) =>
        Sqrt(Abs(2 * Physics2D.gravity.y * body.gravityScale * deltaHeight));

    public bool Jumping => TweenRunning;

    public void StopJump() => Tween?.Kill();

    public void StartJump() {
        var grounded = OverlapGround(down, new Vector2(-0.01f, 0.01f));
        if (!grounded) return;

        var y0 = Body.position.y + jumpHeight;
        var y1 = y0 + powerJumpExtraHeight;

        Elevate(y0);
        Tween = DOVirtual.Float(y0, y1, powerJumpChargingTime, Elevate).SetEase(powerJumpEase);

        void Elevate(float target) => Body.SetSpeed(up, CalculateElevatingSpeed(Body, target - Body.position.y));
    }
}

public class DashCommand: TweenMoveBehavior {
    public MonoBehaviour MoveController { get; set; }
    public float dashSpeed = 40;
    public float dashDuration = 0.2f;
    public int fireCount = 1;

    public bool Dashing => TweenRunning;

    public void Dash(Vector2 direction) {
        if (direction == Vector2.zero || fireCount <= 0) return;
        --fireCount;

        var oldX = Body.velocity.x;
        MoveController.enabled = false;
        Tween = TweenExtension.Timed(dashDuration, KeepDashVelocity)
            .OnComplete(() => {
                Body.velocity = new Vector2(oldX, 0);
                MoveController.enabled = true;
            });
        KeepDashVelocity();

        void KeepDashVelocity() => Body.AddForce(direction * dashSpeed - Body.velocity, ForceMode2D.Impulse);
    }
}

There are also some other classes. Please comment if there's anything else needed.

I was taught that I should ask questions with a concise yet informative excerpt. Sorry that I failed to do that.

7
  • The base class feels cleaner than the static method. However, with the base class you could change the instance field protected Worker TheWorker to protected virtual method. Yes, each class would have to implement that ~getter, but probably would be the most flexible.
    – Erik Eidt
    Apr 25, 2022 at 21:44
  • @ErikEidt What do you think of it violating the rule that "we shouldn't inherit for sharing functionality over responsibility"?
    – Seideun
    Apr 25, 2022 at 22:18
  • This is why I suggest using a method for the state over the instance field. Once the instance field is no longer shared functionality, then the remainder is shared responsibility. These rules are all much more important when you have an API boundary, library or business boundary, but with self contained software, software is still soft, so context matters regarding the "rules".
    – Erik Eidt
    Apr 25, 2022 at 22:32
  • 1
    Your example looks incomplete. Is OnSleep really public and non-virtual in the initial code? Who calls it, and how?
    – Doc Brown
    Apr 26, 2022 at 1:55
  • 1
    I will be happy to remove my close vote ("needs details or clarity") after you clarified my former question.
    – Doc Brown
    Apr 26, 2022 at 13:11

2 Answers 2

1

Reverse the dependency. The Worker is the class that is supposed to be stopping itself when its container goes to sleep, so let it subscribe to the container. This allows you to avoid repeating yourself while removing the implementation detail from the A and B classes.

interface ICanSleep
{
    event EventHandler OnSleepEvent;
}

class Worker
{
    public Worker(ICanSleep container)
    {
        container.OnSleepEvent += (s,e) => this.Stop();
    }

    public void Stop()
    {
        //Stop or whatever
    }
}

class Base : ICanSleep
{
    public event EventHandler OnSleepEvent;
}

class A : Base
{
    protected readonly Worker _worker;

    public A()
    {
        _worker = new Worker(this);
    }
}
3
  • Maybe the overhead of event is a necessary cost of staying dry
    – Seideun
    Apr 26, 2022 at 17:01
  • 1
    FWIW I think you are setting yourself a very high bar for DRY. Adding one line of code in order to clean up a resource that is held in a field that is a member of the same class strikes me as completely normal. If you were repeating the implementation of Stop() that would be bad, but repeating the invocation of Stop seems unavoidable to me.
    – John Wu
    Apr 26, 2022 at 18:36
  • ah! You gave me a brand new insight. I see that i should make clear of the differences between invocation and implementation. Thank you
    – Seideun
    Apr 26, 2022 at 23:11
1

Context

What is A, What is B?

If this was a purely syntactic/implementation question sure fine, why not? The name has no bearing on the question.

Unfortunately we really need to know what makes A and B unique.

For example here are two structures:

struct A
{
  int x;
  int y;
  int z;
};

struct B
{
  int x;
  int y;
  int z;
};

This feels like duplication to me, should I introduce a base class, create an interface, or do some other transformation?

Answer: It Depends on what A and B are.

Too satisfy the curious in this context A are Cartesian co-ordinates. B are Polar co-ordinates.

In which case no, a base class makes no sense, they aren't similar at all. A user would not interact with these members in the same way.


Considerations

There is a reason for there being an interface, and an implementation. In this way users can work with the Concern of Usage (the interface) without caring about how a developer wishes to provide that in their own separate Concern of Implementation (the class).

So the first question is OnSleep() a member of an interface that makes sense to your users? If this interface does not exist, then while these two OnSleep() functions are syntactically identical, they are actually semantically different. If so then these aren't duplicates - DRY does not apply.

If there is a common interface great. Now is the perfect time to reify that interface (if it doesn't exist) and hide these two classes as implementation details behind some factory method/class.

Done that, now onto how to de-duplicate a common implementation pattern.

  1. Don't. These are two separate classes for a reason after all. You've already factored most of the logic into the worker. All this code is doing is linking the interface to the implementation. Can't get simpler without loosing flexibility.

  2. Realise that these aren't just aren't the same repeated function, but the same repeated class. They might look different but that is just because it is done two ways at two slightly different times. Collapse them down to one class, and apply standard refactoring techniques such as Sprout and Split.

  3. You really shouldn't get here. Sprout and Split would do a much better job. But maybe you don't have time, or license to rewrite one of, or both classes to that extent. Sigh.

    • Use a macro. Provide the common implementation details within and pass the differences by argument.
    • Use a base class. It should explicitly implement the interface with the common implementation pattern. They should be sealed. It should provide hooks to allow for extension. Your derived classes should only override the hooks, or those members of the original interface not provided for in the common implementation. Doing otherwise is technically possible, but it will lead to development hell later.

Just a word of caution:

Option 3 (however you implement it) increases the cohesion between both classes. This will mean that when you circle back to make a change to A there is a very good chance it will also mean change to the common code, and hence a change to B (even if you didn't touch it directly). This can drastically increase the surface area of a change, and the surface area of testing. If you take this route be absolutely certain that the extracted behaviour is truly a common implementation pattern and not a fluke of chance.

2
  • I have added some context. Is that clear? If not, please tell me and I will add more. I have read your #considerations. I have also browsed many many posts on the design issue on the Internet. I think I have come to a somewhat tangible conclusion that implementation-level duplication is not something I should proactively concern about, as long as the duplication does not appear on the user side.
    – Seideun
    Apr 26, 2022 at 14:06
  • You should only care about it when its option 2 - its the same class done two ways. The correct approach is always to collapse and refactor.
    – Kain0_0
    Apr 27, 2022 at 2:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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