kde review kartesio

[Sven Brauch]

Hey!

A good thing, I think such a tool could be useful to me too (and I
know a lot of other people to whom it might be useful). Here's what I
noticed from a quick look (some has been said already I think):

* You probably shouldn't track the kdev4 file in the repository, same
goes for screenshots

* zorbaneural is a very fancy dependency, it's not even in arch's AUR.
You should put the git URL into the cmake message.

* I put x**2 into the fit box and clicked Fit, and it crashed:
http://paste.kde.org/741026/

* Did you think about laying out the UI around a splitter? On my
screen, the table takes most of the area and the plot is quite small,
and I can't change that...

* It would be useful to be able to import data in some way, e.g. from
a CSV file. I don't see a way to get data into the program except
typing every number into the cells -- or is there another way? If
there is, could it be made more obvious eventually?

* What does the code in calculations.cpp:117 do? It looks quite
curious. Isn't there a more elegant solution (it looks a bit like a
QChar::isLetter() implementation)?

* calculations.cpp:505 and 584 the same code like in 117 again? It's
weird enough to have that stuff once, but copied multiple times is bad
imho ;)

* Your code uses mixed tab- and space indent (sometimes it uses tabs,
sometimes spaces for no apparent reason). Most KDE apps use only
spaces, you might consider if you want to do that too. Sometimes, the
indent is even missing completely; you should indent one level after
each opening curly parenthesis.

* Same goes for the whole formatting of the code, it's pretty
inconsistent. For example, look at the spaces around operators or so.

* Instead of writing to /tmp/kartesiotmp.txt you should probably use
QTempFile. That will also take care of the deleting the temp file when
it gets deallocated so you don't need to exec (scary and
platform-dependent) rm commands.

* calculations.cpp:277 this makes no sense, there's a statement behind
a "return"

In general, you're mixing a lot of plain C / stdlib stuff into Qt
code. Is there a reason for that? For example, in calculations.cpp:148
you take text from a text field, convert it to a byte array, convert
it to a char* and then pass it to a function. Why not just pass the
QString? You can iterate over a QString like
foreach ( const QChar& c, myqstring ) { ... }
or also
for ( int i = 0; i < myqstring.size(); i++ ) { ... }
if you like that better, and you can also index it like a char*, as in
mystring[i+1] or so.

Also, nothing in your code is const and everything is public, although
almost everything could be const and private, but I won't get started
on that now ;)

This is not meant as a list of what you must fix, it's just my two cents.

Cheers!
Sven

2013/5/10 Tomaz Canabrava <tcanabrava at kde.org>:
> Annma, I find that proposal *very* good.
>
> I'm a bit distant of KDE programming - I know - because my day job is making
> me work 12h+ creating scientific tools.
> ( actually - one of the tools that I created here was a... Solver, to fit
> curves on points... )
>
> Tomaz
>
>
> 2013/5/10 David Edmundson <david at davidedmundson.co.uk>
>>
>> The app sounds awesome.
>>
>> From the application life cycle page you linked:
>> > When you have made one of more releases and want to continue to develop
>> > it, the term 'playground' does no longer apply to you. That is the right
>> > time to move out of here
>>
>> There are no releases on download.kde.org under unstable. Have you
>> made these releases elsewhere? If so can you provide a link.
>>
>> Thanks
>>
>> David Edmundson
>
>