Review Request: KCalc: JJ: Group digits in n-base mode (bin, oct, hex)
Christoph Feck
christoph at maxiom.de
Wed Jul 4 10:53:18 UTC 2012
> On July 3, 2012, 10:59 p.m., Christoph Feck wrote:
> > Thanks for the patch :) Unfortunately, it is too late for 4.9, so you should target 4.10 (master branch).
> >
> > Also, I am not so sure the dedicated configuration is really needed, but nice to have.
> >
> > Some remarks in the review below.
>
> Michael Skiba wrote:
> Thank you for the quick review :-)
> This is my very first codepatch for a KDE bundled application (and my first usage of the reviewboard), your input is very much appreciated :)
>
> I had the idea to make it a user adjustable setting when the feature requester suggested grouping binary numbers by 8, representing one byte, which is very reasonable, while I found it much easier to check/copy binary numbers grouped by 4. Also he suggested grouping hex numbers by 4, which is perfectly reasonable as well, but my favorite hex editor groups them by two. So I thought if in doubt (and there are no official rules for it) make it flexible, so everyone can choose their own personal favorite. Does this approach bloat the settings menu too much or is it ok? (as I said most of what I've done so far were textbook examples which print something in a commandline ;-) )
As for the final decision about the new settings, the current maintainer of KCalc is Evan Teran. I would suggest to add him as a reviewer, because I don't know if he monitors the mailing list.
> On July 3, 2012, 10:59 p.m., Christoph Feck wrote:
> > kcalcdisplay.cpp, line 456
> > <http://git.reviewboard.kde.org/r/105427/diff/1/?file=71181#file71181line456>
> >
> > I strongly suggest to create a new method for the digit grouping, and call it with a different argument, depending on number base.
> >
> > Additionally, the comments seem to make the code more complicated than it is. Reduce the amounts of comments, and let the code describe itself. Only add comments where the code does not do what it seems to do.
> >
>
> Michael Skiba wrote:
> It's rather difficult for me to judge which comments are needed and which are not, since I don't have really much real world programming experience, though I agree that it makes the actual code harder to read.
>
> Regarding the function you thought about something like this?
> void KCalcDisplay::setText(const QString &string)
> {
> text_ = string;
> bool special = (string.contains(QLatin1String( "nan" )) || string.contains(QLatin1String( "inf" )));
> // I'd leave this one as it is, since it relies on KGlobal
> if ((num_base_ == NB_DECIMAL) && groupdigits_ && !special) {
> if (string.endsWith(QLatin1Char( '.' ))) {
> text_.chop(1);
> text_ = KGlobal::locale()->formatNumber(text_, false, 0);
> text_.append(KGlobal::locale()->decimalSymbol());
> } else {
> text_ = KGlobal::locale()->formatNumber(text_, false, 0);
> }
> }else if ((num_base_ == NB_BINARY) && groupdigits_ && !special) {
> text_ = groupdigits(text_, binaryGrouping_);
> }else if ((num_base_ == NB_OCTAL) && groupdigits_ && !special) {
> text_ = groupdigits(text_, octalGrouping_);
> }else if ((num_base_ == NB_HEX) && groupdigits_ && !special) {
> text_ = groupdigits(text_, hexadecimalGrouping_);
> }
>
> update();
> emit changedText(text_);
> }
>
> QString KCalcDisplay::groupdigits(QString displayString, int numDigits)
> {
> QString tmpDisplayString;
> int stringLength = text_.length();
>
> for (int i = stringLength; i > 0 ; i--){
> if(i % numDigits == 0 && i != stringLength){
> tmpDisplayString = tmpDisplayString + " ";
> }
>
> tmpDisplayString = tmpDisplayString + displayString[stringLength - i];
> }
>
> return tmpDisplayString;
> }
>
>
> Also I was wondering (and I hope I don't stress your patience to much here) whether it would be more efficient to change the if statement to something like this:
> if(groupdigits_ && !special){
> if (num_base_ == NB_DECIMAL) {
> if (string.endsWith(QLatin1Char( '.' ))) {
> text_.chop(1);
> text_ = KGlobal::locale()->formatNumber(text_, false, 0);
> text_.append(KGlobal::locale()->decimalSymbol());
> } else {
> text_ = KGlobal::locale()->formatNumber(text_, false, 0);
> }
> }else if (num_base_ == NB_BINARY) {
> text_ = groupdigits(text_, binaryGrouping_);
> }else if (num_base_ == NB_OCTAL) {
> text_ = groupdigits(text_, octalGrouping_);
> }else if (num_base_ == NB_HEX) {
> text_ = groupdigits(text_, hexadecimalGrouping_);
> }
> }
>
>
> I've not yet uploaded an updated patch because I wasn't sure if this section is as you've expected it, so I've not yet modified the last part in the code. All the other suggestions above are in the code already and seem to work. :-)
You are right, the "if" statement can be pulled out this way. Inside the "if", I would use a switch/case chain, and maybe rename the method to "groupDigits".
- Christoph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105427/#review15352
-----------------------------------------------------------
On July 4, 2012, 7:31 a.m., Michael Skiba wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105427/
> -----------------------------------------------------------
>
> (Updated July 4, 2012, 7:31 a.m.)
>
>
> Review request for KDE Utils.
>
>
> Description
> -------
>
> Completing a JJ from KBO: Enable grouping of non-decimal numbers. I've added new options in the settings menu to allow the user to set individual grouping width.
> KCalc now displays "11 1101 0110" in binary mode instead of "1111010110", the user can change that behavior to any value he or she wants for example bytewise: "11 11010110".
>
> The user can set individual widths, which are stored for binary, octal and decimal. I've set the following defaults:
> binary = 4 ['1110 1010']
> octal = 4 ['5323 1356']
> hexadecimal = 2 ['A2 F4 C2 11']
>
>
> This addresses bug 247151.
> http://bugs.kde.org/show_bug.cgi?id=247151
>
>
> Diffs
> -----
>
> general.ui 15bee34
> kcalc.h 01cc37e
> kcalc.cpp 76bb123
> kcalc.kcfg 6b613bd
> kcalcdisplay.h 84c1908
> kcalcdisplay.cpp c1c238f
>
> Diff: http://git.reviewboard.kde.org/r/105427/diff/
>
>
> Testing
> -------
>
> Save/Restoring of saved individual values work.
> Calculation and conversation is correct.
>
>
> Screenshots
> -----------
>
> Grouped binary number
> http://git.reviewboard.kde.org/r/105427/s/614/
> Updated settings window
> http://git.reviewboard.kde.org/r/105427/s/615/
>
>
> Thanks,
>
> Michael Skiba
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-utils-devel/attachments/20120704/08bb8d8f/attachment-0001.html>
More information about the Kde-utils-devel
mailing list