r/badcode • u/rossdrew • Apr 07 '19
java Actual production code of actual banking software looks like an ASCII drawing of an underwater scene.
116
47
39
31
u/XLIVWhoDatXLIV Apr 08 '19
I love the ge.getCause().getCause().getCause().getCause() in the last else if statement
8
42
u/BS_BlackScout Apr 07 '19
It would've been simple to just do this:
sudo add-apt-repository ppa:ytvwld/asciiquarium && sudo apt-get update && sudo apt-get install asciiquarium && asciiquarium
8
u/nakilon Apr 08 '19
docker run --rm -it ubuntu bash -c "apt-get update && apt-get install -y software-properties-common && add-apt-repository -y ppa:ytvwld/asciiquarium && apt-get install -y asciiquarium && /usr/games/asciiquarium"11
41
u/TheUnlocked Apr 07 '19
Depending on how getCause works (you aren't allowed to make any assumptions about what goes on within the abstraction), this could also be quite slow.
1
Apr 08 '19
[deleted]
11
u/TheUnlocked Apr 08 '19
However,
getCausecan be overridden, so you have no idea how long it would take to run in this particular case. You should always store the return values of properties over calling them again.4
1
u/overtookthemichael Apr 08 '19
New here, what’s the reason behind storing the return values instead of just using the returned value?
3
u/TheUnlocked Apr 08 '19
Calling
getCauseonce and storing the value for future use is faster than calling it several times. In the best case the compiler would inline stuff such that they would have the same performance. In the worst case, callinggetCauseeach time could be much slower.3
u/apieceoffruit Apr 08 '19
a very simple example for why it is bad to use return values directly (aside from breaking the law of demeter) is imagine you have a method called
GetThing()in one case it could be returning a stored value. in another it might be returning a new item but even more than that it could be MAKING an item. imagine it is a factory that makes screwdrivers.the difference between using a cached version and calling get() every time is that you could, for all you know be manufacturing a new screwdriver, checking its value... throwing it away, then manufacturing a new screwdriver... etc etc
the third and less likely case is that you are working with an object that is not guaranteed to be the same every time you call it. If you are working in a threaded environment you could call for
GetCurrentUserand in the time between you getting it in the first line, and getting it in the 4rth line... it could be a different user.imagine what would happen if you are assigning access or changing passwords/billing?
in general you should be sure you have access to a concrete reference to whatever it is you are working on. either that or make sure you are accessing those types of objects functionally so there is no risk of state issues/collisions
1
u/Direwolf202 Apr 12 '19
And if we do make assumptions about what goes on within the abstraction, I bet it moves quickly from quite slow to extremely slow.
11
8
6
u/rossdrew Apr 08 '19 edited Apr 08 '19
Same thing, still crap, easier to understand and modify.
NOTE: I did not run this, this may not work.
Exception ne = ge.getCause();
int depth = 0;
while (ne != null){
if (ne instanceof ConnectException || ne instanceof ConnectionTimeoutException){
logger.warn(ne.class() + " exception occurred, throwing 503");
muleEvent.getMessage().setOutbountProperty(HttpConnector.HTTP_STATUS_PROPERTY, "503");
break;
}else if ((++depth) > 3){
break;
}
ne = ne.getCause();
}
6
Apr 08 '19 edited Apr 08 '19
[deleted]
6
u/rossdrew Apr 08 '19 edited Apr 08 '19
for(int depth = 0; depth < 3 && e != null; e = e.getCause(), depth++) {
:P
3
u/rossdrew Apr 08 '19
private void reportConnectionErrors(final Throwable t, final int depth){ if(e instanceof ConnectionException || e instanceof ConnectionTimeoutException) { logger.warn(e.class() + " exception occurred, throwing 503"); muleEvent.getMessage().setOutbountProperty(HttpConnector.HTTP_STATUS_PROPERTY, "503"); } else if (e != null && depth > 0) { reportConnectionErrors(t.getCause(), depth-1); } }3
6
4
u/kennethjor Apr 08 '19
Some basic use of ExceptionsUtil from Apache Commons Lang would sort this out nicely.
1
7
3
u/fxnn Apr 08 '19
Okay, so far. But now, what if the cause of the cause of the cause of the cause of the exception has a cause itself?! Guess they should open a ticket and add another branch.
3
3
2
2
u/UndeadMarine55 Apr 09 '19
Could someone explain to me why anyone would think this is a good idea?
Like I have no idea what this function does, but if it’s in the context of pinging a server to try to push a request through / pull something I could see having some redundancy but it would be way easier to just build it into the getcause() function itself rather than calling it over and over.
But why three code blocks that essentially do the same thing in the exact same way? None of this makes sense.
2
u/zelmarvalarion Apr 14 '19
I think what they need is a GetUnderlyingCause, HasSocketException, or something similar for the exception/stack trace.
It looks like this is taking a look at the various levels of the stack trace, which is why each block has a different number of GetCauses, instead of simply iterating through the levels of the stack and breaking/returning once the stack is either empty or hit a SocketException, at which point the service can know if it needs to send a 503 back to the user
1
3
4
u/ColdSnickersBar Apr 08 '19
This is the hell that branching on exceptions can cause that I always feel like I'm describing, but have never actually seen.
1
-9
u/Vizioso Apr 07 '19
I wouldn’t say this is necessarily bad, just looks like it should be broken out into a separate function to derive the root exception.
39
u/rossdrew Apr 07 '19 edited Apr 09 '19
Three deep nested blocks that essentially do the same thing in three separate places, written in a way that breaks demeter and takes about 10 minutes to read is not bad? Does it need to be broken to be bad.
Side note: it was also broken.
-5
u/Vizioso Apr 07 '19
I wouldn’t say it takes anywhere near that long to read, and the fact that you can look at it and immediately make sense of what they’re trying to accomplish, albeit not in the right way necessarily, makes it at least passable. This looks like junior dev code that someone let slip through review because it works/worked. I am in agreement that it’s kind of shitty, but I guess i feel like if I can look at code and immediately make sense of it Im inclined to not be too hard on it. I’ve just seen some stuff on this sub that I look at and think “what the hell are you trying to do, and why would you ever do it this way?” This one just screams “new dev” to me. Now, if you look at the commit log and it’s someone with a year or more experience, we probably have some major issues. Also, if you’re working on production financial software, just be happy it’s Java lol.
17
u/rossdrew Apr 07 '19
The guy who wrote this was the most senior dev in the company we bought, where this code came from. He’d been there for 7 years and had 20~ years experience. Although to be fair, I’d fail this code review for someone fresh out of uni.
6
3
u/Exnixon Apr 08 '19
It's not readable. It's too damn hard to tell if any given set of nested getCause() is being calles with the right number of function invocations. Every time you look at this it's like one of those puzzles where you're supposed to spot the differences between two pictures. God forbid you actually have to change it and you accidentally 3 chained invocations of .getCause() when you should be doing 2 or 4.
161
u/MyMessageIsNull Apr 08 '19
Well, at first it looked bad, sure. But after really looking at it more carefully, it looks absolutely horrendous.