0
\$\begingroup\$

I have been programming about 4 months now and I am looking some way to improve my code. Currently I am using a book "JAVA an introduction to problem solving and programming" to learn programming. I think I understand basics of loops and if statements.

After couple of projects in this book, I feel like I am only using what I know and not trying to use new things to improve my code. I hope you guys can critique my code and help me improve.

My questions are...

  1. How is the structure?
  2. What other methods can I use to improve this program?
  3. Any other criticism and advice you wish to tell me.

Picture of the problem.

Suppose we can buy a chocolate bar from the vending machine for $1 each. Inside every chocolate bar is a coupon. We can redeem six coupons for one chocolate bar from the machine. This means that once you have started buying chocolate bars from the machine, you always have some coupons. We would like to know how many chocolate bars can be eaten if we start with N dollars and always redeem coupons if we have enough for an additional chocolate bar.

For example, with 6 dollars we could consume 7 chocolate bars after purchasing 6 bars giving us 6 coupons and then redeeming the 6 coupons for one bar. This would leave us with one extra coupon. For 11 dollars, we could have consumed 13 chocolate bars and still have one coupon left. For 12 dollars, we could have consumed 14 chocolate bars and have two coupons left.

Write a program that inputs a value for N and outputs how many chocolate bars we can eat and how many coupons we would have left over. Use a loop that continues to redeem coupons as long as there are enough to get at least one chocolate bar.

import java.util.Scanner;

public class chap4num9 {

    public static void main(String[] args) {
        Scanner userInput = new Scanner(System.in);

        System.out.println("How much money do you have?");
        int userMoney = userInput.nextInt();

        int chocoBars = 0;
        int userCoupon = 0;     

        chocoBars =+ userMoney;

        if (userMoney >= 6) {
            chocoBars = chocoBars + (userMoney / 6);
            userCoupon = userMoney / 6;
        }

        while (userCoupon >= 6) {
            chocoBars = chocoBars + 1;
            userCoupon = userCoupon - 6;
        }

        System.out.println("You will have total of " + chocoBars + " bars and " + userCoupon + " coupons left.");

    }
}
\$\endgroup\$
2
  • 3
    \$\begingroup\$ Please include a transcript of the problem statement instead of a photo. \$\endgroup\$
    – Mast
    Commented Sep 13, 2018 at 14:59
  • 1
    \$\begingroup\$ Welcome to Code Review! Your image of text isn't very helpful. It can't be read aloud or copied into an editor, and it doesn't index very well. Please edit your post to incorporate the relevant text directly. \$\endgroup\$ Commented Sep 13, 2018 at 15:21

2 Answers 2

2
\$\begingroup\$

chap4num9 is not a proper Java class name. Java class names begin with a capital letter and each word starts with an uppercase letter. It should be Chap4Num9.

Always close Closeable resources like Scanner. Prefer the use of a try-finally block to do so.

userMoney should be final. This reduces the cognitive load on the reader of your code, since they know the value will not change after it is set.

There are no chocoBars. There are chocolateBars. Don’t use abbreviations or shorthand. It’s harder to read, and there are no assurances that the reader will know what you’re trying to describe.

userCoupon could probably be just coupons. It’s easier to read, and should be plural because there’s probably more than one. Likewise userMoney and money.

Don’t assign chocolateBars and then immediately reassign it. Just set it to the value it’s supposed to be.

While you don’t need a money variable, it does make it easier to understand that money = # of chocolate bars. If you really wanted to, you could remove it by doing something like int chocolateBars = userInput.nextInt(); int coupons = chocolateBars;.

Doing a computation to minimize your number of loop iterations is a premature optimization that makes the code harder to read. Write code to be easy to read until you have an actual performance problem and you know where the bottleneck is. Then test to make sure your optimization actually fixed the problem.

Your code has two bugs in it. (0) You have the incorrect number of coupons after your if block. If money < 6, coupons is 0. If money >= 6, your optimization computes the wrong value. (1) If you have N coupons and you spend 6 to gain a chocolate bar and a coupon, you should have N - 5 coupons, not N - 6. So with 8 money, you should wind up with 9 bars and 3 coupons.

If you were to apply all my suggestions, your code might look something like:

import java.util.Scanner;

public class Chap24Num9 {

    public static void main(final String[] args) {
        try (final Scanner userInput = new Scanner(System.in)) {
            System.out.println("How much money do you have?");
            final int money = userInput.nextInt();

            int chocolateBars = money;
            int coupons = money;

            while (coupons >= 6) {
                coupons = coupons - 6;
                chocolateBars = chocolateBars +1;
                coupons = coupons + 1;
            }

            System.out.println("You will have total of " + chocolateBars + " bars and " + coupons + " coupons left.");

        }
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ Helped me think through my code one more time. Thanks a lot. \$\endgroup\$
    – coco
    Commented Sep 16, 2018 at 1:23
1
\$\begingroup\$

In addition to Eric Stein's great remarks:

1) You should always store "magic numbers" - in this case the number of coupons needed for a free bar - in a constant. Especially in a case where you use the same number in multiple places (possibly in multiple files) and you need to change that number, things go very wrong, if you miss any of those numbers.

private final int NUMBER_OF_COUPONS_FOR_ADDITIONAL_BAR = 6;

2) There is no need to use a loop here to calculate the values here. Using integer division and the modulo operation they can be calculated directly. To find the formulas I used pencil and paper any write down the results for the money amounts 1 to 16 and used that to identify patterns. People with more practice could probably determine them directly from the text.

final int money = userInput.nextInt();

int chocolateBars = money + (money - 1) / (NUMBER_OF_COUPONS_FOR_ADDITIONAL_BAR - 1);
int coupons = (money - 1) % (NUMBER_OF_COUPONS_FOR_ADDITIONAL_BAR - 1) + 1;

System.out.println("You will have total of " + chocolateBars + " bars and " + coupons + " coupons left.");

(EDIT: Just realized the task explicitly asks for a loop, oh well)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.