This week's book giveaway is in the OCAJP 8 forum. We're giving away four copies of OCA Java SE 8 Programmer I Study Guide and have Edward Finegan & Robert Liguori on-line! See this thread for details.
I need honest opinions about my sites, both in terms of presentation and demonstrated skill level. I will never have a better oportunity with so many web developement professionals here. Especially you Frank since you do server side development and are a fellow role player to boot(D&D rules!). I realize some shortcomings and I will list them: at both sites 1)html mixed upper and lower case. I now know lower case and use of quotes as per XML(XHTML) is preferred. 2)I didnt make effective use of subdirectories(you might not notice but I know) the rest are at the Magic Shop site 3)too many hoops for user to jump through 4)really bad blown up image that loads slowly 5)member variables that should be local(most likely) 6)indiscriminant use of String(no StringBuffers) 7)SingleThreadModel probably not scaleable 8)"sports fans enter here" should probably be deleted but I am loath to do it. 9)I will probably get rid of the brainbench certificate gifs 10)database entry is incomplete. in particular the weapons and armor section. the source code of the servlets of the Magic Shop is available at my angelfire site(bottom of page or click the "My Resume" link). source code is also available from javaguy.yi.org(the magic shop site) but you have to fill out a form and I have to authorize you then email you. so get the source from the angelfire site. these are my first 2 efforts, I really need some feedback. Thanks in advance PS: ShoppingCartServlet is perhaps the most interesting. It worked out really slick the way I can call it with doGet() or doPost(), and I do call it both ways.
[This message has been edited by Randall Twede (edited January 22, 2001).]
This isn't really moderator specific. Are you willing to open this discussion to everyone in the Servlets forum? You might get some odd or harsh comments, but you'll get a lot more people looking at your sites, and many of the visitors are very skilled and thoughtful. If you say yes to this, I'll move the thread and add a long reply I'm currently writing off-line. ------------------ Frank Carver JavaRanch Moderator/Administrator
sure Frank. I just posted here because your forum is so full of experts. If you think I will get more responses that is great. also I accept your bribe that you will give me your evaluation.
Joined: Jan 07, 1999
OK, here's some initial observations, please don't feel I'm digging at you - you did ask for comments! Also bear in mind that these are just my opinions. Others may have wildly different ideas. Please note that these are just a selection of comments , not a complete full review (I can't really afford the time for that) - at least it should give you something to be going on with. 1. Site design: + Lose the "under construction" it's a sure sign of a web newbie. Professional web designers know in their bellies that a web site is always under construction and just make sure that what is there is consistent and of high quality. + You really have to think about the look and feel of the sites. The Magic shop in particular has a different style for every page which is very offputting. Choose a single background, font and layout style for all the pages in a site so a user feels "at home". The Angelfire front page is very "busy" and could do with a bit of consistency betwen sections. Surf a lot, and study the sites you like. + You are serving the site froma slow connection, so please rethink the use of those great big graphics. Particularly the store with those big strips of blurry room and no text to help you decide what it's about. + Your Angelfire front page seems to have some sort of animation on it which causes netscape to gobble up sysyem resources and slow my machine right down. This is not a good idea. Think twice, or three times, before including any animations or client-side dynamic content, particularly on a front page. Something like this will just drive people away. + You really need some sort of always-available site navigation. It's really hard to tell where you are in the sites and what else might be available. Look at any of the high-traffic sites and you'll see page headers which indicate where you are, and buttons to get you straight to other useful pages.
2. Shopping: + I kept getting "no database entry" for lots of the things I chose ("+1 dagger", for example). It's ok to have items not "in stock", but please give a different message. Users don't need or want to know about technical problems. + It's very hard to shop when the price for the items is not shown with the item name. Please consider displaying more information with the items in each section - at least price and number in stock, but consider other things like a description, delivery time etc. + Your shopping cart refers to "the left pane". It's much better to give the pane a heading (such as "Category List") and refer to it by name. This allows you to move your panes about later without having to change the text.
3. Java general points: + Please consider making your source code available as a jar file. It's not only quicker to download, but preserves filenames and extensions, and is much simpler for someone to grab (one right-click/save-as/choose-name as opposed to 10). + I guess I'm more used to my own style, and to the Java Ranch style which is fairly close. I find that your Java code seems very "bunched up". Please add some blank lines to help visually separate sections such as member variables, methods etc.
3.1 ServletUtilities.java + I assume most or all of this is unmodified from the original. However I would prefer a more descriptive name than "filter". This is normally known as htmlEncode, and has a partner htmlDecode which does the converse.
3.2 FileIO.java + You don't need the "extends Object", it's the default. + Comments without useful content are worse than no comments at all. If you are going to use JavaDoc, then fill in all the fields and make sure they are correct and up to date, otherwise just don't bother. Also, please reconsider comments which just express something built in to Java and familiar to all reasonably experienced programmers. Lose the comment on the constructor. + It's generally considered bad practice to declare more than one variable or member in a single statement as it limits readability and future flexibility for no benefit. + The member variables "string", "line" and "file" should not be members, as they are only used within specific methods. Declare them where they are used. + Again, think hard about names. The names you have chose for the FileIO class and its methods are not particularly obvious. How would a user guess that one difference between "append" and "write" is that "append" adds a newline ? + As I look through this class I do feel that you have got a lot more code here than is necessary. You don't need a BufferedWriter if you are just writing one string and then closing the file, and are you aware that all Writers have a "write(String)" method so you don't need to create a PrintWriter just to do a "print"? Study the standard APIs. + This class would fail a "Pure Java" check as it uses a hard-coded newline character rather than using System.getProperty("line.separator") + Your "write" and "append" methods are quite similar, consider refactoring them to separate common code (XP.s "Once And Only Once" (OAOO), or the Pragmatic Programmers' "Don't Repeat Yourself" (DRY) principle). + It's generally considered a bad thing to build up Strings using "+", especially in loops as this causes a lot of new Strings to be created then abandoned. Using a StringBuffer is a much better choice. To summarise, here's a comparison between the original and a suggested example rewritten version incorporating the above points: ORIGINAL CODE:
3.3 CheckoutServlet.java + bunched up, members should be local variables etc. as above. + I'm really worried that this servlet is SingleThreadModel. This is a really bad idea unless there is some sort of external resource you are accessing which is only single tasking. You should never use member variables in a servlet, for this reason. + Your "doPost" method is far too big. It tries to do too much. Split it up into calls to methods each of no more than 10 lines or so, and consider splitting some of it out into separate classes. + You have hard-coded the database details. These really should be externally specified (servlet init parameters are a good choice, as is JNDI, property files or whatever). I'd also recommend that you look into connection pools, either as code that you incorporate into your application, or provided by the servlet container. Managing database connections in a servlet is both inconventient and slow. + Putting all your code in your "doPost" method also has some subtle side effects. If you can't load the database driver, you just return from doPost without sending anything to the outputStream - this will cause an unsightly 500 error. If all that processing was in a separate method, you could still just return from it, but the main "doPost" could still continue to write out some sort of user-friendly error message. + I'm also worried by the somewhat messy mixture of Java and HTML. This says to me that you should be separating the two somehow, wither by converting all the database processing to a bean and wrapping it in a JSP, or by using a templating mechanism such as WebMacro, FreeMarker or XSLT to fill an external template with your values. + It's almost always good practice in a servlet to provide handlers for both doGet and doPost, even if one is just a call to the other. You never know when you might want to pass a form using GET in testing to see the values in the URL, for example.
3.4 DatabaseServlet.java + lots of comments as for CheckoutServlet. + Ugh! cut-n-paste code! The database connection code in this servlet is the same as in CheckoutServlet. You should never do this, particularly with a piece of code like this which includes so many things which might change. Ask yourself "how many places would I need to change and recompile if I changed the ODBC DSN or the databse driver or the database?". If the answer is more than one (hint, it is...) then your code is much less maintainable than it should be. Get this stuff out of your servlets and into a connection pool or at least into another class used by all the servlets. Let "Once And Only Once" become your mantra, and keep refactoring your code until you get there.
3.5 GetUserInfoServlet.java + all the usual comments. + Remember that getParameter() can return null, so don't risk NullPointerExceptions. My top tip is to always put the constant string first in an "equals" if you have one; don't say "if (name.equals(""))", say "if ("".equals(name))".
3.6 NameServlet.java, MyCookieServlet.java + all the usual comments
3.7 StartingBalanceServlet.java + I don't think most people have a "Calligrapher" font. Please at least provide an alternative, but ideally rethink why you need it at all.
I hope the above gave you something to think about. Now let's see what everyone else has got to say ...
thanks Frank(and anyone else who answers) for spending time to help me. We all like compliments but criticisms are probably more helpful. I am pleased with the new look of the main page http://javaguy.yi.org/examples/JavaGuy.html I removed 4 gifs and one link. It is a start. Now to get rid of that slow ugly blown up image. the original idea was ok but it doesnt work in reality. In IE the image does pop up tooltips but im not sure if it does in netscape. It is going anyway. note: you will not be able to actually enter the magic shop from the above link uless you already have a cookie. Use this link instead. http://javaguy.yi.org when you get to the page where the wizard asks you your name leave it blank and hit submit and see what his reply is.
[This message has been edited by Randall Twede (edited January 24, 2001).]