Declarative Conditionals

Intro

Join me for a code review of a complex nested control structure. I believe this is worthy of your time because it demonstrates perfectly how we can simplify complex logic by translating imperative syntax into a more behavior-driven language. This approach not only enhances readability but also cleans up our code, highlights its business value, and provides a quick and clear context for maintenance when we revisit the feature after six months.

Problem

I discovered the following piece of code while code reviewing a pull request from a group of external enterprise consultants. It belongs to a method of a Vue component that applies different styles to an input field based on various state conditions. This component utilized the options-api, and it had additional methods for error handling, related computed and data properties. However, I decided to exclude all of these to focus entirely on the control structure.

Take a look at the following code:

export const dynamicStyles = (type) => {
  if (!fieldsIdentified[type]) {
    if (focusElement[type]) {
      if (fieldsValid[type]) {
        return styles.label;
      } else {
        return styles.labelInvalid;
      }
    } else {
      if (
        (!fieldsValid['cvv'] || !fieldsValid['cardNumber']) &&
        !fieldsValid.hasEmptyState
      ) {
        return styles.labelNotFocus;
      } else {
        return styles.labelInvalidNotFocus;
      }
    }
  } else {
    if (fieldsValid[type]) {
      return styles.labelIdentified;
    } else {
      return styles.labelInvalid;
    }
  }
};

The syntax of this code is technically correct, and it works as intended. However, I believe you'll agree that it raises more questions than it answers, especially for those unfamiliar with it.

Insight

In my opinion, code reviews are essential for offering a fresh perspective on code, especially when the reviewer is not closely involved with the implementation of a feature. This is crucial because it mirrors the experience of another team member who may need to extend or maintain the feature in the future, or when we revisit it months later to debug an issue.

This is a key concept of code reviews that took me some time to fully understand.

Code reviews are not just about technical syntax or functional implementation details; they are also about the communication records that we are about to approve and commit to our codebase.

Code reviews provide an excellent opportunity for someone who is not directly involved in the implementation to assess how quickly a fresh pair of eyes can understand and work with the code being reviewed. In other words, it's the perfect time to ensure that the code itself will effectively communicate with our future selves, reminding us of what we did (the implementation) and why we did it (the business or user value), instead of depending on comments, memory, or documentation.

Questions

When reviewing our original code, we should ask ourselves the following questions to gauge its clarity and maintainability:

• Can you determine the purpose and effects of this function?
• Can you identify all the dependencies associated with such function?
• Does the code clearly convey the acceptance criteria for the business or the value created for the user?

Additionally, consider these maintainability-focused questions:

• Where would you start if you needed to make a change to this function?
• How would you ensure that your changes do not introduce any side effects?
• How would you approach adding tests for this function?

It's crucial to keep these fundamental questions in mind during code reviews. If we can't provide clear and straightforward answers, we may find it challenging to revisit the code later, potentially wasting significant time to reacquaint ourselves with it. This not only delays the development of new features but also introduces complexities that accumulate as technical debt.

Solution

Considering all these factors, we can now begin to make suggestions for improving how this component conveys our implementation decisions and the business value of our user.

We can rely on two patterns and best practices to shape our suggestions. The first is behavior-driven development BDD, which translates implementation details into a user-focused perspective. The second is the Triple A pattern AAA from unit testing, which helps us clearly outline the structure of the function body, indicating where we "arrange" elements and where do we "act" on them. The shift in language within the function will decode the function's context, making it easier to assert conditions as we go mentally through the code without manual testing.

Let's begin

Clarity

Let's begin by examining how we discuss the states for this control structure. We have a field with multiple states, which highlights a gap in our implementation, making it difficult to answer the following questions with the current syntax of our code:

• How many states are we actually dealing with?
• Do each field type have the same states?
• Do we have a single source of truth for our fields and all their states?

To address this, let's consolidate and clearly outline all the conditions for fields and states using simple terms in a reactive object called fieldsState, as follows:

  const fieldsState = reactive({
    cardNumberEmpty: true,
    cardNumberValid: false,
    cardNumberFocused: false,
    cardNumberValidate: false,
    cvvEmpty: true,
    cvvValid: false,
    cvvValidate: false,
    cvvFocused: false,
    cvvRequired: false,
  });

Declarative

Our next challenge is how to avoid using a linear language to express an abstract structure.

if (fieldsValid[type]) {
  return styles.label;
} else {
  return styles.labelInvalid;
}

To clarify the example above, we need to uncover how the state of fieldsValid is being defined. We lack context about what the type could be, other than knowing it must correspond to a key within fieldsValid. Additionally, we understand that a property from a styles object is being returned for various cases.

Moreover, the language used to convey the intent of the original logic is very vague. While some variable names are acceptable, it becomes challenging to understand their impact on the logic when they are used in compounded statements.

  if (
    (!fieldsValid['cvv'] || !fieldsValid['cardNumber']) &&
    !fieldsValid.hasEmptyState
  )

The logical condition above does not resemble natural language at all. As a result, it is quite challenging to capture the role it plays using simple phrases.

If you had to describe this condition, what name would you give it?

Would a non-technical person deduce and understand this condition?

To effectively understand and work with this code, we need to manually test it each time to regain its context. This process is not only time-consuming, but it can also create blind spots that may lead to unintended side effects, ultimately slowing down development. While unit tests can assist in this regard, introducing them may still be challenging due to the high level of abstraction and vague language used in the code.

So let's begin by switching the way our code talks to us from an imperative format to a declarative language that flows like natural speech.

Declarative language focus communicating what something does rather than how it is done.

We can now proceed to create properties for each logical condition using clear names, such as isInvalidAndNotEmpty, and assign the corresponding logical conditions that match each field's acceptance criteria.

Notice how we are communicating from the perspetive of the user or what the user sees, making the business value explicit in our code.

  const field = {
    isInvalidAndNotEmpty: !fieldsValid && !fieldsState[`${type}Empty`],
    isFocusedAndNotEmpty:
      fieldsState[`${type}Focused`] && !fieldsState[`${type}Empty`],
    isValidAndNotFocused: fieldsValid && !fieldsState[`${type}Focused`],
    isInvalidAndNotFocused: !fieldsValid && fieldsState[`${type}Validate`],
  };

We have consolidated all the logical conditions into the arrange block of our function, adhering to the Triple A pattern. With the improved clarity of our fieldsState, we now have an easy way to extend conditions if necessary and know exactly where to modify each condition, minimizing the potential for side effects.

We have also improved our communication records by following BDD. This will make it much easier to regain context or onboard a new team member when we revisit this logic in the future because we have effectively translated technical syntax into natural language that describes behavior.

  if (field.isInvalidAndNotEmpty)
  // is a lot better than 
  if (!fieldsValid[type] && !fieldsValid.hasEmptyState)

Flatten

The complexity of nested control structures makes it difficult to understand the various states, conditions, and returns involved. For example, it can be challenging to determine the total number of states we are actually working with here. Moreover, we may lose track of the specific state combination needed to return each different style. In both cases, this significantly increases the cognitive load on anyone working with this logic.

// I removed all returns to focus just on the conditional nesting. 
if (!fieldsIdentified[type]) {
  if (focusElement[type]) {
    if (fieldsValid[type]) {} 
    else {} // what would be the logical condition needed to get to this else block?
  } else {
    if (
      (!fieldsValid['cvv'] || !fieldsValid['cardNumber']) &&
      !fieldsValid.hasEmptyState
    ) {}
    else {}
  }
} else {
  if (fieldsValid[type]) {}
  else {}
}

I'm not sure about you, but this structure is quite painful for me. So let's move forward by unnesting the logical structures. Since we have arranged them in the previous step, we can now act on them by transforming the nesting into sequential execution. In other words, we can flatten the nesting according to the order in which the conditions are evaluated within our function. This is achievable because we return a value after each condition is checked.

Here’s what that looks like:

  if (field.isFocusedAndNotEmpty) return styles.label;
  if (field.isInvalidAndNotEmpty) return styles.labelInvalid;
  if (field.isValidAndNotFocused) return styles.labelIdentified;
  if (field.isInvalidAndNotFocused) return styles.labelInvalidNotFocused;

  return styles.labelNotFocused;

This is a significant improvement. We can clearly and immediately see that there are four variations and one base or default state.

An important aspect to note is that we have also addressed all the negative 'else' cases of our logical conditions. These blocks are notoriously difficult to comprehend because they rely on multiple scattered dependencies.

  // there is no translation between the behaviour and return with syntax like this: 
  else {
    return styles.labelInvalid;
  }

We had 5 else statements in our old code. Some which were nested forcing us to consider even more dependencies.

With our refactor we have not only flattened the nesting but eliminated all negative cases.

The changes are subtle yet significant. We have greatly reduced the cognitive load needed to understand our functionality. Additionally, we have made excplicit the 'causes' and 'effects', without abstraction. Most importantly, the code reads like natural language, making it accessible even for non-technical stakeholders.

Decoupling

We are incorporating several external elements into this function, which makes it quite fragile due to coupling. The elements I can identify at first glance are: fieldsIdentified, focusElement, fieldsValid, and styles.

No matter which framework we are using, this situation indicates potential problems. While this used to be acceptable with the options-api in Vue. We now have improved patterns to address these concerns, and one of the main benefits of the composition-api is to resolve this by colocating related deps outside the component itself.

Therefore we can continue our code review by proposing several methods to decouple all dependencies while also highlighting them. A sensible approach would be to create a method called dynamicStyles within a composable for our feature. This would allow us to use it as follows:

const fieldStyle = computed(() => dynamicStyles(styles)

The only external dependency will be styles, as CSS modules are typically loaded at the component level. However, we need to make the deps of our method customizable from outside the function. To achieve this, we can modify the function signature of our method as follows:

export const dynamicStyles = (
  styles,
  deps = { type, fieldsValid, fieldState }
) => {}

This makes our new method pure, allowing us to achieve the best of both worlds: external customization and straightforward usage with defaults through the composable. Moreover, these defaults clearly indicate which dependencies are required for this function. Beautiful!

We can take it one step further by deconstructing our deps early, this helps declutter the logic in our function body:

export const dynamicStyles = (
  styles,
  deps = { type, fieldsValid, fieldState }
) => {
  const { type, fieldsValid, fieldState } = deps

}

Types

We can wrap up our code review by adding types to our logic. This will not only increase the robustness of our implementation but also take care of the few magic strings we are using throughout our complex structure.

  if (
    (!fieldsValid['cvv'] || !fieldsValid['cardNumber']) &&
    !fieldsValid.hasEmptyState
  )

While in other areas, we use keyed access instead.

if (!fieldsIdentified[type]) {
  if (focusElement[type]) {
    if (fieldsValid[type]) {}
  }
}

We can remove the magic strings by creating the following two base types:

export type Field = 'cvv' | 'cardNumber';
export type State = 'Empty' | 'Valid' | 'Focused' | 'Validate';

Field takes care of strong typing our external dependency type. While State provides type safety for all state combinations required in our fieldsState reactive const.

Here’s how I would type the remainder of our logic for completeness:

export type Styles = {
  label: string;
  labelNotFocused: string;
  labelInvalid: string;
  labelInvalidNotFocused: string;
  labelIdentified: string;
};

export type Field = 'cvv' | 'cardNumber';
export type State = 'Empty' | 'Valid' | 'Focused' | 'Validate';

export type FieldsValid = boolean;

export type FieldState = 'cvvRequired' | `${Field}${State}`;

export type FieldsState = {
  [key in FieldState]: boolean;
};

export type Deps = {
  type: Field;
  fieldsValid: FieldsValid;
  fieldsState: FieldsState;
};

Tests

Unfortunately, we couldn't implement Test-Driven Development (TDD) for this code since it was originally created without tests before our review. However, since Behavior-Driven Development (BDD) guided our refactor, we can now introduce tests with minimal effort. This will establish a solid feedback loop to confidently extend, debug, or maintain this feature moving forward.

Unit

I will spare you the implementation details of the overall feature but since we are working with the composition-api from Vue, we can test our improvements in isolation because they are driven only by a reactive constant and a computed property. This is important to document the business or acceptance criteria for the feature.

import { toValue } from 'vue';
import { describe, test, expect, vi, beforeEach } from 'vitest';
import { useComposable } from './composable';

// MOCKS
const stylesMock = {
  labelNotFocused: 'LabelNotFocused',
  labelInvalidNotFocused: 'LabelInvalidNotFocus',
  labelIdentified: 'LabelIdentified',
  label: 'Label',
  labelInvalid: 'LabelInvalid',
} as const;

// TESTS
describe('Unit Tests', async () => {
  test('fieldStyle: returns LabelNotFocus class by default', async () => {
    const { fieldsStyle } = useComposable(stylesMock);

    const expectedClass = stylesMock.labelNotFocused;

    expect(fieldsStyle('cardNumber')).toBe(expectedClass);
    expect(fieldsStyle('cvv')).toBe(expectedClass);
  });

  test('fieldStyle: returns Label class while being inputted', async () => {
    const {
      fieldsStyle,
      TEST: { fieldsState },
    } = useComposable(stylesMock);

    const expectedClass = stylesMock.label;

    fieldsState.cardNumberValid = false;
    fieldsState.cardNumberFocused = true;
    fieldsState.cardNumberEmpty = false;

    expect(fieldsStyle('cardNumber')).toBe(expectedClass);

    fieldsState.cvvValid = false;
    fieldsState.cvvFocused = true;
    fieldsState.cvvEmpty = false;

    expect(fieldsStyle('cvv')).toBe(expectedClass);
  });

  test('fieldStyle: returns LabelInvalid class on invalid input blur', async () => {
    const {
      fieldsStyle,
      TEST: { fieldsState },
    } = useComposable(stylesMock);

    const expectedClass = stylesMock.labelInvalid;

    fieldsState.cardNumberFocused = false;
    fieldsState.cardNumberValid = false;
    fieldsState.cardNumberEmpty = false;
    fieldsState.cardNumberValidate = true;

    expect(fieldsStyle('cardNumber')).toBe(expectedClass);

    fieldsState.cvvFocused = false;
    fieldsState.cvvValid = false;
    fieldsState.cvvEmpty = false;
    fieldsState.cvvValidate = true;

    expect(fieldsStyle('cvv')).toBe(expectedClass);
  });

  test('fieldStyle: returns LabelInvalidNotFocus class after input was focused but left empty', async () => {
    const {
      fieldsStyle,
      TEST: { fieldsState },
    } = useComposable(stylesMock);

    const expectedClass = stylesMock.labelInvalidNotFocused;

    fieldsState.cardNumberFocused = false;
    fieldsState.cardNumberValid = false;
    fieldsState.cardNumberEmpty = true;
    fieldsState.cardNumberValidate = true;

    expect(fieldsStyle('cardNumber')).toBe(expectedClass);

    fieldsState.cvvValid = false;
    fieldsState.cvvValidate = true;
    fieldsState.cvvFocused = false;
    fieldsState.cvvEmpty = true;

    expect(fieldsStyle('cvv')).toBe(expectedClass);
  });

  test('fieldStyle: returns Identified class after valid input blur', async () => {
    const {
      fieldsStyle,
      TEST: { fieldsState },
    } = useComposable(stylesMock);

    const expectedClass = stylesMock.labelIdentified;

    fieldsState.cardNumberValid = true;
    fieldsState.cardNumberEmpty = false;
    fieldsState.cardNumberFocused = false;
    fieldsState.cardNumberValidate = true;

    expect(fieldsStyle('cardNumber')).toBe(expectedClass);

    fieldsState.cvvValid = true;
    fieldsState.cvvEmpty = false;
    fieldsState.cvvFocused = false;
    fieldsState.cvvValidate = true;

    expect(fieldsStyle('cvv')).toBe(expectedClass);
  });

  test('fieldsValid: starts with correct initial state ', () => {
    const {
      TEST: { fieldsState },
    } = useComposable(stylesMock);

    expect(fieldsState.cardNumberValid).toEqual(false);
    expect(fieldsState.cvvValid).toEqual(false);
  });

  test('fieldsValid: returns properly when cvv is required', async () => {
    const {
      fieldsValid,
      TEST: { fieldsState },
    } = useComposable(stylesMock);

    fieldsState.cvvRequired = true;
    fieldsState.cardNumberValid = true;
    fieldsState.cvvValid = true;

    expect(toValue(fieldsValid)).toBeTruthy();

    fieldsState.cvvValid = false;

    expect(toValue(fieldsValid)).toBeFalsy();
  });

  test('fieldsValid: returns properly when cvv is not required', async () => {
    const {
      fieldsValid,
      TEST: { fieldsState },
    } = useComposable(stylesMock);

    fieldsState.cvvRequired = false;
    fieldsState.cardNumberValid = false;
    fieldsState.cvvValid = true;

    expect(toValue(fieldsValid)).toBeFalsy();

    fieldsState.cardNumberValid = true;
    expect(toValue(fieldsValid)).toBeTruthy();
  });
});

Type

We are dynamically generating certain types in our code. On one hand, chaining types is a good practice because it reflects the flow of state throughout our application. On the other hand, it can create fragility if we lack immediate feedback while working with a dynamic type system. To address this issue, I have added type tests to our types file, focusing specifically on cases that could negatively impact our implementation.

This is are the type tests I proposed to include:

type UNIT_TESTS = {
  fieldsValid: Expect<TypeOf<FieldsValid, ToBe, boolean>>;
  fieldStates: Expect<
    TypeOf<
      FieldState,
      ToEqual,
      | 'cvvEmpty'
      | 'cvvValid'
      | 'cvvFocused'
      | 'cvvValidate'
      | 'cvvRequired'
      | 'cardNumberEmpty'
      | 'cardNumberValid'
      | 'cardNumberFocused'
      | 'cardNumberValidate'
    >
  >;
  fieldsState: Expect<
    TypeOf<
      FieldsState,
      ToEqual,
      {
        cvvEmpty: boolean;
        cvvValid: boolean;
        cvvValidate: boolean;
        cvvFocused: boolean;
        cvvRequired: boolean;
        cardNumberEmpty: boolean;
        cardNumberValid: boolean;
        cardNumberFocused: boolean;
        cardNumberValidate: boolean;
      }
    >
  >;
  deps: Expect<
    TypeOf<
      Deps,
      ToEqual,
      {
        type: 'cvv' | 'cardNumber';
        fieldsValid: boolean;
        fieldsState: {
          cvvEmpty: boolean;
          cvvValid: boolean;
          cvvValidate: boolean;
          cvvFocused: boolean;
          cvvRequired: boolean;
          cardNumberEmpty: boolean;
          cardNumberValid: boolean;
          cardNumberFocused: boolean;
          cardNumberValidate: boolean;
        };
      }
    >
  >;
};

I created a small library called TSPrism to easily test such dynamic types. You can find it on NPM.

Final Code

We are done! Here is the complete code after implementing all our suggestions. Thank you for sticking with me; I hope you feel as satisfied as I do.

Crushing complex nested control structures always feels great.

Take it all in and compare. You be the judge, did our suggestions improved the commit?

export const dynamicStyles = (
  styles: Style,
  deps: Deps = { type, fieldsValid, fieldState }
) => {
  const { type, fieldsValid, fieldState } = deps;

  console.log(fieldsValid, type);

  const field = {
    isInvalidAndNotEmpty: !fieldsValid && !fieldState[`${type}Empty`],
    isFocusedAndNotEmpty:
      fieldState[`${type}Focused`] && !fieldState[`${type}Empty`],
    isValidAndNotFocused: fieldsValid && !fieldState[`${type}Focused`],
    isInvalidAndNotFocused: fieldsValid && fieldState[`${type}Validate`],
  };

  if (field.isInvalidAndNotEmpty) return styles.labelInvalid;
  if (field.isFocusedAndNotEmpty) return styles.label;
  if (field.isValidAndNotFocused) return styles.labelIdentified;
  if (field.isInvalidAndNotFocused) return styles.labelInvalidNotFocused;

  return styles.labelNotFocused;
};

You can play with the whole thing in a playground by clicking the button below:

Final thoughts

Some might question whether spending so much time on a code review is worthwhile. I would say yes, but it obviously depends on the situation. I know we do not always have the time to do this. However, I firmly believe that the best time to address technical debt is before we merge changes into our main branch, when the context is clear, and the implementation details are still fresh in our minds.

In this specific case, however, I believe a thorough review was necessary. We are dealing with a third-party library over which we have no control, meaning its implementation could change at any time. At the same time, our components and design system need to sync perfectly with this external dependency.

It quickly became clear that this would be a high-maintenance feature requiring support from multiple teams. This is why I felt it was important to invest the time in this review.

I have found that when we adopt the trunk development practice of making small commits through short-lived branches, it's rare that extensive reviews such as this become necessary. Another practice I have found helpful is pair programming which can also help prevent this. Additionally, there's a reason that Test-Driven Development TDD concludes with a refactor (symbolized by blue).

Accidental complexity can easily creep into our codebase if we let imperative implementations dominate for too long.

In my opinion, we have made a valuable contribution to ourselves, our teammates, non-technical stakeholders, and the actual users of our feature. We have effectively communicated through code using a common language that is understandable to both technical and non-technical stakeholders. This will improve communication about this feature across the organization and in its documentation.

I believe it was all well worth the time because communication, maintenance, extensibility, and debugging will now be much easier.

Cheers!