8,300 lines of dependent code to *drum roll* create one record

8,300 lines of dependent code to drum roll create one record

Posted by Voxmanns@reddit | talesfromtechsupport | View on Reddit | 57 comments

**For context - this is a project I was called in to as an emergency resource. Basically, come fix our mistakes...anyways...

Oh yeah. Just found this one recently. One transaction runs 170 lines of code to verifying the creation of a single record across 3-4 classes.

The total line count of those classes and their corresponding test class is just over 8,300 lines of code. One of the classes on its own is over 2400. I saw a method that had over 300 lines just on its own. What a trooper.

The first kicker (maybe not, there's a lot of great stuff in here....8 layer if else statements....)

Okay, one of my favorites is that the big mama class is a global class and is integrated with an external system. This system, as evident by dozens of unnecessary permission checks, can have multiple users hitting this class at the same time.

That's a real problem because -every- -single- -method- is static and **THE SOURCE RECORDS ARE STORED AS GLOBAL STATIC VARIABLES**

For those who may be unaware or just unfamiliar, "static" means that there cannot be multiple instances of that variable even if multiple "things" are referencing that method/variable. In other words, if you have two requests come in at the same time - you could very easily end up swapping the source records mid process and corrupting the data on one or both sets of data. And you would never know because, while corrupted, all the data is likely still in a valid format and will pass through the system without a system error. Kind of a big deal when it's directly associated to every deal in your sales process.

But the thing that really prompted me to post this was such a level of "I don't care about the next guy" that I am actually stunned. I think I may be in denial still...

There is a class, and he's amazing. We know the rules - don't hard code values that are susceptible to change. If at all possible (and reasonable) don't hard code a constant that may change in the future.

This class, I shit you not, is just shy of 500 lines of grade-a, organic, non-GMO constants baby. They're global, they're hard coded, and they're susceptible to change.

But, that wasn't enough. It's the gift that keeps on giving. After spending the better part of 12 hrs wishing I was dragging my head across the pavement at high speeds, I noticed the comment at the top of the code (slightly paraphrased).

"Class to hold the static values...to avoid hard coding."

Sometimes, man, I really do wonder. I feel like I have done a pretty good job remaining positive about this absolute mess of a transaction - but why they gotta spit in my face like that? It's hard coded, for 500 consecutive lines, right below that message. That's EXCLUSIVELY what this class is.

C'mon man. Just....DAMN!

**Final bit of context, because idk what everyone's familiar with, this platform has built in functionality for getting the exact values at the time of running. There isn't a single thing in that constants class that needs to be declared as a hardset constant in the code. And there are no checks to verify that the transactionId remains consistent.

[-]

al-mongus-bin-susar@reddit

Bruh quit the yapping this is tame. Only 170 lines in a transaction? Weak. I've written 3x that for a toy project. Only 300 lines in a method? That's child's play. Barely worth giving a second thought to. The static globals are more of a concern but at that point just make the system queue requests and take them sequentially.

[-]

bendingoutward@reddit

So, I recently dropped a client that came to me with a PHP app. That's something I already don't want to deal with.

It's a weird mashup of bits and pieces of several PHP frameworks. The pieces used are often feature redundant and absolutely do not play together. New hotness gets announced on a blog or something? The function that handles routing for a very specifically shaped request is getting added. For a month.

Literally the only documentation in the whole hodgepodge was a comment buried deep in the source (which gets bigger with every request, as every request literally generates and writes a new PHP file):

// This will never not be painful

[-]

livaoexperience@reddit

8,300 lines to create one record? 🤦‍♂️ This code is the perfect storm of chaos. Static variables, hardcoded constants, and permission checks galore. The cherry on top? A class 'to avoid hard coding' that's literally just hardcoded constants. Sometimes I wonder if they were just trying to create the most complicated system possible for fun.

[-]

Voxmanns@reddit (OP)

Legitimately. I remember being a jr dev and stumbling through the process of programming. I THOUGHT it was self evident stuff.

"Don't hard code values" -> If I am about to hard code a value, I am probably not solving this correctly. Find a better solution.

So, truly, it looks like an intentional effort to overcomplicate the system. The more I look at it, the more it feels like a conspiracy of sabotage isn't so far-fetched. It goes beyond bad code and seriously resembles what I would build if I wanted it to be a malignant program.

[-]

ultimatex115@reddit

I believe by hardcoded values they meant putting if x == "something". Instead that would be if x == arbitraryConst so it's easier to keep track off when used in more places

[-]

Voxmanns@reddit (OP)

Yes, that's a specific aspect of this platform. I tried to explain it but it's really hard to explain for me haha. I don't know how to translate it to more general development lingo.

The system has native functionality for metadata. That is, I can get you property, reference, and method from any object as a native aspect of the platform.

So if I have something like

MyObject:
  var status = "True"
  var statusOptions = ["True","False"]

Then I can and should reference that with something like

var objVar = myObject.new();
var acceptedVal = objVar.getStatusOptions()[0];
if (objVar.status = acceptedVal) {...}

And if they want that as a constant, so it's not tied to the instance of the object, it'd be something like

const acceptedVal = MyObject.getStatusOptions()[0];

But they declared it as

const acceptedVal = "True"

Obviously my example isn't the totally correct way to do it either. But hopefully that gives a better idea. With their code, the acceptedVal may no longer even exist as an option in the statusOptions field and you would never know unless the system bricks up somewhere and you can trace it back to "oh they are reference this property of this object" which is a f%@#ing pain hahah.

[-]

meitemark@reddit

The only thing I hardcode is that pi = 3.15

[-]

polarbear128@reddit

That's not even correct on rounding.

[-]

TinyNiceWolf@reddit

Nah, it's just rounding to the nearest 1/20th.

[-]

-MazeMaker-@reddit

There must be some algorithm to approximate pi using a d20

[-]

TinyNiceWolf@reddit

There is! Draw a circle on a table. Then draw a square around the circle, touching it on all four sides. Throw the d20 (or any die). Note whether it lands in the part of the square that's inside or outside the circle. (Throws outside the square don't count.) Repeat for a good long while. Then take the ratio of inside-circle throws to total throws and multiply by four. The result approximates pi.

[-]

-MazeMaker-@reddit

I was thinking of this method but using the die rolls to get x and y coordinates. It still wouldn't give you a reason to round to the nearest 20th, which is what got me thinking about it in the first place.

[-]

TinyNiceWolf@reddit

Last time I had to round to the nearest fraction was when checking tax rates, which in the US can be (for example) 6% or 7.125% or 8.9%. I had to divide the amount charged in tax by the subtotal, then display both the correct rate as a percentage, and the rate that was actually charged.

It turns out that rounding to the nearest 1/200th of a percent works kind of OK for that, as it allows for regions that set their rates in tenths of a percent (like 8.9%) and regions that set their rates in eighths of a percent (like 7.125%).

[-]

meitemark@reddit

The hardcoding says that 3.15 is correct. Also, just to add to the chaos, horisontal/vertical is set to "meh, close enough", and the kerning between r and n is reduced to zero.

[-]

Voxmanns@reddit (OP)

I fucking can't dude LOL

[-]

meitemark@reddit

When typing speeds go over 40WPM the letters K and L will switch places. Sometimes.

[-]

iiiinthecomputer@reddit

Good old keming

[-]

cabezametal@reddit

I feel your pain, yet somehow I always found a huge satisfaction when I got to refactor these kind of f'ups.
I hated to deal with them, I cursed at the authors, yet the dopamine peak that I got every time I pushed a commit safely deleting them was worth it (somehow) extra points when the tests went properly along as they should.

And yes, Im aware, risky as always, refactoring is a painful art xD

[-]

Voxmanns@reddit (OP)

Oh, believe you me, I felt like an absolute G when I finished going through every method in the big mama class and planned its refactoring. My estimate came out to something like 4-6x less complexity (based on LoC) just by shedding all the unnecessary references and reorganizing some logic. Something like 5 methods, I am pretty sure, do literally nothing because they were programmed to just return false every time.

This is just one of potentially many. The namespace of the big mama class has several hundred classes underneath it. All I can really gather is that this application is like a custom made middleware sitting between 4-5 applications. So I have to imagine there's more of this throughout the entire application.

I hope the client agrees to let me start fixing these issues. It'd probably take a whole year (optimistic) to refactor this application into something reasonable. But I can already hear the other agencies screaming "We'll do it all in one go for a million bucks" and I am just so worried for them haha.

[-]

RahnIAm@reddit

I’m gonna start writing methods just for returning true and false, so I can always call them and know what they’re gonna do.

Public bool MyMainMethod() { If (returnTrue() == true) { return returnTrue(); } else { return returnFalse(); } }

Yeah, pretty sure something like that will make me l33t and/or have people cursing my may for the next 20 years.

[-]

joquarky@reddit

I love these because the goal is absolutely clear: make it better but with the exact same output for a given input.

I'm so burned out on agile.

[-]

APiousCultist@reddit

"We are what they grow beyond. Such is burden of all coders."

-The Previous Guy and/or Yoda

[-]

Cornflakes_91@reddit

climbing statisfaction mountain to reach dopamine peak

[-]

RelativisticTowel@reddit

I'm a maintainer for a codebase that's older than me (pretty sure some of the code in there is pushing 50). We have single ~~methods~~ ~~functions~~ subroutines with 5000+ lines. Very few nested conditionals/loops... Because you don't need to nest them if you don't use them, it's goto all the way down. There's even a couple longjmps still in there sigh.

I love/hate my job.

[-]

Vidya_Vachaspati@reddit

COBOL?

[-]

RelativisticTowel@reddit

Fortran 77 🥹

[-]

Vidya_Vachaspati@reddit

In my first job, I worked on COBOL code that was written before I was born. This was in the early 90's. I am sure the code is still running somewhere.

[-]

Psychological-Elk260@reddit

You mean like in banks?

It is also part of the ISO 2023 standard. Lol.

[-]

RelativisticTowel@reddit

You'd be surprised how far you can get with "make sure the new standard includes this archaic piece of junk, or we're gonna need billions to update it".

[-]

Vidya_Vachaspati@reddit

Most likely banks.

I am not surprised it is part of the ISO 2023 standard. One of our old timers used to joke that this code will outlive all of us.

[-]

Psychological-Elk260@reddit

Sorry, it was a statement of fact. It is used in banks and the financial sector and some military ones still.

[-]

TheArmoredKitten@reddit

Yeah our version control only goes back three decades, but there's probably some older stuff too.

I have a high suspicion your codebase is related to vital infrastructure for a financial institution.

[-]

RelativisticTowel@reddit

No, it's scientific computing stuff. Another area where it's not unusual to deal with code that old.

[-]

fresh-dork@reddit

i still remember printing CreateDialogEx - that damn thing was 15 pages long

[-]

Voxmanns@reddit (OP)

Yo that's actually crazy haha. This is all fresh code - written less than 2 years ago. There are definitely some comments in there that indicate some organic bloat through projects but there's also 300 lines of commented code at the end of one of the other helper classes so...there's that.

I couldn't imagine trying to maintain a 5000 line subroutine. How does that even happen?

[-]

efahl@reddit

Sounds like grist for thedailywtf.com

[-]

fresh-dork@reddit

just for shiggles, i did a line count for some recent ETL work i did - it handles a top level record and 4 dependent lists of records. total of ~2k lines, only globals are a few constant declarations.

[-]

Voxmanns@reddit (OP)

Whewww now that's a loader! I imagine most of that must be in handling the dependent lists yeah?

[-]

fresh-dork@reddit

mostly it's poking fields in the right place, handling inconsistencies, some validation and type conversion. it's python, so a lot of stuff is handled compactly

[-]

Voxmanns@reddit (OP)

I gotcha. So just a bunch of 'stuff' to satisfy a big process. Well hell man, well done keeping it all straight. That's a whole application right there.

[-]

fresh-dork@reddit

thank fuck for unit tests and model validators

[-]

KelemvorSparkyfox@reddit

I - Wow.

In a previous job, I supported an interface between two very different ERP systems. They were created by different software houses, but after a series of mergers and acquisitions, are now owned by the same one. Anyway, this interface had more RPG code than one of the ERP systems it connected to. But it worked. It had its own reference tables and translations, and even the rounding errors from truncating six decimal places to five, and then dividing the value by 0.0327 didn't cause too many issues.

Then they bought Oracle. This was to replace the code-lite ERP. However, they didn't want to write a brand new interface, so they piggy-backed Oracle off the existing one. A new set of translation tables were set up to hold an ever-expanding list of site codes so that when a new site went live, they just added it and its transactions were diverted.

Oracle also needed some code for this. There were a some gargantuan PL/SQL stored procs written. One of them would receive incoming transactions from the old interface and generate one to four inventory movement records according to requirements. When Oracle was retired, thirteen years later, there was still a comment in some unimplemented error handling: Shree to add later. Shree was one of the consultants who helped to ~~implement~~ inflict Oracle on the company. She didn't leave everything undone, though*. The stored proc to push inventory movements the other way could have made use of a custom table to translate between Oracle stock movement types and Prism stock movement types. It didn't. Instead, the biggest If...ElseIf...ElseIf... structure I've ever seen handled that.

*Joking aside, Shree and her colleagues did an excellent job, given the restrictions that they faced. The code might have been a little rough and ready, but it did the job and provided near real-time communication.

[-]

Status-Bread-3145@reddit

I feel for you on "shared globals". That "global" mess us a prime example of "constants aren't, variables won't".

And when you start making changes, it is going be "make one change, then determine whether all the remains functions / code still work as intended". Rinse and repeat as necessary.

And, to be safe, have a copy of the unmodified entire code base saved in multiple places so that if something goes sideways, you can say "it worked like that in the original code so whatever issue you are having is not related to any of the current changes".

Were the devs drunk on Lord of Rings and decided that they would create the equivalent of the "one ring to rule them all"?

[-]

Voxmanns@reddit (OP)

I genuinely don't know. I don't know what it is about this particular project, but it has struck some nerve in me. Maybe some form of disillusionment and realizing just how fucked all of these people are getting.

I mean, this is millions of dollars a year. I am looking at the fall out of just one project that is going to cost several million to fix (if it ever gets fixed, let alone how much it's bleeding just by existing). These happen like clockwork, and not just in the platform I am working with.

It kinda breaks my heart man. I feel ridiculous being this upset over code and work but like...damn dude. It's really, really bad. That's so much money every year that could be going to ANYTHING but this. And it all trickles down to, in part, why products and services are so expensive. It's so unfuriating, and so disappointing. Got me fucked up.

[-]

sideflanker@reddit

In other words, if you have two requests come in at the same time - you could very easily end up swapping the source records mid process and corrupting the data on one or both sets of data

You appear to be working within the Salesforce platform. Static keyworded vars/structs are actually scoped to the transaction. Multiple record updates or external integrations being processed simultaneously will track their own versions of anything defined as static.

"Class to hold the static values...to avoid hard coding."

You are right in that Salesforce has a dedicated system for storing static values, but they aren't exposed in test classes. Using that option you'd need to either: expose all record data to the test class (not recommended for various reasons) or hardcode expected static values in a test data factory. So now you have the same issue except the values are listed in two areas instead of one.

All constants should use the FINAL keyword to prevent edits during runtime. But what you essentially have is a config file within the limitations of Salesforce.

Custom metadata types (another Salesforce system) and static resources (uploaded files) are also available but they aren't much better for various reasons I don't feel like typing up.

Semantically, yes it's the wrong usage of the phrase "hard coding", but at the same time it's the easiest way to handle things within the platform.

The total line count of those classes and their corresponding test class is just over 8,300 lines of code. One of the classes on its own is over 2400. I saw a method that had over 300 lines just on its own. What a trooper.

One thing to know is that one org may have had multiple different vendors building code over the course of years. Even within the same vendor, sometimes it'll be different teams that handle different phases of the project because the original team members rolled off before the next phase was negotiated and signed off for approval. Project requirements are often times figured out as they go which is probably why you see some logic that doesn't make sense anymore.

Experience levels of Salesforce devs vary wildly. Depending on the company, many won't have compsci degrees and are working with limited formal training. You'll definitely want to lower you expectations.

[-]

muusandskwirrel@reddit

It’s that way so that alllllll the hard coded garbage references the one master garbage goblin

It’s the goblin king, where you go with your offering up update a master string.

Turn a whoseit into a whatsit there? It’s that way everywhere.

[-]

Voxmanns@reddit (OP)

LMAO "It may be trash, but we'll make sure it's CONSISTENTLY trash." it feels like.

I will be bringing this up to them in no uncertain terms. I was laughing until I saw the static issue and that made it go from bad to scary code for me. And you better believe my plan puts isolating that goblin king from the rest of the code base as fast as possible. Whether or not they see reason enough to go along with it.....not so sure.

[-]

Tom2Die@reddit

It might be bad, but named constant is far better than magic number (which is what I assume the comment meant by "hard-coded") imo.

[-]

Tight_Syllabub9423@reddit

Hey, at least there are comments

[-]

Jonathan_the_Nerd@reddit

"Dear Friend, I apologize for writing a long ~~letter~~ method. I did not have time to write a short one."

[-]

Voxmanns@reddit (OP)

Yeah, I hold nothing against the original developers. I told my POC at the client

"I feel bad for you, and I feel bad for the guys who had to make this."

The developers look like they mostly came from different frameworks. I think they wrote what they were familiar with and there just wasn't any time for the senior devs to critique it or design it properly. So they just went with it. Can't really blame them there. I'd want to keep my job too.

This is 100% on the management of that dev team. They came in, sold a promise, delivered enough of the promise to lull the client into a sense of "good enough" and then dipped. It's unfortunate, but that's business.

Frankly, there's enough useless code and just blatant fundamental design flaws that I don't think litigation is off the table. This is, to me, an undeniable case of professional negligence. That's ultimately for them to decide - but if I was them I'd be looking for blood.

[-]

robertcrowther@reddit

"Class to hold the static values...to avoid hard coding."

It's never got to 500 lines long but I'll admit to having done similar things in the past. The idea being that everything I wanted to make a setting could sit in this class for now and then once I'd got stuff initially working I'd set up reading config files to populate those things.

I wonder if this is one of those things that was initially some sort of prototype or proof of concept that then got handed over to some poor junior developer to 'fix' with little context.

[-]

nico282@reddit

Probably there’s a “Will fix after acceptance” comment dated June 1996 somewhere.

[-]

Voxmanns@reddit (OP)

Try 2019/2020 haha. This code is pretty young.

[-]

Voxmanns@reddit (OP)

Most likely. It was a big implementation by a big firm known for big fuck ups like this one.

[-]

Turbojelly@reddit

Look for the code that wrote the code.

[-]

that_one_wierd_guy@reddit

this is what happens when a non it manager insists

1this is the standard to follow

2it must always be doing "something"