[akerro]

FUCK, you might think it's funny, but I literally work with one senior developer (10+ years experience in pure Java). He write code like that, during review insists this is OK because he's been writing such code for over a decade! I inherited this project after he moved to another country and is not legally to work remotely for company in the UK. He has permissions to override jenkins+sonarqube, so he merges code without test coverage. His code has literally 0 tests. Only today I was rewriting code like:

   if(i == 1) {}
   else if(b == true){}
   else if(s == "string") {}
   else if(response.status() == Status.OK){}
   else if(hereIsARecursiveMethodAlwaysReturningFalse()){}
   ... 3 more

notice each `if` block has different variable of different type!

[hinkley]

I have a Good Fences Make Good Neighbors trick I use, and some people call it a Canary Build.

When contributors to two different modules keep running into contract violations, you set up a CI build that triggers when either of the modules is built. If it fails it means that something got broken. It doesn't stop the build pipeline, but it warns you that garbage is about to come out the other end.

There's a general dynamic between people where peer pressure does not work when the delay between action and consequence grows too long. Nobody truly internalizes how upset other people are when they are found out for something bad they did a year ago, a month ago, or in some cases days ago (hence why roommates fight so often about chores). But getting called out for something you did two hours ago has sorted out an awful lot of bad behavior.

And the nice thing about the Canary Build is that in many CI tools you can set it up and not give him any permissions.

[lostcolony]

"It doesn't stop the build pipeline, but it warns you that garbage is about to come out the other end."

Why doesn't it? Prevent it from merge and build; require the dev to either fix it, or convince the rest of the team that the change should be allowed.

[J-Kuhn]

You make an API change.

The API change is communicated and approved by both parties.

You build the first thing.

Canary fails, because API has changed.

You change the other to match the new API.

Canary fails again, because it uses the most recent non-failed build.

???

Owner of the canary build is now shunned.

[lostcolony]

I think we must have very different expectations as to what a contract violation is.

[tomc1985]

I mean, that isn't exactly hideous code. Sometimes you need to check a variety of different conditions in sequence, and I don't think a switch statement works here.

So what if they're different types? It's not like your passing those variables to functions right there. And are we really so robotic that we can't understand different types in a conditional?

Except the recursive function bit. Why bother if its always false...

[akerro]

> It's not like your passing those variables to functions right there

Yes it did. Case blocks had several lines.

>So what if they're different types?

It makes it hard to read and destroys expectation of what possible cases there are. It makes it hard to test as a lot of test preparation/mocking is necessary.

>Except the recursive function bit. Why bother if its always false...

Yea, that the point! Because there was no test for it

[tomc1985]

Wait, so what's the remedy for the different types issue then? Should he coerce all these diverse types into one type just to do the comparison? That just seems wasteful.

Assuming he has to do the check that way, of course.

[theflyinghorse]

Better question is "Why are they gating all this crap here like that? What is the presumed set of expected outcomes?". This code is bordering somewhere on the line between sabotage and incompetence and it smells really bad.

[akerro]

My solution was to change the checks into single methods and move one god-decision-making-method into classes that are components with their own logic.

[hedberg10]

> I mean, that isn't exactly hideous code

It is. When you read it in isolation here, it's fine. If you have to read pages and pages of it and you have to concentrate/have half your brain working on the logic of the code, you don't want to annoy your brain with details like these.

[specialist]

I gently encourage you to dust off your copy of the CIA Sabotage Field Manual and set about getting your tormenter fired.

[lmilcin]

At least he didn't put each one of those statements in separate classes to make you hunt for half a day to figure out what his code does.

[kazinator]

Is there a bug in there? What's a better way to structure those tests, if they are logically valid?

[IggleSniggle]

Depends on the actual situation, but just from looking at it I would guess that named functions instead of nebulous unrelated nested branches would be a start.

Edit: it’s also just kind of a code smell that suggests the overall structure is not well thought out, with that particular collection of tests. Could be fine in that respect in context tho

[akerro]

As far as our manual testing goes, there was no bug in it. But also there were no tests for it. Writing tests for it is horrible. You need to prepare test for each switch/case branch and it adds a lot of mocking, faking data, setting up variables...

[joshxyz]

Fucking hell that's a nightmare haha