Hi Adam, Thanks for the updated patch.
Here's some feedback: First, overall this patch looks good-- nice work. There are a small number of detailed issues for you to address which are given below. Please resubmit after addressing these issues. 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. * 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. * 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. 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. 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. 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. * 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 an updated patch: > > http://adammonsen.com/tmp/story_24_part_a_12220.patch.gz > > Unit tests look good. ------------------------------------------------------------------------- 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://sourceforge.net/services/buy/index.php
