3
\$\begingroup\$

I would like to know how I might improve my code? I just started coding and liking it a few days ago so I am quite new to this whole world...

The objective is to calculate the amount of water one has drunk in a day. Depending on the user's input, the app tells him or her if the amount of water drunk is great or not.

Now I would like know how I might improve this whole code? And I'd be grateful for some advice (what I could improve etc)...

package com.ahmed.everything;
import java.util.*;
import java.util.concurrent.TimeUnit;

public class DrinkingWater {
    public static void main(String [] args) throws Exception {
    Scanner sc = new Scanner(System.in);

    System.out.print("Amount of water your bottle can hold: ");
    // entering amount of water bottle can hold
    double litre = sc.nextDouble();
    System.out.print("How many bottles have you drunk today? : ");
    // amount of bottles for today
    double bottles = sc.nextDouble(); 
    // calculating water drunk
    double total = (bottles * litre);

    // result
    if (bottles == 1) {
    System.out.print("You've only drunk one bottle today! :(" + "\n");
    }
    else {
        System.out.println("Today, you have drunk " + bottles + " bottles. Which equals to " + total + " liters of water.");
    }

    // advice
    if (total < 4) {
        System.out.println("You did not drink enough water, drink at least 4 liters to stay healthy!");
    }

    else if (total >= 4) {
        System.out.println("Good job! You've drunk enough water today! :)");
    }
    else;


    //short time span
    TimeUnit.SECONDS.sleep(1);
    System.out.print(".");
    TimeUnit.SECONDS.sleep(1);
    System.out.print(".");
    TimeUnit.SECONDS.sleep(1);
    System.out.println(".");
    TimeUnit.SECONDS.sleep(1);

    System.out.println("Did you like our little app? Please review it from 1-5. 1 being considerer as very good and 5 as very bad.");
    int note = sc.nextInt();
    if (note <= 4 || note >= 1) {
        System.out.println("Thanks for your review.");
    }else {
        System.out.println("That's better, we'll do our best the next time");
    }

    }
}
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Please check that I haven't misrepresented your code, and correct it if I have. \$\endgroup\$ – Toby Speight Jul 4 at 10:56
3
\$\begingroup\$

Check your if expression in review part:

if (note <= 4 || note >= 1) {
    System.out.println("Thanks for your review.");
}else {
    System.out.println("That's better, we'll do our best the next time");
}

This will always print "Thanks for your review" since whatever the value of note will be always true either for <= 4 or >= 1.

Using an IDE (for example IntelliJ, netbeans, eclipse) helps you mitigate this kind of problems, but it's still better to always review your code.

mitigate always true in if expression


Confusing result and advise:

I'm not sure if the application is happy that I drank enough water or sad because I only drank 1 bottle.

As stated in your question:

The objective is to calculate the amount of water one has drunk in a day. Depending on the user's input, the app tells him or her if the amount of water drunk is great or not.

Then I think you don't need to tell them how many bottles they drink or at least remove the sad face to avoid confusion.

confusing result and advise


User input problems:

Case 1: What if the user input a letter instead of number? This will happen:

enter image description here

Case 2: What if the user input a negative number? Then this will happen:

enter image description here

As you can see in case 1, this will break your program. Although in case 2 the program will not crash, I think it's impossible to drink negative amount of water.

To solve this kind of issue, you can use try/catch expression. Something like this:

double litre = 0;
boolean isInvalidInput = false;
do {
    try {
        System.out.print("Amount of water your bottle can hold: ");
        // entering amount of water bottle can hold
        litre = sc.nextDouble();
        isInvalidInput = false;
    } catch (InputMismatchException e) {
        isInvalidInput = true;
    }
    sc.nextLine();
} while(isInvalidInput || litre < 0);

If you use the code above, this will be the output:

tryCatch

As you can see, the program will ask the same question repeatedly until the user enters a correct input

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for being so kind and reviewing my code. I do not know how to use try/catch expressions yet. Just put it on my to-do list. Thanks again! I'll try it out \$\endgroup\$ – Djabone Jul 8 at 10:37
1
\$\begingroup\$
  • Do not forget to close the scanner
  • No need for obvious comments, try to comment why not what, we understand what already
  • Why do you have the empty else; ?
  • Be consistent using println, no need for + "\n"
  • Make use of printf
  • Use loops or methods instead of copy / paste
  • Your last condition is always true, add tests to your apps
  • if (bottles == 1) but no sad face for 0 bottles? It is even worse..
  • Initialise / introduce variables just before you need them, not somewhere irrelevant (see how I moved double total = (bottles * litre);)

Here is what I have after applying the changes..

import java.util.Scanner;
import java.util.concurrent.TimeUnit;

class DrinkingWater {
    public static void main(String[] args) throws Exception {
        try (Scanner sc = new Scanner(System.in)) {
            System.out.print("Amount of water your bottle can hold: ");
            double litre = sc.nextDouble();

            System.out.print("How many bottles have you drunk today? : ");
            double bottles = sc.nextDouble();

            if (bottles == 1) {
                System.out.println("You've only drunk one bottle today! :(");
            } else {
                System.out.printf("Today, you have drunk %s bottles. Which equals to %s liters of water.\n", bottles, total);
            }

            double total = (bottles * litre);
            if (total < 4) {
                System.out.println("You did not drink enough water, drink at least 4 liters to stay healthy!");
            } else if (total >= 4) {
                System.out.println("Good job! You've drunk enough water today! :)");
            }

            for (int i = 0; i < 3; i++) {
                TimeUnit.SECONDS.sleep(1);
                System.out.print(".");
            }

            System.out.println("Did you like our little app? Please review it from 1-5. 1 being considerer as very good and 5 as very bad.");
            int note = sc.nextInt();
            if (note <= 4 || note >= 1) {
                System.out.println("Thanks for your review.");
            } else {
                System.out.println("That's better, we'll do our best the next time");
            }
        }
    }
}

Keep up the good work.

\$\endgroup\$
  • \$\begingroup\$ Thank you so much for your time as well. I'll try to see if I understand everything it now does and will get back to you asap :) \$\endgroup\$ – Djabone Jul 8 at 11:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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