Performing various Wireshark services I am building an application that opens Wireshark services (Wireshark has several services) in order to to different things on Wireshark files like edit, change format, statistics, etc. Each option usually uses a different service, so I want to build my classes with inheritance.

I was wondering if what I want to do is appropriate.

Main class WiresharkServices with the following members and methods:

public class WiresharkProcesses
{
    protected string _filePath; //the file path who send to Wiresahrk process
    protected string _capinfos;
    protected string _dumpcap;
    protected string _editcap;
    protected string _mergecap;
    protected string _rawshark;
    protected string _text2pcap;
    protected string _tshark;
    protected string _wireshark;

    public void initializeServices()
    {
        if (Directory.Exists(@"C:\Program Files (x86)\Wireshark"))
        {
            _capinfos = @"C:\Program Files (x86)\Wireshark\_capinfos.exe";
            _dumpcap = @"C:\Program Files (x86)\Wireshark\_dumpcap.exe";
            _editcap = @"C:\Program Files (x86)\Wireshark\editcap.exe";
            _mergecap = @"C:\Program Files (x86)\Wireshark\_mergecap.exe";
            _rawshark = @"C:\Program Files (x86)\Wireshark\_rawshark.exe";
            _text2pcap = @"C:\Program Files (x86)\Wireshark\_text2pcap.exe";
            _tshark = @"C:\Program Files (x86)\Wireshark\_tshark.exe";
            _wireshark = @"C:\Program Files (x86)\Wireshark\_wireshark.exe";
        }
        else if (Directory.Exists(@"C:\Program Files\Wireshark"))
        {
            _capinfos = @"C:\Program File)\Wireshark\_capinfos.exe";
            _dumpcap = @"C:\Program Files\Wireshark\_dumpcap.exe";
            _editcap = @"C:\Program Files\Wireshark\editcap.exe";
            _mergecap = @"C:\Program Files\Wireshark\_mergecap.exe";
            _rawshark = @"C:\Program Files\Wireshark\_rawshark.exe";
            _text2pcap = @"C:\Program Files\Wireshark\_text2pcap.exe";
            _tshark = @"C:\Program Files\Wireshark\_tshark.exe";
            _wireshark = @"C:\Program Files\Wireshark\_wireshark.exe";
        }
    }
}

When the application is running, I am of course checking if Wireshark is installed on the machine. If not, I throw an exception:

WiresharkServices wservices = new WiresharkServices();
wservices .initializeServices();

and in each class its own methods.

Child class example which receives a file path to convert it to another Wireshark format:

public class Editcap : WiresharkProcesses
{
    private string _newFileName;

    public void startProcess(string filePath)
    {
        FileInfo file = new FileInfo(filePath);
        _newFileName = file.FullName.Replace(file.Extension, "_new") + ".pcap";
        ProcessStartInfo editcapProcess = new ProcessStartInfo(string.Format("\"{0}\"", _editcap))
        {
            Arguments = string.Format("{2}{0}{2} -F libpcap {2}{1}{2}", file.FullName, _newFileName, "\""),
            WindowStyle = ProcessWindowStyle.Hidden,
            RedirectStandardOutput = true,
            RedirectStandardError = true,
            CreateNoWindow = true,
            UseShellExecute = false,
            ErrorDialog = false
        };

        using (Process editcap = Process.Start(editcapProcess))
        {
            editcap.WaitForExit();
        }
    }

    public string getNewFileName()
    {
        return _newFileName;
    }
}

The only thing you are getting out of this inheritance is which directory should be used to access the executable. And you are only using that to define a number of (what should be constant) strings. The fact that you have an error in your value for _capinfos in the seconds case should be an indication that this is not the best way to perform this action.

To accomplish the task of initializing the strings, I would instead do something like this.

public void initializeCommands(String path) {
    _capinfos = path + "_capinfos.exe";
    _dumpcap = path + "_dumpcap.exe";
    _editcap = path + "editcap.exe";
    _mergecap = path + "_mergecap.exe";
    _rawshark = path + "_rawshark.exe";
    _text2pcap = path + "_text2pcap.exe";
    _tshark = path + "_tshark.exe";
    _wireshark = path + "_wireshark.exe";
}

You can wrap this in something that determines the path or even support non-standard paths.

But what this really comes down to is that you are using inheritance just so you have easy access to some constants you might want to use. You are not defining any methods that would specify actual code used between multiple classes that execute Wireshark processes. If Editcap were passed in the path to the executable, it would no longer have a need to the parent class. If the coupling is this loose, inheritance is generally not the best idea.


Python process tracklist, get cumulative timestamp of each track The code below parses a tracklist (sample input below) and generates a six-member tuple, including cumulative timestamps in mm:ss. (sample output at bottom).

couldn't easily see how to rewrite it from a for-loop to a generator get_track_with_cumulative_timestamp(itt)
I think we don't need a custom object, i.e. avoid this approach
generator will need to internally store state (tmm, tss) being the end-time of each song. So we yield each result with the start-time of each song; incrementing the time-arithmetic comes after that.
are there better idioms for the (mm,ss) modulo-60 counters? decimal.Decimal seems overkill. I suppose we could always pull a trick internally using one float to represent both (mm,ss)

Current not-so-great procedural code:

tracklist = """
1. Waiting For A Miracle 5:02
2. Bedroom Eyes 5:01
3. In Over My Head 4:31
4. Higher Ground / Written-By – S. Wonder* 3:38
5. Hot Blood 4:15
6. Running Away 4:28
7. I've Had Enough 3:47
8. Blacklisted / Guitar [Other] – Jonny Fean* 4:11
9. Last Thing At Night 2:49"""

tracklist = [t for t in tracklist.split('\n') if t]

import re

pat = re.compile(r'(?P<no>[0-9]+)\. (?P<name>.*) (?P<mm>[0-9]+):(?P<ss>[0-9]+)')

(tmm,tss) = (0,0)
result = []

for t in tracklist:
    m = pat.match(t)
    #print(m.groups())
    lmm, lss = int(m['mm']), int(m['ss'])

    result.append((int(m['no']), tmm, tss, lmm, lss, m['name']))
    #yield (int(m['no']), tmm, tss, lmm, lss, m['name']))

    tss += lss
    tmm += lmm
    if tss >= 60:
        tmm += 1
        tss -= 60

# Output looks like this:
for _ in result: print('{} | {:02d}:{:02d} | {}:{:02d} | {}'.format(*_))

1 | 00:00 | 5:02 | Waiting For A Miracle
2 | 05:02 | 5:01 | Bedroom Eyes
3 | 10:03 | 4:31 | In Over My Head
4 | 14:34 | 3:38 | Higher Ground / Written-By – S. Wonder*
5 | 18:12 | 4:15 | Hot Blood
6 | 22:27 | 4:28 | Running Away
7 | 26:55 | 3:47 | I've Had Enough
8 | 30:42 | 4:11 | Blacklisted / Guitar [Other] – Jonny Fean*
9 | 34:53 | 2:49 | Last Thing At Night

Implicit tuples

(tmm,tss) = (0,0)

This shouldn't need any parens.

Generator

  couldn't easily see how to rewrite it from a for-loop to a generator

It actually is quite easy. Make your code into a function, delete result, and replace result.append with yield.

Time spans

  are there better idioms for the (mm,ss) modulo-60 counters?

Yes!

Custom objects

  I think we don't need a custom object

Named tuples take one line to declare, and suck less than unstructured data. So do that, at least.

Underscores

I just noticed that you're looping with an underscore as your loop variable. By convention this means "I'm not going to use this value"... but then you used it anyway. Give this variable a meaningful name.

Example

import re
from collections import namedtuple
from datetime import timedelta

TRACK_PAT = re.compile(r'(?P<no>[0-9]+)\. (?P<name>.*) (?P<mm>[0-9]+):(?P<ss>[0-9]+)$', re.M)
Track = namedtuple('Track', ('number', 'time', 'length', 'name'))

def parse_track(body):
    t_total = timedelta()
    for match in TRACK_PAT.finditer(body):
        length = timedelta(minutes=int(match['mm']), seconds=int(match['ss']))
        yield Track(int(match['no']), t_total, length, match['name'])
        t_total += length

for track in parse_track(
"""
1. Waiting For A Miracle 5:02
2. Bedroom Eyes 5:01
3. In Over My Head 4:31
4. Higher Ground / Written-By – S. Wonder* 3:38
5. Hot Blood 4:15
6. Running Away 4:28
7. I've Had Enough 3:47
8. Blacklisted / Guitar [Other] – Jonny Fean* 4:11
9. Last Thing At Night 2:49"""
):
    print(f'{track.number} | {track.time} | {track.length} | {track.name}')


Creating a generic function for objects of different classes How might I refactor this to get rid of all the if-else?

  def self.dispatches_for(object)
    scope = Dispatch.asc(:days)
    if object.class == Habit
      scope.where(:habit=>object).map{|d|d}
    elsif object.class == User
      scope.where(:coach=>object).map{|d|d}
    elsif object.class == Content
      scope.where(:content=>object).map{|d|d}
    end
  end

def self.dispatches_for(object)
  klass = object.class.to_s.downcase.to_sym
  raise "unacceptable class" unless klass.in? [:habit, :coach, :content]
  scope = Dispatch.asc(:days)
  scope.where(klass=>object).map{|d|d}
end


Test a number for primality and return smallest divisor different from 1 if it’s is composite In this exercise we were asked to test a number for primality and print the results. Additionally – in the case the number is composite – the smallest divisor different from 1 should be printed out. Negative input values shall not pre processed and result in a message.

The second part of that task to improve the performance of the test and wipe out as many redundant and unnecessary comparisons as possible. We were not allowed to use any built-in methods to check for primality.

I have concerns mainly about properly handling input I’m rejecting and naming things (talking about result here).

/**
 * Tests a positive integer for primality.
 * Prints out whether a number is prime, composite or neither.
 */
public class PrimalityTest {
    public static void main(String[] args) {
        // We only test one number, so no other arguments are allowed.
        if (args.length > 1) {
            System.out.println("Too many arguments. Please enter a positive integer prime canidate.");
            System.exit(1);
        }

        int primeCandidate = 0;
        try {
            primeCandidate = Integer.parseInt(args[0]);
        } catch (NumberFormatException e) {
            System.out.println("The entered value was not an integer.");
            System.exit(1);
        }

        if (primeCandidate < 0) {
            System.out.println("Negative numbers are never prime.");
            System.exit(1);
        }

        int result = smallestDivisor(primeCandidate);
        if (result == -1) {
            System.out.println(primeCandidate + " is prime.");
        } else if (result == 1 || result == 0) {
            System.out.println(primeCandidate + " is neither prime nor composite.");
        } else {
            System.out.println(primeCandidate + " is composite and has the smallest divisor " + result + ".");
        }
    }

    /**
     * Returns the smallest divisor of composite numbers or
     * 1 if the number is 1 (not composite, not prime) or
     * -1 if the number is prime (not composite).
     */
    public static int smallestDivisor(int primeCandidate) {
        if (primeCandidate == 0 || primeCandidate == 1) {
            return primeCandidate;
        } else if (primeCandidate <= 3) {
            // 2 and 3 are prime.
            return -1;
        } else if (primeCandidate % 2 == 0) {
            // All numbers greater 2 and divisable by 2 are not prime and the smallest divisor is 2
            return 2;
        } else if (primeCandidate % 3 == 0) {
            // All numbers greater 3 and divisable by 3 are not prime and the smallest divisor is 3
            return 3;
        } else {
            // Checks divisability of numbers in the form of 6k+1 and 6k-1.
            // It starts with: 5 and 7, 11 and 13, 17 and 19, 23 and 25, ...
            for (int i = 5; i * i <= primeCandidate; i += 6) {
                if (primeCandidate % i == 0) {
                    return i;
                } else if (primeCandidate % (i + 2) == 0) {
                    return i + 2;
                }
            }
            // No divisor found, the number is prime.
            return -1;
        }
    }
}

Single responsibility principle

You method smallestDivisor() violates the SRP because it does too many things.

It is

calculating the smallest divisor
calculating primes
checking if the number is neither composite nor prime

You should consider to just calculate what the name of the method tells you.

Method names should be made out of a verb or a verb phrase, so getSmallestDivisor() would be better.

So how to handle the edge cases ? That is easy, add methods to evaluate them.

private static boolean isPrime(int number, int smallestDivisor){
    return number == smallestDivisor;
}

private static boolean isComposite(int number, int smallestDivisor){
   return number != smallestDivisor && smallestDivisor > 1;
}

The getSmallestDivisor() method should then return as shown below

public static int smallestDivisor(int primeCandidate) {

    if (primeCandidate =< 1) {
        return primeCandidate;
    }
    if (primeCandidate % 2 == 0) {
        return 2;
    }
    if (primeCandidate % 3 == 0) {
        return 3;
    }

    // Checks divisability of numbers in the form of 6k+1 and 6k-1.
    // It starts with: 5 and 7, 11 and 13, 17 and 19, 23 and 25, ...
    for (int i = 5; i <= sqrt(primeCandidate); i += 6) {
        if (primeCandidate % i == 0) {
            return i;
        } else if (primeCandidate % (i + 2) == 0) {
            return i + 2;
        }
    }

    return primeCandidate;
}

to be tested like:

    int smallestDivisor = getSmallestDivisor(primeCandidate);
    if (isPrime(primeCandidate, smallestDivisor)) {
        System.out.println(primeCandidate + " is prime.");
    } else if (isComposite(primeCandidate, smallestDivisor)) {
        System.out.println(primeCandidate + " is composite and has the smallest divisor " + smallestDivisor + ".");
    } else {
        System.out.println(primeCandidate + " is neither prime nor composite.");
    }

Input

you haven't checked an edge case: that a user might not pass any argument into the application.
you should extract the reading of the argument to a separate method and maybe add a possibility, if the given input doesn't match the requirements, to enter the value again. So e.g no arguments provided, value < 0 or not an integer.


SSE Assembly vs GCC Compiler - Dot Product I am currently taking an introductory course in computer architecture.
Our goal was to write a dot-product function in x86 Assembly which would use SSE and SIMD (without AVX).

I am not to that confident with my solution:

#include <time.h>
#define NUM 25600
//Example: scalarProduct using SIMD
extern float scalarProduct(float *, float *, int size);

float vsC(float * a, float * b, int size){
    float sum = 0;
    for(int i = 0; i < size; i++){
        sum += b[i]*a[i];
    }
    return sum;
}
int main(int argc, char ** argv){
    float * a = malloc(NUM * sizeof(double));
    float * b = malloc(NUM * sizeof(double));
    for(int i = 0; i < NUM; i++){
        a[i] = 1;
        b[i] = 1.0/(i+1);
    }
    clock_t start, end;
        double cpu_time_used;

         start = clock();
     float sum = scalarProduct(a,b,NUM);
         end = clock();
     cpu_time_used = ((double) (end - start))/CLOCKS_PER_SEC;
     printf("%.15f\n", cpu_time_used);
     printf("Solution %.15f\n", (double)(sum));
     start = clock();
     sum = vsC(a,b,NUM);
     end = clock();
     cpu_time_used = ((double) (end - start))/CLOCKS_PER_SEC;
     printf("%.15f\n", cpu_time_used);
     printf("Solution %.15f\n", (double)(sum));
}

Assembly File

.intel_syntax noprefix
.text
.global scalarProduct

scalarProduct:
        mov rax, rdx
        xorps xmm0, xmm0
        mov rcx, 0
start:
        cmp rax, 4
        jl end
        movdqu xmm3, [rsi + rcx]
        movdqu xmm2, [rdi + rcx]
        vmulps xmm1, xmm2, xmm3
        haddps  xmm7, xmm1
        haddps xmm7, xmm7
        psrldq xmm7, 4 //Shift to pos0
        addss xmm0, xmm7
        xorps xmm7, xmm7
        sub rax, 4
        add rcx, 16
        jmp start
end:
        cmp rax, 0
        je ret
        dec rax
        movss xmm1, [rsi + rcx]
        movss xmm2, [rdi + rcx]
        mulss xmm1, xmm2
        addss xmm0, xmm1
        add rcx, 4
        jmp end
ret:
        ret

Obviously, this Assembly - Code is far from perfect. How could I do better using basic SIMD and SSE?

The second thing which made me wonder is, I indeed outperformed GCC on a Xeon processor, which is irritating.

Compiling the code with:

gcc -o main -O7 main.c scalarProduct.S

Shows the following result:

./main
0.000015000000000
Solution 10.727574348449707
0.000026000000000
Solution 10.727569580078125

How would I have to improve my C Code so that GCC can step up?

DISCLAIMER:

My homework does not affect my grades and its editing is optional.

While harold reviewed your assembly code, I'll just comment on how you are compiling your C code:

Increase the duration of the benchmark

Your code runs for a very short amount of time, only tens of microseconds. This is too short to get accurate measurements:

clock() only has a resolution of a microsecond, so this is not insignificant compared to the duration.
the processor's caches, branch predictors and prefetchers might not have had time to fully warm up.
interrupts and other processes running on your system add noise.

Aim to run your benchmarks for at least a second. That might be easier said than done: if you increase the size of the arrays you might become memory bandwidth bound instead of CPU bound. And if you naively just repeat the computations with the small array, the compiler might notice and just calculate the sum once and the multiply it by the number of repeats, depending on the level of optimization used.

Use a proper optimization level

There is no such thing as -O7. The highest supported optimization level for GCC is -O3. However, even that does not enable all possible optimzations. In particular, GCC is a bit careful when it comes to floating point math, and tries to ensure the code is correct even if there are infinities, NaNs and denormals. It also knows that floating point math is not strictly associative and commutative, and so will try to keep operations in the same order as you specified them, which prevents it from using some vectorization tricks. If you don't care about that, you can enable -ffast-math, or use -Ofast.

Consider using -mtune=... and/or -march=...

If you don't specify any specific CPU, then on an x86-64 platform, GCC will output code that can run on any 64-bit Intel or AMD CPU, and might not be able to use certain SSE instructions that are not available in the x86-64 baseline. Also, the compiler will assume a certain CPU for instruction timing, delay slots, and other micro-architecural optimizations, which might not be ideal for the Xeon CPU you are running the code on. Typically you would use -march=native to ensure the compiler will provide code using all features of the CPU you are compiling on, but that might cause it to use AVX instructions if your CPU supports those.

Consider using SSE intrinsics

Instead of having a pure assembly version and a pure C version, you can have something inbetween by using SSE intrinsics. These are functions that are compiled into specific CPU instructions. However, the function arguments and return values are just variables (either regular ones or special vector type variables), not registers. The compiler will pick registers as it sees fit, and will also be able to reorder the intrinsics calls if possible, using its knowledge about the CPU's micro-architecture, and if you use the intrinsics in a loop then the compiler can unroll the loop for you.

While GCC and Clang are able to vectorize certain loops, they are quite bad at using horizontal operations such as haddps, so if you help it by using intrinsics you might get code comparable to the best hand-optimized assembly.

Some results

If I increase the size of the array to 25600000, and compile with -O7 (which will effectively be -O3), I get the following results on an AMD Ryzen 9 3900X:

0.008444000000000
Solution 16.000000000000000
0.018092000000000
Solution 15.403682708740234

With -Ofast I get:

0.008399000000000
Solution 16.000000000000000
0.006617000000000
Solution 16.419670104980469

So clearly there is a speed-up when going to -Ofast, but the resulting solution is also different.


Create a new object that excludes specified keys I have 2 data structures with me in JS. One is an array say of length N. Let's consider N = 4.

const arr = ['a', 'b', 'c', 'd'];

The second one is an object:

const obj = {
  a: 0,
  b: 2,
  c: 3,
  z: 5
};

I wrote a code in Javascript, to create a new object out of obj which should not contain the keys which are not present in the array arr. As of now, the time complexity of the code is \$O(n*n)\$ i.e., \$O(4*4)\$.

standardizeFilter(queryParams: any, inputType: Array<string>) {
  const filter = {};

  for (const key of Object.keys(queryParams)) {
    if (inputType.indexOf(key) > -1) {
      filter[key] = this.parseValues(queryParams[key]);
    }
  }
  return filter;
}

I'm wondering if there is a much optimal way to convert this \$O(n²)\$ to \$O(n)\$.

From a short review;

Naming

standardizeFilter is too specific/specialized, perhaps standardizeObject?
queryParams also seems a very specific name, whereas the functionality is very general, I would just call it keys
inputType is not a type, it's an object, I would propose source

For each key you perform both a indexOf and a [] access, you can drop the indexOf

I would propose something like this:

function standardizeObject(source, keys){

  const o = {};

  for(const key of keys){
    const value = source[key];
    if(value !== undefined){
      o[key] =  this.parseValues(value);
    }
  }

  return o;
}


Smart but simple pointers For practice, I've implemented my own smart pointer class. It's a 'unique pointer', meaning it doesn't allow copies of itself (when copying from a into b, a is 'emptied' after the copying, i.e. the raw pointer is set to 0), so there can never be more than one time where delete is called on the raw pointer.

It utilizes RAII to bound the memory deallocation to the object lifetime, so it is just a basic smart pointer.

#ifndef SMARTPOINTER_CPP
#define SMARTPOINTER_CPP
#include <iostream>

template <typename T>
class SmartPointer {
public:
    SmartPointer(T* pointer = NULL) : pointer(pointer) { }
    ~SmartPointer(){
        delete pointer;
    }
    SmartPointer(SmartPointer& other){
        *this = other;
    }
    SmartPointer& operator=(SmartPointer& other){
        this->pointer = other.pointer;
        other.pointer = NULL;

        return *this;
    }
    T& operator*() const {
        return *pointer;
    }
    T* operator->() const {
        return pointer;
    }
private:
    T* pointer;
    template <typename E>
    friend std::ostream& operator<<
        (std::ostream& stream, const SmartPointer<E>& smartPointer);
};
template <typename E>
std::ostream& operator<<(std::ostream& stream, const SmartPointer<E>& smartPointer){
    stream << smartPointer.pointer;
}
#endif

This is not unique_ptr, it is auto_ptr, which has problems, mainly that copying a value modifies it, which is counter intuitive.

Assuming you really do not have move semantics and cannot do better (you should, C++11 is old now), there are still some problems:

There is no way to release a pointer. If I am not happy with your SmartPointer I want to do something like SmartPointer<T> sp; shared_ptr<T> ssp = sp.release();.
You forgot to return the stream inside ostream& operator <<. This should be a compilation error.
There is no need for friend. I don't know who made it popular to make the ostream& operator << a friend but I'd like to give that person a stern look. Without friend this works just fine:

template <typename E>
std::ostream& operator<<(std::ostream& stream, const SmartPointer<E>& smartPointer){
    return stream << &(*smartPointer);
}

Your copy-constructor is wrong. It copies the pointer eventually resulting in a double delete. You probably meant to have other.pointer = nullptr;.
Consider making the assignment operator return void. You are supposed to return *this to allow chaining like in a = b = c = nullptr;. However, since the assignment takes the value away there is no point chaining assignments, so better cause a compiler error.
Your assignment operator causes a memory leak in the following case:

SmartPointer<int> sp1 = new int{1};
SmartPointer<int> sp2 = new int{2};
sp1 = sp2;

The memory for the first allocation is never deleted.

Edit: There is a different memory leak case:

SmartPointer<int> sp = new int;
sp = sp;

I would suggest nullptr instead of NULL, but I guess your compiler does not know what that is.


Generating all combinations of an array I'm generating all combinations of an array, so for instance, ["a", "b", "c", "d"] will generate:

[
  "a",    "b",    "ab",   "c",    "ac",
  "bc",   "abc",  "d",    "ad",   "bd",
  "abd",  "cd",   "acd",  "bcd",  "abcd"
]

Here's the code I've written that does complete this task.

What I'd like to know is if there is a better way, as iterating over the array twice feels like I'm cheating, or the complexity of the code is much more computationally expensive than it needs to be.

Also, the name for a function that takes an array and returns the combinations, what might that be called? Combinator seems inappropriate.

var letters = ["a", "b", "c", "d"];
var combi = [];
var temp= "";
var letLen = Math.pow(2, letters.length);

for (var i = 0; i < letLen ; i++){
    temp= "";
    for (var j=0;j<letters.length;j++) {
        if ((i & Math.pow(2,j))){
            temp += letters[j]
        }
    }
    if (temp !== "") {
        combi.push(temp);
    }
}

console.log(combi.join("\n"));

A recursive solution, originally seen here, but modified to fit your requirements (and look a little more JavaScript-y):

function combinations(str) {
    var fn = function(active, rest, a) {
        if (!active && !rest)
            return;
        if (!rest) {
            a.push(active);
        } else {
            fn(active + rest[0], rest.slice(1), a);
            fn(active, rest.slice(1), a);
        }
        return a;
    }
    return fn("", str, []);
}

Test:

combinations("abcd")

Output:

["abcd", "abc", "abd", "ab", "acd", "ac", "ad", "a", "bcd", "bc", "bd", "b", "cd", "c", "d"]

Regarding the name: Don't name it permutations; a permutation is an arrangement of all the original elements (of which there should be n! total). In other words, it already has a precise meaning; don't unnecessarily overload it. Why not simply name it combinations?


Dispatching Vuex getter via setInterval I am fetching the friend list for a chat application using a Vuex store. I need to dispatch the getter via an interval. Is there a best practices way to do this?

<template>

Who's Online

            <UserListItem v-for="user in friendList" :user="user" :key="user.user_id" />

</template>

Responding to your question

  Is there a best practices way to [dispatch the getter via an interval]?

You could consider using requestAnimationFrame() (read more about using that with setInterval() in articles like this one) but that might be overkill since you are just updating a list.

Another option to consider is using websockets (presuming your server-side would support it). That way the client-side could be notified of updates to the list instead of having to request them periodically.

Bug in the sample code

The sample code shows these methods:

    created() {
        this.fetchFriendList()
        setInterval(this.fetchFriendList, 3000)
    },
    destroy() {
        clearInterval(this.fetchFriendList)
    },

However, clearInterval() takes an intervalID as a parameter, not a function reference.

  Parameters

  intervalID
  The identifier of the repeated action you want to cancel. This ID was returned by the corresponding call to setInterval().1

So instead of passing the method reference this.fetchFriendList, pass an identifier returned from the call to setInterval() in the created method.

1https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/clearInterval#Parameters


Trig based functions in sfml, angling of sprites void moveCircle(float dt, int radius) { //so center of screen will be rotation point
        setPosition(400+cos(_angle)*radius, 400 - sin(_angle)*radius);

        _angle += (2*pi)*dt;

        if (_angle >= (2 * pi)) {
            _angle -= (2 * pi);
        }
        this->rotate(-2*dt*180);

    }

sf::Vector2f rotateCoordinate(sf::Vector2f mouseCoords, float angle) {
            return sf::Vector2f((mouseCoords.x*cos(angle) + mouseCoords.y*sin(angle)), (mouseCoords.y*cos(angle) - mouseCoords.x*sin(angle)));
    }

void angleShipToMouse(sf::RenderWindow* window) {

        if (sf::Mouse::isButtonPressed(sf::Mouse::Button::Left)) {

            sf::Vector2f mousePosition(sf::Mouse::getPosition(*window));
            std::cout << mousePosition.x << ", " << mousePosition.y << std::endl;

            if (mousePosition.x > _windowWidth || mousePosition.x < 0 || mousePosition.y<0 || mousePosition.y>_windowHeight) {
                return;
            }

            //position of mouse relative to ship in 0 rotation reference frame(absolute)
            sf::Vector2f mouseRelativeShip(mousePosition.x - getX(), (mousePosition.y - getY()));

            //adjusting coordinates to any rotation the sprite has to account for change in orientation of axis.
            mouseRelativeShip = rotateCoordinate(mouseRelativeShip, _angle*(pi/180));

            float angle = atan2(mouseRelativeShip.y, mouseRelativeShip.x) *(180 / pi); //atan2 produces negative angles QUADS 1&2, positive in QUADS 3&4
            this->rotate(angle);

            _angle = this->getRotation();
        }
    }

So these are different fns obviously the first one is unrelated really to the other 2 - just want your opinion on how its done since i didnt look at any examples of others before doing it. I'm wondering if my angleShipToMouse is good code because I honestly got really confused on whether I wanted the local coordinates of the object to show y+ as being "above" the sprite or to just keep it as y- and how this would affect the rotation, in fact it took a long time of just tinkering to  get it to work and I'm unsure specifically why this setup works with y- pointing up. I understand trig but the whole axis issue and the fact that rotation is measured clockwise here when it's normally anti-clockwise, really messed up my brain, lol.

The circle one just moves in a circle with the space ship angled in a way that looks like its travelling around it.

Single Responsibility
Overall, I see one really big thing that I think needs to be addressed before anything else can be addressed. Namely, your class is doing way too many things. A class should have one single purpose. It should avoid relying on other classes as much as reasonably possible. This is known as loose coupling. A class that figures out positions and angles of a ship should not have to know anything about windows or mouse buttons. As such, I'd change the prototype for void angleShipToMouse to something like this:
void angleShipToMouse(const sf::Vector2f& mousePosition)

I'd then remove the logic that gets the mouse button state and checks whether a button is pressed and whether the position is within the window bounds and put those somewhere else in your program.
Do as few transformations of data as necessary
Several places in your code you're converting between degrees and radians. There are several problems with doing this:

It becomes difficult to figure out or remember which units a variable is in, particularly if it's named something generic like angle or rotation.
It's inefficient in both the space the program takes up and the time it takes to execute. It might not matter here, but maybe it will later.
It can introduce inaccuracies. Again, they may not be noticeable in this code, but they could be multiplied later.

In general, all code other than code which displays values to users is probably going to want to operate in radians. All math functions in the standard library use radians. You should probably be using radians internally and only converting to degrees for display.
Also, when you do want to convert between degrees and radians, you should write a named function to do it and call that function rather than always writing the values inline. It's easy to mistype one of the values and someone not familiar with the transformation might not immediately understand it looking at the code. If you had functions named degreesToRadians() and radiansToDegrees(), it becomes immediately obvious to anyone reading the code what you're doing.
Avoid magic numbers
In moveCircle() you have the value 400 twice in the first line. What does it represent? I'm guessing that the window or view this is in is 800x800 pixels, so (400, 400) is the center? If so, you should at least have a constant defined for that. I recommend something like:
const sf::Vector2f kWindowCenter(400, 400);

Or better yet, pass in the point you want to rotate around! What happens if you want to let the user resize the window (or view)? What if you want to rotate around a different point?


CodeIgniter AJAX messages submission security issue I have a small social networking site built in CodeIgniter. Any registered user can send messages to others by visiting their profile.

Today I noticed that one user sent bulk messages to 200 users. How he was able to do that?

Suggestions to make the code secure are welcome.

I have a textarea and a send button on the profile page.

jQuery code on profile page (View)

$("#send").click(function(event){
    var msg=$("#quick_message").val();
    var uid=$(this).attr('uid');

    if(msg.length > 0) {
        $("#msg_status").html('');

        $.post("<?=base_url()?>message/send_message", {"ids":uid,"msg":msg}, function(data){

        $("#msg_status").html('Message sent.');
        $("#quick_message").val('');
        });
    } else {

    $("#msg_status").html('Write something to send message.');
    }
});

Here is my controller

  // send message
   function send_message()
 {
    if (!$this->users->is_logged_in()) {
    redirect('signin');
    }

    $user_id=$this->session->userdata('user_id');
    $ids=trim($this->input->post('ids'));
    $msg=trim($this->input->post('msg'));
    $msg=htmlspecialchars($msg);
    $msg=$this->replaceTolink($msg);
    $msg=$this->replaceTowinks($msg);

    $pieces=explode(",", $ids);
    foreach ($pieces as &$user_id2) {
    $this->db->insert('messages', array('user_id1' => $user_id,'user_id2' => $user_id2,'message' => $msg));
    }

    return true;
}

What I need to improve in my code and how to protect the code to send bulk messages?

First of all you should use site_url() in your jquery code instead of base_url() as it provides url flexibility. For more info visit http://codeigniter.com/user_guide/helpers/url_helper.html

And in your controller code i suggest you to use bulk insert instead of loop insert. So you may use this piece of code:

foreach( $pieces as $user_id2)
{
    $insert_arr[] = array('user_id1'=> $user_id, 'user_id2'=> $user_id2, 'message'=>$msg);
}

$this->db->insert_batch('messages', $insert_arr);


Expanding tilde inside assignments and strings in bash Inspired by the work Charles Duffy did in this answer and the work of Jonathan Leffler in this one and because I couldn't leave well-enough alone.

I went and wrote expandTilde.sh:

#!/bin/bash

doExpand() {
  local path
  local -a resultPathElements

  for path in "$@"; do
    : "$path"
    case $path in
      "~+")
        path=$PWD
        ;;
      "~+"/*)
        path=$PWD/${path#"~+/"}
        ;;
      "~-")
        path=$OLDPWD
        ;;
      "~-"/*)
        path=$OLDPWD/${path#"~-/"}
        ;;
      "~")
        path=${HOME-~}
        ;;
      "~"/*)
        path=$HOME/${path#"~/"}
        ;;
      "~"[0-9]|"~"[+-][0-9])
        local num=${path#"~"}
        local op=${num%%[0-9]*}
        num=${num#[+-]}
        local opath=$path
        if [ "$op" = "-" ]; then
          ((num+=1))
        fi
        path=${DIRSTACK[@]: $op$num:1}
        : "${path:=$opath}"
        ;;
      "~"*)
        local username=${path%%/*}
        username=${username#"~"}
        IFS=: read -r _ _ _ _ _ homedir _ < <(getent passwd "$username")
        if [ "$homedir" ]; then
            if [[ $path = */* ]]; then
              path=${homedir}/${path#*/}
            else
              path=$homedir
            fi
        fi
        ;;
    esac
    resultPathElements+=( "$path" )
  done
  local result
  printf -v result '%s:' "${resultPathElements[@]}"
  printf '%s\n' "${result%:}"
}

expandAssign() {
  local -a pathElements
  IFS=: read -r -a pathElements <<<"$1"
  : "${pathElements[@]}"
  doExpand "${pathElements[@]}"
}

expandString() {
    doExpand "$1"
}

So two questions:

Did I miss any cases or get any cases wrong? (Tests in the github repo.)
Can this be improved in any meaningful ways?

case is most useful when its order of expansion is considered. if you construct your conditions in such a way that a previous match will narrow the possibilities for the current match, then you can get much nearer much quicker to your target match. what's more, case seems always to make better sense as a function in itself - so that it might call itself recursively when matches are ambiguous. consider the following:

   pathcase()
        case    $1 in  ([!~]*|'') ! :;;       ### ^tilde or else
        (*/*)
            pathcase "${1%%/*}" "${1#*/}"     ### squeeze to relevant ele
        ;;
        (?)
            path=${HOME-~}${2+/$2}            ### let expansions work, to
        ;;
        (?-)
            path=${OLDPWD:+$OLDPWD${2+/$2}}   ### its not always set
        ;;
        (?+)
            path=$PWD${2+/$2}                 ### it should be
        ;;
        (?*[!-+0-9]*|??*[-+]*)                ### negate
            pathchk -p -- "${1#?}" &&         ### requires standards comp
            eval '[ "$1" != '"$1 ] &&         ### which makes this safe
                  path=$1${2+/\$2}"           ### and this
        ;;
        (${BASH_VERSION+$(( path  =
         ${#DIRSTACK[@]}-${1#?+}-1))}|\
         ${BASH_VERSION+${path##-*}}*)        ### matches !bash||bad inde
            unset path
           ${path?bad tilde: "$1${2+/$2}"}    ### break w/ error
        ;;
        (?+*)
            path=${DIRSTACK[${1#?+}]}${2+/$2}
        ;;
        (*) path=${DIRSTACK[path]}${2+/$2}
        esac


Class representing both cartesian and spherical coordinates I have had little experience with OO programming, being a typical engineer I tend to hack together functional code. However, I'm trying to change that and I see value in implementing OO when modelling the physical world. I've written my first ever class for a position vector. I want an instance of the class to be able to store both the cartesian [x, y, z] and polar [z, theta, phi] coordinates of a position. Updating one attribute automatically updates the others. This class will be later be reused in other classes via composition.

I want an instance of my class to do the following:

>> p = PositionVector([15,5,5])
>> p.cartesian
[15,5,5]
>> p.polar
[16.583123951777, 0.0, 1.2645189576252271]
>> p.x = 3
>> p.cartesian
[3,5,5]
>> p.polar
[7.681145747868608, 0.7853981633974483, 0.8619682853367363]
>> p.theta = 0.5
>> p.cartesian
[5.117141702137862, 2.7955072527614058, 5.0]
>> p.polar
[7.681145747868608, 0.5, 0.8619682853367363]

The class I've written so far is below and while it works as expected, my limited understanding of Python classes and the property function tells me there's a better way to do this. I don't know if the methods for x y z r theta and phi need to be fully written since they are simply elements within each list/array but can't figure out how to get around this. I also don't know if this is the safest and most pythonic way of structuring a class object:

class PositionVector(object):
  def __init__(self, vector, system='cartesian'):
    if system == 'cartesian':
      self.cartesian = vector
      self.polar = self.position_vector_polar(self.cartesian)
    if system == 'polar':
      self.polar = vector
      self.cartesian = self.position_vector_cartesian(self.polar)

  @property
  def cartesian(self):
    return self._cartesian
  @cartesian.setter
  def cartesian(self, value):
    self._cartesian = value
    self._polar = self.position_vector_polar(self._cartesian)
    self._x, self._y, self._z = self._cartesian
    self._r, self._theta, self._phi = self._polar

  @property
  def polar(self):
    return self._polar
  @polar.setter
  def polar(self, value):
    self._polar = value
    self._cartesian  = self.position_vector_cartesian(self._polar)
    self._x, self._y, self._z = self.cartesian
    self._r, self._theta, self._phi = self.polar

  @property
  def x(self):
    return self._x
  @x.setter
  def x(self, value):
    self._x = value
    self._cartesian[0]  = self._x
    self._polar = self.position_vector_polar(self._cartesian)

  # repeat above for y and z variables

  @property
  def r(self):
    return self._r
  @r.setter
  def r(self, value):
    self._r = value
    self._polar[0]  = self._r
    self._cartesian = self.position_vector_cartesian(self._polar)

  # repeat above for thets and phi variables

  def position_vector_cartesian(self, polar):
    r, theta, phi = polar
    x = r * math.cos(theta) * math.sin(phi)
    y = r * math.sin(theta) * math.sin(phi)
    z = r * math.cos(phi)
    return [x, y, z]

  def position_vector_polar(self, cartesian):
    x, y, z = cartesian
    r = math.sqrt(x ** 2 + y ** 2 + z ** 2)
    theta = math.atan(y/x)
    phi = math.acos(z/r)
    return [r, theta, phi]

Note that the lists will be be replaced with NumPy arrays so I can perform algebra on the vectors, but for now I just want to get the class structure correct.

As far as I understand the code, a PositionVector object holds both cartesian and polar coordinates. This is surely redundant: multiple representations of the same position hold no more information than a single one (note that there are many more coordinate systems, and you cannot possibly account for all of them). I recommend to keep the position in a preferred coordinate system, and provide methods to convert it to and from other representations.

Edit: addressing concerns in the comment:

  What about if I want to set a new radius, r? What would be the most
  appropriate way to update [x, y, z]?

This is a very important question I didn't elaborate on in the answer. In the algebraic world we want to reason in terms of objects, as opposed to coordinates. Coordinates are secondary - they are just instrumental for calculations.

In other words, you do not want to set new radius. You want to apply a transformation (translate, rotate, scale, etc).

  I wrote the class with the
  ability to apply vector algebra in mind, if I follow your suggestion,
  would vector algebra be straight forward, regardless of which
  ordinates I am working with?

I seriously hope so.


C++ Stack using template I'm learning C++, so I wrote a stack with the help of templates. I'm wondering if there is anything I can improve or if I did something vastly wrong.

Stack.h

#pragma once
#include <ostream>

template <class Type>
struct Node {
    Node(Type data, Node<Type>* next)
        : data(data), next(next) {}
    Node* next;
    Type data;
};

template <class Type>
class Stack
{
public:

    Stack() : length(0), topNode(NULL) {
    }

    ~Stack() {
        while (!isEmpty()) {
            pop();
        }
    }

    void push(Type data) {
        Node<Type>* newNode = new Node<Type>(data, topNode);
        topNode = newNode;
        ++length;
    }

    Type pop() {
        if (!isEmpty()) {
            Node<Type>* popped = topNode;
            Type poppedData = popped->data;
            topNode = popped->next;
            --length;
            delete popped;
            return poppedData;
        }

        throw new std::exception("Stack underflow!");
    }

    bool isEmpty() {
        return length == 0;
    }

    void print() const {
        Node<Type>* tempTop = topNode;
        while (tempTop != NULL) {
            std::cout << tempTop->data << endl;
            tempTop = tempTop->next;
        }
    }

    int count() const {
        return length;
    }

private:
    Node<Type>*         topNode;
    int                 length;

};

Main.cpp

#include <iostream>
#include "Stack.h"

using namespace std;

int main() {
    Stack<int> myStack;

    // Push some values
    myStack.push(2);
    myStack.push(4);
    myStack.push(8);
    myStack.push(16);
    myStack.push(32);

    // We pop the 32
    myStack.pop();

    // Display count after the pop
    int lastPopped = myStack.pop();
    cout << "Popped value: " << lastPopped << ", Count: " << myStack.count() << endl;

    // Print whole stack
    cout << endl << "Stack print: " << endl;
    myStack.print();

    // Exit program when the 'any' key is pressed.
    system("PAUSE");
    return 0;
}

If you provide a destructor you should handle copying and assigning. In other words follow the Rule of 3 (or 5 if you care about move semantics). You can also disallow them but then there should be move constructor and assignment.

Otherwise you will get in trouble with double freeing when calling a function void foo(Stack<int> s).

empty() doesn't change the stack. So it should also be const.

Consider adding a peek() method that returns a reference to the top value of the stack (with const and non-const version).

T& peek()
{
    return topNode->data;
}

const T& peek() const
{
    return topNode->data;
}

The reason for using the reference is to avoid a unnecessary copy.

Having a print method interacting with the console is not a good idea. What if you want to show the contents in a gui instead?

Instead add iterators to be able to inspect what is on the stack without having to capture the output that print() generates.


Intersection of subset between two lists of dicts I want to find the intersection of two lists of dicts.

Note that equality—i.e.: what is to appear in the intersection—is given by which keys they must have equal. See the obj_equal_on function.

def obj_equal_on(obj0, obj1, keys):
    for key in keys:
        if obj0.get(key, False) != obj1.get(key, None):
            return False
    return True

def l_of_d_intersection(ld0, ld1, keys):
    """ Find intersection between two lists of dicts.
          At best, will return `ld0` in full. At worst: [].

    :param ld0 :type [DictType]
    :param ld1 :type [DictType]
    :param keys :type ListType

    :returns intersection of ld0 and ld1 where key is equal.
    """

    return [obj1 for obj0 in ld1 for obj1 in ld0
            if obj_equal_on(obj0, obj1, keys)]

Basic testing (self-contained for SE, usually I'd subclass unittest.TestCase):

def run_example(ld0, ld1, keys, expected_result):
    result = tuple(l_of_d_intersection(ld0, ld1, keys))
    assert result == expected_result, '{0} != {1}'.format(result,expected_result)

def main():
    run_example(
        ld0=[{'foo': 'bar', 'haz': 'more'},
             {'can': 'haz', 'more': 'haz'},
             {'foo': 'jar', 'more': 'fish'}],
        ld1=[{'foo': 'bar'},
             {'can': 'haz'},
             {'foo': 'foo'}],
        keys=('foo',),
        expected_result=({'foo': 'bar', 'haz': 'more'},)
    )

    run_example(
        ld0=[
            {'orange': 'black', 'blue': 'green', 'yellow': 'red'},
            {'blue': 'yellow'},
            {'orange': 'red', 'yellow': 'blue'}
        ],
        ld1=[
            {'orange': 'black', 'yellow': 'red'},
            {'blue': 'yellow'},
            {'orange': 'red', 'yellow': 'blue'}
        ],
        keys=('orange', 'yellow'),
        expected_result=({'orange': 'black', 'blue': 'green', 'yellow': 'red'},
                         {'orange': 'red', 'yellow': 'blue'})
    )

My example works, but my code is rather horribly inefficient.

Although a list comprehension is generally an efficient way to go about things, in this case it means you check every obj0 against every obj1, even if you've already found a match. To avoid that:

out = []
for obj0 in ld0:
    for obj1 in ld1:
        if obj_equal_on(obj0, obj1, keys):
            out.append(obj0)
            break
return out

Beyond that, you could consider using a custom immutable class rather than dictionaries, implementing __eq__ and __hash__ and using sets rather than lists to efficiently determine the intersection.


Admin CSS sidebar template Could I get some feedback on my simple start to an admin template:
http://jsfiddle.net/spadez/GHKxW/3/

HTML

Dashboard

Title

Content

CSS

* {
    margin: 0;
    padding: 0;
    border: 0;
}

li {
    list-style: none;
}
a {
    text-decoration: none
}

html, body {height: 100%; max-height: 100%; padding:0; margin:0;}
.wrapper {display:table; width:100%; height:100%;}
.wrapper > div {display:table-cell; vertical-align: top;}
.wrapper .left {width:200px; background-color:#34495E}

#btn_pri {
    background-color: 60A0DF;
    color: white;
}
#btn_sec { background-color: #e74c3c;
color:white;
}

.navigation {
}
.navigation li {
}
.navigation li span {
    display: inline-block;
    background: url(http://www.otlayi.com/web_images/content/free-doc-type-sprite-icons.jpg) no-repeat left center;
    width: 16px;
    height: 16px;
    overflow: hidden;
    margin-left: 15px;
    margin-right: 15px;
}
.navigation li a {
    display: block;
    color: white;
    padding-top: 15px;
    padding-bottom: 15px;
    font-family:'Helvetica Neue', Helvetica, Arial, sans-serif;
    font-size: 14px;
}
.navigation li a:hover {
    background-color: #495C6D;
}
.navigation li a.active {
    background-color: #495C6D
}
#rss span {
    background-position: -52px -68px;
}
#photos span {
    background-position: -90px -66px;
}
#links span {
    background-position: -45px 0;
}

h1 {font-family: 'Helvetica Neue', Helvetica, Arial, sans-serif;
    font-size: 30px; font-weight: 500;}

.logo {height: 50px; margin-bottom: 30px;}

.btn {
    letter-spacing: 1px;
    font-weight:bold;
    padding-left: 10px;
    padding-right: 10px;
    height: 34px;
    border-radius:3px;
    border: 0px;
    text-transform:uppercase;
    font-size:12px;
}

#btn_pri {
    background-color: #60A0DF;
    color: white;
}
#btn_sec { background-color: #e74c3c;
color:white;
}

This looks pretty good for a start.

the only thing that I see right away is your CSS formatting

a {
    text-decoration: none
}

html, body {height: 100%; max-height: 100%; padding:0; margin:0;}
.wrapper {display:table; width:100%; height:100%;}
.wrapper > div {display:table-cell; vertical-align: top;}
.wrapper .left {width:200px; background-color:#34495E}

#btn_pri {
    background-color: 60A0DF;
    color: white;
}
#btn_sec { background-color: #e74c3c;
color:white;
}

Here you switch between what I would call Minified CSS and Egyptian Brackets and the last rule here is a mix.

you should always stay consistent with your Formatting, especially in CSS because it can get confusing when you start adding more rules

so if you did this in all Egyptian Formatting.

a {
    text-decoration: none
}

html, body {
    height: 100%;
    max-height: 100%;
    padding:0;
    margin:0;
}

.wrapper {
    display:table;
    width:100%;
    height:100%;
}

.wrapper > div {
    display:table-cell;
    vertical-align: top;
}

.wrapper .left {
    width:200px;
    background-color:#34495E
}

#btn_pri {
    background-color: 60A0DF;
    color: white;
}

#btn_sec {
    background-color: #e74c3c;
    color:white;
}

This is how I would do it personally, it is clear what rule is what, and it looks neat and tidy as well.

but how ever you decide to do it, please be consistent.

As far as these Empty Rules.

.navigation {
}
.navigation li {
}

I assume that you are going to use them in the future for something,  if not, get rid of them, they will clutter up your code.


An Awk program to find all primes till N I developed Awk code to print all primes till N where N is given as command line argument. I think I took pretty much all the optimizations into account. Could someone help if it could be optimized even more. The code is as follows:

#!/usr/bin/awk -f

BEGIN{
  for (i=oddify(ARGV[1]); i>2; i=i-2){
    flag=1 #prime
    for(j=3; j<=ceil(sqrt(i)); j=j+2){
      if (i%j==0){
        flag=0 #not prime
        break
      }
    }
    if (flag==1){printf("%d\n", i) }
  }
  printf("2\n")
}

function ceil(n){
  return (n == int(n)) ? n : int(n)+1
}

function oddify(n){
  return (n%2 != 0) ? n : n-1
}

How to run: awk -f prime.awk 10000

Bug: when I run the program with argument 1, it prints 2 even though 2 > 1.

The code is not optimized at all. It computes ceil and sqrt way too often, in a way that is obvious to optimize. All other prime programs on this site already do that, have a look at them.

The code is not optimized enough. It contains i = i + 2, which should have been i += 2 from the beginning. A third of these calls can be omitted using the standard prime searching technique that has been mentioned more than enough on this site.

The variable name flag is terrible. It should be is_prime instead.

Having the most interesting code in the BEGIN block is bad design. You should define a function is_prime and another function print_primes_up_to. This allows you to declare i and j as local variables.

In the function ceil, the parameter name n is confusing since this name implies to the human reader that it is a natural number. In reality it is a floating point number though. A better name for this variable is x.

The code is missing some documentation. It should clearly state up to which number it reports correct results. It should also state the first number for which a wrong result is printed. Internally, many AWK implementations use 64-bit floating point numbers, therefore I guess this number is somewhere around \$2^{53}\$.


IP Whitelist function I've just created a function to check if your IP is within an array of IPs. The IP array can contain ranges like 192.168/170.50.0/255. I have a working example, but it seems a bit clunky to me.
Is there anything wrong with my method, or anything I could improve?
Link to Ideone example.
<?php

function checkIpWhitelist($whitelist, $yIp){

    $ips = $whitelist;
    $ySplitIp = explode('.', $yIp);

    $found = null;

    // Loop each IP.
    foreach($ips as $ip){

        // Explode it into parts.
        $splitIp = explode('.', $ip);

        // Each part.
        foreach($splitIp as $index => $part){

            // If this IP has failed a part, skip the rest of it.
            if($found === false){
                continue;
            }

            // Default to an array of just the part value.
            $range = array($part);

            // If the part contains a /, it's a range.
            if(strpos($part, '/') !== false){

                // Split the range.
                $range = explode('/', $part);

                // Make a new array, containing the full range.
                $range = range($range[0], $range[1]);
            }

            // If the part is in the range.
            if(in_array($ySplitIp[$index], $range)){
                $found = true;
            } else {
                $found = false;
            }
        }

        // If $found is true, all IP sections are matched. If not, reset $found to null.
        if($found === true){
            return true;
        } else {
            $found = null;
        }
    }

    return $found;
}

$yIp = '192.168.59.3';
$whitelist = array(
    array(
        '192.168.60.1',
        '192.168.0/50.3',
        '192.160/170.59.4'
    ),
    array(
        '192.168.60.1',
        '192.168.0/50.3',
        '192.160/170.59.3',
        '192.168.59.3'
    ),
    array(
        '192.168.60.1',
        '192.168.0/50.3',
        '192.160/167.59.3',
        '190/193.160/169.50/60.0/5',
        '192.168.59.3'
    )
);

foreach($whitelist as $list){
    var_dump("\n\n" . (checkIpWhitelist($list, $yIp) ? 'IP in range!' : 'IP not in range') . "\n\n");
}
?>

The examples return:

IP not in range
IP in range!
IP in range!

It took me a while until I understood your range notation. Consider switching to CIDR syntax. Networks are routed using CIDR so you usually can express ownership of an IP address by CIDR notation.

Another thing to note is that your code does not support IPv6. IPv6 will become more and more important over the next years. I recommend supporting it right now to be safe!

Now to your code:

Your function does several things at once. It loops through the provided ranges and checks each range. This should be separated into two functions: One reusable function that checks whether the IP address matches the whitelist entry and another one that calls the first function for each whitelist entry:

function checkIpWhitelist($ranges, $ip) {
    foreach ($ranges as $range) {
        if (isIpInRange($range, $ip)) return true;
    }

    return false;
}

Your algorithm indeed is overly complex and also has an unnecessary overhead because of that range call. range will create an array using that range, this takes time and memory!

I suggest normalizing the provided whitelist entry first: Convert any non-range to a range containing only that number and convert it to a data structure that is easier to handle than a string (array containing integers). Note that I am using array_map for that. In my opinion this functional programming style is easier to read and understand. You can easily write it as a normal foreach loop, though!

function isIpInRange($rangeString, $ip) {
    $range = array_map(function ($item) {
        $parts = explode('/', $item);
        if (count($parts) == 1) $parts[1] = $parts[0];

        return array_map('intval', $parts);
    }, explode('.', $rangeString));

Provided the user put in a syntactically correct range $range will be an array containing 4 arrays with two integers (start and each of the range) each. We can now check each part. I put a not around the whole expression in the if, as I find it more natural to read (“unless $ip[$i] is in the range the result is false”)

    $ip = explode('.', $ip);
    for ($i = 0; $i < 4; $i++) {
        if (!($range[$i][0] <= $ip[$i] && $ip[$i] <= $range[$i][1])) return false;
    }
    return true;
}

Spice it up using sanity checking! Currently the algorithm accepts dumb inputs, such as -9.94143.255/123.1/2/3/3.5. We maybe should check that first. This is a security feature, isn't it? Validating IP addresses is easy using PHP:

if (!filter_var($ip, FILTER_VALIDATE_IP)) {
    throw new InvalidArgumentException('Provide a valid ip address, please. "'.$ip.'" is not valid');
}

Validating your range is a little bit harder, but still possible. I suggest a regular expression for checking the basic structure and afterwards validating the ranges of the numbers. I am using Exceptions for error reporting here, but you can use anything you want.

// before normalizing
if (!preg_match('!^\d+(/\d+)?\.\d+(/\d+)?\.\d+(/\d+)?\.\d+(/\d+)?$!', $rangeString)) {
    throw new InvalidArgumentException('Provide a valid range, please. "'.$rangeString.'" is not valid');
}

// after normalizing
for ($i = 0; $i < 4; $i++) {
    // negative and non-numbers caught by the regular expression
    if ($range[$i][0] > $range[$i][1] || $range[$i][0] > 255 || $range[$i][1] > 255) {
        throw new InvalidArgumentException('Provide a valid range, please. "'.$rangeString.'" is not valid');
    }
}

Compiling that together we get these shiny two functions:

function checkIpWhitelist($ranges, $ip) {
    foreach ($ranges as $range) {
        if (isIpInRange($range, $ip)) return true;
    }

    return false;
}

function isIpInRange($rangeString, $ip) {
    if (!filter_var($ip, FILTER_VALIDATE_IP)) {
        throw new InvalidArgumentException('Provide a valid ip address, please. "'.$ip.'" is not valid');
    }

    if (!preg_match('!^\d+(/\d+)?\.\d+(/\d+)?\.\d+(/\d+)?\.\d+(/\d+)?$!', $rangeString)) {
        throw new InvalidArgumentException('Provide a valid range, please. "'.$rangeString.'" is not valid');
    }

    $range = array_map(function ($item) {
        $parts = explode('/', $item);
        if (count($parts) == 1) $parts[1] = $parts[0];

        return array_map('intval', $parts);
    }, explode('.', $rangeString));

    for ($i = 0; $i < 4; $i++) {
        // negative and non-numbers caught by the regular expression
        if ($range[$i][0] > $range[$i][1] || $range[$i][0] > 255 || $range[$i][1] > 255) {
            throw new InvalidArgumentException('Provide a valid range, please. "'.$rangeString.'" is not valid');
        }
    }

    $ip = explode('.', $ip);
    for ($i = 0; $i < 4; $i++) {
        if (!($range[$i][0] <= $ip[$i] && $ip[$i] <= $range[$i][1])) return false;
    }
    return true;
}


Using cookies in PHP I was wondering if there was a better way to do this. Would that mean there would need to be no client-side refreshing to show the cookies when they are created?

<?php
class cookieMonster
{
    public static $lastDestroyed;

    public function __construct() {  }

    /**
     * Register a cookie with the browser.
     * @param string $name Name to be given to the cookie
     * @param string $value Value to be put inside the cookie
     * @param int $expire How long the cookie will last, leave at 0 to expire on end of session/when the browser closes
     * @return bool True if cookie is created and false if not
     */
    public function registerCookie($name, $value = '', $expire = 0)
    {
        return setcookie($name, $value, $expire);
    }

    /**
     * Summary of retrieveCookie.
     * @param string $name The name of the cookie to retrieve.
     * @return bool True if found/not empty and false if not.
     */
    public function retrieveCookie($name)
    {
        $return = true;
        isset($_COOKIE[$name]) ?: $return = false;
        !empty($_COOKIE[$name]) ?: $return = false;
        return $return ? $_COOKIE[$name] : $return;
    }

    /**
     * Destroy a cookie variable, this can be undone, but not after a second has been destroyed.
     * @param mixed $name
     * @return bool
     */
    public function destroyCookie($name)
    {
        $return = true;
        isset($_COOKIE[$name]) ?: $return = false;
        !empty($_COOKIE[$name]) ?: $return = false;
        if ($return) :
            static::$lastDestroyed = ["name"=>$name, "value"=>$this->retrieveCookie($name)];
            unset($_COOKIE[$name]); return true;
        else :
            return false;
        endif;
    }

    /**
     * Restores the last destroyed cookie, can only be done for the very last cookie that was destroyed.
     * Does not require a param as the last destroyed is held in the class.
     * @return bool
     */
    public function restoreLastCookie()
    {
        $allow = true;
        static::$lastDestroyed ?: $allow = false;
        return $allow ? $this->registerCookie(static::$lastDestroyed['name'], static::$lastDestroyed['value']) : false;
    }
}

When reading through your code I find several things which I would change/remove.

The first is the empty constructor. A class without a constructor and a class with an empty constructor behave the same. I think it creates unnecessary noise in your code, so I would remove it.

Second is the naming of your methods. Since the class name is CookieMonster I find the suffix Cookie in registerCookie(), retrieveCookie() and destroyCookie() unnecessary. You could also name them register(), retrieve() and destroy() which I find more concise.

In your retrieveCookie() method I think there is room for improvement. The language construct empty also checks if the variable/key exists before determining if it evaluates as empty. So you could refactor the method body to the following:

/*
 * I have also renamed the method according to the
 * suggestions above.
 */
public function retrieve($name)
{
    if(!empty($_COOKIE[$name])) {
        return $_COOKIE[$name];
    }
    return false;
}

This is also true for your destroyCookie() method.

Regarding your destroyCookie() method. It doesn't destroy the cookie, but merely unsets the value inside the superglobal $_COOKIE. The cookie would be loaded again on the next request and would therefore never be deleted until it expires. To destroy a cookie you will have to redeclare it with an empty value and an expiration time in the past using the native PHP function setcookie() as you did when setting the cookie in registerCookie(). Unsetting the cookie inside the $_COOKIE superglobal is important too, as this can prevent any further code accessing a to be deleted cookie value. This effectively denies the code the possibility to rely on unpredictable data and helps reduce extremely hard to find bugs.

When destroying a cookie you will also have to make sure it has the same parameters as when set. This includes both the path and domain, When destroying cookies you should also be aware of the timezone complications outlined in this question. An example:

/*
 * Create a timestamp one hour in the past, which is 3600 seconds.
 */
$expire = time() - 3600;

setcookie('mycookie', null, $expire);

/*
 * If a path and/or domain was specified when the cookie was
 * created they too should be included.
 */
setcookie('mycookie', null, $expire, $path, $domain);

Talking about the path and domain leads me to the next part. When creating a cookie with your registerCookie() method I have no way to set a path and/or domain as well as I cannot declare whether the cookie should only be sent over secure connections (SSL etc.) or if it is only restricted to HTTP requests leaving javascript unable to read/update/delete the cookie (this is often preferable when dealing with security).

Therefore I would change the method signature to include this.

/*
 * Renamed according to suggestions.
 */
public function register($name, $value, $expire, $path, $domain, $secure, $httponly) {...

The $path, $domain, $secure and $httponly parameters can usually have default values. See the documentation of setcookie() for more information.

Now when I am able to change these settings I will also require them to be available when destroying the cookie as two of them are required, namely $path and $domain. Therefore you should also change your destroyCookie() method signature.

/*
 * Renamed according to suggestions.
 */
public function destroy($name, $path, $domain) {...

Again these can have default values matching the register() method signature.

Now to the last thing. I cannot see the use of and have never required the possibility to restore a deleted cookie. I would even go as far as sayings its a security issue. Consider the following. Your authentication system destroys the cookie when the user logs out. Then somewhere inside your code the cookie is restored before a response is sent to the client. Now your authentication system has been undermined by one single method call. This cannot be good and you're right!

I would simply remove the method and its related class property and focus on making this a utility class which wraps the cookie functions with reasonable logic. Such logic could be ensuring the cookie $name is in fact a string and that is doesn't contain invalid characters. This is also appropriate for your other methods.

I would urge you to take a look at the Symfony cookie wrapper. They have a dedicated class to this and cookies are set using the setCookie() method in response headers, which you might also find interesting.

I hope this can help guide you, happy coding!


Creating  element and append data to it I've made the following and have a few questions:

Do I have some (big) mistakes in this approach? (bad practices, 'this code is trash'...)

1.1 If I do, can you suggest what to fix?
Are appended element created in good way?
Code in success function seems to be a little long.  Any way to refactor?

CSS code

.no-display-table {
    display: none;
}

HTML code

    ...

    ...

...

 Selected options

JavaScript code

$(function() {
    $('#options-table').addClass('no-display-table');

    $('#product-option-select').change(function() {
        if($('.append-select select').length > 0)
            $('.append-select select').remove();

        var selectedOptionId = $(this).val();

        $.ajax({
            type: 'get',
            url: '../option_types/' + selectedOptionId,
            success: function(data) {
                var element = '<select id="value-select" class="form-control">';
                element += '<option selected disabled>Select value...</option>';

                for(var i=0; i<data.length; i++) {
                    element += '<option value="' + data[i].id + '">' + data[i].name + '</option>';
                }

                element += '</select>';
                $('.append-select').append(element);

                $('.append-select').on('change', '#value-select', function() {
                    $('#options-table').removeClass('no-display-table');

                    var selectedValue = $('#value-select option:selected').text();
                    var element = '
' + selectedValue + '	';
                        element += ' x
';

                    $('#options-table tbody').append(element);

                    $('#options-table').on('mouseover', '.remove', function() {
                        $('.remove').css('cursor', 'pointer');
                    });

                    $('#options-table').on('click', '.remove', function() {
                        $(this).parent().parent().remove();

                        if($('#options-table tbody tr').length == 0)
                            $('#options-table').addClass('no-display-table');
                    });
                });
            }
        });
    });
});

My 2 cents:

You could use $().hide() instead of assigning your custom hiding css class
Similarly you could use $().show() instead of removing your custom hiding css class
You do not have to check the length of a jQuery query, just call remove(), it will work.
Your building of html inside the success callback is ugly, have a helper function to construct it and consider using DOM functions instead of HTML building. I personally tend to show/hide the select element and then rebuild the list with this helper function:

function setOptions( id , array )
{
  //Get the element
  var select = document.getElementById( id ) , i
  //Remove potentially existing children, somewhat heartless
  while (select.firstChild)
    select.removeChild(select.firstChild);
  //Add the new children
  for( i = 0 ; i < array.length ; i++ )
  {
    var element = new Option( array[i].id , array[i].name );
    select.appendChild(element);
  }
}

Assiging the listener in the success callback is also too much, consider building a listener that takes into account new elements on document instead $(document').on('change', etc.
Same for the mouseover / click event listeners

Simply having the document element ( or any other parent element that stays in the DOM ) and the selector inside the on call should activate the listeners for new elements.

function initializeListeners()
{
  $(document).on('change', '#value-select', function() {
      $('#options-table').removeClass('no-display-table');

      var selectedValue = $('#value-select option:selected').text();
      var element = '
' + selectedValue + '	';
          element += ' x
';

      $('#options-table tbody').append(element);

      $('#options-table').on('mouseover', '.remove', function() {
          $('.remove').css('cursor', 'pointer');
      });

      $('#options-table').on('click', '.remove', function() {
          $(this).parent().parent().remove();

          if($('#options-table tbody tr').length == 0)
              $('#options-table').addClass('no-display-table');
      });
  });
}


Counter in FASM assembly I am making a simple counter in assembly. It counts to a billion and exits. However, the performance of this is really horrible. A Java application (that does the same) outperforms it by around 10x. How can I optimize this code?

include 'include/win32ax.inc'

section '.data' Data readable writeable
  outhandle  DD ?
    inhandle   DD ?
    incr DD 0
    numwritten DD ?
    inchar     DB ?
    numread DD ?
section '.text' code readable executable
  start:
     invoke  AllocConsole
      invoke  GetStdHandle,STD_OUTPUT_HANDLE
         mov [outhandle],eax
       invoke  GetStdHandle,STD_INPUT_HANDLE
        mov [inhandle],eax
        invoke  WriteConsole,[outhandle],"Starting...",11,numwritten,0

        jmp loopcount
       loopcount:
       inc [incr]
       cmp [incr], 1000000000;1 billion. ;]
       jne loopcount
       invoke  WriteConsole,[outhandle],"   Done count",13,numwritten,0
       invoke  ReadConsole,[inhandle],inchar,2,numread,0
       invoke ExitProcess,0
.end start

Load [incr] into a register instead, for example eax, and increment the register.

Instead of cmp [incr], 1000000000 and jne, try loading 1000000 into the ecx register and using the loop opcode.

The advice above to use loop is probably obsolete. Instead it may be faster to do load 1000000 into a register like ebx, and do a dec ebx and jnz loop.

Intel's Branch and Loop Reorganization to Prevent Mispredicts seems to recommend you code a loop like this ...

loop_top:
... do work inside the loop here ...
cmp ebx,1000000
jz loop_end ; happens rarely
inc ebx
jmp loop_top ; happens often
loop_end:

... in order to optimize the branch prediction.

Unrolling the loop might make it faster. In fact given how dumb the algorithm is, the best optimization is to immediately 1000000 to [incr] without looping.


Groovy: find all elements in a list equal to the max element Is there any simpler way to find all elements in a list that are equal to the max element.

List v = [ 1,2,3,4,5,5  ]
def max = v.max()
def maxs = v.findAll { it == max }

Thanks!

How you've done it for the simple example is exactly how I would do it.  I may use groupBy if I was dealing with a more complex object.

    List v = [ 1,2,3,4,5,5 ]
    def max = v.max()
    def results = v.groupBy {it}.get(max)
    assert [5,5] == results


Setting the position of a navigation highlight How can I make my function better and favor functional programming style?

On this single-page site I have a fixed navigation list. Each item in the list has a transparent background and I have a #nav-highlight div that I move with JavaScript and animate with CSS for an interesting effect while scrolling through the page.

Everything works, but I'm not happy with my implementation. There's one part that doesn't make sense without the comments to explain it and I need to use continue and break for it to work this way. I'd prefer to move more toward a functional style, but I couldn't figure out how to do this without a for loop.

Here's the relevant function. You can see the code in context at GitHub and the page is hosted here:

function setHighlightPosition(nav) {
  const documentEnd = document.documentElement.scrollHeight - window.innerHeight;
  const scrollTop = window.scrollY;
  const scrollMiddle = scrollTop + (window.innerHeight / 2);
  const highlight = document.getElementById('nav-highlight');

  let highlightItem = 0;
  for (let i = 0; i < nav.positions.length; i += 1) {
    const item = nav.positions[i];
    if (!item.targeted) continue;

    const target = document.getElementById(item.id);
    const position = target.offsetTop;

    // If the target is close to the top of the window, set the highlighted item
    // and don't check any more.
    if (
      (Math.abs(position - scrollTop) < 25) ||
      (scrollTop < position && position - scrollTop < 50)
    ) {
      highlightItem = i;
      break;
    }

    // Check the position of a target near the middle of the window if there
    // were no targets near the top of the window.
    if (scrollMiddle > position) {
      highlightItem = i;
    }
  }

  if (scrollTop === documentEnd) {
    highlightItem = nav.positions.length - 1;
  }

  highlight.style.transform = `translateY(${nav.positions[highlightItem].position}px)`;
  highlight.dataset.position = highlightItem;
}

Here's the affected HTML

Objective

Bio

Skills

History

Certification

Connect

Refactoring

Disclaimer: Sorry, I didn't have a solid chunk of time to test the code, so please consider it as a direction rather than an exact prescription.

It's always possible to break away from loop execution interfering keywords like continue and break. It is also almost always possible to quickly rewrite the code in a "more functional" way. Question is, what exactly is the motivation.

First, in functional world it all about function. And even though my code below does not use function composition, I'm adding getPosition(), isCloseToTop(), and isScrollAfterPosition() to simplify a few conditions. Another benefit of that is that these local functions' names are replacing the ugly comments.

Second, nav.positions.findIndex(...) is used to find out whether there's an item close to top. This is made as an independent step in the code and IMO it's more expressive this way.

If there's no item close to the top of window, we can try to find the last position before the scroll. Again, now it's implemented as a dedicated step (via map() and filter()).

Hopefully, it's now closer to what you wanted the code to be.

function setHighlightPosition(nav) {
  const scrollTop = window.scrollY;
  const scrollMiddle = scrollTop + (window.innerHeight / 2);

  const getPosition = item => document.getElementById(item.id).offsetTop;
  const isCloseToTop = position =>
    Math.abs(position - scrollTop) < 25 ||
    scrollTop < position && position - scrollTop < 50;
  const isScrollAfterPosition = position => scrollMiddle > position;

  const indexOfItemCloseToTop = nav.positions.findIndex(item => item.targeted && isCloseToTop(getPosition(item)));

  if (indexOfItemCloseToTop >= 0) {
    highlightItem = indexOfItemCloseToTop;
  } else {
    const positionsBeforeScroll = nav.positions
      .map((item, itemIndex) => (item.targeted && isScrollAfterPosition(getPosition(item))) ? itemIndex : null)
      .filter(index => index !== null);
    highlightItem = positionsBeforeScroll[positionsBeforeScroll.length - 1];
  }

  const documentEnd = document.documentElement.scrollHeight - window.innerHeight;
  if (scrollTop === documentEnd) {
    highlightItem = nav.positions.length - 1;
  }

  const highlight = document.getElementById('nav-highlight');
  highlight.style.transform = `translateY(${nav.positions[highlightItem].position}px)`;
  highlight.dataset.position = highlightItem;
}

BTW, I moved variable declarations around so that the declarations are located next to their first usages. This reduces brain cycles and eliminates the need of scrolling the code up and down.


Setting various types of properties on a JCR node This code checks value's type and cast it into appropriate type, then puts it into a javax.jcr.Node.

The checking looks long and boring. Is there a way to refactor it?  I'm using Java 7.

/**
 * Set node's property.
 */
public static void setProperty(Node node, String propertyName, Object propertyValue) throws RepositoryException {
    if (node == null) {
        throw new IllegalArgumentException("Something wrong");
    }
    if (propertyName == null) {
        throw new IllegalArgumentException("Property name is invalid");
    }

    if (propertyValue == null) {
        node.setProperty(propertyName, (Value) null);
    } else if (propertyValue instanceof Value) {
        node.setProperty(propertyName, (Value) propertyValue);
    } else if (propertyValue instanceof Node) {
        node.setProperty(propertyName, (Node) propertyValue);
    } else if (propertyValue instanceof Binary) {
        node.setProperty(propertyName, (Binary) propertyValue);
    } else if (propertyValue instanceof Calendar) {
        node.setProperty(propertyName, (Calendar) propertyValue);
    } else if (propertyValue instanceof Date) {
        Calendar cal = Calendar.getInstance();
        cal.setTime((Date) propertyValue);
        node.setProperty(propertyName, cal);
    } else if (propertyValue instanceof BigDecimal) {
        node.setProperty(propertyName, (BigDecimal) propertyValue);
    } else if (propertyValue instanceof String) {
        node.setProperty(propertyName, (String) propertyValue);
    } else if (propertyValue instanceof Long) {
        node.setProperty(propertyName, ((Long) propertyValue).longValue());
    } else if (propertyValue instanceof Double) {
        node.setProperty(propertyName, (Double) propertyValue);
    } else if (propertyValue instanceof Boolean) {
        node.setProperty(propertyName, (Boolean) propertyValue);
    } else if (propertyValue instanceof InputStream) {
        node.setProperty(propertyName, inputStreamToBinary((InputStream) propertyValue));
    } else if (propertyValue instanceof Collection) {
        CollectionFactory factory = CollectionFactoryImpl.getInstance();
        ArrayList<Value> values = new ArrayList<Value>();
        for (Object value : (Collection<Object>) propertyValue) {
            values.add(createValue(value, factory));
        }
        node.setProperty(propertyName, values.toArray(new Value[values.size()]));
    } else {
        throw new IllegalArgumentException("Something wrong");
    }
}

@Test
public void test() {
    Node node = createNode("test");
    setProperty(node, "name", "name");
}

My idea would be to encapsulate the job of casting and converting into some kind of a Strategy pattern.

I would use an enum for that, namely NodeSettingStrategy:

public static enum NodeSettingValueStrategy {
    UNKNOWN(null) {
        @Override
        public void set(Node node, String propertyName, Object value) {
            throw new IllegalArgumentException("Cannot set property to a value of type " + value.getClass());
        }
    },

    DATE(Date.class) {
        @Override
        public void set(Node node, String propertyName, Object value) {
            node.setProperty(propertyName, (Long) value);
        }

    },

    LONG_PRIMITIVE(long.class) {
        @Override
        public void set(Node node, String propertyName, Object value) {
            node.setProperty(propertyName, (long) value);
        }
    },

    LONG(Long.class) {
        @Override
        public void set(Node node, String propertyName, Object value) {
            node.setProperty(propertyName, (Long) value);
        }
    },

    STRING(String.class) {
        @Override
        public void set(Node node, String propertyName, Object value) {
            node.setProperty(propertyName, (String) value);
        }
    },

    NODE(Node.class) {
        @Override
        public void set(Node node, String propertyName, Object value) {
            node.setProperty(propertyName, (Node) value);
        }
    };

    private final Class<?> supportedType;

    private NodeSettingValueStrategy(Class<?> supportedType) {
        this.supportedType = supportedType;
    }

    public static NodeSettingValueStrategy of(Class<?> type) {
        for (NodeSettingValueStrategy strategy : NodeSettingValueStrategy.values()) {
            if (strategy.supportedType == type) {
                return strategy;
            }

        }
        return NodeSettingValueStrategy.UNKNOWN;
    }

    public abstract void set(Node node, String propertyName, Object value);
}

Then I want to hide this NodeSettingStrategy by a thin abstraction, namely NodeSetter:

public static class NodeValueSetter {
    private final NodeSettingValueStrategy strategy;
    private final String propertyName;
    private final Object value;

    private NodeValueSetter(NodeSettingValueStrategy strategy, String propertyName, Object value) {
        this.strategy = strategy;
        this.propertyName = propertyName;
        this.value = value;
    }

    public void setTo(Node node) {
        strategy.set(node, propertyName, value);
    }

    public static NodeValueSetter of(final String propertyName, final Object value) {
        NodeSettingValueStrategy settingStrategy = NodeSettingValueStrategy.of(value.getClass());
        return new NodeValueSetter(settingStrategy, propertyName, value);
    }

}

Combine them together:

@Test
public void should_able_to_set_property() throws Exception {
        Node node = new Node();

        Node givenNode = new Node();
        String someString = "The brown fox jumps over a lazy dog";
        long givenLong = 234234234234L;

        for (NodeValueSetter setter : Arrays.asList(
                NodeValueSetter.of("a-string", someString),
                NodeValueSetter.of("a-number", givenLong),
                NodeValueSetter.of("a-node", givenNode))) {
            setter.setTo(node);
        }

        assertThat(node.getProperty("a-string"), is(someString));
        assertThat(node.getProperty("a-number"), is(givenLong));
        assertThat(node.getProperty("a-node"), is(givenNode));
    }


Using an instance of PDO class as global variable inside 'query' methods I have one PHP file named database.php:

 try {
     $connection= new PDO(DB_DNS, DB_USER, DB_PASS);
     $connection->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
} catch(Exception $e) {
     echo "Error: ".$e->getMessage();
     die();
}

and I have a user.php with class Users:

require_once(__DIR__."/../includes/config.php");
require_once(__DIR__."/../includes/database.php");

class User
{
    public function pronadjiSveUsere() {
        global $connection;
        $upit = $konekcija->prepare("SELECT * FROM users");
        $rezultat_upita = $upit->execute();
        return $rezultat_upita;
    }

    public function find_by_id($id = 0) {
        try {
            global $connection;
            $upit = $konekcija->prepare("SELECT * FROM users WHERE id = :id");
            $rezultat_upita = $upit->execute(array($id));
            return $rezultat_upita->fetch(PDO::FETCH_OBJ);
        } catch (Exception $e) {
            echo $e->getMessage();
            die();
        }
    }
}

So, you can see that there is a global $connection in each of the methods. Is this bad and why? If so, what would you do, as parameter passing, or with a constructor?

Use global scope for as little as possible. EVER.

Explicitly passing the connection it into the method is always the safe and happy.  It allows for better testing by adhering to dependancy inversion principles.

You could also do it inside of the constructor if the the connection doesn't change during the life of the object, but I'm not a fan of this--I'd prefer a intitial() method to be called after instantiation or explicitly setting it with a set_connection() method.


Go to next visible cell without loop I have an autofiltered table in Excel. I have to copy/paste values based on certain conditions and I also have to perform this on all visible cells in a particular column. I have written the code and it works well but the only thing is that it takes a lot of time as there are many rows. Can anyone please help me how to thrash time required? Here's the code. Thanks!

Sub FilterAndCopy()

Windows("Epson Itemcodes.xlsm").Activate
    Range("A" & i).Select
    Selection.Copy

Windows("Epson ASINs.xlsx").Activate
    Range("U1048576").End(xlUp).Offset(0, -12).Select

If ActiveCell.Value <> "Itemcode" Then

If ActiveCell.Value = "" Then
    ActiveSheet.Paste

    Else

    If ActiveCell.Value = Workbooks("Epson Itemcodes.xlsm").Sheets("Sheet1").Range("A" & i).Value Then
    ActiveSheet.Paste

    Else
    ActiveCell.Value = "Conflct"

    End If
    End If

Else
Windows("Epson Itemcodes.xlsm").Activate
Range("I" & i).Value = "No match found"

End If

If ActiveCell.Value <> "Itemcode" Then

With ActiveSheet
Do

ActiveCell.Offset(-1, 0).Activate
Do While ActiveCell.EntireRow.Hidden = True
ActiveCell.Offset(-1, 0).Activate
Loop

If ActiveCell.Value <> "Itemcode" Then

If ActiveCell.Value = "" Then
    ActiveSheet.Paste

    Else

    If ActiveCell.Value = Workbooks("Epson Itemcodes.xlsm").Sheets("Sheet1").Range("A" & i).Value Then
    ActiveSheet.Paste

    Else

    ActiveCell.Value = "Conflct"

    End If
    End If

Else
Exit Do

End If

Loop
End With

End If

End Sub

Before you write another line of code, watch this Excel VBA Introduction Part 5 - Selecting Cells (Range, Cells, Activecell, End, Offset).

Everyone here wants to help you but, and I know that you read this,  you have got to stop selecting and activating cells.  You also need to describe your data.  Posting screenshots wouldn't hurt either.  So if I sound mean but you wrote 72 lines of code to copy maybe 12 values.

Range.Copy, Range.Cut, Range.Insert and by default act on only the visible cells.

Sub FilterAndCopy()
    Dim Source As Range, Target As Range

    With Workbooks("Epson ASINs.xlsx").Worksheets("Sheet1")
        Set Source = .Range("U2", .Range("U" & .Rows.Count).End(xlUp)).SpecialCells(xlCellTypeVisible)
    End With

    With Workbooks("Epson Itemcodes.xlsm").Worksheets("Sheet1")
        Set Target = .Range("U" & .Rows.Count).End(xlUp).Offset(1)
    End With

    Source.Offset(1).Copy Desination:=Target

End Sub


Matching values to a lookup worksheet I have a worksheet that contains values, and I need to match one of the values to a lookup worksheet. This syntax works like it should, however, it takes roughly 10 minutes to fully execute. This could be due to the size of the worksheets: Sheet1 has roughly 18,000 rows and Sheet2 has roughly 20,000 rows.

Is there any optimization that can take place in this syntax?

Function TwoMatches()
Dim wSheet As Worksheet: Set wSheet = Sheets("Sheet2")
Dim lookupSheet As Worksheet: Set lookupSheet = Sheets("Sheet1")
Dim lr As Long: lr = wSheet.Cells(wSheet.Rows.Count, "A").End(xlUp).Row
Dim destSheet As Worksheet: Set destSheet = Sheets("Sheet2")
Set lookupRange = Sheets("Sheet1").Range("$A:$C")
Dim Found As Range

'Add a column at the beginning to Trim() and Remove Hyphens
Columns("A:A").Insert Shift:=xlToRight, CopyOrigin:=xlFormatFromLeftOrAbove

'Add Clean Sheet1 Name As Header Text
ActiveWorkbook.Sheets("Sheet2").Range("A1").Value = "Clean Sheet1 Name"

'This is the worksheet formula that I want to use for the used range
With wSheet
    lr = .Cells(.Rows.Count, "D").End(xlUp).Row
    form = "=TRIM(SUBSTITUTE(D2,""-"", """"))"
    .Range("A2:A" & lr).Formula = form
End With

'Get the Last Column With Data
ActiveWorkbook.Sheets("Sheet2").Range("P1").Value = "LookupVal"

For i = Range("A" & Rows.Count).End(3).Row To 2 Step -1
    'Pulling value where column O is not null
    If Not IsEmpty(Cells(i, "O").Value) Then
    With Sheets("Sheet1")
        'Filter criteria one is where column B IS NOT NULL
        .Range("$A:$C").AutoFilter Field:=2, Criteria1:="<>"

        Set Found = .Range("A:A").Find(What:=wSheet.Cells(i, "A").Value, _
                               LookIn:=xlValues, LookAt:=xlWhole, SearchOrder:=xlByRows, SearchDirection:=xlNext, _
                               MatchCase:=False)

        If Not Found Is Nothing Then
            wSheet.Cells(i, "P").Value = Found.Offset(, 2).Value
        End If
    End With
    'Pulling value where column O is null
    Else
    With Sheets("Sheet1")
        'Filter criteria one is where column B IS NULL
        .Range("$A:$C").AutoFilter Field:=2, Criteria1:="="
        Set Found = .Range("A:A").Find(What:=wSheet.Cells(i, "A").Value, _
                               LookIn:=xlValues, LookAt:=xlWhole, SearchOrder:=xlByRows, SearchDirection:=xlNext, _
                               MatchCase:=False)

        If Not Found Is Nothing Then
            wSheet.Cells(i, "P").Value = Found.Offset(, 2).Value
        End If
    End With
    End If
Next i
'Removing the column added for the Clean Match
Range("A:A").Delete
End Function

General points

both wSheet and destSheet points to "Sheet2", I suppose you don't need both
while you can save a few lines using :, it's generally considered to be a bad practice to use them.
functions are generally more or less portable, hardcoding sheet names in them makes it impossible. You could pass sheet names as arguments
lookUpRange is not declared

You should always use Option Explicit in your code

Dim Found As Range - it's name suggests it's a boolean, not a range
lr = wSheet.Cells(wSheet.Rows.Count, "A").End(xlUp).Row - you set the value at the beginning, but never use it, just overwrite
ActiveWorkbook.Sheets("Sheet2").Range("P1").Value = "LookupVal" You've declared variables for Sheet2, why don't use them?
... and a lot of other similar issues

General performance

you can use these at the beginning of your codeto speed up your code:
Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual
to perform calculations after inserting the formulas use
Application.calculate

Specific suggestion

.Range("$A:$C").AutoFilter Field:=2, Criteria1:="<>"
Set Found = .Range("A:A").Find(What:=wSheet.Cells(i, "A").Value, _
    LookIn:=xlValues, LookAt:=xlWhole, SearchOrder:=xlByRows, SearchDirection:=xlNext, _
    MatchCase:=False)
If Not Found Is Nothing Then
    wSheet.Cells(i, "P").Value = Found.Offset(, 2).Value
End If

It's difficult to give the best solution without looking your data, but here I'd definitely use some formula (Index / match / row / ...) instead of filtering the data thousands of times. Or at least sort data to avoid some of the filtering.


Java 8 String Unique Permutations in Parallel Is there a better way to do this Java 8 String Unique Permutations in Parallel

package com.bos;

import java.util.Date;
import java.util.Set;
import java.util.concurrent.ConcurrentSkipListSet;
import java.util.stream.IntStream;

public class StringPermutation {
    private static Set<String> set = new ConcurrentSkipListSet<String>();

    public static void permutation(String str) {
        permutation("", str);
    }

    private static void permutation(String prefix, String str) {
        int n = str.length();
        if (n == 0) {
            set.add(prefix);
        } else {
            IntStream.range(0, n).parallel()
                    .forEach(i -> permutation(prefix + str.charAt(i), str.substring(0, i) + str.substring(i + 1, n)));
        }
    }

    public static void main(String[] args) {
        Date startDate = new Date();
        long startTime = System.nanoTime();
        System.out.println("Started at " + startDate);
        permutation("ferrao");
        set.stream().forEach(System.out::println);
        Date endDate = new Date();
        long totalTime = System.nanoTime() - startTime;
        System.out.println("Ended at " + endDate + " total time=" + totalTime + " nanosec");

    }
}

The presented way is not a functional solution. It uses streams but it relies on forEach, which is the imperative way of using a Stream.

To make it better, we need to think about what we really want to do. We want to build all the permutations of the characters in a String. That means we need to have a method permutations(String) that will return all the permutations. Using Java 8, we can return a Stream<String> which will corresponds to the Stream of all the permutations.

To build those permutations, we can have a recursive algorithm:

If the String is empty, there are no characters, so the only result is a Stream that contains the empty String. This is the base case.
If the String is non-empty:

we remove the first character, build all the permutations without it and then prepend the first character to all the results.
then we remove the second character, build all the permutations without it and then prepend the second character to all the results.
etc. until we hit the last character.

Using Java 8, this is an implementation:

public static Stream<String> permutations(String str) {
    if (str.isEmpty()) {
        return Stream.of("");
    }
    return IntStream.range(0, str.length())
                .boxed()
                .flatMap(i ->
                  permutations(str.substring(0, i) + str.substring(i+1)).map(t -> str.charAt(i) + t)
                );
}

The base-case is handled by the if: when the String is empty, we return Stream.of(""): a Stream that contains only the empty String/

The main part is after: we create an IntStream over the indexes of the String. (Since there are unfortunately no flatMapToObj operation, we need to box each int into an Integer.) Then that's where the magic happens: for each index, we recurse by building all the permutations again, leaving the index i, and each of the result is mapped to prepend the character at index i. Since this recursive call returns a Stream<String>, this result is flat mapped (with flatMap) to have a single Stream.

With this utility method, your code is then just

permutations("ferrao").forEach(System.out::println);

That brings also a big advantage: the method is returning a Stream<String>, that is to say, the method does not actually perform the calculation. It is then up-to the caller to decide when to do it, if they want it to happen in parallel or not, and especially what the end container will be (a List, a Set, no container at all with just post-processing afterwards).

For example, to have it performed in parallel, you just need to do

permutations("ferrao").parallel().forEach(System.out::println);