Are numbers prime multithreading I'm trying to learn some multithreading and this seems like a good example, want to apply this to frustum culling later.
The goal is to find prime numbers in randomly generated vector of numbers.
Single threaded solution is simple, multithreaded way is to divide vector of random numbers in two parts, solve each one in the same time and merge results.

Here is the time it took to solve:

mt:

solved in: 17185.4 ms

solved in: 15832.2 ms

solved in: 16012.5 ms

solved in: 16354.9 ms

st:

solved in: 26942 ms

solved in: 30577.9 ms

solved in: 26403.4 ms

solved in: 29625.7 ms

It obviously works, but is the code correct?

This is from main.cpp:

 #include "primeNumbers.h"
 int main(){
    PrimeNumbers numbers(100000, 0, 5000000);
    PrimeNumberSolver* solver = new PrimeNumberSingleThread;
    //PrimeNumberSolver* solver = new PrimeNumberMultiThread;
    numbers.Solve(solver);
    delete solver;
 }

This is the rest of the code:

//primeNumbers.h
#include <mutex>
#include <thread>
#include <vector>
#include <iostream>
class PrimeNumberSolver abstract {
public:
    bool isPrime(int n) {
        for (int i = 2; i < n/2; ++i) {
            if (n % i == 0)
                return false;
        }

        return true;
    }

    virtual void solve(const std::vector<int>& source, std::vector<int>& result) = 0;

};

class PrimeNumberSingleThread final : public PrimeNumberSolver {
    friend class PrimeNumbers;
private:
    virtual void solve(const std::vector<int>& source, std::vector<int>& result) override{
        for (unsigned i = 0; i < source.size(); ++i) {
            if (isPrime(source[i]))
                result.push_back(source[i]);
        }
    }
};

class PrimeNumberMultiThread final : public PrimeNumberSolver {
    friend class PrimeNumbers;

private:
    virtual void solve(const std::vector<int>& source, std::vector<int>& result) override {
        std::vector<int> r1;
        std::vector<int> r2;

        //split source
        int half = source.size() / 2;
        int end = source.size();

        std::mutex m;

        //run two threads
        std::thread t1([&]() {
            for (int i = 0; i < half; ++i) {
                //std::lock_guard<std::mutex> lock(m);  //do i need to lock mutex before accessing source
                if (isPrime(source[i]))
                    r1.push_back(source[i]);
            }
        });
        std::thread t2([&]() {
            for (int i = half; i < end; ++i) {
                //std::lock_guard<std::mutex> lock(m);  //do i need to lock mutex before accessing source
                if (isPrime(source[i]))
                    r2.push_back(source[i]);
            }
        });

        //join threads
        if(t1.joinable()) t1.join();
        if(t2.joinable()) t2.join();

        //merge results
        result.insert(result.end(), r1.begin(), r1.end());
        result.insert(result.end(), r2.begin(), r2.end());
    }
};

class PrimeNumbers {
public:
    PrimeNumbers(int size, unsigned min, unsigned max) {
        std::mt19937 rng;
        for (int i = 0; i < size; ++i) {
            rng.seed(std::random_device()());
            std::uniform_int_distribution<std::mt19937::result_type> dist6(min, max);
            randoms.push_back(dist6(rng));
        }
        std::cout << "Numbers are generated" << std::endl;
    }

    void Solve(PrimeNumberSolver* solver) {
        double time = glfwGetTime();
        solver->solve(randoms, primes);
        std::cout << "solved in: " << (glfwGetTime() - time) * 1000.0 << " ms" << std::endl;
    }

    void printPrimes() {
        for (unsigned i = 0; i < primes.size(); ++i)
            std::cout << primes[i] << std::endl;
    }

    std::vector<int> randoms;
    std::vector<int> primes;
};

This is a poor way to test primality:

bool isPrime(int n) {
    for (int i = 2; i < n/2; ++i) {
        if (n % i == 0)
            return false;
    }
    return true;
}

Even sticking with trial division, we can stop at √n, which is generally much less than n/2.  It's also well worth treating 2 as a special case, to reduce the iterations by half (i.e. i += 2 instead of ++i).

Why are we using signed integers, given that we don't need any negative values?

You say the "output is tested", but don't show any of the test code.  It's useful to see how the test works, to identify tests that are missing or erroneously passing.  I mention this because it's notoriously difficult to create reliable tests for multithreaded code.

In PrimeNumberMultiThread::solve, we have opportunity to reduce duplication, and we don't need to hard-code the number of threads to two.  Instead, we can use as many threads as we have cores available, using a for loop to start them and another for loop to join them all.

The mutex m and the commented-out lock_guard should just be removed: these aren't necessary, because only one thread accesses each vector between the worker threads starting and finishing (which points are memory barriers).

Instead of result.insert(), the general case might benefit from std::move algorithm - in any case, it's well worth a reserve before adding the results.

With OpenMP, much of the work can be automated for you - you'll just need a custom reduction to combine vectors - I think that's available from OpenMP 4.0 onwards.

An alternative to using vectors is to use an array of flags to indicate which values are to be included in the result (the flags could even be in the input vector: overwrite the non-primes with zero to discard them).  Then there's no reallocation within either thread, and you can use OpenMP of older versions because we no longer need a custom reduction.

Here's one version that uses the OpenMP approach I've just described:

#include <algorithm>
#include <iostream>
#include <iterator>
#include <random>
#include <vector>

bool isPrime(unsigned n) {
        if (n < 2) return false;
        if (n < 4) return true;
        if (n%2 == 0) return false;
        for (auto i = 3ul;  i*i < n;  i += 2) {
            if (n % i == 0)
                return false;
        }

        return true;
}

std::vector<unsigned> solve(std::vector<int>& source)
{
    auto const len = source.size();
    std::size_t result_size = 0;

#pragma omp parallel for reduction(+:result_size)
    for (std::size_t i = 0;  i < len;  ++i) {
        if (isPrime(source[i])) {
            ++result_size;
        } else {
            source[i] = 0;
        }
    }

    std::vector<unsigned> result;
    result.reserve(result_size);

    std::copy_if(source.begin(), source.end(),
                 std::back_inserter(result),
                 [](unsigned i){ return i; });

    return result;
}

int main(){
    const std::size_t size = 1000000u;
    const unsigned min = 0u;
    const unsigned max = 5000000u;

    std::vector<int> randoms;
    randoms.reserve(size);

    {
        std::uniform_int_distribution<std::mt19937::result_type> dist(min, max);
        std::mt19937 rng;
        rng.seed(0);            // for a reproducible test
        for (std::size_t i = 0;  i < size;  ++i) {
            randoms.push_back(dist(rng));
        }
    }

    auto primes = solve(randoms);
    std::clog << "Found " << primes.size() << " primes "
              << " from " << randoms.size() << " inputs\n";
}

Compile with g++ -fopenmp, or the equivalent for your compiler.

To test a single-threaded version, we just have to remove (manually, or with #ifdef) the #pragma omp line.

Note that since C++17, we're able to specify an execution policy for std::copy_if, so we could reduce that code even further, especially if we're not fussy about the order of the results:

#include <execution>

std::vector<unsigned> solve(std::vector<int>& source)
{
    std::vector<unsigned> result;

    std::copy_if(std::execution::par_unseq
                 source.begin(), source.end(),
                 std::back_inserter(result),
                 [](unsigned i){ return isPrime(i); });

    return result;
}

I haven't been able to test this, as I'm still working with an old standard library that doesn't have <execution>.


Stack based non-recursive Fibonacci - Go implementation The following function computes a the last element in a Fibonacci sequence in Go language.

package main

import (
    "fmt"
)

type Element struct {
    Next  *Element
    Value interface{}
}
type Stack struct {
    Top   *Element
    Count int
}

func (stack *Stack) Push(value interface{}) {
    el := Element{
        Value: value,
        Next:  stack.Top,
    }
    stack.Top = &el
    stack.Count++
}
func (stack *Stack) Peek() (value interface{}) {
    if stack.Top != nil {
        value = stack.Top.Value
    }
    return
}
func (stack *Stack) Pop() (value interface{}) {
    if stack.Top != nil {
        value = stack.Top.Value
        stack.Top = stack.Top.Next
        stack.Count--
    }
    return
}
func (stack *Stack) Size() int {
    return stack.Count
}

func Fibonacci(n int) int {
    if n == 0 {
        return 0
    }
    if n == 1 {
        return 1
    }

    //for n >= 2 the initial stack has the 0 and 1 values
    stack := Stack{}
    stack.Push(0)
    stack.Push(1)

    // loop from n to 2
    for n > 1 {
        //pop two top elements from stack
        term1 := stack.Pop().(int)
        term2 := stack.Pop().(int)

        //push back the first popped element, this will become term2 on the next iteration
        stack.Push(term1)

        //push the sum of the two elements, this will be the Top element on the next iteration
        stack.Push(term1 + term2)

        //move to the next iteration
        n--

    }

    //the top element in stack represents the last element in the Fibonacci sequence for a given n position
    return stack.Pop().(int)
}
func FibonacciTest() {
    fmt.Println("F0 -> 0", 0 == Fibonacci(0))
    fmt.Println("F1 -> 1", 1 == Fibonacci(1))
    fmt.Println("F2 -> 1", 1 == Fibonacci(2))
    fmt.Println("F3 -> 2", 2 == Fibonacci(3))
    fmt.Println("F4 -> 3", 3 == Fibonacci(4))
    fmt.Println("F5 -> 5", 5 == Fibonacci(5))
    fmt.Println("F6 -> 8", 8 == Fibonacci(6))
    fmt.Println("F7 -> 13", 13 == Fibonacci(7))
    fmt.Println("F8 -> 21", 21 == Fibonacci(8))
    fmt.Println("F9 -> 34", 34 == Fibonacci(9))
    fmt.Println("F20 -> 6765", 6765== Fibonacci(20))
}
func main() {
    FibonacciTest()

}

I'm new to Go language. I'm interested if this code can be improved or what I can add to be more like a Go style. I'm not very sure about the .(int) "conversion" and any explanation will be appreciated.

https://play.golang.org/p/q57CvDaqRV

I'm not convinced that the mechanism of using a stack is a good one. Why did you choose that data structure? In Go, a slice would be a much more appropriate structure, and also probably a lot more memory efficient. What you are looking for is a 2-element queue, not a 2-member stack.

The need for you to pop() term1 and then push it back again, is an indication that the stack is the wrong structure.

Consider a much simpler Fibonacci method, that does essentially the same logic as your method, but .... simpler.

func Fibonacci(n int) int {
    // seed the sequence with the next 2 numbers in the sequence
    fib := []int{0, 1}

    // advance the future 2 sequences as necessary.
    for i := 0; i < n; i++ {
        current := fib[0]
        next := fib[1]
        // remove the first member
        fib = fib[1:]
        // add the next member
        fib = append(fib, current+next)
    }

    return fib[0]
}

This uses a Go slice as a queue, shifting the value from the front of the queue, and adding another value to the end of the queue.

A trick is computing the two values "ahead" instead of the two values "before" the "current" item. This allows you to have simpler logic for the first and second results (but at the expense of calculating a single value you will never use).

See this running in the playground


Coderbyte: String Reduction From Coderbyte:

  Have the function StringReduction(str) take the str parameter being
  passed and return the smallest number you can get through the
  following reduction method. The method is: Only the letters a, b, and
  c will be given in str and you must take two different adjacent
  characters and replace it with the third. For example "ac" can be
  replaced with "b" but "aa" cannot be replaced with anything. This
  method is done repeatedly until the string cannot be further reduced,
  and the length of the resulting string is to be outputted. For
  example: if str is "cab", "ca" can be reduced to "b" and you get "bb"
  (you can also reduce it to "cc"). The reduction is done so the output
  should be 2. If str is "bcab", "bc" reduces to "a", so you have "aab",
  then "ab" reduces to "c", and the final string "ac" is reduced to "b"
  so the output should be 1.

Source ( passed all validations ):

var replacements =
{
  "ab" : "c",
  "ac" : "b",
  "bc" : "a",
  "ba" : "c",
  "ca" : "b",
  "cb" : "a"
};

function StringReduction(s)
{
  var original, key;
  while( s != original )
  {
   original = s;
   for( key in replacements )
      s = s.replace( key , replacements[key] );
  }
  return s.length;
}

// keep this function call here
// to see how to enter arguments in JavaScript scroll down
StringReduction(readline());

The funny part of this challenge is that you write against time, so the first version was a terrible, recursive approach. Since then I rewrote it a few times before posting to CR, thinking each time mistakenly that it was perfect, at this point I would like to get your feedback on it.

A few minor notes:

    Have the function StringReduction(str)

strictly speaking, the parameter name is different:

function StringReduction(s)

:-)

var original, key;

I'd put the variable declarations to separate lines. From Code Complete 2nd Edition, p759:

  With statements on their own lines, the code reads from top to bottom, instead
  of top to bottom and left to right. When you’re looking for a specific line of code,
  your eye should be able to follow the left margin of the code. It shouldn’t have to
  dip into each and every line just because a single line might contain two statements.

It could prevent some diff/merge conflicts (you modify the first variable, a coworker the second one at the same time).
key could be declared inside the while loop.

JavaScript variables declare outside or inside loop?
Effective Java, Second Edition, Item 45: Minimize the scope of local variables

As @konijn pointed out in his comment, that may confuse maintainers. Anyway, extracting it out into a separate function solves the issue (#6).
The name original could be lastString or lastValue since it's not the original string, it's the result of the last loop.
Indentation is not consistent. Most of the places it's two spaces but the while loop is only one space indented, the body of the for loop has three spaces.
I'd extract out the for loop to a string translate(string, replacements) function for better readability.


Retrieve random element from array not contained in second array I have a function, getUnusedName that returns a random name, but first checks if it's already used by any object within a second array.

Observing how many times the function is called across multiple runs shows a pretty wide variation as I would expect when using random. Is there a more efficient way of achieving this?

As well as a general code review, I'd also like to ask:

If possible, I'd like to avoid removing items from names, which is why I have chosen the random solution. Is this a bad choice? What are my alternatives?
Is there a cleaner way to check if a name is already in use? Using Array.filter then checking the resulting length seems a little verbose.

var names  = ['Steve', 'Adam', 'Phillip', 'Thomas', 'John', 'Joseph'];
var people = [];
var calls  = 0;

function getUnusedName() {
    calls++;

    var randomName = names[Math.floor(Math.random() * names.length)];

    if (people.filter(function (val) { return val.name === randomName; }).length === 0) {
        return randomName
    } else {
        return getUnusedName();
    }
}

for (var i = 0; i < names.length; i++) {
    var name = getUnusedName();

    people.push({
        name: name,
        age: (i * i) + 20,
    });
}

console.log('getUnusedName calls: ' + calls);
console.log(people);

The choice of the algorithm will depend on how many elements you have in your array, and how many of them you want to select. If you want to pull all names in random order, then shuffling is probably the best choice.

You can easily create a copy of names using names.slice() and shuffle the copy instead of the original array:

function randint(a, b) {
    return a + Math.floor(Math.random() * (b - a));
}

function shuffle(list) {
    list.forEach(function(value, i) {
        var j = randint(i, list.length);
        list[i] = list[j];
        list[j] = value;
    });
    return list;
}

var names  = ['Steve', 'Adam', 'Phillip', 'Thomas', 'John', 'Joseph'];
console.log(shuffle(names.slice()));

Alternatively, you can shuffle an array of numbers [0..N], and use them as indices to the names array:

function range(start, stop) {
    var list = [];
    for (var i = start; i < stop; i++) {
        list.push(i);
    }
    return list;
}

var indices = shuffle(range(0, names.length));

indices.forEach(function(i) {
    console.log(names[i]);
});

Shuffling has a much better time complexity than the original algorithm. For an array of 1000 names, the innermost loop of the original algorithm is executes ~6 million times, while the innermost loop of the shuffle algorithm is executed only 1000 times.


Depth-first search in Python I wrote this DFS in Python and I was wondering if it correct. Somehow it seems too simple to me.

Each node is a class with attributes:

visited: Boolean
sons: list of nodes linked to the current node.

This code should work both for directed and undirected graphs.

def DFS(node):

    node.visited = True

    if node == None:
        return

    // perform some operation on the node
    do_something(node)

    to_visit = [son in node.sons if not son.visited]
    for son in to_visit:
        DFS(node)

DFS should keep track of all the nodes visited. Not the node.

The node only properties is it self, and it's children.

Check this amazing implementation:

graph = {'A': set(['B', 'C']),
         'B': set(['A', 'D', 'E']),
         'C': set(['A', 'F']),
         'D': set(['B']),
         'E': set(['B', 'F']),
         'F': set(['C', 'E'])}

def dfs(graph, start):
    visited, stack = set(), [start]
    while stack:
        vertex = stack.pop()
        if vertex not in visited:
            visited.add(vertex)
            stack.extend(graph[vertex] - visited)
    return visited

dfs(graph, 'A') # {'E', 'D', 'F', 'A', 'C', 'B'}

As you can see DFS is called just once and it keeps track of visted nodes all the way, it will use pop(-1) so it will Depth-search, you could either change to pop(0), so it would Breadth-First Search. It's the same concept, the only difference is which node is visited in which order.

Your implementation will have some problems if the graph is too deep, because it could raise a Maximum recursion depth exceeded.


Determine if a word can be constructed from list of subsets The question is explained well by examples in the comments below. Also I request verifying complexity:  O( n * n!), where n is the number of words in the subsets. Review my code for optimizations, best practices etc.

/**
 * Question is better explained by examples:
 * 1. given
 *    i/p subsets  - ["un", "xy", "te", "i", "d"]
 *    and i/p string is : united should result in true.
 *    and i/p string of : union should result in false.
 *
 * 2. i/p subsets - ["tonyl", "pqr", "pqrbri",]
 *    and i/p string of : briton should be true.
 *    and i/p string of : japan should result in false.
 *
 * Complexity: O( n * n!) where n is numnber of subset words.
 *
 */
public final class StringFromSubSets {

    private StringFromSubSets() {}

    private static int getFactorial (int length) {
        int factorial = 1;
        for (int i = 1; i <= length; i++) {
            factorial = factorial * i;
        }
        return factorial;
    }

    private static void swapWithNext (List<String> subStrings, int i) {
        assert subStrings != null;
        assert i >= 0 && i < subStrings.size() - 1;

        String str = subStrings.get(i);
        subStrings.set(i, subStrings.get(i + 1));
        subStrings.set(i + 1, str);
    }

    private static String join (List<String> list) {
        assert list != null;

        final StringBuffer sb = new StringBuffer();
        for (String str : list) {
            sb.append(str);
        }
        return sb.toString();
    }

    private static Set<String> getPermutations(List<String> subStrings) {
        assert subStrings != null;

        final Set<String> set = new HashSet<String>();
        int factorial = getFactorial(subStrings.size());
        int counter = 0;

        while (counter < factorial) {
            for (int i = 0; i < subStrings.size() - 1; i++) {
                swapWithNext (subStrings, i);   // O(1)
                String joinedString = join (subStrings); // O(n)
                set.add(joinedString);
                counter++;
            }
        }
        return set;
    }

    public static boolean stringGotFromSubSets(List<String> subStrings, String wordToFind) {
        if (subStrings == null) {
            throw new NullPointerException("The substring provided is null.");
        }

        final Set<String> joinedStrings = getPermutations(subStrings);
        for (String joinedString : joinedStrings) {
            if (joinedString.contains(wordToFind)) return true;
        }

        return false;
    }

    public static void main(String[] args) {
        List<String> listOfString = new ArrayList<String>();
        listOfString.add("sachin");
        listOfString.add("tendulkar");
        listOfString.add("rahul");
        listOfString.add("dravid");
        System.out.println("Expected true, Actual " + stringGotFromSubSets(listOfString, "int"));
        System.out.println("Expected true, Actual " + stringGotFromSubSets(listOfString, "idsa"));
        System.out.println("Expected true, Actual " + stringGotFromSubSets(listOfString, "sachindravid"));

        System.out.println("Expected false, Actual " + stringGotFromSubSets(listOfString, "sehwag"));
        System.out.println("Expected false, Actual " + stringGotFromSubSets(listOfString, "ganguly"));
    }

}

Your getPermutations() fails to generate all permutations.  Consider…

public static void main(String[] args) {
    List<String> list = Arrays.asList(args);
    for (String s : getPermutations(list)) {
        System.out.println(s);
    }
}

If you run java StringFromSubSets 0 1 2 3 | sort, the result is:

0123
0132
0312
1023
1203
1230
2130
2301
2310
3012
3021
3201

There should be 4! = 24 permutations, but you have produced only 12.  For example, where is 0321?  In other words,

public static void main(String[] args) {
    // Simpler initialization than the original
    List<String> listOfString = Arrays.asList(new String[] {
        "sachin", "tendulkar", "rahul", "dravid"
    });
    System.out.println("Expected true, Actual " +
        stringGotFromSubSets(listOfString, "drahultendulkars"));
}

… prints Expected true, Actual false.


Does this function have to be nested? Recent JavaScript student here. I ran afoul of some weirdness with event listeners that led to my code being unresponsive. After some reading and tweaking, I arrived at a version that worked in JSFiddle, but not in the browser. I finally got it working in the browser, but now my code looks like this:

window.onload = function () {
    function bgnAddChllng() {
        alert("This is a test alert.");
    }
var addChllng = document.getElementById("addChllng");
addChllng.addEventListener('click', bgnAddChllng, false);
}

But the use of window.onload and nesting the actual useful code in an anonymous function seems bloated/hacky to me. What, if anything, can I do to clean it up?

Available on a JSFiddle here.

window.onload = function () {

Mixing DOM-0 event handlers and addEventListener is inconsistent, pick one and stick with it.

window.addEventListener("load", function () {

function bgnAddChllng() {

Don't disemvowel your variables. This isn't Wheel of Fortune, vowels don't cost anything.

function beginAdditionChallenge() {

var addChllng = document.getElementById("addChllng");

Use proper indentation. JSFiddle has a handy TidyUp button to do it for you. Consider a more descriptive ID for the button.

var button = document.getElementById("challengeButton");

addChllng.addEventListener('click', bgnAddChllng, false);

Switching between single and double quotes is inconsistent, pick one and stick with it. useCapture defaults to false, there's no need to explicitly pass it.

button.addEventListener("click", beginAdditionChallenge);

  nesting the actual useful code in an anonymous function seems bloated/hacky to me.

It's a pretty common practice, but if you don't like it, give the function a name and don't nest it, just as you did with the button click event handler.

function beginAdditionChallenge() {
    alert("This is a test alert.");
}

function initChallenge() {
    var button = document.getElementById("challengeButton");
    button.addEventListener("click", beginAdditionChallenge);
}

window.addEventListener("load", initChallenge);


Finding the maximum value in a binary tree I have to find the maximum value in a binary tree. This is how I did it iteratively:

int maxValue(Node* node)
{
    if (node == nullptr)
        throw "BT is empty";

    int max = node->data;
    for (Node* left = node; left != nullptr; left = left->left)
    {
        if (left->data > max)
            max = left->data;
    }

    for (Node* right = node; right != nullptr; right = right->right)
    {
        if (right->data > max)
            max = right->data;
    }

    return max;
}

I don't know if this is the best way to do it. How can this be improved? Is there a recursive solution?

Trees are often most useful when they're sorted. If the tree is sorted, you can just descend into the right side of the tree.

Since we're assuming an unsorted tree, we have to search the whole thing. Let's build this up by cases. First assume that the current node has the largest value:

int maxValue(Node *node)
{
    if (node == nullptr)
        throw "BT is empty";

    max = node->data;
    return max;
}

Nice, but not likely. We can do better. What if the largest value is in the left side of the tree?

int maxValue(Node *node)
{
    if (node == nullptr)
        throw "BT is empty";

    max = node->data;

    if(node->left != nullptr) {
        int leftMax = maxValue(node->left);
        if(max < leftMax)
            max = leftMax;
    }

    return max;
}

Great! Now we have a function that will check its left side for larger values, all the way down the left side. But what if the largest value is on the right of some node? We'd better cover that case too:

int maxValue(Node *node)
{
    if (node == nullptr)
        throw "BT is empty";

    int max = node->data;

    if(node->left != nullptr) {
        int leftMax = maxValue(node->left);
        if(max < leftMax)
            max = leftMax;
    }

    if(node->right != nullptr) {
        int rightMax = maxValue(node->right);
        if(max < rightMax)
            max = rightMax;
    }

    return max;
}

Now since we only have  to check for NULL that will throw on the first node we can optimize slightly:

int maxValue(Node *node)
{
    if (node == nullptr)
        throw "BT is empty";

    return maxValueNonNull(node, node->data);
}
int maxValueNonNull(Node* node, int currentMax)
{
    if (node == NULL)
    {    return currentMax;
    }

    currentMax = currentMax > node->data ? currentMax : node->data;

    int leftMax  = maxValueNonNull(node->left,  currentMax);
    int rightMax = maxValueNonNull(node->right, currentMax);

    return leftMax > rightMax ? leftMax : rightMax;
}

That should do it.


Implementation of oil-painting filter for images This is web exercise 3.1.14. from the book Computer Science An Interdisciplinary Approach by Sedgewick & Wayne:

Set pixel (i, j) to the color of the most frequent value among pixels
with Manhattan distance W of (i, j) in the original image.

Here is my program:
import java.awt.Color;

public class OilPaintingFilter
{
    public static int findNumberOfPixels(int distance)
    {
        int numberOfPixels = 4;
        if (distance == 1) return numberOfPixels;
        int increment = 8;
        for (int i = 2; i <= distance; i++)
        {
            numberOfPixels += increment;
            increment += 4;
        }
        return numberOfPixels;
    }
    public static Color[] findNeighboringColors(Picture picture, int col, int row, int distance)
    {
        int width = picture.width();
        int height = picture.height();
        Color[] pixels = new Color[findNumberOfPixels(distance)+1];
        int counter = 0;
        for (int j = col-distance; j <= col+distance; j++)
        {
            for (int i = row-distance; i <= row+distance; i++)
            {
                if ((Math.abs(col-j) + Math.abs(row-i)) <= distance)
                {
                    pixels[counter] = picture.get(Math.abs(j)%width,Math.abs(i)%height);
                    counter++;
                }
            }
        }
        return pixels;
    }
    public static Color findMostFrequentNeighbor(Color[] pixels)
    {
        int[] colors = new int[pixels.length];
        for (int i = 0; i < pixels.length; i++)
        {
            for (int j = 0; j < pixels.length; j++)
            {
                if (pixels[i].equals(pixels[j]))
                {
                    colors[i]++;
                }
            }
        }
        int max = 0;
        int index = 0;
        for (int i = 0; i < pixels.length; i++)
        {
            if (colors[i] > max)
            {
                max = colors[i];
                index = i;
            }
        }
        return pixels[index];
    }
    public static Picture applyPaintingFilter(Picture picture, int distance)
    {
        int width = picture.width();
        int height = picture.height();
        Picture newPicture = new Picture(width,height);
        for (int col = 0; col < width; col++)
        {
            for (int row = 0; row < height; row++)
            {
                Color newColor = findMostFrequentNeighbor(findNeighboringColors(picture, col, row, distance));
                newPicture.set(col,row,newColor);
            }
        }
        return newPicture;
    }
    public static void main(String[] args)
    {
        Picture picture = new Picture(args[0]);
        int distance = Integer.parseInt(args[1]);
        Picture newPicture = applyPaintingFilter(picture, distance);
        newPicture.show();
    }
}

Picture is a simple API written by the authors of the book. I checked my program and it works. Here is one instance of it:
Input (picture of a puffin taken from Pinterest):

with Manhattan distance = 10
Output:

Is there any way that I can improve my program?
Thanks for your attention.

Just my few cents...
There is no reason to treat distance == 1 differently:
    public static int findNumberOfPixels(int distance)
    {
        int numberOfPixels = 0;
        int increment = 4;
        for (int i = 1; i <= distance; i++)
        {
            numberOfPixels += increment;
            increment += 4;
        }
        return numberOfPixels;
    }

Furthermore, this method returns the same output for the same input every time. But you are calling it once for every pixel. It might be better to compute this value only once.


Implementation of stack using pointers Please review my code and let me know how I can possibly improve it.

#include<iostream>

using namespace std;

class node
{
    private:
                    int data;
    public:
                  node *next;
                    node(int element)
                    {
                            data=element;
                            next=NULL;
                    }
                    int getdata()
                    {
                        return(data);
                    }
};

class stack
{
    private:
                    node *start;
    public:
                    void push(int element);
                    int pop();
                    void traverse();
                    stack()
                    {
                        start=NULL;
                    }
};

inline void stack::push(int element)
{
    node *ptr;
    node *temp=start;
    ptr=new node(element);
    if(start==NULL)
    {
        start=ptr;
        ptr->next=NULL;

    }
    else
    {
            while(temp->next!=NULL)
            {
                temp=temp->next;
            }
            temp->next=ptr;
            ptr->next=NULL;
    }
}

inline int stack::pop()
{
    node *temp=start;
    node *top=start;
    while(temp->next!=NULL)
    {
        temp=temp->next;
    }
    while(top->next!=temp)
    {
        top=top->next;
    }
    top->next=NULL;
    int item=temp->getdata();
    delete (temp);
}

inline void stack::traverse()
{
    node *temp=start;
    while(temp!=NULL)
    {
        cout<<"data is"<<temp->getdata();
        temp=temp->next;
    }
}

int main()
{
  stack a;
    for(int i=0;i<10;i++)
    {
        a.push(i);
    }
    a.traverse();
    for(int i=0;i<5;i++)
    {
        a.pop();
    }
    a.traverse();
    return(0);
}

Please fix your indentation all over the header.  It should be consistent with everything else.
You don't need the return 0 at the end of main().  Reaching this point implies successful termination, and the compiler will just insert it for you.
In common stack implementations, pop() is void (merely pops off the top element).  This is useful as it'll be much easier to terminate the function early if the stack is empty.

For getting an element from the top, you would also have a top() function.  It will also have to account for an empty stack, but it would be best to check for an empty list first.
As @ChrisW has mentioned, the node class should be defined within the stack class as private.  Also, there shouldn't be anything public inside the node class.  If you put it within stack, its private data can still be accessed without an accessor.
A member function like getdata() should be const as it doesn't modify any data members.  You also don't need the () around data.

int getdata() const
{
    return data;
}

Moreover, you shouldn't have this function at all.  A basic node structure should just have a data field and a pointer to the next/previous node in the list.  As mentioned, anything containing this class will have access to its data.
Prefer an initializer list instead of the constructor assignments:

node(int element) : data(element), next(NULL) {}

One advantage to this is that if you have any const data members, you'll be able to initialize them within this list.  But with a constructor like yours, that won't work.
You must have a destructor for a data structure that utilizes manual memory allocation.  What happens if your structure object goes out of scope, but you haven't already popped every element?  You'll have a memory leak.

To prevent this, have the destructor traverse the list, using delete on each node.
You can make your structure more intuitive and safer with a empty() function:

bool empty() const { start == NULL; }

This can just be defined within the class declaration and should be public.

It is assumed that the list is empty if the head points to NULL, otherwise you can instead compare a size member to 0 (if you even want to bother with maintaining a size).


Does this Actor implementation has synchronization problems? As a part of a job test, I wrote following implementation of the Actor execution model:

import java.util.LinkedList;
import java.util.Queue;
import java.util.concurrent.Executor;
public abstract class Actor implements Runnable {
  private final Executor executor;

  /** current token */
  private Message message=null;
  /** rest of tokens */
  private Queue<Message> queue = new LinkedList<Message>();

  private boolean isFired=false;

  public Actor(Executor executor) {
    this.executor = executor;
  }

  /**
   * Frontend method which may be called from other Thread or Actor.
   * Saves the message and initiates Actor's execution.
   */
  protected void post(Message message) {
    synchronized(this) {
        if (isFired) {
            queue.add(message);
            return;
        }
        this.message=message;
        isFired=true; // to prevent multiple concurrent firings
    }
    executor.execute(this);
  }

  @Override
  public void run() {
    for (;;) {
        this.message.run();
        synchronized(this) {
            this.message = queue.poll();  // consume token
            if (this.message==null) {
                isFired=false; // allow firing
                return;
            }
        }
    }
  }

  protected abstract class Message {
    protected abstract void run();
 }
}

Reviewers concluded, in particular, that "Actor class have synchronization issues", but did not want to explain why.

Can you see any synchronization problems here?

The code, along with a test, can be downloaded from https://github.com/rfqu/CodeSamples/tree/master/src/simpleactor.

There is a problem.. You post() your message and it gets assigned in synchronized block, but it gets accessed in non-synchronized block. The field is not volatile therefore even though the reference to this.message might be visible to ThreadB, the inner state of the object might be still invisible. So message.run() may get you into a trouble.

In order to make things right this.message should be marked as volatile. Read about safe publishing in Java for more details.


Counting the elements of one array that are less than or equal to some other numbers I have a method which compares two array О(n2), maxes with each element in nums against <= condition then it prints out the new array with total count for each case, but it's too slow for large arrays, what's the most efficient way to solve this problem?

def counts(nums, maxes)
  total = 0
  total_array = []

  maxes.each do |m|
    nums.each do |n|
      if n <= m
        total += 1
      end
    end

    total_array.push(total)

    total = 0
  end

  total_array
end

nums = [2,10,5,4,8]
maxes = [3,1,7,8]

print counts(nums, maxes) => # [1,0,3,4]

# Explanation:

# 1. maxes[0] >= nums[0]
# 2. maxes[1] >= 0
# 3. maxes[2] >= nums[0], nums[2], nums[3]
# 4. maxes[3] >= nums[0], nums[2], nums[3], nums[4]

For your current code, the more Ruby-esque and functional way to write it would be:

def counts(numbers, maxima)
  maxima.map do |max|
    numbers.count { |n| n <= max }
  end
end

Don't mess around with "manual" counting and array-appending when you don't have to.

As for general improvements:

Sort the numbers. If you sort them, you won't have to loop through the whole array, you just have to count up to the first case where the comparison returns false. Or you can use Array#bsearch_index to find the "split point" in \$O(\log n)\$ time. Better yet, invert the condition and the index you find is also the count you want:

def counts(numbers, maxima)
  sorted = numbers.sort
  maxima.map do |max|
    index = sorted.bsearch_index { |n| n > max } # note inverted condition
    index || sorted.size
  end
end

The above works because it's using #bsearch_index in its "find minimum" mode (i.e. find the first index for which the block returns true), but since the condition is flipped it'll find the first number that shouldn't be counted. And that number's index equals the count of the elements before it. E.g. using your example input, what happens is:

The numbers get sorted [2, 10, 5, 4, 8] => [1, 4, 5, 8, 10]
For a given maximum, we do a binary search for the index
If the maximum is, say, 7, we'll find index 3 (value 8), since that's the first place where n > max (i.e. 8 > 7).
And since indices are zero-based, 3 is also the total/count we want.

You can also sort the maxima array from high to low. Start with the highest maximum, and filter the numbers array to only include those that fit the n <= max condition. The length of the filtered array is the first count. To find the next count, we only need to find a subset of the filtered array because a lower maximum will always result in a smaller array and a lower count. And so on for the next, smaller, maximum. It's a bit trickier, since you still need to output the totals in the original (unsorted) order.

def counts(numbers, maxima)
  # create an array of [value, index] tuples, sorted (descending) by value
  pairs = maxima.each_with_index.sort_by(&:first).reverse

  result = pairs.each_with_object({numbers: numbers, counts: []}) do |(max, index), memo|
    memo[:numbers].select! { |n| n <= max } # note in-place modification
    memo[:counts] << [memo[:numbers].size, index]
  end

  # re-sort the counts so they match the original order
  result[:counts].sort_by(&:last).map(&:first)
end

which could also be written as:

def counts(numbers, maxima)
  pairs = maxima.each_with_index.sort_by(&:first).reverse

  # create a copy, since we'll be modifying it
  search_space = numbers.dup

  counts = pairs.map do |max, index|
    search_space.select! { |n| n <= max } # note in-place modification
    [search_space.size, index]
  end

  counts.sort_by(&:last).map(&:first)
end

Or you can do everything: Sort both numbers and maxima, and use bsearch_index to figure out where to split the numbers array instead of select!. However, all that sorting and extra busywork might end up being slow.

def counts(numbers, maxima)
  pairs = maxima.each_with_index.sort_by(&:first).reverse
  search_space = numbers.sort

  result = pairs.each_with_object({search_space: search_space, counts: []}) do |(max, index), memo|
    count = memo[:search_space].bsearch_index { |n| n > max }
    count ||= memo[:search_space].size
    memo[:search_space].slice!(0, count)
    memo[:counts] << [count, index]
  end

  result[:counts].sort_by(&:last).map(&:first)
end

And of course, you can memoize the total for a given maximum, so if you encounter the same maximum twice or more, you can just recall a known count without re-searching an array.


Merging 3 sorted linked lists I already know how to merge two linked list, but I needed to code a function that'll merge three already sorted linked lists into one.

I wrote a code and it is working fine, it takes care of null lists and so. I just don't know if it's efficient enough. What do you think? How can I improve this code?

typedef struct clist {
int num;
struct clist *next;
} clist;

clist *cmerge(clist *c1, clist *c2, clist *c3) {
// Input parameters are pointers to the first items of the lists, out parameter is the pointer to the first item of the merged list
clist *ret = NULL, *curr = NULL, *last = NULL;
int i=1,n;
while(i) {
    i = 0;
    n = (c1) ? c1->num : (c2) ? c2->num : (c3) ? c3->num : 0;
    if(c1 && c1->num <= n) {
        n = c1->num;
        i = 1;
    }
    if(c2 && c2->num <= n) {
        n = c2->num;
        i = 2;
    }
    if(c3 && c3->num <= n) {
        n = c3->num;
        i = 3;
    }
    switch(i) {
    case 1:
        curr = c1;
        c1 = c1->next;
        break;
    case 2:
        curr = c2;
        c2 = c2->next;
        break;
    case 3:
        curr = c3;
        c3 = c3->next;
        break;
    default :
        curr = NULL;
    }
    if (!ret) ret = curr;
    else last->next = curr;
    last = curr;
}
return ret;
}

The code works as it is, but it uses a number of local variables that aren't actually needed.  An alternative approach might be this:

clist *cmerge(clist *c1, clist *c2, clist *c3) {
    clist *ret;
    clist *curr = NULL;
    while (1) {
        if (c3 && (!c2 || (c2 && c3->num < c2->num))) {
            clist *temp = c2;
            c2 = c3;
            c3 = temp;
        }
        if (c2 && (!c1 || (c1 && c2->num < c1->num))) {
            clist *temp = c1;
            c1 = c2;
            c2 = temp;
        }
        // use c1 as next node
        if (curr) {
            curr->next = c1;
            curr = curr->next;
        } else {
            ret = curr = c1;
        }
        if (c1) {
            c1 = c1->next;
        } else {
            return ret;
        }
    }
    return ret;   // never actually reached
}

The code works by arranging the three lists so that the smallest next number is always pointed to by c1.  That simplifies appending the node to the result list.

Another approach is to use the two-list merge twice.  That approach has the advantage that it could easily be adapted to be a variadic function taking any number of lists at a slight cost in efficiency.

Performance Testing

Results of time testing on two Linux boxes are shown below, demonstrating that this code runs measurably faster than the original.

First, here's the test harness code:

#include <stdio.h>
#include <stdarg.h>
#include <stdlib.h>
#include <string.h>
#include <assert.h>
#include <stdbool.h>
#include <time.h>
#include <math.h>

/*
 * the various implementations go here
 */

/* start at some number in (0,100) and increment by
 * some other number in (0,10) for count iterations
 * passed pointer is assumed to have count contiguous
 * nodes.
 */
clist *fill_random_list(int count, clist *curr) {
    clist *first = NULL, *last = NULL;
    assert(curr != NULL);
    int increment = rand() % 10;
    for (int n=rand() % 100 ; count; --count, ++curr) {
        curr->num = n;
        n += increment;
        curr->next = NULL;
        if (last == NULL) {
            first = last = curr;
        } else {
            last->next = curr;
            last = curr;
        }
    }
    return first;
}

// return true iff list is sorted
bool is_sorted(clist *list)
{
    if (list == NULL) {
        return true;
    }
    int prev = list->num;
    for (list = list->next; list; list = list->next) {
        if (prev > list->num) {
            return false;
        }
        prev = list->num;
    }
    return true;
}

int main()
{
    const size_t list_size = 5000000;
    srand(time(NULL));
    clist *one = malloc(list_size * sizeof(clist));
    clist *two = malloc(list_size * sizeof(clist));
    clist *three = malloc(list_size * sizeof(clist));
    clist *big;
    struct {
        const char *name;
        clist* (*func)(clist*, clist*, clist*);
        double elapsed;
    } tests[] = {
        { "original", cmerge_orig, 0 },
        { "Edward", cmerge_edward, 0 },
        { "mdfst13", cmerge_mdfst13, 0 },
        { NULL, NULL, 0}
    };

    for (int iterations = 100; iterations; --iterations) {
        for (size_t i=0; tests[i].func; ++i) {
            fill_random_list(list_size, one);
            assert(is_sorted(one));
            fill_random_list(list_size, two);
            assert(is_sorted(two));
            fill_random_list(list_size, three);
            assert(is_sorted(three));
            clock_t start = clock();
            big = tests[i].func(one, two, three);
            tests[i].elapsed += (double)(clock() - start)/CLOCKS_PER_SEC;
            assert(is_sorted(big));
        }
    }
    // print results
    for (size_t i = 0; tests[i].func; ++i) {
        printf("%12s\t%.10f\t%f%% %s than %s\n", tests[i].name, tests[i].elapsed,
            100.0*fabs(tests[i].elapsed-tests[0].elapsed)/tests[0].elapsed,
            (tests[i].elapsed > tests[0].elapsed ? "slower" : "faster"),
            tests[0].name
        );
    }
    free(one);
    free(two);
    free(three);
}

The code creates three sorted lists with random-ish values.  It uses assert() to rather gracelessly bail out on error (either a supposedly sorted list isn't or out of memory) and the timing doesn't account for possible rollover, but it works well enough for this purpose.

Merging 3 lists, each 5,000,000 items, cumulative time for 100 iterations

x86_64, 8 core, 3.4GHz, gcc 5.3.1

    original    2.3338780000    0.000000% faster than original
      Edward    2.2184270000    4.946745% faster than original
     mdfst13    4.1236940000    76.688499% slower than original

ARM v7, 4 core, 900MHz, gcc 4.6.3

    original    57.1399730000   0.000000% faster than original
      Edward    53.4537210000   6.451267% faster than original
     mdfst13    63.4887120000   11.110854% slower than original

Compile command:

  gcc -O2 -Wall -Wextra -pedantic -std=c99 mergetest.c -o mergetest


segment tree for adding a array with single index updates I have written a program to sum up a value between a range using segment tree and this tree also supports single index updates.
I am wondering if I have followed all the python rules for names and other pythonic nuances. Please let me know your comments.

Tree has below functions:

Modify - helps in modifying the individual array elements.
Query - given a range this function will return the sum of all the elements in the range. Such as given [3, 7) will sum up index 3 to 6 excluding 7.

def next_power_2(n):
    n -= 1
    n |= n >> 1
    n |= n >> 2
    n |= n >> 4
    n |= n >> 8
    n |= n >> 16
    n += 1
    return n

class segment_tree(object):
    def __init__(self, n, data):
        self.total_elements = n
        self.n = next_power_2(n) #segment tree is always balanced binary tree
        self.tree = [0] * self.n + data + [0] * (self.n - n)

    def build(self):
        for i in range(self.n-1, 0, -1):
            self.tree[i] = self.tree[2*i] + self.tree[2*i+1]

    def modify(self, pos, val):
        if pos < 0 or pos >= self.total_elements:
            print("check your inputs")
            return -1
        pos = pos + self.n
        self.tree[pos] = val
        while pos/2 > 0:
            self.tree[pos/2] = self.tree[pos] + self.tree[pos^1]
            pos = pos/2

    def query(self, left, right):
        if left > self.total_elements or left < 0:
            print("check your inputs")
            return -1
        if right > self.total_elements or right < 0:
            print("check your inputs")
            return -1
        left = self.n + left
        right = self.n + right
        s = 0
        while left < right:
            if left&1:
                s += self.tree[left]
                left += 1
            if right&1:
                right -= 1
                s += self.tree[right]
            left /= 2
            right /= 2
        return s

    def __repr__(self):
        return " ".join(str(i) for i in self.tree)

print("enter the number of elements")
n = int(raw_input())
print("enter the elements as space seperated integer values")
data = [int(i) for i in raw_input().split()]
st = segment_tree(n, data)
st.build()
print("enter the position and value which you want to modify as space seperated integer values")
pos, val = map(int, raw_input().split())
print("pos", pos, "value", val)
st.modify(pos, val)
print("enter the left and right range as space seperated integer values")
left, right = map(int, raw_input().split())
print("left", left, "right", right)
print("sum for the given range is:", st.query(left, right))
print(st)

I'd implement next_power_2 like this, using int.bit_length:

def next_power_2(n):
    "If n > 0, return smallest power of 2 greater than or equal to n."
    return 1 << (n - 1).bit_length()

This is more than twice as fast as than the version in the post, and it works for arbitrarily large integers.


Incremental Sieve of Eratosthenes using iterators I am in the process of learning Rust and am still having some issues with borrowing and ownership. I find myself trying to borrow mutable references that are already borrowed as immutable references etc...

I noticed that when I try to iterate over the primes without owning them, I encounter issues trying to modify the composites hash map. I was able to get around this by calling to_owned on the vector. Is this the right way to handle this?

I also see that when I build the code, I get warnings about unused code for warning: struct is never constructed: 'Sieve' and warning: method is never used: 'new', am I constructing them incorrectly?

use std::collections::HashMap;

struct Sieve {
    composites: HashMap<u64, Vec<u64>>,
    x: u64,
}

impl Sieve {
    fn new() -> Sieve {
        Sieve {
            composites: HashMap::new(),
            x: 2,
        }
    }
}

impl Iterator for Sieve {
    type Item = u64;

    fn next(&mut self) -> Option<u64> {
        let x = self.x;
        self.x = self.x + 1;
        match self.composites.get(&x) {
            Some(numbers) => {
                for _num in numbers.to_owned() {
                    self.composites
                        .entry(x + _num)
                        .and_modify(|v| v.push(_num))
                        .or_insert(vec![_num]);
                }
                self.composites.remove(&x);
                self.next()
            }
            None => {
                self.composites.insert(x * x, vec![x]);
                Some(x)
            }
        }
    }
}

fn main() {
    let mut sieve = Sieve::new();

    println!("{:?}", sieve.next()); // 2
    println!("{:?}", sieve.next()); // 3
    println!("{:?}", sieve.next()); // 5
    println!("{:?}", sieve.next()); // 7
}

Here is the code on the Rust playground.

I previously posted a version of the Sieve of Eratosthenes using experimental Rust features, this made it difficult to get feedback on. I have refactored the code to use iterators

Your code is totally fine and idiomatic, but I would change some minor points:

use std::collections::HashMap;

struct Sieve {
    composites: HashMap<u64, Vec<u64>>,
    current: u64,
}

impl Default for Sieve {
    fn default() -> Self {
        Sieve {
            composites: HashMap::new(),
            current: 2,
        }
    }
}

impl Sieve {
    pub fn new() -> Sieve {
        Default::default()
    }
}

impl Iterator for Sieve {
    type Item = u64;

    fn next(&mut self) -> Option<u64> {
        fn next_prime(composites: &mut HashMap<u64, Vec<u64>>, x: u64) -> u64 {
            match composites.get(&x) {
                Some(numbers) => {
                    for num in numbers.to_owned() {
                        composites
                            .entry(x + num)
                            .and_modify(|v| v.push(num))
                            .or_insert_with(|| vec![num]);
                    }
                    composites.remove(&x);
                    next_prime(composites, x + 1)
                }
                None => {
                    composites.insert(x * x, vec![x]);
                    x
                }
            }
        }

        let prime = next_prime(&mut self.composites, self.current);
        self.current = prime + 1; // This number will be the next to be tested

        Some(prime)
    }
}

fn main() {
    let mut sieve = Sieve::new();

    assert_eq!(sieve.next(), Some(2));
    assert_eq!(sieve.next(), Some(3));
    assert_eq!(sieve.next(), Some(5));
    assert_eq!(sieve.next(), Some(7));
}

It is good to implement Default when you can build your data structure without parameters (related answer).
This is a good practice to put assertions instead of prints. That's easier to change to code and verify that it is still ok.
The naming of the variables is important; but I do not pretend that mine is perfect, though. By the way, _num should not be prefixed with an underscore because it is used.
You can run clippy to catch some common error. Clippy warns you that you create a vec![_num] at each iteration. You should give to or_insert_with a closure that will build the correct vector.
The more important refactoring that I did is to make the algorithm more "functional". I think that yours is difficult to reason about because it calls recursively the Iterator::next method and rely on internal state.


Reading vertex data from a file I have a function that reads numbers from a file and constructs a Vector3. Can anyone think of how I can make improvements to this since I am new to C++?

The file has a list of numbers like so:

0.200153
-6.30333
-2.02161
etc...

The function look like this:

//uses 3 vectors to store x,y,z then combines them at the end to make the full PxVec3

void ModelLoader::LoadVertex(std::string fileLocation)
{
    //clears vector so it doesnt break when loading multiple models
    vertexArray.clear();

    std::ifstream file2;
    file2.open(fileLocation);

    if (!file2.is_open())
        std::cout << "Failed to load vertex data" << std::endl;

    //individual x,y,z values for the vertex arrays
    std::vector<float> x;
    std::vector<float> y;
    std::vector<float> z;

    int counter = 0;

    //grabs each line add places them into a vector to be used later
    while (!file2.eof())
    {
        if (counter == 3)
            counter = 0;

        std::string num;
        file2 >> num;

        switch (counter)
        {
        case 0:
            x.push_back(stof(num));  //converts string to float
            break;
        case 1:
            y.push_back(stof(num));
            break;
        case 2:
            z.push_back(stof(num));
            break;
        default:
            break;
        }
        counter++;
    }
    file2.close();

    //adds x,y,z values to the vector to complete to full PxVec3
    for (int i = 0; i < x.size(); i++)
        vertexArray.push_back(PhysicsEngine::PxVec3(x[i], y[i], z[i]));
}

Interface

The name LoadVertex(…) suggests that it's going to parse the string, rather than parse the file with the given filename.  I suggest LoadVertexFile(…) for clarity.

The parameter should be a const std::string&, to avoid unnecessarily copying the string.

You test whether file2.is_open(), but the code proceeds anyway if it isn't open.  The error message is printed to std::cout; I would recommend std::cerr, if you must print anything at all.  (Throwing an exception would be better.)  Furthermore, there is no way to programmatically tell whether the whole method succeeded.  What if the number of entries in the file is not a multiple of 3?

Implementation

I don't see any point in building three vectors x, y, and z, then transposing the results to populate vertexArray.  All you need is three temporary floats.  You don't need a counter.  Just read three entries at a time into those three floats.

You don't need to read a std::string and convert it using stof().  The iostream interface knows how to read floats.

You don't need to .close() the file explicitly; the ifstream's destructor takes care of that for you.

Suggested solution

bool ModelLoader::LoadVertexFile(const std::string& fileLocation)
{
    std::ifstream file(fileLocation);
    if (!file) {
        std::cerr << "Failed to load vertex data\n";
        return false;
    }

    float x, y, z;
    vertexArray.clear();
    while (file >> x >> y >> z)
    {
        vertexArray.push_back(PhysicsEngine::PxVec3(x, y, z));
    }
    return true;
}


Get a limit number to test all the prime numbers it contains in C My task was:

Write a program that accepts an integer as input and then displays all the prime
numbers smaller than or equal to that number.

And my code is:

#include <stdio.h>
void FindPrime(int number);

int main()
{
    int userInput, counter;
    printf("Please enter a limit number to test all the prime numbers it contains:\n");

    scanf("%d", &userInput);

    printf("The prime numbers %d contains are:\n", userInput);

    for (counter =  2; counter < userInput; counter++)
    {
        FindPrime(counter);
    }

    return 0;
}

void FindPrime(int number)
{
    int numberTest, nextNumber = 1;

    for (numberTest = 2; numberTest < number; numberTest++)
    {
        if (number%numberTest == 0 && numberTest < number)
            continue;
        else
            nextNumber++;
    }

    if (nextNumber == (number - 1))
        printf("%d ", number);
}

Can this code be improved?

Personally, I would put FindPrime above main, to avoid having to declare it.
Only have one variable declaration per line. With more than one, it becomes harder to see where a variable comes from. It doesn't really matter with a short program, but will for more complex ones.
Your print statements contain text which contradict what the task says (contains rather than less than)
userInput needs to be validated - Make sure that the user has entered a number, and it's greater than 0.
As mentioned, it's better that a function does one thing, rather than multiple. So your FindPrime function should be responsible for finding one, and the caller should be responsible for printing it.
In FindPrime, the if statement contains a condition that's already been tested for in the for loop, so you can simplify the if condition by removing it.
Always use {} for if (and while, for etc) statements, even if they're single line. Sooner or later, you'll add another line and break things, causing a very subtle error. And in this example, I'd reverse the condition, since the normal case doesn't do anything.

if (number%numberTest != 0 )
{
    nextNumber++;
}

With regards to the algorithm, it can certainly be improved, but that may be out of your reach at the moment.


Managing state in Clojure Where should you store state and how should it be managed in a ClojureScript application? Take the following code for example - it's a "game" where you travel either down or left.

(ns cljsfiddle)

; initialise the state
(set! (.-x js/window) 0)
(set! (.-y js/window) 0)

; state modifiers
(defn inc-x! [value] (set! (.-x js/window) (inc (.-x js/window))))
(defn inc-y! [value] (set! (.-y js/window) (inc (.-y js/window))))

(.addEventListener js/window "keyup"
    (fn [event]
      (let [key-code (.-which event)]
       (cond
        (= key-code 49) (inc-x!)
        (= key-code 50) (inc-y!)))
      (print (.-x js/window) (.-y js/window))))

I'm transitioning to ClojureScript from JavaScript and I realize this is written in a fairly JavaScript-y way. In addition, any advice on how to rewrite this in a more Clojure-esque way is welcomed.

State management is a pretty complex topic currently in the cljs-sphere. Not because it is so hard to do, but because people are still trying to figure out what the simplest approach to it might be. And this causes some very interesting solutions.

However, as you are new I will start with the simplest and most accepted one.
Use an atom.

;initialize state
(def xy-atom (atom {:x 0 :y 0}))

(defn inc-x! [i]
  (swap! xy-atom update-in [:x] + i))
(defn inc-y! [i]
  (swap! xy-atom update-in [:y] + i))

; or use a method to update any of the keys
(defn inc-any! [key i]
  (swap! xy-atom update-in [key] + i))

Your addEventListener function is ok, just change:

 (print (.-x js/window) (.-y js/window))

to

(print (:x @xy-atom) (:y @xy-atom)) ;or
(print @xy-atom)

If you're curious about more sophisticated approaches to handling state in cljs look here:

https://github.com/tonsky/datascript which mimics a database in the frontend
https://github.com/Day8/re-frame Which resembles a data flow approach and provides strict patterns for updates and event listeners to state.

Both of them are very interesting, just from the concept point of view. However, to get started, take it slow and play around with the atom thing.


Implement a Clock in JavaScript How idiomatic is my code?

var Clock = function(hour, minute) {
  this.hour = hour || 0;
  this.minute = minute || 0;

  this.plus = function(minutes) {
    computed_minutes = this.minute + minutes
    if (computed_minutes > 60) {
      this.minute = computed_minutes % 60
      this.hour += computed_minutes / 60
    } else {
      this.minute = computed_minutes
    }
    if (this.hour >= 24) {
      this.hour = this.hour - 24
    }
    this.hour = Math.round(this.hour)
    this.minute = Math.round(this.minute)
    return this;
  },

  this.minus = function(minutes) {
    computed_minutes = this.minute - minutes
    if (computed_minutes < 0) {
      this.minute = 60 + computed_minutes % 60
      this.hour -= (1 + Math.abs(computed_minutes / 60))
    } else {
      this.minute = computed_minutes;
    }
    if (this.hour < 0) {
      this.hour = 24 + this.hour
    }

    this.hour = Math.round(this.hour)
    this.minute = Math.round(this.minute)

    return this;
  },

  this.equals = function(other) {
    return this.hour == other.hour && this.minute == other.minute;
  }

}

Clock.at = function(hour, minute) {
  c = new Clock(hour, minute);
  return c;
}

Clock.prototype.toString = function() {
  function format(n) {
    return n >= 10 ? n : "0" + n;
  }
  return format(this.hour) + ":" + format(this.minute)
}

module.exports = Clock;

Some issues:

Every instance of Clock gets its own instance of plus and minus rather than attaching these functions to the prototype.
Clock.at sets a global variable c.
Clock.prototype.toString creates the format function each time it is called; you can move this out of the function and into a different scope so it is only created once.
plus and minus do a lot of checking that could be moved into a single set function and then this can also be called from the constructor (so that the same checks and rounding is also done in the constructor).
24 and 60 appear as magic constants. In this context it is easy to understand what they are but it would be better to assign them to named constants (HOURS_PER_DAY and MINUTES_PER_HOUR) that identify why those magic numbers are being used.

My suggestions:

module.export = (function(){
    var Clock = function( hours, minutes ) {
      this.set( hours, minutes )
    }

    Clock.at = function(hours, minutes) {
      return new Clock(hours, minutes);
    }

    Clock.HOURS_PER_DAY = 24;
    Clock.MINUTES_PER_HOUR = 60;

    Clock.prototype.set = function( hours, minutes ){
        var hrs = Math.round( hours || 0 );
        var mns = Math.round( minutes || 0 );

        this.hour = ( hrs + Math.floor( mns / Clock.MINUTES_PER_HOUR ) ) % Clock.HOURS_PER_DAY;
        if ( this.hour < 0 )
        {
            this.hour += Clock.HOURS_PER_DAY;
        }

        this.minute = mns % Clock.MINUTES_PER_HOUR;
        if ( this.minute < 0 )
        {
            this.minute += Clock.MINUTES_PER_HOUR;
        }

        return this;
    }

    Clock.prototype.plus = function(minutes) {
     return this.set( this.hour, this.minute + minutes )
    },

    Clock.prototype.minus = function(minutes) {
     return this.set( this.hour, this.minute - minutes );
    },

    Clock.prototype.equals = function(other) {
     return this.hour == other.hour && this.minute == other.minute;
    }

    function format(n) {
        return n >= 10 ? n : "0" + n;
    }

    Clock.prototype.toString = function() {
          return format(this.hour) + ":" + format(this.minute)
    }

    return Clock;
})();


Custom PHP MySQL database queries function - Suggestions for improvement or additions For my website needs I made a custom procedural MySQL function in PHP which can INSERT, UPDATE, SELECT or DELETE data from database. I would like a review and any suggestions for improvement or addition of new features.

function mysqli_database_query($sql, $values, $param = NULL, $result_param = NULL) {
    require "dbconn.script.php";
    // String to array conversion
    if (!is_array($values)) {
        $values_array = array($values);
    } else {
        $values_array = $values;
    }
    // Number of values checker and string of parameter types creation
    $number_of_values = count($values_array);
    if ($number_of_values > 1) {
        $param_types = implode("", array_map(function($val) { return gettype($val)[0]; }, $values_array));
    } else {
        $param_types = gettype($values_array[0])[0];
    }
    // SQL query execution
    $stmt = mysqli_stmt_init($conn);
    mysqli_stmt_prepare($stmt, $sql);
    mysqli_stmt_bind_param($stmt, $param_types, ...$values_array);
    mysqli_stmt_execute($stmt);
    // SQL query results management
    if (preg_match("/^SELECT.*/", $sql)) {
        $results = mysqli_stmt_get_result($stmt);
        # RETURN ASSOCIATIVE AND NUMERIC ARRAY COMBINED EVEN IF RESULT PARAMETER IS INVALID
        if ($result_param === NULL || $result_param !== "assoc" || $result_param !== "num" || $result_param !== "row") {
            return mysqli_fetch_array($results, MYSQLI_BOTH);
        # RETURN ASSOCIATIVE ARRAY
        } else if ($result_param === "assoc") {
            return mysqli_fetch_array($results, MYSQLI_ASSOC);
        # RETURN NUMERIC ARRAY
        } else if ($result_param === "num") {
            return mysqli_fetch_array($results, MYSQLI_NUM);
        # RETURN ONE ROW
        } else if ($result_param === "row") {
            if ($param === NULL || $param !== "id") {
                return mysqli_fetch_row($results);
            }
            # RETURN ONE ROW ID
            if ($param === "id") {
                $result = mysqli_fetch_row($results);
                return $result["id"];
            }
        }
    }
    if (preg_match("/^INSERT.*/", $sql)) {
        # RETURN LAST INSERTED ID
        if ($param === "insert_id") {
            return mysqli_insert_id($conn);
        # RETURN NUMBER OF AFFECTED ROWS
        } else if ($param === "affected_rows") {
            return mysqli_affected_rows($conn);
        } else {
            return false;
        }
    }
    if (preg_match("/^UPDATE.*/", $sql)) {
        # RETURN NUMBER OF AFFECTED ROWS
        if ($param === "affected_rows") {
            return mysqli_affected_rows($conn);
        } else {
            return false;
        }
    }
    if (preg_match("/^DELETE.*/", $sql)) {
        # RETURN NUMBER OF AFFECTED ROWS
        if ($param === "affected_rows") {
            return mysqli_affected_rows($conn);
        } else {
            return false;
        }
    }
};

This monster of a function is hard to use and hard to maintain. Besides, it is bloated with duplicated and/or unnecessary code. Last but not least, the way require "dbconn.script.php"; is called will make your code run slower and make some features (such as transactions) unavailable.
Too many connections
Your current code is connecting to the database every time it runs a query. You shouldn't do that. Always connect strictly  once and then pass a connection variable as a function parameter.
Useless regexp
You already control the return type buy means of $param variable. No need to check the query type manually. It is useless and error prone. What if your query is CALL? SET? REPLACE?
Passing values as array is not that hard.
With your function you have a choice, whether to pass $values as array or not. It makes the code hard to read. At the same time adding two square brackets to your $values is not a big deal. So just just make $values an array mandatory.
Too much tasks for a function
The first question I must ask, why only a single function to perform so many tasks? All right, I can make it you are not familiar with OOP and don't want to use a class (which would be the best solution of course). But why not to make a set of functions, each serving for its own purpose? Why following that horrible practice of procedural mysqli when you have to write a helluvalot of words every time you calling a simple function (mysqli_stmt_get_result($stmt) vs. $stmt->get_result())?
Here is your intended function call
$rows = mysqli_database_query($sql, $id, "affected_rows");

compare it with with a code which is shorter but more meaningful and readable at the same time:
$rows = mysqli_delete($sql, [$id]);

A function name should be meaningful and tell you what does this function do. It should be a function name, not a parameter.
In your place I would create a set of functions. Say, a basic one that just performs a query
function mysqli_database_query($conn, $sql, $values, $return_result = true)
{
    $types = $types ?: str_repeat("s", count($params));
    $stmt = $conn->prepare($sql);
    $stmt->bind_param($types, ...$params);
    $stmt->execute();
    return ($return_result) ? $stmt->get_result() : $stmt;
}

and then add some helper functions for various tasks
function mysqli_delete($conn, $sql, $values)
{
    $stmt = mysqli_database_query($conn, $sql, $values, false);
    $return $stmt->affected_rows();
}
function mysqli_assoc($conn, $sql, $values)
{
    $result = mysqli_database_query($conn, $sql, $values);
    $return $result->fetch_assoc();
}
function mysqli_all($conn, $sql, $values)
{
    $result = mysqli_database_query($conn, $sql, $values);
    $return $result->fetch_all(MYSQLI_ASSOC);
}

you can use the following  function instead of that awkward $param = "id" of yours, for any column, not only id. Just select the desired column, i.e. SELECT user_id FROM table ...:
function mysqli_cell($conn, $sql, $values)
{
    $result = mysqli_database_query($conn, $sql, $values);
    $row = $return $result->fetch_row();
    return ($row) ? $row[0] : false;
}


Find largest connected cells of value 1 or greater My goal is to find the largest connected "cell" of 1's in a matrix.

My approach has been to search the surrounding area of a cell that is not on a border and then if it hasn't been visited, and has a value of 1, increment the group.  I am absolutely sure that there are better, more recursive solutions out there, but I would greatly appreciate if someone would at least point me in the right direction.  I feel as though I'm getting closer.  Algorithms are still very new to me.

Example test case

[[0, 0, 1, 1],
 [0, 0, 1, 0],
 [0, 0, 1, 1],
 [0, 0, 0, 0]];

Answer is 5 because there are 5 connected 1s of the largest group

    <html>
<head>
</head>
<body>
<script type="text/javascript">
test = [[0, 0, 1, 1],
        [0, 0, 1, 0],
        [0, 0, 1, 1],
        [0, 0, 0, 0]];

// Answer should be 5

var Cell = function(x, y, val) {
    this.row = x;
    this.col = y;
    this.val = val;
    this.visited = false;
}

Cell.prototype.visit = function() {
    this.visited = true;
}

var CellMatrix = function(arr) {
    inst = [];
    for(i=0; i < arr[0].length; i++) {
        for(k=0; k < arr.length; k++) {
            inst.push(new Cell(i, k, arr[i][k]));
        }
    }
    this.length = inst.length;
    //console.log(inst);
    return inst;
}

function find(row, col, cells) {
    for(i=0; i < cells.length; i++) {
        cell = cells[i];
        if((cell.row == row) && (cell.col == col) && (!cell.visited))
            return cell;
        else {
            continue;
        }
    }
    return false;
}

function findLargestGrouping(cells, rows, cols) {
    console.log(cells);
    groups = []
    for(i=0; i < cells.length; i++) {
        cell = cells[i];
        // not a border cell
        if( (cell.row > 0) && (cell.row < (rows - 1))  )
        {
            if( (cell.col > 0) && (cell.col < (cols - 1)) ) {
                // console.log("cell at row " + cell.row + " col " + cell.col + " and val " + cell.val);
                groups.push(searchNeighbors(cell, cells));
            }
        }
        else { continue; }
    }
    return Math.max.apply(Math, groups);
}

function searchNeighbors(cell, matrix) {
    neighbors = [[cell.row - 1, cell.col - 1], // top left
                [cell.row - 1, cell.col], // top center
                [cell.row - 1, cell.col + 1], // top right
                [cell.row, cell.col - 1], // center left
                [cell.row, cell.col + 1], // center right
                [cell.row + 1, cell.col - 1], // bottom left
                [cell.row + 1, cell.col], // bottom center
                [cell.row + 1, cell.col + 1]];

    results = 0;
    for(i=0; i < neighbors.length; i++) {
        _row = neighbors[i][0];
        _col = neighbors[i][1];
        _cell = find(_row, _col, matrix);
        console.log("cell at row " + cell.row + " col " + cell.col + " and val " + cell.val);
        if(_cell) {
            if(!_cell.visited) {
                _cell.visit();
                results += 1;
            }
        }
    }
    return results;
}

window.onload = function() {
    matrix = new CellMatrix(test);
    results = findLargestGrouping(matrix, 4, 4);
    console.log(results);
}
</script>
</body>
</html>

You have a lot of global variables. Almost all of them, actually. I only see two var declarations, and those two are probably the ones that matter least (though all declarations matter - don't skip 'em).

You're lucky you're not calling functions with loops from inside other loops. Otherwise they'd both be using the same i variable, and things would get weird. Or rather, get broken.

So fix that first! Try running your code through jshint, for instance, to get some basic refactoring/bug checking.

Secondly, while I applaud striving for object-oriented JS, it seems overwrought in this case.

If the task is indeed to find cells with a 1 in them, and all other cells have a 0, then there's a shortcut to take. Since zero is false'y, and one is truth'y, they can pull double-duty as the visited flag: If a cell is zero, it's either been visited or is not worth visiting. In other words, we can ignore every cell that's false'y.

So if you just set cells to 0, false, or null along the way, they'll be ignored later on. Same effect as your visited, but without the need for instantiating objects.

As for recursion, that's definitely an option here. Here's one such implementation:

function largestGroup(matrix) {
  var counts = [0];

  // count connected cells, starting with cell i,j
  function countConnected(i, j) {
    if(matrix[i][j]) {
      matrix[i][j] = null;
      return neighborsOf(i, j).reduce(function (sum, coord) {
        return sum + countConnected(coord[0], coord[1]); // recursion
      }, 1);
    }
    return 0;
  }

  // get neighbor coordinates of i,j
  // (ignores those that are false'y)
  function neighborsOf(i, j) {
    return [
      [i-1, j], [i+1, j],     // up and down
      [i, j-1], [i, j+1],     // left and right
      [i-1, j-1], [i-1, j+1], // diagonally up
      [i+1, j-1], [i+1, j+1]  // diagonally down
    ].filter(function (coord) {
      // keep only those coordinates that match cells that
      // a) exist, and
      // b) contain something thruthy
      return matrix[coord[0]] && matrix[coord[0]][coord[1]];
    });
  }

  // copy matrix to avoid modifying the original
  matrix = matrix.slice().map(function (row) {
    return row.slice();
  });

  // look for cells with something in 'em, and
  // start counting connected cells from there
  for(var i = 0 ; i < matrix.length ; i++) {
    for(var j = 0 ; j < matrix[i].length ; j++) {
      if(matrix[i][j]) {
        counts.push(countConnected(i, j));
      }
    }
  }

  return Math.max.apply(null, counts);
}

It starts by copying the matrix, since it's going to be modifying it, and we don't want to modify the input - other code may hold references to it.

After that, it starts looping through its copy, looking for the first truth'y cell. It then calls countConnected with that cell's coordinates, which, in turn, calls itself on each neighboring cell that's of interest (i.e. not zero). Any cell that's visited gets set to null, which, like zero, is false'y. Thus visited cells are ignored.

Each truthy cell that's visited by countConnected will add 1 to the count; any false'y cells will add zero. And since cells are set to null the first time they're counted, they won't be counted twice.


Filtering out objects in a CollectionViewSource I hired a developer as a senior dev. He was the only one that successfully completed my coding test. Now that he's on the project, I'm reviewing some of his code and seeing constructs like this:

e.Accepted = true;

if ((_selectedDocumentCategoryFilter == null) || (_selectedDocumentCategoryFilter.PicklistStringId == 0))
{ }
else if (docToFilter.CategoryId == _selectedDocumentCategoryFilter.PicklistStringId)
{ }
else
{ e.Accepted = false; }

if ((_selectedDocumentTypeFilter == null) || (_selectedDocumentTypeFilter.PicklistStringId == 0))
{ }
else if (docToFilter.DocumentTypeId == _selectedDocumentTypeFilter.PicklistStringId)
{ }
else
{ e.Accepted = false; }

To my mind there are several issues with this, but I don't want to come down too hard if this is considered generally acceptable, these days. There is, of course, always more than one way to code a solution. I learned to code many years ago, so perhaps things have changed or I'm just set in my ways. But this just looks like awkward logic with the empty condition bodies and falling through to further logic even after it is determined that the item is not accepted.

I'd have approached it with something like this:

e.Accepted = false;

if ((_selectedDocumentCategoryFilter != null) &&                                  // If the doc category filter is populated,
    (_selectedDocumentCategoryFilter.PicklistStringId != 0) &&                    // and there is an actual value selected (i.e. not blank or All)
    (docToFilter.CategoryId != _selectedDocumentCategoryFilter.PicklistStringId)) // but the docToFIlter doesn't match the catagory,
  return;                                                                         // let's get out. No need for further checks.

if ((_selectedDocumentTypeFilter != null) &&                                      // If the doc type filter is populated,
    (_selectedDocumentTypeFilter.PicklistStringId != 0) &&                        // and there is an actual value selected (i.e. not blank or All)
    (docToFilter.DocumentTypeId != _selectedDocumentTypeFilter.PicklistStringId)) // but the docToFilter doesn't match the type,
  return;                                                                         // let's get out. No need for further checks.

e.Accepted = true;

What do you think? Should his code be considered acceptable?

Edit: I do appreciate the discussion, but the responses are going a little deeper than I expected. (Which is a good thing.) This was intended as just a quick check to see if folks agreed that the empty conditions were a strange way to implement, along with falling through to additional checks after final state determination. My approach was intended to address those elements, just as a comparison, not a complete re-write, as much context was left out in an attempt to keep it simple.

To add a bit of context, there is no future condition to be added in the empty braces. The logic is simply determining if the doc object conforms to the selected filters. A filter could be null or zero depending on the state of the UI and, in either case, we consider the filter as unselected.

I find neither approach is acceptable.

The first one with the empty blocks only confuses. It lacks comments so you can never be sure whether there is something missing or intentionally left empty.

The second solution is also a bad one because it concatenates multiple conditions in a single giant if without any hint what they are actually testing.

To me maintainable and self-documenting code looks different. This means that instead of writing comments you should create a helper variable for each condition. You then can put them inside the if. Now it is self-explanatory and anyone can easily understand it (provided you pick really good names for the variables).

var documentCategoryFilterExists = (_selectedDocumentCategoryFilter?.PicklistStringId != 0);
if (documentCategoryFilterExists)
{
    var docToFilterMatches = (docToFilter.CategoryId == _selectedDocumentCategoryFilter.PicklistStringId);
    if (!docToFilterMatches)
    {
        return;
    }
}

I'm not saying that these variable names are perfect but without knowing your exact use case there's not much you can come up with based on only a few lines of code. But I think you know what I mean.

I still find this nested ifs aren't pretty... but at least they are less misterious now.

But this is how refactoring usually works. You change your code a find a new pattern that you can replace with an ever more efficient code. So the next step could be replacing the magic 0 with a constant and use the ? null operator in conjuction with ?? coalesce:

const int emptyPicklistId = 0;

var picklistStringId = (_selectedDocumentCategoryFilter?.PicklistStringId ?? emptyPicklistId);
var docToFilterMatches = (docToFilter.CategoryId == picklistStringId);
if (!docToFilterMatches)
{
    return;
}


Passing Row Index of TableViewCell (custom) to another view controller I have been trying to achieve the below requirements to get them working.  Am I doing it the right way?

We have a table with customised Cell (say: firstViewController)
Customised Cell has a button, click event in this button, and carries row index value to another view controller.  In this view controller I will use this row index to access the data model from first view controller.

Annotation: Each row has values associated with unique Dictionary value in Array based on row Index.

Pseudo Code: (pasted only necessary code)

FirstViewController.h

 @interface firstViewController : UIViewController <UITableViewDataSource, UITableViewDelegate>

 @property NSMutableArray *MasterRecords;   //  Each element in Array is dictionary.
 @property (weak, nonatomic) IBOutlet UITableView *tblMasterTable;
 @end

FirstViewController.m

-(UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath
{

    static NSString *columnIdfer = @"items";
    StudentCell *cell = (StudentCell *)[tableView dequeueReusableCellWithIdentifier:columnIdfer];
    if ( cell == nil)
    {
        NSArray *nib = [[NSBundle mainBundle] loadNibNamed:@"StudentCell" owner:self options:nil];
        cell = [nib objectAtIndex:0];
    }

    if (indexPath.row < [self.MasterRecords count])
    {

        cell.iRecordIndex = indexPath.row;
    }

    return cell;

}

In the above code I set the iRecordIndex to IndexPath.row

StudentCell.h

@interface StudentCell : UITableViewCell
@property NSInteger iRecordIndex;
@property (weak, nonatomic) IBOutlet UIButton *btnSubmit;  // It pushes viewController.
@end

StudentCell.m

- (IBAction)btnSubmitPressed:(id)sender {

     NextViewController *sVC = [[NextViewController alloc]     initWithRecIndex:@"NextViewController" bundle:nil Index:self.iRecordIndex];

  // Pushing NextViewController to NavigationViewController.
}

NextViewController.h

@interface NextViewController : UIViewController <MKMapViewDelegate>
- (id)initWithRecIndex:(NSString *)nibNameOrNil bundle:(NSBundle *)nibBundleOrNil  Index:(NSInteger )iRecIndex;
@property NSInteger iRecordIndex;
@end

NextViewController.m

 - (id)initWithRecIndex:(NSString *)nibNameOrNil bundle:(NSBundle *)nibBundleOrNil  Index:(NSInteger )iRecIndex
{
      self = [super initWithNibName:nibNameOrNil bundle:nibBundleOrNil];
    if (self) {
        // Custom initialization
        self.iRecordIndex = iRecIndex;
    }

    return self;
}

-(void) viewWillAppear:(BOOL)animated
{
    //Get FistViewController.   say  ==> fvc.

    NSMutableArray *mRec = [fvc MasterRecords];
    NSMutableDictionary *mDict = [mRec objectAtIndex:self.iRecordIndex];

    NSLog (@" value1 is %@ ", [mDict valueForKey:@"value1"]);
    NSLog (@" value2 is %@ ", [mDict valueForKey:@"value2"]);

   // Using value1, and value2 to display MKMapView.
}

Is this the right approach, or should I improve it?

There are several problems with your code.

First, you should use more descriptive names for your instance variables/properties.

 @interface firstViewController : UIViewController <UITableViewDataSource, UITableViewDelegate>

Class names in Cocoa are conventionally named with a prefix in capitals followed by the name of the class in TitleCase.

 @property NSMutableArray *MasterRecords;   //  Each element in Array is dictionary.

Names of instance variables and properties should be in camelCase.

    static NSString *columnIdfer = @"items";
StudentCell *cell = (StudentCell *)[tableView dequeueReusableCellWithIdentifier:columnIdfer];
if ( cell == nil)
{
    NSArray *nib = [[NSBundle mainBundle] loadNibNamed:@"StudentCell" owner:self options:nil];
    cell = [nib objectAtIndex:0];
}

You should register the nib of your cell before using your table view. This eliminates the need for instantiating a cell yourself every time it's need, and also does away with if (cell == nil), as dequeueReusableCellWithIdentifier: will always return a valid cell, provided you set the table view up properly. Proper code with these things in mind would be:

- (void)viewDidLoad
{
    [super viewDidLoad];
    . . .
    [self.tblMasterTable registerNib:[UINib nibWithNibName:@"StudentCell" bundle:nil];
}

- (UITableViewCell *)tableView:(UITableView *)tableView cellForRowAtIndexPath:(NSIndexPath *)indexPath
{
    StudentCell *cell = (StudentCell *)[tableView dequeueReusableCellWithIdentifier:@"items"];
    . . .
    return cell;
}

- (IBAction)btnSubmitPressed:(id)sender {
    NextViewController *sVC = [[NextViewController alloc]     initWithRecIndex:@"NextViewController" bundle:nil Index:self.iRecordIndex];
    // Pushing NextViewController to NavigationViewController.
}

Following the MVC pattern, your view should know nothing about your controllers. You could reimplement the above using a view controller as the cell's delegate, passing a suitable identifier in a suitable method. The delegate view controller could then manage instantiating and presenting/pushing other view controllers.

- (id)initWithRecIndex:(NSString *)nibNameOrNil bundle:(NSBundle *)nibBundleOrNil  Index:(NSInteger )iRecIndex;

Several problems with this. You're specifying the return type as id, but I assume it always returns a NextViewController. As suggested in the comments, current practice is to use instancetype as the return type instead of id. For more information on id vs instancetype check this answer out.

You do not attempt to make a distinction between RecIndex and Index in the method name. The argument names do not match what the method name tells us. The implementation of this method doesn't help matters.

-(void) viewWillAppear:(BOOL)animated
{
//Get FistViewController.   say  ==> fvc.

NSMutableArray *mRec = [fvc MasterRecords];
NSMutableDictionary *mDict = [mRec objectAtIndex:self.iRecordIndex];

NSLog (@" value1 is %@ ", [mDict valueForKey:@"value1"]);
NSLog (@" value2 is %@ ", [mDict valueForKey:@"value2"]);

// Using value1, and value2 to display MKMapView.
}

Inconsistent spacing. Make up your mind about how you want to space out your method implementations/declarations and use that style consistently throughout your code.

CodeReview does not appreciate pseudo-code. Please post all relevant bits of code.

Again, instance variables are badly named. Use more descriptive names.

This view controller should not know much about it's presenting view controller.

It should declare properties for the information it needs to get from its presenting view controller. The presenting view controller can then set these properties up after instantiating but before presenting this view controller.

NextViewController *vc = . . .;
vc.mRec = . . .;

[self.navigationController pushViewController:vc animated:YES];

Or in the prepareForSegue: method if using storyboards.


Dockerfile for Laravel deployment I'm looking for reviews about my first dockfile. I don't want to develop bad habits.

The purpose is to quickly deploy my Laravel (5.4) application. I'm going to connect it with others database containers (mognodb and mysql).

I used Ubuntu as parent image because it's my development environment but I planned to change it to debian or alpine later.

# Use an official Ubuntu LTS as a parent image
FROM ubuntu:16.04

MAINTAINER name <email>

#================================= Dependencies ================================

# Install dependencies
RUN apt-get -qq update && \
    apt-get -qq install -y --no-install-recommends \
        apache2 \
        composer \
        curl \
        git \
        libapache2-mod-php7.0 \
        libssl-dev \
        libsslcommon2-dev \
        npm \
        php-curl \
        php-dev \
        php-mbstring \
        php-mysql \
        php-pear \
        php-xml \
        php-zip \
        php7.0 \
        phpunit \
        pkg-config \
        zip && \
    # Get repository for node 8 and install it
    curl -sL https://deb.nodesource.com/setup_8.x | bash && \
    apt-get -qq install -y nodejs && \
    # Remove useless package : curl
    apt-get autoremove --purge -y curl && \
    # Clean temporary apt data
    rm -rf /var/lib/apt/lists/*

# Install php dependencies
RUN pecl -q install mongodb

#================================= Php Settings ================================

# Enable Mongo driver
RUN echo "extension=mongodb.so" >> /etc/php/7.0/cli/php.ini && \
    echo "extension=mongodb.so" >> /etc/php/7.0/apache2/php.ini

#=============================== Apache Settings ===============================

# Set Apache environment variables
ENV APACHE_RUN_USER www-data
ENV APACHE_RUN_GROUP www-data
ENV APACHE_LOG_DIR /var/log/apache2
ENV APACHE_PID_FILE /var/run/apache2.pid
ENV APACHE_RUN_DIR /var/run/apache2
ENV APACHE_LOCK_DIR /var/lock/apache2

# Create Apache directories
RUN mkdir -p $APACHE_RUN_DIR $APACHE_LOCK_DIR $APACHE_LOG_DIR

# Enable 'mod_rewrite' for rewrite URL then remove 'index.php'
RUN a2enmod rewrite

# Copy Apache configuration file
COPY apache2.conf /etc/apache2/apache2.conf
COPY 000-default.conf /etc/apache2/sites-available/000-default.conf

#================================= App Download ================================

# Create App directory
RUN mkdir /var/www/app
WORKDIR /var/www/app

# Copy sources without version control
RUN git clone --branch=v2.0.1 \
    https://my_name:my_key@bitbucket.org/app/repository.git . \
    && find . -name ".git*" -type f -delete

# Add write access to storage and cache
RUN chmod -R a+w storage/ bootstrap/cache/

#========================== Download / Install Vendors =========================

# Download PHP vendors
# Clean cache
RUN composer -q install && \
    composer clear-cache

# Download CSS/JS vendors
# Compile required assets
# Clean cache and download
RUN npm -q install && \
    npm run production && \
    rm -rf node_modules && \
    npm cache clean --force

#================================= App Settings ================================

# Set .env file with relevent settings
RUN sed -i 's/APP_ENV=\S*/APP_ENV=production/' .env && \
    sed -i 's/APP_DEBUG=\S*/APP_DEBUG=false/' .env &&

#=================================== Cleanup ===================================

# Clean temporary Laravel data
RUN php artisan cache:clear && \
    php artisan view:clear && \
    php artisan config:cache

# Remove useless folders
RUN rm -rf /var/www/html

#================================ Run Container ================================

# Make port 80 available to the world outside this container
EXPOSE 80

# Run Apache in the background
ENTRYPOINT [ "/usr/sbin/apache2" ]
CMD ["-D","FOREGROUND"]

After building the image size is 550 MB.
And after run the container the application work like expected.

What is right, and wrong with this dockerfile? How could I optimise it?

I don't much about Docker, so I can't comment much on that.
I've read through the file and the steps seem to make sense.
I can comment on some details.

The comment here doesn't match what the code does,
because the find command will only delete files like .gitignore and .gitattributes, but the .git directory itself with the full history remains:

# Copy sources without version control
RUN git clone --branch=v2.0.1 \
    https://my_name:my_key@bitbucket.org/app/repository.git . \
    && find . -name ".git*" -type f -delete

If you don't need the version control history,
perhaps you can use a zip file release instead of cloning.
Or, you might want to change the find command to delete the .git directory too,
by dropping the -type f condition.

The sed commands here duplicate the variable names,
seem overcomplicated to use \S* instead of simply .*,
can be more strict to not perform the replacement in comment lines,
and can be combined into a single command:

RUN sed -i 's/APP_ENV=\S*/APP_ENV=production/' .env && \
    sed -i 's/APP_DEBUG=\S*/APP_DEBUG=false/' .env &&

Like this:

RUN sed -ie '/^APP_ENV=/s/=.*/=production/' -e '/^APP_DEBUG=/s/=.*/=false/' .env