maelvls dev blog

maelvls dev blog

Software engineer. I write mostly about Kubernetes and Go.

23 Nov 2019

Go Happy Path: the Unindented Line of Sight

While perusing how other Kubernetes developers are implementing their own reconciliation loop, I came across an interesting piece of code.

The author decided to use the if-else control flow at its maximum potential: the logic goes as deep as three tabs to the right. We cannot immediately guess which parts are important and which aren’t.

func (r *ReconcileTrial) reconcileJob(instance *trialsv1alpha3.Trial, desiredJob *unstructured.Unstructured) (*unstructured.Unstructured, error) {
    var err error
    logger := log.WithValues("Trial", types.NamespacedName{Name: instance.GetName(), Namespace: instance.GetNamespace()})
    apiVersion := desiredJob.GetAPIVersion()
    kind := desiredJob.GetKind()
    gvk := schema.FromAPIVersionAndKind(apiVersion, kind)

    deployedJob := &unstructured.Unstructured{}
    deployedJob.SetGroupVersionKind(gvk)
    err = r.Get(context.TODO(), types.NamespacedName{Name: desiredJob.GetName(), Namespace: desiredJob.GetNamespace()}, deployedJob)
    if err != nil {
        if errors.IsNotFound(err) {
            if instance.IsCompleted() {
                return nil, nil
            }
            logger.Info("Creating Job", "kind", kind,
                "name", desiredJob.GetName())
            err = r.Create(context.TODO(), desiredJob)
            if err != nil {
                logger.Error(err, "Create job error")
                return nil, err
            }
            eventMsg := fmt.Sprintf("Job %s has been created", desiredJob.GetName())
            r.recorder.Eventf(instance, corev1.EventTypeNormal, JobCreatedReason, eventMsg)
            msg := "Trial is running"
            instance.MarkTrialStatusRunning(TrialRunningReason, msg)
        } else {
            logger.Error(err, "Trial Get error")
            return nil, err
        }
    } else {
        if instance.IsCompleted() && !instance.Spec.RetainRun {
            if err = r.Delete(context.TODO(), desiredJob, client.PropagationPolicy(metav1.DeletePropagationForeground)); err != nil {
                logger.Error(err, "Delete job error")
                return nil, err
            } else {
                eventMsg := fmt.Sprintf("Job %s has been deleted", desiredJob.GetName())
                r.recorder.Eventf(instance, corev1.EventTypeNormal, JobDeletedReason, eventMsg)
                return nil, nil
            }
        }
    }

    return deployedJob, nil
}

The outline of this function doesn’t tell us anything about what the flow is and where the important logic is. Having deeply nested if-else statements hurt Go’s glanceability: where is the “happy path”? Where are the “error paths”?

By refactoring and removing else statements, we obtain a more coherent aligned-to-the-left path:

The green line represents the “core logic” and is at the minimum indentation level. The red line represents anything out of the ordinary: error handling and guards.

And since our eyes are very good at following lines, the line of sight (the green line) guides us and greatly improves the experience of glancing at a piece of code.

Here is the actual code I rewrote:

func (r *ReconcileTrial) reconcileJob(instance *trialsv1alpha3.Trial, desiredJob *unstructured.Unstructured) (*unstructured.Unstructured, error) {
    var err error
    logger := log.WithValues("Trial", types.NamespacedName{Name: instance.GetName(), Namespace: instance.GetNamespace()})
    apiVersion := desiredJob.GetAPIVersion()
    kind := desiredJob.GetKind()
    gvk := schema.FromAPIVersionAndKind(apiVersion, kind)

    deployedJob := &unstructured.Unstructured{}
    deployedJob.SetGroupVersionKind(gvk)

    err = r.Get(context.TODO(), types.NamespacedName{Name: desiredJob.GetName(), Namespace: desiredJob.GetNamespace()}, deployedJob)
    switch {
    case errors.IsNotFound(err) && instance.IsCompleted():
        // Job deleted and trial completed, nothing left to do.
        return nil, nil
    case errors.IsNotFound(err):
        // Job deleted, we must create a job.
        logger.Info("Creating Job", "kind", kind, "name", desiredJob.GetName())
        err = r.Create(context.TODO(), desiredJob)
        if err != nil {
            logger.Error(err, "Create job error")
            return nil, err
        }
        eventMsg := fmt.Sprintf("Job %s has been created", desiredJob.GetName())
        r.recorder.Eventf(instance, corev1.EventTypeNormal, JobCreatedReason, eventMsg)
        msg := "Trial is running"
        instance.MarkTrialStatusRunning(TrialRunningReason, msg)

        return nil, nil
    case err != nil:
        logger.Error(err, "Trial Get error")
        return nil, err
    }

    if !instance.IsCompleted() || instance.Spec.RetainRun {
        eventMsg := fmt.Sprintf("Job %s has been deleted", desiredJob.GetName())
        r.recorder.Eventf(instance, corev1.EventTypeNormal, JobDeletedReason, eventMsg)
        return nil, nil
    }

    err = r.Delete(context.TODO(), desiredJob, client.PropagationPolicy(metav1.DeletePropagationForeground))
    if err != nil {
        logger.Error(err, "Delete job error")
        return nil, err
    }

    return deployedJob, nil
}

You can also take a look at Matt Ryer’s Idiomatic Go Tricks where he presents some ways of keeping your code as readable as possible:

Tags