On Tue, 2007-12-11 at 20:00 -0500, Van Mittal-Henkle wrote: [...] > ConfigEntity.java > * In general we don't want to commit commented out code. The reason > being is that over time it tends to clutter up active code and it > becomes harder to know if there is a reason for keeping it around. If > it is useful to keep some code commented out while you have work in > progress, please note the date it was commented out in the comment and > when it's ok to remove. So if the code you commented out isn't useful > for your ongoing work, please go ahead and remove it.
Done. And in general I totally agree, so I'm glad you mentioned this! Unless we need to support deprecation of APIs and such, the source control change log might even be sufficient for a historical record of what happened and why. I also removed a couple of member variables that weren't referenced anywhere (scheduleMeetingOnHoliday, daysForCalDefinition) as well as their corresponding hibernate mapping and DML/DDL in the sql scripts. > * Including a text comment relating to reorganization is reasonable as > you did. I would suggest adding a date to this too, so it will be > easier to know when this comment can be removed. I thought it might even be cleaner just to exclude the comment, too. I'm happy to add it back if you want. The one place where it seemed to make the most sense to have the comment was in upgrade_to_163.sql, and I left one in there. I didn't have time to evaluate the possibility of completely removing the SYSTEM_CONFIGURATION table because I had to get my patch in before Kim's. Perhaps after Kim commits tonight we'll have a better idea... I thought she mentioned something about more fields disappearing from that table soon. > * You'll notice that there was already commented out code in > ConfigEntity.java (relating to noOfInterestDays) without a date or > explanation. Please go ahead and delete this code. Done. > ConfigEntity.hbm.xml > * A comment in ConfigEntity.java about refactoring "nameSequence" should > be sufficient so that a comment isn't also needed in the HBM file. > Please go ahead and remove the commented out lines. Done. > ConfigConstants.java, OfficeConfigConstants.java > * since the constants being removed is no longer referenced or needed, > please go ahead and remove the commented out code and refactoring > comments. Done. > StringUtils.java > * a join method was added here. There are similar methods that can be > found in Apache Commons StringUtils > (http://commons.apache.org/lang/api/org/apache/commons/lang/StringUtils.html). > I would suggest looking here first when there is a utility > method you need. The signatures of these methods do not exactly match > what you implemented, but see if one can serve your purpose. By > leveraging other open source libraries as much as we can, we can keep > the Mifos code base focused on the problem domain and reduce the amount > of code we need to maintain. Ah, good call. I got rid of my new join() in favor of Apache Commons' StringUtils join() method. I started refactoring StringUtils, but it was a slippery slope. :) I'll address the problems with StringUtils in a separate post to this list. Look for a subject of "custom StringUtils refactor plan". > * you may find that there are other utility methods we can use from > Apache Commons to replace implementations currently in Mifos. > > Thanks! > --Van Here's a summary of the changes I made. This might be appropriate for the commit log. I'm a fan of verbose version control commit logs, since that's where I often look for answering those "why did they do that?" questions. * First part of work on Story 24: moving miscellaneous configuration items out of the database into properties files. * cleaned up commented code from ConfigEntity.java and ConfigEntity.hbm.xml * removed empty no-arg constructor for ConfigEntity * removed NAME_SEQUENCE_DEFAULT constant and modified code that fetches the NAME_SEQUENCE column from the SYSTEM_CONFIGURATION table. See ClientRules#getNameSequence() for a replacement data source for this configured value. And here's the updated patch against revision 12220: http://adammonsen.com/tmp/story_24_part_a_12220-2.patch.gz All unit tests pass. -Adam -- Adam Monsen ------------------------------------------------------------------------- SF.Net email is sponsored by: Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
