• Post Reply
  • Bookmark Topic Watch Topic
  • New Topic

What you think of this style?

 
Ivan Jouikov
Ranch Hand
Posts: 269
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I just wrote this nifty class, and I wanted to get some feedback from you guys. I wanted to know what you think of my coding style, and if you have any thought or suggestions - all welcome. Please be as rough as possible - the more criticizm - the better

Here's the abstract class:




Here's 1 implementation of that abstract:

 
Peter den Haan
author
Ranch Hand
Posts: 3252
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
A few things I noticed offhand.
  • I believe that you need compelling reasons to upgrade from an accepted standard. You don't seem to offer much rationale for SimpleBundle. I'm probably just thick, but exactly how does ResourceBundle prevent you from writing a database-backed implementation?
  • Since using the Spring framework, I've become an interface fetishist. If you are writing a framework and want others to be able to implement SimpleBundle, inheritance introduces too tight a coupling. IMHO SimpleBundle should be an interface, and you can turn the present SimpleBundle into an AbstractSimpleBundle convenience class if you wish.
  • You need to think about threading and thread safety and document it in the contract.
  • Your style is generally fine, except that IMHO G-d intended constructors to come first in a class.
  • [list]You're using more code comments than I generally encourage - ever since reading Code Complete I tend to prefer improving the expressiveness of the code itself over commenting it. Also, as a rule, comments should describe what the code tries to achieve rather than how it goes about doing it:Is a case in point; for a developer, the comment does not add any value to the code.
  • The formatting of your javadoc comments will not survive the javadoc process unless you start using <p> and <br> tags.
  • DEBUG is not a constant. It should not be capitalised.
  • Debugging output should use a logging framework such as Log4J, JDK 1.4 logging or Commons Logging.
  • DynamicResourceBundle.getString() does a number of different things and is much too large IMHO. The contents of this method could be a string of method calls that (without any comments ) provide a high level overview of what the method does.
  • You don't seem to provide any fallback functionality where a key that is not found in the most specific bundle is pulled from a less specific one; a crucial bit of ResourceBundle functionality.
  • - Peter
     
    Ivan Jouikov
    Ranch Hand
    Posts: 269
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    I'm probably just thick, but exactly how does ResourceBundle prevent you from writing a database-backed implementation?

    How? Method getString(String,Locale) is declared as abstract and is entirely up for you to be implemented, which means you have freedom to do whatever the hell you want. You can make it query the database. And method getSuffixes(Locale), works very nice with database, for instance, you could append those suffixes to the table names when queriyng, in order to find the most relevant table.

    IMHO SimpleBundle should be an interface, and you can turn the present SimpleBundle into an AbstractSimpleBundle convenience class if you wish.

    Hmm.. I am really into interfaces myself as well. I just thought Having an interface-like helper class would help out a lot. But I guess your proposal makes more sense. Thx.

    You need to think about threading and thread safety and document it in the contract.

    Good point. Should be as simple as synchronizing a given DynamicProperties...

    Your style is generally fine, except that IMHO G-d intended constructors to come first in a class.

    G-d? You mean GOD? Lol believers all around me. I am the only heathen here?

    Is a case in point; for a developer, the comment does not add any value to the code.

    Yah I am starting to notice I'm becoming a comment maniac. Thx for the doc.

    Debugging output should use a logging framework such as Log4J, JDK 1.4 logging or Commons Logging.

    Ahh you're right. It's about time I overcome my childish urges and step up to the big play. I had a bad experience using drugs back in 1969. Oh wait..... Point taken!



    You don't seem to provide any fallback functionality where a key that is not found in the most specific bundle is pulled from a less specific one; a crucial bit of ResourceBundle functionality.

    What are you talking about? getSuffixes and getString do just that!
     
    Maulin Vasavada
    Ranch Hand
    Posts: 1873
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Hi Ivan,

    I observed a below description in your javadocs,

    ----
    * I wrote it because I hated Java's implementation of resource bundles, and I
    * thought they were making it overly complicated. Also, they relied on classes
    * as resources, which I also didn't like.
    ----

    To me its not something I would put it in my Javadocs of API I am trying to build in the way it is put.

    Instead I would write something like,
    -------
    This class is supposed to be simplified smaller version of Java's resource bundles which seems complicated and relying on classes as resources.
    -------


    Well, its just my opinion. Its your API at the end

    Also, I tried to wrote little API recently for Infix2Postfix Conversion and Evaluation of Postfix. I have put it on Sourceforge.net at
    http://ji2pccae.sourceforge.net. See if you have some input on that...
    (btw, you would see the link in my signature as well)

    The sourceforge ftp thing doesn't seem to work for me properly so the zip file at project page at http://www.sourceforge.net/projects/ji2pccae doesn't seem to work after download so I had to create that little index page ...

    Regards,
    Maulin
     
    Max Habibi
    town drunk
    ( and author)
    Sheriff
    Posts: 4118
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Ivan, Maulin,

    Could you please reformat your post so it's about 80 characters per line? Otherwise, I'll have to do it, and I hate doing it.

    Thanks,
    M
     
    Ivan Jouikov
    Ranch Hand
    Posts: 269
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    I just copy pasted my code. You ever thought of setting fixed width for post cells, rather than having us count chars on every single line, and cut them off at the end?
     
    Jim Yingst
    Wanderer
    Sheriff
    Posts: 18671
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    You ever thought of setting fixed width for post cells, rather than having us count chars on every single line, and cut them off at the end?

    Nope. The abilty of a browser to format text to fit the user's desired window size is a good thing, more often that not anyway. And your overly-long lines would still be a problem in a fixed width environment, as they'd either be cut off, or would require a scrollbar for that panel. (But at least they wouldn't ruin the rest of the page anymore.) The current system works well for the vast majority of users. Formatting your lines to 80 chars or less is good practice in general, IMO, not just here. I know that there are many screens and editors that conveniently display more than that - but there are still many that don't. Not everyone sees your code in the same viewing environment you do, and it's worthwhile to to do your best to ensure the code looks good on everyone's environment, not just yours.

    Note - a good IDE is invaluable for this sort of thing. Eclipse or IntelliJ or others can easily reformat your code to a new maximum line length, with little effort from you.
     
    • Post Reply
    • Bookmark Topic Watch Topic
    • New Topic