- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Edit Admin
Frist off, it seems pretty clear this code got through every review it was exposed to. All zero of them.
Admin
Frist off... now there's a nice invective!
Frist off, ++!
Edit Admin
I admire the happy world you live in, where code reviews are actually meaningful and people care about non-optimal code --- it works, it gets approved. No one cares if it could be improved, there's a new sprint to worry about.
Admin
Don't forget that the original coder forgot to release resources in a finally block. I bet you they have IO issues due to the application keeping so many files open.
Edit Admin
They also didn't pass a character encoding parameter into FileReader, which means it's using their system's default encoding, whatever that is. To be fair, that'll probably always work for reading urls, but why rely on a "probably" when you could easily have a "definitely"?
Edit Admin
That's "zero++" to you, sir.
Edit Admin
I'd bet this code predates Java 8 significantly - the choice of using a Vector over and ArrayList and the use of the Enumeration type instead of Iterator gives a whiff that at least the tutorial or book they were using predates Java 1.2
Edit Admin
@Balor64 ref
Unless they also explicitly control the encoding used by whatever app & process to write that source file, choosing a specific encoding is no more "definitely" successful than is the system default.
if we want to assume the file is created on that same server by some process, leaving the encoding unspecified is probably the safer bet. Doubly so if this is software that's sold to multiple customers who might be anywhere on Earth. We'd like to believe Unicode solved all that, but it didn't completely.
Admin
Where we're going, we don't need close().
Edit Admin
I think I'd prefer it if they explicitly specified
Charset.defaultCharset()to prove that this was their definite intent and that they hadn't just forgotten about it.Admin
Why did they count the records with
recCnt? Was it because they didn't know that there's a method for that?Oh, wait a moment.
I see
vURLs.size()later on...Edit Admin
Admin
The article's first sentence immediately reminded me of this: "I will take these cotton balls from you with my hand, and put them in my pocket." So I will take that code from you with my hand, and put it in my pocket...
Admin
Agreed, the array conversion is certainly a relic of the pre-generics era (so 1.4 or earlier), and I've not seen new code written using Vector and Enumeration in 20+ years... the latter occasionally comes up with old bad APIs (bits of the servlet spec), but the former is pretty obscure these days. This has to be early-2000s code... maybe even late 90's...
Edit Admin
Pff, why close a Fille? It's Java, objects are garbage collected! /s... (till you get a "senior" Java developer saying that seriously and you just want to go cry silently as far as possible).
As for Charset, you folks all seem to expect whoever wrote that "thing" even knows what an encoding is. I wouldn't bet on it.
Edit Admin
"Pff, why close a Fille? It's Java, objects are garbage collected! /s... (till you get a "senior" Java developer saying that seriously and you just want to go cry silently as far as possible)."
Oof - that comment from a senior dev would send me into a spiral wondering how many files out there were missing lines of text because he didn't close the BufferedWriter and what the impact on processing would be. Hopefully no major accounting errors because of it!
Admin
I also like that they created the enumeration as
eUrlsabove the for loop, but then re-create it asefor theforloop.