CODEWITHSUNDEEP
javaooprefactoring
Geek
Geek

Reputation: 27183

How can I refactor this code and apply OO patterns?

I have four RichTable instances in my class and there is a notion of current table instance . Depending on a flag resetAll I need to clear out selections of either all the tables or all tables except the current one . If resetAll is true then clear out everything , otherwise leave out the current one . The index of the current table is passed as a parameter to the method that does the clean up action.

The call for clearing out everything looks like this :

clearSubTypeSettings(true,-1);

The call for clearing all but the current one looks like this :

clearSubTypeSettings(true, col);

The implementation of the above method is this :

private void clearSubTypeSettings(boolean resetAll, int exceptControl) {
    if (!resetAll) {
        clearAllExceptCurrent(exceptControl);
    } else {
        clearAll();
    }
 }

Now these two methods clearAllExceptCurrent(exceptControl) and clearAll() look almost the same . Here are the implementations :

  private void clearAll() {
        for (int i = 0; i < SUBTYPE_TABLES; i++)
            if (getSubTypeTable(i).getSelectedRowKeys() != null) {
                RichTable richTable = getSubTypeTable(i);
                RowKeySet rowkeySet = richTable.getSelectedRowKeys();
                rowkeySet.clear();
                AdfFacesContext.getCurrentInstance().addPartialTarget(richTable);
            }
    }

And 

private void clearAllExceptCurrent(int exceptControl) {
    for (int i = 0; i < SUBTYPE_TABLES; i++)
        if (i != exceptControl && getSubTypeTable(i).getSelectedRowKeys() != null) {
            RichTable richTable = getSubTypeTable(i);
            RowKeySet rowkeySet = richTable.getSelectedRowKeys();
            rowkeySet.clear();
            AdfFacesContext.getCurrentInstance().addPartialTarget(richTable);
        }
}

I feel like I am writing duplicate redundant code here and will complicate maintenance in future . How can I improve this code and make it more object oriented ?

Upvotes: 0

Views: 91

Answers (2)

Andreas Fester
Andreas Fester

Reputation: 36630

You can let clearAll() delegate (=> OOP pattern) to clearAllExceptCurrent() (=> improve code by removing duplicated code, make it more maintainable):

private void clearAll() {
   clearAllExceptCurrent(-1);
}

The only difference between your two methods is the condition i != exceptControl in clearAllExceptCurrent(). By passing -1 this condition is always true and therefore effectively non-existent.

Upvotes: 3

devrobf
devrobf

Reputation: 7213

The bulk of the repeated code is the bit that clears the table. So how about:

private void clearTable(int id) {
    if (getSubTypeTable(i).getSelectedRowKeys() != null) {
        RichTable richTable = getSubTypeTable(i);
        RowKeySet rowkeySet = richTable.getSelectedRowKeys();
        rowkeySet.clear();
        AdfFacesContext.getCurrentInstance().addPartialTarget(richTable);
    }
}

Then:

 private void clearAll() {
    for (int i = 0; i < SUBTYPE_TABLES; i++) {
        clearTable(i);
    }
 }

private void clearAllExceptCurrent(int exceptControl) {
    for (int i = 0; i < SUBTYPE_TABLES; i++) {
        if (i != exceptControl) {
            clearTable(i)
        }
    }
}

EDIT: Moved if statement inside clearTable

Upvotes: 0

Related Questions