An Example Code Review

This is a companion piece to my article https://medium.com/@ExplosionPills/on-code-reviews-in-2021-71febc88f231. Here, I take a code review that I did in the recent past and extract some points of interest for discussion. This includes diffs or explanations of the code changes as well as direct quotes from the review. I will also editorialize with explanations and corrections to the review itself.

This is for a React Native mobile retail app, but I will attempt to make the content generic enough to be understood for any software development language or platform. Some identifying features of the app or its developers have been intentionally obscured. I compliment anyone from the team who sees this as they did a great job, worked well with this initial feedback, and provided great feedback themselves.

Any Mobiquity developers who would like to see the review itself, just let me know.

Introduction

This was a general review of one of the early stages of the application. My approach was to merge in some of the functionality and then create my own merge request that included some refactoring. I discuss this as one possible approach to code review in my previous article. This includes a substantial amount of changes which does go against conventional wisdom and even my own advice that pull requests should be kept relatively small, but there are a couple of sticking points:

1. I am from the US and was working with a team that was entirely in India. This dramatically increases the turnaround time for making changes.
2. This was in the relatively early stages of the app, so I wanted to try to get some advice and best practices out quickly as there were many fresh features being introduced.

I also tried to offset some of the overhead for the review by including inline comments explaining my changes. I asked the developers to review my comments and changes in lieu of normal development time for a day or two in an attempt to ease the stress of having to read the large review while also having development work. I could have taken a different or better approach, but we were still ultimately able to achieve our goals with the app, and I was very satisfied with the product’s code quality.

I’ll start with my merge request’s description:

Refactor and Improvements

This merge request includes all of my suggested changes for the logic, code organization, and naming in the app. For the most part, this does not include any functional changes. It attempts to carry over all of the existing app functionality identically with a couple of notable functional changes:

1. The refresh token stored in the application state is automatically used when it’s necessary. This doesn’t really apply to the existing app, but this functionality can be carried over.
2. The sign in screen UX has been updated to better support user keyboard interaction. You should be able to “next” and “submit” from the keyboard. This was done as part of a larger refactor for InputField.

Some smaller functional changes that attempt to improve user experience include better request and error handling.

All of the other changes will be explained in inline comments in the merge request. Some common themes are:

• Unifying naming conventions, clarifying names, and correcting spelling.
• Splitting up components for clarity, organization, and potential reusability.
• Refactoring state management for specificity, clarity, and moving items as close as possible to their intended use.
• Adding unit tests to include examples of reducer tests, epic tests, and Page tests.
• Removing as many eslint-disable comments as possible.

These changes are suggestions for how I would write the app. I am certain that I made some mistakes, and I am certain that it can be improved upon as well. I think some of these changes are also subjective. Please keep this in mind when reviewing the changes and feel free to either suggest improvements or to make future alterations.

I think that I tend to struggle with introductions and conclusions. I state that the code “for the most part” doesn’t include functional changes and undercut that by explaining the major functional changes. My explanation of the refresh token, This doesn’t really apply to the existing app, but this functionality can be carried over, is also unclear to me. It may have made more sense in context at the time, but I find this difficult to understand. I think that my point is that this is a feature that isn’t used now but can be used in the future.

I can also reduce my use of qualifying words. For example, “potential reusability” should just be “reusability.” My goal in qualifying statements is to avoid being presumptuous… I don’t assume that the code will be more reusable. However, I should be confident in this goal and imbue this confidence in the team as well. It would also reduce verbosity which is important for anyone reading the review.

I think this introduction and explanation of my changes and their goals is okay, but I should have read through it and edited it a couple more times to provide clarity. Don’t forget to review your review.

My Feedback in the Review

I’ve split the feedback into logical sections. Content from the actual review will be quoted. I’ve also provided a diff of the original code when it adds clarity. Otherwise, I summarize the changes that were made.

Introducing a Reusable Component

I created a BiometryCheckbox component. Originally, the code had the checkbox and other biometry checks inline on the existing sign-in page.

This element handles the functionality for displaying the “Use Face ID,” etc. checkbox. It was convenient to split it into its own component to simplify the sign in page, and it could potentially be reused in other places such as the registration page.

This encapsulates the sensor check and biometric type text.

Creating generic reusable components is difficult to do because it can be the wrong abstraction. I wrote this component because I knew it was going to be used on multiple pages in the same way and to provide an example of an abstract, reusable component for the team.

While abstraction often implies reuse, it is only one goal. Abstraction can also provide clarity and organization. In this case, it made sense to me that since the biometric checkbox should only appear if the sensor is available, the component itself can encapsulate the logic for the sensor.

Unnecessary Code / Types

FunctionComponent is only needed to simplify specifying the prop types for functions that are components. Otherwise, the component is implicitly a function component as long as it returns a JSX element.

The page-level component will never have props because they are top level components.

I also adopted the “Page” suffix to clarify these.

This is a bit specific to React semantics. The SignIn component is compatible with FunctionComponent, so the correctness here is debatable, but all other things being equal, I err on the side of using less code rather than more. Fresh and even experienced React developers may not know what FunctionComponent does here, or that it is not needed, so I see it as a distraction that can be safely removed.

You could make the argument that in the future, this component will take props that will make FunctionComponent useful. If you introduce props and want type safety, then you would have to import and add this type. I prefer to avoid code that serves no purpose now but might be useful in the future for a few philosophical reasons:

• YAGNI (you ain’t gonna need it) — don’t add functionality until it is deemed necessary.
• Code should be optimized for reading over writing, so removing a distraction is better than including code that may or may not be used in the future.
• This is Typescript and React specific, but inferred typing, where sufficient, is less verbose than explicit typing. Your tools may vary, so don’t think of this as an absolute best practice.

Moreover, in this specific case, this is a top-level component which can never have props, so FunctionComponent will never add additional type safety. I added the “Page” suffix to signify this. I’m ambivalent about how useful these suffixes are since they can be redundant and difficult to enforce programmatically, but since our teams do both React and Angular development, and the Page suffix makes more sense with Angular’s style guide, I like the cross-platform consistency approach. As with many developers, I will forever be tortured by the question of whether this naming decision was the right choice.

Finally, I brought this up as a learning opportunity. The developer may not have been aware that the explicit FunctionComponent type was unnecessary. If they did know, this would give them an opportunity to justify their choice, and we would both learn something.

Symbol Name Formatting

I changed the locale files to use camelCase. There doesn’t appear to be a set standard for i18n/React Native, although examples I found do use camelCase. This is more in line with how object definitions are done more universally.

There seems to be a fine line between what is a nitpick or personal preference and a tangible improvement in code writing. Naming, and particularly name formatting, are at the forefront of this battleground. The reference to Phil Karlton’s quote, There are only two hard things in Computer Science: cache invalidation and naming things, is thrown around often, and probably much more often than issues in cache invalidation are encountered.

When it comes to this change, I don’t have an issue with the naming convention itself, but my goal is to achieve consistency in naming overall. Other examples of inconsistencies in the locale file naming were the use of groupings like menu and landing on the same level as SignIn, Switch_Language, and STORE . Any of these naming formats would be okay, but a lack of consistency can make values more difficult to identify. This can affect a new developer and even an existing developer on the project in a few ways:

1. The lack of consistency itself can be distracting.
2. Without a consistent reference, the developer will have to spend extra time choosing how they want to name new values.
3. Manually named values may be inconsistent or incompatible with those created by automated tools which can propagate inconsistency.

When it comes to things like naming conventions, you should follow your organization’s standards, and, unless there is a good reason, you should try to follow industry or library standards as well. These don’t always exist as in my i18n React Native example, so sometimes you have to make a choice, but you should only deviate if you can justify the reason why.

My feedback can be too verbose. I also used the qualifier “more” twice in the same sentence. What I should have written is this follows our standards for variable and object naming.

Variable Naming

The purpose of these values is mostly the same, but they were renamed for clarity:

* isErrorVisible -> isEmailErrorVisible since this error is specific to the email field

* setX -> setIsX to match the property name that is being set (in the case of these booleans).

There are a few quality aspects going on here that I will discuss, but this continues the theme of correct naming. I don’t fault anyone for choosing names that are not optimal on the first pass. In fact, I think that code review is a great opportunity to offer clearer or more accurate variable and property names. As long as the author and reviewer maintain the grateful, inquiring, and kind mindset, there can be a free exchange of potential names that will approach the correct one.

It seems to me, at least in my organization, that code reviews don’t point out potential name improvements as often as they could. Even I am guilty of this for a variety of reasons. Naming is difficult, but one way to improve it is through the ideas offered through a code review. Suggesting an improvement to a name does not require a change to the name either, but even the suggestion can provide more clarity about the original name to the author and reviewer.

In this example, the renaming was done for clarity and consistency. Names should be clear with as little context as necessary. isErrorVisible is understandable — it is a boolean that indicates whether an error is visible or not — but in the context of the page, there can be multiple errors. Thus, to provide clarity, I changed it to isEmailErrorVisible to indicate how the error is generated and where it is displayed. An even more accurate name may be shouldShowEmailError. This may be better since we currently have a mixture of is and should prefixes.

We could also have an errors object with properties or use a form management library. The former is a bit more difficult to manage in React, so this choice is contextual to the tools. The latter would require more overhead, but it would not be a bad option in the larger application context. Finally, this is following React’s own naming and usage standards according to their documentation.

On the other hand, the is properties control whether the errors are displayed, and the should property is related to a functional aspect of user interaction. As there is a separation in functionality, a separation in the naming strategy may be appropriate. Your team may want to adjudicate naming strategies in cases such as these, but this takes additional time and isn’t always necessary. If the team is tacitly consistent with an implicit naming strategy, that’s probably an indication that it’s working well.

I learned a long time ago that it’s helpful to prefix booleans that operate like flags such as these with is, should, has . There is a worthy debate about how useful this practice is, but it helps to distinguish values that manage control flow versus those that contain content or some other functionality. My team and I seem to have adopted this naming strategy more or less automatically, so we’ll stick with it for now.

The React state setter names should match with the state variables except for the set prefix. This makes the state value easier to identify in the context of its setter. It also provides consistency. Above, there is isErrorVisible and setErrorVisible versus isButtonTap an setIsButtonTap . I’d rather short circuit the thought process and simply prefix the setter with set.

My review here is incomplete. isErrorVisibleOnTop is unclear and probably unnecessarily verbose. There are some other clarifications I could have made, but it is a step in the right direction. The review has to come to an end at some point.

Along the same lines:

For convention and clarity, we prefix all of the selector functions with select. This also allows us to use names that match selected values. That is, we don't have to do isLoader = useSelector(isLoading) and instead could do isLoading = useSelector(selectIsLoading).

For style declaration files:

This is an example of a more general change to the style definitions. Style properties should be camelCased and not PascalCased as with other object property definitions. Only class/interface/component definitions should be in PascalCase.

I also removed the Style suffix from style properties since they will almost always be prefixed by the style object that has style in its name, style. which would make the suffix redundant. It was also not being used consistently. I can see why it might be used in cases such as this where you have a library prop that has a style suffix too, but I think it's ultimately clearer and more succinct to leave it off.

contentContainer is my attempt to be more clear. This styles a generic container for the page’s content. content or container may be appropriate names as well. When it comes to naming, there is always room for debate, but I’ll reiterate that you ultimately have to make a decision and move on.

Properties and Names from Third Parties

Not all of the information (e.g. token_type) needs to be stored in the state -- only what we need to use. This was originally done because the API responses were being passed into the state directly, but for clarity, the state should only hold values that it needs and they should be named according to the application code conventions (camelCase property names).

The purpose of the application state layer is to transition properties that we may receive from the API or otherwise and make them palatable to the application logic, using our terms and naming conventions.

string | null generally doesn't need to be used. We can make the property optional instead with a question mark ?, but this should be avoided in cases where it's not needed too since that creates more conditional chains in the application logic.

There is a temptation to align properties with API responses. If you have total control over the API definition, you can certainly reuse types across platforms, and the teams should develop the types and names together. You can even have a shared library of types or data structure definitions.

Often, you will have to consume third party APIs you don’t control whose conventions will not match your application’s. These third party APIs may also respond with properties that are not used by the application. I believe it is better to include properties as-needed rather than to create definitions that match the entire shape of the API response. You can create an object definition that matches the useful properties of the API since you will need to access these, but the objects used by the application should follow your standards.

Then, in your application, you can use this as authState.authToken = authApiResponse.access_token.

The final point about optional properties has one TypeScript-specific point. There is a functional difference between an optional property ? and the string | null union type, but there’s no practical difference here. I use this as an opportunity to show the developers the preferred semantics which conveniently take up less space and are easier to read.

It could be correct to not make these properties optional. When the application state first loads, they are missing, and it is theoretically possible to retrieve an access token without a loyalty token and vice versa. However, Properties of the auth state will not be used unless they have been successfully retrieved together. This provides theoretical type safety, but it also enforces checks where they may not be needed. For instance, you will always have to do authState.?authToken rather than authState.authToken, and you may have to provide additional conditional checks to handle the undefined states where this will practically provide no value since authState is only usable in components that are rendered after successful authorization. TypeScript can allow you to simplify this via type guards, so I think both options are viable.

Grammar and Pluralization in Naming

Since we are only displaying the details for individual stores, this should be singular: “StoreDetails” rather than “StoresDetails.”

These names have been updated to match corresponding action names which were also updated to start with verbs. For example, there could be a deleteOffers. signIn itself is a verb.

Keep in mind the relevance of using nouns versus verbs when necessary. Often a verb signifies an action such as a function call whereas a noun is an object instance or property. There are not hard and fast rules for the part of speech to use in naming, so the discussion should always be open. In this case, I’m also pointing out that there will be multiple epics/actions related to offers, so the previous name offers is overloaded.

I should clarify this in the review, but I typically intend retrieve and get to mean different things. In this case, retrieve will procure the credentials from local storage where as get refers to GET API calls.

A couple of things here. Items is the name of the property containing the array of offer objects from the API, but this seems to apply to other APIs as well. For clarity, we can call this offers as the list of current offers in state.

The Offer type should be singular since the object refers to a single offer, so an array of offers would be Offer[] and not Offers[].

[offerItem] in TypeScript is literally an array that has a single offerItem object. Most likely this was intended to be offerItem[] which would be an array of these objects that can be of any length (or empty). interface / object shape types should also be PascalCase, and property names should be camelCase.

These are a few more items related to naming showcasing the importance of analyzing various aspects of naming and the process it takes to get it right. Pluralization is important since a plural variable name implies a collection. The language or platform that you are working in may have conventions for naming collections as well.

I prefer to avoid using generic names such as “items” or “things.” In this case, Items came from the API response, but I’ll restate that strictly using names that come from APIs is not necessary. In this case, the interface being defined is for a component that displays a list of offers, so the more descriptive names make sense (state.offers = offersApiResponse.Items).

The last paragraph points out a misunderstanding of TypeScript array declarations that is specific to TypeScript. I offer the correction, but it is possible that the intended type was indeed [Offer] rather than Offer[]. The inquiring mindset will help reach a proper resolution for this.

Finally, the component being referenced is called OffersList which includes the plural “offers.” I’m ambivalent about this too since you could make the argument that “list” implies a collection and thus OfferList would be the correct name. I think that OfferList is ambiguous since it might be related to a list of properties related to a single offer rather than a list of offers. It’s possible that there is an even better name than these.

User Experience, OS, and Library APIs

This changes an email field in the app so that it uses the email address keyboard for mobile devices. By default, this was using the standard text keyboard and automatically capitalized the first letter. Sometimes, developers may simply not be aware of the options available for specific user experience improvements such as available keyboards. This is an opportunity for more senior developers or anyone who is familiar to share this knowledge with the team. In this case, I would include a link to the library’s input APIs that allow you to change the OS keyboard for input fields.

Once developers realize that these powers are available, they’ll be able to use them for other purposes and disseminate them to the rest of the team. It doesn’t hurt to periodically review the documentation and changelog for the libraries you commonly use. Developers, testers, and other team members should point out these potential user experience improvements. The libraries and APIs we have are so powerful nowadays that there is often a specific improvement available.

Unnecessary Asynchronization

I can’t speak for other platforms, but misuse and misunderstanding of synchronization is common in web technologies. In this case, the code was making the result of an email string validation asynchronous. However, this code doesn’t depend on anything asynchronous since validate and email.trim are not asynchronous.

This will require some synchronization where this function is called, e.g. async/await. This adds unnecessary complexity. Moreover, Promise.resolve() returns an object and all objects in JavaScript are truthy. if (validateEmail(email)) will always be true before the correction. My hope is that the lesson in this feedback will be applied more broadly. Asynchronous functionality should only be used when it’s needed.

State Management

I created this section because there is content specific to Redux and React. Without at least a high level understanding of Redux state management, these examples may not make sense. I will give a crash course.

Redux is a state management library and paradigm where the application state or store is a first class high level aspect of the software. The View is the output of the state. Actions are dispatched and update the state which in turn updates the View. Actions can be dispatched based on user interactions and other events. The state is managed by reducers which are synchronous functions that take the current state and a dispatched action as input and return the new state as output.

Visualization of Redux Software Architecture

React-Redux allows you to select state properties and also allows you to dispatch actions to update the state, often, though not always, in response to user interaction.

Organizing States

Rather than dispatch two separate actions, we consolidate the actions since they should succeed and fail together.

Dispatching two separate actions is not necessarily a bad approach, but you must analyze your state machine to see if this is desired. If you dispatch separate actions that are not dependent, you will automatically have four states:

1. Initial state (no action resolved)
2. First action has resolved, but the second has not
3. Second action has resolved, but the first has not
4. Both actions have resolved.

This also does not take into account the possibility of either action failing which results in additional possible error or recovery states. Even if the action is fire-and-forget, errors need to be noted and recovered from.

In the example above, the token is either unavailable or not useful if the sign in request fails. Thus, even though these are separate events, they are dependent, and so a single action should encapsulate both. This simplifies the possible states of your application and gives you a single point of failure for error handling. There is a worthy discussion about the level of encapsulation for actions, and this could still be two separate actions that operate in a chain. I’d prefer to keep a single action for operations that must succeed or fail together which doesn’t eliminate the addition of an action that only performs one of the operations where that may be needed.

Avoid Unnecessary Value Passthrough

I made another change to the SignInPage where I removed the retrieval of an application state access_token value and the use of that value in actions. I left the uninteresting diff out since it’s just removing a couple of lines using access_token .

The access_token is no longer needed here since it’s used in the API handling layer. It’s generally better to access application state as close as possible to where it is needed so that it is more accurate / not passed through and also more consolidated. In this case, we were selecting the accessToken from the state so that we could pass it to a request to be used as part of an API call. Instead, the API handler can get this from the state. Otherwise, we would have to select and pass this through on any page that needed to make such a call.

A good heuristic is that if you retrieve a value in one layer only to pass it to another layer without operating on it, instead you should do the retrieval in the relevant layer. This depends on your software architecture, but this is a fairly common problem in hierarchical component-based architectures where an ancestor may need to pass data to a distant descendant (or “cousin”), but we have a wealth of patterns and libraries available to assuage this problem now. This includes Redux which this app is using.

The original code was selecting the access token from Redux in the SignIn component and then passing this token when dispatching the sign-in action. Ultimately, the API layer uses this token for authenticating requests. My suggestion was to have the API layer retrieve the token for requests that require authentication.

This is specific to React and Redux, but this requires explicit retrieval of the application state in the API layer (applicationStore.getState()). This is not a bad thing, but it can make testing more complex since you will have to mock the importing of applicationStore. There may be other ways around this, but for now, I prefer this explicit retrieval in the relevant layer over passing the value through different layers without operating on it.

Conclusion

I hope this example code review helps developers learn what a code review should look like. One thing that is absent from the review that should be more common is questions. I took a different approach with this review since it is made up almost entirely of suggestions. My goal was to keep this as an open discussion for additional feedback as well as to provide some standardization for the code and as teaching materials.

I have learned a lot writing this article and going through this code review multiple times, and the review itself was created by going over the original code multiple times. Every iteration can improve quality, but you can’t forget to compromise on a reasonable stopping point. I’ll take that advice right now and end this here.