What you think of this style?

[Ivan Jouikov]

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:

 /*
 * Created on 25.06.2004 at 0:10:48
 * Author: Ivan Jouikov ([e-mail])
 * Project: abLogic
 */
package org.ablogic.i18n;
 
import java.util.*;
 
/**
 * This class is similar to {@link java.util.ResourceBundle}.
 * 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.
 * 
 * Basically, this class will deal with internationalization (i18n), and help 
 * programmers make their applications locale-independent.  In other words, 
 * same thing as {@link java.util.ResourceBundle}.  You should read every 
 * SimpleBundle implementation's own documentations, because SimpleBundle 
 * allows complete freedom (unlike {@link java.util.ResourceBundle}), to have
 * your own way of accessing variables, and making them available in whatever place 
 * you wish (DB, files, user input...).
 * 
 * @author Ivan Jouikov ([e-mail])
 * @see org.ablogic.i18n.DynamicResourceBundle
 */
public abstract class SimpleBundle
{
/**
 * This method simply calls {@link #getString(String, Locale)} using the given 
 * path and {@link #getDefaultLocale()} locale.
 * @param path Where the string is located.
 * @throws MissingResourceException If we couldn't find the resource to get the string from.
 * @return String, associated with the supplied path.
 */
public String getString( String path ) throws MissingResourceException
{
return getString( path, getDefaultLocale() );
}

/**
 * This method would retrieve a string of text from a resource located at the
 * given path.  Every class that implements SimpleBundle must describe its own 
 * "protocol" for the path.  
 * @param path Where the string is located.
 * @param locale Locale used for this request.
 * @throws MissingResourceException If we couldn't find the resource to get the string from.
 * @return String, associated with the supplied path.
 */
public abstract String getString( String path, Locale locale ) throws MissingResourceException;

/**
 * This locale will be used in all {@link #getString(String)} requests.
 * By default, it will be the JVM's locale.
 */
protected Locale defaultLocale;

/**
 * Returns the default locale that will be used in all {@link #getString(String)} requests.
 */
public Locale getDefaultLocale()
{
return defaultLocale;
}

/**
 * Sets the default locale that will be used in all {@link #getString(String)} requests.
 */
public void setDefaultLocale( Locale locale )
{
defaultLocale = locale;
}

/**
 * Constructs a new SimpleBundle that uses JVM's locale as default.
 */
public SimpleBundle()
{
defaultLocale = Locale.getDefault();
}

/**
 * Constructs a new SimpleBundle that uses given locale as default.
 */
public SimpleBundle( Locale locale )
{
setDefaultLocale(locale);
}

/**
 * This is a helper method for your SimpleBundle implementations that will 
 * construct a sorted list of suffixes that you should try for your resource
 * whenever you're using {@link #getString(String)}.
 * 
 * For instance, if you implemented a file-based SimpleBundle, and you're working 
 * with a file name "Resource.txt", and the user requests some String with a Russian
 * locale, country set to Russia, this method will return list of suffixes similar to:
 * 
 * <ul>
 * <li>"_ru_RU"</li>
 * <li>"_ru"</li>
 * <li>""</li>
 * </ul>
 * 
 * The most relevant suffixes will be listed first.  You should cycle through all 
 * the possible files ("Resources_ru_RU.txt","Resources_ru.txt","Resources.txt" - 
 * it's up to you where you want to place the suffix, it can go after .txt), and 
 * if you don't find a match, you should throw a {@link MissingResourceException}.
 * 
 * @param locale
 * @return
 */
protected String [] getSuffixes( Locale locale )
{
// Really, there are only 4 possibilitities:
// 4: _lang_country_variant
// 3: _lang_country
// 2: _lang
// 1: nothing at all
// so, we'll base our work on this.

// Where we keep them all
List suffixes = new LinkedList();

// True if value was specified and is not an empty string
boolean language = locale.getLanguage().length() != 0; 
boolean country = locale.getCountry().length() != 0;
boolean variant = locale.getVariant().length() != 0;

// Adding them in reverse order, to place the most relevant ones to the top

// #4
if ( language && country && variant )
suffixes.add( "_" + locale.getLanguage() + "_" + locale.getCountry() + "_" + locale.getVariant() );

// #3
if ( language && country )
suffixes.add( "_" + locale.getLanguage() + "_" + locale.getCountry() );

// #2
if ( language )
suffixes.add( "_" + locale.getLanguage() );

// #1
suffixes.add("");

return (String [])suffixes.toArray( new String[suffixes.size()] );
}
}
 

Here's 1 implementation of that abstract:

 /*
 * Created on 22.06.2004 at 14:51:43
 * Author: Ivan Jouikov ([e-mail])
 * Project: abLogic
 */
package org.ablogic.i18n;
 
import java.io.*;
import org.ablogic.misc.*;
import java.util.*;
 

/**
 * This bundle will work with {@link java.io.File Files}, rather than classes or
 * CLASSPATH-specific .properties.  It will make heavy use of 
 * {@link org.ablogic.misc.DynamicProperties DynamicProperties} to ensure that all 
 * the files can be reloaded at any time.
 * 
 * Here I will describe the "protocol" used in {@link #getString(String)} and
 * {@link #getString(String, Locale)}, for the "path" to your precious resources.
 * 
 * First of all, all the resources will be stored inside {@link java.util.Properties}
 * files located inside the root directory (specified at {@link #DynamicResourceBundle(File)})
 * inside of as many subfolders as you want.  Filenames that will be considered 
 * must end with ".txt".  In order for different locales to be supported, those files 
 * should have a suffix containing language, country code, or variant
 * (as described in {@link java.util.Locale}).  Here's the formula used for
 * the computation of the suffix:
 * 
 * "_" + language + "_" + country + "_" + variant
 * 
 * It's a <i>hard</i> formula, meaning, that you can't specify things in any other 
 * order.  Language, country, and variant must be valid OSI codes, as defined in 
 * {@link java.util.Locale}.  Here's an example of what a resource for Russians living
 * in Russia would look like:
 * 
 * "Resource_ru_RU.txt"
 * 
 * Same resource, but for French in general:
 * 
 * "Resource_fr.txt"
 * 
 * Same resource, for everyone else:
 * 
 * "Resource.txt"
 * 
 * Basically, DynamicResourceBundle (thanks to {@link org.ablogic.i18n.SimpleBundle}),
 * will try and use the MOST relevant resource to the specified locale.
 * Now remember how I mentioned that you can have as many folders as you want?
 * Here's an example of what a fully qualified path would look like:
 * "/firstFolder/more/another/folder/fileName.property.name"
 * 
 * <ol>
 * <li>Folders will be relative to the root specified in
 * {@link #DynamicResourceBundle(File)}.</li>
 * <li>"/" in the beginning is optional.</li>
 * <li>You can have as many folders as you want</li>
 * <li>fileName will contain <b>no</b> dots</li>
 * <li>Only files with extension ".txt" will be considered.</li>
 * <li>"property.name" is the name of the property inside the file</li>
 * </ol>
 * 
 * Keep in mind that one of the beast features of DynamicResourceBundle is that 
 * it is <b>completely dynamic</b>, meaning you can add new resource files to the 
 * root folder, delete other ones, modify existing ones, and they will all get 
 * synchronized with the cache nearly instantly (how fast they'll get synchronized 
 * is specified by {@link org.ablogic.misc.FileMonitor#getExecutionInterval()}).
 * This allows for interprise applications to run and be modified without having to
 * restart any servers.
 * 
 * This implementation is fairly efficient, being able to easily handle hundreds of 
 * resource files.  It will start taking up some processor performance starting around 
 * thousand files (depending on your system of course).  <b>If that occurs, one simple 
 * solution is to decrease the "sync-interval" of {@link org.ablogic.misc.FileMonitor},
 * which is set to 1 second by default.  Crank it up to a few seconds, maybe even 10, 
 * which will GREATLY increase the performance of DynamicResourceBundle.</b>
 * 
 * @author Ivan Jouikov ([e-mail])
 */
public class DynamicResourceBundle extends SimpleBundle implements FileChangeListener
{
/** Determines if we are debugging or not. */
static boolean DEBUG = true;

// Method used for debugging
void debug( String text )
{
if ( DEBUG )
{
// Get the stack element from the PREVIOUS method, not this one.
StackTraceElement where = new Exception().getStackTrace()[1];
System.out.println("[DEBUG " + where.getFileName() + "#" + where.getLineNumber() + "]: " + text);
}
}

/**
 * This is the extension that all resources must have in order to be 
 * used.  It should not be modified.
 */
static final String extension = ".txt";

/**
 * This map's keys are files and its values are DynamicProperty's.  
 * Basically, it's used as cache for all the i18n stuff.
 */
Map cache;

/** This is the directory where all i18n files are located. */
File root;

/**
 * Creates new DynamicResourceBundle using specified root as its base directory.
 * It's ok to use this method, but I suggest using {@link DynamicResourceBundleFactory}.
 * @param root Directory, from where all the bundles will be collected.
 * @throws FileNotFoundException If the given root does not exist.
 * @throws IllegalArgumentException If the given root is not a directory.
 */
public DynamicResourceBundle( File root ) throws FileNotFoundException, IllegalArgumentException
{
// Make sure root exists
if ( !root.exists() )
throw new FileNotFoundException("Specified root \"" + root + "\" does not exist!");

// Make sure it's a directory
if ( !root.isDirectory() )
throw new IllegalArgumentException("Specified root \"" + root + "\" MUST be a directory!");

// If everything is OK
debug("Creating new " + this.getClass().getName() + " with root on directory \"" + root + "\"...");
cache = new HashMap();
this.root = root;

// Register ourselves with file monitor
FileMonitor.getInstance().addFileChangeListener(this,root);
}

/** Return true if this class is in debug mode, and vice versa. */
public boolean getDebug()
{
return DEBUG;
}

/**
 * Determines if this class will be printing out debugging info or not.
 * All the debug info will go into System.out.
 * @param deubg True if we should be debugging.
 */
public void setDebug( boolean debug )
{
DEBUG = debug;
}

public static void main( String [] args )throws Exception
{
SimpleBundle bundle = new DynamicResourceBundle( new File("C:/Documents and Settings/SysOp/My Documents/AB LOGIC/Bundle/") );
Thread.sleep(5000);
while(true)
{
try
{
System.out.println("RU: " + bundle.getString("main.test") );
System.out.println("EN: " + bundle.getString("main.test", Locale.US) );
System.out.println("folder: " + bundle.getString("/folder/main.test") );
}
catch (Exception e)
{
e.printStackTrace();
}
Thread.sleep(5000);
}
}
 
/**
 * We will use this to keep our cache neat and clean  :) .
 */
public void fileModified(FileChangeEvent event)
{
debug(event.toString());
// If some file was modified, DynamicProperties will take care of that

// If some file was added - let's stick it into our cache
if ( event.fileCreated() )
{
// Make sure it's not a directory
if ( event.getFile().isDirectory() )
return;

// Make sure it's a .txt file
if ( !event.getFile().getName().endsWith(".txt") )
return;

// add to cache
debug("New file in directory \"" + root + "\" has been created!  Adding to the cache..." );
try
{
cache.put( event.getFile(), new DynamicProperties(event.getFile()) );
}
catch (FileNotFoundException e) // now this should never happen
{
e.printStackTrace();
}
catch (IOException e) // if there's a problem reading or something
{
e.printStackTrace();
}
}
// if some file was deleted - remove it from the cache
else if ( event.fileDeleted() )
{
debug("Removing file \"" + event.getFile() + "\" from the cache because it was deleted.");
// Ahh, don't you love .equals() ?  :) 
cache.remove( event.getFile() );
}
}
 
/**
 * For more info on what path should look like, consult the description of this 
 * {@link DynamicResourceBundle class}.
 * @inheritDoc
 * @throws IllegalArgumentException If the specified path does not contain
 * a dot separating a resource from a property, or it doesn't contain a 
 * property name.
 */
public String getString(String path, Locale locale) throws MissingResourceException
{
// Make sure path contains dot (".") - separator of file from properties
int indexOfDot = path.indexOf(".");
if ( indexOfDot == -1 )
throw new IllegalArgumentException("Specified path \"" + path + "\" does not contain a dot separating a resource from a property.");

// Get the property name
// Offset starting index by 1 to account for its lenght ( dot = 1 char )
String propertyName = path.substring(indexOfDot+1, path.length()).trim();
if ( propertyName.length() == 0 )
throw new IllegalArgumentException("Specified path \"" + path + "\" doesn't provide a property name!");

debug("Retrieving property for path \"" + path + "\" and locale \"" + locale + "\"...");

// This is the file path to the resource without suffixes and ".txt"
String resourcePath = root.getAbsolutePath() + File.separatorChar + path.substring(0,indexOfDot);

// Retrieve all the suffixes sorted in "most relevant" order
String [] suffixes = getSuffixes(locale);

// Lets try every single one
suffixLoop: for (int i = 0; i < suffixes.length; i++)
{
// Compose file of filePath + suffix + ".txt"
File resource = new File(resourcePath + suffixes[i] + extension);
debug("Trying resource \"" + resource + "\"...");

// Let's see if that resource is in our cache
Object cacheObject = cache.get(resource);
if ( cacheObject == null )
{
debug("Isn't in cache...");
continue suffixLoop;
}

// If the resource IS in our cache - let's return the property
// (or at least attempt to)
debug("Found in cache!  Retrieving property \"" + propertyName + "\"...");
String value = ((DynamicProperties)cacheObject).getProperty(propertyName); 

// if property wasn't found - throw an exception
if ( value == null )
throw new MissingResourceException("Resource \"" + resource + "\" was found, but we couldn't find the property named \"" + propertyName + "\"!", path, propertyName);

// if everything went smooth - return the property
return value;
}

// If none of the suffixes worked - the resource is missing
throw new MissingResourceException("Resource \"" + path + "\" could not be found...", path, propertyName);
}
 

}
 

[Peter den Haan]

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:

// Make sure it's not a directory[/list]
if ( event.getFile().isDirectory() ) return;

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]

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]

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]

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]

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]

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.