Summing same index values in 2D arrays in javascript After solving a problem, I came to this conclusion.

let n = 10;
let array = [
  [1, 5, 3],
  [4, 8, 7],
  [6, 9, 1]
];
let answer = array.map((val) => new Float32Array(n).fill(val[2], val[0] - 1, val[1]));
console.log(Math.max(...answer.reduce((r, a) => a.map((b, i) => (r[i] || 0) + b), [])))

Okay, so what this does is it creates an array of n elements with the value of the third element from the index from the range of the first value to the second value.

Indexes start from 1 here.

Like for [1, 5, 3], (n = 10), it creates an array of 10 elements with the value 3 from the index 1 to the index 5 and else everything is just blank or 0.

Then for [4, 8, 7], (n = 10), it creates an array of 10 elements with the value 7 from the index 4 to the index 8 and else, everything is just blank or 0.

Then for [6, 9, 1], (n = 10), it creates an array of 10 elements with the value 1 from the index 6 to the index 9 and else, everything is just blank or 0.

Now we have to sum all the matching columns and return the maximum value, which I do by

Math.max(...answer.reduce((r, a) => a.map((b, i) => (r[i] || 0) + b), []))

Everything works fine and the test cases work but the program is not at all efficent, with refrence to this problem, my code just timeouts.

I think the problem is when I sum the values. That is what is taking the most memory.

What can be an alternate method to sum the same index array values? Being it the most efficient

The problem is, that you create an array of length n for each operation (in the input array), and then sum them by calling reduce on the result and after that you finally find the maximum of that resulting array. But you only need one initial array to sum up in, and you can find the maximum in the same operation - here in a rather verbose form:

function arrayManipulation(n, arr) {
  var res = [];
  var max = 0;

  for (var i = 0; i < arr.length; i++) {
    for (var j = 0; j < n; j++) {
      if (i === 0) {
        res.push(j >= arr[i][0] - 1 && j <= arr[i][1] - 1 ? arr[i][2] : 0);
      }
      else {
        res[j] += j >= arr[i][0] && j <= arr[i][1] ? arr[i][2] : 0;
      }
      max = Math.max(max, res[j]);
    }
  }
  return max;
}

which can be narrowed down to this more succinct form:

function arrayManipulation(n, arr) {
  var res = [];
  var max = Number.MIN_VALUE;

  for (var i = 0; i < arr.length; i++) {
    for (var j = arr[i][0]; j <= arr[i][1]; j++) {
      res[j] = (res[j] || 0) + arr[i][2];
      max = Math.max(max, res[j]);
    }
  }
  return max;
}

Here is used, that it is only needed to loop through the intervals of the array that is actually manipulated by each operation in the input array. So for instance in the example operations, it is for the first operation only necessary to add 3 to the values in places from 1 to 5 inclusive.

Update

Building on the idea from the comment by weegee a modification of my solution above could handle that:

function arrayManipulation(n, arr) {
  arr = arr.sort((a, b) => a[0] < b[0] ? -1 : a[0] === b[0] ? 0 : 1);
  var max = Number.MIN_VALUE;

  for (var i = 0; i < arr.length; i++) {
    var sum = arr[i][2];
    for (var j = i + 1; j < arr.length; j++) {
      if (arr[j][0] <= arr[i][1])
        sum += arr[j][2];
      else
        break;
    }
    max = Math.max(sum, max);
  }

  return max;
}

First the operations are sorted according to their start position (first element in the sub array), and then the operations are iterated and summed up as long as there is an overlap from operation to operation. Finally the current sum is compared to the existing maximum sum in max.


String Extension improvement I am creating a string extension to validate some input.

My scenario is when we have a string it will format it according to following guideline.
If the sample string is “One Two Three Four Five” and the length is <=20 return same. If more than 20 then “One Two Three F Five”. If still length is more than 20 the “One Two T  F Five” etc...

Is there any way I can improve this code? I thought it can use recursion?

public static string ToShortName(this string name)
        {
            if (name.Length <= 20) return name;

            var namelist = name.Split(' ');

            var length = name.Length;

            //add spaces
            length = length + namelist.Count() - 1;

            var output = "";
            var indexList = new List<int>();
            for (var i = namelist.Count() - 2; i >=1 ; i--)
            {
                indexList.Add(i);
                if (length - namelist[i].Length <= 20)
                {
                    break;
                }
                length = length - namelist[i].Length;
            }

            foreach (var i in indexList)
            {
                namelist[i] = namelist[i].Substring(0, 1);
            }
            output = string.Join(" ", namelist);

            return output;
        }

You should remove this

length = length + namelist.Count() - 1;

because length already includes the spaces.

This

length = length - namelist[i].Length;

should be

length -= namelist[i].Length - 1;

because namelist[i].Length-1 is the number of characters that are removed.

Also: why i >=1 - 1 instead of i >= 0?

You can also do it in a single loop like this:

        for (var i = namelist.Count() - 2; i >=1 && length > 20; i--)
        {
            length -= namelist[i].Length - 1;
            namelist[i] = namelist[i].Substring(0, 1);
        }

        return string.Join(" ", namelist);


Bash script that updates Intel e1000e driver I am now writing a small bash script that updates an Intel NIC driver to the latest version from the official website. Is there any way to improve\simplify the script? I want to avoid a lot of "if...else" stuff. Helpful tips will be appreciated!

#! /bin/bash

DRV_PKG_NAME="e1000e-3.0.4.tar.gz"
DRV_PKG_URL="http://downloadmirror.intel.com/15817/eng/e1000e-3.0.4.tar.gz"

# Downloading driver and extracting it to current directory
echo "Downloading and extracting driver package..."

if wget -q ${DRV_PKG_URL} && tar zxvf ${DRV_PKG_NAME} >/dev/null ; then
    echo "Done!"
else
    exit 1
fi

# Installing required build dependencies if necessary
echo "Installing build dependencies..."

if apt-get install -y build-essential linux-headers-$(uname -r) >/dev/null ; then
    echo "Done!"
else
    exit 1
fi

# Going into the driver source directory
cd e1000e-3.0.4/src/

# Building driver and updating initramfs
echo "Building module and updating initramfs..."

if { make install && update-initramfs -k all -u; } >/dev/null ; then
    echo "Done!"
else
    exit 1
fi

echo "Purging unnecessary build dependencies..."

if apt-get -y purge build-essential >/dev/null ; then
    echo "Done!"
else
    exit 1
fi

# Restarting iface
echo "Restarting iface!"

{ ifdown eth0  && ifup eth0; } &>/dev/null

# Checking installed driver version
if [[ $(modinfo -F version e1000e) == "3.0.4-NAPI" ]] ; then
    echo "Driver succesfully installed!"
    exit 0
else
    echo "Something Wrong...Try to re-install!"
    exit 1
fi

Simplifying the if-else that exit on failure

When the failure case of an if exits, you could move the success case out of the if, right after it, which will make it a bit shorter. And instead of an if, you could further simplify using the || operator. For example given this pattern:

if cmd; then
    echo "Done!"
else
    exit 1
fi

You could simplify as:

cmd || exit 1
echo "Done!"

You could apply this to all your ifs where you exit in case of failure.

But rather than just fail quietly, it would be better to do as @dopeghoti suggested and create a fail function and print some text in case of failure.

fail() {
    echo "$*"
    exit 1
}

cmd || fail 'Ouch, command failed'
echo "Done!"

Excessive comments

You have many obvious comments that add no value at all:

# Downloading driver and extracting it to current directory
echo "Downloading and extracting driver package..."

# Going into the driver source directory
cd e1000e-3.0.4/src/

In general, it's best when the code is self-explanatory. And in your script it's already the case, so you can remove all comments.

Error checking

Although you were careful to check errors in general, you forgot one:

cd e1000e-3.0.4/src/

Sure, if the tar earlier was successful, then this directory must exist, right? But what if one year later you update your script to get the package from e1000e-3.1.14.tar.gz instead and (heaven forbid) forget to update this directory name later in the script? If the cd command fails the script will happily continue and run the remaining commands, building whatever source you may have in the current directory instead of the right one. This will be safer:

cd e1000e-3.0.4/src/ || fail oopsie

Use more constants

It would be better to move more constants near the top of the file:

DRV_BASE_NAME=e1000e-3.0.4
DRV_SRC_DIR=$DRV_BASE_NAME/src
DRV_PKG_NAME=$DRV_BASE_NAME.tar.gz
DRV_PKG_URL=http://downloadmirror.intel.com/15817/eng/$DRV_PKG_NAME
MODULE_NAME=e1000e
MODULE_VERSION=3.0.4-NAPI

It will make the script easier to maintain.


ListView + ArrayAdapter: Setting OnClickListener to items I've recently written a Java program that displays a list of Word objects using a ListView and a custom ArrayAdapter.
I also want to set an OnClickListener on the items that plays an audio file (specified in the Word class).
This is the code I've written for it:
WordAdapter.java:

/* Set OnClickListener on item */
    // Create MediaPlayer for the audio file
    final MediaPlayer audio = MediaPlayer.create(getContext(), currentWord.getAudioResourceId());
    // Set OnClickListener
    listItem.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            if (!audio.isPlaying()) {
                audio.start();
            }
        }
    });

Is there a more efficient way to accomplish this behaviour?

Well it's not about performance in this case, but you could use dependency injection, which means you could pass the instance of the listener outside to the Adapter. Then just say

listItem.setOnClickListener(onClickListner);
This could make your code more readable

UPDATE

Example shows, how to pass an OnClickListener outside from the class.

So, I would like to add more code, to make sure we can learn from it together:

public class Malacka{

  private OnClickListener callbackListener;

  public Malacka(OnClickListener callback){
     this.callbackListener = callback;
  }

  public void getView(View view, int position, View parent){
    // apply viewHolder pattern here and do your stuff
   itemView.setOnClickListener(callbackListener);
  }
}


Bit exchange operation I have been working on a certain task these days and after several hours of torture, I have done it! However, I believe the code that I have "crafted" is quite difficult to be understood and to be honest, I wouldn't be able to understand it if I wasn't the one who made it. Anyways, this is my code. I would like to receive some advice about what I could do to make it a little bit more simple and, by any chance, shorter:

 using System;
 class BitExchangeAdvanced
  {
    static void Main()
{
    int  num1, num2,  p, q, k, loop;
    uint n, n1, bit;
    while (true)
    {
     // input number

        Console.WriteLine("Input n:");
        if (uint.TryParse(Console.ReadLine(), out n))
        {
            if (n >= 0)
            {
                break;
            }
        }
    }

    // input first position
    while (true)
    {
        Console.WriteLine("Input p:");
        if (int.TryParse(Console.ReadLine(), out p))
        {
            if (p >= 0)
            {
                break;
            }
        }
    }

    // input second position

    while (true)
    {
        Console.WriteLine("Input q:");
        if (int.TryParse(Console.ReadLine(), out q))
        {
            if (q >= 0)
            {
                break;
            }
        }
    }

    // input the limit
    while (true)
    {
        Console.WriteLine("Input k:");
        if (int.TryParse(Console.ReadLine(), out k))
        {
            if (k > 0)
            {
                break;
            }
        }
    }

        // check which position is first
    if (p>q)
    {
        num1 = q;
        num2 = p;
    }
    else
    {
        num1 = p;
        num2 = q;
    }

        // the loop is used in the "for" loop
    loop = num1;

        // check if the inpputs are valid for completing the operation
    if (num2 - num1 < k || num1 + k > 32 || num2 + k > 32)
    {
        Console.WriteLine("Overlapping or out of range");
    }
    else
    {

        // use "for" in order to complete the formula {p, p+1, …, p+k-1}
        for (num1 = loop; num1 < (loop + k); num1++)
        {
                bit = (n >> num1) & 1; // check the first value
                if (bit == 1)
                {
                    n1 = (bit << num1 + (num2 - num1)) | n; // input the value to its analog in num2
                    bit = (n >> num1 + (num2 - num1)) & 1; // complete the opposite operation
                    if (bit == 1)
                    {
                        n1 = (bit << num1) | n1;
                    }
                    else
                    {
                        n1 = ~((bit + 1) << num1) & n1;
                    }
                }
                else
                {
                    n1 = ~((bit + 1) << num1 + (num2 - num1)) & n; // same as the above but with different operations
                    bit = (n >> num1 + (num2 - num1)) & 1; // complete the opposite operation again
                    if (bit == 1)
                    {
                        n1 = (bit << num1) | n1;
                    }
                    else
                    {
                        n1 = ~((bit + 1) << num1) & n1;
                    }
                }
                n = n1;
                num2 = num2 + 1; // completes the second formula {q, q+1, …, q+k-1}
        }

        Console.WriteLine("The result is: {0}", n); // print the result
    }

  }
 }

The task is: Write a program that exchanges bits {p, p+1, …, p+k-1} with bits {q, q+1, …,q+k-1} of a given 32-bit unsigned integer. The first and the second sequence of bits may not overlap.

I think this is suggesting roughly (maybe exactly) the same technique as @harold does in his answer, but I'm not entirely certain that I understand his answer, so I can't say with certainty whether it's the same or not.

I'd consider two facts in coming to an answer. First, that where two bits are identical, we don't need to do anything to swap them--i.e., swapping a 1 with a 1 or a 0 with a 0 doesn't change anything.

Therefore, we only need to do anything where the bits are not equal to each other. In this case, we have to flip both bits--i.e., the one that was a 0 has to become a 1, and the one that was a 1 has to become a 0.

At least to me, that immediately brings an exclusive-or to mind. An exclusive-or produces a 1 if one input or the other (but not both) is a 1. Looking at it slightly differently, it produces a 1 if the bits are not equal to each other, and a 0 if they are equal to each other. Looked at from yet a slightly different angle, an XOR with 0 leaves a bit unchanged, and an XOR with 1 always flips the bit (changes a 0 to a 1, or a 1 to a 0).

With that in hand, the job becomes fairly simple: we start by XORing the bits in the two ranges. This gives us 0s where they contain identical bits (meaning we want to leave them unchanged). It gives us 1s where they contain differing bits (meaning we want to flip the bits in both ranges).

We then XOR that mask with the bits in each range. That leaves the bits in each range unchanged where they were identical to start with, and flips the bits in each range where they were different.

For example, let's consider swapping the first 4 bits of a byte with the second 4 bits:

first four: 0 1 0 1
last four:  0 1 1 0

When we XOR these, we get:

mask:       0 0 1 1

We then XOR that back with each of the input ranges:

first four: 0 1 0 1
mask:       0 0 1 1
result:     0 1 1 0

last four: 0 1 1 0
mask:      0 0 1 1
result:    0 1 0 1

And now the first four have the same values as the last four did to start with (and vice versa).

Looking more specifically at your code, it has a fair number of places I think it could be made simpler and probably shorter. You start by reading four numbers, each with a block of code like this:

while (true)
{
 // input number

    Console.WriteLine("Input n:");
    if (uint.TryParse(Console.ReadLine(), out n))
    {
        if (n >= 0)
        {
            break;
        }
    }
}

The only things that change from one of these to the next are the string you display and the identity of the variable you're reading. This is an ideal candidate for being turned into a function. The function needs to read a number (just as you've done it above) and return the value when it's successful. Then the code in main will just call that function for each of the numbers it needs to read:

n = read_number("Input n:");
p = read_number("Input p:");
q = read_number("Input q:");
k = read_number("Input k:");

It's probably possible to avoid having to pass the string as well--you'd pass the variable itself, and figure out the name of that variable using reflection to build your string. That takes some rather more advanced use of .NET though, and I wouldn't advise it for now.

When it comes to finding which range is first:

if (p>q)
{
    num1 = q;
    num2 = p;
}
else
{
    num1 = p;
    num2 = q;
}

I think I'd just use p and q, but swap them if they're out of order:

if (p>q) {
    temp = p;
    p = q;
    q = temp;
}

(...and wish you were using C++, so you didn't have to re-implement basic operations like this for the 4 billionth time).

I've already advised a complete rewrite of the swapping code itself, so I guess there's not much point in reviewing that part of your code.


overloaded const and non-const class methods returning references in C++ I have a data-structure class in C++ with an accessor to some object (may be large) and I have const and non-const methods using this accessor so I need to overload it. I am looking for a critique of the code below - maybe there is a way to accomplish the same thing that is cleaner?

The way I understand it, there are two ways to achieve this without duplicating the code in the accessor in the following case, the method get(). I am not sure if there are serious issues with either of these two methods and I would like some guidance here.

I like method A because:

only one const_cast
const-version of the method get() returns a copy
the non-const method gets the non-const reference directly

I don't like method A because:

the non-const method get() is const only by contract, (not checked by compiler)
harder to get a const-reference, though not impossible

I like method B because:

the const-ness of the const method get() is checked by compiler
copy of the returned object is controlled by the user

I don't like method B because:

requires two const_casts which is hard to read

here is the (minimal) example code of the two cases.

/**
 * summary:
 * Two classes with an overloaded method which is
 * guaranteed (by contract) not to change any
 * internal part of the class. However, there is a
 * version of this method that will return a non-const
 * reference to an internal object, allowing the user
 * to modify it. Don't worry about why I would ever
 * want to do this, though if you want a real-world
 * example, think about std::vector<>::front()
 *
 * The difference between A and B can be summarized
 * as follows. In both cases, the second method merely
 * calls the first, wrapped with the needed
 * const_cast's
 *
 * struct A {
 *     int& get();
 *     int  get() const;
 * };
 *
 * struct B {
 *     const int& get() const;
 *           int& get();
 * };
 *
 **/

struct A
{
    int _val;

    A() : _val(7) {};

    // non-const reference returned here
    // by a non-const method
    int& get()
    {
        // maybe lots of calculations that you do not
        // wish to be duplicated in the const version
        // of this method...
        return _val;
    }

    // const version of get() this time returning
    // a copy of the object returned
    int get() const
    {
        // CONST-CAST!!?? SURE.
        return const_cast<A*>(this)->get();
    }

    // example of const method calling the
    // overloaded get() method
    int deep_get() const
    {
        // gets a copy and makes
        // a copy when returned
        // probably optimized away by compiler
        return this->get();
    }
};

struct B
{
    int _val;

    B() : _val(7) {};

    // const reference returned here
    // by a const method
    const int& get() const
    {
        // maybe lots of calculations that you do not
        // wish to be duplicated in the non-const
        // version of this method...
        return _val;
    }

    // non-const version of get() this time returning
    // a copy of the object returned
    int& get()
    {
        // CONST-CAST!? TWO OF THEM!!?? WHY NOT...
        return const_cast<int&>(const_cast<const B*>(this)->get());
    }

    // example of const method calling the
    // overloaded get() method
    int deep_get() const
    {
        // gets reference and makes
        // a copy when returned
        return this->get();
    }
};

int main()
{
    A a;
    a.get() = 8;  // call non-const method
    a.deep_get(); // indirectly call const method

    B b;
    b.get() = 8;  // call non-const method
    b.deep_get(); // indirectly call const method
}

const-version of the method get() returns a copy

Why is this a good thing?

  the non-const method get() is const only by contract, (not checked by compiler)
      harder to get a const-reference, though not impossible

What?

  the const-ness of the const method get() is checked by compiler
      copy of the returned object is controlled by the user

What does it matter if it is const if you have to go to the effort of making a copy.

  requires two const_casts which is hard to read

Really!

PS. getters and setters are an anti pattern (unless you are a container).
Exposing the internal implementation like this tightly couples your class to any code that uses it. This makes modifying your code in the future much harder. It is best to make your methods do the work rather than expose the members and do the work outside the class.

class X
{
     Y   value;

     public:
         // Why over complicate things.
         // This has the same functionality.
         Y&       get()       {return value;}
         Y const& get() const {return value;}
 };

OK so you want the code to get the value in one location:

class Z
{
     Y   value;

     public:
         Y&       get()       {return value;}

         // Only really need one cast.
         // We know that get() is not modifying the object.
         // If it was you can't share functions anyway.
         // Thus it is safe to cast away const-ness before calling get.
         // Because you are returning a const& from this function it is
         // fine and you don't need to add const-ness back anyway.
         Y const& get() const {return const_cast<Z&>(*this).get();}
 };


"Camera Purchase" challenge Here is the text of the problem:

  There are 5 brands of cameras sold on your website – Nikon, Canon, Sigma, Fujifilm and Sony. Each of these brands sell different cameras at distinct prices. For simplicity, each brand is represented by an integer between 1 to 5. Your website has q shoppers each of whom has a list of brands they are willing to buy from. Based on every user's buying history, you know that they are most likely to buy the kth cheapest camera amongst the brands they like. Your task involves finding this kth cheapest camera for each shopper.

  Given a list of brands and the prices of their cameras, the list of shoppers and their preferences, for each shopper, determine the kth cheapest camera. If no such camera exists, print -1.

  Input Format

  The first line contains integer n denoting the total number of cameras being sold. The second line contains an array of n integers describing the brands of the cameras. The third line contains an array of n integers describing the prices of those particular cameras.

  After that, q queries describing the shoppers' preferences follow. A single query is described in 3 lines.

  d represents the number of brands the shopper will buy from.
  d space separated integers in ascending order denoting the index of the brands.
  kth cheapest camera to display.

  Constraints

  1 ≤ n ≤ 105
  1 ≤ q ≤ 105
  1 ≤ pricesi ≤ 109
  1 ≤ brandsi ≤ 5
  1 ≤ lj ≤ 5
  1 ≤ k ≤ 5 * 105
  all pricesi are distinct

  Output Format

  Output exactly q lines. In the jth of them print a single integer denoting the price of the camera lens the jth shopper will buy. If no such lens exists, print -1.

Here is my solution:

#include <cmath>
#include <cstdio>
#include <vector>
#include <iostream>
#include <algorithm>
#include <unordered_map>
using namespace std;

int main() {
    int n;
    cin >> n;
    vector<int> brands;
    for (int i = 0; i < n; i++){
        int tmp;
        cin >> tmp;
        brands.push_back(tmp);
    }

    unordered_map<int, vector<int>> dict;
    for (int i = 0; i < n; i++){
        auto it = dict.find(brands[i]);
        if( it == dict.end() ) {
        dict.insert({brands[i], vector<int>()});
        }
    }
    for (int i = 0; i < n; i++){
        int tmp;
        cin >> tmp;
        dict[brands[i]].push_back(tmp);
        }

    int q;
    cin >> q;
    for (int i = 0; i < q; i++){
        int number_of_brands;
        cin >> number_of_brands;
        vector<int> indexes;
        for (int i = 0; i < number_of_brands; i++){
            int tmp;
            cin >> tmp;
            indexes.push_back(tmp);
            }
        int k;
        cin >> k;
        vector<int> potential_cameras;
        for (auto i : indexes){
            for (auto x : dict[i]){
                potential_cameras.push_back(x);
                }
            }
        sort(potential_cameras.begin(), potential_cameras.end());
        if (potential_cameras.size() >= k){
            cout << potential_cameras[k - 1] << '\n';
            }
        else {
            cout << -1 << '\n';
        }
    }
    return 0;
}

This algorithm times out for every single one of the bigger sized test cases. It is one of my very first attempt at writing C++ code, so please be merciless with any bad habit or code smell you can spot, but in this specific case I am curious to know what the main issue is, and if I could have used more suitable data structures than the ones I chose.

vector<int> potential_cameras;
        for (auto i : indexes){
            for (auto x : dict[i]){
                potential_cameras.push_back(x);
                }
            }
        sort(potential_cameras.begin(), potential_cameras.end());

Let's review the important constraints:

At most 5 brands
At most 10000 cameras
Up to 10000 queries

So this code is executed up to 10000 times, and each time is filtering up to 10000 cameras and then sorting up to 10000 cameras.

Given that each query includes at least one brand out of the five, there are only 31 possible brand combinations. So by pre-computing you could execute this code 31 times rather than 10000 times, and since it's the obvious candidate for the bottleneck that might give you a 300-times speedup.

If that's still not fast enough, then you need to look at linear time median algorithms. But frankly if the queries are evenly distributed then ~300 queries per brand combination is much larger than \$\lg 10000 \approx 13\$, so it's probably faster to quicksort once per combination than to execute a linear time median once per query. If you really want to overkill it then you could create a custom data structure which supports multiple median queries by maintaining state of which ranges of the array have been partitioned around which pivot, but KISS, YAGNI.

Alternatively, but still violating KISS, you could do just five sorts (one per camera) and then some complicated multiple binary chop. I'd still prefer one sort per brand combination.


Optimizing mcrypt encryption class further The following is a very simple implementation of an Encryption class using mcrypt whereby the functions encrypt and decrypt are called statically by a PHP script on GET and on POST respectively:

class Encryption
{
    const CIPHER...

    static function encrypt($plaintext)
    {
        $td = mcrypt_module_open(self::CIPHER, "", self::MODE, ""); # <= move this outside?
        mcrypt_generic_init($td, self::KEY, 0);
        $ciphertext = mcrypt_generic($td, $plaintext);
        mcrypt_generic_deinit($td);
        mcrypt_module_close($td);
        return base64_encode($ciphertext);
    }

    static function decrypt($ciphertext)
    {
        $td = mcrypt_module_open(self::CIPHER, "", self::MODE, ""); # <= repeated
        mcrypt_generic_init($td, self::KEY, 0);
        $plaintext = mdecrypt_generic($td, base64_decode($ciphertext));
        mcrypt_generic_deinit($td);
        mcrypt_module_close($td);
        return $plaintext;
    }
}

Ignoring other aspects of this stripped down code, what I want to achieve is to reduce code duplication by perhaps moving $td = mcrypt_module_open(self::CIPHER, "", self::MODE, ""); outside. Can this be done without combining the two methods or is what I have already at the optimum?

You can actually refactor most of the code into a separate function:

static function encryptdecrypt($text)
{
    $td = mcrypt_module_open(self::CIPHER, "", self::MODE, "");
    mcrypt_generic_init($td, self::KEY, 0);
    $text = mcrypt_generic($td, $text);
    mcrypt_generic_deinit($td);
    mcrypt_module_close($td);
    return $text;
}

static function encrypt($plaintext)
{
    return base64_encode(encryptdecrypt($plaintext));
}

static function decrypt($ciphertext)
{
    return encryptdecrypt(base64_decode($ciphertext));
}


PHP Memento design pattern implementation I am a self-taught programmer. I decided to learn design patterns by reading about them, and then coming up with their implementation in PHP. I know that learning would be virtually impossible without code review.

I will be grateful if you could review my implementation of the Memento design pattern and point to any shortcoming in the implementation of the pattern and general OOP principles.

My implementation tries to imitate the behaviour of a code repository (e.g. Git), where you can commit and checkout code based on previous commits. Below I present how a client can interact with the code.

/**
* Memento - a class that 'remembers' previous states
*/
class Repository
{
    /**
    * @var Commit[]
    */
    private $commits = [];

    /**
    * Commit objects are pushed into an array
    */
    public function saveCommit(Commit $commit)
    {
        array_push($this->commits, $commit);
    }

    /**
    * Rewind code to a selected commit by the commit key
    *
    * @param integer $commitKey The index key of a commit  to rewind code to
    * @return string Rewinded code
    */
    public function rewindCode($commitKey)
    {
        $rewindedCode = '';
        for($i=0; $i <= $commitKey; $i++) {
            $rewindedCode .= $this->commits[$i]->getCode();
        }
        return $rewindedCode;
    }

    public function getCommits()
    {
        return $this->commits;
    }

    /**
    * Get all commited code so far
    * @return string
    */
    public function getCommitedCode()
    {
        $commitedCode = '';
        foreach($this->commits as $commit) {
            $commitedCode .=  $commit->getCode();
        }
        return isset($commitedCode) ? $commitedCode : '';
    }
}

-

/**
* A class/data structure that represents a single commit
*/
class Commit
{
    private $timestamp;
    private $code;

    public function __construct($code)
    {
        $this->code = $code;
        $this->timestamp = time();
    }

    public function getCode()
    {
        return $this->code;
    }
}

-

/**
* Originator - a class that can save itself and retrieve states from the Memento object (Repository)
*/
class CodeBase
{
    private $repository;
    private $code;

    public function __construct(Repository $repo)
    {
        $this->repository = $repo;
    }

    public function writeCode($code)
    {
        $this->code = $this->code . $code;
    }

    /**
    * Extract code added since the last commit
    * @return string Code that has not yet been commited
    */
    private function getNewCode()
    {
        $commitedCode = $this->repository->getCommitedCode();
        return $newCode = str_replace($commitedCode, '', $this->code);
    }

    /**
    * Save state (code) to Memento (Repository)
    */
    public function commitToRepository()
    {
        $newCode = $this->getNewCode();
        $this->repository->saveCommit(new Commit($newCode));
    }

    /**
    * Rewind code to the selected commit (Retrieve saved state)
    */
    public function checkoutFromRepository($commitKey)
    {
        $this->code = $this->repository->rewindCode($commitKey);
    }

    public function getCode()
    {
        return $this->code;
    }

}

-

$repo = new Repository();
$codeBase = new CodeBase($repo);

$codeBase->writeCode('First day code');
$codeBase->writeCode('Second day code');
$codeBase->commitToRepository();
$codeBase->writeCode('Third day code');
$codeBase->commitToRepository();

$codeBase->checkoutFromRepository(0); // Roll back to the first commit
echo $codeBase->getCode(); // Prints: "First day code Second day code"

$codeBase->checkoutFromRepository(1); // Roll back to the second commit
echo $codeBase->getCode(); // Prints: "First day code Second day code Third day code"

After a bit of experimenting, I get there

I'm also a self-taught programmer, but I've been at if for 33 years. I've seen a lot of things, but I never heard of a Memento design pattern. So, eventhough it is quite clear from your explanation and code what it is, the first thing to to is to look up what it exactly is. Here for instance:

https://sourcemaking.com/design_patterns/memento

There are just too many fancy names for these things, but I understand what they get at. I think we can also agree that you've tried to make the absolute simplest implementation of this design pattern; Just keeping track of one string, nothing more.

Let us discuss the code, starting from the beginning, which is your last piece of source code.

Your CodeBase class, the originator, internally needs a Repository class, the memento, to keep track of its state. Why do you declare it outside the CodeBase class and pretent it a argument to the construction of it? It is not. So the constructor of CodeBase should be:

public function __construct()
{
    $this->repository = new Repository();
}

Now declaring repository private actually makes sense.

Within CodeBase you have a method called getNewCode(). It works by removing the committed code from all the code in the CodeBase class. Moreso, you use this to commit new code with commitToRepository(). That just doesn't feel right. The state of your originator is basically a single string. You would want to remember that state, so you should commit the whole string, not a part of it. Remember that currently you only append the code strings at the end, but that's probably not how it will stay. If you edit the code string in the middle your method will not work anymore. That's why I get a bad feeling looking at that code. So CodeBase should look like this:

class CodeBase
{
    private $repository;
    private $code = '';

    public function __construct()
    {
        $this->repository = new Repository();
    }

    public function getCode()
    {
        return $this->code;
    }

    public function setCode($code)
    {
        return $this->code = $code;
    }

    public function appendCode($code)
    {
        $this->newCode = $this->newCode . $code;
    }

    public function commitCode()
    {
        $this->repository->commit($this->code);
    }
}

Which looks a lot cleaner. Yes, I have ommitted the roll-back method. More on that later. The point is that you can extend CodeBase to be a full fletched code editor and all these methods will still work.

I will agree that it does look as if we just shifted the problem to the Repository class, and you might be right. But the design pattern is about externalizing the object's internal state, and that's what I've done.

Here's is how I would define the Repository class:

class Repository
{
    private $commits = [];

    public function commit($state)
    {
        $this->commits[], $state);
    }

    public function commitCount()
    {
      return sizeof($this->commits);
    }

    public function rollBack($steps,$permanent = TRUE)
    {
       if ($stepsBack <= 0) throw new Exception('Invalid step back value');
       $commitCount = $this->commitCount();
       if ($steps > $commitCount)) return '';
       $commitIndex = $commitCount-$steps;
       $state       = $this->commits[$commitIndex];
       if ($permanent) $this->commits = array_slice($this->commits, 0, $commitIndex);
       return $state;
    }
}

It's a very basic implementation, mainly to show you the rollBack() method and what's going on with the externalized state of CodeBase. I've left out the Commit class for simplicity. So, yes, I store the complete code on every commit. Not efficient, I know, but necessary. The rollBack() method is what is important here. It normally does a real roll back, in other words, it is permanent. So a roll back method in the CodeBase class would look like this:

public function rollBackCode($steps)
{
   $this->repository->rollBack($steps);
}

Nevertheless this would not be the way I would inplement the Memento Design Pattern. The main thing that I don't like is that the Repository is part of the state of the CodeBase, which is your originator. Moreso, you only use one Memento object to store everything, whereas I think it would be more logical to have one of those objects per state you want to remember. This leads me to the following code:

class CodeState // memento
{
    private $code;

    function __construct(CodeBase $codeBase)
    {
      $this->setCode($codeBase);
    }

    public function getCode(CodeBase $codeBase)
    {
      $codeBase->setCode($this->code);
    }

    public function setCode(CodeBase $codeBase)
    {
      $this->code = $codeBase->getCode();
    }

}

And:

class CodeBase // orginator
{
    private $code;

    function __construct($code = '')
    {
      $this->setCode($code);
    }

    public function getCode()
    {
      return $this->code;
    }

    public function setCode($code)
    {
      $this->code = $code;
    }

    public function appendCode($code)
    {
      $this->code .= $code;
    }
}

And that is all that is needed. I still have to show how the 'caretaker' can use these two classes.

$commits  = array();              // I cannot use '[]' yet
$codeBase = new CodeBase();

$codeBase->appendCode('First day code');
$codeBase->appendCode('Second day code');
$commits[1] = new CodeState($codeBase);
$codeBase->appendCode('Third day code');
$commits[2] = new CodeState($codeBase);

$commits[1]->getCode($codeBase);  // Roll back to the first commit
echo $codeBase->getCode().'
'; // "First day code Second day code"

$commits[2]->getCode($codeBase);  // Roll back to the second commit
echo $codeBase->getCode().'
'; // "First day code Second day code Third day code"

Now this is much cleaner. You might not completely agree with the names of the methods, but I think you can see my basic idea here. You can add in timestamps in the CodeState class and roll back to any commit. There are no akward array or string manipulations.

Let's be clear, there's not much wrong with your implementation, or my first attemp, it's just that they don't comply with the design pattern you were trying to follow. All three ways work. Purists would say that only the last one is correct and the other two are feeble attempts at writing code by amateurs. So be it. Just write the code you are happy with. Following exact design patterns can be useful but it should never hinder you from creating code that does something useful.

Having said all that, I would also like to refer you to this blog posting:

https://blog.svpino.com/2015/04/22/the-thing-with-code-clarity-you-cant-be-proud-of-something-I-cant-read

Yes, this post has become too long, sorry for that.


Grouping two sets of data The following method retrieves a list of users and the subscriptions for each user, from my database. That said, it works, but it runs pretty dang slow. I'd like to improve the speed on it. Currently, it fetches a mere 90 records, and takes nearly 20 seconds to do so, partly because, for every record of final output, it's grouping the subs per user, and doing a fetch on that user to obtain userId, email, and username (all three as User below).

public IEnumerable<ActiveSubscriptionsSummary> ActiveSubscriptionsByUser(SubscriptionType type)
{
    var subs = new List<ActiveSubscriptionsSummary>();

    var subscriptions = _bss.ActiveSubscriptions(type);
    var subsByUser = subscriptions
        .GroupBy(s => s.UserId,
            s => s,
            (key, g) => new ActiveBuilderSubscriptionsSummary
            {
                User = _cs.ById(key),
                Subscriptions = g.ToList()
            })
        .ToList();

    return subsByUser;
}

_cs.ById above refers to a service call that retrieves the user:

public ICustomer ById(Guid id)
{
    return GetBackend().ById(id);
}

All I use from that method is UserId, UserName, and Email (in the ActiveSubscriptionsByUser method, that is).

_bss above refers to a service call, with this method:

public IEnumerable<Subscription> ActiveSubscriptions(string subscriptionType)
{
    var subs = _sms.CurrentSession.Query<Subscription>()
                    .Where(s => (s.SubscriptionType == subscriptionType) &&
                                (s.EndDate >= DateTime.Today.Date))
                    .ToList();
    return subs;
}

My goal here is to speed this thing up, as I know over time that "90" will be a pretty small number, and we'll potentially be showing 100's, if not 1000's, of subs on the screen. And 20 seconds for 90 equates to a long dang time for 1000+.

Is my GroupBy potentially something that can be better coded to make this run faster? Could I potentially join two queries "users" and "subs" in some better way?

Update
This is C#, running against an nHibernate-hooked SQL DB, outputting to Angular.

Your performance is going to suffer until you can create a "WHERE IN"-style query for users, either by building a string or by combining Expressions.

Or, if you want to keep it simple, just put the active criteria in your query, don't pull it up front. I'd recommend this approach.

Add .Expand("Users") on the active subscriptions query to pull back the related Users. I would avoid getting your ActiveBuilderSubscriptionSummary involved until you get your results back, at which point you can then project the query results to a collection of ActiveBuilderSubscriptionSummary objects using a standard LINQ .Select.

If you are hesitant to remove or change your method because you need to reuse it, or it is comfortably nestled in a repository, I'd refactor to use a stored proc instead. Otherwise, faced with the choice, it is way better to duplicate your query in code than to force an eager load of data up front.


Trabb Pardo – Knuth - Algorithm in streams Trabb Pardo and Knuth devised this simple procedure as a way to explore the functionality of various languages. The wiki page describes it as:

  The Trabb Pardo–Knuth algorithm is a program introduced by Donald Knuth and Luis Trabb Pardo to illustrate the evolution of computer programming languages.

The algorithm is simple:

ask for 11 numbers to be read into a sequence S
reverse sequence S
for each item in sequence S
    call a function to do an operation
    if result overflows
        alert user
    else
        print result

The standard 'function' is to calculate the following:

$$
    f(x) = \sqrt{|x|} + 5 \times x^3
$$

The limiting factor is traditionally the value 400.0.

Here's the Java 8 code to accomplish the traditional 11 input values:

import java.util.Arrays;
import java.util.Scanner;
import java.util.function.IntToDoubleFunction;
import java.util.stream.IntStream;

public class TPK {

    public static void main(String[] args) {
        try (Scanner scanner = new Scanner(System.in);) {
            int[] values = IntStream.generate(scanner::nextInt).limit(11).toArray();
            int[] reversed = IntStream.range(0, values.length)
                    .map(index -> values[values.length - index - 1]).toArray();
            IntToDoubleFunction function =
                    val -> Math.sqrt(Math.abs(val)) + 5 * val * val * val;
            Arrays.stream(reversed)
                    .mapToDouble(function)
                    .forEach(val -> System.out.println(
                            val > 400.0 ? "TOO LARGE" : String.format("%.5f", val)));
        }
    }

}

Instead of generating an array of \$N\$ elements and then reversing it, I'd find it more natural to push values onto a stack and pop off one by one.

It would seem that the "standard" and "traditional" elements would be good as constants, defined near the top of the file:

the number 11
the function to apply
the limit

So that you can play with them easily without making edits inside the main, unchanging part of the code.

Lastly, you have an unnecessary ; in the try statement.

The ability to unit test would be nice, though in this simple example it might take substantial refactoring and lead to over-engineering, and so might not be worth it. Just a thought anyway.


Display content if form field is filled - Do not display if field is empty I am using the following code within the footer of my test site. I am using WordPress and Advanced Custom Fields

The intention is:

To display the HTML container .social-media--icons if any of the fields contain content within the CMS.
Then, within the container .social-media--icons, display the relevant icon and href if that field is filled.

I have included strlen(trim($twitter)) > 0 as I understand this will detect if the field only has spaces.

I appear to have this working locally and performing the above tasks. I am looking to find out if there is a clearer, optmisied method of performing these tasks

  <img src="<?php echo get_template_directory_uri(); ?>/svg/arrow.svg" alt="Up Arrow">

&copy;
    <?php echo date('Y'); ?>

  <?php

    $facebook = get_field('facebook', 'option');
    $twitter = get_field('twitter', 'option');
    $instagram = get_field('instagram', 'option');
?>

  <?php if(isset($facebook) && strlen(trim($facebook)) > 0 || isset($twitter) && strlen(trim($twitter)) > 0 || isset($instagram) && strlen(trim($instagram)) > 0): ?>

    <?php if(isset($facebook) && strlen(trim($facebook)) > 0): ?>
    <a class="social-link ie9" href="<?php echo $facebook; ?>" target="_blank" rel="noopener">
        <img src="<?php echo get_template_directory_uri(); ?>/svg/facebook.svg" alt="Facebook Icon">

    <?php endif; ?>

    <?php if(isset($twitter) && strlen(trim($twitter)) > 0): ?>
    <a class="social-link ie9" href="<?php echo $twitter; ?>" target="_blank" rel="noopener">
        <img src="<?php echo get_template_directory_uri(); ?>/svg/twitter.svg" alt="Twitter Icon">

    <?php endif; ?>

    <?php if(isset($instagram) && strlen(trim($instagram)) > 0): ?>
    <a class="social-link ie9" href="<?php echo $instagram; ?>" target="_blank" rel="noopener">
        <img src="<?php echo get_template_directory_uri(); ?>/svg/instagram.svg" alt="Instagram Icon">

    <?php endif; ?>

  <?php endif; ?>

Oh yes, I recommend some DRYing principles here.  Don't ask php to do the same thing over and over for the same result in a loop -- execute those tasks, just once, before entering the loop.

Rather than repeating whole portions of mark up only to change a few values, create a lookup array of the social networks that you want to cycle through and only replace those values in a reusable piece of the dom.

The social variables are always going to be "set", because they are unconditionally declared when get_field() is called.  There is no point in isset().

If there are any qualifying socials, then the parent
 will enclose all qualifying -tagged-wrapped s.

This ALSO makes it easy for you to scale your application.  In case you ever want to add more socials in the future, just add a new element to $socials (assuming all of the other data is consistent) and you won't need to touch any part of the processing code.

Code: (Demo)

$socials = ['facebook', 'twitter', 'instagram'];
$templateDirectory = get_template_directory_uri();
foreach ($socials as $social) {
    $socialHref = trim(get_field($social, 'option'));
    if (strlen($socialHref)) {
        $socialLinks[] = sprintf(
            '

            ',
            $socialHref, $templateDirectory, $social, ucfirst($social)
        );
    }
}
if (isset($socialLinks)) {
    echo '
' , implode(' ', $socialLinks) , '
';
}

Output:


Determine what hazard type does the building belong using GeoDjango Intersects Below is my query to identify what hazard type does the buildings belong:

def floodhazard_tbl(request):
    if request.method == "GET":
        reference_high = FloodHazard.objects.filter(hazard='High')
        reference_medium = FloodHazard.objects.filter(hazard='Medium')
        reference_low = FloodHazard.objects.filter(hazard='Low')

        #get all ids based on filter
        ids_high = reference_high.values_list('id', flat=True)
        ids_medium = reference_medium.values_list('id', flat=True)
        ids_low = reference_low.values_list('id', flat=True)

        # create a list
        k = []
        response_high = []
        response_medium = []
        response_low = []
        bldg_id_high = []
        bldg_ids_high = []
        bldg_id_medium = []
        bldg_ids_medium = []
        filtered_bldg = []

        # this code is results a messy JSON data that need underscore.js to manipulate
        # in order for us to use datatables
        for myid in ids_high:
            getgeom = FloodHazard.objects.get(id=myid).geom
            response_high = PolyStructures.objects.filter(geom__intersects=getgeom).values(
                'brgy_locat', 'municipali').annotate(counthigh=Count('brgy_locat'))
            bldg_id_high = filter(None,response_high.values_list('id',flat=True))
            bldg_ids_high = bldg_ids_high + bldg_id_high
            k.append(response_high)

        for myid in ids_medium:
            getgeom = FloodHazard.objects.get(id=myid).geom
            response_medium = PolyStructures.objects.exclude(id__in=bldg_ids_high).filter(geom__intersects=getgeom).values(
                'brgy_locat', 'municipali').annotate(countmedium=Count('brgy_locat'))
            bldg_id_medium = filter(None,response_medium.values_list('id',flat=True))
            bldg_ids_medium = bldg_ids_medium + bldg_id_medium
            k.append(response_medium)

        filtered_bldg = bldg_ids_medium + bldg_ids_high

        for myid in ids_low:
            getgeom = FloodHazard.objects.get(id=myid).geom
            response_low = PolyStructures.objects.exclude(id__in=filtered_bldg).filter(geom__intersects=getgeom).values(
                'brgy_locat', 'municipali').annotate(countlow=Count('brgy_locat'))
            k.append(response_low)

        #to_json = list(k for k,_ in itertools.groupby(k))
        result = []
        for d in chain.from_iterable(k):
            result.append(d)

        return HttpResponse(list(json.dumps(result)), content_type='application/json')

I have this method (algo):

After the response_high query, I get all the building that DID NOT intersect to the HIGH type of hazard and so on and exclude it in the next query (intersects) to avoid duplication (because there is a tendency that the building will intersect to the hazard polygon several times).

The code works but the response is slow, take 6 seconds to show the output.
As it is messed up, any optimization "technique" is appreciated.

I don't have a lot of experience with GeoDjango specifically, so there are some parts here that I can't review, but there are some spots that stand out as needing improvement.

The performance issues in your view

Any requests that aren't a GET will trigger a 500 error.
Your logic tends to repeat iteself when it doesn't need to.
You are triggering a ton of queries to generate the response.

Non-GET requests trigger a 500 error

Your floodhazard_tbl view specifically checks for a GET request and then returns the list of results based on that, but it doesn't actually return anything if a different request type is sent. Because of this, Django will trigger an exception (because no response was returned) which will trigger a 500 error for any non-GET response. You should instead return a HttpResponseNotAllowed response for any non-GET request, which is the equivalent of a 405 response.

Walking through how the response is generated

Right now you are going through the follow steps to get the buildings that intersect a specific type of hazard:

Filter the FloodHazard objects by the specific type.
Get all of the ids of the filtered objects.
You go through the list of ids, one-by-one, and do the following:

Retrieve the FloodHazard object for the id.
Get all of the PolyStructure objects which intersect with it.
Add the ids of those PolyStructure objects to a master list of PolyStructure ids for the type.
Add a serialized version of these PolyStructure objects to a master list of results.

You'll notice that steps 2 and 3.1 can cancel themselves out. And because you are getting the FloodHazard object again, you are causing a N+1 query at 3.1 because the object has not been cached. There is also a N+1 query happening at 3.2 when you get the PolyStructure objects.

The database queries that are triggered

Now, you have multiple N+1 queries happening in the body of your view that is likely leading to the increased response time. When working with a large number of objects this can be the slowest part of your response, as each query adds a small amount of overhead that can quickly build up into a 5+ second response.

So, to give you a rough idea of how many queries are being executed here, lets take a count

3 queries are the beginning for getting the list of FloodHazard objects for each type.

Note that this retrieved ids for n objects.

For each FloodHazard object you are

Getting the same FloodHazard object (n extra queries, n + 3 so far)
Getting the intersecting PolyStructure objects (n extra queries, 2n + 3 so far)
Getting the ids of the PolyStructure objects (n * (2/3) extra queries, one for each type excluding "Low")

So while the 2.3 doesn't trigger an extra n queries, depending on the number of "Low" FloodHazard objects there could still be a significant number of queries triggered.

After adding this all up, you will see that roughly 3 queries are triggered for each FloodHazard object in the original query. So if there were 100 "High" FloodHazard objects, you would be triggering 303 queries to generate the response.

How things can be improved

There are four easy fixes that can be made to significantly drop the number of queries made, which will end up improving performance in the long run.

Fixing the 500 error on non-GET requests

This one is the easiest fix, as it just requires you to return a HttpResponseNotAllowed at the end of your method.

def floodhazard_tbl(request):
    from django.http import HttpResponseNotAllowed

    if request.method == "GET":
        # We'll get to this
        pass

    return HttpResponseNotAllowed(['GET'])

So now if anyone makes a POST (or anything other than GET) request, they will get a 405 response with the Allow header set to GET.

Cutting out the duplicate FloodHazard queries

Right now you are retrieving a list of ids of FloodHazard filtered down to specific types, and then you are going through the list of ids and making a query to get the FloodHazard object back. Since you don't actually do anything else with the ids of these FloodHazard objects, you can skip the call to values_list and just use the original query when iterating over the list of objects.

high_hazards = FloodHazard.objects.filter(hazard='High')
medium_hazards = FloodHazard.objects.filter(hazard='Medium')
low_hazards = FloodHazard.objects.filter(hazard='Low')

for hazard in high_hazards:
    pass

for hazard in medium_hazards:
    pass

for hazard in low_hazards:
    pass

So now instead of re-querying the FloodHazard objects, you will have it available as hazard in each iteration. You can still get the geometry as hazard.geom, and you cut out n queries without changing the behaviour of the view.

Now you're down from 303 queries to 203 queries, just by making the one simple change.

Getting the areas covered by the FloodHazard objects

Right now you are getting the PolyStructure objects for each FloodHazard area one-by-one, in an attempt to get all PolyStructures objects that fall within the area of the FloodHazard objects. You can improve the performance here by getting the union of all of the FloodHazard areas, and then getting all PolyStructure objects within that combined area.

GeoDjango provides a Union aggregate as well as a Collect aggregate, with the main difference between them being that Union will remove any overlapping boundaries while Collect will just combine them all together. You are going to need to determine which one works best for your dataset in the long run.

high_hazards = FloodHazard.objects.filter(hazard='High').aggregate(combined_geom=Collect('geom'))
medium_hazards = FloodHazard.objects.filter(hazard='Medium').aggregate(combined_geom=Collect('geom'))
low_hazards = FloodHazard.objects.filter(hazard='Low').aggregate(combined_geom=Collect('geom'))

high_area = high_hazards['combined_geom']
medium_area = medium_hazards['combined_geom']
low_area = low_hazards['combined_geom']

This will combine the geom field for each object while retrieving the list of objects and place it in the combined_geom index of the queryset that is returned. I've split each of these out into their own *_area variables, which will contain the GEOSGeometry or GeometryCollection object representing the combined geometry fields.

Retreiving all of the PolyStructures objects for a set of FloodHazard objects

With the object returned by Union or Collect, you can use it in most geospatial queries, including the __intersects filter on geometry fields. So you would still use your existing PolyStructures query but instead of passing getgeom you would pass the *_areas object instead.

high_structures = PolyStructures.objects.filter(geom__intersects=high_area)

So now instead of making n queries to get the PolyStructures objects within each FloodHazard area, you are just making a single query that gets them all at once.

Now you're down from 203 queries to 106 queries (one new query for each type), but the logic within your views has changed as a result.

Getting the ids of the PolyStructures objects so they are excluded from other types

In order to get the ids of the structures, you have two different options: use the values_list('id', flat=True) method like you already are, or use map to retrieve the id field from each of the objects in the queryset we used before. Most likely the values_list option is going to be faster, but it's useful to note that it's not always the only option.

high_ids = high_structures.values_list('id', flat=True)
medium_ids = high_structures.values_list('id', flat=True)

Now instead of these ids being retrieved and grouped on an object-by-object basis, these are retrieved all at once with a single query.

Now you're down from 106 queries to 8 queries, which all comes together to be a significant decrease.

Putting it all together

So now if you combine all of the suggestions into a single view, you get 34 lines of code (including comments and whitespace) that executes 8 queries to the database and returns the same response.

# There are three different types of hazards, we are going to get the combined geometry for each type
high_hazards = FloodHazard.objects.filter(hazard='High').aggregate(combined_geom=Collect('geom'))
medium_hazards = FloodHazard.objects.filter(hazard='Medium').aggregate(combined_geom=Collect('geom'))
low_hazards = FloodHazard.objects.filter(hazard='Low').aggregate(combined_geom=Collect('geom'))

# Get the areas from the query
high_area = high_hazards['combined_geom']
medium_area = medium_hazards['combined_geom']
low_area = low_hazards['combined_geom']

# Get all structures in each of the areas

high_structures = PolyStructures.objects.filter(geom__intersects=high_area)
high_ids = high_structures.values_list('id', flat=True)

medium_structures = PolyStructures.objects.exclude(id__in=high_ids).filter(geom__intersects=medium_area)
medium_ids = medium_structures.values_list('id', flat=True)

combined_ids = list(high_ids) + list(medium_ids)

low_structures = PolyStructures.objects.exclude(id__in=combined_ids).filter(geom__intersects=low_area)

# Serialize the structures

high_results = high_structures.values('brgy_locat', 'municipali').annotate(counthigh=Count('brgy_locat'))
medium_results = medium_structures.values('brgy_locat', 'municipali').annotate(countmedium=Count('brgy_locat'))
low_results = low_structures.values('brgy_locat', 'municipali').annotate(countlow=Count('brgy_locat'))

# Combine all of the structures into a unified response

results = list(high_results) + list(medium_results) + list(low_results)

# Return all of the results as JSON
return JsonResponse(results, safe=False)


Protect against XSS with .htaccess file My .htaccess redirects everything to my index.php, but I wonder if it's vulnerable to XSS attacks. Am I safe?

.htaccess file

RewriteBase /

RewriteCond %{REQUEST_FILENAME} !-f
RewriteCond %{REQUEST_FILENAME} !-d
RewriteRule .*$ index.php/$1 [L]
php_value auto_append_file none

index.php

<?php
require_once 'dbliteconnect.php';
$req['status'] = (php_sapi_name() == 'cli') ? "true" : 'false';
if (php_sapi_name() == 'cli' || (isset($_SERVER['argc'])) && $_SERVER['argc'] > 0)     {
$var = isset($argv[1]) ? $argv[1] : '';
$var = strreplace("/", "", $var);
$req['var'] = $var;
}
if (isset($_SERVER['REQUEST_URI']) && php_sapi_name() != 'cli-server') {
$var = strreplace('/'.basename(DIR_), "", $_SERVER['REQUEST_URI']);
$var = str_replace("/", "", $var);
$var = str_replace(".html", "", $var);
$req['var'] = $var;
}
if (php_sapi_name() == 'cli-server') {
$var = $_SERVER['REQUEST_URI'];
$var = substr($var, 1);
$var = strreplace("/", "", $var);
$var = str_replace(".html", "", $var);
$req['var'] = $var;
}
request($req);
?>

The common definition of XSS is that code faults which allow someone to manipulate your website appearance, access prohibited sections of data, view inaccessible files or possibly steal credentials and cheat your app's users into entering data on the wrong website.

One of the most commonly used XSS vulnerabilities is what we call "free input" when the app allows users to insert HTML or javascript into a form which using POST or GET HTTP methods.

The reason why XSS exists is not sanitizing user free input, allowing someone to inject code into your form which can lead to injecting remote javascript which can write the data you enter into the app database and stealing your information.

A very common method to do this is for phishing purposes to steal login credentials and account information.

To prevent script injections and attempts to modify your PHP’s global and request variables, use the following in your .htaccess:

<IfModule mod_rewrite.c>
RewriteCond %{QUERY_STRING} (\<|%3C).*script.*(\>|%3E) [NC,OR]
RewriteCond %{QUERY_STRING} GLOBALS(=|\[|\%[0-9A-Z]{0,2}) [OR]
RewriteCond %{QUERY_STRING} _REQUEST(=|\[|\%[0-9A-Z]{0,2})
RewriteRule .* index.php [F,L]
</IfModule>

You may use this free online XSS scanner to ensure the cleanness of your pages, http://xss-scanner.com/ and don't forget to use mod_security which would help reduce this kind of attacks.

By the way, you never trust user provided data inputs which include $_POST, $_GET, file uploads, cookies and HTTP headers (like User-Agent and Referer) such data must always be treated as untrusted and needs to be secured for each context especially if you're going to insert these inputs to your database.

Here is a very useful article including tips and tricks of .htaccess
http://viralpatel.net/blogs/21-very-useful-htaccess-tips-tricks/


A* implementation of 8 puzzle I'm trying to implement 8 puzzle problem using A Star algorithm. The goal state is: 0 1 2 3 4 5 6 7 8 and the heuristic used is Manhattan distance. Here's the code:

from copy import deepcopy

class puzzle:
    def __init__ (self, starting, parent):
        self.board = starting
        self.parent = parent
        self.f = 0
        self.g = 0
        self.h = 0

    def manhattan(self):
        inc = 0
        h = 0
        for i in range(3):
            for j in range(3):
                h += abs(inc-self.board[i][j])
            inc += 1
        return h

    def goal(self):
        inc = 0
        for i in range(3):
            for j in range(3):
                if self.board[i][j] != inc:
                    return False
                inc += 1
        return True

    def __eq__(self, other):
        return self.board == other.board

def move_function(curr):
    curr = curr.board
    for i in range(3):
        for j in range(3):
            if curr[i][j] == 0:
                x, y = i, j
                break
    q = []
    if x-1 >= 0:
        b = deepcopy(curr)
        b[x][y]=b[x-1][y]
        b[x-1][y]=0
        succ = puzzle(b, curr)
        q.append(succ)
    if x+1 < 3:
        b = deepcopy(curr)
        b[x][y]=b[x+1][y]
        b[x+1][y]=0
        succ = puzzle(b, curr)
        q.append(succ)
    if y-1 >= 0:
        b = deepcopy(curr)
        b[x][y]=b[x][y-1]
        b[x][y-1]=0
        succ = puzzle(b, curr)
        q.append(succ)
    if y+1 < 3:
        b = deepcopy(curr)
        b[x][y]=b[x][y+1]
        b[x][y+1]=0
        succ = puzzle(b, curr)
        q.append(succ)

    return q

def best_fvalue(openList):
    f = openList[0].f
    index = 0
    for i, item in enumerate(openList):
        if i == 0:
            continue
        if(item.f < f):
            f = item.f
            index  = i

    return openList[index], index

def AStar(start):
    openList = []
    closedList = []
    openList.append(start)

    while openList:
        current, index = best_fvalue(openList)
        if current.goal():
            return current
        openList.pop(index)
        closedList.append(current)

        X = move_function(current)
        for move in X:
            ok = False   #checking in closedList
            for i, item in enumerate(closedList):
                if item == move:
                    ok = True
                    break
            if not ok:              #not in closed list
                newG = current.g + 1
                present = False

                #openList includes move
                for j, item in enumerate(openList):
                    if item == move:
                        present = True
                        if newG < openList[j].g:
                            openList[j].g = newG
                            openList[j].f = openList[j].g + openList[j].h
                            openList[j].parent = current
                if not present:
                    move.g = newG
                    move.h = move.manhattan()
                    move.f = move.g + move.h
                    move.parent = current
                    openList.append(move)

    return None

#start = puzzle([[2,3,6],[0,1,8],[4,5,7]], None)
start = puzzle([[5,2,8],[4,1,7],[0,3,6]], None)
# start = puzzle([[0,1,2],[3,4,5],[6,7,8]], None)
#start = puzzle([[1,2,0],[3,4,5],[6,7,8]], None)
result = AStar(start)
noofMoves = 0

if(not result):
    print ("No solution")
else:
    print(result.board)
    t=result.parent
    while t:
        noofMoves += 1
        print(t.board)
        t=t.parent
print ("Length: " + str(noofMoves))

For the given test case it finds a solution but it's very slow and I'm doutbful about the implementation of the heuristic function. Please suggest on how it can be improved. Thanks

Okay, I realized what I was doing all wrong.
The Manhattan distance defined here is not admissible. Consider an initial state: 0 1 7 2 3 4 5 6 8. So the possible number of moves required to move 7 to its original position (i.e index 7) is 3 (down->down->left) but here the heuristic estimates it as 5 (i.e 7-2). So a better way of defining heuristic would be :

def manhattan(self):
        h = 0
        for i in range(3):
            for j in range(3):
                x, y = divmod(self.board[i][j], 3)
                h += abs(x-i) + abs(y-j)
        return h


Parsing MWNumericArray to C# code optimization This is my function returning matrices from Matlab.  This code run very slowly.
I was trying to use Parallel.For in-order to run it in parallel threads but I got different results than running it on one thread.

Am I iterating the matrix in a wrong order? Row, col vs col, row?
Can you please review this code any suggest some performance boost tips?

public static SerializableMatrix ParseNumericMatrix(MWNumericArray matrix)
{
    SerializableMatrix resultMatrix;
    int[] dimensions = matrix.Dimensions;

    if (dimensions.Length > 3 || dimensions.Length < 2)
    {
        throw new ArgumentException("Input matrix dimensions are too long", "Three is the max dimensions");
    }
    var height = dimensions[0];
    var width = dimensions[1];
    resultMatrix = new SerializableMatrix() { Width = width, Height = height, Data = new List<double[]>() };

    if (dimensions.Length == 2)
    {
        var dataArray = new double[width * height];
        for (int i = 0; i < height; i++)
        {
            for (int j = 0; j < width; j++)
            {
                dataArray[i * width + j] = (double)matrix[i, j];
            }
        }
        resultMatrix.Data.Add(dataArray);
        return resultMatrix;
    }
    if (dimensions.Length == 3)
    {
        int depth = dimensions[2];
        for (int depthIndex = 0; depthIndex < depth; depthIndex++)
        {
            var dataArray = new double[width * height];
            for (int i = 0; i < height; i++)
            {
                for (int j = 0; j < width; j++)
                {
                    // matlab indices start at 1 , not 0!!!
                    dataArray[i * width + j] = (double)matrix[i + 1, j + 1, depthIndex + 1];
                }
            }
            resultMatrix.Data.Add(dataArray);
        }
    }
    return resultMatrix;
}

As usual with performance problems the only way of knowing for sure is to use a profiler and check which piece of code is taking the longest time. However looking at the code you are copying the matrix element by element which is typically the slowest way to go. Often there is a method which can do that as a bulk operation for you.

I don't know much about the Matlab stuff as I've never used it but I found this question on Stackoverflow which has a couple of answers showing a single line statement of copying it. Based on one of these answers your code for the 2D case could look like this:

double[,] data = (double[,])matrix.ToArray(MWArrayComponent.Real);

I assume in the 3D case you'll get a double[,,] back which you might have to take apart afterwards so it fits your List<double[]> type.

Apart from that:

This exception has a misleading message in case the dimensions are less than 2

if (dimensions.Length > 3 || dimensions.Length < 2)
{
    throw new ArgumentException("Input matrix dimensions are too long", "Three is the max dimensions");
}


Insert row into database using values from a html form submission I have a comments page on my site and I was wondering if it is vulnerable to some sort of sql attack or something else. At the moment users can type what they want, they can even write a php script and it will upload to the database. Is this a problem and how can I fix it.
This is the script I use:
$name= $_POST['name'];
$email= $_POST['e_mailaddress'];

if(isset($_POST['comments']) AND $_POST['comments']!=''){
    $comments= $_POST['comments'];
    $comments_sent = 'true';

$dbcn = new connection();

$sql= "INSERT INTO contact (name, email, comments) VALUES (:name, :email, :comments);";

$query = $dbcn->dbconnect()->prepare($sql);

$results = $query->execute(array(
":name"=> $name,
":email"=> $email,
":comments"=> $comments
));
}

With prepared statements, I don't see anything that would immediately make it susceptible to sql injection (this isn't to say there isn't something that is there at a deeper level - I'm not a security auditor nor intimately familiar with php).  The "or something else" part remains quite large.

Look at OWASP (Open Web Application Security Project) which goes through a large set of possible attack vectors through the web.

Such a page is susceptible to various scripting injection where someone inserts a script (or flash, or even something like an image from another site) to be executed on another client machine. From the OWASP top 10 - 2013 pdf the XSS attack can be seen and described on page A3 (page 9).


refactor Python code with lots of None type checks When I wrote this it just felt messy due to the null checks.  Any good ideas on how to clean it up would be most appreciated.

 def getItemsInStock(self):
        itemsInStock = []
        items = self.soup.find_all("ul", {'class':re.compile('results.*')})
        getItems = [x for x in items if x.find("div", class_="quantity")]
        if getItems:
            itemsInStock += getItems
        pages = self.combThroughPages()
        if pages:
            for each in pages:
                self.uri = each
                soup = self.createSoup()
                items = soup.find_all("ul", {'class':re.compile('results.*')})
                if items:
                    inStock = [x for x in items if x.find("div", class_="quantity")]
                    if inStock:
                        itemsInStock += inStock
        if itemsInStock:
            return self.__returnItemDetailAsDictionary(itemsInStock)
        else:
            return None

As an example,

    getItems = [x for x in items if x.find("div", class_="quantity")]
    if getItems:
        itemsInStock += getItems

can be written

    itemsInStock.extend(x for x in items if x.find("div", class_="quantity")]

extend (and for that matter, +=) are no-ops on empty lists.

Now, I'm not sure what some of the function you call return, but I don't like that getItemsInStock returns None if no items are in stock -- it makes more sense to return an empty list in that case. If your other functions did the same, you wouldn't need any of your if guards. Another example:

    pages = self.combThroughPages()
    if pages:
        for each in pages:
            ...

can be written

    for page in self.combThroughPages():
        ...

but only if combThroughPages returns an empty list when there are no, um, pages, I guess? (It's not clear from your function name.)


Implementation, Ceaser Cipher in C++ I'm working on learning C++ coming from python. I'm doing some projects to help me out understanding the language a bit more.

My code works I just want to know if there is anything that I can improve on it or best practices in C++, maybe something that I should have done differently.

// Ceaser Cipher implementation

#include <iostream>
#include <string>

char shiftF(char ch, int shift, const char type)
{
    if (type == 'e')
    {
        return ch == 'z' || ch == 'Z' ? ch - 25 : ch + shift;
    }
    else if (type == 'd')
    {
        return ch == 'z' || ch == 'Z' ? ch - 25 : ch - shift;
    }
}

std::string encode(std::string str, int shift, char type)
{
    std::string tempMsg;
    for (auto ch : str)
    {
        if (isalpha(ch))
        {
            tempMsg += shiftF(ch, shift, type);
        }
        else if (isspace(ch))
        {
            // There is probably a better way to do this.
            tempMsg += " ";
        }
        else if (isalnum(ch))
        {
            continue;
        }
    }
    return tempMsg;
}

int main(int argc, char *argv[])
{
    int choice;
    std::cout << "What do you want to do? 1.Encrypt, 2.Decrypt: ";
    std::cin >> choice;
    std::string result;
    const char dec = 'd';
    const char enc = 'e';

    if (choice == 1)
    {
        int key;
        std::cout << "Enter encryption shift: ";
        std::cin >> key;

        std::string msg;
        std::cout << "Enter a message: ";

        // This doesn't work for some reason?
        // std::cin.clear();
        // std::cin.sync();

        std::cin.ignore();
        std::getline(std::cin, msg);

        result = encode(msg, key, enc);
    }

    else if (choice == 2)
    {
        int key;
        std::cout << "Enter decryption shift: ";
        std::cin >> key;

        std::string msg;
        std::cout << "Enter a message: ";

        std::cin.ignore();
        std::getline(std::cin, msg);

        result = encode(msg, key, dec);
    }
    else
    {
        std::cout << "Wrong option, exiting!";
    }

    std::cout << "Message encoded: " << result;
}

Your functions are doing too much. Remember: one function, one responsibility. That is, the encode function actually both encodes and decodes and that behavior is controlled via its arguments. In particular, the choice of behavior is offloaded to shiftF based on a char. There is no error if that char is anything other than 'd' or 'e'. This is something that we should absolutely try to avoid: we want to catch as many problems as we can at compile-time, but even runtime errors are preferred to silent errors!

On closer inspection, there is no need for such a complication. In fact, the only difference between encoding and decoding is the sign of shift. So we could write:

std::string encode(const std::string& str, int shift)
{
    std::string tempMsg;

    std::transform(str.cbegin(), str.cend(), std::back_inserter(tempMsg), [&](char ch) -> char
    {
        if (isspace(ch))
            return ' ';

        return ch == 'z' || ch == 'Z' ? ch - 25 : ch + shift;
    });

    return tempMsg;
}

std::string decode(const std::string& str, int shift)
{
    return encode(str, -1 * shift);
}

What is happening here?

We pass the input message str as a const-ref and not by-value as in your original code. When you pass by-value, the object gets copied and in this case there is no reason for it. In this case, you could actually even pass the object by-reference only, and modify it in-place but we're not doing it here.
We use a standard function std::transform with a lambda function that encapsulates the logic of shiftF. You need to include <algorithm> for this use.
The third argument for std::transform is std::back_inserter found from <iterator>, which takes care of inserting at the back of the string tempMsg.
Conceptually, the division of encoding and decoding into separate functions is cleaner and more logical. Whenever you can implement another function in terms of other functions, it's likely a good idea because you don't have to repeat yourself leading to less maintenance decreasing the chances for bugs.

Once you adopt this approach, you can get rid of the const char variables from your main program, and just call the correct encode or decode function inside your if-statement. As a side remark, you could also return meaningful error codes in your main. For example, if the choice is invalid, return from the else-branch e.g., EXIT_FAILURE.


Passing objects to functions safely and efficiently I would like to have this code reviewed. I am learning C++ from a C background. How would I improve this code to make it more C++ like? This is a simple program that I wrote, which is a stripped down version of the actual full program I'm writing.

#include <iostream>
using namespace std;

class Doge{
public:
    Doge();
    Doge(Doge&);
    ~Doge();
    int GetAge() const { return itsAge; }
    void SetAge(int age) { itsAge = age; }

private:
    int itsAge;
};

Doge::Doge(){
    cout << "Doge Constructor" << endl;
    itsAge = 5;
}

Doge::Doge(Doge&){
    cout << "Doge Copy Constructor" << endl;
}

Doge::~Doge(){
    cout << "Doge Destructor" << endl;
}

Doge * Function( Doge * theDoge){
    cout << "In Function" << endl;
    cout << "Scott is now " << theDoge->GetAge() << endl;
}

int main(){

    cout << "Make Doge" << endl;
    Doge Scott;
    cout << "Scott is " << Scott.GetAge() << endl;
    Scott.SetAge(10);
    Function(&Scott);
    cout << endl;
    return 0;
}

It doesn't feel natural to initialize a Doge with a fixed age:

Doge::Doge(){
    cout << "Doge Constructor" << endl;
    itsAge = 5;
}

It would be more natural to make age a mandatory constructor parameter:

Doge::Doge(int itsAge): itsAge(itsAge) {
    std::cout << "Doge Constructor" << std::endl;
}

The copy constructor doesn't actually copy anything.
Either remove it, or implement copying, for example:

Doge::Doge(Doge& other){
    std::cout << "Doge Copy Constructor" << std::endl;
    this->itsAge = other.itsAge;
}

This function is declared to return a Doge *, but it returns nothing:

Doge * Function( Doge * theDoge){
    cout << "In Function" << endl;
    cout << "Scott is now " << theDoge->GetAge() << endl;
}

This raises a compiler warning, at least with g++.

You probably implemented the destructor just to see when it gets called.
That's fine, for testing.
When you're done, just remember to remove it if it's not needed.


Given some integers, find triplets where the product of two numbers equals the third number I am trying to solve a coding challenge:

  Given N integers A1, A2, …, AN, count the number of triplets (x, y, z) (with 1 ≤ x < y < z ≤ N) such that at least one of the following is true:

  Ax = Ay × Az, and/or
  Ay = Ax × Az, and/or
  Az = Ax × Ay

  Sample case 1

  5 2 4 6 3 1

  In Sample Case #1, the only triplet satisfying the condition given in the problem statement is (2, 4, 5). The triplet is valid since the second, fourth, and fifth integers are 2, 6, and 3, and 2 × 3 = 6. thus the answer here is 1.

  Sample case 2

  2 4 8 16 32 64

  The six triplets satisfying the condition given in the problem statement are: (1, 2, 3), (1, 3, 4), (1, 4, 5), (1, 5, 6), (2, 3, 5), (2, 4, 6). so the answer here is 6.

My Code in python:

import itertools
count=0
for t in itertools.combinations(l,3):
    if t[0]*t[1]==t[2] or t[1]*t[2]==t[0] or t[0]*t[2]==t[1]:
        count+=1
print(count)

This is the naive way of generating all possible 3 length combinations and checking for the condition. This works fine for smaller input but when the inout size increase time complexity increases. I am assuming for an example that has 1,2,3,6,8 the combinations generated are (2,3,6),(2,3,8) 2,3,6 satisfy the condition so the checking for 2,3,8 is unnecessary and can be avoided. How can I modify my code to take advantage of this observation ?

Your combinations(…, 3) loop makes your algorithm O(N3).

It's easy to improve it to be O(N2).  The question is, essentially: for every pair of entries, how many occurrences of their product are in the list?  So, make an indexes data structure to help you find, in O(1) time, where the product might be located.

from collections import defaultdict
from itertools import combinations

a = [int(ai) for ai in input('Input: ').split()]

indexes = defaultdict(set)
for i, ai in enumerate(a):
    indexes[ai].add(i)

triplets = set()
for x, y in combinations(range(len(a)), 2):
    for z in indexes[a[x] * a[y]].difference([x, y]):
        triplets.add(tuple(sorted((x, y, z))))

print(len(triplets))

Here, I've chosen to stick closer to the notation used in the challenge itself, with a being the list, and x, y, z as the indexes of entries (but 0-based rather than 1-based).


Measure distance using echo signal from Ultrasonic sensor I was not happy with original code for Ultrasonic sensor and decided to write my own implementation.

I managed to do this, but it still looks weird. Could you help me to improve it?

#include <wiringPi.h>
#include <stdio.h>
#include <sys/time.h>

#define Trig    0
#define Echo    1

double raw_ultrasonic_measure(int trig, int echo) {
  digitalWrite(Trig, LOW);
  delayMicroseconds(1);

  digitalWrite(Trig, HIGH);
  delayMicroseconds(10);
  digitalWrite(Trig, LOW);

  while(!(digitalRead(Echo) == 1));
  int time1 = micros();

  while(!(digitalRead(Echo) == 0));
  int time2 = micros();
  return (double)(time2 - time1) / 100;
}

double ultrasonic_measure(int trig, int echo) {
  double value1 = raw_ultrasonic_measure(trig, echo);
  delay(1);
  double value2 = raw_ultrasonic_measure(trig, echo);
  delay(1);
  double value3 = raw_ultrasonic_measure(trig, echo);
  delay(1);
  double value4 = raw_ultrasonic_measure(trig, echo);
  delay(1);
  double value5 = raw_ultrasonic_measure(trig, echo);
  return (value1 + value2 + value3 + value4 + value5) / 5;
}

int main(void) {
  wiringPiSetup();
  pinMode(Echo, INPUT);
  pinMode(Trig, OUTPUT);

  while(1){
    double value = raw_ultrasonic_measure(Trig, Echo);
    printf("%0.1f cm\n", value);
    delay(1000);
  }

  return 0;
}

raw_ultrasonic_measure measures the duration of an echo staying high. It is not related to the distance in any way, it is just a duration of trig signal.

You need a time elapsed from the beginning of a signal to reception of echo - that is, a duration of echo being low.

In a quick and dirty way,

        signal_start = micros();
        digitalWrite(Trig, HIGH);

        while (digitalRead(Echo) == 0) {
            ;
        }

        echo_received = micros();
        digital_write(Trig, LOW);

        return (echo_received - signal_start) / (2 * speed_of_sound);

In a real world application you would want to debounce echo, provide for a timeout, etc.

You may also want to account for the time taken by a first call to digitalWrite, but I doubt it will make any perceptible difference.

ultrasonic_measure is in fact a loop:

        double value = 0.0;
        for (int i = 0; i < 5; i++) {
            value += raw_ultrasonic_measure(Trig, Echo);
        }
        return value / 5;

One benefit of such approach is an ability to pass a number of samples as a parameter.


Reverse Polish Notation (RPN) Calculator I am trying to solve a codewar challenge (link):

Your job is to create a calculator which evaluates expressions in Reverse Polish notation.
For example expression 5 1 2 + 4 * + 3 - (which is equivalent to 5 + ((1 + 2) * 4) - 3 in normal notation) should evaluate to 14.
For your convenience, the input is formatted such that a space is provided between every token.
Empty expression should evaluate to 0.
Valid operations are +, -, *, /.
You may assume that there won't be exceptional situations (like stack underflow or division by zero).

I have 2 problems, the first is that the code looks awful and I don't know how to improve it tbh,
I also want this code to be able to compute multiple digit numbers and i feel like i need to start from scratch if I want to add that.
Note : This is my first time writing a question so if there's anything wrong with it please tell me and I'll change it.
def calc(expr):
    stack = []
    for i in range(len(expr)):
        if expr[i].isdigit():
            stack.append(expr[i])
        if expr[i-1] == '.':
            stack.pop()
        if expr[i] == '.':
            stack.pop()
            stack.append(expr[i-1] +  expr[i] + expr[i+1])
        if expr[i] == '+':
            stack.append(float(stack[-2]) + float(stack[-1]))
            for i in range(2):
                del stack[-2]
        if expr[i] == '-':
            stack.append(float(stack[-2]) - float(stack[-1]))
            for i in range(2):
                del stack[-2]
        if expr[i] == '*':
            stack.append(float(stack[-2]) * float(stack[-1]))
            for i in range(2):
                del stack[-2]
        if expr[i] == '/':
            stack.append(float(stack[-2]) / float(stack[-1]))
            for i in range(2):
                del stack[-2]
    if stack == []:
        return 0
    else:
        return float(stack[0])

The initial approach is full of redundant if conditions for capturing float numbers and duplicated set of statements (stack.append, for loop, del ...) for all 4 arithmetic operations.

The whole idea is achievable with a single traversal facilitated by operator (provides convenient equivalents of mathematical operations) and ast.literal_eval feature (to evaluate both int and float types):

import operator
from ast import literal_eval

math_operators = {'+': operator.add, '-': operator.sub,
                  '*': operator.mul, '/': operator.truediv}

def calc(expr):
    tokens = expr.split()
    res = []
    for t in tokens:
        if t in math_operators:
            res[-2:] = [math_operators[t](res[-2], res[-1])]
        else:
            res.append(literal_eval(t))

    return res[0] if res else 0

Test cases:

exp = '5 1 2 + 4 * + 3 -'
print(calc(exp))   # 14

exp = '5.5 10.6 -'
print(calc(exp))   # -5.1

exp = ''
print(calc(exp))   # 0


Solution to the CodingBat Array-3 [fix34] I was studying for arrays from coding-bat and encountered this:

The question is: (fix34)

  Return an array that contains exactly the same numbers as the given
  array, but rearranged so that every 3 is immediately followed by a 4.
  Do not move the 3's, but every other number may move. The array
  contains the same number of 3's and 4's, every 3 has a number after it
  that is not a 3 or 4, and a 3 appears in the array before any 4.

  fix34{1, 3, 1, 4} → {1, 3, 4, 1}

  fix34{1, 3, 1, 4, 4, 3, 1} → {1, 3, 4, 1, 1, 3, 4}

  fix34{3, 2, 2, 4} → {3, 4, 2, 2}

  fix34{2, 3, 5, 3, 2, 4, 4} → {2, 3, 4, 3, 4, 5, 2 }

Code:

public static int[] fix34(int[] nums) {

    // first i stored numbers which are not 3 or 4
    ArrayList<Integer> others = new ArrayList<Integer>();

    for (int i = 0; i < nums.length; i++ )
    {
        if ( nums[i] != 3 && nums[i] != 4 )
            others.add(nums[i]);
    }

    // i created a null array with same length as nums array
    int[] result = new int[nums.length];

    // first i replaced 3's in their specific place, then i replaced 4 near them.
    // so that other spots are 0
    for ( int i = 0; i < nums.length - 1; i++ )
    {
        if ( nums[i] == 3)
        {
            result[i] = 3;
            result[i+1] = 4;
        }
    }

    // now i filled 0s with temped other numbers which i stored in an arraylist.
    // these numbers' size suits with the numbers of zeros
    int j = 0;
    for ( int i = 0; i < result.length; i++ )
    {
        if ( result[i] == 0 )
        {
            int temp = others.get(j);
            j++;
            result[i] = temp;
        }

    }

    return result;

}

I like using ArrayLists, but is it a silly solution? How can I do this with a single loop?

It can easily be done in O(n) i.e a single loop traversal. Think simple .Modify the below program to suit your understanding and find the algorithm in the comments.
Just a brief:

Traverse the array from 0 to get the first position where you meet 3 say curIdx. Now you know that curIdx+1 can be replaced.
Traverse the array from the last to get the last position of 4, say lastIdx. Now you know that lastIdx can be replaced.
Swap curIdx and lastIdx if its within the array's range.

void reArrange(int[] arr)
{

int[] arr = {2, 3, 5, 3, 2, 4, 4};
int lastIdx = arr.length-1;
int curIdx= 0 ;
while(true){ //I know when to exit. Trust me!!!
    // Where art thou my 3. I need to replace the index after you.
    while(curIdx<arr.length && arr[curIdx]!=3){
        curIdx++;
    }
    curIdx++;

    // Gotcha Mr 4. You need to lead 3 safely.
    while(lastIdx>0 && arr[lastIdx]!=4){
        lastIdx--;
    }

    // As I said, trust me and I will help you exit
    if(curIdx>=arr.length || lastIdx<=0){
        break;
    }

    // Swap Time to rearrange Mr 4's after 3.
    int temp = arr[curIdx];
    arr[curIdx] = arr[lastIdx];
    arr[lastIdx] = temp;
}

// Did I do it correctly? Lets check.
for(curIdx=0;curIdx<arr.length;curIdx++){
    System.out.print(arr[curIdx] + "  ");
}

}

Note:

I am rearranging the 4's after 3's without considering the order.
I assume that the input meets the constraints provided.


Inserting products scraped from the web into a database I am trying to speed up the runtime of my program as the API can make calls.  However after adding the inserts to a database 3 mins turns in 7 mins after 180 API calls, which can be one a second.  How can I further improve the inserts into the DB?

I am using mysql database, but potentially could change if it will increase performance

url = getSignedUrl(params)
resp = requests.get(url)
responseSoup=BeautifulSoup(resp.text)

#EXTRACT FROM XML
quantity = ['' if product.amount is None else product.amount.text for product in responseSoup.findAll("offersummary")]
price = ['' if product.lowestnewprice is None else product.lowestnewprice.formattedprice.text for product in responseSoup.findAll("offersummary")]
prime = ['' if product.iseligibleforprime is None else product.iseligibleforprime.text for product in responseSoup("offer")]

#UPDATE DB
for zz in zip(asins.split(","), price,quantity,prime):
    cursor = conn.cursor()

    if zz[1] == "Too low to display":
        print "Scraping..."
        zz = scrapeTooLow(zz[0])

    dat = [zz[0], zz[1], zz[2], zz[3]]
    if zz[1] != "":
        priceFormat = float(zz[1].replace("$", "").replace(",", ""))
        mep = round(priceFormat*1.35, 2)
        dp = round(priceFormat*1.38, 2)
        fp = round(priceFormat*1.17, 2)
    else:
        priceFormat = ""
        mep = ""
        dp = ""
        fp = ""
    dat.extend([mep,dp,fp])
    print dat
    try:
        cursor.execute("""INSERT INTO data VALUES (%s,%s, %s, %s, %s, %s, %s)""",dat)
        conn.commit()
    except:
        conn.rollback()
print count, i

For performance questions you should really take a profiler to it and
make sure that the Python code isn't accidentally the culprit first.

The numbers you quote for this sound way too high, though of course we
can't see the actual data.

Now for the code.

Even though this is running Python 2 it should still be made compatible
with Python 3, in particular:

Use print as a function, that is, print(...), for consistency.
Avoid zip for big lists, instead use izip from itertools.
Use consistent syntax, also read
PEP8 regarding naming,
because Python names should be consistenly lower case with
underscores - since your names are at least consistent that is less of
an issue.
A program should have a if __name__ == "__main__": ... block,
assuming this not part of a bigger script where such a block is
already in place.`

Apart from that more generally:

Use more descriptive names. zz, dp, fp, mep, dat tell the
reader absolutely nothing about the meaning.
Random float literals should be named, or create a separate
Use functions to reuse functionality and to decompose the whole
program into clear steps.
Related things should be place together, e.g. the cursor call can be
moved to the end where the database functionality is located instead
of at the start of the loop.  Btw. are you sure that you acquire a new
transaction automatically?  It would help to know which particular
library is being used.

Also:

The replace call can be done in one step, "$,".replace("", "").
The XML extraction looks pretty verbose.  Unfortunately I'm not so
sure about the types, though I'd imagine this could be written a bit
more clearly with at least a helper function for two of the steps,
quantity and prime:

def empty_or_text(thing):
    return '' if thing is None else thing.text

It also makes sense to cache things instead of recomputing it all over
again, in this case that applies to the findAll calls for
"offersummary".
dat is simply zz[0:4].
Since strings are immutable the assignment of the empty string can be
done in one line, e.g. a = b = "".

And finally as has been said, it might make sense to group inserts into
larger batches unless you require the transactional behaviour for some
reason, or you can't simply rerun a batch.

Also take a look at
this Stackoverflow post
to deal with the insert statement a bit better.

That's it.  I'm afraid without a bit more information about the data
structures it's harder to infer how it should be structured instead.

At this I point I have the following for the modified code:

url = getSignedUrl(params)
resp = requests.get(url)
responseSoup = BeautifulSoup(resp.text)

def empty_or_text(thing):
    return '' if thing is None else thing.text

# EXTRACT FROM XML
offerSummaries = responseSoup.findAll("offersummary")

quantity = [empty_or_text(product.amount)
            for product in offerSummaries]

prime = [empty_or_text(product.iseligibleforprime)
         for product in responseSoup("offer")]

price = [product.lowestnewprice is None else product.lowestnewprice.formattedprice.text
         for product in offerSummaries]

from itertools import izip

# UPDATE DB
for zz in izip(asins.split(","), price, quantity, prime):
    if zz[1] == "Too low to display":
        print("Scraping...")
        zz = scrapeTooLow(zz[0])

    priceFormat = mep = dp = fp = ""
    if zz[1] != "":
        priceFormat = float(zz[1].replace("$,", ""))
        mep = round(priceFormat * 1.35, 2)
        dp = round(priceFormat * 1.38, 2)
        fp = round(priceFormat * 1.17, 2)

    dat = zz[0:4] + [mep, dp, fp]
    print(dat)
    try:
        conn.cursor().execute("INSERT INTO data VALUES (%s, %s, %s, %s, %s, %s, %s)", dat)
        conn.commit()
    except:
        conn.rollback()

print(count, i)