This discussion is archived
9 Replies Latest reply: Jan 16, 2013 10:23 AM by abillconsl RSS

Can you look at my code?

984458 Newbie
Currently Being Moderated
This code is just made up of a single for loop, and multiple if statements. It allows the user to input a number to count from and then they input what they want to count to.

import java.util.Scanner;
class gh {
     public static void main(String[] args) {
          Scanner o = new Scanner(System.in);
          int a,b,c,d,e=1,f;;
          System.out.print("Enter Number to Count From: (0 = 1)");
          a = o.nextInt();
          if(a==0) {
               a=1;
          }
          if(a<0) {
               System.out.println("Number Cannot be 0 or Less");
          }else{
               System.out.print("Enter Number to Count To: ");
               b = o.nextInt();
               if(b<=0 || b==a){
                    System.out.println("Invalid");
               }else{
                    d=b+1;
                    f = b-a;
                    for(c=a;c<d;c++){
                         if(c==d) break;
                         System.out.println(c);
                    }
                    System.out.println("You have counted " + f + " times from " + a);
               }
          }
     }
}

As you can tell it's pretty basic, and the only thing I added apart from when I worked with this earlier was, the basic function to print out how many times the number has counted, and that if the user inputs 0 as a number to count from, it will automatically be placed as a 1.

I hope you like it, you can criticize me, or help me make it to the point where it can be more compact, or more advanced.

Edited by: 981455 on Jan 12, 2013 12:44 AM
  • 1. Re: Can you look at my code?
    EJP Guru
    Currently Being Moderated
    Does it do what it's supposed to do?
  • 2. Re: Can you look at my code?
    984458 Newbie
    Currently Being Moderated
    EJP wrote:
    Does it do what it's supposed to do?
    It fortunately does work. It does all of what it said, so you can use it as a source code to learn from, if you don't already know how to do it. But, if you do you use it, keep in mind that I am a beginner myself and this is pretty basic and can be more compact.
  • 3. Re: Can you look at my code?
    EJP Guru
    Currently Being Moderated
    You misunderstand. I am not exactly a beginner, and I am not seeking to learn anything from you. My point is that it is futile to ask people to review your code without mentioning whether or not it works as expected, and, if not, what it does instead, and also of course what was expected.
  • 4. Re: Can you look at my code?
    TPD-Opitz-Consulting-com Expert
    Currently Being Moderated
    You should follow Java naming conventins:
    http://www.oracle.com/technetwork/java/codeconvtoc-136057.html

    And aside this to attract the forum users learn froum rules and especially how to use code tags:
    https://forums.oracle.com/forums/ann.jspa?annID=1535

    bye
    TPD
  • 5. Re: Can you look at my code?
    aksarben Journeyer
    Currently Being Moderated
    Another tip: Don't use vague subject lines like "Can you look at my code?" This conveys nothing about what your problem is (or even if there is a problem at all). Be specific if you want people to actually read your question.
  • 6. Re: Can you look at my code?
    abillconsl Explorer
    Currently Being Moderated
    Please, first of all, use [ code ] tags (omit the spaces between the brackets and the word code when you do though). Example:

    [ code]
    import java.util.Scanner;
    class gh {
    public static void main(String[] args) {
    Scanner o = new Scanner(System.in);
    int a,b,c,d,e=1,f;;
    System.out.print("Enter Number to Count From: (0 = 1)");
    a = o.nextInt();
    if(a==0) {
    a=1;
    }
    if(a<0) {
    System.out.println("Number Cannot be 0 or Less");
    }else{
    System.out.print("Enter Number to Count To: ");
    b = o.nextInt();
    if(b<=0 || b==a){
    System.out.println("Invalid");
    }else{
    d=b+1;
    f = b-a;
    for(c=a;c<d;c++){
    if(c==d) break;
    System.out.println(c);
    }
    System.out.println("You have counted " + f + " times from " + a);
    }
    }
    }
    }
    [ code ]
    This will produce:
    import java.util.Scanner;
    class gh {
    public static void main(String[] args) {
    Scanner o = new Scanner(System.in);
    int a,b,c,d,e=1,f;;
    System.out.print("Enter Number to Count From: (0 = 1)");
    a = o.nextInt();
    if(a==0) {
    a=1;
    }
    if(a<0) {
    System.out.println("Number Cannot be 0 or Less");
    }else{
    System.out.print("Enter Number to Count To: ");
    b = o.nextInt();
    if(b<=0 || b==a){
    System.out.println("Invalid");
    }else{
    d=b+1;
    f = b-a;
    for(c=a;c<d;c++){
    if(c==d) break;
    System.out.println(c);
    }
    System.out.println("You have counted " + f + " times from " + a);
    }
    }
    }
    }
    So then you'll need to indent it - unless you past already indented code.

    Next, I would suggest not exiting when the end user enters a negative number (or some other undesireable number). Instead show the error message and loop back again. So this would suggest some filtering of the input too, because if the end user enters, say a "b", or a number greater than an int can hold you get an exception thrown. You really don't want that, but you want the end user just to see a message explaining what is wrong with the entry.

    So I would suggest that you return "next()" instead of nextInt() and send the result of the call to a method that validates it in all the ways you want ... is an int, and is not greater than a certain amount, etc. Return a boolean from this method.

    Put both reads in loops like
    while (true) {
    ... and break from the loop when you get a good entry and loop back if you don't.

    These are just a couple of things to consider. And there are many other specific ways to handle it, but I am trying to offer advice that is perhaps just a step or up up from where you are.

    Good going on the early attempt and keep working at it!

    ~Bill

    Edited by: abillconsl on Jan 15, 2013 3:25 PM
  • 7. Re: Can you look at my code?
    800268 Expert
    Currently Being Moderated
    Eclipse says:
    Line 5: The value of the local variable e is not used
    Line 5: Unnecessary semicolon
    You have bad variable names:
    o = input (or scanner)
    a = countFrom
    b = countTo
    c = i (only used as for loop counter)
    d is redundant, your for-loop condition just could be (i <= countTo)
    f = numbersCounted

    You should declare variables in the smallest scope possible:
    int countFrom = input .nextInt();
    int countTo= input .nextInt();
    int numbersCounted = countTo - countFrom;
    for(int i = countFrom; i <= countTo; i++) {
    Your if inside the loop is never true (loop condition c < d means c is never d)

    The text of the last print out is incorrect (you only count one time from a (to b)). Perhaps you meant to print
    "You counted " + numbersCounted + " numbers " from " + countFrom (for bonus do a separate message whena single number is counted).
  • 8. Re: Can you look at my code?
    939520 Explorer
    Currently Being Moderated
    Another suggestion:
    Don't put business logic in main(). Instead, instantiate the class in main and call a function that performs the business logic.
    Also, javadoc on what the class and/or functions do (not how it does it)
    Here's an example, unrelated to your code:
     
    /** asks user to input a set of integers, displays them sorted */ 
    class NumberSorter { 
    
      public void performSort(){ 
      } 
    
      public static void main(String[] args) { 
         
        NumberSorter sort = new NumberSorter(); 
       
        sort.performSort(); 
      } 
    } 
  • 9. Re: Can you look at my code?
    abillconsl Explorer
    Currently Being Moderated
    936517 wrote:
    Another suggestion:
    Don't put business logic in main(). Instead, instantiate the class in main and call a function that performs the business logic.
    Respectfully, those are two separate issues. The first issue has to do with segmentation by function ... MVC. The second has more to do with OOPS. But I wouldn't expect a beginner posting this type of code to be too interested in the former at this time, while the later is indeed a good suggestion.

    ~Bill

Legend

  • Correct Answers - 10 points
  • Helpful Answers - 5 points