Coding Style – Should I Follow a Bad Coding Style Just to Follow the Established Conventions at My Workplace?

coding-styleconventions

I've been working at my job for about a year. I primarily do work in our GUI interface which uses methods from a C backend, but I generally don't have to deal with them except for return values. Our GUI is structured pretty reasonably, given our limitations.

I've been tasked with adding a function to the command line portion of the program. Most of these functions are like 300 lines long and difficult to use. I'm trying to gather pieces of them to get at specific alarm information, and I'm having trouble keeping organized. I know that I'm making my testing more complicated by doing it in a single long function.

Should I just keep everything in a huge function as per the style of the existing functions, or should I encapsulate the alarms in their own functions?

I'm not sure if its appropriate to go against the current coding conventions or whether I should just bite the bullet and make the code a little more confusing for myself to write.

In summary, I'm comparing

showAlarms(){
    // tons of code
}

against

showAlarms(){
   alarm1();
   alarm2();
   return;
}

alarm1(){
    ...
    printf(...);
    return;
}

EDIT: Thanks for the advice everyone, I decided that I'm going to design my code factored, and then ask what they want, and if they want it all in one I can just cut from my factored code and turn it back into 1 big function. This should allow me to write it and test it more easily even if they want it all the code in a single definition.

UPDATE: They ended up being happy with the factored code and more than one person has thanked me for setting this precedent.

Best Answer

This is really between you and your team mates. Nobody else can tell you the right answer. However, if I may dare read between the lines, the fact that you call this style "bad" gives some information that suggests it's better to take it slow. Very few coding styles are actually "bad." There are ones I would not use, myself, but they always have a rhyme or reason to them. This suggests, to me, that there's more to the story than you have seen so far. Asking around would be a very wise call. Someone may know something you don't.

I ran into this, personally, on my first foray into real-time mission-critical coding. I saw code like this:

lockMutex(&mutex);
int rval;
if (...)
{
    ...
    rval = foo();
}
else
{
    ...
    rval = bar();
}
unlockMutex(&mutex);
return rval;

Being the bright and shiny OO C++ developer I was, I immediately called them out on the bug risks they had by manually locking and unlocking mutexes, rather than using RAII. I insisted that this was better:

MutexLocker lock(mutex);
if (...)
{
    ...
    return foo();
}
else
{
    ...
    return bar();
}

Much simpler and it's safer, right?! Why require developers to remember to unlock their mutexes on all control flow path when the compiler can do it for you!

Well, what I found out later was that there was a procedural reason for this. We had to confirm that, yes indeed, the software worked correctly, and there was a finite list of tools we were permitted to use. My approach may have been better in a different environment, but in the environment I was working in, my approach would easily multiply the amount of work involved in verifying the algorithm ten fold because I just brought a C++ concept of RAII into a section of code that was being held to standards that were really more amenable to C-style thinking.

So what looked like bad, downright dangerous, coding style to me was actually well thought out and my "good" solution was actually the dangerous one that was going to cause problems down the road.

So ask around. There's surely a senior developer who can work with you to understand why they do it this way. Or, there's a senior developer who can help you understand the costs and benefits of a refactor in this part of the code. Either way, ask around!

Related Topic