Bob,
Some brief feedback about PropertyManager.java ...
* methods in here should not be public. All public entry points are in
viewer. viewer calls the the routines in PropertyManager. They are in the
same package so they should not be public in PropertyManager.
* I am not sure that 'synchronized' goes there. But there are lots of race
conditions in the code, so just leave it for now.
* You need to understand something about String comparisions:
if (returnType == "String") return info.toString();
This type of comparison works *only* if returnType is an 'interned'
string. An 'interned' string is guaranteed to be from a special string
pool, so when comparing interned strings you can just compare the object
pointers without looking at the string contents.
Now, all string constants are guaranteed to be interned, so that I why I
use this idiom for lots of internal dispatching.
But, you need to be careful about when/where you use this. If you ever
expect to pass this in as a parameter from JavaScript, then it probably
will *not* be interned. In addition, naive/unsuspecting Java programmers
will certainly get bitten by this when they make use of the API.
The rule is ... inside an external API entry point you must intern any
parameters (using param = param.intern(); or equiv) if you are going to be
making these types of == comparisons with strings.
* We should probably use the '==' string object compare mechanism for the
propertyName. That will replace the equalsIgnoreCase calls. These property
names will be case sensitive.
* I expect the number of properties to grow to at least one hundred.
Therefore, over time we will evolve some clean mechanism to dispatch based
upon propertyName.
* Regarding variables ... within the bodies of functions/methods,
variables should be declared before/as they are used, not at the beginning
of the block. This is especially true if the loop is really a for loop. I
suggest the following transformation of your code:
String simpleReplace(String str, String strFrom, String strTo) {
String sout = "";
int ipt;
int ipt0 = 0;
int lfrom = strFrom.length();
if (str == null || lfrom == 0) return str;
while ((ipt = str.indexOf(strFrom, ipt0)) >=0) {
sout = str.substring(ipt0,ipt) + strTo; // <--- BUG HERE ?
ipt0 = ipt + lfrom;
}
sout = sout + str.substring(ipt0,str.length());
return sout;
}
-->
String simpleReplace(String str, String strFrom, String strTo) {
int fromLength = strFrom.length();
if (str == null || fromLength == 0)
return str;
int ipt;
int ipt0;
String sout = "";
for (ipt0 = 0;
ipt = str.indexOf(strFrom, ipt0);
ipt0 = ipt + fromLength)
sout += str.substring(ipt0, ipt) + strTo;
sout += str.substring(ipt0, str.length());
return sout;
}
As a minor note, Java provides a StringBuffer data structure that may be
more efficient when you are catting lots of things together.
* getProperty should not take a 'returnType'
Miguel
-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems? Stop! Download the new AJAX search engine that makes
searching your log files as easy as surfing the web. DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid3432&bid#0486&dat1642
_______________________________________________
Jmol-developers mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/jmol-developers
