Avoid duplicate conditional checks in multiple boolean conditions

Question

I have a method that returns either true or false based on a value contained by a java.util.List.

List<Long>list=new ArrayList<Long>(){{ add(1L); add(2L); add(3L); }}; public boolean checkId(Long id) { return list.contains(id); } 

Based on the return value of this method and several other conditions a test is required to be performed.

boolean isIdChanged=true; boolean isAjaxRequest=true; Long id=1L; Test test = new Test(); if(id!=null && id>0 && test.checkId(id) && isIdChanged || id!=null && id>0 && test.checkId(id) && !isAjaxRequest) { //... do something } 

Assuming isIdChanged, isAjaxRequest and id have dynamic values.

In the if test, it can be noticed that the conditional expression id!=null && id>0 && test.checkId(id) is combined (repeated) on both side of the short-circuit || operator.

Is there a precise way to avoid this duplicate check?

---

One way to assign the result of this test to a boolean variable first like so,

boolean check=id!=null && id>0 && test.checkId(id); 

and then the conditional test in the if statement can be trimmed like,

if(check && isIdChanged || check && !isAjaxRequest) { //... do something } 

But I doubt it is still not sufficient. There should be a more precise way to achieve this.

---

Based on the comment below, the actual code is as follows (it is related to the JSF component library, Primefaces).

@Override public List<StateTable> load(int first, int pageSize, String sortField, SortOrder sortOrder, Map<String, String> filters) { int rowCount = stateService.rowCount().intValue(); if(rowCount<=currentPage(first, pageSize)*pageSize-pageSize) { first-=pageSize; } if(id!=null&&id>0&&stateService.checkId(id)&&(isIdChanged||!FacesContext.getCurrentInstance().getPartialViewContext().isAjaxRequest())) { int currentRow=(int) (stateService.getCurrentRow(id)-1); // Returns the current row number from the database. final DataTable dataTable = (DataTable) FacesContext.getCurrentInstance().getViewRoot().findComponent("form:dataTable"); first=currentRow-(currentRow%pageSize); dataTable.setFirst(first); //Sets the starting index of the page holding the row based on the id supplied as a query-string. isIdChanged=false; } setRowCount(rowCount); return stateService.getList(first, pageSize, sortOrder, sortField); } 

In this method test has been replaced by stateService. The method is intended to fetch data from a database in a page-wise manner.

The first two parameters as their names imply, indicate the starting index where the rows are to be retrieved and the number of rows in a page - the page size respectively. The rest may be unrelated to the question.

The first line inside the method returns the number of rows in a given table.

The immediate conditional check evaluates to true only during the deletion of rows held by a table structure (Primefaces DataTable) on the client-side. When a user deletes rows on the last page, the previous page is opened automatically as soon as he/she deletes all of the rows in the last page.

Afterwards the statements inside an if block are meant to highlight a row. When an id is supplied as a GET request parameter, the page containing that row should be opened automatically and the row corresponding to the id (supplied as a query-string) is highlighted by a different background colour of that row on the client-end.

Something similar happens, when you click this link pointing to a question on StackOverflow which automatically opens up the fifth page containing the answer this link is pointing to.

And finally, the number of rows are set by calling a super class method setRowCount() and the List is returned from the database.

Answer 1

Why don't you just write it like this? It should be logically equivalent.

if (id != null && id > 0 && test.checkId(id) && (isIdChanged || !isAjaxRequest)) { // do stuff } 

---

On a side note, I would move the null and positive id checks into your "checkId" method. In fact, I think in this case, these checks are redundant and unnecessary. You're just checking if an id is in an existing list that you're maintaining. If it's null, it's unlikely it will exist in the list (if you are preventing arbitrary values from being added). An id outside of the range of expected values will just not be in the list in the first place and therefore is unneeded as well.

But, if you must perform these checks, do it within the checkId method instead.

public boolean checkId(Long id) { // we don't have null ids if (id == null) return false; // we're expecting positive ids if (id <= 0) return false; return list.contains(id); } 

This reduces your outer condition to simply:

if (test.checkId(id) && (isIdChanged || !isAjaxRequest))