62

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.

  • 141
    Optimised and "follows best practices" are different things, semantically he might be correct. – Jay Gould Feb 8 at 15:21
  • 47
    How specifically can you demonstrate that it's not optimized? – Johns-305 Feb 8 at 15:22
  • 33
    This question could be rephrased as: “what do I do when a colleague has completed a task in a manner that our boss considers acceptable, but I wouldn’t if I was the boss. If I am tasked with a related problem I believe I will have to redo the work before starting my own”. – jmoreno Feb 9 at 19:34
  • 21
    How is this tagged pair-programming? Did you program those 3000 lines together with him? Maybe you're not pair-programming but doing a code review? But then you say code review does not exist. I'm confused – Thomas Weller Feb 10 at 15:36
  • 8
    If speed is irrelevant, what metric is it optimized for? – Stig Hemmer Feb 11 at 8:52

20 Answers 20

116

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.

  • Comments are not for extended discussion; this conversation has been moved to chat. – Jane S Feb 10 at 0:24
  • 14
    I agree that there are cases where old code, even if ugly or not following good practices, doesn't need to be refactored if the odds of needing to make a change to it again are very low, or if there are other priorities on the list. However, there is still the consideration that this employee will continue to write new code in the same monolithic style, and that is a problem that needs to be addressed and not just ignored. – Flater Feb 11 at 0:56
  • 4
    I agree with the answer, except that it's forgetting about the responsibility as a professional domain specialist. If OP was hired as a plumber and there was a rusty pipe, they should inform their boss. As a programmer, I feel it's OP's duty as a profressional to inform the boss about this. "It works now, but as soon as the coworker gets hit by a bus, it will take extremely long for anybody else to make changes in that part." Then it's up to the boss to set the priorities. – R. Schmitz Feb 11 at 10:27
  • 2
    A 3000 line function is never necessary [code lines; there may be legit cases of 3000 lines of data]. That realization is not arbitrary; to the contrary, it is a fundamental and universal truth with modern compilers. That is even more so in languages like Java or C# which do not have C/C++'s translation unit boundary and therefore optimize easily across function calls in different files. – Peter A. Schneider Feb 11 at 14:49
  • 1
    @everyone It may be the first step in creating a process, but if OP doesn't have a concrete process (or the boss's buy in that one should be developed), the coworker could just shrug and move on without changing anything. – zarose Feb 11 at 15:04
65

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.

  • 62
    I really disagree that any 1,000 line function is going to be clearer and more readable than the equivalent broken up into smaller functions. I find it really, really unlikely that a 1,000 function doesn't have duplicated blocks of code that could be pulled out into functions. I really don't want to read 1,000 lines to figure out what a function does. – DaveG Feb 8 at 17:46
  • 55
    There's no way a 1k line function does one thing. – ajacian81 Feb 8 at 20:48
  • 48
    @DaveG Sounds like you haven't yet encountered the 10-levels-deep indirection spread across 50 files alternative. Sometimes a readable, well-named 1000-line function can evoke a sigh of relief that it didn't go heavy on indirection. This is especially true when it's implementing a complex, multi-step workflow. (That's not to say that careful abstraction wouldn't have been a somewhat better alternative.) – Roy Tinker Feb 8 at 22:16
  • 27
    I think that's something of a false dichotomy. Refactoring a 1000+ line function does not necessitate vast levels of indirection. Also, there really is no way a 1000+ line function could not be sensibly broken up into at least a handful of smaller functions that encapsulate coherent sections of the larger. – Dancrumb Feb 8 at 22:45
  • 34
    @Dancrumb It entirely depends on the function. If it's spaghetti code, sure. But if it's a state machine, then please do make it one 1000-line function, especially if you've got diagrams for it too. We can all read that, and maintain it. My horror is the CS design-pattern "expert" fresh from uni who wants to split that into a dozen interconnected files and functors and god knows what, just because hey, design patterns. No. No, no, no. No. Nope. Hell no. – Graham Feb 8 at 22:56
40

I deleted my previous answer because it was irrelevant after the latest edits.

Let's see what we have:

  1. Your boss is not programming-literate.
  2. Your colleague might leave soon and you'll inherit the project.
  3. Your boss wants results yesterday.
  4. Your new project is going to be a big, steaming pile of unmentionables.

Cool. Let's start with the boss. He doesn't understand programming, but he understands time and money. He also wants things Cheap, or you'd have more resources, and Fast. You must explain to him what he's getting himself into:

enter image description here

You've already said he doesn't care about quality - which is fine. It's his decision to make. What he doesn't understand is that bad quality prohibits fast work. Eventually you hit a brick wall, the technical debt catches up with you, you just can't write any more code that doesn't break existing stuff. This is where burnout and updated CVs are found. He must understand that software development is a marathon, not a sprint race.

At this point you can mention that feature you rolled out last month, which was rolled out pretty quickly but you're still fixing bugs about it.

  • 18
    @angelhumberto You don't have to make the other guy look bad explicitly. You can just say that, in its current state, the project will be very difficult to extend in the future, and this is a direct result of the push to do it fast. Focus on the problem, not the person. – rath Feb 8 at 18:27
  • 5
    @Fattie A person who passes judgement of someone's abilities solely on the basis of second-hand incomplete description of their work, made from a self-evidently biased point of view, is also a fool IMO. – alephzero Feb 8 at 23:35
  • 1
    This is a good answer thanks for the time I will take your suggestion in consideration – angel humberto Feb 8 at 23:59
  • 1
    Your text ("bad quality prohibits fast work") appears to contradict your diagram ("if you give up good quality, you can have it fast and cheap"). – Daniel Wagner Feb 9 at 3:58
  • 2
    Honestly, I don't think this triangle works for software development. You can't throw more money at a software project and make it go faster because there aren't necessarily resources you can just buy. Adding people has significant costs (especially on brownfield projects), and technology that reduces the development time (by providing more functionality out of the box) doesn't necessarily exist or often doesn't correctly handle edge cases that are still time consuming to implement. So the good/fast combination is as much a unicorn as the center, and the good/cheap combination is, too. – jpmc26 Feb 9 at 4:32
25

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.

  • This is all true and this is a great answer. However you CAN explain it to the boss simply, in a few words. I have stated how in my answer. – Fattie Feb 8 at 19:49
  • 2
    Yeah true. I mean it comes down to soft skills really in explaining money expenses to a non-programmer. Not to sound offensive, but based on the problem OP described, I don't think he has it. It's common in the developer world though that many developers lack soft skills needed to explain things from a programmer to non programmer and back to a programmer. Myself included. – Dan Feb 8 at 20:07
  • 3
    It sounds by his explanation that the function is organized into large conditional blocks. So I imagine it would be quite possible to refactor that 3000 line method. – Džuris Feb 8 at 21:15
  • While your suggestion might work I am not okey working like that. The fact that I have to spend a day just looking that función to fix a small problem or to add another bug called "feature" would make my eyes bleed. Thanks for the answer – angel humberto Feb 8 at 23:51
  • 1
    (aside) "adding or modifying something meant you scrolled all the way to the bottom and simply modified the output buffer to change things." - Oftentimes this is how biological evolution works (on the 'functional' level). That code evolved itself, instead of 'being engineered' :) – Zeus Feb 10 at 8:27
16

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.

  • 2
    In this case i can bet that the code can be split in simpler function that would be easy to read and to modified – angel humberto Feb 8 at 23:53
  • 8
    @angelhumberto - Your boss doesn't care what you're willing to bet on. If you want to convince him, you'll need to demonstrate a specific problem (and "the function is too long" is not a specific problem) and be able to explain how your proposed changes will solve that problem. – Dave Sherohman Feb 9 at 11:13
  • @DaveSherohman 'I don't understand this function because it's too long' is a specific problem. Unfortunately, it's the same problem I had reading James Joyce and not phrased as a problem with the code. – Pete Kirkham Feb 11 at 11:00
14

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.

  • 3
    Not sure about this, you are basically knowing when and how you are going to die and no doing anything to prevent it – angel humberto Feb 8 at 23:46
  • Definitely disagree with that. Fixing the problem now could cost a week of work. Compared to 1 year from now where the code has changed and things have been added to it, the cost to refactor (combined with the cost to try and remember what the code did) could be a lot more. – Nicolas Feb 11 at 20:06
11

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.

  • Code Reviews does not exist – angel humberto Feb 8 at 16:18
  • 4
    @angelhumberto Then you now have a reason start them. – Johns-305 Feb 8 at 16:32
  • I don't think it will work as I said we are a small team of only 3 developer who work on their on projects. What I want to know is what to do knowing that the co-worker y doing things like that – angel humberto Feb 8 at 17:31
  • 8
    @angelhumberto Right, but please read the Answers. One common theme is that 3000 lines is not in itself a problem. You have to identify an actual problem before doing anything. – Johns-305 Feb 8 at 17:35
  • The question is done in the descrip. I know the problem what should I do about it? Should I tell my boss? – angel humberto Feb 8 at 17:49
10

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.

  • Please read my first edit to understand why I am concerned about if I should or shouldn't report it – angel humberto Feb 8 at 16:05
  • @angelhumberto: and if you leave the company before your co-worker? YAGNI is a business expression, and means don’t do work until you need to. If and when you have to take it over is soon enough to take it over. Until then, you don’t need to do anything about it. – jmoreno Feb 9 at 19:00
10

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.

  • Even this can be a smokescreen, as bad tests can touch code they don't evaluate. – Weckar E. Feb 11 at 7:51
8

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.
  • I like you answer. Thanks for the reply – angel humberto Feb 8 at 23:35
7

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.

  • 3
    If my boss didn't understand programming, I doubt I would tell them that essentially "My coworker gets his job done just fine, and he could probably take over my project relatively easily. However, I would struggle a lot if I had to do his job." – that other guy Feb 8 at 23:03
  • I like the part where you said "Let your boss make the final decision on which is more important for this project "it s done" or "its maintainable"?". Overall good answer thanks for the time – angel humberto Feb 8 at 23:44
6

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.

  • Nice answered thanks for the time I will take in consideration when taking the final decision – angel humberto Feb 8 at 23:56
4

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.

  • I like your answer and I do agree with you when you said I had a mountain to climb but it is a mountain with 90°, no equipment and if that wasn't enough I would said the mountain is frozen and ward by gigantic dragon's xD – angel humberto Feb 8 at 23:41
  • Thank for taking the time to answer – angel humberto Feb 8 at 23:42
3

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.

  • Please read my first edit in order to understand why I am concerned of the 3000 lines method – angel humberto Feb 8 at 15:42
3

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)
2

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.

  • 11
    Feedback from a computer can't be wrong Uhhh... sure it can. – EJoshuaS Feb 8 at 21:11
  • 1
    Only pales in comparison to feedback from the Internet ;) – ypercubeᵀᴹ Feb 9 at 16:54
1

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.

0

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]'

0

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.

0

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.

protected by Jane S Feb 9 at 4:02

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.