I run across this type of code often (ASP.Net MVC)

[HttpPost]
public string Delete(ObjectId id)
{
    var record = _Database.Get<Record>(id);
    if (record == null)
    {
        return "No matching record!";
    }
    _Database.Remove<Record>(id);
    return "Deleted record!";
}

If the user is interested in deleting the record, does it matter if someone else beat them to it?

[HttpPost]
public string Delete(ObjectId id)
{
    _Database.Remove<Record>(id);
    return "Deleted record!";
}

Isn't the HTTP 200 OK status code enough to indicate success, the user or the software knows the context of the request it made:

[HttpPost]
public void Delete(ObjectId id)
{
    _Database.Remove<Record>(id);
}

Simple things to think about when reviewing code for simplicity, one line of code is much easier to maintain than seven.

When we develop libraries (whether internal or public), it helps to have a rapid ability to make changes and test them in a consuming application.

Building

• Setup the library with automatic versioning and a nuspec
  • Setup library assembly version to auto increment build and revision
    • AssemblyInfo –> [assembly: AssemblyVersion("1.0.*")]
      • This autoincrements build and revision based on time of build
    • Major & Minor
      • Major should be changed when you have breaking changes
      • Minor should be changed once you have a solid new release
      • During development I don’t increment these
  • Create a nuspec, version this with the code
    • nuspec - set version to <version>$version$</version>
    • This uses the assembly’s version, which is auto-incrementing
• Make changes to code
• Run automated build (ruby/rake)
  • run “rake nuget”
  • nuget task builds nuget package and copies it to a local nuget feed
    • I use an environment variable to point at this so I can change it on a machine level!
    • The nuget command below assumes a nuspec is checked in called Library.nuspec next to the csproj file

  • $projectSolution = 'src\\Library.sln'
    $nugetFeedPath = ENV["NuGetDevFeed"]
    
    msbuild :build => [:clean] do |msb|
      msb.properties :configuration => :Release
      msb.targets :Build
      msb.solution = $projectSolution
    end
    
    task :nuget => [:build] do
      sh "nuget pack src\\Library\\Library.csproj /OutputDirectory " + $nugetFeedPath
    end

• Setup the local nuget feed as a nuget package source (this is only required once per machine)
• Go to the consuming project
• Update the package
  • Update-Package Library
  • or Install-Package

• TLDR
  • change library code
  • run “rake nuget”
  • run “Update-Package library” in the consuming application
  • build/test!

If you manually execute any of this process, especially copying files, you will find it a burden to develop the library and will find yourself dreading it, and even worse, making changes downstream instead of updating the shared library for everyone’s sake.

Publishing

• Once you have a set of changes that you want to release, consider versioning and possibly increment the minor version if needed.
• Pick the package out of your local feed, and copy it to a public / shared feed!
  • I have a script to do this where I can drop the package on a batch file
  • Replace apikey with your nuget feed's apikey
  • Take out the confirm(s) if you don't want them

  • @ECHO off
    echo Upload %1?
    set /P anykey="Hit enter to continue "
    
    nuget push %1 apikey
    
    set /P anykey="Done "

• Note: helps to prune all the unnecessary versions during testing from your local feed once you are done and ready to publish

• TLDR
  • consider version number

    <li>run command to copy to public feed </li>
  </ul>
  

Note: in this article when I refer to a commit, I mean the commit you plan to share with the rest of the team, if you have local commits that you plan to amend/combine, I am referring to the final result.

In time you will find these easier to do as you develop, however, all of these are valuable before checking in! The pre commit review is a nice time to polish what might have been several hours of intense work, during which these things were the last things on your mind! If you are concerned about losing your work in the process of responding to these questions, first do a check-in and amend it as you go (assuming you are using a tool such as git that supports this), rolling the result into one nice commit for everyone else.

Did you review your commit, change by change, with a diff utility?

• If not, this is a list of reasons why you might want to start!

Did you test your changes?

• If the test is valuable to be automated, is it?
• If it’s a manual testing scenario, did you at least try the basics manually?

Are the additions/changes formatted consistently with the rest of the project?

• Lots of automated tools can help here, don’t try to manually format the code, that’s a waste of time and as a human you will fail repeatedly.
• Are these consistent: tabs versus spaces, indentation, spacing, braces, line breaks, etc
• Resharper is a great example of a tool that can automate this for you (.net)

Are naming conventions respected?

• Did you accidently use abbreviations, unless you have a good reason to use them?
• Does capitalization match the conventions in the project/language?

Are files partitioned?

• Sometimes we add new code in existing files in a pinch, it’s a good idea to split these out if they don’t belong
• ie: are new classes defined in new files, if this is something your project values?

Is there commented out code?

• If you are removing an existing feature, get rid of it, that is why we have VCS
• If it’s not done yet, then why are you checking it in?
  • Perhaps a stash commit (git)?

Did you leave debug or unnecessary changes?

Do you understand all of the changes?

• Programming doesn't have to be Magic

Are there spelling mistakes?

• Including your commit message!

Is your commit message concise?

Is there follow up work?

• Are there tasks you didn’t write down that you need to follow up with?
• Are readability or reorganization changes needed?
• This might be amended into the final commit, or it might be future work that needs added to the backlog.

Are there other things your team values that you should review?

In the show LOST, the Swan Station had a button that “had to be pushed” every 100 minutes to avoid disaster. Several characters in the show took it upon themselves to have faith and religiously push the button, resetting the clock and averting the unknown “disaster”. There are striking similarities in this story to the code we write every day. Here are some common ones that I encounter:

• “I don’t know what it does but the application doesn’t work without it”
• “I added that code because I saw it in other similar places, I didn’t understand it, but thought it was necessary.” (for consistency, or to make things “work”)
• “An error message recommended it”
• “I copied that code” (and didn’t look at what it was doing)
• “It was suggested in a forum online and it fixed my problem so I left it”

In all of these cases we haven’t done our due diligence to understand what the code we are writing is actually doing. In the rush to get things done it seems like we’re willing to push any button (add any line of code) just to get our desired result and move on. All of the above explanations are common things we encounter, and are valid ways to work through a problem we have, but when we find a solution to a task we are working on (whether a bug or a feature), we should take a moment to reflect on what we don’t understand. Remove what isn’t necessary, comprehend and simplify what is.

Why is it detrimental to commit code we don’t understand?

• Perpetuates unnecessary code
• If you copy code that isn’t necessary, someone else is more likely to do so, especially peers
• Perpetuates tech debt
• Adding unnecessary code leads to extra code that must be understood, maintained and eventually cleaned up in longer lived projects
• Tech debt begets tech debt as other developers copy or use this code as guidelines in similar situations
• Increases maintenance
• How do we know the code is simplified if we don’t understand it?
• Perpetuates a lack of ownership
• Makes it seem ok to commit anything so long as it “gets the job done”
• Perpetuates the notion that programming is magic
• If we don’t take the time to understand every line of code we add, then we are contributing to the notion that it is simply enough to make the code work, regardless of how.

TLDR

Don’t commit code that you don’t understand, take the time to understand it, simplify it and then commit it!