2
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ I assume it's currently working as intended? \$\endgroup\$ – Mast May 11 '17 at 7:35
  • \$\begingroup\$ Yes sir. It is working perfectly fine \$\endgroup\$ – amazingsaluyot May 11 '17 at 7:44
2
\$\begingroup\$

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 and setUpView. I'd have preferred to see this go one step further, though. Instead of extends 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 to MainPanel and InnerPanel 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 and addActionListener)

1 Advice on algorithms is probably out of date... research the state-of-the-art algorithms, please.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.