1
\$\begingroup\$

I am automating a webform, and this is what I've coded so far. I am curious if you find the code good and readable. But most important to me is to get some feedback for learning purposes.

The remove account page:

public class RemoveAccountsPage extends Page {

    private Boolean accountsToRemove() {
        by = By.className("zero-results-filter");
        return ((elementExists(by)) ? true : false);
    }

    private void selectAllAccounts() {
        by = By.id("accounts-select-all");
        element = waitForPresenceOfElement(by);
        element.click();
    }

    private void clickRemoveButton() {
        by = By.id("account-delete");
        element = waitForPresenceOfElement(by);
        element.click();
    }

    private void confirmRemovingAccounts() {
        by = By.id("confirm");
        element = waitForPresenceOfElement(by);
        element.click();
        //driver.findElement(By.id("NO_DEAL_VIA_MP")).click()
    }

    public void removeAccounts() throws Exception {
        String request = Page.URL + "/hu/accounts/index.html";
        goToWebPage(request);

        Boolean result = accountsToRemove();
        if(result == false) {
            selectAllAccounts();
            clickRemoveButton();
            confirmRemovingAccounts();
        }

        Thread.sleep(1000);
    }
}

Main class:

public class AccountManager {
    public static void main(String args[]) throws Exception {
        Bot bot = new Bot();
        bot.setUp();

        ........

        RemoveAccountsPage removeAccountsPage = new 
        RemoveAccountsPage();
        removeAccountsPage.removeAccounts();

        ........

        bot.tearDown();

    }   
}
\$\endgroup\$
1
\$\begingroup\$

Some lines may be simplified.

1) elementExists() already returns boolean, so use it directly:

return ((elementExists(by)) ? true : false);

to

return elementExists(by);

2) You may inline a method calls into condition statements:

Boolean result = accountsToRemove();
if(result == false) {

to 

if(!accountsToRemove()) {

3) Name the main test method correspondingly:

removeAccounts()

to

testRemovingAllAccounts()
userRemovesAllAccounts()
\$\endgroup\$
0
\$\begingroup\$

It would be nice to see your implementation of Page.

I find the points made by Dmytro very valid and want to add a couple of things more.

Regarding your accountsToRemove() method; somehow this method doesn´t have a name that tells me exactly what it does. I´d expect to give a list of accounts that should be removed, but instead tells me whether an account can be removed right?

On the same note your waitForPresenceOfElement(by) returns an element located by 'by', however I´d expect for it just to wait.

I assume that your by and element variables are global in your Page class, in which case you don´t have to pass them as parameters. However I don´t like global variables, and so I´d tell you to remove them and keep the parameters =)

Since you already have a base Page where you find your elements etc, I´d go one step further and have another method clickElement(...) instead of calling element.click(). This helps you in case the element is not clickable or with StaleReferenceElementException (pretty common I´d say)

\$\endgroup\$

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.