Methods with more than four if statements (if(), else if(), else()) pose a number of related problems. First, due to the verbosity of Apex, numerous if statements will necessarily mean longer methods. Second, because each statement creates another logical path through the method, each also represents at least one additional test case to be written. The fewer the number of logical paths through your code, the simpler it is to understand, test, and refactor. Unfortunately, sometimes the business logic we are calculating requires a number of conditionals. For instance, calculating tax for goods sold needs to know the category of product, the buyer's tax exempt status, the price, and the location it was sold. That's easily four or more if statements. Look at this example:

  public Double calculateTax(Id goodsSold, Account buyer){
    if(buyer.tax_exempt__c) {
      if(buyer.shippingState_taxed == true){
        if(goodsSold.price__c >= TAX_THRESHHOLD_LIMIT){
          return .85 * goodsSold.price__c;
        } else {
          return 0;
        }
      }
    } else {
      if(buyer.shippingState_taxed == true){
        if(goodsSold.price__c >= TAX_THRESHHOLD_LIMIT){
          return .85 * goodsSold.price__c;
        } else {
          return 0;
        }
      }
    }
  }

As it turns out, determining the tax rate is related to, but not the same thing, as calculating the tax due. One way to simplify this code and reduce the number of if statements is to build a method on the Account object to determine if the account's shipping address is in a taxable state and if the account is tax exempt or not. Creating a formula field on the account for should_be_taxed allows us to simplify to this:

  public Double calculateTax(Id goodsSold, Account buyer){
    if(buyer.should_be_taxed && 
goodsSold.price__c >= TAX_THRESHHOLD_LIMIT){
      return 8.5;
    }
  }

Creating that formula field on Account allows us not only to reduce code length, but also cut down on our branching logic and write this so that it reads easier in English. Other often overlooked methods for reducing if statements are enums and method extrapolation. For instance, you can write methods that extrapolate the complexity of making a decision into its own method and focus the original method on acting on that decision. In our tax calculation method, that might mean creating a method called shouldBeTaxed (account buyer) that returns true or false. Like our formula field, this would let the calculateTax method act on that decision rather than making it and then acting on it. Ultimately, there will be times where a single method has to be complex and host a number of if statements, but these can and should be established as their own methods.

Like the raw number of if statements, this question aims to identify overly-complex if statements. As developers, we sometimes write if statements like this:

    if(x == true && y == true && z == false && 
(a == true || b == true)){
      
    }

The problem with if statements like this is twofold: first, they're hard to understand, especially when we're not dealing with variables such as x, y, z, a, and b, but full objects with methods and fields. Secondly, these are much harder to debug. Concentrating the decision-making on a single if statement reduces the decision tree, but it means that when any one of those conditions is not correct, you'll inevitably end up breaking the conditions out into nested if statements and log output until you've identified which condition isn't being met. Like the first question, enums and method extraction can help overcome this type of complexity.

I find I run into this one pretty often. As developers, we tend to code to expectations. For example, I expect a given variable to only have three possible values, so I code an if() then an else if(), and then a final else if() to handle my three possible values. This works fantastically for two years until someone adds a fourth value to the options. Suddenly, the if block isn't handling all the options, and unexpected behaviors (bugs) happen. Always including an else clause, even if it does nothing more than create a self-improvement case or e-mail the team, at least handles the unexpected situation.

First, let me state an exception to this rule. Factory methods for producing test data are exempt from this rule. For instance, a factory method for returning an order may require passing in a user, an account, a contact, and an opportunity, as well as… well, you get the idea. Unlike test methods, most "live" methods accept parameters for one of two reasons: direct manipulation of that parameter or making decisions through conditional logic. Limiting each method to no more than four incoming parameters forces developers to extrapolate decision-making into dedicated methods and encourages developers to write short, concise, and tightly-focused data-manipulation methods.

This is easily the most contentious question. Larger methods are inevitably harder to follow and more complex. The primary benefit of keeping your methods short is that they, by necessity, must also be simple. It's very hard to have multiple logic paths in a method that's only 30 lines long. Additionally, this simplifies your testing as well, as a 30-line method with only four parameters is unlikely to need to test a great number of edge cases. So, 30 lines seem concrete and quantifiable enough, but I think this could use some explanation, as not everything makes sense as a line. Conditional logic keywords such as if/else each count as a line, but I wouldn't count a line with nothing but a { on it. If your teams coding style uses a line return after the method declaration before the {, the { is a freebie line. So, why 30 lines? I admit this is a fairly arbitrary choice, but over the years I've found 30 lines to be the sweet spot. I can normally create or refactor most methods into no more than 30 lines, not counting comments. The key to making this work is to compose complex logic methods from small, tightly-focused methods.

Like the line count for methods, this line count is as contentious as it is easy to judge. Software is a bit like thermodynamics. The second law of thermodynamics teaches us that over time, things progress from order to entropy. Over time, code inevitably goes from order to chaos. No one starts off writing massive classes with multiple responsibilities, but over time, they have a tendency to grow that way. Keeping a fixed limit on class length is a guard against just throwing features, methods, and utilities into any given class regardless of what it's responsible for. Account classes are responsible for account logic, not logic for validating an account's shipping address. That's what an AddressVerificationUtilities class is for. The reasoning behind a change to how address verification works is different and separate from the logic behind changing other account data, and keeping them separate allows you to change, for instance, the validation of an address without affecting the account overall. Keeping your class sizes fixed is a simple way to ensure you're actually maintaining single responsibility, which in turn helps lower complexity.

This one is pretty simple and stems from the trenches of Java software development. I remember once reading the class name AccountDataFactoryFactory and thinking to myself that I understood all the words in that name, but not the name itself. If our classes are to be singularly focused, we should intuitively name them. I like to accomplish this in two ways. The first is by naming the classes as simply and plainly as possible; the second to prefix classes with a type designation. In my infamous AccountDataFactoryFactory, the class existed to dynamically return a factory capable of generating account data. It was a factory class for building factories. Regardless, the name wasn't as intuitive as it could have been. I think a much clearer name in that situation would have been DynamicAccountDataFactoryGenerator. The same holds true for our Apex classes. It's good to designate factories as such in their class name, but when we go off script building factories to return factories, we need to spell that out, as well. To that end, I like to preface all my class names with consistent prefixes that identify the functionality of the class. Test classes are thus prefaced with test_, factories are prefaced with fact_, and so on. The exception to this is any class focused on an sObject. In those cases, the sObject itself becomes the prefix.

The following are the class types and their prefix:

With these prefixes in place, I can easily rely on my IDE's autocompletion to help narrow down and find the class I'm looking for. There are many variations on this theme, and some development teams I've worked with have ultimately decided to use the sObject as the prefix for all classes, arguing that a factory should only handle one sObject type. Either way, the idea is to establish a consistent naming scheme for objects.

Variables are, by their nature, likely to change. Removing variability, whenever we can, inevitably leads to more robust software, if only because there's one less moving part. I like to always look for the use of constants and enums during code reviews to see how the team has used them. The Salesforce1 platform gives us a couple of unique features that can help us adopt constants and enums. One of the most common reasons I've come across for teams not using constants is that the constant value "may change." With the Salesforce1 platform, we can have the best of both worlds: data that is changeable yet held constant during code runtime. Because constants are defined as static final variables on the class, and because Apex allows us to set the definition of a static final variable when it is defined, we can establish constants in code based on a custom setting, or custom metadata that looks something like this:

Public static final string BASE_URL = appSettings.get('base_url');

This of course assumes we have a class named appSettings with a static method .get() that returns a value from a custom setting or custom metadata. Here, we get all the benefits of a constant during execution while still being able to change values without code deployment.

This isn't necessarily a code smell or something I'd fail a code review over. It is, however, something that is not generally needed anymore. With our ability to provide httpCalloutMocks for callouts, the biggest reason for variant logic based on execution context has gone away. Code reviews are an excellent way of identifying this kind of tech-debt and, if you've set up mock factories, take a few minutes to fix it. At the very least, file a ticket or record the presence of test.isRunningTest() so it can be taken care of in the future.

This one I will fail a code review on. Tests without asserts are liabilities, not tests. The number of required asserts is up to your team, but every test method should call at least one assert method. I prefer more assertions and would recommend at least two: one to assert your test data was set up as you intended, and a second to prove the code works as intended (or not, in the case of a negative test). Unit tests are there to help your team in the long run, and tests without asserts accomplish none of those goals. Make sure every test has active, intelligent assert methods! What do I mean by intelligent? You'll know unintelligent when you see it; it'll be about as useful as this assertion:

System.assert(true);

This is almost an automatic code review fail. There are times when SeeAllData=true is required. However, as the platform continues to evolve, the need for this annotation becomes increasingly rare. And 90% of the time I find this annotation in use, it's due to one of three things. 1: the class was last updated several years ago, 2: the developer didn't know any better, or 3: the developer knew better, but didn't want to refactor. Only about 10% of the time do I find a test that actually still requires the annotation. One of the most common use cases for this annotation used to be pricebooks. Tests that dealt with pricebooks used to require @seeAllData=true, but ever since Summer 2014, we've been able to use Test.getStandardPricebookId();. Unless you're testing things such as approval processes, this annotation is a code smell and should be refactored away.

A Visualforce controller should instantiate a single object. This forces you to create and adhere to a wrapper object. Your wrapper object can thus be independently unit tested. Your controller's unit tests should create and manipulate the wrapper object extensively and with as many different users, roles, profiles, and permission set combinations. This wrapper will contain all the objects and values that the Visualforce page relies on, and its creation is deeply dependent on the access permissions to that data. Testing with different users, roles, permissions, and permissions sets ensures that your Visualforce page works when the user does, or does not, have access to the data. Not only does this practice simplify writing the Visualforce code and simplify the Apex controllers, it makes it much easier to deal with multiple object types on the same page, especially if you're interacting with them via JavaScript.

It may seem unintuitive at first, but asking if the code in question adheres to the DRY principle (Don't Repeat Yourself) is a powerful way to identify and squash complexity. If the code in question repeats itself or the functionality of another method, you've not only doubled your testing workload, but have set up a situation where any bug fixes or enhancements have to be duplicated, as well, if you remember. DRY helps you know where to refactor code out into a shared method or class. This question may be the most important for squashing complexity because the simplest way to reduce complexity in any software project is to remove code. Identifying and eliminating duplicate code helps, if for no other reason, by eliminating code. Non-existent code is the least likely to have bugs.

Remember, too, that functionality may be duplicated not in code, but declaratively. If a piece of code fired by a trigger can now be replaced, or is replaced in other circumstances by a process builder process, then the code isn't DRY. Refactoring these situations where workflow rules, validation rules, and processes duplicate decision-making or data manipulation methods means identifying which solution is more in line with the platform's design principles. That may very well mean deleting code in favor of a process or replacing a trigger with a validation rule. As developers, we must remember that mastering the platform means using the right tool for the job, especially when that tool is not code.