12
Oct
2017
Code Smells: Improving a sense of smell in low level debugging

Code Smells…But what is exactly a code smell?

Most developers are good at scrutinising code to look for bugs and errors – does the code do what it is supposed to do? Code smells are indicators about parts of the code which are not habitable – is the code readable, easily understandable and extendable, and maintainable? The analogy of code that smells bad is used to identify areas which need refactoring, and good smells can be samples of code to learn best practice from. An example of bad smells are badly named functions/variables that don’t reflect what they are doing or for, duplicated code, ambiguous and overly-complex or missing useful comments, and so on.

Bad smell example

Let’s start with a bad smell. There are many examples we could pick from, but let’s start with a short one:


static void _Delay(uint16_t microseconds)
{
   uint16_t i;
   for (i = 0u; i < microseconds; i++)
   {
      /* Delay 10usec */
      CLOCK_DELAY_US(10u);
      WATCHDOG_RESET();
   }
}

 

Why is this a bad smell example?

There are several bad smells here – the first one is that the function is not doing what you’d expect it to from the parameter name – it’s not delaying for microseconds, but for 10 microseconds for every microsecond requested! Finally, each time it does a delay it is “kicking the watchdog” in tech-speak! The watchdog is designed to detect any system crashes, and restart the system if required. Constantly resetting the watchdog means that if a hardware error occurs, or anything goes wrong that might cause a deadlock or crash, the device won’t be reset by the watchdog – which is crazy!

Another bad smell example

There’s a little more to this partial-function example than the previous one. It can be affectionately described as a waltzing example (due to all the forwards-and back shifting):


case GET_SECTOR_COUNT :
   /* Get number of sectors on the disk (DWORD) */
   if ((send_cmd(CMD9, 0) == 0) && rcvr_datablock(csd, 16)) 
   {
      if ((csd[0] >> 6) == 1) 
      {
         /* SDC version 2.00 */
         csize = csd[9] + ((WORD)csd[8] << 8) + 1;
         *(DWORD*)buff = (DWORD)csize << 10; } else { /* SDC version 1.XX or MMC*/ n = (csd[5] & 15) + ((csd[10] & 128) >> 7) + ((csd[9] & 3) << 1) + 2; csize = (csd[8] >> 6) + ((WORD)csd[7] << 2) + ((WORD)(csd[6] & 3) << 10) + 1;
         *(DWORD*)buff = (DWORD)csize << (n – 9);
      }
      res = RES_OK;
   }
break;

 

Why is this a bad smell example?

Even with the comments, it is very hard to follow what exactly is intended. There is a lot of nesting, cryptic names, “magic numbers” (numbers which probably mean something that isn’t described), and lots of bit-shifting. Regardless of what actually happens, the result it always OK.

Good smell example

Here is an example of our own code, a create thread function from our home-developed and well unit-tested embedded operating system (Satoru):


void Core::CreateThread(void *stackBuffer,
   size_t stackSize,
   void (*entryPoint) (),
   uint8_t priority)
{
   // Disable interrupts during this function
   CriticalSection section;

   SatoruAssert(mCreatedThreadCount < Config::MaxNumberOfThreads);

   size_t nextSlot = 0U;
   for (size_t i = 0; i < Config::MaxNumberOfThreads; i++)
   {
      if (threadPool[i].Status == ThreadStatus_Free)
      {
         nextSlot = i;
         break;
      }
   }

   // Good to go
   threadPool[nextSlot].Status = ThreadStatus_Active;
   threadPool[nextSlot].StackBottom = stackBuffer;
   threadPool[nextSlot].SavedStackPointer =
      Context_InitialiseStackFrames(stackBuffer, stackSize, entryPoint, threadShutdown);
   threadPool[nextSlot].EntryPoint = entryPoint;
   threadPool[nextSlot].Priority = priority;
   mCreatedThreadCount++;
}

 

Why is this a good smell example?

You might notice that there are not many comments in this example, and that is because the structure and naming conventions allow you to follow the logic of the function without much trouble. The critical section object safely handles the disallowing and re-allowing of interrupts during the creation of the thread until the function exits. Then, the next available slot is selected for the new thread, and the new thread is configured using obviously named enums and variables.

So that looks great… but is it testable?

Yes, it is! Using mocks we can create a unit test to verify that a thread is created successfully. Here is just one of the tests for it:


TEST_F(TestScheduler, First_created_thread_is_active_others_free)
{
   uint8_t mockStack[TestScheduler::MockStackSize];
   // Given
   // .. Scheduler is initialised (done in test setup)
   // .. and critical section count was reset (in test setup)

   // When
   Scheduler::Core::CreateThread(mockStack, sizeof(mockStack), ArbitraryThreadMain, LowestPriority);

   // Then... check that the first thread is Active and the subsequent ones are Free
   ThreadStatus firstThreadStatus = Scheduler::Core::GetThreadStatus(IndexOfFirstThread);
   ASSERT_EQ(ThreadStatus_Active, firstThreadStatus);

   size_t IndexOfSecondThread = IndexOfFirstThread + 1;
   for (size_t i = IndexOfSecondThread; i < Config::MaxNumberOfThreads; i++)
   {
      ThreadStatus threadStatus = Scheduler::Core::GetThreadStatus(i);
      ASSERT_EQ(ThreadStatus_Free, threadStatus);
   }

   // Then... that the thread was created inside a critical section,
   ASSERT_TRUE(AssertCriticalSectionWasEnteredAndLeftNTimes(1));
}

 

Real-life smells analogy (a litter tray)

Those of you who have cats, especially indoor cats, will know what a stink cats can make when they use a litter tray! However, if you’re staying in the house you can become accustomed with the smell and not notice it as strongly. Upon returning to the house after being outdoors, the smell can seem even more intense. This is a problem quite like coding smells, but you can fix it by improving your sense of smell!

 

Regularly breathe fresh air.

In code-terms, this means regularly looking at examples of other code. This can be as simple as looking at (or writing) code for another team, working on your own R&D projects, regularly writing fresh code, or using a new set of eyes such as apprentices.

Check the litter.

Review each others code, and not just a cursory tick in the box exercise but a thorough in-depth review to look for improvements that could be made. Don’t merge pull requests until the changes have been made, to help lift the overall quality of the project.

Have visitors.

Ideally the visitors could be external such as software coaches, but this could also be people from other teams.

We’d love to hear your thoughts on bad smells – so feel free to leave a comment below!

 

 

 

Share on FacebookShare on LinkedInTweet about this on TwitterShare on Google+

Comment