HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Binary Bayes network classifier in Java - Part I/II

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
bayespartjavaclassifierbinarynetwork

Problem

I was working on the binary Bayesian network classifier I asked about earlier.

See also Part II/II.

See also the next iteration of Part I.

TERMINOLOGY
We are given a directed acyclic graph (dag) \$G = (V, A)\$, where \$V\$ is the set of nodes and \$A \subseteq V \times V\$ is the set of directed arcs, and a weight function \$p \colon V \to [0, 1]\$. For any node \$u \in V\$, \$parents(u) =\{v \in V \colon (v, u) \in A\}\$, which is a set of parent (or incoming) nodes of \$u\$. Now, each node \$u\$ corresponds to some part that "works" with probability \$p(u)\$ and "fails" with probability \$1 - p(u)\$. However, if any such \$u\$ has a non-empty set of parents and at least one of the parents failed, \$u\$ fails unconditionally.

MISSION I wrote a REPL (read, evaluate, print, loop) program, which allows its users to build a binary Bayesian network and perform queries on it; for example, p(not Radio, Battery | not Moves, Ignition), or "what is the probability that radio does not work and battery does work if we know that ignition is in order and the car does not move.

SAMPLE INPUT

```
# Create nodes.
new Battery 0.9 # Comment
new Radio 0.9
new Ignition 0.95
new Fuel 0.95
new Starts 0.99
new Moves 0.99

# Print a node.
Fuel
del Fuel
echo Removed Fuel
Fuel
new Fuel 0.95

# Create arcs.
connect Battery to Radio
connect Battery to Ignition
connect Ignition to Starts
connect Fuel to Starts
connect Starts to Moves

# Check a couple of arcs.
echo Battery connected to Radio?
is Battery connected to Radio # Must be 'true'.
echo Radio connected to Battery?
is Radio connected to Battery # Must be 'false'.

# Remove an arc (Battery, Radio).
disconnect Battery from Radio
echo Removed arc (Battery, Radio).
echo Is Battery connected to Radio?
is Battery connected to Radio
connect Battery to Radio

# Print a couple of nodes.
Battery
Starts
Radio

# List all possible system states with their probabilities.
list

# Try a couple of queries.
p(Battery|n

Solution

I like comments. I like Javadoc. But comments that say what are suspicious - good comments say why, not what. Given self-explanatory code, Javadoc on private members is overkill: you have Javadoc everywhere, and it's distracting.

/**
 * This method implements the actual REPL (Read, Evaluate, Print, Loop).
 * 
 * @param fileNames the array containing the names of the files to execute.
 *                  This array may be null, in which case the program reads
 *                  from the console. If array is not {@code null}, executes
 *                  the files in the order they appear in the array.
 */
private void loop(String[] fileNames) {


The method has outrageous cyclomatic complexity, is breaking the Single Responsibility Principle (SRP) by design, and is poorly named - granted, naming a method that's doing 4 things is certainly a hard thing to do. But given read, evaluate, print and loop, I really wonder why a method that does all 4 would simply be called loop.

Speaking of loops:

for (;;) {


Clearly you're abusing the for construct here. A more idiomatic "infinite loop" would be while (true).

Code Snippets

/**
 * This method implements the actual REPL (Read, Evaluate, Print, Loop).
 * 
 * @param fileNames the array containing the names of the files to execute.
 *                  This array may be null, in which case the program reads
 *                  from the console. If array is not {@code null}, executes
 *                  the files in the order they appear in the array.
 */
private void loop(String[] fileNames) {

Context

StackExchange Code Review Q#105041, answer score: 12

Revisions (0)

No revisions yet.