0
\$\begingroup\$

This is part of the code I wrote in Java yesterday. It is for sending a body request with some parameters as a pair of keys and values, in a sorted list. Also, this list must have a signature, or the proposal I sent to the API endpoints won;t receive a correct answer (just because the API will refuse it).

The signature will be generated based on a base URL (which contains the parameters) and a secret key.

What kind of refactoring I can do on this code?

protected HashMap<String, Object> body(String[] keys, Object[] values) {
    HashMap<String, Object> params = new HashMap<>();
    params.put("nonce", System.currentTimeMillis());
    params.put("timestamp", System.currentTimeMillis() * 1000);
    for (int i = 0; i < keys.length; i++) {
        params.put(keys[i], values[i]);
    }
    List<String> listOfKeys = new ArrayList<>(params.keySet());
    Collections.sort(listOfKeys);
    StringBuilder baseUrlBuilder = new StringBuilder();
    for (String key : listOfKeys) {
        baseUrlBuilder.append("?").append(key).append("=").append(params.get(key));
    }
    String baseUrl = baseUrlBuilder.toString();
    HashMap<String, Object> body = new HashMap<>();
    for (String key: listOfKeys) {
        body.put(key, params.get(key));
    }
    try {
        Mac encoding = Mac.getInstance("HmacSHA256");
        SecretKeySpec secretKey = new SecretKeySpec(API_SECRET_KEY.getBytes(StandardCharsets.UTF_8), "HmacSHA256");
        encoding.init(secretKey);
        byte[] calcDigest = encoding.doFinal(baseUrl.getBytes(StandardCharsets.UTF_8));
        StringBuilder hexBuilder = new StringBuilder();
        for (byte b : calcDigest) {
            hexBuilder.append(String.format("%02x", b));
        }
        String signature = hexBuilder.toString();
        body.put("signature", signature);
    } catch (NoSuchAlgorithmException | InvalidKeyException e) {
        throw new RuntimeException(e);
    }
    return body;
}
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

Performance

At least by selecting the tag you ask for performance improvements. As a rule of thump you should not optimize for performance unless you have measured the performance and verified, that this particular code is really a bottle neck.

Having written this another rule of thumb is: The fastest way to do something is not doing it. In your code you iterate over the same collection twice:

for (String key : listOfKeys) {
    baseUrlBuilder.append("?").append(key).append("=").append(params.get(key));
}
//...
for (String key: listOfKeys) {
    body.put(key, params.get(key));
}

This could be combined in one loop:

for (String key : listOfKeys) {
    baseUrlBuilder.append("?").append(key).append("=").append(params.get(key));
    body.put(key, params.get(key));
}

Another chance for this type of improvement is the sorting of the parameter keys. You did it explicitly by calling Collections.sort(listOfKeys); which forces you to create an new collection (of type Arraylist) before hand that does an implicit additional iteration over the key set.

By choosing a TreeMap to hold the parameters explicit sorting would not be needed.

Last but not least: at the end both maps, params and body, have the same content except the signature added to the body. Looks like that creating the second map can also be accelerated by not doing it. ;o)

Of cause in your example all optimizations give a rather theoretical acceleration, most likely below measurement threshold and definitely not recognizable... ;o)

General coding

Naming

Finding good names is the hardest part in programming. So always take your time to think about your identifier names.

Read an follow the Java Naming Conventions.

This does not only apply to the casing of the identifiers. It also applies to how names are "constructed". E.g. method names should be or start with a verb (a "do"-word...). In your case the method name should rather be createSignedRequest().

The reason why we do this is that the code, that uses this method more or less reads like an English sentence:

Map<String, Object> request = requestBuilder.createSignedRequest();

From this perspective it might better be shorter;

Map<String, Object> request = requestBuilder.createSigned();

Avoid technical terms in identifiers

You declare this variable:

StringBuilder baseUrlBuilder = new StringBuilder();

The part "Builder" is not necessary. When omitting it, this line reads a little better:

  baseUrl.append("?").append(key).append("=").append(params.get(key));

Avoid abbreviations unless they are very common

The variable name calcDigest does not ned the "calc" prefix since there is only one digest in this scope, but if you want to prefix it you should at least use the appropriate complete adjective: calculatedDigest. The point here is so spare a person not familiar with your code some extra brain cycles when trying to understand what you where trying to do.
And keep in mind that this person might be yourself in 5 month... ;o)

Program against interfaces

It looks like you know this practice since you use the List interface but you are not consequent when declaring variables and the return type as HashMap instead of Map.

No code behind try/catch

Especially if your catch block converts the raised exception into another to be thrown the return statement should be inside the try block. This makes it easier to user your IDEs automated refactoring when extracting the content of the try block to its own method later, when it became bigger.

If the catch block does not re-throw an exception it should have its own return statement returning a neutral element, an object (or value) that can be processed by the calling code without special treatment. Resist to return a potential uninitialized variable declared before the try statement or a literal null! This is how NullPointerExceptions are made...

Single Responsibility / Separation of Concerns

A class or method should have only one task and it should do this well.

Your method seams to have 3 tasks:

  1. converting the input to a map
  2. converting the key/value pairs into a string
  3. calculating the signature.

All thee parts should have their own method. Finally, along with the use of Javas stream-API the code could look like this:

protected Map<String, Object> body(String[] keys, Object[] values) {
  SortedMap<String, Object> body = initializeBodyWith(keys, values);
  String baseUrl = createBaseUrlFrom(body);
  try {
    String signature = calculateSignatureFor(baseUrl);
    body.put("signature", signature);
    return body;
  } catch (NoSuchAlgorithmException | InvalidKeyException e) {
    throw new RuntimeException(e);
  }
}

private SortedMap<String, Object> initializeBodyWith(String[] keys, Object[] values) {
  SortedMap<String, Object> body = new TreeMap<>();
  body.put("nonce", System.currentTimeMillis());
  body.put("timestamp", System.currentTimeMillis() * 1000);
  for (int i = 0; i < keys.length; i++) {
    body.put(keys[i], values[i]);
  }
  return body;
}

private String createBaseUrlFrom(SortedMap<String, Object> body) {
  return body.entrySet()
      .stream()
      .map(parameter -> String.format("?%s=%s", parameter.getKey(), parameter.getValue()))
      .collect(Collectors.joining());
}

private String calculateSignatureFor(String baseUrl) {
  Mac encoding = Mac.getInstance("HmacSHA256");
  SecretKeySpec secretKey = new SecretKeySpec(API_SECRET_KEY.getBytes(StandardCharsets.UTF_8), "HmacSHA256");
  encoding.init(secretKey);
  byte[] calcDigest = encoding.doFinal(baseUrl.getBytes(StandardCharsets.UTF_8));
  return Stream.of(calcDigest)
      .map(b -> String.format("%02x", b))
      .collect(Collectors.joining());
}

Avoid duplicated code

This is just a guess but I think that you have another place in your code where your have the very same conversion of a map into an URL-string. This is a code duplication.

Singe this conversion is separated in a method of its own it is easy to move it to a class of its own and use this new class in both places:

public class BaseUrl {

  public String createFrom(SortedMap<String, Object> body) {
    return body.entrySet().stream()
        .map(parameter -> String.format("?%s=%s", parameter.getKey(), parameter.getValue()))
        .collect(Collectors.joining());
  }

}

protected Map<String, Object> body(String[] keys, Object[] values) {
  SortedMap<String, Object> body = initializeBodyWith(keys, values);
  String baseUrl = new BaseUrl().createFrom(body);
  try {
    String signature = calculateSignatureFor(baseUrl);
    body.put("signature", signature);
    return body;
  } catch (NoSuchAlgorithmException | InvalidKeyException e) {
    throw new RuntimeException(e);
  }
}

private SortedMap<String, Object> initializeBodyWith(String[] keys, Object[] values) {
  SortedMap<String, Object> body = new TreeMap<>();
  body.put("nonce", System.currentTimeMillis());
  body.put("timestamp", System.currentTimeMillis() * 1000);
  for (int i = 0; i < keys.length; i++) {
    body.put(keys[i], values[i]);
  }
  return body;
}

private String calculateSignatureFor(String baseUrl) {
  Mac encoding = Mac.getInstance("HmacSHA256");
  SecretKeySpec secretKey = new SecretKeySpec(API_SECRET_KEY.getBytes(StandardCharsets.UTF_8), "HmacSHA256");
  encoding.init(secretKey);
  byte[] calcDigest = encoding.doFinal(baseUrl.getBytes(StandardCharsets.UTF_8));
  return Stream.of(calcDigest).map(b -> String.format("%02x", b)).collect(Collectors.joining());
}

It may not look like a big thing but if this conversion would be a little more complex and less stable over time and occurs more often then twice having a single point to change is a valuable thing.

\$\endgroup\$
0
\$\begingroup\$

The use of raw collection types is to me a bit of a code smell.

protected HashMap<String, Object> body(String[] keys, Object[] values) {

The declaration HashMap<String, Object> doesn't provide any indication to what the values are. Whoever has to use this method, has to look at the source code to see what the method returns. I prefer returning a domain object that provide domain specific methods for accessing the HashMap (and hide everyhing else). In addition of providing information of what the object represents, it prevents accidental or intentional tampering with the data that might break the implementation further down the stream. I would provide an example, but to be frank, I have no idea what data the map contains (and this is the exact problem my suggestion tries to solve :) ).

Is handling the key-value pairs as two distinct arrays the most convenient way to handle the data? This adds possible places where the implementation can break, especially as you don't do any error checking for the array lengths. Again I would use a domain object to represent the data instead of raw arrays. And again I would provide an example, but the names keys and values don't provide any insight to what that the arrays actually contain so I don't know what I would replace it with.

And whatever you do, don't use a generic KeyValue object. Java does not have a generic KeyValue class for this exact reason: it would be abused everywhere and result in endless amounts of code that suffer from the problems I have described in the above two paragraphs.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.