PDA

View Full Version : [help!] java program - one more final touch




makbarg
Oct 4, 2010, 10:22 AM
i cant find out how to solve this program error:
it says break cannot be used outside of loop or switch.. but still...


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.



naples98
Oct 4, 2010, 11:15 AM
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.

elppa
Oct 4, 2010, 11:20 AM
Use System.exit(0) instead of break on line 77.

You are not in a loop at that stage.

naples98
Oct 4, 2010, 11:34 AM
Use System.exit(0) instead of break on line 77.

You are not in a loop at that stage.

I missed the break on line 77 but you don't need to use System.exit(0) or break on that line.

plinden
Oct 4, 2010, 11:42 AM
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.
for(i=0; i<dvd.length; i++)
{
if(movieTitleReturn.equals(dvd[i].getMovieTitle()));
indexRent = i;
break;
}

lee1210
Oct 4, 2010, 11:53 AM
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:
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

macsmurf
Oct 4, 2010, 01:34 PM
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


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");
}
}

chown33
Oct 4, 2010, 02:29 PM
... I've made a new version of your program. ...

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.

macsmurf
Oct 4, 2010, 02:37 PM
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.

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 :)

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.

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

mrbash
Oct 4, 2010, 02:57 PM
The main problem is that you can't make switches on strings in Java.

I think a beta for Java7 is slated for release mid October, and will get that feature.

ulbador
Oct 4, 2010, 03:00 PM
The main problem is that you can't make switches on strings in Java.

If I was going to do this I would probably use static ints, or maybe even the hashcode of the string...


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.

macsmurf
Oct 4, 2010, 03:22 PM
I think a beta for Java7 is slated for release mid October, and will get that feature.

Yeah, I've read that too. However, closures have been booted out again, which is extremely disappointing. Don't get me started ;)

chown33
Oct 4, 2010, 04:12 PM
The main problem is that you can't make switches on strings in Java.

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:
case 'A': ..code here..
break;
case 'B': ..code here..
break;

This assumes, however, that switch/case has been taught.