Generating random numbers from thread timing in C++ I have the following code blocks that generate apparently random numbers by using thread timing variations.

I am looking for a general code review, and a specific focus on whether the results are "true random numbers".

For computers with more than two CPUs

#include<chrono>
#include<iostream>
#include<thread>
using namespace std;

#define Mili 7
#define Base 7
#define Thrs 2

typedef unsigned char      Num;
typedef unsigned long long Out;

volatile Out out,tmp;

void inline thr(Num const num){
 while(true)
  out=out*Base+tmp,tmp=tmp*Base+num;
}

int main(){
 thread ths[Thrs];
 for(Num i=0;i<Thrs;++i)
  ths[i]=thread(thr,i);
 this_thread::sleep_for(chrono::milliseconds(Mili));
 while(true)
  cout.write((char*)&out,sizeof(out));
}

For computers with two CPUs

#include<iostream>
using namespace std;

#define Base 7
#define Iter 53

typedef unsigned char      Num;
typedef unsigned long long Out;

volatile Out out,tmp;

int main(){
 for(Num i=0;i<Iter;++i)
  #pragma omp parallel for schedule(runtime)
  for(Num num=0;num<Iter;++num)
   out=out*Base+tmp,tmp=tmp*Base+num;
 while(true){
  #pragma omp parallel for schedule(runtime)
  for(Num num=0;num<Iter;++num)
   out=out*Base+tmp,tmp=tmp*Base+num;
  cout.write((char*)&out,sizeof(out));
 }
}

For computers with one CPU:

#include<chrono>
#include<iostream>
#include<thread>
using namespace std;

#define Nano 1
#define Mili 7
#define Base 7
#define Thrs 2

typedef unsigned char      Num;
typedef unsigned long long Out;

volatile Out out,tmp;

void inline thr(Num const num){
 while(true)
  out=out*Base+tmp,tmp=tmp*Base+num;
}

int main(){
 thread ths[Thrs];
  for(Num i=0;i<Thrs;++i)
   ths[i]=thread(thr,i);
 this_thread::sleep_for(chrono::milliseconds(Mili));
 while(true)
  cout.write((char*)&out,sizeof(out)),
  this_thread::sleep_for(chrono::nanoseconds(Nano));
}

A true random number generator (TRNG) is a source of bits such that each bit has a probability of exactly 1/2 of being 1. Your code is obviously not a TRNG since its bias will heavily depend on the timing of operations.

A pseudo-random number generator (PRNG) is a deterministic algorithm that produces bits which satisfy statistical tests designed to verify randomness. Your code is obviously not deterministic so it is not a PRNG.

What you have here is a source of entropy: a bit stream which is not fully deterministic. You're measuring the timing of your program which depends on the synchronization between wall time and the CPU, so you're using jitter as a source of entropy.

CPU jitter may be a potential source of entropy. This paper by Stephan Müller analyzes CPU jitter on an x86 CPU. It has acceptable statistical properties if done right. However, there is a far cry from having acceptable statistical properties to being a good entropy source. The Linux kernel maintainers debated whether to include Müller's code as an entropy source, and concluded against it. They found two problems:

Having good statistical properties is good enough for some applications, but it is not good enough for security. For security, unpredictability is necessary. Even a knowledgeable adversary must not be able to reproduce the output. That includes, for example, an adversary who has the exact same hardware running the exact same program.
The CPU, in fine, does not contain much if any actual physical source of entropy. At the physical level, it's pretty much a deterministic system, albeit an extremely complex one which is difficult to analyze fully. It's unclear whether the physical uncertainties are enough to reach macroscopic levels.

To make things worse (much worse!) your implementation of CPU jitter measurement is deeply flawed. CPU jitter randomness is based on the lack of synchronization between the real-time clock (RTC) and the CPU's clock. But your code doesn't go very far in this direction: it spends most of its time doing sleep_for()! sleep_for() is based on interrupts driven by the RTC, and so are context switches. Confronting the RTC with itself is fully predictable, so the only source of unpredictability is the timing of the few computations that you make.

I haven't made any mathematical analysis of your computations (what you do with out). The only reason I can see to use multiplication here is to exercise different parts of the CPU to increase jitter. You should document this kind of design choices.

An obvious potential problem with your function is that it's prone to short cycles. The transformation (out, tmp) → (out*Base+tmp, tmp*Base+num) has cycles; if you fall into them you'll emit repetitions for a while until the scheduler hits. Just for fun, I ran your code and inspected the output visually, which is degree 0 of statistical testing (serious statistical testing uses tools that run for hours). The single-CPU version emitted sequences of zeroes () now and then… that's an obvious failure.

0000c9f0  5c 4d 68 41 e1 95 77 a6  17 2e bb 7a c6 37 d6 27
0000ca00  54 a4 7f f0 5f ee 5b fc  54 a4 7f f0 5f ee 5b fc
*
0000cbb0  a1 48 55 36 71 47 4b 75  4f a1 8d c1 60 fc c8 f0
0000cbc0  52 f5 22 43 e1 1a c0 4d  52 f5 22 43 e1 1a c0 4d
0000cbd0  52 f5 22 43 e1 1a c0 4d  74 47 ad 9f bd ee 5f 90
0000cbe0  74 47 ad 9f bd ee 5f 90  74 47 ad 9f bd ee 5f 90
*
0000cc40  74 47 ad 9f bd ee 5f 90  e3 2e c7 50 a8 84 5a 1c
0000cc50  fb ea 92 19 2f 6c 98 8e  0b 6a 7a 9a ac 59 87 03
0000cc60  73 ac 9f 43 2f 52 ab e1  2b d8 86 ab 7b 97 98 fc

(The first column is the offsets, then , all in hex. A * stands for a sequence of all-zero lines.)

On the programming side, you should initialize all variables. Leaving variables uninitialized doesn't increase entropy (they're predictable on a given platform), makes your program harder to analyze, and allows the compiler to optimize parts of your code away (for example, since you can't distinguish between a final value of out that was produced from a particular starting value, and a final value that was produced from a different starting value, the compiler is allowed to perform no computation until the first time the value of out is written to). Also note that operations like out=out*Base+tmp are not performed atomically, there can be a context switch between the time out is read and the time out is written to, in which case the work of the other thread(s) is be overwritten.

Note: sometimes “TRNG” is used to mean a source of entropy, which I discuss below. An actual RNG can be built on top of a source of entropy (preferably multiple independent sources) by seeding a PRNG with the source of entropy. By this very weak definition, you have a TRNG, but as we've seen one that's neither good nor new.

All in all, you seem to be trying to generate random numbers by flailing around in the dark. That doesn't work. Generating random numbers is very difficult. In order to be taken seriously, a random number generator needs to come with a detailed analysis, including rationales for each sources, collected statistical data, mathematical models, etc. Not a code dump. Showing that you did no research by demonstrating ignorance of common terminology doesn't help your credibility either.


OOP air conditioner model I had a phone interview and we shared a screen to do a simple challenge. Instructions were ambiguous but I basically had to implement how an air conditioner would work in general. I wrote the following as I talked it out with the interviewer.

So I made a class with a parameter that would be a room temp and then added two properties, heatingSystem and coolingSystem and set them as booleans. Then wrote a function called setTemperature that should’ve been a while loop that changes the temperature and setting of the A/C until it reaches the set temperature.

class AirConditioner {
   constructor(temperature) {
       this.temperature = temperature;
       this.coolingSystem = false;
       this.heatingSystem = false;
   }

  temperatureSet(temperature) {
    while (this.temperature != temperature) {
       if (this.temperature < temperature) {
           // turn on the cooling system and turn off heating system
           if (this.coolingSystem) {
               this.coolingSystem = false;
               this.heatingSystem = true;
               this.temperature++;
           } else {
             this.heatingSystem = true;
             console.log('Temperature has been lowered to ' + this.temperature);
             this.temperature++;
           }
       } else if (this.temperature > temperature) {
           // turn off the cooling system and turning the heating
           if (this.heatingSystem) {
               this.coolingSystem = true;
               this.heatingSystem = false;
               console.log('Temperature has been increased to ' + this.temperature);
               this.temperature--;
           } else {
             this.coolSystem = true;
             console.log('Temperature has been lowered to ' + this.temperature);
             this.temperature--;
           }
       } else {
           // turn off cooling/heating system but keep the fan un
           console.log('temp is the same')
       }
    }
      console.log(`this.temperature: ${this.temperature} temperature: ${temperature}`)
  }
}

const AC = new AirConditioner(78);
console.log(AC);
AC.temperatureSet(63);
console.log(AC);

I honestly don't like the code and can already see a lot of possibilities on how to refactor or add other functions for re-usability. I was wondering what your suggestions are.

I don't know the position you were interviewed for. Below is the embedded engineer point of view.

A room temp is not a class parameter. The AC cannot control it directly, so this.temperature++ and this.temperature-- are incorrect. The AC may only read the temp sensor.
The desired temperature OTOH is a class parameter. You should go an extra mile and provide the interface to set it. BTW, I'd expect temperatureSet to do just that: set the desired temperature.
As implemented, the program terminates as soon as a certain temperature is reached. I expect an AC system to run more or less forever.
In the if (this.temperature < temperature) clause you turn on heater no matter what. Move the this.heatingSystem = true; line outside of this.coolingSystem clause.

Assuming the above recommendations are implemented, the basic functionality should look like this:

    while(true) {
        ambientTemperature = this.temperatureSensor.read();
        if (ambientTemperature < this.targetTemperature) {
            if (this.coolingSystem) {
                this.coolingSystem = false;
            } else {
                console.log('Temperature has been lowered to ' + ambientTemperature);
            }
            this.heatingSystem = true;
        } else {
        ....

Of course there is a plenty of room to improve, but again that depends on interviewer expectations, and the AC schematics you'd agree upon.


Monitor, read and display data from a dynamic log file I am developing a plugin for intellij that basically monitors a log file and displays the data in a tool window within the IDE in real-time. I have managed to get it working but I would like to know the areas where I can improve on. I'm specifically interested in the structure of my code and the log file monitoring method followed.

The createToolWindowContent method is called when the log panel is opened for the first time.

public class LogToolWindowLoader implements ToolWindowFactory {

private UIOperations ui;
private LogFileFunctions log_func;
private ToolWindow toolWindow;
@Override
public void createToolWindowContent(Project project, ToolWindow toolWindow) {

    this.toolWindow = toolWindow;
    //this generates the log viewer GRID within the IDE
    ui = new UIOperations(toolWindow);
    ui.createTable();

    //Create new thread to retrieve data from log file. this is a continuous process, hence the new Thread
    new Thread(){
        public void run(){

            log_func = new LogFileFunctions();

            log_func.connectToLog();

            while(true){
                String[] result = log_func.getData();
                if (result!=null) {
                    //GUI updated on the Event Dispatcher Thread
                    SwingUtilities.invokeLater(new Runnable() {
                        @Override
                        public void run() {
                            ui.updateTable(result);
                        }
                    });
                }
                else {
                    try {
                        Thread.sleep(1000);
                    } catch (InterruptedException e) {
                        e.printStackTrace();
                    }
                }
            }
        }
    }.start();}

UI operations are handled in this class:

public class UIOperations {

private ToolWindow toolWindow;
private DefaultTableModel table_model;
private JBTable table;
private final String headers[]= {"header1","header2","header3","header4","header5","header6"};

public UIOperations(ToolWindow toolWindow){
    this.toolWindow = toolWindow;
}

public void createTable(){

    JComponent panel = new JPanel(new BorderLayout()); //new BorderLayout extends panel completely inside the tool Window
    panel.setBackground(JBColor.WHITE);
    toolWindow.getComponent().add(panel);

    //set column names
    table_model = new DefaultTableModel(headers,0);
    table = new JBTable(table_model);

    JScrollPane scrollPane = new JBScrollPane(table);
    panel.add(scrollPane);

    //table formatting
    table.setShowHorizontalLines(false);
    table.setShowVerticalLines(false);
    table.setFillsViewportHeight(true);//sets table background colour

    System.out.println("Log Viewer Table Generated..");

}

public void updateTable(String[] str){

    table_model.addRow(str);

}
}

Log file processing is handled in this class:

public class LogFileFunctions {

private BufferedReader in;

//reads the log file to retrieve data.
public String[] getData(){

    //System.out.println(System.getProperty("user.dir")); gives the executing directory
    String str; //holds string read from file
    String[] result=null; //holds tokenized string

    try {
        str = in.readLine();

        if (!(str == null)) {
            result = str.split("[|]");
        }

    }catch (FileNotFoundException e) {
        e.printStackTrace();
    } catch (IOException e) {
        e.printStackTrace();
    }

    return result;
}

//connect FileReader to log file
public void connectToLog() {

    try {
        FileReader fileReader = new FileReader(getClass().getResource("log.txt").getPath());
        in = new BufferedReader(fileReader);

        System.out.println("Log File location: " + getClass().getResource("log.txt").getPath());
        } catch (FileNotFoundException e) {
            System.out.println("ERROR: log.txt file not found at " + getClass().getResource("log.txt").getPath() );
        }
    }
}

EDIT:  I would appreciate pointers on how to optimize my code

LogToolWindowLoader.java

Constant or user property

Thread.sleep(1000);

This is a nice example of magic number.
It's better that you create at least a private static final int for it, but even better could be the possibility to change that value by a property file, with a default of 1000 if the property isn't there.
Like this you could give the users easily control that value, because in mine opinion there could be people who want it slower or faster.

To the infinitive

while(true){

I never like to see this hack.
It's a infinitive loop with no possibility to stop it.
Why not make it a variable and give the user a possibility to stop loop?

UIOperations.java

Constants

private final String headers[]= {"header1","header2","header3","header4","header5","header6"};

A good usage of the final word and array but you create this array for each instantiation of the class.
Better could make that field also static so you have instantiated this variable once for all the different instantiations of UIOperations.

Final keyword in other situations

private ToolWindow toolWindow;

This field could be final because you set it in the constructor and there is no setter for setting this at a later point.

LogFileFunctions.java

Javadoc preffered

//reads the log file to retrieve data.

//connect FileReader to log file

This could be better set as java doc in stead of normal comments.
Like this if you generate the javadoc, people who read that will also see your comment.

same or not the same

if (!(str == null)) {

What the hell?
I hope this is old code created by resolving problems because otherwise this is actually your biggest issue (In mine personally insight).
For the moment you check if it's equal and then reverse the result.
There is nothing wrong with just checking for not equal :

if (str != null) {

Naming

String str; //holds string read from file
String[] result=null; //holds tokenized string

Code should be self explaining, so if you need to put comment behind a variable name to explain it, there is something wrong.
Mine suggestion is :

String lineFromFile;
String tokenizedLine=null;

Opening and closing resources

As last in this class is for me the absence of closing your resources.
This is just a nice example to create memoryleaks.

Summary

You see you have good experience in programming, it's a nice and readable code with really tiny things I could point out.
I hope this review could help you in one or more points.


Using extdebug to display where a function was defined I wrote a tool similar to which that would display the file and line number where a function was defined. Is there a way I can do the same for aliases? Also, any comments or critiques on the code?

function where() {
  cmd="$1"
  info=$(command -V "$cmd" | head -n 1)
  #rc="$PIPESTATUS[0]"

  case $info in
    *function*)
      shopt | grep -q extdebug
      extdebug=$?
      ! ((extdebug)) && shopt -s extdebug
      declare -F "$cmd"
      ! ((extdebug)) && shopt -u extdebug
      ;;
    *) echo "$info" ;;
  esac
}

Output looks like:

$ where ls ; where l ; where bad ; where where
ls is /bin/ls
l is aliased to `ls -Al --color=auto'
bash: command: bad: not found

where 6 /home/harleypig/projects/dotfiles/.bash_sources.d/tools

I realized, even though it was working, I was capturing the output of the shopt|grep command, not the exit status.

function where() is not the right syntax and only works in some versions of bash and zsh (and a few others). It is better to write either function where or where().

shopt | grep -q extdebug is expected to always return 0 for the exit code, since extdebug is always in the output of shopt whether it's turned on or off, which is not really your intention when you test for its status. Instead, you should write shopt -q extdebug.

Looking at ! ((extdebug)) && shopt -s extdebug, I don't know if ! ((extdebug)) evaluates to true or false. It turns out it is true when extdebug equals 0 and false otherwise. This runs in counter to how the exit status of a shell command works: 0 for success and non-zero for failure. Both shopt -q extdebug and shopt | grep -q extdebug return 0 (success) if extdebug is, respectively, set and present in the output of shopt, and 1 otherwise. So you'd be setting extdebug when it is already set, and unsetting it when it is already unset. It is better to be explicit and write ! shopt -q extdebug && shopt -s extdebug instead.

It's not clear to me why info=$(command -V "$cmd") does not suffice. You should document cases where this messes up and piping to head -n 1 is needed. Though probably a tiny bit less portable, info=$(type "$cmd") works fine, too, and is shorter to type (no pun intended) and conveys the intent better.

You don't have to limit to processing only the first argument passed to this function, just as command does not process only its first argument. The change required is trivial. I include the whole modified function below, with improved formatting:

function where
{
    while (( $# > 0 ))
    do
        cmd=$1
        info=$(type "$cmd")
        case $info in
            *function*)
                #! shopt -q extdebug && shopt -s extdebug
                shopt -s extdebug
                declare -F "$cmd"
                shopt -u extdebug
                #shopt -q extdebug && shopt -u extdebug
                ;;
            *)
                echo "$info"
                ;;
        esac
        shift
    done
}

The shell option check is superfluous, so I've commented that out. Now the output would look like:

$ where ls l bad where
ls is /bin/ls
l is aliased to `ls -Al --color=auto'
bash: type: bad: not found

where 6 /home/harleypig/projects/dotfiles/.bash_sources.d/tools

The same could probably be done for alias, but this is a much more involving process. You probably need to define a function named alias that calls the builtin alias in a dotfile that is sourced first, utilize caller, do some arithmetic to get the right line number and save the result in an associative array. I'll post an update when I could get it to work.

Addendum

I have a proof of concept to do the same for alias that roughly works though not always 100% accurate:

declare -A alias_info

function alias
{
    builtin alias "$@"
    while (( $# > 0 ))
    do
        case $1 in
            *=*)
                IFS='=' read alias_name alias_command <<< "$1"
                read line_no_in_sub caller_name source_file <<< "$(caller 0)"
                if [[ -n "${source_file}" ]] # alias definition in sourced file
                then
                    # line_no_in_sub is the line number relative to the function/script block,
                    # so we have to add to it the line number of the function.
                    shopt -s extdebug
                    func_line_no=$(awk '{print $2}' <<< "$(declare -F "${func_name}")")
                    shopt -u extdebug
                    alias_info[$alias_name]="$(( line_no_in_sub + func_line_no )) ${source_file} ${alias_name} is aliased to \`${alias_command}'"
                else # alias definition from terminal
                    alias_info[$alias_name]="$(caller) $0 ${alias_name} is aliased to \`${alias_command}'"
                fi
                ;;
            *)
                ;;
        esac
        shift
    done
}


C system() function vulnerability Suppose we have the following program:

#include <stdlib.h>
#include <stdio.h>

int main()
{
        char *user = getenv("USER");
        char buffer[4096];

        if (user) {
                snprintf(buffer, sizeof buffer, "/bin/echo %s", user);
                system(buffer);
        }

        return 0;
}

Is it possible for a user to cause this program to execute arbitrary commands?

If this is actually in live code somewhere, then whomever wrote it should be forced to write I will never call system on tainted user input again 1 googol times with a dull pencil.  Yes.  As written, this code contains an exploit.  Here's a simple example.

tmp$ cat foo.c
#include <stdlib.h>
#include <stdio.h>

int
main() {
    char *user = getenv("USER");
    char buffer[4096];
    if (user) {
        snprintf(buffer, sizeof buffer, "/bin/echo %s", user);
        printf("running %s\n", buffer);
        system(buffer);
    }
    return 0;
}
tmp$ gcc foo.c
tmp$ mkdir /tmp/xxx
tmp$ ls -ld /tmp/xxx
drwxr-xr-x  2 daveshawley  users  68 Nov 30 22:25 /tmp/xxx/
tmp$ USER='foo; rm -fr /tmp/xxx' ./a.out
running /bin/echo foo; rm -fr /tmp/xxx
foo
tmp$ ls -ld /tmp/xxx
ls: /tmp/xxx: No such file or directory
tmp$


Extension method splitting string on each capital letter I'm creating my own game and for some statistics I need to get the names of specific objects. The problem was that I had them declared like "SmallFireBall" but I wanted them to appear like"Small Fire Ball" because that's something the user is able to see, so I decided to create my own extension method to help me out:

public static string SplitOnCapitalLetters(this string inputString)
{
    List<char> cleanString = inputString.ToList();
    for (int i = 1; i < cleanString.Count; i++)
    {
        if (char.IsUpper(cleanString[i]))
        {
            char[] temp = new char[cleanString.Count - i];
            for (int j = 0; j < temp.Length; j++)
            {
                temp[j] = cleanString[j + i];
            }
            cleanString[i] = ' ';
            cleanString.Add(' ');
            int index = 0;
            for (int j = i + 1; j < cleanString.Count; j++)
            {
                cleanString[j] = temp[index];
                index++;
            }
            i++;
        }
    }
    return new string(cleanString.ToArray());
}

However, I'm not quite confident about it. It seems quite big and maybe a little bit ugly, so please address any performance or code style issues.

With regex it's virtually a one-liner:

var words =
    Regex.Matches("SmallFireBall", @"([A-Z][a-z]+)")
    .Cast<Match>()
    .Select(m => m.Value);

var withSpaces = string.Join(" ", words);

Regex.Matches - searches an input string for all occurrences of a regular expression and returns all the matches. MSDN
[A-Z][a-z]+ - matches strings that begin with a capital letter and are followed by one or more lowercase letters

Output:

  Small Fire Ball

Let's review your code anyway and optimize it a little bit. Even without regex it still can be very short.

Use a StringBuilder for building string dynamically
You can use a foreach loop for strings too

public static string SplitOnCapitalLetters2(this string inputString)
{
    var result = new StringBuilder();

    foreach (var ch in inputString)
    {
        if (char.IsUpper(ch) && result.Length > 0)
        {
            result.Append(' ');
        }
        result.Append(ch);
    }
    return result.ToString();
}

The 3rd alternative would be only LINQ:

public static string SplitOnCapitalLetters3(this string inputString)
{
    // starts with an empty string and accumulates the new string into 'result'
    // 'next' is the next character
    return inputString.Aggregate(string.Empty, (result, next) =>
    {
        if (char.IsUpper(next) && result.Length > 0)
        {
            result += ' ';
        }
        return result + next;
    });
}


Need to condense the following into a looping function I have the following code, which works as I need it to, however, it repeats itself over and over.

I'm relatively new to PHP and haven't grasped recursive functions yet. Which I've been told I'll need to understand as this function could potentially have many levels.

if ($_POST['parent'] == "None") {
        $parent = "";
    } else {
        $parent = $connection->real_escape_string($_POST['parent']);
    }

    if (!empty($parent)) {

        $query = $connection->query("SELECT * FROM pages WHERE filename='$parent'");

        while($row = $query->fetch_array()) {

            $path_arr = explode('/',$row['path_to']);
            end($path_arr);
            $key = key($path_arr)-1;

            $parent1 = $path_arr[$key];

            if (!empty($parent1)) {

                $query = $connection->query("SELECT * FROM pages WHERE filename='$parent1'");

                while($row = $query->fetch_array()) {

                    $path_arr = explode('/',$row['path_to']);
                    end($path_arr);
                    $key = key($path_arr)-1;

                    $parent2 = $path_arr[$key];

                    if (!empty($parent2)) {

                        $query = $connection->query("SELECT * FROM pages WHERE filename='$parent2'");

                        while($row = $query->fetch_array()) {

                            $path_arr = explode('/',$row['path_to']);
                            end($path_arr);
                            $key = key($path_arr)-1;

                            $parent3 = $path_arr[$key];

                            if (!empty($parent3)) {

                                $query = $connection->query("SELECT * FROM pages WHERE filename='$parent3'");

                                while($row = $query->fetch_array()) {

                                    $path_arr = explode('/',$row['path_to']);
                                    end($path_arr);
                                    $key = key($path_arr)-1;

                                    $parent4 = $path_arr[$key];

                                }
                            }
                        }
                    }
                }
            }
        }
    }

    $path_to = rtrim(ltrim($parent4.'/'.$parent3.'/'.$parent2.'/'.$parent1.'/'.$parent.'/'.$filename,'/'),'/');

As you can see there are minimal differences within the nested while loops, but I'm really not sure how to make the function loop for each level.

I was pointed to this, and while I understand what the factorial is, I still am unsure on how to refactor my code into a looping function.

Any pointers?

UPDATE

Certainly moving in the right direction of what I'm hoping to achieve with Simon André Forsberg's suggestion

However (I probably should have made this clearer in my explanation), The loop(s) determine whether a 'file' (a database entry, masquerading as a file) has a parent 'file', returns that parent 'file', then checks that parent 'file' for a parent 'file' and returns it and so on and so on until there are no more parents.

It is used to create a dynamic path directory.

like so /parent4/parent3/parent2/parent1/filename/

The line of code that follows the loops (again, I probably should have included this), is below...

$path_to = rtrim(ltrim($parent4.'/'.$parent3.'/'.$parent2.'/'.$parent1.'/'.$parent.'/'.$filename,'/'),'/');

So each level of the loop outputs to this string.

I have a feeling I may have to do this using palacsint's method of taking the working part of each loop and creating that as a function.

I may be wrong but could Simon's method output to an array? first parent in pos 0, second in pos 1, etc?

However, then the query would have to have the filename='$parent' updated to look at the last array entry.

$query = $connection->query("SELECT * FROM pages WHERE filename='$parent'");

Have I bitten off more than I can chew?

You seem to always end with the check: Is there another parent? Or as it can be formulated, while there is another parent, grab that parent.

Luckily for you, there are while loops.

while (!empty($parent)) {
    $query = $connection->query("SELECT * FROM pages WHERE filename='$parent'");
    while($row = $query->fetch_array()) {
        $path_arr = explode('/',$row['path_to']);
        end($path_arr);
        $key = key($path_arr)-1;
        $parent = $path_arr[$key];
        break; // break from the inner loop that fetches each row
    }
}

This code will continue looping while there is a parent available. Note that the new parent uses the same variable as the previous one, this is an important part in making this loop work.

Other suggestions:

Use prepared statements! It is good that you seem to sanitize your inputs by using the real_escape_string method. But prepared statements is always better. And then you wouldn't have to use the real_escape_string call.
Since this code only gets the first result, you don't need the inner while loop and can replace it by a if instead.

while (!empty($parent)) {
    $query = $connection->query("SELECT * FROM pages WHERE filename='$parent'");
    if ($row = $query->fetch_array()) {
        $path_arr = explode('/',$row['path_to']);
        end($path_arr);
        $key = key($path_arr)-1;
        $parent = $path_arr[$key];
    }
}

I do have to ask though: What's the point in just grabbing the parent until there are no more parents? To store all the parents in an array, let's do like this:

$parents = array();
while (!empty($parent)) {
    $parents[] = $parent; // add parent to the array
    $query = $connection->query("SELECT * FROM pages WHERE filename='$parent'");
    if ($row = $query->fetch_array()) {
        $path_arr = explode('/',$row['path_to']);
        end($path_arr);
        $key = key($path_arr)-1;
        $parent = $path_arr[$key];
    }
}

Now, when this loop finishes, $parents contains all the parents that was not empty. Now you can loop through that using a foreach loop, or use it with implode or whatever you'd like :)


Calculate strides to run up stairs The staircase has between 1 and 50 flights of stairs, inclusive. Each flight of stairs has between 5 and 30 steps, inclusive. StepsPerStride is between 2 and 5, inclusive. It takes 2 strides to turn around on the landing.

Example:

Input: {15, 15}, StepsPerStride: 2, Returns: 18

There are two flights with a landing in between. 8 strides to get to the first landing, 2 strides to turn around, and 8 more strides to get to the top makes 8+2+8=18 strides

 public class RunUpTheStairs : IRunUpTheStairs
    {
        public ushort CalculateTotalStrides(byte[] stairs, byte stride)
        {
            if (!this.Validate(stairs, stride))
            {
                throw new ArgumentOutOfRangeException("stairs and / or stride are out of range.");
            }

            byte numberOfStrides = 0;
            foreach (byte flight in stairs)
            {
                numberOfStrides += (byte)(flight / stride);

                if (flight % stride != 0)
                {
                    numberOfStrides++;
                }
            }

            ushort totalStrides = (ushort)(numberOfStrides + (stairs.Length - 1) * 2);
            return totalStrides;
        }

        private bool Validate(byte[] stairs, byte stride)
        {
            if (stairs.Length < 1 || stairs.Length > 50)
            {
                return false;
            }

            if (stairs.Any(nextFlight => nextFlight < 5 || nextFlight > 30))
            {
                return false;
            }

            return stride >= 2 && stride <= 5;
        }
    }

I am posting this for code review. Any improvements for performance will be much appreciated too!

You have one exception for different parameters and I believe it is wrong. You should separate validation of each argument and also use clear message instead of just "something is out of range".

Also I recommend to use ArgumentOutOfRangeException only for arguments that represent single value that is out of range. Length of an array argument is not an argument itself. Some invalid value inside the array is not an argument too.

So define validation methods

private bool IsStairsSizeValid(byte[] stairs)
{
    return stairs.Length >= 1 && stairs.Length <= 50;
}

private bool AreStairsValuesValid(byte[] stairs)
{
    return stairs.All(nextFlight => nextFlight >= 5 && nextFlight <= 30);
}

private bool IsStrideValid(byte stride)
{
    return stride >= 2 && stride <= 5;
}

and then use them

if (!IsStairsSizeValid(stairs))
{
    throw new ArgumentException("Stairs has invalid size.", nameof(stairs));
}

if (!AreStairsValuesValid(stairs))
{
    throw new ArgumentException("Stairs contains invalid value.", nameof(stairs));
}

if (!IsStrideValid(stride))
{
    throw new ArgumentOutOfRangeException(nameof(stride), stride, "Stride is out of range.");
}

Also you should always check arguments of reference type on null in public API:

if (stairs == null)
{
    throw new ArgumentNullException(nameof(stairs));
}

I don't see any reason the numberOfStrides variable should have byte type. You use it within integer calculations so there is no point in byte. Declare it as int and remove casting to byte in the loop.

int numberOfStrides = 0;
foreach (var flight in stairs)
{
    numberOfStrides += flight / stride;

If you have division and remainder calculation for the same values it can be better to use Math.DivRem method:

numberOfStrides += Math.DivRem(flight / stride, out var remainder);
if (remainder != 0)
{
    numberOfStrides++;
}

There is no need to store result of calculations to local variable if you return it immidiately. So you can write just

return (ushort)(numberOfStrides + (stairs.Length - 1) * 2);


Using for loops create new object with other objects with matching id's I'm not sure if this is the best way to write this code..

object.array.forEach((item) => {
  object.otherarray.array.forEach((asset) => {
    if(item.fields.image.sys.id === asset.sys.id) {
      item.fields.image['fields'] = asset.fields;
    }
  })
});

any help would be appreciated!

In this loop object.otherarray.array you use id property to to ensure to have necessary match. So I assume this item.fields.image.sys.id === asset.sys.id always should have only one match. And it resulted in the following code:

object.array.forEach((item) => {
    item.fileds.image.fields = object.otherarray.array.filter(
        (asset) => item.fields.image.sys.id === asset.sys.id
    )[0].fields;
});

Some explanation:

object.otherarray.array.filter(
        (asset) => item.fields.image.sys.id === asset.sys.id
    )[0];

is an equivalent of your item.fields.image.sys.id === asset.sys.id. In other words, firstly we find necessary asset. Then when we have it we can use fields property to assign it to item.fileds.image.fields.

I also have another variant of my remake:

object.array.forEach((item) => {
    const imageId = item.fields.image.sys.id;
    item.fileds.image.fields = object.otherarray.array
        .filter((asset) => imageId === asset.sys.id)
        [0].fields;
});

While it is more verbose version, it is more clear and structive so I would recommend to use this one.

I am open to your comments as I suppose I could not understand your correctly.

Edit: as @MarcRohloff said, you can use find instead of filter. But note, that `find has poor support in browsers:

object.array.forEach((item) => {
    const imageId = item.fields.image.sys.id;
    item.fileds.image.fields = object.otherarray.array
        .find((asset) => imageId === asset.sys.id)
        .fields;
});


Limiting function invocations using closures Problem statement:

  We have a number of functions in our web application that are getting called too frequently. We'd like you to create a function that, given a function and a time interval, returns a new function that we can call as often as we like, but ensures that the original function is never called more than once per interval.

  Example usage:

limitedDoSomething = rateLimit(doSomething, 500);

  Now calls to limitedDoSomething will simply call doSomething, but never more than once every 500 milliseconds.

Test Cases:

function logHello(){ // just a sample function
  console.log('Hello!');
}

limitedDoSomething = rateLimit(logHello, 500);

limitedDoSomething(); // should log “Hello!”

window.setTimeout(function(){
  limitedDoSomething();  // should not log “Hello!"
}, 400);

window.setTimeout(function(){
  limitedDoSomething();  // should log “Hello!"
}, 500);

My solution:

function rateLimit(func, limit){
  var lastInvokedTimestamp;

  return function(){
    if(typeof lastInvokedTimestamp === 'undefined' || Date.now() - lastInvokedTimestamp >= limit){
      lastInvokedTimestamp = Date.now();
      console.log('go ahead!');
      return func();
    } else {
      console.log('too early!');
      return;
    }
  };

}

// run test cases
function logHello(){ // just a sample function
  console.log('Hello!');
}

limitedDoSomething = rateLimit(logHello, 500);

limitedDoSomething(); // should log “Hello!”

window.setTimeout(function(){
  limitedDoSomething();  // should not log “Hello!"
}, 400);

window.setTimeout(function(){
  limitedDoSomething();  // should log “Hello!"
}, 500);

Equivalent jsBin

I would love to know if there is a way to improve my solution.

This is called a debounce function, and you are doing it ok.

Here are some small improvements I can think of.  Nothing big:

Initialise lastInvokedTimestamp, so that it'll never be undefined.
Cache current time.
Track the threshold of next call instead, so that it becomes one addition per interval instead of one subtraction per entrance.
Pass arguments into the debounced function.
Simplify test cases's setTimeout.

function rateLimit ( func, interval ) {
  var nextInvokeTimestamp = 0;

  return function(){
    var now = Date.now();
    if ( now < nextInvokeTimestamp  ) {
      return console.log('too early!');
    }
    nextInvokeTimestamp = now + interval;
    console.log('go ahead!');
    return func.apply( this, arguments );
  };

}

// run test cases
function logHello( message ){
  console.log( message || 'Hello!' );
}
limitedDoSomething = rateLimit( logHello, 500 );
limitedDoSomething( "Welcome!" ); // should log “Welcome!”
setTimeout( limitedDoSomething, 400 );  // should not log “Hello!"
setTimeout( limitedDoSomething, 500 );  // should log “Hello!"

You may notice that David Walsh is doing it quite differently.

Using setTimeout defers the actual call to a later event loop, instead of current event, to let things settle down a bit.
Because of the delay (which can be as low as 0, i.e. 4ms), he can call with the latest arguments in the interval, for example the latest mouse move. A bigger interval lets you catch more calls, with a slower respond.

It is not better or worse; it's just a different speed bump for different needs.
If your requirement is simply "reduce multiple calls into one", you are doing fine.


Create a deck of playing cards & draw a hand of cards I built a very basic blackjack program a couple of weeks ago, but I've learned a lot since then, and would like some input on a couple of basic functions for card games: a build_deck function and a draw_hand function.

It seems pretty efficient use of code (at least compared to my previous attempt) but I'm sure there are some ways I can improve on it.

import random

def build_deck():
# The deck is a list of tuples, with each tuple representing a card.
# Each card has a name, face, suit, and value.
    deck = []
    value = 0
    face = ''
    for value in range(13):
        value += 1
        suit_list = ['Hearts', 'Diamonds', 'Clubs', 'Spades']
        for suit in suit_list:
            if value == 1:
                face = 'Ace'
            elif value == 11:
                face = 'Jack'
            elif value == 12:
                face = 'Queen'
            elif value == 13:
                face = 'King'
            else:
                face = str(value)
            name = 'the %s of %s' % (face, suit)
            # The deck is shuffled only once, when the deck is first built.
            random.shuffle(deck)
            deck.append((name, face, suit, value))
    return deck

def draw_hand(hand_size):
    # The player receives a card into their hand while the same card is removed from the deck.
    player_hand = []
    for draw in range(int(hand_size)):
        selected_card = (random.choice(deck))
        print('You drew %s.' % (selected_card[0]))
        player_hand.append(selected_card)
        deck.remove(selected_card)
    return player_hand

deck = build_deck()
player_hand= draw_hand(input('How many cards do you want to draw? There are %s cards in the deck.' % (str(len(deck)))))

Shuffling the Deck

Your comment # The deck is shuffled only once, when the deck is first built is wrong.  You are shuffling the deck 52 times; once after before each card is added:

    deck = []
    for value in range(13):
        suit_list = ['Hearts', 'Diamonds', 'Clubs', 'Spades']
        for suit in suit_list:
            ...
            random.shuffle(deck) # This line is executed 13 * 4 times!
            deck.append((name, face, suit, value))
    return deck

Additionally, you are grabbing cards from a random location in the middle of the deck, so it doesn't matter if you shuffled the deck or not:

        selected_card = (random.choice(deck))
        ...
        deck.remove(selected_card)

You should build the complete deck, and then shuffle it once:

    deck = []
    for value in range(13):
        suit_list = ['Hearts', 'Diamonds', 'Clubs', 'Spades']
        for suit in suit_list:
            ...
            deck.append((name, face, suit, value))

    random.shuffle(deck)
    return deck

And then, deal from the top of the deck:

        selected_card = deck.pop(0)

Suits

    for value in range(13):
        value += 1
        suit_list = ['Hearts', 'Diamonds', 'Clubs', 'Spades']
        for suit in suit_list:

The suit_list doesn't change.  It doesn't need to be recreated for each of the 13 card ranks.  You should move it out of the inner loop.

There it is still a local variable.  Code which wants to use your deck of cards might want/need to know the what all the suits in your deck are.  Is it a normal playing card deck, or a deck of Tarot cards?  Is the spade suit represented as "Spades", "SPADES", "S", "♠" or "♤"?

An enum would be a better entity to use as a card suit:

from enum import Enum

Suit = Enum('Suit', 'Clubs, Diamonds, Hearts, Spades')

Then a program could safely refer to the spade suit as Suit.Spades, and iinstead of:

        suit_list = ['Hearts', 'Diamonds', 'Clubs', 'Spades']
        for suit in suit_list:

You'd simply iterate over the Suit enum:

        for suit in Suit:

You can still use suit.name to get a nice string for the suit.

range(start, end, step)

Instead of writing:

    for value in range(13):
        value += 1
        ...

use the fact that a range can start at any value, so you don't need to add 1 to the card's rank at each iteration; just iterate over a different range:

    for value in range(1, 13+1):
        ...

Each tuple represents a card ...

Ok ... lemme think.  The card[0] is the card's rank, and card[1] is the card's suit?  Or do have have that backwards ; card[0] is the suit and card[1] is the rank?

With a tuple, it is easy to forget which member is stored in which field.  It is way easier to use a named tuple:

from collections import namedtuple

Card = nametuple('Card', 'name, face, suit, value')

Then we wouldn't have to remember; card.rank is the the card's rank, and card.suit is the card's suit.

The only downside is ... uhh ... no, sorry, there are no downsides.  A namedtuple is just as efficient time and space wise.  You'd create the cards like this:

        deck.append(Card(name, face, suit, value))

And instead of

    print('You drew %s.' % (selected_card[0]))

you'd write:

    print('You drew %s.' % selected_card.name)

Formatting

The inner parenthesis are unnecessary here:

    print('You drew %s.' % (selected_card[0]))

They take the value "the Ace of Spaces", and ... return that string unaltered.  So we end up with the expression str % str, and since the first string contains only one %s code, the argument is directly used.

if you had used (selected_card[0], ), that would have constructed a tuple of 1 values, a string, which could also be applied to that format string.  Without the trailing comma, you don't have a tuple.

It is easier to use f-strings.  There, the format arguments are placed directly in the format codes, instead of at the end where they have to be match up by positions.  Instead of:

            name = 'the %s of %s' % (face, suit)

you could write:

            name = f'the {face} of {suit}'

which is slightly more compact, and much easier to tell where the values are going in the resulting string.

Hand size

def draw_hand(hand_size):
    player_hand = []
    for draw in range(int(hand_size)):

Wait ... what is that int(...) doing there?  What are you expecting to pass to draw_hand, if not an integer?  Why would you allow it to be a string?

It should be the caller's responsibility for any string to integer conversions.  This function should only expect an integer for hand_size.

Improved code

Using NamedTuple instead of namedtuple, a Rank enumeration, and reducing Card to just a tuple of rank and suit, with str(card) corresponding to the card's name, and card.rank.value for the card's value ... along with some other structural modifications:

from random import shuffle
from enum import Enum
from typing import NamedTuple, List
from itertools import product

Suit = Enum('Suit', 'Clubs, Diamonds, Hearts, Spades')
Rank = Enum('Rank', 'Ace, 2, 3, 4, 5, 6, 7, 8, 9, 10, Jack, Queen, King')

class Card(NamedTuple):
    rank: Rank
    suit: Suit

    def __str__(self):
        return f"{self.rank.name} of {self.suit.name}"

def build_deck() -> List[Card]:
    deck = [Card(rank, suit) for rank, suit in product(Rank, Suit)]
    shuffle(deck)
    return deck

def draw_hand(hand_size: int) -> List[Card]:
    return [deck.pop(0) for _ in range(hand_size)]

def print_hand(hand: List[Card]) -> None:
    print("You drew:", ", ".join(str(card) for card in hand))

if __name__ == '__main__':
    deck = build_deck()
    n = int(input(f"How many cards? There are {len(deck)} cards in the deck: "))
    hand = draw_hand(n)
    print_hand(hand)

Result:

  How many cards? There are 52 cards in the deck: 5
  You drew: 2 of Hearts, 9 of Clubs, 5 of Hearts, 7 of Hearts, Queen of Clubs


Validating basic data objects I'm playing around with trying to do things in a better way, and in a class I have for sending emails, I have the concept of a recipient. This gets passed into my other classes essentially as a struct that is a little more descriptive than an associative array.

I'd like to know if the Recipient is going to be valid to pass to the rest of my code, and hence want a way to determine if the email address has any basic errors in it. (I'm aware that validating emails can only be done by sending them an email, for the purposes of this, just ignore this fact!)

Anyway, I have this class that looks like this:

class Recipient
{
    public $email_address, $name;

    public function __construct($email_address, $name) {
        $this->email_address = $email_address;
        $this->name = $name;
    }

    function emailIsValid() {
        return filter_var($this->email_address, FILTER_VALIDATE_EMAIL);
    }

    public function isValid() {
        return ($this->emailIsValid() && $this->name != '');
    }
}

And I'm calling it like:

$recipient = new Recipient('test@example.com', 'Example Name');

then, later when I'm about to send an email to something I can run:

if ($recipient->isValid()) {
    // add to the list of recipients
}

I'm interested on all and any feedback on this - I'm not that confident that this is a 'good' way to do things because:

This class doesn't really do anything, and is really a pretty struct
The validation isn't specific to this place, I may want to check emails in other places
Should the validation happen in the constructor? Should it be a separate class? What would that look like?
Should there be an interface for every class? Would that live at the top of this file?
Should I throw exceptions when it's not valid? Should they be an object as well? How?

Let's look at all of your questions, one by one:

This class doesn't really do anything, and is really a pretty struct

Yes and no. This class has a clear task within the application: it is passed around, carrying data. It represents a number of values that belong together. It's a model. Having data containers isn't just OK, it's good practice. You can use type-hinting, and doc-blocks to make your code easier to use.
You call it a glorified struct, well: in C, a FILE * is just a struct, too, but you rarely use it as such: There's an entire API that handles the nitty-gritty for you, in the same way that you can use this model to validate the data you are setting.

Code that requires this data, either to store it in a DB, or present it to the client side in a view, can then be written with a clear contract:

public function showRecipient(Recipient $recipient)
{
    //if an instance is passed, its email address is expected to be set and valid
}

But how do you best make sure when, and where to validate the email address:

The validation isn't specific to this place, I may want to check emails in other places

Don't. If you are processing data that belongs to, or is part of the dataset that makes up a Recipient, the thing to do is to wrap this data in the appropriate container: a Recipient instance.
If you validate the email elsewhere in your code, you are repeating yourself. Simply pour it into an object, that makes sure the data is valid, and pass that object around. Whatever values you extract from it (through getters), is guaranteed to be validated.

Should the validation happen in the constructor? Should it be a separate class? What would that look like?

The validation should Not be done in the constructor. I can construct an instance, and re-assign/reset the properties to a particular value, bypassing the constructor's  validation.
A separate class, then? No. Really... why would you centralize all data validation/sanitation? Such a class would be either an all-static monster, or you'd initialize all validation code, every time you want to validate a single value.

I'd make my properties protected, and use setters. These setter methods are tailored to the type of property they are setting, and can perform the necessary sanitation and validation. Simple.

Should there be an interface for every class? Would that live at the top of this file?

Depends. Is it worth while? Are there similarities between other models (database mapping: getId() and setId methods)? Is there a certain type of functionality you want to add (for example implementing one of the Traversable interfaces).
Hard to say from what code you shared, but if this is the only data-class then: no. An interface is probably not required.

Should I throw exceptions when it's not valid? Should they be an object as well? How?

Yes: a class has just one job. This class' job is to contain valid data. Raw data is passed to its setters, which validate/sanitize this data. If the data is malformed/invalid, the Recipient class is not responsible for it: the code that supplied the data is. The Recipient should throw an exception (it encountered exceptionally formed data), and the calling code should handle the problem.
Should these exceptions be objects: Of course, they are. There's no way around them. If you mean: should I throw or set a property, the answer is: throw. Your class encountered a problem. Setting a property or returning false assumes the calling code is checking return values or flags. This is not a given (return values are often ignored, and error flags seldom checked).
Throwing an exception forces the calling code to respond

To answer the how question, here's how I'd write your class:

class Recipient
{
    protected $email = null;
    protected $name = null;
    public function __construct(array $data = array())
    {
        foreach ($data as $key => $val)
        {
            //transform key to method
            $method = 'set'.ucfirst(strtolower($key));
            if (method_exists($this, $method))
                $this->{$method}($val);
        }
    }
    public function setEmail($email)
    {
        if (!filter_var($email, FILTER_VALIDATE_EMAIL))
        {//not a valid email address, throw exception
            throw new InvalidArgumentException(
                sprintf(
                    '%s expects valid email address, %s is not',
                    __METHOD__,
                    $email
                )
            );
        }
        $this->email = $email;
        return $this;
    }
    public function setName($name)
    {
        //add checks/sanitation you need: check length, check format, check for numeric chars...
        $this->name = trim($name);
        return $this;
    }
    public function getEmail()
    {
        return $this->email;
    }
    public function getName()
    {
        return $this->name;
    }
}

That should do it. Usage examples:

$data = array('email' => 'foo@bar.com', 'name' => 'foo');
$recipient = new Recipient($data);
echo 'From instance - email: ', $recipient->getEmail();
$recipient->setName('    Foo');
echo 'After setter: ', $recipient->getName();//leading spaces are removed

Now, why did I default the constructor argument to an empty array? Simply because it allows you to do this:

$blank = new Recipient();
$obj->EmailGetter($queryObject, $blank);//gets email, sets it on instance
$blank->setName('Unknonwn');

I can create an object, then fetch the data as I go along, and set the properties accordingly.
As far as the exception business is concerned:

$error = new Recipient(
    array(
        'email' => 'invalid'
    )
);//error
//or
$test = new Recipient();
$value = 'invalidaddress';
try
{
    $test->setEmail($value);
    echo 'If $value is valid, this will echo its value again here: ', $test->getEmail();
}
catch (InvalidArgumentException $e)
{//echos "invalidaddress was refused:  Recipient::setEmail expects valid email address invalidaddress is not"
    echo $value, ' was refused: ', $e->getMessage();
}

I've reviewed a couple of validation + data-model related questions, so I thought I'd add some of my (lengthy) reviews here, too. For completeness' sake:

details on Traversable interface
Bulk setting + inheritance
Some more bulk setting
Vaguely related


Search set, return item Is there a more efficient way of achieving the same result besides setting up a database?

# In reality, contains 4 million tuples.
stuff_tuple = (
    ('hello', 241, 1),
    ('hi', 243, 3),
    ('hey', 242, 2)
)

# Convert tuple to set for faster searching?
stuff_set = set(stuff_tuple)

# Only first two values are known ("hello", 241), ('hi', 243), etc.
for item in stuff_set:
    if item[0] == 'hello' and item[1] == 241:
        print item[2]

If all your queries are similar to the example, you would obviously benefit from storing the data in a dict where a tuple of two values is the key, and the value is a list of ints -- all of the third items that share the same key.

from collections import defaultdict

stuff_dict = defaultdict(list)
for t in stuff_tuple:
    stuff_dict[t[:2]].append(t[2])

for item in stuff_dict['hello', 241]:
    print item


Comma formatted list printer I'm working on a list practice project from the book "Automate the Boring Stuff with Python" which asks for this:

Write a function that takes a list value as an argument and returns a
string with all the items separated by a comma and a space, with and
inserted before the last item. For example, passing the previous spam
list to the function would return 'apples, bananas, tofu, and cats'.
But your function should be able to work with any list value passed to
it. Be sure to test the case where an empty list [] is passed to your
function.

So far I've come up with this:
def comma_code(iterable):
    '''
    Function that loops through every value in a list and prints it with a comma after it,
    except for the last item, for which it adds an "and " at the beginning of it.
    Each item is str() formatted in output to avoid concatenation issues.

    '''
    for i, item in enumerate(iterable):
        if i == len(iterable)-1 and len(iterable) != 1: # Detect if the item is the last on the list and the list doesn't contain only 1 item (BONUS)
            print('and ' + str(iterable[-1]))           # Add 'and ' to the beginning
        elif len(iterable) == 1:                        # BONUS: If list contains only 1 item,
            print('Just ' + str(iterable[-1]))          # replace 'and ' with 'Just '
        else:                                           # For all items other than the last one
            print(str(iterable[i]) + ',', end=" ")      # Add comma to the end and omit line break in print

There's heavy commenting because I'm fairly new and I'm trying to leave everything as clear as possible for my future self.
Now I wonder if there's a better way of doing this and also (subjective question) if there is something in my code that I should change for better readability and/or style. As I said, I'm fairly new and I would like to pick good coding practices from the beginning.
These are a couple of lists I ran through the function:
spam = ['apples', 'bananas', 'tofu', 'cats']
bacon = [3.14, 'cat', 11, 'cat', True]
enty = [1]

And this is the working output:
apples, bananas, tofu, and cats
3.14, cat, 11, cat, and True
Just 1

Thanks in advance.

An alternate approach

The routine is same for all types of inputs, except for when the len(list) == 1. This is why we can use a generator function  to simplify the routine, and then a simple if statement to work with the exception

def comma_code(iterable):
    result = ', '.join(str(value) for value in iterable[:-1])
    return f"{result}, and {iterable[-1]}"

Now, since we have to deal with a special case, an if-statement can work
def comma_code(iterable):
    if not iterable: return None
    if len(iterable) == 1: return f"Just {iterable[0]}"

    result = ', '.join(str(value) for value in iterable[:-1])
    return f"{result}, and {iterable[-1]}"

The question hasn't stated what to do if the size of the container is 0, hence I have just returned None. As stated by @Stef in the comments, you can also return an empty string which is just ''.
I would prefer None as it becomes easy to trace back if an issue occurred. Moreover, you can also design a simple function that would check if the container is empty. If it is then the function can print a custom warning which can help you in debugging your code
Explanation
result = ', '.join(str(value) for value in iterable[:-1])

This is called a generator function, we're basically appending str(value) + ', ' for every value in iterable. iterable[:-1] so that we only iterate till the second last element.
to be fully verbose, :-1 says everything in iterable up till the last element


Spoj's 1st problem in tdd using Ruby I am pretty new to TDD and is following problems on spoj.pl for practice. I need help with following code; it was my first attempt.

Problem: Your program is to use the brute-force approach in order to find the Answer to Life, the Universe, and Everything. More precisely... rewrite small numbers from input to output. Stop processing input after reading in the number 42. All numbers at input are integers of one or two digits. Mentioned here.

Tests:

describe "life universe and everything" do

 before(:each) do
  @life = Life.new
 end

 it "should accept input" do
  @life.stub(:gets) { "5, 4, 23, 42,  4" }
  @life.input.should == [5,4,23,42,4]
 end

 it "check number validity" do
   @life.valid?(23).should be_true
   @life.valid?(42).should be_false
 end

 it "process input" do
   array1 = [5,4,23,42,4]
   @life.process_input(array1).should == [5,4,23]
   array2 = [1,4,76,34,90]
 @life.process_input(array2).should == [1,4,76,34,90]
end

end

Code:

class Life

 def input
  puts "List comma(',') seperated numbers. Press enter when done."
  numbers_string = gets
  numbers = numbers_string.split(',')
  numbers.map {|n| n.strip.to_i}
 end

 def valid?(number)
  (number==42) ? false : true
 end

 def process_input(numbers)
  processed_numbers = []
  numbers.each do |number|
  break unless valid?(number)
  processed_numbers << number
 end
  processed_numbers
end

end

I would like it to be reviewed and would be glad to know my mistakes and what can I do to improve quality of code. Thanks!

Congratulations.  That's a pretty nice first attempt.

I see only a few issues:

The problem statement shows that you will receive one number per line; your code expects one line with multiple numbers, separated by commas.
The problem statement asks you to print the numbers; your program doesn't.
When run, the test writes its prompt to $stdout.  Tests should be quiet when passing.

The issue with the prompt can be fixed with an expectation:

@life.should_receive(:puts)\
.with("List comma(',') separated numbers.  Press enter when done"

If your intent is to solve a slightly different problem, then we won't worry about the differences between what your program does and what they're asking for.  But read below the fold for a simple test and implementation, TDD Style, for the stated problem.

This problem can be solved pretty simply.  For example,

  loop do
    n = gets.chomp.to_i
    puts n
    break if n == 42
  end

What test or tests could you write that would allow you to have code this simple?  Let's see if we can get there from nothing:

class Life
  def process_input
  end
end

describe Life do

  it "should copy numbers from $stdin to $stdout" do
    life = Life.new
    life.should_receive(:gets).with(no_args).and_return('41')
    life.should_receive(:puts).with(41)
    life.process_input
  end

end

Running that, we find that the spec fails because the code never called gets.  We add the call to gets, then find that the spec fails because the code never called puts.  Adding them, we get a passing test:

class Life
  def process_input
    puts gets.to_i
  end
end

Of course, we've got no loop, so let's tell the test there should be multiple calls to gets and puts.  We'll use .ordered to tell rspec that the calls should occur in a certain order.

it "should copy numbers from $stdin to $stdout" do
  life = Life.new
  life.should_receive(:gets).ordered.with(no_args).and_return('41')
  life.should_receive(:puts).ordered.with(41)
  life.should_receive(:gets).ordered.with(no_args).and_return('42')
  life.should_receive(:puts).ordered.with(42)
  life.process_input
end

Now there needs to be a loop.  Let's add it:

def process_input
  loop do
    puts gets.to_i
  end
end

Our test neither passes nor fails.  Instead, it hangs.  That's because we didn't give it any way to get out of the loop.  Let's add the termination condition:

def process_input
  loop do
    n = gets.to_i
    puts n
    break if n == 42
  end
end

So, we can write a test that lets us have pretty simple code.  But in a few ways, this test is bothersome.  One reason is that it is too picky about the way that the I/O is done.  What if we changed the program to use print instead of puts?  It would have produce exactly the same output, but the test would fail.  Also, having the test hang when the program fails to terminate isn't very good.  We want tests that pass or fail quickly and cleanly, with no guessing.  So let's rewrite our test (and the program) using dependency injection for I/O.

Starting over, we're going to change the the Life program so that it takes and input object and an output object:

class Life

  def initialize(input = $stdin, output = $stdout)
    @input = input
    @output = output
  end

  def process_input
  end

end

And our test:

require 'stringio'

describe Life do

  it "should copy numbers from input to output" do
    input = StringIO.new("41\n")
    output = StringIO.new
    life = Life.new(input, output)
    life.process_input
    output.string.should == "41\n"
  end

end

Using StringIO instances as mock I/O objects is very handy, and lets us test the result of the I/O rather than the actions that produce the I/O.  That's nice.

Since our program doesn't process anything yet, the test fails:

  1) Life should copy numbers from $stdin to $stdout
     Failure/Error: output.string.should == "41\n"
       expected: "41\n"
            got: "" (using ==)

Let's fix that:

  def process_input
    @output.print @input.gets
  end

The test now passes.  Let's modify the test to show that it should loop:

  it "should copy numbers from $stdin to $stdout" do
    input = StringIO.new("41\n42\n")
    output = StringIO.new
    life = Life.new(input, output)
    life.process_input
    output.string.should == "41\n42\n"
  end

The test fails:

  1) Life should copy numbers from $stdin to $stdout
     Failure/Error: output.string.should == "41\n42\n"
       expected: "41\n42\n"
            got: "41\n" (using ==)

Let's add the loop:

  def process_input
    loop do
      @output.print @input.gets
    end
  end

Now it hangs again!  That's because I/O objects like StringIO and $stdin just return a nil when they run out of input.  Our program is happily printing nil over and over.  Let's make the test die nicely when the loop fails to terminate:

  it "should copy numbers from $stdin to $stdout" do
    input = StringIO.new("41\n42\n43\n")
    def input.gets
      super.tap { |s| raise "no more input" unless s }
    end
    output = StringIO.new
    life = Life.new(input, output)
    life.process_input
    output.string.should == "41\n42\n"
  end

Here we monkey patch our input objects so that when it runs out of input, it raises an error rather than just returning nil.  The result:

  1) Life should copy numbers from $stdin to $stdout
     Failure/Error: super.tap { |s| raise "no more input" unless s }
     RuntimeError:
       no more input

Great!  let's get the loop to terminate when it encounters that "42":

  def process_input
    loop do
      s = @input.gets
      @output.print s
      break if s == "42\n"
    end
  end

Now we've got a robust test that doesn't care about the details of how I/O is done, and code that is pretty simple.

There's just one more enhancement I'd make to this program, and that is to have it exit when it runs out of input.  While not strictly required by the problem statement, a program that prints blank lines endlessly upon EOF is obnoxious!  Now we need another "it" in our test:

  it "should exit on EOF" do
    input = StringIO.new("41\n<EOF>")
    def input.gets
      s = super
      case s
      when "<EOF>"
        nil
      when nil
        raise "no more input"
      else
        s
      end
    end
    output = StringIO.new
    life = Life.new(input, output)
    life.process_input
    output.string.should == "41\n"
  end

Our monkey-patched input object just got a little more interesting.  We want it to return "nil" on end of input, because that's how the loop will detect end of input in real life, but we want it to raise an exception if the code under test tries to get more input after the nil.  This test fails:

  1) Life should exit on EOF
     Failure/Error: raise "no more input"
     RuntimeError:
       no more input

So let's make the loop exit on end of input:

  def process_input
    loop do
      s = @input.gets
      @output.print s
      break if s.nil?
      break if s == "42\n"
    end
  end

Now the program meets the stated requirements, and our own requirement of not filling the output with blank lines on end-of-input.


Importing multiple worksheets I use the following code to import multiple worksheets from another workbook and do some processing. The importing time is too long. Can anyone suggest a more efficient way to import? Should I be looking into more information in the source files for copy? Can anyone please suggest a way to speed up the import function?

I am not a professional developer, so I might have left out some key points to note. Please let me know if I need to look into something else, as well.

Sub SKR_Import()
On Error GoTo errorhandler
Application.ScreenUpdating = False
Application.DisplayAlerts = False
Dim wb1 As Workbook
Dim wb2 As Workbook
Dim Sht As Worksheet
Set wb1 = ActiveWorkbook
Dim fd As FileDialog
Dim filechosen As Integer
Dim filename As String
Dim i As Long
Set fd = Application.FileDialog(msoFileDialogOpen)
fd.AllowMultiSelect = True
fd.Title = "Select Excel workbooks to import all sheets"
filechosen = fd.Show
If filechosen = -1 Then
    For i = 1 To fd.SelectedItems.Count
        Set wb2 = Workbooks.Open(fd.SelectedItems(i))
        For Each Sht In wb2.Sheets
            Sht.Activate
            ActiveSheet.Copy after:=wb1.Sheets(wb1.Sheets.Count)
        Next Sht
        wb2.Close SaveChanges:=False
    Next i
End If
wb1.Activate
Application.ScreenUpdating = True
Exit Sub
errorhandler:
msgBox Error, vbCritical, "Error"
wb2.Close SaveChanges:=False
Application.ScreenUpdating = True
Application.DisplayAlerts = True
End Sub

First, no need to do :
Sht.Activate and then
ActiveSheet.Copy after:=wb1.Sheets(wb1.Sheets.Count)

This is heavily slowing the process (avoid Activate and Select as much as possible).

Then you can turn off more (see comments) in the Application to speed up your code :

Dim CalcMode As Long
With Application
    .ScreenUpdating = False
    .DisplayAlerts = False
    'Turn off events (like Workbook_Open, Worksheet_Change, ...)
    .EnableEvents = False
    'Turn off the Status Bar
    .DisplayStatusBar = False
    'Store initial calculation mode
    CalcMode = .Calculation
    'And set it to manual
    .Calculation = xlCalculationManual
End With 'Application

I've amended a bit your error handler to go back to the part where we reset all those parameters for a regular use.

Opening your workbooks as read-only (as you don't modify them), should improve the speed a bit.

Finally, the use of With/End With allow the code to know which object you are using for multiple rows and will provide a better/faster access to it.

So all of this will speed up your code, but copying sheets is a bit ressources-demanding so it'll probably still be a long, especially if you are opening big files!

Sub SKR_Import()
On Error GoTo ErrorHandler

    Dim CalcMode As Long
    With Application
        .ScreenUpdating = False
        .DisplayAlerts = False
        .EnableEvents = False
        .DisplayStatusBar = False
        CalcMode = .Calculation
        .Calculation = xlCalculationManual
    End With 'Application

    Dim wB1 As Workbook
    Dim wB2 As Workbook
    Dim Sht As Worksheet
    Dim fD As FileDialog
    Dim FileChosen As Integer
    Dim FileName As String
    Dim i As Long

    Set fD = Application.FileDialog(msoFileDialogOpen)
    With fD
        .AllowMultiSelect = True
        .Title = "Select Excel workbooks to import all sheets"
        FileChosen = .Show
    End With

    Set wB1 = ActiveWorkbook
    With wB1
        If FileChosen = -1 Then
            For i = 1 To fD.SelectedItems.Count
                'Open as read-only
                Set wB2 = Workbooks.Open(fD.SelectedItems(i), , True)
                For Each Sht In wB2.Sheets
                    Sht.Copy after:=.Sheets(.Sheets.Count)
                Next Sht
                wB2.Close SaveChanges:=False
            Next i
        End If
    End With 'wB1

ProperExit:
On Error GoTo 0
    wB1.Activate
    With Application
        .ScreenUpdating = True
        .DisplayAlerts = True
        .EnableEvents = True
        .DisplayStatusBar = True
        .Calculation = CalcMode
    End With 'Application
Exit Sub
ErrorHandler:
    MsgBox Err.Number & vbCrLf & Err.Description, vbCritical + vbOKOnly, "Error"
    wB2.Close SaveChanges:=False
    Resume ProperExit
End Sub


How can I refactor Api::SessionsController? I have session controller for mobile devices. I would like to refactor it a little, for example because my session controller is sub class of Devise::SessionsController I had to duplicate error methods from BaseController.

class Api::V1::SessionsController < Devise::SessionsController
  include Devise::Controllers::Helpers
  prepend_before_filter :require_no_authentication, only: [:create]
  skip_load_and_authorize_resource
  before_filter :ensure_params_exist, only: [:create]

  respond_to :json

  def create
    if user_find params[:user][:member_id]
      self.resource = warden.authenticate!(auth_options)
      sign_in(resource_name, resource)
      resource.reset_authentication_token!
      resource.save!
    else
      error :not_found
    end
  end

  def destroy
    sign_out(resource_name)
  end

  private

  def user_find(id)
    User.find_by member_id: id
  end

  def ensure_params_exist
    unless params[:user]
      error :bad_request
    end
  end

  def error type, context=nil
    render json: { message: error_message(type, context) }, status: type
  end

  def error_message type, context
    scope = [:api, :error, self.controller_name.to_sym]
    scope.push context if context
    t(type, scope: scope)
  end

end

class Api::V1::BaseController < ApplicationController
  load_and_authorize_resource

  respond_to :json

  rescue_from CanCan::AccessDenied do |exception|
    render_status :unauthorized
  end

  private

  def error type, context=nil
    render json: {message: error_message(type, context)}, status: type
  end

  def error_message type, context
    scope = [:api, :error, self.controller_name.to_sym]
    scope.push context if context
    t(type, scope: scope)
  end

  def render_status state
    render status: state, json: {}
  end
end

Put the common error methods in a Module and include it in each Controller where you need them (in a similar way like you already include Devise::Controllers::Helpers, but this time with your own namespace and module name.)

As for the method signatures

def error type, context=nil
def error_message type, context
def render_status state

it's common to leave the parenthesis out when calling class methods in class context (because they oftentimes make up a nice DSL), but please don't do it when defining methods, that can lead to annoying ambiguities.


Count line of code for a JavaScript project Here is my bash script that I just wrote to count line of code for JavaScript project.

It will list number of:

Comment lines
Blank lines
All lines

Here is my script:

#!/bin/bash

fileExt="*.js"

allFiles=$(find ./ -name "$fileExt")

commentLines=$((perl -ne 'print "$1\n" while /(^\s+\/\/\s*\w+)/sg' $allFiles;\
perl -0777 -ne 'print "$1\n" while /(\*\*.*?\*\/)/sg' $allFiles) | wc -l)

blankLines=$(grep '^[[:space:]]*//' -r --include $fileExt | wc -l)

allLines=$(echo $allFiles | xargs wc -l | tail -n 1 | cut -d " " -f 2)

echo -e "\nTotal comments line is: $commentLines.\n\
Total blank lines is: $blankLines.\n\
\nTotal all lines is: $allLines."

Let me explain it a little bit:

First, we need to list all files that end with ".js" within the project:

allFiles=$(find ./ -name "$fileExt")

Second, we count all comment lines:

commentLines=$((perl -ne 'print "$1\n" while /(^\s+\/\/\s*\w+)/sg' $allFiles;\
perl -0777 -ne 'print "$1\n" while /(\*\*.*?\*\/)/sg' $allFiles) | wc -l)

There are 2 types of comment lines in JavaScript:

Line start with only space and // or only //

Example:

//this is a comment line

// this is a comment line
    // this also is a comment line

All above are comment lines, and we got 3 lines total here. But the following is not comment lines:

function foo(params 1) { // return void

}

The line contains // return void is not considered as a comment, so we do not need to count it.

And for the kind of comment line. I using the regex with perl to print all comment lines that match with regex:

/(^\s+\/\/\s*\w+)/sg

Multi-line comment (JSDOC):

Example:

/**
 * return something
 * @param {object} obj
 * return void
*/

So we need to count all number of lines that start from the line with /** and end with */, we have 5 lines here.

I use this regex to match:

perl -0777 -ne 'print "$1\n" while /(\*\*.*?\*\/)/sg' $allFiles

So the total number of comment lines is the sum of two types comment lines above.

Third

We need to count blank lines.

I use the following command to match:

grep '^[[:space:]]*//' -r --include $fileExt | wc -l

Finally

We need to count all lines:

echo $allFiles | xargs wc -l | tail -n 1 | cut -d " " -f 2

I wonder if my solution is good enough or not.

fileExt="*.js"
allFiles=$(find ./ -name $fileExt)

This is a bug.  The wildcard in $fileExt will be expanded by the shell, and cause a syntax error when the current directory has more than one matching file in it:

$ touch a.js b.js
$ fileExt="*.js"
$ find ./ -name $fileExt
find: paths must precede expression: `b.js'
find: possible unquoted pattern after predicate `-name'?

You need to quote the variable.  Better yet, use the globstar option to populate an array and eliminate find altogether:

fileExt="js"
shopt -s globstar
declare -a allFiles=( **/*.$fileExt )
grep … "${allFiles[@]}" # to use the array

This regex does not match blank lines:

  grep '^[[:space:]]*//'

Just /* is enough to start a multi-line comment in JavaScript (not /**).

The script reads every file four times.  That is slow.  Since you're using Perl already, just let it count everything.  Then you don't need to capture the file names at all, since they are used only once:

#!/bin/bash
shopt -s globstar
exec perl -nle '
    BEGIN {
        @ARGV=grep -f, @ARGV or die "no matching files\n";
        $comment = $blank = 0;
    }
    if    ( m{ ^\s*/\* }x .. m{ \*/ }x   or   m{ ^\s*// }x ) { $comment++ }
    elsif ( m{ ^\s*$ }x ) { $blank++ }
    END { print "$comment comment lines; $blank blank lines; $. total lines" }
' **/*.js

At this point we're barely using bash anymore, and the globbing can be moved inside the Perl script, using the File::Find module:

#!/usr/bin/perl -wnl
BEGIN {
  use strict;
  use File::Find;
  my $ext="js";
  my @dirs=( @ARGV ? @ARGV : '.' );
  @ARGV=();
  find(sub { -f and /\.$ext$/o and push @ARGV, $File::Find::name }, @dirs );
  our ($comment, $blank) = (0, 0);
}

if    ( m{ ^\s*/\* }x .. m{ \*/ }x   or   m{ ^\s*// }x ) { $comment++ }
elsif ( m{ ^\s*$ }x ) { $blank++ }

END {
  print "$comment comment lines; $blank blank lines; $. total lines";
}


Apply arbitrary action during recursive generator function I've got this recursive generator function. It will traverse an object looking for "leafs", any keys that don't point to sub-objects.

Then it applies one of two actions on the leaf and yields it.
Any suggestions for improvement would be appreciated. :)

const obj = {
  a: 'a',
  b: 'b',
  c: {
    d: 'd',
    e: 'e',
  }
};

const clone = toClone => JSON.parse(JSON.stringify(toClone))

function* leafIterator(object, action) {
  for (const key of Object.keys(object)) {
    if (typeof object[key] === 'object') {
      for (const inner of leafIterator(object[key], action)) {
        let oclone = clone(object);
        oclone[key] = inner;
        yield oclone;
      }
    }
    else {
      let oclone = clone(object);
      action(oclone, key);
      yield oclone;
    }
  }
}

for (let o of leafIterator(obj, (s, key) => delete s[key])) {
  console.log('result', o);
}

Good things:

all lines terminated with a semi-comon
strict equality used when checking for objects

I would suggest using const by default since none of the variables/functions get reassigned. If you determine reassignment is necessary then use let. This will help avoid accidental re-assignment.

And is the variable naming oclone Hungarian notation? If so, why not use it for all variables instead of cloned variables?


PreserveIds using inheritance id db auto incrementing I am working on this project where I like to PreserveIds once inserted into the database. The id are auto incrementing and I return them once the data in inserted. I like to know if there is a better way using inheritance vs using a for each loop and checking if the value is 0?

 class Program
    {
        static void Main(string[] args)
        {
            var States = new List<StatesModel>();
            var CaliforniaCity = new List<CityModel>();
            CaliforniaCity.Add(new CityModel() { Id = 2, Name = "Los Angeles" });
            CaliforniaCity.Add(new CityModel() { Id = 3, Name = "Sacramento" });
            CaliforniaCity.Add(new CityModel() { Id = 4, Name = "San Francisco" });
            States.Add(new StatesModel() { Id = 1, Name = "California", CityModels = CaliforniaCity });

            var PhiladelphiaCity = new List<CityModel>();
            PhiladelphiaCity.Add(new CityModel() { Id = 5, Name = "Philadelphia" });
            PhiladelphiaCity.Add(new CityModel() { Id = 6, Name = "Pittsburgh" });
            PhiladelphiaCity.Add(new CityModel() { Id = 7, Name = "Harrisburg" });
            States.Add(new StatesModel() { Id = 8, Name = "Pennsylvania", CityModels = PhiladelphiaCity });

            var MichiganCity = new List<CityModel>();
            MichiganCity.Add(new CityModel() { Id = 9, Name = "Detroit" });
            MichiganCity.Add(new CityModel() { Id = 10, Name = "Lansing" });
            MichiganCity.Add(new CityModel() { Id = 11, Name = "Grand Rapids" });
            States.Add(new StatesModel() { Id = 12, Name = "Michigan", CityModels = MichiganCity });

///Insert Data in database

            PreserveIds(States)

///Update data in database

            foreach (var State in States)
            {
                Console.WriteLine(string.Format("{0}", State.Name));
                Console.WriteLine(string.Format("{0} = {1}", State.Id ,State.OriginaId));
                foreach (var city in State.CityModels)
                {
                    Console.WriteLine(string.Format("{0}", city.Name));
                    Console.WriteLine(string.Format("{0} = {1}", city.Id, city.OriginaId));
                }
            }

        }

        public static void PreserveIds(IEnumerable<StatesModel> States)
        {
            if (States != null)
            {
                foreach (var State in States)
                {
                    if (State.OriginaId == 0)
                    {
                        State.OriginaId = State.Id;
                    }

                    foreach (var city in State.CityModels)
                    {
                        if (city.OriginaId == 0)
                        {
                            city.OriginaId = city.Id;
                        }
                    }
                }
            }
        }
    }

Model

    public class StatesModel
    {
        public int Id { get; set; }
        public int OriginaId { get; set; }
        public string Name { get; set; }
        public List<CityModel> CityModels { get; set; }
        public StatesModel()
        {
            CityModels = new List<CityModel>();
        }
    }

    public class CityModel
    {
        public int Id { get; set; }
        public int OriginaId { get; set; }
        public string Name { get; set; }
    }

I would change implementation if Id property in both your models into:

public class StatesModel
{
    private int id;

    public int Id
    {
        get
        {
            return id;
        }
        set
        {
            id = value;

            if (OriginalId == 0)
                OriginalId = value;
        }
    }

    public int OriginalId { get; private set; }

    // (...)
}

// The same for CityModel

and remove your PreserveIds method all together. You can also add private setter to OriginalId just to make sure that nobody would override this value.


Data formatting, splitting objects & deleting key's/values I am quite bad at data manipulation and after a lot of time I was finally able to get it working how I wanted. The thing is I don't like all the delete operations, I think the code can be a lot cleaner and the runtime seems somewhat slow on a lot of data. Code readability is very important here but it needs to be small, contained and fast. ES6+ i welcome and of course so are tips and general information.

Input Data:

{
  agency: "",
  agent: "",
  agent_email: "",
  agent_phone: "",
  date_of_birth: "2007-05-30",
  delivery_address: "address - 1",
  delivery_city: "city - 1",
  delivery_country: "PE",
  delivery_state: "state - 1",
  delivery_zipcode: "zip_code - 1",
  facebook: "",
  instagram: "",
  name: "Dustin Peter",
  primary_email: "wagnerpeter@bailey-schroeder.example.com",
  primary_phone_number: "+31622064234",
  residential_address: "address - 0",
  residential_city: "city - 0",
  residential_country: "LC",
  residential_state: "state - 0",
  residential_zipcode: "zip_code - 0",
  secondary_email: "garzajustin@vasquez.example.com",
  secondary_phone_number: "+39349330249",
  shoe_size: "38"
  snapchat: "",
  twitter: "",
  url: "",
  youtube: "",
}

End Result:

Data Set 1:

{
  name: "Dustin Peter",
  primary_email: "wagnerpeter@bailey-schroeder.example.com",
  primary_phone_number: "+31622064234",
}

Data Set 2:

{
  date_of_birth: "2007-05-30",
  delivery_address: {
    address: "address - 1",
    city: "city - 1",
    country: "PE",
    state: "state - 1",
    zip_code: "zip_code - 1",
  }
  residential_address: {
    address: "address - 0",
    city: "city - 0",
    country: "LC",
    state: "state - 0",
    zip_code: "zip_code - 0",
  }
  secondary_email: "garzajustin@vasquez.example.com",
  secondary_phone_number: "+39349330249",
  shoe_size: "38",
}

My Working Code:

const dataSet1 = {
  name: data.name,
  primary_email: data.primary_email,
  primary_phone_number: data.primary_phone_number,
};

const dataSet2 = {};
Object.keys(data).forEach((prop) => {
    if (data[prop]) { dataSet2[prop] = data[prop]; }
});

dataSet2.residential_address = {
    address: dataSet2.residential_address,
    city: dataSet2.residential_city,
    country: dataSet2.residential_country,
    state: dataSet2.residential_state,
    zip_code: dataSet2.residential_zipcode,
};

dataSet2.delivery_address = {
    address: dataSet2.delivery_address,
    city: dataSet2.delivery_city,
    country: dataSet2.delivery_country,
    state: dataSet2.delivery_state,
    zip_code: dataSet2.delivery_zipcode,
};

delete dataSet2.name;
delete dataSet2.primary_email;
delete dataSet2.primary_phone_number;
delete dataSet2.residential_city;
delete dataSet2.residential_country;
delete dataSet2.residential_state;
delete dataSet2.residential_zip_code;
delete dataSet2.delivery_city;
delete dataSet2.delivery_country;
delete dataSet2.delivery_state;
delete dataSet2.delivery_zip_code;

You can use Object.entries() and .filter() with RegExp /date|delivery|residential|secondary|shoe/ and .test() to filter data from data, create a function expecting property name and optional separator character to concatenate to property name to replace in returned object spread as values passed to second parameter of Object.assign()

const data = {
  agency: "",
  agent: "",
  agent_email: "",
  agent_phone: "",
  date_of_birth: "2007-05-30",
  delivery_address: "address - 1",
  delivery_city: "city - 1",
  delivery_country: "PE",
  delivery_state: "state - 1",
  delivery_zipcode: "zip_code - 1",
  facebook: "",
  instagram: "",
  name: "Dustin Peter",
  primary_email: "wagnerpeter@bailey-schroeder.example.com",
  primary_phone_number: "+31622064234",
  residential_address: "address - 0",
  residential_city: "city - 0",
  residential_country: "LC",
  residential_state: "state - 0",
  residential_zipcode: "zip_code - 0",
  secondary_email: "garzajustin@vasquez.example.com",
  secondary_phone_number: "+39349330249",
  shoe_size: "38",
  snapchat: "",
  twitter: "",
  url: "",
  youtube: ""
}

let [filters, filtered] = [
  ['date', 'delivery', 'residential', 'secondary', 'shoe']
, (data, props) => Object.entries(data)
  .filter(([key, value]) =>
    new RegExp(props.join`|`)
    .test(key))
];

const handleProps = (prop, r = '', props = filtered(data, filters)) =>
  ([...p] = props.filter(([key, value]) =>
    new RegExp(prop).test(key)).map(([key, value]) =>
    (r ? {[key.replace(new RegExp(prop + r), '')]: value} : value))
    , r ? p : p.pop());

const dataSet2 = {
 date_of_birth:handleProps('date_of_birth')
, delivery_address: Object.assign({}, ...handleProps('delivery', '_'))
, residential_address: Object.assign({}, ...handleProps('residential', '_'))
, secondary_email: handleProps('secondary_email')
, secondary_phone_number: handleProps('secondary_phone_number')
, shoe_size: handleProps('shoe_size')
};

console.log(dataSet2);


Text-based terrain generator (Part 2) I've updated my text-based terrain generator a lot. Even though I haven't worked on it all that much. I really just want to know, like usual, is there anything I can improve? How can I shorten the code? What can be made more efficient? Yada yada yada...

#!/usr/bin/env python
from random import choice
from os import system, name

class Tiles(object):
    tree = '\033[7;1;32m  \033[0m'
    land = '\033[7;32m  \033[0m'
    sand = '\033[7;1;33m  \033[0m'
    water = '\033[7;1;34m  \033[0m'

class Sizes(object):
    small = 10
    med1 = 20
    med2 = 30
    large = 40

sizes = (
    Sizes.small, Sizes.med1, Sizes.med2, Sizes.large)

tiles = (
    Tiles.land, Tiles.sand, Tiles.tree, Tiles.water)

def generate(world_size):
    system('cls' if name == 'nt' else 'clear')
       for _ in range(world_size):
            print(''.join(choice(tiles) for _ in range(world_size)))

if __name__ == "__main__":
    generate(choice(sizes))

Also, this is python 2.7 I just prefer to use print() over print.

Your terran is randomly generated, you may use a "transition" map like this:

transitions = {Tiles.land: [Tiles.sand] +
               [Tiles.tree] * 2 +
               [Tiles.land] * 5,
               Tiles.sand: [Tiles.water, Tiles.sand],
               Tiles.tree: [Tiles.tree, Tiles.land],
               Tiles.water: [Tiles.water] * 10
               }

It read "When you're near a land, you have a chance to peek sand, two to pick tree, and 5 to pick another land".

Then you may want to code something like this to pick a tile according to the neighbors:

def pick_tile_for(world, x, y):
    surrounding = []
    if x > 0:
        surrounding.append(world[x - 1][y])
    if y < len(world) - 1:
        surrounding.append(world[x][y + 1])
    if y > 0:
        surrounding.append(world[x][y - 1])
    if x < len(world) - 1:
        surrounding.append(world[x + 1][y])
    surrounding = [tile for tile in surrounding if tile is not None]
    if len(surrounding) == 0:
        return None
    excluded = set(surrounding[0])
    for tile in surrounding:
        excluded = excluded - set(transitions[tile])
    next = list(chain(*[[t for t in transitions[tile] if t not in excluded]
                        for tile in surrounding]))
    return choice(next)

Finally you may want to use this "not so random" tile picker:

def generate(world_size):
    system('cls' if name == 'nt' else 'clear')
    world = []
    for x in range(world_size):
        world.append([None for _ in range(world_size)])
    world[choice(range(world_size))][choice(range(world_size))] = Tiles.land
    for x in range(world_size):
        for y in range(world_size):
            if world[x][y] is None:
                world[x][y] = pick_tile_for(world, x, y)
    for x in range(world_size - 1, -1, -1):
        for y in range(world_size - 1, -1, -1):
            if world[x][y] is None:
                world[x][y] = pick_tile_for(world, x, y)
    for line in world:
        print ''.join(line)

Note that I'm iterating the world twice, one in reverse, because my "first land" is randomly placed, I do not start from a corner, the world is built around the first randomly placed land.

I think there may be a lot of better ways to do this, but it was fun to do. You also may try to read about "flood fill".

Ceveat: I'll never build rivers with this kind of algorithm.


This script works but keeps timing out with longer inputs. How can I improve its speed? [Ruby] The script takes a string such as this: (15, 176) (65, 97) (72, 43) (102, 6) (191, 189) (90, 163) (44, 168) (39, 47) (123, 37) and breaks it into an array of arrays. It then computes the largest possible number of choices, with the guideline that each selected array has to have both numbers smaller than the previous selection. For the above string, the right output would be 4.

people = File.read(ARGV[0])
people = people.sub("\(", '').sub(/\)$/, '').split("\) \(")
people.map! { |p| p.split(', ') }

count = 0
test = people.permutation.to_a
test.each do |perm|
  test_count = 0
  perm.each_with_index do |item, i|
    test_case = perm[i+1]
    unless test_case == nil
    test_count += 1 if item[0] > test_case[0] && item[1] > test_case[1]
    end
  count = [count, test_count].max
  end
end

p count

Anyways, it works but is sloooow, especially with longer strings. How can I speed this up? I know the nested loops can be problematic and as the string grows the number of permutations grows. Any advice would be much appreciated.

Edit: I am running this on Ruby 1.8.7.

Edit: New attempt at the problem/script:

people = '(15, 176) (65, 97) (72, 43) (102, 6) (191, 189) (90, 163) (44, 168) (39, 47) (123, 37)'
#people = File.read(ARGV[0]).chomp
people = people.sub("\(", '').sub(/\)$/, '').split("\) \(")
people.map! { |p| p.split(', ').map {|a| a.to_i } }

ht = people.sort_by { |h| h[0] }
wt = people.sort_by { |w| w[1] }
a_i = []

ht.each do |item|
  a_i << wt.index(item)
end

def subgen( array, count=1, t=(array.length+1) )
  if array.length == 1
    return count
  end
  without = subgen(array[0..-2], count, t)
  if array[0] > array[-1] || array[-1] > t
    return without
  else
    with = subgen(array[0..-2], count+=1, t = array[-1])
    return [with, without].max
  end
end

current = 0
a_i.each_with_index do |item, j|
  test = a_i[j..-1]
  current = [current, subgen(test)].max
end

p current

This has been thoroughly tested and works, but there has to be a way to make it faster. Any ideas?

So, you are generating every possible permutations of the array, which is a O(n!) operation. This is huge.

I think this can be done with an O(n²) algorithm. I would do :

create an array containing the pairs, ordered by the first value
create another one containing the pairs, ordered by the second value
create a third array containing, for each index of the first array, the index in the second array where we can find the corresponding value.
find every subset of this third array whose values are in ascending order.

The result is the length of the biggest subset.

Example

first array | second array | third array
------------+--------------+------------
15,176      | 102,6        | 7
39,47       | 123,37       | 3
44,168      | 72,43        | 6
65,97       | 39,47        | 4
72,43       | 65,97        | 2
90,163      | 90,163       | 5
102,6       | 44,168       | 0
123,37      | 15,176       | 1
191,189     | 191,189      | 8

The ascending subsets are :

7 8
3 6 8
3 4 5 8
6 8
4 5 8
2 5 8
5 8
0 1 8
1 8
8

The biggest subset is (3 4 5 8), and its size is 4.

Complexity

The sorts can be done with O(n log n)
The creation of the third array is O(n²)
To find the subset, I think we can do that with O(n²) (I'm not sure about that)

So, the whole thing should be O(n²).

The algorithm for finding the ascending subsets is not trivial. I'm sure it will be fun to implement !