How should I react when a co-worker says his 3000 line method is optimized? Should I report it to my boss?

Question

I have a co-worker who said his 3000 line method is the most optimized possible. How do I react professionally to that?

Should I communicate this to the boss, who does not know anything about programming?

Note that we are a small team of only three programmers that are at the same level and each one has his own piece of the project that we manage and code ourselves as we want, while that piece of code do what the boss wants it to do.

• My biggest concern here is that my co-worker might end the relationship with the company at some point and I will have to take care of that piece of the project he was working on. How the hell do I read a 3000 line method?

My first thought will be to start all over again from zero and as I already did that with my current piece of project, having in mind that "the boss" doesn't understand anything about programming and he only cares that the program works and the time it takes us to make it work. I am pretty sure he will get at least a little mad.

• I had seen the method, it does a lot of things (a lot) and it has a conditionals block (big ones) meaning that if he calls the method with parameter A = 1 the first block is executed and the others ignored and so on... I have told him that he could split those blocks on different methods so it will be easy to read and understand hopping that he would see the benefits of that and would do it with the rest of the gigantic method, but I don't think he sees the benefits. He just said that he did "something" like that because every conditional block is inside a C# region.

NOTES FROM COMMENTS:

• As my co-worker said it is a critical method because it does every single calculations of a particular part of the program

• The language used to programming is C#.

• The speed of the code is not relevant here.

• Code reviews do not exist here. As I said, each one of us works on his own and so long as everything works, no one cares about the how it works.

• Assume that every single line of those 3000 lines are from actual code, not from spaces or comments.

Answer 1

If you're concerned about it you should actually read the code and offer suggestions (sounds like a great time to push for code review!). It may be 3000 lines out of need. Deciding that just because there are 3000 lines means it's wrong or bad is arbitrary.

[Edited based on updates]

You say that the code's speed is not relevant. At this point it sounds like it's just ugly code. The best course of action since you've already given them suggestions (since you're not their supervisor, etc.) would probably be to simply accept it and move on. If you ever need to work on their code it sounds like it's divided up enough that it could be easily broken into multiple functions, but as-is it works and you may never need to touch it.

Work on what your bosses want you to work on and make suggestions and improvements where you can fit them. If you try to fix everything wrong you see all at once you'll stress yourself out for no good reason.

Answer 2

I'd focus on the maintainability issue.

Depending on circumstances, a 1,000 line function that does one thing and is well-documented can be more readable and maintainable than a 20 line function that defers every decision to a ten calls deep stack of utility functions that each had special cases grafted on over time as requirements changed (oddly specific rant, I know).

The checklist for having big functions:

1. Remove the need for users of the function to understand it completely: there should be documentation that treats the function as a black box and only describes its behaviour. User code does not get to rely on anything that isn't documented in this specification, which needs to be an explicit point in code reviews for caller functions.

2. Automate verification of the function. All current use cases should exist as unit tests, so if it ever becomes necessary to modify that function, you can do so quickly with the confidence that nothing else breaks as a result.

Length of a function often correlates with how easy it is to understand, but that is not a hard and fast rule.

Answer 3

I have worked on legacy code before where the entire website is handled in a single file that is approximately 100,000 lines of code. That's right. Everything about the site is done in a single file, single function. It got to a point where adding or modifying something meant you scrolled all the way to the bottom and simply modified the output buffer to change things. Like if someone said they wanted to change a sentence, we do a regex to search the sentence and simply replaced it with the new sentence.

We eventually got to the point where it became so bloated only a few people were "experts" at modifying the output buffer. It was ultimately decided to simply toss the file, and redo the entire site with a modern approach.

I think that is what will happen here. Maintain the 3k function, and if he goes, simply toss the code. That's what I would do, rather than waste time trying to convince someone something is better. It works, is what the argument is and that might be true. Without a boss who knows code or having a good soft-skill, you probably won't get far with trying to convince your equal co-worker to change.

Answer 4

A long method is definitely a code smell, but it does not conclusively indicate that something is wrong. In fact, I would argue that you shouldn't break a method apart purely on length, that is arbitrary. For example, I've seen some long methods for distinct ETL (Extract/Transform/Load) tasks where the length is really driven by the amount of data.

Don't report it to the boss at this point. Find a tangible reason why the method can be improved, then communicate that to the developer in a constructive way.

Answer 5

Don't worry about it. It's not your problem (right now). It's this guys responsibility to maintain the code, not yours. Don't touch it. If your boss asks you to touch it, whether now or later, tell them why you won't.

If your colleague leaves, you buy a book about refactoring. That's the point where you tell your boss this function isn't maintainable.

Answer 6

How do I react professional to that?

"Great, thanks!"

Now, if you feel the code is sub-optimal, regardless of the number of lines, you can test it on the side to see if it meets any performance requirements. This gives you meaningful, actionable information.

If you think the code follows poor practices, you can bring this up in code-review.

Answer 7

You've said on one hand that each developer is responsible for their own code, and yet you wish to report one of your colleagues to your manager for not working to your standards.

If the code works as expected, there's nothing that needs to be said about it until there's a time when it's a problem or there's a time when coding standards are set for the team as a whole.

Answer 8

Ask him to write tests to prove that it works. Check the coverage of the tests. If it's less than 100%, that's a problem.

Then, at least, if you have to maintain it some day, you will be able to make changes without fear of screwing something up without realizing it.

Answer 9

You have two points here:

• In your current position, it's not your problem, you said that your boss ask this coworker to finish the code fast, so he did, leaving behind those 3000 lines with a lot of technical debt, you can mention this to your boss (he doesn't understand programming, but the concept of debt is something that everyone knows) just explain that while the code works, if something breaks or the coworker who write it leaves the company it will take time to understand what he did, he might understand but probably even if he understand that, he will say that is not a problem now, because it's not a problem, at least not yours, what bring us to the next point.

• So, the coworker leave the company and now is your problem (you mention that there are 3 developers, so even in this case could not be your problem), well, (I can talk from my experience) do not touch it unless is necessary, talk to your boss again, remember him that first talk, but don't start refactoring that code until your boss approves or the s#!t hits the fan, because if you start to refactor that code, it might be easy but there is a high probability that it will take you time that your boss expect to focus on something else.

A few things to take in consideration, even if its a small company with a boss that don't understand how software development works:

• As other answer mention, while your boss don't know about development, he know about business (I hope so) so the best way to get him understand why code reviews, unit testing and other practices are necessary is to tell him that these are investments, less bugs, less production downtime, etc.
• I don't know who is the senior in your team, if is the one with the 3000 lines, you might have a hard time convincing your boss that what he is doing is not the best approach. This applies to if you are the one with less experience.
• You might want to talk with your coworker, and try to understand why he did that, he mention that is optimized this could mean that the code works, as you says, and that is fast (you might not take speed relevant but for business sometimes it's a huge deal) but he knows that it has technical debt and he plans to fix that, he just follow your boss command to finish that fast.

Answer 10

I think the getting hit by a bus example might be the best.

I would go to your boss, and say "I have concerns about the codes maintainability, if the employee gets hit by a bus or otherwise unavailable it will take me ## weeks to figure it out. Are you ok with me being unavailable to work on other project for ## weeks if this project ever need maintenance if other person is not available.?"

Let your boss make the final decision on which is more important for this project "it s done" or "its maintainable"?

For all we know the boss could be yay, the customer will have to pay us 2x as much because if they want the modifications done they will have to pay for it.

This means that the most important thing to the boss is what will the client pay for.

Answer 11

Preventing problems is cheaper than waiting for them to happen and then solving them. Your boss likes cheap.

Ask your boss if he expects your code to be used for a long time and if changes are likely if customers pay for them.

In the likely event that you get a yes for both then suggest that you would like to have new code you write reviewed by your peers. It won't take them long and the extra time will be paid tenfold because errors are way cheaper to fix the earlier they are detected. An error found in code which is fresh in the mind is easier to fix than one found by the client with the usual less than helpful error reports from clients. Assure him that you won't be asking for much code review, maybe once each pair of weeks. If he asks if you are unsure about the quality of your code assure him that such is not the case at all. But 6 eyes have a wider view than 2 and code review is a standard industry practice because its benefits far outweigh the minimal costs.

If he goes with it, when your pals find errors in your code be sure to mention it to your boss. Mention how much more time would you have had to spend debugging the issue if the code had gone into production. Or the loss of client confidence. Mention how much this team work improves the product even if you work on different projects.

If he goes for this step he will easily make himself the next one: having everyone's code reviewed.

If he is not willing to accept someone volunteering to have his code reviewed then there is no chance you'd get him to let you review your pal's code.

Answer 12

From what you are describing, you have a mountain to climb and a team to drag up it.

I don't think I would specifically talk about the 1k line method, I would start by bringing up best practices with your boss in a 1:1. Ask him if the team has any coding guidelines or best practices that they follow. Assuming the answer is no, gather some links to some articles on best practices for whatever programming language you are using. I try to stay with coding guides from big companies... companies everyone will have heard of like google, Microsoft, etc. and start with their coding guides.

Bring those to your boss along with some articles about how implementing best practices helps... what are the benefits, etc. Don't bring your message, you are the messenger. You bring glad tidings of ways to be more efficient, save money, have fewer defects, and the list goes on...

I'm thinking your boss would react better to that approach. Then once you get him hooked (hopefully), you have a team discussion about them and let's start following them. (I would throw in some procedural best practices like code reviews and the like also, not just coding guidelines.) Then if you can get them to buy in this far, then you start applying those guidelines and reviewing new code (and old code as you run into it).

"Hey, I found this large method that according to best practice ABC, we should split up into smaller methods that have a single responsibility, etc." and go from there.

To be honest, I doubt you will get far with any of this but this is how I would approach it.

Answer 13

If the boss doesn't know anything about programming then I wouldn't bother telling him. By the sounds of it, your co-worker will do that for himself, especially since he's bragging to you.

The code needs to work, that's all your boss cares about. Of course, size and speed are important factors if it's a large project but if it's small or medium sized, working is good enough.

Ask your co-worker to either show you the code running or for the code itself so you can check it.

Answer 14

Apart from the "3000 LOC" there were 3 other phrases in the OP caught my eye:

• Should I communicate this to the boss, who does not know anything about programing?
• My biggest concern here is that my co-worker might end the relationship with the company at some point and I will have to take care of that piece of the project he was working on.
• Code Reviews do not exist here.

Somebody's leaving is a business risk, perhaps a risk that's easy for a non-programmer to understand, and a risk that's mitigated by code reviews.

You might tell your boss that programmers should (at minimum) understand each other's work -- what it does and how it's implemented -- in case one of them falls under a bus. You could add that's normal/professional.

Then ask your colleague, during a code review, "how does this work?"

You said your concern was ...

How the hell do I read a 3000 line method?

... so a code review should explain that -- i.e. how they explain, how they read it.

You might want to insist "you should use subroutines", but if the colleague is hostile to change (attached to the existing implementation) perhaps it's pointless to insist. Just be sure you understand it, so you could change it if you had to (e.g. if you inherit it).

There are other benefits to code reviews ...

• Bug detection (but you spot a bug during a code inspections)
• Knowledge transfer (you might learn from each other by reading each other's code and discussing it)
• Better integration between modules (see also Conway's law)

Answer 15

I try to use a static analyzer to do this. Feedback from a computer can't be wrong, and that way you don't have to personally confront coworkers. You just have to agree on code rules or take a basic set of existing rules.

Answer 16

I sense two major flaws here:

• do you offer a solution to the problem? Just telling "this code is horrible" won't work. It could be an opinion, it does not bring value to the company; bosses and business want solutions.

• do you have a reviewing process? Peer reviews of the code should be done regardless of the size of the team: actually providing detailed information on how the code can be refactored is better than any possbile complaint, and solves also the above problem.

I get from your edits that the answer to the second is "no", so maybe the right thing to point out to the boss is to talk about the process and not pointing out single "code failures" that he is not able to evaluate properly, unless he trusts you big time.

If you tell the boss:

Listen, I have an idea to make our work more productive: we can write better code, adapt better to change in the business, reduce the bugs and have a mantainable code base that is easier to pick up also for newcomers

maybe he would listen more carefully. You are offering solutions, not pointing out a problem.

Answer 17

What is the function trying to do? How complex is that task? What optimisation category did they aim for (speed, accuracy, usability)? Without any of that information it will be hard to make a judgement. You can always take a look at the code. If there are inefficiencies, point them out like 'The code is good! Quickie [timesaver/performance booster/whatever], you can always do X to avoid [bottleneck]'

Answer 18

You may be able to convince your coworker with technical reasons that the compiler can inline for them, so this source-level "optimization" isn't helpful.

Hopefully you're exaggerating what they said about it being the whole 3000 line method most optimized possible, or else your coworker is probably clueless about performance optimization and just latching on to something they read once. People who think they know something but don't really understand are sometimes the hardest to convince. I've had several exchanges in stack-overflow comments with people refusing to believe they were wrong, but unable to give a coherent technical explanation that made any sense.

As an asm optimization expert (SO gold badges in [x86], [assembly], [performance], [sse] tags, etc.), I can tell you it's nearly impossible that this function is "the most optimized possible", even if your coworker spent years profiling and tuning it (on some specific hardware? with some specific OS and compiler version?). A function that big will always have room for small tweaks (or new ideas for big changes) that could make it faster, or smaller (machine-code) at the same speed (maybe more hyperthreading friendly to do the same work in fewer instructions).

I don't think the C# compiler + JIT is so bad that it can't inline method calls for you, especially if they only have one call site. I don't know C# (mostly C and C++), but does it have anything like a static inline non-member function that the compiler can inline instead of emitting a stand-alone definition for, and will do so even if the function is large-ish? Or anything like GNU C __attribute__((always_inline))? Your coworker could use these to feel like they're getting the optimization they think is important without making the source a nasty mess.

---

But more importantly, "being optimized" is only worth a tradeoff in readability when the simple baseline version (that you wrote as a starting point, and to benchmark the optimized version against) is slower than you want. You can't tell if you're actually optimizing anything if you don't have a starting point to compare against, and thus judge any readability or machine-code size / instruction-cache footprint tradeoff vs. the speedup.

Writing a less-readable "optimized" version without a simple baseline is usually a mistake, unless you already think you know from experience how the simple version would compile and that it wouldn't be efficient enough. Usually you'd have a simple version as part of a unit-test for a manually unrolled / vectorized version. (This manual-inlining case is maybe different, though. It doesn't make any individual piece of logic more complex or "strangely" implemented. Or does it? Is there manual optimization between blocks?)

Inlining is very often worth it for small functions, but calling a big block of code from multiple call sites only uses the instruction-cache footprint once. It does pay the function-call overhead every time, though, so microbenchmarking just the one function, without the full context of the program, can make excessive inlining and unrolling look good. Usually we're fine leaving the decision to modern compiler heuristics; they're usually pretty well tuned, especially if they can do profile-guided optimization to find loops that are actually hot. (JIT compilers operate at runtime, so they do have profiling data if they care to use it. Usually from making a not-fully optimized version or interpreting at first, then using profiling data to speculatively inline virtual methods and stuff like that.)

Sometimes optimization doesn't hurt readability, but in this case it clearly does.

In C++, I often write little static inline helper functions that will inline into a larger function I'm optimizing the crap out of with SIMD intrinsics. When I look at the compiler-generated asm, there's exactly zero downside in machine-code efficiency, and a nice upside in readability of the source. No stand-alone definition appears anywhere in the executable for these helper functions, so they're not even bloating the executable.

---

If you want to press the issue, ask your coworker if they've looked at the JIT-compiler's asm output for their method and profiled it, and found those big conditional blocks allowed the compiler to optimize in a way it couldn't with inlining.

Being aware of what your compiler + hardware can do efficiently is not always a bad thing, if you let that inform your coding choices when it doesn't hurt readability.

It's tempting to get sucked into optimizing something that doesn't need to be optimized. Especially if you're only thinking of optimizing for speed of this one function if it was called in a hot loop, when it's not going to be in a hot loop. If it's called infrequently, code cache might be cold, so compact is better. (Fewer cache lines of code to load from main memory.)

This kind of argument only helps if you could factor out any common helper functions from this 3000 line method. Putting each block into a separate function won't make the machine-code smaller. It might make the decision logic for which function to dispatch to more localized, resulting in a smaller I-cache footprint for that, though. And maybe fewer 4k pages touched / loaded from disk / i-TLB footprint.

Answer 19

A few thoughts:

• Your manager could possibly provide conflict resolution and mediation, but he can't really weigh in on the technical merits, so he or she will needs to rely on your expertise. If it comes down to your expertise, then I think you can solve this.
• You are worried about the consequences of leaving the code alone. If the consequences are maintenance costs, what better way for your colleague to learn that lesson than by having to work hard the next time they have to change the function? Experience can be a good teacher.
• The real benefits of breaking up the cost are readability, testability, and simplicity. Without your colleague understanding those benefits, asking them to break it up will create artificial code boundaries. Showing them a better way will be helpful.

Looking at it differently: Leadership

In my opinion, with your manager not being a technical leader, going to them to solve this will minimize the opportunity for your colleague to learn and will risk creating a rift between you and him/her.

If you don't have a technical leader to escalate this to, then be that leader by helping your team mate grow into a better developer.

We can distill all that down into two possible approaches:

Leave it be

Let them do it their way and when this code becomes a problem, help them realize why it's a problem and help them find a few key code blocks they can split out. They'll learn. Seriously, it's ok. I've done this in leadership roles. It's just code. It can be changed later.

Show the way

Show them an alternative with all of the benefits realized. Either rewrite their code in a branch by decomposing it into the right layers, make the code crystal clear (good function and variable names) and write very good unit tests. Write tests which would have been impossible to write given the old code. When presenting this code to your colleague, come up with an arbitrary requirements change you want to pretend to make, walk them through implementing it so they can see how easy it is to implement it and make sure it works.