Tell me more ×
Stack Overflow is a question and answer site for professional and enthusiast programmers. It's 100% free, no registration required.
up vote 112 down vote favorite
24

I saw the following code in a specific commit for MongoDB's Java Connection driver (which has been updated since posting this question), and it appears at first to be a joke of some sort. Can someone explain what the following code does?

if (!((_ok) ? true : (Math.random() > 0.1))) {
    return res;
}
share|improve this question
8  
Which part of it is confusing you? – Oli Charlesworth May 30 at 9:58
2  
i think it's confusing. this code is executed in a catch block ! – Proviste May 30 at 10:00
5  
@MarkoTopolnik: Is it? It could be written much more clearly as if (!ok || Math.random() < 0.1) (or something similar). – Oli Charlesworth May 30 at 10:01
4  
github.com/mongodb/mongo-java-driver/commit/… you are not first, see comment to that line – msangel May 30 at 10:03
show 3 more comments

4 Answers

up vote 179 down vote accepted

After inspectng the history of that line, my main conclusion is that there has been some incompetent programming at work.

  1. That line is gratuitously convoluted. The general form

    a? true : b

    for boolean a, b is equivalent to the simple

    a || b

  2. The surrounding negation and excessive parentheses convolute things further. Keeping in mind De Morgan's laws it is a trivial observation that this piece of code amounts to

    if (!_ok && Math.random() <= 0.1)
  return res;

  3. The commit that originally introduced this logic had

    if (_ok == true) {
  _logger.log( Level.WARNING , "Server seen down: " + _addr, e );
} else if (Math.random() < 0.1) {
  _logger.log( Level.WARNING , "Server seen down: " + _addr );
}

    —another example of incompetent coding, but notice the reversed logic: here the event is logged if either _ok or in 10% of other cases, whereas the code in 2. returns 10% of the times and logs 90% of the times. So the later commit ruined not only clarity, but correctness itself.

    I think in the code you have posted we can actually see how the author intended to transform the original if-then somehow literally into its negation required for the early return condition. But then he messed up and inserted an effective "double negative" by reversing the inequality sign.

  4. Coding style issues aside, stochastic logging is quite a dubious practice all by itself, especially since the log entry does not document its own peculiar behavior. The intention is, obviously, reducing restatements of the same fact: that the server is currently down. The appropriate solution is to log only changes of the server state, and not each its observation, let alone a random selection of 10% such observations. Yes, that takes just a little bit more effort, so let's see some.

I can only hope that all this evidence of incompetence, accumulated from inspecting just three lines of code, does not speak fairly of the project as a whole, and that this piece of work will be cleaned up ASAP.

share|improve this answer
7  
Shouldn't the operator in 2. be && according to De Morgan's? – Otto Harju May 31 at 14:26
9  
Additionally this appears to be, as far as I can tell, the official 10gen Java driver for MongoDB so in addition to having an opinion on the Java driver, I think it gives me an opinion on the code of MongoDB – Chris Travers Jun 1 at 2:20
2  
Excellent analysis of just a few lines of code, I might just turn it into an interview question! Your fourth point is the real key why there's something fundamentally wrong with this project (the others could be dismissed as unfortunate programmer's bugs). – Abel Jun 4 at 22:22
@ChrisTravers It is the official mongo java driver for mongo. – assylias Jun 5 at 0:00
2  
It's fixed: jira.mongodb.org/browse/JAVA-836 – Artur Czajka 22 hours ago
up vote 9 down vote

https://github.com/mongodb/mongo-java-driver/commit/d51b3648a8e1bf1a7b7886b7ceb343064c9e2225#commitcomment-3315694

11 hours ago by gareth-rees:

Presumably the idea is to log only about 1/10 of the server failures (and so avoid massively spamming the log), without incurring the cost of maintaining a counter or timer. (But surely maintaining a timer would be affordable?)

share|improve this answer
4  
Not to nitpick but: 1/10th of the time it will return res, so it will log the other 9/10 times. – Supericy May 30 at 10:09
8  
@Supericy That's definitely not nitpicking. That's just yet more evidence of this person's terrible coding practices. – Anorov May 31 at 14:32
up vote 5 down vote

Add a class member initialized to negative 1:

  private int logit = -1;

In the try block, make the test:

 if( !ok && (logit = (logit + 1 ) % 10)  == 0 ) { //log error

This always logs the first error, then every tenth subsequent error. Logical operators "short-circuit", so logit only gets incremented on an actual error.

If you want the first and tenth of all errors, regardless of the connection, make logit class static instead of a a member.

As had been noted this should be thread safe:

private synchronized int getLogit() {
   return (logit = (logit + 1 ) % 10);
}

In the try block, make the test:

 if( !ok && getLogit() == 0 ) { //log error

Note: I don't think throwing out 90% of the errors is a good idea.

share|improve this answer
This one is not thread safe... – Nicolas Bousquet Jun 1 at 11:06
True, it should be made thread safe. – tpdi Jun 2 at 9:27
up vote 0 down vote

I have seen this kind of thing before,

There was a piece of code that could answer certain 'questions' that came from another 'black box' piece of code. In the case it could not answer them, it would forward them to another piece of 'black box' code that was really slow.

So sometimes previously unseen new 'questions' would show up, and they would show up in a batch, like 100 of them in a row.

The programmer was happy with how the program was working, but wanted some way of maybe improving the software in the future, if possible new questions were discovered.

So, the solution was to log unknown questions, but as it turned out, there were 1000's of different ones. The logs got to big, and there was no benefit of speeding these up, since they had no obvious answers. But every once in a while, a batch of questions would show up that could be answered.

Since the logs were getting to big, and the logging was getting in the way of logging the real important things he got to this solution:

only log a random 5%, this will clean up the logs, whilst in the long run still showing what questions/answers could be added.

So, if an unknown event occurred, in a random amount of these cases, it would be logged.

I think this is similar to what you are seeing here.

share|improve this answer
Except that we're talking about a database driver here... wrong problem space, IMO! – Steven Schlansker Jun 6 at 6:06

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.