Distribute stats randomly

Question

I have returned to programming after a year without practice and this is also my first time using Java. I know there is a more efficient way to do this so any suggestions would be appreciated.

I am trying to distribute stats randomly to a character in my game. However, I can't just go through each stat and increase/decrease each one randomly. I need the total increases to be equal to the total decreases. So if one stat goes up 5%, another will need to decrease 5% to compensate.

Egg make_egg(){

float newgr = growth_rate;
float newms = maxSpeed;
int newsf = starve_frames;
int newls = lifespan;

ArrayList<Integer> stats = new ArrayList<Integer>();
stats.add(0);
stats.add(1);
stats.add(2);
stats.add(3);

float percentage = 0;
int roll_stat = 0;
while(stats.size() > 1){
  percentage = random(0,1);
  //get the value of a random element in the arraylist
  roll_stat = stats.get(round(random(0, stats.size()-1)));
  //remove(int) is ambiguous for an ArrayList<Integer>
  //am I doing this right?
  stats.remove(Integer.valueOf(roll_stat));

  //--------------LOSSES-------------//
  if(roll_stat == 0){           //gr
      newgr -= percentage*growth_rate_delta;
      if(newgr <= minimum_growth_rate){
        newgr = minimum_growth_rate;
      }
  }
  else if(roll_stat == 1){      //ms
      newms -= percentage*maxspeed_delta;
      if(newms <= minimum_maxspeed){
        newms = minimum_maxspeed;
      }
  }
  else if(roll_stat == 2){      //sf
     newsf -= round(percentage*starve_frames_delta);
     if(newsf <= starve_frames_minimum){
        newsf = starve_frames_minimum;
      }
  }
  else if(roll_stat == 3){      //ls
     newls -= round(percentage*lifespan_delta);
     if(newls <= lifespan_minimum){
        newls = lifespan_minimum;
      }
  }      

  roll_stat = stats.get(round(random(0, stats.size()-1)));
  stats.remove(Integer.valueOf(roll_stat));

  //--------------GAINS-------------//
  if(roll_stat == 0){            //gr
      newgr += percentage*growth_rate_delta;
      if(newgr >= maximum_growth_rate){
        newgr = maximum_growth_rate;
      }
  }
  else if(roll_stat == 1){      //ms
      newms += percentage*maxspeed_delta;
      if(newms >= maximum_maxspeed){
        newms = maximum_maxspeed;
      }
  }
  else if(roll_stat == 2){      //sf
     newsf += round(percentage*starve_frames);
     if(newsf >= starve_frames_maximum){
        newsf = starve_frames_maximum;
      }
  }
  else if(roll_stat == 3){      //ls
     newls += round(percentage*lifespan_delta);
     if(newls >= lifespan_maximum){
        newls = lifespan_maximum;
      }
  }

}

return new Egg(x, y, newgr, newms, newsf, newls, generation + 1);
}

I know this will not work if there are an odd number of stats, but I can deal with that later. One other problem this has is that two stats will always decrease and two stats will always increase. However, the ideal effect would be if, for example, one stat could increase by 9%, and the other three stats could decrease by 5, 3, and 1% each.

Answer 1

Reviewing the code as it is, rather than the problem with the statistical distribution.

Indentation

Your indentation is off and thus makes the code hard to read. Everything inside the method should be indented. Also it seems like some of the indentations are differently deep. Indentations in Java are usually 4 spaces.

Access modifiers

Your method does not have an access modifier (at least not explicitly), so I assume you forgot to add one. Access modifiers are important to control from where in the code the method (or class or member etc.) can be acccessed. Since your method does not have one, it implicitly has the default modifier, which is package-private. This might be correct, but if you just forgot it, use private if the method is only called within the same class.

Naming

Your variable names are unclear. In the context of float newgr = growth_rate; one might guess that newgr may mean something like "new growth rate", but without context it is completely unclear what it means. Use longer and more explicity names.

Also the names on the right part of the assignment are unconventional, because in Java you usually want to use camelCase for variables and methods, and UpperCamelCase aka PascalCase for classes and interfaces.

Your variable declarations should probably be something like this:

float newGrowthRate = growthRate;
float newMaxSpeed = maxSpeed;
int newstarveFrames = starveFrames;
int newLifeSpan = lifeSpan;

The same applies to some other variable names further down your code.

---

Your percentage variable is not really a percentage, because it can have values between 0 and 1. Percentages are values between 0 and 100, so you could change the scaling and use a random calue between 0 and 100, or simply rename the variable to something like factor or scaling. If you don't like any of those names and can't come up with a better one, change it to normalizedPercentage.

Style

In addition to the indentation, you are using newlines and whitespace in an unusual way.

For example

while(stats.size() > 1){

should be written like

while (stats.size() > 1) {

This makes the code more readible and consistent. Also around operators there should be spaces, like percentage * maxspeed_delta instead of percentage*maxspeed_delta.

Switch-Case

Using the switch-syntax you can make your code more readible where you are using many if-else blocks with integer comparison conditions.

So this

if(roll_stat == 0){           //gr
    newgr -= percentage*growth_rate_delta;
    if(newgr <= minimum_growth_rate){
        newgr = minimum_growth_rate;
    }
}
else if(roll_stat == 1){      //ms
    newms -= percentage*maxspeed_delta;
    if (newms <= minimum_maxspeed) {
        newms = minimum_maxspeed;
    }
}

can become this

switch (roll_stat) {
    case 0:
        newgr -= percentage * growth_rate_delta;
        if(newgr <= minimum_growth_rate){
            newgr = minimum_growth_rate;
        }
        break;
    case 1:
        newms -= percentage * maxspeed_delta;
        if (newms <= minimum_maxspeed) {
            newms = minimum_maxspeed;
        }
        break;
}

Don't forget the break after every case. If you have an else at the end (instead of an else if) use the default: case.

Comments

//get the value of a random element in the arraylist
roll_stat = stats.get(round(random(0, stats.size()-1)));

This comment is obsolete, because it just says what the code in the next line says anyway. Instead try to write self-documenting code, for example by selecting the random index first and saving it in a variable, so that the variable's name tells you what it is. Also you can directly get a random integer value, so you don't have to round. Then you get

Random random = new Random();
// nextInt(n) takes the upperbound exclusively, thus not stats.size() - 1
int randomIndex = random.nextInt(stats.size());
randomStat = stats.get(randomIndex);

Comments should tell why something is done, not what is done. What is done should be told by the code.

---

You are using //--------------LOSSES-------------// and //--------------GAINS-------------// to separate logical parts of your code. It is better to move these parts into methods and have the methods name explain what is done.

Removing items from ArrayList<Integer>

In a comment you ask whether you do this correctly. If I understand your intent correctly, then you are not doing it right, and it only makes no difference in your case because your items' values are the same as their indexes.

It is important to note that int and Integer are not the same thing. They are two different types, Integer being a wrapper for the primitive int. In some cases the JVM automatically "boxes" these ints into Integer objects or "unboxes" them the other way around. Since an ArrayList<Integer> contains Integer objects, when doing stats.remove(Integer.valueOf(roll_stat)); the ArrayList will search for the contained object if it exists in the list, and stats.remove(roll_stat); will remove the item at the index roll_stat. Since you randomly select an item by calculating a random index, you might as well save the index in a variable (see my example above where I do it for better readability anyway) and then remove by index.

Initializing your ArrayList

This seems a bit verbose:

ArrayList<Integer> stats = new ArrayList<Integer>();
stats.add(0);
stats.add(1);
stats.add(2);
stats.add(3);

Instead you can do this:

ArrayList<Integer> stats = new ArrayList<>(Arrays.asList(0, 1, 2, 3));

Answer 2

Most of your code is just repeating the same instructions over and over again. Define arrays newStats, deltas, and statMinimums, and your entire LOSSES block can be collapsed into one line of code: newStats[roll_stats] = max(newStats[roll_stats]-percentage*deltas[roll_stats],statMinimums[roll_stats])

However, before you try to figure out how to implement what you want, you need to clearly define what it is that you want. What exactly do you want as far as the distribution of stat changes? There are several different methods, depending on what distribution you want. For instance, if there are n stats, you can:

1. Generate an array of n percentages.
2. Calculate the average percentage.
3. Subtract the average from each element.
4. Do a for-loop of i from 0 to n-1 and set newStats[i] = min(max(newStats[i]+percentage[i]*deltas[i],statMinimums[i]),statMaximums[i])