1.4.2014

Refactoring: Pushing Behavior Into The Domain

"Reduce duplication, bugs and shotgun surgery, all while increasing testability by pushing behavior into the domain!"

12.29.2013

Refactoring: Renaming Ambiguous Variables When Reading Code

"How many times a day do you find yourself trying to figure out what variables represent because they are named ambiguously?"

12.19.2013

YAGNI: Ask about actual scenarios even if the features seem reasonable.

Reasonable feature requests

It's difficult in software development to shift the focus to problems and goals instead of features, features, features. As I've written about before, it's ideal to focus on the value first, and match the features to the value.

Sometimes we encounter feature requests that seem reasonable enough that we don't feel the need to back track to discover the value. This happens for many reasons:

  • We've encountered similar requests in the past, maybe with other projects, and it seems natural to add it to the current project.
  • We make assumptions about the value of the feature and the feature seems reasonable to attain the value.
  • We assume what scenarios would prompt the request and assume these situations exist.
  • Sometimes this all happens subconsciously!

Ask anyways

However, I've been surprised how many times:

  • What was needed in a past project wasn't actually needed and all of the sudden it's being added to yet another project under the assumption that it was of value historically. This becomes a self-perpetuating problem!
  • What was of value in a past project isn't of value in a current project.
  • The value isn't what we assume it to is, the problem we think we are solving isn't the same as past problems or the goals aren't the same as past goals.
  • The situations we assume exist probably don't, and even if they do, they may not be the same magnitude of a problem as in the past and thus might not require the same type of solution.
  • Past solutions (features) may not have been ideal.

Just ask!

Just this week I encountered a request that upon further inspection was prompted by a subset of the problems in a past project. Subconciously, I almost implemented the feature with the full set of problems in mind from the past. By inquiring about situations that prompted the request, we were able to create a solution that was a subset of the past solution! This reduced the time and money necessary to deliver value to the customer.

TLDR: Even if a feature seems reasonable, ask about the situations that prompted it. I've often been pleasantly surprised :)

11.1.2013

Using invariants to avoid null check insanity

Null checks aren't fun, but even worse are the ever ambiguous run-time NullReferenceExceptions we might otherwise receive.

Variant

Take the following code:

public class Family
{
    public List<string> Names;
}

// consumer creating a family
var family = new Family();
family.Names = new[] {"John", "Jane"}.ToList();

// consumer adding a name
family.Names = family.Names ?? new List<string>();
family.Names.Add("Baby");

// consumer searching names
var searchName = "Bob";
var hasSomeoneNamed = family.Names != null && family.Names.Contains(searchName);


  • Note: each // consumer comment delineates a separate example of using the Family type
  • There's a whole extra line of code just to add a family member!
  • Searching isn't a simple query, it's also a preventative null check and clever usage of the short circuiting && operator.
  • This extra noise is especially confusing to novice programmers.

Invariant

Invariant - "never changing"

A few simple changes can make things much simpler for consumers. If we require a list of names upon creation of a family, we can do the following:

public class Family
{
    public readonly List<string> Names;

    public Family(IEnumerable<string> names)
    {
        Names = names.ToList();
    }
}

  • As a consumer I can see that Names is readonly, which means it can't be changed after creation.
  • Seeing that the constructor also requires a list of names, even if I didn't know the implementation details it would be safe to assume that the names list is never going to be null, unless someone is out to troll me!
  • Even without requiring a list of names, what would be the purpose of a Names list that was always null?
  • We could go further by including this invariant in the class description or with CodeContracts and bring in static analysis support to help avoid null checks, but readonly alone is a great start.

Look at the impact on consumers:

// consumer creating a family
var names = new[] { "John", "Jane" };
var family = new Family(names);

// consumer adding a name
family.Names.Add("Baby");

// consumer searching names
var searchName = "Bob";
var hasSomeoneNamed = family.Names.Contains(searchName);

I'd much rather maintain this code!

One step further with Encapsulation

We could also encapsulate the Names list:

public class Family
{
    protected readonly List<string> Names;

    public Family(IEnumerable<string> names)
    {
        Names = names.ToList();
    }

    public void AddName(string name)
    {
        Names.Add(name);
    }

    public bool HasSomeoneNamed(string searchName)
    {
        return Names.Contains(searchName);
    }
}

Now our consumers don't even have to be aware of the fact that Names exists let alone that it might be null:

// consumer creating a family
var names = new[] { "John", "Jane" };
var family = new Family(names);

// consumer adding a name
family.AddName("Baby");

// consumer searching names
var searchName = "Bob";
var hasSomeoneNamed = family.HasSomeoneNamed(searchName);


  • This is suggested by the principles behind the Law of Demeter.
  • All null checking would be confined to the Family type, thus making it simple to see the lack of necessity.

However, I usually don't go this far:

  • This can lead to a lot of boiler plate code, which itself has readability and maintainability concerns, especially when redefining all the list operations on the Family type (AddName/RemoveName/EnumerateNames etc).
  • I prefer to wait to create methods on types until I see benefits of re-use among consumers and can compare that to the purpose of my type in the first place.

I prefer the invariant only approach, giving consumers the guarantee that Names won't be null and letting them take it from there.

Serialization concerns

  • If you are serializing objects to a database or other medium, be aware of how these impact your invariants.
  • readonly can cause a lot of friction in serialization, if it does, try an auto property with a public getter and protected/private setter, but be aware that deserializers may leave this null.
    • protected/private setters guarantee that at least code consumers aren't modifying the field.
  • I strongly recommend a good understanding of your serializer and possibly even some unit/integration tests to verify this invariant.

Conclusion

Null check insanity is often a sign of design smell, invariants are a great first step in the direction of creating a solid contract between producers and consumers of a type.

It may seem like work to enforce invariants, but the dividends in maintainability and readability are worth it. Think how often you stumble upon null checks, or the lack thereof. Understanding these patterns will make them second nature.

10.17.2013

TeamCity - Automatic Testing of Feature Branch Merging

Risk of Conflict

One of the drawbacks of Feature Branching is the likelihood of merge conflicts as time moves forward. Even with the best of intentions we all get busy and forget to merge our integration branch into our feature branch or vice verse. A mechanism to let us know when conflict occurs would be helpful.

Mitigating Conflict with Continuous Integration

Continuous Integration can help mitigate this risk by automatically merging changes in a feature branch with the integration branch and testing the result. Conflict may arise from the merge itself, or from compilation, testing and other stages of a deployment pipeline.

Leveraging TeamCity

Scenario

  • git for VCS
  • master is the integration branch
  • feature branches start with feature_
  • windows, so the script below uses batch file syntax

Setup

  • Add a new Build Configuration

    • General Settings
      • Name: Automatically Merge and Test Feature Branches
    • Click "Version Control Settings"
      • Choose "Create and attach new VCS root"
        • Type of VCS: Git
        • Fetch URL: set based on your project
        • Default branch: master
          • Or if you have a "primary" feature branch you can use that instead
        • Branch specification: +:refs/heads/(feature_*)
          • This will automatically monitor all branches that start with feature_, even new ones!
          • ()s denote what to show in the TeamCity UI, ie: refs/heads/feature_one will show as feature_one
          • See Working with Feature Branches for more
        • Authentication: set based on your project
        • Save the VCS root
      • Back on the Version Control Settings page
        • VCS checkout mode: Automatically on agent (if supported by VCS roots)
          • This ensures the agent has a copy of the git repository, not just the contents of the last commit
        • Clean all files before build: checked
          • This forces the build directory to be cleaned before each build, otherwise the git repository and files from the last merge would remain and your script would need to be able to handle both the first build as well as subsequent builds. I prefer to work from the assumption of a clean environment at the start of each run.
    • Click "Add Build Step"

      • Runner type: Command Line
      • Name: Merge
      • Run: Custom script
      • Custom script:

        "%env.TEAMCITY_GIT_PATH%" fetch origin
        "%env.TEAMCITY_GIT_PATH%" checkout -b master origin/master
        "%env.TEAMCITY_GIT_PATH%" config --local user.email "automerge@merge.com"
        "%env.TEAMCITY_GIT_PATH%" config --local user.name "Auto Merge"
        "%env.TEAMCITY_GIT_PATH%" merge --no-commit %teamcity.build.branch%
        
      • The script above is crude to demonstrate the process, feel free to modify it to fit your needs

        • First, fetch everything
        • Then, checkout the master branch
        • Next, set user & email
          • git requires this even with --no-commit on a merge
        • Finally, merge the feature branch into master
      • Save the Build Step
    • Go to "Build Triggers"
      • Choose "Add new trigger"
      • Choose "VCS Trigger"
      • If you used master as the default branch in your VCS root, you want to exclude it in the branch filter as it doesn't need to be merged with itself :)
        • -:refs/heads/master
      • Save the trigger

Things to try

  • Run your new build configuration
  • Try committing in a feature branch that results in a merge conflict
    • Wait for notification
  • Try fixing the merge conflict in the feature branch
    • Wait for notification
  • Add subsequent Build Steps for
    • Compilation
    • Testing
  • Consider creating your own Build Step via a template to re-use this script across projects
    • or Consider checking this script into your repository so it can be version-ed with the rest of your project
  • Modify the script to handle other feature branching and integration branching setups

On auto pushing "successful" merges

Some teams like the idea of pushing if the merge succeeds and tests pass. I guess that depends on your particular team and how you work together. That said here are my thoughts:

  • A merge is a change and should require human review.
  • Tests (if they exist), aren't guaranteed to cover the merged features.
  • The bang for the buck is notifying us when merge conflicts begin.

If auto pushing is of value, here's how you can modify the above to accomplish that:

  • Drop the --no-commit on the merge
  • Set real user name & email you want on the merge commit
  • Add a push command to the end of the script
  • And probably want to detect if there's even a need for a merge commit before pushing or this could turn into a never ending loop :)

Wrap Up

Obviously this can't mitigate all forms of conflict. As Martin Fowler points out, semantic conflict will be very difficult to detect, especially if there isn't much test coverage.

Nonetheless, checking the basics can help us catch issues faster and remind us to keep the flow of information between integration and feature branches open.