Java class: for getOrder, getData, updateData operation

Question

Problem Statement :

Design a class for three operations for given set of integers :

int getOrder(int data) /* order meaning rank in list */
int getData(int order)
void updateData(int oldData, int newData) /* [Most used operation] */

My solution

class App {
 Set<Integer> appData; 
 public App(List<Integer> data) {
 this.appData = new TreeSet<Integer>(new Comparator<Integer>() {
 @Override
 public int compare(Integer o1, Integer o2) {
 if(o1 > o2) {
 return -1;
 } else if (o1 < o2) {
 return 1;
 } else {
 return 0;
 }
 }
 });
 appData.addAll(data);
 }
 public int getOrder(int m) {
 int counter = 1;
 for(Integer i : this.appData) {
 if(i == m) {
 return counter;
 }
 counter ++ ;
 }
 return -1;
 }
 public int getdata(int order) {
 int counter = 1;
 for(Integer data : this.appData) {
 if(counter == order) {
 return data;
 }
 counter++;
 }
 return -1;
 }
 public void updatedata(int olddata, int newdata) {
 if(appData.contains(olddata)) {
 this.appData.remove(olddata);
 }
 this.appData.add(newdata);
 }
}

• Simon Forsberg: What is the variable mark in your code? I don't see it declared anywhere. When copying code to Code Review, please make sure that you copy your code, all your (required) code, exactly your code, and nothing but your code, so help you.... code.
• Simon Forsberg: Are you able to use Java 8 ?
• NooB8374: No.. No java 8. Only Java 6.

Answer 1

(Please post working code next time.)

So first off, there are imports missing, so this doesn't compile as it is, and the variable mark doesn't exist.

import java.util.List;
import java.util.Set;
import java.util.TreeSet;
import java.util.Comparator;

Either you import by name, or the whole package, just stick with one.

...
 if(counter == order) {
 return data;
 }
...

Then you should generally specify the visibility on all things, otherwise you end up with package visibility, which is probably not what you want.

public class App {
...
private Set<Integer> appData;

Use of generics is good, the constructor is okay except for the unnecessary manual comparison. This should be either using TreeSet.descendingSet, or Comparator.reverseOrder, e.g.:

public App(List<Integer> data) {
 this.appData = new TreeSet<Integer>(Comparator.<Integer>reverseOrder());
 appData.addAll(data);
}

Some whitespace is also off and the spelling of methods should be consistent, i.e. getdata should be get getData and updatedata should be updateData as is mentioned in your spec.

That just to get you started here.

• Simon Forsberg: When posting on Code Review, it is very normal to just skip the imports, especially for java.util classes.
• ferada: I don't agree because that means I have to do work to get it to compile.
• rolfl: I think this would be a good discussion for chat in the 2nd monitor
• NooB8374: Is treeset best thing for these operations ? I don't see any mention of complexity on java doc of that. Was confused between priority queue and treeset..