Code review comment for lp://qastaging/~toineo/unity-lens-applications/calculator

Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

Hey toineo,

Thanks for this awesome work!
so some comments on the proposed branch:

+ if ((result = process_easter (ref search_string, ref model)) != 0)
I would just make process_easter returns true or false and don't care about the result (we don't really need to differentiate them)

----------------

549 + string uri;
550 + Icon icon;
551 + uri = "unity-calc";
552 + string result_string = _(format_result_output (search_string, result));
-> can you pease fix the ident?

575 + string uri;
576 + Icon icon;
577 + uri = "unity-calc";
578 + string result_string = _(format_result_output (search_string, result));
-> same remark

Btw, you have this code twice, can you try a while loop with an arg being true as long as the result is invalid and we can still tweak the search by adding ')' and such?

----------------------

63 + private Gee.HashMap<string,DelegateWrapper1> TwoLettersFct;
64 + private Gee.HashMap<string,DelegateWrapper1> ThreeLettersFct;
65 + private Gee.HashMap<string,DelegateWrapper1> FourLettersFct;
66 + private Gee.HashMap<string,DelegateWrapper1> FiveLettersFct;

As we discussed, I would prefer an double HashMap with 2,3,4,5 has a primary index to select the right subhashmap reference

------------------------
256 + /* FIXME : at this time, something like tan7 is evaluated
257 + * as tan(7). Do we accept this behavior ? */

-> it's fine for now ;)

Awesome work! can you please look at those remarks and see how to fix them? The rest looks good :-)

Thanks again for working on that!

« Back to merge proposal