0
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ This seems broken - what is that exception doing in the constructor? \$\endgroup\$ – Pimgd Jul 26 '16 at 9:17
  • \$\begingroup\$ constructor may indeed throw exception - stackoverflow.com/questions/1371369/…. \$\endgroup\$ – Tarun Jul 26 '16 at 9:22
  • \$\begingroup\$ I don't know what you want with an object that is not eligible for usage. Should it be abstract !? Second, attribute could be final. \$\endgroup\$ – rdllopes Jul 26 '16 at 9:37
  • \$\begingroup\$ @rdllopes getAttributeValue should have been public, modified my answer. \$\endgroup\$ – Tarun Jul 26 '16 at 9:40
  • 2
    \$\begingroup\$ @Tarun That is what happens when you don't copy your exact code that you have tested and verified that it works. I'd recommend that you create a new question with your correct code if you want to start over in the review process to make the reviews focus more on the stuff that you want to focus on. \$\endgroup\$ – Simon Forsberg Jul 27 '16 at 15:42
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ why do you say that object is not usable? I can get object of MyProfile page use it further in test to validate attributes - public MyProfilePage goToManageYourAccount() throws Exception { manageYourAccountLink.click(); return new MyProfilePage(MyProfilePage.ProfileAttribute.DEPARTMENT); } \$\endgroup\$ – Tarun Jul 26 '16 at 10:01
  • 1
    \$\begingroup\$ @Tarun try assigning it to a variable and returning the result of the variable - you cannot create a test case that will actually reach the return if you do that. Always throwing exception in constructor is... well, it's just bad. \$\endgroup\$ – Pimgd Jul 26 '16 at 10:03
  • \$\begingroup\$ There should not have been exception on constructor. It was a mistake on writing question ``` throw new IllegalStateException("This is not My Profile page"); ``` \$\endgroup\$ – Tarun Jul 26 '16 at 12:44
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Throwing exception from page object constructor is an exceptional flow, when control is not on right page/page leads to 404 etc a method relying on instance og page object can get it as - new MyProfilePage(MyProfilePage.ProfileAttribute.DEPARTMENT) why getAttributeValue can be deleted. How else would I get text content of attribute webelement? \$\endgroup\$ – Tarun Jul 26 '16 at 10:08
  • \$\begingroup\$ @Tarun show me an example usage of getAttributeValue. \$\endgroup\$ – Pimgd Jul 26 '16 at 10:14
  • \$\begingroup\$ updated question with test and test uses getAttributeValue \$\endgroup\$ – Tarun Jul 26 '16 at 11:43
  • \$\begingroup\$ @Tarun Have you ran the test and tested it? I can see the code just fine, don't worry about the rollback (codereview.stackexchange.com/revisions/135934/4) - it throws the exception, yes? And then the assert never runs. \$\endgroup\$ – Pimgd Jul 26 '16 at 12:21
  • \$\begingroup\$ yup, assert does not run, since there is "throw Exception" in constructor. Having it in question is wrong. \$\endgroup\$ – Tarun Jul 26 '16 at 12:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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