2
\$\begingroup\$

Main Goal

My goal is to create a chat app for my company (they are all software developers). I first make a console version and if it is working fine, I will make a GUI version. The program will run on an exchange drive, so using networking features is not neccessary.

Project description

The software will consist of three programs. One is for managing the users: create a new one, delete a user, and so on. The second program is for displaying messages. The third program is for write messages. Later when I make a GUI version, all programs get united to one.

Current status

I have finished the user management program. If you want to test it on your local machine, you have to create a ".save" folder in the same directory your class files are. Otherwise the program crashes.

My Question

Is the code acceptable in matters of readability and code quality? Is there something to improve in matters of english grammar and writing?

(I dont want to implement high security features, because in my company we trust each other. Maybe I will write an Obfuscator class, that obfuscate the strings before they get stored on the disk, or something like that. But if you also have some hints in matters of security for me that are not to hard to implement, you can let me know.)

User.java

import java.io.Serializable;

public class User implements Serializable {
    private String name;
    private String password;

    public User(String name, String password) {
        setName(name);
        setPassword(password);
    }

    public String getName() {
        return new String(name);
    }

    public void setName(String name) {
        this.name = name;
    }

    public void setPassword(String password) {
        this.password = password;
    }

    public boolean checkPassword(String password) {
        return this.password.equals(password);
    }
}

UserManager.java

import java.util.List;
import java.util.ArrayList;
import java.io.File;
import java.io.Serializable;
import java.io.FileOutputStream;
import java.io.ObjectOutputStream;
import java.io.FileInputStream;
import java.io.ObjectInputStream;
import java.io.IOException;

public class UserManager implements Serializable {
    private static final File SAVE_FILE = new File(".save/users");

    private List<User> users;

    public UserManager() {
        if (SAVE_FILE.exists()) {
            loadUsers();
        } else {
            users = new ArrayList<>();
            saveUsers();
        }
    }

    private void saveUsers() {
        try (FileOutputStream fos = new FileOutputStream(SAVE_FILE);
             ObjectOutputStream oos = new ObjectOutputStream(fos)) {
            oos.writeObject(users);
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

    @SuppressWarnings("unchecked")
    private void loadUsers() {
        try (FileInputStream fis = new FileInputStream(SAVE_FILE);
             ObjectInputStream ois = new ObjectInputStream(fis)) {
            users = (ArrayList<User>) ois.readObject();
        } catch (IOException e) {
            e.printStackTrace();
        } catch (ClassNotFoundException e) {
            e.printStackTrace();
        }
    }

    public User retainUser(String name, String password) {
        for (User user : users) {
            if (user.getName().equals(name) && user.checkPassword(password)) {
                return user;
            }
        }
        return null;
    }

    // returns a success message or an error message
    public String createUser(String name, String password) {
        if (name.isEmpty()) {
            return "Name can't be empty.";
        }

        if (password.isEmpty()) {
            return "Password can't be empty";
        }

        // check if name already exits
        for (User user : users) {
            if (user.getName().equals(name)) {
                return "A user with that name already exists.";
            }
        }

        User user = new User(name, password);
        users.add(user);
        saveUsers();
        return "The new user was created successfully.";
    }

    public List<String> retainUserNames() {
        List<String> userNames = new ArrayList<>();
        for (User user : users) {
            userNames.add(user.getName());
        }
        return userNames;
    }

    // returns true if delete request was successful
    public boolean deleteUser(String name, String password) {
        for (User user : users) {
            if (user.getName().equals(name) && user.checkPassword(password)) {
                users.remove(user);
                saveUsers();
                return true;
            }
        }
        return false;
    }
}

UserManagementProgram.java

import java.util.List;
import java.util.Scanner;

public class UserManagementProgram {
    public static void main(String[] args) {
        UserManagementProgram ump = new UserManagementProgram();
        ump.mainLoop();
    }

    private Scanner scanner;
    private UserManager userManager;

    public UserManagementProgram() {
        scanner = new Scanner(System.in);
        userManager = new UserManager();
    }

    public void mainLoop() {
        while (true) {
            System.out.println("[1] Show all users");
            System.out.println("[2] Create user");
            System.out.println("[3] Delete user");
            System.out.println("[4] Close");
            System.out.print("Input: ");
            String input = scanner.nextLine();
            System.out.println();

            switch (input) {
                case "1":
                    printUsers();
                    break;

                case "2":
                    createUser();
                    break;

                case "3":
                    deleteUser();
                    break;

                case "4":
                    return;
            }
            System.out.println();
        }
    }

    private void printUsers() {
        List<String> userNames = userManager.retainUserNames();
        for (String name : userNames) {
            System.out.println(name);
        }
    }

    private void createUser() {
        System.out.print("Name: ");
        String name = scanner.nextLine();
        System.out.print("Password: ");
        String password = scanner.nextLine();

        System.out.println(userManager.createUser(name, password));
    }

    private void deleteUser() {
        System.out.print("Name: ");
        String name = scanner.nextLine();
        System.out.print("Password: ");
        String password = scanner.nextLine();

        if (userManager.deleteUser(name, password)) {
            System.out.println("Success.");
        } else {
            System.out.println("Either name or password are incorrect.");
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Why do you want to create this app after all? There are several existing apps already. You could add that information to the first paragraph of your question. \$\endgroup\$ – Roland Illig Jan 25 at 16:04
  • \$\begingroup\$ For the same reason some companies build cars even if other companies produce also cars already. \$\endgroup\$ – Dexter Thorn Jan 25 at 16:30
  • \$\begingroup\$ Even car companies won't reinvent every part of a car; they often buy some parts from other manufacturers. Similarly, even if you don't want to use an existing app, you could still use a framework which can handle creating/deleting a user for you. And then just make the other parts yourself. Not saying that you definitely should do this, just that in most cases it would be a good option. \$\endgroup\$ – Andre O Jan 26 at 10:51
  • \$\begingroup\$ Readability usability: your code lacks Javadoc comments. Six months from now you won't remember this stuff, and no one besides you knows what it does. Document it while you still have the design fresh in your mind. \$\endgroup\$ – markspace Jan 27 at 5:42
2
\$\begingroup\$

In your current implementation you are caching the users in users field in UserManager, but this has a chance of data loss if there are multiple instances of the application is run and new user are created there.

Password should always be hashed. Though I understand it is an internal app, this is a good practice to ensure anyone's password is not accidentally exposed.

getUserNames is a more appropiate/simpler name instead of retainUserNames.

In UserManager.createUser method, instead of returing exact error message return an enum or error code and then render the error message based on that in your CLI app. This makes handling/updating error messages or providing multilingual support easier later.

Though not a high priority for a simple program such as this, I would recommend using a cli library for parsing command line arguments.

There seems to be no way to update a user's password.

| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

I get that security isn't critical in this case, but it's probably still good to store the passwords in a hash. Some people may be using the same password for other websites and you don't want to compromise their security on those sites. There should be libraries to handle hashing passwords and it would be much better (and simpler) to use them than to try and create your own cryptography algorithm.

I'm also not sure if calling a User method in a User class is the best approach, both for naming or as a way to initalise variables. But I don't usually program in Java so I'm not sure. Maybe someone else can give some advice on that.

| improve this answer | |
\$\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.