[kde-edu]: Review Request: Titration calculator in Kalzium

Wed Aug 4 17:56:45 CEST 2010

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

Hi
Sorry that it took so long. 

Basicaly the idea of the program is nice. But IMHO the code needs quite some more work/love. I think this comes from porting the old program to c++/qt, so there is still old code in it.
I hope i don't discourage you by that many positions. I didn't check everithing but i think you get the idea.

My english is pretty bad, so if something isn't clear ask.

Regards Eti

/trunk/KDE/kdeedu/kalzium/src/calculator/calculator.h
<http://reviewboard.kde.org/r/4667/#comment6589>

    Update the comment too.

/trunk/KDE/kdeedu/kalzium/src/calculator/calculator.cpp
<http://reviewboard.kde.org/r/4667/#comment6590>

    I once have been told to remove the not needed spaces. They are indicated with that red color.

/trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.h
<http://reviewboard.kde.org/r/4667/#comment6667>

    Why QFrame? QWidget should be suficiant.

/trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.h
<http://reviewboard.kde.org/r/4667/#comment6591>

    No variable should be public. Make them private.
    If another class needs that value, make a function to get or set the value. That way you have a nice api.
    Also indicate with a prefix (m_) that those varaibles are members of that class.

/trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp
<http://reviewboard.kde.org/r/4667/#comment6654>

    the plot function does far more then plot. 
    Trying to split it in i.e dataGathering, axis, calculation, formating output or so, might help reading and maintain the code.
    For instantce, it's hard to find out where the actual calculation takes place.

/trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp
<http://reviewboard.kde.org/r/4667/#comment6594>

    Why do you use the QImage class for the plot? QGraphicsView or kplot would imho have much more comfort. Check also the code from the other kdeedu plot programs to see how they do it.
    To generate an image (pixmap in this case) you can always do 
    pix = QPixmap::grabWidget(m_myPlotWidget);

/trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp
<http://reviewboard.kde.org/r/4667/#comment6592>

    QColor(Qt::lightGray) would do the same.

/trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp
<http://reviewboard.kde.org/r/4667/#comment6607>

    Can you give meanigfull names to the variables.
    Some valiables seem also to be italian;)

/trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp
<http://reviewboard.kde.org/r/4667/#comment6612>

    Would a 
    if ( !uid.talbeWidget->item(0,0)->text().isEmpty() )
    also do the trick?

/trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp
<http://reviewboard.kde.org/r/4667/#comment6604>

    Try to fromat the code cleaner and try to make smarter if/else's. This else is way to long. It's not obvius where it ends.
    (KDevelop has a nice code format function that does the boring work.)

/trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp
<http://reviewboard.kde.org/r/4667/#comment6666>

    I am not sure if that method eats unnessery RAM. Acessing the tableWidget directly might be a better approach.

/trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp
<http://reviewboard.kde.org/r/4667/#comment6614>

    do you realy need to convert the QString to char? (QList?)

/trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp
<http://reviewboard.kde.org/r/4667/#comment6665>

    Use QString() insteat of "".
    Put visible Strings always in a i18n() function for translation.
    There is twice the same "if" statement.

/trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp
<http://reviewboard.kde.org/r/4667/#comment6664>

    Thats the second thime you do this. 
    Makeing a function that gets the data from tableWidget and put it in a QList of some sort would make the code cleaner.

/trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp
<http://reviewboard.kde.org/r/4667/#comment6653>

    QString::number(a) would do the same.

/trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp
<http://reviewboard.kde.org/r/4667/#comment6663>

    Ah, there is a qgraphicsScene/view. I can do more than that;) qt documentation is your frend.

/trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp
<http://reviewboard.kde.org/r/4667/#comment6655>

    Regexp?

/trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp
<http://reviewboard.kde.org/r/4667/#comment6656>

    Adding a html (or somthing else) file somewhere and loading it here would be nice. that way the help can be edited without changing the source code.

/trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp
<http://reviewboard.kde.org/r/4667/#comment6662>

    some default values would help understand the programm. if everthing is empty it's not easy to "just" try the program out.

- Rebetez

On 2010-07-17 11:17:50, Luca Tringali wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4667/
> -----------------------------------------------------------
> 
> (Updated 2010-07-17 11:17:50)
> 
> 
> Review request for KDE-Edu.
> 
> 
> Summary
> -------
> 
> These changes are needed to implement the titration calculator for kalzium. Anyway it's necessary to add the files titrationCalculator.cpp, titrationCalculator.h, titrationCalculator.ui (you can find it here http://zorbaproject.uuuq.com/upload/titrationCalculator.tar.gz) into the directory src/calculator/. I cannot do this because post-review allows only to edit existing files and not to add new files, and I do not have any svn account. 
> Tell me if I did something wrong.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeedu/kalzium/src/CMakeLists.txt 1145939 
>   /trunk/KDE/kdeedu/kalzium/src/calculator/calculator.h 1145939 
>   /trunk/KDE/kdeedu/kalzium/src/calculator/calculator.cpp 1145939 
>   /trunk/KDE/kdeedu/kalzium/src/calculator/calculator.ui 1145939 
>   /trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.h PRE-CREATION 
>   /trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp PRE-CREATION 
>   /trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.ui PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/4667/diff
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> Titration example
>   http://reviewboard.kde.org/r/4667/s/460/
> 
> 
> Thanks,
> 
> Luca
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-edu/attachments/20100804/d42c38d8/attachment-0001.htm 