current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I'm trying to design a binary relation interface in Java and then implement it, but I'm not sure if I'm doing it right. I'd really appreciate a little feedback just so I know if I'm way in the wrong direction or not.

This is what I've got so far:

Interface:

public boolean compare(Object that, Object ths);
// compares relations to see if they are equal.

public Set<Y> getY(X value);
//return set of all Y values with relation X

public Set<X> getX(Y value);
//return set of all X values with relation Y

public void clear();
//clear relation of values

public Set<Relation<X, Y>> delY(X value);
//remove all relations containing given Y

public Set<Relation<X, Y>> delX(Y value);
//remove all relations containing given X

public String write(Object that);
//output Relation in string format

public Y addPair (X valX, Y valY);
//adds the entry (X, Y) to the Relation

public Y delPair (X valX, Y valY);
//deletes entry (X, Y) from the Relation

Implementation:

int size;

String[][] basket = new String[16][2];
String[][] bag = basket;



public String addPair(String X, String Y){

    basket[size][0] = X;
    basket[size][1] = Y;

    String pair = basket[size][0] + basket[size][1];
    size++;
    return pair;

}

public boolean compare(String[][] that, String[][] ths){
    that = basket;
    ths = bag;
    boolean equal;

    if(that.equals(ths))
        equal = true;
    else
        equal = false;
    return equal;
}


public Set<String> getY(String X){

    Set<String> Y = new TreeSet<String>();

    for(int i = 0;i<size;i++)
    {
        if(basket[i][0]==X)
            Y.add(basket[i][1]);
    }

    return Y;

}
public Set<String> getX(String Y){

    Set<String> X = new TreeSet<String>();

    for(int i = 0;i<size;i++)
    {
        if(basket[i][1]==Y)
            X.add(basket[i][0]);
    }

    return X;

}

public String write(Object o){


    o = basket;
    String out = "";
    out += o.toString();
    System.out.println("OUT: " + out);
    return out;

}



public void clear(){
 for(int i = 0;i<size;i++)
    {
        if(basket[i][0]!= null){
            basket[i][0] = null;
            basket[i][1] = null;
        }
    }
}

public Set<String> delY(String X, Relation r){
    Set<String> Y = new TreeSet<String>();

    for(int i = 0;i<size;i++)
    {
        if(basket[i][0]==X){
            Y.add(basket[i][1]);
            basket[i][1]=null;
        }
    }

    return Y;

}

public Set<String> delX(String Y){
   Set<String> X = new TreeSet<String>();

    for(int i = 0;i<size;i++)
    {
        if(basket[i][1]==Y){
            X.add(basket[i][1]);
            basket[i][1]=null;
        }
    }


   return X;
}

public String delPair (String X, String Y){
    X = "Orange";
    Y = "Fruit";
    for(int i = 0;i<size;i++)
    {
        if(basket[i][0]==X && basket[i][1]==Y){
            basket[i][1]=null;
            basket[i][0]=null;
        }
    }

     return (X + Y);

}
share|improve this question
add comment

1 Answer

up vote 2 down vote

There are a couple of things in here that concern me.... but, the most concerning is:

Use .equals() to compare String values

Code like this:

if(basket[i][0]==X){

will fail when you get data that is not String-constant data.

That line should be:

 if(basket[i][0].equals(X)){

This problem is found all over your code.

In addition, there are other problems:

  • your arrays are pre-sized. This code:

    String[][] basket = new String[16][2];

    is potentially OK if you are initializing the array, but what if there are more than 16 pairs?

  • it would be better to create a 'Pair' class to contain your two strings, than to use the 2-size array on the basket. Then your basket could be something like:

    StringPair[] basket = new StringPair[SIZE];

    your StringPair class could look something like:

    public class StringPair {
    private final String first, second;

    public StringPair(String first, String second) {
        this.first = first;
        this.second = second;
    }

    ..... getFirst() .....
    ..... getSecond() .....

}

  • compare(String[][] that, String[][] ths) is a method that needs no state data. It should be a static method, or a utility method. Alternatively, it should be a simple 1-value method compare(MyClass that) ... which, for what it is worth, would be very similar to the equals(...) method.

    but, that method is pretty broken. It has the two input parameters that and ths, but it then overwrites those values with the internal versions bag and basket.

    that = basket;
ths = bag;
boolean equal;

if(that.equals(ths))
    equal = true;
else
    equal = false;
return equal;

    now, since you set bag = basket earlier, this will always return true.

    Further, that last 5 lines of the method can be replaced with:

    return (that.equals(ths));
share|improve this answer
add comment

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.