102

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.

  • 117
    It's highly doubtful that the company you work for takes the position that your functions should be 300 lines long because "that's the way we've always done it." That's not "coding style conventions;" it's just bad style. – Robert Harvey Dec 12 '16 at 22:17
  • 64
    This is the kind of thing you should be discussing with other members of your team, to figure out what practices you see are conventions that they consider important for everyone to follow, and what are things that someone just did once and that you don't need to emulate. There will no doubt be some amount of both. – Servy Dec 12 '16 at 22:19
  • 17
    @beginnerBinx That just comes down to how you phrase it. Don't phrase it as, "X is doing something really stupid, do I also have to do that stupid thing." but rather as something like, "I notice that X is doing this one thing, is there a particular reason it does that the way it does; would it be a problem if I didn't do the same thing when writing Y?" It then becomes their decision of whether to bash the old code, or explain why they had to do it that way (whether it's grudgingly or enthusiastically). – Servy Dec 12 '16 at 22:29
  • 11
    Good or bad style is frequently argued about and infrequently agreed upon. University teaching is that functions should be short but if this obscures the meaning, don't do it. The test should be clarity, rather than unthinkingly following a set of rules. – quickly_now Dec 13 '16 at 4:46
  • 9
    Random people on the internet cannot mind read your team mates, so all answers you get here is just peoples personal preferences. You should talk to your team. Is the long function a deliberate design principle? Would they like you to continue the style of long function or would they want you to write what you consider the cleanest? – JacquesB Dec 13 '16 at 9:43

10 Answers 10

102

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!

  • 60
    Arguably the more dangerous part of the mutex example is the apparent lack of code comments explaining why it couldn't use RAII. Granted, if this explicit lock/unlock is all over the codebase, it could be unwieldy to add a comment for each instance. In that case maybe a primer document should be written that new devs are required to be familiar with. – Kelvin Dec 13 '16 at 21:15
  • 10
    Sure, talking to a senior is always good, and refactoring must be done with care (and with testing). And of course, concurrency/mutexes are a special beast on their own. But when seeing functions with >300 lines, I would not invest too much time in looking for a "hidden technical reason" why these functions are too big. The reason why functions become too lengthy are always the same, and they are not technical. – Doc Brown Dec 14 '16 at 6:41
  • 8
    @DocBrown Just because they are not technical does not mean they are not reasons. It is important for developers to remember that they are one part of a larger machine. (I can't count the number of times I have had to relearn that lesson) – Cort Ammon Dec 14 '16 at 14:50
  • 4
    @DocBrown Either way, it's the senior developers that one should be talking to. I chose the example that I did because its easy to find countless arguments for why stupid code is stupid. Finding the arguments for why code that looks stupid is actually not stupid is harder. – Cort Ammon Dec 14 '16 at 15:08
  • 3
    @DocBrown From your comments and answers, I think I can tell that the difference of our opinions is that you are starting from the assumption that the code is already "bad," based on what evidence you have been given, while I would prefer to take paths which explore whether or not the code is "bad" first. – Cort Ammon Dec 14 '16 at 15:12
84

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.

  • 127
    Even competent programmers commit these crimes when under tight deadlines. – gardenhead Dec 13 '16 at 3:54
  • 13
    @gardenhead, I can understand not refactoring a 300-line function to save time, but there is no possible way that writing a 300-line function will save you time. – Dangph Dec 13 '16 at 5:15
  • 39
    @gardenhead: when I go to a surgeon because I need urgent help, I still expect him to take the time to wash his hands before he starts to perform on me. This is something lots of people have to learn in our business to achieve real competence: always leave code in a better state behind than it was before, and that this will also improve the ability to hold deadlines. 300 lines functions typically grow day by day, by people who do not care, and they will always use "the deadline" as an excuse. – Doc Brown Dec 13 '16 at 7:39
  • 17
    @Dangph it saves time when you only add 5 lines to the function instead of refactoring it. Those usually grows when the initial function is long (30+ lines) and the next programmer thinks "this is not my code, I will not refactor not to break it, just add a few lines here and there". – Džuris Dec 13 '16 at 9:34
  • 7
    @Juris, as I understand it, the question is about creating a new function, not refactoring existing ones. If you are writing a new function, you will not save time by making it into a single monster function. – Dangph Dec 13 '16 at 9:41
42

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.

  • 6
    What if the codebase is a greenhouse with nothing but broken windows? – Alan Shutko Dec 13 '16 at 2:56
  • 8
    @AlanShutko Then make sure your resume is current? Find a different employer now. – David Montgomery Dec 13 '16 at 3:31
  • 12
    Code seldom "collapses". It is just getting harder and harder to add new features, takes more and more time, and the estimates for cleaning up the code will increase - which means it gets even harder to get any time budget for the necessary clean up. – Doc Brown Dec 13 '16 at 8:28
  • 8
    Wow, what an arbitrary analogy the "broken window" theory seems to be. – Samthere Dec 13 '16 at 12:15
  • 4
    TDD have nothing to do with that. Whether you're going go Business/Domain/Test/Nothingness driver design that don't change anything about follow the basics of clean code. Sorry but it's really tiring to see 'TDD' quoted everywhere where it not even in the subject – Walfrat Dec 13 '16 at 12:24
25

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!

  • " Try to stick to a flow that will make sense to people who are used to the old style." - so, is your advice to program in the same manner as whatever incompetent clown did before? Adding 300 lines function is not going to make it easy. – BЈовић Dec 13 '16 at 8:47
  • 11
    I did not say to stick to a flow that is similar. I said stick to a flow that they will understand. – John Wu Dec 13 '16 at 8:50
  • 2
    Good advice. When I refactored a single command into 5 functions in this answer, I subtly changed the semantics by pre-computing values inside a logical expression which originally were only computed conditionally. The reviewers caught it though ;-). The serious advice is to review and test thoroughly. Each change can introduce errors, and that may be the main reason nobody has made them. – Peter A. Schneider Dec 13 '16 at 15:53
6

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.

  • 1
    "There's no reason, it's just our policy." – user251748 Dec 13 '16 at 15:05
5

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.

  • 5
    If it takes that much socializing and multiple meetings to convince developers that a 300 line method is too long, then I don't think talk is going to help. The problem with the above approach is that if you ask for permission to write good code, you are on the defensive. But if you just write good code and take the time to refactor existing code, then anybody who objects is on the defensive to explain why a 300 line method is good and to justify spending the time putting it back. – Brandon Dec 13 '16 at 3:27
  • 3
    I can tell you that if I was invited to a series of meetings to discuss lines of code per function limits, I would find a way to escape such meetings. There is no correct number and you'll never get people to agree. Sometimes, people need to be shown a better way. – Brandon Dec 13 '16 at 3:31
  • Sometimes, splitting up a huge function (and naming things better and adding comments and documentation as I figure it out is the necessary first step before savely mqking a change. – JDługosz Dec 13 '16 at 8:41
  • @Brandon You probably don't mean it to sound this way but what I'm hearing is that you're right and everyone else can just deal with it. As long as your code works it doesn't matter if the rest of the team doesn't understand it. It's certainly not worth your time bothering to teach them anything. You're perfectly happy to watch them continue to create 300 line functions while you write code your own way. – candied_orange Dec 13 '16 at 13:46
  • 1
    @CandiedOrange I certainly did not mean going against the team. What I meant was that some topics are best learned by seeing them in action, but trying to get a team to make a democratic decision on style is not going to be productive. I have seen consistency used as an excuse for writing unreadable code. Result? More unreadable code. I could set up a series of meetings to talk about it, or I could just fix it, and then show people the results. – Brandon Dec 14 '16 at 2:29
3

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.

  • 2
    If you ask for permission on every little refactoring, you'll forever have unmaintainable code, which is dangerous for a business. Managers only look at the cost of a change, but rarely look at the cost of not changing. – Brandon Dec 13 '16 at 3:34
  • 5
    OP has been asked to write a function not modifying hundreds of lines of code – meda Dec 13 '16 at 4:46
  • 2
    @meda exactly! I'm in very much the same situation. I'm developing new modules for the company intranet and I'm finding what may be described as "years of neglect beyond repair", with server software and libraries not being updated at all since 2006. I'm not supposed to clean this mess, but It takes me longer to code because of the limitations. (Imagine MySQl 5.2, MySQL 5.0). I cannot update anything because probably the existing code will not run on newer versions due to deprecated code. – roetnig Dec 13 '16 at 14:28
  • 3
    "If it ain't broke, don't fix it." I have a 20 year old car that leaks a little oil. Worth spending money on? No. Reliable? Yes. – user251748 Dec 13 '16 at 15:01
3

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.

  • Im not refactoring the current code, I'm just writing a new function, not sure if I should just follow the 'convention' and code it in a 300+ line monstrosity or break it up into logical chunks like I would normally do but has not been done in this part of the code base. – Justin Dec 13 '16 at 17:44
  • 1
    do they have rules to ensure you write new methods in 300+ lines? if not, i would write it the right way. but i would absolutely go out of my way to make sure I test them including the branches. I have been through similar experiences and usually you do win them over. trick is to do it over time and build rapport. – skipy Dec 15 '16 at 10:38
3

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.

  • 1
    I thoroughly agree and add, "Might you get fired or otherwise in trouble for not following what others have done?" Then the answer is yes! Following every bad thing the company requires you to do and deal with it. – Rob Dec 13 '16 at 10:54
1

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.

protected by maple_shaft Dec 13 '16 at 17:42

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.