CX Works

CX Works brings the most relevant leading practices to you.
It is a single portal of curated, field-tested and SAP-verified expertise for SAP Customer Experience solutions

Example of a Code Review Checklist

23 min read

Example of a Code Review Checklist

As outlined in Tips for an Effective SAP Commerce Cloud Code Review, it's important to be able to deliver code reviews consistently across your team. This page provides a checklist of items to verify when doing code reviews. Every team for every project should have such a checklist, agreed upon by all reviewers and maintained along the way. It is not static – it will evolve with the maturity of the team. This article is intended to provide you with a starting point for your checklist.

Table of Contents

General Code Review Recommendations

There are plenty of recommendations for good quality Pull Requests out there. For example:

  • Is every piece of code in the right place (e.g. service logic in a service, controller logic in a controller, conversion logic in a converter or populator, model logic in an interceptor, reusable view code in a tag)?
  • Do all classes have only one responsibility?
  • Do all methods do only one thing?
  • If a method has a side-effect, is it clear from the name, and otherwise documented?
  • Are all classes/methods/variables named properly so that the code is self-describing?
  • Is everything as private as possible (i.e. only the intended public interface is public)?
  • Are too many things private? This could be an indication that you should extract an object.
  • Are all files within a reasonable size (e.g., less than 100 lines of code)?
  • Are all methods less than 10 lines of code?
  • No Law of Demeter violations (providing whole objects to methods when all that’s needed is the value of one attribute of them, aka "don't talk to strangers")?
    • According to the Law of Demeter, a method M of object O should only call following types of methods:
      1. Methods of Object O itself;
      2. Methods of Object passed as an argument;
      3. Method of object, which is held in instance variable;
      4. Any Object which is created locally in method M

      This is a design guideline more than an absolute rule. By following it, the objects are less dependent on the internal structure of other objects, and the software is more maintainable and adaptable.

    • The following is an example violation of the Law of Demeter:

      Law of Demeter Violation
      public void process(Order o) {
       
              // as per rule 1, this method invocation is fine, because o is a argument of process() method
              Message msg = o.getMessage();
       
              // this method call is a violation, as we are using msg, which we got from Order.
              // We should ask order to normalize message, e.g. "o.normalizeMessage();"
              msg.normalize();
       
              // this is also a violation, instead using temporary variable it uses method chain.
              o.getMessage().normalize();
       
              // this is OK, a constructor call, not a method call.
              Instrument symbol = new Instrument();
       
              // as per rule 4, this method call is OK, because instance of Instrument is created locally.
              symbol.populate(); 
      }
       
      public Book(AuthorRepository repository, String isbn){ 
         AuthorLocator locator = repository.getAuthorLocator();
         this.author = locator.getAuthor(isbn);
       
      }
       
      public Country getFirstBookCategoryFromXML(XMLMessage xml) { 
             return xml.getXML().getBooks().getBookArrary(0).getBookHeader().getBookCategory();
      }
  • Is everything tested? Do the tests test enough? Are there edge cases that should be included?
  • Are there tests for private methods? This is a code smell.
  • Every class should have a small comment describing what it represents/does. Public methods should have comments describing what they are for, or when to call them if this isn’t obvious from the code. Comments shouldn’t describe what the method does (this is visible from looking at the code).
  • There shouldn’t be any commented-out code.
  • There should be no debug statements like console.logSystem.out.println() e.printStackTrace() or the likes.
  • The use of null leads to null pointers. Avoid the use of null anywhere in your code.
    • A method should never return null.
    • Avoid null checks on returning values. As methods should not return null.
    • When a method receives parameters, they should be checked against null values.


SAP Commerce Cloud Specific Recommendations

Those recommended practices are specific to SAP Commerce Cloud. See also  Coding Standards

Architecture

  • The dependencies between extensions and classes should make sense (e.g. a Service Layer extension should not depend on a storefront extension). 

    Example of component dependency
    The following image represents a simple request and how the components should interact with each other.

  • Extensions in your Integrated Development Environment (IDE) need to be aligned with what is in extensioninfo.xml.
  • The use of the customize folder(link) should only be used in rare situations when there is no other solution than patching some files in the platform.

  • Lower priority checklist:

    • The structure of the extensions should be on a functional basis (i.e.: payment) rather on architectural category (Data Transfer Object (DTO), Facades).
    • Check integration components have dedicated extension(s) except for presentation layer.
    • Never modify directly commerce extensions but override using Spring in your custom extensions.

Code Formatting

  • Ensure all the files have the same encoding (UTF-8).

  • Use a consistent code formatter. You can also check the Coding Standards  page.

    • For Eclipse IDE users, there is nothing to do because code formatting related settings are in the repository (provided org.eclipse.jdt.ui.prefs, org.eclipse.jdt.core.prefs are committed as they should).
    • For the IntelliJ IDEA users, there are two options: they can use the y-intellij-template or configure IDE by themselves. 
    • Do not apply auto formatting to xml files. It may lead to changes in a whole file so it will be hard to find the real change.

Logging

  • Do not use   System.out

  • Recommendations for log levels:
    • ERROR - Showstoppers, any kind of issue that results in the termination of a process on an exception state and or is unrecoverable.
      • Eg: Database connection issues, Infrastructure problems, external resources unavailable.
    • WARNING - Issues that might impact the application but does not call the system or a process to stop or that the system is programmed to work around.
      • Eg: Automatic updates fail where a recovery strategy is defined, Using default values for something that should have been configured.
    • INFO - This level is used to mark major changes in the state of a process or the system.
      • Eg: Cronjob or other automatic process start/ends normally, a new configuration is activated, before and after calling a web service.
    • DEBUG - Level used to provide diagnostic information for the developers. On major state changes of the application.

      •  Eg: Whenever a business rule causes a change in the flow of the application (if/else/case/for/while), Whenever reaching external resources such as web services.
    • TRACE - Cover any changes on objects that might require investigation whenever Java Debug is not an option (Production environment). Should be used to show the granular information about objects and attributes.
      • Eg: Before and after any complex process that modifies any information.

  • Always use the same library as the platform: slf4j.

  • Common sense is required whenever placing logging points. Using too few might not provide the required information and using too much might create giant log files that are hard to maintain and to use when searching for useful information.
  • Avoid unsafe logs. NullPointers and/or other exceptions might cause a bigger issue and mask the original source of the problem. 
  • Standard run level for the platform must not be too verbose (The standard file can be found in the bin/platform/project.properties file).

    project.properties
    # The log level and the appenders for the rootLogger to use 
    log4j.rootLogger=info, CONSOLE
     
    # Log level for log messages of the Spring framework used in the SAP Commerce Cloud Multichannel Suite 
    log4j.logger.org.springframework=warn, CONSOLE

Unit tests

  • Check F.I.R.S.T rules (Fast, Independent, Repeatable, Self-Validating).
  • Check if the @UnitTest or @IntegrationTest  annotation was used to specify the test type.
  • Check the existence of  @Test  annotation for each test method.
  • The test should contain assertions.
  • Check that there are no Integration tests where Unit tests would have been sufficient (there should be a LOT more pure unit tests and integration tests). More information can be found here .
  • Use  @Before  rather than instance variable initialization (in constructor or instance variable declaration level) to ensure test independence. In the following code example, the method marked with @Before will be executed before each of the @Test methods, ensuring that a clean new instance of the object Calculator will be used for each of the tests.

    @RunWith(MockitoJUnitRunner.class)
    public class CalculatorTest {
        @Mock
        Adder adder;
        @Mock
        Subtractor subtractor;
        @Mock
        Divider divider;
        @Mock
        Multiplier multiplier;
        Calculator calculator;
        @Before
        public void setUp() {
            calculator = new Calculator(adder,subtractor,divider,multiplier);
        }
        @Test
        public void testAdd() {
            BigDecimal value = calculator.add(2,2);
            verify(adder).add(eq(2),eq(2));
        }
        @Test
        public void testDivide() {
            BigDecimal value = calculator.divide(2,2);
            verify(divider).divide(eq(2),eq(2));
        }
         
    }
  • Lower priority checklist:

    • Ensure integration tests have their own test data, don't use actual project data
    • For unit tests, make sure there are no cases where we are mistakenly testing the mock object
    • Check that assertions are really doing what the original requirement is expecting to avoid false positives, e.g. a user story says we need to use the order date to calculate something but the assertion expects the usage of a delivery date.
    • Check that tests are covering both positive and negative scenarios, e.g. we want to test more than just the sunny day scenario

Spring

  • Check correct usage of Spring inheritance (use of parent and list merge, proper use of aliases)

  • The SAP Commerce Cloud name convention of the class implementing an interface is  CustomProjectXxxx and not  XxxxImpl

  • Check there is not too much duplication in the bean overriding

  • Check Spring scopes (the classic missing "prototype" scope for a "Data" bean used by a converter). An example of how a Data object scope should be declared:

    <!--  Bean declaration for Data classes with scope Prototype - START-->
    <bean id="userData" class="de.hybris.platform.b2bacceleratorfacades.company.data.UserData" scope="prototype" />
  • Lower priority checklist:

    • Use constructor injection only for mandatory field. You can also mark the setter method with the @Required annotation.
    • Overriden classes in the same sub-package than the original.(i.e.:  de.hybris.platform.commercefacades.order.impl.DefaultCheckoutFacade  ->  com.mycustomer.myproject.commercefacades.order.impl.MyProjectCheckoutFacade )

    • @Cachable  does not work with private method or self invoking method (see http://docs.spring.io/spring/docs/3.1.x/spring-framework-reference/html/cache.html > Enable caching annotations).

    • Declare all beans in configuration files, do not rely on component scanning (except for @Controllers). You may, however, use @Resource annotation at the field or method level in your class, not all injected properties need to be declared in the configuration file.
    • Check Spring is used to instantiate application level objects (avoid manual creation of Singleton).
    • Avoid getting beans from the Registry. But when you do need, get by name rather than by type: replace  Registry.getApplicationContext().getBean(CacheManager.class)  by  Registry.getApplicationContext().getBean("ehCacheManager",CacheManager.class)

    • Use XML configuration, not JavaConfig.

Annotation

  • Use @Resource instead of @Autowired, both in tests and the main code. This is because autowiring by name is the preferred mode in SAP Commerce Cloud platform due to multiple implementations existing for many interfaces, and @Autowired works by type. Moreover, @Autowired does not work in ServicelayerTest.
  • Except in the WebApplicationContext where the @Controller annotation is the norm to declare controllers, do not use component-scan (@Service@Component)–you must declare all beans in a spring xml configuration file
  • Do not use JavaConfig–only xml configuration files.
  • You may:
    • use the @Resource annotation to autowire the dependencies.
    • or declare them in the xml configuration file.
    • but NEVER do both at the same time for the same bean and the same property!
  • When you declare dependencies in the configuration file and the class cannot work without those dependencies, add the @Required annotation to the setters to have spring fail fast in case the dependencies is missing (that will make clear to future users of your bean/class what are the mandatory dependencies). However, when that makes sense, the class could have a default value for the dependencies so that it is optional
  • To inject SAP Commerce Cloud properties in Controller, use @Value or the configurationService.
  • To inject SAP Commerce Cloud properties in any other spring bean, use a placeholder ${} in the xml configuration file, or the configurationService .
  • Use the JUnit @Test annotation for each test method, and the Hybris @UnitTest , @IntegrationTest or the other test annotations at the class level to differentiate test types.
    • In JUnit tests, use  @Mock @InjectMocks and more generally the mockito framework.

Controllers

  • Avoid "Controller driven development". Controllers should not contain any business logic.
  • Check  AbstractController  and  AbstractPageController , if there are too many methods they should probably be moved to Spring MVC Interceptors in accordance with SRP (Single Responsibility Principle) to have a good separation of concern. 

    • Functions related to user experience (DeviceDetectionBeforeControllerHandler, SetUiExperienceBeforeControllerHandler).
    • Theme (ThemeBeforeControllerHandler), analytics (SeoRobotsFollowBeforeViewHandler, AnalyticsPropertiesBeforeViewHandler).
    • Security (SecurityUserCheckBeforeControllerHandler).
    • BTG (BtgSegmentBeforeViewHandler).
    • Populating the model with data common to a lot of controllers (CmsPageBeforeViewHandler).

  • Lower priority checklist

    • It could make sense to use sessionService.executeInLocalView() rather than adding method parameters to the interfaces of each layer (use carefully). Below is an example of "executeInLocalView" can be seen. We perform a flexible search temporarily disabling the search restriction. What is important to keep in mind is that any modification is happening only in this scope (not visible to other threads).

      executeInLocalView
      (SearchResult<? extends ItemModel>) sessionService.executeInLocalView(new SessionExecutionBody()
      {
        @Override
        public Object execute()  
        { 
          sessionService.setAttribute(FlexibleSearch.DISABLE_RESTRICTIONS, Boolean.TRUE); 
          return flexibleSearchService.search(query); 
         }
      });

Populators

  • Do not put business logic in the populators.
  • Converter versus populator (if you use populator, do not populate the Data object in the converter). The converter, as seen below, allows you to have a list of one or more different populators. That usually happens when you have different objects nested, and you want to have different ways to populate each of them. That being said, when you have a simple Data Object conversion, you can simply put the conversion in the Converter. But, if one day that object changes and you see the need  to use different conversions, you can move the Data Object conversion to multiple populators and remove whatever is in the converter.

    Converter with one or more populator
    <bean id="defaultProductHighlightConverter" parent="abstractPopulatingConverter">
        <lookup-method name="createTarget" bean="productFeatureData"/>
        <property name="populators">
            <list>
                <ref bean="categoryPopulator"/>
                <ref bean="categoryDescriptionPopulator"/>
            </list>
        </property>
    </bean>
  • Store only primitive values, simple Java types (String, Boolean) or other Data objects in Data object (do not store Models!)
  • Lower priority checklist:

    • Use the same name as Model for mapped attributes.
    • Define the Data object depending on the context (ProductData for Category page is different from the one for Product).
      productFacade.getProductForOptions(productModel,PRODUCT_OPTIONS) should be used in the facade, tag lib and controller to be able to cache ProductData objects in a single point.

    • Use converters/populators only to populate Data objects that are displayed. Data objects are tailored towards your output (be it a web page or web service output etc.)

Session

  • Don't put large objects into the session and prefer Primary Keys (PKs) in particular for an array or collection. If objects are cached it shouldn't be an issue. Pay attention to the usage of:
    • SAP Commerce Cloud Service layer:  sessionService.setAttribute()

    • SAP Commerce Cloud Jalo layer:  JaloSession.getCurrentSession().setAttribute()

    • MVC Controller:  session.setAttribute()  (from  HttpSession  in method parameters)

  • Don't put Service Layer Model objects on the session.
  • Check  DefaultCartService.hasSessionCart()  is called before using  DefaultCartService.getSessionCart()  as it is done in  DefaultCartFacade.getSessionCart() , this way no extra carts will be created for anonymous users.

Model

  • Do not modify model for transient information (bad practice and risk in case of call to  saveAll )

  • Lower priority checklist:

    • If you try to display the same object twice during the request without modifying it, you usually get the same object but you can not depend on this. Better keep a reference to this object in your own code

Cronjob

  • Code to abort a cronjob that is expected to run for a long time should be implemented. This way you'll have a way to stop it if ever an issue arises.
  • Check the logic is externalized and not implemented in the  JobPerformable  class directly.
  • For export, check populators and DTOs (similar concepts to the website) are used.
  • Multi-threading:

    • Do not use class variable for singleton or tenant scope bean for thread safety.

    • If large dataset is being processed, check multithreading is used.
    • Search for  synchronized ThreadLocalvolatile, Collections.synchronizedCollection / Set / List / Map...()Thread.sleepConcurrentHashMapExecutorServiceThread .

    • Check Exception are caught so thread does not die.

    • Each thread needs to be initialized with a tenant and it's own SAP Commerce Cloud session (no session sharing).
    • Close the session and deactivate the tenant at the end of the thread execution.
    • Do not share Models between threads and do not pass Models when initializing a new thread (Models are not thread-safe).
    • ThreadLocal  or MDC variables should be cleaned at the end of the request (use filter).

ImpEx

  • Check added/modified impex dependencies. It needs to run without errors even when running in batch. The example below is a real case where the impex was initially misplaced in essential-data.impex. When the developer ran it in hAC, it worked perfectly. When the system was initialized, it failed, because at that moment when essential-data.impex ran, no catalog existed. The correct place to put the impex below was in catalogs-sync.impex.

    Dependent ImpEx Example
    INSERT_UPDATE syncattributedescriptorconfig;includedinsync;copyByValue;syncJob(code)[path-delimiter=!][unique=true];attributeDescriptor(qualifier[default='europe1Prices'], enclosingType(ComposedType.code[default='Product'] ))[unique=true];
    ;0;0;sync mystoreb2bProductCatalog:Staged->Online;
  • Check if the impex is can be imported more than once
  • Check if there are any impex customizations

  • Check if there are any custom processors. Ideally you wouldn't need them

Interceptors

  • The goal of the Service Layer interceptors should be to ensure data integrity (interceptor are low level), it should not execute complex business logic as it is better to keep the logic in a service.
  • An interceptor should not do any expensive operations or it could be a have a big impact on performance (check for the conditions of execution)
  • Lower priority checklist:

    • Check the use of the correct type of interceptors
    • Limited to the correct types or it could be a performance killer
    • The interceptor should work for all application/processes (e.g. Cronjob and the store front) and the context. If it is not the case, it probably means the logic should be at in a higher layer

Dynamic Attribute

  • Ensure that the logic behind the Dynamic Attribute concerns directly the type, for which you want to define it. Otherwise, it is better to keep the logic in a proper service.

Transaction

  • Check proper use of @Transactional, TransactionBody or Transaction - More information can be found here
  • Check the transaction doesn't contain synchronous calls to external systems or it doesn't contain slow code (e.g. sending an email), the longer the transaction is open the more likely we are to have performances issues due to row locking.
  • Check for potential deadlocks.
  • Lower priority checklist:

    • Check transactions are coarse grained, Facade is usually a good level or a service that is executing business logic.
    • Be careful with transaction defined with AOP with a wide interception scope (e.g. **.*Service) where we lose control of where we are intercepting for transaction management.
    • Check for any side effects due to Delayed Store (e.g. writing and then reading the same data within a transaction)
    • Always use a finally block.

FlexibleSearch

  • For relations with a lot of objects, avoid iterating model using the Model getter and instead use a FlexibleSearch query, see the "Lazy Loading" section of the Models page. 
  • Make sure that any search methods on the DAOs are not returning null objects, it's a better approach to return empty lists instead.
  • Lower priority checklist:

    • Check if it does not use values that are more than necessary. For example, FlexibleSearch statement using  new Date()  would never be cached effectively because it will be precise to the millisecond – but often truncating to the day would be accurate enough and be cacheable.

    • Check indexes exist for the search criteria when necessary, e.g. if you see that a certain search is always full scanning a table in the execution plan, the addition of an index will help.
    • If search criteria contains '%', check the search is not executed on two many rows as an index will not be effective in this case.
    • Ensure prepared statements are used for the flexible search query by always using parameterized queries e.g. don't add the values directly to the query itself.

Service Layer versus Jalo Layer

  • ServiceLayer should always be used first. If all options have been exhausted then Jalo can be used.
  • Configuration - Use the  ConfigurationService interface rather than the  de.hybris.platform.util.Config  object.
  • You can check other common ServiceLayer vs. Jalo cases below:

    • Execute in local view - sessionService.executeInLocalView should be used rather than  JaloSession.createLocalSessionContext.

    • Use Model constants - The ProductModel.CODE should be used rather than Product.CODE.

    • Getting the current User - The  UserService  should be used rather than the  JaloSession .
    • Enumerations - Direct access to enumeration values should be used rather than calls to  EnumerationManager.getEnumerationValue() .
    • Flexible Search - Use the  FlexibleSearchService  rather than  FlexibleSearch .
    • Catalog Version - A catalog version should be set for the current using CatalogService.setSessionCatalogVersion()  rather than using the equivalent function in the  CatalogManager .

    • Getting a User by UID - The  UserService should be used rather than the  UserManager .

Initialization/update

  • After initialization, the website should be totally functional. This is critical not only for the first deployment, but for any automated testing and developer setup
  • Ensure no errors during the SAP Commerce Cloud initialization/update
  • Use the recommended  @SystemSetup approach, including for data patches
  • Clear separation between essential data and project data (an update should not override business user modifications).
  • Clear separation between core project data and sample project data. When the project is deployed in production, you only want the project data to be populated and not the sample data used only in the development phase.

Documentation

  • After a code update, check if its javadoc was also updated accordingly.

  • Each protected method needs javadoc explaining how extensibility is designed.

  • Synchronization blocks needs to be documented explaining the reason for synchronization.

  • Avoid automatically generated commenting (such as JAutoDoc) as it often provides no insight, cluttering the code with unhelpful commenting, and gives the false impression that code has been thoughtfully commented by the developer.

Backoffice Framework

  • NEVER define variables of type org.zkoss.zk.ui. Component in classes which are not of Type Component itself, unless you know the ZK lifecycle and how to handle the cleanup manually. (This happens very often in Editors)
  • Renderer classes must be stateless, i.e. they shouldn't contain any variables (except services or other beans which don't change at runtime).
  • Avoid the usage of cascading listeners, it makes the code extremely hard to understand.
  • All classes must be in the src folder, not web/src, because of cockpit inheritance.


Conclusion

You should now have an idea of some of the common high and low priority checks you could use while reviewing code. Depending on your team's size and experience, you may not require all of these checks in your code review. What is important is that your entire team of developers agrees on what does need to be checked as part of a code review and that checklist is always available for people to reference. If you have not already done so we encourage you to read Tips for an Effective SAP Commerce Cloud Code Review, which describes all other aspects that make for a more successful code review.f

Overlay