\$\begingroup\$
\$\endgroup\$
2
I've used Abstract classes and static methods wherever they seemed to be convenient. I've serialized the hashmap entry and read/write it from/to a file.
Please review my code and point me to the mistakes I made. How far away am I from writing production quality code? What are the major drawbacks in my code/design?
import java.io.*;
import java.util.*;
public class Main {
public static void main(String args[]) {
int prompt;
File myfine = new File("std.bat");
Scanner sc = new Scanner(System.in);
if (myfine.length() > 0) {
try (ObjectInputStream ois = new ObjectInputStream(new FileInputStream("std.bat"))) {
Authentication.uniqueStudent = (HashMap<String, Student>) ois.readObject();
} catch (FileNotFoundException e) {
e.printStackTrace();
} catch (IOException e) {
e.printStackTrace();
} catch (ClassNotFoundException e) {
e.printStackTrace();
}
}
while (true) {
System.out.println("If you want to make a fresh entry PRESS 1 \n If you want to manipulate data in an existing student entry PRESS 2 " +
"\n If you want to save and quit PRESS 3");
prompt = sc.nextInt();
sc.nextLine();
switch (prompt) {
case 1:
Student s1 = new Student();
System.out.println("Enter the student name");
String temp = sc.nextLine();
s1.setName(temp);
System.out.println("Enter the Student Grade");
int grade = sc.nextInt();
s1.setGrade(grade);
s1.setID();
Authentication.uniqueStudent.put(s1.getID(), s1);
System.out.println("The unique student ID is " + s1.getID());
break;
case 2:
System.out.println("Enter the student ID");
String str = sc.nextLine();
if (Authentication.uniqueStudent.containsKey(str)) {
Student objective = Authentication.uniqueStudent.get(str);
System.out.println("The students name is " + objective.getName() + "\nThe students grade is " + objective.getGrade());
System.out.println("The subjects taken by the student are");
ArrayList<Courses> c = objective.getCourses();
if (c != null) {
for (Courses cr : c) {
System.out.println(cr.toString());
}
}
else if(c==null){
System.out.println("The student has not enrolled in any course ");
}
System.out.println("The balance in the students account is " + objective.getTotalMoney());
if (objective.getTotalMoney() >= 800) {
System.out.println("You have enough money to take " + (objective.getTotalMoney() / 800) + " more courses");
System.out.println("Would you like to take more courses ? \nEnter 1 for yes \n" +
"Enter 2 for no");
int token = sc.nextInt();
sc.nextLine();
if (token == 1) {
HashSet<Courses> allCourses = new HashSet<>();
for (Courses cs : Courses.values()) {
allCourses.add(cs);
}
if (c != null) allCourses.removeAll(c);
System.out.println("Select from the following courses");
for (Courses course : allCourses) {
System.out.println(course);
}
String stringu=sc.nextLine();
Courses crs=null;
try {
crs=Courses.valueOf(stringu);
}
catch (IllegalArgumentException e){
System.out.println("Wrong Subject entered try again");
continue;
}
System.out.println("Debugging "+crs);
objective.addSubject(crs);
System.out.println("Now the student has ");
for(Courses cs: objective.getCourses()){
System.out.println(cs);
}
}
}
} else {
System.out.println("Invalid Student ID");
}
break;
case 3:
try (ObjectOutputStream ois = new ObjectOutputStream(new FileOutputStream("std.bat"))) {
ois.writeObject(Authentication.uniqueStudent);
} catch (FileNotFoundException e) {
e.printStackTrace();
} catch (IOException e) {
e.printStackTrace();
}
System.out.println("Saving and Quiting");
return;
}
}
}
}
public enum Courses {
history101,
mathmatics101,
english101,
chemistry101,
computerscience101;
}
import java.io.Serializable;
import java.util.ArrayList;
public class Student implements Serializable {
private String name;
private String ID;
private ArrayList<Courses> courses=new ArrayList<>();
private int totalMoney=3200;
public int getGrade() {
return grade;
}
public void setGrade(int grade) {
this.grade = grade;
}
private int grade;
public String getName() {
return name;
}
public void setName(String name) {
this.name = name;
}
public String getID() {
return ID;
}
public void setID() {
String pick ="abcdefghijklmnopqrstuvwxyz12345";
String unique = "" + getGrade();
do {
unique=""+getGrade();
for (int i = 0; i < 4; i++) {
int index = (int) (Math.random() * pick.length());
unique+=pick.charAt(index);
}
}while (Authentication.uniqueStudent.containsKey(unique));
this.ID = unique;
}
public ArrayList<Courses> getCourses() {
return courses;
}
public void setCourses(ArrayList<Courses> courses) {
this.courses = courses;
}
public int getTotalMoney() {
return totalMoney;
}
public void setTotalMoney() {
this.totalMoney -=800;
}
public void addSubject(Courses obj){
setTotalMoney();
courses.add(obj);
}
public static void main(String args[]){
Student stu=new Student();
stu.addSubject(Courses.chemistry101);
}
}
import java.util.HashMap;
public abstract class Authentication {
public static HashMap<String,Student> uniqueStudent=new HashMap<>();
}
asked May 26, 2018 at 19:43
-
1\$\begingroup\$ If this code block is actually in separate files, please show where they stop and start. \$\endgroup\$Reinderien– Reinderien2018年05月26日 21:43:34 +00:00Commented May 26, 2018 at 21:43
-
\$\begingroup\$ @Reinderien Probably at every new import section. I split them up for convenience of OP and reviewer(s). \$\endgroup\$Mast– Mast ♦2018年05月26日 21:53:47 +00:00Commented May 26, 2018 at 21:53
1 Answer 1
\$\begingroup\$
\$\endgroup\$
1
Some thoughts:
- The class containing the
main
method is usually named for your application or is simplyApplication
. - Your
main
method could be split into multiple methods, at least including- creating
Authentication.uniqueStudent
fromstd.bat
, - handling each
case
and - handling
objective.getTotalMoney() >= 800
.
- creating
std.bat
is an unfortunate name for a serialised object, because.bat
is usually associated with Batch script files. I don't know what the standard is for this, but I imagine something like.object
,.jo
(for Java object) or.Student
would work.- The code only ever reads and writes a single student to the same file. A production application would support many students, and would use a more portable system like a database to persist information.
- Naming things according to what they contain or do rather than what they are is very useful for comprehension. For example,
myfine
(sic) could be something likestudentFile
andsc
could beinputScanner
. Short variable names are a thing of the past, before code completion became ubiquitous. - It would be difficult to automate command line interaction with your system since it is interactive. I would instead make it accept parameters to decide what to do and which information to do it with, like almost every shell tool (
find
,grep
,wc
, etc.). Interactivity can then be tacked on if at all necessary. 800
is a "magic" amount of money in two places in your code. It should ideally be configurable since it clearly would change over time in the real world.- Being able to pass an existing student ID would be extremely useful if this code is to be used with an existing system.
- Four random characters chosen from 31 does make for a lot of possible IDs, but why not just use a serial number and persisting that information?
- Using
abcdefghijklmnopqrstuvwxyz12345
as the possible characters for an ID is a bit strange. Why not six through nine or zero? Why not upper case characters? Why not the entire base 64 range, all printable characters or even all of Unicode? - I can't tell at a glance whether
(int) (Math.random() * pick.length())
manages to land on each index uniformly. These solutions are more obviously intuitively correct. - Having a getter and setter for each field is considered an antipattern by many developers. I don't have an authoritative reference, but I would encourage reading up on the arguments given.
- In general methods returning
null
should only happen in case of a bug, because it complicates handling the response. So a method which returnsArrayList<Foo>
should return an empty list if there are no results, notnull
. You can also look into optionals for Java 8 and newer. - You have unused code, such as
String unique = "" + getGrade();
, where the value is thrown away two lines below, andStudent.main
. Authentication
has nothing at all to do with authentication.
In conclusion, this code is mostly fine for homework, but I'm afraid it's far from production ready.
-
\$\begingroup\$ Thank you so much for your suggestions. I'll keep in mind all that you pointed out while making my next project !! \$\endgroup\$Chiranjeev Thomas– Chiranjeev Thomas2018年05月27日 07:36:23 +00:00Commented May 27, 2018 at 7:36
lang-java