Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refractor refresh #218

Merged
merged 4 commits into from
Apr 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ public CommandResult execute(Model model) throws CommandException {
// show the employees within this department
model.updateFilteredEmployeeList(e -> department.hasEmployee(e));

model.refresh(); // defensive coding

return new CommandResult(String.format(MESSAGE_ADD_EMPLOYEE_TO_DEPARTMENT_SUCCESS, employee, department));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ public CommandResult execute(Model model) throws CommandException {

model.setDepartment(departmentToEdit, editedDepartment);
model.updateFilteredDepartmentList(Model.PREDICATE_SHOW_ALL_DEPARTMENTS);
model.refresh(); //defensive coding

return new CommandResult(String.format(MESSAGE_EDIT_DEPARTMENT_SUCCESS, editedDepartment));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ public CommandResult execute(Model model) throws CommandException {
// show the employees within this department
model.updateFilteredEmployeeList(e -> department.hasEmployee(e));

model.refresh(); // defensive coding

return new CommandResult(String.format(MESSAGE_REMOVE_EMPLOYEE_FROM_DEPARTMENT_SUCCESS, employee, department));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public CommandResult execute(Model model) throws CommandException {
model.deleteEmployee(employeeToDelete);
model.updateFilteredDepartmentList(Model.PREDICATE_SHOW_ALL_DEPARTMENTS);
model.updateFilteredLeaveList(Model.PREDICATE_SHOW_ALL_NON_EMPTY_LEAVES);

model.refresh();
return new CommandResult(String.format(MESSAGE_DELETE_EMPLOYEE_SUCCESS, employeeToDelete));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ public CommandResult execute(Model model) throws CommandException {
model.setEmployee(employeeToEdit, editedEmployee);
model.updateFilteredEmployeeList(PREDICATE_SHOW_ALL_EMPLOYEES);
model.updateFilteredLeaveList(Model.PREDICATE_SHOW_ALL_NON_EMPTY_LEAVES); // not req but defensive programming
model.refresh();
return new CommandResult(String.format(MESSAGE_EDIT_EMPLOYEE_SUCCESS, editedEmployee.toStringAllFields()));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ public CommandResult execute(Model model) throws CommandException {
model.updateFilteredEmployeeList(predicate);
final Leave leaveToFilter = leaveToAdd;
model.updateFilteredLeaveList(l -> l.equals(leaveToFilter));
model.refresh(); // defensive coding

return new CommandResult(String.format(MESSAGE_ADD_LEAVE_SUCCESS, employeeToAdd, leaveToAdd));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public CommandResult execute(Model model) throws CommandException {
final List<Leave> leavesToFilter = leavesToAdd;

model.updateFilteredLeaveList(l -> leavesToFilter.contains(l));

model.refresh(); // defensive coding
return new CommandResult(builder.toString());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ public CommandResult execute(Model model) throws CommandException {
final Leave leaveToFilter = leaveToDelete;
model.updateFilteredLeaveList(l -> l.equals(leaveToFilter));

model.refresh(); // defensive coding
return new CommandResult(String.format(MESSAGE_SUCCESS, employeeToDelete, leaveToDelete));
}

Expand Down
6 changes: 6 additions & 0 deletions src/main/java/seedu/sudohr/model/Model.java
Original file line number Diff line number Diff line change
Expand Up @@ -286,4 +286,10 @@ public interface Model {
* Deletes an employee {@code employee} from all leaves in SudoHR.
*/
void cascadeDeleteUserInLeaves(Employee employeeToDelete);


/**
* Refreshes filteredLists in SudoHr
*/
void refresh();
}
23 changes: 20 additions & 3 deletions src/main/java/seedu/sudohr/model/ModelManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ public class ModelManager implements Model {
private final FilteredList<Employee> filteredEmployees;
private final FilteredList<Department> filteredDepartments;
private final FilteredList<Leave> filteredLeaves;
private final FilteredList<Employee> refreshedEmployees;
private final FilteredList<Department> refreshedDepartments;
private final FilteredList<Leave> refreshedLeaves;
private final SortedList<Leave> sortedLeaves;

/**
Expand All @@ -45,16 +48,20 @@ public ModelManager(ReadOnlySudoHr sudoHr, ReadOnlyUserPrefs userPrefs) {
this.sudoHr = new SudoHr(sudoHr);
this.userPrefs = new UserPrefs(userPrefs);

filteredEmployees = new FilteredList<>(this.sudoHr.getEmployeeList());
filteredDepartments = new FilteredList<>(this.sudoHr.getDepartmentList());
filteredLeaves = new FilteredList<>(this.sudoHr.getLeavesList(), PREDICATE_SHOW_ALL_NON_EMPTY_LEAVES);
refreshedEmployees = new FilteredList<>(this.sudoHr.getEmployeeList(), (l) -> true);
refreshedDepartments = new FilteredList<>(this.sudoHr.getDepartmentList(), (l) -> true);
refreshedLeaves = new FilteredList<>(this.sudoHr.getLeavesList(), (l) -> true);
filteredEmployees = new FilteredList<>(refreshedEmployees);
filteredDepartments = new FilteredList<>(refreshedDepartments);
filteredLeaves = new FilteredList<>(refreshedLeaves, PREDICATE_SHOW_ALL_NON_EMPTY_LEAVES);
sortedLeaves = new SortedList<>(this.filteredLeaves, new LeaveSortedByDateComparator());
}

public ModelManager() {
this(new SudoHr(), new UserPrefs());
}


// =========== UserPrefs
// ==================================================================================

Expand Down Expand Up @@ -91,6 +98,16 @@ public void setSudoHrFilePath(Path sudoHrFilePath) {
userPrefs.setSudoHrFilePath(sudoHrFilePath);
}

@Override
public void refresh() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just double confirm, setting predicate here to be true / false of an already filtered list will not introduce new unnecessary behaviour right? it wont remove all entires or display all entries right

refreshedEmployees.setPredicate((e) -> false);
refreshedDepartments.setPredicate((d) -> false);
refreshedLeaves.setPredicate((l) -> false);
refreshedEmployees.setPredicate((e)->true);
refreshedDepartments.setPredicate((d)->true);
refreshedLeaves.setPredicate((l)->true);
}

// =========== SudoHr
// ================================================================================

Expand Down
42 changes: 1 addition & 41 deletions src/main/java/seedu/sudohr/model/SudoHr.java
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,6 @@ public void addEmployee(Employee employee) {
public void setEmployee(Employee target, Employee editedEmployee) {
requireNonNull(editedEmployee);
employees.setEmployee(target, editedEmployee);
refreshDepartments();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defensive prog aside, just wondering where's the logic for edit employee command? Where is the new refresh method called for edit employee command?
saw it removed here but no refresh() call to EditCommand? Or is it handled elsewhere

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be no need to add in logic for edit employee since we are currently just tracking the number of employees in each leaves/department, editing employee has no effect
Though for defensive programming sake, it would be better to refresh it

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh right. Normal edits to employee (eg name) will be updated because rendering logic handled diffly.

refreshLeaves();
}

/**
Expand All @@ -200,8 +198,6 @@ public void setEmployee(Employee target, Employee editedEmployee) {
*/
public void removeEmployee(Employee key) {
employees.remove(key);
refreshDepartments();
refreshLeaves();
}

// =========== Department-Level Operations
Expand Down Expand Up @@ -262,7 +258,6 @@ public void removeDepartment(Department key) {
public void addEmployeeToDepartment(Employee p, Department d) {
requireAllNonNull(p, d);
d.addEmployee(p);
refreshDepartments();
}

/**
Expand All @@ -274,20 +269,8 @@ public void addEmployeeToDepartment(Employee p, Department d) {
public void removeEmployeeFromDepartment(Employee p, Department d) {
requireAllNonNull(p, d);
d.removeEmployee(p);
refreshDepartments();
}

/**
* Refreshes the department list.
*/
public void refreshDepartments() {
UniqueDepartmentList currList = new UniqueDepartmentList();
for (Department d : departments) {
currList.add(d);
}
departments.setDepartments(new UniqueDepartmentList());
departments.setDepartments(currList);
}

/**
* Gets the number of employees in the specified department.
Expand All @@ -312,8 +295,6 @@ public void cascadeDeleteEmployeeToDepartments(Employee employeeToDelete) {
dept.removeEmployee(employeeToDelete);
}
}
refreshDepartments(); // defensive programming
refreshLeaves(); // defensive programming
}

/**
Expand All @@ -329,8 +310,6 @@ public void cascadeEditEmployeeToDepartments(Employee employeeToEdit, Employee e
dept.setEmployee(employeeToEdit, editedEmployee);
}
}
refreshDepartments(); // defensive programming
refreshLeaves(); // defensive programming
}

// =========== Leave-Level Operations
Expand Down Expand Up @@ -377,8 +356,6 @@ public void addLeave(Leave leave) {
*/
public void deleteLeave(Leave leave) {
leaves.remove(leave);
refreshDepartments();
refreshLeaves();
}

/**
Expand Down Expand Up @@ -412,7 +389,7 @@ public boolean hasEmployeeOnLeave(LeaveDate date, Employee employee) {
public void addEmployeeToLeave(Leave leave, Employee employee) {
requireAllNonNull(leave, employee);
leave.addEmployee(employee);
refreshLeaves();

}

/**
Expand All @@ -421,19 +398,6 @@ public void addEmployeeToLeave(Leave leave, Employee employee) {
public void deleteEmployeeFromLeave(Leave leave, Employee employee) {
requireAllNonNull(leave, employee);
leave.deleteEmployee(employee);
refreshLeaves();
}

/**
* Refreshes the leave list
*/
public void refreshLeaves() {
UniqueLeaveList currList = new UniqueLeaveList();
for (Leave l : leaves) {
currList.addLeave(l);
}
leaves.setLeaves(new UniqueLeaveList());
leaves.setLeaves(currList);
}

/**
Expand All @@ -457,8 +421,6 @@ public void cascadeUpdateUserInLeaves(Employee employeeToEdit, Employee editedEm
leave.setEmployee(employeeToEdit, editedEmployee);
}
}
refreshDepartments();
refreshLeaves();
}

/**
Expand All @@ -471,8 +433,6 @@ public void cascadeDeleteUserInLeaves(Employee employeeToDelete) {
leave.deleteEmployee(employeeToDelete);
}
}
refreshDepartments();
refreshLeaves();
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,11 @@ public void cascadeUpdateUserInLeaves(Employee employeeToEdit, Employee editedEm
public void cascadeDeleteUserInLeaves(Employee employeeToDelete) {
throw new AssertionError("This method should not be called.");
}

@Override
public void refresh() {
throw new AssertionError("This method should not be called.");
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,11 @@ public void cascadeUpdateUserInLeaves(Employee employeeToEdit, Employee editedEm
public void cascadeDeleteUserInLeaves(Employee employeeToDelete) {
throw new AssertionError("This method should not be called.");
}

@Override
public void refresh() {
throw new AssertionError("This method should not be called.");
}
}

/**
Expand Down Expand Up @@ -404,5 +409,9 @@ public void updateFilteredEmployeeList(Predicate<Employee> predicate) {
public void updateFilteredDepartmentList(Predicate<Department> predicate) {

}

@Override
public void refresh() {
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,12 @@ public void cascadeUpdateUserInLeaves(Employee employeeToEdit, Employee editedEm
public void cascadeDeleteUserInLeaves(Employee employeeToDelete) {
throw new AssertionError("This method should not be called.");
}

@Override
public void refresh() {
throw new AssertionError("This method should not be called.");

}
}

/**
Expand Down Expand Up @@ -409,5 +415,9 @@ public void removeEmployeeFromDepartment(Employee p, Department d) {

}

@Override
public void refresh() {
}

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,11 @@ public void cascadeUpdateUserInLeaves(Employee employeeToEdit, Employee editedEm
public void cascadeDeleteUserInLeaves(Employee employeeToDelete) {
throw new AssertionError("This method should not be called.");
}

@Override
public void refresh() {
throw new AssertionError("This method should not be called.");
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,11 @@ public boolean checkEmployeeExists(Id id) {
throw new AssertionError("This method should not be called.");
}

@Override
public void refresh() {
throw new AssertionError("This method should not be called.");
}

}

/**
Expand Down Expand Up @@ -471,5 +476,9 @@ public boolean checkEmployeeExists(Id id) {
return sudoHr.checkEmployeeExists(id);
}

@Override
public void refresh() {
}

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,11 @@ public void cascadeEditEmployeeToDepartments(Employee employeeToEdit, Employee e
public Leave getLeave(LeaveDate date) {
throw new AssertionError("This method should not be called.");
}

@Override
public void refresh() {
throw new AssertionError("This method should not be called.");
}
}

/**
Expand Down Expand Up @@ -492,6 +497,10 @@ public void updateFilteredEmployeeList(Predicate<Employee> predicate) {
public void updateFilteredLeaveList(Predicate<Leave> predicateShowAllLeave) {
}

@Override
public void refresh() {
}

@Override
public boolean checkEmployeeExists(Id id) {
requireNonNull(id);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,11 @@ public void cascadeDeleteUserInLeaves(Employee employeeToDelete) {
public boolean checkEmployeeExists(Id id) {
throw new AssertionError("This method should not be called.");
}

@Override
public void refresh() {
throw new AssertionError("This method should not be called.");
}
}

/**
Expand Down Expand Up @@ -437,5 +442,9 @@ public boolean checkEmployeeExists(Id id) {
return sudoHr.checkEmployeeExists(id);
}

@Override
public void refresh() {
}

}
}