4
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ What is the code after that if condition? \$\endgroup\$ – fge May 28 '13 at 9:47
  • \$\begingroup\$ After if, there is nothing other than a return statement as the edit I made indicates. \$\endgroup\$ – Tiny May 28 '13 at 19:39
13
\$\begingroup\$

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))
\$\endgroup\$
  • \$\begingroup\$ The call to the checkId() method is costly, since it may involve a remote method call like EJB. Therefore, right now, I cannot check for null and/or negative ids inside this method (depending upon my requirements only). Thank you. \$\endgroup\$ – Tiny May 28 '13 at 19:42
  • 1
    \$\begingroup\$ In that case, I would add a wrapper method so you can perform the checks and use that. That way you encapsulate any optimization logic that you may have which would lead to even less replication. \$\endgroup\$ – Jeff Mercado May 29 '13 at 0:41

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.