How can I refactor this code and apply OO patterns?

Question

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 ?

Andreas Fester · Accepted Answer · 2013-01-24 13:53:17Z

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.

devrobf · Accepted Answer · 2013-01-24 13:47:14Z

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

Collectives™ on Stack Overflow

How can I refactor this code and apply OO patterns?

2 Answers 2

Comments

Comments

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Related