You are currently browsing the Software Engineering 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.

ES2020 Optional Chaining & Nullish Coalescing

Tuesday, January 12th, 2021

Of the various Features proposed in ES2020, perhaps two of the simplest features will prove to be the most useful, at least in terms of simplification and maintenance are concerned.

Specifically, the Optional Chaining Operator and Nullish Coalescing Operator are of particular interest as they are certain to result in less verbose, less error prone expressions.

Optional Chaining

In a nutshell, Optional Chaining provides a syntax for undefined / null checks when performing nested object references using a simple question mark appended by a dot (?.) notation.

For instance, consider how many times you may have written defensive expressions similar to the following:

Or perhaps you have assigned intermediate values to temporary variables to perform the same:

The need to check for possible reference errors quickly becomes tedious, and with each lookup we increase the potential for introducing bugs. Utilities can be implemented for delegating these checks, but ultimately, this just moves the problem from one context to another, resulting in additional points for failure.

With Optional Chaining, however, accessing properties safely becomes considerably less verbose, as the examples above can be simplified to:

Reference checks when invoking functions also become simplified:

And dynamic property references can safely be performed as well:

Nullish Coalescing

In addition, combined with the Nullish Coalescing Operator, Optional Chaining becomes even more succinct as one can specify a value to resolve to rather than the default (undefined) by simply using a double question mark (??) notation. For example:

Moreover, Nullish Coalescing, while intended as a compliment to Optional Chaining, also solves additional problems when dealing with falsy values. For instance, consider how many times you may have written something similar to the following:

With the Nullish Coalescing Operator, we can avoid the problems outlined above as only undefined and null values will evaluate to true, so falsy values are safe:

Since Nullish Coalescing only checks for undefined and null, the above holds true for all other falsy values, so false, empty strings, and NaN are safe as well..

Optional Chaining w/Destructuring

One thing to note is that Optional Chaining does not resolve when destructuring. So, for example, the following will throw an exception:

Interestingly, though, combined with Nullish Coalescing, an exception will not be raised; though, the default will not be assigned, either:

Conclusion

As can be seen, ES2020 has no shortage of new features on offer to be excited about and, while arguably not as exciting as other features, Optional Chaining combined with Nullish Coalescing will certainly prove to be valuable additions.

Both Optional Chaining and Nullish Coalescing proposals are currently at Stage 4 and are available in most modern browsers as well as via @babel/plugin-proposal-optional-chaining and @babel/plugin-proposal-nullish-coalescing-operator.

Synchronizing package.json with yarn.lock

Sunday, January 5th, 2020

After having used Yarn almost exclusively for the past couple of years, there has been one nagging issue which seemed to continually crop up. Specifically, the inability to have a project’s package.json dependency versions kept in sync with the actual versions in yarn.lock. And so, while running yarn upgrade results in all packages being updated to the latest versions (as specified via the given semver ranges), the versions defined in package.json are not updated to reflect that which they have been upgraded to.

This can prove problematic as, one can not easily discern a project’s dependency versions by simply viewing their respective values in package.json.

In particular, as part of process, after each production release I have scripts which are executed to automate the process of updating all project dependencies to their respective latest Minor and Patch revisions prior to opening master for new development. While the scripts manage the updates and committals internally, each project’s package.json would remain unmodified, making it challenging to determine which packages have been upgraded, and which have not. Having to automate or manually inspect the yarn.lock files is less than ideal, and quite cumbersome to say the least.

Fortunately, like most things in the Javascript world, there is a package for this; syncyarnlock, which provides exactly what one would need to ensure that the dependency versions defined in package.json are kept in sync with the project’s yarn.lock.

Simply install syncyarnlock, and execute with the options applicable to your needs.

For example, to sync a project’s package.json with the project’s yarn.lock, and have the ranges remain intact while updating the versions to reflect what will actually be installed, simply run: syncyarnlock -s -k.

This will result in the dependency ranges being preserved, while also updating their versions to reflect the versions that will actually be installed.

And with that, we have proper syncing. A definite time-saver!

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.

BDD/TDD Mental Models

Thursday, February 13th, 2014

Recently, I shared a simple 8-step procedure with my team which outlines some of the general questions I tend to ask myself when writing tests, even if, perhaps, only subconsciously so.

While quite simple in form, and somewhat obvious in process, this procedure helps to develop a useful mental model from which practical steps can be applied to common testing scenarios; which, in turn, helps to provide clarity of general design considerations, while also helping to guide specific implementation decisions.

First things First

Arguably, the single most important aspect of testing (and software development in general, for that matter) is to acquire a solid understanding of the problem domain; for, without having (at minimum) a general understanding of the problem one is intending to solve, important details are likely to be omitted which would have otherwise been considered, and thus, covered by our tests. Spend time understanding exactly what problem your code is intended to solve, then begin thinking about what to test for. Understand the Problem.

Small Steps

Once confident that a good understanding of the problem has been reached, we can then get started on writing our initial tests. Consider this as a first pass, if you will, whereas we are only concerned with getting our tests to pass in the simplest (typically, least elegant) way possible. The initial implementation code can be as raw (and ugly), as needed, as this can (and will) be addressed after our initial tests are passing. If we are writing tests against code that does not yet exist, then we will first write the implementation code (the code that is being tested), directly within the test case itself. Once the test passes, we can then refactor the code out from our test and into the SUT (code we are testing). If the code already exists (we are writing new tests against existing code), we still need to understand and consider the implementation of the code itself, and not just simply write tests against it. Reviewing and critiquing existing code is an excellent way of gaining a quick understanding of a given system. Seize initial opportunities. Start off slow.

Clean Pass

Once we’ve written our initial tests and they are passing, we can then safely go back into our new or existing implementation code and refactor it to our hearts content. If we break something, our tests will let us know. After all, one of the most rewarding aspect afforded by unit testing is the ability to refactor our code freely with little worry or concern that we will unintentionally break something without knowing. If something breaks, are tests will inform us. Tomorrow never comes in Software Development. Clean up as you go along.

Negative Tests

The most obvious tests to write are those which are against the things we are expecting the code to do. But what about if the code is used incorrectly? What if an argument is required and it is not provided, or it is of an invalid type? Does our code throw an exception? Does it simply return undefined? What should it do? These are all questions we should be asking ourselves once our expected test cases are passing. After that, we need to start thinking about ways to have our code appropriately respond to negative cases – we don’t want the entire app to become in an unpredictable state just because an uncaught exception was thrown due to some simple string formatting argument not being passed, etc.. Test the exceptional; Test the unexpected.

Stateless Tests

One of the most important considerations to make both during and especially after all of the above points have been considered, is the statelessness of the system while being tested. Always ask yourself, “Am I resetting the state of all my test’s dependencies back to an expected state?”. This is perhaps one of the most commonly overlooked, yet crucially important consideration to make. A good example illustrating why this is important can be found in the common scenario of a test that invokes a method which triggers an event. If any previously executed tests which handle the event have not been properly tore down (e.g. afterEach), the object will still exist; and thus handle the event. This typically results in a change in state, more often than not causing an unexpected error to be thrown. Always use set-ups (e.g. beforeEach) to configure your tests environment, fixtures, any dependencies your test requires to operate properly. If you are setting values on anything outside the context of your tests; always use mocks, stubs and tear-down methods (e.g. afterEach) to reset them back to an expected state. Remember, while your tests are not part of your applications source, they are certainly part of your projects source; this, in effect, requires them to be viewed as first class citizens; subject to the same quality design and implementation as project source. Tests will need to evolve and be continually maintained. Treat the test environment with respect; ensure you return it in a predictable state. Leave it the way you found it.

Continued Improvement

While the above description of Stateless Tests clearly states that the test environment should remain stateless, and thus “remain as we found it” prior to our tests, our actual implementations code should always be improved when improvements can be made; hence, The Broken Windows Theory is one we should all strive to live by. This especially holds true in the context of writing tests/specs against existing code. If the code is not up to par in any way – fix it. Ask yourself: “How easy was it for me to understand what this code does?”. “Is it documented in a meaningful way?”. “Would it be easier to understand if I added some quick examples?” (Often, adding examples is simple a matter of pointing to, or annotating the source with the test cases themselves). We can have the greatest, most elegant framework and foundation on which to build the greatest apps in the world, but if we allow ourselves to let our code quality degrade, our apps will gradually decay into chaos. Set a higher standard, and live by it. Leave the source better than you found it.

Meaningful Tests

It is quite easy to get caught up in the perceived quality of a system’s tests simply by measuring it against general Code Coverage metrics. This is a subject I have spoken to at length many times. While code coverage certainly has it’s purpose, and can be helpful, it is often not very reflective of reality. Judge your tests not by the number of test cases or units tested, but rather, judge based on the meaningfulness of each specific test case itself. Ask yourself “What is the overall value of this test?”, “Am I testing the obvious?” (such as a simple getter/setter). Focus on what’s important, test whats of most value first. This will afford one the satisfaction of knowing that if time constraints or something comes up which requires shifting focus to something else, the most important test cases are covered. Focus on what’s important.

Know when you are done

It is quite possible for one to go on refactoring beyond what is essential. As such, it’s important to know when you’re done. Some questions to ask yourself are: “Does the code do what it needs to do?”, “Is the code clean and understandable, performant, efficient, etc.?”. “Does it have adequate coverage?” If these questions can be answered in the affirmative, then you’re most likely done. Many times, it’s tempting to continually refactor; as the more one refactors, the more opportunities for further abstractions begin to arise. When confident that your most important objectives have been met, you’re done. No when to stop.

Concluding Thoughts

It is important to note that the above considerations are by no means exhaustive – and this is intentionally so; as each point is specifically intended to provide just enough guidance to sufficiently ask the right questions, and thus solve problems in a pragmatic manner.

Over the years, I have found that it can be particularly helpful for developers new to a specific domain, or new to TDD/BDD in general, to consider the steps listed above from time to time in a general, summarized form. After doing this regularly, it becomes second nature; engrained in one’s daily development process.

  1. Understand the Problem
  2. Start off slow
  3. Clean up as you go along
  4. Test the unexpected
  5. Leave the test environment the way you found it
  6. Leave the source better than you found it
  7. Focus on what’s important.
  8. No when to stop

Quick Tip: Backbone Collection Validation

Sunday, January 19th, 2014

Often times I find the native Backbone Collection implementation to be lacking when compared to it’s Backbone.Model counterpart. In particular, Collections generally lack in terms of direct integration with a backend persistence layer, as well as the ability to validate models within the context of the collection as a whole.

Fortunately, such short comings can easily be circumvented due to the extensibility of Backbone’s design as a generalized framework. In fact, throughout my experience utilizing Backbone, I can assert that there has yet to be a problem I have come across which I was unable to easily solve by leveraging one of the many Backbone extensions, or, more often than not, by simply overriding Backbone’s default implementation of a given API.

Validating Collections

Perhaps a common use-case for validating a collection of Models can be found when implementing editors which allow for adding multiple entries of a given form section (implemented as separate Views), whereby each section has a one-to-one correlation with an individual model. Rather than invoke validation on models from each individual view, and manage which model’s are in an invalid state from the context of a composite view, it can be quite useful to simply validate the collection from the composite view which, in turn, results in all models being validated and their associated views updating accordingly.

Assuming live validation is not being utilized, validation is likely to occur when the user submits the form. As such, it becomes necessary to validate each model after their views have updated them as a result of the form being submitted. This can be achieved quite easily by implementing an isValid method on the collection which simply invokes isValid on each model within the collection (or optionally, against specific models within the collection). A basic isValid implementation for a Collection is as follows:

As can be seen in the above example, the Collection’s isValid method simply invokes isValid on it’s models. This causes each model to be re-validated which, in turn, results in any invalid models triggering their corresponding invalidation events, allowing for views to automatically display validation indicators, messages, and the like; particularly when leveraging the Backbone.Validation Plugin.

This example serves well to demonstrate that, while Backbone may not provide everything one could ever ask for “out of the box”, it does provide a design which affords developers the ability to quickly, easily, and effectively extend the native framework as needed.