Spot the bug

Code A

STDMETHODIMP CMIB2Iface::CollectData(LONG DeviceId, BSTR IpAddress) {
	list<CString>::iterator listIter;
	for(listIter = ccmList.begin(); listIter != ccmList.end(); listIter++) {
		CString ccmIP = *listIter;
		long tempCCMID = getDeviceID(ccmIP);

		// Handle it only if device ID is valid
		if (tempCCMID != -1) {
				CString component;
				long eventType = 0;
				if (endPointType == 0) {
					eventType = 47;
					component = "Gateway Link - ";
				} else if (endPointType == 1) {
					eventType = 48;
					component = "Unity Link - ";
				}

				component += endPointName;
				// Construct an event.
				LunaEvent ev;
				CString attributes = "linkDestination = " + endPointIP;
				ev.attributes = attributes.AllocSysString();
				ev.eventTypeId = eventType;
				ev.deviceId = theCCMID;
				ev.component = component.AllocSysString();
				ev.timestamp = (COleDateTime::GetCurrentTime()).operator DATE();

				try {
					if (endPointStatus == 0) {
						if (tempCCMID == theCCMID) {
							// Raise/Update an event whenever status is 0
							if (prevEndPointStatus == 0) {
								// Case 00
								unsigned long result;
								MY_LOG_DBG(functionName << ": Updating Event on Component : " << component);
								if (faultManager->updateEvent(ev, &result) == S_OK) {
									MY_LOG_DBG(functionName << ": Updating Event Secceeded");
								}
							} else {
								// Case 10
								unsigned long result;
								MY_LOG_DBG(functionName << ": Raising Event on Component : " << component);
								if (faultManager->raiseEvent(ev, &result) == S_OK) {
									MY_LOG_DBG(functionName << ": Raising Event Secceeded");
								}
							}
						}
					} else {
						// Case 11 and 01
						// Always clear the event, and for all CCMs in the cluster.
						unsigned long result;
						MY_LOG_DBG(functionName << ": Clearing Event on Component : " << component);
						if (faultManager->clearEvent(ev, &result) == S_OK) {
							MY_LOG_DBG(functionName << ": Clearing Event Succeeded");
						}
					}
				} catch(...) {

				}

				SysFreeString(ev.component);
				SysFreeString(ev.attributes);
		} // End of if (deviceID != 0)
	} // End of for loop
	return 0;
}

Code B


STDMETHODIMP CMIB2Iface::CollectData(LONG DeviceId, BSTR IpAddress)
	{
	this -> m_sLog.SetIpAddress(IpAddress);

	/*   ---------------------------------------------------------------------   */

	m_oInspector.AddCollatedRowInspectorFn_Post(FilterIfTypeInfoCollect,
						(void *) this);

	/*   ---------------------------------------------------------------------   */

	m_oInventory.ClearAllCollectors();

	if (BOMBED(DoTableGenericInit(m_oInventory, sIfaceTblColln, m_sLog)))
		{
		_log(L_ERROR, "Mib2 collect table init failed \n");
		return S_OK;
		}

	/*   ---------------------------------------------------------------------   */

	if (PASSED(m_oInventory.CollectInventory (DeviceId, IpAddress)))
		{
		GetIfFaultsExistForDevice(DeviceId, LINK_DOWN_NUM , m_iFaultExist, m_sLog);
		m_oInventory. InspectData(DeviceId, eForInventory);
		m_oInventory. SaveData   (DeviceId, eForInventory);
		}

	return S_OK;
	}

Code Review Department

Code Review Department

If tasked at reviewing the 2 pieces of code listed above, chances are that, you might as a reviewer, want to get rid of the task of reviewing A, than spend any more time on an obviously bad effort at coding. Having badly styled code around, allows bugs to remain, and fixes to be nothing more than half hearted patches which neither the patcher nor the original coder can recognize after the act is done.

One bad turn soon begets another and the rate at which the bad code degenerates into sphagetti mess can only be matched by the speed at which engineers long for a total rewrite of this mess. (Failing which they secretly start yearning for a change of job or worse, stick to the same one in a lost/zombie sort of way or decide what they really want to be is a manager)

Humans are not compilers

Human Compiler

Human Compiler

Unlike compilers, humans being are quite easily put off by bad code and no act of redemption can save a bad piece of code from degenerating further and further until it can only be discarded. This is frequently the reason why engineers advocate re-architecture, which is hugely more expensive than adapting existing code. The lost business oppurtunity that this represents can be huge and the very effort of doing so could run into millions of dollars per year for a major software company like CISCO.

Any act of review on such a bad piece of code can only seen as a sham – who after all in their right mind would want to spend so much time reviewing someone else’s code when it would so obviously take such a long time to do it.

So the first step in getting working reveiws is to not let the process to be viewed as sham – and the only way that this can be achieved is by respecting the reviewer’s time and patience.

How do you respect a reviewers time ?

By giving readable code (see above)

How do you make code readable ?

  1. By avoiding coding smells
  2. bi folwing sm cdng convntns(by following the same coding conventions aka coding style)

Twenty different people and their cousins would have an equal number of vastly differing coding styles. If all of these are to be heaped on same page or on the same reviewer, again its equivalent to implicitly admitting that you have no control and that the whole process is a sham. Therefore getting the same style of code to be used EVERYWHERE is what has to be ENFORCED for successful code reviews to happen.

My experience

In the only place i have personally seen code reviews to actually work, a body of code could be rejected simply because a character exceeded the max allowed no of column width of 80 or because in a single location in code, variable was not aligned. The rationale behind this was the assumption that that the coder did not know about this rule and therefore there would be many other places where this mistake would be made.

The act of rejecting the whole body of code due to single misfitting character sends the right signals and soon it becomes a matter of pride to get code accepted without major comments.

The other advantages in having readable code is that

  1. Simple Bugs are easier to spot
  2. Design issues are easier to spot
  3. Review happens faster (that builds more support for review)
  4. Bug fixes are easier
  5. Module upgrades or re-architecture would be easier
  6. SPotting reuse chances are easier – and hence aids faster development
  7. tois which so often happens are much more easier

How can the organizations aid in creating readable code

  1. Use tools that can perform static analysis based on accepted coding styles and flags issues, before reviewer even reads one line of code
  2. Make code readability indexes, part of statistics of the project so that they can be easily tracked
  3. Use tools that can reformat existing code base to the accepted style, no matter what that is, so that its easy to get off the block when the process starts

1 out of 2 is still only 50%

Fixing the code review-ability is only half the fight. Where would a reviewer be, without a programmer who wants the code to be reviewed ? In a team of colleagues, its only more than easy for everyone to scratch everyone else’s back and implicitly give easy reviews. Its also natural for colleagues to contest one others reviews. Therefore, there has to be an external entity and stimulus to engage in code reviews.

Of-course enforcing the code standards using tools part of the build process and enforced through management numbers would be a good start, though that alone might not make it stick.

My experience is that usually stimulus takes the form of bragging factor or nerd pride amongst peers. When an obviously respected person  / geek is to review your code, you would want it to be more shiny and that extra perfect.

Companies like Cisco can easily make this happen by having more tech leads to review the code, and having a system of recommendations where recommended parts of code can even get promoted to technical evangelists like Stroustrup or Gosling once a year for review, which can be such an honor. Tracking  programmers, coding bullshit index would also be a good measure for wanting a review.

The human factor

Coding and reviewing code, is ultimately a very human activity and any mechanical solutions to the problems in this domain are bound to fail. Ultimately a more humane and social solution would be required to transform this bit of ‘work’ into something more than statistic, and something of an advantage.

“Any fool can write code that a computer can understand. Good programmers write code that humans can understand.”

– Martin Fowler

Advertisements