Find max-number div by three to be formed by array elements Given an array of non-negative integers, find the largest multiple of 3 that can be formed from array elements.
For example, if the input array is {8, 1, 9}, the output should be "9 8 1", and if the input array is {8, 1, 7, 6, 0}, output should be "8 7 6 0".

Looking for code-review, best-practices and optimizations.
public final class DivThree {

    private DivThree() {}

    public static int divThree(int[] a) {
        final Queue<Integer> queue0 = new LinkedList<Integer>(); // if num % 3 == 0, store num in queue0.
        final Queue<Integer> queue1 = new LinkedList<Integer>(); // if num % 3 == 1, store num in queue1.
        final Queue<Integer> queue2 = new LinkedList<Integer>(); // if num % 3 == 2, store num in queue2.

        Arrays.sort(a);

        int sum = 0;
        for (int i = 0; i < a.length; i++) {
            sum = sum  + a[i];

            int remainder = a[i] % 3;

            if (remainder == 0) {
                queue0.add(a[i]);
            } else  if (remainder == 1) {
                queue1.add(a[i]);
            } else {
                queue2.add(a[i]);
            }
        }

        if (sum % 3 == 1) {
            if (!queue1.isEmpty()) {
                queue1.remove();
            } else {
                if (!queue2.isEmpty()) {
                    queue2.remove();
                } else {
                    return -1;
                }
                if (!queue2.isEmpty()) {
                    queue2.remove();
                } else {
                    return -1;
                }
            }
        } else if (sum % 3 == 2) {
            if (!queue2.isEmpty()) {
                queue2.remove();
            } else {
                if (!queue1.isEmpty()) {
                    queue1.remove();
                } else {
                    return -1;
                }
                if (!queue1.isEmpty()) {
                    queue1.remove();
                } else {
                    return -1;
                }
            }
        }

        final List<Integer> list = new ArrayList<Integer>();
        list.addAll(queue0);
        list.addAll(queue1);
        list.addAll(queue2);

        Collections.sort(list);

        int result = 0;
        int pow = 1;
        for (int i = 0; i < list.size(); i++) {
            result = result  + pow * list.get(i);
            pow = pow * 10;
        }

        return result;
    }
}

public class DivThreeTest {

    @Test
    public void test1() {
        int[] a1 = {1, 2, 3, 4};
        assertEquals(432, DivThree.divThree(a1));
    }

    @Test
    public void test2() {
        int a2[] = {8, 1, 7, 6, 0};
        assertEquals(8760, DivThree.divThree(a2));
    }

}

For example, if the input array is {8, 1, 9}, the output should be “9 8 1″

So, let's say that the input array is { 4, 8, 15, 16, 23, 42 }, what should the output be then? At first I expected it to be "4 8 15 16 23 42", but then @Marc-Andre made me realize that in fact, that's not the case. I misunderstood the question at first.

Although not explicitly written, all the numbers in the array are required to be less than 10. I strongly suggest you add a check in your loop: (which btw can be written as for-each instead, which also applies to your last loop for looping through the list)

for (int i : a) {
    sum = sum  + i;

    if (i >= 10) {
        throw new IllegalArgumentException("All numbers must be less than 10.");
    }

    int remainder = a % 3;
    ...
}

I have to say though that the example you give is a bit weird, as your example output is a string separated by space. The question though is about what number can be formed, which don't speak anything about string at all.

In addition to this, you also suffer from a case of severe code-duplication. This can be treated easily with the medicine of extracting method. This extracted method also deserves a little simplification as I really don't like that you're doing isEmpty-remove-isEmpty-remove, why not just check if size > 2 in the first place? (Alright, size can be a costly operation compared to isEmpty for some queue implementations, but LinkedList is not one of those - at least not anymore).

private static boolean removeOneOrTwo(Queue<?> removeOneFrom, Queue<?> removeTwoFrom) {
    if (!removeOneFrom.isEmpty()) {
        removeOneFrom.remove();
    } else if (removeTwoFrom.size() < 2) {
        return false;
    } else {
        removeTwoFrom.remove();
        removeTwoFrom.remove();
    }
    return true;
}

Note that here I actually use Queue<?>, more or less just for the fun of it but mostly for the reason that for this method, it doesn't need to know what kind of Queue we're dealing with. It only needs to be able to know the size of it and remove items.

By doing this simple method extraction, we've reduced your original method a bit:

if (sum % 3 == 1) {
    if (!removeOneOrTwo(queue1, queue2)) {
        return -1;
    }
} else if (sum % 3 == 2) {
    if (!removeOneOrTwo(queue2, queue1)) {
        return -1;
    }
}

Although now that I'm dealing with that code anyway, I have to add: return -1; smells. -1 is a special return value for "no possible combination found". In this case I would find 0 a better choice of return value for that, as that would correspond to an empty array (and is a multiple of 3). Either that, or throw an exception. In this case, I would find returning zero perfectly reasonable.


Small 3D and 4D vector utilities I'm a hobby programmer and have started 2 month ago with C++.  I had some knowledge of Python, and I learned it all through Internet tutorials.

I'm looking for your help, since I don't have a tutor I would like to have a comment about my programming style.  I used this small "library" to create the Snake4D game (see bottom of question for further information).

Here is a little program that handles mathematical vectors, 4D and 3D.  Specifically, I ask you to find any improvements in the code's readability and in the algorithms.

Here is the "vec.h" file:

class V4{
private:
    double coords[4];
 public:
    V4();
    V4(double x, double y, double z, double w);
    ~V4();

    void set_cooord(int i, double x); //set_coord (index of coord i, value x)

    double module();
    double operator[](unsigned int i);

    void operator =(V4);
    V4 operator +(const V4);
    V4 operator -(const V4);
    V4 operator *(double k); //moltiplication of vector by a scalar k

    double dot4(V4); //the dot product

    V4 cross(V4 u, V4 v,V4 w); //the cross product
};

Here is the "vec.cpp" file:

/****************
V4 class
*****************/
V4::V4(){
    //set all coords to 0. if it's called without specific coord
    coords[0] = 0.;
    coords[1] = 0.;
    coords[2] = 0.;
    coords[3] = 0.;
}

V4::V4(double x, double y, double z, double w){
    coords[0] = x;
    coords[1] = y;
    coords[2] = z;
    coords[3] = w;
}

V4::~V4(){}//nothing to destroy?

void V4::set_cooord(int i, double x){ //i index, x value
    coords[i] = x;
}

double V4::module(){
    double sum = 0;
    for(int i= 0; i<4; i++){
        sum += coords[i]*coords[i];
    }
    return sqrt(sum);
}

double V4::operator[](unsigned int i){
     //if(i > 4){return;}  //list index out of range ? how to throw give a compile error?
     return coords[i];
 }

void V4::operator =(V4 v1){
    for(int i= 0; i<4; i++){
        coords[i] = v1[i];
    }
}

V4 V4::operator+(V4 v1) {
    double c[4]; // array of 4 double which will be transfered to the returned vector
     for(int i= 0; i<4; i++){
         c[i] = coords[i] + v1[i];
     }
     return V4(c[0],c[1],c[2],c[3]);
 }

 V4 V4::operator-( V4 v1) {
     double c[4];
     for(int i= 0; i<4; i++){
         c[i] = coords[i] - v1[i];
     }
     return V4(c[0],c[1],c[2],c[3]);
 }

V4 V4::operator*(double k){
    double c[4];
    for(int i= 0; i<4; i++){
        c[i] = coords[i] * k;
    }
    return V4(c[0],c[1],c[2],c[3]);
}

double V4::dot4(V4 v1){
    double sum = 0;
    for(int i= 0; i<4; i++){
       sum += coords[i]*v1[i];
    }
    return sum;
}

// requires three external vector and doesn't take any coords of the actual vector
//intended use:
// V4 v1(x,y,z,w), v2(x,y,z,w), v3(x,y,z,w), result, op;
// result = op.cross(v1,v2,v3)
V4 V4::cross(V4 u, V4 v, V4 w){
    //source Steve Hollasch master thesis
    //partial product
    double A = (v[0]*w[1])-(v[1]*w[0]);
    double B = (v[0]*w[2])-(v[2]*w[0]);
    double C = (v[0]*w[3])-(v[3]*w[0]);
    double D = (v[1]*w[1])-(v[2]*w[1]);
    double E = (v[1]*w[3])-(v[3]*w[1]);
    double F = (v[2]*w[3])-(v[3]*w[2]);

    double x =  (u[1]*F)-(u[2]*E)+(u[3]*D);
    double y = -(u[0]*F)+(u[2]*C)-(u[3]*B);
    double z =  (u[0]*E)-(u[1]*C)+(u[3]*A);
    double wc = -(u[0]*D)+(u[1]*B)-(u[2]*A); //defined as wc to not conflic with w vector

    return V4(x,y,z,wc);
}

Here are the source (.h and .cpp) file.

I already see some improvements:

I use the vectors as public members, because if they are private, I cannot get access to all the really comfortable functions they offer.
Instead of giving vertex hand by hand for the generation of the hypercube, there should be an algorithm that does this.
It could be used a general purpose class V containing the basic methods and having a variable int dimension then specialize the V3 and V4 inheriting the class V.

PS: I've made a little clip that shows the Snake4D game written in C++, Qt and OpenGL.

Very good.

Couple of points:

I would remove the set() method, and allow its work to be done by the operator[]. To do this you need return the value you want by reference.

double &  operator[](unsigned int i);
  //  ^^^  Return double by reference.

This allows you to do;

V4 x;
x[1] = 12;

When you pass objects as parameters it is usally best to pass by const reference. This means that no copy will be made and the original object can not be modified by the method using it:

V4 operator +(V4 const & rhs);
                      ^^^

Note I always put the const on the right of the type (rather than the left). Admittedly this is a religious war so choose a side and stick to it (there is only one minor side instance were it makes difference). But I find it makes reading the type name easier (as types are read from right to left with const always binding to the left (except where it is the leftmost lexeme in the type then it binds right).

Methods that do not change the state of the object should be declared const:

V4 operator -(V4 const & rhs) const;
                        //    ^^^^^  indicates that calling this method will not
                        //           change the state of the object.

Note: This means you should provide two versions of operator[] a normal one that allows mutation and a const version that can be used in contexts where it is read only.

double&        operator[](unsigned int i)        { return coord[i];}
double const&  operator[](unsigned int i) const  { return coord[i];}

Note: array access is not validated against the array bounds. Normally you don't check as we do not want to impose an extra burden on the responsible just because there are bad developers. But it is traditional to add at() method that does the same work as operator[] but also validates the bounds of the array access.

double&        at(unsigned int i)        { if (i > 3) {throw std::range_error("BLA");} return coord[i];}
double const&  at(unsigned int i) const  { if (i > 3) {throw std::range_error("BLA");} return coord[i];}

Implementing operator + - * / is easily done if you implement them in terms of += -= *= /=. This also provides the user of your object the opportunity to use your class more efficiently when a new object is not required (though this should be a secondary though).

V4 operator *(V4 const & rhs) const;
V4 operator *=(V4 const & rhs);       // This does change the sate of the object

V4 V4::operator*(V4 const& rhs) const { V4 result(*this); result *= rhs; return result;}

Using a for loop is easy. But sometimes it can be simpler to use the standard algorithms to accomplish simple tasks over short containers:

double sum = 0;
for(int i= 0; i<4; i++){
    sum += coords[i]*coords[i];
}

This can be replaced with:

sum = std::accumulate(coords, coords + 4, 0);

You can find good documentation here: http://www.sgi.com/tech/stl/table_of_contents.html (look for section: 5 Algorithms)

In the moto of not doing extra work. Don't write methods that do nothing:

V4::~V4(){}//nothing to destroy?

Remove this. The class automatically provides a desttructor that will correctly destroy nothing.

I actually like using void as the return type of assignment (I think it has no down side). But traditionally assignment should return a reference to self. This then allows the object to be part of an assignment chain.

V4  a;
V4  b;
V4  c;

a = b = c; // assignment chaining.
// It also allows you to pass the object as a parameter to a mehtod:

doStuff( a = b); // Assign a to b. Then call doStuff using a.

It is also a good idea to define the streaming operators for most classes. This allowsd you to serialize and de-serialize the class to a stream (file/string etc).

So now the class looks like this.

class V4
{
    double coords[4];
 public:
    V4();
    V4(double x, double y, double z, double w);

    double module() const;
    double&       operator[](unsigned int i)       {return coords[i];}
    double const& operator[](unsigned int i) const {return coords[i];}

    V4&  operator = (V4 const& rhs);
    V4&  operator +=(V4 const& rhs);
    V4&  operator -=(V4 const& rhs);
    V4&  operator *=(double k); //moltiplication of vector by a scalar k

    V4   operator +(V4 const& rhs)  const {V4 result(*this); result += rhs; return result;}
    V4   operator -(V4 const& rhs)  const {V4 result(*this); result -= rhs; return result;}
    V4   operator *(double rhs)     const {V4 result(*this); result *= rhs; return result;}

    double dot4(V4 const& rhs) const; //the dot product

    V4 cross(V4 const& u, V4 const& v,V4 const& w) const; //the cross product
};

Then rest of the interesting ones:

double V4::module() const
{
    return std::sqrt(std::accumulate(coords, coords + 4, 0.0);
}

V4&   V4::operator +=(V4 const& rhs)
{
    std::transform(coord, coord+4, rhs.coord, coord, std::plus);
    return *this;
}
V4&   V4::operator -=(V4 const& rhs)
{
    std::transform(coord, coord+4, rhs.coord, coord, std::minus);
    return *this;
}
V4&   V4::operator *=(double k)
{
    std::transform(coord, coord+4, coord, std::bind1st(std::multiplies, k));
    return *this;
}


Observer pattern implementation I'm reading Head First Design Patterns and I'm currently reading about the observer pattern. Is what I produced correct?

#include <iostream>
#include <string>
#include <vector>
#include <algorithm>

class ObserverInterface
{
public:
    virtual void update(std::string message) = 0;
};

class concreteObserver : ObserverInterface
{
    std::string name;
    std::string message;
public:
    concreteObserver(std::string name)
        :name(name)
    {}
    void update(std::string message)
    {
        this->message = message;

        std::cout << "Dear " << name << ", " << message << " from eBay" << std::endl;
    }
};

class SubjectInterface
{
public:
    void subscribe(concreteObserver *subscriber)
    {
        subscribers.push_back(subscriber);
    }
    void unsubscribe(concreteObserver *subscriber)
    {
        subscribers.erase(std::remove(subscribers.begin(), subscribers.end(), subscriber), subscribers.end());
    }
    void notify(std::string message)
    {
        std::vector<concreteObserver*>::const_iterator itr = subscribers.begin();
        while (itr != subscribers.end())
        {
            (*itr)->update(message);
            ++itr;
        }
    }
private:
    std::vector<concreteObserver*> subscribers;
};

class concreteSubject : public SubjectInterface
{
public:
    void newMessage(std::string msg)
    {
        notify(msg);
    }
};

int main(int argc, char* argv[])
{
    // EBAY
    concreteSubject ebay;

    // USERS
    concreteObserver user1("Steven");
    concreteObserver user2("John");

    // SUBSCRIBERS TO EBAY
    ebay.subscribe(&user1);
    ebay.subscribe(&user2);

    // EBAY SENDS NEW PROMOTIONS TO SUBSCRIBERS
    ebay.newMessage("enjoy free weekend listings");

    // UNSUBSCRIBE FROM EBAY
    ebay.unsubscribe(&user2);

    // EBAY SENDS NEW PROMOTION TO SUBSCRIBERS
    ebay.newMessage("free listings extended until 1st August");

    std::cout << std::endl << std::endl;
    system("pause");
    return 0;
}

Inheritance is by default private
class concreteObserver : ObserverInterface
                        ^^^^ Need public: here

Mark overriden methods:
void update(std::string message) // override ???

endl vs '\n'
Don't use endl unless you absolutely want to flush the buffer.
Probably OK here. Just wanted to note it.
Don't pass pointers:
    void subscribe(concreteObserver *subscriber)

Is subscriber every going to be NULL?
Do you need to check if subscriber has been destroyed?
Personally. I would pass by reference and document that a subject must live longer than the observers. Not that just because you pass by reference does not mean you can hold a pointer.
    void subscribe(concreteObserver& subscriber) // pass by reference
    {
        subscribers.push_back(&subscriber);      // but save pointer.
    }

But if you have a lot of dynamically allocated stuff this may not be suffecient. You could pass a std::weak_ptr then check of the subscriber is still available before sending the message.
Use the new foreach loop
        std::vector<concreteObserver*>::const_iterator itr = subscribers.begin();
        while (itr != subscribers.end())
        {
            (*itr)->update(message);
            ++itr;
        }

Easier to write as:
        for(auto& sub: subscribers) {
           sub->update(message);
        }


Conway life game implementation with scala I've tried to create optimal in terms of performance and memmory consumption. But also I've tried to make it functional and scala way. I want to get you comment on how to make it more 'scala'stic

object LifeGame extends App {
    trait Matrix extends Iterable[(Int, Int)] {
        def apply(p: (Int, Int)): Boolean
        def update(p: (Int, Int), b: Boolean): Unit
        def newInstance: Matrix;
    }

    class SparseMapMatrix extends Matrix {
        type Point = (Int, Int)

        private var data = Set[(Int, Int)]()

        def apply(p: Point): Boolean = data.contains(p)

        def update(p: Point, b: Boolean) = if (b) data += p else data -= p

        def newInstance = new SparseMapMatrix()

        def iterator = data.iterator

        override def toString() = {
            val sb = new StringBuilder()
            def best(func: (Int, Int) => Int)(p1: Point, p2: Point) = (func(p1._1, p2._1), func(p1._2, p2._2))
            val minBoundary = ((0, 0) /: iterator)(best(math.min))
            val maxBoundary = ((0, 0) /: iterator)(best(math.max))
            for (i <- minBoundary._1 to maxBoundary._1) {
                sb.append("\n")
                for (j <- minBoundary._2 to maxBoundary._2) {
                    sb.append(if (this((i, j))) "x" else " ")
                }
            }
            sb.append("\nmin=%s, max=%s" format(minBoundary, maxBoundary))
            sb.toString()
        }
    }

    object Engine {
        def apply(input: Matrix): Matrix = {
            val result = input.newInstance
            def block2D(pp: (Int, Int)): Seq[(Int, Int)] =
                for (ii <- block1D(pp._1); jj <- block1D(pp._2)) yield (ii, jj)
            val liveCells = for (p <- input.iterator.flatMap(block2D).toSet[(Int, Int)].par) yield {
                val offsets = block2D(p).filter(_ != p)
                val nn = offsets.map(p => input(p)).count(_ == true)
                case class State(l: Boolean, n: Int)
                val newValue = State(input(p), nn) match {
                    case State(true, n) if n < 2 => false
                    case State(true, 2) | State(true, 3) => true
                    case State(true, n) if n > 3 => false
                    case State(false, 3) => true
                    case State(value, _) => value
                };
                 if(newValue) Some(p) else None
            }
            liveCells.seq.foreach { _ match { case Some(p:Point) => result(p) = true; case _ => ;}}
            result
        }
    }

    def block1D(i: Int) = i - 1 to i + 1

}

The main-things I changed:

Matrix is now completely immutable
Code is more self-documenting (extract complex code to methods, etc.)
I deleted the unnecessary inheritance
I don't like apply-methods which are used as contains-methods -> name change
I deleted parentheses in side-effect free methods
Many syntax improvements

The code:

object LifeGame extends App {
  case class Point(x: Int = 0, y: Int = 0)

  object Matrix {
    def empty: Matrix =
      Matrix(Set.empty)
  }

  case class Matrix(private val data: Set[Point]) {

    def contains(p: Point): Boolean =
      data contains p

    def update(p: Point, b: Boolean) =
      if (b) copy(data+p) else copy(data-p)

    def iterator: Iterator[Point] =
      data.iterator

    override def toString = {
      def best(func: (Int, Int) => Int)(p1: Point, p2: Point) =
        Point(func(p1.x, p2.x), func(p1.y, p2.y))

      val minBoundary = (Point() /: iterator) { best(math.min) }
      val maxBoundary = (Point() /: iterator) { best(math.max) }

      val sb = StringBuilder.newBuilder
      for (i <- minBoundary.x to maxBoundary.x) {
        sb.append("\n")
        for (j <- minBoundary.y to maxBoundary.y) {
          sb.append(if (this contains Point(i, j)) "x" else " ")
        }
      }
      sb.append("\nmin=%s, max=%s" format (minBoundary, maxBoundary))
      sb.toString
    }
  }

  object Engine {

    def newCell: (Boolean, Int) => Boolean = {
      case (true, 0 | 1) => false
      case (true, 2 | 3) => true
      case (true, _) => false
      case (false, n) => n == 3
    }

    def apply(input: Matrix): Matrix = {
      def block1D(i: Int) =
        Seq(i-1, i, i+1)

      def block2D(p: Point) =
        for (x <- block1D(p.x); y <- block1D(p.y)) yield Point(x, y)

      def points =
        (input.iterator flatMap block2D).toSet.par

      def calcCell(p: Point) = {
        val neighbours = block2D(p) filter { p != }
        val alive = neighbours map { input contains } count { true == }

        if (newCell(input contains p, alive)) Some(p)
        else None
      }

      val cells = (points map calcCell).seq collect { case Some(p) => p }
      (Matrix.empty /: cells) { (result, p) => result(p) = true }
    }
  }
}


Optimizing reverse movmskb code I have some code that performs (nearly) the opposite function of a pmovmskb.

It takes as input a 16-bit bitfield and expands it into 2 xmm registers.
(I'm limited to working with AVX1 code).

The code I've come up with is:

const SliverToSliceMask = 0x0001-0002-0004-0008-0010-0020-0040-0080

movdqu xmm14,[rip+SliverToSliceMask]    ;xmm14 = mask to isolate bits 0..7
movdqa xmm15,xmm14                      ;xmm15 = mask
pslldq xmm15,1                          ;convert mask from bit 0..7 to bit 8..15
and edx,r8d                             ;combine the two slivers
  //Now expand the sliver into a slice
movd xmm0, edx              ;xmm0 = sliver
pshuflw xmm0,xmm0,$00       ;Paste the sliver 4 times (expanding it to 8 bytes).
pshufd xmm0, xmm0, $50     ;Paste the sliver 2 more times (expanding it to 16 bytes)
movdqa xmm1,xmm0           ;make a copy,expanding it to 32 bytes (in 2 regs)
//now xmm0 and xmm1 are filled with 16 copies of the input sliver, mask out the bit
//that is intended per word
pand xmm0,xmm14             ;mask off the words for bit 0..7
pand xmm1,xmm15             ;mask off the words for bit 8..15
pcmpeqw xmm0,xmm14          ;set 1's to $FFFF and 0's to 0
pcmpeqw xmm1,xmm15          ;Thus expanding the sliver (1 bit per outer sliver)
                            ;to a slice (16 bits per sliver)

Is there a way to optimize this code. I would esp. like to get rid of the memory access to fetch the mask.

Your code is SSE but not AVX, so here is the SSE version to generate the mask. The second part is same as yours. I modified it from my other code and NEVER tested:

; generate the bit mask
pcmpeqd         xmm15, xmm15  ; FFFFFFFF FFFFFFFF FFFFFFFF FFFFFFFF
psrlq           xmm15, 63     ; 00000000 00000001 00000000 00000001
movdqa          xmm14, xmm15  ;
psllq           xmm14, 17     ; 00000000 00020000 00000000 00020000
por             xmm15, xmm14  ; 00000000 00020001 00000000 00020001
movdqa          xmm14, xmm15
psllq           xmm14, 34     ; 00080004 00000000 00080004 00000000
por             xmm15, xmm14  ; 00080004 00020001 00080004 00020001
movdqa          xmm14, xmm15
psllq           xmm14, 4      ; 00800040 00200010 00800040 00200010
punpcklqdq      xmm15, xmm14  ; 00800040 00200010 00080004 00020001

; generate the second bit mask
movdqa          xmm15, xmm14
psllw           xmm15, 8      ; 80004000 20001000 08000400 02000100

; load and broadcast DX
movd            xmm0, edx     ; xmm0 = sliver
pshuflw         xmm0, xmm0, 0 ; xmm0 = sliver * 4
pshufd          xmm0, xmm0, 0 ; xmm0 = sliver * 8
movdqa          xmm1, xmm0

; apply the mask
pand            xmm0, xmm14   ; apply mask
pand            xmm1, xmm15   ; apply mask
pcmpeqw         xmm0,xmm14    ; mask to FFFF
pcmpeqw         xmm1,xmm15    ; mask to FFFF

If you are using AVX, you can omit those movdqa and the code will look better. The SSE version is long and I am not sure its better than a simple memory read, as your did.


Searching text, starring, flagging and joins I have an interesting performance concern that I would like to address before it becomes a serious issue. I created a SQL Fiddle to demonstrate the query, and the explain statement can be seen on Depesz.

I have a collection phrases in different languages, and 'translations' that link phrase ids together (source and destination). The phrases can be searched by keyword in a particular source language, and find translations of that phrase in the destination language. The translations can also be starred and flagged, and the user id and translation id are held in respective tables for this functionality.

If a keyword and source language matches, those phrase ids are joined on the translations table; then, that set is joined on new_phrases again where the destination language matches. That set is also joined on favorite_translations and translation_flags, and if the user id matches in those sets, favorited and flagged are true, otherwise false.

SELECT P1.phrase_id as src_phrase_id,
        P1.author as src_author,
        P1.language as src_language,
        P1.text as src_text,
        P2.phrase_id as dst_phrase_id,
        P2.author as dst_author,
        P2.language as dst_language,
        P2.text as dst_text,
        P1.translation_id,
        P1.community_rating,
        CASE WHEN P1.favoriter_id = $1 THEN true
            ELSE false
            END
        as favorited,
        CASE WHEN P1.flagger_id = $1 THEN true
            ELSE false
            END
        as flagged
    FROM (
        SELECT P.phrase_id, P.author, P.language, P.text,
               T.translation_id, T.destination_id, T.community_rating,
               FT.user_id as favoriter_id, TF.user_id as flagger_id
        FROM new_phrases P
        INNER JOIN translations T ON P.phrase_id = T.source_id
        LEFT JOIN favorite_translations FT ON T.translation_id = FT.translation_id
        LEFT JOIN translation_flags TF ON T.translation_id = TF.translation_id
        WHERE P.tsv_text @@ plainto_tsquery($2)
        AND P.language = $3
        AND T.destination_language = $4
        ORDER BY T.community_rating DESC
        OFFSET $5
        LIMIT 25
        ) P1, new_phrases P2
    WHERE P2.phrase_id = P1.destination_id;

My concern is that the growth of these tables is going to cause a crazy cost increase. I limited the result set of the search to 25, and it's paginated. However, I'm fairly certain the favorite_translations and translation_flags joins are being performed on the set before it is limited to 25, and I'm not sure how to work around that if that's actually the case. Perhaps I'm doing the LEFT JOINs in the wrong spot?

What can I do to improve the performance of this query? I have gone through 5 iterations of this query, improving it each time, but I feel that more can be done. I'm just not sure how.

One thing addressed in the comments is the LIMIT clause. The text search results should be limited to 25 before any other JOINs are performed. However, I'm not incredibly experienced with SQL, and I'm pretty sure that the 3 joins are done before the 25 result LIMIT is applied, causing a massive slow-down. This could be the primary source of improvement (but I don't know how, thus the question)!

The way you join is the problem.

Okay now that's out the way let me explain. These left joins you're doing are bloating your result set:

LEFT JOIN favorite_translations FT ON T.translation_id = FT.translation_id

  The LEFT JOIN keyword returns all rows from the left table (table1),
  with the matching rows in the right table (table2). The result is NULL
  in the right side when there is no match. - (w3schools)

what this means is that for two favorites on the same post you get two results into your set. This bloats your result set significantly as soon as you get more and more flags and favorites.

There is a rather simple fix for that.

Since in the end you only want to know about wether the user favorited / flagged or not, the "correct" way to go about this is a subselect to the count, something like this:

((SELECT COUNT(*) FROM favorite_translations
         WHERE user_id = $1 AND translation_id = T.translation_id) > 0) AS favorited

This also eliminates the less than nice-looking Case-Statement you got there, removes two joins and nets us a smaller execution plan, that even prevents multiple results for the same translation pair.

So this is the new execution plan on depesz


C++ SFML Animation System I'm attempting to write an animation system on top of SFML, and I've come up with a design where I can wrap sf::Drawable objects into a RenderComponent class, and the RenderComponent class can be given an Animation object at any time, which will overwrite a previous animation if one exists. Here is what I'm looking for.

Am I using the std::unique_ptr correctly/optimally?
Should I be using a pointer to store the Animation?
Is my method of settings the animation (with a variadic template) too complicated, and is there a better way?
I would normally separate the code into header and implementation, but for the brevity, I am uploading it in pure headers. Ignore that please.
Any general advice.

Here is the code:

Animation base class

class Animation {
    typedef std::chrono::high_resolution_clock hrc;

private:

    std::chrono::time_point<hrc> m_now;

protected:

    unsigned int m_us;      // us for microseconds
    unsigned int m_endUs;

    void UpdateTime() {
        auto end = hrc::now();
        auto diff = end - m_now;
        m_now = end;
        auto msDuration = std::chrono::duration_cast<std::chrono::microseconds>(diff);
        m_us += (unsigned int)msDuration.count();
    }

public:

    Animation() {
        m_us = 0;
        m_now = hrc::now();
    }

    bool finished() {
        return m_endUs <= m_us;
    }

    virtual bool Update(sf::Sprite& spr) = 0;

};

Animation child class

class FadeIn : public Animation {

public:

    FadeIn(int ms) {
        m_endUs = ms * 1000;
    }

    // Updates the sprite based on the timeline, and returns if the animation is over
    virtual bool Update(sf::Sprite& spr) {
        UpdateTime();
        if (finished()) return true;

        sf::Color color = spr.getColor();
        color.a = (int)((float)m_us / m_endUs * 255);
        spr.setColor(color);
        return false;
    }

};

Render Component

class RenderComponent {

private:

    sf::Texture m_texDefault;
    std::unique_ptr<Animation> m_animationPtr;

public:

    RenderComponent() { }
    RenderComponent(sf::Drawable* element, sf::Vector2u size) {
        sf::RenderTexture rt;
        rt.create((unsigned int)size.x, (unsigned int)size.y);
        rt.draw(*element);
        m_texDefault = rt.getTexture();
    }

    template <typename T, typename... Args>
    void SetAnimation(Args... args) {
        m_animationPtr = std::make_unique<T>(args...);
    }

    void draw(sf::RenderTarget* target) {
        sf::Sprite sprite;
        sprite.setTexture(m_texDefault);

        // Handle animation and set pointer to null if done
        if (m_animationPtr) {
            if (m_animationPtr.get()->Update(sprite)) {
                m_animationPtr = nullptr;
            }
            sf::Color c = sprite.getColor();
        }
        target->draw(sprite);
    }

};

A helper function

sf::Vector2u floatRectToVec2u(sf::FloatRect r) {
    sf::Vector2u vec;
    vec.x = (unsigned int)ceil(r.width);
    vec.y = (unsigned int)ceil(r.height);
    return vec;
    auto start = std::chrono::high_resolution_clock::now();
}

Main function

int main()
{
    sf::RenderWindow window(sf::VideoMode(200, 200), "SFML works!");
    sf::CircleShape shape(100.f);
    shape.setFillColor(sf::Color::Green);
    RenderComponent circle(&shape, floatRectToVec2u(shape.getGlobalBounds()));
    circle.SetAnimation<FadeIn>(1000);

    while (window.isOpen())
    {
        sf::Event event;
        while (window.pollEvent(event))
        {
            if (event.type == sf::Event::Closed)
                window.close();
        }

        window.clear();
        circle.draw(&window);
        window.display();
    }

    return 0;
}

Animation base class

typedef std::chrono::high_resolution_clock hrc;

In modern C++, use an alias-declaration:

using hrc = std::chrono::high_resolution_clock;

It is arguably more readable.

std::chrono::time_point<hrc> m_now;

unsigned int m_us;      // us for microseconds
unsigned int m_endUs;

The types and names of the last two members are not helpful.  (By the way, unsigned int = unsigned.)  And they should be private:

private:
    using time_point = std::chrono::high_resolution_clock::time_point;
    time_point m_now;

    using duration = std::chrono::high_resolution_clock::duration;
    duration m_time_elapsed;
    duration m_total_time;

The derived classes should have read-only access to m_total_time:

protected:
    auto total_time() const
    {
        return m_total_time;
    }

void UpdateTime() {
    auto end = hrc::now();
    auto diff = end - m_now;
    m_now = end;
    auto msDuration = std::chrono::duration_cast<std::chrono::microseconds>(diff);
    m_us += (unsigned int)msDuration.count();
}

This function is a bit complex because of the conversion between different types.  They can be simplified:

void UpdateTime()
{
    auto time = hrc::now();
    time_elapsed += time - m_now;
    m_now = time;
}

Animation() {
    m_us = 0;
    m_now = hrc::now();
}

The constructor should provide a means to set the m_total_time member.  And it should use member initializer clauses instead of assignment:

template <class Rep, class Period>
explicit Animation(const std::chrono::duration<Rep, Period>& total_time)
    : m_now{hrc::now()}
    , m_time_elapsed{}
    , m_total_time{total_time} // std::chrono::duration supports conversion
{
}

(The support for different durations is for convenience.)

bool finished() {
    return m_endUs <= m_us;
}

You are missing const.

virtual bool Update(sf::Sprite& spr) = 0;

Good.

Animation child class

FadeIn(int ms) {
    m_endUs = ms * 1000;
}

Missing explicit — an integer is not logically a FadeIn.  Type mismatch (you are using unsigned in the base class).  With the design mentioned, just do this:

using Animation::Animation;

And the constructors will work as expected.

// Updates the sprite based on the timeline, and returns if the animation is over
virtual bool Update(sf::Sprite& spr) {
    UpdateTime();
    if (finished()) return true;

    sf::Color color = spr.getColor();
    color.a = (int)((float)m_us / m_endUs * 255);
    spr.setColor(color);
    return false;
}

Missing override.  Don't use C-style casts.  float may be too imprecise for this calculation.  Don't put the whole if statement on a single line.

The color algorithm should be in a separate function:

private:
    sf::Color get_color() const noexcept
    {
        auto color = spr.getColor();

        double ratio = static_cast<double>(m_time_elapsed) / m_total_time;
        color.a = static_cast<int>(ratio * 255);
        return color;
    }

Also, if overflow is not a concern, just multiply first and then divide to avoid the floating point.  And then the function can be simplified:

virtual bool Update(sf::Sprite& spr) override
{
    UpdateTime();

    if (finished()) {
        return true;
    } else {
        spr.setColor(get_color());
        return false;
    }
}

Render Component

RenderComponent() { }
RenderComponent(sf::Drawable* element, sf::Vector2u size) {
    sf::RenderTexture rt;
    rt.create((unsigned int)size.x, (unsigned int)size.y);
    rt.draw(*element);
    m_texDefault = rt.getTexture();
}

Good — except for the C-style casts:

remove them if possible;
otherwise, use unsigned{size.x} if possible;
otherwise, use static_cast.

template <typename T, typename... Args>
void SetAnimation(Args... args) {
    m_animationPtr = std::make_unique<T>(args...);
}

You are missing perfect forwarding:

template <typename T, typename... Args>
void SetAnimation(Args&&... args)
{
    m_animationPtr = std::make_unique<T>(std::forward<Args>(args)...);
}

void draw(sf::RenderTarget* target) {
    sf::Sprite sprite;
    sprite.setTexture(m_texDefault);

    // Handle animation and set pointer to null if done
    if (m_animationPtr) {
        if (m_animationPtr.get()->Update(sprite)) {
            m_animationPtr = nullptr;
        }
        sf::Color c = sprite.getColor();
    }
    target->draw(sprite);
}

Always turn warnings on — unused c variable should issue a warning.  (I am pretty sure sprite.getColor() has any side effects.)

A helper function

sf::Vector2u floatRectToVec2u(sf::FloatRect r) {
    sf::Vector2u vec;
    vec.x = (unsigned int)ceil(r.width);
    vec.y = (unsigned int)ceil(r.height);
    return vec;
    auto start = std::chrono::high_resolution_clock::now();
}

This function is a pure math function, so should probably be noexcept.  The name is a bit awkward — it does not mention ceil at all.  Also, it seems that ceil should be std::ceil.  And what does the last line do?

If sf::Vector2u can be constructed with the coordinates, the code is simplified:

sf::Vector2u ceil_vector(sf::FloatRect r)
{
    return {std::ceil(r.width), std::ceil(r.height)};
}

Main function

int main()
{
    sf::RenderWindow window(sf::VideoMode(200, 200), "SFML works!");
    sf::CircleShape shape(100.f);
    shape.setFillColor(sf::Color::Green);
    RenderComponent circle(&shape, floatRectToVec2u(shape.getGlobalBounds()));
    circle.SetAnimation<FadeIn>(1000);

    while (window.isOpen())
    {
        sf::Event event;
        while (window.pollEvent(event))
        {
            if (event.type == sf::Event::Closed)
                window.close();
        }

        window.clear();
        circle.draw(&window);
        window.display();
    }

    return 0;
}

The main function looks nice.  (I don't why you are explicitly specifying 100.f here instead of 100, but maybe there's a good reason.)  return 0; is redundant for main and can be omitted.  event can be declared in the inner loop with for.


Counting distinct lists in a list I'm trying to remove the duplicate from the list and count the list after removing the duplicates

seq = [[1,2,3], [1,2,3], [2,3,4], [4,5,6]]
new_seq = [[1,2,3], [2,3,4], [4,5,6]]
count = 3

My code takes around 23 seconds for around 66,000 lists in a list

How can I make my code faster?

def unique(seq):
    new_seq = []
    count = 0
    for i in seq:
        if i not in new_seq:
            new_seq.append(i)
            count += 1
    return count

Your function is slow because it is O(n2): each element being added to new_seq has to be compared against every previously added element.

To deduplicate a sequence, use a set.  Constructing the set is only O(n) because it uses hashing.

Then, to obtain the size of the set, use len().

def unique(seq):
    return len(set(tuple(element) for element in seq))


Uri Routing PHP Code I do programing with core php & i don't want to like use any cms or mvc framework.   if any handy ideas then welcome it.

index.php file

<?php
/** INCLUDE BSIC CONFIGURATION FILE */
strip_php_extension();

/** GET REQUEST NAME */
$request_uri = trim(filter_input(INPUT_GET, 'uri_req'),'/');

/** CALL REQUESTED PAGE */
_PageCall($request_uri);

?>

function.php file

<?php
/**
 * strip_php_extension()
 *
 * check website url in .php file name is exist
 *
 * @param null
 * @return redirect the page with removing .php extension from the url bar
 */
function strip_php_extension()
{
  $uri = filter_input(INPUT_SERVER,'REQUEST_URI');
  $ext = substr(strrchr($uri, '.'), 1);
  if ($ext == 'php')
  {
    $url = substr($uri, 0, strrpos($uri, '.'));
    redirect($url);
  }
}

/**
 * _PageCall()
 *
 * check in web current web request
 *
 * @param null,filename
 * @return return file if or return 404 file
 */
function _PageCall($file_name=null){
    $pagedir = ROOT.DIRECTORY_SEPARATOR.PAGEDIR;

    /* Every File Request Must Be in HTML */
    if(".html"===substr($file_name, -5)):
        $file_name = trim($file_name,'.html');
        if(file_exists($pagedir.$file_name.'.php')):
            require_once $pagedir.$file_name.'.php';
        else:
            redirect('404.html');
        endif;
    else:
        if(NULL==$file_name||$file_name=='index.html'):
            require_once $pagedir.'index.php';
        elseif($file_name=='a'):
            require_once $pagedir.'ajax.php';
        else:
            redirect(SITEURL.'404.html');
        endif;
    endif;
} ?>

Guard against ../ in the uri_req parameter

I'm seeing a possible security hole. A big one. Consider the following URL:

http://yoursite.com/index.php?uri_req=../../hacking_scripts/dump_database.php

Now let's pretend for a moment that I'm criminal who has previously breached the web server that hosts your web application. Your app is stored at:

/www/himanshu/index.php

Now when I breached the web server, I created the following file:

/hacking_scripts/dump_database.php

Which contains PHP code to connect to your database as the same username and password as your web site, because after all, I can view the raw PHP code and found a database connection string with a username and password.

The dump_database.php script basically queries for all the tables your web site's user has available to it, does a SELECT * on the table and dumps out the raw data in the resulting HTML file. Upon hitting the URL above, your router gleefully loads that file and executes the PHP inside because I placed ../../ in the uri_req parameter.

Some other improvements:

Don't redirect to a 404 Not Found page, because the HTTP status will first be a 301 redirect to a GET request that results in a 200 OK response. Your intended response is 404 Not Found, so set the HTTP status to that value and simply include the 404.html file.

Web crawlers and search engines, when encountering a 404 Not Found, will not attempt (or should not attempt) to hit that URL again. In your setup, they will be back to the same "Not Found" URL over and over again.
The _PageCall function name doesn't really tell you what it does. It routes the request. Why not call it something like routeRequest?
Add a try-catch around the calls to other PHP files, then return a 500 Server Error response. This way you don't get ugly stack traces showing up in production, and you can create your own custom, prettier looking error page for unhandled catastrophic errors.

function _PageCall($file_name=null) {
    try {
        // route the request
    } catch (Exception $ex) {
        http_response_code(500);
        require_once($pagedir . "500.html");
        // Log the error message and stack trace
    }
}

Remove duplicated 404 Not Found logic by separating the detecting of a file to load versus the actual executing of that file

function _PageCall($file_name=null) {
    try {
        $pagedir = ROOT.DIRECTORY_SEPARATOR.PAGEDIR;
        $page = getFullFilePath($pagedir, $file_name);

        if (!isset($page)) {
            $page = $pagedir . '404.html';
            http_response_code(404);
        }

        require_once($page);
    } catch (Exception $ex) {
        http_response_code(500);
        require_once($pagedir . "500.html");
        // Log the error message and stack trace
    }
}

function getFullFilePath($pagedir, $file_name=null) {

    /* Every File Request Must Be in HTML */
    if(".html"===substr($file_name, -5)):
        $file_name = trim($file_name,'.html');
        if(file_exists($pagedir.$file_name.'.php')):
            return $pagedir.$file_name.'.php';
        else:
            return null;
        endif;
    else:
        if(NULL==$file_name||$file_name=='index.html'):
            return $pagedir.'index.php';
        elseif($file_name=='a'):
            return $pagedir.'ajax.php';
        else:
            return null;
        endif;
    endif;
}

Whitespace is your friend. For readability put spaces around all operators and the conditions for if statements:

if(".html"===substr($file_name, -5)):

Becomes:

if (".html" === substr($file_name, -5)):

And put some empty lines between IF statements:

$file_name = trim($file_name,'.html');

if (file_exists($pagedir . $file_name . '.php')):

Add additional functions for detecting certain kinds of files so that:

if (".html" === substr($file_name, -5)):

Becomes:

if (isHtmlFile($file_name)):

Use a consistent variable naming convention. I see $pagedir and $file_name. In the very least avoid using all lower case letters. Separate words by an underscore or use camelCase.

This isn't a bad script to do simple routing. Really the most important change is to fix the security flaw allowing you to specify any file path in the URL parameter.


Get an object with null values from an array of nested objects I have an array of nested objects. How do I get an object with null values using only a particular key in the array of objects?

let headers = [{
    title: 'Arun',
    id: 'arunId',
    onClick: 'onClickArun'
  },
  {
    title: "George",
    id: 'georgeId',
    onClick: '',
    children: [{
        title: 'David',
        id: 'davidId',
        onClick: 'onClickDavid'
      },
      {
        title: 'Patrick',
        id: 'patrickId',
        onClick: 'onClickPatrick'
      }
    ]
  },
  {
    title: 'Mark',
    id: 'markId',
    onClick: 'onClickMark'
  }
]

const headersMap = ({
  onClick,
  children
}) => (onClick ? {
  onClick
} : _.map(children, headersMap));

const headersFlatMap = _.flatMap(headers, headersMap);

const headerObj = _.reduce(_.map(headersFlatMap, 'onClick'), (ac, a) => ({ ...ac,
  [a]: null
}), {});

console.log(headerObj)
<script src="https://cdnjs.cloudflare.com/ajax/libs/lodash.js/4.17.15/lodash.js"></script>

The code works good, but is there a way to optimize it?

Expected Output:

{
  "onClickArun": null,
  "onClickDavid": null,
  "onClickPatrick": null,
  "onClickMark": null
}

Any help is greatly appreciated. Thanks.

Performance.

map and flatMap create copies of the arrays.
For each new item added to the result you rebuild the whole results object ({ ...ac, [a]: null})
For each property in the result you create a temp object to hold the key value pair.

These CPU, memory allocation, and GC burdens that can be avoided.

Some will argue that the overhead for small data sets is trivial and thus inconsequential.

Performance always matters and coding without though of performances leads to poor design habits that will bite. Many small inconsequential performance extras can quickly sum up to overall poor performance. Especially true in JavaScript where the target platform's capabilities vary so greatly. What is inconsequential on a top end desktop can be unusable on a $15 tablet.

Design

If you have followed the Code Review posting rules and your code is production code then it is very poor quality, as it is flat, in the global scope, and not reusable at all.

You should always write code inside a function. Even if its just an example writing functions will result in better design.

Style

The indenting is poor and obscuring the logic.
You have redundant code. Unneeded brackets () in headersMap.
Poor naming of the _.reduce callbacks arguments

Rewrite

Using your algorithm and rewriting into a named function your code is more readable, reusable, and cleaner (name space wise) as

function namedEvents(eventList) {
    const map = ({onClick, children}) => onClick ? {onClick} : _.map(children, map);
    const reduce = (result, child) => ({...result, [child]: null});
    return _.reduce(
        _.map(_.flatMap(eventList, map), 'onClick'),
       reduce,
       {}
    );
}

Avoiding the needless re-creation of results for each entry you get

function namedEvents(eventList) {
    const map = ({onClick, children}) => onClick ? {onClick} : _.map(children, map);
    const reduce = (result, child) => (result[child] = null, result);
    return _.reduce(
        _.map(_.flatMap(eventList, map), 'onClick'),
       reduce,
       {}
    );
}

Redesign

To avoid the CPU overhead the code can be written as a simple recursive function. The functions below are an order of magnitude quicker than your algorithm.

There is no need to load lodDash.js thus improving the page load time.

As you already use recursion I will assume that there are no cyclic references in the dataset.

Your code ignores children if onClick is truthy so will do the same

For the best re-usability the key "onClick" name can be passed as an argument.

function namedEvents(eventList, results = {}, key = "onClick") {
    for (const child of eventList) {
        if (child[key]) { results[child[key]] = null }
        else if (child.children) { namedEvents(child.children, results, key) }
    }
    return results;
}

or

function namedEvents(eventList, results = {}, key = "onClick") {
    for (const child of eventList) {
        if (child[key]) {
            results[child[key]] = null;
        } else if (child.children) {
            namedEvents(child.children, results, key);
        }
    }
    return results;
}

Or not having an adaptive key name

function namedEvents(eventList, results = {}) {
    for (const {onClick, children} of eventList) {
        if (onClick) { results[onClick] = null }
        else if (children) { namedEvents(children, results) }
    }
    return results;
}

Or

function namedEvents(eventList, results = {}) {
    return eventList.reduce((result, {onClick, children}) =>
            onClick? (results[onClick] = null, results) :
                children ? namedEvents(children, results) : results,
        results
    );
}


Code to classify elements on np.array on lists The idea of the code is to count how many elements in testy_L2 are the same with those on testy. So for example, in the first row of conf_matrix we have that 17 elements on testy_L2 are 0., one element is 1., and two elements are 2., so we have 17 correct numbers and 3 wrong numbers for 0. in testy_L2.
What do you think of this code? Is there another way to make it simpler/more elegant?
list_0 = []
a,b,c = 0,0,0
for j in indexes_0[0]:
    if testy_L2[j] == 0.0:
        a += 1
    if testy_L2[j] == 1.0:
        b += 1
    if testy_L2[j] == 2.0:
        c += 1
list_0.append(a)
list_0.append(b)
list_0.append(c)

list_1 = []
a,b,c = 0,0,0
for j in indexes_1[0]:
    if testy_L2[j] == 0.0:
        a += 1
    if testy_L2[j] == 1.0:
        b += 1
    if testy_L2[j] == 2.0:
        c += 1
list_1.append(a)
list_1.append(b)
list_1.append(c)

list_2 = []
a,b,c = 0,0,0
for j in indexes_2[0]:
    if testy_L2[j] == 0.0:
        a += 1
    if testy_L2[j] == 1.0:
        b += 1
    if testy_L2[j] == 2.0:
        c += 1
list_2.append(a)
list_2.append(b)
list_2.append(c)

Inputs:
testy:
array([1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1., 1.,
       1., 1., 1., 2., 2., 2., 2., 2., 2., 2., 2., 2., 2., 2., 2., 2., 2.,
       2., 2., 2., 2., 2., 2., 2., 2., 0., 0., 0., 0., 0., 0., 0., 0., 0.,
       0., 0., 0., 0., 0., 0., 0., 0., 0., 0., 0.])

testy_L2:
array([0., 0., 0., 1., 1., 0., 1., 0., 0., 1., 0., 1., 0., 0., 1., 1., 1.,
       0., 1., 1., 2., 2., 2., 2., 2., 2., 2., 2., 2., 2., 2., 2., 2., 2.,
       2., 2., 2., 2., 2., 2., 2., 2., 1., 0., 0., 0., 0., 0., 0., 0., 0.,
       0., 0., 0., 2., 0., 2., 0., 0., 0., 0., 0.])

Indexes_0, Indexes_1, Indexes_2:
for i in range(len(testy)):
    indexes_0, indexes_1, indexes_2 = np.where(testy == 0.0), np.where(testy == 1.0), np.where(testy == 2.0)

Output:
conf_matrix:
conf_matrix = np.array([list_0,list_1,list_2])

array([[17,  1,  2],
       [10, 10,  0],
       [ 0,  0, 22]])

You have essentially one chunk of code that's duplicated three times:
list_N = []
a,b,c = 0,0,0
for j in indexes_N[0]:
    if testy_L2[j] == 0.0:
        a += 1
    if testy_L2[j] == 1.0:
        b += 1
    if testy_L2[j] == 2.0:
        c += 1
list_N.append(a)
list_N.append(b)
list_N.append(c)

This could be easily wrapped in a function:
def process_the_stuff(indexes):
    acc = []
    a,b,c = 0,0,0
    for j in indexes[0]:
        if testy_L2[j] == 0.0:
            a += 1
        if testy_L2[j] == 1.0:
            b += 1
        if testy_L2[j] == 2.0:
            c += 1
    acc.append(a)
    acc.append(b)
    acc.append(c)
    return acc

Then just:
list_0 = process_the_stuff(indexes_0)
list_1 = process_the_stuff(indexes_1)
list_2 = process_the_stuff(indexes_2)

I'd clean up some other stuff though:

I'm not a fan of assigning multiple variables on a line in the vast majority of cases. I think a, b, and c should be initially assigned on separate lines.
The three append calls at the bottom could be cleaned up a bit by using extend, or just simply returning a tuple/list directly.
All three if checks are necessarily exclusive of each other (since testy_L2[j] can't be equal to 0, 1, and 2 at the same time). Those should be elifs instead so that once one is true, the others aren't checked.

After fixing these things, you'd be left with:
def process_the_stuff(indexes):
    a = 0
    b = 0
    c = 0
    for j in indexes[0]:
        if testy_L2[j] == 0.0:
            a += 1
        elif testy_L2[j] == 1.0:
            b += 1
        elif testy_L2[j] == 2.0:
            c += 1
    return [a, b, c]  # May make more sense to return a tuple instead unless you need a list

list_0 = process_the_stuff(indexes_0)
list_1 = process_the_stuff(indexes_1)
list_2 = process_the_stuff(indexes_2)

Minor, but "indexes" should be "indices" for the plural of "index".


Repository Pattern with Dapper + Unity I just finished the Pluralsight courses about Inversion of Control and Mirco-ORMs, and I am struggling with the implementation aspect of some of the concepts.

This is my (simplified) repository implementation using Dapper:

public class AppRepository : IAppRepository
{

    public string TableName { get; } = "apps";
    private IDbConnection _db;

    public AppRepository(IDbConnection db = null)
    {
        if (db == null)
            _db = Bootstrap.container.Resolve<IDbConnection>(); // Default: connection from the simple injector container
        else
            _db = db;
    }

    public List<App> GetAll() => _db.Query<App>(QueryBuilder.BuildGetAll(TableName)).ToList();

}

QueryBuilder is a static class, shared among all repositories, which creates the different sql strings.

The Bootstrap class exposes a UnityContainer:

public static class Bootstrap
{
    public static UnityContainer container;

    private static IDbConnection _db;
    public static void Start(string connectionStringId)
    {
        container = new UnityContainer();

        _db = GetDbConnection(connectionStringId);

        // Register your types:
        container.RegisterInstance<IDbConnection>(_db);
    }

    public static IDbConnection GetDbConnection(string connectionStringId)
    {
        var connectionString = ConfigurationManager.ConnectionStrings[connectionStringId].ConnectionString;
        IDbConnection db = new MySqlConnection(connectionString);
        return db;
    }
}

Then I read a post about NOT to use this kind of pattern:

  by far the most common IoC mistake is to wrap up the container in a public static or singleton class that is referenced throughout the code base. It is important to realize that this is not dependency injection, it is service location which is widely regarded as an anti-pattern. I cannot over-emphasize how important it is to move away from this design and to inject your dependencies from the root of your application. In fact, virtually all other IoC mistakes come about as a direct result of this misunderstanding.

I've used IDbConnection db = null because 99% of the time I will be using a single thread with a single connection, but the other 1% I will be transfering data from one server to another.

I have ~60 models (60 tables), so constantly injecting the IDbConnection connection seems like a hassle, specially if further down the path I have to change it somehow. At the same time, Unity is doing nothing useful at this point in time. I could just let the connection be a public property of the static Bootstrap class. I've read some opinions about injectors being more more annoying than helpful.

For the moment I am building a console application, and in the Main function there is a Bootstrap.Start() that lets me forget about IDbConnection for the rest of the code.

I've been trying to find some examples with good patters using Dapper + dependency injection, but I haven't found anything yet that clarifies my doubts.

I don't think you get much benefit from using the IDbConnection. What I do is just use connection in my repository and use IoC just for the IRepository. The other problem with your code is that there is nothing disposing your connection. Since it's a console app it might not be a big deal, but typically you want to open a connection, do a bunch of work and then close it. If this was a web app for instance you'd run out of pools in your connection pool. Typically you want your IoC being the one to new up instances of classes, not pass in one you already created.


Sliding window to solve "longest substring, no repeating chars" Question: Longest Substring Without Repeating Characters

  Given a string, find the length of the longest substring without
  repeating characters.

  Examples:

  Given "abcabcbb", the answer is "abc", which the length is 3.

  Given "bbbbb", the answer is "b", with the length of 1.

  Given "pwwkew", the answer is "wke", with the length of 3. Note that
  the answer must be a substring, "pwke" is a subsequence and not a
  substring.

Solution:
We use a sliding window to define the current substring. In my solution, we start with head = position 0 and tail = position 0 in a way where
[H][T]a b c a b c b b and NO characters have been marked as "seen". After the first iteration, head=0, tail=1, our positioning is [H]a[T]b c a b c b b and our first character a at position 0 has been seen.

We then advance the tail and add the character seen to a hashmap hm. If we've already seen this character, we advance the head point to one position past the position the character was last seen. We are done once the tail position is equal to the length of the string.

Why I am posting here asking for feedback is that my #1 concern is that there's no proper way to define my sliding window method. How am I supposed to explain my method to an interviewer? I can't define it mathematically, as it's not [head, tail) nor is it (head, tail). What would be a way to improve my sliding window method so that it's well-defined and I can re-use this strategy every time I need to use a sliding window ?

class Solution {
    int head = 0;
    int tail = 0;
    int max = 0; // current max length

    public int lengthOfLongestSubstring(String s) {
        Map<Character, Integer> hm = new HashMap<>(); // value = position of character
        int currSize = 0;

        while(tail < s.length()) {
            tail++;
            char curr = s.charAt(tail-1);
            if (!(hm.containsKey(curr) && hm.get(curr) >= head)) {
                hm.put(curr, tail-1);
                compareMax(++currSize);
            } else {
                head = hm.get(curr)+1;
                hm.put(curr, tail-1);
                currSize = tail-head;
            }
            System.out.println("head: " + head + ", tail: " + tail + ", size: " + currSize);
        }
        return max;
    }

    void compareMax(int currSize) {
        if (currSize > max) {
            max = currSize;
        }
    }
}

Output for 'abcabcbb':

head: 0, tail: 1, size: 1
head: 0, tail: 2, size: 2
head: 0, tail: 3, size: 3
head: 1, tail: 4, size: 3
head: 2, tail: 5, size: 3
head: 3, tail: 6, size: 3
head: 5, tail: 7, size: 2
head: 7, tail: 8, size: 1

Summary: stay with your implementation.

I don't think a re-usable sliding window fits well to your algorithm.

What are the operations that come to one's mind with both a generic sliding String window and with your algorithm (I'll stay with your terminology of head and tail, although personally I'd name them just the other way round)?

advanceHead() generic op, not used by your algo (see jumpHead())
advanceTail() generic op, used by your algo
jumpHead(int newPos) only specific to your algo
getSubstring() generic op, not used by your algo
findInSubstring(char ch) maybe generic op, not really used by your algo (see below method)
lastOccurrenceBeforeTail(char ch) used by your algo, but I wouldn't expect such a method in a generic sliding window

So, there's hardly any overlap between your algo's sliding window and operations one would expect from a re-usable generic sliding window.

If you'd extract a sliding window from your algorithm, it would be burdened by quite specific methods that no-one else would ever use.

If you were to use a generic sliding window in your algorithm, you'd still have to maintain the character-positions storage and do the head-jumping individually.

One optional refactoring of your code could be to extract a NonRepeatingSlidingWindow with two public methods:

advance() increments the tail, jumps the head if necessary and returns a "finished"-flag
getLength() returns the current size of the sliding window.


Adding a zero to file names I've recently discovered the power of sed; regular expressions seem like they'll take a lifetime to master. This tiny script uses both to add a zero before the single digit in file names.

Can this be done more easily without sed? Is the regex sound?

#!/bin/bash

# > addzero.sh <
# Adds a zero before the single digit in file names separated by an underscore.
# Designed to help keep files in sequence in directory listings. Otherwise, you get listings like
# "_0.txt, _1.txt, _10.txt, _11.txt, _12.txt, ..., _2.txt, _20.txt, _21.txt, _22.txt, ..."

for filename in $(ls -1 ./*); do
  if [[ $filename =~ _[0-9]\. ]]; then
    new_filename=$(echo $filename | sed -n 's/_\([0-9]\)\./_0\1\./p')
    mv $filename $new_filename
  fi
done

What's wrong

for filename in $(ls -1 ./*); do

Ouch! As a general rule (with very few exceptions), do not use ls in scripts. What you wrote is almost equivalent to for filename in ./*; do, except that if there are any non-printable characters, whitespace characters, or \[?* in the file names, they will be mangled if you use ls. You don't need the ./ (except to ensure that the file name doesn't start with a -) but it doesn't hurt.

[[ $filename =~ _[0-9]\. ]]

could be written in a slightly simpler and more portable way: [[ $filename = *_[0-9].* ]]. And since you can use a simple glob pattern, you might as well not have iterated over all file names: for filename in *_[0-9].*; do. But there's a better way to express your script, to take advantage of BASH_REMATCH; see below.

echo $filename

Always put double quotes around variable and command substitutions. Exception: when you understand why you need to leave the double quotes off and why it's safe to do so. When the shell sees a variable substitution ($foo or ${foo}) or a command substitution (`foo` or $(foo)) outside double quotes, the result of the substitution undergoes word splitting and globbing (filename generation). That was one of the problems with $(ls -1 ./*) earlier. This should be echo "$filename".

In fact, it would be better printf "%s" "$filename", because echo itself performs expansions. In bash, unless you've set non-default options to enable backslash expansion, the only problem is that a few arguments beginning with a - look like option, and in this specific case the filename will begin with ./.

There is an edge case where your sed call won't work: if you have a file name that ends with a newline character. This doesn't happen in practice unless someone has made a mistake (like a rogue script or a bad copy-paste) or is deliberately trying to trick your script — so watch out in security contexts.

Incidentally, this is one of the few cases where it's safe to leave out the double quotes around a command substitution: in a variable assignment, there is an implicit pair of double quotes around the right-hand side, so new_filename=$(…) is equivalent to new_filename="$(…)". Note that this does not extend to export VARIABLE="$(value)", where the double quotes are necessary.

We now turn to your question about the use of sed. It is not necessary here; you can perform this substitution in bash. Bash has a pattern replacement parameter substitution feature, but it's limited to a constant replacement text. Bash also has a way to extract substrings from regexp matches with =~, through the BASH_REMATCH variable. After the match, ${BASH_REMATCH[0]} contains the portion of the string matched by the regexp, ${BASH_REMATCH[1]} contains the portion matched by the first parenthesized group and so on.

mv $filename $new_filename

Again, double quotes.

A working script

One possibility is to extract the text to replace from the regexp match, then perform a string replacement on it. Since I'm not using ./*, the file name may begin with a -, so I take care to use -- on the call to mv to ensure that the file name isn't seen as an option.

for filename in *; do
  if [[ $filename =~ _[0-9]\. ]]; then
    from=${BASH_REMATCH[0]}
    mv -- "$filename" "${filename//$from/${from/_/_0}}"
  fi
done

Another possibility is to match the whole file name as a regexp and splice a 0 into the bits. Note that this will behave differently in a corner case: if there are several occurrences of _[0-9]\., the code above replaces the first occurrence, while this replaces the last occurrence.

for filename in *; do
  if [[ $filename =~ ^(.*_)([0-9]\..*)$ ]]; then
    mv -- "$filename" "${BASH_REMATCH[1]}0${BASH_REMATCH[2]}"
  fi
done

You can also use other constructs and avoid regexps altogether, but it's more complicated. An advantage is that the script then works in all shells, not just bash.

for filename in *_[0-9].*; do
  digit=${filename%.${filename##*_[0-9].}}; digit=${digit##*.}
  mv -- "$filename" "${filename%%_[0-9].*}_0${digit}.${filename#*_[0-9].*}"
done

There are other methods to perform this kind of file renaming. If you're on Debian or derivative (including Ubuntu) or have the rename Perl script that's floating around (note that this is not the standard Linux rename utility):

rename 's/_([0-9]\.)/_0$1/' *_[0-9].*

Or with mmv:

mmv -x '*_[0-9].*' '#1_0#2.#3'

Or in zsh, after autoload zmv:

zmv '(*_)([0-9].*)' '${1}0$2'

Further reading

The rename and quoting tags on Unix Stack Exchange.


has_cycle in directed and undirected graphs I wrote a program that can tell if a graph has_cycle or not.

Can you please suggest more elegant and eloquent ways for this program?
(Perhaps better ways to represent a graph with vertices and edges?)

def has_cycle(g, directed=False):
    def dfs(x, entered, exited, tree_edges, back_edges):
        if x not in entered:
            entered.add(x)
            for y in g[x]:
                if y not in entered:
                    tree_edges[y] = x
                elif (not directed and tree_edges.get(x, None) != y
                      or directed and y not in exited):
                          back_edges.setdefault(y, set()).add(x)
                dfs(y, entered, exited, tree_edges, back_edges)
            exited.add(x)
        return (tree_edges, back_edges)
    for x in range(len(g)):
        if dfs(x, entered=set(), exited=set(), tree_edges={}, back_edges={})[1]:
            return True
    else:
        return False

Test Cases - Directed Graphs

'''    A0 →  C2 → E4
graph: ↓  ↗  ↓   ↗
       B1    D3     '''
a, b, c, d, e = range(5)
g = [
    {b, c}, # a
    {c},    # b
    {d, e}, # c
    {e},    # d
    set()   # e
]

assert not has_cycle(g, True)

g[a] = {b}
g[c] = {a, d, e}
assert has_cycle(g, True)

Test Cases - Directed Graphs

# undirected graph: A0 - B1 - C2
a, b, c = range(3)
g2 = [
    {b},    # a
    {a, c}, # b
    {b}     # c
]
assert not has_cycle(g2, False)

# undirected graph: A0 - B1 - C2 - A0
g2[a].add(c)
g2[c].add(a)
assert has_cycle(g2, False)

Comments regarding graphs:

The typical way to describe a graph G is G = {V, E} where V is the set of vertices and E is a bag of edges. The number of vertices is denoted n. The number of edges e [1]. Convention is to re-label the vertices by number either from 1 to n or from 0 to n-1 for the sake of programming conventions.

E is a bag because redundant edges matter in several important graph algorithms (e.g. minimum cut). If u and v are vertices then a non-directed edge is annotated u--v and a directed edge from u to v as u-->v.

For sparse graphs, the bag of edges is typically represented as a sequence [e.g. array or list] of tuples (u,v). For dense graphs an adjacency matrix of n rows and n columns is more typical ("sparse" and "dense" are terms of art, profiling to determine the data structure is appropriate).

Using mathematical language for variables and constants will make for more readable code, as will following conventions for vertex labeling and the data structures for edges.

[1] Art of Computer Programming: Volume 4A, Knuth.


Making an array of spiral numbers Context
I was solving a task on CodeSignal here
You can view the task on the link I provided above.
I have also put the task at the bottom of the post.
Code
def check(matrix, num):
    for arr in matrix:
        for number in arr:
            if number != num:
                return True
    return False

def spiralNumbers(n):
    array = []
    array_dup = []
    for i in range(n):
        arr1 = []
        for j in range(n):
            arr1.append(j)
        array.append(arr1)

    for i in range(n):
        arr1 = []
        for j in range(n):
            arr1.append(j)
        array_dup.append(arr1)

    selected_row, selected_num_pos = 0, -1
    count = 1
    run = True
    while run:
        its_neg = False
        for i in range(selected_num_pos+1, n):
            if array_dup[selected_row][i] == -1:
                its_neg = True
                break
            array[selected_row][i] = count
            array_dup[selected_row][i] = -1
            count += 1
        if its_neg:
            selected_num_pos = i-1
        else:
            selected_num_pos = i

        its_neg = False
        for i in range(selected_row+1, n):
            if array_dup[i][selected_num_pos] == -1:
                its_neg = True
                break
            array[i][selected_num_pos] = count
            array_dup[i][selected_num_pos] = -1
            count += 1
        if its_neg:
            selected_row = i-1
        else:
            selected_row = i

        its_neg = False
        for i in range(selected_num_pos-1, -1, -1):
            if array_dup[selected_row][i] == -1:
                its_neg = True
                break
            array[selected_row][i] = count
            array_dup[selected_row][i] = -1
            count += 1
        if its_neg:
            selected_num_pos = i+1
        else:
            selected_num_pos = i

        its_neg = False
        for i in range(selected_row-1, -1, -1):
            if array_dup[i][selected_num_pos] == -1:
               its_neg = True
               break
            array[i][selected_num_pos] = count
            array_dup[i][selected_num_pos] = -1
            count += 1
        if its_neg:
            selected_row = i+1
        else:
            selected_row = i

        run = check(array_dup, -1)

    return array

Question
The Code I wrote works without any error and returns the expected output, but the code seems a bit long for this problem. I wanted to know how can I make this code shorter and more efficient?
The Task

You're using non-standard names. Usual names are A for the matrix, i for the row index, and j for the column index.
List repetition and comprehension make the initialization shorter and faster.
Using deltas for the four directions can avoid quadrupling code.
You can know when to change direction by checking whether the next element in the current direction is already set.
Your check is inefficient. Instead of searching the matrix for an unset spot, just do while count <= n**2:. Or, try to loop through the range of numbers.
Your current code crashes for n = 0 because it always enters the loop, as you compute run only at the end. With while count <= n**2: you'd succeed.

def spiralNumbers(n):
    A = [[0] * n for _ in range(n)]
    i = j = di = 0
    dj = 1
    for A[i][j] in range(1, n**2 + 1):
        if A[(i+di) % n][(j+dj) % n]:
            di, dj = dj, -di
        i += di
        j += dj
    return A

The % n is just a little trick to avoid checking for index-out-of-bounds.


Manipulating filenames using Python I was tasked with creating a script to be able to rename some files and then move them into different folders based on a code in the filename.

import os
import shutil

path = (r"C:\user\reports")

filelist = [ f for f in os.listdir(path) if f.endswith(".xml") ]

for f in filelist:
    x = os.path.join(path, f)
    os.remove(x)

filelist = [ f for f in os.listdir(path) if f.endswith(".pdf") ]

for f in filelist:
    os.rename(os.path.join(path, f),os.path.join(path, f[31:]))

filelist = [f for f in os.listdir(path) if f.endswith(".pdf")]

for f in filelist:
    if "A" in f[-6:]: shutil.copy(os.path.join(path, f),r"C:\user\reports\folderA")
    if "B" in f[-6:]: shutil.copy(os.path.join(path, f),r"C:\user\reports\folderB")
    if "C" in f[-6:]: shutil.copy(os.path.join(path, f),r"C:\user\reports\folderC")
    if "D" in f[-6:]: shutil.copy(os.path.join(path, f),r"C:\user\reports\folderD")
    if "E" in f[-6:]: shutil.copy(os.path.join(path, f),r"C:\user\reports\folderE")
    if "F" in f[-6:]: shutil.copy(os.path.join(path, f),r"C:\user\reports\folderF")
    if "G" in f[-6:]: shutil.copy(os.path.join(path, f),r"C:\user\reports\folderG")
    if "H" in f[-6:]: shutil.copy(os.path.join(path, f),r"C:\user\reports\folderH")
    if "I" in f[-6:]: shutil.copy(os.path.join(path, f),r"C:\user\reports\folderI")

filelist = [f for f in os.listdir(path) if f.endswith(".pdf")]

for f in filelist:
    x = os.path.join(path, f)
    os.remove(x)

First, it removes any XML files in the folder. Then it strips the first 31 characters (it will always be 31 characters) of the filename, leaving just a name and a code. It then checks if the final 6 characters contain a "code" and then move the file to the correct folder based off that code. After every file has been moved, it will delete every file in the parent folder.

I know my current code could be improved, but it's been a couple of years since I've done Python so I've literally done it the easiest way (from what I could Google) possible. Any improvements or suggestions to what features of Python I should be looking at would be great.

Don't reinvent the wheel

For getting the files with a specific extension you should use glob:

xmlFiles = glob.glob('*.xml')

(Note that this will work relative to the current path.)

Avoid code duplication

You have some serious code duplication going on, replace it my appropriate code like:

#I don't really know the semantics of your paths so you should rename this accordingly
allowedPaths = 'ABCDEFGHI'
for filename in filelist:
    for character in allowedPaths:
        if character in f[-6:]: shutil.copy(os.path.join(path, f),
                                            os.join.path(path, r"folder" + character))

Magic numbers

Avoid these magic numbers by replacing them with "constants"

PREFIX_LENGTH = 31
POSTFIX_LENGTH = 6
#...
os.rename(os.path.join(path, f),os.path.join(path, f[PREFIX_LENGTH:]))
#...
if character in f[-POSTFIX_LENGTH:]:


refactor javascript - show and hide fields based on current selection I need help refactoring the javascript. http://jsfiddle.net/BDLYP/

I was able to hide and show fields, but I know this can be improved upon.

If #state is empty, hide #high-schools and #other_high_schools

If #state is picked, show #high-schools and show #other_high_schools (if #high-schools is empty)

I imagine the multiple .live can be cut down. I am fairly new to javascript/jQuery

<select name="state" id="state">
    <option value=""></option>
    <option value="NY">NY</option>
    <option value="CA">CA</option>
</select>

    <select name="high_school_id" id="high_school">
        <option value=""></option>
        <option value="1">Degrassi High</option>
        <option value="2">Bayside High</option>
    </select>

Enter hs if your school is not listed above
        <input type="text" name="other_high_school" id="other-hs-field" />

$(document).ready(function() {
   checkHighSchool();

   $('#high_school').live('change', function() {
    checkHighSchool();
  });

  $('#state').live('change', function() {
    checkHighSchool();
  });
});

function checkHighSchool() {
  var state = $("#state").val();
  var high_school = $("#high_school").val();

  if(state) {
    $("#high-schools").show();
  }

  if(!state) {
    $("#other_high_school").hide();
    $("#high-schools").hide();
    $("#other-hs-field").val('');
    //alert('no state selected');
  }

  if(high_school && state) {
    $("#other_high_school").hide();
    $("#other-hs-field").val('');
  }

  if(!high_school && state) {
    $("#other_high_school").show();
  }
}

I will give you a sample which is very simplified. I hope you can understand what I am doing here.

HTML

<select class="toggle-data">
    <option></option>
    <option value="1" data-toggle="#foo-one">foo one</option>
    <option value="2" data-toggle="#foo-two">foo two</option>
</select>

    foo one

    foo two

JS/jQuery

$('.toggle-data').on('change', function() {
    var toggle = this.options[this.selectedIndex].getAttribute('data-toggle'),
        toggle_class = 'unique_classname';
    $('.'+toggle_class+', '+toggle).toggle().removeClass(toggle_class);
    $(toggle).addClass(toggle_class);
});

Now you have a single function to do all of your show/hiding and you would just format you html accordingly.


Displaying a scheduled job under a scheduled time I use at to schedule jobs and atq to display the scheduled jobs. But it mildly irritates me in that I have to look up each job individually.  I wrote a Python script to display the scheduled job under the scheduled time.

(In my world all the commands will begin 'task')

So instead of

stafflinux$atq
8   Mon Aug 18 09:00:00 2014 a joseph
10  Tue Aug 19 09:00:00 2014 a joseph
15  Thu Aug 21 09:00:00 2014 a joseph
12  Fri Aug 22 09:00:00 2014 a joseph
9   Thu Aug 21 09:00:00 2014 a joseph
14  Sat Aug 30 09:00:00 2014 a joseph
7   Sun Aug 17 09:00:00 2014 a joseph
6   Mon Aug 18 09:00:00 2014 a joseph
11  Sat Aug 30 09:00:00 2014 a joseph
stafflinux$

My script produces

stafflinux$./atscript.py
8  Mon Aug 18 09:00:00 2014 a joseph
task "buy a four-way plug adapter"
task "see guy about meeting"
-----------------------------------------
10  Tue Aug 19 09:00:00 2014 a joseph
task "bring back personal effects from office"
-----------------------------------------
15  Thu Aug 21 09:00:00 2014 a joseph
task "book tickets for next week"
-----------------------------------------

I'm looking for any feedback - particularly in terms of 'pythonic' style and any and all tricks I may have missed:

#!/usr/bin/python
import os
os.system("atq > attemp.txt")
file = open("attemp.txt")
line = file.readline()
while line:
    number =line[:2]
    print line.strip()
    os.system("at -c "+ number+ "| grep task")
    line=file.readline()
    print '-----------------------------------------'
print line
os.system("rm attemp.txt")

Your script creates a temporary file unnecessarily, uses os.system which is not recommended, and other messy things. Consider this alternative:

#!/usr/bin/env python

import subprocess
import re

re_job_number = re.compile(r'\d+')

for atq_line in subprocess.Popen(['atq'], stdout=subprocess.PIPE).stdout:
    job_number = re_job_number.search(atq_line).group()
    print atq_line.strip()
    for at_line in subprocess.Popen(['at', '-c', job_number], stdout=subprocess.PIPE).stdout:
        if at_line.startswith('task '):
            print at_line.strip()
    print '-' * 40

Advantages:

No need for a temporary file to save the atq output, you can process it directly
For running shell commands, subprocess is the recommended way, os.system is discouraged
No need for a grep process, you can handle the output filtering of at -c in Python itself

Also, writing the first line this way makes the script more portable:

#!/usr/bin/env python

If you write as #!/usr/bin/python then you are effectively hard-coding the Python path, and it might not be the same in all systems.

Finally, I recommend writing print(something) instead of print something, to keep your script ready for migrating to Python 3 someday.


Normalizing Invoices I need to move data from one database to another, and since I don't have SSIS I'm doing this ETL with T-SQL scripts.

One of the source tables contains invoice details, and features a column that contains the number of units invoiced per size (it's clothing); as I transfer the data over to my database, I'm normalizing this information. The query works fine, the execution plan looks exactly as I expected, doesn't recommend adding any indexes, ...but it takes about 10 minutes to process.

The source table Staging.dbo.[SourceTable] contains about 780K rows; the query inserts 1.5M rows into the destination table [DestinationDatabase].dbo.InvoiceDetailSizes.

The query is scheduled to run daily, as part of an overnight process - the 10 minutes don't really matter, but still I want to be sure everything is as efficient as it can be.

with cteSizedInvoices (
    InvoiceNumber, InvoiceLine, SizeRangeCode, UnitsPerSize
)
as (select
        src.f2,
        cast(src.f5 as int),
        src.f23,
        src.f19
    from Staging.dbo.[SourceTable] src
    where src.f1 = '01'
)

--insert into [DestinationDatabase].dbo.InvoiceDetailSizes (InvoiceDetailId, SizeId, Units, DateInserted)
select
    detail.Id InvoiceDetailId,
    sz.Id SizeId,
    buckets.Units,
    getdate()
from
    cteSizedInvoices src
        inner join [DestinationDatabase].dbo.InvoiceHeaders header on src.InvoiceNumber = header.Number
        inner join [DestinationDatabase].dbo.InvoiceDetails detail on src.InvoiceLine = detail.LineNumber
                                                                   and detail.InvoiceHeaderId = header.Id
        inner join [DestinationDatabase].dbo.SizeRanges ranges on src.SizeRangeCode = ranges.Code
        cross apply (select id SizeIndex, cast(item as int) Units from [DestinationDatabase].dbo.BucketString(src.UnitsPerSize, 5, 1, 1)) buckets
        inner join [DestinationDatabase].dbo.Sizes sz on buckets.SizeIndex = sz.SizeRangeIndex
                                                      and sz.SizeRangeId = ranges.Id
    left join [DestinationDatabase].dbo.InvoiceDetailSizes dst
        on detail.Id = dst.InvoiceDetailId
        and sz.Id = dst.SizeId
where
    buckets.Units <> 0
    and dst.id is null;

Here is the BucketString table-valued function that I'm using - it's adapted from this Stack Overflow answer, modified so that I could specify an "offset", because my data doesn't start at position 1:

create function [dbo].[BucketString] (
    @values varchar(max),
    @bucketSize int,
    @bufferSize int = 1,
    @offset int = 0)

returns @result table (id int, item varchar(max))

begin

    with buckets as
    (
        select 1 id

        union all

        select t.id + 1
        from buckets t
        where id = t.id
            and t.id < len(@values)/(@bucketSize+@bufferSize)+1
    )

    insert into @result
    select
        id,
        substring(@values, @offset + ((id - 1) * (@bucketSize + @bufferSize) + (case when @bufferSize-1 = 0 then @bufferSize else @bufferSize-(@bufferSize-1) end)), @bucketSize) string
    from buckets
    option (maxrecursion 0)

    return;

end

The query will not insert anything if the data already exists in the destination table, but in order to find out if it exists, I need to run the whole thing, so whether I'm inserting 1.5M rows, or 0, it's still ~10 minutes.

Can it be optimized in any way?

You use the value of (@bucketSize + @bufferSize) inside of a where clause and to calculate a value in a select statement, I think if you took that and created it's own variable, that it might give you some performance increase.  as well as some of the other arithmetic that could be done from the start and not calculated during the actual query

len(@values)/(@bucketSize+@bufferSize)+1

could be declared from the beginning

DECLARE maxID
SET maxID = LEN(@values) / (@bucketSize + @bufferSize) + 1

After you do that it is a toss up whether or not to Declare another Variable for the Addition of the @bucketSize + @bufferSize, but it might be worth a try.

Also please use some spaces in your equations, you use plenty of white space everywhere else.

I have a minute or two so I will also say this...

since this is going over so many records and that simple Variable freed up a good 10 seconds, I think that it might be worth it to move as much static calculation out of the query as possible, I know this is less about what SQL was meant for, but I also think this is more about being Dynamic as well.

Maybe I am going a little too far here, but this should still do the same thing as the original

create function [dbo].[BucketString] (
    @values varchar(max),
    @bucketSize int,
    @bufferSize int = 1,
    @offset int = 0)

returns @result table (id int, item varchar(max))

begin

    DECLARE @bucketAndBufferSize
    DECLARE @maxID
    DECLARE @bufferMinusBufferMinusOne
    DECLARE @bufferMinusOne

    SET @bucketAndBufferSize = @bucketSize+@bufferSize
    SET @maxID = LEN(@values) / (@bucketAndBufferSize) + 1
    SET @bufferMinusBufferMinusOne = @bufferSize - (@bufferSize - 1)
    SET @bufferMinusOne = @bufferSize - 1

    with buckets as
    (
        select 1 id

        union all

        select t.id + 1
        from buckets t
        where id = t.id
            and t.id < @maxID
    )

    insert into @result
    select
        id,
        substring(@values, @offset + ((id - 1) * @bucketAndBufferSize + (case when @bufferMinusOne = 0 then 1 else @bufferMinusBufferMinusOne end)), @bucketSize) string
    from buckets
    option (maxrecursion 0)

    return;

end

I apologize for the mismatched casing, it's habit to capitalized those words...

When bufferSize is unchanged you want it to be 1 so let's just do that and take out the extra call to the variable.

I took out all the arithmetic that wasn't reliant on information from the query and made them their own variable, this isn't going to be a huge difference, but I think it will be faster than doing the arithmetic inside the query itself, it's like being distracted by that red ball all the time, it distracts you momentarily but you get the job done.


Creating an async consumable object in Node Background

I have a situation where I have fixed number of objects with their own routines that would need to be called with the same initial message.

To accomplish this, I have a Factory that binds objects to a Composite object whose main method is to call methods on its subordinates without having to know how many it has.

This routine is to be called upon receiving a message from a queue, the client to that queue is responsible for initiating that procedure.

Code/ Question

Just to keep the code looking succinct, here are the individual modules for completeness.

//-------------------------------------------------------------------
// module1/index.js
//-------------------------------------------------------------------

function A () {}

A.prototype.method = function(msg, cb) {
  console.log('A class: %j', msg );
  cb(null, 'done message');
};

//-------------------------------------------------------------------
// module2/index.js
//-------------------------------------------------------------------

function B () {}

B.prototype.method = function(msg, cb) {
  console.log('B scraper: %j', msg);
  cb(null, 'done message');
};

Here is the factory that has the job of knowing what it imports and attaching the objects defined above to the Factory's caller:

//-------------------------------------------------------------------
// - factory.js
//-------------------------------------------------------------------

var A = require('./module1');
var B = require('./module2');

function Factory () {
  var o = Object.create(null);
  o.obj1 = new A();
  o.object2 = new B();
  return o;
}

Now here's the part that is in question. The method method does some 'clever' stuff to make something that the async library can understand and use. This code runs, and it about as efficient as it needs to be but I would like to ask if there is another, more readable way to build an object for use in async.parallel().

//-------------------------------------------------------------------
// - composite.js
//-------------------------------------------------------------------
var async = require('async');

function Composite () {
  this.objects = new Factory();

}

Composite.prototype.method = function(msg) {
  var self = this;
  var enum = Object.keys(this.objects);
  var newObj = {};
  // for each member-name in the obj.
  enum.forEach(function (name) {
    // put a function in the newObj
    newObj[name] = function (callback) {
      // who calls the object's method with some initial values
      // and a callback
      self.objects[name].method(msg, function(err, results){

        if (err) { callback(err); }
        callback(null, results);

      });

    };
  });
  // actually do something
  async.parallel(
    newObj,
    function(err, results) {
      console.log(results);
  });
};

module.exports = Composite;

This is just here for extra context.

//-------------------------------------------------------------------
// RMQ Client
//-------------------------------------------------------------------
var Composite = require('./composite');
var Client = {};

Client.on('message', startJob, msg)

function startJob (msg) {
  var comp = new Composite();
  comp.method(msg);

}

There is an error that needs to be handled correctly:

      self.objects[name].method(msg, function(err, results){

        if (err) {
           return callback(err);
        }
        return callback(null, results);

      });

You need to return the callback on the error, otherwise if an error comes down, you will hit both callbacks.


Function to determine the direction of an arrow I have a function in a project that I feel could be written better. The purpose of the function is to return the direction of an arrow based on two parameters.

positiveDirection: The direction for a positive result. The values could be increasing or decreasing.
changeType: Whether the result was positive, negative or no-change.

I guess it's the if statement that is bothering me the most; it looks like it could be reduced based on some logic gate that I don't know of.

  getArrowDirection = (positiveDirection, changeType) => {

    let direction = null

    if(changeType === 'no-change') {
      return direction
    }

    if(
        (changeType === 'positive' && positiveDirection === 'increasing') ||
        (changeType === 'negative' && positiveDirection === 'decreasing')
      ) {
      direction = 'up-arrow'
    } else if(
        (changeType === 'positive' && positiveDirection === 'decreasing') ||
        (changeType === 'negative' && positiveDirection === 'increasing')
      ) {
      direction = 'down-arrow'
    }

    return direction
  }

The test for if(changeType === 'no-change') { is redundant and not needed as it will fall through if the other tests fail and return null anyways.

You could also break the statement up returning the result as needed and falling through if they fail for null.

const getArrowDirection = (dir, type) => {
    if (type === "positive") {
        if (dir === "increasing") { return "up-arrow" }
        if (dir === "decreasing") { return "down-arrow" }
    }else if (type === "negative") {
        if (dir === "increasing") { return "down-arrow" }
        if (dir === "decreasing") { return "up-arrow" }
    }
    return null;
}

Assuming that you have given all possible values you can make assumptions and reduce the code further

const getArrowDirection = (dir, type) => {
    if (type !== 'no-change') {
        if (type === "positive") { return dir === "increasing" ? "up-arrow" : "down-arrow" }
        return dir === "increasing" ? "down-arrow" : "up-arrow";
    }
    return null; // returning null is not the best. Returning undefined would
                 // be better and would not need this line
}

You can use an object as a lookup using the combined strings.

const getArrowDirection = (() => {
    const directions = {
        positiveincreasing: "up-arrow",
        negativedecreasing: "up-arrow",
        positivedecreasing: "down-arrow",
        negativeincreasing: "down-arrow",
    };
    return (dir, type) => directions[type + dir] ? directions[type + dir] : null;
})();