The answer is that the calls to verify the non-state-changing methods can be removed.

 Method calls on another object fall into one of two categories:
  • State-changing: methods that have side effects and change the world outside the code under test, e.g., sendEmail(), saveRecord(), logAccess().
  • Non-state-changing: methods that return information about the world outside the code under test and don't modify anything, e.g., getUser(), findResults(), readFile().
You should usually avoid verifying that non-state-changing methods are called:
  • It is often redundant: a method call that doesn't change the state of the world is meaningless on its own. The code under test will use the return value of the method call to do other work that you can assert.
  • It makes tests brittle: tests need to be updated whenever method calls change. For example, if a test is expecting mockUserService.isUserActive(USER) to be called, it would fail if the code under test is modified to call user.isActive() instead.
  • It makes tests less readable: the additional assertions in the test make it more difficult to determine which method calls actually affect the state of the world.
  • It gives a false sense of security: just because the code under test called a method does not mean the code under test did the right thing with the method’s return value.
Instead of verifying that they are called, use non-state-changing methods to simulate different conditions in tests, e.g., when(mockUserService.isUserActive(USER)).thenReturn(false). Then write assertions for the return value of the code under test, or verify state-changing method calls.
Verifying non-state-changing method calls may be useful if there is no other output you can assert. For example, if your code should be caching an RPC result, you can verify that the method that makes the RPC is called only once.
With the unnecessary verifications removed, the test looks like this:
@Test public void addPermissionToDatabase() {
  new UserAuthorizer(mockUserService, mockPermissionDb).grantPermission(USER, READ_ACCESS);



  // Verify only the state-changing method.
  verify(mockPermissionDb).addPermission(USER, READ_ACCESS);
}


That’s much simpler! But remember that instead of using a mock to verify that a method was called, it would be even better to use a real or fake object to actually execute the method and check that it works properly. For example, the above test could use a fake database to check that the permission exists in the database rather than just verifying that addPermission() was called.

You can learn more about this topic in the book Growing Object-Oriented Software, Guided by Tests. Note that the book uses the terms “command” and “query” instead of “state-changing” and “non-state-changing”.
3 comments
Labels: , ,


By Marc Eaddy

 Programming languages provide basic types, such as integers, strings, and maps, which are useful in many contexts. For example, a string can be used to hold everything from a person's name to a web page URL. However, code that relies too heavily on basic types instead of custom abstractions can be hard to understand and maintain.

 Primitive obsession is the overuse of basic ("primitive") types to represent higher-level concepts. For example, this code uses basic types to represent shapes:
vector<pair<int, int>> polygon = ...
pair<pair<int, int>, pair<int, int>> bounding_box = GetBoundingBox(polygon);
int area = (bounding_box.second.first  - bounding_box.first.first) *
           (bounding_box.second.second - bounding_box.first.second);
pair is not the right level of abstraction because its generically-named first and second fields are used to represent X and Y in one case and lower-left (er, upper-left?) and upper-right (er, lower-right?) in the other. Worse, basic types don't encapsulate domain-specific code such as computing the bounding box and area.

Replacing basic types with higher-level abstractions results in clearer and better encapsulated code:
Polygon polygon = ...
int area = polygon.GetBoundingBox().GetArea();
Here are some other examples of primitive obsession:
  • Related maps, lists, vectors, etc. that can be easily combined into a single collection by consolidating the values into a custom higher-level abstraction. 
    map<UserId, string> id_to_name;
map<UserId, int> id_to_age;
    		 
    map<UserId, Person> id_to_person;
  • A vector or map with magic indices/keys, e.g. string values at indices/keys 0, 1, and 2 hold name, address, and phone #, respectively. Instead, consolidate these values into a higher-level abstraction.
    person_data[kName] = "Foo";
    		 
    person.SetName("Foo");
  • A string that holds complex or structured text (e.g. a date). Instead, use a higher-level abstraction (e.g. Date) that provides self-documenting accessors (e.g. GetMonth ) and guarantees correctness. 
    string date = "01-02-03";
    		 
    Date date(Month::Feb, Day(1), Year(2003));
  • An integer or floating point number that stores a time value, e.g. seconds. Instead, use a structured timestamp or duration type. 
    int timeout_secs = 5;
    		 
    Duration timeout = Seconds(5);
It's possible for any type—from a lowly int to a sophisticated red-black tree—to be too primitive for the job. If you see code that uses a lot of basic types that would be clearer or better encapsulated by using a higher-level abstraction, refactor it or politely remind the author to keep it classy!
3 comments
Labels: , ,


By Chris Lewis and Bob Nystrom
It's easy to get carried away creating long identifiers. Longer names often make things more readable. But names that are too long can decrease readability. There are many examples of variable names longer than 60 characters on GitHub and elsewhere. In 58 characters, we managed this haiku for you to consider:

Name variables
Using these simple guidelines
Beautiful source code

Names should be two things: clear (know what it refers to) and precise (know what it does not refer to). Here are some guidelines to help:

• Omit words that are obvious given a variable's type declaration.
// Bad, the type tells us what these variables are:
String nameString; List<datetime> holidayDateList;
// Better:
String name; List<datetime> holidays;

• Omit irrelevant details.
// Overly specific names are hard to read:
Monster finalBattleMostDangerousBossMonster; Payments nonTypicalMonthlyPayments;
// Better, if there's no other monsters or payments that need disambiguation:
Monster boss; Payments payments;


• Omit words that are clear from the surrounding context.
// Bad, repeating the context:
class AnnualHolidaySale {int annualSaleRebate; boolean promoteHolidaySale() {...}}
// Better:
class AnnualHolidaySale {int rebate; boolean promote() {...}}

• Omit words that could apply to any identifier.
You know the usual suspects: data, state, amount, number, value, manager, engine, object, entity, instance, helper, util, broker, metadata, process, handle, context. Cut them out.

 There are some exceptions to these rules; use your judgment. Names that are too long are still better than names that are too short. However, following these guidelines, your code will remain unambiguous and be much easier to read. Readers, including "future you,” will appreciate how clear your code is!
6 comments
Labels: , , ,

Bug Reports
  • You can use a bug report to track the entire story of a bug/feature/refactoring, adding context such as the original problem description, the design discussions between the team, and the commits that are used to solve the problem. This lets you easily see all related commits in one place, and allows others to easily keep track of the status of a particular problem.
  • Most commits should reference a bug report. Standalone commits (e.g. one-time cleanups or other small unplanned changes) don't need their own bug report, though, since they often contain all their context within the description and the source changes.
Informative commit messages and bug reports go hand-in-hand, providing context from different perspectives. Keep in mind that such context can be useful even to yourself, providing an easy reminder about the work you did last week, last quarter, or even last year. Future you will thank past you!
3 comments
Labels: , ,


Maintainers are burdened with understanding, documenting, and testing both classes when only one is really needed. Code must handle the case when hibernate is true, even though all callers pass false, as well as the case when Sleep returns an error, even though that never happens. This results in unnecessary code that never executes. Eliminating those smells simplifies the code:

class Human { ...
  void Sleep() { age += kSevenHours; }
};

 Here are some other YAGNI smells:
  • Code that has never been executed other than by tests (a.k.a. code that is dead on arrival)
  • Classes designed to be subclassed (have virtual methods and/or protected members) that are not actually subclassed
  • Public or protected methods or fields that could be private
  • Parameters, variables, or flags that always have the same value
Thankfully, YAGNI smells, and code smells in general, are often easy to spot by looking for simple patterns and are easy to eliminate using simple refactorings.

Are you thinking of adding code that won't be used today? Trust me, you aren't gonna need it!


1 comment
Labels: , ,

  • Introduce an explaining variable.
    // Subtract discount from price.
finalPrice = (numItems * itemPrice)
    - min(5, numItems) * itemPrice * 0.1;
    		 
    price = numItems * itemPrice;
discount =
    min(5, numItems) * itemPrice * 0.1;
finalPrice = price - discount;
  • Extract a method. 
    // Filter offensive words.
for (String word : words) { ... }
    		 
    filterOffensiveWords(words);
  • Use a more descriptive identifier name. 
    int width = ...; // Width in pixels.

    int widthInPixels = ...;
  • Add a check in case your code has assumptions. 
    // Safe since height is always > 0.
return width / height;
    		 
    checkArgument(height > 0);
return width / height;
    • There are cases where a comment can be helpful:
    • Reveal your intent: explain why the code does something (as opposed to what it does). 
      // Compute once because it’s expensive.
    • Protect a well-meaning future editor from mistakenly “fixing” your code. 
      // Create a new Foo instance because Foo is not thread-safe.
    • Clarification: a question that came up during code review or that readers of the code might have. 
      // Note that order matters because...
    • Explain your rationale for what looks like a bad software engineering practice. 
      @SuppressWarnings("unchecked") // The cast is safe because...
    On the other hand, avoid comments that just repeat what the code does. These are just noise:
    // Get all users.
userService.getAllUsers();
    		 
    // Check if the name is empty.
if (name.isEmpty()) { ... }
    Share on Twitter Share on Facebook
    10 comments
    Labels: , , ,


    In the recent months, we’ve been taking a hard look at the discipline of Engineering Productivity as a logical next step in the evolution of test automation. In that same vein, we’re going to rethink what an Engineering Productivity focused conference should look like today.  As we pivot, we will be extending these changes to GTAC and because we expect changes in theme, content and format, we are canceling the upcoming event scheduled in London this November. We’ll be bringing the event back in 2018 with a fresh outlook and strategy.

    While we know this may be disappointing for many of the folks who were looking forward to GTAC, we’re excited to come back with a new format which will serve this conference well in today’s environment.

    Share on Twitter Share on Facebook
    34 comments
    Labels:

    Aim to get most of your changes approved in the first round of review, with only minor comments. If your code reviews frequently require multiple rounds of comments, these tips can save you time.


    Spend your reviewers’ time wisely—it’s a limited resource. If they’re catching issues that you could easily have caught yourself, you’re lowering the overall productivity of your team.
    Before you send out the code review:
    When you’re addressing code review comments:
    Share on Twitter Share on Facebook
    15 comments
    Labels: , ,