I recently held a code review for a bug fix I made and one of the reviewers commented that I should check the return code of GetDlgItem in a line similar to this:

GetDlgItem(IDC_BUTTON_REMOVE)->EnableWindow(FALSE);

so that I can check the pointer before using it. But if we’re going to that much trouble then technically we should do something more like this:

CWnd *pRemoveBtn = GetDlgItem(IDC_BUTTON_REMOVE);

if(pRemoveBtn)

      pRemoveBtn->EnableWindow(FALSE);

else

      // Write out to log file

…or perhaps something a little more generic like a macro or inline function which would help keep the line count down but would also make the logging not as specific for each usage.

I don’t know about you, but I’ve never checked GetDlgItem before using it. One simple jaunt through the code during runtime will let you know if it works or not. It’s a different story when you placed the controls on the dialog dynamically, but when you’re using static controls simple unit tests will easily find any typos or bugs in those function calls.

He then commented that during future maintenance say that the programmer renames the control from IDC_BUTTON_REMOVE to IDC_BUTTON_DELETE. While that would definitely bork up the code, I feel that making changes like that fall under what I call “programmer responsibility”. If you change the resource ID of a button, you better do a search (and replace) on all your code for the old resource ID, delete the old resource ID definition (if it’s not used elsewhere) to prevent erroneous compilations with the no longer used resource ID and maybe also run through those dialogs once or twice to make sure that all the old resource ID references are actually gone.

Sure checking for the return code from GetDlgItem will have prevented your app from crashing if you left an old resource ID in there, but if you did that then it’s your own fault. You were either lazy or didn’t care to check the whole source. Also, if you had checked GetDlgItem for NULL before continuing then any references to the old resource ID would not cause the app to crash. The control would simply not perform the action. In the case of EnableWindow, the control would not change to enabled or disabled. It would take either a keen eye in the dialog or a meticulous hand in the logs to see that EnableWindow was not performed, especially in a real time app. In this case, I could definitely argue that letting the app just crash would be preferable since any old references would definitely be caught. So…

Is there such a thing as programmer responsibility or should absolutely every effort be taken to prevent bugs?