| « Prev | Page 1 | Page 2 | Page 3 | Next » |
No, probably not. Normally you just put that in a finally block at the very beginning and that takes care of it. This guy probably didn't know any better. |
|
Closing connections, opening connections... Who has the time for such nonsense? Just keep them open!
If connection limit is reached, a) raise the limit b) add new hardware and c) do some magic. |
|
Fixed?
Lipstick on a pig |
If fifteen minutes is just a "shave" off your commute, your commute is way too fucking long. |
|
Not bragging, but I'm working on an application whose lead developer insists on writing code just like that... from "don't close connection" down to "we use datareaders for scalar stuff" and "stored procedures are for sissies".
Captcha: immitto. Those guys are imitating us fo sho. |
Better but not what I'd called fixed, still having thought about it let's keep it slow and sluggish, making them open a connection every time and run 11 SQL queries. We have to give them some incentive to upgrade, I suppose. |
Re: Swallowed by the Beast
2010-08-23 09:35
•
by
Mister Cheese
(unregistered)
|
...except you screwed the logic and return true if there's at least one row in any of the tables, instead of at least one row in all of them... |
|
Surely TRWTF is that a function named "isWidgetReferenced" is actually returning 'available'? - that is, whether the passed widget number is NOT referenced?
|
|
Who knows, maybe getConnection() enables some kind of connection pooling? That said, the fetching is kind of weird.
|
So if count is greater than zero, the widget is not referenced? What, do the tables keep track of places where the widget doesn't occur? |
Re: Swallowed by the Beast
2010-08-23 10:00
•
by
kjordan
(unregistered)
|
|
No, if it's greater than 0, it means that the widget is not available.
|
Re: Swallowed by the Beast
2010-08-23 10:07
•
by
honnza
(unregistered)
|
good point |
|
If all you want to do is checkj for the existence of at least on occurrence of a value in a table then you'd usually do well to do something like (in Oracle):
select Could save you do all the work to count the other ten million rows that you don't care about. Also, "(0 < count)" ... is that irritating to anyone else? "(count > 0)" seems a lot more intuitive to me. |
|
Does Java still not support the "using" pattern for disposable objects? I'm no fan of pointless syntactic sugar but this pattern would be a useful addition to the language. I like Java a lot but in all honesty I'm grateful to be working on .NET these days.
|
We've been waiting for Java 7 for quite a while now. It'll have something similar to "using" for automatic resource management. |
Re: Swallowed by the Beast
2010-08-23 10:19
•
by
The Nerve
(unregistered)
|
That sounds like a neat feature, but I don't think there's plans to incorporate anything like that in the future. That said, it's not necessary these days to manually close connections for most applications: they're moving toward dependency injection (IoC pattern) for the pooled resources. |
Doing it that way may have really bad performance on some databases if you use transactions, because then count(*) is not a simple operation. (You can't just return the number of rows because an other transaction may have a lock on some of them and thus delete them (This prevent the database from just returning the number of rows)). So I think that doing a select with limit 1 and then testing for an empty result is a better solution. |
It supports finalizers for objects, not quite the same as destructors that you have in C++. The actual method in which connections are handled here should be transparent, but when you let go of your object it should either close the connection or return it to the pool for reuse, dependent on which method was used to retrieve it. The final object's finalizer can be implemented to do exactly that and you won't have to "remember" to do it yourself. Of course, the problem is you don't know exactly when this will happen, i.e. it is not guaranteed to happen as you leave scope, unlike in C++ where it is. |
Re: Swallowed by the Beast
2010-08-23 10:25
•
by
Slim Dave
(unregistered)
|
Yep, quite possibly you're right -- this is an Oracle-specific solution, and Oracle would stop retrieving rows once the first has been found. And readers don't block writers in Oracle either of course. Syntactical and locking differences -- another reason for using stored procedures? |
Re: Swallowed by the Beast
2010-08-23 10:27
•
by
Larry
(unregistered)
|
TRWTF is that people who don't know to type finally {
will probably not be educated enough to type The code is only as smart as the smartest idiot. |
Re: Swallowed by the Beast
2010-08-23 10:33
•
by
anon
(unregistered)
|
I was thinking the same thing... there's nothing like trying to puzzle out the real meaning of an "opposite named function". 'commoveo' - commoveo and fix this function name you idiot! |
|
TRWTF (although a terrifyingly common one) is catching the base Exception class and treating all failures as the same.
In this case, if the database is down, the code will act as if it has found a widget in the database, which could have all kinds of interesting unintended consequences in the logic down the line. |
Re: Swallowed by the Beast
2010-08-23 10:35
•
by
Henning Makholm
(unregistered)
|
No. |
Re: Swallowed by the Beast
2010-08-23 10:37
•
by
Anonymous
(unregistered)
|
Yep, that's basically it but they've opted to extend the try..catch construct instead of implementing a new keyword. It's all just syntactic sugar and I must admit I agree with the Java philosophy that syntactic sugar is bad (the proposal notes: "like all syntactic sugar, this construct removes a bit of Java’s 'what you see is what you get' character"). But it's a small change that offers big returns in terms of code quality - if it's actually used...! |
|
From the article:
Well, it's a funky way to do a while loop; from what I remember, this is actually how the JVM implements a while loop. As in regards to the other comments, about Java not having any destructors and such: true, it could do with those, but apart from that, JDBC is a bit of a mess when it comes to design. You always end up with an enormous amount of boilerplate code just to do a simple query. |
Re: Swallowed by the Beast
2010-08-23 10:43
•
by
Larry
(unregistered)
|
TRWTF is that Java does not have a while loop, which would do the same thing without all the extra punctuation. |
Re: Swallowed by the Beast
2010-08-23 10:51
•
by
Anonymous
(unregistered)
|
It's been a long time since I've done Java but even I can tell you that it definitely has a while loop! |
Re: Swallowed by the Beast
2010-08-23 10:55
•
by
Henning Makholm
(unregistered)
|
It is not a while loop written using the for statement. It is an if statement written using the for statement. See the unconditional break at the end of the body? |
Re: Swallowed by the Beast
2010-08-23 11:35
•
by
The Web is the Root of All Info
(unregistered)
|
Or you commute is that unpredictable. My commute? 20 min. if you hit every green light along the way, 90 min. if everyone else decided to commute at the exact same time as you. |
Re: Swallowed by the Beast
2010-08-23 11:48
•
by
wtf
(unregistered)
|
Hmm. My commute is about two chapters of a typical science fiction novel. I love public transit! |
Re: Swallowed by the Beast
2010-08-23 12:02
•
by
Mason Wheeler
|
Standard Yoda Conditional for you. The people who invented C had more marketing skill than technical competence, and they made a compiler that thinks anything can be a boolean and called it a "feature." And now all the descendant languages are stuck with it and all the ugly syntax that it spawned, such as writing conditionals backwards. |
Re: Swallowed by the Beast
2010-08-23 12:03
•
by
Another Public Transit User
(unregistered)
|
My commute is normally the time that I like to shave with my portable razor, brush my teeth (I use a water bottle for my rinse), and apply deodorant. You've probably met me if you ride the bus enough. |
|
TRWTF is that nobody has recommended a database view. It will certainly simplify the process of adding the missing table references by not requiring a new build/deploy.
|
Re: Swallowed by the Beast
2010-08-23 12:14
•
by
North Shore Beach Bum
(unregistered)
|
How would my luck with green lights / competing traffic affect your commute time? Under perfect conditions, my commute is 15 minutes; if the lights conspire against me and I hit rush hour, I'm looking at 90 minutes. Thankfully I'm paid for getting the job done, not for occupying a desk 40 hours a week. |
Re: Swallowed by the Beast
2010-08-23 12:24
•
by
Anonymous Coward
(unregistered)
|
He's also not closing the result set or the statement. |
|
Actually the issue was that they used = to mean assign and == to mean compare so coders may accidentally put
but it will compile, assign x to 0 and evaluate to false and fail at runtime if the coder meant
Therefore to avoid this error someone came up with the idea of forcing people to write
so that if you forget and accidentally write
it will fail at compile time as you can't assign 0. It may have been a better idea in hindsight to use := for assignment and = for comparison which is perhaps more intuitive although coders would still have got it wrong, probably using = when they meant to assign, and most likely would still get code that compiles but fails to work. |
I'm a fan of strategically used syntactic sugar, but the "using" keyword/clause could possibly do more harm than good in this particular context. The reason is that an idiot/incompetent programmer will still be an idiot/incompetent one with or without it. Let's look at some of the original codesod: rs = connection.createStatement().executeQuery(query); It is not only the creation of a java.sql.Connection object, but the creation of a great many number of java.sql.Statement (we only need one) and many java.sql.ResultSet objects, with none of them being cleared, re-used or closed. Imagine the dumbass who wrote that code being equipped with a Java equivalent of "using". Assuming his pineapple-level IQ gets him to realize "shit, I need to close them thingies", wrapping everything in "using" clauses, we would have: 1. Even more redundant, unreadable, obfuscated code, and 2. Would not compensate for the dumbass' inability to understand basal things like object indefinite lifetimes in Java or the fundamental fact that resources have cost and are limited. Adding "dispose pattern" syntactic sugar will not help here because the problem is deep, systemic and systematic. Syntactic sugar is like neosporing, and dumbass programmers are like skin cancer. You can't use the former to effectively treat the later. |
Re: Swallowed by the Beast
2010-08-23 12:40
•
by
boing boing
(unregistered)
|
if((moron > idiot) || (idiot < moron)) { . . . } |
Well, to their credit, the ability to evaluate things to "0" and evaluate it as false comes very handy for pointer arithmetic and manipulation. I highly doubt that the people who write conditionals backwards do it for a C influence. I'd put on a lack of analytical skills and subpar understanding of code aesthetics (CS stuff that no one should be allowed to graduate without.) Put a folk like that under stress till they go way over their head, and voila, you get code like that in no time. |
I put my finally blocks at the end. But that's just me. |
Re: Swallowed by the Beast
2010-08-23 13:03
•
by
Scott Selikoff
|
Bug: You never called rs.next(), as required in JDBC to read a record from a result set. Normally, rs.next() is called inside of a while() loop for multiple records, and inside an if() statement for a single record. In this case, the if() statement is not required to call rs.next(), since you expect exactly one row every time (even with 0 records, the query should return a single row with value 0), although it is good practice to always wrap rs.next() with a conditional clause to avoid reading an invalid result set. |
Re: Swallowed by the Beast
2010-08-23 13:10
•
by
EngleBart
(unregistered)
|
You just broke the API and all of the unit tests will fail now. The code as posted returns true only if the Widget is NOT referenced. I think the method was written on opposite day. Alternative theory, the method was originally isWidgetUnrefereneced() but someone pointed out that it should be named to reflect positive logic in accordance with the project coding standards. The developer renamed it without inverting the result (so s/he would not break the unit tests). |
|
Funny, one of the first things I noticed was that the method isWidgetReferenced returns true if the widget ISN'T referenced (and is therefore available), and false if it IS reference.
|
|
Frits is right; the advisablility of moving the multiple table lookups into a union really depends. For instance, the code short-circuits lookups if count != 0 on any given lookup. The database will not short-circuit the union.
For code clarity, if the db side of it was fast, I would throw it all into a union. If I needed performance, I would use multiple, short-circuiting lookups as was done here. But I would never use a for "loop" in that manner. |
|
This code needs a FOR-CASE statement. ;)
Captcha: sino. Code looks much better, si? No. |
Re: Swallowed by the Beast
2010-08-23 13:28
•
by
EngleBart
(unregistered)
|
|
Java (and I think dotNet) require the if expression to be a boolean (bool).
if (x = 5) will evaluate to an int and fail at compile time. However boolean answer = f(); if (answer = true) would still suffer from the same problem, assign true to answer and then evaluate to true. Of course, with booleans, I always think the comparison operator is unnecessary. if (answer == true) can be changed to if (answer) if (answer == false) can be changed to if (!answer) I am pretty sure that James G. would never write if (answer == true) over if (answer) and probably did not even consider it when designing Java. |
Re: Swallowed by the Beast
2010-08-23 13:34
•
by
Henning Makholm
(unregistered)
|
Your version has material code inside the loop that is executed in more than one iteration. Therefore it is not the true for-case pattern. The utility of "switch instead of a constant array" is in general much more debatable than the canonical for-case construction. |
Re: Swallowed by the Beast
2010-08-23 13:37
•
by
EngleBart
(unregistered)
|
Anyone we learns to read right to left first will not have a problem with this. I worked with a Jordanian who had Arabic as his first language. He called "<" "greater than" because he learned it from right to left in his elementary math class. It made perfect sense as long as he read the whole clause right to left. The problem was he would read the clause left to right, but still read the symbol as learned right to left. bidi bidi bom bom |
|
If the database was properly normalized with cascading updates/deletes, then you should only have to check the primary widget table.
Failure to exist in the widget table should indicate failure to exist in the entire DB! Of course, from the code seen here, this developer may have still messed it up. |
Re: Swallowed by the Beast
2010-08-23 13:45
•
by
Kevin
(unregistered)
|
Yes, this is much neater, much quicker, and still as completely wrong as the original (and all proposed variants I've seen). This whole thing is a ticking time bomb, just add users to speed up the clock. 8:30 a.m. Sally checks if WIDGET_ID = 5 is available. It is. 8:31 a.m. Tom checks if WIDGET_ID = 5 is available. It is. 8:32 a.m. Sally adds WIDGET_ID = 5 to some WIDGET table. 8:33 a.m. Tom adds WIDGET_IT = 5 to some other WIDGET table. Uh-oh. This is one of those great bugs that remains hidden during all unit testing, is completely unseen during the initial rollout to a small user base, and blows up in your face on the fateful afternoon all 250 of your users are scrambling to add their widgets before month end (you know, the one time you really, really do not want your application to blow up). Correct solution: when you need a new WIDGET_ID, get it from a sequence-generator (in Oracle, <sequence_name>.NEXTVAL). |
| « Prev | Page 1 | Page 2 | Page 3 | Next » |