current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I think that the following code isn't really nice. I can't see however another way (other than a switch statement) to better implement this. Especially since I don't think replacing this with the strategy pattern for instance is a better way.

String status;
if(userHasContact&&contactHasUser) {
    status = "HAS_CONTACT";
} else if(userHasContact&&!contactHasUser) {
    status = "PENDING";
} else if(!userHasContact&&contactHasUser) {
    status = "ACCEPT_REQUEST";
} else {
    status = "ADD_CONTACT";
}

I want to return the value of status to the view (this code is from the controller) of the page to show to the user whether that profile he is seeing is already a contact, pending (he already sent a status), accept (it was sent to him), or remove (he already has him as contact).

share|improve this question
    
A switch statement wouldn't work "out of the box" for this, as you would need to switch over two independent variables at once. –  Simon André Forsberg yesterday
2  
Give us some context — why do you need this code in the first place? –  200_success♦ yesterday
    
why do i want to switch((!userHasContact)+((!contactHasUser)<1)){ –  hildred 23 hours ago
    
@hildred This isn't possible in Java, as booleans have no numerical value. It gets a bit longer, see index in my answer. –  maaartinus 22 hours ago

3 Answers 3

up vote 5 down vote accepted

I think it is time to bring out the enum!

Enums in Java are pretty handy overall when there is a fixed number of possible values. In this case there is only four possible values, so an enum can be a good candidate for the job.

public enum Status {
    HAS_CONTACT(true, true),
    PENDING(true, false),
    ACCEPT_REQUEST(false, true),
    ADD_CONTACT(false, false);

    private final boolean hasContact;
    private final boolean hasUser;

    private Status(boolean hasContact, boolean hasUser) {
        this.hasContact = true;
        this.hasUser = true;
    }

    public static Status findStatusWith(boolean hasContact, boolean hasUser) {
        for (Status status : Status.values()) {
            if (status.hasContact == hasContact && status.hasUser == hasUser) {
                 return status;
            }
        }
        return null;
    }
}

You can get the string representation of the enum by calling .name() or .toString()

Status status = Status.findStatusWith(userHasContact, contactHasUser);
String statusDescription = status.name();

Overall I don't recommend using Strings for this kind of data, you have no guarantee that your String will only have one of the four possible values. What if the String suddenly says "ACCPT_SMTHNG"? An enum is much more preferable in this way.

share|improve this answer
    
Interesting approach. The enums are a good idea. Thank you for taking the time to write this! –  Moody 23 hours ago
3  
I strongly disagree with returning null in the impossible case. If it can't happen, then throwing is best (in case it nonetheless happens, you see the problem immediately). –  maaartinus 22 hours ago
up vote 5 down vote

In cases like this I prefer finding the "major" condition, and then using a ternary for the "minor" condition. Your situation is slightly unusual in that it is so neat.... but, first do a function extraction so that you can take advantage of an early-return (no status variable).... and then consider a solution like:

private static final String getStatus(boolean userHasContact, boolean contactHasUser) {
    if (userHasContact) {
        return contactHasUser ? "HAS_CONTACT" : "PENDING";
    }
    return contactHasUser ? "ACCEPT_REQUEST" : "ADD_CONTACT";
}

The early return makes the variable management easier, and the logic conditions are more clear like that.

share|improve this answer
    
I appreciate your comment and the time you took to write this. I like your approach as well and will keep this in mind! –  Moody 23 hours ago
up vote 1 down vote

Obviously, enum is the way to go, and finding the proper member can be done without looping if you convert the two booleans into an index (just treat them as binary digits of a number between 0 and 3).

Storing the booleans in the enum makes sense anyway. Concerning findStatusWith, I'd probably go with rolfl's answer, or even a nested ternary, as it provides a shorter solution.

@RequiredArgsConstructor @Getter
public enum Status {
    HAS_CONTACT(true, true),
    PENDING(true, false),
    ACCEPT_REQUEST(false, true),
    ADD_CONTACT(false, false);

    private static int index(boolean hasContact, boolean hashUser) {
        return (hasContact ? 2 : 0) + (hasUser ? 1 : 0);
    }

    public static Status findStatusWith(boolean hasContact, boolean hasUser) {
         return FOR_CONTACT[index(hasContact, hasUser)];
    }

    private static final Status[] FOR_INDEX = new Status[Status.values().length];
    static {
         for (Status s : Status.values()) {
             FOR_INDEX[index(s.hasContact, s.hasUser)] = s;
         }
    }

    private final boolean hasContact;
    private final boolean hasUser;
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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