Commons Math
  1. Commons Math
  2. MATH-775

In the ListPopulation constructor, the check for a negative populationLimit should occur first.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 3.0
    • Fix Version/s: 3.1
    • Labels:

      Description

      In the ListPopulation constructor, the check to see whether the populationLimit is positive should occur before the check to see if the number of chromosomes is greater than the populationLimit.

      1. MATH-775.txt
        7 kB
        Reid Hochstedler

        Activity

        Hide
        Reid Hochstedler added a comment -

        Proposed patch for MATH-775.

        Show
        Reid Hochstedler added a comment - Proposed patch for MATH-775 .
        Hide
        Thomas Neidhart added a comment -

        Good catch.
        After looking at the class, I see even more problems:

        • setPopulationSize allows to change the population size, but does not check the chromosome list for compatible size
        • addChromosome does not check the population size at all
        • the chromosome list given as parameter to the ctor and setChromosomes is assigned directly, allowing the list to be changed from the outside later on

        From my understanding the following should happen:

        • setPopulationSize should shrink the existing chromosome list if it is larger than the new size (optionally we could also remove this method)
        • addChromosome should throw an exception if adding a chromosome would exceed the population size
        • when setting an externally provided chromosome list, the entries should be copied to a new internal list
        Show
        Thomas Neidhart added a comment - Good catch. After looking at the class, I see even more problems: setPopulationSize allows to change the population size, but does not check the chromosome list for compatible size addChromosome does not check the population size at all the chromosome list given as parameter to the ctor and setChromosomes is assigned directly, allowing the list to be changed from the outside later on From my understanding the following should happen: setPopulationSize should shrink the existing chromosome list if it is larger than the new size (optionally we could also remove this method) addChromosome should throw an exception if adding a chromosome would exceed the population size when setting an externally provided chromosome list, the entries should be copied to a new internal list
        Hide
        Thomas Neidhart added a comment -

        The patch has been committed in r1310103, together with other changes that have been outlined before.

        Additionally, I have added two new methods:

        • public void addChromosomes(Collection<Chromosome> c)
        • protected List<Chromosome> getChromosomeList()

        and made setChromosomes deprecated.

        Rationale:

        The internal state of ListPopulation shall be protected, and shall not be changeable from the outside as it was possible before. When adding chromosomes, the entries are added to the internal list, instead of setting the internal list reference to the provided list.

        Derived classes can get access to the internal list via getChromosomeList (we could also make the internal list protected, is there a policy in CM?).

        The setters throw appropriate exceptions to keep the internal state consistent, and addChromosome also throws an exception if the population would exceed the population limit.

        Show
        Thomas Neidhart added a comment - The patch has been committed in r1310103, together with other changes that have been outlined before. Additionally, I have added two new methods: public void addChromosomes(Collection<Chromosome> c) protected List<Chromosome> getChromosomeList() and made setChromosomes deprecated. Rationale: The internal state of ListPopulation shall be protected, and shall not be changeable from the outside as it was possible before. When adding chromosomes, the entries are added to the internal list, instead of setting the internal list reference to the provided list. Derived classes can get access to the internal list via getChromosomeList (we could also make the internal list protected, is there a policy in CM?). The setters throw appropriate exceptions to keep the internal state consistent, and addChromosome also throws an exception if the population would exceed the population limit.
        Hide
        Thomas Neidhart added a comment -

        I changed the internal list in ListPopulation to a protected member and removed the getChromosomeList again. This way it is cleaner imho.

        Show
        Thomas Neidhart added a comment - I changed the internal list in ListPopulation to a protected member and removed the getChromosomeList again. This way it is cleaner imho.
        Hide
        Sebb added a comment -

        Using a getter is better.
        Exposing a field means that it is harder to make changes later, e.g. should there be a need to synch. the field or make it read-only.

        As a general rule, mutable fields should never be exposed.

        Show
        Sebb added a comment - Using a getter is better. Exposing a field means that it is harder to make changes later, e.g. should there be a need to synch. the field or make it read-only. As a general rule, mutable fields should never be exposed.
        Hide
        Thomas Neidhart added a comment -

        hmm, I see your point. My concern was that there is already a public getChromosomes method that I have now changed to return an unmodifiable version of the internal list. As derived classes need access to the internal list (to alter it), I added a protected getChromosomeList method, but somehow this did not feel right.

        Maybe a different name for the method would suffice?

        Show
        Thomas Neidhart added a comment - hmm, I see your point. My concern was that there is already a public getChromosomes method that I have now changed to return an unmodifiable version of the internal list. As derived classes need access to the internal list (to alter it), I added a protected getChromosomeList method, but somehow this did not feel right. Maybe a different name for the method would suffice?
        Hide
        Thomas Neidhart added a comment -

        Fixed in r1325422.

        I kept the name getChromosomeList for the getter.

        Show
        Thomas Neidhart added a comment - Fixed in r1325422. I kept the name getChromosomeList for the getter.

          People

          • Assignee:
            Unassigned
            Reporter:
            Reid Hochstedler
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 1h
              1h
              Remaining:
              Remaining Estimate - 1h
              1h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development