I am planning to create a POS swing application (personal project) for small scale businesses in my local town. I just want to know if this is the right way of implementing MVC in a swing app (I have no professional experience in creating swing apps). This is my initial implementation for the app (Login) starting from domain to view.
Domain:
public class Account extends AbstractDomain {
private String username;
private String password;
public String getUsername() {
return username;
}
public void setUsername(String username) {
this.username = username;
}
public String getPassword() {
return password;
}
public void setPassword(String password) {
this.password = password;
}
}
Repository (Initial implementation. Planning to use HSQL and Hibernate):
public class AccountRepository {
private Map<String, Account> accountMap;
public AccountRepository() {
accountMap = new HashMap<String, Account>();
Account account1 = new Account();
account1.setId(1L);
account1.setUsername("jeromepogi");
account1.setPassword("password");
Account account2 = new Account();
account2.setId(2L);
account2.setUsername("pogijerome");
account2.setPassword("password");
Account account3 = new Account();
account3.setId(3L);
account3.setUsername("jeromepogi123");
account3.setPassword("password");
accountMap.put(account1.getUsername(), account1);
accountMap.put(account2.getUsername(), account2);
accountMap.put(account3.getUsername(), account3);
}
public Account findByUsername(String username) {
return accountMap.get(username);
}
}
Service:
public class AccountService {
private AccountRepository accountRepository;
public AccountService() {
//planning to use spring for dependencies
accountRepository = new AccountRepository();
}
public Account login(String username, String password) throws LoginException{
Account account = accountRepository.findByUsername(username);
if(account == null || !account.getPassword().equals(password)){
throw new LoginException();
}
return account;
}
}
View:
public class LoginView extends JFrame {
private JLabel lblUsername;
private JLabel lblPassword;
private JTextField txtUsername;
private JPasswordField txtPassword;
private JButton btnLogin;
private JButton btnExit;
public LoginView(){
initFrame();
initComponents();
setUpView();
}
public void initFrame(){
setTitle("POS");
setResizable(false);
setSize(400, 200);
setLocationRelativeTo(null);
setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
}
public void initComponents(){
lblUsername = new JLabel("Username: ");
lblPassword = new JLabel("Password: ");
lblUsername.setFont(new Font(null, Font.BOLD, 20));
lblPassword.setFont(new Font(null, Font.BOLD, 20));
txtUsername = new JTextField(15);
txtPassword = new JPasswordField(20);
txtUsername.setFont(new Font(null, 0, 20));
txtPassword.setFont(new Font(null, 0 ,20));
btnLogin = new JButton("Login");
btnExit = new JButton("Exit");
getRootPane().setDefaultButton(btnLogin);
}
public void setUpView(){
MainPanel mainPanel = new MainPanel();
InnerPanel innerPanel = new InnerPanel();
innerPanel.add(lblUsername);
innerPanel.add(txtUsername);
innerPanel.add(lblPassword);
innerPanel.add(txtPassword);
innerPanel.add(btnLogin);
innerPanel.add(btnExit);
mainPanel.add(innerPanel, BorderLayout.CENTER);
add(mainPanel);
}
public void showView(){
setVisible(true);
}
public void exit(){
dispose();
}
public JTextField getTxtUsername() {
return txtUsername;
}
public JPasswordField getTxtPassword() {
return txtPassword;
}
public JButton getBtnLogin() {
return btnLogin;
}
public JButton getBtnExit() {
return btnExit;
}
private class MainPanel extends JPanel{
public MainPanel(){
super(new BorderLayout());
setUp();
}
private void setUp(){
setBorder(BorderFactory.createEmptyBorder(10, 10, 10, 10));
}
}
private class InnerPanel extends JPanel{
public InnerPanel(){
super(new GridLayout(0, 2));
setUp();
}
private void setUp(){
TitledBorder border = BorderFactory.createTitledBorder(BorderFactory.createLineBorder(Color.BLACK), "Login");
border.setTitleJustification(TitledBorder.CENTER);
setBorder(BorderFactory.createCompoundBorder(border, new EmptyBorder(10, 10, 10,10)));
}
}
}
Controller:
public class LoginController {
private LoginView loginView;
private AccountService accountService;
public LoginController() {
loginView = new LoginView();
accountService = new AccountService();
setActionListeners();
}
public void start(){
loginView.showView();
}
public void login(){
try {
String username = loginView.getTxtUsername().getText();
String password = String.valueOf(loginView.getTxtPassword().getPassword());
accountService.login(username, password);
// To main frame
} catch (LoginException e) {
// Show error dialog
}
}
public void setActionListeners(){
loginView.getBtnLogin().addActionListener(new ActionListener() {
public void actionPerformed(ActionEvent e) {
login();
}
});
loginView.getBtnExit().addActionListener(new ActionListener() {
public void actionPerformed(ActionEvent e) {
loginView.exit();
}
});
}
}
Main:
public class Main {
public static void main(String[] args) {
final LoginController loginController = new LoginController();
SwingUtilities.invokeLater(new Runnable() {
public void run() {
loginController.start();
}
});
}
}
So this is my initial implementation of the app. I just want to know if I am in the right path.
-
\$\begingroup\$ I assume it's currently working as intended? \$\endgroup\$Mast– Mast ♦2017年05月11日 07:35:32 +00:00Commented May 11, 2017 at 7:35
-
\$\begingroup\$ Yes sir. It is working perfectly fine \$\endgroup\$amazingsaluyot– amazingsaluyot2017年05月11日 07:44:37 +00:00Commented May 11, 2017 at 7:44
1 Answer 1
This review is basically split into two parts. The first one is a really bad (and unfortunately pretty personal) beating, the second is a bit more differentiated.
I would really feel like a broken record, but hey ... It's high time I reviewed some outdated, EOL'd technology with the same beginner (and tutorial) mistakes made again ... Or maybe I don't.
Just read the first section of this answer of mine, if you're interested in what I have to say on the choice of technology.
Of special note: You don't do the "setVisible(true);
in the constructor" that I dislike so much, which is great.
What you do do is being annoyingly abstract. Just for one small login view, you have 5 classes, where 3 are sufficient. I guess what's bothering me most is the use of Service
and Repository
and AbstractDomain
.
You specifically state that you're intending to use HSQL and Hibernate to interact with a database. This automatically makes both your Service and Repository superfluous, the moment you use the proper tools bundled with those two. Consider the following:
@Entity
@Data
public class Account {
@NotNull
@PrimaryKey
@Size(min = 5, max = 12)
private String username;
@NotNull
private String password;
}
This is hibernate validation constraints with Lombok's @Data
, because I'm too lazy to type out the getters.
If we now wanted to retrieve users by their username, we can use the EntityManager
as follows:
Account acc = EntityManager.find<Account>(Account.class, username);
if (acc == null) { /* account does not exist */
// hash & compare the password anyways to prevent timing attacks
throw new LoginException();
}
// well, we're basically logged in here...
This code accomplishes all the things that happen in AccountRepository
and AccountService
, just in a very likely correct way, with caching and other really fancy stuff, written by people smarter than you and me.
Which reminds me ... You have a username and a password. Just to be on the safe side, you want to get the password stored in a way that makes it basically impossible for you (or anyone else that might steal the data in your database) to guess the actual password. This means you should use a proper secure hashing algorithm (Blowfish, BCRYPT, SCRYPT, ...1 ) and a one-time hash.
</beating>
On to greener pastures or something:
- Again: good job on not falling into the trap of
setVisible(true);
in the constructor. - Good use of composition and separated responsibilities in the View with
initFrame
,initComponents
andsetUpView
. I'd have preferred to see this go one step further, though. Instead ofextends JFrame
, it's generally more modern to have an instance-field, to properly encapsulate interactions with the GUI and keep the exposed interface as small as possible. (This also applies toMainPanel
andInnerPanel
btw.) - The names are chosen pretty nicely, except for the component names in the View, where a lot of systems hungarian is used. Prefixing variables (and fields) with their type in the name (
lbl
,btn
,txt
,...) is tempting, but it shouldn't be done. There's no reason to not trust in your IDE to be able to tell you the type of a variable. - MVC is properly implemented, thumbs up
- There is a few missed opportunities where Lambda-Expressions could be used (especially with
invokeLater
andaddActionListener
)
1 Advice on algorithms is probably out of date... research the state-of-the-art algorithms, please.