Simplify this code

Questions & Answers
1

Here’s a coroutine, it has two very similar loops that lerps the cam.LerpX or cam.LerpY depending on the CameraBoundary.Mode, the code works fine but whenever I see repeating lines of code it usually means that there is room for improvement, can this be simplified to a single loop? I don’t understand how to have a variable that references either cam.LerpX or cam.LerpY.

   IEnumerator TransitionCoroutine(CameraBoundary.Mode mode, bool transitionIn)
    {
        float time = transitionIn ? transitionTime : -transitionTime;
        float lerpValue = transitionIn ? 1 : 0;

		if (mode == CameraBoundary.Mode.Vertical)
        {
            while (cam.lerpX != lerpValue)
            {
				if(cam.lerpX == 1)
					yield return new WaitForSeconds(0.1f);
				if (controller.transitioning)
                     yield break;

                cam.lerpX = Mathf.Clamp01(cam.lerpX + time * Time.deltaTime);
                yield return null;
			}
        }
        else if (mode == CameraBoundary.Mode.Horizontal)
        {
            while (cam.lerpY != lerpValue)
            {
                if(cam.lerpY == 1)
					yield return new WaitForSeconds(0.1f);
                if (controller.transitioning)
                    yield break;
                    
                cam.lerpY = Mathf.Clamp01(cam.lerpY + time * Time.deltaTime);
                yield return null;
            }
        }
    }
3

It is a very good instinct to react to repeating lines with “that could be improved” - it usually is true. In this case a simple factoring out of the block of the branch and eliminating differences with a common variable should do it:

private IEnumerator TransitionCoroutine (CameraBoundary.Mode mode, bool transitionIn) {
    float time = transitionIn ? transitionTime : -transitionTime;
    float lerpValue = transitionIn ? 1 : 0;

    if (mode == CameraBoundary.Mode.Vertical) { yield return LerpRoutine(cam.lerpX, lerpValue); }
    else if (mode == CameraBoundary.Mode.Horizontal) { yield return LerpRoutine(cam.lerpY, lerpValue); }
}

private IEnumerator LerpRoutine (float camLerp, float lerpValue) {
    while (camLerp != lerpValue) {
        if (camLerp == 1)
            yield return new WaitForSeconds(0.1f);
        if (controller.transitioning)
            yield break;

        camLerp = Mathf.Clamp01(cam.lerpY + time * Time.deltaTime);
        yield return null;
    }
}

Some further possible improvements:

  • instead of if-else-if-else... chain use a switch-case for branching on the value of CameraBoundary.Mode (I’m assuming it is an enum)

  • as Piyush_Pandey said, cache the wait object (common type is YieldInstruction), even if just as a local variable before the while loop

2

Without repeating code you try something like this:

	IEnumerator TransitionCoroutine(CameraBoundary.Mode mode, bool transitionIn)
	{
		float time = transitionIn ? transitionTime : -transitionTime;
		float lerpValue = transitionIn ? 1 : 0;

		float lerpTarget;

		if (mode == CameraBoundary.Mode.Vertical)
		{
			lerpTarget = cam.lerpX;
		}
		else
		{
			lerpTarget = cam.lerpY;
		}

		while (lerpTarget != lerpValue)
		{
			if(lerpTarget == 1)
				yield return new WaitForSeconds(0.1f);
			if (controller.transitioning)
				yield break;

			lerpTarget = Mathf.Clamp01(lerpTarget + time * Time.deltaTime);
			yield return null;
		}
	}
4

I’ve decided to go with what Strotosome and Sonky108 have suggested:

 IEnumerator TransitionCoroutine(CameraBoundary.Mode mode, bool transitionIn)
{
    float time = transitionIn ? transitionTime : -transitionTime;
    float lerpValue = transitionIn ? 1 : 0;
	float lerpTarget = mode == CameraBoundary.Mode.Vertical ? cam.lerpX : cam.lerpY;
	WaitForSeconds waitForMillisecond = new WaitForSeconds(0.1f);

	while (lerpTarget != lerpValue)
	{
		if (lerpTarget == 1)
			yield return waitForMillisecond;
		if (controller.transitioning)
			yield break;

		lerpTarget = Mathf.Clamp01(lerpTarget + time * Time.deltaTime);

		if (mode == CameraBoundary.Mode.Vertical)
			cam.lerpX = lerpTarget;
		else
			cam.lerpY = lerpTarget;

		yield return null;
	}
}

Thanks for the suggestions!