101

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.

Justin
  • 913
  • 2
  • 6
  • 8
  • 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
  • @Servy thanks, I figure I should do that, but I wanted to see what some other opinions were before I starting bringing it up. I think the reason I hesitate is because I dont want to seem like I think I know more than the senior developers(I definitely don't) and so I'm hesitant to criticize the way the system is built. – Justin Dec 12 '16 at 22:26
  • 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
  • 3
    Possible duplicate of [My boss asks me to stop writing small functions and do everything in the same loop](http://softwareengineering.stackexchange.com/questions/335783/my-boss-asks-me-to-stop-writing-small-functions-and-do-everything-in-the-same-lo) – gnat Dec 13 '16 at 00:47
  • 3
    300 line function is par for the course in the industry. If the software is selling, and paying the companies bills, is the standard really all that bad? Everything has an opportunity cost. – whatsisname Dec 13 '16 at 03:54
  • 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 04:46
  • 3
    It's important to differentiate between code conventions and code. Just because a lot of code is structured badly, does not mean it's the actual company convention. – Allan S. Hansen Dec 13 '16 at 07:46
  • 5
    @quickly_now: universities seldom teach practical programming. The practice of writing smaller functions comes from experienced programmers, not from academics. And, a 300 line function is typically much more obscure than 20 functions with ~15 lines, assumed the names of that 20 functions are chosen well. – Doc Brown Dec 13 '16 at 07:47
  • 1
    Well, your example "solution" is just as bad as the current "standard", if not worse. The only thing you do is move code into blocks and give each block a name, making the developer scroll more. It does not have any benefit. – Stephan Bijzitter Dec 13 '16 at 08:30
  • 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 09:43
  • @BCdotWEB You don't use 'grather' in your shop? Wow, Ive though every one new that word. –  Dec 13 '16 at 14:54
  • Be the change you wish to see in the world ~ Ghandi – Bradley Thomas Dec 13 '16 at 15:22
  • 1
    @nocomprende: First time I've ever heard that word. It doesn't even Google. – Robert Harvey Dec 13 '16 at 20:04
  • 2
    @RobertHarvey that's why it is so useful. An anti-word. Cromulent. Utterly meaningless. Gets you out of a whole lot of trouble. But, I grather it starts showing up, and then it will no longer work. –  Dec 13 '16 at 20:34
  • 4
    There's nothing wrong with a 300 line function if each of those lines is simple. – Miles Rout Dec 13 '16 at 22:05
  • 2
    Instead of "less than `N` lines", a better benchmark for your function may be "does `N` task(s)", where `N` is usually `1` (unless there's a very good reason for it to do multiple tasks). Then, analyse each function's assigned task(s), breaking it (them) down into sub-tasks and determine which belong in their own functions. If done well, the result is that each function is as long as it needs to be to do its assigned task(s), yet (usually) easy to understand; each function's length will also reflect the complexity of its assigned task(s), which is a useful quality. – Justin Time - Reinstate Monica Dec 13 '16 at 22:39
  • 1
    For example, while breakdancing, taking a step, balancing the budget, and adding two numbers can each be seen as a single task, so to speak, I'd expect `breakdance()` and `balance_budget()` to be a lot more complex than `take_a_step()` and `add()`, respectively. – Justin Time - Reinstate Monica Dec 13 '16 at 22:39
  • 1
    It sounds like you're of the same mind, and hopefully your coworkers will be, as well. – Justin Time - Reinstate Monica Dec 13 '16 at 22:41
  • 1
    That said, sometimes longer functions make more sense than shorter ones. For example, for an interrupt handler, that handles `N` distinct interrupts, I would consider the perfect length to be exactly `N + 4` or `N + 5` lines, depending on coding style: 1 line for the function's signature, 1 line for a `switch (get_interrupt_code())`, (optional) 1 line for opening brace, 2 lines for closing braces, and 1 line for each interrupt (which consists of the `case` statement, followed by either `// Falls through.` or `function_name(); break;` All `break`s are lined up, as are all function calls. – Justin Time - Reinstate Monica Dec 13 '16 at 23:08
  • Easy to maintain, easy to read, and easy to spot errors, regardless of how long it is. (And assuming that more than a handful of interrupt codes can be generated, it's likely that it _will_ be a bit on the long side.) – Justin Time - Reinstate Monica Dec 13 '16 at 23:10
  • 4
    OTOH, maybe you should be happy they're "only" 300 lines and not 3000. – shoover Dec 14 '16 at 00:07
  • http://thecodelesscode.com/case/123 – Shadur Dec 15 '16 at 12:06
  • For that specific code, you should really only split into alarm1() alarm2() if there is code reuse. If that is the only place this code is used, there is no point in splitting it. Add comments to help reading along the 300 lines instead. Splitting everything into 10 lines functions that are never reused doesn't help the situation at all. Now everyone has to browse through hundreds of useless badly named functions, instead of one person having to sort out 300 lines of code. TLDR Long functions are not a problem if they are documented properly and do not repeat code. – Drunken Code Monkey Dec 16 '16 at 04:40

10 Answers10

101

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!

Deduplicator
  • 8,591
  • 5
  • 31
  • 50
Cort Ammon
  • 10,840
  • 3
  • 23
  • 32
  • 61
    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 06: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
  • @CortAmmon: yes, but your answer is giving an example why code can look bad or strange because of hidden, technical reasons. Though this is surely true for lots of code, I have my doubts if that argument should be transferred to the typical case of a function which has become too large because noone cared for the code quality over years. – Doc Brown Dec 14 '16 at 15:03
  • 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
  • Sure, seniors must be on board. But the goal should not be to find reasons why bad code should stay as it is, but ways to improve it. – Doc Brown Dec 14 '16 at 15:09
  • 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
  • @CortAmmon: I think a "hard to use" function with >300 lines already proved itself to be bad, this is often evidence enough. Exploration in the direction "is this really bad", as you suggested it, or "can we find an obscure reason not to change this" is IMHO wasted time. Exploration should instead go into the direction "how can we safely improve this, without breaking anything". And that is where the team must be on board. – Doc Brown Dec 15 '16 at 06:48
83

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.

Doc Brown
  • 199,015
  • 33
  • 367
  • 565
  • 128
    Even competent programmers commit these crimes when under tight deadlines. – gardenhead Dec 13 '16 at 03: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 05: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 07: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 09: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 09:41
  • 4
    @Dangph yeah, I am just speculating about how those previous functions got to the current state. – Džuris Dec 13 '16 at 09:43
  • 1
    @Juris, yes, it's often interesting to look at the first revision of the code in source control. – Dangph Dec 13 '16 at 09:47
  • 2
    Unit testing addresses uncertainty associated with a refactor. My confidence in refactoring my code went way up when i had a decent suite of tests to run before commiting. – Gusdor Dec 13 '16 at 10:19
  • 1
    I have seen functions of well over 1000 lines where the length is quite deliberate: the comment is usually "we leave everything in one function to make it faster" [sic]. – Calchas Dec 13 '16 at 15:44
  • 1
    @Calchas I saw a function once which was meant to find a substring in a string. It was commented "we're dealing with really long strings here, so log our progress". It called a logging callback which was file bound every 400 characters when the string to search in was a million or a few million characters long. Took the function from less than a second to at least a few minutes. Sigh. While I'm here, deadlines have been used as an excuse at my workplace and people are very willing to add completely new logical ideas to old functions. – Millie Smith Dec 15 '16 at 04:35
  • I loved the surgeon thing maybe you should add that quote in your post :) – Walfrat Dec 15 '16 at 12:32
  • 2
    Luckily, surgeons aren't forced to perform a 8-hour heart surgery in one hour. It's easy for them to talk a patient's family into allowing their relative to lie on that table for 8 hours, they'll agree no matter what because they recognize that the result far outweighs the time frame it's achieved in. I welcome the day when this becomes commonplace in our line of work but until that happens, well... I'm going with @gardenhead on this one. – Wim Ombelets Dec 16 '16 at 12:30
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.

badp
  • 1,870
  • 1
  • 16
  • 21
linuxunil
  • 1,441
  • 10
  • 17
  • 6
    What if the codebase is a greenhouse with nothing but broken windows? – Alan Shutko Dec 13 '16 at 02:56
  • 8
    @AlanShutko Then make sure your resume is current? Find a different employer *now*. – David Montgomery Dec 13 '16 at 03: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 08:28
  • "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." I live in a town like this, I would not like to work with code like this. – Pharap Dec 13 '16 at 09:37
  • 2
    @DavidMontgomery not everyone has the liberty to change employers because they don't like the style,but you can always lead by example, leave the code better than you found it, like a good boy scout. – CodeMonkey Dec 13 '16 at 10:54
  • 9
    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
  • But TDD is dead, you know @Walfrat :D – Filip Bartuzi Dec 13 '16 at 14:35
  • 1
    I think giving a book (or two!) on coding style to your coworkers would be rightly seen as an insult. – Alex Reinking Dec 14 '16 at 00:12
  • 1
    @AlexReinking 'The words you say mean nothing. The way you say them mean everything.' - Eileen Parra. You can throw the book on the head of a coworker... or you can approach them in a more friendly way. Don't you remember that time when a friend came to you talking about a good book and you got very entrusted about it? – linuxunil Dec 14 '16 at 11:29
  • I have to fundamentally disagree with Hunt and Thomas here unless their quote has been taken out of context... Their position seems to be that businesses should address technical debt for the sake of it, which is not pragmatic in the slightest. Address technical debt and improve code as you encounter it - or the workload can stymie even the biggest projects. – James Snell Dec 15 '16 at 01:42
24

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!

John Wu
  • 26,032
  • 10
  • 63
  • 84
  • " 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 08: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 08:50
  • 2
    Good advice. When I refactored *a single command* into 5 functions in [this answer,](http://stackoverflow.com/questions/33239625/optimize-ternary-operator/33241669#33241669) 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 - Reinstate Monica Dec 13 '16 at 15:53
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.

Bernard
  • 8,859
  • 31
  • 40
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.

Community
  • 1
candied_orange
  • 102,279
  • 24
  • 197
  • 315
  • 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 03: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 03: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 08: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
  • Some people want to learn and will take opportunities that come along, others do not and won't. You can't teach a programmer to sing. –  Dec 13 '16 at 14:58
  • @nocomprende that's certainly true. But I'd rather work with those who try to work with the team. Your audience isn't your textbook author. It's your team. If you can't work with them consider finding another job with people you can respect. – candied_orange Dec 13 '16 at 17:06
  • I mean, I think there are times where a function can't be broken cleanly or it needs to be really efficient and 300 lines is fine, but a 300 line function that just prints things is silly to me so I felt like I should ask. – Justin Dec 13 '16 at 17:19
  • @beginnerBinx I doubt there is ever a good reason for a 300 line function. The existence of a single 300 line function is reason for alarm. The existence of many is reason for staging an intervention. I would think of that intervention as an attempt to save the company and your job. Because if that intervention failed and I was told I had to write 300 functions I'd start updating my resume. – candied_orange Dec 13 '16 at 17:32
  • 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 02:29
  • @Brandon consistency is a small element of the bigger point. Code should be readable, by your team. Consistency only plays a part in that. Teaching a new style should certainly involve looking at actual working code. It just shouldn't ONLY involve looking at actual working code. Talking to human beings is part of your job as a professional developer. – candied_orange Dec 14 '16 at 02:57
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.

skipy
  • 139
  • 2
  • 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.

JacquesB
  • 57,310
  • 21
  • 127
  • 176
  • 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
2

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.

meda
  • 210
  • 1
  • 6
  • 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 03:34
  • 5
    OP has been asked to write a function not modifying hundreds of lines of code – meda Dec 13 '16 at 04: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. –  Dec 13 '16 at 15:01
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.

Erik Eidt
  • 33,282
  • 5
  • 57
  • 91