Skip to main content
Commonmark migration
Source Link

#I am the wrong reviewer

I am the wrong reviewer

##Why the most popular libraries are inherently slow.

Why the most popular libraries are inherently slow.

##Evaluation.

Evaluation.

###Documentation.

Documentation.

###Performance.

Performance.

###Cost

Cost

###Source code.

Source code.

###Snippet (A) Noise

Snippet (A) Noise

#I am the wrong reviewer

##Why the most popular libraries are inherently slow.

##Evaluation.

###Documentation.

###Performance.

###Cost

###Source code.

###Snippet (A) Noise

I am the wrong reviewer

Why the most popular libraries are inherently slow.

Evaluation.

Documentation.

Performance.

Cost

Source code.

Snippet (A) Noise

Source Link
Blindman67
  • 22.9k
  • 2
  • 17
  • 40

#I am the wrong reviewer

Coders can be grouped into two types, those that will use libraries for anything and everything, giving automatic trust too 3rd party code, and those that see libraries as inherently slow, a potential source of unknown bugs, and will only use a library after thorough evaluation of its source, documentation, performance, and cost (kicking and screaming the whole way, "No No... dont corrupt my perfect precious code... " ;) ).

I am of the latter and thus maybe not who you want evaluating your code. However as nobody has yet provided a review I will give mine.

##Why the most popular libraries are inherently slow.

A library is designed to work in unknown environments and be usable by both professional, amateur programmers, and many monkeys. A library can not trust any data or adherence to methodologies.

To protect the libraries stability it must vet anything that is exposed to the user (code that uses the library). Library code must constantly be on guard that internal state is not mutated.

Your code is lacking some basic protection and there are many ways to have it fail, not because your code logic is bad, but rather that the monkey using your code is feeding it bananas skins rather than the banana.

You need to add more vetting of arguments, and properties that come from external (un-trusted) sources.

##Evaluation.

This evaluation is in terms of my expectations of a library, is subjective and is certainly not how everyone would see it. The other camp will say, does it run? Yes, does it like my data? Yes great stuff add it to the pile.

###Documentation.

There is no documentation apart from that in the source. You should have a separate document that contains, at minimum, a Library overview, how too guide, abstraction explanation and complete API reference.

You should not have documentation spread randomly in your code. Even if this is just a hobby project at least move the documentation to the top of the source file in one comment.

That said the documentation you have given for each function is good, though the top METHODS: comment, listing API methods, is so sparse it provides little useful information.

You are also missing details about errors that can be thrown explicitly by your code or can be expected by the JS context.

###Performance.

Without running the code and based on just viewing the source I would say your code has good performance. You are not using any of the common anti performance patterns. Memory use is also good with not too much wastage and GC overhead.

Good is not great, there is thus room for improvement, anyone that has used GA's will know that they can be time costly functions with run times being very long.

I don't see any performance oriented patterns like object pools, pre-allocation, object reuse, and in-lining (Yes JS optimizers will inline for you, if anyone knows how to ensure that do tell)

###Cost

This is a hobby project (I assume) and dollars and cents don't come to play, well kind of. Cost is time, does you code save the users time. The time is not just adding the library to the project, it is in learning the API and reliability, you dont want bug hunting to enter the library costing users time.

Time savings. I ask, can I write the same (and tailored to my needs) in the same or lower time cost. Based on the fact anyone using the library will already be familiar with GA's, and an effective line count at ~250 you are at the time saving border, experienced codes would see using your lib as a time cost (I can write it in less time than it takes to evaluate it), less experienced coders will see a time saving.

As the primary designer you should ask your self, "Can I add more value (time benefit) to the library?"

###Source code.

Sorry to say the source code is of poor quality.

The primary reason is that you are using old school JS. The current JS version is ES8, and if it was not for the use of let (which you only use in for loops) and one function using a default parameter, your code is plain old ES5.

You need to move the source up to the current version of ECMAScript if you want people to evaluate it positively.

Code style points

There would be more but I am limiting this mostly to ES5 style points.

  • The naming convention used by JavaScript is camelCase. No library will ever be popular if you use snake_case.

  • Too much noise. You code is full of unneeded comments (I am excluding he function documentation but referring to the many // comments), redundant declarations and assignments (See snippet (A) below), and why are you wrapping () around so many expressions and functions for no reason?..

  • Exposing functions that are not part of the API generate_random_number, generate_random_string, get_algorithm_data_type

  • Lines too long, Break up long lines, You have lines over 250 characters long. Keep em under 80 characters, or 120 max. (needs to fit the display)

  • ES6+ offers many ways to improve the source code and will remove many lines from your code see snippet (B) moving one function from ES5 style to the current ES8 (I am still learning it so may have missed a feature)


###Snippet (A) Noise

        //Generates a random number
        generate_random_number: (function(minimum, maximum) {

            /*
            minimum => The minimum number the function should generate (-Inf, Inf)
            maximum => The maximum number the function should generate (-Inf, Inf)
            */

            //The return number the method returns
            var return_number = 0;

            return_number = Math.random() * (maximum - minimum) + minimum;

            return return_number;
        }),

With my comments

         /* This function should not be part of the exposed API */
         //Generates a random number /* NOISE This comment adds an "a" and is of no benefit at all */
         generate_random_number: (function(minimum, maximum) { 
            /* min and max are acceptable common names without ambiguity
               preference the shorter names to reduce noise levels */

             /* Move following comment to documentation */ 
             /*
             minimum => The minimum number the function should generate (-Inf, Inf)
             maximum => The maximum number the function should generate (-Inf, Inf)
             */

             //The return number the method returns /* NOISE really is that what return_number is for???? :P */
             var return_number = 0; /* REDUNDANT no need for assignment of 0 */ 
             /* REDUNDANT the variable is not needed */
             return_number = Math.random() * (maximum - minimum) + minimum;

             return return_number; /* Can return the random number directly rather than via a variable. */

         }),

Rewrite moves function out of genetic_algorithm also include the string function, the first is as you would write it if you included it in the API the second is how it can be written outside the API and thus trust the caller

      const randomNum = (min, max) => Math.random() * (max - min) + min;
 
      // as property of genetic_algorithm
      randomString(len, generator = randomNum) => {
         var result = "";
         if(!isNaN(len) && typeof generator === "function"){
             len = Math.floor(len); // Or same as len |= 0
             len = len > 0 ? len : 0;
             while (len--) { result += String.fromCharCode(generator(0, 65535)) }
             return result;
         }
         // You add your error policy here
      }
       
      // as hidden private function 
      const randomString = (len, generator = randomNum) => {
         var result = "";
         while (len--) { result += String.fromCharCode(generator(0, 65535)) }
         return result;
      }

Snippet (B) Old school to New

        evaluate_population: (function (input, population, generations, mutation_rate, expansion_multiplier, maximum_population,average_fitness_score,time_began) {
            var return_results = {};
            var has_found_fittest = false;
            var fittest = {};
            var average_accuracy = 0;
            var time_elapsed = 0;
            for (let i = 0; i < population.length; i++) {
                if (population[i].individual === input) {
                    has_found_fittest = true;
                    fittest = population[i];
                    break;
                }
            }
            average_accuracy = (average_fitness_score / input.length);
            time_elapsed = (Date.now() - time_began);
            return_results.has_found_fittest = has_found_fittest;
            return_results.fittest_individual = fittest;
            return_results.population = population;
            return_results.generations = generations;
            return_results.mutation_rate = mutation_rate;
            return_results.expansion_multiplier = expansion_multiplier;
            return_results.maximum_population = maximum_population;
            return_results.average_fitness_score = average_fitness_score;
            return_results.average_accuracy = average_accuracy;
            return_results.time_elapsed = time_elapsed;
            return return_results;
        }),

Commented

          /* Use function shorthand */
          /* Destructure arguments so that order is not important 
             Extract needed properties and properties you don't want in the return object*/
         evaluate_population: (function (input, population, generations, mutation_rate, expansion_multiplier, maximum_population,average_fitness_score, time_began) { 
             var return_results = {}; /* Not needed return directly
             var has_found_fittest = false;
             var fittest = {}; /* UNNEEDED assignment I think as I would use undefined so that it would throw if treated as a individual */
             var average_accuracy = 0;  /* UNNEEDED variable */
             var time_elapsed = 0;  /* UNNEEDED variable */
             for (let i = 0; i < population.length; i++) { /* Unsafe array */
                 if (population[i].individual === input) { /* Unsafe property access*/
                     has_found_fittest = true;
                     fittest = population[i];
                     break;
                 }
             }
             average_accuracy = (average_fitness_score / input.length); /* UNNEEDED assignment */
             time_elapsed = (Date.now() - time_began); /* UNNEEDED assignment */

             /* Use spreed operator most of the following can be done in a single line (8 characters).  ...args, */
             return_results.has_found_fittest = has_found_fittest;
             return_results.fittest_individual = fittest;
             return_results.population = population;
             return_results.generations = generations;
             return_results.mutation_rate = mutation_rate;
             return_results.expansion_multiplier = expansion_multiplier;
             return_results.maximum_population = maximum_population;
             return_results.average_fitness_score = average_fitness_score;
             return_results.average_accuracy = average_accuracy;
             return_results.time_elapsed = time_elapsed;

             return return_results; /* Return directly */
         }),

Rewrite

        evaluatePopulation({input, population, timeBegan, ...args})
            var hasFoundFittest = false;
            var fittestIndividual;
            if (Array.isArray(population) && Array.isArray(input)) {
                for (let i = 0; i < population.length; i++) {
                    if (population[i] && population[i].individual === input) {
                        hasFoundFittest = true;
                        fittestIndividual = population[i];
                        break;
                    }
                }
                return {
                    hasFoundFittest, 
                    fittestIndividual, /* This will be undefined if no fittest found, This may not be what you want */
                    population,
                    averageAccuracy : args.averageFitnessScore / input.length,
                    timeElapsed : Date.now() - timeBegan,
                    ...args
                };
            }
            /* There is no established error methology so just throwing */
            throw new Error("Bad banana!");
        }),