3
\$\begingroup\$

I perform an if-check (commented in the code) to determine whether a user can access certain actions or not. The user will be redirected to the index page if they try to access the login.action while already logged in.

I have this kind of if-check in many of my Action classes (containsKey() or !containsKey(), depending on the action) and it is looking very redundant.

How can I improve on what I am trying to achieve?

public String execute() throws Exception {
    // redirect if user is already logged in
    if(session.containsKey("currentId")) {
        return "index";
    }

    Integer currentId = mm.getCurrentId(member.getMemberId());
    String currentAccessType = mm.getMemberInfo("member_access_type", member.getMemberId());

    System.out.println("Current ID: " + currentId);
    System.out.println("Current Access Type: " + currentAccessType);

    session.put("currentId", currentId);
    session.put("currentAccessType", currentAccessType);

    System.out.println("You have logged in!");

    return SUCCESS;
}
\$\endgroup\$
2
  • \$\begingroup\$ Why would you disallow a page to authenticated users? What is the use case for this action? \$\endgroup\$ Dec 30, 2014 at 8:06
  • \$\begingroup\$ Users who are already logged in, should no longer login again. \$\endgroup\$
    – k_rollo
    Dec 30, 2014 at 8:13

2 Answers 2

2
\$\begingroup\$

Aspect-Oriented programming would seem to be the way to go here. Since you're using Struts2, use Interceptors to wrap around the relevant actions. Inside the interceptors, do your check and redirect. You want two, one for the affirmative, and one for the negative.

\$\endgroup\$
4
  • 4
    \$\begingroup\$ Hi, Eric! Welcome to Code Review. Please provide more detail to your answer, such as some code to clarify what you are saying, or add an explanation to what you have said. \$\endgroup\$ Dec 30, 2014 at 4:10
  • \$\begingroup\$ Hi, thanks for your response. I have not used Interceptors before. Sample code would be greatly helpful. \$\endgroup\$
    – k_rollo
    Dec 30, 2014 at 6:00
  • \$\begingroup\$ Upvoted and marked accepted answer because it gave me the main clue. Please proceed to this StackOverflow thread for the implementation. \$\endgroup\$
    – k_rollo
    Dec 30, 2014 at 10:13
  • \$\begingroup\$ Also this StackOverflow answer could help on the theoretical part of the question, and introduces CDI Interceptors in addition to Struts2 Interceptors, assuming you are on Java EE >= 6 \$\endgroup\$ Feb 27, 2015 at 11:16
2
\$\begingroup\$

Other points:

public String execute() throws Exception {

Don't declare exceptions that you don't throw. I know IDE inserts them for you. But it prevents compiler from giving you a heads-up in case you forget to catch a checked exception. When it happens it means hours spent on fixing a trivial mistake and the perception of the quality of the code you deliver will be worse. Search for execute() throws Exception and throws Exception in your project and delete or change with appropriate exceptions.

    // redirect if user is already logged in
    if(session.containsKey("currentId")) {
        return "index";
    }

Comments are a code smell. How to determine if a user is logged-in is an application-wide concern and put in a common method. Put magic strings, numbers, etc literals in constants. Make a habit of using namespacing or some other method to prevent key collisions in session. Think what happens if some library decides to set the "currentId" key. For example:

public class WebInfrastructure {
    public static final String CURRENT_ID = "company-name.application-name.currentId";

    public static boolean isUserLoggedIn(Map<String, Object> session) {
        return session.containsKey("currentId")
    }
}

Don't use "System.out.println" in web applications. Use a real logger. This level of logging is TRACE, which generally means it is used to track bugs in application server.

    System.out.println("Current ID: " + currentId);
    System.out.println("Current Access Type: " + currentAccessType);

They should look something like this:

    logger.trace("You have logged in!");

This string literals should be constants also:

    session.put("currentId", currentId);
    session.put("currentAccessType", currentAccessType);

Think what would happen if you mistyped one of these.

EDIT

Where should I put the isUserLoggedIn() method? In the Action class or in the Interceptor?

It can be both. To begin with you can just refactor the existing code like this:

import static my.company.my.application.WebInfrastructure.isUserLoggedIn;
// .......
public String execute() throws Exception {
    if (isUserLoggedIn(session)) {
    // .......

Later if and when you setup an interceptor should something like this:

import static my.company.my.application.WebInfrastructure.isUserLoggedIn;
// .......
public String intercept(ActionInvocation invocation) throws Exception {
    // .......
    session = actionInvocation.getInvocationContext().getSession()
    // .......
    if (authenticatedUsersNotAllowed(invocation) && isUserLoggedIn(session)) {
        // do something

// authenticatedUsersNotAllowed can be something like this
private static boolean authenticatedUsersNotAllowed(ActionInvocation invocation) {
    return invocation.getAction().getClass()
               .isAnnotationPresent(NonAuthenticatedUsersOnly.class);
}

The example code above is just to demonstrate that isUserLoggedIn should be defined once and used everywhere; independent of how you solve your boilerplate problem. I haven't used struts2 myself, so the rest is just to give you and idea.

Users who are already logged in, should no longer login again.

Let me write the current behavior as a user story:

Given some UserOne is logged in 
When I try to log in as UserTwo 
Then I am redirected to index page, silently;
And still authenticated as UserOne.

Instead it should be something like this:

Given some UserOne is logged in 
When I try to log in as UserTwo 
Then old session is invalidated
Before UserTwo is logged in.

and

Given some UserOne is logged in 
When I try to log in as the same user
Then "You are already logged in!" warning message is shown
And I am redirected to index page;
\$\endgroup\$
1
  • \$\begingroup\$ Thank you for the tips. Where should I put the isUserLoggedIn() method? In the Action class or in the Interceptor? \$\endgroup\$
    – k_rollo
    Dec 30, 2014 at 8:17

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.