\$\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.
answered May 26, 2018 at 22:33
-
\$\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