Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/

edited May 23, 2017 at 12:41

Community Bot

• 1

Inline Side-by-side Side-by-side Markdown

edited body

edited Mar 17, 2014 at 20:56

palacsint

• 30.4k
• 9
• 82
• 157

Inline Side-by-side Side-by-side Markdown

answered Mar 8, 2014 at 16:11

palacsint

• 30.4k
• 9
• 82
• 157

1. It looks thread-safe for me, I've not found any concurrency-related issue except that you might want to to use a ThreadLocalRandom or use separate SecureRandom instances with a ThreadLocal for performance reasons. See also. Is SecureRandom thread safe?

boolean valid = true;

Instead of this local variable you could return immediately if you know the result:

 public boolean isJSONValid(final String str, final int id) {
 try {
 final JSONObject obj = new JSONObject(str);
 final JSONArray data = obj.getJSONArray("lv");
 final int n = data.length();
 for (int i = 0; i < n; ++i) {
 final JSONObject lv = data.getJSONObject(i);
 JSONObject v = lv.getJSONObject("v");
 if (v.getInt("userId") != id) {
 return false;
 }
 }
 } catch (JSONException ex) {
 return false;
 }
 return true;
 }

1. The following fields could be local variables in run:

private Connection[] dbConnection = null;
private PreparedStatement preparedStatement = null;
private ResultSet rs = null;
private HashMap<String, Connection> tableStatement = new HashMap<String, Connection>();

Effective Java, Second Edition, Item 45: Minimize the scope of local variables

1. The null assigment is unnecessary here (in both cases):

if (preparedStatement != null) {
 try {
 preparedStatement.close();
 preparedStatement = null;
 } catch (SQLException e) {
 LOG.error("Threw a SQLException in finally block of prepared statement " + getClass().getSimpleName(),
 e);
 }
}
for (Connection con: dbConnection) {
 if (con != null) {
 try {
 con.close();
 con = null;
 } catch (SQLException e) {
 LOG.error("Threw a SQLException in finally block of dbConnection " + getClass().getSimpleName(), e);
 }
 }
}

It only nulls a local variable/parameter which will be destroyed anyway an the end of the block/method. I guess you wanted to null the field, for that you need a this. prefix but as above, the fields could be also local variables in run, it's unnecessary.

1. A guard clause could make the second loop flatten:

    for (Connection con: dbConnection) {
    if (con == null) {
    continue;
    }
    try {
    con.close();
    } catch (SQLException e) {
    LOG.error("Threw a SQLException in finally block of dbConnection " + getClass().getSimpleName(), e);
    }
    }
   

LOG.error("Threw a SQLException in finally block of prepared statement " 
 + getClass().getSimpleName(), e);

If you are using Logback or Log4j the getClass().getSimpleName() is unnecessary, you can set in the log4j.xml or logback.xml to log the class name for every log statement.

1. Instead of System.nanoTime(), you could use a stopwatch class, like Guava Stopwatch or Apache Commons StopWatch).

1. You could extract out the increment part of this method for higher abstraction level:

private static void handleException(String cause, boolean flagTerminate) {
 LOG.error(cause);
 AtomicInteger count = exceptionMap.get(cause);
 if (count == null) {
 count = new AtomicInteger();
 AtomicInteger curCount = exceptionMap.putIfAbsent(cause, count);
 if (curCount != null) {
 count = curCount;
 }
 }
 count.incrementAndGet();
 if (flagTerminate) {
 System.exit(1);
 }
}

It's easier to read, it's easier to get an overview what the method does (without the details) and you can still check them if you are interested in. Furthermore, you need to read and understand less code (not the whole original method) if you want to modify just a small part of it.

 private static void handleException(String cause, boolean flagTerminate) {
 LOG.error(cause);
 incrementExceptionCount(cause);
 if (flagTerminate) {
 System.exit(1);
 }
 }
 private static void incrementExceptionCount(final String cause) {
 ...
 }

1. I guess you could use here the same structure with AtomicLongs:

    private static void incrementExceptionCount(final String cause) {
    AtomicInteger previousValue = exceptionMap.putIfAbsent(cause, new AtomicInteger());
    if (previousValue != null) {
    previousValue.incrementAndGet();
    }
    }
   

You could also extract a similar method for AtomicLongs, since currently it's duplicated in the code.

1. Anyway, instead of the ConcurrentHashMap<Long, AtomicLong> structures I'd use Guava's AtomicLongMap. It was designed for these situations.

(Effective Java, 2nd edition, Item 47: Know and use the libraries The author mentions only the JDK's built-in libraries but I think the reasoning could be true for other libraries too.)

1. The logic in the method parameter is duplicated here.

} catch (ClassNotFoundException e) {
 handleException(e.getCause() != null ? e.getCause().toString() : e.toString(), LnPRead.flagTerminate);
} catch (SQLException e) {
 handleException(e.getCause() != null ? e.getCause().toString() : e.toString(), LnPRead.flagTerminate);
}

It could be moved a new method:

 private void handleException2(Exception e) {
 handleException(e.getCause() != null ? e.getCause().toString() : e.toString(), LnPRead.flagTerminate);
 }

A few explanatory local variable would make it more readable:

 private void handleException2(final Exception e) {
 final Throwable cause = e.getCause();
 final String exceptionString;
 if (cause == null) {
 exceptionString = e.toString();
 } else {
 exceptionString = cause.toString();
 }
 handleException(exceptionString, LnPRead.flagTerminate);
 }

Reference: Chapter 6. Composing Methods, Introduce Explaining Variable in Refactoring: Improving the Design of Existing Code by Martin Fowler:

Put the result of the expression, or parts of the expression, in a temporary variable with a name that explains the purpose.

And Clean Code by Robert C. Martin, G19: Use Explanatory Variables.

1. The randomID local variable is unnecessary here:

private int generateRandomId(Random r) {
 int randomID;
 if (r.nextFloat() < LnPRead.percentageValidId / 100) {
 randomID = r.nextInt(LnPRead.endValidRange - LnPRead.startValidRange + 1) + LnPRead.startValidRange;
 } else {
 randomID = r.nextInt(LnPRead.endNonValidRange - LnPRead.startNonValidRange + 1)
 + LnPRead.startNonValidRange;
 }
 return randomID;
}

You could return immediately (and use an explanatory variable too):

 private int generateRandomId(final Random r) {
 final boolean generateValidId = r.nextFloat() < LnPRead.percentageValidId / 100;
 if (generateValidId) {
 return r.nextInt(LnPRead.endValidRange - LnPRead.startValidRange + 1) + LnPRead.startValidRange;
 } else {
 return r.nextInt(LnPRead.endNonValidRange - LnPRead.startNonValidRange + 1)
 + LnPRead.startNonValidRange;
 }
 }

Note that how easy to decide now whether the if or the else branch generates valid IDs. (I've just guessed here, you might need to invert the condition.)

1. I'd move the random.nextDouble() inside the selectRandomTable method.

double randomNumber = random.nextDouble() * 100.0;
TableConnectionInfo table = selectRandomTable(randomNumber);

Nothing indicates the the parameter should be between 0 and 100. It sould be an internal part of the method to choose a proper random number.

1. In the same method, the IllegalStateException should contain some debugging information, like the generated random number. It would help debugging.

2. columnsList.split(",") could be extracted out to a local variable. It's used multiple times.

durationOfRun = Long.parseLong(prop.getProperty("TOTAL_RUNNING_TIME").trim());
sleepTime = Long.parseLong(prop.getProperty("SLEEP_TIME").trim());

The parsing and trimming could have a separate function:

 private static long loadLong(Properties properties, String key) {
 return Long.parseLong(properties.getProperty(key).trim());
 }

You can create similar functions for the boolean and integer fields too.

1.

for (String arg: tableNames) {

arg is not too descriptive here. I'd call it tableName.

1. I'd rename isJSONValid to isJsonValid or isValidJson. I think it would be easier to read (although it's inconsistent with the JSON library's naming but I think that's a minor issue). From Effective Java, 2nd edition, Item 56: Adhere to generally accepted naming conventions:

   While uppercase may be more common, a strong argument can made in favor of capitalizing only the first letter: even if multiple acronyms occur back-to-back, you can still tell where one word starts and the next word ends. Which class name would you rather see, HTTPURL or HttpUrl?

2. The isJSONValid contains two magic numbers:

JSONArray data = obj.getJSONArray("lv");
int n = data.length();
for (int i = 0; i < n; ++i) {
 JSONObject lv = data.getJSONObject(i);
 JSONObject v = lv.getJSONObject("v");
...

They are the lv and v strings. Constants with longer and more descriptive name would help readers to figure out what should be the content of these fields. You could also rename the variable names too (data, lv, v) to express the intent.

1.

final int n = data.length();
for (int i = 0; i < n; ++i) {

It seem a microoptimization, I guess the JVM will cache that for you if it counts. (JVM option to optimize loop statements)

1.

 * @param str
 * @param id
 * @return

I'd remove these comments. (Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)

1.

public boolean isJSONValid(final String str, final int id) {

I'd rename the parameter to jsonString from str.

1.

private HashMap<String, Connection> tableStatement = new HashMap<String, Connection>();
private final LinkedHashMap<String, TableConnectionInfo> tableLists; 

HashMap<...> reference types should be simply Map<...>. See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces

1.

if (id >= LnPRead.startValidRange && id <= LnPRead.endValidRange) {

startValidRange sounds like a method (the name starts with a verb/action). I'd call it validRangeStart.