[Kmymoney-devel] Review Request 109803: Add CSV export capability.

[Thomas Baumgart]

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


I just did a more formal review and noticed a few things that might receive some improvements. Sorry for being late with the review.


kmymoney/plugins/csvexport/csvexportdlg.h
<http://git.reviewboard.kde.org/r/109803/#comment23370>

    Using the xxxDecl classes is somewhat old fashioned. No need for that in this case. You can directly apply the multi inheritance to CsvExportDialog and don't need CsvExportDlgDecl at all. See http://apidocs.meego.com/1.2/qt4/designer-using-a-ui-file.html for a good tutorial. Personally, I am currently using the single inheritance method in another project, but that does not mean anything.



kmymoney/plugins/csvexport/csvexportdlg.h
<http://git.reviewboard.kde.org/r/109803/#comment23371>

    make that "const QString& getAccountid(const QString& str)"



kmymoney/plugins/csvexport/csvexportdlg.h
<http://git.reviewboard.kde.org/r/109803/#comment23372>

    In case you don't return a reference to the QDate object, there's no need for the return value to be const.



kmymoney/plugins/csvexport/csvexportdlg.h
<http://git.reviewboard.kde.org/r/109803/#comment23373>

    Same here with the const return value.



kmymoney/plugins/csvexport/csvexportdlg.h
<http://git.reviewboard.kde.org/r/109803/#comment23374>

    Not sure if using a default argument for a slot is a good idea. Please see http://harmattan-dev.nokia.com/docs/library/html/qt4/signalsandslots.html#signals-and-slots-with-default-arguments



kmymoney/plugins/csvexport/csvexportdlg.cpp
<http://git.reviewboard.kde.org/r/109803/#comment23375>

    In case you eliminate the xxxxDecl class, you would place your call to "setupUi(this)" here.



kmymoney/plugins/csvexport/csvexportdlg.cpp
<http://git.reviewboard.kde.org/r/109803/#comment23376>

    I would use "!listTrans.isEmpty()" here and avoid the continue stuff and embrace the earliest date condition and latestDate assignment with it.



kmymoney/plugins/csvexport/csvexportdlg.cpp
<http://git.reviewboard.kde.org/r/109803/#comment23377>

    Do you really need to sort the list here or can we improve performance by sorting it once at the end of this method?



kmymoney/plugins/csvexport/csvexportdlg.cpp
<http://git.reviewboard.kde.org/r/109803/#comment23378>

    Can't you use MyMoneyFile::accountByName()? It seems to have the same functionality.



kmymoney/plugins/csvexport/csvwriter.cpp
<http://git.reviewboard.kde.org/r/109803/#comment23379>

    Can you use "m_plugin->m_dlg->deleteLater()" here?



kmymoney/plugins/csvexport/csvwriter.cpp
<http://git.reviewboard.kde.org/r/109803/#comment23381>

    For latin1 non-translatable strings one should use the following construct:
    
    QLatin1String("Investment")
    
    See here http://www.macieira.org/blog/2011/07/qstring-improved/



kmymoney/plugins/csvexport/csvwriter.cpp
<http://git.reviewboard.kde.org/r/109803/#comment23382>

    Such constructs are the reason for so many performance problems (though not here I assume): why don't you simply write
    
      str += QLatin1String("R,");
    



kmymoney/plugins/csvexport/csvwriter.cpp
<http://git.reviewboard.kde.org/r/109803/#comment23383>

    Same here, adding an empty string is just nonsense in my eyes. Using
    
      str += QLatin1Char(',');
    
    is perfectly enough.



kmymoney/plugins/csvexport/csvwriter.cpp
<http://git.reviewboard.kde.org/r/109803/#comment23384>

    Better use
    
      strStatus.clear();



kmymoney/plugins/csvexport/csvwriter.cpp
<http://git.reviewboard.kde.org/r/109803/#comment23385>

    From here on many QLatin1String candidates.


- Thomas Baumgart


On April 19, 2013, 7:52 p.m., Allan Anderson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109803/
> -----------------------------------------------------------
> 
> (Updated April 19, 2013, 7:52 p.m.)
> 
> 
> Review request for KMymoney.
> 
> 
> Description
> -------
> 
> Add CSV export capability.  Modify existing KMyMoney File menu in order to make menu item positions more logical.
> 
> 
> This addresses bug 317614.
>     http://bugs.kde.org/show_bug.cgi?id=317614
> 
> 
> Diffs
> -----
> 
>   kmymoney/plugins/CMakeLists.txt 81ca458 
>   kmymoney/plugins/csvexport/CMakeLists.txt PRE-CREATION 
>   kmymoney/plugins/csvexport/csvexportdlg.h PRE-CREATION 
>   kmymoney/plugins/csvexport/csvexportdlg.cpp PRE-CREATION 
>   kmymoney/plugins/csvexport/csvexportdlgdecl.ui PRE-CREATION 
>   kmymoney/plugins/csvexport/csvexporterplugin.h PRE-CREATION 
>   kmymoney/plugins/csvexport/csvexporterplugin.cpp PRE-CREATION 
>   kmymoney/plugins/csvexport/csvwriter.h PRE-CREATION 
>   kmymoney/plugins/csvexport/csvwriter.cpp PRE-CREATION 
>   kmymoney/plugins/csvexport/kmm_csvexport.desktop PRE-CREATION 
>   kmymoney/plugins/csvexport/kmm_csvexport.rc PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/109803/diff/
> 
> 
> Testing
> -------
> 
> Exported numerous checking and investment CSV files, and then imported into KMyMoney via CSV import (discovering a few minor issues in the existing KMyMoney accounts in the process.)
> Ran astyle and Krazy2.
> 
> 
> Thanks,
> 
> Allan Anderson
> 
>

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