I'm relatively new to Java, I'm making a desktop application with swing based on the MVC pattern, and I need advice on how to improve the view controller.
The application will perform the CRUD of the User entity .
The UserView looks like this:
The buttons: Search... , Edit... , Add... will open dialogs that allow you to perform the operations in question. (Ignore the free space as I will put other components there in the future).
And the Navigation buttons: First,Previous,Next,Last will allow browsing through the User records present in the database.
Example: when clicking on next the labels of the view will make a setText with the data of the next user present in a list and so on.
This is the UserViewController class :
public class UserViewController {
private UserView userView;
private final UserService userService;
private List<User> users;
private int index = 0;
public UserViewController(UserView userView) {
this.userView = userView;
this.userService = new UserService();
users = userService.getAllUsers();
addListeners();
initUserView();
}
protected void initUserView() {
initUserInUserView(getUserFromList(index));
}
public void addListeners() {
userView.addButtonsListener(new CrudButtonsListener());
userView.addNavigationListener(new NavigationButtonsListener());
userView.addCancelListener(new DialogsCancelButtonsListener());
userView.addAddUserOkListener(new DialgosSubmitButtonsListener(0));
userView.addEditUserOkListener(new DialgosSubmitButtonsListener(1));
}
private class CrudButtonsListener implements ActionListener {
@Override
public void actionPerformed(ActionEvent e) {
switch (e.getActionCommand()) {
case "Clients":
userView.getUserClients().show();
break;
case "Search...":
userView.getSearchUser().show();
break;
case "Edit...":
userView.getEditUser().setInputFieldData(getUserFromList(index));
userView.getEditUser().show();
break;
case "Add...":
userView.getAddUser().show();
break;
case "Delete":
int userId = userView.getUserId();
int option = JOptionPane.showConfirmDialog(userView.getMainPanel(), "Do you want to delete this User?",
"Delete Warning", JOptionPane.YES_NO_OPTION);
deleteUser(userId, option);
break;
}
}
}
private class NavigationButtonsListener implements ActionListener {
@Override
public void actionPerformed(ActionEvent e) {
switch (e.getActionCommand()) {
case "First":
index = 0;
initUserInUserView(getUserFromList(index));
break;
case "Previous":
index--;
if (index < 0) {
index = 0;
JOptionPane.showMessageDialog(userView.getMainPanel(), "You have reached the beginning");
return;
}
initUserInUserView(getUserFromList(index));
break;
case "Next":
index++;
if (index >= users.size()) {
index = users.size() - 1;
JOptionPane.showMessageDialog(userView.getMainPanel(), "The are not more records to show");
return;
}
initUserInUserView(getUserFromList(index));
break;
case "Last":
index = users.size() - 1;
initUserInUserView(getUserFromList(index));
break;
}
}
}
private class DialgosSubmitButtonsListener implements ActionListener {
private final int actionListenerNum;
public DialgosSubmitButtonsListener(int actionListenerNum) {
this.actionListenerNum = actionListenerNum;
}
@Override
public void actionPerformed(ActionEvent e) {
switch (actionListenerNum) {
case 0:
if (userView.getAddUser().isEmptyFixedAttributes()) {
JOptionPane.showMessageDialog(userView.getAddUser().getDialog(), " Some Data is Empty");
} else {
User user = userView.getAddUser().getNewUser();
user.setPassword("123");
createNewUser(user);
}
break;
case 1:
if (userView.getEditUser().isEmptyFixedAttributes()) {
JOptionPane.showMessageDialog(userView.getEditUser().getDialog(),
"Fixed attributes can't be empty");
} else {
User newUser = userView.getEditUser().getNewUser();
User oldUser = userView.getUser();
updateUser(oldUser, newUser);
}
break;
}
}
}
private class DialogsCancelButtonsListener implements ActionListener {
@Override
public void actionPerformed(ActionEvent e) {
userView.getAddUser().dispose();
userView.getEditUser().dispose();
userView.getSearchUser().dispose();
userView.getUserClients().dispose();
}
}
private void initUserInUserView(User user) {
userView.setFieldData(user);
userView.setUser(user);
}
private void createNewUser(User newUser) {
userService.createUser(newUser);
users.add(newUser);
JOptionPane.showMessageDialog(userView.getAddUser().getDialog(),
"User *** " + newUser.getId() + " *** created with success");
userView.getAddUser().clear();
}
private void updateUser(User oldUser, User newUser) {
newUser.setPassword(oldUser.getPassword());
userService.updateUser(newUser, oldUser.getId());
updateUserFromList();
JOptionPane.showMessageDialog(userView.getEditUser().getDialog(),
"User *** " + oldUser.getId() + " *** updated successfully");
}
private void deleteUser(int userId, int option) {
if (option == 0) {
userService.deleteUser(userId);
deleteUserFromList(userId);
JOptionPane.showMessageDialog(userView.getMainPanel(), "User *** " + userId + " *** deleted with success");
}
}
private User getUserFromList(int index) {
return users.get(index);
}
private void updateUserFromList() {
users.clear();
users = userService.getAllUsers();
}
private void deleteUserFromList(int userId) {
Iterator<User> iter = users.iterator();
while (iter.hasNext()) {
User user = iter.next();
if (user.getId() == userId) {
iter.remove();
}
}
}
}
The UserService class is in charge of performing the CRUD operations and obtaining the necessary data from the database.
- CrudButtonsListener() takes care of opening the dialogs.
- NavigationButtonsListener() allows you to view and navigate through all the user records present by iterating through the users list that contains those records. I would like to improve the number of lines of code, should I separate the last methods of the class and put them in another class called utils?
- DialogsSubmitButtonsListener() Dialogs contain buttons to submit and perform crud.
- DialogsCancelButtonsListener() each dialog has a cancel/quit button, this class is in charge of closing the dialogs when these buttons are clicked.
Should I separate the methods at the end of the class and put them in another class "ControllersUtils" ?
Tips to improve this controller.
In general the code looks good to me; each one has his own style and there are things that I would do differently, but we would enter the field of opinions and it is better to avoid it.
Details that I think would objectively improve the code:
You have several nested classes with specific functions. Being the main class not very big I don't see a problem that they are still there, but you were wondering if you could move the methods created below to another place. I would move that code up, leaving the nested classes at the bottom for a matter of order: code from the main class and then the nested ones (it could also be done the other way around, the goal is to group the code). This is purely for readability.
As I told you yesterday in the SOes chat, it is better to put an action command to each button so as not to use the text of the button to identify it, since it would not allow you to easily change the language of your UI.
You have a method with the following:
The first line clears the list of objects, but this call is unnecessary since you then assign another list to that attribute, thus discarding the original one (the garbage collector).