Army strength based on level and modifiers Right so I was doing a quick project, and sought a quick way to finish one section of my Python code. I handed it in, but I'm well aware that it's bad code, but I was wondering if there was a good way to condense this code into fewer lines.

def nGen_army(level, strength, weakness):
    if weakness == 1:
        if level == 0:
            army = round(random.randint(1000,5000),4)
            return army
        if level == 1:
            army = round(random.randint(10000,25000),4)
            return army
        if level == 2:
            army = round(random.randint(50000,75000),4)
            return army
        if level == 3:
            army = round(random.randint(100000,200000),4)
            return army
        if level == 4:
            army = round(random.randint(200000,500000),4)
            return army
    if strength == 1:
        if level == 0:
            army = round(random.randint(10000,25000),4)
            return army
        if level == 1:
            army = round(random.randint(50000,100000),4)
            return army
        if level == 2:
            army = round(random.randint(100000,300000),4)
            return army
        if level == 3:
            army = round(random.randint(300000,800000),4)
            return army
        if level == 4:
            army = round(random.randint(800000,1100000),4)
            return army
    else:
        if level == 0:
            army = round(random.randint(5000,10000),4)
            return army
        if level == 1:
            army = round(random.randint(25000,50000),4)
            return army
        if level == 2:
            army = round(random.randint(75000,150000),4)
            return army
        if level == 3:
            army = round(random.randint(150000,250000),4)
            return army
        if level == 4:
            army = round(random.randint(300000,900000),4)
            return army

Here's one way to do it:

from random import randint

def n_gen_army(level, strength, weakness, params=dict_of_dicts):
    """Returns random floats according to ranges specified by params."""
    if strength:
        key = 'strength'
    if weakness:
        key = 'weakness'
    else:
        key = 'default'

    lower_bound, upper_bound = params[key][level]

    return round(randint(lower_bound, upper_bound), 4)

weakness_dict = {
                0: (1000, 5000),
                1: (10000, 50000),
                2: (50000, 75000),
                3: (100000, 200000),
                4: (200000, 500000),
                }

strength_dict = {
                0: (10000, 25000),
                1: (50000, 100000),
                2: (100000, 300000),
                3: (100000, 300000),
                4: (800000, 1100000),
                }

default_dict = {
               0: (5000, 10000),
               1: (25000, 50000),
               2: (75000, 150000),
               3: (150000, 250000),
               4: (300000, 900000),
               }

dict_of_dicts = {'weakness': weakness_dict,
                 'strength': strength_dict,
                 'default': default_dict
                }

>>> n_gen_army(2, 0, 0)
115061.0

Some other notes:

In your original code, if strength and weakness are both 1, then only the setting for weakness matters.  My code preserves that, but is that behavior really what is intended?  Or did you want elif instead of your second top-level if?
Why are you using round?  Do you really want floats returned instead of integers?  If you want integers, don't use round.  If you want integers rounded to the nearest 10 or 100, why not generate random integers in a 10-fold (or 100-fold) reduced range and then multiply be 10 (or 100)?
Avoid capitalization in function names if you can.  PEP8 and all that.  All-lowercase (for functions) is good python style.
You're only using one function from random, so from random import randint is better than loading in the whole module with import random.
You seem to be using weakness and strength as boolean variables so if weakness: is better than if weakness == 1.


Validating IBAN in Ruby Looking for tips on how to improve my code and areas that I can better follow Object Orientated principles. Is this class structure ok? How is the code overall?

The problem: There are two IBAN standards: IBAN-10 and IBAN-13. IBAN-10 is made up of 9 digits plus a check digit. IBAN-13 is made up of 12 digits plus a check digit. Spaces and hyphens may be included in a code but are not significant.

The check for IBAN-10 is calculated by multiplying each digit by its position, summing these products together and taking the modulus 11 of the result. Substituting X if the result is 10.

The check digit for IBAN-13 is calculated by multiplying each digit alternately by 1 or 3, summing these products together and taking the modulus 10 of the result and subtracting this value from 10. Then with this result take the modulus of ten so as to reduct the number to a single digit.

Return true if it is a valid IBAN-10/IBAN-13

class IBAN

  def initialize(number)
    puts "number: #{number}"

    @number = number
    @number.gsub!(/[^0-9]/i, '')
    return @number
  end

  def number
    @number
  end

  # # check IBAN length and if valid proceed to process IBAN type
  def check_number_format
    iban_length = self.number.length

    unless iban_length == 10 || iban_length == 13
      puts "Not a valid IBAN - incorrect length"
      return false
    else
      self.validate_iban_type
    end
  end

  def validate_iban_type
    case self.number.length
    when 10
      @iban_ten = IBANTEN.new(self.number)
      @iban_ten.process_ten_digit_iban
    when 13
      @iban_thirteen = IBANTHIRTEEN.new(self.number)
      @iban_thirteen.process_thirteen_digit_iban
    else
      return false
    end
  end
end

class IBANTEN

  def initialize(iban_number)
    @iban_number = iban_number
  end

  def iban_number
    @iban_number
  end

  def process_ten_digit_iban
    number_split = self.iban_number.split('')
    final_digit = number_split[-1]
    running_total = 0

    number_split[0, (number_split.length - 1)].each_with_index do |num, index|
      # multiply each digit by its position
      sum = num.to_i * (index.to_i + 1)
      running_total += sum.to_i
    end
    # get the modulus of the running total if it exuals 10 replace it with X otherwise return the result
    digit_sum_total = running_total % 11
    calculated_result = digit_sum_total == 10 ? "X" : digit_sum_total

    if calculated_result == final_digit.to_i
      puts "Valid IBAN-10 Number"
      return true
    else
      puts "Invalid IBAN-10 Number"
      return false
    end
  end

end

class IBANTHIRTEEN

  def initialize(iban_number)
    @iban_number = iban_number
  end

  def iban_number
    @iban_number
  end

  def process_thirteen_digit_iban
    number_split = self.iban_number.split('')
    final_digit = number_split[-1]
    running_total = 0

    number_split[0, (number_split.length - 1)].each_with_index do |num, index|
      # if the remainder equals zero multiply by 1 otherwise multiplu by 3
      sum = (index % 2 == 0 ) ? num.to_i * 1 : num.to_i * 3
      running_total += sum.to_i
    end
    # modulo 10 of the result and subtracting this value from 10, and then taking the modulo 10 of the result again to produce a single digit
    calculated_result = ( (10 - (running_total % 10) ) % 10)

    if calculated_result == final_digit.to_i
      puts "Valid IBAN-13 Number"
      return true
    else
      puts "Invalid IBAN-13 Number"
      return false
    end
  end
end

@iban_v13_valid = IBAN.new("978 0 471 48648 0")
@iban_v13_valid.check_number_format

@iban_v13_invalid = IBAN.new("9780470059021")
@iban_v13_valid.check_number_format

@iban_v10_valid = IBAN.new("0 471 60695 2")
@iban_v10_valid.check_number_format

@iban_v10_invalid = IBAN.new("0-470-84525-6")
@iban_v10_invalid.check_number_format

@iban_invalid_length = IBAN.new("0-470-84525-618423")
@iban_invalid_length.check_number_format

I would look to implement a structure with separate implementations for the three cases:

IBAN13
IBAN10
Neither of the above

You can provide that with something along these lines, in which IBAN#new is overridden to detect the number of integers provided, and to initialise the correct object to process it.

The real trick here is to provide the Invalid module, which responds to everything that you expect the Thirteen and Ten objects to respond to, in an implementation similar to a Nil Object pattern – in this case an Invalid Object.

I've omitted the implementation of the #valid? etc classes, but would point you to the Lisbn gem in which we have implemented an extremely fast algorithm https://github.com/ragalie/lisbn/blob/master/lib/lisbn/lisbn.rb#L111. Benchmarked against Enumerable-based methods, which often look quite elegant, this algorithm is incredibly fast – 40x faster than the same logic implemented in some ISBN gems.

Here's a code outline:

module IBAN
  def self.new(string)
    numeric = string.gsub(/[^0-9]/,"")
    case numeric.size
    when 13
      then Thirteen.new(string)
    when 10
      then Ten.new(string)
    else
      Invalid
    end
  end

  class Thirteen
    def initialize(number)
      @number = number
    end

    def checksum
      # TODO
    end

    def valid?
      # TODO
    end

    attr_reader :number
  end

  class Ten
    def initialize(number)
      @number = number
    end

    def checksum
      # TODO
    end

    def valid?
      # TODO
    end

    attr_reader :number
  end

  module Invalid
    def self.valid?
      false
    end

    def self.checksum
      nil
    end
  end
end

ps. You might prefer to implement Invalid as a singleton instead of as a module.

    ...
    else
      Invalid.instance
    end
    ...

  class Invalid
    include Singleton

    def valid?
      false
    end

    def checksum
      nil
    end
  end
end


Dynamic array of integers I implemented a vector class in C for integers. Any feedback welcome.

header:

#define DEFAULT_CAPACITY 1000

// Define a vector type
typedef struct
{
  int nrOfElements;     // Current number of elements.
  int capacity; // Possible capacity.
  int *data;    // Pointer to array elements.
} CVectorInt;

typedef enum {VECTOR_SUCCESS, VECTOR_OUTOFBOUNDS, VECTOR_MEMORYERROR} vector_error;

vector_error CVectorInit(CVectorInt *vector);
vector_error CVectorAppend(CVectorInt *vector, int value);
vector_error CVectorGet(CVectorInt *vector, int index, int * value);
vector_error CVectorSet(CVectorInt *vector, int index, int value);
void CVectorFree(CVectorInt *vector);

.c:

#include "CVectorInt.h"
#include <stdio.h>
#include <stdlib.h>

vector_error CVectorInit(CVectorInt *vector)
{
    int i = 0;

    vector->capacity = DEFAULT_CAPACITY;
    vector->nrOfElements = 0;
    vector->data = malloc(sizeof(int) * vector->capacity);
    if(vector->data == NULL)
        return VECTOR_MEMORYERROR;

    // Initialization in C never hurts.
    for(i = 0; i<DEFAULT_CAPACITY; i++)
        vector->data[i] = 0;

    return VECTOR_SUCCESS;
}

vector_error CVectorAppend(CVectorInt *vector, int value)
{
    int * ptr = NULL;
    int i = 0;

    // Resize vector if there is no space for more elements.
    if(vector->capacity == vector->nrOfElements)
    {
        vector->capacity *= 2;
        ptr = realloc(vector->data, vector->capacity * sizeof(int));
        if(ptr == NULL)
        {
            return VECTOR_MEMORYERROR;
        }
        else
        {
            vector->data = ptr;

            // Initialize new elements to 0.
            for(i = vector->nrOfElements; i<vector->capacity; i++)
                vector->data[i] = 0;
        }
    }

    vector->data[vector->nrOfElements++] = value;

    return VECTOR_SUCCESS;

}

vector_error CVectorGet(CVectorInt *vector, int index, int * value)
{
    if(index < vector->nrOfElements && index >= 0)
    {
        // Pass value at given index.
        *value = vector->data[index];
        return VECTOR_SUCCESS;
    }else
    {
        return VECTOR_OUTOFBOUNDS;
    }
}

vector_error CVectorSet(CVectorInt *vector, int index, int value)
{
    if(index >= vector->capacity)
        return VECTOR_OUTOFBOUNDS;

    vector->data[index] = value;

    return VECTOR_SUCCESS;

}

void CVectorFree(CVectorInt *vector)
{
  free(vector->data);
}

Usage:

#include <stdio.h>
#include <stdlib.h>
#include "CVectorInt.h"

int main(void)
{
    //error handling omitted for simplicity

    CVectorInt v;
    int i = 0, temp = 0;

    CVectorInit(&v);
    for(i = 0; i<100; i++)
    {
        CVectorAppend(&v, i*2);
    }

    for(i = 0; i<100; i++)
    {
        CVectorGet(&v, i, &temp);
        printf("%d ", temp);
    }

    CVectorFree(&v);

    return 0;
}

I think you should use uint for things where you cannot have negative values.

vector_error CVectorGet(CVectorInt *vector, int index, int * value)
{
    if(index < vector->nrOfElements && index >= 0)

This check, for instance, wouldn't need the comparison with 0 if you used uint.

In the comments I already said something about overflow. Now, you can indeed say "that's such a big amount of elements, people are not going to surpass this" and not build it in. That's okay. Be sure to add it in as a comment in the header though, because you don't want to accidentally find out that this is a problem.


QuickSort Algorithm (Python) This is my implementation of Quick Sort Algorithm :
def quick_sort(sequence):
    if len(sequence)<= 1:
        return sequence
    else:
        pivot = sequence.pop() # To take the last item of "sequence" list

    greater =[]
    lower = []

    for n in sequence:
        if n > pivot :
            greater.append(n)
        else:
            lower.append(n)

""" return the quiqk sorted "lower" and "greater" lists ,and pivot as a list in between """
    return quick_sort[lower] + [pivot] + quick_sort[greater]

How can I improve it ?

The main single advantage of quicksort is that it sorts in-place. Your implementation does not. It requires an additional memory for left and right lists, linear in terms of the initial array length.
Returning the concatenation of the three lists also doesn't help performance.
An obvious improvement is to reimplement it in-place.


Simple command line parsing I'm making a project that needs a simple cross-platform command-line parser. I made this. Hope you enjoy!
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <stdarg.h>

#define __NUM_OF_ELEMENTS(ARR) (sizeof(ARR)/sizeof(ARR[0]))
#define __HAS_ARG(FLAGS, ARG) (FLAGS & (1 << ARG))

void panic(const char *fmt, ...){
    fprintf(stderr, "error: ");

    va_list arglist;
    va_start( arglist, fmt );
    vfprintf(stderr, fmt, arglist);
    va_end( arglist );

    exit(EXIT_FAILURE);
}

void help(const char * progName){
    char * whoami = 0;
    (whoami = strrchr(progName, '/')) ? ++whoami : (whoami = progName);
    printf(

           "%s is intended to do blah\n"
           "Options:\n"
           " --help                Display This message\n"
           " --file [file name]    Input file to use\n",

    whoami);
    exit(EXIT_SUCCESS);
}

int main(int argc, char *argv[])
{
    if(argc == 1){
        help(argv[0]);
    }

    const char * args[] = {"--help", "--file", "--2cool4scool"};
    enum                  {HELP_ARG, FILE_ARG, TOO_COOL_FOR_SCHOOL_ARG};
    unsigned int flags = 0;

    //skip prog name
    for(unsigned int arg = 1, i = 0; arg < argc; ++arg){
        for(i = 0; i < (unsigned int)__NUM_OF_ELEMENTS(args); ++i){
            if(!strcmp(argv[arg], args[i])){
                flags |= 1 << i;
                goto CMD_ARGUMENT_FOUND;
            }
        }

        //argument is not found / unkown arg
        panic("Argument '%s' not found\n", argv[arg]);

        CMD_ARGUMENT_FOUND:;
    }

    if(__HAS_ARG(flags, HELP_ARG)){
        help(argv[0]);
    }
    if(__HAS_ARG(flags, TOO_COOL_FOR_SCHOOL_ARG)){
        puts("Hell YA!");
    }

    return 0;
}

Since this is a single-translation-unit program, mark your panic and help as static.
About this:
(whoami = strrchr(progName, '/')) ? ++whoami : (whoami = progName);

"Yikes". Generously we call this write-only code. Unroll it to
char *whoami = strrchr(progName, '/');
if (whoami)
    whoami++;
else
    whoami = progName;

In this loop, your i declaration should be moved down:
for(unsigned int arg = 1, i = 0; arg < argc; ++arg){
    for(i = 0; i < (unsigned int)__NUM_OF_ELEMENTS(args); ++i){

should be
for (unsigned int arg = 1; arg < argc; ++arg) {
    for (int i = 0; i < (unsigned int)__NUM_OF_ELEMENTS(args); ++i) {

to narrow its scope.
Don't use goto. Factor out a function where the double loop termination is done using a return.


Google maps API code I have inherited a site which uses the Google API to create a map on my webpage. I don't know much about JavaScript but from the research I've done, I dont think this code is optimal as it is not loading asynchronously. Any help or pointers would be appreciated.

function initialize() {
var myLatlng = new google.maps.LatLng(<?php echo get_post_meta($post->ID,'school_latitude',true); ?>, <?php echo get_post_meta($post->ID,'school_longitude',true); ?>);

var myOptions = {
  zoom: 11,
  center: myLatlng,
  mapTypeId: google.maps.MapTypeId.ROADMAP
 }

var map = new google.maps.Map(document.getElementById("map_canvas"), myOptions);

var marker = new google.maps.Marker({
  position: myLatlng,
  map: map,
  title: 'School'
});
}

function loadScript() {
var script = document.createElement("script");
script.type = "text/javascript";
script.src = "http://maps.google.com/maps/api/js?sensor=false&callback=initialize";
document.body.appendChild(script);
}

window.onload = loadScript;
</script>

var myLatlng = new google.maps.LatLng(<?php echo get_post_meta($post->ID,'school_latitude',true); ?>, <?php echo get_post_meta($post->ID,'school_longitude',true); ?>);

Instead of echoing directly onto the script, consider putting the values in an array, convert it to JSON using json_encode, and echo the JSON to a variable outside your script. Then use that variable like any other JS variable. This is possible because JSON is (most of the time) valid JS object syntax. Afaik, this is how Drupal passes down configs from server to the client.

<?php
  $configs = [
    'lat' => get_post_meta($post->ID,'school_latitude',true),
    'lng' => get_post_meta($post->ID,'school_longitude',true)
  ];
?>

var configs = <?php echo json_encode($configs); ?>; // var configs = { "lat": "VALUE", "lng": "VALUE" };

initialize(configs.lat, configs.lng);

The advantage of this approach is that you lessen the area where PHP is in touch with JS, reducing any breakage caused by some mishap in printing values.

function loadScript() {
var script = document.createElement("script");
script.type = "text/javascript";
script.src = "http://maps.google.com/maps/api/js?sensor=false&callback=initialize";
document.body.appendChild(script);
}

window.onload = loadScript;

I'm pretty sure you did it this way because you wanted it to load async. First off, onload fires after the entire page, images and all are loaded and is probably the very last event that fires in a page load cycle. If you have a page that's heavy with images, onload will take forever to fire. Consider listening to DOMContentLoaded instead.

Next is the entire procedure itself. If you take a look at the Google script, there are hints that it's already doing the same thing. So I believe this procedure isn't necessary. Using a script tag should suffice.

In the end, only the following should be necessary. Order of the two shouldn't matter since the maps script will probably call callback asynchronously. initialize would have been defined by then.

<script src="http://maps.google.com/maps/api/js?sensor=false&callback=initialize"></script>
<script>
  function initialize() {
    ...
  }
</script>


Parse JSON array into different Object using Reduce I have an script that is receiving some JSON and then I am parsing it out using reduce.  I am sure there is a more elegant way to do it.

JSON

 var request = {
    "operation":"rate_request",
    // JSON removed to make more readable.
    "freight":
    {
        "items":
        // here is the array I am targeting
        [{
            "item":"7205",
            "qty":10,
            "units":10,
            "weight":"19.0000",
            "paint_eligible":false
        },
        {
            "item":"7677",
            "qty":10,
            "units":4,
            "weight":"5.0000",
            "paint_eligible":false
        }],
    // more JSON removed
}

Here is function to parse out and then loop through:

function parseItems(json){
    var resultsReduce = new Object()
    var itemKeys = ['item','qty', 'units','weight', 'paint_eligable']
    itemKeys.forEach(function(item){
        if (item){
            resultsReduce[item] = json.freight.items.reduce(function(array, object) {
                   return array.concat(object[item])
            }, [])
        }
    })
    var itemXML = ''

    for(var i = 0; i < resultsReduce.item.length; i++){
        var weight = Number(resultsReduce['weight'][i])
        var qty = Number(resultsReduce['qty'][i])
        var units = Number(resultsReduce['units'][i])
        itemXML += '<LineItem>'
                + '<PieceCount>' + units * qty + '</PieceCount>'
                + '<NetWeight UOM="Lb">' + weight * qty + '</NetWeight>'
                +'<GrossWeight UOM="Lb">' + weight * qty + '</GrossWeight>'
                + '<FreightClassification>' + '50' + '</FreightClassification>'
                +'</LineItem>'
    }
    return itemXML
}

The parseItems function turn the JSON into an object with arrays:

  {
 "item": [7205,7677],
 "qty": [10,10],
 "units": [10,4],
 "weight": ["19.0000","5.0000"],
 "paint_eligible": [false, false]
 }

Then the function loops through the object (manually by targeting the keys) and loops through the loop of the array. Like I mentioned, I am sure it can be more elegant. Any thoughts on how you would make it cleaner?

This can be handled with a single Array.prototype.forEach loop. Consider the following:

function parseItems(json){
  var output = '';
  json.freight.items.forEach(function (item) {
    output += `
      <LineItem>
        <PieceCount>${item.units * item.qty}</PieceCount>
        <NetWeight UOM="Lb">${item.weight * item.qty}</NetWeight>
        <GrossWeight UOM="Lb">${item.weight * item.qty}</GrossWeight>
        <FreightClassification>50</FreightClassification>
      </LineItem>`;
  });

  return output;
}

No need to iterate over everything repeatedly for this.

Edit: Made a fiddle for you


Enum usage to restrict class attribute values I am using enums in python3 for the first time but unsure if I am using them correctly.

I want to restrict the 'allowed values' for a class attribute ArticleMetadata.article_type to those defined within an ArticleTypes Enum class.

Here's my code:

from enum import Enum

class ArticleMetadata(object):

    ArticleTypes = Enum('ArticleTypes', 'original modified', module=__name__)

    def is_in_enum(self, value, Enum):
        """Verify that a value is contained within a defined Enum object.

        Raises:
            ValueError: If the value is not contained within the defined Enum.
        """
        if value in Enum.__members__:
            return True
        else:
            raise ValueError("Value {0} not in Enum list.".format(value))

    def __init__(self, **kwargs):
        """Set class attributes."""

        self.article_type = (kwargs['article_type'] if 'article_type' in kwargs.keys()
                              and self.is_in_enum(kwargs['article_type'], self.ArticleTypes)
                              else None
                              )

Usage

> foo = ArticleMetadata(article_type='original')
> foo.article_type
'original'
> bar = ArticleMetadata(article_type='invalid')
*** ValueError: Value invalid not in Enum list.

This code works, but having to write an is_in_enum method seems clunky, and I would have thought that this functionality would be natively part of enums somehow??

Also, no errors are raised if I later run foo.article_type = 'invalid', which doesn't seem right!

Am I missing a common usage pattern?

You get no benefit for raising an error in is_in_enum. It's actually a hindrance.
I'd expect to use that function as followed:

if foo.is_in_enum(key):
    # do something
else:
    # do something

However, to correctly use your function I'd have to use:

try:
    foo.is_in_enum(key)
except ValueError:
    # do something
else:
    # do something

Instead just change it to return your if statement.
You can use **kwargs with keyword arguments. Use that instead.

And so I'd use:

from enum import Enum

class ArticleMetadata(object):
    ArticleTypes = Enum('ArticleTypes', 'original modified', module=__name__)

    def in_enum(self, value, Enum):
        """Verify that a value is contained within a defined Enum object."""
        return value in Enum.__members__

    def __init__(self, article_type=None, **kwargs):
        """Set class attributes."""
        if article_type is None or not self.in_enum(article_type, self.ArticleTypes):
            raise ValueError("Value {} not in Enum list.".format(article_type))
        self.article_type = article_type


C++ shared_singleton I actually feel bad posting "yet another singleton"... I wrote the following one many years ago and had recently found another application for it. We had many threads, each running the same function that requires the use of a boost::asio::io_service instance. It was best that all threads shared the same io_service instance and also that that instance be destroyed before main returned. That last requirement (whose validity I now question, but, whatever..) meant no global or static object.

Here's a function that returns an instance of an object wrapped in a shared_ptr. What's special about it though is that we keep an additional reference count so the singleton is eagerly destructed when the last shared_ptr is gone.

For example, if you call the function twice, you have two shared_ptrs, each with a ref-count of 1, but our singleton's ref-count is 2. On the other hand, if you call the function only once and copy the returned shared_ptr, the shared_ptr has a ref-count of 2 but our singleton's ref-count is at one. Either way, the singleton instance gets destructed when the last shared_ptr is destructed.

One disadvantage of this code is that the class is instantiated from its default constructor.

#include <memory>
#include <mutex>

// Deleter function given to the shared_ptr returned by get_shared_singleton.
template<typename T>
void release_shared(std::mutex& m, int& n, T*& p)
{
    std::lock_guard<std::mutex> lg(m);
    if(!--n)
    {
        delete p;
        p = 0;
    }
}

template<typename T>
std::shared_ptr<T> get_shared_singleton()
{
    static std::mutex m;

    std::lock_guard<std::mutex> lg(m);

    static int n = 0;   // Ref count.
    static T* p = 0;

    if(!p) p = new T();
    ++n;

    return std::shared_ptr<T>(p, std::bind(release_shared<T>, std::ref(m), std::ref(n), std::ref(p)));
}

The requirements, more clearly stated:

You wrote a function foo() that requires an instance of object X to perform its duty.
foo() can be invoked concurrently from different threads.
Concurrent foo()s must share the same instance of object X.
If no foo() is running, there must be no instance of object X alive.
foo() is in a library, you have no control of main() or the lifetime of threads that invoke foo().

What is the best code to provide a shared instance of an object that is both lazily-constructed and eagerly-destructed?

You can use a static weak_ptr in the function to cache the shared instance without extending its lifetime:

template<typename T>
std::shared_ptr<T> get_shared_instance()
{
    static std::mutex m;
    static std::weak_ptr<T> cache;

    std::lock_guard<std::mutex> lg(m);

    std::shared_ptr<T> shared = cache.lock();
    if (cache.expired()) {
        shared.reset(new T);
        cache = shared;
    }
    return shared;
}

So, the first caller will find a default-initialized weak_ptr counts as expired, and construct a new T.

While the first caller is active (and keeping its new T alive), concurrent callers will get the same object, cached in the weak_ptr.  When the last concurrent user is done, the object will be destroyed.

A subsequent caller will find the weak_ptr expired (same as before the first call), and create a new T ... etc. etc.


Enumerate k-combinations in Clojure (P26 from 99 problems) I've been playing with Clojure for the last few evenings, going through the well known 99 problems (I'm using a set adapted for Scala).

  Problem 26

  Given a set S and a no. of items K, returns all possible
  combinations of K items that can be taken from set S.

Here's my solution:

(defn combinations [k s]
  (cond
    (> k (count s)) nil        ;not enough items in sequence to form a valid combination
    (= k (count s)) [s]        ;only one combination available: all items
    (= 1 k) (map vector s)     ;every item (on its own) is a valid combination
    :else (reduce concat (map-indexed
            (fn [i x] (map #(cons x %) (combinations (dec k) (drop (inc i) s))))
            s))))

(combinations 3 ['a 'b 'c 'd 'f])

My basic solution is to take each item from the given sequence (map-indexed) and recurse to generate combinations of size K - 1 from the remaining sequence. The termination conditions are described above.

I'm still a complete Clojure newbie and would welcome comments on structure, efficiency, readability, resemblance to idiomatic Clojure, etc. Feel free to be brutal, but please remember I've been doing Clojure for only a few hours :)

I'm less interested in alternative mathematical methods for generating k-combinations, more interested in feedback on whether this is passable Clojure.

I'm currently reading "Joy of Clojure" so I'm (very) far from being "fluent" in Clojure but what I noticed is:

your solution is clever but quite complicated, you use "imperative" habits like indexed iteration
try to keep with simple abstractions like sequence first and rest and the solution will work with any Clojure collection - see example below
your solution use cond with three checks for k - consider using condp

Here's my code:

(defn subsets [n items]
(cond
    (= n 0) '(())
    (empty? items) '()
    :else (concat (map
                    #(cons (first items) %)
                    (subsets (dec n) (rest items)))
                  (subsets n (rest items)))))


Clone a linked list with a random pointer (part 2) This question is a follow-up question from here, where I implement the method from @vnp.

I have tested my code and it works, but the 3 steps processing seems a bit long to me. I also tried to write a few if/else, which seems a bit cluttering (e.g. code like newList.nextNode = newList.nextNode.nextNode if newList.nextNode else None). I'm wondering if there's any room to improve my code to make it even smaller and more elegant.

class LinkedListNode:
    def __init__(self, ID):
        self.ID = ID
        self.nextNode = None
        self.randomNode = None
    # return cloned new list
    def cloneList(self):
        # first step, add cloned node after each node
        node = self
        while node:
            newNode = LinkedListNode(node.ID)
            newNode.nextNode = node.nextNode
            node.nextNode = newNode
            node = node.nextNode.nextNode
        # 2nd step, setup random pointer
        node = self
        while node:
            node.nextNode.randomNode = node.randomNode.nextNode
            node = node.nextNode.nextNode

        # 3rd step, split existing and new list
        node = self
        newHead = node.nextNode
        newList = newHead
        while node:
            node.nextNode = node.nextNode.nextNode
            newList.nextNode = newList.nextNode.nextNode if newList.nextNode else None
            node = node.nextNode if node else None
            newList = newList.nextNode if newList else None

        return newHead

if __name__ == "__main__":
    node0 = LinkedListNode(0)
    node1 = LinkedListNode(1)
    node2 = LinkedListNode(2)
    node3 = LinkedListNode(3)
    node4 = LinkedListNode(4)
    node0.nextNode = node1
    node1.nextNode = node2
    node2.nextNode = node3
    node3.nextNode = node4
    node0.randomNode = node2
    node1.randomNode = node0
    node2.randomNode = node1
    node3.randomNode = node4
    node4.randomNode = node3

    newHead = node0.cloneList()

    while newHead:
        print (newHead.ID, newHead.nextNode.ID if newHead.nextNode else -1, newHead.randomNode.ID)
        newHead = newHead.nextNode

Cloning

I would expect from a cloning process to not modify the original instance, even if the modifications are transient and the original state is restored.
Your implementation adds nodes in the original list during the process.
After the clone is ready,
you clean up and restore the original list,
but this approach is surprising,
and a mistake could destroy the integrity of the original list.

I suggest to stick with more conventional approaches that produce a clone without modifying the original instance, for example by using an intermediary map to store the mapping of old nodes to new nodes, so that the random links can be correctly recreated.

Usability

How would you use this linked list in a program?
I see several usability issues:

The concept of a list and a node are not separated. This is confusing.
Common methods of a list are missing. How to add a node? The implementation doesn't offer an easy way to do that, users have to do that manually. They can only do that if they know the implementation, especially the linking between the nodes.

In other words, there's a total lack of encapsulation,
leading to serious usability consequences.

Conventions

Some Python conventions (PEP8) are violated, for example:

Function and variable names should use snake_case
There should be a blank line between functions of a class

There are others too. I suggest to go over PEP8 and follow the recommendations.


Calculating exponential moving averages with multiples threads I calculate the moving averages of a specific measure. However, multiples threads can access this class in order to compute the average.

public class AvgUtils {

    /** The alpha. */
    private static final double ALPHA = 0.9;

    private double avgOldValue = 1.0;

    public synchronized double computeAvg(double pValue) {
        if (avgOldValue == 0.0d) {
            avgOldValue = pValue;
            return pValue;
        }

        double lValue = avgOldValue + ALPHA * (pValue - avgOldValue);
        avgOldValue = lValue;
        return lValue;
    }
}

In the other class, I'm obliged to create a new instance of this class in other every time to do the calculations.

Can my class be refactored or redesigned in a better way ?

Javadoc, Javadoc, Javadoc

Your variable names are all sub-par. Why the unnecessary abbreviations (f.e. Avg)? Why the odd prefixes (f.e. lValue)?

Here is my simple and ultimate tip when it comes to names:

  Name things after what they are doing or what they contain.

And my ultimate rule:

  You're never allowed to use one letter variable names, with the only exception being dimenstions.

/** The alpha. */
private static final double ALPHA = 0.9;

That is a prime example of an unhelpful comment.

Overall, I don't see why your class is static. It seems like it will be used like an instance class but you don't want to pass the reference around, you simply created a singleton here.

Consider creating an instance class and then pass the reference/instance to the threads which share that instance.


HKDF and compare constant time in Rust I have implemented HKDF in Rust, which passes all test vectors from RFC 5869, using HKDF with HMAC-SHA256. I also have a function to compare byte arrays in constant time, which I hope someone could confirm is correctly done. Any type of feedback is welcome.

EDIT: I mainly mean feedback on hkdf_compute(). HMAC in hkdf_extract() is defined in another module and not included here.

HKDF

pub struct Hkdf {
  pub salt: Vec<u8>,
  pub data: Vec<u8>,
  pub info: Vec<u8>,
  pub hmac: usize,
  pub length: usize,
}

impl Drop for Hkdf {
  fn drop(&mut self) {
    //println!("DROPPING");
    self.salt.clear();
    self.data.clear();
    self.info.clear()
  }
}

impl Hkdf {
  /// Return HMAC matching argument passsed to Hkdf.
  pub fn hkdf_extract(&self, data: &[u8], salt: &[u8]) -> Vec<u8> {
    let hmac_res = Hmac {
        secret_key: salt.to_vec(),
        message: data.to_vec(),
        sha2: self.hmac
    };

    hmac_res.hmac_compute()
  }

  /// The HKDF Expand step. Returns an HKDF.
  pub fn hkdf_compute(&self) -> Vec<u8> {
    // Check that the selected key length is within the limit.
    if self.length as f32 > 255_f32 * (self.hmac / 8) as f32 {
        panic!("Derived key length above max. 255 * (HMAC OUTPUT LENGTH IN BYTES)");
    }

    let n_iter = (self.length as f32 / (self.hmac / 8) as f32).ceil() as usize;

    let mut con_step: Vec<u8> = vec![];
    let mut t_step: Vec<u8> = vec![];
    let mut hkdf_final: Vec<u8> = vec![];

    for x in 1..n_iter+1 {
            con_step.append(&mut t_step);
            con_step.extend_from_slice(&self.info);
            con_step.push(x as u8);
            t_step.extend_from_slice(&self.hkdf_extract(
                &con_step,
                &self.hkdf_extract(&self.data, &self.salt))
            );
            con_step.clear();

            hkdf_final.extend_from_slice(&t_step);
    }

    hkdf_final.truncate(self.length);

    hkdf_final
  }
}

Compare constant time:

#[inline(never)]
/// Comparison in constant time.
pub fn compare_ct(x: &[u8], y: &[u8]) -> bool {

  let length = x.len();

  if length != y.len() {
    false;
  }

  let mut result: u8 = 0;

  for n in 0..length {
    result |= x[n] ^ y[n];
  }

  result == 0
}

In general you should try and avoid floating point operations for any cryptographically specific code. Floating points have issues with regards to precision, and even I don't see any issue for this specific code, they always require special attention, e.g. during reviewing of the code.

Similarly, using recursive code is strongly discouraged for cryptographic algorithms. If your code can be coded using loops and only local variables you should really do so. I imagine that goes against the idea of Rust (and is thus up for debate), but crypto code generally requires special attention. If you keep to recursive calls make sure you document them as such, both inline and in the function description.

Although you nicely implemented the Drop destructor it makes more sense to create either a class with just a keyInputMaterial (now called data I presume) or simply keep all the data in parameters / local variables. The idea of just keeping the keyInputMaterial is that you may reuse the object to derive other keys using the function but with different salt / info. In that case you obviously need to retrain the Drop to clear the input key material.

Note that the internal key handling (padding, initial hashing) of HMAC is always the same. So a sneaky speed up is to perform pre-calculation on the salt which acts as the HMAC key (mainly useful if there is a lot of output expected, this speedup matters more for e.g. PBKDF2).

Your constant time function looks very much run-of-the-mill constant time, which in this case is great :) I don't think HKDF warrants a function where HMAC(k, x) and HMAC(k, y) are compared using a random key - but now you know about that neat little trick.

Note that you may want to make sure that you document that the comparison needs to be done in constant time or include it in a verify function (beware of truncated values - you did this in your code, but you need to consider it in the calling code as well!).

Note that I'm not a Rust expert, so I won't comment on the vector handling / slicing etc.


Optimization - Prime Factorizations of Numbers <= 10^7 I am using Trail Division Method with a pre-calculated list of primes to calculate the prime factorization of all numbers less than M (M <= 10^7).

I am using an array of vectors of pairs.  The format for 10 is as follows:

PF[10][0].first = 2 // Base
PF[10][0].second = 1 // Exp
PF[10][1].first = 5
PF[10][1].second = 1

My approach is working fine but it is too slow.  For M=10^7 it took 36.841 sec to compute PF of all numbers <=M on my system.

Questions

Which is the best approach for this question?
For my approach what other optimizations can i do?

My code

#include <iostream>
#include <fstream>
#include <vector>
#include <utility>
#include "math.h"
#include "stdio.h"
using namespace std;

const int lim=45000;
const int Max=10000000;
char prime[lim];

vector<pair <int,unsigned char> > PF[Max];

void prep()
{
    //Calculation of Prime Numbers
    for(int i=1;i<lim;prime[i++]=1);
    for(int i=2;i*i<lim;i++)
        if(prime[i])
            for(int j=i+i;j<lim;prime[j]=0,j+=i);

    for(int i=2;i<Max;i++) {
        int num=i;
        unsigned char pq=0;
        //Check for powers of 2
        while(num%2==0)  {
            pq++;
            num=num/2;
        }
        if(pq>0)
            PF[i].push_back( make_pair(2,pq) );
        int pan=num;
        //Loop for all primes j such that j*j<num
        for(int j=3;j*j<=num;j+=2) {
            if(prime[j]) {
                pq=0;
                while(num%j==0) {
                    pq++;
                    num=num/j;
                }
                if(pq>0)
                    PF[i].push_back( make_pair(j,pq) );
            }
        }
        if(num>1)
            PF[i].push_back( make_pair(num,1) );
    }
}

main()
{
    prep();
}

Timing

My Code

real    0m36.841s
user    0m36.624s
sys     0m0.265s

Igor ostrovsky Code

real    0m41.628s
user    0m41.390s
sys     0m0.265s

I did this a while ago, in the interest of reducing ratios of factorials

#include <utility>
#include <vector>

std::vector< std::pair<int, int> > factor_table;
void fill_sieve( int n )
{
    factor_table.resize(n+1);
    for( int i = 1; i <= n; ++i ) {
        if (i & 1)
            factor_table[i] = std::pair<int, int>(i, 1);
        else
            factor_table[i] = std::pair<int, int>(2, i>>1);
    }
    for( int j = 3, j2 = 9; j2 <= n; ) {
        if (factor_table[j].second == 1) {
            int i = j;
            int ij = j2;
            while (ij <= n) {
                factor_table[ij] = std::pair<int, int>(j, i);
                ++i;
                ij += j;
            }
        }
        j2 += (j + 1) << 2;
        j += 2;
    }
}

std::vector<int> factor( int num )
{
    std::vector<int> factors;
    factors.reserve(30);
    do {
        factors.push_back(factor_table[num].first);
        num = factor_table[num].second;
    } while (num != 1);
    return factors;
}

I believe it will be much faster than yours, because I was able to avoid ever using division (/ and %).  As a matter of fact, I don't use any multiplies either.

(It's about 6x faster with g++ than the code in the question, and the actual factor generation part is marginally faster, about 10%, than lol4t0's.  Returning factors in a std::vector<int> takes 5x as long as the factor generation.)

Test program: http://ideone.com/pt9nu

Original application:

http://ideone.com/Weeg6


Does PDO::quote helps me to protect me against sql injection? I have to process raw sql user input with Laravel. I've backed them up as follows:
$query = DB::table('table_name');

function escapeString($str) {
    return DB::connection()->getPdo()->quote($str);
}

$column = escapeString($rule['query']['operand']);
$value = escapeString($rule['query']['value']);
if(in_array($rule['query']['operator'], $query->operators)) {
    return $column . ' ' . $rule['query']['operator'] . ' ' . $value;
}

Is that enough, or can I still be attacked over it?
I read:

https://stackoverflow.com/questions/18951057/escape-raw-sql-queries-in-laravel-4 - recommend it
https://www.php.net/manual/de/pdo.quote.php - they do not recommend it, but it seems possible

(This question was postet originaly at https://stackoverflow.com/questions/63091979/is-my-code-protected-against-sql-injection, but STA suggest to post this question here again)
Update:
I figured out, how to use value in variable binding. Also I changed escapeString to
$column = preg_replace('/[^a-zA-Z_]/', '', $rule['query']['operand']);

Thats fine for alle columns names and I am pretty sure that this is safe. This filtering approch ist also used in https://stackoverflow.com/questions/10080850/using-a-whitelist-on-user-input

The PDO::quote() function is not correct for making column names safe. It's basically a wrapper for the QUOTE() function in MySQL:

The string is returned enclosed by single quotation marks and with each instance of backslash (), single quote ('), ASCII NUL, and Control+Z preceded by a backslash. If the argument is NULL, the return value is the word “NULL” without enclosing single quotation marks.

The enclosing single-quote marks make the result usable as a string value in SQL, but you are using it for a column name. So you would end up with:
'mycolumn' = 'myvalue'

The single quotes make it not act like a column in your expression. The QUOTE() function is for string or date values, not identifiers like column names or table names.
You posted an update, where you changed to using a bound query parameter for the value, and you used a regular expression replacement function to strip out any non-word characters from the column name.
But this still risks the column being a reserved SQL keyword, which will cause errors in your code:
SELECT = 'myvalue'

To avoid this, the column name should be enclosed in backticks:
`SELECT` = 'myvalue'

If you enclose the column in backticks, then the column may be a reserved keyword, or it can even have punctuation, whitespace, international characters. Those are legitimate for SQL identifiers, but your escaping function would not allow it.
You should make sure if the column name contains a literal back-tick, you double it, which will make it act like a literal back-tick in your SQL.
function quoteIdentifier($str) {
    return '`' . preg_replace('/`/', '``', $str) . '`';
}


Print a pattern of text based on a line from an Excel file I started to learn Python not long ago to automate certain tasks I work on. I want to optimize a script I wrote that takes a line in an Excel file and prints a pattern of text based on this line's content. Here's an example of output from my code:

mgmt_cli add access-rule layer "network" position bottom name "thirdline" source.1 "Value1front" source.2 "Value2front" source.3 "x3front" destination "Dest1front" service "HTTP" track.type "Log" action "Accept"

How would you optimize this code?

Link to the example file (just click Valider et Télécharger le Fichier) http://dl.free.fr/hFlGmywIN
Link to full code : https://pastebin.com/CR5SXHRQ

import xlrd
import re
loc = ("Dir\StackoverflowExample.xlsx")

wb = xlrd.open_workbook(loc)
sheet = wb.sheet_by_index(0)
sheet.cell_value(0, 0)

# Extracting number of rows

def CaseMulti(CellContent, CellName):
 #takes a cell and a string as parameters, prints a pattern like Cell.X ="string", X starts with 0 and ends at the number of line in the Cell
 case = str(CellContent)
 x= re.split ("\s", case)   #splits the cell contents in a list using regex where space is the separator for the list's elements
 if name == CellContent:  # deals with the cell with spaces between objects no clue how it work but it does
  return "name " + '"{}"'.format(CellContent)
 else:
  s=0
  string =""
  anycells = ["any","Any", "ANY"]  # tests if the soure, destination or service has any ==> returns nothing
  if str(x[0]) in anycells :
   return " "
  else:
   if len(x)==1:    # case where te cell has one line (or a word with no spaces)
    string= CellName + " " +'"{}"'.format(case)

   else:            # case where multilines/mutli spaces
    while s<len (x) :
     listelement = x[s]
     string = string + CellName+'.'+str(s+1)+ ' '  +'"{}"'.format(listelement)
     s=s+1

   return string

for numberofrows in range (5):

 print "Rule " + str (numberofrows)
 name= sheet.cell_value(numberofrows,1)
 source = sheet.cell_value(numberofrows,2)
 destination = sheet.cell_value(numberofrows,3)
 protocole = sheet.cell_value(numberofrows,4)
 log = sheet.cell_value(numberofrows,5)
 action = sheet.cell_value(numberofrows,6)
 #firewall = sheet.cell_value(numberofrows,7)

 try:
  print ("mgmt_cli add access-rule layer \"network\" position bottom " + str(CaseMulti(name,"name")) +  str(CaseMulti(source, " source")) + str(CaseMulti(destination, " destination")) +  str(CaseMulti(protocole, " service")) +   ' track.type "Log" ' + str(CaseMulti(action, " action")) )
 except :
  numberofrows +=1

STYLING
According to PEP-8, there are a few improvements to be made to your code:

Imports: Your imports should go in alphabetical order
Variable/Function Names: You should use snake_case instead of camelCase
Parameter Names: This may just be how I view your code, but how your parameters are named look like your taking in objects. You should use lowercase variables for better readability.
Spacing: You should use 4 spaces for each indent. For variable declaration and assignment, the = should be separated by a space on each side for readability, like so: s=0 to s = 0.
Main Guard: You should wrap any regular code in a if 'name == 'main' guard, to make sure that the code is only being run when that file is the main file, and not being imported by anything.

STRING FORMATTING
All throughout your code, you put strings together using + and .format. Luckily there is an easier way, by putting an f before the string like so: f"content here". This erases the need to have verbose + in your strings, and you no longer have to cast int to str with this method, you can just write the code in the brackets {your code here}.
Example:
return "name " + '"{}"'.format(cell_content)
to
return f"name \"{cell_content}\""
Here is the updated code:
import re
import xlrd

loc = ("Dir\StackoverflowExample.xlsx")

wb = xlrd.open_workbook(loc)
sheet = wb.sheet_by_index(0)
sheet.cell_value(0, 0)

# Extracting number of rows

def case_multi(cell_content, cell_name):
    #takes a cell and a string as parameters, prints a pattern like Cell.X ="string", X starts with 0 and ends at the number of line in the Cell
    case = str(cell_content)
    x = re.split ("\s", case)   #splits the cell contents in a list using regex where space is the separator for the list's elements
    if name == cell_content:  # deals with the cell with spaces between objects no clue how it work but it does
        return f"name \"{cell_content}\""
    else:
        s = 0
        string = ""
    if str(x[0]).lower() == "any":
        return " "
    else:
        if len(x) == 1: # case where the cell has one line (or a word with no spaces)
            string = f"{cell_name} \"{case}\""
        else: # case where multilines/mutli spaces
            while s < len(x):
                listelement = x[s]
                string += f"{cell_name}.{str(s + 1)} \"{listelement}\""
                s += 1

        return string

def main():

    for number_of_rows in range(5):

        print "Rule " + str(number_of_rows)
        name= sheet.cell_value(number_of_rows, 1)
        source = sheet.cell_value(number_of_rows, 2)
        destination = sheet.cell_value(number_of_rows, 3)
        protocole = sheet.cell_value(number_of_rows, 4)
        log = sheet.cell_value(number_of_rows, 5)
        action = sheet.cell_value(number_of_rows, 6)
        #firewall = sheet.cell_value(number_of_rows, 7)

        try:
            print ("mgmt_cli add access-rule layer \"network\" position bottom " + str(case_multi(name,"name")) +  str(case_multi(source, " source")) + str(case_multi(destination, " destination")) +  str(case_multi(protocole, " service")) +   ' track.type "Log" ' + str(case_multi(action, " action")) )
        except:
            number_of_rows += 1

if __name__ == '__main__':
    main()


Script to pick random answers for a vocabulary quiz I am making a small script, which, when run in the console on Vocabulary.com, picks one answer (the third or fourth) and clicks. Then the code checks if the answer was wrong or right and goes to the next question. Because the animations between the time the question is answered and the appearance of a "next question" button is very long, I had to use a weird mix of functions and setTimeout(). Although the code works, I feel like it is messy and maybe won't always work. How could I make this code perform better and make sure it is easy to edit?

To run the script, go on https://www.vocabulary.com and run this code in the console.

function clickNextButton() {
    for (v = 0; v < 4; v++) {
        document.getElementById("questionContainer").getElementsByTagName("a")[v].click();
    }

    function finalClick() {
        var foo = document.getElementById("blurbPane").getElementsByTagName("button")[0];
        foo.click();
    }

    setTimeout(finalClick, 1000);

}

var myElem = document.getElementsByClassName('source')[0];
if (myElem == null){
    console.log("no source")
} else{
    myElem.parentNode.removeChild(myElem);
}

var mathRandom = Math.floor(Math.random() * 2 + 2);
var selectChoices = document.getElementById("questionContainer").getElementsByClassName("choices")[0].getElementsByTagName("a")[mathRandom];
selectChoices.click();
setTimeout(partA, 1000);

function partA() {
    var ifMessage = document.getElementsByClassName("messagePane")[0].getElementsByTagName("h3")[0];
    var myColor = window.getComputedStyle(ifMessage).getPropertyValue("color");
    if (myColor == "rgb(204, 0, 0)") {
        clickNextButton();
    } else if (myColor == "rgb(83, 144, 7)") {
        document.getElementsByClassName("nextQuestion")[0].click();
    }
}

You can skip animations from elements using JQuery stop. This method is appled to a Jquery selector, and it will stop the animation running on it.

So using it, you can force to finish all the animations related to the question container after firing the click


Generating Unique Colors I just wrote a small class that generates perfectly unique colors(or that is the plan anyway). I found the original index shifting snippet online, and was just wondering if it could be improved in anyway, or if any problems could arise from using it.

Here is the class which is used to create the unique colors.

public class UniqueColorGenerator
    {
        private int _colorIndex;
        public UniqueColorGenerator()
        {

        }

        public Color Next()
        {
            _colorIndex++;
            byte red = ( byte )( _colorIndex & 0x000000FF );
            byte green = ( byte )( ( _colorIndex & 0x0000FF00 ) >> 08 );
            byte blue = ( byte )( ( _colorIndex & 0x00FF0000 ) >> 16 );
            return Color.FromArgb( red , green , blue );
        }
    }

I am using this for "geometry picking" in OpenGL. It works beautifully, too. Here is a picture of 16,384 quads with their own unique color... or I guess I should say "unique shade of red and black".

When I first ran it, I could swear that there were obvious duplicates, but there aren't. Not only did the picking work flawlessly, but I also ran through the Colors and didn't get a single collision. Though I am still wondering if any problems could arise from how the generator is currently built—besides hitting integer max, which will also never happen.

Also, how can I randomize the colors more, so instead of getting different shades of red, I could receive more colors along the spectrum?

You don't need the constructor, as it's empty.

You're taking red as the low bits and blue as the high bits, which is backwards.

You appear to be using System.Drawing and not System.Windows.Media, judging by the overloads. If so, there is an overload that takes an int:

public class UniqueColorGenerator
{
    private int _colorIndex;

    public Color Next()
    {
        return Color.FromArgb(++_colorIndex);
    }
}

You also wouldn't need to use &, as casting to a byte will cut off the upper bits.

return Color.FromArgb(
    (byte)(_colorIndex >> 24),
    (byte)(_colorIndex >> 16),
    (byte)(_colorIndex >> 8),
    (byte)_colorIndex);


NumPy array as quasi-hash table Motivation: I have a large number of large read-only dicts with large string keys mapped to tuples of two floats. They are taking up a large amount of heap space.

Proposed solution: a 3xn NumPy array, with the first column being the hash value of the key, and sorted by that column. Only contains and getitem ops are required for my needs.

I realize that this solution is \$O(log(n))\$ rather than \$O(c)\$ but I assume that this isn't that big a problem for my application as there aren't that many lookups per request and my primary problem is memory constraints.

I'm not sure how well searchsorted performs here though, or if I should try to change the stride somehow so that the first column is contiguous or something like that. I should also probably be using a record array.

import numpy

class FCD(object):

  def __init__(self, d):
    data = []
    for key, value in d.iteritems():
      data.append((hash(key), value[0], value[1]))
    data.sort()
    self.data = numpy.array(data)

  def __getitem__(self, key):
    hkey = hash(key)
    ix = numpy.searchsorted(self.data[:, 0], hkey)
    if ix < len(self.data) and self.data[ix][0] == hkey:
      return self.data[ix][1:]
    else:
      raise KeyError

  def __contains__(self, key):
    hkey = hash(key)
    ix = numpy.searchsorted(self.data[:, 0], hkey)
    return ix < len(self.data) and self.data[ix][0] == hkey

I think this looks pretty efficient.  Let me add a few more minor, detailed comments (more than 1 year after the question was asked; I hope they are still useful):

After you convert to a NumPy array, the keys are longer guaranteed to be unique (if someone re-uses your code in a different context, etc).  This will cause searchsorted to fail.  You might want to add a check such as:

assert len(self.data[:, 0]) == len(np.unique(self.data[:, 0])

to make sure users and readers of your code know that the first column of your array must be unique.
I agree that record arrays (aka structured arrays) would be better.  Your code would be more readable because instead of things like:

self.data[ix][1:]

you could write:

self.data[ix]['Float_1'], self.data[ix]['Float_2']

(but please use more descriptive names for your context than 'Float_1 etc.!)

Also, and more importantly: without using a structured array, your hash keys will be stored as floats instead of ints.  That will take extra memory, and make future calls to searchsorted and __eq slightly less efficient.
Your __contains__ method seems overly stringent to me, which would result in it being slower.  Do you need to call searcshorted in __contains__ at all?  Wouldn't something like this suffice and be more efficient?

def __contains__(self, key):
    hkey = hash(key)
    return np.in1d(self.data[:, 0], hkey).any()

You are duplicating the __contains__ check in your __getitem__ code.  I'd just say it this way:

if __contains__(self, key): return self.data[ix][1:]

Very minor, but the idiom is always import numpy as np.  It's probably best to use that so you can do np.searchsorted etc. instead of typing numpy.searchsorted as that's what most NumPy users do.


Python numerical integration Could the time complexity of this definite integral algorithm be improved?

import numpy as np

def integral(f, left, right, epsilon):
    w = int((right-left)/epsilon)
    domain = np.linspace(left, right, w)

    heights = [f(((a+b)*0.5)) for a, b in zip(domain[:-1], domain[1:])]

    return sum(heights)*epsilon

# Test an example
print('Result:', integral(lambda x: np.sin(x), 1, 10, 0.01))

By changing the start and end points to start+epsilon/2 and end-epsilon/2, we can simplify the calculation tremendously. This means that the domain is what we want to evaluate the function at, removing the zip and averaging logic, secondly, because we now have heights of the same length as domain, we can just call f(domain), to have it update in place. Finally, we can use np.sum to return the answer. Put together, these changes result in simpler code, and roughly 100x faster (especially for small epsilon)

def integral1(f, left, right, epsilon):
    left += epsilon / 2
    right -= epsilon / 2
    domain = np.linspace(left, right, (right-left) // epsilon)
    return np.sum(f(domain), dtype = np.float128) * epsilon


Decompose a value as a sum of square numbers My daughter had a question on her maths homework which was to write a value as a sum of 4 or fewer square numbers.

For example, x = 73 could be 36 + 36 + 1.

I came up with a really brute force algorithm that shows all combinations but when the values get above 1000 it becomes quite slow.

Is there a clever way of achieving this?
Here is my algo in F#

let squares = [ 0.. 100 ] |> Seq.map (fun x -> x * x) |> Seq.toList

let rec calc total attempts squares accu results =
  match (total, attempts, squares) with
  | (0, _, _) -> accu :: results
  | (_, 0, _) -> [] :: results
  | (x, _, _) when x < 0 -> [] :: results
  | (_, _, []) -> [] :: results
  | total, attempts, squares ->
    let filteredSquares = squares |> Seq.filter ((>=) total )
    filteredSquares
      |> Seq.collect(fun sq ->
                          calc (total - sq)
                               (attempts - 1)
                               (filteredSquares |> Seq.toList)
                               (sq :: accu)
                               results) |> Seq.toList

let res =
  calc 8058 4 squares [] []
  |> Seq.filter(fun lst -> lst <> [])
  |> Seq.sortBy (fun lst -> lst.Length)
  |> Seq.take 1
  |> Seq.toList

Additional comments on better F# code would be appreciated as well.
One thing I was wondering was whether I could make it lazy using sequences.

let squares = [ 0.. 100 ] |> Seq.map (fun x -> x * x) |> Seq.toList

I don't think limiting the squares this way is a good idea. Instead, if you moved it inside calc, you could write it as:

let squares =
    Seq.initInfinite id
    |> Seq.map (fun x -> x * x)
    |> Seq.takeWhile (fun x -> x <= total)
    |> Seq.toList

calc is a pretty bad name: it doesn't say anything about what the function calculates.

If you limit yourself to returning always only the shortest sum, the signature of the method could be simplified quite a lot: you could get rid of attempts, accu and results. This would also somewhat simplify the implementation.

And even if you don't do that, I think that filtering out empty lists should be done inside calc, not outside.

If you simplify the signature, you could then apply memoization to make the computation much faster. Memoization will help here, because what's slowing down the computation is calculating the result for the same total over and over.

The implementation could look like this:

let rec calculateSquaresSum =
    let calculateSquaresSum' total =
        if total = 0 then
            []
        else
            let squares =
                Seq.initInfinite (fun x -> x + 1)
                |> Seq.map (fun x -> x * x)
                |> Seq.takeWhile (fun x -> x <= total)
                |> Seq.toList
                |> List.rev

            squares
            |> Seq.map (fun sq -> sq :: (calculateSquaresSum (total - sq)))
            |> Seq.minBy (fun sum -> sum.Length)

    memoize calculateSquaresSum'

This will calculate the result for values around 10.000 immediately, but 100.000 takes a few seconds, so there likely is a more efficient way to implement this.

Though if you're okay with finding just some decomposition with at most the given length, not necessarily the shortest one, having the attempts parameter makes sense and makes the computation much faster when the decomposition is possible. (E.g. calculateSquaresSum(1000000 - 1, 4) returns immediately with [998001; 1849; 100; 49], while calculateSquaresSum(100000 - 1, 3) takes few seconds to return with a negative answer.)

The code:

let rec calculateSquaresSum =
    let calculateSquaresSum' (total, attempts) =
        if total = 0 then
            Some []
        elif attempts = 0 then
            None
        else
            let squares =
                Seq.initInfinite (fun x -> x + 1)
                |> Seq.map (fun x -> x * x)
                |> Seq.takeWhile (fun x -> x <= total)
                |> Seq.toList
                |> List.rev

            squares
            |> Seq.map (fun sq ->
                match calculateSquaresSum (total - sq, attempts - 1) with
                | None -> None
                | Some tail -> Some (sq::tail))
            |> Seq.tryPick id

    memoize calculateSquaresSum'


Filtering queries based on the current user state I have a Symfony2 project. I have an Entity Asset which can have relations with Category. I store a categoryCount-field within the Asset to determine if an asset has one, to select those fast via a DQL-query depending on the current user state.

Now found myself writing something along the lines of this in my AssetRepository:

/**
 * @param string $orderBy
 * @param int $mode
 *
 * @return \Doctrine\ORM\Query
 */
public function getQueryForAll($orderBy = 'DESC', $mode = self::FILTER_WITHOUT_QUARANTINED)
{
    $qb = $this->createQueryBuilder('asset');
    $qb->orderBy('asset.updatedAt', $orderBy);

    $filter = $this->getQuarantinedCriteria($mode);

    if ($filter) {
        $qb->addCriteria($filter);
    }

    return $query = $qb->getQuery();
}

/**
 * @param int $mode
 *
 * @return Criteria|null
 */

protected function getQuarantinedCriteria($mode = self::FILTER_WITHOUT_QUARANTINED) {
    $filter = null;

    if ($mode === self::FILTER_WITHOUT_QUARANTINED) {
        $filter = Criteria::create()
            ->andWhere(Criteria::expr()->gt('categoryCount', 0));
    } else if ($mode === self::FILTER_ONLY_QURANTINED) {
        $filter = Criteria::create()
            ->andWhere(Criteria::expr()->eq('categoryCount', 0));
    }

    return $filter;
}

This doesn't feel right to me. The $mode flag triggers my code smell alarm, I also am worrying about passing it into each asset querying method.

Furthermore, since I will have to set the mode depending on the state of the current logged in user, I also am worried that I will produce code duplications by doing a lot of if-user-has-the-rights-checks in my actions or services:

/**
 * @Route("/all_assets/{page}",
 *         name="get_paginated_assets",
 *         requirements={"page" = "\d+"},
 *         defaults={"page" = "1"},
 *         options={"expose"=true}
 * )
 */

public function getPaginatedAssetActions($page)
{
    $repo = $this->getAssetRepository();

    $isAllowedToViewRestrictedAssets =  $this->getCurrentUser()->isAdmin();

    if ($isAllowedToViewRestrictedAssets) {
        $mode = Entity\AssetRepository::FILTER_SHOW_ALL;
    }

    $query = $repo->getQueryForAll($mode);

    $pagerfanta = new Pagerfanta(new DoctrineORMAdapter($query));
    $data = $this->getCurrentPagedResults($pagerfanta, $page);

    $view = $this->view();
    $view->setFormat('json');
    $view->setData($data);

    return $view;
}

Is this the right way to implement the logic to enable filters in a Repository? Feedback on whether there is a more decoupled way would be highly appreciated.

You can try to use SQLFilter for this purpose. It allows you to tune SQL query based on some logic. You can pass information about your user's state into filter through DI container and use this information to tune your SQL query. Please note that unlike repositories - SQLFilter works on SQL level, not on DQL. More information can be found into Doctrine's documentation.


Optimizing a Makefile It's been pointed out (see the first comment) that my makefile rebuilds all source files regardless of changes made.

# Variables #

TARGETS := libAurora.a libAurora.so
    # The names of targets that can be built.
    # This is used in the list of valid targets when no target is specified, and when building all targets.

TARGET_DIRECTORY := ./Binaries
    # The place to put the finished binaries.

CXX := g++
    # The compiler to use to compile the source code into object files.

CXXFLAGS := -I. -Wall -Wextra -fPIC -g -O4
    # Options passed to the compiler when compiling source files into object files.

AR := ar
    # The archiver to use to consolidate the object files into one library.

ARFLAGS := -rcs
    # Options to be passed to the archiver.

SOURCE_FILES := $(shell find Source -type f -name *.cpp)
    # Because it's inconvenient to maintain an up-to-date list of all the source files
    # that should be compiled, we just search for all .cpp files in the project

OBJECT_FILES := $(SOURCE_FILES:.cpp=.o)

.PHONY: Default
    # The default target, which doesn't actually do anything except give the user instructions,
    # can be called even if nothing needs to be updated.

.INTERMEDIATE: %.o
    # Specifying the object files as intermediates deletes them automatically after the build process.

# Rules #

Default:
    @echo "Please specify a target, or use \"All\" to build all targets.  Valid targets:"
    @echo "$(TARGETS)"

All: $(TARGETS)

lib%.a: $(OBJECT_FILES)
    $(AR) $(ARFLAGS) $(TARGET_DIRECTORY)/$@ $(OBJECT_FILES)

lib%.so: $(OBJECT_FILES)
    $(AR) $(ARFLAGS) $(TARGET_DIRECTORY)/$@ $(OBJECT_FILES)

What can I do to improve it?

I think the suggestion in the link you mention is to not bother with making your object files "intermediate".  I tend to agree.

If make deletes the object files after the link step then it has to re-build them all at the next invocation of make.  Without this, the next time you make, it can look at the dependencies of the object files (as specified by the makefile, the part after the :) and only rebuild them if the stated dependencies have a later timestamp.

You might also want a make clean target to remove the objects via rm, so that you can delete them when you really want to.  But the typical workflow for using make is to keep your object files around for extended periods and type make when you want to build what you've changed.


Connect and login to MySQL database I am new to PHP so this is largely compiled from examples around the web. It seems to work fine. Are there any ways in which it can be improved upon? Is it secure?

<?php
    mysql_connect("localhost", "myusername", "mypassword");
    mysql_select_db("companyDB");

    function user_login($username, $password)

{
    ob_start();

    $username = mysql_real_escape_string($username);
    $password = md5($password);

    $sql = "select * from users where username='$username' and password ='$password' LIMIT 1";
    $result = mysql_query($sql) or die('Please change your mysql query !');

    if(mysql_num_rows($result) > 0)
    {
        $_SESSION['username'] = $username;
        $_SESSION['loggedin'] = true;

        $row = mysql_fetch_row($result);
        $access_level = $row[4];
        $_SESSION['accessLevel'] = $access_level;

        while (ob_get_status())
        {
            ob_end_clean();
        }

        header("Location: /index.php");

    }
    else
    {

    }
}
?>

Well, mysql() is being deprecated, so be aware of that. mysqli() and pdo() are the new standards. I'd say continue learning on the old one, everything you learn can be translated to the new.

When connecting to a database, it is best to give that connection a name, such as $conn so that it can be referenced later. This is so that you can specify which connection you wish to use when selecting databases, or if you need to check if the connection is successful. Same for selecting the database. So...

$conn = mysql_connect("localhost", "myusername", "mypassword");
if( ! $conn ) {
    //handle error and exit script...
}

$companyDB = mysql_select_db("companyDB", $conn);
if( ! $companyDB ) {
    //handle error and exit script...
}

You've started output buffering (ob_start()), but not done anything with it except clear it. Usually you'll want to examine the output for errors, or dump it directly onto the page before clearing it. But for a "first" script this is fine. Just be aware that there is more to it than just starting and clearing.

Try to avoid or die() short circuiting. It works but was not intended for this purpose. die() shouldn't even be in live environments, its meant as a debugging tool. Instead you should handle any errors more elegantly, such as displaying what happened, logging errors, etc...

Be careful of magic numbers. You've told us that $row[4] is the access level, but why? What if it changes? This should probably be grabbed with a more specific SQL statement. I'm not much of  SQL guy, so I can't help you out here, but I imagine there should be a better way.

Location headers should be terminated with exit afterwards to prevent the rest of the script from running.

If you don't have anything to put in an else block, then there is no need to declare one.

You also need to close connections to your mysql using mysql_close().


Javascript 5 Module Pattern internal name I'm learning about the Javascript module pattern from an array of online sources. The following suits my basic needs at the moment:

var module = function(){

    // Create the return module and its configuration
    var module = {},
    config = {
        css:{
            classes:{
                heading:'h1',
                navbar:'nav-bar'
            },
            ids:{
                widget:'widget',
                block:'block'
            }
        },
        userName:'user',
        token:'UUID'
    };

    // Private methods
    function _moduleMethod() {
        console.log(config.userName);
    }

    // Add functionality to the module's init()-ialising method
    function init() {
        _moduleMethod();
    }

    // Add init, and any other methods, to obj
    module["init"] = init;

    // Return a the module as a Public API
    return module;

}();

// When the DOM is ready, run the module
document.addEventListener("DOMContentLoaded", function() {
    module.init();
});

My question is regarding the naming of the internal return object the same as the external variable "module". My thinking is that it's OK given closure and scoping but most of the examples I've seen name the internal object something like "obj" or "my". Am I going to run into issues down the line, say with the Augmenting module pattern?

Because of the scoping inside a closure you will not run into a problem.

The Revealing Module Pattern

I want to introduce this type of module pattern because it does not need the helper object module. Instead of collecting all public functions inside an object called module return directly an object with all public functions. This will reduce code and result in a better to reading code.

var module = function () {
    var config = {
        css: {
            classes: {
                heading: 'h1',
                navbar: 'nav-bar'
            },
            ids: {
                widget: 'widget',
                block: 'block'
            }
        },
        userName: 'user',
        token: 'UUID'
    };

    function _moduleMethod() {
        console.log(config.userName);
    }

    function init() {
        _moduleMethod();
    }

    // Return an object as public API
    return {
        init: init
    }
}();


Counting multiples of 3 or 5 using threads I have a modified version of the first Project Euler problem — where I find the sum of all the numbers below the value 2100000000 that are multiples of 3 or 5 using a multi-threaded program.

The concept of the partially-multi-threaded version: we have a value sum, where we add together all the multiples of 3 (that are not multiples of 3 and 5). Then, we add all the multiples of 5 (that were multiples of 3 and 5) to sum.

On my computer, this code executes at (an average)  4050 milliseconds.  How can the execution time be reduced?

MultipleOf3.java

//Calculates the sum of multiples of 3 from 3 to 2100000000.
public class MultipleOf3  implements Runnable {
    private volatile int sum = 0;

    public void run() {
        int counter = 1;
        for(int i = 3; i < 2100000000; i += 3) {

          //If the multiple of 3 ends with a 5 or 0, then skip it because
           // it is a multiple of 5.
          if(counter == 5) {
                counter = 1;
                continue;
           }

           sum += i;
           counter++;
        }
    }

   public int getSum() {
       return sum;
   }
}

MultipleOf5.java

// Calculates the sum of multiples of 5 from 5 to 2100000000.

    public class MultipleOf5  implements Runnable {
         private volatile int sum = 0;

         public void run() {
              //This time we added all multiples of 5 - including the
             //values that were skipped that were multiples of 3 and 5
             for(int i = 5; i < 2100000000; i += 5) {
                sum += i;
             }
         }

         public int getSum() {
             return sum;
         }

    }

Main.java

public class Main {

    private static int parallelSolution() throws InterruptedException{
        int totalSum = 0;

        MultipleOf3 multipleOf3 = new MultipleOf3();
        Thread newThread = new Thread(multipleOf3);

        MultipleOf5 multipleOf5 = new MultipleOf5();
        Thread secondThread = new Thread(multipleOf5);

        newThread.start();
        secondThread.start();

        newThread.join();
        secondThread.join();

        totalSum += multipleOf3.getSum();
        totalSum += multipleOf5.getSum();

         return totalSum;
   }

    public static void main(String[] args) throws InterruptedException  {
        long startTime = System.currentTimeMillis();
        int sum = parallelSolution();
        long stopTime = System.currentTimeMillis();
        long elapsedTime = stopTime - startTime;

        System.out.println("The total sum = " + sum);
        System.out.println("Elapsed time is " + elapsedTime);
    }

}

Bug

Both sums are much too large to fit into a 32-bit int.  So this code returns an incorrect answer.

Gaussian solution

If you first calculate how many multiples of whatever there are, you can use the Gaussian formula to get the sum directly.

public class MultipleOf implements Runnable {

    private final int interval;
    private final long ceiling;
    private long sum;

    public MultipleOf(int interval, long ceiling) {
        this.interval = interval;
        this.ceiling = ceiling;
    }

    @Override
    public void run() {
        // n is the number of multiples of interval that are less than ceiling
        long n = (ceiling - 1) / interval;
        sum = interval * (n * (n+1) / 2);
    }

    public long getSum() {
        return sum;
    }

}

This also requires only one multiples class and removes the magic numbers from the class.

You can run the code like

    private static long parallelSolution(long ceiling) throws InterruptedException {
        long totalSum = 0;

        MultipleOf multipleOf3 = new MultipleOf(3, ceiling);
        Thread thread3 = new Thread(multipleOf3);

        MultipleOf multipleOf5 = new MultipleOf(5, ceiling);
        Thread thread5 = new Thread(multipleOf5);

        MultipleOf multipleOf15 = new MultipleOf(15, ceiling);
        Thread thread15 = new Thread(multipleOf15);

        thread3.start();
        thread5.start();
        thread15.start();

        thread3.join();
        thread5.join();
        thread15.join();

        totalSum += multipleOf3.getSum();
        totalSum -= multipleOf15.getSum();
        totalSum += multipleOf5.getSum();

         return totalSum;
    }

When I ran this with almost your original main, I got an answer in 2 milliseconds.  Your original code ran in 6000 milliseconds in my test.  Of course, running without threads takes 0 milliseconds.

I also changed the thread names to be more descriptive.

Overhead

I'd try running your solution without threads and see if it is faster or slower.


Showing/hiding controls based on event I have the following code in a portion of my program that hides/shows certain elements based on the status of a certain checkbox:

private void enableFolderVariableRemoval_CheckedChanged(object sender, EventArgs e)
{
    if (enableFolderVariableRemoval.Checked)
    {
        cleanFolderTextPanel.Visible = true;
        cleanTextPanel.Visible = true;
    }
    else
    {
        cleanFolderTextPanel.Visible = false;
        if (cleanFilenameTextPanel.Visible == false)
        {
            cleanTextPanel.Visible = false;
        }
    }
}

Is there a better way to handle this without a whole bunch of conditionals that set other controls to hide/show?

Not sure if there are any other constraints but here is one possible solution:

private void enableFolderVariableRemoval_CheckedChanged(object sender, EventArgs e)
{
    cleanFolderTextPanel.Visible = enableFolderVariableRemoval.Checked;
    cleanTextPanel.Visible = cleanFolderTextPanel.Visible || cleanFilenameTextPanel.Visible;
}


Scissors, Paper, Rock in Python The game works, but I was wondering if anyone could help me with better structure for the code, simplification, or just any other improvements.

from random import randrange

# assigns the amount of turns and starts the program
def Main():
    print "~A game of Scissors, Paper, Rock!~"
    global turns
    turns = raw_input("How many times do you want to play? : ")
    # tests for int input and restarts if not an int
    try:
        turns = int(turns)
        print "\nYou will play", turns, "turn(s) against the computer! \n"
        play()
    except ValueError:
        print "That's not a valid input! Integers only, please! \n"
        Main()

#determines the player's choice and sets the computers choice
def play():
    global options
    options = {1: "Scissors", 2: "Paper", 3: "Rock"}
    global turns
# loops for different choices, and calls the comparison function
    while turns > 0:
        try:
            global pchoice
            pchoice = int(raw_input("Choose one! \n <1> Scissors \n <2> Paper \n <3> Rock \n"))
            global cchoice
            cchoice = randrange(1,4)
            comparison()
        except KeyError:
            print "an invalid input! Please try again. \n"
            play()
        turns -= 1

# compares player's choice with the computer's and returns a tie, win, or loss
def comparison():
    print "You chose:", options[pchoice], "and the computer chose:", options[cchoice]
    if int(pchoice) == cchoice:
        print "A tie! \n"
    elif pchoice == 1 and cchoice == 3 or pchoice == 2 and cchoice == 1 or pchoice == 3 and cchoice == 2:
        print "You lose! \n"
    else:
        print "You win! \n"

Main()

I'd like to focus on two key issues:

Global variables

You should strive to have no global variables in your program, and it is entirely possible to eliminate them from this program.

If your functions only ever use local variables, then you can understand your code by analyzing each function separately.  However, when you use global variables, you will need to analyze the entire program at once, because the values could be affected by code anywhere in the program.

For example, there is no reason why turns needs to be global.  By making it global, you cannot be sure, just by looking at the play() function, that the loop will execute turns number of times, because you have to worry about the possibility that comparison() modifies its value.  Rather, turns should be local to play().  (Actually, it should be a parameter.)

def play(turns):
    # loops for different choices, and calls the comparison function
    while turns > 0:
        try:
            pchoice = int(raw_input("Choose one! \n <1> Scissors \n <2> Paper \n <3> Rock \n"))
            cchoice = randrange(1,4)
            comparison(pchoice, cchoice)
        except KeyError:
            print "an invalid input! Please try again. \n"
            play(turns)     # Problematic.  I'll say more about this later…
            return
        turns -= 1

Use of functions as if they were GOTO labels

Suppose that at the "How many times do you want to play?" prompt, you hit Enter many times, then hit ControlC.  You would get a stack trace like this:

How many times do you want to play? : ^CTraceback (most recent call last):
  File "cr64216.py", line 45, in <module>
    Main()
  File "cr64216.py", line 15, in Main
    Main()
  File "cr64216.py", line 15, in Main
    Main()
  File "cr64216.py", line 15, in Main
    Main()
  File "cr64216.py", line 15, in Main
    Main()
  File "cr64216.py", line 15, in Main
    Main()
  File "cr64216.py", line 15, in Main
    Main()
  File "cr64216.py", line 15, in Main
    Main()
  File "cr64216.py", line 15, in Main
    Main()
  File "cr64216.py", line 15, in Main
    Main()
  File "cr64216.py", line 15, in Main
    Main()
  File "cr64216.py", line 15, in Main
    Main()
  File "cr64216.py", line 7, in Main
    turns = raw_input("How many times do you want to play? : ")
KeyboardInterrupt

That deep stack trace is a sign of improper recursion.  A properly constructed program should only have one frame in the stack at that point, namely Main().

Rather than recursing, you should implement the retry mechanism using a loop.

def Main():
    print "~A game of Scissors, Paper, Rock!~"

    # Ask until a valid number of turns is given by the user
    while True:
        turns = raw_input("How many times do you want to play? : ")
        # tests for int input and restarts if not an int
        try:
            turns = int(turns)
            print "\nYou will play", turns, "turn(s) against the computer! \n"
            break
        except ValueError:
            print "That's not a valid input! Integers only, please! \n"

    play(turns)


Getting database values grouped in an array I have a database column with dates corresponding to sales orders. I get the count of orders in each month. Currently in my code, I use a switch statement. I know it is not a neat way of coding. So I need to know if there is any way I can do it more precisely. I need the output in such a way as below to form a chart in JavaScript.

$datayearly = array(
    'labels' => array('JAN', 'FEB', 'MAR', 'APR', 'MAY', 'JUN', 'JUL', 'AUG', 'SEP', 'OCT', 'NOV', 'DEC'),
    'series' => array(array(
        'name' => 'series-1',
        'data' => array($jan, $feb, $mar, $apr, $may, $jun, $jul, $aug, $sep, $oct, $nov, $dec)
    ))
);

Php Code for fetching from database and screenshot of the database.

$thisyear =  date("Y");
$jan=$sep=$feb=$mar=$apr=$may=$jun=$jul=$aug=$sep=$oct=$nov=$dec=0;
$year[0] =0;
$stmt = $conn1->prepare("SELECT * FROM salessummary WHERE dateoforder LIKE :key");
$stmt->execute(array('key' => "%{$thisyear}%"));
foreach ($stmt as $row) {
    $month = date('m', strtotime($row['dateoforder']));
    switch ($month) {
        case "01":
        $jan++;
        case "02":
        $feb++;
        case "03":
        $mar++;
        case "04":
        $apr++;
        case "05":
        $may++;
        case "06":
        $jun++;
        case "07":
        $jul++;
        case "08":
        $aug++;
        case "09":
        $sep++;
        case "10":
        $oct++;
        case "11":
        $nov++;
        case "12":
        $dec++;
    }
}
$high = max($jan, $feb, $mar, $apr, $may, $jun, $jul, $aug, $sep, $oct, $nov, $dec) + 10;
$datayearly = array(
    'labels' => array('JAN', 'FEB', 'MAR', 'APR', 'MAY', 'JUN', 'JUL', 'AUG', 'SEP', 'OCT', 'NOV', 'DEC'),
    'series' => array(array(
        'name' => 'series-1',
        'data' => array($jan, $feb, $mar, $apr, $may, $jun, $jul, $aug, $sep, $oct, $nov, $dec)
    ))
);
$temp = json_encode($datayearly);

SQL query

Of course, all the data calculations should be done on the database side. So the main improvement would be an SQL query like this

select upper(date_format(dateoforder, "%b")), count(1)
from salessummary group by month(dateoforder);

PDO magic

Next, let's utilize the PDO's magic constant that can load your data into the neat associative array, PDO::FETCH_KEY_PAIR, with month abbreviations as keys and order count for the given month as values

So our code would be

$thisyear =  date("Y");
$sql = "select upper(date_format(dateoforder, "%b")), count(1)
        from salessummary group by month(dateoforder) WHERE dateoforder LIKE ?";
$stmt = $conn1->prepare($sql);
$stmt->execute(array("$thisyear%"));
$data = $stmt->fetchAll(PDO::FETCH_KEY_PAIR);

Data gathering

It will give us the information for all the data that exists in the database, but with obvious gaps. To fill them, let's run a simple loop

$result = [];
foreach (range(1,12) as $num) {
    $mon = strtoupper(date("M", strtotime("$thisyear-$num-15")));
    $result[$mon] = $data[$mon] ?? 0;
}

and then assemble the final data structure

$datayearly = array(
    'labels' => array_keys($result),
    'series' => array(array(
        'name' => 'series-1',
        'data' => array_values($result)
    ))
);

Note that the null coalesce operator (i.e. ??) exists in PHP7 only. If your PHP version is outdated, then you could use the old form,

     $result[$mon] = isset($data[$mon]) ? $data[$mon] : 0;


Is there a better way to traverse an inheritance tree? I have a method that ascends an inheritance tree and returns the element at the root, progressively yielding the elements if appropriate:

# Finds the root of the hierarchy.
def root
  element = self
  until element.root?
    yield element if block_given?
    element = element.parent
  end
  yield element if block_given?
  element
end

# Walks the inheritance tree, yielding each element from self to #root.
alias until_root root

If I have a hierarchy like:

1 > 2 > 3

3.root should return 1 and 3.until_root &block should yield 3, then 2, then 1, and finally return 1.

The line which yields elements appears twice in order to yield the actual root element, something I would've liked to accomplish inside the loop.

How can I improve this algorithm?

One way to only have one yield statement would be to recursion instead of a loop:

def root(&blk)
  yield self if blk
  if root?
    self
  else
    parent.root(&blk)
  end
end

Of course the usual caveats regarding recursion in ruby apply, so this isn't necessarily a good idea: It is somewhat expensive and can lead to stack overflows if you recurse deep enough (though it's rather unlikely, you'd encounter a tree deep enough to reach that limit).

Unrelated to this, the name until_root seems somewhat clunky to me. I would recommend each_ancestor instead. In fact I think it would be good idea to have the method return an enumerable of ancestors when no block is given. Then it could just be called ancestors. Of course that would require separating ancestors and root into two separate methods. root could then just be defined as ancestors.last:

# Returns an Enumerable containing the ancestors of this node, starting with
# the node itself and ending with the root.
# If a block is supplied, each ancestor will be yielded to the block instead
# of returning an enumerable.
def ancestors
  return enum_for(:ancestors) unless block_given?

  element = self
  until element.root?
    yield element
    element = element.parent
  end
  yield element
end

def root
  ancestors.last
end

If you do it that way, you can also leave out the if block_given? part of the yield statements because it will return early when no block was given.

As a matter of fact, you can now remove the second yield completely by changing the loop to loop while element is not nil instead of until the root is reached (assuming element.parent will return nil if the element is the root):

def ancestors
  return enum_for(:ancestors) unless block_given?

  element = self
  while element
    yield element
    element = element.parent
  end
end


React container component This is my first reactJS project and I am trying to build a page out of my components. I thought it would be best to create a section component and then put appropriate components inside of it.

I'm not sure if this is the correct way... it works but looks a bit weird when there is more than one component. Also VS Code yells at me if I put an apostrophe in a heading.

MySection.js

            {this.props.content}

Homepage.js

      <MySection
        bgColor={'bg-gray'}
        content={
          [
            <MediaObject heading={'today's rates!'} />,
            <MediaObject heading={'some other heading'} />,
            <MediaObject heading={'other other heading'} />
           ] />

      <MySection
        bgColor={'bg-gray'}
        content={
            <MediaObject heading={'today's rates!'} />,
        } />

You don't have to put content as a prop, you can do this:

{this.props.children}

and then you can use your component as:

 <MySection bgColor="bg-gray">
    <MediaObject heading="today's rates!" />
    <MediaObject heading="some other heading" />
    <MediaObject heading="other other heading" />
 </MySection>

<MySection bgColor="bg-gray">
  <MediaObject heading="today's rates!" />
</MySection>

So whatever is written inside the "body" of the component, is passed on as this.props.children.

Regarding VSCode yelling at you about the apostrophe is that you were closing the apostrophe before the string end:

heading={'today's rates!'}

If you use a single quote inside a string, use double quotes:

heading={"today's rates!"}

or, you can escape the quote:

heading={'today\'s rates!'}


Prevent a UITextField from being input with aphabetic characters The goal is to disallow typing alphabetic characters as input in a UITextField.

Please tell me if this approach is convenient.

func textField(_ textField: UITextField, shouldChangeCharactersIn range: NSRange, replacementString string: String) -> Bool {
        let charSet = CharacterSet.letters
        let existingTextHasDecimalSeparator = textField.text?.range(of: ".")
        let replacementTextHasDecimalSeparator = string.range(of: ".")
        let existingTextHasAlphabeticCharacters = textField.text?.rangeOfCharacter(from: charSet)
        let replacementTextHasAlphabeticCharacters = string.rangeOfCharacter(from: charSet)

        if existingTextHasDecimalSeparator != nil,
            replacementTextHasDecimalSeparator != nil {
            return false
        } else if existingTextHasAlphabeticCharacters != nil ||
                    replacementTextHasAlphabeticCharacters != nil {
                return false
        } else {
            return true
        }
    }

If you want to disallow letters as input, why do you also test the old string of the text field? And why do you test, if the old and the new string contain a dot? The very simple solution to forbid letters is to use the one liner

func textField(_ textField: UITextField, shouldChangeCharactersIn range: NSRange, replacementString string: String) -> Bool {
    return string.rangeOfCharacter(from: CharacterSet.letters) == nil
}

or

func textField(_ textField: UITextField, shouldChangeCharactersIn range: NSRange, replacementString string: String) -> Bool {
    return string.rangeOfCharacter(from: CharacterSet(charactersIn: "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ")) == nil
}

if you want to be more specific about the disallowed characters.


Printing future leap years My code prints the next 20 leap years:

def loop_year(year):
    x = 0
    while x < 20:
        if year % 4 != 0 and year % 400 != 0:
            year +=1
            ##print("%s is a common year") %(year)
        elif year % 100 != 0:
            year +=1
            print("%s is a leap year") % (year)
            x += 1

loop_year(2020)

I want to use fewer lines. I thought of using a for but some people said it was a waste of time. What else can I do?

I can't see a way to make this shorter (I can't see how is this relevant to any performance issue in Python) as your solution is straight forward but I have some advice:

Use calendar module to get rid of those ugly conditions.

calendar.isleap(year)

Formatting:

Try formatting your strings using format() (it's much cleaner looking):

This:

print("%s is a leap year") % (year)

Would become this:

print('{} is a leap year!'.format(year))

In your method, add another parameter which will be the number of years (this will make your code more user-friendly) - you don't want somebody to modify the number of leap years inside your block code. Just make it happen when they are calling the method:

def loop_year(year, number_of_years):
    ...

More, your variable names should have specific names based on their usage: I would personally change x to leap_year_counter. When it comes to a simple counter, your naming solution it's also ok, but if your code will become more robust, you'll have a hard time figuring what is what.

IF/ELSE Statements:

As others mentioned, you have year += 1 in your both statements, so just move it after them. More, in this case, you can also remove the else statement.

# ...
if calendar.isleap(year):
    print('{} is a leap year!'.format(year))
    leap_year_counter += 1
year += 1
# ...

Finally, your method will look like this:

import calendar

def loop_year(year, number_of_years):
    leap_year_counter = 0
    while leap_year_counter < number_of_years:
        if calendar.isleap(year):
            print('{} is a leap year!'.format(year))
            leap_year_counter += 1
        year += 1
loop_year(2016, 20)