[help!] java program - one more final touch

Discussion in 'Mac Programming' started by makbarg, Oct 4, 2010.

  1. macrumors newbie

    Joined:
    Aug 7, 2010
    #1
    i cant find out how to solve this program error:
    it says break cannot be used outside of loop or switch.. but still...


    Code:
    import java.util.*;
    public class Store {
    	
    	public static void main (String[] args){
    	Scanner console = new Scanner(System.in);
    	
    	String 	movieTitle, status, userIn;
    	double 	movieLength;
    	int i; 
    	int indexRent = 0;
    	int indexReturn = 0;
    	
    	
    	DVD dvd[]	= new DVD [6];
    	dvd[0] 		= new DVD ("Iron Man",3.5,"In");
    	dvd[1] 		= new DVD ("Death Note",2.5,"In");
    	dvd[2] 		= new DVD ("Shrek",2,"In");
    	dvd[3] 		= new DVD ("Transformer",3.25,"In");
    	dvd[4] 		= new DVD ("The Eye",3,"In");
    	dvd[5]	 	= new DVD ("Fantastic 4",4,"In");
    	
    	System.out.println("ENTER COMMAND:");
    	System.out.println("A) Renting DVD(s)");
    	System.out.println("B) Returning DVD(s)");
    	System.out.println("C) All DVD informations");
    	System.out.println("D) Exit program");
    	System.out.println("Please enter your choice: ");
    	
    	userIn = console.nextLine();
    	
    		if (userIn.compareTo("A") == 0 || userIn.compareTo("a") == 0)	{
    			System.out.println("Enter DVD title: ");
    			String movieTitleReturn = console.nextLine();
    				for(i=0; i<dvd.length; i++)	
    					{
    					if(movieTitleReturn.equals(dvd[i].getMovieTitle()));
    					indexRent = i;
    					break;
    					}
    				if(indexRent == i)
    					{
    					dvd[indexRent].dvdRent();
    					}
    				
    				else
    					{
    					System.out.println("Invalid input - back to main menu");
    					}
    				
    		}
    		
    		else if (userIn.compareTo("b") == 0 || userIn.compareTo ("B") == 0)	{
    			
    			System.out.println("Enter DVD title: ");
    			String movieTitleReturn = console.nextLine();
    			for(i=0; i<dvd.length; i++)
    				{
    				if(movieTitleReturn.equals(dvd[i].getMovieTitle()));
    				indexReturn = i;
    				break;
    				}
    			if(indexReturn == i)
    				{
    				dvd[indexReturn].dvdReturn();
    				}
    			
    			else
    				{
    				System.out.println("Process cannot be done - wrong movie title");
    				}
    
    		}
    		
    		else if (userIn.compareTo("c") == 0 || userIn.compareTo ("C") == 0)	{
    			for(i=0; i == dvd.length; i++)
    				{
    				dvd[i].printDetails();
    				}
    			
    		}
    		
    		else if (userIn.compareTo("d") == 0 || userIn.compareTo ("D") == 0)	{
    			
    			System.out.println("Exiting program");
    			break;
    			}
    		else
    			{
    			System.out.println("Input is invalid");
    			}
    	}
    }



    thanks for your time.
     
  2. macrumors member

    naples98

    Joined:
    Sep 9, 2008
    Location:
    Houston
    #2
    Take another look at each of the for loops where you use break. Neither one of those loops ever fully executes which is probably why you are getting the error.

    Follow the logic in each for loop and try to figure out why they don't ever execute at least one time through.
     
  3. macrumors 68040

    elppa

    Joined:
    Nov 26, 2003
    #3
    Use System.exit(0) instead of break on line 77.

    You are not in a loop at that stage.
     
  4. macrumors member

    naples98

    Joined:
    Sep 9, 2008
    Location:
    Houston
    #4
    I missed the break on line 77 but you don't need to use System.exit(0) or break on that line.
     
  5. macrumors 68040

    plinden

    Joined:
    Apr 8, 2004
    #5
    It's unrelated to the problem you mention here, but this loop doesn't look right. See if you can work out what i is at the break.
    Code:
    	for(i=0; i<dvd.length; i++)	
    	{
    		if(movieTitleReturn.equals(dvd[i].getMovieTitle()));
    		indexRent = i;
    		break;
    	}
     
  6. macrumors 68040

    lee1210

    Joined:
    Jan 10, 2005
    Location:
    Dallas, TX
    #6
    There's plenty going on here... naples98 has pointed out a few serious problems already. I will just state that you should always use {} for the body of an if. Using a few extra lines of code is OK, and it will help avoid the problem you just encountered.

    Also:
    Code:
    for(i=0; i == dvd.length; i++)
    This is very unlikely what you want. Execution will stop whenever i is not equal to the length of your array. (looks like plinden already mentioned this)

    movieTitle, status, and movieLength are never used. May as well get rid of them to avoid confusion.

    indexRent and indexReturn are only initialized once. Initialize them each time you are going to use them.

    elppa told you why you got the error relating to the use of break outside of a loop... but i think it's not wrong that you have a break there, but rather that you're missing a loop. Right now, when the user runs this they get to perform exactly one action. This means messages like "back to the main menu" are a lie. You need a big loop for the main menu, at which point that break will do exactly what you want.

    -Lee
     
  7. macrumors 65816

    macsmurf

    Joined:
    Aug 3, 2007
    #7
    You don't need to declare variables in the beginning of the method as in C. Declare them when you need them. There are several variables you don't use.

    You don't need to iterate through an array using an integer as in C. Use foreach instead (see code). Better yet, use a List instead of an array.

    Nested loops and if statements + breaks are evil. They make the code hard to read and hard to debug. Use private methods instead.

    Normalize the input before you test it (toLowercase in this instance);

    Don't use compareTo when you don't need to establish ordering. Use equals() instead.

    Don't prefix all your methods in DVD with dvd. It is already apparent that we're talking about DVDs. return() is an invalid method name in Java so I would use checkIn()/checkOut() instead.

    Don't Repeat Yourself (This is called the DRY principle). Extract duplicated code into methods.

    Use a while loop to check for exit command.

    Use proper indentation. In other words, follow the Java coding guidelines.

    Use methods such as isIn() to check if a movie is in. Don't use text strings in the constructor to specify whether a movie is in or out. Use a boolean (or just assume that it's in at construction time).

    I've made a new version of your program. I would probably make a Store class and make a tiny Main class to start it up, but it should show you what I mean. Ask yourself: which version would you rather read/debug? (I haven't handled the case that a movie can be rented out several times.)

    EDIT: I've removed most of the code to avoid you risking being accused of cheating. The comments and the below should get you started

    Code:
    import java.util.Scanner;
    
    public class Store {
    
        private DVD[] dvds = {
                new DVD("Iron Man", 3.5, true),
                new DVD("Death Note", 2.5, true),
                new DVD("Shrek", 2, true),
                new DVD("Transformer", 3.25, true),
                new DVD("The Eye", 3, true),
                new DVD("Fantastic 4", 4, true)
        };
    
        public static void main(String[] args) {
            Store store = new Store();
            store.execute();
        }
    
        private void execute() {
            Scanner console = new Scanner(System.in);
            String mainCommand = getMainCommandFromUser(console);
    
            while (!mainCommand.equals("d")) {
                if (mainCommand.equals("a")) {
                    rentCommand(console);
                } else if (mainCommand.equals("b")) {
                    returnCommand(console);
                } else if (mainCommand.equals("c")) {
                    printDetails();
                } else {
                    invalidInput();
                }
                mainCommand = getMainCommandFromUser(console);
            }
    
            System.out.println("Exiting program");
        }
    
        private String getMainCommandFromUser(Scanner console) {
            printMainMenu();
            return console.nextLine();
        }
    
        private void printMainMenu() {
            System.out.println("");
            System.out.println("ENTER COMMAND:");
            System.out.println("A) Renting DVD(s)");
            System.out.println("B) Returning DVD(s)");
            System.out.println("C) All DVD informations");
            System.out.println("D) Exit program");
            System.out.println("Please enter your choice: ");
        }
    
        // TODO
    
        private void invalidInput() {
            System.out.println("Invalid input");
        }
    }
    
     
  8. macrumors 603

    Joined:
    Aug 9, 2009
    #8
    This is obviously a homework program. By writing it for them, you've effectively made the OP into a plagiarist or cheater if they hand in this program as their own.

    The usual practice here is to explain what to fix or change, but to leave the actual programming to the OP.

    And I agree with most of your suggestions, except they may not have been taught or shown the foreach loop yet. Ditto for the use of List. And I wouldn't use an if-else chain on a String, but a switch on first letter.
     
  9. macrumors 65816

    macsmurf

    Joined:
    Aug 3, 2007
    #9
    I agree somewhat and I would normally not do that. However, the code is so badly broken that I felt the only way to explain the issues was to make an example. All my points are general in nature. Also, the guy has obviously worked at it for a while. In addition, the code is not complete and still have some issues that I didn't mention.

    Furthermore, it is my experience that beginning programming classes don't focus on good code but more on working code.

    But you have a point, of course :)

    The main problem is that you can't make switches on strings in Java.
     
  10. macrumors 6502

    Joined:
    Aug 10, 2008
    #10
    I think a beta for Java7 is slated for release mid October, and will get that feature.
     
  11. macrumors 68000

    ulbador

    Joined:
    Feb 11, 2010
    #11
    If I was going to do this I would probably use static ints, or maybe even the hashcode of the string...

    Code:
    String myColor = "red";
    
    switch (myColor.hashcode()) {
    
    case "red".hashcode():
    System.out.println("You had red");
    break;
    
    
    }
    

    If you are doing just single characters instead of multiple characters (two different data types), you can probably switch directly on it.

    case 'a' not case "a".. Single quotes will tell it that it is a character and not a string.

    If that doesn't work, a single character can be converted directly to an integer:

    The getNumericValue() method of the Character object should be what you need.
     
  12. macrumors 65816

    macsmurf

    Joined:
    Aug 3, 2007
    #12
    Yeah, I've read that too. However, closures have been booted out again, which is extremely disappointing. Don't get me started ;)
     
  13. macrumors 603

    Joined:
    Aug 9, 2009
    #13
    Which is why I said "on first letter", which would be a char, which can be used as a case value. And for better similarity between the main-menu's instructions, which use upper-case letters, I would have used upper-case as the canonical form. Example:
    Code:
    case 'A': ..code here..
       break;
    case 'B': ..code here..
       break;
    
    This assumes, however, that switch/case has been taught.
     

Share This Page