Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.

share|improve this question

migrated from stackoverflow.com Feb 8 '11 at 14:54

This question came from our site for professional and enthusiast programmers.

1  
@Anon: I think it will be moved to programmers.stackexchange.com. –  John Saunders Feb 8 '11 at 14:37
1  
Thanks @John :) –  Anonymous Feb 8 '11 at 14:38
2  
Switch to a distributed source control. Its free and wont cause you pain and suffering. They make questions like this obsolete. –  acidzombie24 Feb 9 '11 at 9:27
4  
@acidzombie this question has nothing to do with TFS or the kind of VCS –  Bart Jul 13 '11 at 13:42

22 Answers 22

up vote 146 down vote accepted

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.

share|improve this answer
45  
+1 it's obvious, but if anyone out there is not doing this, then they are doing it wrong! –  David Heffernan Feb 8 '11 at 14:28
6  
+1 Actually, it is not that obvious but if you don't do it you'll be sorry. –  Nemanja Trifunovic Feb 8 '11 at 16:50
14  
+1 Also, if you think this is too much work, you are probably committing too much at once. –  mpeterson Feb 8 '11 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 Feb 8 '11 at 19:16
4  
If it's not worth looking through, it's probably not worth checking in. –  Robert Jeppesen Feb 8 '11 at 22:35

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?

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer

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:

REFACTORING: Rename X to Y

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.

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer

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.)
share|improve this answer

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.

share|improve this answer

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).

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer

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

share|improve this answer

Search and replace curse words ;-)

share|improve this answer
1  
+1 if you ever had to do it –  Filip Dupanović Jul 13 '11 at 14:38

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.

share|improve this answer

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...

share|improve this answer
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 Feb 8 '11 at 19:10
7  
Don't all of those steps just move from when you checkin, to when you push? –  user13278 Feb 8 '11 at 19:30
2  
+1 for the use of pantsweasel –  Dan Williams Feb 8 '11 at 21:45
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. –  Robert Jeppesen Feb 8 '11 at 22:39

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)

share|improve this answer
2  
This can be set as a check-in policy, so that no code can be checked in without associating to a work item. –  John Saunders Feb 8 '11 at 19:59

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.

share|improve this answer

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.

share|improve this answer

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.

share|improve this answer
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 Feb 8 '11 at 17:44
1  
Exactly. This is why you don't use tabs. –  Alex Budovski Feb 9 '11 at 2:16

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)
share|improve this answer
11  
+1 - that's a pretty complete list right there! DON'T BREAK THE BUILD!! –  jmo21 Feb 8 '11 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 Feb 8 '11 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. –  Alex Budovski Feb 9 '11 at 2:12
2  

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

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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