Using enum to reduce page object method

Question

I had written following page object class which represents user profile page. The profile contains department, teams, email and login labels and corresponding values. Initially I wrote a page object class as -

public class MyProfilePage extends PageObject{

public MyProfilePage() throws Exception {
    super(departmentLabel);
    throw new IllegalStateException("This is not My Profile page");
}

private static final LabelElement departmentLabel = new LabelElement("Department Label",
        locateByCSSSelector(".department"));
private static final LabelElement teamLabel = new LabelElement("Teams Label",
        locateByCSSSelector(".teams"));
private static final LabelElement emailLabel = new LabelElement("Emails Label",
        locateByCSSSelector(".email"));
private static final LabelElement loginLabel = new LabelElement("Login Label",
        locateByCSSSelector(".login"));

public String getDepartment() {
    return departmentLabel.getText();
}

public String getTeam() {
    return teamLabel.getText();
}

public String getEMail() {
    return emailLabel.getText();
}

public String getLogin() {
    return loginLabel.getText();
}
}

Then I decided to use enum and reduce the number of class variable and methods -

public class MyProfilePage extends PageObject{

public MyProfilePage(ProfileAttribute profileAttribute) throws Exception {
    super(getProfileAttributeElement(profileAttribute));
    throw new IllegalStateException("This is not My Profile page");
}

public enum ProfileAttribute {
    DEPARTMENT("department"), TEAM("teams"), EMAIL("email"), LOGIN("login");

    private String attribute;

    ProfileAttribute(String attribute) {
        this.attribute = attribute;
    }

    public String getAttribute() {
        return attribute;
    }
}

private static HtmlElement getProfileAttributeElement (ProfileAttribute profileAttribute) {
    return new LabelElement("Profile Attribute", locateByCSSSelector("."+
            profileAttribute.getAttribute()));
}

public String getAttributeValue(ProfileAttribute profileAttribute) {
    return getProfileAttributeElement(profileAttribute).getText();
}
}

A curtailed version of TopHeader class which gets to the Myrofile page -

public class TopHeader extends PageObject {

public MyProfilePage goToManageYourAccount() throws Exception {
    manageYourAccountLink.click();
    return new  MyProfilePage(MyProfilePage.ProfileAttribute.DEPARTMENT);
}

}

This is how a test uses it -

public class MyProfileTest extends SelTestCase {

LoginUserNavigation loginUserNavigation = new LoginUserNavigation();

@Test(groups = "shouldDisplayProfileAttributesOnMyProfilePage")
public void shouldDisplayProfileAttributesOnMyProfilePage() throws Exception {
    MyProfilePage myProfilePage = loginUserNavigation
            .loginAsDefaultUser(getEnvironment())
            .getTopHeader()
            .goToManageYourAccount();
    assertThat("department is wrong", myProfilePage.getAttributeValue
            (MyProfilePage.ProfileAttribute.DEPARTMENT), is(equalTo("Technology")));
}

}

Have I made it any better? Could I improve it further?

Answer 1

Besides the formatting issue, I have observed two points to improve.

Since, there is an exception on constructor, the object is not eligible for usage anyway. I don't know the intention behind it but maybe you prefer a private constructor.

Second, attribute in ProfileAttribute could be final.

Answer 2

Throwing exceptions in the constructor is weird; you should either add a comment explaining OR just remove the exception. If you're going to always disallow creation of a PageObject, provide a static method which will do the things you want, rather than throwing exceptions all the time.

Exceptions are for exceptional flow - not for "business as usual". Right now your program reads like "... and then it crashes, which is completely normal." This sort of thing, without documentation, leads to maintenance nightmare. Why was this done? It's highly irregular! But if it was done this way, there must have been a reason? I mean, normally, there is no sane reason to do the sort of thing you just did...

public String getAttributeValue(ProfileAttribute profileAttribute) {
    return getProfileAttributeElement(profileAttribute).getText();
}

This can be deleted. The only constructor throws an exception, so barring weird reflection (in which case, you're doing it wrong), any methods which rely on instances cannot be called. The other option I can think of is if you have a subclass that uses this method. Then this method would see some use - but it's existence remains questionable.

Without the exception, however, you can make use of it as normal.