15
\$\begingroup\$

The code has been test to work by the parameters listed below I just wanted to know if there were any details that I missed or blatantly overlooked.

Develop a Java application that determines whether any of several department-store customers has exceeded the credit limit on a charge account. For each customer the following facts are available:

  • account number

  • balance at the beginning of the month

  • total of all items charged by the customer this month

  • total of all credits applied to the customers account this month

  • allowed credit limit

The program should input all of these facts as integers, calculate the new balance (new balance = beginning balance + charges - credits) For those customers whose credit limit is exceeded, the program should display the message "Credit Limit Exceeded".

import java.util.Scanner;

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

      int accnum = 0;
      int begBalance = 0;
      int itemTotal = 0;
      int creditsApplied = 0;
      int creditLimit = 0;
      int newBalance = 0;

      System.out.println("Welcome to the Credit Calculator");

      System.out.println("Please Enter Customer Account Number (or -1 to quit): "); 
      accnum = input.nextInt();
        if (accnum != -1)
        {
            System.out.println("Please Enter Customer Beginning Balance: "); 
          begBalance = input.nextInt();
            if (begBalance < 0)
            {
                System.out.println("Please enter a positive amount");
                System.out.println("Please Enter Customer Beginning Balance: ");
                begBalance = input.nextInt();   
            }
        }



      while(accnum != -1)
      {
         System.out.println("Please Enter Total of Items Purchased this Month: "); 
         itemTotal = input.nextInt();
            if (itemTotal < 0)
            {
                System.out.println("Please enter a positive amount");
                System.out.println("Please Enter Total of Items Purchased this Month: ");
                itemTotal = input.nextInt();    
            }


         System.out.println("Please Enter Credits applied to Customer Account: "); 
         creditsApplied = input.nextInt();

         System.out.println("Please Enter Customer Credit Limit: "); 
         creditLimit = input.nextInt();


         newBalance = begBalance + itemTotal - creditsApplied;




            if(newBalance > creditLimit)
            {
                System.out.println("Credit Limit Exceeded");
            }



            System.out.println("Please Enter Customer Account Number (or -1 to quit): "); 
              accnum = input.nextInt();
                if (accnum != -1)
                {
                    System.out.println("Please Enter Customer Beginning Balance: "); 
                  begBalance = input.nextInt();
                    if (begBalance < 0)
                    {
                        System.out.println("Please enter a positive amount");
                        System.out.println("Please Enter Customer Beginning Balance: ");
                        begBalance = input.nextInt();   
                    }
                }


      }

        if(accnum < 0)
        {
            System.out.println("Thank You for using Credit Calculator");
            System.out.println("Goodbye");
        }


   }

}
\$\endgroup\$
  • 3
    \$\begingroup\$ One thing that is fairly important on this site (something we won't help with, as it is part of the review) is formatting the code properly. Follow a standard and stick to it. Right now the code is not very pleasant to read that. \$\endgroup\$ – Emz Dec 22 '15 at 10:56
7
\$\begingroup\$

Loop logic

There's definitely something strange going on with your looping. Why do you have so many checks on accnum equalling zero? Why do you ask for account number in multiple places?

As it is, it's hard to even answer that by eyeballing the code, because it's so bulky. So let's start by pulling some things into methods:

private int getAccountNumber(Scanner input) {
    System.out.println("Please Enter Customer Account Number (or -1 to quit): "); 
    return input.nextInt();
}

We can take the same approach with the other input/output sections, to now get a clearer version of your algorithm:

  System.out.println("Welcome to the Credit Calculator");

  accnum = getAccountNumber(input);
  if (accnum != -1)
  {
      begBalance = getBeginningBalance(input);
  }

  while(accnum != -1)
  {
     itemTotal = getItemTotal(input);
     creditsApplied = getCreditsApplied(input);
     creditLimit = getCreditLimit(input);

     newBalance = begBalance + itemTotal - creditsApplied;

     if(newBalance > creditLimit)
     {
         System.out.println("Credit Limit Exceeded");
     }

     accnum = getAccountNumber(input);
     if (accnum != -1)
     {
         begBalance = getBeginningBalance(input);
     }
  }

   if(accnum < 0)
   {
       System.out.println("Thank You for using Credit Calculator");
       System.out.println("Goodbye");
   }

Okay, now we see the problem more clearly, those first lines before the while loop are checked again at the end of the loop. That's definitely something we can improve on, so let's do it:

  System.out.println("Welcome to the Credit Calculator");

  while(true)
  {
     accnum = getAccountNumber(input);
     if (accnum == -1)
     {
         break;
     }

     begBalance = getBeginningBalance(input);
     itemTotal = getItemTotal(input);
     creditsApplied = getCreditsApplied(input);
     creditLimit = getCreditLimit(input);

     newBalance = begBalance + itemTotal - creditsApplied;

     if(newBalance > creditLimit)
     {
         System.out.println("Credit Limit Exceeded");
     }
  }

  System.out.println("Thank You for using Credit Calculator");
  System.out.println("Goodbye");

Notice how I also removed that last check- if we've broken out of that loop, we already know that accnum < 0 is true.


Formatting

It's important to pick good conventions and stick with them. One guideline is the language. For example, in Java it's conventional to put curly braces on the same line, rather than a new one (like I did in that getAccountNumber example). It's also conventional to use camelCased variables without too much abbreviation: accountNumber rather than accnum. Make sure you're consistent with your indentation too, as that's one way the eye quickly gauges control flow.


Usability

None of your requirements actually say anything about looping until the user enters some special quit code. Is this really what you want? Have you considered just running through the process once? Or looping forever, forcing the user to quit out by using whatever means the console provides for doing so, rather than providing your own mechanism?

This is somewhat a judgement call, but having to enter a specially defined magic number in a particular place isn't a very user-friendly way to quit a program, so either of the above might be a better user experience. They also mean you now have a simpler program- you want to guard against adding extra code or features that don't actually match any of your requirements.

Similarly, do you actually need this question/answer style? Would receiving the information from command line arguments be acceptable? If so, you could cut your entire program down to a few lines.


A note on OO

One last thing, since I noticed a few other answers mentioning taking a more OO approach, I'm going to disagree.

In my opinion one of the best ways to make sure you misunderstand OO is to apply it where it's not appropriate. Here your entire application is concerned with input/output to the console, the only real behaviour it has other than UI concerns is a single very simple calculation and a greater than check. Trying to "object-orient" that is much more likely to baffle than teach.

Understanding OO is a very important part in becoming a proficient Java programmer, but I'd say that you definitely took the right approach keeping this particular program procedural.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your input this is very important on my path to mastering Java. I have some questions for clarification, when I find myself typing a large bulky program such as this should I stick to a general rule of breaking it down into classes and set/get methods? \$\endgroup\$ – WXHXIXTE Dec 22 '15 at 22:06
  • \$\begingroup\$ Also is the consensus is that the format is poorly constructed or that it is just bulky, I wrote this in eclipse and it turn out great format wise but transferring it here was were I ran into a few issues. But again thank you. \$\endgroup\$ – WXHXIXTE Dec 22 '15 at 22:20
  • \$\begingroup\$ @WXHXIXTE Well, this actually isn't a large program. Compared to almost all real software projects in the wild, it's tiny. I would suggest you first focus on splitting things out into methods like I did, and only once you start working on project where that's not sufficient should you start thinking about classes. \$\endgroup\$ – Ben Aaronson Dec 23 '15 at 14:13
  • \$\begingroup\$ @WXHXIXTE For now, probably the main clue you should be looking out for that something would make a good class is if you have some operations which work on a particular set of data. If you can create a class that encapsulates that data (keeps it private) and provides the same operations, then it's probably a good idea. \$\endgroup\$ – Ben Aaronson Dec 23 '15 at 14:17
2
\$\begingroup\$

OO

I suggest an Object Oriented approach:

Make a CreditInfo class and the main part of your program will be just:

CreditInfo info = CreditInfo(
    positiveInput("Enter ...") ...
);
System.out.Println(info.calculateCredit());

The positiveInput function will allow for nicer input reading. You may put in a UserInterface class.

\$\endgroup\$
2
\$\begingroup\$

Structure

Right now, you just have all your code in the main method. This is not good for several reasons: It's hard to maintain, it's hard to test (you have to manually enter the data you want to test each time!), and it's hard to reuse part of the code.

So what would be a better approach? OOP certainly comes to mind. If you haven't looked into it yet, I would suggest to do that.

For such a small task, splitting the code into different methods might be enough though.

What you definitely want is to extract the code that does the actual calculation for each customer in such a way that it is independent of any input or output (ie it accepts the given facts as input, and returns true or false). This will make it easily reusable and also (automatically) testable.

Formatting

Your formatting is quite hard to read and not consistent. Your indentation is off, you have quite a lot of unnecessary vertical whitespace, etc.

Just paste your code into any IDE and let it format it for you.

Misc

  • Declare variables as late as possible, instead of at the beginning of a method. This increases readability.
  • Do not shorten variable names, as it decreases readability. A reader has to spent time thinking about what eg beg in begBalance means.
  • Try to avoid duplication (eg the second print when wrong data was entered, or how integer input is read in general). You could for example write a method int getInteger(String promptText, String errorText) which is then used to get the begBalance, itemTotal, etc.
\$\endgroup\$
2
\$\begingroup\$

Welcome to CodeReview, please thoroughly read through the remarks. Feel free to post a follow-up question when you've implemented all the remarks you deemed be considered as overall positive feedback.

Some nit-picks:

Code Style

Right now the code is not very pleasant to read as you are very inconsistent with spaces and line breaks.

Magic Constants

(or -1 to quit) if I miss that line I might not understand the following either:

  • if (accnum != -1)
  • accnum != -1
  • ...

Instead move it to a constant like private final static int QUIT_CODE = -1; and make use of that in the code, it makes it much more clear what the -1 represents, also makes it easier to change it in the future if you so decide.

Result

public class CreditCalc
{
    private final static int QUIT_CODE = -1;

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

        ...

        System.out.println ("Welcome to the Credit Calculator");
        System.out.println ("Please Enter Customer Account Number (or " + QUIT_CODE + " to quit): "); 
        accnum = input.nextInt ();
        if (accnum != QUIT_CODE) {

        ...

Also note I added spaced after method calls.

Object Orientation

The goal somewhere along the pipeline is to make sure that one .class is responsible for one thing. I would move credit card information to a separate class. Then just access is with getters/setters.

\$\endgroup\$
  • 1
    \$\begingroup\$ You don't need to close System.in (and you probably shouldn't). \$\endgroup\$ – assylias Dec 22 '15 at 11:36
  • \$\begingroup\$ @assylias, I stand corrected. Reference In this case particular case it doesn't matter if you close it or not. However if you later try to invoke System.in you can't. You will get an exception. So it is very bad practice to close it. \$\endgroup\$ – Emz Dec 22 '15 at 11:39
  • \$\begingroup\$ My personal experience is to still wrap it with try-with-resources, and pass that Scanner instance around until the program terminates. \$\endgroup\$ – h.j.k. Dec 22 '15 at 14:50
  • \$\begingroup\$ As soon as the try-with-resource block ends it gets released no? \$\endgroup\$ – Emz Dec 22 '15 at 15:08
  • \$\begingroup\$ @Emz right, so the idea is to handle all usage of System.in with the Scanner instance, inside the try-block. If it so happens that a program is jumping between user inputs and other processing logic, it may result in an usually large try-block in main(). Since this is the intended usage, I don't think it's necessarily a code smell (when it comes to tighter scoping of variables). \$\endgroup\$ – h.j.k. Dec 22 '15 at 23:53

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.