4
\$\begingroup\$

I have the following method:

public void test() {

    List<Integer> slopeChanges = new ArrayList<Integer>();
    slopeChanges.add(0);
    slopeChanges.add(0);
    slopeChanges.add(1);
    slopeChanges.add(0);
    slopeChanges.add(1);

    List<Double> sixProbabilites = new ArrayList<Double>();

    int count_neg10 = 0, count_neg11 = 0, count_10 = 0, count_1neg1 = 0, count_0neg1 = 0, count_01 = 0;

    for(int i=0; i<slopeChanges.size(); i++) {
        if(i+1 != slopeChanges.size()) {
            if((slopeChanges.get(i) == -1) && (slopeChanges.get(i+1) == 0)) {count_neg10++;}
            else if((slopeChanges.get(i) == -1) && (slopeChanges.get(i+1) == 1)) {count_neg11++;}
            else if((slopeChanges.get(i) == 1) && (slopeChanges.get(i+1) == 0)) {count_10++;}
            else if((slopeChanges.get(i) == 1) && (slopeChanges.get(i+1) == -1)) {count_1neg1++;}
            else if((slopeChanges.get(i) == 0) && (slopeChanges.get(i+1) == -1)) {count_0neg1++;}
            else if((slopeChanges.get(i) == 0) && (slopeChanges.get(i+1) == 1)) {count_01++;}
        }else {
            if((slopeChanges.get(i) == -1) && (slopeChanges.get(0) == 0)) {count_neg10++;}
            else if((slopeChanges.get(i) == -1) && (slopeChanges.get(0) == 1)) {count_neg11++;}
            else if((slopeChanges.get(i) == 1) && (slopeChanges.get(0) == 0)) {count_10++;}
            else if((slopeChanges.get(i) == 1) && (slopeChanges.get(0) == -1)) {count_1neg1++;}
            else if((slopeChanges.get(i) == 0) && (slopeChanges.get(0) == -1)) {count_0neg1++;}
            else if((slopeChanges.get(i) == 0) && (slopeChanges.get(0) == 1)) {count_01++;}
        }
    }

    System.out.println(slopeChanges);

    if(count_neg10 != 0) {System.out.println("count_neg10 = " + count_neg10);}
    if(count_neg11 != 0) {System.out.println("count_neg11 = " + count_neg11);}
    if(count_10 != 0)    {System.out.println("count_10 = " + count_10);}
    if(count_1neg1 != 0) {System.out.println("count_1neg1 = " + count_1neg1);}
    if(count_0neg1 != 0) {System.out.println("count_0neg1 = " + count_0neg1);}
    if(count_01 != 0)    {System.out.println("count_01 = " + count_01);}

    if(count_neg10 != 0) {sixProbabilites.add((double)count_neg10 / ((double)slopeChanges.size()));}
    if(count_neg11 != 0) {sixProbabilites.add((double)count_neg11 / ((double)slopeChanges.size()));}
    if(count_10 != 0) {sixProbabilites.add((double)count_10 / ((double)slopeChanges.size()));}
    if(count_1neg1 != 0) {sixProbabilites.add((double)count_1neg1 / ((double)slopeChanges.size()));}
    if(count_0neg1 != 0) {sixProbabilites.add((double)count_0neg1 / ((double)slopeChanges.size()));}
    if(count_01 != 0) {sixProbabilites.add((double)count_01 / ((double)slopeChanges.size()));}
}

The slopeChanges list contains symbols that are either 0, 1, or -1. I would like to count frequencies of sub-blocks that are either 01, 10, -11, 1-1, -10, 0-1. The method that I showed does what I want, for example, this block of symbols 00101 should result in count_10 = 2 which means 10 sub-block occurs twice while 01 (i.e., count_01 = 2) occurs twice. Please note that the we consider the first symbol is also last.

As you can see, there are lot of duplication in the code in order to perform what I want. Is there a better way to re-write this function in a very elegant style?

\$\endgroup\$
  • \$\begingroup\$ are you sure of this: this block of symbols 00101 should result in count_10 = 2 which means 10 sub-block occurs twice \$\endgroup\$ – Adam Silenko Oct 22 '18 at 17:03
  • \$\begingroup\$ @AdamSilenko Yes, I mentioned that we consider the first symbol is also last. \$\endgroup\$ – Adam Amin Oct 22 '18 at 17:04
1
\$\begingroup\$

Reducing Duplication

The first if statement can be removed by taking a step back and realising that what we really want is the index of the next element, which is usually i + 1 but wraps back around to the start of the list. We can use the modulo % operator to achieve this.

The inner if-else statements can be refined by storing the slope changes in a 2d array, imagining a chart with the x and y axis corresponding to the current and next values of slopeChanges respectively.

Looping over this array (ignoring any slope changes that you're not interested in), we can both print the number of each slope change that occurred and add the probability of it occurring to the probability array.

I also separated the core logic of the program into it's own class and method, allowing for easier re-use.

Example of changes, I took some liberties by adjusting the output text slightly, I felt it no longer made sense to print the variable names since they had been removed.

public class Main
{
  public static void main(String[] args)
  {
    List<Integer> slopeChanges = new ArrayList<Integer>();
    slopeChanges.add(0);
    slopeChanges.add(0);
    slopeChanges.add(1);
    slopeChanges.add(0);
    slopeChanges.add(1);

    List<Double> sixProbabilities = SlopeCounter.CalculateProbabilities(slopeChanges);
  }
}

public class SlopeCounter
{
  public static List<Double> CalculateProbabilities(List<Integer> slopeChanges)
  {
    List<Double> sixProbabilities = new ArrayList<Double>();

    int slopeChangeSize = slopeChanges.size();

    int[][] slopeChangeCounts = new int[3][3];

    for(int i = 0; i < slopeChangeSize; i++) {
        int nextIndex = (i + 1) % slopeChangeSize;

        int currentValue = slopeChanges.get(i);

        int nextValue = slopeChanges.get(nextIndex);

        // Relying on the fact that both currentValue and
        // nextValue are between -1 and 1 we can add 1 to
        // both values to store in a 2d array

        slopeChangeCounts[currentValue + 1][nextValue + 1]++;

        // This will also count non-slopes, i.e. 11, 00, -1-1,
        // but we can ignore these
    }

    System.out.println(slopeChanges);

    for (int j = 0; j < 3; j++) {
        for (int k = 0; k < 3; k++) {
            if(j == k) continue; // Skip non-slopes

            int changeCount = slopeChangeCounts[j][k];

            if(changeCount == 0) continue; // Skip slopes that didnt happen

            System.out.println("Slope " + (j-1) + " to " + (k-1) + " = " + changeCount);

            sixProbabilities.add((double)changeCount / ((double)slopeChangeSize));
        }
    }

    return sixProbabilities;
  }
}
| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

The 2nd way is to use the array with unique index calculated from current and next value:

public void test() {

    List<Integer> slopeChanges = new ArrayList<Integer>();
    slopeChanges.add(0);
    slopeChanges.add(0);
    slopeChanges.add(1);
    slopeChanges.add(0);
    slopeChanges.add(1);

    /*
    static final int count_neg10 = 2; // -1 + 3 * (0 + 1);
    static final int count_neg11 = 5; // -1  + 3 * (1 + 1);
    static final int count_10 = 4; // 1 + 3 * (0 + 1);
    static final int count_1neg1 = 1; // 1 + 3 * (-1 + 1);
    static final int count_0neg1 = 0; // 0  + 3 * (-1 + 1);
    static final int count_01 = 6; // 0  + 3 * (1 + 1);*/

    int[] count = {0, 0, 0, 0, 0, 0, 0};
    String[] count_names = {"count_0neg1", "count_1neg1", "count_neg10", "", "count_10", "count_neg11", "count_01"};

    List<Double> sixProbabilites = new ArrayList<Double>();

    for(int i = 0, j = slopeChanges.size() -1; i<slopeChanges.size(); i++) {
      if (slopeChanges.get(i) != slopeChanges.get(j)) {
        count[slopeChanges.get(i) + 3 * (slopeChanges.get(j) + 1) ]++;
      } 
      j = i;
    }

    System.out.println(slopeChanges);

    for(int i = 0; i<7; i++) {
      if(count[i] != 0) {
        System.out.println(count_names[i] + " = " + count[i]);
        sixProbabilites.add((double)count[i] / ((double)slopeChanges.size()));
      }
    }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ it return [0, 0, 1, 0, 1] count_10 = 2 count_01 = 2 \$\endgroup\$ – Adam Silenko Oct 22 '18 at 19:44
0
\$\begingroup\$

You can remove some else using two indexes, and aggregate if.

public void test() {

    List<Integer> slopeChanges = new ArrayList<Integer>();
    slopeChanges.add(0);
    slopeChanges.add(0);
    slopeChanges.add(1);
    slopeChanges.add(0);
    slopeChanges.add(1);

    List<Double> sixProbabilites = new ArrayList<Double>();

    int count_neg10 = 0, count_neg11 = 0, count_10 = 0, count_1neg1 = 0, count_0neg1 = 0, count_01 = 0;

    for(int i = 0, j = slopeChanges.size() -1; i<slopeChanges.size(); i++) {
      if (slopeChanges.get(j) == -1) {
        if (slopeChanges.get(i) == 0) {count_neg10++;}
        else if(slopeChanges.get(i) == 1) {count_neg11++;}
      } 

      else if (slopeChanges.get(j) == 1) {
        if (slopeChanges.get(i) == 0) {count_10++;}
        else if(slopeChanges.get(i) == -1) {count_1neg1++;}
      } 

      else if (slopeChanges.get(j) == 0) {
        if (slopeChanges.get(i) == -1) {count_0neg1++;}
        else if(slopeChanges.get(i) == 1) {count_01++;}
      }
      j = i;
    }

    System.out.println(slopeChanges);

    if(count_neg10 != 0) {
      System.out.println("count_neg10 = " + count_neg10);
      sixProbabilites.add((double)count_neg10 / ((double)slopeChanges.size()));
    }
    if(count_neg11 != 0) {
      System.out.println("count_neg11 = " + count_neg11);
      sixProbabilites.add((double)count_neg11 / ((double)slopeChanges.size()));
    }
    if(count_10 != 0)    {
      System.out.println("count_10 = " + count_10);
      sixProbabilites.add((double)count_10 / ((double)slopeChanges.size()));
    }
    if(count_1neg1 != 0) {
      System.out.println("count_1neg1 = " + count_1neg1);
      sixProbabilites.add((double)count_1neg1 / ((double)slopeChanges.size()));
    }
    if(count_0neg1 != 0) {
      System.out.println("count_0neg1 = " + count_0neg1);
      sixProbabilites.add((double)count_0neg1 / ((double)slopeChanges.size()));
    }
    if(count_01 != 0)    {
      System.out.println("count_01 = " + count_01);
      sixProbabilites.add((double)count_01 / ((double)slopeChanges.size()));
    }
}
| improve this answer | |
\$\endgroup\$
0
\$\begingroup\$

Taking the response from @Adam_Silenko as a new base (j is like a previous circular index) and doing some change to generalize the algorithm.

You have some options:

Using a switch statement to decrease the number of branch, with some magic formula:

//...
for(int i = 0, j = slopeChanges.size() -1; i<slopeChanges.size(); i++) {
  switch (2* slopeChanges.get(j) - slopeChanges.get(i)) {
    case 2* (-1)-( 0): count_neg10++; break;
    case 2* (-1)-( 1): count_neg11++; break;
    case 2* ( 0)-(-1): count_0neg1++; break;
    case 2* ( 0)-( 1): count_01++;  break;
    case 2* ( 1)-(-1): count_1neg1++; break;
    case 2* ( 1)-( 0): count_10++;  break;
  }
  j = i;
}
//...

Or, maybe more short and efficient, using a two dimensional array (and incrementing the value since an index can't be negative):

// ...
int[][] count = new int[3][3];
for(int i = 0, j = slopeChanges.size() -1; i<slopeChanges.size(); i++) {
  count[slopeChanges.get(j)+1][slopeChanges.get(i)+1]++;
  j = i;
}
// ...

Or even, mix the two methods, using a simple array where the index comes from a formula like in the first option.

After that, you surely have to encapsulate the logic of your algorithm (counting and results) into classes with a good interface, for usability.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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