Cartesian Product in Go I am still fairly new to go, and would appreciate any tips on style, best practices, etc, but am especially interested to know if this non-recursive cartesian product implementation can be made significantly faster (eg, when the number of results in the result set is on the order of 1e9).

I've played around with adding more goroutines, but parallelism doesn't seem to help much, if at all.  I may be missing a much better approach though.

https://play.golang.org/p/H-M6CbmeFoV

package main

import (
    "fmt"
)

// Given a mixed base, returns a function that:
//
// Increments a number, represented as a slice of digits, defined
// in that base.  For example, if our base is 2 3 2, we'll count
// like this:
//
// 0 0 0 ; 0 0 1; 0 1 0; 0 1 1; 0 2 0; 0 2 1;
// 1 0 0 ; 1 0 1; 1 1 0; 1 1 1; 1 2 0; 1 2 1;
func mixedBaseInc(bases []int) func(*[]int) {

    return func(digits *[]int) {
        ret := *digits
        i := len(ret) - 1
        for {
            base := bases[i]
            ret[i] = (ret[i] + 1) % base
            noCarry := ret[i] != 0

            if noCarry || i == 0 {
                return
            }
            i--
        }
    }
}

func pick(indexes []int, params [][]interface{}) []interface{} {
    ret := make([]interface{}, len(params))
    for i, x := range indexes {
        ret[i] = params[i][x]
    }
    return ret
}

func XProd(params ...[]interface{}) chan []interface{} {
    var paramLens, digits []int
    numElms := 1
    c := make(chan []interface{})

    for _, x := range params {
        paramLens = append(paramLens, len(x))
        numElms *= len(x)
        digits = append(digits, 0)
    }

    inc := mixedBaseInc(paramLens)

    go func() {
        defer close(c)
        for i := 0; i < numElms; i++ {
            c <- pick(digits, params)
            inc(&digits)
        }
    }()

    return c
}

func main() {
    for x := range XProd([]interface{}{1, 2, 3}, []interface{}{4, 5}) {
        fmt.Println(x)
    }
}

I think you should replace mixedBaseInc with a generator that returns the combinations of indexes. That would simplify XProd by taking out numElms and the construction of digits.

That gives you the option to parallelise XProd by instantiating more instances of the goroutine that outputs the product vectors (because the closure no longer binds digits). If that is the bottleneck then that improves throughput.

However it depends on the program where this is used; if most of the work is done by the consumer of the output vectors then the best speed-up is for the consumer to consume in a way that can be parallelised.

An alternative approach is to build the output vectors one element at a time — https://github.com/schwarmco/go-cartesian-product/blob/master/cartesian.go for example . That solution has pros and cons and it's a bit more complicated to increase its parallelism, but it might be much better on some inputs (perhaps if there are a large number of small input sets to the product).


Developing a JavaScript library I am slowly learning to master JavaScript and particularly the art of self-executing/invoking functions. I have developed a simple JavaScript plugin and I think I have followed the correct standards.

It is pretty basic in what it does, it is to store information on a oData BATCH request, which provides functions to add to the batch request, get the string representation (which can then be used in a HTTP request), and I am expanding on it as we speak.

However, the code is as follows. It was using jQuery but not any more due to resource constraint.

I am generally after comments on what could have been done better. I am doing this in an attempt to reduce the window namespace. Therefore BatchInfo and BatchCacheRequest (which are a bit more specific to my code) are not clogging up the name space, nor do I have to keep an array anywhere do hold the BatchInfo objects.

(function () {
    var info = [],
        b = {
            clear: function () {
                info = [];
            },

            get: function () {
                return info;
            },

            add: function (command, url, data) {
                info.push(new BatchInfo(command, url, data));
                return info;
            },

            toString: function () {
                var batchString = ['--batchfull'];

                batchString.push('Content-Type: multipart/mixed; boundary=batchitems');

                for (var i = 0; i < info.length; i++) {
                    batchString.push('');
                    batchString.push('--batchitems');
                    batchString.push('Content-Type: application/http');
                    batchString.push('Content-Transfer-Encoding: binary');
                    batchString.push('');
                    batchString.push(info[i].Command + ' ' + info[i].URL + ' HTTP/1.1');

                    if (info[i].Command !== 'DELETE') {
                        batchString.push('Content-Type: application/json;type=entry');
                    }

                    batchString.push('');
                    batchString.push(info[i].Data);
                }

                return batchString.join('\r\n') || '';
            }
        };

    function BatchInfo(command, url, data) {
        this.URL = url || '';
        this.Command = command || '';
        this.Data = data || '';
    }

    function BatchCacheRequest(info, description) {
        this.Info = info || '';
        this.Description = description || '';
        this.SubmitDateTime = new Date().toString('yyyy-MM-dd HH:mm');
    }

    window.batch = b;

})();

Usage is:

batch.add('POST', 'http://www.mywebsite.com', '[some data to post]');

Then toString() will return me a string which is formatted like a BATCH request (which I will use later on in the code).

Is there anything I'm doing stupidly bad here?

Not "stupidly bad", but you're invoking .push() far more times than needed. It's a variadic function, so group the items into single invocations.

Additionally, I'd probably put a toString() method on the BatchInfo.prototype that will give back the string representation of the object.

Then put the objects directly into what was the batchString array. That way the .toString() method will be invoked when you call .join().

I've included additional small changes below as well.

(function () {
    var info = [],
        b = {
            clear: function () {
                info.length = 0; // Clear all references
            },
            get: function () {
                return info;
            },
            add: function (command, url, data) {
                info.push(new BatchInfo(command, url, data));
                return info;
            },
            toString: function () {
                // Put the `BatchInfo` objects directly into the Array.
                // Now we don't need the `for` loop.
                return ['--batchfull',
                        'Content-Type: multipart/mixed; boundary=batchitems']
                           .concat(info).join('\r\n'); // Not needed-- || '';
            }
        };
    function BatchInfo(command, url, data) {
        this.URL = url || '';
        this.Command = command || '';
        this.Data = data || '';
    }
    // Handle the `toString()` representation here
    BatchInfo.prototype.toString = function() {
        var parts = ['', '--batchitems',
                     'Content-Type: application/http',
                     'Content-Transfer-Encoding: binary',
                     '',  this.Command + ' ' + this.URL + ' HTTP/1.1'];

        if (this.Command !== 'DELETE') {
            parts.push('Content-Type: application/json;type=entry');
        }
        parts.push('', this.Data);
        return parts.join('\r\n');
    };
    function BatchCacheRequest(info, description) {
        this.Info = info || '';
        this.Description = description || '';
        this.SubmitDateTime = new Date().toString('yyyy-MM-dd HH:mm');
    }
    window.batch = b;
})();


Optimising and error handling Linq query I have 2 tables as below:

Downtime

DowntimeCode

I am then doing this to get the Description field from the DowntimeCode table for the DowntimeCode with the sum highest value in the Downtime table for the current date and a specified line:

public string TopCodeForToday(string line)
    {
        var date = DateTime.Now;

        using (var db = new PillowContext())
        {
            var qry = (from d in db.DownTimes
                        where DbFunctions.TruncateTime(d.DateTime) == DbFunctions.TruncateTime(date) && d.Line == line
                        group d by new { d.Code }
                            into g
                            let total = g.Sum(x=>x.Amount)
                            orderby total descending
                            select new { g.Key.Code, Total = total }).ToList();

            for (int i = 0; i <= 1; i++)
            {
                foreach (var item in qry)
                {
                    int x = item.Code;
                    var result = from r in db.DownTimeCodes
                        where r.Code == x
                        select r.Description;

                    return result.FirstOrDefault();
                }
            }
        }
        return "Fail";
    }

Now this is working OK but I can't help but think there is a simpler way of doing it. I'm also not too hot on error handling (hence there not being any). Can anyone make any recommendations on how to improve and streamline the above?

Not having any error handling in your TopCodeForToday() method, means if anything goes wrong, you're putting the burden of catching any exceptions onto the caller.

And this isn't necessarily a bad thing. It depends on what the client code is expecting.

We don't know what class this method is a member of. The problem is that the naming of the method doesn't convey any information about what's going on, and whether the calling code should wrap it with a try/catch block. If the class is clearly an object that accesses a database and only does SELECT operations, then it's probably fine. Otherwise the method's name should say what it's doing - SelectTopCodeForToday() would be a better name regardless of all of the above, be it only because it starts with a verb.

I don't think returning a string that says "Fail" is a good idea. Replace FirstOrDefault() with First() and let it throw an exception, and then your client code will know that something is wrong with the data.

The problem with returning a "Fail" string, is that it's a valid string. And then the client code needs to verify whether the function returned "Fail" or not. By throwing, you only handle the "happy" path, making your code more focused; and you handle exceptional situations with ...exceptions.


Phone Number and Email Address Extractor - Is there any way to simplify this code? I'm very new to programming and I don't feel confident about the readability of this program.

This program gets the text you copied then extracts phone numbers and email addresses in the text. Once those are extracted, you can paste them anywhere(Eg: notepad).

Are there any ways that I can improve this code? Any suggestions will surely be appreciated. Thanks!

import pyperclip, re
# Python 3 - 30/04/2020
# emailAndNumberExtractor.py - finds phone and email addresses on the clipboard

emailRegex = re.compile(r'''(
    [a-zA-Z0-9._%+-]+           # username
    @                           # @ symbol
    [a-zA-Z0-9.-]+              # domain name
    (\.[a-zA-Z]{2,4})           # dot something
    )''', re.VERBOSE)

phoneRegex = re.compile(r'''(
    (\d{3}|\(\d{3}\))?          # area code
    (\s|-|\.)?                  # separator
    \d{3}                       # first 3 digits
    (\s|-|\.)                   # separator
    \d{4}                       # last 4 digits
    (\s*(ext|x|ext.)\s*\d{2,5})?# extension
 )''', re.VERBOSE)

all_emails = "\nEMAILS FOUND:\n"
all_numbers = "\nNUMBERS FOUND:\n"

# get copied text and paste it to text var
text = str(pyperclip.paste())

# check if there are phone numbers/ emails in the text
if len(emailRegex.findall(text)) > 0 :
    for email in emailRegex.findall(text):
        all_emails += '\n\t' + email[0]
else:
    all_emails += "\n\tSorry, there are no emails."

if len(phoneRegex.findall(text)) > 0:
    for num in (phoneRegex.findall(text)):
        all_numbers  += '\n\t' + num[0]
else:
    all_numbers += "\n\tSorry, there are no phone numbers."

# Collects all numbers and emails found
matches = all_numbers + "\n" +  all_emails

print(text)
pyperclip.copy(matches)
print("Copied to clipboard:")
print(matches)
```

There are a couple of pointers. I possibly got too ahead of myself with the solution, but hopefully, a step-by-step walkthrough clarifies everything.
I am happy to answer any questions you have.

The overall suggestion is:

"""Email and number extractor - finds phone and email addresses on the clipboard.
Python 3 - 30/04/2020
"""
import re
from io import StringIO

import pyperclip

class PastingIO(StringIO):
    def write_items_to_string(self, items, items_name: str):
        self.write(f"{items_name} found:\n\n".upper())

        if items:
            for item in items:
                self.write(f"\t{item}\n")
        else:
            self.write(f"\n\tSorry, there are no {items_name}")

        self.write("\n")  # padding newline

def main():
    email_regex = re.compile(
        r"""(
            [a-zA-Z0-9._%+-]+           # username
            @                           # @ symbol
            [a-zA-Z0-9.-]+              # domain name
            (\.[a-zA-Z]{2,4})           # dot something
        )""",
        re.VERBOSE,
    )

    phone_regex = re.compile(
        r"""(
            (\d{3}|\(\d{3}\))?          # area code
            (\s|-|\.)?                  # separator
            \d{3}                       # first 3 digits
            (\s|-|\.)                   # separator
            \d{4}                       # last 4 digits
            (\s*(ext|x|ext.)\s*\d{2,5})?# extension
        )""",
        re.VERBOSE,
    )

    # get copied text and paste it to text var
    # text = str(pyperclip.paste())
    text = """
        This is sample text.
        hello@you.com
        There is nothing here.
        rofl@mao.org
        234-654-1234 is a telephone number.
        So is 123-456-7890!
        However, 1234-32-342 is invalid.
        Hello World!
        invalid@email.world should be invalid all-together.
        Check out this email: a@b.c
    """

    output = PastingIO()

    print("Working on:", text, sep="\n")

    matching_targets = {
        "numbers": {"regex": phone_regex,},
        "emails": {"regex": email_regex,},
    }

    for target_name, target_dict in matching_targets.items():
        regex = target_dict["regex"]
        target_dict["results"] = [match[0] for match in regex.finditer(text)]
        output.write_items_to_string(target_dict["results"], target_name)

    match_summary = output.getvalue()

    pyperclip.copy(match_summary)
    print("Copied to clipboard:", match_summary, sep="\n")

if __name__ == "__main__":
    main()

Walkthrough

"""Email and number extractor - finds phone and email addresses on the clipboard.
Python 3 - 30/04/2020
"""

is a module docstring.
It basically supersedes the comments of the same content that you had in your code.
The module docstring is more powerful.
For example, other people can invoke it with help():

~$ python -c "import emailAndNumberExtractor; help(emailAndNumberExtractor)"
Help on module emailAndNumberExtractor:

NAME
    emailAndNumberExtractor

DESCRIPTION
    Email and number extractor - finds phone and email addresses on the clipboard.
    Python 3 - 30/04/2020

FUNCTIONS
    main()

FILE
    ~\emailandnumberextractor.py

import re
from io import StringIO

import pyperclip

I did not touch either the re import or your re.compile statements.
You will have to decide for yourself if these are correct.
I did however include a sample text to match against.

StringIO is used here as a sort of temporary file.
When you look for and find matches, these matches should be collected, ideally in a mutable sequence like a list.
This is your data.
In your code, you irretrievably intertwine that data with its display, here the print of a str.
You did this through string concatenation.
But what if you would like to forward the list of found matches somehow, i.e. use it a second time, in another context?
You cannot, because the data is mingled with the string.

As such, StringIO will be a virtual, in-memory file for us to write to.
It will hold the formatting, i.e. indentations and newlines.
The data will come from somewhere else and be kept separate.
Since we do not need a real file, this will do.
I chose this over multi-line string formatting, since that is not straightforward to do and has many caveats.

class PastingIO(StringIO):
    def write_items_to_string(self, items, items_name: str):
        self.write(f"{items_name} found:\n\n".upper())

        if items:
            for item in items:
                self.write(f"\t{item}\n")
        else:
            self.write(f"\n\tSorry, there are no {items_name}")

        self.write("\n")  # padding newline

This class definition might be the trickiest part.
Don't worry if you haven't learned about classes yet.
It can be understood this way:

PastingIO, our new custom class, is inheriting from StringIO.
This means that PastingIO will have all the functionalities of its parent, StringIO.
I explained the latter earlier.
The reason I did this was to extend StringIO by a simple functionality.
This functionality is the function write_items_to_string.
If function definitions occur in classes, they are called methods.

Methods are much like normal functions, but since they are methods of a class, they usually do something with their class.

Now, doing something with the class itself, PastingIO, is not getting us far.
See, a class is like a blueprint.
It contains all the instructions of how something should look like and behave.
But it is not yet something substantial.
A class is like a construction plan, but we are interested in the house that will be built based on that plan.
The plan itself is a (to us, useless) piece of paper.

To build the "house", instantiation is needed.
This is done in the code when we call PastingIO().
The parantheses are important.
It is the instruction to actually build an object from the class definition.
Lastly, we give this thing a name, by just assinging it to a variable (output).
More on that later.

The object we get is like StringIO, but with the added functionality.
You will note that this functionality is much like the loops you defined to concatenate to the string.
I created this method since those loops do identical things.
Now, you won't have to repeat yourself anymore.
This is in adherence to the DRY principle: don't repeat yourself.

As such, I hope write_items_to_string is self-explanatory.
The self just refers to the object instance we created, output.
It means that we act on output.
In this case, imagine the write method to just write to a file, like you do with real files using with open("file.txt", "w") as f: f.write("Hello"), only virtually.
It builds and holds our output string, including all the formatting.

Here, a class is important to hold the string.
A function alone cannot (rather: should not) hold onto anything (referred to as a state).

Skipping the re.compile() statements, they are unchanged.
The text statement is just a sample text to work on.

output = PastingIO()

This is the line mentioned above.
output is now an instance of our custom PastingIO class.
It has all the capabilities of a StringIO (much like a virtual text file), with the added write_items_to_string method, capable of modifying the content of output.

    matching_targets = {
        "numbers": {"regex": phone_regex,},
        "emails": {"regex": email_regex,},
    }

This is a nested dictionary. matching_targets is a dictionary, but each of its values is also a dictionary.
This affords us a neat and organized way to store all related data and functionalities, without repeating ourselves.

    for target_name, target_dict in matching_targets.items():
        regex = target_dict["regex"]
        target_dict["results"] = [match[0] for match in regex.finditer(text)]
        output.write_items_to_string(target_dict["results"], target_name)

This is where, finally, the business happens.
One key aspect I found in your code was that findall was called twice.
You compiled the regex outside the loops, only once, which is great.
But even findall only needs to be called once for each regex.
Note that I replaced it with finditer, a simple iterator that does the same thing here.
It returns matches only if you ask it to (lazy stuff), which we do in a loop.

(List) comprehensions are faster than the "manual" equivalent using for or while loops.
The list at target_dict["results"] will hold all the found strings (well, only the first found capture group).
Note that we iterate over matching_targets, and thus do both emails and numbers in one sweep.
The results are then found in for example matching_targets["emails"]["results"], so two keys are required.

Note that this data would not strictly have to be stored in the dict, since we do nothing with it later on.

The last line calls the write_items_to_string method of output.
At first, output is empty.
We write to it, and it retains its contents across those loops, building up a virtual text file.

String concatenation in loops is usually a bad idea.
Strings are immutable, that means even just appending a single letter will lead to the creation of an entirely new object.
Lists are mutable.
So a good alternative approach to what was done here would be to collect string components in a mutable sequence, like a list, and then joining them together once afterwards, using join.

match_summary = output.getvalue()

just gets the text contents of the output object.
match_summary is now a string, printed to the specifications in write_items_to_string.
If you want different newlines, indentations etc., look there.

Note that if you did not want each item on a newline, and would be happy with printing the "raw" output, that is suddenly much, much easier:

emails = ["hello@world.org", "a@b.c"]
result = f"Emails found: {emails}"
print(result)

is all that is needed:

Emails found: ['hello@world.org', 'a@b.c']

pyperclip.copy(match_summary)
print("Copied to clipboard:", match_summary, sep="\n")

Note that I saved a print call here by just separating the arguments with a comma.
They are separated according to the sep argument.

if __name__ == "__main__":
    main()

This is a common pattern used to prevent the file from running when being imported.
If a Python file, i.e. module, is run directly as a script, as is the case here, its __name__ attribute will be set to "__main__".
As such, the main() function is executed, as desired.

However, notice how in the docstring explanation above, I imported your module to call help on it.
In such a case, without the __name__ == "__main__" safeguard, the module would also run, which would then of course be undesired.
As such, it is a good idea to keep modules importable.


A Very Poetic Program This program is basically a rhyming dictionary. It makes use of Sphinx's groups phonemic dictionary of English which includes a lot of words broken into their respective phonemes, and if we assume that, at least for most cases, anytime the two last phonemes match, the two words rhyme, then it's very easy to whip up a program that does so. Here's my try at this:

import requests
import os
import re

def load_library():
    with open('library.txt', 'r') as library:
        lines = library.readlines()

    return [x for x in lines if not x.startswith(';;;')]

def rhyme(string):

    if os.path.isfile('library.txt'):
        library = load_library()
    else:
        url = "http://svn.code.sf.net/p/cmusphinx/code/trunk/cmudict/cmudict-0.7b"
        response = requests.get(url, stream=True)

        if not response.ok:
            print("Error loading library")

        with open('library.txt', 'wb+') as library:
            for block in response.iter_content(32):
                library.write(block)

    library = load_library()
    string = string.upper()

    phonemes = [y for y in library if y.startswith(string)][0][len(string):]
    phonemes_split = str.strip(phonemes).split(' ')

    matches = [z for z in library if z.endswith(phonemes_split[-2] + ' ' + phonemes_split[-1] + '\n')]
    matches = [z.strip().split(' ') for z in matches]

    return matches

Of course it's naive to assume that EVERYTIME the last two phonemes of two different words match, they rhyme, but most of the times, they do.

This program returns a list of matching words broken into their respective phonemes.

I spent two years studying English literature before I dropped out in favor of pursuing computer programming so I'm qualified to say that I barely know what a phoneme is.

Library loaded twice

If library.txt exists, it will get loaded twice:

if os.path.isfile('library.txt'):
    library = load_library()
else:
    # download the file ...

library = load_library()

I guess this was just an oversight, with a simple fix:

if not os.path.isfile('library.txt'):
    # download the file ...

library = load_library()

Filter while loading

Instead of loading the library first and then filtering out the commented lines, it would be better to filter as you read:

with open('library.txt', 'r') as library:
    return [line for line in library if not line.startswith(';;;')]

Use generator expressions for better performance

It's a pity to loop over the entire list and filtering when you will only use the first match:

phonemes = [y for y in library if y.startswith(string)][0][len(string):]

You would do better with a generator expression,
which is more or less as simple as replacing the surrounding [...] with (...).
However, generators are not subscriptable, so to get the first element you must use the next(...) builtin:

phonemes = next(y for y in library if y.startswith(string))[len(string):]


Determining whether a string can be rearranged into a palindrome For a string, count the number of occurences of each character. If at most one character has odd count then the string can be a palindrome else it's not.

public class Palindrome {
        //count the chars in the input string
            //if not more than one character has odd count then palindrome is possible
            public static boolean isPalindromPossible(String input)
            {
                int[] charCount = new int[128];
                for(int i = 0 ; i < input.length() ; i++)
                {
                    charCount[(int)input.charAt(i)]++;
                }
                int oddCount = 0 ;
                for(int i  = 0 ; i < 128 ; i++)
                {
                    if(charCount[i] % 2!=0)
                    {
                        oddCount++;
                    }
                }

                if(oddCount != 0 && oddCount != 1)
                {
                    return false;
                }
                return true;
            }

            public static void main(String[] args) {
                String input = "aacaacc";
                System.out.println(isPalindromPossible(input));
            }
    }

int

All the counters are inherently non-negative. I recommend to make them unsigned, just to clarify your intentions.
What to return

Computing return value

    if (condition) {
        return false;
    }
    return true;

is a long way to say

    return !condition;

Actual condition

    oddCount != 0 && oddCount != 1

is a long way to say

        oddCount > 1

So I recommend to simply

    return oddCount < 2;

Efficiency

I wouldn't bother about performance at all. However, you may shave few cycles:

    ....
    charCount[(int)input.charAt(i)] ^= 1;
    ....
    oddCount += charCount[i];

thus avoiding taking modulo and making decisions inside the loop. I don't think it will make any difference however.
Assumptions and restrictions

Traditionally A man, a plan, a canal, Panama! is considered a palindrome. You really need to say what is a palindrome in your setting.


Password checker in PHP I have written a password checker using PHP, consisting of many if else statements. Is there any possible way to shorten this code?

function passtest($pass) {
            if (!empty($pass)) { //check if string is empty
                if (ctype_alnum($pass)) { //check if string is alphanumeric
                    if (7 < strlen($pass)){ //check if string meets 8 or more characters
                        if (strcspn($pass, '0123456789') != strlen($pass)){ //check if string has numbers
                            if (strcspn($pass, 'abcdefghijklmnopqrstuvwxyz') != strlen($pass)) { //check if string has small letters
                                if (strcspn($pass, 'ABCDEFGHIJKLMNOPQRSTUVWXYZ') != strlen($pass)) { //check if string has capital letters
                                    return "
Password passed";
                                }
                                else {
                                    return "
No capital letter";
                                }
                            }
                            else {
                                return "
No small letter";
                            }
                        }
                        else {
                            return "
No number";
                        }
                    }
                    else {
                        return "
Password is short";
                    }
                }
                else {
                    return "
Password has special character";
                }
            }
            else {
                return "
Password field is empty";
            }
        }

I have an answer which also changes the logic of your code but I think there is a good reason why you should consider it:

function passtest($pass) {
    $errors = array();
    if (empty($pass)) $errors[] = 'Password field is empty';
    if (ctype_alnum($pass)) $errors[] = 'Password has special character';
    [...]

    return '
' . implode('
', $errors);
}

Since it appears like this is somehow shown to the user I would notifyabout all  the errors that happened in choosing the password, so that they can all be corrected in a single try.

Another sidenote: Since your code reminds me of the time where I was just getting started and this seems to be at least somewhat related:
Don't save passwords, save their hashes. And use a good hashing implementation. If you're using PHP >= 5.5, there is a really easy way.

And thanks to "Fge", I can also give you a slightly less easy way for earlier PHP versions (>= 5.3.7).


yandex-maps service for save places I made script which create marker on the map after mouse left button click. And after mouse right button click marker removed.

All markers coordinates storage in browser localStorage.

I use yandex-maps technoogy.

JSFIDDLE

code:

ymaps.ready(init);

var myMap,
    collectionMarkers,
    myPlacemark;

function init(){
    myMap = new ymaps.Map("map", {
        center: [55.76, 37.64],
        zoom: 7
    });

    collectionMarkers = new ymaps.GeoObjectCollection({}, {
        preset: "islands#redIcon"
    });

    myMap.geoObjects.add(collectionMarkers);

    renderSavedPoints();

    myMap.events.add('click', function (e) {
        var coords = e.get('coords');
        //console.log('lat:', coords[0], 'lng:', coords[1]);

        var placemark = new ymaps.Placemark([coords[0], coords[1]]);
        collectionMarkers.add(placemark);

        addToStoragePoint({
            lat: coords[0],
            lng: coords[1]
        });
    });

    collectionMarkers.events.add('contextmenu', function(e) {
        var coords = e.get('target').geometry.getCoordinates();

        var pointLat = coords[0],
            pointLng = coords[1];

        //console.log('right click', pointLat, pointLng);

        var points = getPoints(),
            newPoints = [];

        newPoints = points.filter((point) => point.lat != pointLat && point.lng != pointLng);

        localStorage.points = JSON.stringify(newPoints);
        renderSavedPoints();
    });
};

function getPoints() {
    return localStorage.points ? JSON.parse(localStorage.points) : [];
};

function addToStoragePoint(pointObj) {
    console.log('addToStoragePoint start');
    var points = getPoints();
    points.push(pointObj);
    localStorage.points = JSON.stringify(points);
};

function renderSavedPoints() {
    var points = getPoints();

    collectionMarkers.removeAll();

    points.forEach((point) => {
        var placemark = new ymaps.Placemark([point.lat, point.lng]);
        collectionMarkers.add(placemark);
    });
};

You should be using function such as:

localStorage.setItem('points', JSON.stringify(points));
localStorage.getItem('points');

If you can use es6:

// instead of
var coords = e.get('target').geometry.getCoordinates();

var pointLat = coords[0],
    pointLng = coords[1];

// Could write
const [pointLat, pointLng] = e.get('target').geometry.getCoordinates();

// Why save points as variable if it's only used once
var points = getPoints(),
    newPoints = [];

newPoints = points.filter((point) => point.lat != pointLat && point.lng != pointLng);
// Could just do
var newPoints = getPoints().filter(point => point.lat != pointLat && point.lng != pointLng);


Set state in life cycle events from props This is a ReactJS component that accepts a date and displays how long ago this date was (5 seconds ago, 3 minutes ago, 5 hours ago, ...) + it uses a setTimeout to automatically update itself when the state.displayDate changes.

What's going on:

On ComponentWillMount: set state.date from props
Update the state.displayDate with a setTimeout

Is this a good pattern or what are the alternatives?

var MomentAgo = React.createClass({
  propTypes: {
    date: React.PropTypes.string.isRequired
  },
  getInitialState: function() {
    return {
      date: null,
      displayDate: null
    };
  },
  componentWillMount: function (){
    this.setState({ date: this.props.date });
  },
  componentDidMount: function() {
    //with a setTimeout:
    this.setState({ displayDate: date.format('DD MMMM YYYY') });
  },
  render: function() {
    return(<span>{this.state.displayDate}</span>);
  }

According to Facebook, setting initial state from props is an anti-pattern.
What happens here is that state is set from props in ComponentWillMount. It would seem that this is thus also the same anti-pattern. Or is this the correct way to do it?

In order to keep to the React pattern and stick to immutability of props simply separate your code to have a parent component which carries out the logic and passes the the date to the child component which can then be displayed in the child's render component.

It would then look something like this:

var React = require('React');

var MomentDisplay = React.createClass({
    propTypes: {
        date: React.PropTypes.string.isRequired
    },
    render: function() {
        return (
            <span>
                {this.props.displayDate}
            </span>
        )
    }
});

var MomentAgo = React.createClass({
    getInitialState: function () {
        return {
            date: null,
            displayDate: null
        };
    },
    componentWillMount: function () {
        this.setState({date: this.props.date});
    },
    componentDidMount: function () {
        setTimeout(function(){
            this.setState({displayDate: date.format('DD MMMM YYYY')});
        }, 3000);
    },
    render: function () {
        return (
            <MomentDisplay displayDate={this.state.displayDate} />
            )
        }
});


CodeIgniter maintain data through Controllers I have a main menu in my application.  It loads depending on the role of the user that I validate from a table in my data base, but I have to recreate this menu every time I call a new controller.

User Controller:

class User extends Ci_Controller {

    function __construct(){

        parent::__construct();

        if (!$this->session->userdata('session_user')) {
            $this->session->set_flashdata("mensaje", 'No cuenta con permisos *');
            redirect();
            $this->session_usuario = 1;
        }

        $session_data = $this->session->userdata('session_user');
        $image        = $session_data['img'];
        $nickname     = $session_data['nick'];
        $res          = "";

        $this->layout->setLayout('layout/admin');
        $this->layout->setImage(base_url().$image);
        $this->layout->setNick($nickname);

        $data['module'] = $this->usuarios_model->seleccionarModulos($session_data['perfil'] );

        for ($i=0; $i < count($data['module']) ; $i++) {
            $info['functionality'] = $this->usuarios_model->seleccionarFuncionalidades(     $session_data['usuario'], $data['module'][$i]['id']);
            $data['module'][$i]['route'] = $this->load->view($data['module'][$i]['route'], $info, true);
        }

        foreach ($data['module'] as $key => $value) {
            $res .= $value['route'];
        }

        $this->layout->setMenu($res);

    }

And exactly the same in the Support Controller:

class Support extends Ci_Controller {

    private $user;
    private $permiso;
    private $order;

    function __construct(){

        parent::__construct();

        if (!$this->session->userdata('session_user')) {
            $this->session->set_flashdata("mensaje", 'No cuenta con permisos *');
            redirect();
            $this->session_usuario = 1;
        }

        $session_data = $this->session->userdata('session_user');
        $image        = $session_data['img'];
        $nickname     = $session_data['nick'];
        $res          = "";

        $this->layout->setLayout('layout/admin');
        $this->layout->setImage(base_url().$image);
        $this->layout->setNick($nickname);

        $data['module'] = $this->usuarios_model->seleccionarModulos($session_data['perfil'] );

        for ($i=0; $i < count($data['module']) ; $i++) {
            $info['functionality'] = $this->usuarios_model->seleccionarFuncionalidades(     $session_data['usuario'], $data['module'][$i]['id']);
            $data['module'][$i]['route'] = $this->load->view($data['module'][$i]['route'], $info, true);
        }

        foreach ($data['module'] as $key => $value) {
            $res .= $value['route'];
        }

        $this->layout->setMenu($res);

    }

I know that it's a wrong way to solve this problem, and I want to improve it and reuse the code.

You need to extend CI_Controller with your code and extend your classses from your custom class, not default. I'll try to explain:

At first create MY_Controller.php at /application/core/ with next content:

     <?php

     class MY_Controller extends CI_Controller {
              function __construct() {
                      parent::__construct();
                          // Your checks here
              }
     }

     ?>

Now edit your controllers like this:

was

class Support extends Ci_Controller {

will be

class Support extends MY_Controller {

Now your controller will use constructor from your MY_Controller and still use CodeIgniter's default controller class.

You can read more from the manual.


Music player in Swift I made a simple mp3 player that plays, pauses and stops a given song, shows the time elapsed, and has a volume control.  I read the documentation and can't figure out the difference between the pause and stop methods, although I have configured them to work as I want (i.e., pause lets you resume from where you left off while stop resets the track).

import UIKit
import AVFoundation

class ViewController: UIViewController {

    var player:AVAudioPlayer = AVAudioPlayer()
    var musicPlaying = false
    var timer:NSTimer!

    @IBOutlet weak var currentTime: UILabel!
    @IBOutlet weak var sliderValue: UISlider!

    override func viewDidLoad() {
        super.viewDidLoad()
        let audioPath = NSBundle.mainBundle().pathForResource("bach", ofType: "mp3")!
        do {
            try player = AVAudioPlayer(contentsOfURL: NSURL(string: audioPath)!)
        } catch let error as NSError {
            print(error)
        }
    }

    @IBAction func play(sender: AnyObject) {
        if musicPlaying {
            player.pause()
            musicPlaying = false
        } else {
            player.play()
            musicPlaying = true
            timer = NSTimer.scheduledTimerWithTimeInterval(1.0, target: self, selector: "updateTime", userInfo: nil, repeats: true)
        }
    }

    @IBAction func stop(sender: AnyObject) {
        player.stop()
        musicPlaying = false
        player.currentTime = 0
    }

    func updateTime() {
        let timePlayed = player.currentTime
        let minutes = Int(timePlayed / 60)
        let seconds = Int(timePlayed % 60)
        currentTime.text = NSString(format:"%02d:%02d", minutes, seconds) as String
    }

    @IBAction func sliderChanged(sender: AnyObject) {
        player.volume = sliderValue.value
    }

    override func didReceiveMemoryWarning() {
        super.didReceiveMemoryWarning()
    }

}

Let's talk very generally about AVAudioPlayer.

There are a few relevant methods to discuss here:

play
pause
stop
prepareToPlay

Strictly speaking, prepareToPlay is optional, but if we call play without the player being prepared to play, there will potentially be some latency between calling play and audio actually beginning to play.  If the player wasn't already prepared, the first thing that play will do is prepare it.

From the documentation on prepareToPlay:

  Calling this method preloads buffers and acquires the audio hardware needed for playback, which minimizes the lag between calling the play method and the start of sound output.

And perhaps, the important part to read here is that calling stop will undo the set up.  This is repeated in the documentation on stop:

  Calling this method, or allowing a sound to finish playing, undoes the setup performed upon calling the play or prepareToPlay methods.

So, in your specific case, where we have a view controller that does nothing but play "bach.mp3", we should consider never actually calling stop except perhaps in viewWillDisappear.  Calling stop will simply release resources that have to be re-obtained before the track can be played again.  And if we continue reading the documentation for stop, we can see that the behavior of not resetting the currentTime to zero is well documented and to be expected:

  The stop method does not reset the value of the currentTime property to 0. In other words, if you call stop during playback and then call play, playback resumes at the point where it left off.

So basically, if you're not actually done with that player, stop does the same as pause, but just requires a lot more work.

Realistically, if we want to present to the user behavior which stops the current audio from playing, and will resume the audio from the beginning of the track when play is pressed again (and we have no expectation of starting another track without some other action happening to trigger that), then we should be using pause and then resetting the currentTime to 0.

The only difference between our stop and pause buttons in this specific case should be that stop resets the current time to zero, but both buttons simply pause the player.


Displaying elements of a list, in columns I have a list of strings. I need to list them in rows and columns. Each row should not have more than "cols" number of values. Each of the values in a given row should be "step" away from the previous value. The values should appear only once in the output. Here is what I have. Any better way to write this code?

cols = 4
step = 10
vlist = ["Value" + str(i+1) for i in range(100)]

vlen = len(vlist)
start = 0
while start < vlen and start < step:
    num = 0
    for idx in range(start, vlen, step):
        if num < cols:
            print(vlist[idx], end=", ")
            num += 1
    print("\n")
    start += 1

Fancy iteration in Python is often made easier using the itertools module.  For this case, itertools.islice() can help pick out the values for each row.

from itertools import islice

cols = 4
step = 10
vlist = ["Value" + str(i+1) for i in range(100)]

for row in range(step):
    print(', '.join(islice(vlist, row, cols * step, step)), end=", \n\n")


Removal of subtrees in tree on condition optimally I have a small program that removes subtrees of a tree based on following condition: If a subtree consists only of nodes with "rank" 1, remove it.  I am using recursive DFS approach.  Trees or subtrees does not have any further special properties. I want to ask about your opinions.  Is it possible to make this more elegant/more optimal?

public class Main {

    public static void main(String[] args) {
        List<Node> tree1 = Arrays.asList(
            new Node(1, null, 1),
            new Node(2, 1, 1),
            new Node(3, 1, 1),
            new Node(4, 2, 0),
            new Node(5, 3, 1),
            new Node(6, 5, 0),
            new Node(7, 5, 1),
            new Node(8, 7, 0),
            new Node(9, 5, 1),
            new Node(10, 3, 1)
        );

        List<Node> newTree = new ArrayList<>();
        traverse(tree1.get(0), tree1, newTree);
        if(tree1.get(0).rank != 1 || tree1.get(0).getNumberOfValidSubtrees() != 0) {
            newTree.add(tree1.get(0));
        }

        System.out.println(tree1);
        newTree.sort(Comparator.comparing(Node::getId));
        System.out.println(newTree);
    }

    private static void traverse(Node parent, List<Node> allNodes, List<Node> newTree){
        List<Node> children = allNodes.stream().filter(e -> e.parentId != null && e.parentId == parent.id).collect(Collectors.toList());

        for(Node child : children) {
            traverse(child, allNodes, newTree);

            if(child.rank != 1 || child.getNumberOfValidSubtrees() != 0) {
                parent.incrementValidSubtree();
                newTree.add(child);
            }
        }
    }
}

Node class

class Node{
    int id;
    Integer parentId;

    int rank;
    private int numberOfValidSubtrees;

    Node(int id, Integer parentId, int rank){
        this.id = id;
        this.parentId = parentId;
        this.rank = rank;
        this.numberOfValidSubtrees = 0;
    }

    public int getNumberOfValidSubtrees(){
        return this.numberOfValidSubtrees;
    }

    public void incrementValidSubtree() {
        this.numberOfValidSubtrees++;
    }

    public int getId() {
        return id;
    }

    public String toString() {
        return String.valueOf(id);
    }
}

Data Structure

You don't have a Tree of nodes; you have a List of nodes which describes a tree-like structure in a serialized format.  This is fine for storing the tree of nodes in a flat database table, but it is not a Tree.

Traversing a Tree can be done in \$O(N)\$ time.  Your Depth-First-Search traversal of the List of nodes requires searching the list of nodes repeatedly, once for every node in the list.  This makes the algorithm \$O(N^2)\$.

To properly make your tree, you'd need each node to have its own (possibly empty) list of child nodes:

class Node {
    Node parent = null;
    List<Node> children = new ArrayList<>();
}

A depth first search of this tree could then be performed in \$O(N)\$ time.

Code Review

tree1 and newTree don't reflect a consistent naming strategy.  tree1 and tree2, or oldTree and newTree would be better.

You use tree1.get(0) four times, to get the root node of the tree.  Why not store the root node in a variable?

Node root = tree1.get(0);

Is tree1.get(0) the root node?  Always?  It looks like you make no assumptions of the order of the nodes in the List<Node> structure.  You should probably search for the node with the null parent identifier:

Node root = tree1.stream()
                 .filter(node -> node.parentId == null)
                 .findFirst()
                 .get();

Will the tree always have a root node?  If your processing removes all node, and then you call .get(0) on the resulting list, you'll get an IndexOutOfBoundsException.

You repeat this code:

    if(tree1.get(0).rank != 1 || tree1.get(0).getNumberOfValidSubtrees() != 0) {
        newTree.add(tree1.get(0));

here:

        if(child.rank != 1 || child.getNumberOfValidSubtrees() != 0) {
            // ...
            newTree.add(child);

If your traverse() method actually operated on parent instead of the parent's children, you wouldn't need to duplicate code.

Also, if the traverse() method operated on the parent, it could maintain a count of the subtrees, and you wouldn't need to pollute the Node class with numberOfValidSubtrees.

Moving on to class Node:

Why is the numberOfValidateSubtrees member private, but none of the other members (id, parentId, rank) are?

Should id and parentId be final?  How about rank?

Inconsistent this. usage: You do not need it in getNumberOfValidSubtrees() and in incrementValidSubtree() methods.  OR, you should also use it in getId() and toString().

You can remove the this.numberOfValidSubtrees = 0; initialization by providing the initial value in the declaration:

private int numberOfValidSubtrees = 0;

Suggestions

Will it always be rank == 1 nodes with no subtrees that are deleted?  A more elegant solution would be to allow a Predicate<Node> argument which would return true if a node was to be deleted.

static boolean isRank1WithNoSubtree(Node node) {
    return node.rank == 1  &&  node.children.isEmpty();
}

static void removeDFS(Predicate<Node> predicate) {
    // ...
}

public static void main(String[] args) {
    // ...
    removeDFS(Main::isRank1WithNoSubtree);
    // ...
}

But there are multiple ways of traversing a tree.  Perhaps you would be better with defining methods which traverse the tree, each which take a Consumer<Iterator<Node>> action argument, to call on each tree node as it is visited.

static void removeIfRank1(Iterator<Node> iter) {
    Node node = iter.next();
    if (node.rank == 1  &&  node.children.isEmpty())
        iter.remove();
}

static void traverseDFS(Consumer<Iterator<Node>> action) {
    // ...
}

public static void main(String[] args) {
    // ...
    traverseDFS(Main::removeIfRank1);
    // ...
}

Node -vs- Tree

Personally, I like to draw a distinction between a Node and a Tree.  A tree may be a collection of nodes, each node optionally containing more child nodes.  But the root node is NOT the tree.  The Tree itself can contain additional information about itself, such as the number of nodes it contains.  The Tree class would have methods for adding nodes, removing nodes, traversal, filtering, etc., instead of having to tack those methods on to either the Node class or the Main class.  Eg):

class Tree<E> {

    class Node<E> {
        Node<E> parent = null;
        List<E> children = new ArrayList<E>();
        E data;

        Node(E e) {
            data = e;
        }
    }

    Node<E> root = null;
    int numNodes = 0;

    void traverseDFS(Consumer<Iterator<Node>> action) {
        // ...
    }
}


Entity Framework Lazy loading performance comparison The following code takes about 20 seconds to run with about 200,000 records in the TaskLogs table:

using (var db = new DAL.JobManagerEntities())
    {
        return db.TaskLogs.Select(taskLog => new TaskLog()
        {
            TaskLogID = taskLog.TaskLogID,
            TaskID = taskLog.TaskID,
            TaskDescription = taskLog.Task.TaskDescription,
            TaskType = (TaskType)taskLog.Task.TaskTypeID,
            RunID = taskLog.RunID,
            ProcessID = taskLog.ProcessID,
            JobID = taskLog.JobID,
            JobName = taskLog.Job.JobName,
            Result = taskLog.Result,
            StartTime = taskLog.StartTime,
            TimeTaken = taskLog.TimeTaken
        }).OrderByDescending(t => t.RunID).ThenByDescending(t => t.RunID).ThenByDescending(t => t.StartTime).ToList();
    }

I tweaked it until I got something that runs faster. Here's where I got to:

            using (var db = new DAL.JobManagerEntities())
    {
        db.Configuration.LazyLoadingEnabled = false;
        var tasks = db.Tasks.ToList();
        var jobs = db.Jobs.ToList();
        var result = db.TaskLogs.Select(x => new TaskLog()
        {
            TaskLogID = x.TaskLogID,
            TaskID = x.TaskID,
            RunID = x.RunID,
            ProcessID = x.ProcessID,
            JobID = x.JobID,
            Result = x.Result,
            StartTime = x.StartTime,
            TimeTaken = x.TimeTaken
        }).OrderByDescending(t => t.RunID).ThenByDescending(t => t.StartTime).ToList();

        foreach (var r in result)
        {
            r.TaskDescription = tasks.Single(t => t.TaskID == r.TaskID).TaskDescription;
            r.TaskType = (TaskType)tasks.Single(t => t.TaskID == r.TaskID).TaskTypeID;
            r.JobName = jobs.Single(j => j.JobID == r.JobID).JobName;
        }
        return result;}

Which runs in less than 6 seconds for the same number of records.

The TaskLog table is linked to the Job and Task tables as follows:

The Job and Task tables will have 100s and 1000s of records respectively.

Is there anything else I could do in order to further improve the efficiency of the code?

1) Not sure if it impacts performance, but your first code fragment has one redunant order by:

.OrderByDescending(t => t.RunID).ThenByDescending(t => t.RunID)

2) You could improve performance of your seconds code with "client side indexing" (using a dictionary):

var tasksMap = tasks.ToDictionary(t => t.TaskID);
var jobsMap = jobs.ToDictionary(t => t.JobID);

foreach (var r in result)
{
    var task = tasksMap[r.TaskID];
    r.TaskDescription = task.TaskDescription;
    r.TaskType = (TaskType)task.TaskTypeID;
    r.JobName = jobsMap[r.JobID].JobName;
}


Basic DirectX window class As a C++ beginner learning C++ and DirectX at the same time, I was wondering if a more experienced C++ developer would be able to go over the code and tell me what is done incorrectly or something that you would change.

Window.h

#pragma once

#include <string>
#include <windows.h>

namespace Graphics {

class Window
{
private:
    const char *m_Title;
    int m_Width;
    int m_Height;
    HINSTANCE m_hInstance;
public:
    Window(const char *title, int width, int height);
    ~Window();

    bool Init();
    bool CreateD3DWindow();
    void Loop();

    inline int GetWidth() { return m_Width; }
    inline int GetHeight() { return m_Height; }
};
}

Window.cpp

#include "Window.h"

#include <iostream>

namespace Graphics {

// Forward declarations
LRESULT CALLBACK WndProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam);

Window::Window(const char *title, int width, int height)
    : m_Title(title), m_Width(width), m_Height(height)
{
}

Window::~Window()
{

}

bool Window::Init()
{
    if (!CreateD3DWindow())
    {
        return false;
    }

    return true;
}

void Window::Loop()
{
    MSG msg;
    ZeroMemory(&msg, sizeof(MSG));

    while (msg.message != WM_QUIT)
    {
        if (PeekMessage(&msg, NULL, 0, 0, PM_REMOVE))
        {
            TranslateMessage(&msg);
            DispatchMessage(&msg);
        }
        else
        {
            // Render
        }
    }
}

bool Window::CreateD3DWindow()
{
    HWND hWnd;
    WNDCLASSEX wc;

    m_hInstance = GetModuleHandle(NULL);

    // Window styling and options
    wc.style = CS_HREDRAW | CS_VREDRAW | CS_OWNDC;
    wc.lpfnWndProc = WndProc;
    wc.cbClsExtra = 0;
    wc.cbWndExtra = 0;
    wc.hInstance = m_hInstance;
    wc.hIcon = LoadIcon(NULL, IDI_WINLOGO);
    wc.hIconSm = wc.hIcon;
    wc.hCursor = LoadCursor(NULL, IDC_ARROW);
    wc.hbrBackground = (HBRUSH)GetStockObject(BLACK_BRUSH);
    wc.lpszMenuName = NULL;
    wc.lpszClassName = m_Title;
    wc.cbSize = sizeof(WNDCLASSEX);

    if (!RegisterClassEx(&wc))
    {
        std::cout << "RegisterClassEx failed!" << std::endl;
        return false;
    }

    hWnd = CreateWindowEx(
        WS_EX_APPWINDOW,
        m_Title,
        m_Title,
        WS_OVERLAPPEDWINDOW,
        GetSystemMetrics(SM_CXSCREEN) / 2 - m_Width / 2,
        GetSystemMetrics(SM_CYSCREEN) / 2 - m_Height / 2,
        m_Width,
        m_Height,
        NULL,
        NULL,
        m_hInstance,
        NULL
    );

    if (!hWnd)
    {
        std::cout << "Failed to create window" << std::endl;
        PostQuitMessage(0);

        return false;
    }

    ShowWindow(hWnd, SW_SHOW);

    return true;
}

LRESULT CALLBACK WndProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)
{
    switch (message)
    {
    case WM_CLOSE:
        PostQuitMessage(0);
        return 0;

    case WM_KEYDOWN:
    {
        switch (wParam)
        {
        case VK_ESCAPE:
            PostQuitMessage(0);
            return 0;
        }
    }

    default:
        return DefWindowProc(hWnd, message, wParam, lParam);
    };

    return 0;
}
}

I have one big swooping recommendation that I feel you should address before tackling anything else:

Mind your class invariants!

When designing a class, ideally you want to ensure every single public member function handles correctly any possible valid state of the class, in conjunction with any possible passed argument. In your case, ask yourself:

What happens if Loop() gets called before Init()? What happens if Init() gets called twice in a row? What happens if Create3DWindow() is called after Init()?

A good way to tackle that is to define what's known as a class invariant.

First, jot down on paper a description of the legal internal states of the class. That's known as the class invariant.

For example, you may want to have m_hInstance is either NULL or points to a window instance in your invariant definition, but m_hInstance points to a window instance would be even better, because it allows you to rely on that being true, so you don't have to constantly check.

Second, for each public member function, make sure that the function's behavior is well defined for every possible invariant state, and that it leaves the class in an invariant state after it's done.

Mind you, it doesn't mean that every public member function must "work", just that it doesn't explode, cause memory leaks, etc...

Edit: There are some extreme exceptions to this, such as std::vector<>'s operator[]. But this is an exception, and the class' documentation makes it extremely clear that you enter no-mans land if you call that with an index outside of the bounds.


Two-dimensional array, modified one field at a time Title says it all. I'm basically initializing a two-dimensional array and changing the fields one at a time.

It might be a good idea to do this OO.
A Grid class seems right, perhaps as a subclass of Array. So far I haven't been able to successfully port it. Pointers would probably be a good idea as well.

Note this is kind of a sandbox project at the moment. I'm a novice in JS and trying to wrap my head around how I can handle data efficiently (particularly memory efficient, CPU is not as relevant).

Partial reviews are also welcome.

Array.initialize = function(iRows, iColumns, cInit) {
    var array = [];
    for (var i = 0; i < iRows; ++i) {
        var columns = [];
        for (var j = 0; j < iColumns; ++j) {
            columns[j] = initial;
        }
       array[i] = columns;
    }
  return array;
}

Array.shoot = function(row, column, array) {
  array[row][column] = "H";
  return array;
}

var myGrid = Array.initialize(12, 12, "U");
myGrid = Array.shoot(5, 5, myGrid);
myGrid = Array.shoot(0, 4, myGrid);

console.log(myGrid);

A few things:

Someone's getting a head-start on Battleship, eh? :)
Don't modify native objects/prototypes. Array is the native JavaScript array constructor (a.k.a. "class"), and you're adding initialize and shoot methods to it. Don't.
Well, ok, you can extend native objects, but it's more hygienic to leave it alone. And in this case the methods you're adding are super-specific to your task, and not generic functionality, so that's a double "don't".
I'm pretty sure you have a bug: cInit isn't used for anything, but instead you have an previously undeclared (and therefor undefined) variable called initial. I'm guessing they were supposed to be one and the same.

Now, in terms of memory efficiency: Don't worry about it. JavaScript has automatic memory management and garbage collection, but it's buried deep in the runtime, so it's not something you can fiddle with. It also has dynamic typing, so it's kinda hard to even guess at memory consumption at times. For instance, the number 32 is 8 bytes long, since all numbers are 64-bit IEEE floating point values, yet 32 == "32" happens to be true due to dynamic typing/type-coercion. So the string "32" is for some purposes it's the same number, but it's probably only 2-3 bytes long. Maybe? I don't know. Because behind the scenes, all JS runtimes do a ton of voodoo, so in the end it's really hard to tell what's going on. Besides the runtime's own memory consumption is probably way beyond whatever data you're handling. The runtime might well trade memory for speed somewhere. And another runtime might do the opposite, but they'll both run your code the same.

Point is, memory just isn't a thing you really can worry about in JavaScript the same way you can worry about it in other languages. At least not on the same low level. Of course you can still run out of memory and things like that, but you'd have to try pretty hard. Especially with a simple little 2D array.

Aside: Don't bother with Hungarian notation either. Well, again, you can, but it's a lot of busywork when there's dynamic typing and type-coercion. You have parameters like iRows, which, yeah, should be an integer, but as mentioned all integers are just the Number type - which is really a float. And you could also pass in a numeric string and have it work. So it just gets confusing in a hurry.

Of course, you still want efficient data structures, but since the idea here is to construct a 2D array, well, you're probably best off using... a 2D array. So again, don't worry too much.

As for the code, you have the basics well in hand. As mentioned, you shouldn't be appending functions to the native Array constructor, but otherwise it's fine. Fixing that little thing you get:

function createArray(rows, columns, initialValue) {
  var array = [];
  for (var i = 0; i < rows; ++i) {
    var columns = [];
    for (var j = 0; j < columns; ++j) {
      columns[j] = initialValue;
    }
    array[i] = columns;
  }
  return array;
}

function shoot(row, column, array) {
  array[row][column] = "H";
  return array;
}

Still, I'd use push to append stuff to an array, rather than assigning things to an index.

But you said you might want a more OO-style implementation. However, sub-classing Array would be tricky. Especially as there are no "classes" in JavaScript, per se. It's easier to go with object composition; make your own constructor, and have your array be a property (aka member, aka instance variable) on the instances that constructor creates.

So let's make a Grid constructor and let's give it a shoot method:

function Grid(rows, columns) {
  var r, c, row;
  this.cells = []; // instance variable
  for(r = 0 ; r < rows ; r++) {
    row = [];
    for(c = 0 ; c < columns ; c++) {
      row.push("U");
    }
    this.cells.push(row);
  }
}

Grid.prototype.shoot = function (row, column) {
  this.cells[row][column] = "H";
};

Now, much like before, you can say:

var grid = new Grid(12, 12);
grid.shoot(5, 5);
grid.shoot(0, 4);

console.log(grid.cells); // notice you still have access to the "raw" array

Of course, you may want to do some bounds-checking in shoot to make sure the coordinates make sense, but otherwise... yeah, that's pretty much it.


Easly cancellable operations WPF Several times we need to be able to revert the data the user is inserting to a previous state (cancel data update). In order to keep DRY and for the sake of separation of concerns I thought about a way of just allowing that.

public class RevertableObject<T> : ViewModelBase //implements INotifyPropertyChanged
    where T:class, ICloneable
{
    public RevertableObject(T initialValue)
    {
        Value = initialValue;
        Value = initialValue.Clone() as T;
    }

    private T _oldValue;
    private T _currentValue;
    public T Value
    {
        get { return _currentValue; }
        private set
        {
            _oldValue = _currentValue;
            _currentValue = value;
            OnPropertyChanged();
        }
    }

    public void Revert()
    {
        Value = _oldValue;
    }
}

Some drawbacks of this solution are, but are not limited to:

Dependency on the interface ICloneable which does not have a way of knowing the cloning algorithm being used (member clone vs deep clone)
It only allows one object to be reverted once.

Any insights of better alternatives? Is this code acceptable?

You could empower IEditableObject. Here is a good article. Approach described will allow your model to do not mix concerns - they use memento design pattern.

UPDATE: I do not feel right by copy-pasting that article, so I would still recommend to have a look at it. Quick test though might look like the following:

1) Download and reference BindingOriented.Adapters project in your WPF application.

2) Create model:

public class Contact
{
    public string Name { get; set; }
}

3) Create view model (I used MvvmLight for RelayCommand):

public class MainViewModel
{
    public MainViewModel()
    {
        Contact = new EditableAdapter<Contact>(new Contact() { Name="Dmitry" });
        OKCommand = new RelayCommand(() => Contact.EndEdit());
        CancelCommand = new RelayCommand(() => Contact.CancelEdit());
    }

    public EditableAdapter<Contact> Contact { get; }
    public ICommand OKCommand { get; }
    public ICommand CancelCommand { get; }
}

4) Update MainWindow to include:

<StackPanel>
    <StackPanel.DataContext>
        <vm:MainViewModel/>
    </StackPanel.DataContext>
    <TextBox Text="{Binding Contact.Name}"/>
    <Button Content="OK" Command="{Binding OKCommand}"/>
    <Button Content="Cancel" Command="{Binding CancelCommand}"/>
</StackPanel>

It works :)


Merging the vector-group to get the common vector Given that I have a set of vectors(or called vector-group), like \$\{\mathbf U_1,\mathbf U_2,\cdots,\mathbf U_n\}\$. Below is a simple instance U1, U2, U3

U1 = {0.25, 0.25, 0.5, 0.5, 0.75, 0.75, 0.8};
U2 = {0.21, 0.25, 0.3, 0.6, 0.7, 0.8};
U3 = {0.25, 0.3, 0.7, 0.8};

Now I would like to calculate their common vector , i.e., a vector that contains all of the elements of U1, U2, U3 and has the least length. In addtion, containing all of the elements of U1 means containing
{0.25, 0.25, 0.5, 0.5, 0.75, 0.75, 0.8}, rather than {0.25, 0.5, 0.75, 0.8}.

My firend Richard Xu gives me the following algorithm

i   j   k   min_val
1   1   1   0.21    j++
1   2   1   0.25    i++ j++ k++
2   3   2   0.25    i++
3   3   2   0.3     j++ k++
3   4   3   0.5     i++
4   4   3   0.5     i++
5   4   3   0.6     j++
5   5   3   0.7     j++ k++
5   6   4   0.75    i++
6   6   4   0.75    i++
7   6   4   0.8     i++ j++ k++
//===>common vector
U = {0.21, 0.25, 0.25, 0.3, 0.5, 0.5, 0.6, 0.7, 0.75, 0.75, 0.8}

C function for two vectors

#define min_two(a, b) (a) > (b) ? (b) :(a)

//count denotes the length of common vector of U1, U2
int two_vec_common(double *U1, double *U2, int len1, int len2, double *temp) {
    int i, j;
    double min_val;
    int count = 0;
    i = j = 0;
    while (i < len1 || j < len2) {
        min_val = min_two(U1[i], U2[j]);
        temp[count++] = min_val;
        if(U1[i] == min_val) i++;
        if(U2[j] == min_val) j++;
    }
    return count;
}

C function for three vectors

double min_three(double x, double y, double z) {
    if (x <= y && x <= z) return x;
    if (y <= x && y <= z) return y;
    if (z <= x && z <= y) return z;
}
/*count denotes the length of common vector of U1, U2, U3
  len1, len2, len3 denote the length of vector U1, U2, U3, respectively
  the pointer variable temp stores the values of common vector*/
int three_vec_common(double *U1, double *U2, double *U3, int len1, int len2, int len3, double *temp) {
    int i, j, k;
    double min_val;
    int count = 0;
    i = j = k = 0;
    while (i < len1 || j < len2 || k < len3) {
        min_val = min_three(U1[i], U2[j], U3[k]);
        temp[count++] = min_val;
        if(U1[i] == min_val) i++;
        if(U2[j] == min_val) j++;
        if(U3[k] == min_val) k++;
    }
    return count;
}

Although the algorithm is easy to implement, I don't knot how to write a unified function to calculate the common vector of \$\{\mathbf U_1,\mathbf U_2,\cdots,\mathbf U_n\}\$

Here are some things that may help you improve your code.

Use const where practical

A number of places in the code should have the const keyword added.  For example instead of this:

int three_vec_common(double *U1, double *U2, double *U3,
                     int len1, int len2, int len3, double *temp) {

one might instead write this:

int three_vec_common(const double *U1, const double *U2, const double *U3,
                     int len1, int len2, int len3, double *temp) {

Doing so makes it clear that the inputs are not altered and that the temp pointer is an output.

Avoid buffer overflow

The code as written doesn't necessarily have a problem writing past the end of allocated memory, but it also doesn't have any way to avoid it.  That is, the temp pointer passed in is assumed to have enough space for any result, but the actual allocated length is not passed in with the pointer, making it impossible for the code to avoid a buffer overflow if the buffer does not happen to have enough memory.

Consider checking for bad pointers

It may be that surrounding code checks explicitly for bad (NULL) pointers or has some other means of preventing them from being passed in, but it still might not be a bad idea to check for NULL pointers before dereferncing memory with either reads or writes.

Consider a better way of passing variables

It's not to bad to pass two arrays and their lengths, and even three is not completely onerous, but consider how one might pass fifty arrays into a similar function.  It's obvious that one would not (and should not!) consider actually having one hundred named parameters, so the better way to do this would be to pass in a pointer to an array of such bufffers instead.  To make it simpler, I'd advocate adding a data structure to represent both the length and the array:

struct array {
    size_t size;
    double *data;
};

Try to use descriptive names

The variable names count and min_val are good because they give a good clue as to what they're for, but the name temp is not very descriptive or useful to the reader.

Think carefully about signed versus unsigned

What is the correct interpretation if, say, len1 is a negative number?  I'm guessing that there isn't a reasonable interpretation and that all of the the lengths are expected to be non-negative.  I'd recommend making them all of type size_t.

Make assumptions explicit

The algorithm will only work correctly if all input arrays are sorted in ascending order.  That's not necessarily a bad assumption, but it does need to be explicitly stated in either the program description or the code comments or both.

Generalize the algorithm

If we wanted to accept any number of arrays, we might use a function with this kind of template:

size_t merge(const struct array *arr, size_t arrlen, struct array *out)

If we consider the algorithm generally, it is:

set the current item to the head of each input array
find the minimum among current items
add that minimum value to the output array
advance each input array if the current value is equal to the minimum value
if all lists are not yet empty, go back to step 2

It doesn't matter if there are 2 or 3 or 1000 input arrays.  I'm hoping this is enough to suggest how one might write the remaining general code.


A FIR filter using Modern C++ features I'm diving into "modern CPP", with wide usage of templates and containers,
So I started with FIR filter.

In signal processing, a finite impulse response (FIR) filter is a filter whose impulse response (or response to any finite length input) is of finite duration, because it settles to zero in finite time. This is in contrast to infinite impulse response (IIR) filters, which may have internal feedback and may continue to respond indefinitely (usually decaying).

So I define FIR filter as a class and apply it to single scalar value or to the vector itself
Could you please review my code in terms of C++ coding?
#include <iostream>
#include <vector>
#include <list>
#include <algorithm>
#include <complex>
#include <cmath>
#include <iomanip>
#include <sstream>

// - Class method : operator(), constructor, various methods
// - Templates class and function
// - List and vector containers
// - Functors

template <typename T> std::string to_str(T & arr){
  std::ostringstream os;
  os << "[";  for (auto x: arr){    os<<" "<< x <<" ";  }  os << "]\n";
  return os.str();
}

template <class T> class FIR {
public:
    std::vector< T > coeff;  // Array of coefficents is static all the time
    std::list< T > taps;  // Array of is are shifting each tick
    int order;

    FIR(std::vector< T > v)
    {
       coeff = v;
       order = coeff.size();
       taps = std::list< T > (coeff.size(), 0);
    }
    void reset(){
       taps = std::list< T > (coeff.size(), 0);
    }
    void append( T  x){
      taps.push_front( x);
      taps.pop_back();
    }

    auto sum_product(){
      T  psum = 0;
      typename   std::list< T >::iterator t;
      typename std::vector< T >::iterator c;
      // Can we do better here?
      for (   c =coeff.begin(),     t  =taps.begin();
              c!=coeff.end(),       t !=taps.end();
            ++c             ,     ++t             ){
        psum += (*c) * (*t);
      }
      return psum;
    }

    auto operator() ( T  x){
      append(x);
       T  psum = sum_product();
      return psum;
    }

     auto operator() (std::vector< T > x){
     std::vector< T > y( x.size(), 0);
     for (int i = 0; i<x.size(); ++i){
        y[i] =  operator()(x[i]);
     }
     return y;
     }
};

int main()
{
    std::vector<double> coeff = {0.0,1.0,5.0, 1.0, 0.0};

    FIR<double> fir(coeff);
    std::cout <<"Coeff: " << to_str(fir.coeff);
    std::cout <<"Taps : "  << to_str(fir.taps);

    std::cout<<"\nTRANSFORM IN THE LOOP \n";
    std::vector<double> x_array = {0,1,2,3,4,5,6,0,0,0,0,0,0,13};
    for (auto x:x_array){
      auto p = fir(x);
      std::cout<<p<<"  ";
    }
    std::cout<<"\n";
    fir.reset();

    std::cout<<"\nTRANSFORM WITH FUNCTOR FOR SCALAR\n";
    std::vector<double> y_array( x_array.size(), 0);
    std::transform(x_array.begin(), x_array.end(), y_array.begin(), fir);
    std::cout << to_str(y_array);
    fir.reset();

    std::cout<<"\nAPPLY FUNCTOR FOR CONTTAINER \n";
    auto y_2 = fir(x_array);
    std::cout << to_str(y_2);
    fir.reset();

    return 0;
}

class FIR has nothing private. Should it be struct?

The line
  std::list< T > taps;  // Array of is are shifting each tick

is somewhat contradictory. It taps a list or an array?

I don't see how order is used. Does it need to be a class member at all?

operator()(T x) is very confusing. I would argue that it shall not be an operator at all; if you really want this to be an operator, consider operator<<. A metaphor of shifting into a filter is much more understandable than calling a filter.

// Can we do better here? Yes.


Speed up simple Python function that uses list comprehension I'm extracting 4 columns from an imported CSV file (~500MB) to be used for fitting a scikit-learn regression model.

It seems that this function used to do the extraction is extremely slow. I just learnt python today, any suggestions on how the function can be sped up?

Can multithreading/core be used? My system has 4 cores.

def splitData(jobs):
    salaries = [jobs[i]['salaryNormalized'] for i, v in enumerate(jobs)]
    descriptions = [jobs[i]['description'] + jobs[i]['normalizedLocation'] + jobs[i]['category'] for i, v in enumerate(jobs)]
    titles = [jobs[i]['title'] for i, v in enumerate(jobs)]

    return salaries, descriptions, titles

Full Code

def loadData(filePath):
    reader = csv.reader( open(filePath) )
    rows = []

    for i, row in enumerate(reader):
        categories = ["id", "title", "description", "rawLocation", "normalizedLocation",
                        "contractType", "contractTime", "company", "category",
                        "salaryRaw", "salaryNormalized","sourceName"]

        # Skip header row
        if i != 0:
            rows.append( dict(zip(categories, row)) )

    return rows

def splitData(jobs):
    salaries = []
    descriptions = []
    titles = []

    for i in xrange(len(jobs)):
        salaries.append( jobs[i]['salaryNormalized'] )
        descriptions.append( jobs[i]['description'] + jobs[i]['normalizedLocation'] + jobs[i]['category'] )
        titles.append( jobs[i]['title'] )

    return salaries, descriptions, titles

def fit(salaries, descriptions, titles):
    #Vectorize
    vect = TfidfVectorizer()
    vect2 = TfidfVectorizer()
    descriptions = vect.fit_transform(descriptions)
    titles = vect2.fit_transform(titles)

    #Fit
    X = hstack((descriptions, titles))
    y = [ np.log(float(salaries[i])) for i, v in enumerate(salaries) ]

    rr = Ridge(alpha=0.035)
    rr.fit(X, y)

    return vect, vect2, rr, X, y

jobs = loadData( paths['train_data_path'] )
salaries, descriptions, titles = splitData(jobs)
vect, vect2, rr, X_train, y_train = fit(salaries, descriptions, titles)

def loadData(filePath):
    reader = csv.reader( open(filePath) )
    rows = []

    for i, row in enumerate(reader):
        categories = ["id", "title", "description", "rawLocation", "normalizedLocation",
                        "contractType", "contractTime", "company", "category",
                        "salaryRaw", "salaryNormalized","sourceName"]

A list like this should really be a global variable to avoid the expense of recreating it constantly. But you'd do better not store your stuff in a dictionary. Instead do this:

   for (id, title, description, raw_location, normalized_location, contract_type,
       contractTime, company, category, salary_raw, salary_normalized, source_name) in reader:

       yield salary_normalized, ''.join((description, normalized_location, category)), title

All the stuff is then stored in python local variables (fairly efficient). Then yield produces the three elements you are actually wanting. Just use

   salaries, descriptions, titles = zip(*loadData(...))

to get your three lists again

        # Skip header row
        if i != 0:
            rows.append( dict(zip(categories, row)) )

Rather than this, call reader.next() before the loop to take out the header

    return rows


Count number of isosceles triangles in a set of points Given a set of points, what is the number of isosceles triangles that can be formed with the combinations of all points in the set?

It is certain that 3 collinear points never exists in the set.

#include <stdio.h>

typedef unsigned long long llu;

typedef struct {
    llu x,y;
}point_t;

inline llu distance_p(point_t &p1,point_t &p2)
{
    return (p2.x - p1.x)*(p2.x - p1.x) + (p2.y - p1.y)*(p2.y - p1.y);
}

int main(void){
    llu n;
    point_t ps[1000];
    while (scanf("%llu",&n), n) {
        for (int i = 0; i < n; ++i)
            scanf("%llu %llu",&ps[i].x,&ps[i].y);

        llu ans = 0;
        for (llu i = 0,a,b,c; i < n; ++i) {
            for (llu j = i + 1; j < n; ++j) {
                for (llu k = j + 1; k < n; ++k) {
                    a = distance_p(ps[i], ps[j]);
                    b = distance_p(ps[j], ps[k]);
                    c = distance_p(ps[k], ps[i]);

                    //at least two side equals
                    if(a == b || b == c || c == a){
                        ++ans;
                    }
                }
            }
        }
        printf("%llu\n",ans);
    }
}

But this is slow: \$O(n^3)\$. How can I improve my code?

Try another algorithm

Instead of considering triangles, you could simply consider lines starting from a point.

For each point, compute the distance between this point and every other point; you will get a number of distances.

Now, for every distance from a point, count the number \$n\$ of equal distances, and add \$n*(n-1)/2\$ to the number of isoceles triangles. If you use an std::unordered_map<unsigned long long, unsigned> to count the number of occurrences of every distance from a point, it should give you a \$O(n^2)\$ algorithm, if I'm not mistaken.

Note: this method might count the same triangle several times if a triangle happens to be equilateral. Unfortunately, I don't see a simple way to circumvent this problem.

C++ pedantry

I talked about the math first since it's what really matters, but honestly, your C++ isn't the prettiest C++ around, so here we go for the C++ review:

Please don't use typedef unsigned long long llu;. Even if it's long to write, llu doesn't convey any meaning to anyone reading it. Type the full name and people will know out-of-the-box what type they are using.
distance_p looks like it computes the distance between p1 and p2, but it actually computes the the squared distance between them. Make that clear by naming the function squared_distance.
Also, distance_p does not alter its parameter. Therefore, pass const references instead of simple ones:

inline unsigned long long squared_distance(const point_t &p1, const point_t &p2)
{
    return (p2.x - p1.x)*(p2.x - p1.x) + (p2.y - p1.y)*(p2.y - p1.y);
}

You don't need to use typedef like it is done in C to write point_t instead of struct point_t. In C++, an equivalent typedef is done automagically by the compiler. Simply declare your struct like this:

struct point_t {
    unsigned long long x, y;
};

When using components from the standard library, try to always prefix them with std::, even in they come from the C standard library (std::printf, std::scanf, etc...). Prefixing them may help to avoid name clashes and make it easier to search for standard library components in your code.
Try not to put everything in main. Here, you can easily put the algorithm which looks for isoceles triangles in its own function and let main handle only the input & output operations. Splitting code into small logical bricks instead of tightly coupled ones is known as separation of concerns.

Putting it all together

Since there are many things to take into account. I decided that I could as well show you how I would have implemented count_isoceles_triangles as described using C++11 (that's not exactly how I would have done it, but it's closer to it than what you currently have):

struct point_t
{
    unsigned long long x;
    unsigned long long y;
};

unsigned long long squared_distance(const point_t &p1, const point_t &p2)
{
    unsigned long long a = p2.x - p1.x;
    unsigned long long b = p2.y - p1.y;
    return a*a + b*b;
}

template<std::size_t N>
unsigned count_isoceles_triangles(const std::array<point_t, N> points)
{
    unsigned nb_triangles = 0;

    for (const point_t& p1: points)
    {
        std::unordered_map<unsigned long long, unsigned> distances;
        for (const point_t& p2: points)
        {
            unsigned long long dist = squared_distance(p1, p2);
            distances[dist] += 1;
        }

        for (auto&& dist: distances)
        {
            unsigned count = dist.second;
            nb_triangles += count * (count - 1) / 2;
        }
    }

    return nb_triangles;
}

Note that there are still some additional differences: I used std::array since it's not easy to pass a C array to a function correctly. Also, I didn't check whether p1 == p2 before counting the distances, but it's also something that should be done.


Selecting internationalization preferences I have a query below using CTE. I am wondering if there are some approach other than this. So I am collecting some advise to refactor it. Or does the query look good even though I am using this IN (SELECT InvariantLanguageCultureCode FROM y) statement many times in the query?

DECLARE  @ReportingInstanceID INT

 SET @ReportingInstanceID=34
 ;WITH x AS (
        SELECT  [dbo].[tenant_Reporting_Instance].UserID
        FROM    [dbo].[tenant_Reporting_Instance]
        WHERE   tenant_Reporting_Instance.ReportingInstanceID = @ReportingInstanceID
        )
        ,y AS ( SELECT  [TRCCore].[dbo].[pr_Language].InvariantLanguageCultureCode
        FROM    [TRCCore].[dbo].[pr_Language]
                JOIN [dbo].[tenant_User_Setting] ON tenant_User_Setting.LanguageID = pr_Language.LanguageID
        WHERE   tenant_User_Setting.UserID IN (SELECT UserID FROM x))
        ,Default_User_Settings_CTE ( RoundingTypeInvariantCode, AgingDateValue, CurrencyLabel, JobZoneLinkTypeLabel, MatchPeriodLabel, PayFrequencyLabel, PayTypeLabel )
                  AS ( SELECT   r.RoundingTypeInvariantCode ,
                                a.AgingDateValue ,
                                cur.CurrencyLabel ,
                                jz.JobZoneLinkTypeLabel ,
                                m.MatchPeriodLabel ,
                                pf.PayFrequencyLabel ,
                                pt.PayTypeLabel
                       FROM     dbo.tenant_User_Setting s
                                JOIN TRCCore.dbo.pr_Rounding_Type r ON r.RoundingTypeID = s.RoundingTypeID
                                JOIN dbo.tenant_Aging_Date a ON a.AgingDateID = s.AgingDateID
                                JOIN TRCCore.dbo.translation_pr_Currency cur ON cur.CurrencyID = s.CurrencyID
                                                              AND cur.LanguageCultureCode IN (SELECT InvariantLanguageCultureCode FROM y)
                                JOIN dbo.translation_tenant_Job_Zone_Link_Type jz ON jz.JobZoneLinkTypeID = s.JobZoneLinkTypeID
                                                              AND jz.LanguageCultureCode IN (SELECT InvariantLanguageCultureCode FROM y)
                                JOIN dbo.tenant_Match_Period m ON m.MatchPeriodID = s.MatchPeriodID
                                JOIN dbo.translation_tenant_Pay_Frequency pf ON pf.PayFrequencyID = s.PayFrequencyID
                                                              AND pf.LanguageCultureCode IN (SELECT InvariantLanguageCultureCode FROM y)
                                JOIN dbo.translation_app_Pay_Type pt ON pt.PayTypeID = s.PayTypeID
                                                              AND pt.LanguageCultureCode IN (SELECT InvariantLanguageCultureCode FROM y)
                       WHERE    s.UserID IN (SELECT UserID FROM x)
                     )
        SELECT * FROM Default_User_Settings_CTE

Move the First and second Cte to temp table.

you don't have to refer  ( SELECT InvariantLanguageCultureCode FROM y) more than once in join condition instead use it in one column then join that column with other tables.

Try this.

DECLARE @ReportingInstanceID INT

SET @ReportingInstanceID=34;

SELECT [TRCCore].[dbo].[pr_Language].InvariantLanguageCultureCode
INTO   #temp_InvariantLanguageCultureCode
FROM   [TRCCore].[dbo].[pr_Language]
       JOIN [dbo].[tenant_User_Setting]
         ON tenant_User_Setting.LanguageID = pr_Language.LanguageID
WHERE  tenant_User_Setting.UserID IN (SELECT [dbo].[tenant_Reporting_Instance].UserID
                                      FROM   [dbo].[tenant_Reporting_Instance]
                                      WHERE  tenant_Reporting_Instance.ReportingInstanceID = @ReportingInstanceID)

SELECT [dbo].[tenant_Reporting_Instance].UserID
INTO   #temp_UserID
FROM   [dbo].[tenant_Reporting_Instance]
WHERE  tenant_Reporting_Instance.ReportingInstanceID = @ReportingInstanceID

SELECT r.RoundingTypeInvariantCode,
       a.AgingDateValue,
       cur.CurrencyLabel,
       jz.JobZoneLinkTypeLabel,
       m.MatchPeriodLabel,
       pf.PayFrequencyLabel,
       pt.PayTypeLabel
FROM   dbo.tenant_User_Setting s
       JOIN TRCCore.dbo.pr_Rounding_Type r
         ON r.RoundingTypeID = s.RoundingTypeID
       JOIN dbo.tenant_Aging_Date a
         ON a.AgingDateID = s.AgingDateID
       JOIN TRCCore.dbo.translation_pr_Currency cur
         ON cur.CurrencyID = s.CurrencyID
            AND cur.LanguageCultureCode IN (SELECT InvariantLanguageCultureCode
                                            FROM   #temp_InvariantLanguageCultureCode)
       JOIN dbo.translation_tenant_Job_Zone_Link_Type jz
         ON jz.JobZoneLinkTypeID = s.JobZoneLinkTypeID


Simulated annealing for magic square-of-squares puzzle I'm working on a function that takes in a NumPy array containing only mutually distinct positive square numbers. I want the function to pick an element at random, find if there is a lower positive square number than the number selected: (1) if there is, return the array with the lower positive square number in place of the original element (2) if there isn't, return a copy of the original array.

The reason I'm developing this function is to create a simulated annealing solution to the Magic Square of Squares Puzzle, and this function would serve as one of the mutations to the candidate solution.

I've had a go at the implementation:

def last_square(x):
    xcopy = x.copy()
    index_1 = np.random.randint(0, x.shape[0]), np.random.randint(0, x.shape[1])
    c = x[index_1]-1
    while np.sqrt(c) != int(np.sqrt(c)) or c in set(x.flatten()):
        if c < 2:
            return xcopy
        else:
            c -= 1
    x[index_1] = c
    return x

Will my function always have the behaviour I've specified?

Edit 1

I've been asked some clarifying questions that I'll address here. There were very helpful in getting me to see what else I still had to explain.

Why are you iterating through all possible numbers less than the chosen one, instead of just iterating through the array itself?

I am looking for the next smallest positive square number below the one that was randomly selected from the array that is not in the array.

Why do you care about sqrt in last_square at all? Taking the square root of the elements will not change their ordering.

I agree that removing the square too that not change the ordering of the the square numbers currently within the array, nor does it change the position of a candidate square number c among all positive square numbers.

I care about checking if np.sqrt(c) != int(np.sqrt(c)) because a number whose square root is an integer is a square number.

Do you care whether the lower number is chosen non-deterministically? What should happen if there is more than one lower number? Currently it looks like you've implemented a lookup for the next lowest number closest to the chosen number, which is not what you described in the question.

The randomized selection of an element in the array is of course random, but taking that element being chosen, I'm aiming for the rest of the behaviour of the function to be deterministic. That is to say, given the array and the randomly selected element, then the function will always give the same return.

I want to preserve the mutual distinctness of the positive square numbers within the array, so my goal is to find the next lowest positive square number that is not in the array. I was trying to acheive this by inclusively disjoining c in set(x.flatten()) to the header of the loop, thereby keeping the loop going when c is still a number within the array.

As written, if you find a lower number and copy it to the chosen location, your claim that the array's elements are mutually distinct will be violated.

Using the following

if c < 2:
            return xcopy
        else:
            c -= 1

I am checking if c has gone too low, in which case I return a copy of the original array. If I break from the loop, the following lines will change the original array and return the array with the substitution completed.

x[index_1] = c
    return x

I think what I am having trouble with in part may be when calling np.sqrt on c without checking if c is non-negative. I'll work on correcting that and editing the code when I've resolved that issue.

Let me know if there are further questions. I'm happy to clarify.

This is much more clear thanks to your edit.

Why do you store this as a list of squares? Can you store it as a unique list of positive integers, and square them later? That would make this massively simpler.

Another thing that would make this massively simpler is if you're able to guarantee that the list is maintained in some kind of order. In the example code below I do not assume this, so have to sort it myself. Maintaining order should be easy because the last_square mutation will not change order.

  Will my function always have the behaviour I've specified?

Sounds like you need to write some unit tests :) You will have to mock your random function to make them predictable.

  I care about checking if np.sqrt(c) != int(np.sqrt(c)) because a number whose square root is an integer is a square number.

Fine, but... you said that there was already a guarantee that these numbers are positive squares.

I tried to put together a vectorized version of this that does not loop. Rule number one when writing efficient Numpy code is to attempt to avoid loops. This solution roots, flattens and sorts the data, then looks for the closest gap. I do not know if it will perform better than yours, nor whether this is the most efficient implementation of this approach.

The performance will be influenced by the length of your data and the sparsity of your numbers. For highly sparse numbers the looping approach will probably do better. For highly dense numbers the approach below will probably do better.

def last_square_new(x):
    assert (x > 0).all()
    x = x.copy()

    # The n-dimensional random index. Cannot yet be used as an index.
    nd_index = np.random.default_rng().integers(
        np.zeros(x.ndim), x.shape
    )
    # What will the index of the chosen element be once the array is flattened?
    flat_index = nd_index[0]*x.shape[-1] + nd_index[1]
    # Make this usable as an index.
    nd_index = nd_index[:, np.newaxis].tolist()

    xsq = (np.sqrt(x) + 0.5).astype(np.uint64)     # Sqrt with safe conversion to int
    chosen = xsq[nd_index][0]                      # The randomly chosen element's sqrt
    xsq = xsq.flatten()                            # Flatten the root array
    s_indices = xsq.argsort()                      # Indices that would sort the root array
    flat_index = s_indices[s_indices][flat_index]  # Move the flat index to its sorted position
    xsq = xsq[s_indices][:flat_index+1]            # Sort the root array and truncate

    d_indices = np.arange(xsq.size-1)[np.diff(xsq) > 1]  # Which indices have gaps?
    if d_indices.size != 0:                              # Are there any gaps?
        gap_index = d_indices[-1]                        # Index of the closest gap (low side)
        best = xsq[gap_index+1] - 1                      # Best missing root
        x[nd_index] = best**2                            # Assign it to the output

    return x