Implement HostListManager by Refactoring HostListLoader

remoting/android/java/src/org/chromium/chromoting/HostListManager.java

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.chromoting;
 
 import android.os.Handler;
 import android.os.HandlerThread;
 import android.os.Looper;
 
 import org.chromium.base.Log;
 import org.json.JSONArray;
 import org.json.JSONException;
 import org.json.JSONObject;
 
 import java.io.IOException;
+import java.io.OutputStream;
 import java.net.HttpURLConnection;
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.Locale;
 import java.util.Scanner;
 
-/** Helper for fetching the host list. */
-public class HostListLoader {
+/** Helper for fetching and modifying the host list. */
+public class HostListManager {
     public enum Error {
         AUTH_FAILED,
         NETWORK_ERROR,
         SERVICE_UNAVAILABLE,
         UNEXPECTED_RESPONSE,
         UNKNOWN,
     }
 
     /** Callback for receiving the host list, or getting notified of an error. */
     public interface Callback {
-        void onHostListReceived(HostInfo[] hosts);
+        void onHostListReceived(HostInfo[] response);
+        void onHostUpdated();
+        void onHostDeleted();
         void onError(Error error);
     }
 
+    /**
+     * Represents a response from the directory server.
+     * |error| and |body| can be null and they can't be both non-null.

[Lambros, 2016/05/05 00:00:14]

The way this is phrased, it sounds like |error| and |body| might sometimes both
be null. But sendRequest() never returns an object where error == body == null.
Maybe clearer to phrase this as "exactly one of |error| and |body| is non-null"
so clients don't have to worry about handling the both-null case.

[Yuwei, 2016/05/05 00:40:59]

Basically error==/!=null tells you whether the request succeeds or not, and
|body| gives you the actual response if it succeeds. I'm just thinking of the
possibility of a successful request with no response...

Given the way sendRequest() works I think it is safe to disallow the both-null
state and maybe later we can set |body| to an empty string if the no-response
case really happens. (Not sure how HTTP/HTTPRequest works... Maybe you will
always get an empty string if the response doesn't have a body?)

[Yuwei, 2016/05/06 00:31:58]

I guess it looks clearer now...

+     **/
+    private static class Response {
+        public final Error error;
+        public final String body;
+        public Response(Error error, String body) {
+            this.error = error;
+            this.body = body;
+        }
+    }
+
     private static final String TAG = "Chromoting";
 
     /** Path from which to download a user's host list JSON object. */
     private static final String HOST_LIST_PATH =
             "https://www.googleapis.com/chromoting/v1/@me/hosts";
 
     /** Callback handler to be used for network operations. */
     private Handler mNetworkThread;
 
     /** Handler for main thread. */
     private Handler mMainThread;
 
-    public HostListLoader() {
+    public HostListManager() {
         // Thread responsible for downloading the host list.
 
         mMainThread = new Handler(Looper.getMainLooper());
     }
 
-    private void initNetworkThread() {
+    private void runOnNetworkThread(Runnable runnable) {
         if (mNetworkThread == null) {
             HandlerThread thread = new HandlerThread("network");
             thread.start();
             mNetworkThread = new Handler(thread.getLooper());
         }
+        mNetworkThread.post(runnable);
     }
 
     /**
       * Causes the host list to be fetched on a background thread. This should be called on the
       * main thread, and callbacks will also be invoked on the main thread. On success,
       * callback.onHostListReceived() will be called, otherwise callback.onError() will be called
       * with an error-code describing the failure.
       */
-    public void retrieveHostList(String authToken, Callback callback) {
-        initNetworkThread();
-        final String authTokenFinal = authToken;
-        final Callback callbackFinal = callback;
-        mNetworkThread.post(new Runnable() {
+    public void retrieveHostList(final String authToken, final Callback callback) {
+        runOnNetworkThread(new Runnable() {
             @Override
             public void run() {
-                doRetrieveHostList(authTokenFinal, callbackFinal);
+                doRetrieveHostList(authToken, callback);
             }
         });
     }
 
     private void doRetrieveHostList(String authToken, Callback callback) {
-        HttpURLConnection link = null;
-        String response = null;
-        try {
-            link = (HttpURLConnection) new URL(HOST_LIST_PATH).openConnection();
-            link.setRequestProperty("Authorization", "OAuth " + authToken);
-
-            // Listen for the server to respond.
-            int status = link.getResponseCode();
-            switch (status) {
-                case HttpURLConnection.HTTP_OK:  // 200
-                    break;
-                case HttpURLConnection.HTTP_UNAUTHORIZED:  // 401
-                    postError(callback, Error.AUTH_FAILED);
-                    return;
-                case HttpURLConnection.HTTP_BAD_GATEWAY:  // 502
-                case HttpURLConnection.HTTP_UNAVAILABLE:  // 503
-                    postError(callback, Error.SERVICE_UNAVAILABLE);
-                    return;
-                default:
-                    postError(callback, Error.UNKNOWN);
-                    return;
-            }
-
-            StringBuilder responseBuilder = new StringBuilder();
-            Scanner incoming = new Scanner(link.getInputStream());
-            while (incoming.hasNext()) {
-                responseBuilder.append(incoming.nextLine());
-            }
-            response = String.valueOf(responseBuilder);
-            incoming.close();
-        } catch (MalformedURLException ex) {
-            // This should never happen.
-            throw new RuntimeException("Unexpected error while fetching host list: ", ex);
-        } catch (IOException ex) {
-            postError(callback, Error.NETWORK_ERROR);
+        Response response = sendRequest(authToken, HOST_LIST_PATH, "GET", null, null);
+        if (response.error != null) {
+            postError(callback, response.error);
             return;
-        } finally {
-            if (link != null) {
-                link.disconnect();
-            }
         }
 
         // Parse directory response.
         ArrayList<HostInfo> hostList = new ArrayList<HostInfo>();
         try {
-            JSONObject data = new JSONObject(response).getJSONObject("data");
+            JSONObject data = new JSONObject(response.body).getJSONObject("data");
             if (data.has("items")) {
                 JSONArray hostsJson = data.getJSONArray("items");
 
                 int index = 0;
                 while (!hostsJson.isNull(index)) {
                     JSONObject hostJson = hostsJson.getJSONObject(index);
                     // If a host is only recently registered, it may be missing some of the keys
                     // below. It should still be visible in the list, even though a connection
                     // attempt will fail because of the missing keys. The failed attempt will
                     // trigger reloading of the host-list, by which time the keys will hopefully be
@@ skipping 15 matching lines @@
         final Callback callbackFinal = callback;
         final HostInfo[] hosts = hostList.toArray(new HostInfo[hostList.size()]);
         mMainThread.post(new Runnable() {
             @Override
             public void run() {
                 callbackFinal.onHostListReceived(hosts);
             }
         });
     }
 
+    /**
+     * Updates a host on the background thread. On success, callback.onHostUpdated() will be called,
+     * otherwise callback.onError() will be called with an error-code describing the failure.
+     */
+    public void putHost(final String authToken, final String hostId, final String hostName,
+                        final String publicKey, final Callback callback) {
+        runOnNetworkThread(new Runnable() {
+            @Override
+            public void run() {
+                doPutHost(authToken, hostId, hostName, publicKey, callback);
+            }
+        });
+    }
+
+    private void doPutHost(String authToken, String hostId, String hostName, String publicKey,
+                           final Callback callback) {
+        String requestJson;
+        try {
+            JSONObject data = new JSONObject();
+            data.put("hostId", hostId);
+            data.put("hostName", hostName);
+            data.put("publicKey", publicKey);
+            JSONObject request = new JSONObject();
+            request.put("data", data);
+            requestJson = request.toString();
+        } catch (JSONException ex) {
+            Log.e(TAG, "Error creating put host JSON string: %s", ex.getMessage());
+            postError(callback, Error.UNKNOWN);
+            return;
+        }
+        Response response = sendRequest(authToken, HOST_LIST_PATH + '/' + hostId, "PUT",
+                "application/json", requestJson);
+        if (response.error != null) {
+            postError(callback, response.error);
+        } else {
+            mMainThread.post(new Runnable() {
+                @Override
+                public void run() {
+                    callback.onHostUpdated();
+                }
+            });
+        }
+    }
+
+    /**
+     * Deletes a host on the background thread. On success, callback.onHostUpdated() will be called,
+     * otherwise callback.onError() will be called with an error-code describing the failure.
+     */
+    public void deleteHost(final String authToken, final String hostId,
+                           final Callback callback) {
+        runOnNetworkThread(new Runnable() {
+            @Override
+            public void run() {
+                doDeleteHost(authToken, hostId, callback);
+            }
+        });
+    }
+
+    private void doDeleteHost(String authToken, String hostId, final Callback callback) {
+        Response response = sendRequest(authToken, HOST_LIST_PATH + '/' + hostId, "DELETE",
+                null, null);
+        if (response.error != null) {
+            postError(callback, response.error);
+        } else {
+            mMainThread.post(new Runnable() {
+                @Override
+                public void run() {
+                    callback.onHostDeleted();
+                }
+            });
+        }
+    }
+
     /** Posts error to callback on main thread. */
     private void postError(Callback callback, Error error) {
         final Callback callbackFinal = callback;
         final Error errorFinal = error;
         mMainThread.post(new Runnable() {
             @Override
             public void run() {
                 callbackFinal.onError(errorFinal);
             }
         });
     }
 
     private static void sortHosts(ArrayList<HostInfo> hosts) {
         Comparator<HostInfo> hostComparator = new Comparator<HostInfo>() {
             @Override
             public int compare(HostInfo a, HostInfo b) {
                 if (a.isOnline != b.isOnline) {
                     return a.isOnline ? -1 : 1;
                 }
                 String aName = a.name.toUpperCase(Locale.getDefault());
                 String bName = b.name.toUpperCase(Locale.getDefault());
                 return aName.compareTo(bName);
             }
         };
         Collections.sort(hosts, hostComparator);
     }
+
+    /**
+     * Sends request to the url and returns the response.
+     * @param authToken auth token
+     * @param url the URL to send the request
+     * @param method /GET/POST/PUT/DELETE/etc.
+     * @param requestContentType The content type of the request body. This can be null.
+     * @param requestBody This can be null.
+     * @return The response.
+     */
+    private static Response sendRequest(String authToken, String url, String method,
+                                        String requestContentType, String requestBody) {
+        HttpURLConnection link = null;
+        Error error = null;
+        try {
+            link = (HttpURLConnection) new URL(url).openConnection();
+            link.setRequestMethod(method);
+            link.setRequestProperty("Authorization", "OAuth " + authToken);
+            if (requestContentType != null) {
+                link.setRequestProperty("Content-Type", requestContentType);
+            }
+            if (requestBody != null) {
+                byte[] requestBytes = requestBody.getBytes("UTF-8");
+                OutputStream outStream = link.getOutputStream();
+                outStream.write(requestBytes);
+                outStream.close();
+            }
+
+            // Listen for the server to respond.
+            int status = link.getResponseCode();
+            switch (status) {
+                case HttpURLConnection.HTTP_OK:  // 200
+                    break;
+                case HttpURLConnection.HTTP_UNAUTHORIZED:  // 401
+                    error = Error.AUTH_FAILED;
+                    break;
+                case HttpURLConnection.HTTP_BAD_GATEWAY:  // 502
+                case HttpURLConnection.HTTP_UNAVAILABLE:  // 503
+                    error = Error.SERVICE_UNAVAILABLE;
+                    break;
+                default:
+                    error = Error.UNKNOWN;
+            }
+
+            if (error != null) {
+                return new Response(error, null);
+            }
+
+            StringBuilder responseBuilder = new StringBuilder();
+            Scanner incoming = new Scanner(link.getInputStream());
+            while (incoming.hasNext()) {
+                responseBuilder.append(incoming.nextLine());
+            }
+            String response = String.valueOf(responseBuilder);

[Lambros, 2016/05/05 00:00:14]

Just my own preference, but I would write this as
responseBuilder.toString() (you know that responseBuilder is non-null, so you
don't need the extra null-test that String.valueOf(Object) does). And you could
inline this into the return statement below, if you like.

[Yuwei, 2016/05/05 00:40:58]

That's right... BTW the code comes from HostListLoader :P

[Yuwei, 2016/05/06 00:31:58]

Done.

+            incoming.close();
+            return new Response(null, response);
+        } catch (MalformedURLException ex) {
+            // This should never happen.
+            throw new RuntimeException("Unexpected error while fetching host list: ", ex);
+        } catch (IOException ex) {
+            return new Response(Error.NETWORK_ERROR, null);
+        } finally {
+            if (link != null) {
+                link.disconnect();
+            }
+        }
+    }
 }