You are currently browsing the Refactoring archives.

Code Review Essentials

Friday, June 3rd, 2022

Code Reviews are an essential part of Software Engineering, providing numerous benefits for teams and the products they deliver. Having spent a significant amount of time conducting them for many years now, in this article, we will touch upon some key aspects to consider which, generally speaking, are of particular importance.

Similar to functional testing, Code Reviews provide a unique set of quality controls which help ensure standards are upheld; affording teams the ability to verify a number of critical concerns early on and within the confines of engineering specific constructs. This almost certainly yields a higher return as the time investment required to address issues at this stage requires minimal involvement across teams and functions.

Code Reviews also serve to aid in the verification and upholding of best practices, standards, and conventions across teams and within organizations. These standards can cover a broad range of concerns such as consistency, facilitation of reuse, scalability, security, optimization, readability, simplification, and any other auxiliary criteria specific to a given organization.

Additionally, the Code Review helps to confirm that requirements have been fulfilled in the context of the underlying feature being reviewed as, it is not uncommon for developers to misinterpret requirements.

Likewise, developers are generally focused on solving various small problems in a very particular and limited scope. Because of this, it is inevitable that opportunities will be missed, and oversights will be made. One of the primary responsibility of the Reviewer is to provide a holistic and broad perspective which takes into account not only the soundness of the code being reviewed, but also how it measures, complies, and integrates in the context of the larger system as a whole.

By having another set of eyes, so to speak, we arm ourselves with a very important second line of defense, as well as an agent for opportunity.

One of the most beneficial aspects of Code Reviews is the investment in overall knowledge throughout the team; and ultimately, the ROI it provides. As such, core to the Code Review is the proliferation of knowledge. This applies to both the Reviewee, and the Reviewer alike.

For the Reviewee, when areas of improvement, best practices, optimizations, abstractions and the like are outlined, an opportunity is presented for one to learn new (often improved) techniques which they may not have been aware of otherwise. This holds particularly true for more junior developers who simply have yet to acquire the experiential knowledge obtained by their more senior counterparts. By learning from the experiences of others, the Reviewee can expedite their own growth as a Developer. Here, the expectation is that, overtime, each Reviewee will have fewer and fewer of the same review comments to address as they now have a dedicated platform (even if unofficially so) from which to continually learn.

For the Reviewer, Code Reviews provide an opportunity to share knowledge and insight, while affording one the ability to obtain a broader understanding of the system in its entirety, as this knowledge is vital to providing a successful review.

Additionally, it may be necessary for a Reviewer to devise and provide solutions to problems which the may not have encountered previously and, in order to be effective, a Reviewer must be confident in the feedback and solutions they are providing. This alone affords the Reviewer themselves the ability to gain a deeper understanding of their own knowledge, while also challenging themselves in order to obtain the information necessary to do so. Thus, for the Reviewer, Code Reviews present a tremendous opportunity to not only provide value to others, but also to obtain and enhance their own value as well.

In general, developers more or less tend to work in a rather silo’d manner, primarily focusing on one particular problem space (particularly in the scope of a given feature), and only collaborating when necessitated by DSMs, meetings, or when they or another team member runs into a problem and needs assistance. While much of this is a rather natural by-product of feature development, so to, can it be said that Code Reviews naturally cultivate collaboration; thus, collaboration can be built into our processes by default.

With Code Reviews, no one Developer is ever working completely on their own. This has numerous benefits, many of which have already been outlined above, yet perhaps one of the most significant benefits is that developers are much more likely to double check their work and submit something that they can be proud of when they know someone else on their team will be reviewing their work. Likewise, Reviewers, no matter how experienced, are much more likely to validate and double check their feedback for the exact same reasons. This alone lends itself to higher quality output across the board.

Key Aspects to Consider

While numerous aspects must be considered with respect to conducting Code Reviews, generally speaking, there are common considerations which by and large tend to hold true. While certainly not an exhaustive treatise, what follows is a brief outline of those I have found to provide particular value.

Atomicity: PRs should be atomic (relatively small in nature). If PR is excessively large, it should be rejected and the engineer should be informed to break out the PR into smaller submissions (generally these smaller submissions can be merged to an intermediary branch before being merged to the intended target branch). This is crucial as the surface area for mistakes and missed opportunities is proportional to the amount of code being reviewed. In addition, requiring PRs which are smaller in scope encourages developers to think in terms of smaller units of function and subsystems, which in turn leads to clearer separation of concerns, and encapsulation. As such, it is often helpful to impose a change threshold for submitted PRs.

Compatibility: Changes should remain backwards compatible and not introduce breaking changes (unless expressly coordinated across teams). Reviewers need not checkout each PR and explicitly test each feature being submitted, rather, they should always be cautious of breaking changes, particularly in terms of APIs (e.g. argument positions changing, etc.).

Consistency: PRs must fully adhere to well documented and established standards and conventions; typically supported via commit convention tooling. This is crucial as, consistency and conformity of standards leads to a unified codebase where developers can easily work across packages and features with very limited effort as, the overall structure and coding style is consistent; making it much easier to know where everything should, and is, defined, how modules are organized, and readability is immediate as formatting and structure remains the same across packages and modules.

Clarity: All modules, functions, classes, types, etc. are always be clearly named, defined, remain properly encapsulated, and reside within a logical and appropriate location.

Readability: Readability should be favored over excessive succinctness or overly “clever” implementations which do not read well. Conversely, overly verbose implementations are to be avoided as well. It is important to remain cognizant of the fact that code is read many, many more times than it is written. Moreover, when implementations become hard to reason about, that is often a sign of a poor implementation (usually the result of a specific unit doing too far much). Succinct, yet meaningful names must always be used. Strive to ensure code is self documenting in terms of its intention.

Reusability: Implementations must take reuse into account at all times; be it abstractions to common packages, abstractions within a particular project, or abstractions within a particular scope of a project. In addition, Reviewers should always be on the look out for additions which are redundant and should be removed and replaced with existing APIs available. This includes both internal APIs, as well as third-party libraries. Always ensure native APIs are being leveraged (Array.forEach, etc. rather than explicit for loops) as well as standard third party libraries (lodash.debounce, etc. rather than custom implementations). No redundancies should be introduced, and implementations should fully utilize existing APIs, Modules, Components, etc. throughout the available packages.

Simplicity: Solutions should always be implemented in the simplest way possible. Less is more, this extends down to each line of code. Keep things as simple as possible, but no simpler.

Scalability: Implementations must be performant and optimized to an acceptable and expected level – generalized optimizations must be made, and premature optimizations should only be suggested when necessary.

Securability: Implementations must be secure, keeping standardized security measures in place and ensuring attack vectors and cumulative surfaces are fully understood, accounted for, and securely addressed.

Discoverability: Documentation and / or related tools should follow specific conventions and remain succinct and to the point. Ideal documentation should provide a meaningful, yet brief description, followed by a useful example which speaks for itself (often, unit test expectations can be used verbatim here). On a related note, sources should not contain overly verbose inline comments as well. When, for example, a function has more lines of inline comments than actual implementation code, that’s usually a sign that the code does not read well, or the developer has merely been leaving “note to self” comments. In such cases, strive to provide ways to simplify the implementation such that it achieves better readability by being self documenting.

Accountability: It is crucial that all Team members are aware of the criteria against which their code will be reviewed as, doing so essentially holds developers accountable for ensuring they not only understand what is expected, but are diligent in reviewing their own work prior to submission. Developers should be encouraged to pre-submit PRs for performing a “self review” prior to officially submitting and / or assigning a reviewer. This approach is quite valuable as it provides the developer with a high-level overview of their changes outside of the environment they have been working in, and within the context of the branch to which their changes will be integrated.

While there are certainly other factors to consider when conducting Code Reviews, the above considerations touch upon some of the more fundamental aspects, with the key points hopefully being apparent as, perhaps the most important trait of a successful reviewer is in one’s ability to clearly express intent while also passing this knowledge on to others.

June 3, 2022 Agile / Code Review / Refactoring / Software Engineering
Comments Off on Code Review Essentials

Separation of Concerns: propTypes and Immutable.js

Wednesday, July 25th, 2018

When considering the separation of concerns between Container and Presentational Components (stateful / stateless components), I find it useful to leverage the core concepts of these patterns in order to define a clear boundary between where Immutable data types are used, and where raw JavaScript types are referenced exclusively.

By having a clear separation which compartmentalizes where Immutable types are used and where they are not, team members are afforded the ability to easily determine a components propTypes; as, without having a clear cut-off point, one must give thought as to if a prop passed down to a component will be an Immutable object, or not.

It’s no stretch of the imagination to see how this can quickly lead to code which becomes much harder to maintain than it needs to be. As such, the Container / Presentational Component pattern provides a rather natural boundary for separating these concerns.

Unfortunately; however, while such a boundary may seem rather obvious, it may not always be clearly defined, and this tends to lead to overly complex propType declarations.

For instance, on a number of occasions I’ve seen propTypes declared similar to the following:

Given the above example, it’s obvious that it was unclear to the original implementor (or current maintainer) of SomePresentationalComponent as to what the expected propTypes will ultimately be. In certain cases, it appears someList could be of type array; whereas, in other cases, it could be of type object (e.g. Immutable.List). Likewise, in some cases someItem could be an object, whereas in others it could be an Immutable.Map.

As you can see, this is obviously problematic and indeed a very good candidate for a bug (not to mention, a maintenance headache indeed).

Moreover, it results in all sorts of unnecessary type check permutations before accessing properties. For example, just to check the length of the list:

Likewise, just to get the id of someItem:

At best, this is far from ideal, to say the very least …

Now, obviously the developer could simply define a single propType and refactor Containers which are passing an invalid type; however, it may not always be clear what the type should be, if say, the component is being used by multiple applications to which the developer does not have access, and some of those applications are not using Immutable.js, in which case, it would be best to simply disallow Immutable from the component all together and have consumers of the component update their Containers. In any event, it’s symptomatic of a team not having a clear understanding of what kind of components work exclusively with Immutable data types, and which do not.

Solutions

Fortunately, as one might imagine, there are a couple of very simple solutions to this problem:

  1. Only use Immutable types throughout the entire application.
  2. Segment which components use Immutable types, and which do not.

Now, in some cases the argument for Option #1 may very well be a valid one; however, I find Option #2 to be much more feasible (and flexible) as, it helps to ensure Presentational component are kept pure, and that means only using JavaScript types. For my purposes, this is especially important as I have to maintain a shared library which must limit dependencies as much as possible; and some projects are using Immutable, Redux, etc., and some are not. As always – consider the context.

Pros

By having an internal design contract (or convention) which mandates that Container components are only ever to work with Immutable types and, Presentational components are only ever to be passed JavaScript data types, it becomes much clearer to team members where the boundary is defined, and thus, much easier to maintain a large application over time.

Furthermore, it allows less experienced developers to gradually become acclimated with the React Ecosystem by assigning them tasks focused on presentational features. This can be very useful as it only requires knowledge of core concepts without being inundated with additional libraries and APIs. This approach also affords team members with more experience to focus on the more complex portions of the application (application logic, reducers, containers, etc.).

In addition, destructuring, …rest parameters and related ES6 features can be used much more extensively to simplify implementation when using JavaScript types exclusively, helping to ensure Presentational components are kept intentionally “dumb”. Not to mention, in doing so, testing becomes considerably less complex when working with native JavaScript types – and this is equally important when helping newer developers become productive while still getting up to speed.

And, while not always likely, by reducing our dependency on Immutable.js, we position ourselves for a much more easier migration path in the event we decide to swap out Immutable for another library in the future.

Cons

Arguably, one could be justified in the assertion that only Immutable Data types should be used by both Container and Presentational components (Option #1), and indeed that would be a fair argument if you will be calling toJS() frequently when passing props down to Presentational Components (as there is obviously an inherent expense in doing so).

That being said, there is no reason why one would need to call toJS when passing props to Presentational Components as the Immutable API can be utilized to reduce the given props before being passed down to child components. In such cases, a Higher Order Component can be defined for doing either, which can simplify implementation considerably.

Summary

Like most design decisions, there is rarely a one-size-fits-all approach that perfectly solves any given problem, and what ultimately makes sense in one context, may not always be appropriate in another. However, in the context of when and where Immutable types are used, in most cases it is fair to say there should always be a clear boundary defined, regardless of where that boundary must be.

July 25, 2018 Code Review / Design Patterns / JavaScript / React / Refactoring / Software Engineering
Comments Off on Separation of Concerns: propTypes and Immutable.js

React PropTypes and ES6 Destructuring

Monday, April 24th, 2017

At times one may be justified in the argument that cognitive (over)load is just an expected part of the overall developer experience. Fortunately, there are numerous steps we can take to minimize the general noise which tends to distract our intended focus. One particularly simple – yet effective – example is to remove unnecessary redundancy wherever possible. In doing so, we afford both our peers and ourselves a codebase which, over time, becomes considerably easier to maintain.

For instance, when performing code reviews, more often than not I tend to see considerable redundancy when specifying React PropTypes. Typically, something along the lines of:

As can be seen, with each new prop we are redundantly referencing React PropType lookup paths. And, while the ideal components will have a limited number of props (either connected directly, or passed down), the redundancy still remains for any component which references the same prop type. Considering the number of components a given application may contain, we can rightfully assume that the above redundancy will grow proportionally.

With the above in mind, we can easily reduce the redundancy (as well as micro-optimize the lookup paths) be simply destructuring the props of interest as follows:

While I would consider the above to be simplified enough; one could also take this a step further and destructure the isRequired props, which, in some circumstances, may be useful as well:

Admittedly, this example is rather straight-forward; however, it does help to emphasize the point that only through consistent vigilance can we ensure our source will continue to evolve organically while remaining as simple as possible.

April 24, 2017 Code Review / Design Patterns / ES+ / JavaScript / Quick Tips / React / Refactoring
1 comment

Simplified Partial Application with ES6

Wednesday, June 1st, 2016

When implementing Partial Application in ES6, implementations naturally become quite easier to reason about as default parameters, rest parameters and arrow functions can be leveraged to provide a much more comprehensive implementation.

While on the surface this may appear insignificant, when compared to having relied almost exclusively on the arguments object and Array.prototype to provide the same functionality in ES5, the benefits become rather apparent.

For instance, consider a simple multiply function which, depending on the arity of the invocation, either computes basic multiplication against the provided parameters, or returns a partial application. That is to say, if invoked as a unary function (single argument), the function returns a partial application (a new function which multiplies by the given argument). If invoked as a variadic function (variable amount of arguments), the function returns the product of the arguments.

In ES5, we could implement such a function as follows:

View Pen

Given the above example, in order to inspect and iterate over the provided arguments, we need to rely on the Array.prototype, specifically, we need to invoke Function.prototype.call on Array prototype in order to apply the slice method so as to convert the arguments object to an Array. Additionally, we also have to account for a default value of arguments[0] should it be omitted or NaN.

Not only does this require a superfluous amount of code, but it also results in a more complicated implementation that becomes considerably more verbose, and as a result, more difficult to reason about; especially for developers who may not be familiar with the specific mechanisms employed within the implementation.

ES6 to the rescue …

With the introduction of default parameters, …rest parameters, and Arrow Functions (fat arrows) in ES6, the implementation of the above example can be significantly reduced, and as a result, becomes considerably easier to understand, as we can simply re-write the multiply function as:

View Pen

As can be seen, implementing the multiply function in ES6 not only reduces the SLOC by 1/2 of the previous ES5 implementation, but more importantly, by using rest parameters, it allows us to determine and work with the functions arity in a much more natural way. Moreover, both iterating over the provided arguments and returning the partial application becomes considerably more concise simply by using arrow functions, and the need to account for undefined arguments becomes moot thanks to default parameters.

In addition, variadic invocations of such functions can also be simplified considerably using the ES6 spread operator. For example, in order to pass an Array of arguments to a function in ES5, one would need to call Function.apply against the function, like so:

With ES6 spread operators, however, we can simply invoke the function directly with the given array preceded by the spread operator:

Simple!

Hopefully this article has shed some light on a few of the features available in ES6 which allow for writing implementations which not only read much more naturally, but can be written with considerably less mental overhead.

June 1, 2016 Code Review / ES+ / Functional Programming / JavaScript / Refactoring
1 comment

IIFE in ES6

Wednesday, April 6th, 2016

TL;DR: In ES6, an IIFE is implemented as follows:

…
Unlike ES5, which is syntactically less opinionated, in ES6, when using an IIFE, parenthetical order matters.

For instance, in ES5 an IIFE could be written either as:

… or

As can be seen in the above examples, in ES5 one could either wrap and invoke a function expression in parentheses, or wrap the function expression in parentheses and invoke the function outside of the parentheses.

However, in ES6, the former throws an exception, thus, one can not use:

But rather, the invocation must be made outside of the parentheses as follows:

As an aside for those who are curious, the syntax requirements are specific to ES6 and not a by-product of any particular transpilers (i.e. Babel, Traceur, etc.).

April 6, 2016 Code Review / ES+ / Quick Tips / Refactoring
1 comment

Polymer Behaviors in ES6

Friday, March 25th, 2016

Being a typical aspect of Object Oriented Design, inheritance, and mixins, provide the means by which modular reuse patterns can be facilitated within a given system. Similarly, Polymer facilitates code reuse patterns by employing the notion of shared behaviors modules. Let’s take a quick look at how to leverage them in Polymer when using ES6 classes.

Implementing Behaviors

Implementing a Behavior is quite simple, just define an object within a block expression or an IIFE, and expose it via a namespace, or module loader of choice:

some-behaviors.js

Then, include the behavior in a corresponding .html document of the same name so as to allow the behavior to be imported by subsequent elements:

some-behavior.html

Extending Behaviors

After having defined and exposed a given Behavior, the Behavior can then be extended from element classes by defining a behaviors getter / setter as follows:

Once the behavior has been extended, simply import the behavior in the element’s template (or element bundle, etc.) and it is available to the template class:

Try it

Implementing Multiple Behaviors

Similar to individual behaviors, multiple behaviors can also be defined and extended:

first-behavior.js

second-behavior.js

In certain cases, I have found it helpful to group related behaviors together within a new behaviors (array) which bundles the individual behaviors together:

Note: As can be seen in the FourthBehavior, a behavior can also be implemented as an Array of previously defined behaviors.

Extending Multiple Behaviors

As with extending individual behaviors, multiple behaviors can also be extended using a behaviors getter / setter. However, when extending multiple behaviors in ES6, there are syntactic differences which one must take note of. Specifically, the behaviors getter must be implemented as follows:

Try it

And that’s basically all there is to it. Hopefully this article helped outline how Polymer Behaviors can easily be leveraged when implementing elements as ES6 classes. Enjoy.

March 25, 2016 Design Patterns / ES+ / JavaScript / Refactoring
1 comment
← Older Entries