6
\$\begingroup\$

I have a Java project that copies files and folders to a user's space on the cloud service using a RESTful API. The login design is getting very complicated, and I wanted advice on how to simplify/re-write it.

I wrote this flow to gain an access token from the API and set the destination workspace (directory) through local files if they exist, or dialog boxes prompting the user for these values.

Generally, I set up a User class that dictates login flow and creates dialog boxes, an AuthHandler that requests and holds the access token and a WorkspaceHandler that holds the list of available upload workspaces and current upload workspace. These values either come from a local file if it exists, or else from a dialog request to the user.

User.java

/**
 * Handles all credential request logic.
 *
 */
public class User {
    private AuthHandler auth;
    private WorkspaceHandler work;
    private static User instance = null;
    private String developerKey;
    private String developerSecret;
    private String ciUsername;
    private String ciPassword;
    private boolean saveOption = false;
    private boolean firstRun = true;
    private boolean change;
    private String workspaceId;
    private String workspaceName;
    boolean open = false;


    public String getName() {
        return ciUsername;
    }

    private User() throws ConfigurationException, SecurityException, IOException, InterruptedException {
        auth = AuthHandler.getHandler();
        work = WorkspaceHandler.getHandler();
    }

    public static User getUser() throws ConfigurationException, SecurityException, IOException, InterruptedException {
        if(instance == null) {
            instance = new User();
        }
        return instance;
    }

    public void createUser() throws ConfigurationException, SecurityException, IOException, InterruptedException {
        //access token
        readCredentials();
        while(!auth.login(developerKey, developerSecret, ciUsername, ciPassword)) {
            createCredentials();
            work.deleteWorkspace();
        }
        work.resetWorkspaceList();
        firstRun = false;
        if(saveOption)
            saveToProperties();
        else
            deleteProperties();

        //workspace
        readWorkspace();
    }

    private void readWorkspace() throws ConfigurationException, IOException, InterruptedException {
        if(!new File("config/workspace.properties").exists()) {
            selectWorkspace();
        }
        else {
            PropertiesConfiguration credentialProps = new PropertiesConfiguration("config/workspace.properties");
            workspaceId = credentialProps.getString("workspaceId");
            workspaceName = credentialProps.getString("workspaceName");
            work.setWorkspace(workspaceName, workspaceId);
        }
    }

    public void changeUser() throws SecurityException, ConfigurationException, IOException, InterruptedException {
        change = true;
        do {
            createCredentials();
            if(!change)
                return;
        } while(!auth.login(developerKey, developerSecret, ciUsername, ciPassword));

        if(saveOption)
            saveToProperties();
        else
            deleteProperties();

        work.deleteWorkspace();
        selectWorkspace();

    }

    private void selectWorkspace() throws ConfigurationException, IOException, InterruptedException {
        work.resetWorkspaceList();
        work.requestWorkspace();
    }

    private void deleteProperties() throws IOException {
        Files.deleteIfExists(Paths.get("config/credentials.properties"));
    }

    private void readCredentials() throws ConfigurationException, SecurityException, IOException, InterruptedException {
        if(!new File("config/credentials.properties").exists()) {
            createCredentials();
        }
        else {
            PropertiesConfiguration credentialProps = new PropertiesConfiguration("config/credentials.properties");
            developerKey = credentialProps.getString("key");
            developerSecret = credentialProps.getString("secret");
            ciUsername = credentialProps.getString("username");
            ciPassword = credentialProps.getString("password");
            saveOption = true;
        }
    }

    private void createCredentials() throws SecurityException, IOException, ConfigurationException, InterruptedException {
        if(open) {
            change = false;
            return;
        }
        open = true;
        LoginDialog login = new LoginDialog();

        if (login.isOk()) {
            developerKey = login.getKeyField().getText();
            developerSecret = login.getSecretField().getText();
            ciUsername = login.getUsernameField().getText();
            ciPassword = login.getPasswordField().getText();
            saveOption = login.getSave().isSelected();
        }
        // if you cancel at start, program closes
        else if(firstRun) {
            System.exit(0);
        }
        else {
            change = false;
        }
        open = false;
    }

    private void saveToProperties() throws ConfigurationException, SecurityException, IOException {
        // skip if already saved
        if(new File("config/credentials.properties").exists()) {
            return;
        }

        PropertiesConfiguration config = new PropertiesConfiguration();
        config.setProperty("key", developerKey);
        config.setProperty("secret", developerSecret);
        config.setProperty("username", ciUsername);
        config.setProperty("password", ciPassword);
        config.save("config/credentials.properties");
        CustomLogger.writeToLog("Credentials saved for next run.");
    }


}

AuthHandler.java

/**
 * Handles access token creation.
 *
 */
public class AuthHandler {
    // authentication details
    private AccessToken accessToken;

    private static AuthHandler instance = null;

    // date for access token renewal
    private GregorianCalendar renewDate;

    // JSON parser
    private Gson gson = new Gson();

    private String developerKey;
    private String developerSecret;
    private String ciUsername;
    private String ciPassword;

    public static AuthHandler getHandler() throws ConfigurationException, SecurityException, IOException, InterruptedException {
        if(instance == null) {
            instance = new AuthHandler();
        }
        return instance;
    }

    /**
     * To ensure only one instance of access token.
     */
    private AuthHandler() {
    }

    public boolean login(String key, String secret, String username, String password) throws InterruptedException {
        accessToken = createAccessToken(key, secret, username, password);

        if(accessToken != null) {
            setRenewDate();
            return true;
        }

        return false;
    }

    /**
     * @return Access token for Ci requests.
     */
    public String getAccessToken() {
        return accessToken.getToken();
    }

    /**
     * If access token needs renewal, requests new access token.
     * @throws InterruptedException 
     */
    public void renewToken() throws InterruptedException {
        if(new GregorianCalendar().compareTo(renewDate) > 0)    {
            System.out.print("Renewing access token: ");
            accessToken = createAccessToken(developerKey, developerSecret, ciUsername, ciPassword);
            if(accessToken != null)
                setRenewDate();
        }
    }

    private void setRenewDate() {
        renewDate = new GregorianCalendar();
        renewDate.add(Calendar.SECOND, accessToken.getExpiresIn()/2);
    }

    /**
     * Request access token from Ci server.
     * @param key
     * @param secret
     * @param username
     * @param password
     * @return the access token
     * @throws InterruptedException 
     */
    private AccessToken createAccessToken(String key, String secret, String username, String password) throws InterruptedException {
        this.developerKey = key;
        this.developerSecret = secret;
        this.ciUsername = username;
        this.ciPassword = password;

        HttpPost request = null;
        String reasonPhrase;
        try {
            String credentials = buildCredentials(username, password);
            HttpClient client = buildTokenClient(credentials);

            String postUrl = String.format("https://api.cimediacloud.com/oauth2/"
                    + "token?grant_type=password_credentials&client_id="
                    + "%s&client_secret=%s", key, secret);
            request = new HttpPost(postUrl);

            // execute request
            System.out.print("Requesting access token: ");
            HttpResponse response = client.execute(request);
            reasonPhrase = response.getStatusLine().getReasonPhrase();
            System.out.println(reasonPhrase);
            if(!reasonPhrase.equals("OK"))  {
                JOptionPane.showMessageDialog(new JPanel(), "Invalid credentials, please re-enter.");
                System.out.println("TAST" + EntityUtils.toString(response.getEntity()));
            }
            else    {
                CustomLogger.writeToLog("Access token granted for 24 hours.");
                String responseString = EntityUtils.toString(response.getEntity());
                AccessToken token = gson.fromJson(responseString, AccessToken.class);
                return token;
            }
        } catch (IOException e) {
            e.printStackTrace();
        } catch (SecurityException e) {
            e.printStackTrace();
        } finally {
            request.releaseConnection();
        }
        return null;
    }

    private String buildCredentials(String username, String password) {
        String userPass = username + ":" + password;
        String credentials = new String(
                Base64.encodeBase64(userPass.getBytes()));
        return credentials;
    }

    /**
     * Returns client with authorization details for access token request.
     * @param credentials
     * @return
     */
    private HttpClient buildTokenClient(String credentials) {
        // add authorization header to request
        List<Header> headers = new ArrayList<Header>();
        Header header = new BasicHeader("Authorization", "Basic " + credentials);
        headers.add(header);
        return HttpClients.custom().setDefaultHeaders(headers).build();
    }
}

WorkspaceHandler.java

/**
 * Holds current workspace for uploads and facilitates changing of upload workspace.
 *
 */
public class WorkspaceHandler {
    private AuthHandler auth;
    private Gson gson = new Gson();

    //choices
    private List<String> workspaceList;
    private Workspace[] workspaces;
    private Workspace uploadWorkspace;

    //settings
    private String workspaceId;
    private String workspaceName;

    private static WorkspaceHandler instance;

    public static WorkspaceHandler getHandler() throws ConfigurationException, SecurityException, IOException, InterruptedException {
        if(instance == null) {
            instance = new WorkspaceHandler();
        }
        return instance;
    }

    /**
     * Set up current workspace value and gather workspaces available for user.
     * @throws ConfigurationException
     * @throws SecurityException
     * @throws IOException
     * @throws InterruptedException
     */
    private WorkspaceHandler() throws ConfigurationException, SecurityException, IOException, InterruptedException {
        auth = AuthHandler.getHandler();
    }

    public void resetWorkspaceList() throws IOException, InterruptedException {
        // set up choices
        uploadWorkspace = null;
        workspaceId = workspaceName = null;
        workspaces = getWorkspaces().getItems();
        workspaceList = workspaceToString(workspaces);
    }

    public void deleteWorkspace() throws IOException {
        Files.deleteIfExists(Paths.get("config/workspace.properties"));
    }

    public void requestWorkspace() throws ConfigurationException, IOException, InterruptedException {
        do {
            uploadWorkspace = selectWorkspace(workspaces, workspaceList);
            if(uploadWorkspace == null) {
                JOptionPane.showMessageDialog(null, "Please select a workspace.");
            }
        } while(uploadWorkspace == null);

        if(!uploadWorkspace.getId().equals(workspaceId)) {
            updateWorkspace(uploadWorkspace);
        }
    }

    private void updateWorkspace(Workspace workspace) throws ConfigurationException, SecurityException, IOException {
        workspaceName = uploadWorkspace.getName();
        workspaceId = uploadWorkspace.getId();
        saveToProperties();
        CustomLogger.writeToLog(String.format("Workspace set: %s", workspaceName));
        SysTray.message("Workspace changed", String.format("The next job will upload to %s.", workspaceName));
    }

    private void saveToProperties() throws ConfigurationException {
        PropertiesConfiguration config = new PropertiesConfiguration();
        config.setProperty("workspaceId", workspaceId);
        config.setProperty("workspaceName", workspaceName);
        config.save("config/workspace.properties");
    }

    private Workspace selectWorkspace(Workspace[] workspaces, List<String> workspaceList) throws IOException, HeadlessException, ConfigurationException, SecurityException, InterruptedException {
        String message = "Hello " + User.getUser().getName() + ", \nSelect your desired upload workspace:";

        WorkspaceDialog select = new WorkspaceDialog(message);
        JFrame frame = new JFrame();
        frame.setAlwaysOnTop(true);
        String choice = (String)JOptionPane.showInputDialog(
                frame,
                "Hello " + User.getUser().getName() + ", \nSelect your desired upload workspace:",
                "Upload workspace",
                JOptionPane.PLAIN_MESSAGE,
                null,
                workspaceList.toArray(),
                workspaceName);
        if(choice == null) {
            return uploadWorkspace;
        }

        return workspaces[workspaceList.indexOf(choice)];
    }

    private HttpClient buildAuthorizedClient()  {
        //add authorization token to request
        List<Header> headers = new ArrayList<Header>();
        Header header = new BasicHeader("Authorization", "Bearer " + auth.getAccessToken());
        headers.add(header);
        return HttpClients.custom().setDefaultHeaders(headers).build();
    }

    private WorkspaceList getWorkspaces() throws IOException, InterruptedException {

        HttpClient client = buildAuthorizedClient();

        //set up get request
        String url = "https://api.cimediacloud.com/workspaces";
        HttpGet request = new HttpGet(url);

        //execute request
        HttpResponse response = null;
        WorkspaceList workspaceList = null;
        try {
            response = client.execute(request);
            String reasonPhrase = response.getStatusLine().getReasonPhrase();
            if(!reasonPhrase.equals("OK")) {
                SysTray.message("Error", "Server error occurred. Please try request again.");
                Thread.sleep(3000);
                System.exit(0);
            }
            //parse json response for information
            HttpEntity entity = response.getEntity();
            String responseString = EntityUtils.toString(entity, "UTF-8");
            workspaceList = gson.fromJson(responseString, WorkspaceList.class);
            return workspaceList;
        }
        finally {
            request.releaseConnection();
        }
    }

    private List<String> workspaceToString(Workspace[] workspaces) {
        List<String> workspaceList = new ArrayList<String>();
        for(int i = 0; i < workspaces.length; i++) {
            String name = workspaces[i].getName();
            workspaceList.add(name);
        }
        return workspaceList;
    }

    public String getId() {
        return workspaceId;
    }

    public String getName() {
        return workspaceName;
    }

    public void changeWorkspace() {
        // TODO Auto-generated method stub

    }

    public void setWorkspace(String workspaceName, String workspaceId) {
        this.workspaceId = workspaceId;
        this.workspaceName = workspaceName;
        uploadWorkspace = new Workspace(workspaceName, workspaceId);
    }
}

Couple questions:

  1. Is the overall separation of responsibilities sound?
  2. The login flow is getting extremely polluted with boolean values that control flow, which indicates to me that I need to redesign the login process. Any pointers on how I can do this?
  3. If my code is a mess, simply give me advice on how I should structure this to best manage the values involved.
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Is the overall separation of responsibilities sound?

Nope. Take for example User, the biggest offender. When I see User, the first thought that comes to my mind is a dumb object with user id, first name, last name. Your class looks nothing like that. Worse, it's a singleton! (What year is this? Back when Windows 98 was cool? ;-)

Calling it UserManager would be closer to the truth, but it would still have too many responsibilities. It has a AuthHandler, and it has a WorkspaceHandler. You need to rethink this. A User should be just a container of simple attributes. A UserManager could have responsibilities like this:

  • register a new user
  • find an existing user
  • check if a user exists

Another clear warning sign here is the presence of fields like this:

private String workspaceId;
private String workspaceName;

These look like implementation details of a Workspace class. It might make sense for a User to have an associated Workspace, but a User class should not have to know how a Workspace works. This is violating good encapsulation.

If a User doesn't need to do something with a Workspace, then he shouldn't have a Workspace. For example if the purpose of the Workspace is to save files uploaded by this user, that logic should be outside of the User class. You could have an UploadHandler class for that, which receives a User object, it gets the associated Workspace from a WorkspaceManager, and uploads the file. Something like that.

I suggest to start fresh with a clear mind. Forget about the existing classes and your existing code. Imagine the most basic objects you would need for your purpose. For each and every one of them, think of the single responsibility it has to do. Write them all down, on a blank sheet of paper. If you want to add an additional feature, behavior, check carefully if it falls within the defined single responsibility. If it doesn't then think of another class that could naturally contain that responsibility.

Don't think about implementation. Think at a conceptual level what each class should do, how it should behave, how it can be used together with other classes, without thinking about the implementation. You should not think about files, databases, urls here, you should think at a higher level.

The end result will be your interfaces. Not classes, interfaces. You can go ahead and start coding some interfaces. Simple container objects like a User can be a concrete class, but things like WorkspaceManager, UserManager, CredentialManager should all be interfaces, working with other interfaces or simple container objects. Finally you can move on to creating implementations of the interfaces.

It may seem like a lot of tedious work up front without coding, but it will be worth it. Once your class design is clear on paper in terms of interfaces, the implementation becomes surprisingly straightforward. Give it a try!

The login flow is getting extremely polluted with boolean values that control flow, which indicates to me that I need to redesign the login process. Any pointers on how I can do this?

If you follow the above points faithfully, this might naturally get cleared up.

One good sign of going in the right direction is when you are able to write unit tests for your functionality with dummy authentication classes. Being able to replace the real implementation with a dummy confirms that your authentication is modular enough that you can make changes to it without affecting the rest of the program.

Finally, in the provided code I don't see a good reason to use singletons. After you redesign, I'm sure you will find that you don't actually need singletons, regular classes will be just one. But if you do have to use singletons for something, then use the cleaner, modern enum pattern, for example:

enum UserManager {
    INSTANCE;

    // ...

    User createUser(String username, String password) {
        return new User(username, password);
    }
}
\$\endgroup\$
2
  • 2
    \$\begingroup\$ I have since removed the User object and switched out the singletons for dependency injection. No more bloated global objects and a much cleaner interface. Huge strides in my design maturity. Thank you! \$\endgroup\$
    – jahmezz
    Commented Sep 17, 2014 at 18:09
  • \$\begingroup\$ Glad to hear that! Sounds like you're doing great, keep it up! \$\endgroup\$
    – janos
    Commented Sep 17, 2014 at 18:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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