Acquiring user information about students and classes

I have a Java application that uses a few switch statements to get user information about students and classes.

I have three MySQL tables:

students

student_id | student_name | hometown
classes

class_id | classname | description
student_x_classes

student_id | student_name | class_id | classname

The code works fine, but I must admit, it's pretty ugly. I've been trying to simplify it and break down the big ugly methods to make it more readable but keep struggling.

```
package classselector;

import java.sql.*;
import java.util.Scanner;;

public class ClassSelectorApp {

public static void main(String[] args) throws SQLException {

int menuItem = -1;
while (menuItem != 0) {
menuItem = menu();
switch (menuItem) {
case 1:
createStudent();
break;
case 2:
signUp();
break;
case 3:
listClasses();
break;
case 0:
break;
default:
System.out.println("Invalid Input");
break;

}
}
}

protected static int menu() {
try {
int choice;
Scanner sc = new Scanner(System.in);
System.out.println("\n Class Selection Menu");
System.out.println("****");
System.out.println("0: Exit Menu");
System.out.println("1: Create New Student");
System.out.println("2: Sign Up For a Class");
System.out.println("3: List Classes for All Students");
System.out.println("****");
System.out.println("Enter a choice: ");
choice = sc.nextInt();
return choice;

} catch (java.util.InputMismatchException e) {
System.out.println("Invalid choice!");
} catch (Exception e) {
System.out.println("Something went wrong...");
}
return 0;
}
static void createStudent() {
System.out.println("\nCreate Student\n");
try {
Scanner input = new Scanner(System.in);
System.out.println

Database Structure

Your database will contain a lot of duplication.

The whole point of a mapping table such as student_x_classes is that you can have m:n relations without having to duplicate any of the other data (so you save storage space, and avoid the problem of updating in multiple locations).

But as you include that data inside the table as well, that benefit is gone.

What you want to do is just have the ids in the mapping table.

Security

You sometimes use prepared statements, but not always. This isn't good, as it leads to SQL injection.

Whenever you have something like

"[...] WHERE student_id = " + user_entered_student_id

just use prepared statements to make it secure.

Structure

You have quite a lot of prints all over the place, which makes it a bit hard to follow your code. Your methods also do a lot of things. Ideally, you want that a method only does one thing.

For example:

createStudent reads input data, manages a connection, and writes to the database. What if you want to change your code so that input doesn't come from System.in anymore? You would need to rewrite your code all over the place. Same in case your db connection changes.

So it's best to rewrite this method: createStudent(Connection connection, int id, String name, String homeTown). Now, it only inserts the data into the database, and is easily reusable.

You can restructure the rest of your code in the same way.

If you also want to add additional classes, you could have a Student class which holds data such as the name and the id. Now, you could move your createStudent method there as well (or into a separate StudentDAO).

Formatting

• 2 spaces is not enough. The standard is 4 spaces, and that's really the minimum you want to keep your code readable. If your code is so deeply nested that you need less indentation, fix the nesting instead.

• don't import *, but import the concrete classes you need instead.

• use camelCase, not snake_case (and definitely do not mix the two styles, it's confusing).

Naming

• don't shorten variable names, it makes your code harder to read.

• your variable names could be more exact in general. Eg: input could be scanner (it's not input after all). sql could be insertStudentQuery, rs could be studentNames, rs2 could be classes, I'm not sure what avlClasses are, but there is likely a better name, etc.

StackExchange Code Review Q#122072, answer score: 8