Some (More) AntiPatterns At My Job

At my current employer, I'm surrounded by old-timers who don't want to change their ways. It's not an exaggeration to say about a third of my company got their start on punch cards.

For a long time, everything here was in VB6 before eventually being rewritten in C# (they're just finishing up the conversion this year!). But a lot of code is still very.... VBish. But mostly just bad.

I don't deal with most of that, thouigh. I do the web development, and get to use pretty much whatever tools I want (though I keep the backend in C#, because honestly I love the language). For a while it was myself and a long term contractor, but he left a few months ago. As a Big Project had just completed, they decided not to replace him, but instead I get to cross-train someone from the "Operational Package" team. I got my hands on the one young guy over there (this is his first programming job), as I view him as... salvageable. He's picked up some bad habits from them, but he seems smart enough. We're doing code reviews (something the operational package team doesn't do), in part to try and correct some issues.

So, I get my first pull request form him today, and look through the code to find this gem. He was calling this "DataCleanup()" mehod constantly. He copy/pasted this from the Operational Package code, where it's called on every sql parameter before it goes in, and it's called on every field that comes out before it's hydrated in an object. For instance:

For a long time, everything here was in VB6 before eventually being rewritten in C# (they're just finishing up the conversion this year!). But a lot of code is still very.... VBish. But mostly just bad.

Sounds familiar... did they come up with a brilliant idea to putusing Microsoft.VisualBasic;
everywhere, to 'feel more at home, develop faster and make the code more readable'?

I can forgive this, I've done it to set a breakpoint there. At least he didn't trash the stack trace.
As for the rest of the code:
@Pharylon <a href="/t/via-quote/54528/1">said</a>:<blockquote>```
//Truly, this is madness.
```</blockquote>
Indeed.

At my current employer, I'm surrounded by old-timers who don't want to change their ways. It's not an exaggeration to say about a third of my company got their start on punch cards.

For a long time, everything here was in VB6 before eventually being rewritten in C# (they're just finishing up the conversion this year!). But a lot of code is still very.... VBish. But mostly just bad.

You in the audience might laugh about C# being written as if it is VB6 with weird syntax. Come to think of it, it sounds like @Pharylon is poking fun.

I once worked in a place that had an unholy mix of Fortran and C, with a sprinkling of C++ and JavaScript (with GTK bindings, no less, for UI management), and one mad corner that used Java. They were trying valiantly to rid themselves of the Fortran code, but this process was hindered mightily by the tendency of the old-guard programmers to imitate one of the classic jokes about Real Programmers(TM) : a Real Programmer can write Fortran in any language.

It was deeply scary, some of the code you found when you went digging. It really was exactly as if they thought that the C compiler was just a Fortran compiler that used a strange input syntax, so they wrote string manipulation routines that were 100% C language but ignored decades of how-to guides for strings in C, and treated them exactly as if they were Fortran strings in situations where there was no need to do so. A Fortran string variable is just an array of characters, but there is no magic character that marks the end of a Fortran string. So they were manipulating C strings without NUL terminators.

try {
[...]
} catch (Exception up) {
[...]
throw up; // NO! NONONONONONONO! YOU ARE DESTROYING THE STACK TRACE! EITHER WRAP THE EXCEPTION IF YOU WANT TO MAKE YOUR PURILE JOKE OR JUST USE THE UNADORNED `throw`!!!!! BECAUSE I WILL CUT YOU IF YOU DO THIS TO ME AND I HAVE TO DEBUG WHAT YOU HAVE PAINSTAKINGLY GIVEN TO ME WITHOUT A PROPER STACK TRACE! GAH!
}

So they were manipulating C strings without NUL terminators.jdk^CjfdLk jk3s d#f67howe%^U r89nvy~~owmc63^Dz x.xvcu

Filed under: FTFY
<!-- body is not similar because my other post failed to post you pile of shit! -->

Indeed, although because they were doing it in a Fortran way (Fortran using weird syntax, remember), they didn't have that particular problem. Nevertheless, it gave me the willies every time I saw any of it.

But that was far from the worst thing I saw there. There was, for example, a 1500-line Fortran function of which approximately 90% (measured by LOC count) was conditional on who called it, but in many, many different small if statements. And there was the notorious "evildead.c", all of whose variables and internal functions were named after characters or lines in the film suggested by the filename. And a C file somewhere which attracted complaints about it being hard to edit because of the exclusive checkout policy, where the complainers didn't take into account that the file was in excess of 1.6 MB, and drove a frequently-changed screen, so there were always four or five developers trying to modify it.

By the way, I just checked. The DataCleanup() method is called 18,098 times in our codebase. It's not some cherry-picked monstrosity form the bowels of the codebase no one ever uses. It's a core function that's used a lot.

It's hard. You can easily write good VB.Net, but classic VB is missing a lot of what modern languages take for granted. The good VB6 programmers immediately recognized the value of most of the new stuff and started using it immediately. The bad VB6 programmers tried to write VB6 in VB.Net and complained about everything that changed.

My favorite example is when I had to add some features to a VB6 app that was scheduled to be upgraded to VB.Net in a few months. I structured the methods the way they would be in VB.Net - and it took some extra boilerplate code in VB6 to get it done. The plan was for the people doing the upgrade (it was all done by hand, not a tool) to replace the slightly cumbersome methods with the obvious VB.Net equivalent. Instead, they added more boilerplate to the existing boilerplate.

Wow, someone should remind them that *nullables are not boxed as nullables, but as their underlying type*.
@Pharylon <a href="/t/via-quote/54528/1">said</a>:<blockquote>```cs
//Here, if you suplied "foo" for the defaultValue, but if the value object
//was a DateTime, a method meant to 'clean up' the Data will explode.
```</blockquote>
Wait, what? If you supplied `"foo"` for the `defaultValue`, you won't be in this code path at all since the test on `objectType` is based on `defaultValue`'s type...
@Pharylon <a href="/t/via-quote/54528/1">said</a>:<blockquote>```cs
//Enums can't be null! What the fuck is wrong with these people?
//Plus, if it was null, it'd have been caught earlier in the method.
```</blockquote>
Given how fucked-up the start of the method is (what with trying to detect `Nullable` from a boxed value), I'm not as certain as you are.

In terms of functionality, it certainly doesn't appear to do very much of any use, beyond heating the server room just that little bit more. Overall it's probably not much of an overhead, especially as it comes before / after database access, which should be several orders of magnitude more significant.

That said, it definitely instills a desire to pull out the cluebat and apply it to those reponsible, finger by finger, until they can't physically use a keyboard any more. The generics stuff is particularly splendid.