----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113143/#review41476 -----------------------------------------------------------
kmymoney/dialogs/transactioneditor.cpp <http://git.reviewboard.kde.org/r/113143/#comment30321> Please declare variables at the point of their initialization, there is no point in declaring the number variable outside of the if statement. And use a const reference if you don't intend to change it. const QString &number = tr.splits().front().number(); kmymoney/mymoney/mymoneyfile.h <http://git.reviewboard.kde.org/r/113143/#comment30322> Please pass a 'const QString &' as a parameter instead of by-value. And why does 'strNumericPart' only forward to the 'numericPart' private function, why don't you make 'numericPart' public and remove 'strNumericPart'? kmymoney/mymoney/mymoneyfile.h <http://git.reviewboard.kde.org/r/113143/#comment30323> Please pass a 'const QString &' as a parameter instead of by-value. kmymoney/mymoney/mymoneyfile.cpp <http://git.reviewboard.kde.org/r/113143/#comment30324> Please pass a 'const QString &' as a parameter instead of by-value. The above are just some plain old C++ coding style suggestions. Also should 'numericPart' really belong to the mymoneyfile object? If so I would add some testcases for it in the mymoneyfiletest suite. - Cristian Oneț On Oct. 7, 2013, 8:10 p.m., Allan Anderson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/113143/ > ----------------------------------------------------------- > > (Updated Oct. 7, 2013, 8:10 p.m.) > > > Review request for KMymoney. > > > Bugs: 319801 > http://bugs.kde.org/show_bug.cgi?id=319801 > > > Repository: kmymoney > > > Description > ------- > > If a user's sequence of check numbers is broken by, say 'ATM' or an invoice > number such as 'No 123-001 ABC', the next check number produced will be '1', > entries containing alpha or punctuation characters not being saved. > The fix corrects this by saving the complete entry, and uses any entered > numeric part to calculate the next number in sequence. If an existing > numeric entry is edited, this entry will be taken into account for the next > number. > > There are some quirks. If the entry which led to the current 'next number' > is deleted, it is not possible to revert to the previous, now forgotten, > 'next number', so the produced 'next number' is likely to leave a 'gap', and > may need editing. Also, there is no check that a new 'next number' does not > already exist. For instance, if there is the erroneous sequence 23,23,24, > the 'next number' will be the expected 25. However, if the user corrects the > error by changing a 23 to 22, the new 'next number' will be 23, which also > already exists. I'm not sure if such issues, which exist also in the current > release, are worthy of fixing for a fairly unimportant area, without becoming > more involved. > > > Diffs > ----- > > kmymoney/dialogs/transactioneditor.h f07dafb > kmymoney/dialogs/transactioneditor.cpp 39049cf > kmymoney/mymoney/mymoneyfile.h af0c6fb > kmymoney/mymoney/mymoneyfile.cpp ff7302c > > Diff: http://git.reviewboard.kde.org/r/113143/diff/ > > > Testing > ------- > > Many manual entries checked, including coping with all values in the unit > tests. The unit test runs OK. > > > Thanks, > > Allan Anderson > >
_______________________________________________ KMyMoney-devel mailing list KMyMoney-devel@kde.org https://mail.kde.org/mailman/listinfo/kmymoney-devel
