2
\$\begingroup\$

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 && !host.isReachableOnWan;
    case 1:
        return host.isReachableOnLan;
    case 2:
        return host.isReachableOnWan;
    }
    return false;

}

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

    if (!host.isReachableOnLan && !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) && timeSum < 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();
\$\endgroup\$
  • \$\begingroup\$ I thought about what I needed, and scrapped my ungodly evalFlag I then defined host.isReachable() to return isReachableOnLan || isReachableOnWan. I cached a variable boolean wasReachable = host.isReachable();, and changed the loop to while(((wasReachable && isReachable()) || (!wasReachable() && !isReachable())) && timeSum < checker.loopTimeout) {...}. I've also cleened up everything else like you guys suggested. \$\endgroup\$ – MeetTitan Dec 6 '14 at 7:26
4
\$\begingroup\$

Good names make everything better

public void onItemClick(AdapterView<?> 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<?> 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 && !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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I have cleaned up the top of my onItemClick with your enum suggestion. How would you suggest feeding the while loop the correct boolean value? A method with a switch statement that simply returns my booleans depending on the enum value? \$\endgroup\$ – MeetTitan Dec 2 '14 at 23:08
  • \$\begingroup\$ ferada's answer talks about how to do this. The easy way would to be just replace the existing switch with the enum instead of the int. The HostCheck interface is another good idea as the switch will always resolve to the same case for a given execution. \$\endgroup\$ – unholysampler Dec 2 '14 at 23:18
3
\$\begingroup\$

I think the better OOP choice here would be to introduce a separate object to pass to the checker thread, i.e. new HostCheck(host) with a boolean check() method. You still have the choice of doing it just via an interface, or by adding three new classes, I'd probably go with anonymous classes if it's just an indirection. With Java 8 you could also pass member methods directly.

Even if you decide to keep it this way you still should definitely replace the int with an enumeration instead. evalFlag is also a terrible name for what it does, how about isHostReachable(HostReachabilityType type) or so, even if it would be more verbose it's more describtive. The two ifs could be merged with if (host.isReachableOnLan || host.isReachableOnWan); in the block you'd to tmp = host.isReachableOnLan ? 1 : 2. I doubt that's cleaner though. I'd prefer a method for new ExecuteSshCommand... and then calling that twice like you do now.

Now you'll have to bear with me while I also comment on the rest of the code.

  • In general I'd say the code should be less nested. Just extract everything into separate classes, then it's also testable (hint hint), most importantly the Thread subclass.
  • Are the methods on host thread-safe? Also, if HostCheck executes another thread you should probably wait on it instead of spawning more of them.
  • The printStackTrace is arguably not very good, take a look at this SO answer; I think if the thread was interrupted at that point you'd probably exit to the rest of the method instead, cleaning up the UI state.
  • The blocks with notifyDataSetChanged are duplicated. Put that into a separate method instead.
  • It might also make sense to create a try/catch block around all of the thread to run the setRefreshing(false)/host.isLoading = false in a finally if an error occurs.
  • I get that arg0/1/2/3 are auto-generated, however since you're using arg2 you should give it a better name.
| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

This code can be simplified:

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

To

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

}

But really, you should have something more like this:

public class Host {

private enum Connectivity{
 BOTH {
   public void attempt() {
    new WakeOnLan(host, host.wolBroadcast).start();
    new WakeOnLan(host, host.wanHostName).start();
   }
 }, 
 LAN{
   public void attempt() {
       new ExecuteSshCommand(host, host.sshShutdownCommand).start();
   }
 }, 
 WAN{
   public void attempt() {
       new ExecuteSshCommand(host, host.sshShutdownCommand).start();
   }
 }

   public abstract void attempt();
}



host.connect() {
  // this substitutes for your switch/case
  Connectivity.values()[connectType].attempt();     
}
}

This is rough code. But, it takes advantage of

  • Enums are full-on classes and can have methods. That means, polymorphism.
  • Enums are compiled with ordinals in the the order declared as is 'values()'. That means you have a 1-1 perfect hash mapping.

I don't know enough about android but in Swing, for example, a combo box took, in it's constructor an

Object[]

People generally took this to mean a String[] as that is what they saw in the dropdown. But ask yourself: Why Object[] then? Assume we transpos your case to Swing for a moment. You could pass in your Object[] into the constructor and override the toString() in your enums to display something useful as that is what would show up. Then when the selection is made you receive the callback telling you what it was (as an Object). But you know it's your enum. So, you downcast it and blindly call the 'attempt' method on it.

If you have something similar in your arsenal, consider it. What you have here, actually, is an example of the Command Pattern

| improve this answer | |
\$\endgroup\$

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.