Skip to main content
Code Review

Return to Answer

remove illegal code
Source Link
ggorlen
ggorlen
  • 4.2k
  • 2
  • 19
  • 28

  • There is excessive vertical whitespace in the layout.

  • Since you're using one click listener for all of your buttons, you could put them in an array and write a loop instead of setting the listener on each one by hand.

  • "$equOnScreen" + "0" + " " could be "$equOnScreen0 ".

  • The code:

    if(result != null) {
    return result
}
return null

    Is the same as: return result.

  • Button styling is repetitive; you could add a values/styles.xml like:

    <?xml version="1.0" encoding="utf-8"?>
<resources>
    <style name="calc_button" parent="@android:style/Widget.Button.Small">
        <item name="android:layout_width">100dp</item>
        <item name="android:layout_height">wrap_content</item>
        <item name="android:layout_gravity">center_horizontal|center</item>
    </style>
</resources>

    then use it on each button like:

    <Button
    android:id="@+id/button1"
    style="@style/calc_button"
    android:text="1" />

  • There is excessive vertical whitespace in the layout.

  • Since you're using one click listener for all of your buttons, you could put them in an array and write a loop instead of setting the listener on each one by hand.

  • "$equOnScreen" + "0" + " " could be "$equOnScreen0 ".

  • The code:

    if(result != null) {
    return result
}
return null

    Is the same as: return result.

  • Button styling is repetitive; you could add a values/styles.xml like:

    <?xml version="1.0" encoding="utf-8"?>
<resources>
    <style name="calc_button" parent="@android:style/Widget.Button.Small">
        <item name="android:layout_width">100dp</item>
        <item name="android:layout_height">wrap_content</item>
        <item name="android:layout_gravity">center_horizontal|center</item>
    </style>
</resources>

    then use it on each button like:

    <Button
    android:id="@+id/button1"
    style="@style/calc_button"
    android:text="1" />

  • There is excessive vertical whitespace in the layout.

  • Since you're using one click listener for all of your buttons, you could put them in an array and write a loop instead of setting the listener on each one by hand.

  • The code:

    if(result != null) {
    return result
}
return null

    Is the same as: return result.

  • Button styling is repetitive; you could add a values/styles.xml like:

    <?xml version="1.0" encoding="utf-8"?>
<resources>
    <style name="calc_button" parent="@android:style/Widget.Button.Small">
        <item name="android:layout_width">100dp</item>
        <item name="android:layout_height">wrap_content</item>
        <item name="android:layout_gravity">center_horizontal|center</item>
    </style>
</resources>

    then use it on each button like:

    <Button
    android:id="@+id/button1"
    style="@style/calc_button"
    android:text="1" />
add parent= to style
Source Link
ggorlen
ggorlen
  • 4.2k
  • 2
  • 19
  • 28

  • There is excessive vertical whitespace in the layout.

  • Since you're using one click listener for all of your buttons, you could put them in an array and write a loop instead of setting the listener on each one by hand.

  • "$equOnScreen" + "0" + " " could be "$equOnScreen0 ".

  • The code:

    if(result != null) {
    return result
}
return null

    Is the same as: return result.

  • Button styling is repetitive; you could add a values/styles.xml like:

    <?xml version="1.0" encoding="utf-8"?>
<resources>
    <style name="calc_button">name="calc_button" parent="@android:style/Widget.Button.Small">
        <item name="android:layout_width">100dp</item>
        <item name="android:layout_height">wrap_content</item>
        <item name="android:layout_gravity">center_horizontal|center</item>
    </style>
</resources>

    then use it on each button like:

    <Button
    android:id="@+id/button1"
    style="@style/calc_button"
    android:text="1" />

  • There is excessive vertical whitespace in the layout.

  • Since you're using one click listener for all of your buttons, you could put them in an array and write a loop instead of setting the listener on each one by hand.

  • "$equOnScreen" + "0" + " " could be "$equOnScreen0 ".

  • The code:

    if(result != null) {
    return result
}
return null

    Is the same as: return result.

  • Button styling is repetitive; you could add a values/styles.xml like:

    <?xml version="1.0" encoding="utf-8"?>
<resources>
    <style name="calc_button">
        <item name="android:layout_width">100dp</item>
        <item name="android:layout_height">wrap_content</item>
        <item name="android:layout_gravity">center_horizontal|center</item>
    </style>
</resources>

    then use it on each button like:

    <Button
    android:id="@+id/button1"
    style="@style/calc_button"
    android:text="1" />

  • There is excessive vertical whitespace in the layout.

  • Since you're using one click listener for all of your buttons, you could put them in an array and write a loop instead of setting the listener on each one by hand.

  • "$equOnScreen" + "0" + " " could be "$equOnScreen0 ".

  • The code:

    if(result != null) {
    return result
}
return null

    Is the same as: return result.

  • Button styling is repetitive; you could add a values/styles.xml like:

    <?xml version="1.0" encoding="utf-8"?>
<resources>
    <style name="calc_button" parent="@android:style/Widget.Button.Small">
        <item name="android:layout_width">100dp</item>
        <item name="android:layout_height">wrap_content</item>
        <item name="android:layout_gravity">center_horizontal|center</item>
    </style>
</resources>

    then use it on each button like:

    <Button
    android:id="@+id/button1"
    style="@style/calc_button"
    android:text="1" />
Source Link
ggorlen
ggorlen
  • 4.2k
  • 2
  • 19
  • 28

Crash

Dividing by 0 crashes the app ungracefully. Detect this and show an error message.

Warnings

Check your "Problems" tab in Android studio. It tells you to use @string resources rather than hardcoded strings, to use sp instead of dp for text sizes and to remove the unused import android.widget.Toast.

UX

Since your calculator can't handle multi-digit numbers or multiple operators, I'd prevent users from being able to enter these illegal expressions in the first place. As a user, it's frustrating to take the time to type in some long equation only to be told it's not possible. At the least, accepting multi-digit numbers is pretty easy to offer by not adding a space when a digit is pressed and equOnScreen's last character is also a digit.

As another UX improvement, having a visual color change upon button press can go a long way to making the app feel better.

Tweaks/nitpicks

  • There is excessive vertical whitespace in the layout.

  • Since you're using one click listener for all of your buttons, you could put them in an array and write a loop instead of setting the listener on each one by hand.

  • "$equOnScreen" + "0" + " " could be "$equOnScreen0 ".

  • The code:

    if(result != null) {
    return result
}
return null

    Is the same as: return result.

  • Button styling is repetitive; you could add a values/styles.xml like:

    <?xml version="1.0" encoding="utf-8"?>
<resources>
    <style name="calc_button">
        <item name="android:layout_width">100dp</item>
        <item name="android:layout_height">wrap_content</item>
        <item name="android:layout_gravity">center_horizontal|center</item>
    </style>
</resources>

    then use it on each button like:

    <Button
    android:id="@+id/button1"
    style="@style/calc_button"
    android:text="1" />

Design

  • It seems indirect to build up a string representing the current equation, then split it into a list later. Why not append to the list right from the get go, skipping the string? The reason appears to be you're using equOnScreen to store error messages as well as results and intermediate equations. But there's no reason to use it for error messages; you can set resultOnScreen.text = "error message" (which should be a resource in res/strings.xml) in the error branch and resultOnScreen.text = stringdataIntoCalc in the other. This way, you can avoid all the string manipulation and splitting on equOnScreen.
  • Your single onClick might be a bit overburdened. It might make sense to give number buttons their own handler, operators another one, and equals and clear to each have one as well. This separates concerns a bit and reduces the huge when that will be difficult to maintain if you try to add features to the app.
  • stringdataIntoCalc is somewhat awkward and hard to extend, but it does the job for the stripped-down specification here.

All this said, I'm glad you haven't been distracted by excessive premature optimizations and your straightforward, direct approach pretty much gets the job done commensurate with the goal.