Avoid duplicate conditional checks in multiple boolean conditions

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

Listlist=new ArrayList(){{
    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 load(int first, int pageSize, String sortField, SortOrder sortOrder, Map filters)
{
int rowCount = stateService.rowCount().intValue();

if(rowCount0&&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

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))

StackExchange Code Review Q#26686, answer score: 13