Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

Hi I was wondering if someone could help me explain why I have to override the hashCode() method in this class and provide any other tips on making my code more readable. Thanks!

public class Synapse
{
private InputCell         inputCell;

private float             permanenceValue;

/**
 * Amount the permanenceValue is increased during learning.
 */
public static final float PERMANCE_INCREASE          = 0.015f;
/**
 * Amount the permanenceValue is decreased during learning.
 */
public static final float PERMANCE_DECREASE          = 0.005f;
/**
 * Minimal permanenceValue needed for a Synapse to be connected.
 */
public static final float MINIMAL_CONNECTED_PERMANCE = 0.2f;
/**
 * Initial permanceValues for distal Synapses.
 */
public static final float INITIAL_PERMANENCE         = 0.3f;


// ----------------------------------------------------------
/**
 * Create a new Synapse object.
 *
 * @param inputSource
 */
public Synapse(InputCell inputSource)
{
    this.inputCell = inputSource;
    this.permanenceValue = INITIAL_PERMANENCE;
}


// ----------------------------------------------------------
/**
 * Gets the inputCell for a Synapse object.
 *
 * @return The object providing binary input states to this Synapse.
 */
public InputCell getInputCell()
{
    return this.inputCell;
}


// ----------------------------------------------------------
/**
 * Sets a new inputSource for a Synapse object.
 *
 * @param inputCell
 *            The new inputSource to this Synapse.
 */
public void setInputCell(InputCell inputCell)
{
    this.inputCell = inputCell;
}


// ----------------------------------------------------------
/**
 * Get the private permanenceValue field of a Synapse.
 *
 * @return The permanence value of a Synapse object.
 */
public float getPermanenceValue()
{
    return this.permanenceValue;
}


// ----------------------------------------------------------
/**
 * Sets the permanenceValue for a Synapse object.
 *
 * @param permanenceValue
 *            The new permanenceValue for a Synapse object.
 */
public void setPermanenceValue(float permanenceValue)
{
    this.permanenceValue = permanenceValue;
}


// ----------------------------------------------------------
/**
 * Place a description of your method here.
 */
public void increasePermance()
{
    this.permanenceValue =
        Math.min(1.0f, this.permanenceValue + PERMANCE_INCREASE);
}


// ----------------------------------------------------------
/**
 * Place a description of your method here.
 */
public void decreasePermance()
{
    this.permanenceValue =
        Math.max(0.0f, this.permanenceValue - PERMANCE_DECREASE);
}


// ----------------------------------------------------------
/**
 * Return whether this Synapse's permanenceValue is greater than the minimal
 * connected permanenceValue.
 *
 * @return true if this Synapse is currently connected; otherwise false.
 */
public boolean getConnectedState()
{
    return this.permanenceValue >= MINIMAL_CONNECTED_PERMANCE;
}


// ----------------------------------------------------------
/**
 * Return whether this Synapse's permanenceVale is high enough to be active
 * due to the current input. No need for setter method to change input.
 *
 * @return true if this Synapse is active; otherwise false.
 */
public boolean getActiveState()
{
    return this.inputCell.getActiveState() && this.getConnectedState();
}


// ----------------------------------------------------------
/**
 * Return whether this Synapse is active due to the previous input. No need
 * for setter method to change previous input.
 *
 * @return true if this Synapse was active due to the previous input at t-1.
 */
public boolean getPreviousActiveState()
{
    return this.inputCell.getPreviousActiveState()
        && this.getConnectedState();
}


@Override
public String toString()
{
    StringBuilder stringBuilder = new StringBuilder();
    stringBuilder.append("\n============================");
    stringBuilder.append("\nSynapse Information");
    stringBuilder.append("\ninputSourceXPosition: ");
    stringBuilder.append(this.inputCell.getX());
    stringBuilder.append("\ninputSourceYPosition: ");
    stringBuilder.append(this.inputCell.getY());
    stringBuilder.append("\npermanenceValue: ");
    stringBuilder.append(this.getPermanenceValue());
    stringBuilder.append("\nconnectedState: ");
    stringBuilder.append(this.getConnectedState());
    stringBuilder.append("\nactiveState: ");
    stringBuilder.append(this.getActiveState());
    stringBuilder.append("\npreviousActiveState: ");
    stringBuilder.append(this.getPreviousActiveState());
    stringBuilder.append("\n============================");
    String synapseInformation = stringBuilder.toString();
    return synapseInformation;
}


@Override
public boolean equals(Object object)
{
    if (this == object)
    {
        return true;
    }
    if (this == null || object.getClass() != this.getClass())
    {
        return false;
    }
    Synapse newSynapse = (Synapse)object;
    return this.inputCell != null
        && (this.inputCell.equals(newSynapse.getInputCell()) && this.permanenceValue ==      newSynapse
            .getPermanenceValue());
}


// why does this method need to overriden.
@Override
public int hashCode()
{
    final int prime = 31;
    int result = 1;
    result =
        prime
            * result
            + ((this.inputCell.equals(null)) ? 0 : this.inputCell
                .hashCode());
    result = (int)(prime * result + this.permanenceValue);
    return result;
}
}
share|improve this question

3 Answers

up vote 5 down vote

Code-style remarks

I don't know your code-style. But I stayed to mention these things.

Class-scope indentation level

You don't make indentation for body of class. It's unusual.

public class Synapse
{
public InputCell inputCell;
}

public class Synapse
{
    public InputCell inputCell;
}

Horizontal Alignment

Prefer unaligned declarations and assignments because they point out an important deficiency. If I have long lists that need to be aligned, the problem is the length of the lists, not the lack of alignment. You don't have long list and I suggest you remove horizontal alignment:

public class Synapse
{
private InputCell         inputCell;

private float             permanenceValue;
}

public Class Synapse
{
    private InputCell inputCell;
    private float permanenceValue;
}

Comments

Redundant Comments

A comment is redundant if it describes something that adequately describes itself. Your Javadoc comments say nothing more than the function signature.

Noise Comments

Comments require supporting. Often they are remaining forgotten.

// ----------------------------------------------------------
/**
 * Place a description of your method here.
 */
public void increasePermance()
{
    this.permanenceValue =
        Math.min(1.0f, this.permanenceValue + PERMANCE_INCREASE);
}

Boolean Method Names

When I see method with name getConnectedState I expect that it will return enumeration. But it returns boolean. For method returning boolean I would prefer name as isConnected, isActive, etc.

share|improve this answer
up vote 2 down vote

It seems like you are calculating the value using a mutable field. That means it will return a different value when you set this field. GetHashcode() should return the same value over the life of an object. You can get more details in this question.

share|improve this answer
2  
It's true although it's Java :-) – palacsint Nov 4 '12 at 14:53
up vote 1 down vote

  1. I agree with @fasked (+1), horizontal alignment should be removed. It's a maintenance headache. If you need new field which type or name is longer than the current maximum you have to change lots of (non-relevant) lines in the code.

  2. You should validate your input parameters. Does it make sense to create a Synapse with null InputCell? If not, check it and throw a NullPointerException. The same could be true for the setInputCell method. (Effective Java, Second Edition, Item 38: Check parameters for validity)

  3. You could move 

    this.permanenceValue = INITIAL_PERMANENCE;

    from the constructor to the field declarations:

    private float permanenceValue = INITIAL_PERMANENCE;

  4. You might want to check the input value of the parameter of the setPermanenceValue method as you do in the decreasePermance and increasePermance methods. Should permanenceValue be less than zero or bigger than one? If not check it and throw an IllegalArgumentException.

  5. You usually need the hashCode method when you're using a hash-based collections (sets, lists, maps) such as HashMap, LinkedHashMap, HashSet, LinkedHashSet etc. Anyway, if you override the equals method you should provide the hashCode implementation too. Other developers might rely on it and its absence could cause weird bugs if somebody use your class with a hash-based collection.

    Some useful references:

  6. I think

    if (this == null || object.getClass() != this.getClass()) {

    should be

    if (object == null || object.getClass() != this.getClass()) {

    in the equals method. I'd generate these methods with Eclipse (it's less error-prone):

    @Override
public int hashCode() {
    final int prime = 31;
    int result = 1;
    result = prime * result + ((inputCell == null) ? 0 : inputCell.hashCode());
    result = prime * result + Float.floatToIntBits(permanenceValue);
    return result;
}

@Override
public boolean equals(final Object obj) {
    if (this == obj) {
        return true;
    }
    if (obj == null) {
        return false;
    }
    if (getClass() != obj.getClass()) {
        return false;
    }
    final Synapse other = (Synapse) obj;
    if (inputCell == null) {
        if (other.inputCell != null) {
            return false;
        }
    } else if (!inputCell.equals(other.inputCell)) {
        return false;
    }
    if (Float.floatToIntBits(permanenceValue) 
            != Float.floatToIntBits(other.permanenceValue)) {
        return false;
    }
    return true;
}

    Note the Float.floatToIntBits calls.

  7. I've found the following StringBuilder usage easier to read:

    stringBuilder.append("\ninputSourceXPosition: ").append(inputCell.getX());
stringBuilder.append("\ninputSourceYPosition: ").append(inputCell.getX());
...
share|improve this answer

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.