Custom PDO wrapper class I somewhat recently asked a question on here for advice on a PDO function that I had written. I got some good advice from a user, and went with it. I had used that function for about 3 months which is also about how long I have been using PDO in general, until someone mentioned to me that they would have made it a class.

I have never worked with classes before in PHP, and I wanted to give it a shot. So I did.

Now, since this is my first class, I wanted to get some advice on it and some constructive criticism. I want my code to keep on improving, and I want to know the best way to do some things. I don't know if my class is written well, and knowing how I can improve it will definitely help me out.

I've hosted the project on my GitHub.  The project is being developed there, and everyone is welcome to contribute to it.

class GrumpyPdo {

    private $host, $user, $pass, $db, $charset;
    private $dsn, $databaseObject;

    private $killOnError = true;
    private $echoOnError = false;

    //these options can obviously be changed but not "on the fly" (yet).
    private $opt = [
        PDO::ATTR_ERRMODE            => PDO::ERRMODE_EXCEPTION,
        PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
        PDO::ATTR_EMULATE_PREPARES   => false,
    ];

    function __construct($host, $user, $pass, $db, $charset = "utf8") {

        $this->setData([
            "host" => $host,
            "user" => $user,
            "pass" => $pass,
            "db" => $db,
            "charset" => $charset,
        ]);

        $this->createDBObject();

    }

    function query($query, $values = []) {
        if(!empty($values)) {
            $stmt = $this->databaseObject->prepare($query);
            $stmt->execute($values);
        } else {
            $stmt = $this->databaseObject->query($query);
        }
        return $stmt;
    }

    function setData($data) {
        if(!is_array($data)) {
            $this->handleError("GrumpyPDO: Error Code SD13; The setData method requires an array with \$key => \$value pairs.");
        }
        foreach($data as $k => $v) {
            $this->{$k} = $v;
        }
        return true;
    }

    function createDBObject() {
        $this->dsn = "mysql:host={$this->host};dbname={$this->db};charset={$this->charset}";
        $this->databaseObject = new PDO($this->dsn, $this->user, $this->pass, $this->opt);
    }

    function handleError($error) {
        error_log($error);
        if($this->echoOnError) echo $error;
        if($this->killOnError) die($error);
    }

}

Assuming that there is no other code in the class that you deleted here, I'm not sure why you store all the connection details in the object. I recommend to inline createDBObject into the constructor and drop the now unused fields.

After this inline there is also no usage of your setData method and you can drop it, too. In general I would not work with string identifiers, if you are not forced to. If you rename you field user to username you have to keep in mind the string and fix it to. Not all IDEs will support you here.

The following code will do the same as yours:

<?php

class GrumpyPdo
{
    private $databaseObject;

    private $killOnError = true;
    private $echoOnError = false;

    private $opt = [
        PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
        PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
        PDO::ATTR_EMULATE_PREPARES => false,
    ];

    function __construct($host, $user, $pass, $db, $charset = "utf8")
    {
        $dsn = "mysql:host={$host};dbname={$db};charset={$charset}";
        $this->databaseObject = new PDO($dsn, $user, $pass, $this->opt);
    }

    function query($query, $values = [])
    {
        if (empty($values)) {
            $stmt = $this->databaseObject->prepare($query);
            $stmt->execute($values);
        } else {
            $stmt = $this->databaseObject->query($query);
        }
        return $stmt;
    }

    function handleError($error)
    {
        error_log($error);
        if ($this->echoOnError) echo $error;
        if ($this->killOnError) die($error);
    }
}

Except the fact that your handleError is not used any longer. It is up to you, if you also want to drop it or rebuild your query method to catch PDOException and handle them with your method somehow.

From an OOP-point of view, as an slightly different approach, you might want to extend PDO instead of a delegation (with PHP 7.1 strict typing):

<?php

class GrumpyPdo extends \PDO
{
    /**
     * @var bool
     */
    private $killOnError = true;
    /**
     * @var bool
     */
    private $echoOnError = false;

    public function __construct(string $host, string $user, string $pass, string $db, string $charset = "utf8")
    {
        parent::__construct("mysql:host={$host};dbname={$db};charset={$charset}", $user, $pass, [
            PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
            PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
            PDO::ATTR_EMULATE_PREPARES => false,
        ]);
    }

    public function run(string $query, array $values = []): PDOStatement
    {
        try {
            if (empty($values)) {
                $stmt = $this->prepare($query);
                $stmt->execute($values);
            } else {
                $stmt = $this->query($query);
            }
        } catch (PDOException $e) {
            $this->handleError($e->getMessage()); // if you die, you will lose the stack trace?
            throw  $e;
        }
        return $stmt;
    }

    private function handleError(string $error): void
    {
        error_log($error);
        if ($this->echoOnError) echo $error;
        if ($this->killOnError) die($error);
    }
}

As stated in the comment, you need to think about your "error handling", if you want to stay with these debug flags. And as a drawback of PHP inheritance in this case, it is required to rename query to something else like run, because you can't override the original PDO method.

I nearly missed one last thing. In OOP you always need to be aware of the visibility of your methods and field. Please check my usage of private and public.


Leetcode longest valid parentheses Link here
I'll include a solution in Python and C++ and you can review one. I'm mostly interested in reviewing the C++ code which is a thing I recently started learning; those who don't know C++ can review Python code. Both solutions share similar logic, so the review will apply to either.

Problem statement

Given a string containing just the characters '(' and ')', find the length of the longest valid (well-formed) parentheses substring.

Example 1:
Input: s = "(()"
Output: 2
Explanation: The longest valid parentheses substring is "()".

Example 2:
Input: s = ")()())"
Output: 4
Explanation: The longest valid parentheses substring is "()()".

Example 3:
Input: s = ""
Output: 0

Example 4:
Input: s = "(()()()"
Output: 6

Example 5:
Input: s = "((())((((())))"
Output: 8

Both solutions are Oⁿ and pass all test cases including the time limit however, they are taking time more than I expected, specially the c++ version though both share the same logic. I need to improve time as a priority.
longest_parentheses.py
def check_longest(s):
    opened = []
    closed = []
    cum_distance = 0
    max_distance = 0
    for i, ss in enumerate(s):
        if ss == ')':
            if opened:
                closed.append((opened.pop(), i))
        if ss == '(':
            opened.append(i)
    closed = set(sum(closed, ()))
    for j in range(len(s)):
        if j in closed:
            cum_distance += 1
        else:
            cum_distance = 0
        max_distance = max(max_distance, cum_distance)
    return max_distance

if __name__ == '__main__':
    print(check_longest(')((()()()()'))

Stats:
Runtime: 272 ms, faster than 5.14% of Python3 online submissions for Longest Valid Parentheses.
Memory Usage: 15.5 MB, less than 6.57% of Python3 online submissions for Longest Valid Parentheses.

longest_parentheses.h
#ifndef LEETCODE_LONGEST_PARENTHESES_H
#define LEETCODE_LONGEST_PARENTHESES_H

#include <string_view>

int calculate_distance(size_t p_size, const std::vector<size_t> &closed);
int get_longest(const std::string_view &s);

#endif //LEETCODE_LONGEST_PARENTHESES_H

longest_parentheses.cpp
#include "longest_parentheses.h"
#include <vector>
#include <iostream>

int calculate_distance(size_t p_size, const std::vector<size_t> &closed) {
    int cum_distance = 0;
    int max_distance = 0;
    for (size_t i = 0; i < p_size; ++i) {
        if (std::find(closed.begin(), closed.end(), i) != closed.end()) {
            cum_distance++;
        } else {
            cum_distance = 0;
        }
        max_distance = std::max(max_distance, cum_distance);
    }
    return max_distance;
}

int get_longest(const std::string_view &s) {
    std::vector<size_t> opened, closed;
    for (size_t i = 0; i < s.size(); ++i) {
        auto ss = s[i];
        if (ss == ')') {
            if (!opened.empty()) {
                closed.push_back({opened.back()});
                closed.push_back(i);
                opened.pop_back();
            }
        }
        if (ss == '(') {
            opened.push_back(i);
        }
    }
    return calculate_distance(s.size(), closed);
}

int main() {
    std::cout << get_longest(")()())");
}

Stats:
Runtime: 1276 ms, faster than 5.09% of C++ online submissions for Longest Valid Parentheses.
Memory Usage: 9.3 MB, less than 5.04% of C++ online submissions for Longest Valid Parentheses.

Here are some things that may help you improve your program.
C++ version
Use all of the required #includes
The type std::vector<size_t> is used in the definition of calculate_distance() in the header file, but #include <vector> is missing from the list of includes there.  Also, std::max() is used, but #include <algorithm> is missing from the .cpp file.
Minimize the interface
The .h file is a declaration of the interface to your software.  The .cpp is the implementation of that interface.  It is good design practice to minimize the interface to just that which is needed by outside programs.  For that reason, I would remove the calculate_distance() function from the header.
Make local functions static
With the smaller interface as advocated above, the calculate_distance function becomes an implementation detail used only within the .cpp file.  For that reason, it should be made static so the compiler knows that it's safe to inline the function.
Use a switch rather than a series of if statements
The code currently contains this:
for (size_t i = 0; i < s.size(); ++i) {
    auto ss = s[i];
    if (ss == ')') {
        if (!opened.empty()) {
            closed.push_back({opened.back()});
            closed.push_back(i);
            opened.pop_back();
        }
    }
    if (ss == '(') {
        opened.push_back(i);
    }
}

It would be a little faster and a little easier to read if it were instead written like this:
for (size_t i = 0; i < s.size(); ++i) {
    switch(s[i]) {
        case ')':
            if (!opened.empty()) {
                closed.push_back({opened.back()});
                closed.push_back(i);
                opened.pop_back();
            }
            break;
        case '(':
            opened.push_back(i);
            break;
    }
}

Be careful with signed vs. unsigned
What would it mean if calculate_distance return a negative number?  It probably has no sensible interpretation, so for that reason, I'd recommend having it return an unsigned quantity versus a signed int.
Write test functions
You have provided some test input in the description of the problem, but it would be good to write a full test script to exercise the function.  For this kind of thing, I tend to like to use a test object.  Here's the one I wrote for this code:
class ParenTest {
public:
    ParenTest(std::string_view input, unsigned longest)
        : input{input}
        , longest{longest}
    {}
    unsigned operator()() const {
        return static_cast<unsigned>(get_longest(input));
    }
    bool test() const {
        return longest == operator()();
    }
    friend std::ostream& operator<<(std::ostream& out, const ParenTest& test) {
        auto calculated = test();
        return out << (calculated == test.longest ? "ok  " : "BAD ")
            << "\"" << test.input << "\", " << test.longest << ", got " << calculated << "\n";
    }
private:
    std::string_view input;
    unsigned longest;
};

Now here are some test vectors and a main routine:
int main(int argc, char* argv[]) {
    static const std::vector<ParenTest> tests{
        { "(()", 2 },
        { ")()())", 4 },
        { "", 0 },
        { "(()()()", 6 },
        { "((())((((())))", 8 },
        { "(())(())(()))", 12 },
        { "(())(())(()))(())(())(()))(())(())(()))(())(())(()))(())(())(()))(())(())(()))(())(())(()))", 12 },
        { "(())(())(()))(())(())(())(())(())(()))(())(())(()))(())(()((()))(())(())(()))(())(())(()))", 38 },
        { "(())(())(()))(())(())(()))(())(())(()))(())(())(()))(())(()((()))(())(())(()))(())(())(()))", 38 },
        { "(())(())(()))(())(())(()))(())(())(()))(())(())(()))(())(()((()))(())(())(()))(())(())(()))"
          "(())(())(()))(())(())(()))(())(())(()))(())(())(()))(())(()((()))(())(())(()))(())(())(()))", 38 },
    };
    for (const auto &test : tests) {
        std::cout << test;
    }
}

To both assure correctness and also do some timing, I've used my stopwatch template.  The final version of main looks like this:
#include "longest_parentheses.h"
#include "stopwatch.h"
#include <string_view>
#include <iostream>
#include <vector>

// the ParenTest class goes here

int main(int argc, char* argv[]) {
    static const std::vector<ParenTest> tests{
        { "(()", 2 },
        { ")()())", 4 },
        { "", 0 },
        { "(()()()", 6 },
        { "((())((((())))", 8 },
        { "(())(())(()))", 12 },
        { "(())(())(()))(())(())(()))(())(())(()))(())(())(()))(())(())(()))(())(())(()))(())(())(()))", 12 },
        { "(())(())(()))(())(())(())(())(())(()))(())(())(()))(())(()((()))(())(())(()))(())(())(()))", 38 },
        { "(())(())(()))(())(())(()))(())(())(()))(())(())(()))(())(()((()))(())(())(()))(())(())(()))", 38 },
        { "(())(())(()))(())(())(()))(())(())(()))(())(())(()))(())(()((()))(())(())(()))(())(())(()))"
          "(())(())(()))(())(())(()))(())(())(()))(())(())(()))(())(()((()))(())(())(()))(())(())(()))", 38 },
    };
    for (const auto &test : tests) {
        std::cout << test;
    }
    if (argc != 2) {
        std::cout << "Usage: " << argv[0] << " num_trials\n";
        return 1;
    }

    auto iterations = std::stoul(argv[1]);

    Stopwatch<> timer{};
    bool valid{true}

    for (auto i{iterations}; i; --i) {
        valid &= tests.back().test();
    }

    auto elapsed{timer.stop()};
    if (!valid) {
        std::cout << "The program failed!\n";
        return 2;
    }

    std::cout << iterations << " trials took " << elapsed << " microseconds\n"
        " for an average of " << elapsed/iterations << " microseconds/trial\n";
}

Use a better algorithm
The existing code is not so bad, but it's not as efficient as it could be.  On my machine with the code shown above and with one million trials, it takes 5.66 microseconds per invocation of get_longest() on the longest test input, which is also the last of the set.  We can do better.  Here is an alternative routine that uses a std::vector to keep track of each of the starting ( as they occur, but also does the calculation of span length as it encounters each closing ).  Here's how I did it:
unsigned get_longest(const std::string_view& in) {
    struct Span {
        std::size_t begin;
        std::size_t end;
        Span(std::size_t begin, std::size_t end)
            : begin{begin}
            , end{end}
        {}
        std::size_t len() const {
            return end - begin + 1;
        }
        bool is_strictly_enclosing(const Span& other) const {
            return other.begin - begin == 1 &&
                      end - other.end == 1;
        }
        bool is_contiguous_with(const Span& other) const {
            return begin - other.end == 1;
        }
    };
    std::vector<std::size_t> parenmatch;
    std::vector<Span> spans;
    std::size_t longest{0};
    for (std::size_t i{0}; i < in.size(); ++i) {
        switch(in[i]) {
            case '(':
                parenmatch.push_back(i);
                break;
            case ')':
                if (!parenmatch.empty()) {
                    Span curr_span{parenmatch.back(), i};
                    parenmatch.pop_back();
                    if (!spans.empty() && curr_span.is_strictly_enclosing(spans.back())) {
                        // destroy the last one
                        spans.pop_back();
                    }
                    if (!spans.empty() && curr_span.is_contiguous_with(spans.back())) {
                        // merge the contiguous spans
                        spans.back().end = curr_span.end;
                    } else {
                        spans.push_back(curr_span);
                    }
                    longest = std::max(longest, spans.back().len());
                }
                break;
            default:
                parenmatch.clear();
                spans.clear();
        }
    }
    return longest;
}

There is probably still room for improvement, but here's how this works.  First, it keeps track of each Span of matching and nested parentheses.  So () would be correspond to such a span, as would (()).  The code uses is_strictly_enclosing to test for these.  As an example, in (()), the inner pair is found first and would have a span of {1,2}.  The outer pair is found last and has a span of {0,3}.  If we examine the logic, it's now clear what this code is looking for:
bool is_strictly_enclosing(const Span& other) const {
    return other.begin - begin == 1 &&
              end - other.end == 1;
}

Secondly, there is the case of matching but non-nested parentheses such as ()() or (())().  Here again, we use a member function of Span:
bool is_contiguous_with(const Span& other) const {
    return begin - other.end == 1;
}

Using this code, we get the following timing report:

1000000 trials took 562299 microseconds for an average of 0.562299 microseconds/trial

So this version of the code is about 10x faster.  Note too, that it correctly handles malformed input such as ((*)) by reporting 0 for such a string.
Python version
Use elif for mutually exclusive conditions
The check for the opening ( uses if but it would make more sense to use elif here because the two cases (either ( or )) are the only ones considered.  Making just this one change drops each iteration (using the same very long string as in the C++ code) from 74.167 microseconds to 72.444 microseconds.
Don't update values that are unchanged
The code currently has this sequence:
for j in range(len(s)):
    if j in closed:
        cum_distance += 1
    else:
        cum_distance = 0
    max_distance = max(max_distance, cum_distance)

A quick look at the code will verify that max_distance can only get a new value if the if statement is true, so let's move the line there.  This drops the time down to 71.680 microseconds.
Use a faster algorithm
Once again, what works in the C++ version also works in Python.  Here's a Python version of the algorithm above:
def get_longest(s):
    parenmatch = []
    spans = []
    longest = 0
    for i, ss in enumerate(s):
        if ss == '(':
            parenmatch.append(i)
        elif ss == ')':
            if parenmatch:
                curr_span = (parenmatch.pop(), i)
                if spans and spans[-1][0] - curr_span[0] == 1 and curr_span[1] - spans[-1][1] == 1:
                    spans.pop()
                if spans and curr_span[0] - spans[-1][1] == 1:
                    spans[-1] = (spans[-1][0], curr_span[1])
                else:
                    spans.append(curr_span)
                longest = max(longest, spans[-1][1] - spans[-1][0] + 1)
    return longest

This time, the difference is not as dramatic, and the time for this function is 64.562 microseconds.


Capture with User32.dll or hook? I am trying to capture screenshots as fast as possible from when it's displayed, meaning that I want the latency to be minimal.

Currently, I am using user32.dll to capture a window.  So, if I have a game, I capture that window along with borders and everything. It's very fast, but I wonder if hooking into the API (DirectX) and getting the buffer will be faster than this (I don't know how to do that, though).

Here's how I'm doing it:

    [DllImport("user32.dll")]
    [return: MarshalAs(UnmanagedType.Bool)]
    internal static extern bool GetWindowRect(IntPtr hWnd, out Rect lpRect);
    [DllImport("user32.dll")]
    [return: MarshalAs(UnmanagedType.Bool)]
    internal static extern bool PrintWindow(IntPtr hWnd, IntPtr hdcBlt, int nFlags);

private static MemoryStream PrintWindow(string prcname)
{
    IntPtr hwnd;
    using (var proc = Process.GetProcessesByName(prcname)[0])
    {
        hwnd = proc.MainWindowHandle;
    }
    Rect rc;
    NativeMethods.GetWindowRect(hwnd, out rc);
    using (Bitmap bmp = new Bitmap(rc.Width, rc.Height, System.Drawing.Imaging.PixelFormat.Format24bppRgb))
    {
        using (Graphics gfxBmp = Graphics.FromImage(bmp))
        {
            IntPtr hdcBitmap = gfxBmp.GetHdc();
            try
            {
                NativeMethods.PrintWindow(hwnd, hdcBitmap, 0);
            }
            finally
            {
                gfxBmp.ReleaseHdc(hdcBitmap);
            }
        }
        MemoryStream ms = new MemoryStream();
        bmp.Save(ms, System.Drawing.Imaging.ImageFormat.Jpeg);
        return ms;
    }
}

I give it a process name and it will hook to that certain window.  And as I've said, it's not slow; it's fast. But I think it may be slower than what can be achieved, especially since the actual render of the game is often different from the screen (for example, 30FPS on the game, and it's 60HZ on the screen, meaning 50% of the images will be duplicates).

Any ideas about this?

It's very fast, but I wonder if hooking into the API (DirectX) and getting the buffer will be faster than this (I don't know how to do that, though).

I don't know either.
What you have here, probably hasn't been reviewed yet, because there's essentially nothing blatantly wrong with it.

What you've done well
Pretty much everything, as far as I can tell:

Every IDisposable is wrapped in a using block and thus, whether it blows up or not, things get disposed correctly.

try...finally is the best thing you could do here; if there's an exception, it will bubble up, and the finally block will run in all cases, ensuring the resources cleanly get released.

You're working everything in a MemoryStream, which is the only logical thing to do (hitting the file system would be utter nonsense).

Question marks

Returning the MemoryStream means closing and disposing the stream isn't the responsibility of the method that created it - that's a potential issue. I would try to Freeze() the bitmap and return a Bitmap object instead, although there might be a performance penalty in doing that.

If the method is called in a tight loop, does the result of NativeMethods.GetWindowRect(hwnd, out rc); change at all between calls? Perhaps the method could take a Rect as a parameter?

It being static scares me a little, but it's private and you're not showing the class it's a member of... You'll want the method wrapped in an object that you can create and pass around, not just a global/static "helper" method that's part of some undefined ambient context.


Pyramid array builder I'm developing, just for fun and learning, a TypeScript Class that creates an Array<Array<string>> that represents a pyramid. So, for length = 5 the array would look like :

[
  ['X', 'X', '0', 'X', 'X'],
  ['X', '0', '0', '0', 'X'],
  ['0', '0', '0', '0', '0']
]

The class constructor can take a few arguments:

The final array.
The length (width) of the pyramid, the "blank" String and the "piece" string

My implementation looks like:

PLUNKER

interface IPiramide {
    array?          : Array<Array<string>>,
    private blanco? :         string = 'X',
    private trozo?  :         string = 'O'
}

class Piramide implements IPiramide {

  constructor(piramide : IPiramide = {blanco : 'X', trozo : '0', ancho : 0}){

    this.blanco = piramide.blanco;
    this.trozo = piramide.trozo;

        if(piramide.array){
            this.array = piramide.array;
        }
        else if(piramide.ancho){
            this.build(piramide.ancho);
        }

    }

    static getInstance(ancho : number, blanco : string, trozo : string, inverse ? : boolean) : Piramide {
        return new Piramide({blanco, trozo}).build(ancho, inverse);
    }

    private build (ancho : number, inverse? : boolean = false) {
        this.array = [];
        let numberCount = ancho % 2 == 0 ? 2 : 1;
        let middle = Math.floor(ancho / 2);

        while (numberCount != ancho + 2){
            this.array.push(Array.from({length : ancho}, (el, idx)=>{
                let range = (ancho - numberCount)/2;
                return (idx < range || idx > ancho - range - 1)
                  ? this.blanco
                  : this.trozo;
            }));
            numberCount += 2;
        }
        inverse
          ? this.array = this.array.map((el, idx, arr)=>arr[arr.length - idx - 1])
          : this.array;
        return this;
    }

    public getArray (){
        return this.array;
    }

    public getTrozo() {
      return this.array[Math.floor(this.array.length/2)][Math.floor(this.ancho/2)];
    }

    public getBlanco(){
      if(this.array.length){
        return this.array[1][0];
      }
      return null;
    }

    public getAncho(){
      return this.array[0].length;
    }

    public getAlto(){
      return this.array.length;
    }

    toString(){
        return this.array.map( el=> el.join(' ')).join('\n');
    }

    isConcatenable (piramide : Piramide) : boolean {
      console.log('isConcat', this);
      return this.getAlto() == piramide.getAlto();
    }

    reverse() : Piramide{
      this.array = this.array.map( (el, idx, arr) => arr[arr.length - idx - 1]);
      return this;
    }

    concatRight (...piramides : Array<Piramide>) : Piramide {
      return new Piramide({
        array : this.array.map( (el, i)=> el.concat(...piramides.filter(this.isConcatenable.bind(this)).map( pir => pir.getArray()[i] ) ) ),
        blanco : this.blanco,
        trozo : this.trozo
      });
    }

    concatLeft (...piramides : Array<Piramide>) : Piramide {
      return new Piramide({
        array :  this.array.map( (el, i)=> [].concat(...piramides.filter(this.isConcatenable.bind(this)).map( pir => pir.getArray()[i] ) ) ).map( (el, i)=> el.concat(this.array[i])),
        blanco : this.blanco,
        trozo : this.trozo
      });
    }

    concatAbove(...piramides : Array<Piramide>) {
      return new Piramide({
        array : piramides.reduce( (arr, pir) => arr.concat(pir.getArray()) , []).concat(this.array),
        blanco : this.blanco,
        trozo : this.trozo
      });
    }

    concatBelow(...piramides : Array<Piramide>) {
      return new Piramide({
        array : this.array.concat(piramides.reduce( (arr, pir) => arr.concat(pir.getArray()) , [])),
        blanco : this.blanco,
        trozo : this.trozo
      });
    }

}

It works, but it seems to me that this could be improved (I feel weird with my constructor and static methods).

Could this code be improved?

When writing a simple function to return something if something is true or return something else, you should probably only have one return statement in the function.

increasing the amount of returns in a function makes the function less readable and more prone to mistakes.  if you only need one return statement, then only use one return statement.

Personally I would write this

public getBlanco(){
  if(this.array.length){
    return this.array[1][0];
  }
  return null;
}

like this instead

public getBlanco(){
    return this.array.length ? this.array[1][0] : null;
}

using a ternary is really the cleaner way to do this because you are really saying

if there are items in the array
    return item [1][0]
else
    return null


FileSystemWatcher firing multiple events Like others (FileSystemWatcher Changed event is raised twice) I am facing the problem that events of filesystemwatcher are raised twice. I require to catch all non-duplicate events of watcher in real time. I came up with this. So, I want to know if it will be efficient/over-kill/buggy to use this class.

class ModifiedFileSystemWatcher:FileSystemWatcher
{
    public new event RenamedEventHandler Renamed;
    public new event FileSystemEventHandler Deleted;
    public new event FileSystemEventHandler Created;
    public new event FileSystemEventHandler Changed;

    class BooleanWrapper{
        public bool value { get; set; }
    }

    //stores refrences of previously fired events with same file source
    Dictionary<string, BooleanWrapper> raiseEvent = new Dictionary<string, BooleanWrapper>();

    public int WaitingTime { get; set; } //waiting time, any new event raised with same file source will discard previous one

    public ModifiedFileSystemWatcher(string filename="", string filter="*.*"):base(filename,filter)
    {
        base.Changed += ModifiedFileSystemWatcher_Changed;
        base.Created += ModifiedFileSystemWatcher_Created;
        base.Deleted += ModifiedFileSystemWatcher_Deleted;
        base.Renamed += ModifiedFileSystemWatcher_Renamed;
        WaitingTime = 100;
    }

    void PreventDuplicate(FileSystemEventHandler _event, FileSystemEventArgs e)
    {
        if (_event != null)
        {
            //create a reference
            BooleanWrapper w = new BooleanWrapper() { value = true }; //this event will be fired

            BooleanWrapper t; //tmp
            if (raiseEvent.TryGetValue(e.FullPath, out t))
            {
                //a previous event occurred which is waiting [delayed by WaitingTime]
                t.value = false; //set its reference to false; that event will be skipped
                t = w;//store current reference in dictionary
            }
            else raiseEvent[e.FullPath] = w; //no previous event, store our reference

            //Wait on a separate thread...
            System.Threading.ThreadPool.QueueUserWorkItem(delegate
            {
                System.Threading.Thread.Sleep(WaitingTime);
                if (w.value) //if we are still allowed to raise event
                {
                    _event(this, e);
                    raiseEvent.Remove(e.FullPath);//remove instance from dictionary
                }
            }, null);

        }
    }

    //Same as above with different event
    void ModifiedFileSystemWatcher_Renamed(object sender, RenamedEventArgs e)
    {
        if (Renamed != null)
        {
            BooleanWrapper w = new BooleanWrapper() { value = true };
            BooleanWrapper t;
            if (raiseEvent.TryGetValue(e.FullPath, out t))
            {
                t.value = false;
                t = w;
            }
            else raiseEvent[e.FullPath] = w;
            System.Threading.Thread.Sleep(WaitingTime);
            if (w.value)
            {
                Renamed(this, e);
                raiseEvent.Remove(e.FullPath);
            }
        }

    }

    void ModifiedFileSystemWatcher_Deleted(object sender, FileSystemEventArgs e)
    {
        PreventDuplicate(Deleted, e);
    }

    void ModifiedFileSystemWatcher_Created(object sender, FileSystemEventArgs e)
    {
        PreventDuplicate(Created, e);
    }

    void ModifiedFileSystemWatcher_Changed(object sender, FileSystemEventArgs e)
    {
        PreventDuplicate(Changed, e);
    }

}

I copied your code into a new console application. Your constructor is "eating" the base default constructor, and now I can't pass a path parameter, I have to trust that filename will work if I give it a folder path. Let's see..

public ModifiedFileSystemWatcher(string filename="", string filter="*.*"):base(filename,filter)
{
    /* constructor */
}

This would be easier to read on two lines:

public ModifiedFileSystemWatcher(string filename="", string filter="*.*")
    : base(filename,filter)
{
    /* constructor */
}

Changing the names of constructor arguments is confusing. In this case you're passing filename as the base constructor's path parameter, but filter is passed as filter. I would rename filename to path to avoid confusion.

The optional parameters are surprising, too - a FileSystemWatcher exposes a parameterless constructor, one with only a path, and one with both a path and a filter. Your optional parameters add a constructor for a filter without a path.

This is a rather unusual way of assigning a value in a dictionary:

BooleanWrapper w = new BooleanWrapper() { value = true };
BooleanWrapper t;
if (raiseEvent.TryGetValue(e.FullPath, out t))
{
    t.value = false;
    t = w; //store current reference in dictionary
}

This might work, but I find raiseEvent[e.FullPath] = w; makes the intent much more explicit.


Card and Deck classes I would like to know whether I'm going in a correct way or not in building my Poker game. So far, I've implemented card and deck classes and I would like to see your feedback about my work. Feel free to criticize the code of any regard (organization, order, comments ... etc)

card.h

#ifndef CARD_H
#define CARD_H

#include <string>

const int SUIT_MAX(4);
const int RANK_MAX(13);

class Card
{
    friend class Deck; // Deck Class needs to access to Card Class but not vice versa
public:
    explicit Card();
    explicit Card(const int &suit, const int &rank);

    std::string Card2Str() const;

private:
    int generate_suit();
    int generate_rank();
    int get_suit() const;
    int get_rank() const;
    int m_suit;
    int m_rank;
};
#endif

card.cpp

#include <stdlib.h>     /* srand, rand */
#include "card.h"
#include <iostream>

const std::string SUIT[SUIT_MAX]  = {"S", "H", "D", "C"};
const std::string RANK[RANK_MAX]  = {"2","3","4","5","6","7","8","9","10","J","Q","K","A"};

Card::Card()
{
   m_suit = generate_suit();
   m_rank = generate_rank();
}

Card::Card(const int &suit, const int &rank) : m_suit(suit), m_rank(rank)
{

}

int Card::generate_suit()
{
    return rand() % (SUIT_MAX-1) + 0;
}

int Card::generate_rank()
{
    return rand() % (RANK_MAX-1) + 0;
}

std::string Card::Card2Str() const
{
    return SUIT[get_suit()] + RANK[get_rank()];
}

int Card::get_suit() const
{
    return m_suit;
}

int Card::get_rank() const
{
    return m_rank;
}

deck.h

#ifndef DECK_H
#define DECK_H

#include <vector>
#include <iostream>
#include <fstream>
#include "card.h"

using namespace std;

class Deck
{
public:
      explicit Deck();
      void print_Deck() const;
      void getOneCard();
private:
    std::vector<Card> m_deck;

};

#endif

deck.cpp

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

Deck::Deck()
{
    for (unsigned int i(0); i < SUIT_MAX; ++i)
    {
        for (unsigned int j(0); j < RANK_MAX; ++j)
        {
            Card card(i, j);
            m_deck.push_back(card);
        }
    }
}

void Deck::print_Deck() const
{
    unsigned int count(1);

    for (unsigned int i(0); i < m_deck.size(); ++i)
    {
        std::cout << m_deck[i].Card2Str() << " ";
        if ( count == 13 )
        {
          std::cout << std::endl;
          count = 0;
        }
        ++count;
    }
}

void Deck::getOneCard()
{
    Card cd(m_deck.back().get_suit(), m_deck.back().get_rank());
    m_deck.pop_back();
    std::cout << cd.Card2Str() << std::endl;
}

main.cpp

#include <iostream>
#include <vector>
#include <stdlib.h>     /* srand, rand */
#include <time.h>       /* time */
#include <string>

#include "card.h"
#include "deck.h"

int main()
{
    srand (time(NULL));

    Deck _deck;
    _deck.print_Deck();
    _deck.getOneCard();

    std::cout << std::endl;
    _deck.print_Deck();

    std::cout << std::endl;

    return 0;
}

Prefer nullptr to NULL if you're using C++11.
<stdlib.h> and time.h are both C libraries.  Use the respective C++ libraries <cstdlib> and <ctime>.  The rest are okay.  You don't need to include <vector> in main.cpp.
Since you use a storage container for your deck, you can display it using iterators:

for (auto iter = m_deck.begin(); iter != m_deck.cend(); ++iter)
{
    std::cout << *iter << "\n";
}

If you're using C++11, use a range-based for-loop:

for (auto& iter : m_deck)
{
    std::cout << iter << "\n";
}

I don't know why you're using randomness to generate a card within the class.  The constructor should be given input to determine its rank and suit.  A card shouldn't create itself, and you wouldn't want this with an actual card game.  Remove the generate functions entirely, and instead allow the constructor to receive arguments to determine the card's value.
You also don't need to construct the deck within its constructor.  A deck can still start out empty until cards are passed to it.  That should be handled in client code, especially if someone doesn't want a 52-card deck (or a certain game uses something different).
As an alternative to using std::strings, consider enums for the ranks and suits:

// make sure the first rank is explicitly set at 1
// this will make the last rank equal 13
enum Rank { Ace=1, Two, /* ... */, Queen, King };

enum Suit { Spades, Hearts, Diamonds, Clubs };

With this, you'll no longer need the arrays nor the max constants.  Your m_rank and m_suit members should also be one of these enum type (Rank and Suit respectively).  The enums and the Card class declaration should all then be contained in a namespace.

Within your card class, you'll need functions to convert a given enum value to a char value for displaying.  You could, for instance, do this with a switch statement, which will serve as a look-up table.  This will also make it easier to maintain the ranks and suits.

std::string getRank(Rank rank)
{
    switch (rank)
    {
        case Ace: return "A";
        // ...
        default: throw std::logic_error("invalid rank");
    }
}

// same idea with suits

Also note the default statement.  If an invalid rank is passed in, an std::log_error exception will be thrown.  Regardless of what works best for error-handling, there should be a default statement here to properly handle it.
Rename Card2Str() to CardToString().  The former is just awkward wording.  It should also be lowercase as it's a function and not a type.
getOneCard() doesn't specify where in the deck the one card is drawn.  Since it takes a card from the top of the deck, just call it draw() or deal().  On the other hand, it's void and has no parameters, so it's not returning a card.  If this is supposed to just display a top card, then it's ineffective as it's still destroying the top card.

Better yet, consider having a draw() and a top() (perhaps you just need to peek at the top card).  The functionality also looks a bit unclear.  Since you're using std::vector, a storage container, make use of its member functions.  In this case, use back():

draw():

Card Deck::draw()
{
    Card cd = m_deck.back();
    m_deck.pop_back();
    return cd;
}

top():

// return by const& as this Card should only be read-only
// also make the function const as no data members are modified
Card const& Deck::top() const
{
    return m_deck.back();
}

For both of these functions, be sure the caller is first checking for an empty deck.

Side-note: You can use a different storage container if you, say, need to be able to insert into or draw from somewhere else in the deck besides the top or bottom.  For that, you can use an std::list (doubly linked list).  If you instead want to insert into or draw from the top and bottom, you can use an std::deque (double-ended queue).  Otherwise, stay with what you have.
Your deck is missing something: a shuffle function!  You could utilize std::random_shuffle from the STL to do it nicely for you:

void Deck::shuffle()
{
    std::random_shuffle(m_deck.begin(), m_deck.end());
}

You should also check for an empty deck prior to shuffling.


Entity Component System (ECS) I'm currently working on my own game engine for learning purposes, so I've implemented the first iteration of an entity component system using Java 8.

For my approach, I decided to not use any entity class for the system as an entity only holds data and so there is no class required to list all the components itself. This also means that I've got a Component which holds the data and a System which can have access to multiple components (for example a movement system which required data from the position and velocity component).

My systems also have a render method available (thought render systems are the only one who uses them), because the frame rate is not the same as the ticks.

In my ExitSystem, the InputMultiplexer is just for testing purpose there.

How can I improve the code and mainly the system I've got?

The EntityManager:

public class EntityManager {

private ArrayList<Integer> entities = new ArrayList<>();
private ArrayList<ISystem> componentSystems = new ArrayList<>();

public void addEntity(int ID, ISystem... system) {
    this.entities.add(ID);
    this.componentSystems.addAll(Arrays.asList(system));
}

public void init() {
    for(ISystem system : this.componentSystems)
        system.init();
}

public void update() {
    for(ISystem system : this.componentSystems)
        system.update();
}

public void render() {
    for(ISystem system : this.componentSystems)
        system.render();
}

public void dispose() {
    for(ISystem system : this.componentSystems)
        system.dispose();
}

public int getEntity(int id) {
    return this.entities.get(id);
}
}

The IEntityComponent:

public interface IEntityComponent {

}

The ISystem:

public interface ISystem {

public void init();

public void update();

public void render();

public void dispose();

public IEntityComponent[] get();
}

The ExitData:

public class ExitData implements IEntityComponent {

public int exit = GLFW_KEY_ESCAPE;
public int action = GLFW_REPEAT;
}

The ExitSystem:

public class ExitSystem implements ISystem {

@Override
public void init() {
    InputMultiplexer.getInput().addComponent(new InputComponent.InputKeyComponent() {

        @Override
        public boolean onKeyPress(int key, int action) {
            if(key == ((ExitData)get()[0]).exit && action == ((ExitData)get()[0]).action)
                Game.windowShouldClose();
            return true;
        }
    });
}

@Override
public void update() {

}

@Override
public void render() {

}

@Override
public void dispose() {

}

@Override
public IEntityComponent[] get() {
    return new IEntityComponent[] {new ExitData()};
}
}

Components should hold information, that is / can be different from one instance to another; in your example hardcoded ExitData values with conjunction of creating new instances on each get call make it little different to just using a global field. Frankly I am not sure if Exit should even be a system as such.

Your systems should be getting components in packs (grouped by entity) from common pool (instead of creating it each for it self), and then depending on the kind of components there are in said pack, do something with them or not.

I don't understand what your entities list is supposed to do, you don't use it. Also you put values by value, but retrieve by index, which smells like a bug.

I think your problem is that you chose a bad testing case. Something more appropriate would be:

Make Position (with x and y fields) and Velocity (with xvel and yvel) components.
Make Velocity System, which in update loop for entities with Position and Velocity components does x += xvel; y += yvel;
Make 'Render' System, which simply prints Position values.
Add entities, which all have Position component, optionally have Velocity component, and the starting values of the components is different.

This way you would test against systems interacting on components from same pool, systems choosing to work against component set that matches criteria, grouping components by their entity.

--

By saying Your systems should be getting components in packs (grouped by entity) from common pool (instead of creating it each for it self) I meant to say there is no composition relation between systems and components. Your systems should not have components directly attached to it.

Here is minimal example of ECS with the position, velocity and 'rendering' components. It is not optimized, lacks encapsulation, manager misses few methods (at the very least deleteEntity), but I hope it captures the spirit in least lines of code possible.

public class Component {

    public final static int POS = 1 << 0,
                            VEL = 1 << 1,
                            RENDER = 1 << 2;

    public static class Velocity {
        float velx, vely;
    }

    public static class Position {
        float x, y;
    }

    public static class Rendering {
        String name; // for sake of having at least one field inside.
    }
}

--

public class Systems {

    public static class Velocity {
        public void update(EntityManager manager) {
            int required_components = Component.POS | Component.VEL;
            for (int i = 0; i < manager.size; i++) {
                if ((manager.flag[i] & required_components) == required_components){
                    manager.pos[i].x += manager.vel[i].velx;
                    manager.pos[i].y += manager.vel[i].vely;
                }
            }
        }
    }

    public static class Render {
        public void update(EntityManager manager) {
            int required_components = Component.POS | Component.RENDER;
            for (int i = 0; i < manager.size; i++) {
                if ((manager.flag[i] & required_components) == required_components) {
                    System.out.println(String.format("%s: (%f x, %f y)", manager.rendering[i].name, manager.pos[i].y, manager.pos[i].y));
                }
            }
        }
    }

}

--

public class EntityManager {
    public int flag[];
    public Component.Position pos[];
    public Component.Velocity vel[];
    public Component.Rendering rendering[];
    public final int size;

    public EntityManager(int size) {
        this.size = size;
        pos = new Component.Position[size];
        vel = new Component.Velocity[size];
        rendering = new Component.Rendering[size];
        flag = new int[size];
    }

    public int createEntity(int flag) {
        for (int i = 0; i < size; i++){
            if (this.flag[i] == 0) {
                this.flag[i] = flag;
                if ((flag & Component.POS) > 0) pos[i] = new Component.Position();
                if ((flag & Component.VEL) > 0) vel[i] = new Component.Velocity();
                if ((flag & Component.RENDER) > 0) rendering[i] = new Component.Rendering();
                return i;
            }
        }
        return -1;
    }
}

--

public class GameContainer {

    public static void main(String[] args) {
        new GameContainer();
    }

    EntityManager manager;
    Systems.Render renderingSystem;
    Systems.Velocity velocitySystem;

    GameContainer() {
        manager = new EntityManager(5);
        renderingSystem = new Systems.Render();
        velocitySystem = new Systems.Velocity();

        int id = manager.createEntity(Component.POS | Component.VEL | Component.RENDER);
        if (id > -1) {
            manager.pos[id].x = 10;
            manager.pos[id].y = 10;
            manager.vel[id].velx = 1;
            manager.vel[id].vely = 1;
            manager.rendering[id].name = "player";
        }

        id = manager.createEntity(Component.POS | Component.RENDER);
        if (id > -1) {
            manager.pos[id].x = 0;
            manager.pos[id].y = 0;
            manager.rendering[id].name = "tree";
        }
        startGameLoop();
    }

    void startGameLoop() {
        while(true) {
            velocitySystem.update(manager);
            renderingSystem.update(manager);
        }
    }

}


Adding "_old" to a file name This is a trivial problem. The reason I'm here is to get a better idea of how to use Java's numerous APIs for working with files, folders, and paths. I'm hoping this specific example will help.

Problem Statement

I want to rename a file like this:

  /path/to/myFile.txt

to this:

  /path/to/myFile_old.txt

Ugly Solution

I've been trying to find a concise way to do this, and this is the best I can come up with:

Files.move(
        Paths.get(file),
        Paths.get("/" + FilenameUtils.getPath(file) + FilenameUtils.getBaseName(file) + "_old" + FilenameUtils.getExtension(file)));

It works, which is why I'm not on Stack Overflow, but

It's not portable because of the hardcoded "/".
It's really cumbersome -- specifically I don't like how verbose org.apache.commons.io.FilenameUtils is, and how I have to convert my Strings to Paths with Paths.get.

I've looked at using java.io.File or java.nio.file.Paths.resolve... but I haven't found anything tidy.

I think that the approach of the question lacks only a better usage of the API of java.nio.file.Path.

Using the old java.io.File seems very ugly nowadays, when we have java.nio.*, where all the stuff like file separators is supported much better.

// ...
String originalFile = "/path/to/myFile.txt";
Path originalPath = Paths.get(originalFile);
String backupFileName = buildBackupFileName(originalPath.getFileName().toString());
Path backupPath = originalPath.getParent().resolve(backupFileName);
Files.move(originalPath, backupPath); // add options if necessary
// ...

private static String buildBackupFileName(String fileName) {
  String backupSuffix = "_old"; // or move it in a static constant
  int lastDotIndex = fileName.lastIndexOf(".");
  if (lastDotIndex > -1) {
    return fileName.substring(0, lastDotIndex) + backupSuffix + fileName.substring(lastDotIndex);
  }
  return fileName + backupSuffix;
}

Other ways to build the backup file name: use StringBuilder or String.format, with or without the above FilenameUtils.


Sort a strings collection by a search term I'm trying to implement a search bar in which the user sends a search term and the search bar displays the results.

I have a naive solution which separates the original list into three sublists:
a list with all the exact matches, a list with all the phrases containing the search term and a list with all the phrases that doesn't match. Later on I sort the unmatched list using Edit Distance algorithm.

Is this the right way to do it or should I concern a different approach such as suffix trees, etc. ?

public static <T> List<T> sortListByName(final String name, final List<T> itemsList,
      final Function<T, String> itemNameExtractor, int limit) {
    List<T> res = new LinkedList<>();
    List<T> containedList = new LinkedList<>();
    List<T> unmatchedList = new LinkedList<>();

    // First, separates the list into sublists which have exact word match; the search term is a
    // substring; the search term doesn't appear at all.

    for (T item : itemsList) {
      String currItemName = itemNameExtractor.apply(item);
      if (currItemName.contains(name)) {
        boolean isExactMatch = false;
        for (String term : currItemName.split(" ")) {
          if (term.equals(name)) {
            res.add(item);
            isExactMatch = true;
            break;
          }
        }
        if (!isExactMatch) {
          containedList.add(item);
        }
      } else {
        unmatchedList.add(item);
      }
    }
    Comparator<T> comparator = new Comparator<T>() {
      @Override
      public int compare(final T t1, final T t2) {
        Integer n1Length = itemNameExtractor.apply(t1).length();
        Integer n2Length = itemNameExtractor.apply(t2).length();
        return n1Length.compareTo(n2Length);
      }
    };

    Collections.sort(res, comparator);
    Collections.sort(containedList, comparator);
    res.addAll(containedList);

    if (res.size() >= limit) {
      return res.subList(0, limit);
    }

    // Sort unmatched items by their edit distance rank and concatenate them to the contained
    // matches list.
    List<T> editDistList = EditDist.sortList(name, unmatchedList, itemNameExtractor);

    res.addAll(editDistList);
    if (res.size() > limit) {
      return res.subList(0, limit);
    }

    return res;
  }

1) Regex

You could generate a Pattern instance (regex) to check for exact matching instead of doing String and split acrobatics. If I understand it correctly, you are looking for occurrences of 'name' that are neighbored by 'start of line', 'end of line' or 'whitespace'. For example, if you were to search for "doge" like that, you could use the following regex: (?:^|\\s)(doge)(?:$|\\s)
The fat examples would be matched:
doge
dogeX
X doge X
Xdoge
XdogeX

In Java, you'd do it like this:

Pattern p = Pattern.compile("(?:^|\\s)("+search+")(?:$|\\s)");
if(p.matcher(candidate).find()) { } else { }

Compiling Patterns is slow, so only do it once per method call. If your itemsList is small, it's possible that your raw, manual checking is faster, but if that is the case performance should be no issue anyways.

2) Tree

A specialized Infix Tree that takes care of spaces (And you are looking for infix, not suffix, right?) is naturally the vastly superior solution if you can pay the performance costs of creating and maintaining it. If you decide to use an Infix Tree and you are not satisfied, you could write a Java InfixHashMap which provides O(1) lookup speed. (InfixDictionary: Data structure for Infix string lookup)

3) Functional Java 8

And since you are obviously using Java 8, give up on anonymous classes and go all the way:

    Comparator<T> comparator = (t1,t2)->{
        Integer n1Length = itemNameExtractor.apply(t1).length();
        Integer n2Length = itemNameExtractor.apply(t2).length();
        return n1Length.compareTo(n2Length);
    };


Function for plotting Julia Set The Julia set is the set of complex numbers z that do not diverge under the following iteration:

$$ z = z^2 + c $$

Where c is a constant complex number.

Different values of z reach infinity at different rates. A colourful fractal is produced by colouring the complex plane based on the number of iterations required to reach infinity.

function colour = julia(c, total_iterations, image_size, limits)
%   Calculates julia set by iterating z = z^2 + c, where z and c are complex,
%   and recording when z reaches infinity.
%
%   Inputs:
%
%   c                   Fixed complex number, of form a + bi
%   total_iterations    Number of iterations of z = z^2 + c
%   image_size          2D vector with number of complex coordinates in
%                           x and y directions
%   limits              Vector with 4 elements: min x, max x, min y, max y
%
%   Outputs:
%
%   colour              Matrix of doubles, with size equal to image_size.
%                           Plotting this matrix will produce a julia set

im_step = (limits(4) - limits(3)) / (image_size(2) - 1);
re_step = (limits(2) - limits(1)) / (image_size(1) - 1);

reals = limits(1) : re_step : limits(2);            % Real numbers
imags = limits(3) : im_step : limits(4);            % Imaginary numbers
z = bsxfun(@plus, reals(:), (imags(:) * 1i)');      % Complex coordinates
colour = inf(size(z));                              % Colour of Julia set

for iteration = 1:total_iterations
    index = isinf(z);
    % Only perform calculation on the z values that are less than infinity
    z(~index) = z(~index).^2 + c;
    % Colour depends on number of iterations to reach infinity
    colour(index & isinf(colour)) = iteration;
end

colour = colour'; % Transpose so that plot will have reals on the x axis

end

Example:

image_size = [1000 1000];   % Image size
limits = [-1.5 1.5 -1 1];   % Real and imaginary limits
c = -0.4 + 0.6i;            % Fixed complex number
total_iterations = 300;

colour = julia(c, total_iterations, image_size, limits);

I = imagesc(colour);
colormap(hot)
myaa('publish')

Note: myaa is a 3rd party  anti-aliasing function.

This is a very nice, well commented and well structured code!

Your comments in the start are exactly how they should be. You already know this, but I'll say it anyway. Writing a function where you have comments like this in the start aids the user because help functionName will provide the information needed in order to use the function.

There's is only one minor thing I would do differently. You know the end points, and how many steps there are. You have two options:

Calculate the step-size and use it to create a vector with the correct number of steps using colon
Just use what you know directly in linspace

Instead of calculating re_step and do reals = limits(1) : re_step : limits(2); you can use linspace:

reals = linspace(limits(1), limits(2), image_size(1));
imags = linspace(limits(4), limits(3), image_size(2));

The following comment is incorrect: "% Transpose so that plot will have reals on the x axis"

You are working with complex numbers, and in those cases, ' is the complex conjugate transpose, not the regular transpose. I don't know if you want the transpose or the complex conjugate transpose, but it's nice to keep in mind.


Redirecting mobile devices to another page best practise I am redirecting mobile devices from the root index.php to mobile.php file. I only allow access to mobile.php file only when that if statement is true.

This is how I do it:

Detect Devices and store them into variables
If a mobile device (see variables) accesses the web application redirect to mobile.php
In mobile.php check again the same thing and redirect back to index.php if the device is not a mobile device.

  Is this a good practice to redirect mobile devices to another file?

  Is there any tool to check my script from false positives?

index.php

    <?php #detecting mobile devices
    $iphone = strpos($_SERVER['HTTP_USER_AGENT'],"iPhone");
    $android = strpos($_SERVER['HTTP_USER_AGENT'],"Android");
    $palmpre = strpos($_SERVER['HTTP_USER_AGENT'],"webOS");
    $berry = strpos($_SERVER['HTTP_USER_AGENT'],"BlackBerry");
    $ipod = strpos($_SERVER['HTTP_USER_AGENT'],"iPod");
    $ipad = strpos($_SERVER['HTTP_USER_AGENT'],"iPad");

    if($iphone || $android || $palmpre || $ipod || $berry || $ipad == true)
{
 header('Location: mobile.php');
 exit;
}
    ?>

mobile.php

<?php #detecting mobile devices
$iphone = strpos($_SERVER['HTTP_USER_AGENT'],"iPhone");
$android = strpos($_SERVER['HTTP_USER_AGENT'],"Android");
$palmpre = strpos($_SERVER['HTTP_USER_AGENT'],"webOS");
$berry = strpos($_SERVER['HTTP_USER_AGENT'],"BlackBerry");
$ipod = strpos($_SERVER['HTTP_USER_AGENT'],"iPod");
$ipad = strpos($_SERVER['HTTP_USER_AGENT'],"iPad");

if($iphone || $android || $palmpre || $ipod || $berry || $ipad == true)
{
//do nothing
}
else{ #device not one of the variables return to root file
header('Location: index.php');
exit;
}

 ?>

The global task of mobile detection aside, just in regard of the current implementation.

Every time you see a duplicated code, there is a room for the improvement.

$mobile = false;
$mob_agents = ["iPhone","Android","webOS","BlackBerry","iPod","iPad"];
foreach ($mob_agents as $marker) {
if (strpos($_SERVER['HTTP_USER_AGENT'], $marker) !== FALSE) {
    $mobile = true;
    break;
}

now you can use a $mobile variable to test whether it's a movile client or not

A more sophisticated version would involve basic regular expressions- something like this:

$pattern = "/iPhone|Android|webOS|BlackBerry|iPod|iPad/";
$mobile = preg_match($pattern, $_SERVER['HTTP_USER_AGENT']);


Is this user input field secure? I'm pretty new to asp and c# and I have a couple of concerns with the security of the code I have written (shown below).  Basically all it does is allow you to enter a quantity before you add the item to your cart using a razor form.

Every example I see uses validation on input fields, like:

[Required]
[DataType(DataType.EmailAddress)]

Is that sort of validation (or any other) necessary in this case, or is my form secure enough because the controller requires the input of the product id and the quantity, both of which are limited to integers? Also, is there anything else that I should be concerned about with this form, like Javascript injection?

Thanks for your help!

The view has a razor input form:

@using ( Html.BeginForm( "AddToCart", "Cart" ) )
{
    @Html.HiddenFor(_ => item .ProductID)
    @Html.TextBoxFor(_=> item.Quantity)
    <input type="submit" value="+ Add to cart" />
}

The form is posted to the AddToCart action of the CartController:

[HttpPost]
public ActionResult AddToCart(int id, int quantity)
{
    // Retriebe the product from the database
     var addedProduct = storeDB.Products
         .Single(product => product.ProductId == id);

    // Add it to the shopping cart
    var cart = ShoppingCart.GetCart(this.HttpContext);

    cart.AddToCart(addedProduct, quantity);

    //Go back to the main store page for more shopping
    return RedirectToAction("Index");
}

And here is the AddToCart method:

public void AddToCart(Product product, int quantity)
{
    //Get the matching cart and album instances
    var cartItem = storeDB.Carts.SingleOrDefault(
        c => c.CartId == ShoppingCartId
            && c.ProductId == product.ProductId);

    if (cartItem == null)
    {
        // Create a new cart item if no cart exists
        cartItem = new Cart
        {
            ProductId = product.ProductId,
            CartId = ShoppingCartId,
            Count = quantity,
            DateCreated = DateTime.Now
        };
        storeDB.Carts.Add(cartItem);
    }
    else
    {
        // If the item does exist in the car,
        // than add one to the quantity
        var newCount = cartItem.Count + quantity;
        cartItem.Count = newCount;
    }
    // Save changes
    storeDB.SaveChanges();
}

Thanks again for your help!

You can secure against CSRF (pronounced C-surf) by using ValidateAntiForgeryTokenAttribute and AntiForgeryToken method.

How CSRF (Cross Site Request Forgery) works:

  We have 2 actors, an evil person E and a good person G.   E makes an
  evil html image tag on some forum, that links to your website and puts
  a product in your user's cart.

  If G has a cookie for your site and the cookie is not expired, when G
  goes to E's forum, the browser tries to load the html img and it will
  send a Request to your site. Even though G didn't intend to do that...

Implementation:

View:

@using (Html.BeginForm("AddToCart", "Cart"))
{
   @HtmlHelper.AntiForgeryToken()
   // Code -->
}

Controller:

[HttpPost][ValidateAntiForgeryToken]
public ActionResult AddToCart(int id, int quantity)
{
   // Code -->
}

How it prevents CSRF:

In the view a hidden input field with a unique string is created.
A cookie is sendt to the user with the same string. (I believe it is a httpOnly cookie).
The controller checks if the form contains the same string as is in the cookie.

Because person E, cannot form a valid form post the cross site request will fail. But note that if you have XSS (Cross site scripting) vulnerability this security check can be circumvented.

And another thing... If your 2nd method public void AddToCart(Product product, int quantity) is not an action consider using the [NonAction] attribute, because all public methods in a controller are considered by default Action methods. Meaning it is invokable as an Action method.


An ASCII histogram of a die rolled n times A die is rolled n times. Each time, a random value has been inserted into a vector. This vector then is used to generate a sorted map (wrongly called hashmap in the code) that is later used to create an ASCII histogram.

The histogram, for example, looks like this:

    10
    #
    #
7   #
#   #
#   #     5
#   #     #
# 3 #     #
# # #     #
# # # 1   #
# # # #   #
-----------
1 2 3 4 5 6

And here's the code:

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

std::vector<int> roll(int times)
{
    std::vector<int> rand;

    while (times > 0)
    {
        std::random_device seeder;
        std::mt19937 engine(seeder());
        std::uniform_int_distribution<int> dist(1, 6);
        rand.push_back(dist(engine));

        --times;
    }

    return rand;

}

std::map<int, int> histogram_calculate(int times)
{
    std::vector<int> random_numbers = roll(times);

    std::map<int, int> cnt_hashmap;
    auto max_element = 6;

    for (int i = 1; i <= max_element; ++i)
    {
        cnt_hashmap[i] = 0;
    }

    for (auto iter = random_numbers.begin(); iter != random_numbers.end(); ++iter)
    {
        cnt_hashmap[*iter] += 1;
    }

    return cnt_hashmap;
}

std::string histogram_draw(int times)
{
    std::vector<std::string> ret_vec;
    std::map<int, int> histogram = histogram_calculate(times);

    for (int i = 1; i <= histogram.size(); ++i)
    {
        std::string to_add = "";

        if (histogram[i] > 0)
        {
            to_add = std::to_string(histogram[i]);
            std::string column = "\n";
            int j = 0;

            while (j <= histogram[i])
            {
                column += "#";
                column += "\n";

                ++j;
            }

            to_add += column;
        }

        to_add += "--";
        to_add += std::to_string(i);

        ret_vec.push_back(to_add);

    }

    std::string finalize = "";

    for (auto &str : ret_vec)
    {
        finalize += str;
    }

    return finalize;

}

int main() {

    std::cout << histogram_draw(10) << std::endl;
    return 0;
}

There's three functions:

One to fill the random value vector
One to sort out the histogram.
One to display the histogram.

That's about it.

The roll function generates a random sequence of integers. The loop body shows me that you know how to seed a pseudo-RNG with a true source of randomness but you're doing it for every iteration. You should seed the pseudo-RNG once and then use it in the loop. You know how big your rand vector will be by the time you're done with it so you should reserve memory for it so that it the memory doesn't need to be reallocated as you push integers. So your roll function should probably look like this:

std::vector<int> roll(int times)
{
    std::vector<int> rand;
    rand.reserve(times);

    std::random_device seeder;
    std::mt19937 engine(seeder());
    std::uniform_int_distribution<int> dist(1, 6);

    while (times > 0)
    {
        rand.push_back(dist(engine));
        --times;
    }

    return rand;
}

The next function, histrogram_calculate creates a histogram. The histogram is stored in a std::map<int, int> which is not really the best choice. Think about what we're storing.

1 -> 7
2 -> 3
3 -> 10
4 -> 1
5 -> 0
6 -> 5

We want to know the frequency of six numbers. Since six is a compile-time constant, we could use a std::array<int, 6> instead. Initializing an array to zero is as simple as histogram.fill(0). With a std::map<int, int> you need to set keys to zero in a loop.

auto max_element = 6;

for (int i = 1; i <= max_element; ++i)
{
    cnt_hashmap[i] = 0;
}

Using auto here is not a good idea. In fact, int is shorter and provides more information. Also, max_element should be a constant since it doesn't actually change. The max_element variable is a good start but 6 should be available to the roll function as well. You should create a global constant for 6 or perhaps use a template parameter.

// The number of sides on a die
constexpr int num_sides = 6;

histrogram_calculate iterates the vector of random numbers and counts up the frequencies.

for (auto iter = random_numbers.begin(); iter != random_numbers.end(); ++iter)
{
    cnt_hashmap[*iter] += 1;
}

Here, you're using iterators when you could be using a range-for.

for (const int num : random_numbers) {
    cnt_hashmap[num]++;
}

Wait just a minute!

histogram_calculate makes a call to roll which returns a sequence of random integers but then, it's just thrown away! Do we really need the whole sequence in order to create a histogram?

No. We can create the histogram directly from the sequence of random numbers. There's no need to create an array and then just throw it away.

I'll let you try this yourself.

histogram_draw knows how to generate a histogram. histogram_draw should take a histogram and draw it. histogram_draw should have no idea where the histogram is coming from. histrogram_draw should just draw a histogram.

As @Calak pointed out in the comments, histrogram_draw doesn't actually behave as intended so this question is technically off-topic. Might I suggest that you write code to print a horizontal histogram? English text is typically written left-to-right so it is considerably easier to draw a horizontal histogram than a vertical one.

1 | ####### 7
2 | ### 3
3 | ########## 10
4 | # 1
5 | 0
6 | ##### 5

If you make improvements to your code, feel free to create a new question with your improved code. Just make sure that it functions as intended, otherwise, you risk getting flagged for being off-topic.


Sum the difference of two arrays I have a function in App Engine Java where I compare two patterns stored in an int array:

public static int patternMatch(int [] pattern1, int [] pattern2, int range) {

    int max = range * pattern1.length;

    int match = (pattern1.length - pattern2.length) * range;

    for(int i = 0; i < pattern2.length; i++) {
        match += Math.abs(pattern1[i] - pattern2[i]);
    }

    return (max - match) * 100 / max;
}

I am facing very weird problems with respect to performance of this function between the development server and deployment on app engine as listed below:

This function is called in a loop with the intention of finding best match(es).
Performance for a single iteration is critical as there are lot of iterations.
If I were to not have any logic in this code and directly return any integer, my overall code takes 100 ms to complete on an average.
The above code takes anywhere between 200 - 600 ms.
On the development server, if I replace

int match = (pattern1.length - pattern2.length) * range;

by

int match = Math.abs(pattern1.length - pattern2.length) * range;

somehow the performance improves bringing the time taken down to 200 - 300 ms only. No impact on deployment server.
If I remove Math.abs, the performance improves, bringing the average to 150 ms.
I tried replacing Math.abs with bit operations to derive absolute value. I see huge performance improvement on development server ~160 ms. On deployment server it makes things worse ~700 ms.

What I would like to know here is:

Why and how differently do Development server (Windows 7/eclipse/JDK6) and Deployment server behave in terms of performance tweaks?
Is there any better algorithm to the match?

It's hard to say anything without profiling the whole application. Two possible scenarios about this point:

  If I were to not have any logic in this code and directly return any integer, my overall code takes 100 ms to complete on an average.

#1

I guess the application processes the output of patternMatch, so maybe it handles different results differently which varies in time. Consider the following example:

public static int patternMatch(int [] pattern1, int [] pattern2, int range) {
    return 5;
}

int result = patternMatch( [] pattern1, int [] pattern2, int range) {
if (result < 10) {
    // do nothing
} else {
    // do something complicated and slow
}

Of course, it might not be so obvious. Profile it!

#2

JMV is smart about optimizations. If you put the simple return 5 statement into the method (as above) the JVM might have fooled you and did not even call the patternMatch method and probably eliminated or optimized other codes too which call patternMatch.

  For example, reconsider the code in Listing 4: note
  that main not only computes result but also uses result
  in the output that it prints. Suppose that I make just
  one tiny change and remove result from the println. In
  this case, an aggressive compiler might conclude that it
  does not need to compute result at all.

Source: Robust Java benchmarking, Part 1: Issues

I've created a test class for testing the runtime. It takes only 8,766 ms on my machine.

import java.util.ArrayList;
import java.util.List;
import java.util.Random;

import org.junit.Test;

import com.google.common.base.Stopwatch;

public class AppEngineCompareTest {

    private final Random random = new Random();

    @Test
    public void test() {
        final Stopwatch stopwatch = Stopwatch.createStarted();
        final int count = 20000;
        final int[] ranges = createRandomIntegerArray(count);
        final List<int[]> patternOneList = createPatterns(count);
        final List<int[]> patternTwoList = createPatterns(count);
        System.out.println("gen: " + stopwatch);
        stopwatch.reset();
        stopwatch.start();
        int result = 0;
        for (int index = 0; index < count; index++) {
            final int[] pattern1 = patternOneList.get(index);
            final int[] pattern2 = patternTwoList.get(index);
            final int range = ranges[index];
            result += AppEngineCompare.patternMatch(pattern1, pattern2, range);
        }
        System.out.println("run: " + stopwatch);
        System.out.println(result);
    }

    private int[] createRandomIntegerArray(final int count) {
        final int[] ranges = new int[count];
        for (int index = 0; index < count; index++) {
            ranges[index] = random.nextInt();
        }
        return ranges;
    }

    private List<int[]> createPatterns(final int count) {
        final List<int[]> result = new ArrayList<>(count);
        for (int index = 0; index < count; index++) {
            final int[] patterns = createRandomIntegerArray(60);
            result.add(patterns);
        }
        return result;
    }
}


Number of occurrences of strings in array This is a Hackerrank problem (https://www.hackerrank.com/challenges/sparse-arrays/problem).
We're given an array of strings and an array of queries, and have to return an array of the number of occurrences of each query in the array of strings.
For example: s = ['ab', 'abc', 'aba', 'ab']. q = ['ab', atr', 'abc]. We need to return [2, 0, 1].
The constraints are 1 <= n <= 1000, 1 <= q <= 1000, where n and q are the number of strings and queries respectively, and 1 <= |strings[i]|, |queries[i]| <= 20.
Here is my code:
def matchingStrings(strings, queries):
    results = []
    dic = {q:0 for q in queries}
    for s in strings:
        if s in dic: dic[s] += 1
    for q in queries:
        if q in dic: results.append(dic[q])
    return results

Is there anything I can optimize? Speed, memory usage? Anything else?

if q in dic is pointless. You initialized dic so that it does have all queries.
dic = dict.fromkeys(queries, 0) should be a bit faster.
dic is not a particularly meaningful name, counter or ctr would be better.
Creating results at the start of the function gives the false impression that it's needed there already. I'd create it right before the loop that builds it. Then you'd have the complete counting part first, and the complete querying part afterwards. Or better yet, ...
... use a list comprehension, which is faster and nicer.
Python already has a Counter class that does a lot for you and is probably faster.

from collections import Counter

def matchingStrings(strings, queries):
    ctr = Counter(strings)
    return [ctr[q] for q in queries]

For fun: 1000*1000 isn't super big, and HackerRank's calling code really only needs an iterable, so... this also gets accepted:
def matchingStrings(strings, queries):
    return map(strings.count, queries)


Checking if a string matches an array of valid characters I am wondering what a better solution would be for this bit of code:

public static void main( String[] args )
{
    String name = "hello, ddd";

    boolean[] success = new boolean[ name.length() ];
    for( char character: VALID_CHARS ) {
        for( int index = 0; index < name.length(); index ++ ) {
            if( name.charAt( index ) == character ) {
                success[ index ] = true;
            }
        }
    }

    boolean failed = false;
    for( boolean b: success ) {
        if( ! b ) {
            failed = true;
        }
    }

    System.out.println( "You" + ( failed ? " failed": " succeeded" ) + "." );
}

I have an array of valid characters that can be used for a name object, I want to check if each index of the string is valid, if each index is valid return true, otherwise return false. Any help / guidance is appreciated!

memory is surprisingly fast and efficient. If your string-to-test is even modestly large (like 1K), then you may find the following algorithm is efficient. This is especially efficient when the valid-character set is reusable....

boolean[] isvalid = new boolean[Character.MAX_VALUE + 1];
for (char c : VALID_CHARS) {
    isvalid[c] = true;
}
for (char c : name) {
    if (!isvalid[c]) {
       System.out.println("You failed!.");
       return;
    }
}
System.out.println("You succeeded!.");

Now, in your case, there is no reason why VALID_CHARS cannot be declared as:

private static final boolean[] VALIDCHARS = buildValidChars();
private static final boolean[] buildValidChars() {
    ....
}

I have used, and benchmarked this sort of system to great effect when building the JDOM Character validator... you can see how I did it there.... but using a bit-mask representation to use less memory.... which is a bit faster. I also put together a bit of a benchmarh and writeup for the JDOM Verifier performance.


Generating INSERT query from parameters I use NLog everywhere and I really don't like writing the inserts everytime. You need to write the same variable three times: two for the insert and one for the parameter for example:

<target xsi:type="Database" name="Log" dbProvider="System.Data.SqlClient" connectionString="...">
    <commandText>
        INSERT
        INTO [dbo].[Log]([Foo], [Bar], ...)
        VALUES(@FOO, @BAR, ...)
    </commandText>
    <parameter name="@FOO" layout="..." />
    <parameter name="@BAR" layout="..." />
    ...
</target>

It's very error-prone (I always make a mistake somewhere by forgetting one or two of them) and hard to maintain.

I just want to add the parameters and create the INSERT automatically because it's nothing unusual and can be automated so I came up with this idea:

The commandText must not be empty for database targets and I also need to somehow specify the table name. So initially I put the table name there.

<target xsi:type="Database" name="TestLog" commandText="[dbo].[TestLog]" dbProvider="System.Data.SqlClient" connectionString="Data Source=(local);Initial Catalog=TestDb;Integrated Security=True;Connect Timeout=30">
    <parameter name="@Timestamp" layout="${longdate:universalTime=true}" />
    <parameter name="@LogLevel" layout="${level:uppercase=true}" />
    <parameter name="@Logger" layout="${logger}" />
    <parameter name="@Message" layout="${message}" />
    <parameter name="@Exception" layout="${onexception:${exceptionLayout}}" />
</target>

In this case the INSERT would be:

INSERT INTO [dbo].[TestLog](Timestamp, LogLevel, Logger, Message, Exception)
VALUES(@Timestamp, @LogLevel, @Logger, @Message, @Exception)

Next I use a set of extensions to update the commandText. Fortunatelly the CommandText property has a public setter.

static class NLogExtensions
{
    public static void UpdateCommandText(this NLog.Config.LoggingConfiguration config)
    {
        var tableNameMatcher = new Regex(@"^(\[(?<schemaName>.+?)\].)?\[(?<tableName>.+?)\]$");

        var autoCommandTextDatabaseTargets =
            config.AllTargets
                .OfType<DatabaseTarget>()
                .Where(x => tableNameMatcher.IsMatch(x.CommandText()))
                .Select(x => x);

        foreach (var databaseTarget in autoCommandTextDatabaseTargets)
        {
            databaseTarget.CommandText = databaseTarget.CreateCommandText();
        }
    }

    public static string CommandText(this DatabaseTarget databaseTarget)
    {
        return ((NLog.Layouts.SimpleLayout)databaseTarget.CommandText).OriginalText;
    }

    public static string CreateCommandText(this DatabaseTarget databaseTarget)
    {
        const string insertQueryTemplate = "INSERT INTO {0}({1}) VALUES({2})";

        return string.Format(
                insertQueryTemplate,
                databaseTarget.CommandText(),
                string.Join(", ", databaseTarget.Parameters.Select(x => x.Name.TrimStart('@'))),
                string.Join(", ", databaseTarget.Parameters.Select(x => x.Name)));
    }
}

Then I use it like this:

    var config = NLog.LogManager.Configuration;
    config.UpdateCommandText();

    var logger = NLog.LogManager.GetLogger("Program");
    logger.Debug("Hallo NLog!");

I cannot use C# 6

I've splitted this class into multiple pieces and added a new feature that now allows me to insert NULL into nullable columns.

It starts with the LoggingConfigurationExtensions to get the DatabaseTargets

public static class LoggingConfigurationExtensions
{
    public static IEnumerable<DatabaseTarget> DatabaseTargets(
        this LoggingConfiguration loggingConfiguration,
        Func<DatabaseTarget, bool> predicate
    )
    {
        return loggingConfiguration.AllTargets.OfType<DatabaseTarget>().Where(predicate);
    }
}

The query generation logic now belongs to the DatabaseTargetQueryGenerator

public static class DatabaseTargetQueryGenerator
{
    public static void GenerateInsertQueries()
    {
        // The commandText attribute must conatain at least the table name.
        // Each identifier must be enclosed in square brackets: [schemaName].[tableName].

        // https://msdn.microsoft.com/en-us/library/ms175874.aspx?f=255&MSPPError=-2147217396
        // https://regex101.com/r/19GjqR/1
        // h t t ps://regex101.com/delete/QObrsCHVjaX8gGCysAn6yApJ

        var tableNameMatcher = new Regex(@"^(\[(?<schemaName>[a-z_][a-z0-9_@$#]*)\].)?\[(?<tableName>[a-z_][a-z0-9_@$#]*)\]$");

        var databaseTargets = LogManager.Configuration.DatabaseTargets(databaseTarget => tableNameMatcher.IsMatch(databaseTarget.CommandText()));
        foreach (var databaseTarget in databaseTargets)
        {
            databaseTarget.CommandText = databaseTarget.GenerateInsertQuery();
        }
    }

    private static string GenerateInsertQuery(this DatabaseTarget databaseTarget)
    {
        const string insertQueryTemplate = "INSERT INTO {0}({1}) VALUES({2})";

        return string.Format(
            insertQueryTemplate,
            databaseTarget.CommandText(),
            string.Join(", ", databaseTarget.Parameters.Select(x => x.Name())),
            string.Join(", ", databaseTarget.Parameters.Select(x =>
            {
                var sql =
                    x.Nullable()
                        ? string.Format("NULLIF({0}, '')", x.FullName())
                        : x.FullName();

                // Rename the SqlParameter because otherwise SqlCommand will complain about it.
                x.Name = x.FullName();

                return sql;
            })));
    }
}

another small piece helps me to get the actual commandText

public static class DatabaseTargetExtensions
{
    public static string CommandText(this DatabaseTarget databaseTarget)
    {
        return ((SimpleLayout)databaseTarget.CommandText).OriginalText;
    }
}

these extensions encapsulate the parameter name:

public static class DatabaseParameterInfoExtensions
{
    // https://regex101.com/r/wgoA3q/1
    // h t t ps://regex101.com/delete/BDmR7fAqwYQiT5DW5PKAJFAm

    private static readonly Regex ParamRegex = new Regex("^(?<prefix>.)(?<name>[a-z0-9_-]+)(?:[:](?<null>null))?", RegexOptions.IgnoreCase);

    public static string Prefix(this DatabaseParameterInfo parameter)
    {
        return ParamRegex.Match(parameter.Name).Groups["prefix"].Value;
    }

    public static string Name(this DatabaseParameterInfo parameter)
    {
        return ParamRegex.Match(parameter.Name).Groups["name"].Value;
    }

    public static string FullName(this DatabaseParameterInfo parameter)
    {
        return string.Format("{0}{1}", parameter.Prefix(), parameter.Name());
    }

    public static bool Nullable(this DatabaseParameterInfo parameter)
    {
        return ParamRegex.Match(parameter.Name).Groups["null"].Success;
    }
}

this means I can now create a config like this one

<target xsi:type="Database" name="Log" commandText="[dbo].[Log]" dbProvider="System.Data.SqlClient" connectionString="..">
  <parameter name="@Timestamp" layout="${longdate:universalTime=true}" />
  <parameter name="@LogLevel" layout="${level:uppercase=true}" />
  <parameter name="@Logger" layout="${logger}" />
  <parameter name="@Message" layout="${message}" />
  <parameter name="@Exception:null" layout="${onexception:${exceptionLayout}}" />
</target>

where

commandText="[dbo].[Log] specify the [Schema].[Table] to generate the INSERT for
@Exception:null means it is nullable column


Idle Miner part 2: levels service My previous post had no response, so there is part of the whole - levels service. It stores levels with it's attributes. I'd like to hear any suggestions about code readability and code performance.

(function() {
    'use strict';

    angular
        .module('app.mine')
        .service('levels', LevelsService);

    function LevelsService() {
        var service = {
                addLevel: addLevel,
                getNewLevelCost: getNewLevelCost,
                levels: []
            };

        activate();

        return service;

        function activate() {
            service.addLevel();
        }

        function addLevel() {
            service.levels.push(new Level(service.levels.length + 1));
        }

        function getNewLevelCost() {
            return Math.pow(60, service.levels.length);
        }

    }

    function Level(deep) {
        var level = this;

        level.deep = deep;
        level.workers = [];
    }

    Level.prototype.addWorker = levelAddWorker;
    Level.prototype.dig = levelDig;
    Level.prototype.getNewWorkerCost = levelGetNewWorkerCost;

    function Worker(deep) {
        var worker = this;

        worker.deep = deep;
    }

    Worker.prototype.dig = workerDig;

    function levelAddWorker() {
        var level = this;
        level.workers.push(new Worker(level.deep));
    }

    function levelDig() {
        var level = this;
        return {Coal: Math.pow(10, level.deep - 1)};
    }

    function levelGetNewWorkerCost() {
        var level = this;
        return Math.pow(10, level.deep) * Math.pow(1.4, level.workers.length);
    }

    function workerDig() {
        var worker = this;
        return {Coal: Math.pow(10, worker.deep - 1)};
    }

})();

Too many indirections:

activate just calls addLevel
addLevel just adds a level

And, at least in the posted code, these functions are not used anywhere else.
Is there a merit in having these functions?
You could remove these functions and replace with the simple code that adds one level.

These local variables are just noise in the code:

var level = this;

Inside a Level class, if you use this it's perfectly understandable that you're working with a "level", no need for the extra local variable that just makes your code longer for no apparent benefit.

The name deep might be better as depth.


Splitting apart a comma separated list I have a system that outputs a comma separated string into a single column.  I need to grab the individual values out of that list.  The list could have random spaces and commas that need to be accounted for.  I'm new to MySQL and I have a working example that just seems rather long.  I'm wondering if there is a simpler way to achieve this.

delete from example;

drop procedure if exists testProc;

DELIMITER ;;
CREATE PROCEDURE `testProc`(
  in p_willBeUsedLater int(10) unsigned
)
    MODIFIES SQL DATA
myProc:begin

DECLARE theValue varchar(10);

/* Test Cases - PASSED
DECLARE myList varchar(255) default '';
DECLARE myList varchar(255) default null;
DECLARE myList varchar(255) default '1';
DECLARE myList varchar(255) default '1,2';
*/

/* This passes as well. */
DECLARE myList varchar(255) default ' ,, ,   1,2  ,3, 456 , 7 8 9 ABC ,,  ';

SET myList = trim(myList);

/* If first char is a comma remove it. */
WHILE (LOCATE(',',myList) = 1)
DO
    SET myList = SUBSTR(myList, 2);
    SET myList = trim(myList); /* could have been spaces between internal commas */
END WHILE;

/* If last char is a comma remove it. */
SET myList = REVERSE(myList);
WHILE (LOCATE(',',myList) = 1)
DO
    SET myList = SUBSTR(myList, 2);
    SET myList = trim(myList); /* could have been spaces between internal commas */
END WHILE;
/* Put back to correct order. */
SET myList = REVERSE(myList);

WHILE (length(myList) > 0)
DO
    IF (LOCATE(',',myList) = 0) THEN
        SET theValue = trim(myList);
        SET myList = '';
    ELSE
        SET theValue = trim(substring(myList,1,LOCATE(',',myList)-1));
        SET myList = trim(substring(myList,LOCATE(',',myList)+1,length(myList)));
    END IF;

    INSERT INTO `EXAMPLE`(text1, text2) VALUES(myList, theValue);
END WHILE;
end;;
DELIMITER ;

call testProc(36);

select * from example;

Successful output:

    text1                      text2
--------------------------------------------
    2  ,3, 456 , 7 8 9 ABC         1
    3, 456 , 7 8 9 ABC             2
    456 , 7 8 9 ABC                3
    7 8 9 ABC                  456
                               7 8 9 ABC

text1 just shows the current state of the list shrinking with each loop.  text2 is the money column, that's the correct data I need.

Example-ish code

It's clear that you have anonymized the procedure code by replacing real names by example names. In the future, it is better to use your real object & variable names, as someone may actually have things to say to improve your naming, which is one of the most important aspects to code clarity/readability.

Structure of procedure

There are some really strange things about the way your procedure is structured. For one, the test string that it's meant to parse is hard-coded into the procedure code.

/* This passes as well. */
DECLARE myList varchar(255) default ' ,, ,   1,2  ,3, 456 , 7 8 9 ABC ,,  ';

I feel sure that this is temporary, but if one of my colleague SQL developers handed that to me for a code review, I would tell them they need to go finish the code first so it's actually usable for any test case. So the string to parse should be an argument to the procedure...

CREATE PROCEDURE `sp_ParseCommaSeparatedString`(
  in p_commaSeparatedString varchar(255),
  in p_willBeUsedLater int(10) unsigned
)

And as such, you can just document your test cases in block comment with procedure calls. This also documents to a reasonable extent how the procedure is meant to be used.

/* Test Cases - PASSED
call sp_ParseCommaSeparatedString('');   --0 rows
call sp_ParseCommaSeparatedString(null); --0 rows
call sp_ParseCommaSeparatedString('1');  --1 row
call sp_ParseCommaSeparatedString('1,2');--2 rows
--Complex test case, 5 rows
call sp_ParseCommaSeparatedString(' ,, ,   1,2  ,3, 456 , 7 8 9 ABC ,,  ');
*/

Unused parameter

This is dead code, as the name indicates: in p_willBeUsedLater int(10) unsigned

It would be more beneficial to not add it in until it is ready, so that the stored procedure calls aren't unnecessarily confusing. You would have 2 options here which would be better. MySQL is a bit clunky for something like this, as far as SQL goes, but it can still be done fine.

1) Document as a TODO

CREATE PROCEDURE `sp_ParseCommaSeparatedString`(
  in p_commaSeparatedString varchar(255)
)
  MODIFIES SQL DATA
myProc:begin

/*
 * TODO: implement additional input param:
 *   in p_someSensiblyNamedNumber int(10) unsigned
 * This parameter will be used for [...]
 */

2) Provide a default argument

CREATE PROCEDURE `sp_ParseCommaSeparatedString`(
  in p_commaSeparatedString varchar(255),
  in p_someSensiblyNamedNumber int(10) unsigned
)
  MODIFIES SQL DATA
myProc:begin

IF p_someSensiblyNamedNumber IS NULL THEN
  SET p_someSensiblyNamedNumber = 36;

Either will work fine and which is better really depends on what you are needing to do with it. From the code your presented, that parameter is not used for anything right now and it's not clear from what you named it what it is supposed to do, so... hard to say.

Now we can clean up the top a bit.

DECLARE stringToParse varchar(255);
SET stringToParse = TRIM(p_commaSeparatedString);

Results table

At the moment, you are finding yourself having to delete the rows from the table before you call the procedure, which is not ideal at all and will lead to errors. Instead, do that in your procedure, either at the top when you start, or just before you start inserting values in it (in case the code prior to that takes long, this would mitigate the possibility of another process adding rows into the same table which this is running).

Just check if there are any rows in it, if there are not just skip, and if there are, delete them.

IF EXISTS (SELECT 1 FROM example) THEN
    DELETE FROM example;
END IF;

WHILE (length(myList) > 0)
DO...

Overall approach

SQL clearly was not designed to parse strings of text like a more general language/domain could; if things are not in tables, rows and columns, they are much more difficult to manipulate with SQL. With that in mind, I would say your logic is probably about as good as it is going to get, while still maintaining readability.

Perhaps consider adding a small amount of code comments in the conditionals in your last WHILE section so that the next person who has to look and/or debug this code (which may very well be your future self) will not have too much difficulty.

Nitpicks

DELIMITER ;;

This would make more sense if you set the delimiter to something that looks less like the normal delimiter. I have most often seen used $$, // and || as delimiters.

trim(substring(myList,LOCATE(',',myList)+1,length(myList)));

Try to get in the habit of spacing your operators, commas, etc. to make it easier to read. Also, you are using mixed capitalization of keywords, which just looks a bit messy. Pick one capitalization style and stick to it.

TRIM(SUBSTRING(myList, LOCATE(',', myList) + 1, LENGTH(myList)));


Singleton that holds the app's data I currently have a singleton that holds the translation objects in my app. View controllers can subscribe to its delegate so they can update their views if new translation objects have been added. This is what the class looks like

class TranslationItems {
    var delegate: TranslationItemsDelegate?

    static let shared = TranslationItems()

    private var set = Set<Translation>()

    // Public API

    public func add(object: Translation) {
        set.insert(object)
        delegate?.newItemAdded()
    }

    public func remove(object: Translation) {
        set.remove(object)
        delegate?.someItemDeleted()
    }

    public func getAll() -> [Translation] {
        return Array(set)
    }
}

protocol TranslationItemsDelegate {
    func newItemAdded()
    func someItemDeleted()
}

View controllers can use it like this:

// Add or remove objects
TranslationItems.shared.add(object: translation)

// Subscribe for notifications
TranslationItems.shared.delegate = self

Does this singleton class look ok or could it be improved?

The problem I see there is that when a new view controller subscribes, the previous one stops receiving notifications. There reason is because delegation is a one to one relationship.

"View controllers can subscribe to its delegate" That is wrong, it should be something like: "A view controller can subscribe to its delegate"

If that is what you want, then it is OK.

On the other hand, if you want to have several view controllers listening for those events, you need a 1 to many relationship. You can use NoticationCenter, KVO, Observables (RxSwift or Combine), or create your own solution, like:

addObserver removeObserver an array to save these observers (that conform to a protocol) and when triggering the event, just use observers.forEach { $0.sendEvent... }


Experienced ruby programmer starting with Java I'm a ruby programmer who's writing Java for university. I'd love to get some feedback on how my Java looks and how to improve it to conform with the norms.

import java.util.Set;
import java.util.HashSet;
import java.util.List;
import java.util.ArrayList;

public class Cell {
  private Set<Integer> possibles;
  private List<CellGroup> groups;

  public Cell() {
    this.possibles = new HashSet<Integer>(java.util.Arrays.asList(1,2,3,4,5,6,7,8,9));
    this.groups = new ArrayList<CellGroup>();
  }

  public void register_cell_group(CellGroup group) {
    this.groups.add(group);
  }

  public void set(Integer number) {
    // Make the only possibile the set number
    this.possibles = new HashSet<Integer>();
    this.possibles.add(number);

    this.inform_groups();
  }

  public void not(Integer number) {
    this.possibles.remove(number);

    if (this.possibles.size() == 1) {
      this.inform_groups();
    }
  }

  public boolean is_solved() {
    return this.possibles.size() == 1;
  }

  private void inform_groups() {
    for (int i = 0; i < this.groups.size(); i++) {
      this.groups.get(i).cell_solved(this, this.possibles.toArray(new Integer[0])[0]);
    }
  }
}

Edit: As Landei suggested, use camelCase! Thanks Landei.

Your set function does not need to take an Integer. Idiomatic code would only use an int. Autoboxing means you can still add an int to this.possibles.
Why don't you use is_solved in not?
not is surprising, and I need to read the code to know what it does. I'd suggest naming it remove?
What about this.possibles.iterator().next() instead of the toArray trick?
Use a for each statement in inform_groups:

for(CellGroup g : this.groups) {
     g.cell_solved(this, this.possibles.iterator().next());
}