A continuous improvement development journey with Nathan

This week I had a very exciting development journey with Nathanā€™s help. Nathan gave me some demonstrations on many good-practice patterns/principles that I knew existed but never put into practice, and I got on a journey that involved a lot of continuous improvement and reflections on how I structure and design my code.

My development goal

I am creating a modal to allow people configure how they want to accept tips.
There are three types: fixed amounts, percentage, smart threshold Stripe documentation here. The view is already nicely designed so my goal is to implement the designed view and create the correct model to map from the view to the object expected by Stripe (our backend service actually, but similar thing).

How I approached it in the first iteration

Plan

I was easily addicted to the data structure provided by Stripe šŸ˜‚:

1
2
3
4
5
{
  fixed_amounts: [<integer in cents>, <integer in cents>, <integer in cents>],
  percentages: [<integer>, <integer>, <integer>],
  smart_tip_threshold: <integer>
}

I wanted every component to use this data structure, and want the editing form component to do ā€œsurgicalā€ operations on this object when user makes a change in the input.

Challenges

1- required fields/ array lengths šŸ¤”

Because I want to map the array to three inputs, I immediately faced the issue of what happens if the array is undefined/has less than three values. So I made the first ugly thing (I thought it was cute back then šŸ¶)

1
2
3
4
5
{
  fixed_amounts: [undefined, undefined, undefined],
  percentages: [undefined, undefined, undefined],
  smart_tip_threshold: undefined
}

Then as I mentioned, I planned a ā€œsurgicalā€ approach to change the object, I heavily used this helper method to do some surgeries on the object while user is updating the input:

1
2
3
4
5
6
7
8
/**
 * Set array's element at given index to the given value
 */
export const setValueAtIndex = <T>(
  array: T[],
  value: T,
  index: number
): T[] => [...array.slice(0, index), value, ...array.slice(index + 1)]

2 - cent <-> dollars

We want to display dollars and let user edit in dollars, but the data structure Iā€™m addicted to is in cents. The issue is in the conversion, rounding issues will happen. e.g. if user type 6.4444, input will become 6.444399999999999
So I created a second ugly thing to keep track of the object while user is editing.

1
2
3
4
5
export interface TippingConfigurationInEdit {
  fixed_amounts_in_dollar: Array<number | undefined>
  percentages: Array<number | undefined>
  smart_tip_threshold_in_dollar?: number
}

This also leads to some complicated logic to convert TippingConfigurationInEdit to the object of the original structure in cents + clear the unexpected fields (e.g. clear percentages if user chose to use fixed amounts).

3 - flag errors for individual inputs, and disable/enable buttons correspondingly

Well, once I have something like TippingConfigurationInEdit, thereā€™s nothing too wired for me to feel comfortable to make. So I made this to keep track of each fieldsā€™ errors:

1
2
3
4
5
export interface TippingConfigurationErrors {
  fixed_amounts_in_dollar_errors: Array<boolean | undefined>
  percentages_errors: Array<boolean | undefined>
  smart_tip_threshold_error: boolean | undefined
}

This is where I could have noticed some violation of DRY principle - the logic to check that no errors are present for the current configuration is almost exactly the same as the logic to check all required fields are filled (and equally cumbersome given that not all fields are useful for fixed amount/percentage configuration), yet thereā€™s no easy way to extract this logic.

4 - Organization of components

There are two steps when user are adding a tipping configuration: select outlet -> tipping configuration.

Then there are some fine edge cases for design:

  1. if there is only one outlet, go directly to tipping configuration
  2. if user clicks ā€œeditā€ button instead of ā€œadd newā€, go directly to tipping configuration too, but with different button text and headings for the modal
  3. allow user to go back to outlet selection if there are multiple outlets and user is creating a new one

Because my components are very affective with each other, the parent component controlling which model to open become very ā€œstatefulā€. I wound up having a state structure like this:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
type TippingModalStep =
  | { step: 'off' }
  | {
      step: 'selectOutlet'
      outlets: Outlet[]
    }
  | {
      step: 'editConfig'
      outlet: Outlet
      isNewConfig: boolean
      allowBack: boolean
      // TODO pass original tipping config information to edit modal
    }
  | {
      step: 'deleteConfig'
      outlet: Outlet
    }

See the TODO, this is not even all of it yet:)

My feelings after the 1st iteration

I was super relieved when everything was working and the UI behaviours met the requirement šŸ˜Œ I actually feel quite happy that I ā€œhackedšŸ„·ā€ the way through it , like, I knew it was not pretty here and there, but what can I do when Stripe gives me this cumbersome data structure? and I even told hubby proudly, I did so well surviving the mess šŸ˜‚

My attempts to mitigate the ugly things

I did some efforts to mitigate ugly parts. Iā€™m quite happy I did as it was very helpful for the continuous improvement:

  1. I added comments on the data structure like TippingConfigurationInEdit to explain what was happening and to help people understanding. (I read recently that too much non-doc comment itself is a good indicator of unmaintainable code, but I wasnā€™t relating this with what I was doing back then)
  2. I added lots of unit tests to the big fat util functions I created. These tests will not end up going into prod as the fat utils are not needed in the end, but having the tests in place helped a lot while I was refactoring and checking that only the ā€œexpectedā€ tests are failing.

My discussion with Nathan to go through the 1st iteration

I was expecting to receive some great feedbacks from Nathan to further mitigate the ugly things, but I was not expecting that Nathan helped me discover a whole new path of approaching this.

To start with summarizing some important thoughts I got from Nathanā€™s feedback and questions:

Do authentic modularizing

The whole point of modularizing things is to decouple and to isolate things. So I should not base child/grandchild component design decisions on their parents. More specifically for my approach, I shouldnā€™t have been so obsessed with the Stripe tipping config data structure that only the parent responsible for submitting it should know.

Nathan demoed how he would look at the design and discover the ā€œboundariesā€ between components. By contrast I keep starting from a parent and take things out when parents become too big. Both approaches are necessary from time to time, but the ā€œboundaryā€ way of thinking is what I lack when doing frontend dev.

Donā€™t think about the child component in relationship to its parent, think about how you would write it itself! Then think about the data structure you need, and then put it to the parent.

The concept of ā€œAdapterā€!

Heard the word adapter from blogs but thought it was for larger systems converting big fat data into another set of big fat data. But actually it could be for converting the very ā€œknowledgeableā€ stripe data structure into something nice and simple for a child component that only cares about basic tipping configuration instead of the dynamic one.

Some nice onChange function will adapt the child componentā€™s private, simplified data structure to the fat one.

Stop and track where the smell is baked

I shouldnā€™t have stopped at mitigating smells.

I should track up where the smells come from.

For example, if I keep needing many if blocks to decide which field to operate, should these fields be put together from the very beginning? They are apparently not cohesive, why do they have to stay together?

My 2nd iteration after discussion with Nathan

Resolve challenge 1: required fields/ array lengths

If I know I always need three fields, if I start from the child component, I will immediately know I donā€™t want an array. I want three fields, option1 option2 option3. Crystal clear all of a sudden šŸ”®

Resolve challenge 2: cent <-> dollars

Following 1, if I create childā€™s own component, I can create as I need it - the child DOES NOT EVEN NEED TO KNOW THE CONCEPT OF CENTS, except in the ADAPTER!

Resolve challenge 3: flag errors for individual inputs, and disable/enable buttons correspondingly

Similarly, I could have another childā€™s private component, also with three fields instead of a boolean array, for error1 error2 error3. Again the child does not need to know anything else, so bye bye big data structure and many many if conditions for validation.

Resolve challenge 4: Organization of components

This relates back to the concept of modularization and decoupling. With well defined boundaries between components, a configuration modal will keep the knowledge about steps and going back and forward to itself.

Some general reflections at this stage

It is good to have a first iteration that works in an ugly way, as this will 1) prove things can work 2) set some base grounds to collect feedback and make step-by-step improvements. So I shouldnā€™t feel bad that the first iteration is not pretty. Itā€™s just that I shouldnā€™t stop there. Looking back at this journey with Sam he gave the same advice too and highlighted that this is a typical engineering journey šŸ¾ šŸ¾

I can be more proactive in looking for advice when things are not as nice as I want but I cannot think of a way to improve it. Instead of submitting a not-as-nice version for review and waiting for people to RE-discover the things I already smelled, I could have more proactive and ask for a discussion before I put the PR for review. This will make the conversation easier and save lots more RE-discover time.

2nd Iteration

The biggest thing to be handled in 2nd iteration is where the conversion between the child and the parent componentsā€™ data structure happens.

A nice way Nathan and I settled on:

1
onChange: (formValues: BasicFormValues) => void

Passed from parent to child, so that child can call this onChange and parent can take care of the update of its more comprehensive data structure.