How to convert my classes to Dependency injection? I am still learning to develop my skills in OOP.

It uses a combination of the factory and its real singletons?

As I did more research, I have realized this design pattern is bad because of global state and it is hard to test.

I can't figure how how to convert this OOP to Dependency injection?

Section Class

class Section
{
    // Array contains of instantiations of Section
    protected static $instance;

    // Validation  instance of validation
    public $validation;

    public $something;

    // instance method returns a new section instance.
    public static function instance($name = "default")
    {
        if ($exist = static::getInstance($name)) {
            return $exist;
        }

        static::$instance[$name] = new static($name);
        return static::$instance[$name];
    }

    // Return a specific instance
    public static function getInstance($instanceKey = null)
    {
        if (!isset(static::$instance[$instanceKey])) {
            return false;
        }
        return static::$instance[$instanceKey];
    }

    // Get Validation instance or create it
    public function validation()
    {
        if ($this->validation instanceof Validation) {
            return $this->validation;
        }

        if (empty($this->validation)) {
            $this->validation = Validation::instance($this);
        }

        return $this->validation;
    }

    public function add($something)
    {
        $this->something = $something;
    }

}

Validation Class (Just a random name for now)

class Validation
{
    // Related section instance
    public $section;

    protected function __construct($section)
    {
        $this->section = $section;
    }

    public static function instance($section = "default")
    {

        if (is_string($section)) {
            $section = Section::instance($section);
        }

        return new static($section);
    }

    public static function getInstance($instanceKey = null)
    {
        $temp = Section::getInstance($instanceKey);
        if (!$temp) {
            return false;
        }

        return $temp->validation();
    }

    // Alias for $this->section->add()
    public function add($name) {
        $this->section->add($name);
        return $this->section->something;
    }
}

Testing:

$section = Section::instance("Monkey");
$validation = $section->validation();
echo $validation->add("This is just a test");

First, a few minor comments:

Class properties should almost always be protected or private.  This forces you to use a known tested interface (the class methods) to interact with the state stored in the class.  The properties section, something and validation should have all been protected or private.

Your method add does not add, it sets.  add might be appropriate for concatenation (although I'd prefer append or prepend).  It is best used for real addition.  Your add implementation would have been better named set.

I have added to your example class to highlight the benefits of Dependency Injection.  I have used text and title in place of your something.

Validation Interface

The validation interface defines the way to check for validity.  If we need to check for validity and we are passed an object that implements this interface we can be sure that it will work.  By relying on an interface rather than a specific object we reduce our coupling from a specific object to any object that implements the interface.

interface IValidation
{
    public function isValid($str);
}

Section Class

The constructor accepts all of the parameters that the class needs (the dependencies are injected).  I have used an array in the constructor (as a personal preference to remove a required order of parameters and to use named parameters with the associative array).  Standard SPL exceptions are thrown if the validation objects do not match the Validation interface.

class Section
{
    // Text for the section.
    protected $text;

    // Validation for the text.
    protected $textValidation;

    /// Title for the section.
    protected $title;

    // Validation for the title.
    protected $titleValidation;

    public function __construct(Array $setup)
    {
        $setup += array('Text'             => '',
                        'Text_Validation'  => NULL,
                        'Title'            => '',
                        'Title_Validation' => NULL);

        if (!$setup['Text_Validation'] instanceof IValidation)
        {
            throw new \InvalidArgumentException(
                __METHOD__ . ' requires Text_Validator');
        }

        if (!$setup['Title_Validation'] instanceof IValidation)
        {
            throw new \InvalidArgumentException(
                __METHOD__ . ' requires Title_Validation');
        }

        $this->text            = $setup['Text'];
        $this->textValidation  = $setup['Text_Validation'];
        $this->title           = $setup['Title'];
        $this->titleValidation = $setup['Title_Validation'];
    }

    public function setText($text)
    {
        $this->text = $text;
    }

    public function setTitle($title)
    {
        $this->title = $title;
    }

    // Return whether the section is valid.
    public function isValid()
    {
        return $this->textValidation->isValid($this->text) &&
            $this->titleValidation->isValid($this->title);
    }
}

Validation Class

Notice how this class implements the interface.

class Validation implements IValidation
{
    // The regex for a valid match.
    protected $validMatch;

    public function __construct($validMatch)
    {
        $this->validMatch = $validMatch;
    }

    public function isValid($str)
    {
        return preg_match($this->validMatch, $str);
    }
}

Usage

We can see from the usage some of the benefits of dependency injection.  There is no hardcoded dependencies in any of the classes and we can glue the components together when they are used.  This allows us to define the exact type of validations that we require and pass them to the Section class with injection.

$alphanumUnderscores = new Validation('/^[[:alnum:]_]*$/');
$digits = new Validation('/^[[:digit:]]+$/');

$section = new Section(array('Text_Validation'  => $alphanumUnderscores,
                             'Title'            => 'InvalidTitle',
                             'Title_Validation' => $digits));

if (!$section->isValid())
{
    // The title should be digits.
    echo 'As expected the title is invalid.' . "\n";
}

$section->setTitle(9876);

if ($section->isValid())
{
    echo 'As expected the title is valid with digits.' . "\n";
}


Solution to 90 degree rotation of an NxN matrix The idea is picked up from here and the following is my PHP implementation of it. As it mentions, 90 degree matrix rotation basically shifts columns into rows. For example:

|1,2,3|     becomes      |7,4,1|
|4,5,6|   ----------->>  |8,5,2|
|7,8,9|                  |9,6,3|

Notice how row 1,2,3, after rotation has become column.The solution is built on this pattern.

Here's the code:

class MatrixRotator
{
    public static function rotate($matrix)
    {
        $result_arr=[];
        for($i=0; $i<count($matrix); $i++)
        {
            $inner_arr = [];
            for($j=0; $j<count($matrix[$i]); $j++)
            {
                $item = $matrix[$j][$i];
                array_unshift($inner_arr, $item);
            }
            array_push($result_arr, $inner_arr);
        }
        return $result_arr;
    }
}

print_r(MatrixRotator::rotate([[1,2,3],[4,5,6],[7,8,9]]));

Here's the output:

|7,4,1|
|8,5,2|
|9,6,3|

There's nothing really wrong with your class, but you could use array functions and foreach instead of for.

Also, calling your class MatrixRotator is a bit weird. A class is used to combine and isolate several methods (= functions) and properties (= variables) into a handy package. A rotate() method would probably be part of a set of methods which can modify the matrix. And when that is the case it would make sense to store the matrix data in the matrix class. This would then require a set and get method.

This would result in something like this:

class Matrix
{
  private $data = [];

  public function __construct($data)
  // accepts an array with matrix data and stores it
  {
    $this->data = $data;
  }

  public function rotateClockwise()
  // rotate a matrix once clockwise
  {
    // check that the matrix contains data
    if (isset($this->data[0]) && is_array($this->data[0]))
    {
      // perform the actual rotation
      $rotated = [];
      foreach (array_keys($this->data[0]) as $columnKey)
      {
        $rotated[] = array_reverse(array_column($this->data,$columnKey));
      }
      $this->data = $rotated;
    }
    // return this class so this method can be chained
    return $this;
  }

  public function getData()
  // returns the stored matrix
  {
    return $this->data;
  }
}

It can then be used like this:

// creation of a matrix
$matrix = new Matrix([[1,2,3],[4,5,6],[7,8,9]]);

// rotation and output
echo '
';
print_r($matrix->rotateClockwise()->getData());
echo '
';

It is now easy to put other matrix manipulation methods into this class. For instance, by leaving out the array_reverse() from rotateClockwise() you will get a rotateAntiClockwise() method. So let's do that:

class Matrix
{
  private $data = [];

  public function __construct($data)
  {
    $this->data = $data;
    if (!$this->isValid()) trigger_error("Matrix data is invalid.");
  }

  public function isValid()
  {
    if (!is_array($this->data)) return FALSE;
    foreach ($this->data as $row)
    {
      if (!is_array($row)) return FALSE;
    }
    return TRUE;
  }

  public function reverseEachRow()
  {
    if ($this->isValid())
    {
      foreach ($this->data as $rowKey => $row)
      {
        $this->data[$rowKey] = array_reverse($row);
      }
    }
    return $this;
  }

  public function rotateAntiClockwise()
  {
    if ($this->isValid() && isset($this->data[0]))
    {
      $rotated = [];
      foreach (array_keys($this->data[0]) as $columnKey)
      {
        $rotated[] = array_column($this->data,$columnKey);
      }
      $this->data = $rotated;
    }
    return $this;
  }

  public function rotateClockwise()
  {
    return $this->rotateAntiClockwise()
                ->reverseEachRow();
  }

  public function getData()
  {
    return $this->isValid() ? $this->data : FALSE;
  }

}

$matrix = new Matrix([[1,2,3],[4,5,6],[7,8,9]]);

echo '
';
print_r($matrix->rotateClockwise()->getData());
echo '
';

I left out all comments, to keep the code short, and I also centralized the validity check. This goes beyond your question, but shows you why you wouldn't want to call your class MatrixRotator, it is too specific.


Validation of Identification card fields (string and array) I am validating two variables of an Identification card (cedula in the code). The first one is a string and the second is an array.

My inputs are:

1- e.g. 3-728-2208
2- e.g. array('1' => '3-728-2208', '2' => '1-728-2208')

My method is :

public function validateId($cedula)
{
    $rules = ['cedula' => 'cedula'];

    $input = ['cedula' => $cedula];
    if(is_array($cedula))
    {
        $validator = $this->validateIdIsArray($cedula);
    } else {
        $validator = Validator::make($input, $rules);
    }

    return !($validator->fails());
}

public function validateIdIsArray($cedula)
{
    $rules = ['cedula' => 'cedula'];

    foreach ($cedula as $ids) {
        $input = ['cedula' => $ids];
        $validator = Validator::make($input, $rules);
    }

    return !($validator->fails());
}

My method from Rule:

public function passes($attribute, $value)
{
   return preg_match('/^((\d{1,2}|N|E|PE|\d{1,2}PI)-\d{1,5}-\d{1,6})$/u', $value);
}

Your code looks good, but I notice that you are a Portuguese speaker (as I am), so my only advice would be: Code in English.

There is a good article about idioms and programming called "Why you shouldn’t code in Spanish". In the article the author even uses PHP in the scripts as examples.

Like the old saying:

  "Always code as if the person who ends up maintaining your code is a
  violent psychopath who knows where you live."

We should code our code in a way that the majority of programmers will understand, so in a way we should (almost always) code in english.


Including file based on user input: Efficiency and Security Currently I'm using this code:

<?php
/*
Clean, Simple, Minimal.
*/
$pages = array('home', 'login', 'register', 'logout', 'test');
if (isset($_GET['p']))
{
    $page = $_GET['p'];
    if (in_array($page, $pages))
    {
       include('pages/' . $page . '.php');
    }
}
 else {
    echo 'Hi';
}
?>

I want this to be as secure and efficient as possible, I've seen multiple versions of this. I wasn't sure how to do what i really wanted, I wanted it so that if the requested file .php exists in /pages/ it'll show it, and else throw an error. But last time i did that I was vulnerable to LFI.

Assuming nobody but you can place files in the directory, this will be safe because you are checking the input page against a pre-defined list of page files. Additionally, this code makes the white list dynamic and throws an exception if the page is not found.

As @tim said in the comment, he found this code to be about 5 times slower then yours. So that's good to keep in mind if you ever decide to use this.

Every .php page you put in the pages/ directory will automatically be callable.

if (isset($_GET['p'])) {
    $page  = $_GET['p'] . '.php';
    $pages = array_map('basename', glob('pages/*.php'));

    if (in_array($page, $pages)) {
        include('pages/' . $page);
    } else {
        throw new Exception('Page "' . $page . '" does not exist.');
    }
} else {
    echo 'Hi';
}


Optimize the split of a String from console I have to read a String from console, so I used the tokenizer to split it and add each token in a position on arrayList, is there a way to improve the operation of split?

while ((currentLine = bufferedReader.readLine()) != null ) {
            StringTokenizer string = new StringTokenizer(currentLine, " ");
            while (string.hasMoreTokens()) {
            arrayList.add(string.nextToken());
            }
}

This is probably not your bottleneck, but you could achieve the same result by:

ditching the StringTokenizer in favour of String.split()
replacing the while loop with a call to Collections.addAll(T...)

Like so:
while ((currentLine = bufferedReader.readLine()) != null) {
    Collections.addAll(arrayList, currentLine.split(" ")));
}

The static method Collections.addAll() is preferable to arrayList.addAll(Arrays.asList()):

public static <T> boolean addAll(Collection<? super T> c, T... elements)
Adds all of the specified elements to the specified collection. Elements to be added may be specified individually or as an array. The behavior of this convenience method is identical to that of c.addAll(Arrays.asList(elements)), but this method is likely to run significantly faster under most implementations.


In-order Predecessor BST review I was referred here from StackOverflow, so here is what I posted over there:

So, for an assignment, I have to construct a binary search tree, plus an assortment of related functions. I have them all working, but I'm looking for a way to optimize my way of finding in-order predecessors and successors. Here's my code for finding the predecessor (successor is, of course, nearly identical):

public double findPredecessor(double num) {
        if(this.has(num) == true) {
            node temp = this.root;
            node iterator = this.root;
            node returner = null;

            while(temp.key != num) { //match the passed "num" to a key
                if(num < temp.key) {
                    temp = temp.left;
                } else {
                    temp = temp.right;
                }
            }

            if(temp.left != null) {
                return findMaxInternal(temp.left);
            }

            while(iterator != null) {
                if(temp.key < iterator.key) {
                    iterator = iterator.left;
                }
                else if(temp.key > iterator.key) {
                    returner = iterator;
                    iterator = iterator.right;
                }
                else break;
            }
            return returner.key;
        }
        else return Double.NaN;
    }

Yes, it is part of the assignment that my function take a double as argument, and not a node. I'm looking to see if there is a way that I can do this in one loop, to cut a bit on running time. I know that two successive, non-nested loops still is O(n), but still.

CodeReview is a good place for this question....

There are a few things that you should consider...

Style

node is a bad name for a Java class. Standards for Java are that Class names are CamelCase, and your node class should be called Node.

Using == to compare double values is often a cause of bugs. In this case I believe it is 'safe', but you should be aware that it is, in general, a 'bad idea'.

It is typical, though not required, that when you have an if {.....; return ...} block that you do not have an else block. For example, you have:

if (this.hasNum(...)) {
    .....
    return returner.key;
} else {
    return Double.NaN;
}

In this case the else block is not necessary, and it could simply be:

if (this.hasNum(...)) {
    .....
    return returner.key;
}
return Double.NaN;

Otherwise, the style is good.

Logic

You actually loop through the tree three times. I presume hasNum(...) also walks the tree.

With problems like this you can reduce the problem to a single walk of the Tree.

node match = null;
node current = root;
node predecessor = null;
// walk the tree until we find our exact match....
// Our predecessor node will follow us whenever we take a right branch....
while (current != null) {
    if (current.key == num) {
        match = current;
        break;
    }
    if (num < current.key) {
        current = current.left;
    } else {
        predecessor = current;
        current = current.right;
    }
}
if (match == null) {
    return Double.NaN;
}
if (match.left != null) {
    predecessor = match.left;
    while (predecessor.right != null) {
        predecessor = predecessor.right;
    }
}
if (predecessor == null) {
    return Double.NaN;
}
return predecessor.key;


Read text file filled with numbers and tranfer to an array This is my ProcessDataFileParallel.Java file. I'm taking numbers from a .txt file and putting it into an array in Java. While it works, I would like to improve the code and possibly change some algorithms to better alternatives.

import java.io.BufferedReader;
import java.io.FileReader;
import java.io.IOException;
import java.util.Scanner;

public class ProcessDataFileParallel {

public static void main(String[] args) throws IOException {
    int[] array = new int[100000];
    int index = 0;
    Scanner inputFile = null;
    Scanner scan = new Scanner(System.in);
    boolean running = false;

    String emptyString;
    FileReader file = new FileReader("dataset529.txt");
    BufferedReader br = new BufferedReader(file);
    emptyString = br.readLine();

    try {
        while ((emptyString = br.readLine()) != null) {
            array[index++] = Integer.parseInt(emptyString);

        }
    } finally {
        if (br.readLine() == null)
            br.close();
    }

    inputFile = new Scanner(file);

// Read File from Dataset529.txt
    if(inputFile != null){
        System.out.println("Number of integers in: "  + file);

        try{
            while (inputFile.hasNext() && inputFile.hasNextInt()){
                    array[index] = inputFile.nextInt();
                    index++;
                    inputFile.next();
                }

        }finally{
            inputFile.close();
        }

        // Print dataset529.txt to Array in Java with For Loop below.
        for (int ai = 0; ai < index; ai++){
            System.out.println("Array Index: " + "(" + ai + ")" + "== >" + " " + array[ai]);
        }

    System.out.println("How many Threads?");  // Scanner -  Ask user how many threads

    int n = scan.nextInt();  // Create variable 'n'  to handle whatever integer the user specifies.  nextInt() is used for the scanner to expect and Int.

    Thread[] myThreads = new Thread[n]; // Variable n placed here to determine how many Threads user wanted
    Worker[] workers = new Worker[n]; // Variable N placed here - the number user specified will determine # of workers.

    int range = array.length/n;  //divides the array into n threads (what scanner specifies)
    for (index = 0; index < n; index ++){
        int start = index*range;
        int end = start + range;
        workers[index] = new Worker(start,end, array);

    }

    for( index = 0; index < n; index++){
        myThreads[index] = new Thread(workers[index]);
        myThreads[index].start();
    }

    if (running){
         for (Thread t : myThreads) {
             if (t.isAlive()) {
                 running = true;
                 break;
             }
         }
     } while (running);

     for (Worker worker : workers) {
         System.out.println("Max = " + worker.getMax());
        }
     }

}
}

Worker class:

public class Worker implements Runnable {

            private int start;
            private int end;
            private int randomNumbers[];

            int max = Integer.MIN_VALUE;

            public Worker(int start, int end, int[] randomNumbers) {
                this.start = start;
                this.end = end;
                this.randomNumbers = randomNumbers;
            }

            public void run() {
                for (int index = start; index < end; index++) {

                    if (randomNumbers[index] >max)
                        max = randomNumbers[index];
                }
            }

            public int getMax() {
                return max;
            }

    }

Is there any way I can improve/condense my code?  Also, is there any way to possibly do alternatives that would be better?

Bug

If the array length is not a multiplier of the number of threads, you'll miss the remainder! Quick example:

public static void printRange(int length, int n) {
    int range = length / n;
    for (int index = 0; index < n; index ++){
        int start = index * range;
        int end = start + range;
        System.out.printf("[%s, %s)%n", start, end);
    }
}

// Output for printRange(10, 3):
[0, 3)
[3, 6)
[6, 9)

// Output for printRange(10, 6):
[0, 1)
[1, 2)
[2, 3)
[3, 4)
[4, 5)
[5, 6)

As you can see, you'll miss the 10th element for printRange(10, 3), and up to half the array for printRange(10, 6).

edit: A simplistic approach is to spin a new Thread to handle the remaining elements, but as you can see from the second example, you will end up a relatively high number of five elements for it, whereas the first five threads are only processing (and returning) one element. An alternative approach is to add an extra element per Thread so that you have more threads doing slightly more work, than one single thread doing most of the work.

Multi-threading

The preferred way of doing multi-threading since Java 5 is to use an ExecutorService to help you manage the lifecycle of threads. You should read up more on Oracle's tutorial to understand how they are used.

In addition, since you want each thread to compute and return a result for you, you should be looking at the Callable interface. Implementations override the call() method to return an appropriate result.

try-with-resources

If you are on Java 7 and above, you should use try-with-resources for efficient handling of the underlying I/O resource:

public static void main(String[] args) {
    try (Scanner scanner = new Scanner(System.in)) {
        // ...
    }
}

Hard-coding array length

int[] array = new int[100000];

What happens if your file has more than this number of elements? Actually, you also seem to be processing your file twice, once using the BufferedReader approach, and the second using a Scanner. Is this expected?

edit:

Getting the max value

Your current implementation forgets to actually compare the max of each partition with each other to arrive at the desired result.

  Is this meant to be more like an academic exercise on multi-threading? - myself

The reason why I'm asking is that while it looks like you're looking more at a map-reduce approach to your problem, it may not even be necessary in the first place. Processing 100,000 integers (based on your int[] declaration) or less in a single thread is relatively fast enough on any modern hardware capable of running the JVM. If you are however trying to compare billions of numbers and/or given a (strangely low) memory constraint, then that's when multi-threading will help. However, your question will be quite different at that stage.

Getting the max value (part 2)

From Java 8 onwards, there are two approaches of running tasks asynchronously:

ExecutorService to create Futures, and then getting the results from each of them.
Chaining CompletableFutures (Java 8 onwards).

The code below demonstrates both:

public class AsyncMax {

    private static final int MAX = 10;

    public static void main(String[] args) {
        System.out.println("Using ExecutorService:");
        ExecutorService service = Executors.newWorkStealingPool();
        List<Future<Integer>> esResults = IntStream.range(0, MAX).parallel()
                .mapToObj(i -> service.submit(() -> produce(i).get()))
                .collect(Collectors.toList());
        esResults.stream()
                .map(f -> get(f))
                .max(Comparator.naturalOrder())
                .ifPresent(System.out::println);
        service.shutdown();
        System.out.println("Using CompletableFuture:");
        CompletableFuture<List<Integer>> cfResults = IntStream.range(0, MAX).parallel()
                .mapToObj(i -> produce(i))
                .map(CompletableFuture::supplyAsync)
                .collect(allOf());
        cfResults.thenApply(results -> results.stream().max(Comparator.naturalOrder()))
                .join()
                .ifPresent(System.out::println);
    }
}

For both approaches, IntStream.range(0, MAX).parallel() is used to parallelize the production of MAX integers, via the produce(int) method (shown below). The intermediary esResults and cfResults variables are created solely to highlight where the asynchronous tasks will 'end', to be followed by how the results are collected. One can certainly daisy-chain the method calls completely.

ExecutorService:

Start an ExecutorService implementation, the Java 8 newWorkStealingPool() is an example here.
submit() a Callable<Integer> that returns an Integer upon completion.
Collect the results to a List<Future<Integer>>, i.e. a List of Futures that return Integers.
Stream the List by get()-ting (method shown below) each resulting Integer.
From the Stream<Integer>, call max(Comparator.naturalOrder()) to get the maximum value, and then print it via ifPresent(Consumer).
Last, shutdown() the ExecutorService.

CompletableFuture:

Pass each Supplier<Integer> from produce(int) to CompletableFuture.supplyAsync(Supplier).
Collect each CompletableFuture<Integer> into a resulting CompletableFuture<List<Integer>> via a custom allOf() Collector (method shown below).
From that CompletableFuture, thenApply() it to a Function<List<Integer>, Integer> that derives the maximum value from the List<Integer>.
join() to get the result and print it via ifPresent(Consumer).

Supporting methods:

This produces the given int after sleep()-ing for some time:

private static Supplier<Integer> produce(int i) {
    return () -> {
        try {
            Thread.sleep((int) ((1 + Math.random()) * (MAX - i) * 500));
        } catch (InterruptedException e) {
            Thread.currentThread().interrupt();
            throw new RuntimeException(e);
        }
        System.out.printf("Returning [%s] from Thread[%s]%n",
                i, Thread.currentThread().getName());
        return i;
    };
}

This is a Stream-friendly way of get()-ing from a Future, by catching checked Exceptions first:

private static <T> T get(Future<T> future) {
    try {
        return future.get();
    } catch (InterruptedException | ExecutionException e) {
        Thread.currentThread().interrupt();
        throw new RuntimeException(e);
    }
}

This is a Collector implementation for waiting on all CompletableFutures to finish, before returning the List of results (inspired by this article):

private static <T> Collector<CompletableFuture<T>, ?,
        CompletableFuture<List<T>>> allOf() {
    return Collector.of(ArrayList<CompletableFuture<T>>::new,
            (a, t) -> a.add(t), (a, b) -> { a.addAll(b); return a; },
            a -> CompletableFuture.allOf(
                        a.toArray(new CompletableFuture[a.size()]))
                    .thenApply(v -> a.stream().map(CompletableFuture::join)
                            .collect(Collectors.toList())));
}

Conclusion:

Instead of thinking solely on having to synchronize write access on a resulting array, both the ExecutorService or CompletableFuture approaches are more declarative, and IMO the latter more so that the former. This is achieved by letting us express how to generate our asynchronous tasks, followed by what needs to be done after gathering the results. :)


Finding paths through matrix Given the problem:

  You are given a 2-Dimensional array with M rows and N columns. You are
  initially positioned at (0,0) which is the top-left cell in the array.
  You are allowed to move either right or downwards. The array is filled
  with 1's and 0's. A 1 indicates that you can move through that cell, a
  0 indicates that you cannot move through the cell. Return the number
  of paths from the top-left cell to the bottom-right cell (i.e. (0,0)
  to (M-1,N-1)).

And my implementation:

#include <iostream>

#define ROW M // Fill this in
#define COL N // Fill this in

bool pathCount(int (&matrix)[ROW][COL], int M, int N, int m, int n, int &paths)
{
  std::cout << "@ (" << m << ", " << n << ")" << std::endl;
  if (m == M - 1 && n == N - 1 && matrix[m][n]) {
      ++paths;
      return false;
  } else if (m == M || n == N || !matrix[m][n]) {
      return false;
  }

  while (pathCount(matrix, N, M, m, n + 1, paths));
  while (pathCount(matrix, N, M, m + 1, n, paths));

  return false;
}

int pathCount(int (&matrix)[ROW][COL], int M, int N)
{
  int paths = 0;
  pathCount(matrix, M, N, 0, 0, paths);
  return paths;
}

Are there any improvements or potential bugs in the implementation? I ran it through a few tests. In terms of performance I am seeing this as having worst-case complexity of \$O(n + m)\$ which is the complexity of depth first search. Is that correct?

The complexity of your approach is actually
$$\mathcal{O}\Big((N + M)\frac{(N + M - 2)!}{(N-1)!(M-1)!}\Big),$$ but using a dynamic programming algorithm you can drop it to \$\Theta(MN)\$:

#include <iostream>
#include <chrono>

#define ROW 15
#define COL 15

void pathCount(int (&matrix)[ROW][COL], int M, int N, int m, int n, int &paths) {
    if (m == M - 1 && n == N - 1 && matrix[m][n]) {
        ++paths;
        return;
    }

    if (m == M || n == N || !matrix[m][n]) {
        return;
    }

    pathCount(matrix, N, M, m, n + 1, paths);
    pathCount(matrix, N, M, m + 1, n, paths);
}

int pathCount(int (&matrix)[ROW][COL], int M, int N) {
    int paths = 0;
    pathCount(matrix, M, N, 0, 0, paths);
    return paths;
}

int pathCountV2(int (&matrix)[ROW][COL], int M, int N) {
    if (!matrix[0][0])
    {
        // The starting cell is not traversable.
        return 0;
    }

    int aux[ROW][COL];

    bool zero_encountered = false;

    for (int y = 0; y < ROW; ++y)
    {
        if (zero_encountered)
        {
            aux[y][0] = 0;
        }
        else if (!matrix[y][0])
        {
            aux[y][0] = 0;
            zero_encountered = true;
        }
        else
        {
            aux[y][0] = 1;
        }
    }

    zero_encountered = false;

    for (int x = 1; x < COL; ++x)
    {
        if (zero_encountered)
        {
            aux[0][x] = 0;
        }
        else if (!matrix[0][x])
        {
            aux[0][x] = 0;
            zero_encountered = true;
        }
        else
        {
            aux[0][x] = 1;
        }
    }

    for (int y = 1; y < ROW; ++y)
    {
        for (int x = 1; x < COL; ++x)
        {
            aux[y][x] = matrix[y][x] ? aux[y - 1][x] + aux[y][x - 1] : 0;
        }
    }

    return aux[M - 1][N - 1];
}

static long long get_milliseconds() {
    return std::chrono::duration_cast<
           std::chrono::milliseconds>(
           std::chrono::system_clock::now().time_since_epoch()).count();
}

int main(int argc, char** argv) {
    int matrix[ROW][COL];

    for (size_t y = 0; y < ROW; ++y) {
        for (size_t x = 0; x < COL; ++x) {
            matrix[y][x] = 1;
        }
    }

    matrix[ROW / 2][COL / 2] = 0;
    matrix[2][3] = 0;

    long long ta = get_milliseconds();
    int path_count = pathCount(matrix, ROW, COL);
    long long tb = get_milliseconds();

    std::cout << "Paths: " << path_count << " in " << tb - ta << " ms."
              << std::endl;

    path_count = 0;

    ta = get_milliseconds();
    int path_count2 = pathCountV2(matrix, ROW, COL);
    tb = get_milliseconds();

    std::cout << "Paths: " << path_count2 << " in " << tb - ta << " ms."
              << std::endl;

    return 0;
}

The space complexity of the above algorithm is also \$\Theta(MN)\$, but it can be easily improved to \$\Theta(\min\{M, N\})\$ by processing one column or row at a time.


Numeric and percentage deadband calculation Calculate if the value exceeds percentage <> numeric deadbands of the previous value.

The first function is for percentage. It detects if $value is $deadband% greater or less than $previous.

The second is for numeric difference. It detects if the difference between $value and $previous is greater than $deadband.

function percentage_deadband($value,$previous,$deadband)
{
    if($previous==0 || $value==0 || $previous == $value)
    {
        return false; //if previous or value is 0, we can't work. assume false.
    }
    $percentage = $value / $previous * 100;
    if($percentage > 100){
        return $percentage - $deadband > 100;//if percentage - deadband > 100, we know the deadband is exceeded
    }
    if($percentage < 100){
        return 100 - $percentage > $deadband;//if 100 - percentage is greater than deadband, we know the deadband is exceeded
    }
    return false;
}

function numeric_deadband($value,$previous,$deadband)
{
    if($previous==0 || $value==0 || $previous == $value)
    {
        return false;
    }
    if($previous > $value){
        return $previous - $deadband >= $value;
    }else{
        return $value - $previous >= $deadband;
    }
    return false;
}

Testing the functions

$numeric_deadband = 3;
$numeric_set = [
    0=>['value'=>77,'previous'=>80],//true
    1=>['value'=>78,'previous'=>77],//false
    2=>['value'=>81,'previous'=>78],//true
    3=>['value'=>81,'previous'=>81]//false
];
echo 'numeric set:
';
foreach($numeric_set as $row)
{

    var_dump(numeric_deadband($row['value'],$row['previous'],$numeric_deadband));
    echo '
';
}

echo '
';

$percentage_deadband = 10;
$percentage_set = [
    0=>['value'=>27,'previous'=>39],//true
    1=>['value'=>39,'previous'=>40],//false
    2=>['value'=>41,'previous'=>28],//true
    3=>['value'=>28,'previous'=>31]//false
];
echo 'percentage set:
';
foreach($percentage_set as $row)
{

    var_dump(percentage_deadband($row['value'],$row['previous'],$percentage_deadband));
    echo '
';
}

They seem to work correctly with my small set of test data, but I am not so great with math. I'd love a second opinion.

I'm unclear about what is TRUE and what is FALSE (and how you interpret it).

Is it FALSE = when value is greater than previous by more than deadband?
    (i.e. value > previous + deadband is NOT OK)
    (i.e. value <= previous + deadband is OK)
That's how I did it below, but if you want the opposite, change the "<" sign.

I would change the two functions in one, and give an extra argument (true for numeric, false for percentage):

    function check_deadband($value,$previous,$deadband,$check_numeric) {
        if ($value <= 0) {
            return false;
        }
        if ($check_numeric) {
           // numeric testing
            $difference=abs($previous-$value);
            return $difference < $deadband;
            // ex. value: 74, previous: 70 => difference = 4
            // difference: 4 < deadband: 3 => false, not ok
        } else {
            // percentage testing
            $percentage=abs(($value - $previous) * 100/$previous);
            return $percentage < $deadband;
            // ex. v:111 p:100, (v-p)*100/p:11% < 10% => false, not ok
        }
    }

You eliminated the previous=0 or previous=value cases in your example, but I didn't understand why (you can put them back).
I also not sure that your percentage formula is corect. If you want the difference in percentage, then (value-previous)*100/previous will give you the right answer. (ex. value = 10, previous=5 => percentage is 100 - correct!)
You can check the function with the following testing code:

    $numeric_deadband = 3;
    $percentage_deadband = 10;

    /// GENERATE RADNOM DATA ex. 5.000 rows
    $rows_nr=5000;
    $min=40;
    $max=80;
    for ($i=0; $i<$rows_nr; $i++) {
        $numeric_set[]=array(
            "value"=>rand($min,$max),
            "previous" => rand(40,80));
        $percentage_set[]=array(
            "value"=>rand($min,$max),
            "previous" => rand(40,80));
    }

    $time=microtime(true); // just for testing purposes
    foreach ($numeric_set as $row) {
        // check NUMERIC set
        // remove echos after testing
        echo "NUMERIC >> value: $row[value] vs previous :".
            "$row[previous] (deadband: $numeric_deadband) => ";
        echo check_deadband(
            $row["value"],
            $row["previous"],
            $numeric_deadband,
            true)
            ? "true" : "false";
        echo "
";
    } unset($row);

    // CHECK time in miliseconds
    echo "
Checking deadband for NUMERIC ($rows_nr rows) took:".
        round((microtime(true)-$time)*1000)." miliseconds
";

    $time=microtime(true);
    foreach ($percentage_set as $row) {
        // check PERCENTAGE set
        // remove echo after testing
        echo "PERCENTAGE >> value: $row[value] vs previous:".
        "$row[previous]  (deadband: $percentage_deadband) => ";
        echo check_deadband(
            $row["value"],
            $row["previous"],
            $percentage_deadband,
            false)
            ? "true" : "false";
        echo "
";
    } unset($row);

    echo "
Checking deadband for PERCENTAGE ($rows_nr rows) took:".
        round((microtime(true)-$time)*1000)." miliseconds
";

Hope this works. Feedback would be appreciated!

EDIT:
Changed the script to detect both above and below deadband changes.


Beehive numbers - using goto in C++ I understand that using goto in C++ code is strictly unadvised, but sometimes, it really reduces the number of lines of code like in the following case.

This is my code for SPOJ. I know this does not reduce too many lines of code, but in a big project, it potentially could.

#include<iostream>
#include<cstring>
#include<cstdio>
#include<cmath>
using namespace std;
bool debug=false;
typedef long long int lld;
typedef unsigned long long int llu;
int main(int argc , char **argv)
{
    if(argc>1 && strcmp(argv[1],"DEBUG")==0) debug=true;
    lld n,val;
    long double sq;
    while(true){
        scanf("%lld",&n);
        if(n==-1)break;
        n -= 1;
        if(n%3!=0)goto exit;
        n/=3;
        sq=4*n+1;sq=sqrt(sq);
        if(sq-(int)sq!=0)goto exit;
        n=sq;
        if(n%2!=1)goto exit;
        printf("Y\n");
        continue;
        exit:
            printf("N\n");
            continue;
    }
    return 0;
}

What should I do in such situations? Is there a way to do this by making a function call?

Your use of goto is wholly unjustified, not just because goto is taboo, but because your code has flow-of-control that is hard to follow.  Furthermore, the use of goto is not even an effective way to achieve your goal of compactness.

Before addressing the core concern about goto, I'd like to point out that there is a lot of junk in your code:

Your compiler should have warned you:

beehive.cpp:12:11: warning: unused variable 'val' [-Wunused-variable]
    lld n,val;
          ^
1 warning generated.

You do compile with warnings enabled, right?
#include <iostream> and #include <cstring> are superfluous.  (Put the remaining ones in alphabetical order.)
using namespace std; is superfluous, and even if you used anything in the std namespace, a blanket import like that is a harmful habit.
The debug flag is unused.
typedef unsigned long long int llu; is never used.  Furthermore, the problem guarantees 1 ≤ n ≤ 109, so a long would be sufficient for n.

As you suspected, a function call would help.  Your main() should look like this:

int main() {
    long n;
    while ((1 == scanf("%ld", &n)) && (-1 != n)) {
        puts(is_beehive_number(n) ? "Y" : "N");
    }
    return 0;
}

That provides two very important improvements over your code:

You can see at a glance what the purpose and structure of your program are.  The clutter within the loop is all gone, replaced by a function whose purpose is obvious.  The loop is properly structured — the phony while (true) is replaced by a useful test.
There is now proper separation of concerns.  main() loops, reads the input cases, and prints the results.  Most importantly, the is_beehive_number() is a pure calculation function that accepts a long and returns a bool.

When the calculation code is in its own function rather than embedded in the loop, the flow of control can be expressed so much better!

bool is_beehive_number(long n) {
    if (--n % 3 != 0) return false;
    double sq = sqrt(4 * (n / 3) + 1);
    if (sq != (long)sq) return false;
    n = sq;
    return (n & 1);
}

I don't understand the mathematics behind your code; I just transformed the code mechanically.


Find all the positive divisors of a positive integer This came from this question.

Find all the positive divisors of an integer >= 2.

Can stop when i * i >= number.

Please review for speed and style.

public class IntWithDivisors
{
    public override int GetHashCode()
    {
        return Number;
    }
    public override bool Equals(object obj)
    {
        if(obj is IntWithDivisors)
        {
            return ((IntWithDivisors)obj).Number == this.Number;
        }
        return false;
    }
    public override string ToString()
    {
        return $"number {Number}    Devisors " + string.Join(", ", Divisors);
    }

    public List<int> Divisors { get; } = new List<int>();
    public int Count { get { return Divisors.Count(); } }
    public int Number { get; }
    public IntWithDivisors(int number)
    {
        if (number < 2)
        {
            throw new ArgumentOutOfRangeException();
        }
        Number = number;
        Divisors = IntDivisors(number);
    }
}
public static List<int> IntDivisors(int num)
{
    //Debug.WriteLine($"\nIntDevisors  {num}");
    List<int> intDivisors = new List<int>();
    intDivisors.Add(1);
    intDivisors.Add(num);
    int i;
    int incr;
    if(num / 2 * 2 == num)
    {
        i = 2;
        incr = 1;
    }
    else
    {
        i = 3;
        incr = 2;
    }
    for(; i*i < num; i += incr)
    {
        int numOveri = num / i;
        if (numOveri * i == num)
        {
            //Debug.WriteLine(i);
            intDivisors.Add(i);
            intDivisors.Add(numOveri);
        }
    }
    if(i*i == num)
    {
        intDivisors.Add(i);
    }
    intDivisors.Sort();
    return intDivisors;
}

Why is the implementation of the algorithm a static method outside of the class IntWithDivisors? Is it in class Program? It shouldn't be there.

It is unusual to do complex work in the constructor. A constructor should only perform initialization. See answer of Telastyn in how complex a constructor should be. There are different possibilities to solve this

E.g. lazy evaluation:

private List<int> _divisors;
public List<int> Divisors
{
    get {
        if (_divisors == null) {
            _divisors = IntDivisors(Number);
        }
        return _divisors;
    }
}

Another option is to simply call a method to get the result.

This leads us to the next question: what is the task of the class IntWithDivisors? Do we really need to store the input number together with the output? Do we really need to override Equals and GetHashCode?

I would rather opt for a minimalist but reusable and flexible approach in the LINQ style as extension method implemented as iterator.

I observed that the starting i is one bigger than incr. We can use this fact to simplify the initialization.

Testing if a number is even is usually done with the modulo operator which yields the remainder of the division num % 2 == 0.

numOveri is a strange name. I renamed it to quotient and i to divisor.

The divisors are tested in ascending order; however, the quotients accumulate in descending order. Therefore, we can return the divisors immediately with yield return and store the quotients in a list. We then need to reverse this list before we return its items.

public static class IntExtensions
{
    public static IEnumerable<int> SelectDivisors(this int num)
    {
        yield return 1;

        int incr = num % 2 == 0 ? 1 : 2;
        var largeDivisors = new List<int>();
        for (int divisor = incr + 1; divisor * divisor <= num; divisor += incr) {
            int quotient = num / divisor;
            if (quotient * divisor == num) {
                yield return divisor;
                if (quotient != divisor) {
                    largeDivisors.Add(quotient);
                }
            }
        }

        largeDivisors.Reverse();
        for (int k = 0; k < largeDivisors.Count; k++) {
            yield return largeDivisors[k];
        }

        yield return num;
    }
}

We can use this extension method like this (shown in a little test routine):

int[] numbers = new[] { 9, 12, 15, 16, 17, 27, 54 };
foreach (int num in numbers) {
    Console.Write($"Number {num} has divisors ");
    foreach (int n in num.SelectDivisors()) {
        Console.Write(n + " ");
    }
    Console.WriteLine();
}


Day counter between two dates I wrote this little program, but I'm not sure if it's properly written.  Actually, it can't handle leap years (every year is 365 days long) and you have to write the earlier date before. It seems to be working correctly.

Can you tell me if it's okay or not, and what should I change in it to make it good?

#include <iostream>
using namespace std;

int first_date_month;
int first_date_days;
int first_date_year;
int second_date_month;
int second_date_days;
int second_date_year;
int days;

int month_days[] = {31,28,31,30,31,30,31,31,30,31,30,31};

int main()
{
    cout << "Enter first date: ";
    cin >> first_date_year >> first_date_month >> first_date_days;
    cout << "Enter second date: ";
    cin >> second_date_year >> second_date_month >> second_date_days;

    if(first_date_year == second_date_year)
    {
        if(first_date_month == second_date_month)
            days = second_date_days - first_date_days;
        else
        {
            for(int i = first_date_month; i < second_date_month-1;i++)
                days += month_days[i];
            days += month_days[first_date_month-1] - first_date_days + second_date_days;
        }
    }
    else
    {
        for(int i = 0; i < second_date_month-1; i++)
            days += month_days[i];
        for(int i = first_date_month; i < 12; i++)
            days += month_days[i];
        if(second_date_year - first_date_year >= 0)
        days += (second_date_year - first_date_year - 1)*365 +
            month_days[first_date_month-1] - first_date_days + second_date_days;
    }

    cout << "Days between the two dates: " << days;

    return(0);
}

Do not use using namespace std in global scope.
Never use global variables.  Right away, this will introduce all sorts of problems, including maintainability and bugs.  There are different alternatives to this, one being a struct:

Such a structure just needs the month, day, and year:

struct Date
{
    int month;
    int day;
    int year;
};

Initialize the two Date instances:

// same order as appears in struct
Date date1 = {1, 2, 2000};
Date date2 = {4, 5, 2001};

Access the structures and set the data members (with your code):

std::cout << "Enter first date: ";
std::cin >> date1.year >> date1.month >> date1.day;
std::cout << "Enter second date: ";
std::cin >> date2.year >> date2.month >> date2.day;

If you want to get into encapsulation/information-hiding, I'd recommend a class instead.  If you want to keep this simpler than a struct, just move the globals into main() (but use the variables from my example).  You could also create more specialized functions, thereby not just working in main().  Modularity will help keep your code more organized and maintainable.
I don't like int for these values (dates cannot be negative).  I'd go with std::size_t instead (include <cstddef> to use it).
month_days[] should be a const while in global scope.  As a constant, it can remain there because it cannot be changed by anything else.  However, this will prevent you from accounting for leap-years.  Speaking of which...
To account for leap-years, I'd either:

leave out February's value from the array (it's the only value that could change)
not make the array a constant (the program will handle the values during runtime)

With that, you can allow the program to adjust February's value if a leap-year.


Statistics with Python Write a program to determine the mean, median, and standard deviation of a list of numbers
  Count the duplicate numbers in the code, listing any that occur more than once with the number of times it occurs.

My implementation:

import math

def mean(input):
  return sum(input) / float(len(input))

def median(input):
  sorted_input = sorted(input)
  length = len(input)
  if length < 1:
    return None
  if length % 2 == 0:
    return (sorted_input[length / 2] + sorted_input[length / 2 - 1]) / float(2)
  else:
    return sorted_input[length / 2]

def duplicate_counts(input):
  distinct_input = set(input)
  for distinct in set(input):
    count = input.count(distinct)
    if (count > 1):
      print("{} occurs {} times".format(distinct, count))

def standard_deviation(input):
  return math.sqrt(mean([(x - mean(input)) ** 2 for x in input]))

def main():
  #sample run/test
  the_list = [3, 6, 7, 2, 8, 9, 2, 3, 7, 4, 5, 9, 2, 1, 6, 9, 6]
  print("The mean is: {}".format(mean(the_list)))
  print("The median is: {}".format(median(the_list)))
  print("The standard deviation is: {}".format(standard_deviation(the_list)))
  duplicate_counts(the_list)

if __name__ == "__main__":
  main()

Sample output:

The mean is: 5.23529411765
The median is: 6
The standard deviation is: 2.64640514805
2 occurs 3 times
3 occurs 2 times
6 occurs 3 times
7 occurs 2 times
9 occurs 3 times

Simple exercise I decided to do with Python for practice and to test/try all the excellent feedback I received on my previous two questions. There may be built in functions for these, and feel free to mention them, but for the sake of practice I implemented them this way.

A variable name input is confusing. After all, input is a Python built-in.
standard_deviation calls mean too many times (add a debug printout to mean to see). Better do

def standard_deviation(input):
    input_mean = mean(input)
    return math.sqrt(mean([(x - input_mean) ** 2 for x in input]))

Median can be calculated in \$ O(\log n)\$. Sorting a list in \$O(n \log n)\$ just for a median is an overkill.


MySQL update with PHP; MySQLi prepared statement in loop I'm going to develop an Android application that performs simple MySQL operations by invoking server-sided PHP scripts. PHP-MySQL communication is done by MySQLi extension. Each operation that I'd like to perform is in a separate PHP file, and in a separate class. There's an example of update.php:

<?php
require_once 'functions.php';
if (empty($_POST)) {

    $_POST['UPDATE_DATA'] = array('numer_vin' => 'SIBISIBI', 'model_pojazdu' => 'Vectra', 'marka' => 'Opel');
    $_POST['UPDATE_HEADERS'] = array('table' => 'polisy_oc', 'id' => 'numer_polisy_oc', 'id_value' => '72451');
}

class UpdateRecord
{
    protected $db_connection;

    public function __construct($db_connection) {
        $this->db_connection = $db_connection->connection;

        $this->update($_POST['UPDATE_HEADERS'], $_POST['UPDATE_DATA']);
    }

    private function update($update_headers, $update_data) { //TODO:single query
        if (in_array($update_headers['table'], $_SESSION['tables_white_list']) && in_array($update_headers['id'], $_SESSION['columns_white_list'])) {
            foreach ($update_data as $key => $value) {
                if (checkColumn($key)) {
                    $update = $this->db_connection->prepare("UPDATE {$update_headers['table']} SET $key = ? WHERE {$update_headers['id']} = ?");
                    $update->bind_param('ss', $value, $update_headers['id_value']);
                    $update->execute();
                    $update->close();
                }
            }
        }
    }
}
$db = new Database;
$update = new UpdateRecord($db);
?>

My main concern is whether Depency Injection is properly used regarding database connection (all my other scripts do the same thing).
Here's how Database class is built:

class Database
{
    public $connection;

    function __construct()
    {
        if(!isset($_SESSION)){
            session_start();
        }
        $this->connection = new mysqli('localhost', $_SESSION['login'], $_SESSION['password'], 'Ubezpieczenia_OC');

        if($this->connection->connect_error) {
            doDatabaseConnectionError();
        } else {
            $this->connection->set_charset("utf8");

            if(!isset($_SESSION['tables_white_list']) || !isset($_SESSION['columns_white_list'])){
                $_SESSION['tables_white_list'] = array('klienci', 'polisy_oc', 'parametry_oc', 'modele_samochodow', 'marki_samochodow');
                $_SESSION['columns_white_list'] = getTablesColumns($this->connection);
            }
        }
    }
}

Secondly, it was extremely hard for me to perform MySQL UPDATE query by mysqli extension's prepared statements for variable number of columns. Is above foreach loop solution a serious performance bottleneck and not elastic solution?

I'll stick to reviewing your PHP code, as it doesn't look like it's recieved the attention it deserves!

I'll do what I can to find faults, but I won't be testing anything. Alright, we ready?

Nice! First line and we've found something! What kind file is functions.php!? Filenames should be one of the clearest names in your project. If you can't come up with a good filename, then that's a sign you need to break apart the contents within.
I'm assuming the initial conditional empty() is for testing only? I hope so, because automatically updating data on page load is a disaster waiting to happen. Send a couple hundreds requests to a page that's doing resource heavy database access and you'll happily be sitting in a DoS attack.
It's a little hard to tell, but it looks like those variable sets in that conditional are longer than any desired line length. It makes it much harder for the reader to effienctly read your code if they have to do a lot of horizontal scrolling.

You can reduce this a couple ways. In this case, break down your array over multiple lines:

$_POST['UPDATE_HEADERS'] = array(
        'table' => 'polisy_oc',
        'id' => 'numer_polisy_oc',
        'id_value' => '72451'
    );

This way is substantially easier to read in my opinion.

For other really long lines, you may be able to put data in variables, and then reference those variables.
@200_success had a good point: UpdateRecord isn't a very good name. Although it's hard to rename this class because it really doesn't have a single responsibility. That in itself contradicts the principles of SOLID. You might look towards making a Record class.
__construct($db_connection) should really be __construct(Database $db_connection) to better implement dependency injection practices.
I'd avoid automatically calling update() from the constructor. Make update() public and call it where the object is initialized.
If update() were in a Record class, that'd be OK. But here, I don't think the name is right. For all I know, this could really be updating the record updater!

If you don't change the class name, I suggest this be a method called updateRecord().
Where do we get $_SESSION from? The user. And what's our golden rule? Never trust the user. I think it's a bad idea to check the POST data against session data. Make a concrete white-list somewhere. Either that, or eliminate the need to dynamically reference a table. I'm sure with enough whiteboard space you'll be able to figure it out!
Avoid using the default $key and $value. Give some more descriptive variable names.
What is checkColumn()?

Alright! One file down, now the next?

Database is a very vague name. You got the noun part down, but seeing something like this would kill me!
Something like $connection doesn't need to be public. I'd make that private/protected and encapsulate it.
Add a visibility to your constructor.
Your constructor has too much business logic in it! It should be for constructing the object, nothing more. Break the logic into other methods.
Oh goodness! I do not like how you're setting up the database via the session! That is asking for a lot of trouble! Why can't the credentials be hardcoded? Or maybe put in a config file and access that way? The user has your database credentials!
Your DBAL shouldn't have any type of business logic in it what-so-ever. It defeats the point of having it in a separate class.

And lastly, you mention your code bottlenecking, but you're not sure why. Profile your code, get some concrete numbers, and then work from there. Is it that foreach that's causing the lag, or it say the query? You'll never know unless you test it!

In the comments a couple questions were asked. I'll do my best to answer them:

  functions.php is a file which contains database class and functions that are used by more than one class; shouldn't I use such resolution?

It's vague. What kind of functions are in this file? What kind of functions should go in this file? When is it appropriate to have a new class, versus just add functions into here. It's a problem of extending functional programming along side OO programming.

  Making a Record class would mean that it would have methods like updateClient(), insertClient() etc. inside, which I thought would make connecting app to database harder - wouldn't it be simpler for an app just to invoke desired class which is in separate .php file?

Those methods you list might actually be a better fit in a Client class. But having CRUD operations in a single class is not a bad thing. It keeps all "record" things together, and all "client" things together.

  Why adding class name in __construct parameters implements better dependency injection?

It ensures that the type hinted class is being introduced into the object. If I say __construct(Database $db), I know that a database will be passed. It also helps inheritance efforts. I could code something like __construct(API $api), and then if I have UserAPI and ServiceAPI classes, both of which extend API, then I can pass either of those, too.

  in update class, I check provided table and column with [the $_SESSION global] white-lists, why is it bad?

You don't have control over the session. It's so easy to take advantage of vulnerable sessions, it just takes the right mind. Here's just the a beginner's answer to session hijacking/fixation.

  I want an user of app to be the user of the database either (docs.oracle.com/cd/B28359_01/network.111/b28531/…), that's why I've come up with creating connection by $_SESSION.

From the Oracle link you provided, right off the bat I'll give you text from that page:

  You can store password credentials for connecting to a database by using a client-side Oracle wallet. An Oracle wallet is a secure software container that stores the authentication and signing credentials needed for a user to log in.

I point this out because of several reason:

Oracle isn't advocating PHP sessions. They want you to use their secure software to authenticate. These are two different realms here!
Application users should really only need to be database users if you have that much data. I don't see any reason to do this if you are the average web developer. If you are scaling up to this approach, you will want a team. Things will become very messy if each user get's their own set of databases!

  How can I connect to database by mysqli using password hashes?

You can't. Not hashes anyways. Know the differences between hashing, encrypting, etc.

You may need to rethink your program architecture before getting into this. I don't have the experience to tell you how it should be done.


Converting double to string without scientific notation and without losing digits Requirements:

Must convert a double to a string, without losing any digits.
Cannot use scientific notation.

In C#, no format string exists that you can you pass into Double.ToString(format) to accomplish this, because ToString will use only 15 digits, even though a double can have 17. Although R and G17 can use 17 digits, it may or may not use scientific notation.

Please correct me if I am wrong.

Therefore, I wrote a function to do this. I'm posting it here as an optimization question. If you see flaws, feel free to point them out. I'm confident the code works, but there's plenty of room for improvement.

[Fact]
public void NumberFormats()
{
    double d1 = 123456789012171.23;
    double d2 = 0.0000000000000001;
    double d3 = 10000000000000000;

    string s1 = DoubleToString(d1);
    string s2 = DoubleToString(d2);
    string s3 = DoubleToString(d3);

    Assert.Equal("123456789012171.23", s1);
    Assert.Equal("0.0000000000000001", s2);
    Assert.Equal("10000000000000000", s3);
}

private string DoubleToString(double d)
{
    string R = d.ToString("R", CultureInfo.InvariantCulture).Replace(",", "");
    int i = R.IndexOf('E');

    if (i < 0)
        return R;

    string G17 = d.ToString("G17", CultureInfo.InvariantCulture);

    if (!G17.Contains('E'))
        return G17.Replace(",", "");

    // manual parsing
    string beforeTheE = R.Substring(0, i);
    int E = Convert.ToInt32(R.Substring(i + 1));

    i = beforeTheE.IndexOf('.');

    if (i < 0)
        i = beforeTheE.Length;
    else
        beforeTheE = beforeTheE.Replace(".", "");

    i += E;

    while (i < 1)
    {
        beforeTheE = "0" + beforeTheE;
        i++;
    }

    while (i > beforeTheE.Length)
    {
        beforeTheE += "0";
    }

    if (i == beforeTheE.Length)
        return beforeTheE;

    return String.Format("{0}.{1}", beforeTheE.Substring(0, i), beforeTheE.Substring(i));
}

I know, it's a challenge convert your big doubles to string, but if you can use decimal instead of double, the conversion could be smooth.

static void Main(string[] args)
{
    decimal d1 = 123456789012171.23m;
    decimal d2 = 0.0000000000000001m;
    decimal d3 = 10000000000000000m;

    Console.WriteLine(d1);
    Console.WriteLine(d2);
    Console.WriteLine(d3);
}


VBA code that compares 2 sheets from different systems In this code looking at 2 worksheets that contain similar data from different systems. Column 1 contains a unique staff number so there can be a match on the person then here there is a difference in NiNo ws1.cell(,17) and ws2.cell(,24)(This criteria will change each time the full code is run, to find Date of birth for example) between the sheets then certain values for that person are copied to a 3rd sheet.

How ever there are to be 18 different worksheets all looking at different criteria, so this code will have to run 18 times and will take a while. any ideas how I can speed it up examples please.

I think the answer is to use arrays but I'm not great with arrays.

Sub NINODifferences()

    Application.ScreenUpdating = False
    Application.Calculation = xlCalculationManual

    Dim ws1 As Worksheet, ws2 As Worksheet, ws3 As Worksheet, ws4 As Worksheet
    Dim i As Long, j As Long, iCol As Long, iRow As Long, sl1 as Long, sl2 as Long

sl1 = ws1.Cells(ws1.Rows.Count, 1).End(xlUp).Row
sl2 = ws2.Cells(ws2.Rows.Count, 1).End(xlUp).Row

    Set ws1 = ActiveWorkbook.Sheets("SheetA")
    Set ws2 = ActiveWorkbook.Sheets("SheetB")
    Set ws3 = ActiveWorkbook.Sheets("NINO Differences")' this will be a different sheet as I change the criteria

    iRow = 2
    iCol = 1

        For i = 1 To sl1
            For j = 1 To sl2

                If Trim(ws1.Cells(i, 1).Value2) = Trim(ws2.Cells(j, 1).Value2) Then

                    If Trim(ws1.Cells(i, 17).Value2) <> Trim(ws2.Cells(j, 24).Value2) Then 'this is the criteria so if different.

                        ws3.Cells(iRow, iCol).Value2 = ws1.Cells(i, 1).Value2
                        iCol = iCol + 1
                        ws3.Cells(iRow, iCol).Value2 = ws1.Cells(i, 2).Value2
                        iCol = iCol + 1
                        ws3.Cells(iRow, iCol).Value2 = ws1.Cells(i, 3).Value2
                        iCol = iCol + 1
                        ws3.Cells(iRow, iCol).Value2 = ws1.Cells(i, 17).Value2
                        iCol = iCol + 1
                        ws3.Cells(iRow, iCol).Value2 = ws2.Cells(j, 24).Value2

                        iCol = 1
                        iRow = iRow + 1

                    Else
                    End If

                Else
                End If

            Next j
       Next i

    Set ws1 = Nothing
    Set ws2 = Nothing
    Set ws3 = Nothing

    End Sub

You turn off ScreenUpdating and Calculation and never turn them back on. Make sure you do that.

Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual

You have your variables but the names can be improved. Variable names that describe what your code is doing will make it easier when you or whomever inherits the workbook after you has to edit it later. Declare them just before you use them, ws4 is not used anywhere and can be removed.

    Dim ws1 As Worksheet, ws2 As Worksheet, ws3 As Worksheet, ws4 As Worksheet
    Dim i As Long, j As Long, iCol As Long, iRow As Long, sl1 as Long, sl2 as Long

You're looping through rows and looking at columns using magic numbers (1, 2, 3, 17, 24) that have some meaning. I've no clue why any of them are important. Give a descriptive constant that tells you what it's there for.

Best I can discern you don't want something else done. Remove it.

If ... Then
    'SomeCode
Else
End If

Much cleaner. No "guessing" if an Else was forgotten

If ... Then
    'SomeCode
End If

You check for certain criteria before populating. Create a function for that and allow that to be your check.

If Trim(ws1.Cells(i, 1).Value2) = Trim(ws2.Cells(j, 1).Value2) Then
    If Trim(ws1.Cells(i, 17).Value2) <> Trim(ws2.Cells(j, 24).Value2) Then 'this is the criteria so if different.

Becomes. You now focus on providing what's needed for the function to return it's Boolean (True/False) instead of how it's doing it.

If IsDataValidForPopulation(primarySheet, primarySheetRow, secondarySheet, secondarySheetRow) Then

Below is the body of the function. Trim was replaced with Trim$ to avoid
an implicit conversion.

Private Function IsDataValidForPopulation(ByVal primarySheet As Worksheet, ByVal primarySheetRow As Long, ByVal secondarySheet As Worksheet, secondarySheetRow As Long) As Boolean
    Const PRIMARY_SHEET_CRITERIA_COLUMN As Long = 17
    Const SECONDARY_SHEET_CRITERIA_COLUMN As Long = 24

    If Trim$(primarySheet.Cells(primarySheetRow, 1).Value2) = Trim$(secondarySheet.Cells(secondarySheetRow, 1).Value2) Then
        If Trim$(primarySheet.Cells(primarySheetRow, PRIMARY_SHEET_CRITERIA_COLUMN).Value2) <> Trim$(secondarySheet.Cells(secondarySheetRow, SECONDARY_SHEET_CRITERIA_COLUMN).Value2) Then
            IsDataValidForPopulation = True
        End If
    End If
End Function

Your population is next. The name PopulateData describes what's being done, the parameters populationSheet, populationRow, and populationColumn also self document. You were incrementing iCol several times iCol = iCol + 1 and then set it back at the end. That's been replaced and eliminates the vertical scrolling. Note: the ByRef on populationRow. This allows the incrementation done at the end to be seen by the caller and continue offsetting to the next row.

Private Sub PopulateData(ByVal primarySheet As Worksheet, ByVal primarySheetRow As Long, ByVal secondarySheet As Worksheet, ByVal secondarySheetRow As Long, ByVal populationSheet As Worksheet, ByRef populationRow As Long, ByVal populationColumn As Long)
    'Describe your magic numbers. Rename them appropriately
    Const FIRST_NUMBER As Long = 1
    Const SECOND_NUMBER As Long = 2
    Const THIRD_NUMBER As Long = 3
    Const FORTH_NUMBER As Long = 17
    Const FIFTH_NUMBER As Long = 24
    With populationSheet
        .Cells(populationRow, populationColumn).Value2 = primarySheet.Cells(primarySheetRow, FIRST_NUMBER).Value2
        .Cells(populationRow, populationColumn + 1).Value2 = primarySheet.Cells(primarySheetRow, SECOND_NUMBER).Value2
        .Cells(populationRow, populationColumn + 2).Value2 = primarySheet.Cells(primarySheetRow, THIRD_NUMBER).Value2
        .Cells(populationRow, populationColumn + 3).Value2 = primarySheet.Cells(primarySheetRow, FORTH_NUMBER).Value2
        .Cells(populationRow, populationColumn + 4).Value2 = secondarySheet.Cells(secondarySheetRow, FIFTH_NUMBER).Value2
    End With

    populationRow = populationRow + 1
End Sub

You had Set ws3 = ActiveWorkbook.Sheets("NINO Differences")' this will be a different sheet as I change the criteria which meant you'd need to change the code. Through refactoring and using the parameter populationSheet you now mandate that whoever calls this Sub needs to supply the required sheets.

Private Sub NINODifferences(ByVal primarySheet As Worksheet, ByVal secondarySheet As Worksheet, ByVal populationSheet As Worksheet)

    Application.ScreenUpdating = False
    Application.Calculation = xlCalculationManual

    Dim primarySheetLastRow As Long
    primarySheetLastRow = primarySheet.Cells(primarySheet.Rows.Count, 1).End(xlUp).Row

    Dim secondarySheetLastRow As Long
    secondarySheetLastRow = secondarySheet.Cells(secondarySheet.Rows.Count, 1).End(xlUp).Row

    Const populationColumn As Long = 1
    Dim populationRow As Long
    populationRow = 2

    Dim primarySheetRow As Long
    Dim secondarySheetRow As Long
    For primarySheetRow = 1 To primarySheetLastRow
        For secondarySheetRow = 1 To secondarySheetLastRow
            If IsDataValidForPopulation(primarySheet, primarySheetRow, secondarySheet, secondarySheetRow) Then
                PopulateData primarySheet, primarySheetRow, secondarySheet, secondarySheetRow, populationSheet, populationRow, populationColumn
            End If
        Next secondarySheetRow
    Next primarySheetRow

    Application.ScreenUpdating = True
    Application.Calculation = xlCalculationAutomatic

End Sub

You now have the option to create something like what follows to provide the sheets you need it run on. Note: I suggest renaming the Worksheets.CodeName member do be descriptive and use that in place of ActiveWorkbook.Sheets("SheetA") since you're using the name displayed on the sheet tab. If that get's changed you'll have to change your code.

Public Sub Test()
    Dim ws1 As Worksheet
    Dim ws2 As Worksheet
    Set ws1 = ActiveWorkbook.Sheets("SheetA") 'Use the Worksheet.CodeName member

    Set ws2 = ActiveWorkbook.Sheets("SheetB")
    NINODifferences ws1, ws2, ActiveWorkbook.Sheets("NINO Differences")
    NINODifferences ws1, ws2, ActiveWorkbook.Sheets("FOO Differences")
    NINODifferences ws1, ws2, ActiveWorkbook.Sheets("BAR Differences")
    NINODifferences ws1, ws2, ActiveWorkbook.Sheets("DUK Differences")
    '...
    NINODifferences ws1, ws2, ActiveWorkbook.Sheets("ZZZ Differences")
End Sub


Best way to find the largest sorted suffix in a list - USACO Sleepy Cow Sorting Python Solving this problem.
Right now, my solution entails finding the largest sorted sublist, starting from the beginning, all the way to the end. However, I feel like the way I wrote the sorted_sublist function can definitely be improved. Besides that, I'm looking for any way to improve the way I write my code.
My code:
with open("sleepy.in", "r") as fin:
  n = int(next(fin))
  nums = [int(i) for i in fin.readline().split()]

def sorted_sublist(n, nums):
  for i in range(n):
    sublist = nums[i:n]
    if sublist == sorted(sublist):
      return sublist

def main(n, nums):
 x = sorted_sublist(n, nums)
 return n - len(x)

with open("sleepy.out", "w+") as fout:
 print(main(n, nums), file=fout)

Problem synopsis:
Given a list of numbers, find the number of steps it would take to sort this list by repeatedly placing the first element of this list into another slot, provided that you do this in an optimal fashion.
Example:
Input:
4
4 3 2 1

Step 1. Place the 4 to the back of the list: 3, 2, 1, 4
Step 2. Place the 3 to the third index of the list: 2, 1, 3, 4
Step 3. Place the 2 to the second index of the list: 1, 2, 3, 4
Output: 3, as it took three optimal steps to reach a sorted list

Nice solution and implementation. Few suggestions:

Naming: the name main is generally used for the entry point of the program. In this case, a better name could be steps_to_sort_cows.
Sublist bounds: since n is fixed, instead of sublist = nums[i:n] you can write sublist = nums[i:]

Complexity
The time complexity is \$O(n*n*log(n))\$. Basically, for each cow sort all the remaining cows. Typically, it's considered not optimal for large inputs.
The space complexity is also not optimal. For each cow two lists are created, one for the new sublist and one for the sorted sublist.
Improved
To reduce space and time complexity, find the size of the sorted sublist in this way:
def steps_to_sort_cows(n, cows):
  first_sorted_cow = cows[-1]
  sorted_cows = 0
  for c in reversed(cows):
    if c > first_sorted_cow:
      return n - sorted_cows
    first_sorted_cow = c
    sorted_cows += 1
  return 0

Starting from the end, count the number of sorted cows and then return n - sorted_cows.
This approach takes \$O(n)\$ time and \$O(1)\$ space complexity.


Iterative implementation of the Ackermann function Here's an iterative implementation of the Ackermann function for review:

#include <stack>
#include <iostream>
#include <tuple>
#include <vector>

int Ackermann(int m, int n) {
    std::stack<int> s;
    s.push(m);
    while (!s.empty()) {
        m = s.top();
        s.pop();
        if (m == 0 || n == 0)
            n += m + 1;
        else {
            s.push(m - 1);
            s.push(m);
            n--;
        }
    }
    return n;
}

int main() {
    std::vector<std::tuple<int, int, int>> tests{
        { 0, 0, 1},
        { 1, 0, 2},
        { 1, 1, 3},
        { 2, 1, 5}
    };

    for (auto const &test : tests) {
        using std::get;

        auto result = Ackermann(get<0>(test), get<1>(test));
        if (result == get<2>(test)) {
            std::cout << "Ackermann("
                       << get<0>(test) << ", "
                       << get<1>(test) << ") == "
                       << result << ": passed\n";
        }
        else {
            std::cerr << "Error: Ackermann("
                      << get<0>(test) << ", "
                      << get<1>(test)
                      << ": expected" << get<2>(test)
                      << ", but got: " << result << "\n";
        }
    }
}

Any comments appreciated.

The results of the Ackermann function can grow quite large. An int is only guaranteed to cover up to 32767.

A two-dimensional memorization table would probably be worthwhile.


How many conditions in an "if clause" is acceptable? I'm trying to validate an HTTP Request, and first I want to get sure that all of the information has been posted (and then I'll check the correct format of each incoming data), so to get sure that no Form Spoofing attack is made. However, since this HTTP Post comes from a form, and that form contains almost 40 fields, I came across an if clause like this:

// Here, the if clause has almost 40 conditions to check
// first, second, third, etc. are imaginary names of incoming variables
if (Request["first"] == null || Request["second"] == null || ...)
{
    throw new ApplicationException("Incoming data is incomplete");
}
else
{
    // Now trying to validate the format of incoming data
}

Is it a normal checking mechanism? Is there a better way for doing it?

You could put all of your variable names into an array and loop over it, something like this (pseudocode):

var names = { 'First', 'Second', 'Third' };

for (var name in names) {
    if (Request[names[name]] == null) {
        throw new ApplicationException('Error');
    }
}

// No errors if we reach here


Summarize raw data in new sheet, 2.0 Thanks to zak for some great advice on this previous question. This is a follow up question:

I have an Excel workbook with two sheets: "Raw data" and "Summary". In the raw data sheet there are several blocks with data structured like the image below:

I want to create a summary of the data below, in the sheet "Summary", looking like this:

zak suggested that I remove a bunch of Select, and Activate, and to always explicitly state the Worksheet when creating Ranges. I.e. Sheet.Range(), instead of just Range().

The code I have after including the tips from zak is as follows:

Sub TransferAndSumElements()
    Dim wb As Workbook
    Dim ws1 As Worksheet
    Dim ws2 As Worksheet

    Set wb = ActiveWorkbook
    Set ws1 = Sheets("Raw data")
    Set ws2 = Sheets("Summary")

    '##############
    ' Copy / Paste first row
    '##############
    ws1.Range("A1:B1").Copy _
        Destination:=ws2.Range("A1")

    '##############
    ' Set Item names in summary
    '##############

    With ws2
        .Range("A2").FormulaR1C1 = "Knife"
        .Range("A3").FormulaR1C1 = "Fork"
        .Range("A4").FormulaR1C1 = "Spoon"
        .Range("A5").FormulaR1C1 = "Spork"
        .Range("A6").FormulaR1C1 = "Bowl"
    End With

    '##############
    ' Find Elements and Sum
    '##############

    Dim s As Long           ' Sum of elements for each Item
    Dim str As String       ' String in first column in "Summary" (Knife, Fork ...)
    Dim cellstr As String   ' String in first column in "Raw data" (To be searched through)
    Dim DataRange As Range  ' All rows in sheet 1 ("Raw data")

    ws1.Select              ' Select first worksheet ("This worksheet")
    LastRow = ws1.Cells.Find("*", searchorder:=xlByRows, searchdirection:=xlPrevious).Row
    Set DataRange = Range("A1:A" & LastRow)

    For i = 1 To 5          ' Iterate through Knife, Fork .. in "Summary"...

        str = ws2.Cells(i + 1, 1).Value     ' Extract names
        s = 0                               ' Initialize sum

        For r = 1 To LastRow
            cellstr = ws1.Range("A" & r, "A" & r).Value
            If InStr(cellstr, str) > 0 Then
                s = s + ws1.Range("B" & r, "B" & r).Value
            End If
        Next r

        ws2.Cells(i + 1, 2).FormulaR1C1 = s

    Next i

ws2.Activate

End Sub

Some stuff that I want to improve, but haven't been able to:

Automatically fetch the item names from "Raw data". I've tried creating a Collection, but I end up with an empty row in between the others (Knife, Fork, Spoon, Spork, Blank, Bowl).

I can loop through this list after creating it, and delete the blank row, but I think there should be a way to do this in one go.

I want to create a list of words ("Knife", "Fork" .. ) and write it to Excel in one command. The way I'm doing it now is to manually write each item name.
Do the loops make sense, or should I do it in some other way?

And last but not least:

What about the big picture? Does it make sense to do it like this, or should I do it completely different?

Note: The summary can be sorted alphabetically, by number of elements, or not sorted at all. That doesn't matter.

Tab delimited data dump:

Item    Number
Knife   2
Fork    2
Spoon   1
Spork   2

Item    Number
Spork   2
Fork    3

Item    Number
Bowl    3
Knife   5

I'm not sure if this is a programming exercise or an excel exercise, or a programming in excel exercise. Each will have a different answer. But I can give you a few answers to start.

As a first comment. You are right, setting and getting values into excel cells one at a time is not desirable if you can do it in a large chunk.

Lets put a few tools into place.

The type of collection you are wanting to use to collect data is called a dictionary. You can use the dictionary provided in microsoft using the reference "Microsoft Scripting Runtime" the scrrun.dll. This will give you a key value pair to be able to do ..

Dim toolsDictionary As Dictionary
Set toolsDictionary = New Dictionary
...

If Not toolsDictionary.Exists(nextItemName) Then
    toolsDictionary(nextItemName) = 0
End If

Next you'll need to be able to test if a value is a number. This will let you work out if the row you are on is of any value to you. Here you are dealing with excel, not vba.

WorksheetFunction.IsNumber(someVariable)

Then you can also make use of VBA's setter or letter. Given

Dim inputData as Variant
Set inputData = ws2.Range("a1").CurrentRegion   'setter
inputData = ws2.Range("a1").CurrentRegion       'letter

the value of inputData in the setter will be a range object, while the inputData in the letter will contain a 2D array.

Last little tool is an excel tool.

Range.Sort

Lets not bother with just the labels, but we'll copy the whole data set across, and sort it

ws1.Range("A:B").Copy Destination:=ws2.Range("A:b")
ws2.Range("a:b").Sort Key1:=ws2.Range("b1"), Header:=xlYes

Now we have a few useful tools, so we can get on with the job at hand. And just as a warning. This is excel, so there are lots of ways to do this. This is  a VBA way.

'get our data and sort
ws1.Range("A:B").Copy Destination:=ws2.Range("A:b")
ws2.Range("a:b").Sort Key1:=ws2.Range("b1"), Header:=xlYes

' load our data into an array
inputData = ws2.Range("a1").CurrentRegion
count = ubound(inputData)

' traverse our data
for counter = 1 to count

    ' is it a tool
    isATool = WorksheetFunction.IsNumber (inputData(counter, 2))
    If isATool Then

        theTool = inputdata(counter,1)
        theNumber = inputdata(counter,2)

        ' check if we have a record
        If Not toolsDictionary.Exists(theTool) Then
            toolsDictionary(theTool) = 0
        End If

        ' accumulate the totals
        toolsDictionary(theTool) = theNumber
        Debug.Print toolsDictionary(theTool)

Once you've got all your data, repopulate your inputData. Make sure we offset by 1 to keep our header.

ws2.range("a1:b1").CurrentRegion.Offset(1).clear

count = toolsDictionary.count

inputData = ws2.range("a2:b2").resize(count)
' you could do
' ReDim inputData(1 To 5, 1 To 2). But I don't like to.
for counter = 0 to count - 1    ' dictionary is zero based
     theTool = toolsDictionary.keys(counter)
     theNumber = toolsDictionary.items(counter)
     inputData(counter + 1, 1) = theTool
     inputData(counter + 1, 2) = theNumber
next counter

ws2.Range("a2:b2").Resize(counter,2).Value2 = inputData


(Leetcode) Snakes and Ladders This is a Leetcode problem -

  On an \$N\$ x \$N\$ board, the numbers from 1 to N * N are
  written boustrophedonically (starting from the bottom left of the
  board), and alternating direction each row.  For example, for a 6 x 6
  board, the numbers are written as follows -

  You start on square 1 of the board (which is always in the last row
  and first column).  Each move, starting from square x, consists of
  the following -

  You choose a destination square S with number x+1, x+2, x+3, x+4, x+5, or x+6, provided this number is <= N * N.

  (This choice simulates the result of a standard 6-sided die roll, ie., there are always at most 6 destinations, regardless of
  the size of the board.)

  If S has a snake or ladder, you move to the destination of that snake or ladder.  Otherwise, you move to S.

  A board square on row r and column c has a "snake or ladder" if
  board[r][c] != -1. The destination of that snake or ladder is
  board[r][c].

  Note that you only take a snake or ladder at most once per move; if
  the destination to a snake or ladder is the start of another snake or
  ladder, you do not continue moving. (For example, if the board is
  [[4,-1],[-1,3]], and on the first move your destination square is
  2, then you finish your first move at 3 because you do not
  continue moving to 4.)

  Return the least number of moves required to reach square N * N. If
  it is not possible, return -1.

  Note -

  2 <= board.length = board[0].length <= 20
  board[i][j] is between 1 and N * N or is equal to -1.
  The board square with number 1 has no snake or ladder.
  The board square with number N * N has no snake or ladder.

  Example 1 -

Input: [
[-1,-1,-1,-1,-1,-1],
[-1,-1,-1,-1,-1,-1],
[-1,-1,-1,-1,-1,-1],
[-1,35,-1,-1,13,-1],
[-1,-1,-1,-1,-1,-1],
[-1,15,-1,-1,-1,-1]]

Output: 4

"""
Explanation -

At the beginning, you start at square 1 [at row 5, column 0].

You decide to move to square 2, and must take the ladder to square 15.

You then decide to move to square 17 (row 3, column 5), and must take the snake to square 13.

You then decide to move to square 14, and must take the ladder to square 35.

You then decide to move to square 36, ending the game.

It can be shown that you need at least 4 moves to reach the N*N-th square, so the answer is 4.

"""

I would like to have a performance review of my solution and would also like to know whether I could make it more efficient.

Here is my solution to this challenge (in Python 3) -

# Uses Breadth First Search (BFS)

def snakes_and_ladders(board):

    """
    :type board: List[List[int]]
    :rtype: int
    """

    board_2 = [0]
    rows, cols = len(board), len(board[0])
    row = rows - 1

    while row >= 0:
        for col in range(cols):
            board_2.append(board[row][col])
        row -= 1
        if row >= 0:
            for col in range(cols - 1, -1, -1):
                board_2.append(board[row][col])
            row -= 1

    visited = [0 for i in range(len(board_2))]
    stack = collections.deque()
    stack.append([1,0])
    while stack:
        current_index, current_dist = stack.popleft()
        for i in range(1,7):
            next_index = min(rows * cols, current_index + i)
            if board_2[next_index] != -1:
                next_index = board_2[next_index]
            if next_index == rows * cols:
                return current_dist + 1
            if visited[next_index] == 0:
                visited[next_index] = 1
                stack.append([next_index, current_dist + 1])

    return -1

Type Hinting

You were halfway there! Good job having the type of values accepted and returned in the method docstring. Now, you can use type hints to show in the header of the method what values are accepted and returned, as follows

from typing import List

def snakes_and_ladders(board: List[List[int]]) -> int:

Consistent Spacing

This

for i in range(1,7):

should be this

for i in range(1, 7):

You have good spacing in the rest of your code, but you should stay consistent and apply this spacing everywhere.

The same for this line

stack.append([1,0]) -> stack.append([1, 0])

Magic Numbers

We're coming back to this line again

for i in range(1, 7):

What is 7 supposed to represent? The max number of rows or columns? What if you have to change it later on to apply to smaller/bigger snakes and ladders boards? I would advise using a variable to hold this value, naming it accordingly.

List Comprehension

You use lots of loops with one line in them. Particularly when appending to lists. Luckily, you can add two lists together and it will merge them.

From this

for col in range(cols):
    board_2.append(board[row][col])

to this

board_2 += [board[row][col] for col in range(cols)]

You can do the same for the next loop a couple lines down

From this

for col in range(cols - 1, -1, -1):
    board_2.append(board[row][col])

to this

board_2 += [board[row][col] for col in range(cols - 1, -1, -1)]


Improving performance of A* search in PHP I need to figure out what can be done to improve the performance of a A* search algorithm in PHP. The idea is to look for the shortest path, in number of hops, between two points in a "map". Actual distance is irrelevant.

This is a minimalistic functional version of what I'm doing:

<?php

    /*

        1 ____ 2 ____ 3 ___ 5
        | \            ____/|
        |  \    ______/     |
        |   4__/            |
        |  /                7
        | /
        6

    */

    $points = [
        1 => [2, 4, 6],
        2 => [1, 3],
        3 => [2, 5],
        4 => [1, 5, 6],
        5 => [3, 4, 7],
        6 => [1, 4],
        7 => [5]
    ];

    function points_pathfind($from, $to)
    {
        global $points;

        if (!isset($points[$from]) || !isset($points[$to]))
            return null;

        if ($from == $to)
            return [$from];

        $seen = [$from];  // improves peformance
        $queue = [];
        $step = [$from, null];
        for (;;)
        {
            if ($queue)
            {
                $step = array_shift($queue);
                if ($step[0] == $to)
                {
                    $result = [];
                    while ($step !== null)
                    {
                        $result[] = $step[0];
                        $step = $step[1];
                    }
                    return array_reverse($result);
                }
            }
            foreach ($points[$step[0]] as $point)
            {
                if (!in_array($point, $seen))
                {
                    $seen[]  = $point;
                    $queue[] = [$point, $step];
                }
            }

            if (!$queue)
                return null;
        }
    }

    echo implode(' -=> ', points_pathfind(1, 7));

Output:

  1 -=> 4 -=> 5 -=> 7

The actual code howover will deal with almost 8000 points with a total of over 15000 connections, in which it all become extremely slow to the point that mapping some paths can take a couple seconds. This is unacceptable to my application. I even considered caching all possibilities to a database (which I will have to do anyway) but it is so slow it would take days only to generate this cache. I'm sure its performance can be improved somehow, I just can't figure out how.

I managed to greatly increase performance by avoiding array resizing and memory reallocation. By allocating all array space I need by start and reusing those arrays instead of recreating and resizing them, the performance of the code has dramatically changed from one search every couple seconds to almost a hundred searches per second.

<?php

    /*

        1 ____ 2 ____ 3 ___ 5
        | \            ____/|
        |  \    ______/     |
        |   4__/            |
        |  /                7
        | /
        6

    */

    $points = [
        1 => [2, 4, 6],
        2 => [1, 3],
        3 => [2, 5],
        4 => [1, 5, 6],
        5 => [3, 4, 7],
        6 => [1, 4],
        7 => [5]
    ];

    $seen = $queue = [];
    foreach ($points as $key => $val)
    {
        $seen[$key] = 0;
        $queue[] = [0, -1];
    }

    function points_pathfind($from, $to)
    {
        global $points, $seen, $queue;

        if (!isset($points[$from]) || !isset($points[$to]))
            return null;

        if ($from == $to)
            return [$from];

        $queue[0][0] = $from;

        array_walk($seen, function (&$a) { $a = 0; });
        $seen[$from] = 1;

        for ($cur = $end = 0; $cur <= $end; ++$cur)
        {
            foreach ($points[$queue[$cur][0]] as $next)
            {
                if (!$seen[$next])
                {
                    if ($next == $to)
                    {
                        $result = [$to];
                        for ($jmp = $cur; $jmp != -1; $jmp = $queue[$jmp][1])
                            $result[] = $queue[$jmp][0];
                        return array_reverse($result);
                    }

                    ++$end;
                    $seen[$next]    = 1;
                    $queue[$end][0] = $next;
                    $queue[$end][1] = $cur;
                }
            }
        }
    }

    echo implode(' -=> ', points_pathfind(1, 7));

The general approach of the algorithm howover is still the same. I wonder if there is a more intelligent way.


While loops in SCSS to produce circles for an animation I've finally written my first loop in SCSS, trying to avoid it for quite some time since I thought it was unnecessary (was writing vanilla CSS not so long ago, so I was used to manual work).

However, I feel this code isn't very optimized, especially since I seem to need two different while-loops (one for divs from center to left, and one for divs to the right).

Demo is available at http://codepen.io/pestbarn/pen/myqvEq

The loops

I found myself needing an @if condition for tweaking the margin for the center div by just 1vh.

// generate 6 circles from center to left
$i: 6;
$s: 38;
$t: -19;
$l: -90;
$z: 2;
@while $i > 0 {
  &:nth-child(#{$i}) {
    @include sqsize($s);
    margin-top: $t+vh;
    margin-left: $l+vh;
    z-index: $z;
  }
  $i: $i - 1;
  $s: $s - 2;
  $t: $t + 1;
  @if $l == -30 {
    $l: $l + 16
  } @else {
    $l: $l + 15;
  }
  $z: $z + 1;
}
// generate 5 circles to the right
$i: 11;
$s: 38;
$t: -19;
$l: 52;
$z: 2;
@while $i > 6 {
  &:nth-child(#{$i}) {
    @include sqsize($s);
    margin-top: $t+vh;
    margin-left: $l+vh;
    z-index: $z;
  }
  $i: $i - 1;
  $s: $s - 2;
  $t: $t + 1;
  $l: $l - 13;
  $z: $z + 1;
}

Is there perhaps a way of only needing to write one while-loop for this? From left to right, the div sizes (where 1 is smallest and 6 is largest) go 6 5 4 3 2 1 2 3 4 5 6, in that order.

I ended up rewriting it like this - much more compact and clean. If there are further ways to optimize this, I'm open to suggestions!

// generate circles from left to right

$i: 1; $s: 38; $t: -19; $l: -89.3; $z: 2;

@while $i <= 11 {
    &:nth-child(#{$i}) {
        @include sqsize($s);
        margin-top: $t+vh;
        margin-left: $l+vh;
        z-index: $z;
    }
    $i: $i + 1;
    $z: $z + 1;
    $b: $i <= 6;
    $l: if($b, $l + 15, $l + 13);
        // <= 6 - circles are coming from the left
        // > 6 - circles are going to the right
    $s: if($b, $s - 2, $s + 2);
        // same logic as above - decrease size towards middle
        // and increase size after passing the middle
    $t: if($b, $t + 1, $t - 1);
        // —〃—
}


Calculating Leibniz of Pi I have this Python program for calculating Leibniz of 'pi'. I am not able to shorten it more. Can anyone here optimise/shorten it?

s,a,b=0,[],[]
for i in range(int(input())):a.append(input())
for x in a:
    for j in range(int(x)):s+=pow(-1,int(j))/((2*int(j))+1)
    b.append(s)
    s=0
for i in b:print("{0:.15f}".format(i))

This is only micro optimization, I don't think it will make a difference, but here you go anyway:

b=[]
for i in range(int(input())):
    s=sum([pow(-1,j)/(2*j+1) for j in range(int(input()))])
    b.append("{0:.15f}".format(s))
for x in b:print(b)

Instead of 3 loops, there is only one
Removed unnecessary intermediary variables
Removed int(j), j is already int
Removed extra brackets, for example (2*int(j))+1 is the same as 2*j+1
Removed an unnecessary s=0

You could of course write the whole thing on a single line, but now that's starting to hurt readability...

for x in ["{0:.15f}".format(sum([pow(-1,j)/(2*j+1) for j in range(int(input()))])) for i in range(int(input()))]:print(x)


Can this be called an insertionSort? Can following code be called insertion sort?
I tried implementation according to my underdtanding...

#include<iostream>
void insertionSort(int* array,int length){
       for(int unsortedIndex=1; unsortedIndex<length;unsortedIndex++){
           for(int sortedIndex=0; sortedIndex<unsortedIndex; sortedIndex++){
            if(array[unsortedIndex] < array[sortedIndex]){
                int temp = array[sortedIndex];
                array[sortedIndex] = array[unsortedIndex];
                array[unsortedIndex] = temp;
            }
        }
    }
}

Yes, it is a variant that does the inner loop from the opposite side than the canonical version.

This has the effect that you do a lot more comparisons than you would otherwise because you cannot early out as soon as you find where the element should be inserted to.

Now for the nitpicks:

There is no need to #include<iostream> for this.

Indentation isn't consistent.