This week's book giveaway is in the OO, Patterns, UML and Refactoring forum. We're giving away four copies of Refactoring for Software Design Smells: Managing Technical Debt and have Girish Suryanarayana, Ganesh Samarthyam & Tushar Sharma on-line! See this thread for details.
Hi guys, I'm in the middle of making a portfolio for some Java job applications. I had some quite harsh criticism from my last job interview regarding my code!!
Below is a link to a 5 minute video of a project I have been working on. I talk through what the program does and recview the code.
I'd love to get some brutally honest feedback on the quality of the code and the program in general.
1) You are missing a try/finally block to close your reader.
2) Why did you decide to use Regular Expressions rather than an HTML Parser open source library?
3) Variables should begin with lowercase in Java (WebPages)
4) You catch an Exception, print it out and move on. Is this not fatal?
5) a + " " + b vs new String(a + " " + b). There is no need to call new String. The + is already concatenating into a String
I like that you mention opportunities for change.
Keep in mind that most employers won't watch a 5 minute video. You may want to have a printed version as well. The code and a few bullet points?
I like the way you present the code. It shows that you organize your thoughts well
Few things that I would do is
a) Read the list of URLs from a property file. This way you you don't have to change the code everytime they change the URL, or add a new one
b) Why is HTMLReader full of static members? You can easily make them non static right? Basically, you will have to create an instance of HTMLReader and call the methods on the instance
c) I would seperate the business logic from the data; ie; instead of putting the results of HTMLReader inside the HTMLReader, define a bean that represents the results, and have HTMLReader return the bean
Both of the above changes will help extensibility. Right now you have the same HTMLReader for every website. However, tommorrow if you have to define a custom HTMLReader for a particular website and you had the above 2 changes, you could simply extend HTMLReader and put the custom implementation in the extended HTMLReader.
Thanks guys! Not as brutal as I thought it would be! I recently failed a job interview programming exercise (spectacularly) and was starting to get a complex! Still, I guess we learn more from our failures!
Jayesh mentioned to have a 'bean' represent the results rather than have the front-end access them publicly from the back-end.
Should I make the back-end HTMLReader class itself a bean?
Fraid I have some more negative criticism. The presentation isn't brilliant. Too many ums and ers; you ought to have created a script and read it. I found the continual zooming in and out difficult to follow, and the code appears blurred on my screen.
I would have preferred some explanation why you used Applets, which many people consider obsolete.
Looking at this code, you might have failed the other coding test because of non-standard coding conventions and incorrect exception handling.
Non standard coding conventions are not harmful, but they just annoy people. Depending on how the other's person day is going, non-standard coding conventions might make amuse them or anger them. Coding conventions are kind of a shibboleth among programmers. Usage of non standard coding conventions marks you as an outsider. It might bring up all sort of bias filters in your interviewers head.
Incorrect exception handling is something that a lot of senior programmers take very seriously. See, it's very tough to catch mistakes like "catching an exception and ignoring it" during testing because testing usually happens ina semi-controlled environment, and it's hard to reproduce all exceptions there. OTH, Murphy's law dicatates that you are guaranteed to see every kind of exception atleast once in production. When there is incorrect exception handling, you get weird behavior, and the first time you see an exception is in production, you see that weird behavior in production. Another side effect of the way most software development organizations work is when there are weird problems in production, the senior developers are called in. Junior developers don't get called in because management is looking for a fast resolution. Managers go back to sleep because hey.. once they get senior devs on it, their job is over. The senior devs stay up till late night to find the root cause which often turns out to be mishandled exception. or it might be a problem whose root cause is outside of the application, but because the application "ate" up the exception, it took much longer to find the root cause. Ask any developer with more than 5 years of experience and they can tell you war stories.
So, tl - dr; Don't eat exceptions. Either handle them or report them, or re throw them. The people who are interviewing you are exactly the ones who will pay for your mistakes in exception handling. So, it's in their own interest to kick out any candidate who doesn't do exception handling well.
Campbell Ritchie wrote:I found the continual zooming in and out difficult to follow, and the code appears blurred on my screen..
I liked the zooming. It's better than scrolling through the code
Edit: Maybe it;s just me. I tend to mentally zoom in and out of the code too. Look at the structure.. hmm function names foo, bar and bara.. nice.. lets's go into the entry point.. ahh it calls these 2 other functions.. zoom out..zoom into the first sub function.. repeat
Joined: Oct 13, 2005
No, I found the zooming difficult to watch, and disorienting.
1) It would be better to compile the reg exp outside the loop as it doesn't change.
2) You don't need the parens around matcher.group()
3) Why do you have boolean b instead of just matcher.find() in the while loop?
4) Why are you using | rather than || in the if statement?
1) Your are still catching and ignoring the errors. It is usually better to end the program if something fatal happens. In the for loop you at least try to do that. But return just exists the for loop. It doesn't appear to end the program.
Regarding setArticle() points 1 to 4...noted! I guess my mindset has been that so long as the program functions, to leave the code alone.
But you make some good points that make the code more readable, efficient, and pleasing to any would-be employers!
About the OP class and exception handling, I suppose the exceptions that I catch, I consider 'fatal', as they would render the program unusable.
How should I end the program in the catch block? System.exit(0)? throw RuntimeException()? I have looked into this on different forums
and there seems to be a wide range of opinions on the matter. How do you think I should terminate it?
author & internet detective
There's actually a term called refactoring. It means to improve the code without changing the functionality. That's often good to do for making it more readable. And something you should absolutely do here. The fact that you are showing this off to employers, implies it is the best code you can write. So you should make that so . Also, an employer who reads it might ask you why you did certain things. So if you decide not to do them, you should know why. (For example, you didn't use an HTML Parser because you wanted to practice regular expressions.)
I would use System.exit(0) to end the program. Or omit the URL with the problem and continue on with the others.
I have a few minor points:-
You know about adding to the constructor of a Reader? Did you know that is called the Decorator pattern?
Did I see you using the == operator on Strings? Or was I mistaken?
When catenating Strings, did you consider a StringBuilder?
Nice example of a properties file, but should you call it hard‑coded when they change it every week?
Use System.err for error messages, not System.out.
Don't use System.exit(0) in a catch, because the 0 represents normal termination without problems.
Thanks for the tips! Interesting about StringBuilder. I was considering that originally, but was advised by...not a guru, but certainly an experienced developer that StringBuilder would have more overhead. My original logic was that Strings being immutable would create more objects than StringBuilder and therefore have the bigger overhead. I got brainwashed by his authority and went with Strings!! At the moment, the Applet takes 1min10s to load! I wonder what would happen to that load time if I switch to exclusively StringBuilders?
Joined: Oct 13, 2005
Billy Sclater wrote:Thanks for the tips!
. . . but was advised . . . that StringBuilder would have more overhead. . . .
What rubbish. StringBuilder is designed for fast performance and multiple strings often have much more overhead. As a rule of thumb:
You can use the + operator on Strings as often as you like as long as you are in the same statement.
If you are catenating Strings or similar in multiple statements StringBuilder is probably faster.
You were actually using String#format, which works differently. It uses a Formatter object, and you might do well to stick with that because you get simpler code. Particularly if you need formatting as you create the String.
. . . the Applet takes 1min10s to load! I wonder what would happen to that load time if I switch to exclusively StringBuilders?
No idea why the applet took so long to load. I suspect it has nothing to do with the String catenation, however.