jQuery range slider change text based on hidden field value I am working on a jQuery ratings function that uses the Foundation range slider. My current implementation works correctly on 2 of 7 of my range sliders. However, I am quickly realising that my implementation will become bloated as I have to repeat myself for each of the 7 range sliders.

HTML:

      <%= f.hidden_field :quality %>

    Average

      <%= f.hidden_field :communication %>

    Average

jQuery:

$(document).on("page:load ready", function() {
  function ratings(selector, callback) {
    var input = $(selector);
    var oldvalue = input.val();
    setInterval(function(){
      if (input.val()!= oldvalue){
        oldvalue = input.val();
        callback();
      }
    }, 100);
  }
  ratings('input#agent_score_quality', function() {
    var inputValue = $('input#agent_score_quality').val();

    if(inputValue === '1') { // Terrible
      $("span#sliderOutput1").fadeOut(function() {
        $(this).text('Terrible').fadeIn();
      });
    }
    if(inputValue === '2') { // Poor
      $("span#sliderOutput1").fadeOut(function() {
        $(this).text('Poor').fadeIn();
      });
    }
    if(inputValue === '3') { // Average
      $("span#sliderOutput1").fadeOut(function() {
        $(this).text('Average').fadeIn();
      });
    }
    if(inputValue === '4') { // Good
      $("span#sliderOutput1").fadeOut(function() {
        $(this).text('Good').fadeIn();
      });
    }
    if(inputValue === '5') { // Excellent
      $("span#sliderOutput1").fadeOut(function() {
        $(this).text('Excellent').fadeIn();
      });
    }
  });
  ratings('input#agent_score_communication', function() {
    var inputValue = $('input#agent_score_communication').val();

    if(inputValue === '1') { // Terrible
      $("span#sliderOutput2").fadeOut(function() {
        $(this).text('Terrible').fadeIn();
      });
    }
    if(inputValue === '2') { // Poor
      $("span#sliderOutput2").fadeOut(function() {
        $(this).text('Poor').fadeIn();
      });
    }
    if(inputValue === '3') { // Average
      $("span#sliderOutput2").fadeOut(function() {
        $(this).text('Average').fadeIn();
      });
    }
    if(inputValue === '4') { // Good
      $("span#sliderOutput2").fadeOut(function() {
        $(this).text('Good').fadeIn();
      });
    }
    if(inputValue === '5') { // Excellent
      $("span#sliderOutput2").fadeOut(function() {
        $(this).text('Excellent').fadeIn();
      });
    }
  });
});

How can I improve this code so I don't have to repeat the same thing over and over just with different selectors? My idea is to use a single selector for the sliderOutput and use the .closest() method to target each and insert my text, but I'm new to jQuery, so could I be way off?

First of all you can dramatically simplify your callback function body structure.
Instead of using a series of if (inputValue === ... you might define a simple array of your texts, like:

var texts = [
    'Terrible',
    'Poor',
    'Average',
    'Good',
    'Excellent'
];

Then the callback function entire body becomes as simple as:

// here the example of your first ratings() invocation
$('span#sliderOutput1').fadeOut(function() {
    $(this).text(texts[$('input#agent_score_quality').val() - 1]).fadeIn();
});

Now to avoid rewriting such a callback function for each of your rows, you can make it a named, independent function, then invoke it from your `setInterval() callback, with the needed information about involved row, so the whole JS part looks like this:

$(document).on("page:load ready", function() {

var texts = [
    'Terrible',
    'Poor',
    'Average',
    'Good',
    'Excellent'
];

  function ratings(selectorInput, selectorSlider) {
    var input = $(selectorInput);
    var slider = $(selectorSlider);
    var oldvalue = input.val();
    setInterval(function(){
      if (input.val()!= oldvalue){
        oldvalue = input.val();
        ratingsCallback(input, slider);
      }
    }, 100);
  }

  function ratingsCallback(input, slider) {
    $(slider).fadeOut(function() {
      $(this).text(texts[input.val() - 1]).fadeIn();
    });
  }

  ratings('input#agent_score_quality', 'span#sliderOutput1');
  ratings('input#agent_score_communication', 'span#sliderOutput2');
  // and so on...
});

Finally better solution

(but see also CAVEAT below)

Thanks to combined suggestions from @Chococroc comments and @Margus answer, this is a yet much more minified version of the code.

It only needs that each  element becomes .
Then the whole JS part can be reduced to:

$(document).on("page:load ready", function() {

  var texts = [
      'Terrible',
      'Poor',
      'Average',
      'Good',
      'Excellent'
  ];

  $('.row input').on('change', function() {
    var slider = $(this).closest('.row').find('.slider-output');
    slider.fadeOut(function() {
      slider.text(texts[input.val() - 1]).fadeIn();
    });
  }
});

Note that this solution only focuses on the initial question main issue. Beyond that, as noticed by @Chococroc, it could be enhanced in several ways, like:

allow strict independency between logics and presentation, by using
dedicated class names like js-slider-output, so CSS needs don't mix
with JS needs
ensure to avoid any potential conflict with other
class="row" elements in the whole context, by affecting a
supplemental class to each row, e.g.
,
so the change-event binding becomes $('.js-slider input')...

CAVEAT

Still focused on the main issue (how to reduce JS code) I didn't pay attention to a particular point in the given HTML example: here the source data for the range values resides in hidden elements.

So as pointed out by the OP author, in this case the change event will not fire when value changes.

So what?

from the fact that these elements are hidden, we can assert that the changes don't come from user action
then we can assume that some other JS part is involved to make this change to happen
so we should have the availability to add <this element>.change(); somewhere in this JS part

Hopefully not forgetting some other blocking point...


Matrix falling effect How can I make this matrix effect more like the matrix movie?

#include<stdlib.h>
#include<stdio.h>
int main()
{
srand((unsigned) time(0));
int i,j;
system("color 0a");

while(1)
{
    i = rand() % 2;
    j= rand() % 2;
    if(j)
    {
       printf("%d ", i);
    }
    else
    {
        printf(" %d", i);
    }

}
return 0;
}

Here is how it looks like:

Of course it is better if it was moving but that's all what I can show.

Also it takes a notable time to compile, is there anything wrong I'm doing?

What You Have Now:

You code is simple and concise. That in and of itself can be a desirable quality. I think we can make the code a little bit more concise while gaining some runtime efficiency.

Some changes I would make:

Change srand((unsigned) time(0)); to srand(time(NULL));

While both are correct, this is how I have seen seeds set more often, hence it is the idiomatic approach

Change return 0 to return EXIT_SUCCESS

Exit codes are included in stdlib, since you already using that library, you should also use the provided exit codes.

Change rand()%2 to rand()&1

The modulo operator (%) can be very inefficient on some architectures. Since you are only using rand() to get binary values, we can much more efficiently use the bitwise and operator (&) to get a 1 if the number is odd and a 0 if the number is even.

Change your while loop to remove intermediate variables:

while(1) {
 if(rand() & 1)
   printf("%d ", rand() & 1);
 else
   printf(" %d", rand() & 1);
}

All together I think you could simplify your code to this:

#include<stdlib.h>
#include<stdio.h>
int main() {
srand(time(NULL));
system("color 0a");

while(1) {
  if(rand() & 1)
    printf("%d ", rand() & 1);
  else
    printf(" %d", rand() & 1);
}

return EXIT_SUCCESS;
}

This is a pretty small and eloquent code for generating a matrix style output. If your happy with this you can stop right here.

However, it should be pointed out that printf is a little heavy handed for just printing spaces, ones, and zeros. You could utilize putchar which will, unsurprisingly, put a character to STDOUT.

while(1) {
  if(rand() & 1) {
    putchar(48 + (rand()&1))
    putchar(' ');
  }
  else {
    putchar(' ')
    putchar(48 + (rand()&1));
  }
}

Note that now we have a Magic Number 48 in out code, that is no good! Let's let the preprocessor help us make the code a little bit more readable:

#define ASCII_NUMBER_OFFSET 48
while(1) {
  if(rand() & 1) {
    putchar(ASCII_NUMBER_OFFSET + (rand()&1));
    putchar(' ');
  }
  else {
    putchar(' ');
    putchar(ASCII_NUMBER_OFFSET + (rand()&1));
  }
}

Next is a matter of personal preference. I personally don't like 2 line if statements. If the if statement has 3 lines, it should probably be abstracted into a separate method, if it has one line, that’s perfect, but if we have 2 lines... that's murky water.

We could use the comma operator to clean up the line a little:

#define ASCII_NUMBER_OFFSET 48
while(1) {
  if(rand() & 1)
    putchar(ASCII_NUMBER_OFFSET + (rand()&2)), putchar(' ');
  else
    putchar(' '), putchar(ASCII_NUMBER_OFFSET + (rand()&1));
}

Using the comma operator is a matter of preference. I think that there are times that it is perfectly clear & reasonable to use it, others say the comma operator should never be used because it is too opaque. While that code is more compact, I don't think it helps readability or efficiency.

Lets try abstraction that into a method instead and see how that looks:

while(1) {
  if(rand() & 1)
    printLeft();
  else
    printRight();
}
.
.
.
#define ASCII_NUMBER_OFFSET 48
void printLeft() {
   putchar(' ');
   putchar(ASCII_NUMBER_OFFSET + (rand()&1));
}

void printRight() {
   putchar(ASCII_NUMBER_OFFSET + (rand()&1));
   putchar(' ');
}

That makes the while loop look better but the methods might incur some performance cost. Since your tag Optimization implies that you care most about runtime efficiency and not readability/maintainability lets try something else.

You will notice in your all the code examples above, there is some repetition in the while loop. In all the cases we have 2 statements printing the one or zero. Repetition is often a good thing to focus on because it can alert you to things that can be improved. There is a mantra of Don't Repeat Yourself (DRY) in programming which is designed to help programmers locate inefficiencies and refactor their code for better readability & efficency.

Consider this loop:

int b;
while(1) {
  if(b = rand() & 1)
    putchar(' ');
  putchar(ASCII_NUMBER_OFFSET + (rand() & 1));
  if(!b)
    putchar(' ');
}

Here we have a single binary number printing statement but conditionally print a space before or after the number. From a runtime efficiency perspective this is probably optimal (though I have not done any benchmarking).

If we put this all together we have the following:

#include<stdlib.h>
#include<stdio.h>
#define ASCII_NUMBER_OFFSET 48

int main() {
int b;
srand(time(NULL));
system("color 0a");

while(1) {
  if(b = (rand() & 1))
    putchar(' ');
  putchar(ASCII_NUMBER_OFFSET + (rand() & 1));
  if(!b)
    putchar(' ');
}

return EXIT_SUCCESS;
}

I hope this has given you a lot of different angles to view you code from and given you the tools to decide what you value when improving this code, readability or runtime efficiency.


Finding the comic ID of the last XKCD comic published I decided to sidetrack and create a XKCD viewer. For certain functionality, I needed to be able to find the ID of the last comic published. This was my attempt. I'm using Enlive here to parse the page itself.

I struggled with trying to find a CSS selector to get the text node, then finally gave up and decided to do some manual parsing. It got long, and ugly, but it works! The problem is that the only place I can concretely find page IDs is as a note at the bottom of the page:

  Permanent link to this comic: https://xkcd.com/1988/

To parse that ID at the end of the link out, I need to find the text node, then parse the String. The latter was easy. The former took me a little under an hour due mostly to inexperience with CSS selectors.

What I'm looking for:

Is there a way to get the text node directly via Enlive CSS-like selectors?
Anything else that may simplify this. It's quite a series of transformations. I obviously could separate it down into a few function, but I can't see ever needing the functionality anywhere else, and it's fairly simple to test as is. Any recommendations here?

Use as of posting this:

(find-last-id)
=> 1988

(ns xkcd-viewer.mcve
  (:require [net.cgrand.enlive-html :as e])
  (:import (java.net URL)))

(def base-url "https://xkcd.com/")

; I actually use this a couple time in the real code. It doensn't seem as useful here though.
(defn parse-id?
  "Returns the str-n parsed as a long, or nil if it's unparsable."
  [str-n]
  (try
    (Long/parseLong str-n)

    (catch NumberFormatException _
      nil)))

(defn find-last-id []
  (let [digit? #(Character/isDigit ^Character %)

        id-container (-> (e/html-resource (URL. base-url))
                         (e/select [:#middleContainer])
                         (first)
                         (:content))

        raw-id (->> id-container
                    ; The text node to find is surrounded by <br>s, so
                    (drop-while #(not= (:tag %) :br)) ; get rid of everything before the first br,
                    (drop 1) ; then the br itself,
                    (first) ; then get the text node, then
                    (drop-while (comp not digit?))
                    (take-while digit?)
                    (apply str))] ; then turn the digits into a string to be parsed.

    (if-let [parsed (parse-id? raw-id)]
      parsed
      (throw (RuntimeException.
               (str "Parser broken! Did XKCD change their site?\nFound ID: " raw-id))))))

I'm not sure it's much shorter than what you wrote, but finding stuff in any tree-like data structure is what I created the tupelo.forest library for.

Here is a solution for your problem:

(dotest
  (when false ; manually enable to grab a new copy of the webpage
    (spit "xkcd-sample.html"
      (slurp "https://xkcd.com")))
  (with-forest (new-forest)
    (let [doc         (it-> (xkcd)
                        (drop-if #(= :dtd (:type %)) it)
                        (only it))
          root-hid    (add-tree-enlive doc)
          >>          (remove-whitespace-leaves)
          ;>>          (spyx-pretty (hid->bush root-hid))
          hid-keep-fn (fn [hid]
                        (let [node       (hid->node hid)
                              value      (when (contains? node :value) (grab :value node))
                              perm-link? (when (string? value)
                                           (re-find #"Permanent link to this comic" value))]
                          perm-link?))
          found-hids  (find-hids-with root-hid [:** :*] hid-keep-fn)
          link-node   (hid->node (only found-hids)) ; assume there is only 1 link node
          value-str   (grab :value link-node) ; "\nPermanent link to this comic: https://xkcd.com/1988/"
          result      (re-find #"http.*$" value-str)]
     ;(spyx-pretty link-node)  ;=> {:tupelo.forest/khids [],
                                  ; :tag :tupelo.forest/raw,
                                  ; :value "\nPermanent link to this comic: https://xkcd.com/1988/"}
     ;(spyx result) ; => "https://xkcd.com/1988/"
    )))

Documentation is ongoing, but you can see a lightning talk from the Clojure Conj 2017.


Two HTTP notification methods I have two very similar methods, which make HTTP requests, the difference is that one makes PUT and another GET. Is there any proper Ruby way not to repeat the setup code and not to pass the flag parameter?

def notify_client(url, params)
  uri = URI.parse(url)
  https = Net::HTTP.new(uri.host, uri.port)
  https.use_ssl = !Rails.env.development?
  req = Net::HTTP::Patch.new(uri.path)
  req.body = {data: {attributes: params}}.to_json
  res = https.request(req)
  puts "Response #{res.code} #{res.message}: #{res.body}"
end

def notify_vendor(url, params)
  uri = URI.parse(url)
  https = Net::HTTP.new(uri.host, uri.port)
  https.use_ssl = !Rails.env.development?
  req = Net::HTTP::Get.new(uri.path)
  req.body = {data: {attributes: params}}.to_json
  res = https.request(req)
  puts "Response #{res.code} #{res.message}: #{res.body}"
end

In such cases I try to move repeated code to separate method that accepts block.

def notify_client(url, params)
  notify(url, params) { |path| Net::HTTP::Patch.new(path) }
end

def notify_vendor(url, params)
  notify(url, params) { |path| Net::HTTP::Get.new(path) }
end

def notify(url, params)
  uri = URI.parse(url)
  https = Net::HTTP.new(uri.host, uri.port)
  https.use_ssl = !Rails.env.development?
  req = yield uri.path
  req.body = {data: {attributes: params}}.to_json
  res = https.request(req)
  puts "Response #{res.code} #{res.message}: #{res.body}"
end

That code refactoring quite simple. If you need more details - let me know.


Loading tab-separated tweet data into an array I'm working on this school project and was wondering if there was any way of storing this information better, faster, more professionally. I'm also restricted to only using an array; don't ask me we are not allowed to use ArrayLists yet.

My Code:

public void loadTweets(String fileName){

    try {
        File file = new File(fileName);
        Scanner s = new Scanner(file);

        while(s.hasNextLine()){
            numberOfTweets++;
            s.nextLine();
        }
        tweets = new String[numberOfTweets];
        s.close();

        s = new Scanner(file);
        int counter = 0;
        while(s.hasNextLine()){
            String[] elements = s.nextLine().split("\t");
            tweets[counter] = elements[2];
            counter++;
        }
        s.close();
    } catch (IOException e) {
        e.printStackTrace();
    }
}

File Example:

Each field is separated by a tab and it goes, user > date posted > tweet.

USER_989b85bb 2010-03-04T15:34:46 @USER_6921e61d can I be...
USER_989b85bb 2010-03-04T15:34:47 superstar
USER_a75657c2 2010-03-03T00:02:54 @USER_13e8a102 They reached a
USER_a75657c2 2010-03-07T21:45:48 So SunChips made a bag...
USER_ee551c6c 2010-03-07T15:40:27 drthema: Do something today that
USER_6c78461b 2010-03-03T05:13:34 @USER_a3d59856 yes, i watched...
USER_92b2293c 2010-03-04T14:00:11 RT @USER_5aac9e88: Let no 1 push u
USER_75c62ed9 2010-03-07T03:35:38 @USER_cb237f7f Congrats on...

The prohibition on ArrayList is unfortunate.  One natural solution would be to use Files.readAllLines(), but that returns a List<String>, which is probably off-limits to you.  Likewise, Files.lines() produces a Stream<String>, which would be even better and thus probably even more forbidden to you.

Your workaround is to open the file twice, which is definitely undesirable.  (File I/O is considered "expensive".)  If I had to make a recommendation based on arrays, I would suggest

Files.readAllBytes() to slurp the entire file into a byte array.
Make a String from the byte array.
Use String.split() to form an array of lines.
For each line, retain only the third field.

My reasoning is that you eventually have to read the entire file anyway, so you might as well read it all at once, and only once.  Once you have a string, you can take advantage of String.split().

I would also like to note that catching IOException to print a stack trace is counterproductive.  If you don't have a good way to handle an exception, just let it propagate by declaring public void loadTweets(…) throws IOException.  That way, you're letting the caller know that something went wrong — which is exactly what exceptions are meant for.


Python and Exception Handling IOError I'm trying to work with exception handling by displaying "No Such File" when the file doesn't exist.  I need to use a try statement.

import sys

try:
   f = open("h:\\nosuchdirectory\\nosuchfile.txt","w") # do not modify this line
    f.write("This is the grape file") # do not modify this line
    f.close() # do not modify this line

except IOError:
    print ("No Such File")

Started with:

f = open("h:\\nosuchdirectory\\nosuchfile.txt","w") # do not modify this line
f.write("This is the grape file") # do not modify this line
f.close() # do not modify this line

This is actually an interesting question.

@RuslanOsipov has provided a solution, but technically it is vulnerable to a time-of-check vs. time-of-use race condition.  In other words, there is a brief window between the isfile() call and the open() call where a things could go wrong.  Depending on the application, that could even be considered a security vulnerability.

So, what to do?  For the full story, read Python Issue 12760.  In Python 3.3, open() gained an 'x' mode flag that makes it behave like the code in this question wants it to.  For older versions of Python that lack the 'x' flag, the workaround is:

def create(file):
    fd = os.open(file, os.O_EXCL | os.O_CREAT | os.O_WRONLY)
    return os.fdopen(fd)


Is this session handler secure? Are there any security vulnerabilities in this code?

class Session {
    private $id = array();
    private $data = array();
    private $name = 'session';

    public function __construct() {
        session_start();

        global $db;

        $this->id = isset($_COOKIE[$this->name]) ? $_COOKIE[$this->name] : $this->create_id();

        $id = $this->id;
        $ip = $_SERVER['REMOTE_ADDR'];
        $user_agent = $_SERVER['HTTP_USER_AGENT'];

        $check = $db->query("SELECT session_data FROM sessions WHERE session_id = ? AND user_ip = ? AND user_agent = ?");
        $db->execute(array($id, $ip, $user_agent));

        if ($db->row_count() != 0) {
            $this->data = $db->fetch_column();
            $this->data = unserialize($this->data);
        } else {
            $this->id = $this->create_id();

            $id = $this->id;
            $data = serialize($this->data);
            $time = time();
            $ip = $_SERVER['REMOTE_ADDR'];
            $user_agent = $_SERVER['HTTP_USER_AGENT'];
            $current_page = $_SERVER['REQUEST_URI'];

            $insertion_data = array(
                'session_id'                =>      $id,
                'session_data'              =>      $data,
                'session_last_activity'     =>      $time,
                'user_ip'                   =>      $ip,
                'user_agent'                =>      $user_agent,
                'user_current_page'         =>      $current_page
            );

            $db->insert('sessions', $insertion_data);
        }
    }

    public function end() {
        global $db;

        $this->garbage();
        $id = $this->id;
        $data = serialize($this->data);
        $time = time();
        $ip = $_SERVER['REMOTE_ADDR'];
        $user_agent = $_SERVER['HTTP_USER_AGENT'];
        $current_page = $_SERVER['REQUEST_URI'];

        $name = $this->name;

        // note to self: create update function in db class
        $db->query("UPDATE sessions SET session_data = ?, session_last_activity = ?, user_ip = ?, user_agent = ?, user_current_page = ? WHERE session_id = ?");
        $db->execute(array($data, $time, $ip, $user_agent, $current_page, $id));

        @setcookie($name, $id, time()+3600, '/', '');
    }

    public function create_id() {
        return strtoupper(md5(rand(-1000000, 1000000)));
    }

    public function garbage() {
        global $db;

        $expiration = time() - '2419200';

        $db->query("DELETE FROM sessions WHERE session_last_activity < ?");
        return $db->execute(array($expiration));
    }

    public function regenerate() {
        global $db;

        $old_id = $this->id;
        $new_id = $this->create_id();

        $this->id = $new_id;

        $db->query("UPDATE sessions SET session_id = ? WHERE session_id = ?");
        return $db->execute(array($new_id, $old_id));
    }
}

Nope. Let's walk through it from a hypothetical attackers point of view.

  Okay. It's sending an identification cookie. It's hex. 32 digits long.
  Probably an md5 hash. I wonder what's hashed? I'll throw it in
  oclHashcat to check. I can make a billion guesses a second, so if it's
  a hash of something simple, I'll find out.

  <3 seconds later>

  Huh. It's a number. Lemme grab another session and see if that's a
  number too.

  Yep.

  It looks like these are all 6 digit numbers. I'll grab 10 more to
  check.

  Okay, 6 digits AND they can be positive or negative.

  That's a keyspace of about 2^21. If there are 8 sessions going right
  now, then it costs, on average,

  2^(21 - 1 - 3) requests * 1 KB per request * 0.08 $/GB
  ...
  =1.01 cents to crack a session.
  Alright, I'll start on that and see what I have in the morning.

So: Add more entropy.


A program that spells a given number [1-999,999,999,999] This program created by me spells a number in the standard notation (Billions, Millions etc). I was wondering if there is a better way to do the same thing, or if I have been a victim of some of the bad practices in this language.

I am fairly new to Python and have only been learning it for the past 20 days.
Any other types of suggestions and feedback on my variable naming practices, techniques would be wonderful.

names_dict = {}
names_dict['ones'] = {0: '', 1:'one', 2:'two', 3:'three', 4:'four', 5:'five', 6:'six', 7:'seven', 8:'eight', 9:'nine'}
names_dict['tens'] = {0: '', 2: 'twenty', 3: 'thirty', 4: 'fourty', 5: 'fifty', 6: 'sixty', 7: 'seventy', 8: 'eighty', 9: 'ninty'}

exep = {0: 'ten', 1: 'eleven', 2: 'twelve', 3: 'thirteen', 4: 'fourteen', 5: 'fifteen', 6: 'sixteen', 7: 'seventeen', 8: 'eighteen', 9: 'ninteen'}
sufix = {0: 'billion', 1: 'million', 2: 'thousand', 3: ''}

# int ----> list
def digit_extract(num):
    digit = [0]*12
    iterator = 0
    while num != 0:
        digit[iterator] = num % 10
        iterator += 1
        num //= 10
    return digit[::-1]

#list [3 digit] to str
def hundered(three_digit):
    name = ''
    if three_digit[0] != 0:
        name += names_dict['ones'][three_digit[0]] + ' '  + 'hundered' + ' '

    if three_digit[1] != 1:
        if three_digit[1] != 0:
            name += names_dict['tens'][three_digit[1]] + ' '
        if three_digit[2] != 0:
            name += names_dict['ones'][three_digit[2]]
    else:
        name += exep[three_digit[2]]

    return name

def numberNames():
    num = int(input('Enter the number: '))
    digits = digit_extract(num)
    final_name = ''

    for i in range(4):
        hund = hundered(digits[i*3: (i*3) + 3])
        if hund != '':
            final_name += hund + ' ' + sufix[i] + ' '

    print(final_name.title())

numberNames()

There is no reason for dictionaries when you have an ordered list of items starting at zero with no missing indexes, a list will do just fine.

 suffixes = ['billion', 'million', 'thousand', '']

names_dict could be two seperate variables, because you are only accessing it with literal keys.

ONES = {0: '', 1:'one', 2:'two', 3:'three', 4:'four', 5:'five', 6:'six', 7:'seven', 8:'eight', 9:'nine'}
TENS = {0: '', 2: 'twenty', 3: 'thirty', 4: 'fourty', 5: 'fifty', 6: 'sixty', 7: 'seventy', 8: 'eighty', 9: 'ninty'}

although it may not be as efficient, digit_extract() can be made very simple with a list comprehension:

def digit_extract(num, digits=12):
    return [
        (num // (10**n)) % 10
        for n in range(digits, -1, -1)
    ]

note the default amount of digits, so if your requirements change it will be easy to modifiy your code. If you choose to go this route, then you will have to account for the number of digits not being divisible by three.
Your solution for digit_extract() is also fine, but consider using reversed(digit) instead of digit[::-1] as reversed is more readable.
Since hundred() takes exactly three args, there is no need to pass it a list.

def hundred(hundreds, tens, ones):
    ...

would work instead, and if one wanted to pass a list they could call hundred(*three_digit).
when documenting python functions, the convention is to provide a docstring

def hundered(three_digit):
    """
    list [3 digit] to str
    """

this allows documentation to be accessed by hundred.__doc__.
Consider passing an integer argument to numberNames() and returning a string. Example:

def numberNames(num):
    ...
    return final_name.title()

if __name__ == "__main__":
    print(numberNames(input('Enter the number: ')))

This way numberNames can be reused in other contexts. the if __name__ == "__main__": condition allows your program to be imported as a module and its code reused  without runnint the program.
You can also simplify the for loop by iterating over digits and suffixes seperatly:

for suffix, i in zip(suffixes, range(0, 12, 3)):
    hund = hundered(digits[i:i+3])
    if hund != '':
        final_name += hund + ' ' + suffix + ' '

  feedback on my variable naming practices

Just because you mentioned it, try not to mix camel case (i.e. functionName) and undersocres (i.e. function_name).
pep8 suggests underscores and naming constant variables in all caps.


Shell script to sync dotfiles repository I'm doing the git dot files thing and created a script -- my first Unix shell script -- to synchronise the repo dot files with those in the user's home. It checks which dot files exist in the repo folder (except for .git) and then archives any clashes in the home directory before symlinking to the repo... I wrote this largely by experimentation and Googling. It works, but I'm just checking to see if there's anything I missed or if there are better solutions?

#!/bin/sh

# Archive existing dot files and symlink to the new ones
# Run from ~/dotfiles

# All dot files apart from .git, . and ..
dotFiles=(`ls -da .* | grep -Pxv '\.git|\.+'`)

# Existing dot files to archive
toArchive=()

cd ~

# Archive: Find clashes and tarball+gzip them
for i in ${dotFiles[*]}; do
  toArchive=(${toArchive[@]} `ls -d $i`)
done
if [ ${#toArchive[*]} != 0 ]; then
  archiveFile="dotfiles"`date +%Y%m%d`".tar.gz"
  tar czf $archiveFile ${toArchive[*]} --remove-files
fi

# Create symlinks
for i in ${dotFiles[*]}; do
  dotFileLink="dotfiles/"$i
  ln -s $dotFileLink $i
done

Consider putting the following at the beginning of every script:

set -e
set -u

The first makes your script abort as soon as the first command exits with an error. This ensures that your script doesn’t try to operate after it’s failed. The second yields an error whenever you try to access an empty variable.
I’m not sure why you’re using arrays for dotFiles and toArchives here. Using plain variables should work just as well.
Instead of `…` it’s generally advised to use $(…). This makes the expressions nestable and arguably more readable. Well …

This leaves us with:

set -e
set -u

dotFiles=$(ls -da .* | grep -Pxv '\.git|\.+')

# Existing dot files to archive
toArchive=''

cd ~

# Archive: Find clashes and tarball+gzip them
for i in $dotFiles; do
  toArchive=$toArchive $(ls -d $i)
done
if [ "$toArchive" != "" ]; then
  archiveFile="dotfiles"$(date +%Y%m%d)".tar.gz"
  tar czf $archiveFile $toArchive --remove-files
fi

# Create symlinks
for i in $dotFiles; do
  dotFileLink="dotfiles/"$i
  ln -s $dotFileLink $i
done


Object pool design pattern in golang I am studying design patterns in go. I need a review of this implementation. I started this exercise from wikipedia description of the pattern and implementing step by step all the sentences.

I decided to use two objects: the pool and the object managed by the pool.

type PoolObject struct {
    id int
}

type Pool struct {
    idle   *list.List
    active *list.List
}

When pool is initialized two empty lists were created.

func InitPool() Pool {
    pool := &Pool{
        list.New(),
        list.New(),
    }
    return *pool
}

Any time an object is requested, it's returned from the idle list or created from scratch.

func (p *Pool) Loan() PoolObject {
    if p.idle.Len() > 0 {
        for e, i := p.idle.Front(), 0; e != nil; e, i = e.Next(), i+1 {
            if i == 0 {
                object := e.Value.(PoolObject)
                return object
            }
        }
    }

    object := PoolObject{p.NumberOfObjectsInPool() + 1}
    p.active.PushBack(object)
    return object
}

Any time an object is returned, it's removed from the active list and pushed in the idle list.

func (p *Pool) Receive(object PoolObject) {
    p.idle.PushBack(object)
    for e, i := p.active.Front(), 0; e != nil; e, i = e.Next(), i+1 {
        if object == e.Value.(PoolObject) {
            p.active.Remove(e)
            return
        }
    }
}

You don't seem to have a policy for a maximum pool side. Is that by mistake or by design? What should happen if someone requests an object too many?
Your choice of method names is slightly off. When I talk to an object pool, I don't want to Loan it an object, nor do I want to Receive an object. I want to "borrow," "obtain," "take" or "get" an object from the pool, and I want to "return" or "release" or "put" an object back into the pool.

I'd suggest:

obj = pool.Borrow()
// ...
pool.Return(obj)

Finally, why does your object have an id? It seems unused. I'd suggest just using interface{} as your object type.


Object-to-XML string implementation I have recently implemented a code to accomplish Object-to-String. Can you suggest any pros and cons with regard to the code?

public static string ObjectToXml(object obj)
{
    var oXmlSerializer = new XmlSerializer(obj.GetType());
    var oStringWriter = new StringWriter();
    var oXmlSerializerNamespaces = new XmlSerializerNamespaces();
    oXmlSerializerNamespaces.Add(string.Empty,string.Empty);
    var oXmlWriterSettings = new XmlWriterSettings
    {
        Indent = true,
        OmitXmlDeclaration = false,
        Encoding = Encoding.GetEncoding("ISO-8859-1")
    };

    var oXmlWriter = XmlWriter.Create(oStringWriter,oXmlWriterSettings);
    oXmlSerializer.Serialize(oXmlWriter, obj, oXmlSerializerNamespaces);
    oXmlWriter.Close();
    return oStringWriter.ToString();
}

StringWriter and XmlWriter are both IDisposable hence their usage should be wrapped in a using statement
The prefixing o of your local variables smells a bit of hungarian notation which generally does not convey a lot of useful information.
The encoding has already been mentioned in the comments. In .NET strings are UTF-16 so you use should that instead.
Generally try to declare your variables as close to the point of use as possible. That way it's more obvious from which point on they are being used.

Refactored code:

public static string ObjectToXml(object obj)
{
    var settings = new XmlWriterSettings
    {
        Indent = true,
        OmitXmlDeclaration = false,
        Encoding = Encoding.GetEncoding("UTF-16")
    };

    var namespaces = new XmlSerializerNamespaces();
    namespaces.Add(string.Empty,string.Empty);

    var serializer = new XmlSerializer(obj.GetType());

    using (var stringWriter = new StringWriter())
    {
        using (var xmlWriter = XmlWriter.Create(stringWriter, settings)
        {
            serializer.Serialize(xmlWriter, obj, namespaces);
        }
        return stringWriter.ToString();
    }
}

Update: As pointed out in the comments the suggested refactored code violates point 4 above. This is because I usually try to limit the scope of using blocks so it's easier to see what's going on there.

As pointed out by Jesse you could make some of the local variables class static as they are always the same. If you put generics in the mix then you could also make the serializer class static and the total refactored code could look like this:

public static class SerializerHelper<T>
{
    private static XmlSerializer _Serializer;
    private static XmlWriterSettings _XmlSettings;

    private static XmlSerializerNamespaces _Namespaces;

    static SerializerHelper()
    {
        _Namespaces = new XmlSerializerNamespaces();
        _Namespaces.Add(string.Empty,string.Empty);

        _XmlSettings = new XmlWriterSettings
            {
                Indent = true,
                OmitXmlDeclaration = false,
                Encoding = Encoding.GetEncoding("ISO-8859-1")
            };

        _Serializer = new XmlSerializer(typeof(T));
    }

    public static string ObjectToXml(T obj)
    {
        using (var stringWriter = new StringWriter())
        {
            using (var xmlWriter = XmlWriter.Create(stringWriter, _XmlSettings))
            {
                _Serializer.Serialize(xmlWriter, obj, _Namespaces);
            }
            return stringWriter.ToString();
        }
    }
}


Meteor Form Submission I am hoping a Meteor guru can please review my code below and verify if I am properly using return false and event.preventDefault(), especially in the context of form submission.

The reason being, the Meteor docs use this approach:

// Prevent default form submit
return false;

while other tutorials use this approach:

event.preventDefault(); //prevent page reload

So I want to be sure that I am on the correct path before I proceed with the rest of my code.

Basically, my code gets the value of the username input and checks if the value exists in the database:

HTML

<template name="registration">

{{message}}

    <form id="register_form">
        <input type="text" name="username" placeholder="username">
    </form>
</template>

JS

if (Meteor.isClient) {
    // This code only runs on the client

    Meteor.subscribe("users");

    Template.registration.helpers({
        message: function () {
            return Session.get("message");
        },
        message_defined: function () {
            return Session.equals("message", undefined) ? false : true;
        }
    });

    Template.registration.events({
        "submit #register_form": function (event) {
            // This function is called when the registration form is submitted.

            event.preventDefault(); // Correct usage?

            var form_data = {
                username: event.target.username.value,
            };

            if (!Match.test(form_data.username, String)) {
                Session.set("message", "username must be a string");
                return false;  // Correct usage?
            }

            if (form_data.username.length < 6) {
                Session.set("message", "username must be at least 6 characters");
                return false;  // Correct usage?
            }

            Meteor.call("check_username",form_data.username,function(error, result){
                if (error) {
                    Session.set("message", error);
                    return false;  // Correct usage?
                }
                if (result) {
                    Session.set("message", result);
                    return false;  // Correct usage?
                }
            });
        }
    });
}

Meteor.methods({
    check_username: function (username) {
        // If user exists, return error.

        var user = Meteor.users.findOne({username:username});
        if (user) {
            throw new Meteor.Error("username exists", "That username is already in use.");
        }

        return "username not found";
    }
});

if (Meteor.isServer) {
    Meteor.publish("users", function () {
        return Meteor.users.find();  // This publishes everything - be careful!
    });
}

Case 1
The return here are good if you want to stop the execution of your function here.

Case 2
Meteor.call is asynchronous, so if (error) is not the next element on stack. You have to return false inside your event function, not inside the Meteor.call's callback.

Template.registration.events({
        "submit #register_form": function (event) {
            // This function is called when the registration form is submitted.

            event.preventDefault(); // Correct usage!

            var form_data = {
                username: event.target.username.value,
            };

            if (!Match.test(form_data.username, String)) {
                Session.set("message", "username must be a string");
                return false;  // 1 : Correct if you want to interrupt
            }

            if (form_data.username.length < 6) {
                Session.set("message", "username must be at least 6 characters");
                return false;  // 1 : Correct if you want to interrupt
            }

            Meteor.call("check_username",form_data.username,function(error, result){
                if (error) {
                    Session.set("message", error);
                }
                if (result) {
                    Session.set("message", result);
                }
                //2: Was incorrect
            });
            //2
            return false;
        }
    });

PS: You have to understand that if the submit method doesn't return false, it will send the form to the action URL. So you have to return false before the execution reaches the end of the function.


QuickCheck properties for function that sorts a list of strings I'm reading The Haskell Road to Logic, Math and Programming and for each exercise solution I write some properties to learn QuickCheck. One of the exercises is to write a function that sorts a list of strings (srtStr). I wonder if my properties are right. Also, is there something missing?

propIdempotent ∷ [String] → Bool
propIdempotent xs = srtStr xs == srtStr (srtStr xs)

propLength ∷ [String] → Bool
propLength str = length str == length (srtStr str)

propOrder ∷ [String] → Bool
propOrder xs = process (srtStr xs) True where
    process ∷ [String] → Bool → Bool
    process (x : y : xs) state = process xs (if state then x <= y else False)
    process _            state = state

check ∷ IO ()
check = do
    quickCheck propIdempotent
    quickCheck propLength
    quickCheck propOrder

Correctness of a sorting algorithm consists of showing that the resulting list is in order which your propOrder takes care of and that the resulting list is a permutation of the other list. Your propLength does the latter partially. It prevents you from adding or removing an element out of thin air but doesn't prevent you to replace one with another.

You can write a permutation checker as follows:

First define a subbag relation that is every element of the first list appears in the second.

isSubBag :: Eq a => [ a ] -> [ a ] -> Bool
isSubBag [] ys = True
isSubBag (x:xs) ys = x `elem` ys && isSubBag xs (delete x ys)

Then you can write a permutation checker by applying it both ways.

isPerm :: Eq a => [ a ] -> [ a ] -> Bool
isPerm xs ys = isSubBag xs ys && isSubBag ys xs

Your property comes out as:

propPerm :: [ String ] -> Bool
propPerm xs = isPerm xs (srtStr xs)

Overall, you can improve your process function by eliminating accumulator and using the built-in conjunction. Also a better name for it would be isInOrder

process :: [String] -> Bool
process (x : y : xs) = x <= y && process (y : xs)
process _ = True


Care to review my first hash map? This is my first attempt at a hash map and I feel it is a great start. Although it does have a certain naive feel that I'm having trouble with.

public class HashMap<TKey, TValue>
{
    const int BlockSize = 100000;
    const int NumberOfBlocks = 42950;

    readonly Entry[][] _blocks = new Entry[NumberOfBlocks][];
    readonly IEqualityComparer<TKey> _comparer = EqualityComparer<TKey>.Default;

    public void Add(TKey key, TValue value)
    {
        uint hashCode = (uint)_comparer.GetHashCode(key);
        int blockIndex = (int)(hashCode / BlockSize);
        int slotIndex = (int)(hashCode - blockIndex * BlockSize);

        var block = _blocks[blockIndex];
        if (block == null)
        {
            block = _blocks[blockIndex] = new Entry[BlockSize];
        }

        var entry =  block[slotIndex];
        if (entry == null)
        {
            entry = block[slotIndex] = new Entry(key, value);
        }
        else
        {
            while (entry != null && entry.Next != null)
            {
                if (_comparer.Equals(key, entry.Key))
                {
                    throw new Exception();
                }
                entry = entry.Next;
            }
            entry = entry.Next = new Entry(key, value);
        }
    }

    public bool ContainsKey(TKey key)
    {
        uint hashCode = (uint)_comparer.GetHashCode(key);
        int blockIndex = (int)(hashCode / BlockSize);
        int slotIndex = (int)(hashCode - blockIndex * BlockSize);

        var block = _blocks[blockIndex];
        if (block == null)
            return false;

        var entry = block[slotIndex];
        while (entry != null)
        {
            if (_comparer.Equals(key, entry.Key))
                return true;
            entry = entry.Next;
        }
        return false;
    }

    class Entry
    {
        public readonly TKey Key;
        public readonly TValue Value;
        public Entry Next;

        public Entry(TKey key, TValue value)
        {
            Key = key;
            Value = value;
        }
    }
}

(I'm assuming you're implementing your own hashtable as an exercise/kata instead of using the CLR's, as I haven't seen anything unique here that justifies rolling your own.)

Here's some feedback:

HashMap should implement core interfaces and functionality as other .NET containers do (e.g.: IEnumerable<T>, IDictionary<TKey,TValue>, provide an enumerator implementation). As a guide, look at the Dictionary<TKey,TValue> class and see what interfaces it implements. I'd expect your code to have a similar level of service;
_blocks should be a List instead of an array, and its capacity (NumberOfBlocks) should be passed optionally through an optional constructor argument. Same applies to the equality comparer. Notice that these initial values seem very, very high - allocating a 42950-Entry[] array right off the bet for an empty container, and an additional 100,000 Entry array at first addition;
Why cast hashCode to uint?
Notice the duplicate code between Add and ContainsKey. This should be cleanly refactored;
blockIndex's and slotIndex's calculations do not seem correct. For example, you are dividing by 100,000, so this will work because int.MaxValue < 100000 * 100000. However, if you make this configurable and a lower value is used, you could go past your _blocks size. You should use modulo instead of division. Also, slotIndex will always be zero (or 1 due to rounding) as (hashCode - blockIndex * BlockSize) == (hashCode - hashCode/BlockSize * BlockSize) == (hashCode - hashCode) == 0. NumberOfBlocks is not used at all here, which is weird;
You should use LinkedList instead of rolling your own linked list (again, I'm assuming you're writing some exercise code, otherwise it's a must). Or at least you should define properties instead of exposing the values as public members;
Don't throw Exception. Create your own custom exception with a proper name;
Setting the entry variable in the if statement in Add is needless and just reduces readability.
The while loop in Add is a bit weird. Why do you need to check entry and entry.Next? I think you meant entry.Next only.
Setting entry at the end of the while look is needless and just reduces readability. I'd actually rename entry to previous for clarity.

Did you write any unit tests for this? Make sure you do so.

EDIT: typo: CRL => CLR


Reverse last 2 chars of a String I have this problem:
Given a String of any length, return a new String where the last 2 chars, if present, are swapped. My question is, is there a more delicate way to do this than this way:

public String lastTwo(String str) {
   if(str.length() < 2 || str.equals("")){
      return str;
   }
   String lastTwo = str.substring(str.length() - 2);
   StringBuilder sb = new StringBuilder(lastTwo);
   String lastTwoReversed = sb.reverse().toString();
   return str.substring(0, str.length() - 2) + lastTwoReversed;
}

Rather than generating the substring for the last two chars and reversing it, you could simply add the last two chars in reverse order:

return str.substring(0, str.length() - 2)
    + str.charAt(str.length() - 1)
    + str.charAt(str.length() - 2);


Solving the travelling salesman problem using a genetic algorithm I've got a simple program written in Golang that takes a map as input and tries to solve the travelling salesman problem by using a genetic algorithm. My Crossover method is a real performance killer due to intense use of make() (allocate memory / will provoke the GC).

What I'd like to do - and yet failed to do is outsource the creation of newGenes. pCrossover and therefore the size of newGenes will be the same over all calls of the function, so I'd like to avoid creating that slice again and again.

I am also interested in any suggestions for improvements of my coding style in Golang and other performance improvements I can take a look at.

My source is at Github.

Here is an excerpt of the function I am trying to improve:

func (ts *TravellingSalesman) Crossover() {
    // Crossover
    var nCrossover = int(ts.pCrossover * float64(ts.nGenes))
    var newGenes = make([]ga.Gene, nCrossover)
    for i:=0; i < nCrossover; i++ {
        newGenes[i].Data = make([]int, ts.geneLength)
        n := rand.Intn(ts.nGenes)
        m := rand.Intn(ts.nGenes)

        currentCity := ts.genes[n].Data[0];
        newGenes[i].Data[0] = currentCity
        for k:=1; k < ts.geneLength; k++ {
            nextN := findNextCity(&(ts.genes[n].Data), currentCity)
            nextM := findNextCity(&(ts.genes[m].Data), currentCity)

            existN := isInArray(&(newGenes[i].Data), nextN)
            existM := isInArray(&(newGenes[i].Data), nextM)

            // n exists, m doesnt -> take m
            if existN && !existM {
                newGenes[i].Data[k] = nextM
                currentCity = nextM
            } else if !existN && existM {
                newGenes[i].Data[k] = nextN
                currentCity = nextN
            } else if existN && existM {
                nextRandom := findNextRandomCity(newGenes[i].Data[0:k], &(ts.genes[n].Data))
                newGenes[i].Data[k] = nextRandom
                currentCity = nextRandom
            } else {
                // If both didn't exist, take the shorter one
                distN := ts.distMatrix.GetDistance(currentCity-1, nextN-1)
                distM := ts.distMatrix.GetDistance(currentCity-1, nextM-1)

                // Take the shorter route
                if distN < distM {
                    newGenes[i].Data[k] = nextN
                    currentCity = nextN
                } else {
                    newGenes[i].Data[k] = nextM
                    currentCity = nextM
                }
            }
        }
    }

    copy(ts.genes, newGenes)
}

How about using a slice for Gene.Data?

You just allocate

var allData := make([]int, nCrossover * ts.geneLength)

before entering the for loop and assign

newGenes[i].Data = allData[i * ts.geneLength: (i + 1) * ts.geneLength]

You'd cut down on the amount of allocations (and system calls) without changing much else (except findNextCity and isInArray - but they should be written to accept slices instead of array pointers anyway).

And you'd follow the advice given by the languages creators in Effective Go.


How to refactor the boolean logic of this F# function? I have a list of path I want to filter out. I want to keep only paths matching a specific pattern and remove all paths matching another specific pattern.
To achieve this I pattern matched on each pattern. Here is the code:
let stringToSpan (s: string) =
    System.ReadOnlySpan<char>(s.ToCharArray())

let matchWildcard pattern text =
    System.IO.Enumeration.FileSystemName.MatchesSimpleExpression(stringToSpan pattern, stringToSpan text)

let filterByPattern (includePattern: string Option) (excludePattern: string Option) (paths: string seq) :string seq =
    paths
    |> Seq.filter
        (fun path ->
            let shouldKeep =
                match includePattern with
                | Some pattern -> matchWildcard pattern path
                | None -> true

            if not shouldKeep then
                false
            else
                match excludePattern with
                | Some pattern -> matchWildcard pattern path |> not
                | None -> true)

You can try this snippet with:
seq {"test"; "foo/bar"; "foo/toto"}
|> filterByPattern (Some "foo/*") (Some "*toto*")
|> printfn "%A"

// output: foo/bar

If it matters, this snippet is used inside a dotnet tool to clone multiple git repositories. You can find this tool on GitHub.
I think that we could improve this snippet a bit. Can we?

I would avoid using Option .IsNone and .Value, mainly because .Value will throw an exception if you made a mistake and called it at the wrong time. Almost all F# code will avoid that property and use pattern matching instead or some other function.
Here's another way to write the patternPredicate function, which I renamed to includePath do describe what it actually does:
let includePath (includePattern: string option) (excludePattern: string option) path =
    let isIncluded =
        match includePattern with
        | Some includePattern -> matchWildcard includePattern path
        | None -> true
    let isExcluded =
        lazy
            match excludePattern with
            | Some excludePattern -> matchWildcard excludePattern path
            | None -> false
    isIncluded && not isExcluded.Value

My approach here was to first build up the values isIncluded and isExcluded so that we can write isIncluded && not isExcluded.Value. This breaks the problem down into understandable and explicit parts.
Note the use of a lazy value for isExcluded. This means that it won't be evaluated unless isIncluded is true, so we don't need to call matchWildcard unnecessarily.


Find the missing number and repeated number I am practising interview questions from here.

  Problem : You are given a read only array of n integers from 1 to n.
  Each integer appears exactly once except A which appears twice and B
  which is missing. Return A and B such that output A should precede B.

My approach :

Sum(Natural numbers) = Sum(given numbers) - A + B
Sum_squares(Natural numbers) = Sum_squares(given numbers) - A*A + B*B

where  :

Sum of n Natural numbers is given by : n(n+1)/2
Sum of squares of n Natural numbers is given by : n((n+1)/2)((2n+1)/3)

The above two equations can be simplified to :

(B-A)  = Sum(Natural numbers) - Sum(given numbers)
(B-A)*(B+A) = Sum_squares(Natural numbers) - Sum_squares(given numbers)

Solution:

def repeat_num_and_missing_num(array):
    """ returns the value of repeated number and missing number in the given array
    using the standard formulaes of Sum of n Natural numbers and sum of squares of n Natural Numbers"""
    missing_num = 0
    repeated_num = 0
    x = len(array)
    sum_of_num = 0
    sum_of_squares = 0
    sum_of_num_actual = (x*(x+1))/2
    sum_of_squares_actual = ((x)*(x+1)*(2*x+1) ) / 6

    for num in array:
        sum_of_num += num
        sum_of_squares += num*num

    missing_num = (((sum_of_squares_actual - sum_of_squares) /(sum_of_num_actual - sum_of_num))
                    +(sum_of_num_actual - sum_of_num))/2
    repeated_num = (((sum_of_squares_actual - sum_of_squares) /(sum_of_num_actual - sum_of_num))
                     -(sum_of_num_actual - sum_of_num))/2
    return repeated_num, missing_num

#Test Cases
print repeat_num_and_missing_num([5,3,2,1,1]) == (1,4)

print repeat_num_and_missing_num([1,2,3,4,5,16,6,8,9,10,11,12,13,14,15,16]) == (16,7)

print repeat_num_and_missing_num([1,1]) == (1,2)

How can I make this code better?

Removing trailing whitespace, renaming variables, reordering the code to group statements about the same concepts and introducing temporary variable to avoid writing and computing the same expressions multiple times and adding comments:

def repeat_num_and_missing_num(array):
    """ Return the value of repeated number and missing number in the given array
    using the standard formulaes of Sum of n Natural numbers and sum of squares of n Natural Numbers"""

    sum_of_num = 0
    sum_of_squares = 0
    for num in array:
        sum_of_num += num
        sum_of_squares += num*num

    x = len(array)
    sum_of_num_expected = (x * (x+1)) / 2
    sum_of_squares_expected = ((x) * (x+1) * (2*x+1)) / 6

    # Assuming A is present twice and B is missing:
    # B - A
    b_minus_a = sum_of_num_expected - sum_of_num
    # B^2 - A^2 = (B-A) * (B+A)
    b2_minus_a2 = sum_of_squares_expected - sum_of_squares
    # B + A
    b_plus_a = b2_minus_a2 / b_minus_a

    a = (b_plus_a - b_minus_a) / 2
    b = (b_plus_a + b_minus_a) / 2
    return a, b


Using a loose array just to specify the object properties I'm using this array to specify the properties I want to operate on. I didn't use for..in because there's a property that doesn't follow the same pattern.

const data = {};

[
  'teaching_levels',
  'axes',
  'accessibility_resources',
  'contents',
  'resources',
].forEach((name) => {
  data[name] = {
    options: results[name],
    values: req.query[name],
  };
});

// This property is different
data.licenses = results.licenses;

Is it alright to have an array like that?

I would say, yes, definitely. That saves a lot of manual code writing and makes it easier to change in the event you need to change all of them.

The only difference I would probably make is, add your different properties when you create the object, that will not only slightly improve speed, but will also make the code a tiny bit smaller.

const data = {
    licenses: results.licenses
};

[
  'teaching_levels',
  'axes',
  'accessibility_resources',
  'contents',
  'resources',
].forEach(name => data[name] = {
    options: results[name],
    values: req.query[name],
});


Array splitting in sub-arrays of consecutive elements I want to split an array in sub-arrays of consecutive elements. For example, for the array:

a=[1 2 3 6 7 9 10 15]

I want an output 1 2 3, 6 7, 9 10, 15.

I think that the natural choice is to use a struct for this:

[v,x] = find(diff(a)>1) %find "jumps"

xx=[0 x length(a)]

for ii=1:length(xx)-1
   cs{ii}=a(xx(ii)+1:xx(ii+1)); %output struct array
end

  v =

 1     1     1

  x =

 3     5     7

  xx =

 0     3     5     7     8

The code works correctly but I was wondering if there are smarter ways to do this.

You should preallocate the cs cell array:

[v,x] = find(diff(a)>1); %find "jumps"
xx = [0 x length(a)];
cs = cell(length(a)+1,1);
for ii = 1:length(xx)-1
   cs{ii} = a(xx(ii)+1:xx(ii+1));
end

Style comments:

Try to keep consistent formatting, either put spaces around all equal signs, or around none.
Terminate statements with a semicolon to prevent your function producing output to the command window.


Worst case runtime analysis of a string partitioning algorithm Give all the partitionings of  a string s such that every substring of the partition is a
  palindrome

What is the time complexity of this solution? And, how can I improve my code overall?

List<List<String>> resultLst;
ArrayList<String> currLst;
public List<List<String>> partition(String s) {
    resultLst = new ArrayList<List<String>>();
    currLst = new ArrayList<String>();
    backTrack(s,0);
    return resultLst;
}
public void backTrack(String s, int l){
    if(currLst.size()>0 //the initial str could be palindrome
        && l>=s.length()){
            List<String> r = (ArrayList<String>) currLst.clone();
            resultLst.add(r);
    }
    for(int i=l;i<s.length();i++){
        if(isPalindrome(s,l,i)){
            if(l==i)
                currLst.add(Character.toString(s.charAt(i)));
            else
                currLst.add(s.substring(l,i+1));
            backTrack(s,i+1);
            currLst.remove(currLst.size()-1);
        }
    }
}
public boolean isPalindrome(String str, int l, int r){
    if(l==r) return true;
    while(l<r){
        if(str.charAt(l)!=str.charAt(r)) return false;
        l++;r--;
    }
    return true;
}

From looking at your code, I'd think it would be \$O(n^2)\$ or \$O(n^3)\$. I got this by:

backTrack() is called n times
backTrack() has a for loop, which is executed m-n times
In that for loop, isPalindrome() is called, which has the complexity of \$O(\frac{r-l}2)\$

Putting it together, I get:

$$O(\frac{n(n+1)(2n+1)}6)$$

I am not sure if I'm right; correct me if I'm wrong.

Since 1, 2, 4, and 6 are not significant, we can remove them, to get:

$$O(n^3)$$

I can't think of a better algorithm, but maybe there is one. I will concentrate on the naming of your variables, and other conventions.

Formatting

All right, time to complain about how bad your formatting is, part by part...

List<List<String>> resultLst;
ArrayList<String> currLst;
public List<List<String>> partition(String s) {
    resultLst = new ArrayList<List<String>>();
    currLst = new ArrayList<String>();
    backTrack(s,0);
    return resultLst;
}
public void backTrack(String s, int l){
    if(currLst.size()>0 //the initial str could be palindrome
        && l>=s.length()){
            List<String> r = (ArrayList<String>) currLst.clone();
            resultLst.add(r);
    }
    for(int i=l;i<s.length();i++){
        if(isPalindrome(s,l,i)){
            if(l==i)
                currLst.add(Character.toString(s.charAt(i)));
            else
                currLst.add(s.substring(l,i+1));
            backTrack(s,i+1);
            currLst.remove(currLst.size()-1);
        }
    }
}
public boolean isPalindrome(String str, int l, int r){
    if(l==r) return true;
    while(l<r){
        if(str.charAt(l)!=str.charAt(r)) return false;
        l++;r--;
    }
    return true;
}

First of all, extra newline between methods. This allows easier readability.

Now, to the first section:

List<List<String>> resultLst;
ArrayList<String> currLst;

public List<List<String>> partition(String s) {
    resultLst = new ArrayList<List<String>>();
    currLst = new ArrayList<String>();
    backTrack(s,0); // Line 7
    return resultLst;
}

My suggestions:

Line 7: Space after comma

Well.. that was easy. Next...

public void backTrack(String s, int l){
    if(currLst.size()>0 //the initial str could be palindrome
        && l>=s.length()){
            List<String> r = (ArrayList<String>) currLst.clone();
            resultLst.add(r);
    }
    for(int i=l;i<s.length();i++){ // Line 7
        if(isPalindrome(s,l,i)){
            if(l==i)
                currLst.add(Character.toString(s.charAt(i)));
            else
                currLst.add(s.substring(l,i+1));
            backTrack(s,i+1);
            currLst.remove(currLst.size()-1);
        }
    }
}

Line 1: Space before brace

Line 2: Space after if
Line 2: Space before and after >, or any other operator, for that matter
Line 2: Usually, though optional, there is a space after // in end-of-line comments

Line 3: It is recommended to use two tabs, or 8 spaces, for a continued line.
Line 3: Again, space before and after all operators.

Line 4-5: Here, it should be 4 spaced.

Line 7: Space after for
Line 7: Space before and after operators, twice
Line 7: Space after semicolons
Line 7: Space before brace

Line 8: Space after if
Line 8: Space before brace
Line 8: Space after commas

Line 9: Space after if
Line 9: Space before and after operators
Line 9: ALWAYS put braced for if statements. I explain it here.

Line 11: Again, ALWAYS put braced for if statements, including else.
Line 11: Space after comma
Line 11: Space before and after operators

Line 12: Space before and after operators
Line 12: Space after comma

Line 13: Space before and after operators
Line 13: Space after comma

Line 14: Space before and after operators

Now that was quite a bit. Notice that most of my suggestions appear over and over and over again.

Next...

public boolean isPalindrome(String str, int l, int r){
    if(l==r) return true;
    while(l<r){
        if(str.charAt(l)!=str.charAt(r)) return false;
        l++;r--;
    }
    return true;
}

Line 1: Space before brace (hey, I just realised it rhymes!)

Line 2: Space after if
Line 2: Space before and after operators
Line 2: Here, I do see a reason why you made it a one-liner. I would usually not do so, but I wouldn't complain if I saw it.

Line 3: Space after while
Line 3: Space before and after operators
Line 3: Space before brace

Line 4: Space after if
Line 4: Space before and after operators
Line 4: Again, I do see a reason why you made it a one-liner.

Line 5: Separate into two lines. I would never put two statements on one line, no matter how related.

Done! Result after formatting:

List<List<String>> resultLst;
ArrayList<String> currLst;

public List<List<String>> partition(String s) {
    resultLst = new ArrayList<List<String>>();
    currLst = new ArrayList<String>();
    backTrack(s, 0);
    return resultLst;
}

public void backTrack(String s, int l) {
    if (currLst.size() > 0 // the initial str could be palindrome
            && l >= s.length()) {
        List<String> r = (ArrayList<String>) currLst.clone();
        resultLst.add(r);
    }
    for (int i = l; i < s.length(); i++) {
        if (isPalindrome(s, l, i)) {
            if (l == i) {
                currLst.add(Character.toString(s.charAt(i)));
            } else {
                currLst.add(s.substring(l, i + 1));
            }
            backTrack(s, i + 1);
            currLst.remove(currLst.size() - 1);
        }
    }
}

public boolean isPalindrome(String str, int l, int r) {
    if (l == r) return true;
    while (l < r) {
        if (str.charAt(l) != str.charAt(r)) return false;
        l++;
        r--;
    }
    return true;
}

Naming

Most of your variable names are not exactly good. Names say what the variable is for, and is not just there to identify a from b. I would suggest:

resultLst -> result
currLst -> currentList
resultLst -> result

In partition():

s -> string or toPartition

In backtrack():

s -> string or toPartition
l -> len or length
r -> list or toAdd

In isPalindrome():

str -> string
l -> from
r -> to

Result:

List<List<String>> result;
ArrayList<String> currentList;

public List<List<String>> partition(String string) {
    result = new ArrayList<List<String>>();
    currentList = new ArrayList<String>();
    backTrack(string, 0);
    return result;
}

public void backTrack(String string, int length) {
    if (currentList.size() > 0 // the initial str could be palindrome
            && length >= string.length()) {
        List<String> list = (ArrayList<String>) currentList.clone();
        result.add(list);
    }
    for (int i = length; i < string.length(); i++) {
        if (isPalindrome(string, length, i)) {
            if (length == i) {
                currentList.add(Character.toString(string.charAt(i)));
            } else {
                currentList.add(s.substring(length, i + 1));
            }
            backTrack(string, i + 1);
            currentList.remove(currentList.size() - 1);
        }
    }
}

public boolean isPalindrome(String string, int from, int to) {
    if (from == to) return true;
    while (from < to) {
        if (string.charAt(from) != string.charAt(to)) return false;
        from++;
        to--;
    }
    return true;
}

Misc

Declare all lists at List<Type>
result and currentList should be private


Controller method to validate and save I have built RESTful web services. The services endpoints have a similar logic:

If the input validation succeeds, do a database operation and return the result.
If the db operation fails, return the db error.
If the validation fails, return a response with information on the failed validation.

My code was explicit in the sense you can read the code and understand the flow of the program. A coworker refactored it so that this basic flow is refactored in a utility class which main function takes the request body, the db operation and the handlers. Now the code doesn't express the flow but the basic logic is refactored.

Which code better follows best practice? Would you sacrifice readability for refactoring? Yes, the unit test will be reduced a bit too.

Example:

Before:

save: function (req, res, next) {
 var dao = topicsDaoFactory.create(req.dbSession);

 validate(req.body.topic, constraintsObj,
   function success () {
     dao.save(req.body.topic, function saveHandler (err, result) {
       if (err) {
         res.error(err);
       } else {
         res.send(result);
       }
     });
   },
   function error (errors) {
     res.fail(errors);
   });
}

After:

save: function (req, res) {
  var dao = topicsDaoFactory.create(req.dbSession);

  ControllerUtils.validateAndTryOperation(req, res, {
    input: req.body.topic,
    constraints: constraintsObj,
    operation: function (input, callback) {
      dao.create(Topic.createFromScratch(input), callback);
    }
  });
}

What is the best practice? Would you sacrifice readability for refactoring?

That highly depends on the case. Most of the time I write code for readability. Most of my code reviews here focus more on readability. Only if there's sufficient proof that my code is a bottleneck do I actually refactor it.

So, yes, I prefer your old code. However, it could be done a bit better if you used Promises. It's longer, but avoids callback pyramid. There's a package called BlueBird that converts callback-style APIs into Promises.

save: function (req, res, next) {
  var dao = topicsDaoFactory.create(req.dbSession);

  // Try validating
  validate(req.body.topic, constrainsObj)
  // Listen for validate
  .then(function(){
    return dao.save(req.body.topic);
  }, function(error){
    res.error(error);
  })
  // Listen for dao.save
  .then(function(result){
    res.send(result);
  },function(error){
    res.fail(error);
  });

}


Simple JavaScript tip calculator I've built a fairly simple tip calculator. What I have so far is a bunch of functions to calculate each tip amount and calling each with eventListener.

It works however trying to see if there's a better approach that would modularize the code. I'm thinking possibilities include passing functions as arguments or functions calling other functions but can't figure it out yet.

Here's the code:

var tenPerBtn = document.querySelector('.tenPercent');
var fifteenPerBtn = document.querySelector('.fifteenPercent');
var twentyPerBtn = document.querySelector('.twentyPercent');
var customPerBtn = document.querySelector('.customTipBtn');
var bill = document.getElementById('billInput');
var tipSuggestion = document.getElementById('tipAmount');

function calcTen() {
  var billInput = bill.value;
  var tipAmount = billInput * .1;
  tipSuggestion.innerHTML = 'A ten percent tip would equal $' + tipAmount;
}

function calcFifteen() {
  var billInput = bill.value;
  var tipAmount = billInput * .15;
  tipSuggestion.innerHTML = 'A fifteen percent tip would equal $' + tipAmount;
}

function calcTwenty() {
  var billInput = bill.value;
  var tipAmount = billInput * .2;
  tipSuggestion.innerHTML = 'A twenty percent tip would equal $' + tipAmount;
}

function calcCustom() {
  var billInput = bill.value;
  var customTipAmount = document.querySelector('#customTip').value;
  var tipAmount = billInput * customTipAmount;
  tipSuggestion.innerHTML = 'A ' + customTipAmount + ' percent tip would equal $' + tipAmount;
}

tenPerBtn.addEventListener('click', calcTen)
fifteenPerBtn.addEventListener('click', calcFifteen)
twentyPerBtn.addEventListener('click', calcTwenty)
customPerBtn.addEventListener('click', calcCustom)
Bill Amount: <input type="text" id="billInput">

<button class="tenPercent">Tip 10%</button>

<button class="fifteenPercent">Tip 15%</button>

<button class="twentyPercent">Tip 20%</button>

Custom Tip Percentage: <input type="text" id="customTip"> <button class="customTipBtn">Tip Custom Amount</button>

Eliminate code duplication by using function parameters (numbers in function name is usually a bad smell )
Separate calculating/processing data and DOM manipulations

My suggested solution:

function calcTip(bill, percent) {
    return bill * percent / 100;
}

function getMessage(tip, percent) {
    return 'A ' + percent + ' percent tip would equal $' + tip;
}

function updateFunc(billInput, tipSuggestion) {
    // Here I use closure to save billInput and tipSuggestion
    return function(percent) {
        var bill = parseFloat(billInput.value) || 0;
        tipSuggestion.innerHTML = getMessage(calcTip(bill, percent), percent);
    }
}

var tenPerBtn = document.querySelector('.tenPercent');
var fifteenPerBtn = document.querySelector('.fifteenPercent');
var twentyPerBtn = document.querySelector('.twentyPercent');
var customPerBtn = document.querySelector('.customTipBtn');
var bill = document.getElementById('billInput');
var tipSuggestion = document.getElementById('tipAmount');

var update = updateFunc(bill, tipSuggestion);

tenPerBtn.addEventListener('click', function() { update(10); });
fifteenPerBtn.addEventListener('click', function() { update(15); });
twentyPerBtn.addEventListener('click', function() { update(20); });
customPerBtn.addEventListener('click', function() { update(document.querySelector('#customTip').value); });


Spotify API GET curl with auth using php - is this code good? $spotify_track = '1HbcclMpw0q2WDWpdGCKdS';
    $client_id = '[my_id]';
    $client_secret = '[my_secret]';

    $curl = curl_init();
    curl_setopt($curl, CURLOPT_URL,            'https://accounts.spotify.com/api/token' );
    curl_setopt($curl, CURLOPT_RETURNTRANSFER, 1 );
    curl_setopt($curl, CURLOPT_POST,           1 );
    curl_setopt($curl, CURLOPT_POSTFIELDS,     'grant_type=client_credentials' );
    curl_setopt($curl, CURLOPT_HTTPHEADER,     array('Authorization: Basic '.base64_encode($client_id.':'.$client_secret)));

    $response = curl_exec($curl);
    $token = json_decode($response)->access_token;
    $err = curl_error($curl);
    curl_close($curl);

    if ($err) {
      echo "cURL Error #:" . $err;
      curl_close($curl);
    } else {
          $curl = curl_init();
          curl_setopt_array($curl, array(
          CURLOPT_URL => "https://api.spotify.com/v1/audio-features/".$spotify_track,
          CURLOPT_RETURNTRANSFER => true,
          CURLOPT_FOLLOWLOCATION => true,
          CURLOPT_ENCODING => "",
          CURLOPT_MAXREDIRS => 10,
          CURLOPT_TIMEOUT => 30,
          CURLOPT_HTTP_VERSION => CURL_HTTP_VERSION_1_1,
          CURLOPT_CUSTOMREQUEST => "GET",
          CURLOPT_HTTPHEADER => array(
            "Content-Type: application/json",
            "Authorization: Bearer ".$token,
          ),
        ));
          $track_info = curl_exec($curl);
          var_dump($track_info);
          curl_close($curl);
    }

I've modified the code suggested on stackoverflow and came up with my own solution. I successfully fetch the song information. However, I wonder if this is the proper way in handling API using curl in php. I also noticed the page reloads a little bit slower compared to without the code aforementioned.

There's not much you can do wrong here. It works. I don't want to go into details of how to handle Spotify's API, but I would like to comment on your code.
One thing I noticed is that, if there's an error, you call curl_close() twice. That looks like a bug.
You're using curl two times, and in quite a different way. The first time you set individual options with curl_setopt(), the second time you combine all options and call curl_setopt_array(). Your code would be more consistent if you used the same method both times.
Better still, why not make a simple function to do the curl call for you? Something like this:
function callSpotifyApi($options)
{
    $curl  = curl_init();
    curl_setopt_array($curl, $options);
    $json  = curl_exec($curl);
    $error = curl_error($curl);
    curl_close($curl);
    if ($error) {
        return ['error'   => TRUE,
                'message' => $error];
    }
    $data  = json_decode($json);
    if (is_null($data)) {
        return ['error'   => TRUE,
                'message' => json_last_error_msg()];
    }
    return $data;
}

This function combines all the things that are common between the two curl calls, and therefore saves you writing code twice, and the reader reading it twice. This function also checks if the returned json code was valid and tells you what was wrong. You would normally use json_last_error() to detect json errors, but if a Spotify call returns null then there's surely an error.
Given the above function, the first curl call would now become:
$headers  = ['Authorization: Basic '.base64_encode($client_id.':'.$client_secret)];
$url      = 'https://accounts.spotify.com/api/token';
$options  = [CURLOPT_URL            => $url,
             CURLOPT_RETURNTRANSFER => TRUE,
             CURLOPT_POST           => TRUE,
             CURLOPT_POSTFIELDS     => 'grant_type=client_credentials',
             CURLOPT_HTTPHEADER     => $headers];
$credentials = callSpotifyApi($options);
$token = $credentials->access_token;

I haven't tested this code, so there might be errors. The second call will now be very similar:
$headers  = ['Content-Type: application/json',
             'Authorization: Bearer '.$token];
$url      = 'https://api.spotify.com/v1/audio-features/'.$spotify_track;
$options  = [CURLOPT_URL            => $url,
             CURLOPT_RETURNTRANSFER => TRUE,
             CURLOPT_FOLLOWLOCATION => TRUE,
             CURLOPT_ENCODING       => '',
             CURLOPT_MAXREDIRS      => 10,
             CURLOPT_TIMEOUT        => 30,
             CURLOPT_HTTP_VERSION   => CURL_HTTP_VERSION_1_1,
             CURLOPT_CUSTOMREQUEST  => 'GET',
             CURLOPT_HTTPHEADER     => $headers];
$features = callSpotifyApi($options);
var_dump($features);

Writing calls to Spotify consistently this way makes it easier to read the code, and find and correct bugs.
I didn't really review how you use the curl options, or whether this is the best way to use the Spotify API. Others may have more experience with this.


read an excel that contains population data of counties and compute summaries by state, county As part of the exercise in the book "Automate the boring stuff with Python" chapter 13 on Excel, I had the following exercise:

Reads the data from the Excel spreadsheet
Counts the number of census tracts in each county
Counts the total population of each county
Prints the results

Here is the structure of the spreadsheet

Here is my code and I would like to get reviews on how I can improve this code as I am quite new to Python.

from collections import Counter
import openpyxl

wb = xl.load_workbook("support files/censuspopdata.xlsx")
sheet = wb.active

#Create a unique list of all state names from the state column (which is 2nd column)
states = set()
for cell in list(sheet.columns)[1][2:]:
    states.add(cell.value)

#Initiate the countie_in_state dict to hold a set for each state key as it would be populated when the Excel is read row-by-row
counties_in_state ={}
for state in states:
    counties_in_state[state]=set()

# counter counts the number of census tracts(rows) and counter_pop holds the cumulative population
counter = Counter()
counter_pop = Counter()

#Skip the first row as it contains column names
for row in range(2,sheet.max_row+1):
    #Each row represents a census tract
    state  = sheet.cell(row,2).value
    county = sheet.cell(row,3).value
    pop = sheet.cell(row,4).value

    counties_in_state[state].add(county)
    counter[county] += 1
    counter_pop[county] += pop

# Sort the counties in each state
for state,county_set in counties_in_state.items():
    counties_in_state[state] = sorted(county_set)

print("State","County", "Count", "Pop", sep="\t")
for state,county_set in counties_in_state.items():
    for county in county_set:
        print(state, county, counter[county], counter_pop[county], sep="\t")

There are two ways in which you can change your code. The easiest one is to use a more powerful library, that can do more than just reading excel worksheets. For this I would recommend pandas, in which this task is very few lines:

import pandas as pd

df = pd.read_excel("support files/censuspopdata.xlsx")
df.groupby("County").agg({"State": "last", "CensusTract": "count", "POP2010": "sum"})

#               State  CensusTract  POP2010
# County
# San Francisco    CA            7    31060

But you can also improve your use of vanilla Python, by using set comprehensions, the standard library module collections and following Python's official style-guide, PEP8:

from collections import Counter, defaultdict
import openpyxl

wb = xl.load_workbook("support files/censuspopdata.xlsx")
sheet = wb.active

#Create a unique list of all state names from the state column (which is 2nd column)
states = {cell.value for cell in list(sheet.columns)[1][2:]}

#Initiate the countie_in_state dict to hold a set for each state key as it would be populated when the Excel is read row-by-row
counties_in_state = defaultdict(set)

# counter counts the number of census tracts(rows) and counter_pop holds the cumulative population
counter = Counter()
counter_pop = Counter()

#Skip the first row as it contains column names
for row in range(2, sheet.max_row+1):
    #Each row represents a census tract
    state  = sheet.cell(row, 2).value
    county = sheet.cell(row, 3).value
    pop = sheet.cell(row, 4).value

    counties_in_state[state].add(county)
    counter[county] += 1
    counter_pop[county] += pop

print("State","County", "Count", "Pop", sep="\t")
for state,county_set in counties_in_state.items():
    # Sort the counties in each state
    for county in sorted(county_set):
        print(state, county, counter[county], counter_pop[county], sep="\t")

You could put this code under a if __name__ == "__main__": guard to allow importing from this script and generalize this code so it gains an interface similar to the pandas one:

from collections import Counter, defaultdict
import openpyxl

def update_sum(old, x):
    return old + x

def update_count(old, x):
    return old + 1

def update_last(old, x):
    return x

FUNCS = {"sum": update_sum, "count": update_count, "last": update_last}

def groupby(sheet, key, **agg):
    assert agg, "Must give an aggregation function"
    rows = sheet.rows
    headers = [cell.value for cell in next(rows)]
    funcs = {name: FUNCS[func_name] for name, func_name in agg.items()}
    counters =  defaultdict(lambda: defaultdict(int))
    key_index = headers.index(key)
    assert key_index >= 0, "Key not found in data"
    for row in rows:
        row_key = row[key_index].value
        for i, (name, cell) in enumerate(zip(headers, row)):
            if name in agg:
                counters[row_key][name] = funcs[name](counters[row_key][name], cell.value)
    return dict(counters)

if __name__ == "__main__":
    wb = xl.load_workbook("support files/censuspopdata.xlsx")
    sheet = wb.active

    agg = {"State": "last", "POP2010": "sum", "CensusTract": "count"}
    for county, data in groupby(sheet, "County", **agg).items():
        print(data["State"], county, data["CensusTract"], data["POP2010"], sep="\t")
    # CA    San Francisco   7   31060

This is not the most flexible code, extending it to support e.g. a "mean" is quite hard. For this you would probably have to read the table into a different data structure, sort by the key and then use itertools.groupby. This way you can use any function that acts on the whole group:

from itertools import groupby
from operator import itemgetter

def first(x):
    return next(x)

def last(x):
    return list(x)[-1]

def count(x):
    return len(list(x))

def groupby_agg(sheet, *key, **agg):
    rows = sheet.rows
    headers = [cell.value for cell in next(rows)]
    data = [{col: cell.value for col, cell in zip(headers, row)}
            for row in rows]
    data.sort(key=itemgetter(*key))
    d = {}
    for name, group in groupby(data, key=itemgetter(*key)):
        group = list(group)
        d[name] = {col_name: func(map(itemgetter(col_name), group))
                   for col_name, func in agg.items()}
    return d

...
groupby_agg(sheet, "County", State=first, CensusTract=count, POP2010=sum)
# {'San Francisco': {'CensusTract': 7, 'POP2010': 31060, 'State': 'CA'}}

This has the advantage that you can also use the built-in sum, min, max, statistics.mean, .... It also allows using multiple keys, although it does not give you a multiply nested dictionary in this case:

groupby_agg(sheet, "State", "County", CensusTract=count, POP2010=sum)
# {('CA', 'San Francisco'): {'CensusTract': 7, 'POP2010': 31060}}


Checking whether one string is a permutation of another I solved the standard "write a method to determine if one string is a permutation of the other" question with the following code, using xor, map, and reduce:

from functools import reduce
from operator import xor

def checkPermutation(str1, str2):
    return not bool(reduce(xor, map(ord, str1 + str2)))

The idea is that if the two strings are permutations, the xor sum of the int value of every character of the concatenation of the two strings must be 0.

If I write the algorithm out by hand it seems come down to O(n) time and space:

def checkPermutation(str1, str2):
    # map every character in the strings to its integer value
    map = []
    str = str1 + str2
    for char in str:
        map.append(ord(char))

    # xor every number that was mapped into a sum
    sum = map[0]
    for i in range(1, len(map)):
        sum ^= map[i]

    return not bool(sum)

Essentially I think that's what reduce and map are doing but depending on how they're actually implemented in python, the time and space complexity of two solutions may differ.

For both strings, generate a dictionary containing distinct characters of the string as keys and their respective counts as values.

If the two dictionaries are equal, the strings are permutations of each other.

Instead of manually counting characters using a dict, you can utilize the collections.Counter class. To save time in case the strings are of different length, you can compare their lengths first.

from collections import Counter

def checkPermutation(str1, str2):
    return len(str1) == len(str2) and Counter(str1) == Counter(str2)

This is a \$O(n)\$ time and approximately \$O(1)\$ space solution. Approximately because – presumably – the less distinct characters there are in the input strings, the less space is occupied by the counters.


Project Euler problem #19: counting months that began on Sundays You are given the following information, but you may prefer to do some
  research for yourself.

  1 Jan 1900 was a Monday.
  Thirty days has September,
  April, June and November.
  All the rest have thirty-one,
  Saving February alone,
  Which has twenty-eight, rain or shine.
  And on leap years, twenty-nine.
  A leap year occurs on any year evenly divisible by 4, but not on a century unless it is divisible by 400.

  How many Sundays fell on the first of the month during the twentieth
  century (1 Jan 1901 to 31 Dec 2000)?

public static void problem_19() {
        int day = 4; // Lets number the days of week from 1:7 && SaturDay is the first day;;; note : 1 jan 1901 was Tue day
        int sundays = 0;
        for(int year = 1901; year <= 2000; year++)
            for(int mon = 1; mon <= 12; mon++) {
                day += days(mon, year);
                if(day%7 == 1)
                    sundays++;
            }
        System.out.println(sundays);
    }
    public static int days(int num, int year) {
        int res = 0;
        int feb = 28;
        if(isLeapYear(year))
            feb = 29;
        switch(num) {
            case 1 : res = 31; break;
            case 2 : res = feb; break;
            case 3 : res = 31; break;
            case 4 : res = 30; break;
            case 5 : res = 31; break;
            case 6 : res = 30; break;
            case 7 : res = 31; break;
            case 8 : res = 31; break;
            case 9 : res = 30; break;
            case 10 : res = 31; break;
            case 11 : res = 30; break;
            case 12 : res = 31; break;
        }
        return res;
    }
    public static boolean isLeapYear(int year) {
        if(year%4 == 0)
            if(year%100 == 0) {
                if(year%400 == 0)
                    return true;
            } else
                return true;
        return false;
    }

int day = 4; // Lets number the days of week from 1:7 && Sunday is the first day;;; note : 1 jan 1901 was Tue day

The comment gets lost on the right edge of this narrow screen.

The position of the comment implies that it is specific to the variable day, but that's not true of the first sentence.

For working with % it would be less confusing to use 0 to 6 rather than 1 to 7.

Given that day = 4, why does the comment tell us that 1 jan 1901 was Tue day? What does day represent here? If it's 1900-12-01 then the comment should talk about that date. In general, day is not a self-explanatory name.

            for(int mon = 1; mon <= 12; mon++) {

mon for Monday? Again, not the most helpful name.

                day += days(mon, year);
                if(day%7 == 1)
                    sundays++;

Is overflow of day a risk?

    public static int days(int num, int year) {
        int res = 0;
        int feb = 28;
        if(isLeapYear(year))
            feb = 29;
        switch(num) {
            case 1 : res = 31; break;
            case 2 : res = feb; break;
            ...
            case 12 : res = 31; break;
        }
        return res;
    }

num? That's completely uninformative. I know it's a number because I can see that its type is int.

Early returns would make this shorter and more obviously correct:

        switch (month) {
            case 1 : return 31;
            case 2 : return isLeapYear(year) ? 29 : 28;
            ...

It's bad practice to have a switch without a default, even if the only thing the default does is throw new Exception("This should be unreachable code").

One option would be to group the cases according to a well-known rhyme, and pull the sanity checking out of the switch:

        if (month < 1 || month > 12) throw new IllegalArgumentException("month");

        switch (month) {
            // 30 days have September, April, June and November...
            case 9:
            case 4:
            case 6:
            case 11:
                return 30;

            // ... All the rest have 31 ...
            default:
                return 31;

            // ... except for February alone.
            case 2:
                return isLeapYear(year) ? 29 : 28;
        }

    public static boolean isLeapYear(int year) {
        if(year%4 == 0)
            if(year%100 == 0) {
                if(year%400 == 0)
                    return true;
            } else
                return true;
        return false;
    }

Simplifying from the inside out, and putting in the necessary {} to be sure that the code does what we expect, we get:

    public static boolean isLeapYear(int year) {
        if (year % 4 == 0) {
            if (year % 100 == 0) {
                return year % 400 == 0;
            }
            return true;
        }
        return false;
    }

    public static boolean isLeapYear(int year) {
        if (year % 4 == 0) {
            return y % 100 > 0 || year % 400 == 0;
        }
        return false;
    }

    public static boolean isLeapYear(int year) {
        return year % 4 == 0 && (y % 100 > 0 || year % 400 == 0);
    }

    public static boolean isLeapYear(int year) {
        return (year % 4 == 0 && y % 100 > 0) || year % 400 == 0;
    }

Of course, given that we're only interested in years 1901 to 2000, this could be simplified further.


Cython with variable-length arrays Dynamically growing arrays are a type of array. They are very useful when you don't know the exact size of the array at design time. First you need to define an initial number of elements. (Wikipedia)

I have written a Python solution and converted it to Cython.
Cython can be used to improve the speed of nested for loops in Python.
Where my Cython code is slightly faster.
My Cython solution is obviously not the fastest.
I am trying to perform a nested for loop similar to the one in my Python code as fast as possible in Cython.

It would help to have some experience in C, which I don't.
The main problem that I ran into is that Cython has different scoping rules to Python.
Since C and Python have different scoping rules.
In other words, we cannot create a new vector in the loop and assign it to the same name.

My solution works but is too slow.
Can anyone improve Cython code above by using a more C like approach.
Python
import numpy as np

my_list = [1,2,3]
n = 10
a = 0.5

Estimate_1_list = []
Estimate_2_list = []

for l in my_list:

    # Resizable matrices
    a_mat = np.zeros((l,n+1),float)
    b_mat = np.zeros((l,n+1),float)

    for i  in range(n):
        t = i*a

        for j in range(l):

            # Fill matrices
            a_mat[j,i+1] = a_mat[j,i+1] + np.random.random()

            b_mat[j,i+1] = a_mat[j,i+1]/(2*t+3)

    # Append values of interest to use at different values of matrix size
    Estimate_1_list.append(np.mean(a_mat[:,n]))
    Estimate_2_list.append(np.std(a_mat[:,n]))
results = [Estimate_1_list,Estimate_2_list]

Cython
import cython
# Load cython extension
%load_ext Cython

%%cython
import numpy as np

def my_function(list my_list, int n, int a ):
cdef list Estimate_1_list = []
cdef list Estimate_2_list = []
cdef int l,i,t,j
for l in my_list:

    # Resizable matrices (could I use memory view?)
    a_mat = np.zeros((l,n+1),float)
    b_mat = np.zeros((l,n+1),float)

    for i  in range(n):
        t = i*a

        for j in range(l):

            # Fill matrices
            a_mat[j,i+1] = a_mat[j,i+1] + np.random.random()

            b_mat[j,i+1] = a_mat[j,i+1]/(2*t+3)

    # Append values of interest to use at different values of matrix size
    Estimate_1_list.append(np.mean(a_mat[:,n]))
    Estimate_2_list.append(np.std(a_mat[:,n]))

# Return results
results = [Estimate_1_list,Estimate_2_list]
return results

Tests
# Test cython to show that the function is running
my_list = [1,2,3]
n = 10
a = 0.5
my_function(my_list, n, a)

[[0.13545224609230933, 0.6603542545719762, 0.6632002117071227],
 [0.0, 0.19967544614685195, 0.22125180486616808]]

In addition to Reinderien's answer, may I suggest:
Yes, Use Memoryviews to speed up access
In addition to the code you have, I would also type the a_mat and b_mat matrixes as double[:,::1] following the Typed Memoryviews guide. (the "1" means contiguous and is allows for slightly faster access). You are right in that you can not cdef declare variables in a loop, but you can cdef declare variables at the top of the function, and reassign within the loop.

    ...
    cdef list Estimate_1_list = []
    cdef list Estimate_2_list = []
    cdef int l,i,t,j
    cdef double[:, ::1] a_mat # declare here
    cdef double[:, ::1] b_mat
    for l in my_list:

        # Resizable matrices (could I use memory view?)
        a_mat = np.zeros((l,n+1),float) # assign in loop
        b_mat = np.zeros((l,n+1),float)
    ...

Turn off Boundscheck And Wrap Around check
Once you are sure that code is bug free. You can locally turn off bounds and wraparound checks using the decorators or with statements for additional speed up. (Compiler Directives, Local Directives)


Speed up code based on map with vectors as values Following is the piece of code which represents the bottleneck of my application:

#include <iostream>
#include <chrono>
#include <unordered_map>
#include <vector>
#include <memory>
#include <cassert>
#include <thread>
#include <random>
#include <utility>
using namespace std;
using namespace chrono;

static const size_t len = 30;
static const size_t nGraphics = 0xfffff;
static const size_t nIndexes = 600000;

static default_random_engine generator;
static normal_distribution<float> distribution(len / 2.f, 2);

struct Graphic
{
    Graphic()
        : lnum ((short)distribution(generator))
    {
        // common scenario
        // most of my graphics elements belong to few layers
        assert(lnum < len);
    }

    short lnum;
    unsigned char data[200];
};

static vector<short> order;
static vector<unique_ptr<Graphic>> graphics;
static vector<size_t> indexes;

static void init()
{
    order.reserve(len);
    for (short i = 0; i < len; i++)
        order.push_back(i);

    graphics.reserve(nGraphics);
    for (size_t i = 0; i < nGraphics; i++)
        graphics.emplace_back(new Graphic);

    assert(nGraphics >= nIndexes);

    indexes.reserve(nIndexes);
    for (size_t i = 0; i < nIndexes; i++)
        indexes.push_back(i);
}

static bool is_selected(short lnum)
{
    // worst case scenario (everything has been selected)
    return true;
}

static bool is_valid(Graphic* g)
{
    // worst case scenario (everything is valid)
    return (g != nullptr);
}

static void benchmarks()
{
    unordered_map<short, vector<Graphic*>> selected;
    selected.reserve(len);

    for (size_t i = 0; i < len; i++)
    {
        const auto lnum = order[i];

        if (!is_selected(lnum))
            continue;

        selected.emplace(lnum, vector<Graphic*>());
    }

    for (const auto idx : indexes)
    {
        assert(idx < graphics.size());
        const auto g = graphics[idx].get();

        if (!is_valid(g))
            continue;

        const auto it = selected.find(g->lnum);

        if (it != selected.end())
            it->second.emplace_back(g);
    }

    for (size_t i = 0; i < len; i++)
    {
        const auto lnum = order[i];

        if (!is_selected(lnum))
            continue;

        assert(selected.find(lnum) != selected.end());

        for (const auto g : selected[lnum])
        {
            assert(g);

            if (g->lnum != lnum)
                throw runtime_error("error");

            // perform slow operation on g (third party library)...
            // must follow a specific order
        }

        // simulate operations
        this_thread::sleep_for(microseconds{ selected[lnum].size() });
    }
}

int main()
{
    init();

    const auto t0 = high_resolution_clock::now();
    benchmarks();
    const auto t1 = high_resolution_clock::now();
    const auto elapsed = duration_cast<milliseconds>(t1 - t0).count();
    cout << "Elapsed: " << elapsed << endl;
    cin.get();

    return 0;
}

Most of the work is done inside the third party library (the elapsed time well represents the actual application), but my code, used to prepare the data structure, last more than 50ms on my machine.

Is there any way to reduce this time?

You can use std::iota for the sequential order vector. If it doesnt increase speed it is prettier:

std::vector<short> order(len);
std::iota(order.begin(), order.end(), 0);

The same obviously goes for indexes
Given that you emplace in sequential order here selected.emplace(lnum, vector<Graphic*>()) you might want to switch to emplace_back
If you do not care about memory you can reserve the memory for your graphics vector with a guess during initialization and then call shrink_to_fit afterwards. That way you avoid possibly costly reallocations. However depending on the size of your guess this might significantly increase the memory burden.