My team uses Team Foundation Server for source control, and today I fixed some bugs and smoke test application before I checked it in but I forgot to comment some code. (This code made the UI a little bit strange.)

I want to know what good practices there are before checking code in - I don't want to make this kind of mistake again.


22 Answers 22


One thing I have gotten in the habit of doing is always looking at the diffs of every file I'm about to check in, right before I check them in.

  • 46
    +1 it's obvious, but if anyone out there is not doing this, then they are doing it wrong! Commented Feb 8, 2011 at 14:28
  • 6
    +1 Actually, it is not that obvious but if you don't do it you'll be sorry. Commented Feb 8, 2011 at 16:50
  • 14
    +1 Also, if you think this is too much work, you are probably committing too much at once.
    – mpeterson
    Commented Feb 8, 2011 at 17:53
  • 5
    Also, looking at the diffs makes it easier to know what to put into the descriptive note about what you've been trying to achieve with your changes, especially in case you did multiple fixes.
    – Jonas
    Commented Feb 8, 2011 at 19:16
  • 4
    If it's not worth looking through, it's probably not worth checking in. Commented Feb 8, 2011 at 22:35

You should never check-in commented-out code. If you have code that needs commenting out before check-ins, you are doing it wrong.

As for rules:

  1. Get latest
  2. Fix merge conflicts
  3. Build

    3.1 Fix build errors

  4. Run tests

    4.1 Fix broken tests

  5. Go to 1 (until there is nothing new to get)

Only check in when all steps are complete.

See check-in dance.

Other good practices:

  • Review the list of files to check-in to ensure they are the expected files.
  • Review the changes for each file (deletes/additions/diffs)
  • I did a double take here. Perhaps you mean 'commented-out code'? Myself, I'd lean towards never checking in uncommented code! Commented Feb 8, 2011 at 14:32
  • 11
    +1 - that's a pretty complete list right there! DON'T BREAK THE BUILD!!
    – ozz
    Commented Feb 8, 2011 at 15:12
  • 1
    @Philip - So long as you know that this is not good practice and so long as this is a simple short term intermediary, then this is one of the few cases to break that rule. I find it much more concerning when people check in commented out code so they "won't lose it".
    – Oded
    Commented Feb 8, 2011 at 21:34
  • 2
    @Philip, That's why git is nice. You can commit those WIP changes locally, as often as you like, then before you push to the main repository, you rebase -i and clean up your local history, squashing commits as necessary, so the mainline has no ugly work-in-progress commits. Commented Feb 9, 2011 at 2:12
  • 2
    @james gobarbra.com/hit/new-d1bbb9dcf1a7fdbec622e107ca25e16c :P
    – pkoch
    Commented Feb 9, 2011 at 2:46

I'm not trying to be too much of a pantsweasel here, but the assumption in this question (and all but one of the answers) mostly applies to Centralized VCS, such as TFS, SVN, Perforce, etc.
Fair enough, it's what the OP is using.

On the other hand, however, when using DVCS (such as Mercurial and Git), you usually shouldn't wait to checkin, and most of the things mentioned in answers - such as diffs, get latest, merge, etc - are not necessary. Even things like code reviews and tests are better to do after the checkin (though perhaps before pushing, depending...)
The one exception I saw here (so far) is associating with a work item. Of course, commenting on the checkin is also good...

  • 5
    +1 for commenting on the checkin. It's not policy in my shop, but I try always to leave a descriptive note, if only to jog my memory later on.
    – PSU
    Commented Feb 8, 2011 at 19:10
  • Agreed - I imagine that Oded's workflow could benefit a lot from version control between each of the steps, or, at the very least, between each of the loops. Commented Feb 8, 2011 at 19:14
  • 7
    Don't all of those steps just move from when you checkin, to when you push?
    – user13278
    Commented Feb 8, 2011 at 19:30
  • @user13278 some of them do, but differently. E.g. Merging is a completely different experience - and, you do while pushing, no need for a getlatest-merge-tryagain cycle. And you can do it for a whole bunch of changesets, and not have to remerge every checkin. In general, a lot of those steps don't have much to do with the checking in anymore - e.g. you pull when you want, not because you're checking in (or pushing). Of course you still need to test - but that can be on its own timeframe. Pushing still remains a lot more lightweight, but of course you want to make sure you dont push crap.
    – AviD
    Commented Feb 8, 2011 at 19:36
  • 2
    +1. Associating with a work item is the one hard thing to do in Git or Hg. You'll need to run a whole package, like Kiln. This is the (only) area in which TFS is good. It is harmful for version control though. Commented Feb 8, 2011 at 22:39

Three things that I didn't see in other answers:

Include new files

  • Look for new files that aren't part of your changelist
  • May be specific to SCMs like Perforce -- you have to tell it everything that's in your change.

Revert unchanged files

  • I hate when I'm looking at other people's changes and there is a changelist with nine files but only three of them have been modified.
  • I also revert files with whitespace or otherwise meaningless changes.

Check your submitted commit

  • Make sure that the build stays green after your commit.
  • I used to have a second machine that I would sync, build, and run after my commits so if something went wrong, I could easily jump in and fix it.

Two things when I use Git:

Atomic commits:

  • Only stage individual functional changes for commit.
  • Make commits as small as possible. Make them easy to patch, revert, and understand.
  • I use git add --patch to split my change up into multiple parts if necessary.

View diffs while summarizing

  • I always check in with git commit --verbose so I can see a diff of my change while I'm typing in my commit message. (Or I use my patched git-vim to show the diff.)
  • This makes it much easier to go through your changes and describe the whole commit. Occasionally, I catch unintended changes at this stage. (Describing your change helps you think about it.)
  • +1 for being the only person to mention atomic commits. Commented Feb 9, 2011 at 12:50

A few items of 'good practice' that I enforce on my team's servers are pretty straight forward. First, before you check in, you should always get latest and run a local build, to ensure that no one else has checked anything in that you code will clash with. Additionally, take care of any code conflicts on your local machine, not the server. Once your code, with the latest code downloaded, has been confirmed to build and work properly you are ready for the next step. Run any automated tests then begin your check-in to ensure they still function properly. Then, just to be sure, get latest again.

It's possible, as a TFS Admin, to enforce commenting on all check-ins. I would recommend always putting in check-in comments for your work regardless of if it is enforced or not. If you have the option to do so, enforce it. Make sure the comments are, at the least, a general summary of what you changed since the last time you checked your code in. That way, if something goes wrong, you can look through the check-ins and see, roughly, what was changed in that check-in. It makes debugging a broken build much easier.

Additionally, if you have TFS Admin privileges, enforce rolling builds on check-ins (to make sure everyone else knows right away if their check-in breaks something), and you can set up the server to either perform a gated check-in (if the checked in code breaks the build, the server rejects it), or you can simply have it create a bug and assign it to whoever broke the build.

There are a few other options you can turn on or off to keep everything well in order, or to suggest to your TFS-Admin to turn on to keep things nice and clean...but they are largely preference

  • I like this answer. As QA, sometimes we trace a bug back to the commit that caused it to appear, and it is nice to have descriptive comments available. Also at release time, our shop creates something called release nores, which is a distillation of new features and changes, and the checkin notes are an important source of this information. Commented Feb 8, 2011 at 21:02

Search and replace curse words ;-)

  • Just be sure not to make the clbuttic mistake.
    – Tullo_x86
    Commented Jul 31, 2012 at 23:29

If you are checking in from Windows, check if your code does not have those invisible ^M characters -- editors in UNIX often give errors/warnings 'cause of that.

Also try and replace tabs -- different users will end up seeing tabstops differently some with 4 spaces, some 8 and not good for code readability.

Best approach IMHO is to have a pre-defined script check your code against the coding guidelines of your organization. Loads of source control systems have this functionality.

  • 4
    Checking for the ^M characters only makes sense if a UNIX box is actually involved in the development process in any way. Some companies are all-Windows shops.
    – Dima
    Commented Feb 8, 2011 at 17:44
  • 1
    Exactly. This is why you don't use tabs. Commented Feb 9, 2011 at 2:16
  • Some SCMs handle line endings for you (some do better than others). Perforce (kb.perforce.com/?article=063), git (core.eol in git config), svn (svn:eol-style), etc.
    – idbrii
    Commented Feb 9, 2011 at 2:28
  • @Alex: Or you consistently use tabs everywhere. It doesn't matter which you do so long as you're consistent. Commented Jul 13, 2011 at 15:05
  • @donal, no. Herein is the problem; tabs are subject to your editor's configuration and hence inherently inconsistent. Some "editors" are inconfigurable, such as cmd.exe, and most Linux consoles, so you might even be inconsistent with yourself. Commented Jul 17, 2011 at 21:39

At my company we use check-in reviews. These don't have to be detailed, but just showing someone the diffs in your changelist and talking through them will sometimes highlight the odd typo you've missed in testing.

Our source control server won't allow you to check in unless you note the reviewer's name in the comments (in the form !initials, eg !BW if Bruce Wayne did your review). The reviewer gets an email noting that they helped review. This is open to obvious exploitation but seems to work pretty well.


Whenever possible, I like to associate a check-in with a work item. This gives you some contextual information about WHY this was changed, not just WHAT was changed. TFS has a fairly decent work item tracking system, so this is rather trivial to do at the time of check-in.

(this is in addition to reviewing the diffs of my changes)

  • 2
    This can be set as a check-in policy, so that no code can be checked in without associating to a work item. Commented Feb 8, 2011 at 19:59
  • Good point, John. I am actually hoping to do this very soon where I work.
    – mpeterson
    Commented Feb 8, 2011 at 20:07
  • Enforcing stuff is usually counter-productive. Make sure your people understand that it's good for them instead. Commented Feb 8, 2011 at 22:42

One small thing I do is to not check in files which haven't really changed. These files may have been modified inadvertently, or may have been involved in refactorings which have either been rolled back or otherwise been made moot.

This way, your changeset (associated with a work item) contains exactly the files necessary to satisfy the work item.


To combine all answers here and give a complete checklist

  1. [check in / check out] you should not check in directly to the stream others are working in. You should have a stream strategy: e.g. per developer a stream in which you can check in and check out independently without bothering others: your work will be safe but in your own development stream so [only in your own development stream]. With every check in you associate it with a change record so that your changes are atomic against that change called the change set (so you can distribute individual rfc's / bugs etc... without having to deliver ' everything ' ).

  2. [then rebase with your team stream] it means you get the changes from others in your own stream. During that operation you can see in the merge dialog all the "diffs" and go through them or... if there are thousands and/or you are using not code but also e.g. data models / siebel projects etc... rely on either non trivial merges , trivial-automatic and trivial manual merges , the last category contains the difficult ones. Remember that you are still working in your own stream.

  3. [complete rebase] If all is ok then check in all the changes you just got from the team stream: your own stream is now up-to-date

  4. [deliver] now deliver your work to the team stream. IF you dont want to deliver everything you can also select e.g. 1 specific RFC with that specific versions of files or a set of RFC's / defects solved.

  5. [test deliver] it should go ok but since the chance exists that someone in the meanwhile delivered changes also: you can test if your work works with the latest changes on the team stream. With the same merge dialogs showing differences.

  6. [complete deliver] complete your deliver and your work is now in the team stream.

To make it more complex: Since there is still the chance that the work you delivered = ok BUT you are already working on a further version you should baseline always after deliver and indicate which baseline that one is that is preferred for other users to rebase from. That makes sure that other developers get a recommended one and not the latest version on the stream (if you are working in that scenario). That is also a Triple check so that even IF the latest versions in the team stream are "bad" they are still not the ones that others rebase to or look at and your configuration manager can than merge the previous version to the next version to undo your delivery.

  • the answer from histumness happens 2 times : in step 2 and 6
  • the answer from Oded on check-in dance: idem but an extra layer of deliver and rebase on check in / check out to make sure you work isolated and errors can always easily be taken out even in later stages
  • the answer from answered guildsbounty: get latest is step 2. For the build: it really depends if you HAVE a build... in my world you have inputs from data models, word documents, requirement sheets, config data from informatica, siebel, etc.. , and yes also java code, .net code etc... that all should mingle together. So there is no really "a build" here but more a integration higher up depending if that single e.g. build from your "code" integrates with all the rest of the stuff since you can not know for sure if it is integration stuff and depending on their test environments it could be compiled stuff is needed or at higher deliveries another build because it needs something from another team.
  • the answer from guildsbounty on commenting and required: i think every environment in which you do not have integration of VERSIONS and Changes in change sets (and type: defects, RFC, hotfi) is not mature. I think its chaos if you can not e.g. automate release notes with the amount of bugs and rfcs submitted which are clickable through to the exact versions that are touched (since e.g. version 1 and version 3 of hello.c could very well be delivered but version 2 should not have been delivered because that stuff in there would be part of a later release but some noob already put it in) (so it means a manual decision IF you also want to take out version 3 of hello.c BUT taking version 3 out means you also have to take out all other version touched by that RFC/defect so you need to be able easily and fast with a tool to take the complete thing out)(even if multiple developers worked on parts of that same RFC) but at least you need stuff around it to decide on it etc...). Extra documentation is always handy but by associating change sets you get the full circle: a version <-- a change set <-- work items <-- a ticket/rfc/defect <-- a requirement. Meaning: you know which requirements are fully or completely delivered: one requirement has multiple RFC's or bugs or whatever. The RFC has multiple work items for multiple persons. that work item corresponds to a change set which exists of a set of versions (e.g. version 1 and 3 of hello.c on the integration stream that are ofcourse NOT the versions 1,2,3,4,5 in your development stream that version 3 in the integration stream corresponds to) (hence a version tree and tools to operate on it)
  • the comment from luis.espinal: dont break the build is double checked in rebase and deliver still... there are higher up deliveries for 'release managers and build meisters' who should see change sets and baselines as their info. " Never work directly on main branch" yes, the stream structure can be big or simple but in essence: developers have their own stream, they deliver to a team stream who deliver to a release stream. --> so that the deliveries from all teams (e.g. documentation team, requirements team, development teams, test team) are in their team streams and a release manager or configuration manager grabs the baselines that should go in a release and so makes sure the correct documentation with the correct test versions docs/outcomes with the correct requirement with the correct code versions are all in line for a release.

In your example you give that you forgot to commented out code. Mistakes happen. The configuration management system around it should take care of it. It can really one be that e.g. thousands of changes come in and "builds" and "integrations" take place in a hierarchy of streams on different servers chained and processed in time. So even if after 5 months your commented out code is tested on an integration server because your code needs integration with other code and systems it should still be possible to atomically take out your change set and still carry on. So the best practice is more or less on a higher level. The overall design of the configuration management streams should take care of it. For individual developers the best practice is ofcourse to validate / unit test. But from the big picture to " make it work " the best practice is to follow the system and always provide that meta information of related change sets for the guys later on in the chain.


Some of the following apply more than others (or in different forms) depending on your SCM, so here they go:

  1. Don't break the build - check only code that compiles.
  2. Comment your check ins (and possibly your check outs if your SCM gives you that option.)
  3. Don't keep stuff unchecked for long periods of time.
  4. Check in often (several times a day if possible.)
  5. Label meaningfully.
  6. Label regularly.
  7. Never work directly on the main branch.
  8. Every release to production must have its own label (and a read-only branch off the main branch if possible). When possible do the same for UAT/Integration/Pre-production test releases.
  9. You should be able to build exactly what it's on production from what is in your SCM and from a label.

NOTE : some of the items above seem rather obvious, but you would not believe how many people actually work on the main branch or make changes on production first and then manually create deltas to go on version control... directly on the main branch... and with labels. Sweet like fermented bile mixed with unwashed armpit juice... yeah, like that.


Have a personal check list. Start it empty when you mess up, at an entry. When it becomes second nature remove it from the list.

Run the tests. If they pass check it in. If you mess up and some thing gets past the tests, then write a test.


We do the following ...

  1. Test - We want to make sure that it works. At the very least, we want to know that it does not break anything.

  2. Code review, or at least a buddy check - This is a great way to ensure that the knowledge gets spread around and that people are kept up to date. It also helps catch bugs before checking things in.

  3. Send an advance notice - An advance notice is sent to the group before checkin. The purpose is not only to let others know which files or areas are changing, but it gives them a heads up (should they choose to take notice) in case those changes are expected to affect them.

  4. Some hours after sending the advance notice, the check in is performed, and the group is informed via email. Everyone in the group can know when the particular work on a bug or feature is done.

  5. A copy of the checkin notice is pasted into the fix record associated with the bug or feature. When searching through the records, we find that it is very handy to have an idea of what the fix/feature entailed.


Run your unit tests you've been so diligently working on. Green is good.


Make sure your code is formatted properly (e.g. for Java: select your code and press Ctrl-Shift-F in Eclipse). But be careful doing the same for an entire document.


Always, always, always, check that whatever changes you've done doesn't break the build. Especially minor changes that may seem trivial.

Once I made a very minor change that I didn't think would cause any problems right before I left work for the weekend. Sure enough, that little change broke the build and no nightly test runs were executed for our project. The Q&A chief wasn't too happy about that, and rightly so.


Look for portions of your changes that can go in as standalone units.

Often, by the time I have a working fix or enhancement to the code, there are quite a few changes. Some of them are specific to the behavior change I'm going for; others are refactorings I did to make that change cleaner.

I prefer to check in each refactoring separately, with its own change description, like this:


X made sense before because ... but now it should be Y. This is related to work for issue #9.

Then, once each good refactoring is checked in, the final behavior change is often trivial.

Also, some changes affect many lines of code but aren't very interesting, while other changes are very localized but have an important impact. If these changes are checked in together, it can be hard to read the diffs. So, I keep them separate.

Later, when someone is reading through the change history, it's obvious how things got to the current state of affairs, and why they are this way. It's also trivial to undo my behavior change because it's not tangled up with tons of other edits.


Do what you would do when returning something you borrowed from someone. Make sure it is clean and in good shape. If you did a mess, be sure to clean before returning the code to its owner (in most case, your employer).

  • git helps you clean your mess before committing publicly. Unfortunately, centralised VCSs do not. Commented Feb 9, 2011 at 2:19

I keep a local hg repo for my work.

  • Whenever I check something in, I look over the diff and make sure that all changes are acceptable.
  • I try to note the key feature of the checkin.
  • I try to keep each commit size to one key feature.

I don't claim these are the best, but they do work for me.


When I write code that I know isn't meant to be checked in, I add a line before it containing "//TEMP:" and after it with "//END TEMP.". This, together with doing diff before checking in, promises that I won't check that code in by mistake.


Thoroughly test everything you added or changed. Try every possible case you can think of to try. Don't leave testing to QA. If I had a sandwich for every time I've made some minor change, and then tried some test cases just to be on the safe side, and immediately saw problems, I'd have a lot of sandwiches. I usually say out loud to myself "I'm really glad I tried that..."

You say the UI became strange after your change. If you'd only ran the program and looked at the UI before checking in, would you have noticed the problem?

Not the answer you're looking for? Browse other questions tagged or ask your own question.