Refactor Blocks #35

mandaiy · 2016-09-18T07:22:19Z

Following changes are made in this commit:

Rename classe
As we discussed offline
Extract common functions among block classes
Changed NumberTextHolder interface for better encapsulation
Change return value type of BlockBase#getKind
The previous code uses Class class, which does not represent actual
use case (e.g., ForwardSecBlock#getKind returns SequenceBlock.class).
Replaced Class<? extends Block> type with Enum.
Remove locale-dependent code on parsing numbers
The previous code relies on the Locale.US no matter what locale
this app runs on.
This commit fixes the locale dependent code by extracting the parsing
logic.
Fix the unspoken rule that using scaled values
for communicating between blocks and number pickers.
The previous code scaled values so that the value has no radix
point, which was confusing code.
Fixed by replacing the type with BigDecimal and explicitly use the raw
value.
In accordance with it, changed the type of "number"
in ProgramDataSpec for saving without data loss
Remove code in Unit class for handling quantities, which was ugly and cannot be used widely.
Changed second string ("second/s" -> just "s")

tiwanari · 2016-09-26T07:56:22Z

Thanks for this PR 👍 Because I merged other PRs, there are conflicts :( Please fix them.

Following changes are made in this commit: * Rename classes As we discussed offline * Extract common functions among block classes Changed NumberTextHolder interface for better encapsulation * Change return value type of BlockBase#getKind The previous code uses Class class, which does not represent actual use case (e.g., ForwardSecBlock#getKind returns SequenceBlock.class). Replaced Class<? extends Block> type with Enum. * Remove locale-dependent code on parsing numbers The previous code relies on the Locale.US no matter what locale this app runs on. This commit fixes the locale dependent code by extracting the parsing logic. * Fix the unspoken rule that using scaled values for communicating between blocks and number pickers. The previous code scaled values so that the value has no radix point, which was confusing code. Fixed by replacing the type with BigDecimal and explicitly use the raw value. In accordance with it, changed the type of "number" in ProgramDataSpec for saving without data loss * Remove code in Unit class for handling quantities, which was ugly and cannot be used widely. Changed second string ("second/s" -> just "s")

mandaiy · 2016-09-28T12:36:48Z

Hey @tiwanari, resolved conflicts.
Please take a look at my commit. Thanks.

tiwanari · 2016-09-28T13:47:07Z

Thanks for notifying me :) Please give me a little time to check them.

tiwanari

I made comments but before seeing them, please make the PR enable to be built. There are errors with merged code (ExecutionCondition).

tiwanari · 2016-09-29T00:51:29Z

app/src/main/java/com/pileproject/drive/programming/visual/block/BlockFactory.java

        try {
            return (Class<T>) Class.forName(className);
+


I don't think this blank line makes this program more readable. Is there any specific reason to keep blank lines like this? If no, please remove such lines.

Seriously? I DO THINK SO.

but this is kinda religious war and here's your realm.

I'm sorry, I didn't want to mean it :( I agree that adequate blank lines help code reading.

I couldn't see the consistency between these blank lines here and previous codes at review but now I'm guessing you added blank lines between long lines. It can give a kind of tender impression to codes I think. I wanted to know some of rules, i.e. why you added it here and didn't there.

We haven't talked enough about blank lines beforehand and formatters never specify it. I really appreciate if you share your thoughts :)

Don't take my comment seriously and offensive, please.
For consistency you are right.
And topics of the format are bit off from here.
Anyway I'll merge this. Thanks.

incidentally you can witch-hunt with

$ grep -ElR "catch $.*$ {" app | xargs perl -0777 -pe 's/\n(\s*} catch $.*$ {)\n/$1/g'

for making your realm calm and getting rid of "pagans"

Thanks. The one-liner seems to illustrate a rule "add blank lines before & after a catch clause" :)

btw, `realm' has a bit overwhelmed impression for me... 😓

tiwanari · 2016-09-29T00:52:51Z

app/src/main/java/com/pileproject/drive/programming/visual/block/BlockFactory.java

    public static final int UNDO = 3;
    public static final int LOAD = 4;

-    /**


This is just a question. Did you remove Javadocs from here because the method is private, right?

The answer is Yes

tiwanari · 2016-09-29T01:12:44Z

...src/main/java/com/pileproject/drive/programming/visual/block/repetition/RepetitionBlock.java

@@ -29,14 +30,14 @@
 */
 public abstract class RepetitionBlock extends BlockBase {

-    public static final int FOREVER_WHILE_OFFSET = -1000;
+    static final int FOREVER_WHILE_OFFSET = -1000;


I found this cannot be package private here because it is used in ExecuteCodition. This may be controversial.

Moved to ExecutionCondition, since this field is irrelevant for the operation of blocks.
(btw ExecutionCondition is no longer "condition")

tiwanari · 2016-09-29T01:40:05Z

app/src/main/java/com/pileproject/drive/programming/visual/block/BlockFactory.java

@@ -38,131 +38,93 @@
    public static final int SEQUENCE = 0;
    public static final int REPETITION = 1;
    public static final int SELECTION = 2;
+    @Deprecated


Sorry, this is my fault. I forgot to remove this constant. Please remove it and I will add it in #12 .

tiwanari · 2016-09-29T01:45:10Z

app/src/main/java/com/pileproject/drive/programming/visual/block/NumberTextViewDelegate.java

+import java.math.BigDecimal;
+import java.util.Locale;
+
+public class NumberTextViewDelegate {


Could you give a short description of this class as a Javadoc here?

tiwanari · 2016-09-29T03:31:30Z

app/src/main/java/com/pileproject/drive/util/string/ParseUtil.java

+import java.text.ParseException;
+import java.util.Locale;
+
+public class ParseUtil {


This class also has many blank lines. It's okay if it has reasons.

tiwanari · 2016-09-29T03:32:34Z

app/src/main/java/com/pileproject/drive/util/string/ParseUtil.java

+        }
+    }
+
+    public static BigDecimal bigDecimalValueOf(String value) throws NumberFormatException {


Please give a Javadocs as you made above.

tiwanari · 2016-09-29T03:49:46Z

app/src/main/java/com/pileproject/drive/view/NumberSelectWheelScrollerView.java

-     * @param context      an application context
-     * @param integralPart the length of integral part
-     * @param decimalPart  the length of decimal part
+     * * Constructor.


nit: double *.

tiwanari · 2016-09-29T04:01:51Z

...nxt/java/com/pileproject/drive/programming/visual/block/sequence/SetLeftMotorSpeedBlock.java

-        return Integer.parseInt(numText.getText().toString());
-    }
+    // TODO: set from preference
+    private static final int PRECISION = 1;


1 is okay but it was 0.

tiwanari · 2016-09-29T04:08:36Z

app/src/nxt/java/com/pileproject/drive/programming/visual/block/sequence/TurnLeftSecBlock.java

-    public double getMin() {
-        return 0.0;
+    public Range<BigDecimal> getRange() {
+        return range;
    }

    @Override
    public int action(


I know this is my fault but if you have time, please remove break lines... (this is not mandatory)

tiwanari · 2016-09-29T04:13:39Z

Overall, they are really good and cleaned 👍

* remove UNDO in BlockFactory.java * move FOREVER_WHILE_OFFSET in RepetitionBlock.java to ExecutionCondition.java * add Javadocs on NumberTextDelegate * change the signature of BlockSpaceManagerBase#placeBlock (ArrayList to List) * rename OnTouchNumTextListener to OnTouchNumberTextListener * remove OnNumberTextListener#context and rename holder to mHolder * rename Unit#getUnitString to decorateValue and introduce new class NumberUtil

for the following classes: * NumberTextViewDelegate * Unit * NumberUtil * ParseUtil And cradle.build in the root directory is modified to add the dexmaker libraries which are necessary for mockito

tiwanari

LGTM!

* Refactor Blocks Following changes are made in this commit: * Rename classes As we discussed offline * Extract common functions among block classes Changed NumberTextHolder interface for better encapsulation * Change return value type of BlockBase#getKind The previous code uses Class class, which does not represent actual use case (e.g., ForwardSecBlock#getKind returns SequenceBlock.class). Replaced Class<? extends Block> type with Enum. * Remove locale-dependent code on parsing numbers The previous code relies on the Locale.US no matter what locale this app runs on. This commit fixes the locale dependent code by extracting the parsing logic. * Fix the unspoken rule that using scaled values for communicating between blocks and number pickers. The previous code scaled values so that the value has no radix point, which was confusing code. Fixed by replacing the type with BigDecimal and explicitly use the raw value. In accordance with it, changed the type of "number" in ProgramDataSpec for saving without data loss * Remove code in Unit class for handling quantities, which was ugly and cannot be used widely. Changed second string ("second/s" -> just "s") * Fix PR according to the comments on #35 * remove UNDO in BlockFactory.java * move FOREVER_WHILE_OFFSET in RepetitionBlock.java to ExecutionCondition.java * add Javadocs on NumberTextDelegate * change the signature of BlockSpaceManagerBase#placeBlock (ArrayList to List) * rename OnTouchNumTextListener to OnTouchNumberTextListener * remove OnNumberTextListener#context and rename holder to mHolder * rename Unit#getUnitString to decorateValue and introduce new class NumberUtil * Add and fix tests for the following classes: * NumberTextViewDelegate * Unit * NumberUtil * ParseUtil And cradle.build in the root directory is modified to add the dexmaker libraries which are necessary for mockito * Remove superfluous line-breaks * Add constructor guards for util classes

mandaiy added the enhancement label Sep 18, 2016

mandaiy assigned tiwanari Sep 18, 2016

tiwanari requested changes Sep 29, 2016

View reviewed changes

mandaiy added 5 commits September 29, 2016 22:12

Add and fix tests

f857271

for the following classes: * NumberTextViewDelegate * Unit * NumberUtil * ParseUtil And cradle.build in the root directory is modified to add the dexmaker libraries which are necessary for mockito

Remove superfluous line-breaks

bed4086

Remove an extra comma

1152325

Add constructor guards for util classes

e5fd256

tiwanari approved these changes Sep 30, 2016

View reviewed changes

mandaiy merged commit 17ea9c6 into PileProject:develop Sep 30, 2016

mandaiy deleted the refactor/block branch October 3, 2016 07:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Blocks #35

Refactor Blocks #35

mandaiy commented Sep 18, 2016

tiwanari commented Sep 26, 2016

mandaiy commented Sep 28, 2016

tiwanari commented Sep 28, 2016

tiwanari left a comment

tiwanari Sep 29, 2016

mandaiy Sep 29, 2016

tiwanari Sep 30, 2016

mandaiy Sep 30, 2016

tiwanari Oct 2, 2016

tiwanari Sep 29, 2016

mandaiy Sep 29, 2016

tiwanari Sep 29, 2016

mandaiy Sep 29, 2016

tiwanari Sep 29, 2016

mandaiy Sep 29, 2016

tiwanari Sep 29, 2016

mandaiy Sep 29, 2016

tiwanari Sep 29, 2016

tiwanari Sep 29, 2016

mandaiy Sep 29, 2016

tiwanari Sep 29, 2016 •

edited

Loading

mandaiy Sep 29, 2016

tiwanari Sep 29, 2016

mandaiy Sep 29, 2016

tiwanari Sep 29, 2016

mandaiy Sep 29, 2016

tiwanari commented Sep 29, 2016

tiwanari left a comment

Refactor Blocks #35

Refactor Blocks #35

Conversation

mandaiy commented Sep 18, 2016

tiwanari commented Sep 26, 2016

mandaiy commented Sep 28, 2016

tiwanari commented Sep 28, 2016

tiwanari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiwanari Sep 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiwanari commented Sep 29, 2016

tiwanari left a comment

Choose a reason for hiding this comment

tiwanari Sep 29, 2016 •

edited

Loading