User persistence and login flow

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, ciPa

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);
    }
}

StackExchange Code Review Q#61902, answer score: 2