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

Mon Aug 9 23:49:35 CEST 2010

Ok, as I said, now I'm trying to explain the changes I made.

----Messaggio originale----

Da: etienne.rebetez at oberwallis.ch

Data: 04/08/2010 17.56

A: "KDE-Edu"<kde-edu at mail.kde.org>, "Rebetez Etienne"<etienne.rebetez at oberwallis.ch>, "Luca Tringali"<tringalinvent at libero.it>

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

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

 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

     (Diff revision 2)

   private:

    60
        titrationCalculator * m_titrationCalculator;    // The nuclear calculator

Update the comment too.

DONE

    /trunk/KDE/kdeedu/kalzium/src/calculator/calculator.cpp

     (Diff revision 2)

   void calculator :: slotItemSelection(QTreeWidgetItem *item)

    95

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

DONE

    /trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.h

     (Diff revision 2)

    37
    class titrationCalculator : public QFrame

Why QFrame? QWidget should be suficiant.

DONE

    /trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.h

     (Diff revision 2)

    43
        ~titrationCalculator();

    44
        void resize();

    45
        int zoom;

    46
        int width;

    47
        int height;

    48
        int end;

    49
        int lettere;

    50
        int temponu;

    51
        double a;

    52
        void plot();

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.

DONE

    /trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp

     (Diff revision 2)

   titrationCalculator:: ~titrationCalculator()

    72
    void titrationCalculator::plot()

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.

If you really need it i can do it, but it will make the code really confuse, because the plot drawing is done in different steps and they depend from the result of the previous step.

    /trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp

     (Diff revision 2)

   titrationCalculator:: ~titrationCalculator()

    74
        QImage image(width, height, QImage::Format_Indexed8);

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);

Well, the plot is not very interesting... I added it just because it seems ugly to me an analitic program that doesn't show the result in a graphical way. The only interesting informations came from the notes box. The plot gives just an idea of what you have found, but in a laboratory relation you don't need it. Anyway, I could use kplot, if you want.

    /trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp

     (Diff revision 2)

   titrationCalculator:: ~titrationCalculator()

    76
        value = qRgb(240, 240, 240); // light gray..

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

Seems that the setColor function works fine only with QRGB parameter.

    /trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp

     (Diff revision 2)

   titrationCalculator:: ~titrationCalculator()

    88
        int wm = width/2;

    89
        int hm = height/2;

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

Ok, I also decided that those variables weren't needed, so I deleted them.

    /trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp

     (Diff revision 2)

   titrationCalculator:: ~titrationCalculator()

    110
           QTableWidgetItem *titemp = uid.tableWidget->item(0,0) ;

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

DONE

    /trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp

     (Diff revision 2)

   titrationCalculator:: ~titrationCalculator()

    111
           if (!titemp || titemp->text().isEmpty())

    112
           {

    113
               //go on

    114
           }else {

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.)

DONE

    /trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp

     (Diff revision 2)

   titrationCalculator:: ~titrationCalculator()

    117
            for (int i=0; i<uid.tableWidget->rowCount() ; i++)

    118
           {

    119
                QTableWidgetItem *titem = uid.tableWidget->item(i,0) ;

    120
                QTableWidgetItem *titemo = uid.tableWidget->item(i,1) ;

    121
                if (!titem || titem->text().isEmpty())

    122
                {

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

You were right, i solved it.

    /trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp

     (Diff revision 2)

   titrationCalculator:: ~titrationCalculator()

    127
                        QByteArray ba = yvalueq.toLatin1();

    128
                        char *yvaluen = ba.data();

    129
                        strcpy(yvalue,yvaluen);

    130
                        tmpy = 1;

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

Yes because I need to read one character at a time, QList make things more complex.

    /trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp

     (Diff revision 2)

   titrationCalculator:: ~titrationCalculator()

    154
        if (mreporto!="") uid.note->setText("Theorical Curve: "+mreporto);

    155
        if (mreporto!="") {

    156
            for (int i=0;i<width;i++) {

    157
            double id = i;

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

DONE

    /trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp

     (Diff revision 2)

   titrationCalculator:: ~titrationCalculator()

    188
               int totaldata=0;

    189
               for (int i=0; i<uid.tableWidget_2->rowCount() ; i++){

    190
                   QTableWidgetItem *titem = uid.tableWidget_2->item(i,0) ;

    191
                   QTableWidgetItem *titemo = uid.tableWidget_2->item(i,1) ;

    192
                   if (!titem || titem->text().isEmpty()) {

    193
                       break;

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.
It's another table. Anyway I settled it up.

    /trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp

     (Diff revision 2)

   titrationCalculator:: ~titrationCalculator()

    230
               sprintf( s_cp, "%.2f", equval );

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

    /trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp

     (Diff revision 2)

   titrationCalculator:: ~titrationCalculator()

    248

    249
            QGraphicsScene *scene = new QGraphicsScene (this);

    250
            QPixmap mpixmap = QPixmap::fromImage(image).scaled(zoom,zoom) ;

    251
            scene->addPixmap(mpixmap);

    252

    253
            uid.graphicsView->setScene(scene);

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

I thought this was the best way to use it, but if you know something better, please tell it to me.

    /trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp

     (Diff revision 2)

   titrationCalculator:: ~titrationCalculator()

    278
                            if (!(yvalue[i]=='q' or yvalue[i]=='w' or yvalue[i]=='e' or yvalue[i]=='r' or yvalue[i]=='t' or yvalue[i]=='y' or yvalue[i]=='u' or yvalue[i]=='i' or yvalue[i]=='o' or yvalue[i]=='p' or yvalue[i]=='a' or yvalue[i]=='s' or yvalue[i]=='d' or yvalue[i]=='f' or yvalue[i]=='g' or yvalue[i]=='h' or yvalue[i]=='j' or yvalue[i]=='k' or yvalue[i]=='l' or yvalue[i]=='z' or yvalue[i]=='x' or yvalue[i]=='c' or yvalue[i]=='v' or yvalue[i]=='b' or yvalue[i]=='n' or yvalue[i]=='m' or yvalue[i]=='+' or yvalue[i]=='-' or yvalue[i]=='^' or yvalue[i]=='*' or yvalue[i]=='/' or yvalue[i]=='(' or yvalue[i]==')' or yvalue[i]=='Q' or yvalue[i]=='W' or yvalue[i]=='E' or yvalue[i]=='R' or yvalue[i]=='T' or yvalue[i]=='Y' or yvalue[i]=='U' or yvalue[i]=='I' or yvalue[i]=='O' or yvalue[i]=='P' or yvalue[i]=='A' or yvalue[i]=='S' or yvalue[i]=='D' or yvalue[i]=='F' or yvalue[i]=='G' or yvalue[i]=='H' or yvalue[i]=='J' or yvalue[i]=='K' or yvalue[i]=='L' or yvalue[i]=='Z' or yvalue[i]=='X' or yvalue[i]=='C' or yvalue[i]=='V' or yvalue[i]=='B' or yvalue[i]=='N' or yvalue[i]=='M' or yvalue[i]=='1' or yvalue[i]=='2' or yvalue[i]=='3' or yvalue[i]=='4' or yvalue[i]=='5' or yvalue[i]=='6' or yvalue[i]=='7' or yvalue[i]=='8' or yvalue[i]=='9' or yvalue[i]=='0' or yvalue[i]=='.' or yvalue[i]==',')) break; //if current value is not a permitted value, this means that something is wrong

Regexp?

Well, it's not very nice, but it works fine and it's simple

    /trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp

     (Diff revision 2)

   titrationCalculator:: ~titrationCalculator()

    422
        QMessageBox::information(this, "IceeQt Rapid Help", "There are two ways to use IceeQt:\n\nTheorical Equations\n Here you can fill the table with the equations you have previously obtained for the chemical equilibria. FOR EXAMPLE if you have this reaction A + B -> C + D then you will have the equation K=(C*D)/(A*B) so you must write 'K' in the Parameter column and '(C*D)/(A*B)' in the Value column. If you want to assign a known value to a parameter you can simply write the numeric value in the Value field. FOR EXAMPLE you can use the system \nA=(C*D)/(B*K) \nK=10^-3 \nC=OH \nOH=(10^-14)/H \nH=10^-4 \nB=6*(10^-2) \nThen you have to write D as X axis and A as Y axis: so you will find out how the concentration of A change in function of D concentration.\nPlease don't use parenthesis for exponents: 10^-3 is correct, while 10^(-3) is wrong. \n\nExperimental Values\n You can use this program to draw the plot of your experimental data obtained during a titration and find out the volume of equivalence. It's strongly recommended to insert a even number of points, because of the best fit algorithm, sorted by volume (the X axis value).\n\nPlot\n The plot shows in red the curve that cames from theorical equations, in blue the experimental points, and in green the aproximated curve for experimental points.");

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.

I decided to remove it... its' better to include this text into the kalzium guide.

    /trunk/KDE/kdeedu/kalzium/src/calculator/titrationCalculator.cpp

     (Diff revision 2)

   titrationCalculator:: ~titrationCalculator()

    439
        for (int i=0; i<uid.tableWidget->rowCount() ; i++){

    440
            QTableWidgetItem *titem = new QTableWidgetItem ;

    441
            titem->setText("");

    442
            uid.tableWidget->setItem(i,0,titem);

    443
            QTableWidgetItem *titemo = new QTableWidgetItem ;

    444
            titemo->setText("");

    445
            uid.tableWidget->setItem(i,1,titemo);

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

I included it into the "Example" button code so it fills the tables with the example values.

- Rebetez
Thank you for the interest you put into this.

Luca Tringali
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-edu/attachments/20100809/ba472153/attachment-0001.htm 