Batch process as an OO design solution

I was given the task of batch process a single-level control break program to produce a lecturer information report by lecturer from a university course file.

I have attempted to create a solution using a OO design in Java. I have run into some problems as I don't think the the modules are cohesive as they are not very independent enough which makes it difficult to maintain. Also if the input file had additional lecturer's the code would have to be changed in many places to accommodate this.

I would very much appreciate it if someone would look through my design solution and offer alternatives to address the problems I've highlighted above. Also maybe suggest an alternative solution!! I'm first year student so new to programming.

Question details:


Each record on the Teaching-module file contains details of a
lecture’s teaching load; that is, a record number, the school code,
the Lecturers department number, the lecturer’s identity number, the
lecture’s name, the module number of the module being taught, the
credit hours for that module, and class size. There may be more than
one record for each lecturer, depending on the number of modules he or
she teaches. The Teaching-module file must be sorted into ascending
sequence of lecturer number within department, within school. Tip
treat as a concatenated single field.


The program is to read the sorted teaching-module file sequentially,
calculate the lecture’s contact hours ((class size/50)* credit hours),
and produce the lecturer information report.


Report example:



Below is my solution so far:

```
public class DSAPDAssignmentB2 {

/**
* @param args the command line arguments
*/
public static void main(String[] args) {

String teachMod = ("1,CSM,501,SM0156,Simon Thorne,BCO200,24,30,"+
"2,CSM,500,AC1157,Nigel Jones,BCO104,24,60,"+
"3,CSM,500,SM0156,Simon Thorne,BCO113,12,60,"+

Variable Names: The name should describe the data that is being stored there. list and aList don't give you any context for what they are being used for. Try to avoid single letter names or abbreviations that might not be clear to other people reading the code.

Additionally, when I saw

for(TeachingLoad tl : simon){

I was confused because I expected simon to be an object that represented a person, not a list of things that are associated with the person. In this case, you only use these lists to iterate over, so you could change it to

for(TeachingLoad tl : simon.getTeachingLoad()){

and now there is one less variable to worry about.

For trimOffRecNo(), you only ever use i to get the current element from list (which you do repeatedly). You can just change that to be a foreach loop. This will make the code more readable.

public static ArrayList trimOffRecNo(ArrayList list){
    ArrayList aList = new ArrayList<>();        //Create our ArrayList
    for(String rec : list){                     //Loop through each element in list
        for(int j=0; j<rec.length();j++){       //Loop through each char
            if(rec.charAt(j) == ','){           //Check for first delimiter...
                //...then whip out the rest of the String
                aList.add(rec.substring(j+1, rec.length()));
                break;                                  //Let's have no exceptions here
            }
        }
    }
    return aList;                                       //Return array 
}

Additionally, you can write this code without the inner loop.

public static List trimOffRecNo(List list){
  List result = new ArrayList<>();
  for(String rec : list){
    int index = rec.indexOf(',');
    if (index == -1) {
      //TODO: decide what to do when there is no comma
    } else {
      result.add(rec.substring(index+1, rec.length());
    }
  }
  return result;
}

I would change the function name. The current name seems to imply that you are changing the content you are passing in. Instead, what is happening is that you are extracting values.

Repeated code: The best example of this is createReport(). This could be rewritten along these lines:

public static int printLoad(LecturerDetails details, String dept) {
  int totalHours = 0;

  for (TeachingLoad tl : details.getTeachingLoad()) {
    //calculate hours, add to totalHours, and print content
  }
  return totalHours;
}

public static void createReport() {
  //setup stuff

  int deptHours = 0;

  for (LecturerDetails details : list of professors) {
    deptHours += printLoad(details, getDept(details));
    // between detail stuff
  }
  // end stuff
}

When you are adding up hours, I suspect there might be a bug.

for each item {
    hours = hours + mathForThisItem();
    // other things
    totalHours = totalHours + hours;
}

hours is accumulating with each item in addition to totalHours. I don't know the business logic behind this math, but I suspect that hours is intended to be the hours for just one item.

You can split a String with split() instead of having to iterate over a string's characters to match on a specific. Since you weren't using indexOf() in another point, it might make sense to look over the other methods that are available in the built in classes that come with Java.

updateLecturerFile() has a number of cases where you have magic numbers or values. The link gives a simple example of what I'm talking about and describing why this is not a great idea.

StackExchange Code Review Q#35538, answer score: 5