> 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 > >
_______________________________________________ KMyMoney-devel mailing list KMyMoney-devel@kde.org https://mail.kde.org/mailman/listinfo/kmymoney-devel
