Very simple async MixpanelAPI

I would love to hear feedback on my first open source project (a very simple async API for Mixpanel).

It implements a REST client for this REST HTTP API.

Review requested on the following aspects:

• Code style (formatting, naming etc...)

• Code quality (best practices, performance etc)

• Asynchronous aspects (usage of Executors etc)

• JavaDoc quality

Also I would love to hear feedback on the "non-code" aspects of it

• Github aspects (readme, license, folder structure)

• Maven aspects (pom.xml structure and best practices)

Here is the core code:

```
package org.eranmedan.mixpanel;

import java.io.IOException;
import java.io.InputStream;
import java.io.UnsupportedEncodingException;
import java.net.HttpURLConnection;
import java.net.MalformedURLException;
import java.net.URL;
import java.net.URLDecoder;
import java.util.Date;
import java.util.Map;
import java.util.Map.Entry;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;

import javax.servlet.http.Cookie;
import javax.servlet.http.HttpServletRequest;
import javax.xml.bind.DatatypeConverter;

import org.apache.commons.io.IOUtils;
import org.slf4j.Logger;
import org.slf4j.helpers.NOPLogger;

import com.google.gson.JsonElement;
import com.google.gson.JsonObject;
import com.google.gson.JsonParser;

/**
* A dead simple Mixpanel track API for Java
*
* Example Usage:
*
*
*
*
*
*
* String uniqueId = "50479b24671bf";
* String nameTag = "Test Name";
* String ip = "123.123.123.123";
* Date time = new Date();
* String token = "e3bc4100330c35722740fb8c6f5abddc";
* Map<String, String> props = new HashMap<String, String>();
* props.put("action", "play");
* Logger logger = LoggerFactory.getLogger("MixpanelAPI Test Logger");
*
* MixpanelAPI mixpanelAPI = new MixpanelAPI(token, logger);
*
* mixpanel

-
I'd implement Closeable, AutoCloseable:

public class MixpanelAPI implements Closeable, AutoCloseable {
    ...
}

See: implements Closeable or implements AutoCloseable on Stack Overflow.

-

public MixpanelAPI(String token, Logger logger, ExecutorService threadPool) {
  this.token = token;
  this.logger = (logger == null) ? NOPLogger.NOP_LOGGER : logger;
  this.threadPool = threadPool;
}

I'd check nulls here. Allowing to create an object with an invalid state (when token or threadPool is null) does not look a good idea. You'll get a NullPointerException later which would be harder to debug. (The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies; Effective Java 2nd Edition, Item 38: Check parameters for validity)

Guava has a nice Preconditions API for that.

-

public void track(String event, String nameTag, HttpServletRequest request, String cookieName) {
    track(event, nameTag, request, cookieName, null);
}

I'd create an explanatory variable for the null parameter. It would be easier to read, readers don't have to check the called method to figure out what is that null.

public void track(String event, String nameTag, HttpServletRequest request, String cookieName) {
    Map additionalProperties = null;
    track(event, nameTag, request, cookieName, additionalProperties);
}

(Clean Code by Robert C. Martin, G19: Use Explanatory Variables, p296; Refactoring: Improving the Design of Existing Code by Martin Fowler, Introduce Explaining Variable)

-
You have too many parameters here:

return track(event, distinctId, null, null, null, additionalProperties);

It's a code smell: Long Parameter List (See Long Parameter List in Refactoring: Improving the Design of Existing Code by Martin Fowler). You could introduce a parameter object.

-
I'd be a little bit more defensive here and set a limit here:

String responseBody = IOUtils.toString(inputStream, contentEncoding);

If it accidentally returns a 10GB file you probably waste a lot of network bandwidth and get an OutOufMemoryError later.

-

JsonParser jp = new JsonParser();

It would deserve a longer variable name. Longer names would make the code more readable since readers don't have to decode the abbreviations every time and when they write/maintain the code don't have to guess which abbreviation the author uses.

(Clean Code by Robert C. Martin, Avoid Mental Mapping, p25)

-

logger.warn("Unique ID for mixpanel cookie name: " + cookieName + " was not found, using IP instead");

SLF4J supports {}, use that for easier reading and better performance:

logger.warn("Unique ID for mixpanel cookie name: {} was not found, using IP instead", cookieName);

-

* @return a {@link Future} object returning a Boolean when calling it's
   *         get() method, true means a successful call (at the
   *         moment, true is the only possible return value, any error will
   *         cause an {@link ExecutionException} to be thrown when calling the
   *         future's get method). Note: In most use cases you can ignore
   *         the return value of the Future returned for
   *         performance. The Future is mostly for testing purposes

In that case you could use Future instead.

-
I'd move the Callable to a separate class. I think you could extract out some smaller methods for better readability and fulfilling the SRP.

-

URL apiURL;
try {
    apiURL = new URL(url);

The variable declaration could be inside the try block:

try {
    URL apiURL = new URL(url);

-

InputStream inputStream = connection.getInputStream();

You should close this stream in a finally block or use try-with-resources. See Guideline 1-2: Release resources in all cases in Secure Coding Guidelines for the Java Programming Language

-

if (statusCode == 200) {
    if (responseBody.equals("1")) {
        logger.debug("Mixpanel event reported successfully");
        return true;
    } else {
        String warningMessage = "Mixpanel event not reported successfully. Response Body: "
                + responseBody + " message: \n" + message + ". url: " + url;
        logger.warn(warningMessage);
        throw new Exception(warningMessage);
    }
} else {
    String warningMessage = "Mixpanel response not 200: " + statusCode;
    logger.warn(warningMessage);
    throw new Exception(warningMessage);
}

I think it would be easier to follow with guard clauses:

if (statusCode != 200) {
    ...
    throw new Exception(warningMessage);
}
if (!responseBody.equals("1")) {
    ...
    throw new Exception(warningMessage);
}
logger.debug("Mixpanel event reported successfully");
return true;

-

if (ip == null || ip.length() == 0 || "unknown".equalsIgnoreCase(ip)) {
    ip = request.getHeader("Proxy-Client-IP");
}

ip.length() could be changed to ip.isEmpty(). It's more reada

StackExchange Code Review Q#18732, answer score: 5