Code for dividing gifts equally I recently came across this problem:

  It is Lavanya's birthday and several families have been invited for
  the birthday party. As is customary, all of them have brought gifts
  for Lavanya as well as her brother Nikhil. Since their friends are all
  of the erudite kind, everyone has brought a pair of books.
  Unfortunately, the gift givers did not clearly indicate which book in
  the pair is for Lavanya and which one is for Nikhil. Now it is up to
  their father to divide up these books between them.

  He has decided that from each of these pairs, one book will go to
  Lavanya and one to Nikhil. Moreover, since Nikhil is quite a keen
  observer of the value of gifts, the books have to be divided in such a
  manner that the total value of the books for Lavanya is as close as
  possible to total value of the books for Nikhil. Since Lavanya and
  Nikhil are kids, no book that has been gifted will have a value higher
  than 300 Rupees.

  Suppose there are 4 pairs of books whose cost in Rupees are:

  (3,5), (7,11), (8,8), (2,9)

  By giving the books worth 3,7,8 and 2 to Lavanya and the rest to
  Nikhil, the net difference in value would be 5+11+8+9-3-7-8-2 = 13.
  However, by giving books worth 3,7,8 and 9 to Lavanya and the rest to
  Nikhil, their father can ensure that the difference in values is just
  1. You can verify that you cannot do better than this.

  You task is to help their father decide how to divide the books.

My approach is take the difference of of every pair and arrange them in descending order, where one by one they are subtracted if the previous result was positive or added if previous result was negative. This greedy approach works for most of the test cases but gets stuck at some.

#include <iostream>
#include <algorithm>
#include <cmath>

int main (int argc, char const* argv[])
{
    int n;
    std::cin >> n;
    int difference[n];
    for(int i=0;i<n;i++){
        int a,b;
        std::cin>>a >>b;
        difference[i] = std::abs(a-b);
    }
    std::sort(difference , difference+n,std::greater<int>());
    int diff = difference[0];
    for(int i=1;i<n;i++){
        if(diff > 0){
            diff = diff - difference[i];
        }else{
            diff = diff + difference[i];
        }
        std::cout << diff << std::endl;
    }
    std::cout << std::abs(diff) << std::endl;
    return 0;
}

I have tried to implement recursion but it gets too complicated for me and often takes a lot more time for bigger test cases. Does anyone have any better approach for it?

Here are some testcases:

  TestCase 1: Input: 5 271 6 293 148 194 104 4 207 148 267

  Output: 2

  TestCase 2: Input: 30 130 83 207 104 115 33 149 20 51 128 78 256 246
  99 42 215 187 276 239 263 105 122 294 143 14 241 146 175 175 114 9 56
  248 267 212 62 299 60 82 49 187 159 56 184 9 150 99 195 177 89 157 282
  210 203 176 275 143 73 202 69

  Output: 1

  TestCase 3: Input: 50 185 294 180 119 96 18 82 75 26 25 120 55 40 230
  31 105 112 140 22 227 154 29 187 107 156 182 181 57 122 39 90 59 84
  269 177 180 287 258 6 64 34 126 171 74 107 201 230 218 93 4 197 246 32
  135 104 239 16 284 47 189 75 136 247 158 157 176 89 195 185 147 10 219
  272 180 44 130 133 273 100 225 276 296 222 59 130 25 49 197 60 96 85
  134 283 84 44 139 259 184 85 143

  Output: 0

Apparently this are the only testcases causing trouble.

Greedy solution not correct

First, we can transform the problem as OP did by taking the difference of the book values in each pair, and then trying to split these new values in to two equal halves.  In the example of (3,5), (7,11), (8,8), (2,9), this transforms into trying to split 2 4 0 7 into equal halves, which is achieved as closely as possible with the solution [0,2,4][7].

The greedy solution attempts to place the largest unused value on the smallest pile.  In the example above, the sorted values are 7 4 2 0, and the values are placed in the following way:

    Pile 1      Pile 2
    ------      ------
     7
     7           4
     7           4,2
     7           4,2,0

leading to the correct answer.

However, consider the case of 5 5 4 3 3.  In this case, the greedy method would place the values as follows:

    Pile 1      Pile 2
    ------      ------
     5
     5           5
     5,4         5
     5,4         5,3
     5,4         5,3,3

Ending with piles of size 9 and 11, with a difference of 2.  However, the optimal solution would be piles [5,5][4,3,3] with a difference of 0.

Dynamic Programming

Instead of a greedy method, you can use a dynamic programming method to solve the problem.  The algorithm will be as follows:

Transform the initial input by taking the absolute value of the differences between each pair of books.  This is the new list of values we will use.
Sum the list of values.  Call this sum total.  We are now trying to find a subset of the values that will sum as closely to total/2 as possible (without going over).  You can convince yourself that this subset will provide us with the best answer to the problem.
Create an array of booleans of size (total/2) + 1 and initialize to false, except for element arr[0] which you initialize to true.  Each element of this array contains whether it is possible to reach a sum of that index.  For example, if in the end, arr[15] is true, then it is possible to create a subset that sums to 15.
For each element in the list of values, you traverse the array, and whenever you find arr[j] to be true, you can set arr[j+val] to be true. Note: this traversal needs to start at arr[max-val] and search towards arr[0].  If you traverse in the forwards direction, the act of setting arr[j+val] to true will cause that array element to trigger later in the loop when it shouldn't.  For example, if val were 1, the forwards loop would mistakenly set every element in the array to true.
Once step 4 is finished, your array holds all the possible subsums.  Find the largest possible subsum, and that will be the value of one pile.  Call that value largestSum.  The other pile will have value total - largestSum.  The difference will be total - 2*largestSum, which is the answer to the question.

This approach will have complexity \$O(n * m)\$, where \$n\$ is the number of book pairs and \$m\$ is the total of all the differences.  In the problem description, there are max 150 books with max value 300, so the maximum value for \$m\$ is 45000.

Compare this to the recursive solution which tries to place each value in each pile, which has complexity \$O(2^n)\$.

Sample implementation using dynamic programming

I took your program and adjusted it to use the dynamic programming approach described above.  Here is the result:

#include <iostream>
#include <algorithm>
#include <cmath>

int main (int argc, char const* argv[])
{
    int n;
    std::cin >> n;
    int difference[n];
    int total = 0;
    for(int i=0;i<n;i++){
        int a,b;
        std::cin>>a >>b;
        difference[i] = std::abs(a-b);
        total += difference[i];
    }

    // Find each reachable sum up to total/2.
    int  max = total/2;
    bool arr[max+1] = { true };
    for (int i = 0; i < n; i++) {
        int val = difference[i];
        for (int j = max - val; j >= 0; j--) {
            if (arr[j])
                arr[j+val] = true;
        }
    }

    // Find the max reachable sum.  For that sum, print the difference between
    // one child's sum (i) and the other's (total - i).  Since i is always
    // less than or equal to (total - i), the difference will be total - i - i.
    for (int i = max; i >= 0; i--) {
        if (arr[i]) {
            std::cout << total - i - i << std::endl;
            break;
        }
    }
    return 0;
}


Make an image appear on scroll event I recently was playing around with the idea of having an image appear when the user scrolls and came up with the following. Is there a better or more efficient way to do it?

var pastScroll;
jQuery(document).scroll(function () {
    var currentScroll = jQuery(document).scrollTop();
    if (pastScroll == currentScroll) {
        //console.log("not moving");
        jQuery('#content').css('background-image', 'url("/onScroll/blank.png")');
        // clear background
    }
    else {
        jQuery('#content').css('background-image', 'url("/onScroll/logo.png")');
        jQuery('#content').css('background-repeat','no-repeat');
        jQuery('#content').css('background-position', 'center');
        //console.log('moving');
        pastScroll = currentScroll;
        setTimeout(function () {
            if (pastScroll == currentScroll) {
                //console.log("not moving");
                jQuery('#content').css('background-image', 'url("/onScroll/blank.png")');
            }
        }, 100)
    }
});

You can ignore the use of jQuery instead of $ because it's being used in wordpress and ignore the css style added. I'm just wanting to focus on the conditionals and triggering. Any constructive criticism is appreciated, thanks.

Use CSS to change the background image of an element. It's easy and safe than using css() to change the styles of element from JavaScript. This also keep separation of concerns. Tomorrow, if you want to change the image size, you just have to go to the CSS and update the particular CSS class.

Also, note that the three properties background-image, background-repeat and background-position can be combined into background property.

CSS:

.logo {
    background: url('/onScroll/logo.png') center no-repeat;
}
.blank {
    background: url('/onScroll/blank.png');
}

JavaScript:

$('#content')
    .addClass('logo')       // Add class
    .removeClass('blank');  // Remove class

Wrap the code in IIFE to avoid cluttering global namespace by creating global variables. Also, passing the variables to IIFE will create local alias reducing scope chain operations.

(function ($, document) {
     // CODE HERE
     ...
}(jQuery, document));

Note that jQuery is passed as first parameter and catch-ed in $. Less typing. More readable.
Cache the element references. By doing this, the elements are not needed to be taken from DOM again. Thus improving performance.

// Caching
var $document = $(document),
    $content = $('#content');

Use strict equality operators for comparison. See Which equals operator (== vs ===) should be used in JavaScript comparisons?

if (pastScroll === currentScroll) {

Chaining. jQuery provides feature to chain multiple calls on same elements.

jQuery('#content').css('background-image', 'url("/onScroll/logo.png")');
jQuery('#content').css('background-repeat','no-repeat');
jQuery('#content').css('background-position', 'center');

Instead of this, multiple calls can be chained

jQuery('#content').css('background-image', 'url("/onScroll/logo.png")')
    .css('background-repeat', 'no-repeat')
    .css('background-position', 'center');

OR better, jQuery allows to pass an object to css() where multiple styles can be set in one call.

jQuery('#content').css({
    'background-image': 'url("/onScroll/logo.png")',
    'background-repeat': 'no-repeat',
    'background-position': 'center'
});

Better way is to use CSS classes as explained in #1 above.
Clearing previously set timeouts, if any.

As setTimeout is used, if user keeps scrolling continuously the timeout callback will be executed multiple number of times. This is not really required. You can use clearTimeout to clear previously set timeouts, thus only one timeout will be executed once user has stopped scrolling and 100 ms is passed.

Complete Code:

CSS:

.logo {
    background: url('/onScroll/logo.png') center no-repeat;
}
.blank {
    background: url('/onScroll/blank.png');
}

JavaScript:

// Wrap the code in IIFE
(function ($, document) {
    'use strict';

    // $        = an alias for jQuery
    // document = alias for document object

    // Private variables
    var pastScroll,
        timeout;

    // Caching
    var $document = $(document),
        $content = $('#content');

    $document.scroll(function () {
        var currentScroll = $document.scrollTop();

        // Use strict equality operator
        if (pastScroll === currentScroll) {
            // For multiple method calls on same element, use chaining
            // Use CSS class to set the background image
            $content.addClass('blank').removeClass('logo');
        } else {
            $content.addClass('logo').removeClass('blank');

            pastScroll = currentScroll;

            // Clear previous timeouts
            clearTimeout(timeout);
            timeout = setTimeout(function () {
                if (pastScroll === currentScroll) {
                    $content.addClass('blank').removeClass('logo');
                }
            }, 100);
        }
    });
}(jQuery, document));


Merge two sorted linked lists without recursion Is this an effective way for merging two lists? It seemed too long.

struct Node {
    int item;
    Node *next;
};

Method:

Node* mergeTwo(Node* headA, Node* headB)
{
Node* currentA = headA;
Node* currentB = headB;

Node* headC = NULL;
Node* currentC = NULL;

while (currentA != NULL && currentB != NULL)
{
    if (currentA->item < currentB->item)
    {
        Node* node = new Node();
        node->item = currentA->item;
        node->next = NULL;

        if (headC == NULL)
            headC = node;
        else
            currentC->next = node;

        // update currents
        currentC = node;
        currentA = currentA->next;
    }
    else // currentB->item < currentA->item
    {
        Node* node = new Node();
        node->item = currentB->item;
        node->next = NULL;

        if (headC == NULL)
            headC = node;
        else
            currentC->next = node;

        // update currents
        currentC = node;
        currentB = currentB->next;
    }
}

// one of currentA or currentB will not be NULL
while (currentA != NULL)
{
    Node* node = new Node();
    node->item = currentA->item;
    node->next = NULL;

    // in case like: A={13,15}, B={4,6}
    if (headC == NULL)
        headC = node;
    else
        currentC->next = node;

    // update currents
    currentC = node;
    currentA = currentA->next;
}

while (currentB != NULL)
{
    Node* node = new Node();
    node->item = currentB->item;
    node->next = NULL;

    // in case like: A={3,5}, B={7,8}
    if (headC == NULL)
        headC = node;
    else
        currentC->next = node;

    // update currents
    currentC = node;
    currentB = currentB->next;
}
return headC;
}

You're duplicating the code to copy and merge nodes four times - that can't be good. You could move that into a separate function, but that might become messy because of all the state that has to be changed.

An alternative is to rethink your loop a bit. This idea is to loop while either A or B still have elements to avoid the two other whiles in your code. The internal logic can then be simplified to something like:

while ((currentA != nullptr) || (currentB != nullptr)) {
  int next_item;
  if (currentA == nullptr) {
    // A is empty, pick from B
    next_item = currentB->item;
    currentB = currentB->next;
  } else if (currentB == nullptr) {
    // B is empty, pick from A
    next_item = currentA->item;
    currentA = currentA->next;
  } else if (currentA->item < currentB->item) {
    next_item = currentA->item;
    currentA = currentA->next;
  } else {
    next_item = currentB->item;
    currentB = currentB->next;
  }
  // append to list C
}

Still some duplication, but at least it's just two-line blocks without any logic (and no memory allocations).

Also this should be simplified:

Node* node = new Node();
node->item = currentB->item;
node->next = NULL;

Provide a constructor for your node class:

explicit Node(int item) : item{item}, next{nullptr} {} // >= C++11
explicit Node(int item) : item(item), next(NULL) {}    // old C++

(Add a destructor while you're at it, or use smart pointers to avoid leaks.)

Then you can write:

Node* node = new Node(next_item);
if (headC == nullptr) { // or if (!head) { ... }
    headC = node;
} else {
    currentC->next = node;
}
currentC = node;

Last point: make sure you actually need to duplicate the elements. You could avoid all the allocations by re-using the nodes, if the input lists can be trashed (which is usually the case for a merge sort).


Performing redirects based on taxonomy terms associated with nodes This code performs 301 redirects based on the taxonomy terms associated with the node, as 301 redirects can adversely effect page load time. How could this be best optimized?

 <?php
    /**
     * Implementation of hook_init().
     *calling this hook causes the code below to execute before the page is loaded
     */

    function region_filter_init() {}
     //hook "nodeapi" is called when we are doing something to the node, in this "case" we are "viewing" the node.

    function region_filter_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL) {

      switch ($op) {

        case 'view':

            if( (arg(1) == 0)){ } //do not execute on homepage. if there are no aruments after the base url in arument one is thedelimagazine.com/argument1, if this is 0 then do nothing.

          else {

            $region = getRegion(); //get current region from deli.mmodule

            $national = 'national'; //store string 'national' in variable

            $count = count($node->taxonomy);//nstores number of key/value pairs in array,  if two terms are selected with the node/post then count == 2

            $terms = array();//initialize terms to array
            foreach($node->taxonomy as $term) {
              $terms[$term->name] = $term->name; //prints array of individual terms

            }

        /*case 1 new england*/

        //if there are less than 9 terms  and more than 2 and new england is in the array redirect to new england
        if( $count < 9 &&  $count > 2 && in_array('newengland', $terms) && $region != 'newengland')
        {
          $link = 'http://newengland.thedelimagazine.com/' . drupal_get_path_alias("node/$node->nid", $path_language = '');
              drupal_goto($link, $query = NULL, $fragment = NULL, $http_response_code = 301);
        }
        /*END case 1 new england*/

          /*case 2*/
              if($count == 2 && in_array('national', $terms) && is_numeric(arg(1))) { //if there are 2 terms in taxonomy && national is one of them & the first argument in url is a number such as thedelimagazine.com/12345 (before alias redirect)
              unset($terms['national']);        //remove national from array
              $value = array_shift($terms) ;//set value to the other term left. in array
              if($value == 'los angeles') {//fix for los angelas, because in deli module taxonomy term and subdomain are different only for los angeles
                $value = 'la';
              }

              if($region != $value) { //if region does not already = term (would cause redirect loop)

                  $link = 'http://' . $value . '.thedelimagazine.com/' . drupal_get_path_alias("node/$node->nid", $path_language = '');//go to term left in $value

                  drupal_goto($link, $query = NULL, $fragment = NULL, $http_response_code = 301);//drupal redirect function
              }
            }

            /* ++ END case 2++*/

            /* ++case 3 national+++*/
            else {

        if($count > 10  && in_array('national', $terms)  && $region != 'national' ) {    //if there are moer than 10 terms and national is in the array $terms, and the current region does not already =national
              $link = 'http://national.thedelimagazine.com/' . drupal_get_path_alias("node/$node->nid", $path_language = '');//then set $link to national.thedelimagazine.com.  drupl get path alias gets the path to the node and node id and converts it to clean readable urls that i implemented last year.
              drupal_goto($link, $query = NULL, $fragment = NULL, $http_response_code = 301);     //go to the $link
        }
             /* ++ END case 3 national++*/

             }
         }
          break;
      }
    }

function region_filter_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL) {

      switch ($op) {

        case 'view':

            if( (arg(1) == 0)){ } //do not execute on homepage. if there are no aruments after the base url in arument one is thedelimagazine.com/argument1, if this is 0 then do nothing.

You get an extra pair of parens, and it would be easier to to check for arg(1) ~= 0 rathern the have an empty if block.

          else {

            $region = getRegion(); //get current region from deli.mmodule

            $national = 'national'; //store string 'national' in variable

Don't write dumb comments. Assume the reader understands PHP. Your comment should explain why you want to do that, not reiterate the code.

            $count = count($node->taxonomy);//nstores number of key/value pairs in array,  if two terms are selected with the node/post then count == 2

            $terms = array();//initialize terms to array
            foreach($node->taxonomy as $term) {
              $terms[$term->name] = $term->name; //prints array of individual terms

            }

That was a lot of pointless empty lines.

        /*case 1 new england*/

        //if there are less than 9 terms  and more than 2 and new england is in the array redirect to new england
        if( $count < 9 &&  $count > 2 && in_array('newengland', $terms) && $region != 'newengland')

Again, your comment simply tells me what I could have already gotton from the code. But I'm left thinking: WHAT? What's the deal with the number of terms? What am I checking in the array? What has $region got to do with it?

        {
          $link = 'http://newengland.thedelimagazine.com/' . drupal_get_path_alias("node/$node->nid", $path_language = '');
              drupal_goto($link, $query = NULL, $fragment = NULL, $http_response_code = 301);

Keep your indentation consistent.
            }
            /END case 1 new england/

Don't put end comment like that. I can already see the case is ending thanks to the } and your indentation.

          /*case 2*/
              if($count == 2 && in_array('national', $terms) && is_numeric(arg(1))) { //if there are 2 terms in taxonomy && national is one of them & the first argument in url is a number such as thedelimagazine.com/12345 (before alias redirect)

Again, don't repeat your code in the comments. Explain why your code is doing that.

              unset($terms['national']);        //remove national from array
              $value = array_shift($terms) ;//set value to the other term left. in array
              if($value == 'los angeles') {//fix for los angelas, because in deli module taxonomy term and subdomain are different only for los angeles

See those are the comments that are helpful, because it explains what so special about los agenles.

                $value = 'la';
              }

              if($region != $value) { //if region does not already = term (would cause redirect loop)

Here I'd just say // avoid redirect loop

                  $link = 'http://' . $value . '.thedelimagazine.com/' . drupal_get_path_alias("node/$node->nid", $path_language = '');//go to term left in $value

                  drupal_goto($link, $query = NULL, $fragment = NULL, $http_response_code = 301);//drupal redirect function

This looks pretty much the same as the last case. You should refactor the code.
                  }
                }

            /* ++ END case 2++*/

            /* ++case 3 national+++*/
            else {

        if($count > 10  && in_array('national', $terms)  && $region != 'national' ) {    //if there are moer than 10 terms and national is in the array $terms, and the current region does not already =national
              $link = 'http://national.thedelimagazine.com/' . drupal_get_path_alias("node/$node->nid", $path_language = '');//then set $link to national.thedelimagazine.com.  drupl get path alias gets the path to the node and node id and converts it to clean readable urls that i implemented last year.
              drupal_goto($link, $query = NULL, $fragment = NULL, $http_response_code = 301);     //go to the $link
        }
             /* ++ END case 3 national++*/

             }
         }
          break;
      }
    }

It's been so long so I read the matching pieces of code that I have no idea what these pieces much up to. That a sign that your code is overly complicated.

Here is a quick reworking of the code:

<?php
    // return the region that the user should be on for this node
    function determine_region($node)
    {

        $taxonomy_count = count($node->taxonomy);

        // I doubt this is neccesary, I don't know what
        // $node->taxonomy is, but perhaps you can use that rather
        // then copying the data into an array.
        $terms = array();
        foreach($node->taxonomy as $term) {
            $terms[$term->name] = $term->name;
        }

        // I really have no idea what the logic behind the region is
        if($taxonomy_count == 2 && in_array('national', $terms) && is_numeric(arg(1)))
        {
            $region = array_shift($terms);
            // los ageneles needs special case
            if($region == "los angeles")
            {
                $region = 'la';
            }
            return $region;
        }
        else if($taxonomy_count > 2 && $taxonomy_count < 9 && in_array('newengland', $terms)
        {
            return 'newengland';
        }
        else if($taxonomy_count > 10 &&  in_array('national', $terms) )
        {
            return 'national';
        }
        else
        {
            // none of the cases fit, we'll just keep the current region
            return getRegion();
        }

    }

    // redirect the user to a particular region and node
    function send_redirect($region, $node)
    {
        $link = 'http://' . $region . '.thedelimagazine.com/' . drupal_get_path_alias("node/$node->nid", $path_language = '');//go to term left in $value
        drupal_goto($link, $query = NULL, $fragment = NULL, $http_response_code = 301);
    }

    function region_filter_nodeapi(&$node, $op, $a3 = NULL, $a4 = NULL) {
        // if viewing something other then the home page, consider redirecting
        if($op == 'view' && arg(1) != 0)
        {
            $correct_region = determine_region($node);
            // redirect to correct region if necessary.
            if( getRegion() != $correct_region )
            {
                send_redirect($correct_region, $node);
            }
        }
    }
?>

I think you'll find my version easier to follow. I've divided to task into pieces so that each piece can be simpler.

  This code performs 301 redirects based on the taxonomy terms
  associated with the node, as 301 redirects can adversely effect page
  load time, how could this be best optimized.

The solution is not to do 301 redirects. Telling the browser to go download another page will make loading the page slower. Nothing you can do in your code will change that. All you can do is not tell them to go download another page. Do you really need to redirect the user to a different subdomain? Maybe you do, but if you can get away without doing it, don't do it.


Javascript Slider that controls TinySlider - feels inefficient I feel like the main point of inefficiency is how I create "notches" and must loop through all of the created notches every time the value for the slider is changed.

function sliderLoop() {
    for (var i = slideNotchArray.length - 1; i >= 0; i--) {
      if ($(".slider").val() >= slideNotchArray[i].value) {
        console.log(slideNotchArray[i].slide);
        teamSlider.goTo(slideNotchArray[i].slide);
        break;
      }
    }
 }

It's an exponential nightmare as each call can loop through every notch object, and there can be multiple calls a second. This is the only solution I could have thought of though, and I am more than welcome to someone else's suggestions. I attempted to mitigate the severity by including a break once the first notch has been found, as to not loop through the rest, but I feel as though it is still quite heavy.

https://codesandbox.io/s/currying-bush-h3xyz

My major concerns are in relation to

  function sliderLoop() {
    for (var i = slideNotchArray.length - 1; i >= 0; i--) {
      if ($(".slider").val() >= slideNotchArray[i].value) {
        console.log(slideNotchArray[i].slide);
        teamSlider.goTo(slideNotchArray[i].slide);
        break;
      }
    }
  }

  // Fires slider loop every time the value changes, I had to use mousemove as .change() would only fire once the mouse click was relseased.
  $(".slider").on("change mousemove", function() {
    sliderLoop();
  });

The "change mousemove" event listener, calls sliderLoop multiple times a second. sliderLoop() contains a for loop, which could potentially iterate through many (10+) array index's. Calling a for loop which iterates through so many index's multiple times a second makes me believe this is an inefficient method of going about a slider navigation, however, I am at a loss as to how to improve the code while maintaining the functionality. Would you believe this to be an issue worth worrying about? If so, how could I go about improving this?

A technique that would likely improve this situation is a debounced function. That way the function to be run will only run at certain intervals instead of each time the events occur.

From the Blog of JavaScript consultant David Walsh:

  For those of you who don't know what a debounce function does, it limits the rate at which a function can fire. A quick example:  you have a resize listener on the window which does some element dimension calculations and (possibly)  repositions a few elements.  That isn't a heavy task in itself but being repeatedly fired after numerous resizes will really slow your site down.  Why not limit the rate at which the function can fire?

  Here's the basic JavaScript debounce function (as taken from Underscore.js):

// Returns a function, that, as long as it continues to be invoked, will not
// be triggered. The function will be called after it stops being called for
// N milliseconds. If `immediate` is passed, trigger the function on the
// leading edge, instead of the trailing.
function debounce(func, wait, immediate) {
    var timeout;
    return function() {
        var context = this, args = arguments;
        var later = function() {
            timeout = null;
            if (!immediate) func.apply(context, args);
        };
        var callNow = immediate && !timeout;
        clearTimeout(timeout);
        timeout = setTimeout(later, wait);
        if (callNow) func.apply(context, args);
    };
};

  1

That can be used to create a debounced function that only gets called at an interval - e.g. 250 milliseconds (feel free to adjust to your needs):

var debouncedFunc = debounce(sliderLoop, 250);

And use that function in the callback of the event handler:

// Fires slider loop every time the value changes, I had to use mousemove as .change() would only fire once the mouse click was relseased.
$(".slider").on("change mousemove", function() {
  debouncedFunc();
});

That could also be simplified to the following, since the extra lambda function/closure is pointless:

// Fires slider loop every time the value changes, I had to use mousemove as .change() would only fire once the mouse click was relseased.
$(".slider").on("change mousemove", debouncedFunc);

1https://davidwalsh.name/javascript-debounce-function


Read lines from file, calling a consumer function for each line I wrote a function for iterating over a large file with performance in mind. It takes in an InputStream and reads until it reaches the end of the file. Whenever it detects an end of line it will call the UnsafeConsumer#accept with the line.

UnsafeConsumer is the same as Consumer<T> but has a throws declaration on accept function
stream has to be closed outside the function

I am am not sure if this is the fastest way to do this. Is there anything different that I could do?

public static void fileLines(InputStream stream, UnsafeConsumer<String,IOException> cons) throws IOException{
    StringBuilder lineBuild=new StringBuilder();
    int character;

    while(true){ //read file
        while(true){ //read line
            character=stream.read();

            if(character==-1){
                cons.accept(lineBuild.toString());
                lineBuild.setLength(0);
                return;
            }
            if(character=='\n'){
                cons.accept(lineBuild.toString());
                lineBuild.setLength(0);

                character=stream.read();

                if(character==-1) return;
                else if(character!='\r'&&character!='\n') lineBuild.append((char)character);
                break;
            }
            lineBuild.append((char)character);
        }
    }
}

You are conflating bytes with characters.  An InputStream produces bytes, but you implicitly interpret each byte as a character when you do character=stream.read() followed by the cast lineBuild.append((char)character).  To be clear:

Strings consist of chars, not bytes.
InputStreams and OutputStreams deal with bytes.
Readers and Writers deal with characters.

To read lines as strings, first convert stream to a Reader using InputStreamReader.

BufferedReader br = new BufferedReader(new InputStreamReader(stream, "ISO-8859-1"));

Then, BufferedReader offers a .readLine() method.  Or, since you seem to be interested in a stream-based approach, call .lines(), which produces a Stream<String>.


Oauth 2.0 handler functions for Tornado With Tornado 3.2 they made some updates to auth module and have updated the code. Earlier I was using open id for Google login, since it will be deprecated in the future I am switching the code to Oauth 2.0 login. Also I was using tornado.web.asynchronous, now I have updated the code to use tornado.gen.coroutine

/products is only available for logged in users. Once the user is authenticated from Google or Facebook, a cookie called 'trakr' is set. In implementation of get_current_user() whether 'trakr' is set or not is checked.

class BaseHandler(tornado.web.RequestHandler):
    def get_current_user(self):
        user = self.get_secure_cookie('trakr')
        if not user: return None
        return True

class FAuthLoginHandler(BaseHandler, tornado.auth.FacebookGraphMixin):
    @tornado.gen.coroutine
    def get(self):
        if self.get_current_user():
            self.redirect('/products')
            return

        if self.get_argument('code', None):
            user = yield self.get_authenticated_user(
                redirect_uri=settings.fb_redirect_url,
                client_id=self.settings['facebook_api_key'],
                client_secret=self.settings['facebook_secret'],
                code=self.get_argument('code'))

            if not user:
                self.clear_all_cookies()
                raise tornado.web.HTTPError(500, 'Facebook authentication failed')

            user = yield self.facebook_request("/me", access_token=user["access_token"])

            if not user:
                raise tornado.web.HTTPError(500, "Facebook authentication failed.")

            self.set_secure_cookie('trakr', user.get('email'))
            self.redirect('/products')
            return

        elif self.get_secure_cookie('trakr'):
            self.redirect('/products')
            return

        else:
            yield self.authorize_redirect(
                redirect_uri=settings.fb_redirect_url,
                client_id=self.settings['facebook_api_key'],
                extra_params={'scope': 'email'}
            )

class GAuthLoginHandler(BaseHandler, tornado.auth.GoogleOAuth2Mixin):
    @tornado.gen.coroutine
    def get(self):
        if self.get_current_user():
            self.redirect('/products')
            return

        if self.get_argument('code', False):
            user = yield self.get_authenticated_user(redirect_uri=settings.google_redirect_url,
                code=self.get_argument('code'))
            if not user:
                self.clear_all_cookies()
                raise tornado.web.HTTPError(500, 'Google authentication failed')

            access_token = str(user['access_token'])
            http_client = self.get_auth_http_client()
            response =  yield http_client.fetch('https://www.googleapis.com/oauth2/v1/userinfo?access_token='+access_token)
            user = json.loads(response.body)

            self.set_secure_cookie('trakr', user['email'])
            self.redirect('/products')
            return

        elif self.get_secure_cookie('trakr'):
            self.redirect('/products')
            return

        else:
            yield self.authorize_redirect(
                redirect_uri=settings.google_redirect_url,
                client_id=self.settings['google_oauth']['key'],
                scope=['email'],
                response_type='code',
                extra_params={'approval_prompt': 'auto'})

There are a few small improvements I can see:

Your get_current_user() function is slightly misleading. Currently, you return None if there is no current user, or True if there is. This function isn't returning the current user, its returning if there is a current user.

If I assume the return value of get_secure_cookie() is a basic string, the method can actually be reduced to this single line:

# On a valid return, the return value will be a non-empty string == True.
# Otherwise, it will either be None or '', both of which == False.
return self.get_secure_cookie('trakr')

Since, this function is just passes through of the return value of another function, it really isn't necessary. Unless you wanted to keep it to describe what the return value of self.get_secure_cookie() could mean, I would suggest removing it entirely.
In your get() functions you have this if structure:

if get_current_user():
    self.redirect('/product')
    return

followed by:

if self.get_argument('code', None):
    ...
elif self.get_secure_cookie('trackr'):
    self.redirect('/products')
    return

First you check for a current user; if there is none you continue on. Then, if you get a False return value from self.get_argument('code', None), you immediately check again. From what I can tell from your snippet, unless you expect a user to get logged in during the time it takes to evaluate two if statements, the second user check is redundant.
I would change your naming conventions a bit. The G and F at the beginnings of your classes can be understood when looking at the source code. However, away from the source the F and G can be ambiguous.

I would recommend using the names: FacebookLoginHandler and GoogleLoginHandler.

Also, instead of trackr I would use the full work tracker. Saving 1 character isn't worth it.
I would use str.format() instead of string concatenation. You only do this once and with only two strings. However, string formatting is more Pythonic and it, personally, describes better what the final string will look like:

'Hello' + ' ' + 'world' + '!'   vs.   '{} {}!'.format('Hello', 'world')

My final point is more personal preference than a sure-fire Python convention. Whenever you have to split up a single statement onto multiple lines, you indent the next line 4 spaces in. As alluded to earlier, this fine:

yield self.authorize_redirect(
    redirect_uri=settings.fb_redirect_url,
    client_id=self.settings['facebook_api_key'],
    extra_params={'scope': 'email'}
)

  As a small aside, I don't like having the ending param on a line by itself; its too C/Java-like.

However, my personal preference is to indent them to line up (or as close to in line) with the section of the statement they are related to. Take this 'simple' example:

# Technically OK.
some_long_variable_name = [foobar for foobar in map(ord, 'Hello World!')
    if foobar < 97]

# My preference.
some_long_variable_name = [foobar for foobar in map(ord, 'Hello World!')
                           if foobar < 97]

In the first statement above, its slightly difficult to tell at a glance the if statement is in the list comprehension. Even though they are related syntactically, they currently aren't related spatially.

Now in the second statement, the if statement and the rest of the comprehension are related spatially which, personally, helps the code make more sense at a glance.

Here is how I would structure your code:

yield self.authorize_redirect(redirect_uri=settings.fb_redirect_url,
                              client_id=self.settings['facebook_api_key'],
                              extra_params={'scope': 'email'})

Otherwise, your code looks pretty good. I really wanted to look more into a single Login class because your two classes are so similar. However, a solution did not come quickly to mind and creating one would go past the bounds of typical code review.


Storing values from checkboxes in database I'm getting values from 2 checkboxes and save those values to a database.

I don't like these if statements. How is it possible to simplify the code?

def edit_user(request, user_id):

user = User.objects.get(id=user_id)

if request.POST:

    if request.POST.get('is_staff'):
        user.is_staff = True
    else:
        user.is_staff = False

    if request.POST.get('is_active'):
        user.is_active = True
    else:
        user.is_active = False

    user.save()

    return HttpResponseRedirect(request.META.get('HTTP_REFERER', '/'))

You can use the default parameter from the get method.
Reference

if request.POST:
        user.is_staff = request.POST.get('is_staff', False)
        user.is_active = request.POST.get('is_active', False)
        user.save()


Program to determine ranges of char, short, int and long variables, both signed and unsigned I wrote code to determinate ranges of char,short int and long variables,both signed and unsigned.Please help me to improve my code :) Here is the code:

#include <stdio.h>

//function declerations...
long power(int base, int power);

int main(void) {
    //From 2^(N-1) to 2^(N-1)-1 (Two's complement)
    int intmin = power(2, sizeof(int) * 8 - 1);
    int intmax = power(2, sizeof(int) * 8 - 1) - 1;
    unsigned unsignedintmax = power(2, sizeof(int) * 8) - 1;
    char minchar = -(power(2, sizeof(char) * 8 - 1));
    char maxchar = power(2, sizeof(char) * 8 - 1) - 1;
    unsigned char unsignedcharmax = power(2, sizeof(char) * 8) - 1;
    short shortmin = -(power(2, sizeof(short) * 8 - 1));
    short shortmax = power(2, sizeof(short) * 8 - 1) - 1;
    unsigned short unsignedshortmax = power(2, sizeof(short) * 8) - 1;
    long minlong = power(2, sizeof(long) * 8 - 1);
    long maxlong = power(2, sizeof(long) * 8 - 1) - 1;
    unsigned long unsignedlongmax = power(2, sizeof(long) * 8) - 1;
    minlong*=-1;
    printf("\nSigned char can be minimum: %d and maximum: %d\n", minchar, maxchar);
    printf("\nUnsigned char can be minimum: %d and maximum: %u\n", 0, unsignedcharmax);
    printf("\nSigned short can be minimum: %d and maximum: %d\n", shortmin, shortmax);
    printf("\nUnsigned short can be minimum: %d and maximum: %u\n", 0, unsignedshortmax);
    printf("\nSigned int can be minimum: %d and maximum: %d\n", intmin, intmax);
    printf("\nUnsigned int can be minimum: %d and maximum: %u\n", 0, unsignedintmax);
    printf("\nSigned long can be minimum: %ld and maximum: %ld\n", minlong, maxlong);
    printf("\nUnsigned long can be minimum: %d and maximum: %lu\n\n", 0, unsignedlongmax);

    return 0;
}

long power(int base, int power) {
    long pf = 1;
    for (int i = 0; i < power; i++) {
        pf *= base;
    }
    return pf;
}

Signedness of unqualified char is implementation defined. It may well be possible that char is in fact unsigned. Change char to signed char.
A char is not guaranteed to have 8 bits (it is guaranteed to have at least 8 bits). Use CHAR_BIT instead.
Narrowing types (e.g. assigning long to char) always make me uncomfortable. A better technique to get a value with only MSB set is (TYPE is either char,int,long,whatever)

unsigned TYPE value = ~(((unsigned TYPE) -1) >> 1)


Form data handler class I've started to write a form data handler class in PHP. Can you please review it and point out any mistakes or logic errors, or perhaps if a method can be written in a better way?

define('VALIDATION_ERR', ' couldn`t be validated.');
define('REQUIRED_ERR', ' is required.');
define('PASSWORD_ERR', 'Password and Confirm Password do not match.');

class DataValidator {

protected $errorMsg;
protected $errorFlag;

public function __construct(){
    $this->errorMsg = array();
    $this->errorFlag = 0;
} // constructor

public function checkRequiredData( $dataArray , $requiredFields = NULL ){
    if( count($dataArray) > 0 ){
        if( $requiredFields != NULL ){
            foreach( $dataArray as $key => $val ){
                if( array_key_exists( $key , $requiredFields ) ){
                    $format = $requiredFields[$key];
                    if( !empty($val) && $format != 'none' ){
                        if( !($this->dataFormatCheck($val, $format)) ){
                            $msg = $key . VALIDATION_ERR ;
                            $this->setErrorMsg($msg);
                            $this->errorFlag = 1;
                        }
                    }
                    else{
                        $msg = $key . REQUIRED_ERR;
                        $this->setErrorMsg($msg);
                        $this->errorFlag = 1;
                    }
                }
            }
        } // requiredFields array IF
        else{
            foreach( $dataArray as $key => $val ){
                if( empty($val) ){
                    $msg = $key . REQUIRED_ERR;
                    $this->setErrorMsg($msg);
                    $this->errorFlag = 1;
                }
            }
        }
    } // requiredFields count IF
    else{
        $msg = "data array is empty";
        $this->setErrorMsg($msg);
        $this->errorFlag = 1;
    }

    if( $this->errorFlag == 0 ){
        return true;
    }
    else{
        return false;
    }
} // checkRequiredData

public function getErrorMsg( $last = '' ){
    if( $last == 'last' ){
        return end($this->errorMsg);
    }
    else{
        return $this->errorMsg;
    }
} // getErrorMsg

public function setErrorMsg( $msg ){
    array_push( $this->errorMsg, $msg );
} // setErrorMsg

public function dataFormatCheck( $data , $format ){
    // use switch or if else?
    switch( $format ){
        case 'text' :
            $return = true; // will be implemented later
            break;

        case 'email' :
            $return = $this->checkEmail($data);
            break;

        case 'numeric' :
            $return = $this->checkNumeric($data);
            break;

        case 'alphanumeric' : // will be implemented later
            $return = true;
            break;

        case 'password' :
            $return = $this->checkPassConfirm($data);
            break;

        case 'image'    : // will be implemented later
            $return = true;
            break;

        case 'none' : // will be implemented later
            $return = true;
            break;
    }
    return $return;
} // dataFormatCheck

public function checkAlpha( $data ){
     // will be implemented later
} // check alphabets

public function checkEmail( $data ){
    return filter_var($data, FILTER_VALIDATE_EMAIL);
} // check email

public function checkNumeric( $data ){
    return is_numeric( $data );
} // check Numbers

public function checkAlphaNumeric( $data ){
     // will be implemented later
} // check Alpha Numerics

public function checkPassConfirm( $pass , $cPass ){
    if( $pass != $cPass )
        return false;
    else
        return true;
}

}

Not yet implemented

It's unsafe to just have a comment like this:

// will be implemented later

At least add a TODO (so that an IDE can collect all of these):

// TODO will be implemented later

Or better yet, throw an exception:

throw new Exception('Not implemented');

If your code gets bigger and bigger, it's easy to overlook one or two of your original comments, and then your data will not be filtered correctly.

if-else return true-false

Again, whenever you have an if/else statement that only returns true/false, just return the statement inside the if. So for example

if( $pass != $cPass )
    return false;
else
    return true;

would become

return $pass === $cPass;

Notice also the === instead of ==.

The same goes for this:

if( $this->errorFlag == 0 ){
    return true;
}
else{
    return false;
}

write it like this instead:

return $this->errorFlag == 0;

End-of-Function comments

I think that these only clutter up the code, I would get rid of them.

Inside a function, // endif type comments are a sign of bad code. If you have so many if-statements that you cannot see where each ends, you probably have too many. Try to get rid of some or extract some functionality into a separate method.

Switch or if-else

Since you asked: I think that the switch looks better than an if-else.

Error Flag

You never set this to 0, is this intentional?

Also, you set this flag always directly after calling setErrorMsg. Just move $this->errorFlag = 1; inside setErrorMsg, so you don't have to manage it.

If you would change it to a boolean value, it would also be easier to handle.

Unnecessary local variables

You do this very often:

$msg = $key . VALIDATION_ERR ;
$this->setErrorMsg($msg);

As you don't need msg afterwards, just call setErrorMsg directly:

$this->setErrorMsg($key . VALIDATION_ERR);

Deeply nested checkRequiredData function

You have too many if-else in your checkRequiredData function which makes it really hard to read.

One way to solve this is to pull the checks to the top of the method like this:

function checkRequiredData($dataArray, $requiredFields = NULL) {
    if (count($dataArray) <= 0) {
        $msg = "data array is empty";
        $this->setErrorMsg($msg);
        $this->errorFlag = 1;
        return false;
    }

    if ($requiredFields == NULL) {
        foreach ($dataArray as $key => $val) {
            if (empty($val)) {
                $msg = $key . REQUIRED_ERR;
                $this->setErrorMsg($msg);
                $this->errorFlag = 1;
            }
        }
        return $this->errorFlag == 0;
    }

    foreach ($dataArray as $key => $val) {
        if (array_key_exists($key, $requiredFields)) {
            $format = $requiredFields[$key];
            if (!empty($val) && $format != 'none') {
                if (!($this->dataFormatCheck($val, $format))) {
                    $msg = $key . VALIDATION_ERR;
                    $this->setErrorMsg($msg);
                    $this->errorFlag = 1;
                }
            } else {
                $msg = $key . REQUIRED_ERR;
                $this->setErrorMsg($msg);
                $this->errorFlag = 1;
            }
        }
    }
    return $this->errorFlag == 0;
}

Confusing nested ifs

This code is a bit confusing (it's not that easy to see when there will be no error):

        if (!empty($val) && $format != 'none') {
            if (!($this->dataFormatCheck($val, $format))) {
                $msg = $key . VALIDATION_ERR;
                $this->setErrorMsg($msg);
                $this->errorFlag = 1;
            }
        } else {
            $msg = $key . REQUIRED_ERR;
            $this->setErrorMsg($msg);
            $this->errorFlag = 1;
        }

This might be better:

        if (empty($val) || $format == 'none') {
            $msg = $key . REQUIRED_ERR;
            $this->setErrorMsg($msg);
            $this->errorFlag = 1;
        } else if (!$this->dataFormatCheck($val, $format)) {
            $msg = $key . VALIDATION_ERR;
            $this->setErrorMsg($msg);
            $this->errorFlag = 1;
        } // else: everything is fine

Final Code

If you follow all these suggestions, your checkRequiredData function would look like this:

function checkRequiredData($dataArray, $requiredFields = NULL) {
    if (count($dataArray) <= 0) {
        $this->setErrorMsg("data array is empty");
        return false;
    }

    if ($requiredFields == NULL) {
        foreach ($dataArray as $key => $val) {
            if (empty($val)) {
                $this->setErrorMsg($key . VALIDATION_ERR);
            }
        }
        return $this->hasError;
    }

    foreach ($dataArray as $key => $val) {
        if (array_key_exists($key, $requiredFields)) {
            $format = $requiredFields[$key];
            if (empty($val) || $format == 'none') {
                $this->setErrorMsg($key . REQUIRED_ERR);
            } else if (!$this->dataFormatCheck($val, $format)) {
                $this->setErrorMsg($key . VALIDATION_ERR);
            } // else: everything is fine
        }
    }
    return $this->hasError;
}

public function setErrorMsg( $msg ){
    $this->hasError = true; // set error flag here, use boolean instead of int for easier usage
    array_push( $this->errorMsg, $msg );
}

Which is a lot shorter and a lot easier to understand. It would be best to write some unit tests to verify that it actually does the exact same thing, as this is a major rewrite.


Print values of an array while stripping out some parts of the string Is there a more efficient way to run this? I am especially interested in a way which perhaps does not require a for loop...

shopt -s nullglob

file=
declare -a ARRAY1
ARRAY1=( ~lettus_*.sh )
for file in ${ARRAY1[@]}
do
    ##
    ## Get <name> from /some/path/lettus_name.sh
    ##

    # remove .sh extension
    file=${file%.sh}

    #  remove full path up to and including lettus_
    file=${file//*lettus_}

    echo $file
done

shopt -u nullglob

I'm assuming that file=${file//*lettus_} is supposed to be file=${file##*lettus_}.
Don't do file=. There is no reason to declare the file variable before-hand. Using it is a the loop variable automatically creates it.
Get rid of the temporary ARRAY1 variable (which fyi is not a good variable name). Just do for file in lettus_*.sh. In fact, since you don't quote ${ARRAY1[@]}, word-splitting would make your program fail in some cases.
Why do you want a more efficient way? This should run in a split-second unless you have a million files.


Given a path to a directory, print the path to the biggest file in it Here is my code. I simulated a stack rather than used some regular recursive algorithm because I needed to keep track of two variables: the path to the biggest file and its size. Besides I wrote a few tests.

My solution on Github.

DirectoryIterator.java

import java.io.File;
import java.util.ArrayDeque;
import java.util.Deque;

public class DirectoryIterator {

    private DirectoryIterator() {
        throw new AssertionError();
    }

    public static String getLargestFilePath(File initPath) {
        if (initPath == null) {
            throw new IllegalArgumentException("The init path cannot be null");
        }
        long maxFileLength = 0;
        String maxFilePath = null;
        Deque<File> stack = new ArrayDeque<>();
        stack.push(initPath);
        while (!stack.isEmpty()) {
            File current = stack.pop();
            if (current.isDirectory()) {
                File[] files = current.listFiles();
                if (files != null) {
                    for (File file : files) {
                        stack.push(file);
                    }
                }
            } else if (current.length() > maxFileLength) {
                maxFileLength = current.length();
                maxFilePath = current.getAbsolutePath();
            }
        }
        return maxFilePath;
    }

}

DirectoryIteratorTest.java

import org.junit.Assert;
import org.junit.Test;

import java.io.File;

public class DirectoryIteratorTest {

    private String mTestDir = "C:\\Users\\Maksim\\Downloads\\Test" + File.separatorChar;

    @Test
    public void singleFileIsInit() {
        File initPath = new File(mTestDir + "Android_Accelerometer.png");
        Assert.assertEquals(initPath.getAbsolutePath(), DirectoryIterator.getLargestFilePath(initPath));
    }

    @Test
    public void onlyDirs() {
        File initPath = new File(mTestDir + "only_dirs");
        Assert.assertEquals(null, DirectoryIterator.getLargestFilePath(initPath));
    }

    @Test(expected = IllegalArgumentException.class)
    public void nullPath() {
        Assert.assertEquals(null, DirectoryIterator.getLargestFilePath(null));
    }

    @Test
    public void emptyPath() {
        File initPath = new File("");
        Assert.assertEquals(null, DirectoryIterator.getLargestFilePath(initPath));
    }

    @Test
    public void manyFiles() {
        File initPath = new File("C:\\Users\\Maksim\\Google Drive");
        Assert.assertEquals("C:\\Users\\Maksim\\Google Drive\\Career\\TODO\\AnDevCon Boston 2015.zip", DirectoryIterator.getLargestFilePath(initPath));
    }
}

I find it weird that your function accepts a File but returns a String.  I would choose to return a File.

According to the JavaDoc for listFiles(),

  If this abstract pathname does not denote a directory, then this method returns null. Otherwise an array of File objects is returned, one for each file or directory in the directory.

Based on the documented behaviour, I would say that your if (current.isDirectory()) and if (files != null) checks are redundant, and I would eliminate the current.isDirectory() check.

It might be a good idea to do something about the possible SecurityException if you don't have permission to list a directory.

The throw new AssertionError() in the private constructor is an interesting idea that I haven't seen before.  Perhaps it would be better written as assert false;.


CSV to JSON conversion in Java Since I've looked far and wide for a good example of this to no avail, I have created my own using the JSONArray and JSONObject classes. It took me a while to realize that the push method for JSONObject overwrites everything previously pushed.

public static JSONArray getJSON(String url) throws IOException, JSONException, URISyntaxException, TransformerException{
        HttpClient httpClient = new DefaultHttpClient();
        JSONArray jArray = new JSONArray();
        Object[] q = null;
        Deque<String> queue = new ArrayDeque<String>();

        try{
            HttpGet httpGet = new HttpGet(url);
            HttpResponse response = httpClient.execute(httpGet);
            int responseCode = response.getStatusLine().getStatusCode();
            logger.info("Response Code : " + responseCode);
            if (responseCode != 404){
                logger.info("Response" + response.getEntity().getContent());
                try(BufferedReader reader = new BufferedReader(
                       new InputStreamReader(
                               response.getEntity().getContent()))) {

                 if(reader != null){
                     String aux = "";
                     while ((aux = reader.readLine()) != null) {
                         queue.add(aux);
                     }
                     q = queue.toArray();

                     for(int i = 0; i < q.length; i++){

                         String[] row = q[i].toString().split(",");

                            JSONObject json = new JSONObject();
                            json.put("col0", row[0]);
                            json.put("col1", row[1]);
                            json.put("col2", row[2]);
                            json.put("col3", row[3]);
                            json.put("col4", row[4]);
                            json.put("col5", row[5]);

                            jArray.put(json);

                     }
                 }
                }

             }
                }
                return null;

             }
             return jArray;
        }finally{
            httpClient.getConnectionManager().shutdown();
        }
    }

It would be nice if you provide some information about the libraries used in your code. For example, if I want to read about JSONArray and JSONObject, which json library do I need to search for?

The same observation goes for the java classes HttpGet, HttpResponse, HttpClient and DefaultHttpClient. Showing the imports of those classes would also be helpful.

Having said that I only have one suggestion to your code. It seems like you don't need to create the array of Objects from the queue of strings (Object[] q = queue.toArray();), you could just iterate over the elements in the queue and then process each element.

Instead of adding elements to the jArray object this way

        while ((aux = reader.readLine()) != null) {
            queue.add(aux);
        }
        q = queue.toArray();

        for (int i = 0; i < q.length; i++) {
            String[] row = q[i].toString().split(",");
            JSONObject json = new JSONObject();
            json.put("col0", row[0]);
            json.put("col1", row[1]);
            json.put("col2", row[2]);
            json.put("col3", row[3]);
            json.put("col4", row[4]);
            json.put("col5", row[5]);
            jArray.put(json);
        }

I would add the elements in this other way

        while ((aux = reader.readLine()) != null) {
            queue.add(aux);
        }

        for (String row : queue){
            String[] csvValues = row.toString().split(",");
            JSONObject json = new JSONObject();
            json.put("col0", csvValues[0]);
            json.put("col1", csvValues[1]);
            json.put("col2", csvValues[2]);
            json.put("col3", csvValues[3]);
            json.put("col4", csvValues[4]);
            json.put("col5", csvValues[5]);
            jArray.put(json);
        }


Finding the pairs of intersecting discs I have been following this question on SO, copied below.  I put together a solution, but which is \$O(n^2)\$. In order to get a more efficient \$O(n \log n)\$ or \$O(\log n)\$ solution, I tried to sort that array, but could not get it solved hence would like to ask here how to optimize this solution to get it in less time complexity in C?

  Given an array A of N integers we draw N discs in a 2D plane,
  such that i-th disc has center in (0,i) and a radius A[i]. We
  say that the k-th disc and j-th disc intersect if they have at
  least one common point.

  Write a function

int number_of_disc_intersections(int[] A);

  which given an array A describing N discs as explained above, returns
  the number of pairs of intersecting discs. For example, given N=6 and

A[0] = 1
A[1] = 5
A[2] = 2
A[3] = 1
A[4] = 4
A[5] = 0

  there are 11 pairs of intersecting discs:

  0th and 1st
  0th and 2nd
  0th and 4th
  1st and 2nd
  1st and 3rd
  1st and 4th
  1st and 5th
  2nd and 3rd
  2nd and 4th
  3rd and 4th
  4th and 5th

  so the function should return 11. The function should return -1 if the
  number of intersecting pairs exceeds 10,000,000. The function may
  assume that N does not exceed 10,000,000.

C language solution:

int disc_intersecn(int *A,int N)
{
    int i,j,cnt=0;
    /*
    qsort(A,N,sizeof(A[0]),sort_fn_ascend);
    for(i=0;i<(N-1);i++)
    {
        if(i+A[i] > (i+1-A[i+1]))
        {
            cnt++;
        }
    }
    */

    for (i = 0; i < N; i++)
    {
        for (j = i+1; j < N; j++)
        {
            if (i + A[i] >= j - A[j]) cnt++;
        }
    }

    return cnt;
}

Ajit,

Essentially this is a problem of 1-dimensional collision detection.

A useful tool for that is spatial-partitioning.

The book "Real-Time Collision Detection" by Christer Ericson (Morgan-Kaufmann, 2005) contains a wealth of information.  See pages 300–307 for an excellent discussion of 1-dimensional spatial hashing and pages 316—318 describing Morton keys.  The code given there is in C++ but could easily be adapted to C.

You can view a sample by clicking on "Search Inside This Book" at Amazon and then searching for "Hierarchical Grids".  The 5th link or so is page 300.

—Todd


Simple medical collection/database about patients visits I am writing a system for doctors who are regularly visited by patients. The doctor adds each patient's visit to collection/database. The class can save data to an .xml file and restore it. Can you give your feedback about this code?

// https://github.com/veitsi/jcrf-noweb
import java.util.ArrayList;
import java.util.Date;
import java.io.File;

import javax.xml.bind.JAXBContext;
import javax.xml.bind.JAXBException;
import javax.xml.bind.Marshaller;
import javax.xml.bind.Unmarshaller;
import javax.xml.bind.annotation.XmlElement;
import javax.xml.bind.annotation.XmlRootElement;

@XmlRootElement
public class Jcrf {
    @XmlElement
    Jcrf jcrf;
    @XmlElement
    ArrayList<Visit> vst = new ArrayList<Visit>();

    public Jcrf() {
        super();
    }

    public static void main(String[] args) {
        Jcrf jcrf = new Jcrf();
         jcrf.addTestData();

        System.out.println(jcrf.vst);

        jcrf.toXml();
        jcrf.fromXml();

        System.out.println(jcrf.vst);
    }

    public void toXml() {
        try {
            File file = new File("jcrf.xml");
            JAXBContext jaxbContext = JAXBContext.newInstance(Jcrf.class);
            Marshaller jaxbMarshaller = jaxbContext.createMarshaller(); // output
            jaxbMarshaller.setProperty(Marshaller.JAXB_FORMATTED_OUTPUT, true);
            jaxbMarshaller.marshal(this, file);
            jaxbMarshaller.marshal(this, System.out);
        } catch (JAXBException e) {
            e.printStackTrace();
        }
    }

    public void fromXml() {
        try {
            File file = new File("jcrf.xml");
            JAXBContext jaxbContext = JAXBContext.newInstance(Jcrf.class);
            Unmarshaller jaxbUnmarshaller = jaxbContext.createUnmarshaller();
            jcrf = (Jcrf) jaxbUnmarshaller.unmarshal(file);
            vst = jcrf.vst;
        } catch (JAXBException e) {
            e.printStackTrace();
        }
    }

    public void addTestData() {
        vst.add(new Visit(1, 1, 366));
        vst.add(new Visit(1, 2, 365));
        vst.add(new Visit(2, 1, 364));
    }
}

class Visit {
    @XmlElement
    int ptn;
    @XmlElement
    int nmr;
    @XmlElement
    static final Date dt = new Date();
    @XmlElement
    int ttr;

    public Visit() {
    }

    public Visit(int ptn, int nmr, int ttr) {
        super();
        if (ttr < 300 || ttr > 420)
            throw new IllegalArgumentException("bad temperature");
        this.ptn = ptn;
        this.nmr = nmr;
        this.ttr = ttr;
    }

    @Override
    public String toString() {
        return "Visit [ptn=" + ptn + ", nmr=" + nmr + ", ttr=" + ttr + ", dt="
                + dt + "]";
    }

}

File-systems normally make poor databases because you lose the concept of 'atomicity'... let's take a worst-case example:

public void toXml() {
    try {
        File file = new File("jcrf.xml");
        JAXBContext jaxbContext = JAXBContext.newInstance(Jcrf.class);
        Marshaller jaxbMarshaller = jaxbContext.createMarshaller(); // output
        jaxbMarshaller.setProperty(Marshaller.JAXB_FORMATTED_OUTPUT, true);
        jaxbMarshaller.marshal(this, file);
        jaxbMarshaller.marshal(this, System.out);
    } catch (JAXBException e) {
        e.printStackTrace();
    }
}

What happens if there is an IOException part-way through the jaxbMarshaller.marshal(this, file); ? You end up overwriting the old copy, and losing the new one. Your data is gone.

A more traditional way to prevent this sort of loss is to write the output to a new file, and then do an atomic 'move' operation:

public void toXml() {
    try {
        File file = new File("jcrf.xml.tmp");
        JAXBContext jaxbContext = JAXBContext.newInstance(Jcrf.class);
        Marshaller jaxbMarshaller = jaxbContext.createMarshaller(); // output
        jaxbMarshaller.setProperty(Marshaller.JAXB_FORMATTED_OUTPUT, true);
        jaxbMarshaller.marshal(this, file);
        jaxbMarshaller.marshal(this, System.out);

        // move tmp file to complete output file.
        Path tmp = file.toPath();
        Path target = Paths.get("jcrf.xml");
        Files.move(tmp, target, CopyOption.REPLACE_EXISTING, CopyOption.ATOMIC_MOVE);

    } catch (JAXBException e) {
        e.printStackTrace();
    }
}

Note the use of the atomic Files.move(...)` command.

One day, when your file-system is full, and you run out of space... you will appreciate that change.


Is this an appropriate approach to "tweak" a color to a similar lighter/darker color? I have written this function for the sole purpose to get a similar color either brighter or darker than any given color, depending on an average lightness/darkness. Essentially, I add up the R/G/B values and divide by 3, thus giving the average intensity of each color channel. Then, depending on that result, if it's less than half (darker) I lighten the color, and if it's more than half (lighter), I darken the color. The amount to lighten/darken depends on a given "difference" parameter (Diff).

The purpose and use of this function is in custom drawing controls to color themes, and shading colors based on other colors. If the color is dark, the tweaked color is a little bit lighter. If the color is light, the tweaked color is a little bit darker.

Do you see any shortcomings with my approach? Could I make this any better?

function IntRange(const Value, Min, Max: Integer): Integer;
begin
  Result:= Value;
  if Result < Min then Result:= Min;
  if Result > Max then Result:= Max;
end;

function TweakColor(const AColor: TColor; const ADiff: Integer): TColor;
var
  R, G, B: Byte;
  D: Integer;
  Dir: Integer;
begin
  R:= GetRValue(AColor);
  G:= GetGValue(AColor);
  B:= GetBValue(AColor);
  D:= (R + G + B) div 3; //D = average difference of all 3 color channels
  if D >= (256 div 2) then begin   //If average is a lighter color...
    Dir:= -ADiff;                  //Make the color darker
  end else begin                   //If average is a darker color...
    Dir:= ADiff;                   //Make the color lighter
  end;
  R:= IntRange(R + Dir, 0, 255);
  G:= IntRange(G + Dir, 0, 255);
  B:= IntRange(B + Dir, 0, 255);
  Result:= RGB(R, G, B);
end;

Example Usage:

Canvas.Font.Color:= TweakColor(Canvas.Font.Color, 50);

I think that your algorithm is overly simplistic.  To illustrate its problems, try running the Stack Snippet in this answer, in which I have translated your TweakColor() function into JavaScript for demonstration purposes.

It has three significant flaws, in my opinion:

It is subject to jarring discontinuities when switching from darkening to lightening mode and vice versa.

For example, with Diff=127, try changing the input color from rgb(234,150,0) to rgb(233,150,0).  The output color jumps from maroon to creamy yellow.
It doesn't preserve the hue, which is what most people would consider to be the most important attribute of a color.

In the example above, with rgb(234,150,0) as the input color, as Diff increases from 0, we see the output change from orange to red.  I would expect any tweaking to result in another color that is still recognizably "orange".  If you start from the neighboring rgb(233,150,0), then the color changes to yellow as Diff increases from 0.
Its behavior is hard to characterize.

For example, starting with rgb(255,130,0), and increasing Diff from 0, we see that the output fades from orange to black.  In contrast, starting with rgb(255,125,0), it fades from orange to white.

As another example, starting with rgb(0,80,160) and increasing Diff from 0, we see true blue turning towards cyan as blue is clamped at 255, then trending towards white as green is clamped at 255.

The inconsistency makes no sense to the user.

The remedy is to map the RGB input to a color wheel — I suggest using the hue-saturation-value color model.  Define the tweak to be a hue-preserving transformation.  My suggestion is to vary just the value, but you could also vary saturation instead.

function TweakColor(const AColor: TRGBColor; const ADiff: Integer): TRGBColor;
var
  HSV: THSVColor;
  V, VAdj: Single;
begin
  HSV := RGBToHSV(AColor);
  V := GetVValue(HSV);
  VAdj := (ADiff / 128) * (V - 0.5);
  Result := HSVToRGB(HSV(GetHValue(HSV), GetSValue(HSV), V - VAdj));
end;

/* Your original function, translated into JavaScript */
function tweakColor(aColor, aDiff) {
  function intRange(value, min, max) {
    return value < min ? min
         : value > max ? max
                       : value;
  }
  var r = aColor.r,
      g = aColor.g,
      b = aColor.b;
  var d = (r + g + b) / 3;
  var dir = (d >= (256 / 2)) ? -aDiff : aDiff;
  r = intRange(r + dir, 0, 255);
  g = intRange(g + dir, 0, 255);
  b = intRange(b + dir, 0, 255);
  return new RGBColor(r, g, b);
}

function newTweakColor(aColor, aDiff) {
  var hsv = aColor.toHSV();
  var vAdj = (aDiff / 128) * (hsv.v - 0.5);
  return (new HSVColor(hsv.h, hsv.s, hsv.v - vAdj)).toRGB();
}

function RGBColor(r, g, b) {
  this.r = r; this.g = g; this.b = b;
}

RGBColor.prototype.toString = function() {
  return 'rgb(' + Math.round(this.r) + ',' + Math.round(this.g) + ',' + Math.round(this.b) + ')';
};

/* Based on formulas from http://www.rapidtables.com/convert/color/rgb-to-hsv.htm */
RGBColor.prototype.toHSV = function toHSV() {
  var r = this.r / 255,
      g = this.g / 255,
      b = this.b / 255;
  var cMax = Math.max(r, g, b),
      cMin = Math.min(r, g, b);
  var Δ = cMax - cMin;
  var hue = 60 * ( (cMax == r) ? ((g - b) / Δ) % 6
                 : (cMax == g) ? ((b - r) / Δ) + 2
                               : ((r - g) / Δ) + 4 );
  var sat = cMax == 0 ? 0 : Δ / cMax;
  var val = cMax;
  return new HSVColor((hue + 360) % 360, sat, val);
}

function HSVColor(h, s, v) {
  this.h = h; this.s = s; this.v = v;
}

HSVColor.prototype.toString = function() {
  return 'hsl(' + Math.round(this.h) + ',' + Math.round(100 * this.s) + '%,' + Math.round(100 * this.v) + '%)';
};

/* http://en.wikipedia.org/wiki/HSL_and_HSV#From_HSV */
HSVColor.prototype.toRGB = function toRGB() {
  var c = this.v * this.s;
  var h = this.h / 60;
  var x = c * (1 - Math.abs(h % 2 - 1));
  var r1 = (h < 1 || h >= 5) ? c
         : (h < 2 || h >= 4) ? x : 0;
  var g1 = (h >= 1 && h < 3) ? c
         : (h >= 0 && h < 4) ? x : 0;
  var b1 = (h >= 3 && h < 5) ? c
         : (h >= 2 && h < 6) ? x : 0;
  var m = this.v - c;
  return new RGBColor(Math.round(255 * (r1 + m)),
                      Math.round(255 * (g1 + m)),
                      Math.round(255 * (b1 + m)));
};

var ColorTweakerCtrl = function ColorTweakerCtrl($scope) {
  $scope.inColor  = new RGBColor(234, 150, 0);
  $scope.diff = 127;

  $scope.tweak = function tweak() {
    $scope.inColor.r = Math.round($scope.inColor.r);
    $scope.inColor.g = Math.round($scope.inColor.g);
    $scope.inColor.b = Math.round($scope.inColor.b);
    $scope.diff = Math.round($scope.diff);
    $scope.outColor = tweakColor($scope.inColor, $scope.diff);
    $scope.newColor = newTweakColor($scope.inColor, $scope.diff);
  };
  $scope.tweak();
};
fieldset {
  display: inline-block;
  width: 25%;
}
fieldset#tweak {
  display: block;
  width: 100%;
  border-width: 0;
}
label {
  display: block;
}
.swatch {
  border: 1px solid grey;
  width: 100%;
  height: 1em;
}
input[type=range] {
  width: 40%;
}

  <form ng-controller="ColorTweakerCtrl">
    <fieldset id="tweak">
      Diff
        <input ng-model="diff" type="range" min="0" max="255" ng-change="tweak()">
        <input ng-model="diff" type="number" min="0" max="255" ng-change="tweak()">

    </fieldset>

    <fieldset>
      <legend>Input Color</legend>

      R
        <input ng-model="inColor.r" type="range" min="0" max="255" ng-change="tweak()">
        <input ng-model="inColor.r" type="number" min="0" max="255" ng-change="tweak()">

      G
        <input ng-model="inColor.g" type="range" min="0" max="255" ng-change="tweak()">
        <input ng-model="inColor.g" type="number" min="0" max="255" ng-change="tweak()">

      B
        <input ng-model="inColor.b" type="range" min="0" max="255" ng-change="tweak()">
        <input ng-model="inColor.b" type="number" min="0" max="255" ng-change="tweak()">

    </fieldset>

    <fieldset>
      <legend>Output (Yours)</legend>

      R
        <input ng-model="outColor.r" type="range" min="0" max="255" disabled>
        <input ng-model="outColor.r" type="number" min="0" max="255" disabled>

      G
        <input ng-model="outColor.g" type="range" min="0" max="255" disabled>
        <input ng-model="outColor.g" type="number" min="0" max="255" disabled>

      B
        <input ng-model="outColor.b" type="range" min="0" max="255" disabled>
        <input ng-model="outColor.b" type="number" min="0" max="255" disabled>

    </fieldset>

    <fieldset>
      <legend>Output (Suggested)</legend>

      R
        <input ng-model="newColor.r" type="range" min="0" max="255" disabled>
        <input ng-model="newColor.r" type="number" min="0" max="255" disabled>

      G
        <input ng-model="newColor.g" type="range" min="0" max="255" disabled>
        <input ng-model="newColor.g" type="number" min="0" max="255" disabled>

      B
        <input ng-model="newColor.b" type="range" min="0" max="255" disabled>
        <input ng-model="newColor.b" type="number" min="0" max="255" disabled>

    </fieldset>
  </form>


Determine the class hierarchy of a class My only concern with the following class is the line hierarchy.trimToSize() in determineHierarchy, because it is not necessary for determining the class hierarchy. However, putting it in the constructor clutters the code more than my current solution. Any recommendations or should I just live with it?

Since this class is completely finished and tested, I also look for any other helpful comments. The target version is 1.8.

/** The class hierarchy of a class. */
public class ClassHierarchy {

    private final List<Class<?>> hierarchy;

    /**
     * Creates a ClassHierarchy instance for the given class {@literal clazz}.
     *
     * @param clazz
     *            Class for which the class hierarchy should be determined. Must not be null and must refer to a class
     *            type.
     *
     * @throws IllegalArgumentException
     *             if {@literal clazz} does not refer to a class type
     * @throws NullPointerException
     *             if {@literal clazz} is null
     */
    public ClassHierarchy(Class<?> clazz) {
        validateConstructorArgument(clazz);
        hierarchy = Collections.unmodifiableList(determineHierarchy(clazz));
    }

    /**
     * Returns the class hierarchy as an immutable list.
     *
     * <p>
     * The classes are ordered, starting with the class for which the instance was created and ending with
     * {@code Object.class}.
     *
     * @return the class hierarchy as an immutable list.
     */
    public List<Class<?>> getHierarchy() {
        return hierarchy;
    }

    private static void validateConstructorArgument(Class<?> clazz) {
        Objects.requireNonNull(clazz, "clazz must not be null");

        if (clazz.isEnum() || clazz.isAnnotation() || clazz.isInterface() || clazz.isArray() || clazz.isPrimitive()) {
            throw new IllegalArgumentException("clazz must refer to a class type");
        }
    }

    private static ArrayList<Class<?>> determineHierarchy(Class<?> clazz) {
        ArrayList<Class<?>> hierarchy = new ArrayList<>();
        for (Class<?> currentClass = clazz; currentClass != null; currentClass = currentClass.getSuperclass()) {
            hierarchy.add(currentClass);
        }
        hierarchy.trimToSize();
        return hierarchy;
    }

}

The one comment I have is that I find your if statement to be really hard to read:

if (clazz.isEnum() || clazz.isAnnotation() || clazz.isInterface() || clazz.isArray() || clazz.isPrimitive())

I would at least put that into a small helper to throw that exception:

private void checkClassRefersToClassType()


Relational schema and query for multiplayer tabletop game As an exercise, I was asked to design a database schema (for MS SQL Server) for a tabletop game. The requirements were simple: players compete in matches and there are specific match types (e.g. 1v1 or 2v2). A hypothetical web site that would make use of this database would include things like player match history, a scoreboard page, and a leaderboard page.

My initial design had some flaws and inconsistencies, but after thinking more on it, I came up with the following schema:

create table Players (
    Id int identity(1, 1) primary key,
    PlayerName nvarchar(50) not null
);

create table MatchTypes (
    Id tinyint primary key,
    Name nvarchar(50) not null
);

create table Matches (
    Id int identity(1, 1) primary key,
    MatchTypeId tinyint not null references MatchTypes(Id),
    IsComplete bit not null,
    CompletionDate datetime2 null
);

create table Teams (
    Id int identity(1, 1) primary key,
    MatchId int not null references Matches(Id),
    Score smallint not null,
    IsWinningTeam bit null
);

create table PlayerTeams (
    PlayerId int not null references Players(Id),
    TeamId int not null references Teams(Id)
);

Although the design is fairly simple, I'm sure I've made sub-optimal design decisions. What could be improved on?

In addition, I went ahead and wrote a SQL query to extract data for a hypothetical "Player Match History" page. The goal is something like the following:

Player: Foo
Matches:
  Against Bar                  ... Won!
  Against Quux                 ... Lost
  With Baz, against Bar & Quux ... Lost
  With Bar, against Quux & Baz ... Won!

Essentially, given a player, select all matches that the player participated in, and display them with opponents/teammates.

Here's the query, although I feel like it may be unnecessarily complex.

select pm.MatchId,
    t.Id as TeamId,
    p.Id as PlayerId,
    p.PlayerName,
    case when t.Id = pm.TeamId then 1 else 0 end as IsTeammate,
    pm.Won
from (
    select m.Id as MatchId,
        m.CompletionDate,
        t.Id as TeamId,
        t.IsWinningTeam as Won
    from Players p
    inner join PlayerTeams pt
        on p.Id = pt.PlayerId
    inner join Teams t
        on pt.TeamID = t.Id
    inner join Matches m
        on t.MatchId = m.Id
    where m.IsComplete = 1 and p.Id = 1
) pm
inner join Teams t
    on pm.MatchId = t.MatchId
inner join PlayerTeams pt
    on t.Id = pt.TeamId
inner join Players p
    on pt.PlayerId = p.Id
where p.Id != 1
order by pm.CompletionDate desc

Good work on schema

Overall, I think your schema is clean, easy to follow, and makes sense (in the limited scope of your scenario). Granted the overarching or "whole" schema for something like this would be much larger, but if you kept following this sort of normalization it should be fine.

Aliases

I see at least 5 unclear aliases: p, t, m, pt, pm. Mr. Maintainer would have to read through your entire script to even grasp what these stand for. Your column names are short enough, I really don't think aliases are of much use, and they just make your script more confusing to review. Imagine this kind of notation used on 1000 lines of codes in a real business setting, this should drive my point home.

Common Table Expressions

You have a rather large subquery (which I will address next point) and in SQL Server and several other RDBMS, you can simplify the way it reads by using a CTE. So this:

from (
    select m.Id as MatchId,
        m.CompletionDate,
        t.Id as TeamId,
        t.IsWinningTeam as Won
    from Players p
    inner join PlayerTeams pt
        on p.Id = pt.PlayerId
    inner join Teams t
        on pt.TeamID = t.Id
    inner join Matches m
        on t.MatchId = m.Id
    where m.IsComplete = 1 and p.Id = 1
) pm

You could instead write, at the very beginning of your script:

with pm as (
        select m.Id as MatchId,
            m.CompletionDate,
            t.Id as TeamId,
            t.IsWinningTeam as Won
        from Players p
        inner join PlayerTeams pt
            on p.Id = pt.PlayerId
        inner join Teams t
            on pt.TeamID = t.Id
        inner join Matches m
            on t.MatchId = m.Id
        where m.IsComplete = 1 and p.Id = 1
    )
select /* bunch of work here */
from pm    -- reference CTE in main query as if it were a table

Redundancy

I noticed your subquery (or CTE) is joining some of the same stuff your main query is doing. Look:

from (
    select m.Id as MatchId,
        m.CompletionDate,
        t.Id as TeamId,
        t.IsWinningTeam as Won
    from Players p
    inner join PlayerTeams pt
        on p.Id = pt.PlayerId  -- (1)
    inner join Teams t
        on pt.TeamID = t.Id  -- (2)
    inner join Matches m
        on t.MatchId = m.Id
    where m.IsComplete = 1 and p.Id = 1
) pm
inner join Teams t
    on pm.MatchId = t.MatchId
inner join PlayerTeams pt
    on t.Id = pt.TeamId  -- (2)
inner join Players p
    on pt.PlayerId = p.Id  -- (1)

Best to avoid redundant operations, especially expensive join's, as it makes the execution slower. Only one of each should be needed in most cases.

Nitpick

This preferably should not be used: where p.Id != 1

Instead, use this: where p.Id <> 1

Although both work just the same, != is not ANSI-92 standard.


Find the best pairs of elements based on match weight While answering this question on SO I came up with a very simple solution and I wanted to know if there were improvements that could be made to it.

The function takes an array of participants as inputs, generates a list of pairs using the array of participants with an associated score (to be generated by a function comparing the elements but for now using rand()), then finds the the best possible match and return the results as an array (imagine students being matched for a team assignment).

function generatePairs($elements) {
    // $elements = [
    //     1,2,3,4,5,6
    // ];

    $elementCount = count($elements);
    $output = [];

    // build an array with every unique possible pair
    $possiblePairs = [];
    for($i = 1; $i <= $elementCount; $i++) {
        for($j = $i + 1; $j <= $elementCount; $j++) {
            $possiblePairs[] = [
                'values' => [$elements[$i - 1], $elements[$j - 1]],
                'score' => rand(1, 100) // this would be replaced by a function calculating how closely the current elements match
            ];
        }
    }

    // puts the "best" pairs on top
    usort($possiblePairs, function($a, $b) {
        return $b['score'] <=> $a['score'];
    });

    $used = [];
    foreach($possiblePairs as $key => $pair) {
        if($pair['score'] !== 0 &&
            !in_array($pair['values'][0], $used) &&
            !in_array($pair['values'][1], $used))
        {
            // save the pair to the output array
            $output[] = $pair['values'];
            // store the elements that have already been used
            array_push($used, $pair['values'][0], $pair['values'][1]);
        }
    }

    return $output;
}

A couple improvements:

Pop from array and iterate over remaining items in nested loop

I tried to modify the nested for statements used to add to $possiblePairs and use a functional approach like with array_reduce() but instead found the example in this answer. Consider shifting (i.e. popping from the beginning) from the array to yield an array with the remaining elements with which to generate pairs.

while ($item = array_shift($elements)) {
    foreach($elements as $element) {
        //generate pair with $item and $element

With this approach, there is no need to keep track of counter variables (e.g. $i, $j), use those to index into the array, nor increment those until certain conditions change. It also makes the following line superfluous (so it can be removed):

$elementCount = count($elements);

Store pair as key, and score as value

This way the sorting can be simplified - using asort() with SORT_NUMERIC as $sort_flag. Then reverse the array for descending order using array_reverse().

while ($item = array_shift($elements)) {
    foreach($elements as $element) {
        $possiblePairs[$item.'_'.$element] =  rand(1, 100);
    }
}
asort($possiblePairs, SORT_NUMERIC);
$possiblePairs = array_reverse($possiblePairs, true);

Output from keys

Because of the changes above, the keys are the pairs so use list() and explode() to unpack the pairs.

$used = [];
    foreach($possiblePairs as $pair => $score) {
        list($element1, $element2) = explode('_', $pair);
        if($score !== 0 &&
            !in_array($element1, $used) &&
            !in_array($element2, $used))
        {
            // save the pair to the output array
            $output[] = [$element1, $element2];
            // store the elements that have already been used
            array_push($used, $element1, $element2);
        }
    }
    return $output;

Another optimization

In order to only go through the  (last) foreach loop as few times as necessary, the following conditional could be added after pushing values into $used:

if (count($used) == count($elements)) {
     break;
}

That should prevent excess loop iterations. While the difference may be negligible for small lists, it may be very helpful to reduce the execution time for larger lists.

Demonstration

Check out this playground example for a demonstration. And compare the time output at the end with the original.

| Run | Original             | Updated               |
|-----|----------------------|-----------------------|
|  1  | 0.0002138614654541s  | 0.00012707710266113s  |
|  2  | 0.00021195411682129s | 0.00017905235290527s  |
|  3  | 0.00021100044250488s | 0.00017690658569336s  |
| Avg | 0.0002122720083s     | 0.0001610120138s      |

Here is the updated code:

function stuff($elements = []) {
    $output = [];

    // build an array with every unique possible pair
    while ($item = array_shift($elements)) {
        foreach($elements as $element) {
            $possiblePairs[$item.'_'.$element] =  rand(1, 100); // this would be replaced by a function calculating how closely the current elements match
        }
    }
    //use asort to sort on values and
    asort($possiblePairs, SORT_NUMERIC);
    $possiblePairs = array_reverse($possiblePairs, true);

    $used = [];
    foreach($possiblePairs as $pair => $score) {
        list($element1, $element2) = explode('_', $pair);
        if($score !== 0 &&
            !in_array($element1, $used) &&
            !in_array($element2, $used))
        {
            // save the pair to the output array
            $output[] = [$element1, $element2];
            // store the elements that have already been used
            array_push($used, $element1, $element2);
            if (count($elements) == count($used)) {
               break;
            }
        }
    }
    return $output;
}


Send daily email alerts of the Bitcoin Price My script sends daily alerts of the bitcoin price to my inbox. I am using CoinDesks API, and the following libraries:

requests==2.18.4

schedule==0.5.0

and gmail sender: https://github.com/paulchakravarti/gmail-sender

Here is my code:

"""Send daily Bitcoin price alerts to inbox."""
from time import sleep

from requests import get
import schedule
from gmail import GMail, Message

def get_bitcoin_price():
    """Get current Bitcoin price in USD using Coindesk's real-time API"""
    response = get('https://api.coindesk.com/v1/bpi/currentprice.json').json()

    return response['bpi']['USD']['rate']

def send_email(recipient: str='walid.mujahid.dev@gmail.com'):
    bitcoin_price = get_bitcoin_price()

    # enter actual password, otherwise, nothing happens.
    gmail = GMail('Price Alert <walid.mujahid.open@gmail.com>',
                  password='password')
    message = Message(f'Bitcoin is at {bitcoin_price} right now!',
                      to=recipient,
                      text=f'The current Bitcoin price is {bitcoin_price}.')

    gmail.send(message)

if __name__ == '__main__':
    schedule.every().day.at("06:30").do(send_email)

    while True:
        schedule.run_pending()
        sleep(1)

Define some constants
I'd make some constants and put them at the top of the program because if your program becomes bigger, it'll be harder to change their values:
EMAIL_TIME = '06:30'
SLEEP_TIME = 1
URL = 'https://api.coindesk.com/v1/bpi/currentprice.json'

Never store your credentials in clear-text
Here, you replaced your actual password with a placeholder, but what if you mistakenly commit&push it to your public git repository? More, at the moment I can also take your email and send those nice Viagra emails.
What I usually do is store my credentials in a different config file which is in .gitignore as well (it's not A LOT safer, but at least you can't see them + it adds an extra layer of security anyways).
As an alternative, you can create environment variables to store them there if you'd like.

Error handling
Try to add some exception handling to let the user know if something went wrong with its script. You can even create your own exception class:
class RetrieveUrlException(Exception):
    pass

Which you can then call like this:
...
raise RetrieveUrlException('The URL could not be accessed')


Check if a directed graph contains a cycle I am trying to optimize execution time of this function which detects cycle in a directed graph:

g = {
    1: [2, 3],
    2: [3]
}

cur_path = set()
def isCyclic(g, vertex):
    global cur_path
    cur_path.add(vertex)
    for neighboor in g.get(vertex, []):
        if neighboor in cur_path:
            return True
        else:
            if neighboor in g and isCyclic(g, neighboor):
                return True
    cur_path.remove(vertex)
    return False

I look for redundant calls, or information that I can store somewhere to improve the execution time (which is O(E), right?).

1. Review

There's no docstring. What does the function do? What arguments should I pass? What does it return?
This is the kind of function that would make an ideal candidate for a doctest or two.
The function does not actually determine if a graph contains a cycle. It determines if the graph contains a cycle starting at a given vertex. To detect a cycle, it would be necessary to call the function for each vertex in the graph.
The function uses a global variable for state. This makes it impossible to use the function in a multi-threaded program. It also makes it difficult to debug, because whenever anything goes wrong, you have to reset cur_path to the empty set before you can re-run the function. It would be better if all state were encapsulated inside the function.
else: if ...: can be abbreviated to elif ...:.
In British English the word is spelled "neighbour", and in American English it's spelled "neighbor".
It's better to use the empty tuple () as the default value if vertex is not found in g, instead of the empty list []. That's because the empty tuple is a singleton: each time () is evaluated you get the same object. But each time you evaluate [] a new empty list has to be allocated.
Testing neighbour in g is unnecessary since you already handle the case where vertex is not in g.

2. Revised code

Applying all the comments above:

def cyclic(g):
    """Return True if the directed graph g has a cycle.
    g must be represented as a dictionary mapping vertices to
    iterables of neighbouring vertices. For example:

    >>> cyclic({1: (2,), 2: (3,), 3: (1,)})
    True
    >>> cyclic({1: (2,), 2: (3,), 3: (4,)})
    False

    """
    path = set()

    def visit(vertex):
        path.add(vertex)
        for neighbour in g.get(vertex, ()):
            if neighbour in path or visit(neighbour):
                return True
        path.remove(vertex)
        return False

    return any(visit(v) for v in g)

3. Analysis

The function follows all simple paths in the graph, stopping when it encounters a cycle. Unfortunately, a graph may have exponentially many simple paths without containing any cycles, so the runtime is exponential in the size of the input.

For example, this function constructs a graph with exponentially many simple paths:

def test_graph(n):
    """Return an acyclic graph containing 2**n simple paths."""
    g = dict()
    for i in range(n):
        g[3 * i] = (3 * i + 1, 3 * i + 2)
        g[3 * i + 1] = g[3 * i + 2] = (3 * (i + 1),)
    return g

The graphs it constructs look like this:

Experimentally we can check that the runtime is exponential:

>>> for i in range(10, 20):
...     print(i, timeit(lambda:cyclic(test_graph(i)), number=1))
...
10 0.017517157015390694
11 0.03172751097008586
12 0.06926556502003223
13 0.1300737849669531
14 0.25352559401653707
15 0.5073735360056162
16 0.9933696809457615
17 2.004867063020356
18 3.9877060829894617
19 7.927054518950172

You can see that each increment of i doubles the runtime.

4. An efficient algorithm

To make the algorithm efficient, we must make sure not to visit any vertex more than once, which we can do by keeping a separate set of vertices that have been visited so far. For example:

def cyclic(g):
    """Return True if the directed graph g has a cycle.
    g must be represented as a dictionary mapping vertices to
    iterables of neighbouring vertices. For example:

    >>> cyclic({1: (2,), 2: (3,), 3: (1,)})
    True
    >>> cyclic({1: (2,), 2: (3,), 3: (4,)})
    False

    """
    path = set()
    visited = set()

    def visit(vertex):
        if vertex in visited:
            return False
        visited.add(vertex)
        path.add(vertex)
        for neighbour in g.get(vertex, ()):
            if neighbour in path or visit(neighbour):
                return True
        path.remove(vertex)
        return False

    return any(visit(v) for v in g)

This visits each vertex at most once, and considers each edge at most once, so its runtime is \$ O(V + E) \$.

Experimentally we can check that the runtime is roughly linear in the size of the graph:

>>> for i in range(10, 20):
...     print(i, timeit(lambda:cyclic(test_graph(i)), number=1))
...
10 0.00017403007950633764
11 0.00014279899187386036
12 0.00015402096323668957
13 0.00016507902182638645
14 0.00017680192831903696
15 0.00019660103134810925
16 0.00020132900681346655
17 0.0002140760188922286
18 0.00022624700795859098
19 0.00023716699797660112

5. Avoiding the recursion limit

The implementation uses recursion to search the graph. This means that it uses as many stack frames as there are nodes in the longest path it visits in the graph. But Python has a limited number of stack frames (see sys.getrecursionlimit). To get round this limit, we can use the stack of iterators pattern, like this:

def cyclic(graph):
    """Return True if the directed graph has a cycle.
    The graph must be represented as a dictionary mapping vertices to
    iterables of neighbouring vertices. For example:

    >>> cyclic({1: (2,), 2: (3,), 3: (1,)})
    True
    >>> cyclic({1: (2,), 2: (3,), 3: (4,)})
    False

    """
    visited = set()
    path = [object()]
    path_set = set(path)
    stack = [iter(graph)]
    while stack:
        for v in stack[-1]:
            if v in path_set:
                return True
            elif v not in visited:
                visited.add(v)
                path.append(v)
                path_set.add(v)
                stack.append(iter(graph.get(v, ())))
                break
        else:
            path_set.remove(path.pop())
            stack.pop()
    return False