This is the fifth part in a series of articles in which I’m ‘highlighting’ what I consider to be lousy code provided by IAR in their NXP LPC1764 development kit. This code excerpt is taken from usb_t9.c in the AudioDevice project.
/*************************************************************************
* Function Name: UsbCoreReq
* Parameters: UsbCoreReqType_t Type
*
* Return: Int32U
*
* Description: Return device states
*
*************************************************************************/
Int32U UsbCoreReq (UsbCoreReqType_t Type)
{
switch(Type)
{
case UsbCoreReqDevState:
return(UsbDevCtrl.State.DS);
case UsbCoreReqDevSusState:
return(UsbDevCtrl.State.Suspend);
case UsbCoreReqConfiquration:
return(UsbDevCtrl.Configuration);
case UsbCoreReqInterface:
return(UsbDevCtrl.Interfaces);
case UsbCoreReqDevOwnAddress:
return(UsbDevCtrl.DevAdd);
case UsbCoreReqWakeUpEnableStatus:
return(UsbDevCtrl.Feature.RemoteWakeupEnable);
case UsbCoreReqSelfPoweredStatus:
return(USB_SELF_POWERED);
#if USB_HIGH_SPEED > 0
case UsbCoreReqSpeed:
return(UsbDevCtrl.HighSpeed);
#endif // USB_HIGH_SPEED > 0
}
return((Int32U) -1);
}
I suspect that on first blush many readers will not be too offended by this function. However it contains several things that are pet peeves of mine. I’ve listed them in the order that they occur in the function
- As with most of the code, I found the function header to be almost useless. Having read it multiple times and also having looked at the code, I’m still not really clear what the purpose of this function is other than to provide access to some of the parameters of the structure UsbDevCtrl.
- This function does nothing more than switch on a parameter and return a value. As I outlined in this post, IMHO one should use multiple simple functions rather than one large function such as this. The resulting code is smaller, faster and the purpose of each function is much clearer. It also avoids the problem that is discussed in point 7 below.
- The author has used the variable name ‘Type’. While I know this is legal and it may even be somewhat meaningful in this case, I find the idea of using ‘Type’ as a variable name just plain confusing.
- The function contains no-less than nine return statements. Most coding standards require that a function have a single exit point. I have to admit that if you’d asked me ten years ago whether I thought this was a good idea I’d have probably equivocated. However in recent years I have really made an effort to ensure that all my functions have a single exit point – and I have found it has improved my code a lot. Of course there are the odd cases where it really is unavoidable. However in this case, maintaining a single exit point would have been trivial.
- The argument to the return statement is in parentheses. This seems to be a popular construct, which I simply do not understand. What benefit do the parentheses confer? To those that ask what harm are they doing, then I would suggest that they make the code harder to read.
- The switch statement has no default clause. Yes, one will fall through to the last return statement – but it’s a sloppy way of handling it.
- The pièce de résistance is of course the cast of -1 to an unsigned integer. The function is declared as returning an unsigned integer – but in one case it returns a signed integer that is cast to unsigned. I’m sure that you will not be surprised to hear that none of the functions that call UsbCoreReq() check to see if “-1” has been returned.
I don’t think this example is quite as horrific as some of the others I have highlighted – but it’s still pretty bad. As always your comments make this interesting!
