Should I follow a bad coding style just to follow the established conventions at my workplace?

Question

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.

Answer 1

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!

Answer 2

Honestly, do you believe having functions which are difficult to use, with 300 lines, is just a matter of style? So following the bad example could keep the overall program in a better state because of consistency? I guess you do not believe so, otherwise you would not have asked this question here.

My guess is these functions are so long because in our business there are lots of mediocre programmers who just pile features over features, without caring for readability, code quality, proper naming, unit tests, refactoring, and they have never learned to create proper abstractions. You have to decide for yourself if you want to follow that herd, or if you want to be a better programmer.

Addendum, due to the comments: "300 lines" and "difficult to use" are IMHO strong indications for bad code. To my experience it is very unlikely there is some "hidden technical reason" why such code cannot be implemented in a more readable fashion, comparable to the example of Cort Ammon's answer. However, I agree with Cort you should discuss this with the responsibles in the team - not to find obscure reasons why the code or this "kind of style" cannot be changed, but to find out how the code can be refactored safely without breaking things.

Answer 3

No.

In the book Pragmatic Programmer the author talks about the Broken Window Theory.

This theory state:

Consider a building with a few broken windows. If the windows are not repaired, the tendency is for vandals to break a few more windows. Eventually, they may even break into the building, and if it's unoccupied, perhaps become squatters or light fires inside.

Or consider a pavement. Some litter accumulates. Soon, more litter accumulates. Eventually, people even start leaving bags of refuse from take-out restaurants there or even break into cars.

In the book the author writes a parallel between this theory and code. Once you found a code like this you stop and question yourself that very same question.

And the answer is: No.

Once you leave this broken window - in our case, bad code - this will create the effect that everybody will leave broken windows behind.

And one day the code will colapse.

Give a copy of Clean Code and Clean Coder to everybody.

And while you are in the subject, a copy of TDD is also a good one.

Answer 4

Yes, go for it. Structure != style

You're talking about structure, not style. Style guidelines do not (usually) prescribe structure, since structure is generally chosen for its appropriateness for a specific problem, not appropriateness to an organization.

Be careful

Just be darned sure you're not causing negative consequences in other areas that may not have occurred to you. For example,

• Make sure you are not complicating diffs or code merges because you have obliterated any resemblance to existing code.
• Make sure exception flows bubble up correctly and stack traces are not polluted with a huge pile of illegible nonsense.
• Make sure you are not accidentally exposing public entry points that would cause problems if called directly (don't put them in the map and/or don't export them).
• Do not clutter up the global namespace, e.g. if your smaller functions all require some sort of global context that was previously declared in function-local scope.
• Be sure to look at any logs, and ask yourself if you were on the support team if the logs generated by your code would be confusing when seen interwoven with the logs from any code you do not touch.
• Make sure you do adhere to existing style, e.g. even if they are using old-school Hungarian notation and it makes your eyes bleed, stay consistent with the general code base. The only thing more painful than reading Hungarian notation is reading code that uses a dozen different types of notation depending on who wrote what.

Be gentle

Remember you are part of a team, and while good code is important, it is also very important that your team members are able to maintain the stuff that you wrote when you go on vacation. Try to stick to a flow that will make sense to people who are used to the old style. Just because you're the smartest guy in the room doesn't mean you should talk above everyone's heads!

Answer 5

I don't see this as a code convention. I see this as someone not understanding how to write maintainable code that is easy to understand. I would break up your code into different functions as you suggest and educate your team on the benefits of doing this while trying to understand why they feel 300-line functions are acceptable. I'd be very interested to hear their reasoning.

Answer 6

The answer is in the code review.

The real question is if you turn in well factored code are you going to be the only one willing to work with it?

I, like most people here, believe 300 line functions are an abomination. However, taking Windex to a landfill isn't going to do much good. The problem isn't the code, it's the coders.

Just being right isn't enough. You need to sell people on this style. At least on reading it. If you don't you end up like this poor guy: fired because your skills are too far above your coworkers

Start small. Ask around and find out if anyone else favors smaller functions. Better yet snoop around the code base and see if you can figure out who writes the smallest ones (you may find that the ugliest code is the oldest code and the current coders are writing better code). Talk to them about this and see if you have an ally. Ask if they know anyone else who feels the same way. Ask if they know anyone who hates small functions.

Gather together this small group and talk about making changes. Show them the kind of code you'd like to write. Be sure they can read it. Take any objections seriously. Make the changes. Get your code approved. You now have the power of a meeting behind you. A few more of these and you can produce a one page document that explicitly accepts the new style you've introduced.

Meetings are surprisingly powerful things. They can produce clout. Clout is what you will use to fight those who oppose this movement. And that's what this is at this point. A movement. You're fighting for the right to improve the status quo. People fear change. You'll need to sweet talk, cajole, and prod. But with a little luck you'll turn being right into being accepted.

Answer 7

Before making the statement that the practices are bad, I would first take a step towards understanding the reason for big methods and other practices which might come across as bad.

Then, you would need to make sure test coverage is quite high or really high (as far as you can take without major refactoring). If it is not, I would work towards making coverage really high even though you did not write the code. This helps build rapport and the new team of yours will take you more seriously.

Once you have these bases covered, no one in their right mind would challenge you. As a bonus item, do some micro-benchmarking and that will really add to your case.

Often, the way you word it goes a long way towards changing the code culture. Lead by example. Or even better, wait for a piece of code you can write - refactor and unit test the hell out of it and viola - you will be heard.

Answer 8

This type of question is basically "please mind-read my team mates". Random people on the internet cannot do that, so all the answers you will get are just peoples personal opinions about coding style. The consensus is shorter methods are preferable, but you seem to already think so yourself, so no need to restate that.

So, the current code base seem to use a different coding style that the one you prefer. What should you do? First you need to figure out:

1. Is this a deliberate design decision supported by your current team?
2. Do they want you to follow this style?

There is only one way to find out. Ask.

There could be multiple reasons for having long functions. It could be because the team believes long functions are better than multiple short functions. It could also be because it is legacy code, and team prefers that new code follows a cleaner design. So either choice could land you in trouble as a developer, if you don't talk to the team and understand their reasoning.

Answer 9

Sometimes you have to go with the flow.

Like you said you have been tasked to implement a function in an existing working codebase.

Regardless of the mess, and I understand it is tempting to clean it up. but in the business world you don't always have a chance to refactor code.

I would say just do what you have been asked to do and move on.

If you feel it is worth refactoring or rewriting than you need to bring it up for discussion with the team.

Answer 10

There are different levels of "style".

On one level, usually part of coding conventions, this means where to put white spaces, blank lines, brackets, and how to name things (mixed case, underscores, etc..).

On another level is the programming model, sometimes things like avoiding statics, or using interfaces over abstract classes, how to exposing dependencies, maybe even avoiding some esoteric language features.

Then there's the libraries and frameworks in use by the project.

Sometimes there will be a maximum limit on function size. But rarely is there a minimum limit! So, you should find out which of these things the powers that be consider important, and try to respect that.

You need to find out if the team is adverse to refactoring the existing code, which should be done independently of adding new code when possible.

However, even if they don't want to refactor the existing code, you may be able to introduce some layers in abstractions for the you new code you add, meaning over time you can develop a better code base and maybe migrate the old code as maintenance of it calls for.