patternjavaMinor
Activity Assigner - that randomly assigns each student to an activity
Viewed 0 times
studentassignseachrandomlythatassigneractivity
Problem
I am trying to improve my object-oriented modelling and programming skills and hence made an attempt at solving the problem statement from this link.
Problem Statement
A teacher wants you to help her write a program that will randomly
assign each of her students to an activity at the various activity
tables available in her classroom. Each activity table knows its
activity, the materials needed for the activity, the students at the
table and how many chairs are left. The activity tables available are
the math table, the art table, the reading table and the computer
table and their respective activities are doing math, drawing, reading
and playing on the computer.
I tried my best in coming up with the above objects. Can this be scenario be modelled in a better way?
ActivityTableAssignerMain.java:
ActivityTableAssigner.java:
```
/**
* The primary logic class that loads the total students in the class and
* assigns them to a Random activity table.
*
*/
public class ActivityTableAssigner {
private TablesController tableControlle
Problem Statement
A teacher wants you to help her write a program that will randomly
assign each of her students to an activity at the various activity
tables available in her classroom. Each activity table knows its
activity, the materials needed for the activity, the students at the
table and how many chairs are left. The activity tables available are
the math table, the art table, the reading table and the computer
table and their respective activities are doing math, drawing, reading
and playing on the computer.
I tried my best in coming up with the above objects. Can this be scenario be modelled in a better way?
ActivityTableAssignerMain.java:
public class ActivityTableAssignerMain {
private static final int TOTAL_STUDENTS = 50;
public static void main(String[] args) {
List studentsInClass = getStudentsList();
TablesController tablesController = new TablesController();
tablesController.addActivityTable(new MathActivityTable(10));
tablesController.addActivityTable(new DrawingActivityTable(15));
tablesController.addActivityTable(new ReadingActivityTable(16));
tablesController.addActivityTable(new ComputerPlayActivityTable(8));
ActivityTableAssigner assigner = new ActivityTableAssigner(tablesController);
assigner.assign(studentsInClass);
}
public static List getStudentsList(){
List students = new ArrayList<>();
for(int i=1; i <= TOTAL_STUDENTS; i++){
students.add("Student " + i);
}
return students;
}
}ActivityTableAssigner.java:
```
/**
* The primary logic class that loads the total students in the class and
* assigns them to a Random activity table.
*
*/
public class ActivityTableAssigner {
private TablesController tableControlle
Solution
Generally, it looks good (model wise). I have a few remarks:
Number of students?
You have 50 students, but 49 places for tables. Is this intentional?
Student
I would create a
Error / Exception flow
There is no way to tell if the assignment went wrong, except from the message that goes to the standard out. You could return an result, for example a simple boolean indicating if the assignment was succesful. Alternatively, you could throw a custom
It is not the task of the Assigner to do error logging/printing.
Overengineering?
I think your
The
Assignment algorithm
I would solve the assignment with generating a permutation instead of randomly trying to assign students to tables.
You could for example do this:
The collection of students will be shuffled and you can assign them simply by iterating the collection, because to order is already random.
Better OO modelling of ActivityTable
Consider your constructor:
This creates an 'invalid' class, as it has no Activity. Also you should require that there is always a list of students. Then, you could pass the name of the table as well, and reuse it in the
And in the abstract ActivityTable:
Saved a lot of repetition in the implementing subclasses as well.
Easier to read for loop
This
Can be changed to the 'normal' form:
Rationale is that this is the easiest for loop to read as there are no exceptions from the standard.
Number of students?
You have 50 students, but 49 places for tables. Is this intentional?
Student
I would create a
Student class. Experience taught me that every time I used a simple String to represent a domain object, I ended up by changing it to a full class later on. Just start out with the basics (String name, hashCode() and equals())public class Student
{
private final String name;
public Student (String name)
{
if (name == null) throw new IllegalArgumentException("'name' cannot be 'null'");
this.name = name;
}
public int hashCode()
{
return name.hashCode();
}
..
}Error / Exception flow
There is no way to tell if the assignment went wrong, except from the message that goes to the standard out. You could return an result, for example a simple boolean indicating if the assignment was succesful. Alternatively, you could throw a custom
Exception when it is impossible to assign the students.It is not the task of the Assigner to do error logging/printing.
Overengineering?
I think your
TableController and RandomTableGetter are a bit overengineerd. What is their added value? You can implement a Collection (List, or Set) of ActivityTable and have a getRandomAvailableTable() method in the ActivityTableAssigner. Alternate behaviour can be coded in a different ActivityTableAssigner. The
RandomTableGetter is also doing full-table-checks, so a better name might be RandomAvialableTableGetter.Assignment algorithm
I would solve the assignment with generating a permutation instead of randomly trying to assign students to tables.
You could for example do this:
Collections.shuffle(students)The collection of students will be shuffled and you can assign them simply by iterating the collection, because to order is already random.
Better OO modelling of ActivityTable
Consider your constructor:
public ActivityTable(int chairs) {
this.totalChairs = chairs;
}This creates an 'invalid' class, as it has no Activity. Also you should require that there is always a list of students. Then, you could pass the name of the table as well, and reuse it in the
toString(), so better implement it like this:public ActivityTable(String name, Activity activity, int chairs) {
this.name = name;
this.totalChairs = chairs;
this.activity = activity;
this.studentsAtTable = new ArrayList<>()
}And in the abstract ActivityTable:
public String toString()
{
return this.name;
}Saved a lot of repetition in the implementing subclasses as well.
public class ReadingActivityTable extends ActivityTable {
public ReadingActivityTable(int chairs) {
super("ReadingActivityTable", new ReadingActivity(), chairs);
}
}
public class MathActivityTable extends ActivityTable {
public MathActivityTable(int chairs) {
super("MathActivityTable", new MathActivity(), chairs);
}
}Easier to read for loop
This
for loop (with a start from 1 and a <=) for(int i=1; i <= TOTAL_STUDENTS; i++){
students.add("Student " + i);
}Can be changed to the 'normal' form:
for(int i=0; i < TOTAL_STUDENTS; i++){
students.add("Student " + (i+1));
}Rationale is that this is the easiest for loop to read as there are no exceptions from the standard.
Code Snippets
public class Student
{
private final String name;
public Student (String name)
{
if (name == null) throw new IllegalArgumentException("'name' cannot be 'null'");
this.name = name;
}
public int hashCode()
{
return name.hashCode();
}
..
}Collections.shuffle(students)public ActivityTable(int chairs) {
this.totalChairs = chairs;
}public ActivityTable(String name, Activity activity, int chairs) {
this.name = name;
this.totalChairs = chairs;
this.activity = activity;
this.studentsAtTable = new ArrayList<>()
}public String toString()
{
return this.name;
}Context
StackExchange Code Review Q#148964, answer score: 3
Revisions (0)
No revisions yet.