Finding duplicate char from a string I have written code to find a duplicate char from a string. Can the code be improved?

Input:

         Hello World

output :

         l =  3 its a dup

         o = 2 its a dup

My approach is

step 1: sort the string

step 2: remove all the whitespace

step 3: copy std::string to char [] and loop through the array until different char and count on every dup char if count is more than one its duplicate.

#include<iostream>
#include<algorithm>
#include<set>
#include<string>
#include<string.h>
#include<unistd.h>

void dupcheck(std::string usr)
{
        int count =1;
        char temp[1024]={0};
        strncpy(temp,usr.c_str(),sizeof(temp));
        for(int i=0;temp[i];++i)
        {
                for(int j=i+1;temp[j]==temp[i];++j)
                {
                        if(temp[i]==temp[j])
                                count++;
                                i=j;

                }
                        if(count>=2)
                        {
                                std::cout<<"dup char ="<<temp[i]<<" "<<count<<std::endl;
                                count =1;
                        }
        }

}

int main()
{
        std::string userInput;
        std::getline(std::cin,userInput);
        userInput.erase(std::remove(userInput.begin(), userInput.end(),' '), userInput.end());
        std::sort(userInput.begin(),userInput.end());
        std::cout<<userInput<<std::endl;
        if(!userInput.empty())
        dupcheck(userInput);
}

Please help me to improve the code if possible.

Copying the string in step 3 is unnecessary

You can iterate through the characters of a std::string s using std::string::operator[] and std::string::size():

for (std::string::size_type pos = 0; pos < s.size(); pos++) {
    char c = s[pos]; // do something with the char now
}

Alternatively, you can use std::string::iterator to iterate through the string.

Avoid std::endl in favor of \n

std::endl flushes the stream, which can cause a loss in performance.

Use more whitespace in your code

It's difficult to read code like for(int i=0;temp[i];++i). Instead, use more whitespace as in for (int i = 0; temp[i]; ++i).

I would take a different approach to solve the problem. With your approach, you have to iterate through the string at least once to sort it (step 1), then again to remove the whitespace (step 2), then a third time to look for duplicates. If you copy the string in step 3 that's yet another iteration.

You only need to iterate through the string once. Here's one way to do that:

Iterate through the string's characters (skipping the whitespace characters) and put each character in a std::set<char> if you haven't encountered it before. If you've already encountered it (it's in the std::set) put it in a separate std::set of duplicates. Once you've iterated through the string's character's once, you can iterate through the set of duplicates to print them.

Here's a demo:

#include <iostream>
#include <string>
#include <cctype>
#include <set>

int main() {
    std::string s("Hello world");
    std::set<char> characters;
    std::set<char> duplicates;

    for (std::string::size_type pos = 0; pos < s.size(); pos++) {
        char c = s[pos];

        // std::isspace() accepts an int, so cast c to an int
        if (!std::isspace(static_cast<int>(c))) {
            if (characters.count(c) == 0) {
                characters.insert(c);
            } else {
                duplicates.insert(c);
            }
        }
    }

    std::cout << "Duplicates: ";

    for (std::set<char>::const_iterator it = duplicates.begin(); it != duplicates.end(); ++it) {
        std::cout << *it << " ";
    }

    return 0;
}

Note that it's better to use std::isspace() instead of ' ' to check if a character is whitespace.

If you need to count how many times each character occurs in the string, simply replace std::set<char> duplicates with a std::map<char, int> which maps each character to the number of times it occurs (as shown in @TobySpeight's answer).


Visualization showing overlays of points I'm fairly new to d3 and am wanting to create a visualization that will allow a user to check whether or not a device will be in range of a bluetooth speaker.

I was able to hack together most of what I want, but am not sure if I'm following d3 best practices. This will be eventually integrated with React. Any feedback on style/efficiency is appreciated.

(Note: you'll want to view in the full page)

var svg = d3.select("svg"),
  width = +svg.attr("width"),
  height = +svg.attr("height"),
  radius = 8,
  unitRadius = 64;

var points = d3.range(10).map(function() {
  return {
    x: Math.round(Math.random() * (width - radius * 2) + radius),
    y: Math.round(Math.random() * (height - radius * 2) + radius)
  };
});

var circle = d3.range(1).map(function() {
  return {
    x: width / 2,
    y: height / 2
  };
});

var points = svg
  .selectAll("circle.point")
  .data(points)
  .enter()
  .append("circle")
  .attr("class", "point")
  .attr("cx", function(d) {
    return d.x;
  })
  .attr("cy", function(d) {
    return d.y;
  })
  .attr("r", radius)
  .style("fill", "red");

var circle = svg
  .selectAll("circle.unit")
  .data(circle)
  .enter()
  .append("circle")
  .attr("class", "overlay")
  .attr("cx", function(d) {
    return d.x;
  })
  .attr("cy", function(d) {
    return d.y;
  })
  .attr("r", unitRadius)
  .style("fill", "blue")
  .call(
    d3
    .drag()
    .on("start", dragstarted)
    .on("drag", dragged)
    .on("end", dragended)
  );

function dragstarted(d) {
  d3.select(this)
    .raise()
    .classed("active", true);
}

function dragged(d, i) {
  d3.select(this)
    .attr("cx", (d.x = d3.event.x))
    .attr("cy", (d.y = d3.event.y));

  var inside = d3.selectAll("circle.point").style("fill", function(p) {
    var x = d.x - p.x;
    var y = d.y - p.y;
    var dis = Math.sqrt(x * x + y * y);
    if (dis <= Math.abs(unitRadius - radius)) {
      return "green";
    } else {
      return "red";
    }
  });
}

function dragended(d) {
  d3.select(this).classed("active", false);
}
.overlay {
  fill-opacity: 0.1;
}
<script type="text/javascript" src="//cdnjs.cloudflare.com/ajax/libs/d3/4.13.0/d3.js"></script>
<svg width="960" height="500"></svg>

Overall you have a pretty nice code for someone who is "fairly new to D3", good on you.

The proposed changes here are few:

The big circle

You have just one big circle (the blue one). Therefore, you don't need that mix of d3.range and map for its datum. Also, you don't need an enter selection, since that's just one circle.

So, you can do:

var circleDatum = {
    x: width / 2,
    y: height / 2
};

And:

var circle = svg
    .append("circle")
    .datum(circleDatum)
    //etc...

Mind the fact that you're using circle as the name for two different things!

Don't re-select the same thing again...

Inside the dragged function you have this:

var inside = d3.selectAll("circle.point").style("fill", function(d){
    //etc...

Which basically re-selects all .point circles again. I wrote re-select because you already have a selection for them: points.

Therefore, just do:

points.style("fill", function(d){

Use selectAll(null) in your enter selections

If you don't plan to update the chart, that is, if you don't want to have an update and exit selections, you don't need to select anything in your enter selections.

That being said, you can do:

var points = svg.selectAll(null)
    //etc...

By using selectAll(null) you'll have a cleaner and faster code (not noticeable since you have soo few elements here).

For reading more about selectAll(null) have a look my explanation here: Selecting null: what is the reason behind 'selectAll(null)' in D3.js?.

Minor changes

Instead of putting all the d3.drag inside the call, you can do:

var drag = d3.drag()

And then:

.call(drag)

If you don't use the second argument (the index), don't write the parameter. So, instead of:

function dragged(d, i) {

Just do:

function dragged(d) {

If you like ternary operators, the whole if can be just:

return dis <= Math.abs(unitRadius - radius) ? "green" : "red";

You can use Math.hypot for the hypothenuse (note: this doesn't work on IE)

var dis = Math.hypot(x, y);

Also, if you don't mind old IE versions, you can use let and const (accordingly) instead of var.
Sometimes users don't know that they can interact with the visualisation, and a lot of people don't read the instructions/description. So, adding a visual clue may be useful. For instance, changing the cursor when you hover over the big circle helps to indicate that it is draggable:

.style("cursor", "pointer")

Demo

Here is your code with those changes:

var svg = d3.select("svg"),
  width = +svg.attr("width"),
  height = +svg.attr("height"),
  radius = 8,
  unitRadius = 64,
  drag = d3.drag();

var points = d3.range(10).map(function() {
  return {
    x: Math.round(Math.random() * (width - radius * 2) + radius),
    y: Math.round(Math.random() * (height - radius * 2) + radius)
  };
});

var circleDatum = {
  x: width / 2,
  y: height / 2
};

var points = svg.selectAll(null)
  .data(points)
  .enter()
  .append("circle")
  .attr("class", "point")
  .attr("cx", function(d) {
    return d.x;
  })
  .attr("cy", function(d) {
    return d.y;
  })
  .attr("r", radius)
  .style("fill", "red");

var circle = svg.append("circle")
  .datum(circleDatum)
  .attr("class", "overlay")
  .attr("cx", function(d) {
    return d.x;
  })
  .attr("cy", function(d) {
    return d.y;
  })
  .attr("r", unitRadius)
  .style("fill", "blue")
  .style("cursor", "pointer")
  .call(drag.on("start", dragstarted)
    .on("drag", dragged)
    .on("end", dragended)
  );

function dragstarted(d) {
  d3.select(this)
    .raise()
    .classed("active", true);
}

function dragged(d) {
  d3.select(this)
    .attr("cx", (d.x = d3.event.x))
    .attr("cy", (d.y = d3.event.y));

  points.style("fill", function(p) {
    var x = d.x - p.x;
    var y = d.y - p.y;
    var dis = Math.hypot(x, y);
    return dis <= Math.abs(unitRadius - radius) ? "green" : "red";
  });
}

function dragended(d) {
  d3.select(this).classed("active", false);
}
.overlay {
  fill-opacity: 0.1;
}
<script type="text/javascript" src="//cdnjs.cloudflare.com/ajax/libs/d3/4.13.0/d3.js"></script>
<svg width="960" height="500"></svg>


Simple BloomFilter Class We have some BloomFilters (like a java.util.set without deletion) and we store them in ehcache (you can think it's as a java.util.Map):

For example, Constant.BLOOM_FILTER_CACHE ehcache contains:

key   value
aaa   bloomfilter (of name aaa)
bbb   bloomfilter (of name bbb)
ccc   bloomfilter (of name ccc)

If we call localCacheManager.get(Constant.BLOOM_FILTER_CACHE, aaa), it will return a bloomfilter (of name aaa) or Null (if not initialized).

If we call localCacheManager.put(Constant.BLOOM_FILTER_CACHE, aaa, new BloomFilter(100, 0.001)), it will put a bloomfilte to cache entry aaa.

If we want to add data1 into BloomFilter of name aaa, then we may call add(aaa, data1) and if we want to know if aaa BloomFilter contains data1, we should call contains(aaa, data1) (in that case, it will return true).

If we want to look up or add, we should check the bloomfilter is initialized or not.

package com.xxx.utils.bloomfilter;

import com.clearspring.analytics.stream.membership.BloomFilter;
import com.xxx.utils.cache.ehcache.LocalCacheManager;
import com.xxx.utils.common.Constant;

public class BloomFilterInCache {
  private int bloomFilterElementSize;
  private double bloomFilterErrorRate;
  private LocalCacheManager localCacheManager;

  public BloomFilterInCache(int bloomFilterElementSize,
      double bloomFilterErrorRate, LocalCacheManager localCacheManager) {
    this.bloomFilterElementSize = bloomFilterElementSize;
    this.bloomFilterErrorRate = bloomFilterErrorRate;
    this.localCacheManager = localCacheManager;
  }

  private boolean isBloomFilterExistsByName(String name)
  {
    BloomFilter bf = (BloomFilter) localCacheManager.get(Constant.BLOOM_FILTER_CACHE, name);
    if (bf == null)
      return false;
    return true;
  }

  private void newBloomFilter(String name)
  {
    BloomFilter bf = new BloomFilter(bloomFilterElementSize, bloomFilterErrorRate);
    localCacheManager.put(Constant.BLOOM_FILTER_CACHE, name, bf);
  }

  private void NotExistsThenPut(String name)
  {
    if (!isBloomFilterExistsByName(name))
    {
      newBloomFilter(name);
    }
  }

  public boolean contains(String name, String key)
  {
    NotExistsThenPut(name);
    BloomFilter bf = (BloomFilter) localCacheManager.get(Constant.BLOOM_FILTER_CACHE, name);
    return bf.isPresent(key);
  }

  public void add(String name, String key)
  {
    NotExistsThenPut(name);
    BloomFilter bf = (BloomFilter) localCacheManager.get(Constant.BLOOM_FILTER_CACHE, name);
    bf.add(key);
  }
}

Are there any flaws in this code?

private boolean isBloomFilterExistsByName(String name)
{

The brace belong on the previous line.

isBloomFilterExistsByName(String name)

is...Exists sounds strange.

 private void newBloomFilter(String name)

From the name, I'd expect this method to return a new Bloom Filter instead of putting it somewhere.

 private void NotExistsThenPut(String name)

Java method names starts with lowercase. A better name would be putIfNotExists.

I'd actually prefer a method like getOrCreate.

public boolean contains(String name, String key)

contains and isPresent are wrong names for this. A Bloom Filter is like a Set, but an undependable one. A much better name is mayContain.

This method is an invitation to inefficiency. Imagine a tight loop calling it. On every iteration you look up the Bloom Filter twice (once to make sure that it exists and once to use it). This may easily dominate the cost of the filter's isPresent operation.

I'd suggest to provide methods allowing to use it like

 bloomFilterInCache.getOrCreate("name").mayContain("element");

which allows efficiency via "extract local variable".

 public void add(String name, String key)

Again, an invitation to inefficiency.


Plotting Frequency of Vowels in a Paragraph I wanted to play around with the following idea:

  Give a paragraph, I wanted to find out the relative frequency of usage
  of each of the five vowels. I wanted to plot a pie chart depicting
  this.

Here is my code:

from sys import stdin
from re import sub
import pylab
DataToRead = stdin.read()
VowelData = sub(r"[^aeiou]","",DataToRead) #Take Out Everything which is NOT a vowel
VowelList = ['a','e','i','o','u']
VowelCount = [sum(map(lambda x: x==Vowel,VowelData)) for Vowel in VowelList] # Number of times each vowel appears
print VowelCount
VowelPerc = [x*100.0/sum(VowelCount) for x in VowelCount] #Find the percentage of each
pylab.pie(VowelPerc,None,VowelList,autopct='%1.1f%%')
pylab.show()

It does what it needs to (I think) but I feel there is a better way of doing this (algorithmically speaking). I like Functional Programming so I threw in the Mapper but there is no other reason for that.

I strongly advise against CamelCase for your variable names – consider snake_case instead. Why? Because consistency with existing Python code.

I also suggest better spacing around your operators, e.g. x == vowel instead of x==Vowel and pylab.pie(vowel_perc, None, vowel_list, autopct='%1.1f%%') instead of pylab.pie(VowelPerc,None,VowelList,autopct='%1.1f%%'). In general, consistent, even spacing makes code easier to read.

Do not use map, or more precisely: do not use lambdas. Python has list comprehensions which are exactly as powerful as map and filter, but are considered to be more readable. Here, your line

VowelCount = [sum(map(lambda x: x==Vowel,VowelData)) for Vowel in VowelList]

would become

count = [sum([x for x in vowel_data if x == vowel]) for vowel in vowel_list]

Of course, that is far from optimal as we make five passes through the data. If we use a dictionary, we can reduce this to a single pass:

vowels = "aeiou"
counts = dict([(vowel, 0) for vowel in vowels])
for x in data:
    counts[x] += 1
percentages = [counts[vowel] * 100.0 / len(data) for vowel in vowels]

Note that I got rid of the unnecessary vowel_ prefix here, and that I replaced sum(VowelCount) with the likely cheaper len(data) (but with all “optimizations”, the improvement should be benchmarked, not guessed).

As per the pie-plot documentation, you should consider setting the aspect ratio of your axes, so that the resulting chart doesn't look distorted.


Python Firewall-Connection-Event Filter too slow I've written a little piece of code to filter Firewall-Connection-Events by a certain group of IPs. But the code can't keep up since the input is fairly huge. I am looking for ways to make this code more efficient in order to handle the Firewall-Connection-Events.

[READ_CLIENTS]
The code starts with an empty list (client_IPs). The list is getting filled with unique IPs (read_clients), if the size of the file changes "read_clients" is called again to alter the list.

[READ_EVENTS]
To get the events, I've used While True to loop over the event-File and return the events with yield - if there is no new input, then sleep for 0.1 seconds.

[PROCESS_AND_FILTER]
After this I loop over the generator object to compare each event to each Unique-IP and seperate the result in two files.

    # -*- coding: utf-8 -*-

from IPy import IP
import os
import time

# Create String-Array to compare
path_unique_ips = '/var/log/unique.log'
# Sophos-UTM Packet-Accepted-Event | Connections to Customer-Net
path_sophos_to_customer = '/var/log/packet-accepted-sophos.log'
# match logs
match_log = '/var/log/matched.log'
no_match_log = '/var/log/not_matched.log'
# IP-Filter-Array
client_IPs = []
#get file size of unique ips file
size_ip_file = os.stat(path_unique_ips).st_size

def read_clients(path_unique_ips):
     client_IPs_file = open(path_unique_ips, "r")
     if client_IPs_file.mode == 'r':
               # read all line from client_IPs_file
               new_client_IPs = client_IPs_file.readlines()
               #check for new clients and fill array
               for new_client_IP in new_client_IPs:
                    if new_client_IP not in client_IPs:
                        client_IPs.append(IP(new_client_IP).strNormal())

def read_events(path_sophos_to_customer):
     connection_event_to_customer = open(path_sophos_to_customer, 'r')
     connection_event_to_customer.seek(0, 2)
     if connection_event_to_customer.mode == 'r':
         while True:
            new_event = connection_event_to_customer.readline()
            if not new_event:
                time.sleep(0.1)
                continue
            yield new_event
            #file size of unique IP File changed, re-run the function read_clients
            if size_ip_file is not os.stat(path_unique_ips).st_size:
                read_clients(path_unique_ips)

def process_and_filter(my_events):
    #get events in generator-object from function read_events
    # loop over generator-object, filled with events
    for new_event in my_events:
        print(new_event)
        # loop over event with all ips
        for client in client_IPs:
            # if client-ip found in event write to match.log and break loop to go for next event
            if client in new_event:
                with open(match_log, 'a+') as matched:
                    matched.write(new_event)
                break
        # if ip wasn't in event write event to no_match.log
        else:
             with open(no_match_log, 'a+') as no_match:
                no_match.write(new_event)

if __name__ == '__main__':
     read_clients(path_unique_ips)
     new_events=read_events(path_sophos_to_customer)
     process_and_filter(new_events)

Log_Event_Example :

  Jan 18 14:14:14 17.17.17.17 2019: 01:18-14:14:14 firewall-1
  ulogd[5974]: id="2002" severity="info" sys="SecureNet"
  sub="packetfilter" name="Packet accepted" action="accept" fwrule="653"
  initf="eth1" outitf="eth0" srcmac="aa:bb:cc:dd:ee:ff"
  dstmac="00:11:22:33:44:55" srcip="10.10.10.10" dstip="10.10.10.11"
  proto="6" length="52" tos="0x00" prec="0x00" ttl="127" srcport="58589"
  dstport="22" tcpflags="ACK"

The main performance bottleneck is probably the fact that you keep your already seen IPs in a list. Doing x in list is \$\mathcal{O}(n)\$, so the more IPs there are, the slower this will get. Another factor is that you check if new_client_IP not in client_IPs but then add IP(new_client_IP).strNormal() to the list. So even if you come across the same IP again, it will still not be in the list, since that class probably produces something different than the line (which will have a trailing newline, for example) (and if it does not produce anything different, why have this step at all?) and so you do this possibly expensive step again.

Instead just save them in a set, for which in is \$\mathcal{O}(n)\$. And even better, if you add an already existing element to a set it does not care.

client_IPs = set()
client_IPs_size = 0

def read_clients(path_unique_ips):
    """Check if the file has changed size.
    If it has, extract all IPs from it and update the global variables.
    """
    global client_IPs_size
    current size = os.stat(path_unique_ips).st_size
    if current_size != client_IPs_size:
        client_IPs_size = os.stat(path_unique_ips).st_size
        global client_IPs
        with open(path_unique_ips, "r") as client_IPs_file:
            client_IPs = {IP(line).strNormal() for line in client_IPs_file}

Note that I used with to ensure the file is properly closed, even in an event of an exception. I also used a set as stated above.

I also iterate over the file directly instead of first reading all lines into memory. The whole thing fits nicely inside a set comprehension. I also added a docstring to document what this function does.

Modifying global objects can quickly lead to hard to read code and should be avoided if possible. Here this is a bit harder to avoid, because process_and_filter depends on the global state always being correct at that specific point in the loop. Another way around this would be to use class to keep this global state and make the other functions methods of that class. This might be a bit overkill, though, so I leave that implementation to you if you feel it is necessary.

def read_events(path_sophos_to_customer):
    """Every 0.1 seconds check if a new non-blank line has been added to the end of the file.
    If there has, yield it.

    Read the file containing the list of IPs again if it has changed.
    """
    with open(path_sophos_to_customer, 'r') as connection_event_to_customer:
        connection_event_to_customer.seek(0, 2)   # seek to the end
        while True:
            new_event = connection_event_to_customer.readline()
            if not new_event:
                time.sleep(0.1)
                continue
            yield new_event

            # file size of unique IP File changed, this will read the IPs again
            read_clients(PATH_UNIQUE_IPS)

Since the check for a changed client_IPs is now inside read_clients the if clause is not needed anymore.

I also removed the check if the file is readable. Usually you want your code to either fail loudly (with an exception) if something happens (like a file not being readable) or you catch that exception, do something about it and log that it happened. Having it just silently fail by bypassing it with that if clause does not help you at all.

For the splitting of the events into two logs, I would open both files only once at the beginning. I would also extract the IP from the event (since you have not shown us an example of what an event looks like, I will have to guess and say you could use for example re.search(r'\b(?:\d{1,3}\.){3}\d{1,3}\b', new_event).group()). This moves this from having to iterate over all IPs every time to iterating over the string of new_event, which is hopefully shorter.

def process_and_filter(my_events, match_log, no_match_log):
    """Get events in generator-object from function read_events
    and write either to matched or no match log file.
    """
    with open(match_log, 'a+') as match, open(no_match_log, 'a+') as no_match:
        for new_event in my_events:
            print(new_event)
            ip = extract_ip(new_event)
            file = match if ip in client_IPs else no_match
            file.write(new_event)

With this the calling code becomes:

if __name__ == '__main__':
    new_events = read_events(PATH_SOPHOS_TO_CUSTOMER, MATCH_LOG, NO_MATCH_LOG)
    process_and_filter(new_events)

Note that Python's official style-guide, PEP8, recommends using ALL_CAPS for global constants.


Given a list of numbers and a number, return whether any two numbers from the list add up to For example, given [10, 15, 3, 7] and a number k of 17, return True since 10 + 7 is 17.
How can I improve the below solution?
from itertools import combinations

def add_up_to(numbers, k):

 c_k_2 = list(combinations(numbers, 2))

 return any([True for pair in c_k_2 if sum(pair) == k])

Indents
PEP 8: Use 4 spaces per indentation level.

True if condition else False
The pattern True if condition else False can be replaced by condition if it evaluates to a bool. If you are checking for truthy and falsey values you can replace the pattern by bool(condition).
While this lets us simplify the return statement
return any([sum(pair) == k for pair in c_k_2])

it actually performs worse (as pointed out by Manuel in the comments) without implementing further improvements (see the next point). While you originally create a list of only True values, this simplification will create a list that also includes all the False values, which we do not need at all.

Drop the list
You convert to a list twice in your code, once explicitly by calling the list(...) constructor and once implicitly by wrapping a generator expression with [...]. In your particular use case, both are unnecessary.
When dropping the call to the list constructor, combinations(numbers, 2) will provide an itertools.combinations object, which I would assume behaves similiarly to a generator, which is a lazily evaluated iterable. This way we don't compute and store all possible combinations of numbers in advance, which is perfect for us because we

only need one pair of numbers at a time
want to stop evaluating the expression once we find a pair that matches our criteria

Dropping the brackets [] around our generator expression follows the same logic.
Generator expressions vs. list comprehensions on StackOverflow

Applying these improvements will make the code more efficient, more readable and more concise:
def any_two_elements_add_up_to(numbers, k):
    pairs = combinations(numbers, 2)
    return any(sum(pair) == k for pair in pairs)

# or as a one-liner

def any_two_elements_add_up_to(numbers, k):
    return any(sum(pair) == k for pair in combinations(numbers, 2))

EDIT: Algorithm
I've tried my hand at a faster approach, this is the first solution I came up with. Although it's really simple it still outperforms the above solution by orders of magnitude as it runs in O(n).
Here's the code:
def new_approach(numbers, target):
    counterparts = set()

    for num in numbers:
        if num in counterparts:
            return True

        counterparts.add(target - num)

    return False

Since every number only has one counterpart k - x that would make it add up to k, we can keep a set of all possible counterparts that we've identified so far. Once we come across a number that is in our set of counterparts (i.e. adds up to k with a previous number) we return True. If we don't come across one such number, we simply return False.


JSON REST client proxy generator Trying to create something really lightweight. Sources are on GitHub.

To create a proxy we need to define an interface first, e.g.:

// Fake Online REST API for Testing and Prototyping
[Site("https://jsonplaceholder.typicode.com")]
public interface ITypicode
{
    [Get("posts")]
    Task<BlogPost[]> GetAsync();

    [Get("posts/{0}")]
    Task<BlogPost> GetAsync(int id);

    [Post("posts")]
    Task<BlogPost> PostAsync([Body] BlogPost data);

    [Put("posts/{0}")]
    Task<BlogPost> PutAsync(int id, [Body] BlogPost data);

    [Delete("posts/{0}")]
    Task<BlogPost> DeleteAsync(int id);
}

public class BlogPost
{
    public int UserId { get; set; }
    public int Id { get; set; }
    public string Title { get; set; }
    public string Body { get; set; }
}

At this moment we actually know enough to generate a proxy:

ITypicode typicode = RestClient.Create<ITypicode>();
BlogPost blogPost = await typicode.PutAsync(1, new BlogPost { Body = "Wow!" });
Console.WriteLine(blogPost.Body);

We can inject HttpMessageHandler:

ITypicode typicode = RestClient.Create<ITypicode>(handler);

We can also emit the proxy class for dependency injection:

Type typicodeType = RestClient.Emit<ITypicode>();

About error handling – this exception is thrown for unsuccessful HTTP status codes:

public class RestException : Exception
{
    public RestException(HttpResponseMessage response)
    {
        Response = response;
    }

    public HttpResponseMessage Response { get; }
    public override string ToString() =>
        Response.Content.ReadAsStringAsync().Result;
}

We could specify extra type parameter for the SiteAttribute:

[Site("https://jsonplaceholder.typicode.com", Error = typeof(TypicodeError))]
public interface ITypicode
{
    // …
}

to have generic exception be thrown instead:

public class RestException<TError> : RestException
{
    public RestException(HttpResponseMessage response)
        : base(response)
    {
    }

    public T Error => JsonConvert.DeserializeObject<TError>(ToString());
}

So error response body will be kindly deserialized for us.

It is possible to implement API interface on a server side ASP.NET Web API Controller also to insure compatibility.

What do you think about this design?

To be continued with implementation details.

UPDATE: Adding support for HTTP headers – something like this:

[Site("https://jsonplaceholder.typicode.com")]
public interface ITypicode
{
    [Get("posts/{0}")]
    [Header("X-API-KEY: {1}")]   // req - in
    [Header("Content-Type: {2}; charset={3}")]  // res - out
    Task<BlogPost> GetAsync(
        int id, string apiKey, out string contentType, out string charset);
}

Does it look good? Anything else that might be useful?

Usually when I see your questions there isn't much for me to say, because you usually flesh everything out really well. :) (Probably why this has been unanswered so long.)

That said, I think I do have one comment here:

If you have support to edit the HeaderAttribute or the GetAttribute, I would consider replacing the {0}, {1} (etc.) format symbols with a named format symbol.

[Get("posts/{id}")]
Task<BlogPost> GetAsync(int id);

[Site("https://jsonplaceholder.typicode.com")]
public interface ITypicode
{
    [Get("posts/{id}")]
    [Header("X-API-KEY: {apiKey}")]   // req - in
    [Header("Content-Type: {contentType}; charset={charset}")]  // res - out
    Task<BlogPost> GetAsync(
        int id, string apiKey, out string contentType, out string charset);
}

It makes everything more meaningful.

If it's WebAPI you probably don't have access to the source for these attributes, but you can probably wrap them with a new one to add support for this feature. (Adds a little complexity, but should be really awesome to see happen.)


Thread-safety based on object value I need to create thread-safety on a Web-API method, but only when two requests containing the same property value are ran simultaneously.

I have created a mix of WaitHandles and a ConcurrentDictionary to only hold the threads that have another thread processing a similar item.

Is this code thread-safe? The one that worries me is the ContainsKey with the TryAdd.

private readonly static ConcurrentDictionary _processingGateways = new ConcurrentDictionary();

public Models.Profile GetProfile([FromBody] GetProfileRequest request)
{
    if (!ModelState.IsValid)
        throw new HttpResponseException(Request.CreateErrorResponse(HttpStatusCode.BadRequest, new HttpError(ModelState, true)));

    if (_processingGateways.ContainsKey(request.GatewayId))
        _processingGateways[request.GatewayId].WaitOne();
    _processingGateways.TryAdd(request.GatewayId, new AutoResetEvent(false));

    // Wait for this until prev with same gatewayId has finished
    var device = _clientRegistrar.RegisterMobileEndpoint(Mapper.Map(request));

    AutoResetEvent currentHandle;
    _processingGateways.TryRemove(request.GatewayId, out currentHandle);
    currentHandle.Set();

    return Mapper.Map(device);
}

According to MSDN, all public and protected members of ConcurrentDictionary<TKey, TValue> are thread-safe and may be used concurrently from multiple threads. However, concurrency only applies to the individual actions performed on the instance of the ConcurrentDictionary. The situation will not get messy, if a thread is performing a write operation and an another thread is performing a read operation at the same time. But, there is no protection within the  ConcurrentDictionary class against the situation, when an information of the past is provided, that has since changed and because of that, an invalid operation is performed.

I do not see the reason of calling ContainsKey, since an another thread could remove that particular element between the ContainsKey and the Dictionary value accessor operations and KeyNotFoundException will be thrown.

if (_processingGateways.ContainsKey(request.GatewayId))
// the element could be removed right at this point during execution
    _processingGateways[request.GatewayId].WaitOne(); // KeyNotFoundException is thrown here
_processingGateways.TryAdd(request.GatewayId, new AutoResetEvent(false));

The key to your solution could be the TryGetValue method. It is going to lock the ConcurrentDictionary for reading and return the element (if it exists).

if (_processingGateways.TryGetValue(request.GatewayId, out var Gateway))
    Gateway.WaitOne();
_processingGateways.TryAdd(request.GatewayId, new AutoResetEvent(false));

(Disclaimer: code uses C#7.0 out variables)

Now your are secured against the exception.

If you inspect the code for possible deadlock situations, you might observe that it is possible, that Set gets executed before WaitOne. It is good to know, that the wait returns when all events are set and it does not matter, whether or not that happened before or after wait was called. Great, no deadlocks here!


Optimized computation of pairwise correlations in Python Given a set of discrete locations (e.g. "sites") that are pairwise related in some categorical ways (e.g. general proximity) and contains local level data (e.g. population size), I wish to efficiently compute the mean correlation coefficients between local level data on pairwise locations that are characterized by the same relationships.

For example, I assumed 100 sites and randomized their pairwise relations using values 1 to 25, yielding the triangular matrix relations:

import numpy as np

sites = 100
categ = 25

relations = np.random.randint(low=1, high=categ+1, size=(sites, sites))
relations = np.triu(relations) # set relation_ij = relation_ji
np.fill_diagonal(relations, 0) # ignore self-relation

I also have 5000 replicates of simulation results on each site:

sims = 5000
res = np.round(np.random.rand(sites, sims),1)

To compute the mean pairwise correlation for each specific relation category, I first calculated for each relation category i the correlation coefficient rho[j] between the simulation results res of each unique site pairs j, and then taking the average across all possible pairs with relation i:

rho_list = np.ones(categ)*99

for i in range(1, categ+1):
    idr = np.transpose(np.where(relations == i)) # pairwise site indices of the same relation category
    comp = np.vstack([res[idr[:,0]].ravel(), res[idr[:,1]].ravel()]) # pairwise comparisons of simulation results from the same relation category
    comp_uniq = np.reshape(comp.T, (len(idr), res.shape[1], -1)) # reshape above into pairwise comparisons of simulation results between unique site pairs

    rho = np.ones(len(idr))*99 # correlation coefficients of all unique site pairs of current relation category

    for j in range(len(idr)): # loop through unique site pairs
        comp_uniq_s = comp_uniq[j][np.all(comp_uniq!=0, axis=2)[j]].T # shorten comparisons by removing pairs with zero-valued result
        rho[j] = np.corrcoef(comp_uniq_s[0], comp_uniq_s[1])[0,1]

    rho_list[i-1] = np.nanmean(rho)

Although this script works, but once I increase sites = 400, then the entire computation can take more than 6 hrs to finish, which leads me to question my use of array functions. What is the reason for this poor performance? And how can I optimize the algorithm?

What is the reason for this poor performance? And how can I optimize the algorithm?

You mentioned that sites=100 is a "fast" run, and sites=400 is a "slow" six-hour run, without posting profiling data. I don't know what "fast" is, but if it's around 22 minutes then you're just looking at quadratic growth of your data. I'm going to guess "fast" was less than 22 minutes, and then I'll speculate about your cache size. Perhaps you blew out your L3 (or L2) last-level cache. Which would suggest looking for a divide and conquer approach on overlapping neighborhoods, so each sub-problem is conveniently small enough to fit within cache.

Other than that, perhaps you can "cheat" by changing the problem, approximating results. Consider a fixed budget of 10^4 pairs for which you're willing to compute correlation coefficients. In the sites=400 case, could you maybe rank order pairs so the ones with negligible contribution sort to the bottom of the list, outside of budget? Then they would simply never be considered.


Displaying fields in a sidebar of a custom page template I'm beginning with PHP programming and would appreciate a critique on my code and how I could make it more efficient, rather than repeating the if statements.

What I'm basically doing is getting the fields typed in by the user and displaying these fields in a sidebar of a custom page template.

<?php

            $name = (get_field('name'));
            $address = (get_field('address'));
            $business= (get_field('company'));
            $phone= (get_field('phone'));
            $fax= (get_field('fax'));
            $mobile= (get_field('mobile'));
            $email =  (get_field('email'));
            $personalemail =  (get_field('second_email'));
            $website =  (get_field('website'));
            $facebook =  (get_field('facebook'));
            $twitter =  (get_field('twitter'));
            $linkedin =  (get_field('linkedin'));

            if (!empty($name)) {
                echo $name . "
";
            }

            if (!empty($business)) {
                echo $business . "
";
            }

            if (!empty($address)) {
                echo $address . "
";
            }

            if (!empty($phone)) {
                echo $phone . "
";
            }

            if (!empty($fax)) {
                echo $fax . "
";
            }

            if (!empty($mobile)) {
                echo $mobile . "
";
            }

            if (!empty($email)) {
                echo "$email" . "
";
            }

            if (!empty($personalemail)) {
                echo "$personalemail" . "
";
            }

            if(!empty($website)){
                $final_url = (!preg_match("~^(?:f|ht)tps?://~i", $website))? 'http://'.$website: $website;
                echo "$final_url" . "
";

            }

            if(!empty($facebook)){
                $final_url = (!preg_match("~^(?:f|ht)tps?://~i", $facebook))? 'http://'.$facebook: $facebook;
                echo "";

            }

            if(!empty($twitter)){
                $final_url = (!preg_match("~^(?:f|ht)tps?://~i", $twitter))? 'http://'.$twitter: $twitter;
                echo "";

            }

            if(!empty($linkedin)){
                $final_url = (!preg_match("~^(?:f|ht)tps?://~i", $linkedin))? 'http://'.$linkedin: $linkedin;
                echo "";

            }

            ?>

Your code can be simplified a bit.
The parantheses on the right side is not needed. Instead of

        $name = (get_field('name'));

you can just say

        $name = get_field('name');

However, since you are outputting the values directly, I'd use another approach for preparation. Collect all output strings in an array:

        $result = array();
        $result[] = get_field('name');
        $result[] = get_field('address');
        $result[] = get_field('company');
        $result[] = get_field('phone');
        $result[] = get_field('fax');
        $result[] = get_field('mobile');
        $result[] = mailTo(get_field('email'));
        $result[] = mailTo(get_field('second_email'));
        $result[] = linkTo(get_field('website'));
        $result[] = linkTo(get_field('facebook'), '');
        $result[] = linkTo(get_field('twitter'), '');
        $result[] = linkTo(get_field('linkedin'), '');

The functions mailTo and linkTo will build the links; I'll describe them later. The point is, that now you have a single array with all lines. Some of the lines will be empty, because no data was provided. You can filter those empty lines easily with

         $result = array_filter($result);

To show all the lines with a
 in between, you just implode the array into a single string and echo it:

         echo implode('
', $result);

Now your main functionality is done, and the only thing left is to put the formatting code, that you used repeatedly, into proper functions.

function mailTo($address)
{
    if (isValidEmailAddress($address)) {
        return printf('%1$s', $address);
    } else {
        return '';
    }
}

With this function, you have one single place, where the rendering of an email address is defined, so, if you want to change the appearance (eg. link it to a contact page instead of using mailto), you only have to that once.

Here a new function is introduced to check the validity of the email address. To keep it close to what your original code did, for now we'll only check if it is non-empty.

function isValidEmailAddress($address)
{
    $address = trim($address);
    return !empty($address);
}

You can of course add other validations to it.

The same has to be done to URLs.

function linkTo($url, $linkText = null)
{
    $url = trim($url);
    if (empty($url)) {
        return '';
    }

    if (is_null($linkText)) {
        $linkText = $url;
    }

    if (!preg_match("~^(?:f|ht)tps?://~i", $url)) {
        $url = 'http://' . $url;
    }

    return sprintf('%2', $url, $linkText);
}

As you can see, putting repetitive code sequences into their own functions increases the readability of your code and make future changes easier.


Add one to the last array element The task

is taken from leetcode

  Given a non-empty array of digits representing a non-negative integer,
  plus one to the integer.

  The digits are stored such that the most significant digit is at the
  head of the list, and each element in the array contain a single
  digit.

  You may assume the integer does not contain any leading zero, except
  the number 0 itself.

  Example 1:

  Input: [1,2,3]

  Output: [1,2,4]

  Explanation: The array represents the integer 123.

  Example 2:

  Input: [4,3,2,1]

  Output: [4,3,2,2]

  Explanation: The array represents the integer 4321.

My solution

/**
 * @param {number[]} digits
 * @return {number[]}
 */
var plusOne = function(digits) {
  let i = digits.length;
  let carry = 1;
  let tmp;
  const res = [];
  while(i > 0 || carry) {
    tmp = (--i >= 0 ? digits[i] : 0) + carry;
    carry = tmp / 10 | 0;
    res.unshift(tmp % 10);
  }
  return res;
};

It looks like your logic is fine, but I don't find it very readable. I guess it would be fine if you could clearly explain your logic in a tech interview, but here's how I'd write it just to be more clear.

Use a for loop (that's what they're for!), modify the digits array in place unless they specify you shouldn't, and you can return as soon as you don't have anything left to carry.

var plusOne = function(digits){
    let carry=1;

    for(let i=digits.length-1; i>=0; i--){
        let digitSum = carry + digits[i];
        if(digitSum >= 10){
            digits[i] = digitSum%10;
        }
        else{
            digits[i] = digitSum;
            return digits;
        }
    }

    if(carry == 1){
        digits.unshift(1);
    }
    return digits;
}


Alternative way to extract the most recent file using LINQ All, I have written some code to get the latest files from a directory, the files I am looking for contain the string sProcScript = "sProcScript" and in the selected directory I might have several versions:

sProcScriptBigGuns1001.sql
sProcScriptBigGuns1007.sql
sProcScriptEdAggVal1006.sql
sProcScriptEdAggVal1007.sql
...

and I want to pick up the latest version only (the version number being the number after the file name i.e. 1007 etc.). So in the above this would be sProcScriptBigGuns1007.sql and sProcScriptEdAggVal1007.sql. To do this I wrote the following snippet (as part of a larger method), where strPath is the path to the relevant directory

DirectoryInfo dirInfo = new DirectoryInfo(strPath);
List<FileInfo> listFileInfo = new List<FileInfo>();
listFileInfo = dirInfo.EnumerateFiles().ToList<FileInfo>();

// Loop in to locate special processes.
List<FileInfo> specialProcList = new List<FileInfo>();
foreach (FileInfo fi in listFileInfo)
    if (Path.GetFileNameWithoutExtension(fi.FullName).Contains(sProcScript))
        specialProcList.Add(fi);

// Get distinct.
List<string> nameList = new List<string>();
foreach (FileInfo fi in specialProcList)
{
    string strsProc =
        Path.GetFileNameWithoutExtension(Regex.Replace(fi.Name, @"[\d]", String.Empty));
    if (!nameList.Contains(strsProc))
        nameList.Add(strsProc);
}
List<FileInfo> tmpList;
List<string> tmpNameList = new List<string>();
tmpList = specialProcList.OrderByDescending(f => f.Name).ToList();
foreach (string name in nameList)
{
    foreach (FileInfo fi in tmpList)
    {
        if (fi.Name.Contains(name))
        {
            tmpNameList.Add(fi.Name);
            listFilesFound.Add(fi);
            break;
        }
    }
}

which works great. However, I think there will be a much better and neater way of doing this using LINQ but I am new to LINQ and can't seem to get started with it (the attempts I have made are poor enough to not want to post them here!). Can someone demonstate a better way of doing the above operation using LINQ?

Thanks for your time.

This should do it:

var selectedFileInfos = listFileInfo
    .Where(x => {
        var filename = Path.GetFileNameWithoutExtension(x.FullName);
        return specialProcList.Any(y => filename.Contains(y);})
    .OrderByDescending(x => x.Name)
    .GroupBy(x => Path.GetFileNameWithoutExtension(Regex.Replace(x.Name, @"[\d]", String.Empty))
    .Select(x => x.First());

But be careful: file numbers should be padded with zeros. 0001, 0900, 0999, 1000, 1001. If they are just normally numbered with 1, 900, 999, 1000 etc., even your code will fail.


calc() fallback script I wrote a failback for the fairly new CSS calc() rule. It works fine but I want to use it in a production environment and would appreciate feedback.  Please recommend anything regarding weird/wrong code, possible optimizations, or a way to reduce code size.

// CSS calc() replacement
function calcfailback(){
   var d = document.createElement('div');
   var _body = document.getElementsByTagName('body') [0];
   _body.appendChild(d);
   d.style.visibility = 'hidden';
   d.style.width = "-webkit-calc(10px)";
   d.style.width = "-o-calc(10px)";
   d.style.width = "-moz-calc(10px)";
   d.style.width = "calc(10px)";
   var newwidth = d.offsetWidth;
   if (newwidth == "10"){}
   else{
      function resize(){
         document.getElementById('content').style.height = window.innerHeight - 40 + 'px';
         document.getElementById('content').style.width = window.innerWidth - 300 +'px';
         document.getElementById('sidebar').style.height = window.innerHeight - 40 + 'px';
      };
      resize();
      window.onresize = function (){
         resize();
      }
   };
   _body.removeChild(d)
};
window.onload = calcfailback;

It would probably be a good idea to generalize your code so you can use it with different values. I'm not suggesting you support the full calc() syntax, but making it configurable at least could be useful if you plan on using it in more than one place.

For the code you have though, I just have a couple suggestions:

I typically avoid underscores in variable names, but that's just a preference (although one that a lot of style guides I've seen share).
Replace if (newwidth == "10"){} else with if (newwidth !== 10) {. Instead of doing nothing in the true case, switch the comparison and skip the else. You can also compare directly with the number 10 (using the strict equality operator) instead of doing a string comparison.
Function declarations are technically not legal inside if statements in ECMAScript (they are only allowed at the global scope, or the top level of a function). Browsers tend to support it as an extension, but they behave differently and it can cause problems, so it's best to avoid that. Function expressions are fine, so I assigned an anonymous function directly to window.onresize and called it through that. You could also assign the function to a variable (replace function resize() {...} with var resize = function() {...};).
Instead of calling document.getElementById('content') three times, store the style object in a variable and reference that. You'll gain a speed and file-size advantage.
Semicolons aren't needed after function declarations or after an if statement block.

Here's what I ended up with:

// CSS calc() replacement
function calcfailback() {
    var d = document.createElement('div');

    // 1
    var body = document.getElementsByTagName('body')[0];
    body.appendChild(d);
    d.style.visibility = 'hidden';
    d.style.width = "-webkit-calc(10px)";
    d.style.width = "-o-calc(10px)";
    d.style.width = "-moz-calc(10px)";
    d.style.width = "calc(10px)";
    var newwidth = d.offsetWidth;

    // 2
    if (newwidth !== 10) {
        // 3
        window.onresize = function() {
            // 4
            var contentStyle = document.getElementById('content').style;
            contentStyle.height = window.innerHeight - 40 + 'px';
            contentStyle.width = window.innerWidth - 300 +'px';
            contentStyle.height = window.innerHeight - 40 + 'px';
        };
        window.onresize();
    } // 5
    body.removeChild(d);
} // 5
window.onload = calcfailback;


JavaScript component to display ephemeral notifications I would like to improve my code and organization of JS files so I'm wondering if you guys could tell me what I could do better. I've included a small but typical example of how I often use components.

With this component I'm able to show different kind of notifications to the user, which then disappear after a set amount of time.

I use these components by adding this in the appropriate location: new notification('warning', $('.sourcecode-warning').text(), 20000);. Rip away!

function notification(notificationType, message, delay) {
var _this = this;
this.notificationType = notificationType;
this.className = 'notification';
this.message = message;
this.delay = delay || 3000;
this.removeTimeOut;
this.disappearTimeOut;

this.init = function () {
    $('body').append('

' + _this.message + '

');
    _this.setEvents();

    _this.removeTimeOut = setTimeout(function () {
        _this.startDisappearing();
    }, _this.delay);
};

this.setEvents = function () {
    $('.close-notification').bind('click', function () {
        _this.removeSelf();
    });
};

this.startDisappearing = function () {
    $('.' + _this.className).addClass('disappearing');

    _this.disappearTimeOut = setTimeout(function () {
        _this.removeSelf();
    }, 1200);
};

this.removeSelf = function () {
    clearTimeout(_this.removeTimeOut);
    clearTimeout(_this.disappearTimeOut);

    $('.' + _this.className).remove();
};

if ($('.' + _this.className).length === 0) _this.init();
};

I think you could improve a lot. :)

I don't think you need an init function, it just add stuff and the code is difficult
to follow and understand.

The main function is yet your initialization.

Even the other functions are a big overhead and make the code complicated.

I put inline comment to explain the code.

function notification(notificationType, message, delay) {
  // you don't need to attach things to this, just use variables.
  const classNameSelector = 'notification';
  const disappearingClass = 'disappei'
  if ($('.'+classNameSelector).length === 0)
    return; // if component is still there you want to avoid recreate it, but
            // so don't need to load all the code, just exit at the beginning.

  // what about place in a template or in a hidden div?
  const _notification = $('

' + message +
    '

');

  $(body).append(_notification);

  // here I'm a bit confuse, you have a delay, but you set an arbitrary number
  // to remove the notification box: 1200
  // So I changed it in a way that works for me:
  const startDiappearing = delay / 3
  const disappearingAll = delay - startDiappearing

  // the first timeout is startDisappearing
  let removeTimeout;
  const disappearingTimeOut = setTimeout(function() {
    _notification.addClass('disappearing');
    removeTimeout = setTimeout(function () {
        _notification.remove();
    }, disappearingAll);
  }, startDiappearing);

  // Last but not last the bind to the click event
  _notification.on('click', function() {
    clearTimeout(disappearingTimeOut);
    clearTimeout(removeTimeout);

    _notification.remove();
  });
}

I know you feel there is some duplication of remove() but is just a lib call and just one.

The purpose here is made something simple and short as effectively simple and short, avoiding overengineering some part of code.


16-bit Checksum function in C I was playing with TCP/IP, and came across this 16 bit checksum function:

unsigned short csum(unsigned short *ptr,int nbytes)
{
    register long sum;
    unsigned short oddbyte;
    register short answer;

    sum=0;
    while(nbytes>1) {
        sum+=*ptr++;
        nbytes-=2;
    }
    if(nbytes==1) {
        oddbyte=0;
        *((u_char*)&oddbyte)=*(u_char*)ptr;
        sum+=oddbyte;
    }

    sum = (sum>>16)+(sum & 0xffff);
    sum = sum + (sum>>16);
    answer=(short)~sum;

    return(answer);
}

There were a few things that bothered me about it:

Not using fixed size types (though the author himself stated in the code short was expected to be 16 bits for it to work)
If the input buffer was too large (I believe larger than exactly 65535 bytes) the checksum wouldn't be correct, since the most significant 2 bytes of the checksum variable would overflow.

So I ended up rewriting it:

uint16_t checksum16(void *data, unsigned int bytes){
    uint16_t *data_pointer = (uint16_t *) data;
    uint32_t total_sum;

    while(bytes > 1){
        total_sum += *data_pointer++;
        //If it overflows to the MSBs add it straight away
        if(total_sum >> 16){
            total_sum = (total_sum >> 16) + (total_sum & 0x0000FFFF);
        }
        bytes -= 2; //Consumed 2 bytes
    }
    if(1 == bytes){
        //Add the last byte
        total_sum += *(((uint8_t *) data_pointer) + 1);
        //If it overflows to the MSBs add it straight away
        if(total_sum >> 16){
            total_sum = (total_sum >> 16) + (total_sum & 0x0000FFFF);
        }
        bytes -= 1;
    }

    return (~((uint16_t) total_sum));
}

Since it adds the carry bit right after the sum, you can feed it with a buffer of any size with no worries of the final checksum being incorrect.

After some small tests, the results seem to be matching. Do you guys see any possible bug while using that function? Or any further improvement?

I thought maybe I could add a statement to check if the number of bytes was larger than 65535, and only add the carry on each iteration if it was, otherwise adding the whole sum of carries at the end. That would improve the performance on the cases where the size of the buffer doesn't reach that limit (true to any IP packet, since the header length field is 16 bits, limiting the packet size to 65535).

Since this checksum function is only used for TCP packets, there is no need for supporting packets larger than 65535 bytes. Rather than adding needless overflow checks, you should restrict the bytes parameter by making that a uint16_t.

Note that both the original code and your improvement only work on big endian implementations, unless some outside code byteswapped the whole TCP packet, in which case the code only works on little endian implementations. The + 1 for accessing the last byte makes the latter more probable. This + 1 was not present in the original code, so chances are high that you introduced a bug. Did you test with odd-sized packet length?

You should make the spacing of the code consistent, either by removing all spaces around binary operators (not recommended) or inserting spaces at the appropriate places. Currently it is a mixture of both.

Since you don't modify the incoming buffer, you should declare that. Make the data const void *.

You don't need to write 1 == bytes, since any decent compiler will emit a warning if you would ever accidentally use a single = here. You should write the natural bytes == 1 instead, or if you fear typos, bytes > 0.

The bytes -= 1 is unnecessary and can be left out.


Laravel: Export customer record as CSV Using Laravel, I need to export some value from the DB in a CSV and then upload the CSV via sFTP or return it in a Response.

I'm trying to be use S.O.L.I.D. principles but with this scenario, I'm not sure how to proceed. As I understand I should have one class that handles the CSV, one that handles the sFTP, one for the Response, and one to handle the logic of the model (the mapping in my case). But I don't understand how I can separate them.

Later one, I will have more Model data to export.

<?php

namespace App\Services;

use App\Line;
use Symfony\Component\HttpFoundation\StreamedResponse;
use Storage;

class LinesCsv
{
    const DOCUMENT_TYPE = 20;
    const DELIMITER = ';';

    public function exportCSVFileToSftp($filename = 'export.csv')
    {
        $handle = fopen('php://temp', 'w');
        $handle = $this->buildCsv($handle);
        return Storage::disk('sftp')->put($filename, $handle);
    }

    public function exportCSVFileToResponse($filename = 'export.csv')
    {
        return new StreamedResponse(function () use ($filename) {
            $handle = fopen('php://output', 'w');
            $handle = $this->buildCsv($handle);
            fclose($handle);
        }, 200, [
            'Content-Type'        => 'text/csv',
            'Content-Disposition' => 'attachment; filename="' . $filename . '"',
        ]);
    }

    public function buildCsv($handle, $header = false)
    {
        if ($header) {
            fputcsv(
                $handle,
                array_keys($this->lineMapping(Line::first())),
                self::DELIMITER
            );
        }

        Line::with(['invoice', 'invoice.customer', 'item'])
            ->whereHas('invoice', function ($query) {
                $query->where('is_exportable', 1);
            })
            ->chunk(200, function ($lines) use ($handle) {
                foreach ($lines as $line) {
                    fputcsv(
                        $handle,
                        $this->lineMapping($line),
                        self::DELIMITER
                    );
                }
            });

        return $handle;
    }

    protected function lineMapping(Line $line)
    {
        return [
            'Invoice number'            => $line->invoice->id,
            'Document type'             => self::DOCUMENT_TYPE,
            'Date'                      => $line->invoice->date,
        ];
    }
}

Try to think about this in a more generic way, you basically have:

Select data from DB
Put that data in a CSV (will you in the future want to use other formats like XLSX, etc? If so, we should abstract this step too, but I wont do this right now)
Return that CSV file (doesn't matter to who you're returning)

So we can create a few classes from those steps

Any class that can query the DB, your app probably has a bunch of them
AbstractWriter - this class is an abstraction/interface to write anywhere, you'll need to implement it for specific formats, like Writer/Csv, Writer/Xlsx, Writer/JSON, etc.
AbstractFileReturner - this class is an abstraction/interface to return it to anyone, you'll need to implement it for specific cases, AbstractFileReturner/like Sftp, AbstractFileReturner/HttpResponse, etc.

Any time you need more File formats or Returner, you just implement the AbstractWriter or FileReturner respectively one more time.

Doing all these, you'll be using Single Responsability Principle (each class do only one thing) and Open/Closed (the abstractions/interfaces are closed for modification, but open for extension).

Real code

Ok so I came up with a little bit of code having SOLID in mind and to make it easy to add more file formats and and way outs (Sftp, Response, etc).

This is a photo of the layers I tried to create, it might make the code more understandable.

Now embrace yourself for there are coming a few classes.

I didn't test this code and doens't know much of Laravel, so there might be a few bugs, but you'll get the big picture.

First, a sample controller and model which I thought was your Line.

class SomeModel
{
    // the chunk does not go here, it will go inside who's putting the data into the csv
    public static function someQuery()
    {
        return Line::with(['invoice', 'invoice.customer', 'item'])
            ->whereHas('invoice', function ($query) {
                $query->where('is_exportable', 1);
            });
    }

    public static function lineMapping(Line $line)
    {
        return [
            'Invoice number'            => $line->invoice->id,
            'Document type'             => self::DOCUMENT_TYPE,
            'Date'                      => $line->invoice->date,
        ];
    }
}

class SomeController
{
    public function exportCsvSftpAction()
    {
        $dataCollection = SomeModel::someQuery();
        $filename       = 'output.csv';

        $exporter = new SftpCsvExporter($dataCollection, $filename);
        $exporter->export();
    }

    public function exportCsvResponseAction()
    {
        $dataCollection = SomeModel::someQuery();
        $filename       = 'output2.csv';

        $exporter = new ResponseCsvExporter($dataCollection, $filename);
        $exporter->export();
    }
}

Than there's the first layer: Export, I created an interface to say what it should do (don't mind about how now) and a abstract class that implements some parts of it.

interface Exportable
{
    protected function createFile();

    protected function write($line);

    protected function deliver($filename);
}

abstract class Export implements Exportable
{
    protected $fileHandle = null;
    private   $dataCollection;
    private   $filename;

    public function __construct($dataCollection, $filename)
    {
        $this->createFile();

        // @TODO You should add validation for $dataCollection like
        // checking if it has data, if it really is a Collection so we can call chunk, etc
        // And check $filename for invalid characters
        $this->dataCollection = $dataCollection;
        $this->filename       = $filename;
    }

    protected abstract function createFile();

    protected abstract function write($line);

    protected abstract function deliver($filename);

    // this method is who makes it all work together
    // and let the controller be so simple
    public function export()
    {
        $this->dataCollection->chunk(200, function ($lines) {
            foreach ($lines as $line) {
                $this->write($line);
            }
        });

        $this->deliver($this->filename);
    }
}

Now there's the second layer: Csv, in this layer you will be able to add more file formats in the future if needed.
This layer knows how to write, but doesn't know how to open nor deliver the file.

abstract class CsvExporter extends Export
{
    const DELIMITER = ';';

    private $headerWritten = false;

    protected function write($line)
    {
        if (!$this->headerWritten) {
            fputcsv(
                $this->fileHandle,
                array_keys(SomeModel::lineMapping(SomeModel::first())),
                self::DELIMITER
            );

            $this->headerWritten = true;
        }

        fputcsv(
            $this->fileHandle,
            SomeModel::lineMapping($line),
            self::DELIMITER
        );
    }
}

And finally the third and last layer: this is responsible for opening and delivering the file. So we have Sftp and Response in this layer.

class SftpCsvExporter extends CsvExporter
{
    protected function createFile()
    {
        if ($this->fileHandle !== null) {
            throw new LogicException('Handle already initilized');
        }

        $this->fileHandle = fopen('php://temp', 'w');
    }

    protected function deliver($filename)
    {
        return Storage::disk('sftp')->put($filename, $this->fileHandle);
    }
}

class ResponseCsvExporter extends CsvExporter
{
    protected function createFile()
    {
        if ($this->fileHandle !== null) {
            throw new LogicException('Handle already initilized');
        }

        $this->fileHandle = fopen('php://output', 'w');
    }

    protected function deliver($filename)
    {
        $handle = $this->fileHandle;

        return new StreamedResponse(function () use ($handle, $filename) {
            // I don't know this StreamedResponse so you might want to correct this
            // the only thing left is to close the handle before send the file
            fclose($handle);
        }, 200, [
            'Content-Type'        => 'text/csv',
            'Content-Disposition' => 'attachment; filename="' . $filename . '"',
        ]);
    }
}


Refresh the token before Expiry I have a piece of code that will have to fetch the token to call a different API. The token has an expiry of 10 hours; after 10 hours, it has to re-fetch the token.

I have created a singleton class to do it. Is there anything wrong with this code?

    import java.text.ParseException;
    import java.text.SimpleDateFormat;
    import java.util.Calendar;
    import java.util.Date;
    import java.util.HashMap;
    import java.util.Map;
    import java.util.concurrent.locks.Lock;
    import java.util.concurrent.locks.ReentrantLock;

    public class CredentialsHolder {

        private static CredentialsHolder instance = null;
        private String loginToken;
        private Date loginTokenExpiry;

        private static final String API_BASE_URL_V1 = "https://api.com/rest";
        private static final String API_LOGIN_TOKEN_PATH = "token";
        private static final String API_USER_NAME = "userName";
        private static final String API_PASSWORD = "password";

        private final Lock lock = new ReentrantLock();

        public static synchronized CredentialsHolder getInstance()  {
           if (instance == null)
               instance = new CredentialsHolder();
           return instance;
       }

        private CredentialsHolder()  {
            setAPICredentials();
        }

        private void refreshToken() {
            setAPICredentials();
        }

        private void setAPICredentials() {
            if (Calendar.getInstance().after(toCalendar(loginTokenExpiry))) {
                Map headers = new HashMap();
                headers.put("userName", API_USER_NAME);
                headers.put("password", API_PASSWORD);
                Map criteria = new HashMap();
                criteria.put("headers", headers);
                criteria.put("url", API_BASE_URL_V1 + API_LOGIN_TOKEN_PATH);
                Map response = new ExternalAPICaller().httpGet(criteria);
                Map responseHeaders = ((Map) response.get("headers"));
                System.out.println(responseHeaders);
                loginToken = (String) (responseHeaders.get("Token"));
                try {
                    loginTokenExpiry = new SimpleDateFormat("MM/dd/yyyy h:mm:ss a").parse((String) (responseHeaders.get("TokenExpiry")));
                } catch (ParseException e) {
                    e.printStackTrace();
                    loginTokenExpiry = null;
                }
            }
        }

        public String getAccessToken() {
           if (Calendar.getInstance().after(toCalendar(loginTokenExpiry))) {
              if (lock.tryLock()) {
                 try {
                    refreshToken();
                 } finally {
                    lock.unlock();
                 }
              }
           }
          return loginToken;
        }

        public static Calendar toCalendar(Date date){
            Calendar cal = Calendar.getInstance();
            cal.setTime(date);
            cal.add(Calendar.HOUR, -1);
            return cal;
        }
    }

I have used ReentrantLock to not allow the refresh happen simultaneously by multiple threads. So if a refresh is happening once, the other will continue. (I am trying to refresh the token 1 hour ahead before expiry).

Singleton

There is a definite argument that a Singleton is an anti-pattern.  I won't go into it too much, but just to make it not link-only:

Singletons are hard to unit test, as of course you can only have one.
It's hard to reuse a Singleton if you suddenly find yourself needing two.  E.g. if you have a similar API, you have to copy the whole Singleton even though most of the code is the same.

What should you do instead?  Alternatives to Singleton.

In particular, consider a pattern like

CredentialsHolder apiCredentials = CredentialsHolder.getInstance("api_credentials.txt");

Behind the scenes, this might do a lookup in an instance table.  If found, use it.  If not found, create it.

So

           if (instance == null)
               instance = new CredentialsHolder();

becomes

           CredentialsHolder instance = credentials.get(apiFileName);
           if (instance == null) {
               instance = connectToApi(apiFileName);
               credentials.put(apiFileName, instance);
           }

Configure outside the code

This also moves the login information out of your code and into configuration.  This is often helpful in that you want to share your code but not your configuration.  That is to say, your login information is private, but you are posting your code publicly.  What if you forgot to obscure the info?

Curly brackets

I personally prefer the block form of control structures even when there is only a single statement.  If you do use the single statement form, consider putting it on the same line, e.g.

           if (instance == null) instance = new CredentialsHolder();

But of course, that won't work in my revised code, which has a two statement block.

The counter-argument is that it's not actually necessary in your original code.  But neither is it harmful in your original code.  I find a simple bright line rule of always using them better than trying to evaluate each situation.  This is especially true as the coding situation often changes quickly and in surprising ways.  Using the {} will always work.

Paragraphing

                Map headers = new HashMap();
                headers.put("userName", API_USER_NAME);
                headers.put("password", API_PASSWORD);
                Map criteria = new HashMap();
                criteria.put("headers", headers);
                criteria.put("url", API_BASE_URL_V1 + API_LOGIN_TOKEN_PATH);
                Map response = new ExternalAPICaller().httpGet(criteria);
                Map responseHeaders = ((Map) response.get("headers"));
                System.out.println(responseHeaders);
                loginToken = (String) (responseHeaders.get("Token"));
                try {
                    loginTokenExpiry = new SimpleDateFormat("MM/dd/yyyy h:mm:ss a").parse((String) (responseHeaders.get("TokenExpiry")));
                } catch (ParseException e) {
                    e.printStackTrace();
                    loginTokenExpiry = null;
                }

That's a wall of code, which makes it harder to read.  Consider

                Map<String, String> headers = new HashMap<>();
                headers.put("userName", API_USER_NAME);
                headers.put("password", API_PASSWORD);

                Map<String, Map<String, String>> criteria = new HashMap<>();
                criteria.put("headers", headers);
                criteria.put("url", API_BASE_URL_V1 + API_LOGIN_TOKEN_PATH);

                Map<String, Map<String, String> response = new ExternalAPICaller().httpGet(criteria);
                Map<String, String> responseHeaders = response.get("headers");
                System.out.println(responseHeaders);

                loginToken = responseHeaders.get("Token");
                try {
                    loginTokenExpiry = new SimpleDateFormat("MM/dd/yyyy h:mm:ss a").parse(responseHeaders.get("TokenExpiry"));
                } catch (ParseException e) {
                    e.printStackTrace();
                    loginTokenExpiry = null;
                }

Now it's much easier to see what code goes with what, because the blank lines (vertical whitespace) separate the blocks of code.

You also might consider moving the try/catch block into a separate method.  Then you could say something like

                loginTokenExpiry = parseTokenExpiry(responseHeaders.get("TokenExpiry"));

Don't use raw types

I changed all the raw types to parameterized types.  It's possible that that only works for the ones that you create, like headers and criteria, but I changed all of them.  Change the latter ones back if ExternalAPICaller won't work with them.  And then tell these External people to fix their API.

This eliminates the need to cast the results of get and avoids a compiler warning in some configurations.


Extracting site visit information from a REST API call, including addresses from a MySQL database I'm building a web app that pulls data from a ticketing system via REST API. I'm currently able to pull all of the data I need just fine but the loading process is really slow. I'd love any feedback regarding the optimization of my code.

<?php
include 'schedule.php';

$curl = curl_init();

foreach ($onsites as $value){

curl_setopt_array($curl, array(
    CURLOPT_URL => "https://portal.-----.com/v4_6_release/apis/3.0/service/tickets?conditions=id%3D$value",
    CURLOPT_RETURNTRANSFER => true,
    CURLOPT_ENCODING => "",
    CURLOPT_MAXREDIRS => 10,
    CURLOPT_TIMEOUT => 30,
    CURLOPT_HTTP_VERSION => CURL_HTTP_VERSION_1_1,
    CURLOPT_CUSTOMREQUEST => "GET",
    CURLOPT_HTTPHEADER => array(
        "accept: application/json",
        "authorization: -----",
        "cache-control: no-cache",
        "postman-token: -----",
        "x-cw-usertype: integrator"
    )
));

$response = curl_exec($curl);
$err      = curl_error($curl);

if ($err) {
    echo "cURL Error #:" . $err;
} else {
    $response = json_decode($response, true);

    foreach ($response as $ticket) {
        $ticket_number = $ticket['id'];
        $summary[]       = $ticket['summary'];
        $company[]       = $ticket['company']['name'];
        $site[]          = $ticket['siteName'];
        $addressLine1  = str_replace('# D', '#D', $ticket['addressLine1']);
        $addressLine2  = $ticket['addressLine2'];
        $city          = $ticket['city'];
        $state         = $ticket['stateIdentifier'];
        $zip           = $ticket['zip'];
        $contact[]       = $ticket['contact']['name'];
        $resources     = $ticket['resources'];
        if(empty($addressLine2)) {
            $full_address[]   = "$addressLine1, $city, $state, $zip, USA";
        } else {
            $full_address[]   = "$addressLine1, $addressLine2, $city, $state, $zip, USA";
        }
    }
}
}

mysql_connect("localhost", "root", "") or die(mysql_error());
mysql_select_db("scheduler") or die(mysql_error());

if ($ticket_number = $onsites) {
$current_onsites = array();
$i = 0;

for ($i = 0; $i <= count($ticket_number); $i++) {
    @$current_onsites[$i]['id'] = $ticket_number[$i];
    @$current_onsites[$i]['text'] = $summary[$i];
    @$current_onsites[$i]['location'] = $location[$i];
    @$current_onsites[$i]['company'] = $company[$i];
    @$current_onsites[$i]['site'] = $site[$i];
    @$current_onsites[$i]['location'] = $full_address[$i];
    @$current_onsites[$i]['contact'] = $contact[$i];
    @$current_onsites[$i]['start_date'] = date('Y-m-d H:i:s', strtotime($startDate[$i]));
    @$current_onsites[$i]['end_date'] = date('Y-m-d H:i:s', strtotime($endDate[$i]));
    @$current_onsites[$i]['technician'] = $technician[$i];
    @$current_onsites[$i]['hours'] = $hours[$i];
    @$current_onsites[$i]['status'] = $status[$i];
    @$current_onsites[$i]['done'] = $doneFlag[$i];
    @$current_onsites[$i]['ownerFlag'] = $ownerFlag[$i];
    // Retrieve all the data from the db table
    $result = mysql_query('SELECT * FROM xy WHERE company LIKE "'.@$current_onsites[$i]['company'].'";')
        or die(mysql_error());
    // store the record of the "example" table into $row
    $row = mysql_fetch_array( $result );
    @$current_onsites[$i]['lat'] = $row['lat'];
    @$current_onsites[$i]['lng'] = $row['lng'];
}

print "<pre>";
print_r ($current_onsites);
print "</pre>";

$fp = fopen('results.json', 'w');
fwrite($fp, json_encode($current_onsites));
fclose($fp);
}

curl_close($curl);

?>

Some thoughts:

I honestly don't see how this code "works" as shown.  There are significant problems in your code as I will detail below.
Don't use mysql_* functions.  They have been deprecated in PHP for several years now (since PHP 5.5 in June 2013) and are now totally removed from PHP 7. Consider mysqli or PDO (mysqli being the most similar to mysql in syntax)
It is not clear why you have so many loops here. This will slow down your code if you have large result sets you are working with.  There doesn't seem to be any reason that you couldn't build your SQL query in that first foreach loop after the curl call.
There seems to be a significant mismatch between the variables set in the first foreach loop after the curl call and the foreach loop where you do your DB queries.  There are some variables in the first loop that are not used in the second (so why set them at all?), and there are variable references in the second loop that aren't set in the first.  So either there is more code here that you are not showing, or there is no way this code actually works.
You have very minimal code comments, so it makes it really hard to understand what your intent is in large areas of the code.

Below, I have made some more specific code comments within multi-line comments

<?php
/*
Does your code absolutely need this to run?
If so, this should probably be a require
*/
include 'schedule.php';

/*
After this line you do nothing to check that curl_init() was successful
and continue operationg assuming happy path. What if init fails?
*/
$curl = curl_init();

/*
This could be contributing to your performance problem as you are making these
requests in a loop.  You might consider curl_multi_exec() to make
requests in parallel
*/
foreach ($onsites as $value){

/*
Indent so it is clear this all happens inside above loop
*/
curl_setopt_array($curl, array(
    CURLOPT_URL => "https://portal.-----.com/v4_6_release/apis/3.0/service/tickets?conditions=id%3D$value",
    CURLOPT_RETURNTRANSFER => true,
    CURLOPT_ENCODING => "",
    CURLOPT_MAXREDIRS => 10,
    CURLOPT_TIMEOUT => 30,
    CURLOPT_HTTP_VERSION => CURL_HTTP_VERSION_1_1,
    CURLOPT_CUSTOMREQUEST => "GET",
    CURLOPT_HTTPHEADER => array(
        "accept: application/json",
        "authorization: -----",
        "cache-control: no-cache",
        "postman-token: -----",
        "x-cw-usertype: integrator"
    )
));

$response = curl_exec($curl);
$err      = curl_error($curl);

/*
If this is script that is not exposed to users echoing out errors might be OK.
If this is user-facing, you probably want to log errors and give
user-friendly error messaging
*/
if ($err) {
    echo "cURL Error #:" . $err;
} else {
    $response = json_decode($response, true);

/*
There is no way your application is working the way you expect as
you are overwriting most of these variables with each loop iteration
without actually doing anything with them first
*/
    foreach ($response as $ticket) {
        $ticket_number = $ticket['id'];
        $summary[]       = $ticket['summary'];
        $company[]       = $ticket['company']['name'];
        $site[]          = $ticket['siteName'];
        $addressLine1  = str_replace('# D', '#D', $ticket['addressLine1']);
        $addressLine2  = $ticket['addressLine2'];
        $city          = $ticket['city'];
        $state         = $ticket['stateIdentifier'];
        $zip           = $ticket['zip'];
        $contact[]       = $ticket['contact']['name'];
        $resources     = $ticket['resources'];
        if(empty($addressLine2)) {
            $full_address[]   = "$addressLine1, $city, $state, $zip, USA";
        } else {
            $full_address[]   = "$addressLine1, $addressLine2, $city, $state, $zip, USA";
        }
    }
}
}
/*
Don't use mysql_* functions
*/
mysql_connect("localhost", "root", "") or die(mysql_error());
mysql_select_db("scheduler") or die(mysql_error());

/*
Are you really trying to do an assignment here or a comparison?
You are actually now doing an assignment which is then evaluated
for it's truthiness.
If you are trying to do a comparison, you need to actually use
== or ===
I am not sure why you would be comparing an array ($onsites) against
a single string value ($ticket_number) though.
*/
if ($ticket_number = $onsites) {
/*
Indent everything inside braces
*/
$current_onsites = array();
$i = 0;

/*
Why all the error suppression here?  That is REALLY bad coding practice.
Again since you are overwriting variables in loop above, there is no way
this is working correctly if you have more than one ticket in response.
Querying in a loop should usually be a red flag. You are usually better
off trying to get all the data you need in a single query if possible,
which it appears to be in this case. You could actually build array
of all values you are trying to get from the database and have a
single query such as:
SELECT company, lat, lng FROM xy
WHERE company IN ('company1', 'company2', ...)
*/
for ($i = 0; $i <= count($ticket_number); $i++) {
    @$current_onsites[$i]['id'] = $ticket_number[$i];
    @$current_onsites[$i]['text'] = $summary[$i];
    @$current_onsites[$i]['location'] = $location[$i];
    @$current_onsites[$i]['company'] = $company[$i];
    @$current_onsites[$i]['site'] = $site[$i];
    @$current_onsites[$i]['location'] = $full_address[$i];
    @$current_onsites[$i]['contact'] = $contact[$i];
    @$current_onsites[$i]['start_date'] = date('Y-m-d H:i:s', strtotime($startDate[$i]));
    @$current_onsites[$i]['end_date'] = date('Y-m-d H:i:s', strtotime($endDate[$i]));
    @$current_onsites[$i]['technician'] = $technician[$i];
    @$current_onsites[$i]['hours'] = $hours[$i];
    @$current_onsites[$i]['status'] = $status[$i];
    @$current_onsites[$i]['done'] = $doneFlag[$i];
    @$current_onsites[$i]['ownerFlag'] = $ownerFlag[$i];
    // Retrieve all the data from the db table
/*
Your LIKE is meaningless here since you have no wildcard.  Just use =
or IN if you take above advice and remove query from the loop.
Don't use SELECT *, it is really bad coding habit.  Just specify
the fields you actually need - in this case 'lat' and 'lng'.
*/
    $result = mysql_query('SELECT * FROM xy WHERE company LIKE "'.@$current_onsites[$i]['company'].'";')
        or die(mysql_error());
    // store the record of the "example" table into $row
    $row = mysql_fetch_array( $result );
    @$current_onsites[$i]['lat'] = $row['lat'];
    @$current_onsites[$i]['lng'] = $row['lng'];
}

print "<pre>";
print_r ($current_onsites);
print "</pre>";

$fp = fopen('results.json', 'w');
fwrite($fp, json_encode($current_onsites));
fclose($fp);
}
/*
You should close this resource right after you are done working with it.
*/
curl_close($curl);

?>

If you have any desire to leverage curl_multi_exec, feel free to use this simple REST client I have written (either as is, or as inspiration for your own code) - https://github.com/mikecbrant/php-rest-client


Two index computations differing in execution time Code

I have the following piece of code

        #define equidIND(divid, recip) (size_t)((divid) * (recip) + 0.5)
        ...
        // Get index for average velocity
        //idx_velo_avg    = (size_t)(velo_avg * funmaps_velo->map_velo_step_recip + 0.5);
        idx_velo_avg = equidIND(velo_avg, funmaps_velo->map_velo_step_recip);
        // Get effective velocity
        velo_eff = sqrt(velo_pos_k * velo_pos_k + accel * delta_s);
        // Get index for effective velocity (HOTSPOT)
        idx_velo_eff = equidIND(velo_eff, funmaps_velo->map_velo_step_recip);

Explanation

It contains three lines with operations

First line - idx_velo_avg = ...  Getting index by value using rounding/NNinterpolation:

Mapping value to index by division turned into a multiplication with the reciprocal:
velo_avg * funmaps_velo->map_velo_step_recip
Adding 0.5 to the resulting double
Truncating using a cast (size_t). The values are always > 0 btw.

The second line computes then the effective velocity, using a call to sqrt.
The third line does exactly the same as the first, but with a different base value (first line velo_avg - third line velo_eff)

Profiling

Very Sleepy and AMD CodeXL give similar profiling results

The snapshot is taken from AMD CodeXL.

Question(s)

What I wonder is:

Is this way of computing an index a good, fast and secure one?
Why does the third line apparently more than 4 times slower than the first. At least this is how I interpret the percentage values in CodeXL and Very Sleepy kind of confirms this interpretation. Has it to do with the intermediate call to sqrt?

ASM

As additional information, the assembly view shows the following (link to image)

These two statements are functionally identical:

idx_velo_avg = (size_t)((velo_avg) * (funmaps_velo->map_velo_step_recip) + 0.5);

idx_velo_eff = (size_t)((velo_eff) * (funmaps_velo->map_velo_step_recip) + 0.5);

The only difference is the variable on the left side of the multiplication. Since velo_eff is calculated just before the second of these statements, we can safely assume that the slowdown isn't caused by a cache miss.

So I think the only possible explanation is that the processor has to wait for the result of the square root calculation to emerge from the processing pipeline before it can start calculating idx_velo_eff.

Try calculating the square root first, and see if that helps:

velo_eff = sqrt(velo_pos_k * velo_pos_k + accel * delta_s);
idx_velo_avg = equidIND(velo_avg, funmaps_velo->map_velo_step_recip);
idx_velo_eff = equidIND(velo_eff, funmaps_velo->map_velo_step_recip);

Also, check your compiler optimization settings. A decent compiler should be able to do this sort of thing for you.


Singly linked list implementation I feel like there is room for improvement in the push/pop methods. Any suggestions?

#include <exception>

template <typename T>
class List
{
public:

                    List();
                    ~List();
    void            PushBack(const T &data);
    void            PushFront(const T &data);
    void            PopBack();
    void            PopFront();
    void            Clear();
    bool            IsEmpty() const;
    unsigned int    Size() const;
    T&              Front() const;
    T&              Back() const;

private:

    template <typename T>
    struct Node
    {
        Node(const T &data) : data(data), next(NULL) {}
        T      data;
        Node  *next;
    };

    Node<T> *head;
    Node<T> *tail;
};

template <typename T>
List<T>::List()
    : head(NULL),
      tail(NULL)
{

}

template <typename T>
List<T>::~List()
{
    Clear();
}

template <typename T>
void List<T>::PushBack(const T &data)
{
    if ( !head )
    {
        head = new Node<T>(data);
        tail = head;
        return;
    }

    tail->next = new Node<T>(data);
    tail = tail->next;
}

template <typename T>
void List<T>::PushFront(const T &data)
{
    Node<T> *tmp = head;
    head = new Node<T>(data);
    head->next = tmp;

    if ( !tail )
    {
        tail = head;
    }
}

template <typename T>
void List<T>::PopBack()
{
    //empty list
    if ( !head )
    {
        throw std::out_of_range("Can't pop from empty list");
    }

    //list with one element
    if ( head == tail )
    {
        delete head;
        head = NULL;
        tail = NULL;
        return;
    }

    Node<T> *current = head;
    while ( current->next != tail )
    {
        current = current->next;
    }

    delete tail;
    tail = current;
    tail->next = NULL;
}

template <typename T>
void List<T>::PopFront()
{
    //empty list
    if ( !head )
    {
        throw std::out_of_range("Can't pop from empty list");
    }

    //list with one element
    if ( head == tail )
    {
        delete head;
        head = NULL;
        tail = NULL;
        return;
    }

    Node<T> *tmp = head->next;
    delete head;
    head = tmp;
}

template <typename T>
void List<T>::Clear()
{
    Node<T> *current = head;
    while ( head )
    {
        current = head;
        head = head->next;
        delete current;
    }

    head = NULL;
    tail = NULL;
}

template <typename T>
bool List<T>::IsEmpty() const
{
    return ( head == NULL );
}

template <typename T>
unsigned int List<T>::Size() const
{
    unsigned int size = 0;
    for ( Node<T> *current = head; current; current = current->next )
    {
        ++size;
    }

    return size;
}

template <typename T>
T& List<T>::Front() const
{
    if ( !head )
    {
        throw std::out_of_range("Can't return value from empty list");
    }

    return head->data;
}

template <typename T>
T& List<T>::Back() const
{
    if ( !head )
    {
        throw std::out_of_range("Can't return value from empty list");
    }

    return tail->data;
}

Consider using a different naming convention for your data members and methods, such as camelCase or snake_case.  Your user-defined types are already uppercase, which is okay.  Whichever you choose, it's more readable to keep them separate.
The push/pop methods here imply that this is a stack implementation, but a linked list should be capable of inserting or removing a node at any location in the list.  Consider adding additional functions for this.  For an idea of typical linked list functions, read this.
Since you're using a list class, you should have a size member to maintain.  Your Size() method is calculating the size at each call by traversing, which is O(n).

Instead, have a size data member and update it with other operations:

initialize to 0 in the initializer list
increment with each push
decrement with each pop
reset to 0 with each clear

When you call Size(), it should just return the data member value, which is O(1).

It should also return a std::size_t.  It is closest to the return type used in STL containers, and a very large list size may not fit inside an unsigned int.
Instead of having a return in PushFront(), put the last two lines into an else block.
Shouldn't popBack() check tail if PopFront() checks head?  Although they may be pointing to the same node if there's only one node, but the intent is more clear this way.
You should probably also have a Front() and Back() that return a T const&, in case the reference will not be modified or a const value is needed.
You should have some way of displaying the list.  For that, consider overloading operator<< for the List class (instead of a display function), allowing you to do this:

List list;
std::cout << list;

You could also overload Node's operator, allowing you to output a node within the List class.
std::out_of_range is defined in <stdexcept>, not <exception>.


Individuals reproduce and mutate How can I improve the performance (in terms of computation time) of this code?

# Settings
const nbloci = 100         # length of the genome
const N = 100              # Number individuals in the population
const nbgenerations = 100  # number of generations
const mu = 1/10^5          # mutation rate
const s = 0.01             # effect of a given mutation on fitness

# Packages
using Distributions.Binomial
using Distributions.wsample

# Type and Functions
type Ind
    Genome
end

function makepopulation(N)
    Pop = Array(Ind, N)
    for i = 1:N
        Pop[i] = Ind(ones(Float64, nbloci))
    end
    return(Pop)
end

function calculatefitnesses(Pop)
    fitnesses = Array(Float64, N)
    for (i,I) = enumerate(Pop)
        fit = 1
        for l = I.Genome
            fit *= l
        end
        fitnesses[i] = fit
    end
    return(fitnesses)
end

function mutate(I)
    nbmut = rand(Binomial(nbloci, mu), 1)[1]
    mutposs = rand(1:nbloci, nbmut)
    for mutpos = mutposs
        I.Genome[mutpos] == 1.0 ? I.Genome[mutpos] = mutfit : I.Genome[mutpos] = 1.0
    end
end

function reproduce(Pop)
    fitnesses = calculatefitnesses(Pop)
    newPop = Array(Ind, N)
    for i = 1:N
        newPop[i] = deepcopy(wsample(Pop, fitnesses, 1)[1])
        mutate(newPop[i])
    end
    return(newPop)
end

##### MAIN #####

# Just a useful constant
const mutfit = 1.0-s

# Simulation
Pop = makepopulation(N)             # Create population
for generation = 1:nbgenerations    # Iterate over all generations
    Pop = reproduce(Pop)
end

The above code simulates a population of individuals (Ind) that all have a genome long of nbloci (independent) loci (a locus (=sing. of loci) is a position on a chromosome). makepopulation creates a population of clones at the beginning of the simulation. Then, iterating over all generations (for loop) the function reproduce is called on the population. The function reproduce calls calcfitnesses who return an array of fitnesses. reproduce sample the individuals according to fitnesses and then mutate the sampled individuals by calling the function mutate. After reproducing, all individuals die (no overlap of generations).

In mutate, each locus has a probability mu of mutating. In calcfitness an individual that has for example 8 mutant alleles (allele=variant of a gene) has a fitness of (1-s)^8. In reproduce, individuals are chosen to reproduce with some probability given by their fitness. N designates the population size and nbgenerations obviously designates the number of generations to simulate.

My main doubts and questions

The calcfitness function might eventually be improved by first counting the number of mutants and then using a power function to calculate the fit of an Ind. In such case, I may eventually want to code True/False instead of the Float64 objects 1.0/1.0-s in their genome. I could as well have an Array where at position i is the fitness of on individual that has i mutations. What do you think would be the best solution?
I feel like I used lots of functions and an eventually unnecessary type. It has the advantage to make the code easier to read, but does it significantly slow down the simulations?
I am not 100% sure about the use of the const prefix in my settings either. Would I be better off passing all these settings to each function instead of defining them as constant?
There might have a better solution than deep-copying all individuals forming the next generation. Would it be better to sample them all first and then to deep-copy only the individuals that has been sampled more than once?

You are giving very short names and commneting next to them the meaning

const nbloci = 100         # length of the genome
const N = 100              # Number individuals in the population
const nbgenerations = 100  # number of generations
const mu = 1/10^5          # mutation rate
const s = 0.01             # effect of a given mutation on fitness

It would be better to give long and self-explanatory names without comments:

const genome_length = 100
const population_size = 100
const no_generations = 100
const mutation_rate = 1/10^5
const s = 0.01                # effect of a given mutation on fitness # Too criptic for  me to understand

Many beginners think:

  If I write many many many comments everywhere in my code, it will for sure become better!

But this is not True, you should remove all the meaningless comments such as:

# Settings
# Packages
# Type and Functions

  Maybe using functions will slow down my code very much?

No, the opposite, code inside functions runs much faster.

But you should

  Avoid writing overly-specific types

(Reference is again the link above).

for l = I.Genome

Readibility counts, avoid using names like l and I because it is easy to mistake one for the other.

mutpos = mutposs

same as above.

  Is there a way to improve the calcfitness step, maybe by coding
  true/false instead of 1/1-s?

No, fitness must be a number, if you use a true/false value for the fitness then you are not searching genetically but randomly.