[Mifos-developer] PATCH: story #24, part A
Adam Monsen
Don't try to apply the patch: there are now conflicts. I'm fixing the
patch so it will work with revision 12220.
--
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://sourceforge.net/services/buy/index.php
Van Mittal-Henkle
> I just got hit by Tom's sniper rifle and/or ninja strike. :)
>
> Don't try to apply the patch: there are now conflicts. I'm fixing the
> patch so it will work with revision 12220.
Thanks, let us know when you have an update.
Adam Monsen
wrote:
[...]
> Thanks, let us know when you have an update.
Here's an updated patch:
http://adammonsen.com/tmp/story_24_part_a_12220.patch.gz
Unit tests look good.
--
Adam Monsen
Van Mittal-Henkle
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.
Adam Monsen
[...]
> 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
Van Mittal-Henkle
Thanks for the updated patch. It looks good and has been committed as
revision 12221.
Comments are inline.
>> * In general we don't want to commit commented out code. ...
>> 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.
Agreed. Thanks for the descriptive source controle comment you provided
below.
> 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.
Great- thanks for the cleanup.
> I thought it might even be cleaner just to exclude the comment, too.
> I'm happy to add it back if you want.
Excluding it sounds good.
> I didn't have time to evaluate the possibility of completely removing
> the SYSTEM_CONFIGURATION table..
We can revisit this after wrapping up the configuration related stories.
> Ah, good call. I got rid of my new join() in favor of Apache Commons'
> StringUtils join() method.
Great!
> I started refactoring StringUtils, but it
> was a slippery slope. :)
Yes, these things can involve more work than you might think at first.
> 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.
Agreed. Your summary has been used for the commit.
Cheers,
--Van
Kimloan Ho
> 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.
Hi Adam,
My code for the story of moving currency to the config
file is not involved in moving fields of table
System_Configuration at all. My comment was for your
story because you moved the field name_sequence to
config file. The other 5 fields daysForCalDefinition,
scheduleMeetingOnHoliday, centerHierarchyExist,
groupCanApplyLoans, and clientCanExistOutsideGroup
have been moved to config file. However, the field
BACK_DATED_TXN_ALLOWED hasn't been moved yet so
probably we will revisit it after the config stories
are done as Van suggested. BTW, I thought I said I
will check in the code tomorrow night? I am still
working on it. Not so fast, Adam :-)
kim
____________________________________________________________________________________
Never miss a thing. Make Yahoo your home page.
http://www.yahoo.com/r/hs