The code so bad you won't hire

[AndyC]

Yes, but again I'm specifically talking about this specific ROM for the spectrum having little in common with the functionality required on a bbc micro so there's nothing to "port"

Unlike the Speccy, the BBC has a very clean architecture, with most things having to be done through defined system calls (which is why things like Tube accelerators work so well). Given that, I'd expect the machine specific parts on the BBC version to be suitably abstract, especially given they'd probably be working with source code and not just the binaries.

You'd still have to write some code to replace the sys calls and provide some emulation of the Teletext screen mode the BBC has, but I doubt such conversions were ever 100% automatic. It's even possible the original 6502 code was designed to be ported in this fashion, with the necessary parts that would need changing suitably abstracted. Especially if the developer had done similar things before and was expecting to convert it.

[Vampyre]

Earlier today I was asking some people: what line of code would make you never hire someone?
My anti-hire line was:
s = s.replace("&", "&");

Out of interest - what's so horrifically bad about this? I can see a purpose for it when cleaning up a simple string, so I'm wondering if there's more context behind it.

[SteveSmith]

Out of interest - what's so horrifically bad about this? I can see a purpose for it when cleaning up a simple string, so I'm wondering if there's more context behind it.

I asked exactly the same question on page 3 of this thread.

[AndyC]

It's that it's bodging the results, rather than actually fixing the root cause that's generating the dodgy code in the first place.

[Vampyre]

Ah I see. Fair enough if it's your own code that's faulty in the first place. I work with a lot of third-party APIs and this would be a perfectly valid fix because getting a third-party to change their results/fix their bugs is like pulling teeth.

[Dr beep]

Almost 20 years ago.

IT had a bad solution for a problem. I made a better and I was allowed to implement it.
I needed a user to help but he had no time for the jobs he did.

I took a look on the program he used. Aside from a lot og spaghetticode it was terrible slow.
I rewrote the program in a day and tested it against his old input and output.
He needed an hour to do 1 job, my tool did the same in 1 minute and also discovered a bug in the code.

He had to do 60 files in a week and was happy with my program.
I told his teamleader to never let that other person make a program EVER!

[Einar Saukas]

One more:

Code: Select all

    private long longToLong(long openItemID) {
        Long l = Long.valueOf(openItemID);
        return (l.intValue());
    }

[Vampyre]

Boss: Hey, you, programmer ...
Programmer: Me ?
Boss: We need to code this stuff... by Friday. Presto, presto.
Programmer: But... I program for 6502.
Boss: What ?
Programmer: This is Z80. I don't code for Z80.
Boss: What is the difference ?
Programmer: Well, for start ...
Boss: You are programmer, right ? You program things, right ?
Programmer: Yeeeeeah... I have Z80 instruction table somewhere.

-----------------------------------
6502 has short branches only and and unconditional long jump. The idiom you showed above would be typical for 6502 (or 6800). But it is just guess....

In the words of an owner of a company I was working for at the time (the Diner's one I mentioned a while back): "Programming? It's just typing." In front of all the developers in a meeting. You can imagine the aftermath.

[PeteProdge]

"Hello, I was wholly responsible for all code in Fujitsu's Horizon IT system for the UK's Post Office. Hello? HELLO? I think this line's gone dead. HELLO IS ANYONE THERE?"

Anyway, I've enjoyed reading these, even though I've barely been an amateur developer.

Come to think of it, in my very very early days of dabbling in Spectrum BASIC, when I didn't know the INKEY$ keyword but did know INPUT, I knocked up a Pac Man clone, with INPUT a$ and an on-screen instruction for the user to just press enter if they wanted to move right, or type space then hit enter if they wanted to go up.

Yes, this is extremely terrible. Sqij-level awful ness. It really was the best I could do at the time and I didn't even know that most screen movement in BASIC is largely a case of PRINT AT x,y; " ": LET x=x-1 (or something): PRINT AT x,y; sprite. I was just going to write every possible screen position as a line in BASIC. Yeah, should I really have been allowed within a mile radius of a computer terminal?

Let's just say my enthusiasm definitely eclipsed my coding talent.

[Vampyre]

What you've just described though Pete is a developers life. We have all written sh*t code - anyone who says they haven't is a bold-faced liar. But we recognise what you found out - there's a better way of doing things and do it that way instead. It's called refactoring and is an important part of any developers arsenal.

I've been doing this for 25 years. Even now I'll look at something I did 18 months ago and jaw-drop at the sh*t-ness of it. It's a constant learning process, which is what makes it so damn interesting.

[ParadigmShifter]

People who refactor your code without telling you and it doesn't work how it used to is annoying though.

If it ain't broke don't "fix" it!

[Vampyre]

People who refactor your code without telling you and it doesn't work how it used to is annoying though.

If it ain't broke don't "fix" it!

Oh god, yes. Is there anything worse than performing a code review where you notice swathes of working code has been refactored? Currently have a developer who's a bit of a bugger for doing this - it's driving the lead dev up the wall. I've had to step in several times and give a warning too - but it's falling on deaf ears. Fortunately he's not unleashed on this system very often and is in his own walled system that he can write unintelligible (but working) code for. Bloke sure does love a Static.

[Oloturia]

I remember that in my first job (1999-2000) my boss asked me to take on a project he wrote before my hiring. It was a database manipulation program that held a dozen tables.
The customer needed some kind of tracking for some items to be sterilized. The things could be sterilized a few times before they needed to be disposed of. They were tagged with a barcode and were scanned before entering in the sterilization process. The entire program wasn't working properly. Sometimes an item suddenly changed its values without a reason, i.e. trousers became a pair of gloves, socks became a surgical gown etc.

When I opened the project I noticed that the progressive number that made the barcode, that was also the primary key for the table that kept the sterilization status, was recycled now and then. When an item reached its maximum number of sterilizations, it was copied into a different table that kept the history of that item, still using the barcode as primary key. That explained the unwanted changes: when an operator inserted a record into the current sterilization table, it changed also the values of the same barcode into the historical table (IIRC the two tables were linked together by a third table that held barcode and description), when they added a new item in the historical table, the line with the same barcode was overwritten and the history lost (yes, I know it's confusing... think about me, in my 20s, working for my first time, being flabbergasted by such a show of utter incompetence).
When I asked why they had to recycle the barcodes, being numbers virtually infinite (with their rate of use they would have gone overflow in thousands of years) my boss said that "the customer said so". When I insisted to change this terrible idea of having a unique key that linked tables that held different records and that could be duplicated (yes, a UNIQUE key) he got angry and told me to solve the issue, not to be philosophical about it.

So I deleted all his work and re-did it putting a progressive number as a primary key for the tables. It took me an hour or so, after a year of development.
He didn't feel humiliated by my performance because he thought that I used his work as a base for mine. It was rewritten from scratch.

There was another issue like that. The customer called me a bit disappointed about a bug. I was fully aware of that bug, because the specifications were conflicting with common sense, but my boss said that "the customer said so". Boss was on vacation so I had the opportunity of speaking directly with customer, that explained calmly what they wanted. Turned out boss didn't understand anything as usual. I corrected the bug and customer was happy.
When he got back he got really angry about that. He phoned the customer saying that he lost control of the project and that the software shouldn't be used on production.

That was the last straw. After a couple of months I found a new job (that was even worse, but that's another story).

Unfortunately I couldn't fire my boss, but that was definitely the wiser thing to do then.

[Oloturia]

I've never told this story to anyone before, but here I'm mostly anonymous. Besides, I don't think that neither my boss nor the customer are in business any more, anyway.

I feel lighter now.

[Einar Saukas]

When I asked why they had to recycle the barcodes, being numbers virtually infinite [...]

Have I ever seen anything similar in production? Surely!

Code: Select all

    public Long getNextID() throws DataBaseException {
        Long id = getNextSequenceVal("SQ_STTS_FORMULADESCRIPTION");
        while (hasResult(" SELECT FDS_ID FROM STTS_FORMULADESCRIPTION WHERE FDS_ID = " + id)) {
            id = getNextSequenceVal("SQ_STTS_FORMULADESCRIPTION");
        }
        return id;
    }

[bluespikey]

From the Post Office Horizon document on page 17. Its basically d:=-d :

https://www.postofficehorizoninquiry.or ... 052001.pdf

Code: Select all

Public Function ReverseSign(d)
  If d < 0 Then
    d = Abs(d)
  Else
    d = d - (d * 2)
  End If
  ReverseSign = d
End Function

"Whoever wrote this code clearly has no understanding of elementary mathematics or the most basic rules of programming"

[worcestersource]

Shocking.

[SteveSmith]

That's unbelievable. Not only that it's like someone's first program after completing Hello World, or that it's in VB, but it also must prove they never did code reviews, which for a financial system is crazy. Or if they did, but were laughing at the poor code so much they forgot to go back and fix it. If I'd gone to prison after being convicted by data created by this kind of code, I would probably have literally exploded with anger.

[Vampyre]

Holy hell - that's unforgivably bad!

[R-Tape]

That's probably how I'd have done it in Sinclair BASIC!

I'm not a proper coder - whats so wrong with that example? It works for all eventualities doesn't it?

[SteveSmith]

It technically works, but just doing "ReverseSign = d * -1" would be so much simpler. The fact they've made such a meal of it (and that no-one else either saw it or felt the need to fix it) rings big alarm bells.

[Vampyre]

That's probably how I'd have done it in Sinclair BASIC!

I'm not a proper coder - whats so wrong with that example? It works for all eventualities doesn't it?

Yup, as Steve says it technically works but it should have raised alarm bells with anyone with a basic grasp of mathematics. All it's attempting to do is turn a negative number into a positive or a positive into a negative.

If -5 then ABS(-5) = 5
if 5 then 5 - (5 * 2) = -5

So yes, it technically works. But the far, far simpler solution is to change the Else to:

d = -d (which will convert 5 to -5.)

Steve's example is even better as it's a single line of code that works for positive and negative numbers. The solution is so over-engineered it's ridiculous. A code review should have picked it up (if they even had any) and any developer code reviewing a financial product *should* be looking for these kinds of things.

[ParadigmShifter]

Well it would pass unit tests I guess.

Prefer d = -d to multiplying by -1 (decent compiler will optimise it though I suppose).

I don't mind it having its own function, it's much easier to search for Negate (which is of course what the function should have been called) rather than searching for - which will be all over the place. Which is a reason why operator overloading can be bad as well of course.

I heard the problem with the Fujitsu code was that it was not doing atomic operations and the network the postmasters connected to would often be down/timeout etc. It wouldn't be down to a simple negation issue (and it is technically correct anyway assuming no overflow - unlikely with money handled by post offices even if they use an int rather than Money (although they should probably use that).

VB not having to declare types or variables (at least in the olden pre .NET days) makes it hard to know what arguments it expects though.

[ParadigmShifter]

Steve's example is even better as it's a single line of code that works for positive and negative numbers. The solution is so over-engineered it's ridiculous. A code review should have picked it up (if they even had any) and any developer code reviewing a financial product *should* be looking for these kinds of things.

Not if the argument is an int and you pass INT_MIN of course Maybe VB detects that and errors out, I dunno. C/C++ certainly doesn't. Not sure about C# (although I really should know that lol. I expect it does the same as C/C++ which is not great).

EDIT: Not that they should be using an Int though if it is dealing with money. They shouldn't use a float or a double either and I suspect Abs coerces the value to a double.

There is a Money type but it's very inefficient (10 bytes of data IIRC). EDIT2: Currency in VB6. Changed to Decimal in VB.NET.

Last game I worked on we had to roll our own Currency class since we didn't want to use Decimal and Unity doesn't have a way to display those in editor anyway. We had to write our own property drawer to edit them anyway. We ended up using a Double as underlying type which wasn't ideal cos of rounding issues. Should have used an Int and used pennies as the base unit really, would have got around rounding issues. Originally we had a float but that wasn't accurate enough for large purchases (float only has about 5 decimal places of precision, nowhere near enough).

[Einar Saukas]

Since we are now discussing bloated code... here's another routine I had to fix in production:

Code: Select all

public OpenItem getPaymentNotMade(Collection payments) {
    final List paymentsAsList = new ArrayList(payments);
    Collections.sort(paymentsAsList, new Comparator() {
        public int compare(Object o1, Object o2) {
            try {
                final OpenItem openItem1 = (OpenItem)o1;
                final OpenItem openItem2 = (OpenItem)o2;
                if
(((OpenItem.STATUS_ACTIVE.equals(openItem1.getStatus())) &&
(OpenItem.SUBSTATUS_ACTIVE.equals(openItem1.getSubStatus())))
                        &&
((OpenItem.STATUS_ACTIVE.equals(openItem2.getStatus())) &&
(OpenItem.SUBSTATUS_ACTIVE.equals(openItem2.getSubStatus())))) {
                    if (openItem1.getDueDate().before(openItem2.getDueDate())) {
                        return (1);
                    }
                }
                return (-1);
            } catch (Exception ex) {
                throw (new RuntimeException(ex));
            }
        }
    });
    return ((OpenItem)paymentsAsList.iterator().next());
}

Bonus points to the first person that can explain what's going on!