Loop termination check for multiple types of host reachability tests I have 2 booleans: host.isReachableOnLan, and host.isReachableOnWan

I have a listener onItemClick doing work on network on button event

Then I have an anonymous thread than runs a while loop, evaluating the booleans each iteration with the evalFlag method.

This works, but feels hackish and wrong. Is there a more elegant way of accomplishing the same goal? Hopefully one without my evalFlag method.

I would also like to combine the last two if statements at the top of my onItemClick method, because they feel redundant, but they're there for evalFlag.

Host host;

private boolean evalFlag(int type) {
    switch (type) {
    case 0:
        return !host.isReachableOnLan &amp;&amp; !host.isReachableOnWan;
    case 1:
        return host.isReachableOnLan;
    case 2:
        return host.isReachableOnWan;
    }
    return false;

}

@Override
public void onItemClick(AdapterView&lt;?&gt; arg0, View arg1, int arg2, long arg3) {
    host = MainActivity.mainActivity.listAdapter.getItem(arg2);
    int tmp = -1;

    if (!host.isReachableOnLan &amp;&amp; !host.isReachableOnWan) {
        new WakeOnLan(host, host.wolBroadcast).start();
        new WakeOnLan(host, host.wanHostName).start();
        tmp = 0;
    }

    else if (host.isReachableOnLan) { // want to combine this statement
        new ExecuteSshCommand(host, host.sshShutdownCommand).start();
        tmp = 1;
    }

    else if (host.isReachableOnWan) { // with this one
        new ExecuteSshCommand(host, host.sshShutdownCommand).start();
        tmp = 2;
    }

    final int type = tmp;
    new Thread() {
        @Override
        public void run() {
            MainActivity.mainActivity.swipeRefreshLayout.setRefreshing(true);
            host.isLoading = true;

            MainActivity.mainActivity.runOnUiThread(new Runnable() {
                @Override
                public void run() {
                    MainActivity.mainActivity.listAdapter.notifyDataSetChanged();
                }
            });
            int timeSum = 0;
            HostCheck checker = new HostCheck(MainActivity.mainActivity, host, false);

            while (evalFlag(type) &amp;&amp; timeSum &lt; checker.loopTimeout) {
                final long startTime = System.currentTimeMillis();
                checker.run();
                if(host.isReachable())
                    try {
                        sleep(500);
                    } catch (InterruptedException e) {
                        // TODO Auto-generated catch block
                        e.printStackTrace();
                    }
                timeSum += (int) (System.currentTimeMillis() - startTime);
            }
            MainActivity.mainActivity.runOnUiThread(new Runnable() {
                @Override
                public void run() {
                    MainActivity.mainActivity.listAdapter.notifyDataSetChanged();
                }
            });

            MainActivity.mainActivity.swipeRefreshLayout.setRefreshing(false);
            host.isLoading = false;
        }
    }.start();
Good names make everything better

public void onItemClick(AdapterView&lt;?&gt; arg0, View arg1, int arg2, long arg3) {


This signature was clearly generated by a tool. Even though you only need one of the arguments and it is used on the first line of the function doesn't mean the arguments shouldn't have good names.

What happens if I need to use the long argument later? I haven't been in this code for a while, but I just have to add this one simple line. I'm pretty sure arg2 was the long. Oops, arg3 is the long and the int gets implicitly converted and the complier doesn't care. If the arguments had real names, it would be much harder to make this mistake.

That is some what of a contrived example because you could just look at the signature to see that arg3 is the long. But how about this. I haven't had to implement an OnItemClickListener before. I don't know what the arguments are supposed to mean. Why is there a long and an int? Since you didn't take the time to give the arguments real names, I need to look at the documentation to see what each argument means.

public void onItemClick(AdapterView&lt;?&gt; parent, View view, int position, long id) {


Seeing that gives me a good idea of the context. Then only if I need more information do I have to look at the documentation.



type also doesn't help me know anything about what this variable is doing. Add the fact that it is an int and there tons of possible values, how do I know what the right value is and I use it properly?

enum HostConnectivity { Lan, Wan, Both }

// ...

final HostConnectivity connectionType = tmp;


Now it is clearer what kind of type we are dealing with and what each value means. This also removes the magic numbers from evalFlag(). It makes it easy to see if the switch does the wrong check for one of the cases.

Using tmp can be acceptable when it is a short lived variable that actually is temporary. (See the middle section to see how it can be removed and solve another problem.)



evalFlag() is a very generic name. It sounds like it is doing some operation on the flag variable. But there is no flag variable. Also, flags are generally throughout of as boolean, either set or not. Your type has multiple states.

What the method is doing is performing a check of the host's connectivity. it happens to be using the type to decide what checks to perform. The while loop isn't concerned with how the type is being used, it cares about what the boolean result. checkHostConnetivity() would be a much better name.

Sub-functions make life easier

Extracting the type selection code into a sub-function.

private HostConnectivity chooseConnectivity() {
    if (!host.isReachableOnLan &amp;&amp; !host.isReachableOnWan) {
        return HostConnectivity.Both;
    }
    else if (host.isReachableOnLan) {
        return HostConnectivity.Lan;
    }
    else {
        return HostConnectivity.Wan;
    }
}


Now there is no need for a temporary variable. Also, by separating the choosing from the initialization, you can remove the repeated code that you had a comment about wanting to combine.

final HostConnectivity connectionType = chooseConnectivity();
switch(connectionType) {
  case Both:
    new WakeOnLan(host, host.wolBroadcast).start();
    new WakeOnLan(host, host.wanHostName).start();
    break;
  case Wan:
  case Lan:
    new ExecuteSshCommand(host, host.sshShutdownCommand).start();
    break;
}


Instances never die

There are a number of times when you create a new instance, never store the reference, and just start the operation. You should be careful with this. You need to make sure that these things can not run for ever. The OnItemClickListener might not be able to know if/when the thread should stop running. But it is very likely that you might want to kill the tread before it has completed. It is not clear what WakeOnLan and ExecuteSshCommand are doing and if they might also have a potential to run for a long time. Or maybe they fail and need to be checked later.

It is possible that not storing the references is not a problem in this case. But this type of action should be done with care.