[Kmymoney-devel] Review Request: Refactoring of matching a transaction-under-import

[Łukasz Maszczyński]

> On Nov. 28, 2012, 10:45 p.m., Cristian Oneț wrote:
> > kmymoney/converter/mymoneystatementreader.cpp, line 1498
> > <http://git.reviewboard.kde.org/r/107137/diff/4/?file=96699#file96699line1498>
> >
> >     Why is this copy needed?
> 
> Łukasz Maszczyński wrote:
>     MyMoneyFile::addTransaction() requires a reference to MyMoneyTransaction, not a const reference which is available in MyMoneyStatementReader::addTransaction(). This is due to the fact, that the transaction is modified in MyMoneyFile::addTransaction() just before it is stored. 
>     
>     The instance of the MyMoneyTransaction is not used after passing it to MyMoneyFile::addTransaction(), so passing a copy (which then gets modified there) should not hurt. However this is not the best solution possible - I admit that. Further refactoring is desired, but I don't want to make this review even bigger.
> 
> Cristian Oneț wrote:
>     I thought so, that's why I asked the question. I this case I would change MyMoneyStatementReader::addTransaction(const MyMoneyTransaction& transaction) to  MyMoneyStatementReader::addTransaction(MyMoneyTransaction& transaction) and remove the copying. That way if someone will think of using the transaction object in the future after it was added it will be in a correct state. I think it's more correct this way in the current state of the patch.

Agreed - fixed in rev. 5. 

Ideally, the MyMoneyFile::addTransaction() would not modify MyMoneyTransaction but just store it as is, but that's a totally new topic.


- Łukasz


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107137/#review22731
-----------------------------------------------------------


On Nov. 29, 2012, 5:40 p.m., Łukasz Maszczyński wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107137/
> -----------------------------------------------------------
> 
> (Updated Nov. 29, 2012, 5:40 p.m.)
> 
> 
> Review request for KMymoney.
> 
> 
> Description
> -------
> 
> 1. please note that dependency on Boost is no longer optional (see changes in CMakeLists.txt)
> 
> 2. Architectural changes
>    Until now, the method MyMoneyStatementReader::processTransactionEntry() handled the whole process of importing a transaction - that is: handling the securities, matching and creating payees, and - at the very end of the method - adding the transaction to the ledger.
>    The last step (adding transaction to ledger) is the main target of this refactoring. Its algorithm was as follows:
>    1. find a matching transaction (either existing or scheduled) - using TransactionMatcher::findMatch()
>    2. If an "existing transaction match" is found - handle it (in the old code it's the block starting with a comment "// it matched a simple transaction. that's the easy case")
>    3. Else if a "scheduled transaction match" is found - handle it ("// a match has been found in a pending schedule"...)
> 
>    Code "mapping" is as follows:
>    - step 2 (above) is extracted to handleMatchingOfExistingTransaction()
>    - step 3 (above) is extracted to handleMatchingOfScheduledTransaction()
>    - TransactionMatcher::findMatch() is extracted to TransactionMatchFinder::findMatch() (note: there are two pure-virtual functions that are implemented in ExistingTransactionMatchFinder, ScheduledTransactionMatchFinder classes)
>    - TransactionMatcher::checkTransaction() is extracted to TransactionMatchFinder::findMatchingSplit()
> 
> 3. Memory management changes
>    Raw pointers are no longer used, as these are typically error-prone. Pointers were replaced either with object instances, or boost::optional is used if applicable (e.g. see members of TransactionMatchFinder class).
> 
> 4. dynamic_casts removed (were used on pointers returned by TransactionMatcher::findMatch(), no longer needed - see TransactionMatchFinder::getMatchedTransaction() and getMatchedSchedule() )
> 
> 5. variable/method names - I did my best to keep those meaningful: e.g. "importedTransaction" instead of "t")
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 93af070 
>   kmymoney/converter/mymoneystatementreader.h 758ff00 
>   kmymoney/converter/mymoneystatementreader.cpp 42c4841 
>   kmymoney/dialogs/CMakeLists.txt 9a8d782 
>   kmymoney/dialogs/existingtransactionmatchfinder.h PRE-CREATION 
>   kmymoney/dialogs/existingtransactionmatchfinder.cpp PRE-CREATION 
>   kmymoney/dialogs/scheduledtransactionmatchfinder.h PRE-CREATION 
>   kmymoney/dialogs/scheduledtransactionmatchfinder.cpp PRE-CREATION 
>   kmymoney/dialogs/transactionmatcher.h d09a4cd 
>   kmymoney/dialogs/transactionmatcher.cpp c380877 
>   kmymoney/dialogs/transactionmatchfinder.h PRE-CREATION 
>   kmymoney/dialogs/transactionmatchfinder.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/107137/diff/
> 
> 
> Testing
> -------
> 
> make test
> 
> 
> Thanks,
> 
> Łukasz Maszczyński
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kmymoney-devel/attachments/20121129/0546ad5e/attachment-0001.html>