upload multiple images and resize I've written PHP code to upload multiple images to a server.
This function processes a maximum of 10 images at one time and if any image's width or height is larger than 2000px it will resize it. But the code is a little bit slow.
I want your opinion of this code.
Is there is way to make it more fast and secure?
P.S.: I use an .htaccess file in upload folder to make it more secure.
This is my PHP code:
$img_num=count($_FILES['upload_images']['name']);
if($img_num>0 && $img_num<11){

for($i=0;$i<$img_num;$i++){
if($_FILES['upload_images']['name'][$i]!=''){
$ext=explode(".",$_FILES['upload_images']['name'][$i]);
$ext=strtolower($ext[1]);

if ((($_FILES['upload_images']['type'][$i] == "image/gif")
|| ($_FILES['upload_images']['type'][$i] == "image/jpeg")
|| ($_FILES['upload_images']['type'][$i] == "image/jpg")
|| ($_FILES['upload_images']['type'][$i] == "image/pjpeg")
|| ($_FILES['upload_images']['type'][$i] == "image/x-png")
|| ($_FILES['upload_images']['type'][$i] == "image/png"))
&& ($_FILES['upload_images']['size'][$i] < 12000000)
&& in_array($ext,array("gif","jpeg","jpg","png"))){

$is_jpg=1;$ex='jpg';

switch ($ext){
case 'jpg':$image=imagecreatefromjpeg($_FILES['upload_images']['tmp_name'][$i]);break;
case 'jpeg':$image=imagecreatefromjpeg($_FILES['upload_images']['tmp_name'][$i]);break;
case 'png':$image=imagecreatefrompng($_FILES['upload_images']['tmp_name'][$i]);if($_FILES['upload_images']['size'][$i] < 2000000){$ex='png';$is_jpg=2;}break;
case 'gif':$image=imagecreatefromgif($_FILES['upload_images']['tmp_name'][$i]);$ex='gif';$is_jpg=3;break;
default:$image=imagecreatefromjpeg($_FILES['upload_images']['tmp_name'][$i]);break;
}

$path='upload/img_'.mb_substr(md5($_FILES['upload_images']['name'][$i].rand(0,50)),0,rand(5,10)).'.'.$ex;

$width=imagesx($image);$height=imagesy($image);
$resize=false;
if($width>2000 || $height>2000){
$width_orig=$width;$height_orig=$height;
$width=2000;$height=2000;
$ratio_orig=$width_orig/$height_orig;
($width/$height > $ratio_orig)?$width=$height*$ratio_orig:$height=$width/$ratio_orig;
$resize=true;
}

if($is_jpg==1){
if($resize){
$bg=imagecreatetruecolor($width, $height);
imagecopyresampled($bg, $image, 0, 0, 0, 0,$width, $height,$width_orig, $height_orig);
imagejpeg($bg,$path,75);
imagedestroy($bg);
}
else{imagejpeg($image,$path,80);}
imagedestroy($image);
$img_url[]=$ex;
}

elseif($is_jpg==2){
$bg = imagecreatetruecolor($width, $height);
imagesavealpha($bg, TRUE);
imagefill($bg, 0, 0, imagecolorallocatealpha($bg, 0, 0, 0,127));
$resize?imagecopyresampled($bg, $image, 0, 0, 0, 0,$width, $height,$width_orig, $height_orig):imagecopy($bg, $image, 0, 0, 0, 0,$width, $height);
imagedestroy($image);
imagepng($bg,$path,8);
imagedestroy($bg);
}

elseif($is_jpg==3){
move_uploaded_file($_FILES['upload_images']['tmp_name'][$i], $path);
}

}
}
}

}

and this the .htaccess file:
IndexIgnore *

deny from all
<Files ~ "^(.*)+\.(gif|jpg|png|jpeg|svg|mp4)$">
order deny,allow
allow from all
</Files>

Options -Indexes -ExecCGI
RemoveHandler .php .phtml .php3
RemoveType .php .phtml .php3
php_flag engine off

ForceType application/octet-stream
<FilesMatch "(?i).jpe?g$">
    ForceType image/jpeg
</FilesMatch>
<FilesMatch "(?i).gif$">
    ForceType image/gif
</FilesMatch>
<FilesMatch "(?i).png$">
    ForceType image/png
</FilesMatch>
<FilesMatch "(?i)\.mp4$">
    ForceType video/mp4
</FilesMatch>

Formatting
I'd recommend looking at the accepted PSRs - e.g. PSR-1, PSR-12. Updating the code to comply with them will help make the code readable and consistent with idiomatic PHP code.
Maybe this is just a copy-paste issue, but even if you don't strictly follow PSR-12 please indent nested lines within blocks.
Array syntax
As of the time of writing, PHP 7 has Active support for versions 7.4 and 7.3, and since PHP 5.4 arrays can be declared with short array syntax (PHP 5.4) - i.e. ["gif","jpeg","jpg","png"].
Redundant lines
If you aren't familiar with the principle Don't repeat yourself I recommend reading about it:

The Don’t Repeat Yourself (DRY) principle states that duplication in logic should be eliminated via abstraction;

Let's look at a few place that are redundant:

if ((($_FILES['upload_images']['type'][$i] == "image/gif")
|| ($_FILES['upload_images']['type'][$i] == "image/jpeg")
|| ($_FILES['upload_images']['type'][$i] == "image/jpg")
|| ($_FILES['upload_images']['type'][$i] == "image/pjpeg")
|| ($_FILES['upload_images']['type'][$i] == "image/x-png")
|| ($_FILES['upload_images']['type'][$i] == "image/png"))
&& ($_FILES['upload_images']['size'][$i] < 12000000)
&& in_array($ext,array("gif","jpeg","jpg","png"))){

The last conditional uses in_array() - why not use that for checking the type? That way there is no need to repeat $_FILES['upload_images']['type'][$i] for each type:
if (in_array($_FILES['upload_images']['type'][$i],
     ["image/gif", "image/jpeg", "image/jpg", "image/pjpeg", "image/x-png", "image/png"])
    && ($_FILES['upload_images']['size'][$i] < 12000000)
    && in_array($ext, ["gif","jpeg","jpg","png"])) {

Also the switch statement has three cases that do the same thing: 'jpg', 'jpeg' and default - so those first to can be eliminated.
Tri-state variable $is_jpg
This variable has three possible values, yet has a name that might be conceived as a boolean. And somebody reading the code might not know what the values 1, 2 and 3 signify without scanning through the code to see where those values are assigned. I'd suggest eliminating it and instead of checking to see if the value is 1, 2, or 3, check to see if $ex is 'jpg', 'png', or 'gif' since those values are already being set.


Creating a dictionary I have a Ruby programming task. My code passes all the rspec requirements, but I am not sure if I've done it in the right way. And I'm not quite sure why I need :entries there...

Here is the rspec test:

# # Topics
#
# * Hash
# * Array
# * instance variables
# * regular expressions

require '11_dictionary'

describe Dictionary do
  before do
    @d = Dictionary.new
  end

  it "is empty when created" do
    expect(@d.entries).to eq({})
  end

  it "can add whole entries with keyword and definition" do
    @d.add("fish" => "aquatic animal")
    expect(@d.entries).to eq({"fish" => "aquatic animal"})
    expect(@d.keywords).to eq(["fish"])
  end

  it "add keywords (without definition)" do
    @d.add("fish")
    expect(@d.entries).to eq({"fish" => nil})
    expect(@d.keywords).to eq(["fish"])
  end

  it "can check whether a given keyword exists" do
    expect(@d.include?("fish")).to be_falsey
  end

  it "doesn't cheat when checking whether a given keyword exists" do
    expect(@d.include?("fish")).to be_falsey # if the method is empty, this test passes with nil returned
    @d.add("fish")
    expect(@d.include?("fish")).to be_truthy # confirms that it actually checks
    expect(@d.include?("bird")).to be_falsey # confirms not always returning true after add
  end

  it "doesn't include a prefix that wasn't added as a word in and of itself" do
    @d.add("fish")
    expect(@d.include?("fi")).to be_falsey
  end

  it "doesn't find a word in empty dictionary" do
    expect(@d.find("fi")).to be_empty # {}
  end

  it "finds nothing if the prefix matches nothing" do
    @d.add("fiend")
    @d.add("great")
    expect(@d.find("nothing")).to be_empty
  end

  it "finds an entry" do
    @d.add("fish" => "aquatic animal")
    expect(@d.find("fish")).to eq({"fish" => "aquatic animal"})
  end

  it "finds multiple matches from a prefix and returns the entire entry (keyword + definition)" do
    @d.add("fish" => "aquatic animal")
    @d.add("fiend" => "wicked person")
    @d.add("great" => "remarkable")
    expect(@d.find("fi")).to eq({"fish" => "aquatic animal", "fiend" => "wicked person"})
  end

  it "lists keywords alphabetically" do
    @d.add("zebra" => "African land animal with stripes")
    @d.add("fish" => "aquatic animal")
    @d.add("apple" => "fruit")
    expect(@d.keywords).to eq(%w(apple fish zebra))
  end

  it "can produce printable output like so: [keyword] 'definition'" do
    @d.add("zebra" => "African land animal with stripes")
    @d.add("fish" => "aquatic animal")
    @d.add("apple" => "fruit")
    expect(@d.printable).to eq(%Q{[apple] "fruit"\n[fish] "aquatic animal"\n[zebra] "African land animal with stripes"})
  end
end

Here is my code:

class Dictionary
    attr_accessor :entries

    def initialize
        @hash={}
    end

    def add(a)
        if a.class == Hash
            a.each_pair { |k, v|
                @hash[k]=v
            }
        else
           @hash[a]=nil
       end
    end
    def entries
        @hash
    end

    def keywords
        @hash.keys.sort
    end
    def include?(k)
        @hash.key?(k)
    end

    def printable
        str=[]
        key=@hash.keys.sort
        key.each do |el|
            @hash.each_pair do |k,v|
                if k==el
                    str << "[#{k}] \"#{v}\""
                end
            end
        end
        return str.join("\n")
    end

    def find(str)
        if str.class == Hash
            str.each_pair { |k, v|
               return (@hash.select { |k,v| k.include?(k)})
            }
        elsif str.class == String
            return (@hash.select {|k,v| k.include?(str)})
        end
    end
end

class Dictionary
    attr_accessor :entries

Indentation in Ruby is two spaces, not four. You have that correct in your specs, but not in the application code.

Also, you create here a pair of methods, entries= which you never use anywhere, and entries, which you overwrite further down, anyway. Either get rid of this line, or (preferred!) actually make use of those methods.

    def initialize
        @hash={}

There should be a space on both sides of the = sign. This applies to operators in general.

[Note: there are some style guides that advocate not using spaces around the = sign for optional parameters with default arguments, but a) this is not universally accepted and b) not the case here anyway.]

    end

    def add(a)

This should probably be called << instead.

[Note: I realize, this is prescribed by the spec. IMO, the spec should be changed.]

        if a.class == Hash

You should never compare the class of an object like this. Ideally, you should use runtime ad-hoc polymorphism. If that isn't possible, at least use Object#is_a?, Object#kind_of?, or Module#===.

            a.each_pair { |k, v|
                @hash[k]=v
            }

There are two different camps when it comes to block styles: one camp says to use {/} for one-liners and do/end for multiple lines. The other camp says to use {/} for "functional" blocks and do/end for side-effecting blocks.

Regardless of which camp you follow: your block has three lines and side-effects, so it should use the keyword form.

        else
           @hash[a]=nil
       end
    end

There should be an empty between the two method definitions.

    def entries
        @hash
    end

    def keywords
        @hash.keys.sort
    end
    def include?(k)
        @hash.key?(k)
    end

    def printable

This should be called to_s. to_s is the standard name for a method that converts an object into a textual representation. It is used automatically by methods such as Array#join or Kernel#puts.

[Note: I realize, this is prescribed by the spec. IMO, the spec should be changed.]

        str=[]
        key=@hash.keys.sort
        key.each do |el|
            @hash.each_pair do |k,v|
                if k==el
                    str << "[#{k}] \"#{v}\""

You could save yourself some hassle here by using something else than " to delimit the string.

This pattern, where you initialize an accumulator to an "empty" value and then iterate over a collection while appending to the accumulator at every iteration, is called a Catamorphism, fold, or reduce. In Ruby, it is known as inject.

However, in this case, it can also be done even simpler, with a map.

                end
            end
        end
        return str.join("\n")

The return is superfluous here, the return value of a method is the value of the last expression which is evaluated inside the method body.

    end

    def find(str)
        if str.class == Hash
            str.each_pair { |k, v|
               return (@hash.select { |k,v| k.include?(k)})
            }

There is nothing in the spec about passing Hashes to find. In fact, you named your parameter str clearly indicating that you expect it to be a String. Then why even bother checking for Hashes?

        elsif str.class == String
            return (@hash.select {|k,v| k.include?(str)})

Here you check whether the key includes the search string. However, from the documentation in the spec, it appears that the search string should be a prefix of the key. Unfortunately, there is actually no test in the spec to test for that distinction.

        end
    end
end

This is what it would look like with those suggestions applied:

class Dictionary
  attr_reader :entries

  def initialize
    self.entries = {}
  end

  def add(a)
    case a
    when Hash then entries.merge!(a)
    else           entries[a] = nil
    end
  end

  def keywords
    entries.keys.sort
  end

  def include?(k)
    entries.key?(k)
  end

  def printable
    entries.
      sort.
      map {|k, v| ["[#{k}]", unless v.nil? then %Q["#{v}"] end] }.
      map(&:compact).
      map {|kvp| kvp.join(' ') }.
      join("\n")
  end

  def find(key)
    entries.select {|k, v| k.start_with?(key) }
  end

  private

  attr_writer :entries
end


Simple random number guessing game with various difficulties I have been studying C#, and I am trying to model the Random number guessing game.

1 User inputs the number of try.
2 Computer generates random number.
3 If numbers doesn't match user lose one try
and if they match user gets two bonus try.

Is my program reasonable or is there anything I need to improve?

As player goes to next round, I also want
to increase random number range by 100 to increase
difficulty.For example, first round 1-100
next round 1-200 and so on.
How can I do that? Any help would be appreciated.

 public class GuessGame
 {

    public class Game
    {
        public int attemp { set; get; } = 0;
        public override string ToString()
        {
           return $"The player's attemp <{attemp}>";
        }
    }
    public static void Test()
    {
        Console.WriteLine("Lets play the Low or High Game");
        var game = new Game();

        game.attemp = InputAttempt();
        PlayGame(game);
    }
    public static void PlayGame(Game game)
    {
        Console.WriteLine(game);
        var comNum = GenerateComNum();
            Console.WriteLine($"This is for test: ComNum is {comNum}");
            do
            {
                var userNum = GenerateUserNum();
                CompareNum(userNum, comNum, game);
                Console.WriteLine(game);

            } while (game.attemp >0 );
     }

    static void CompareNum(int userNum, int comNum,Game game)
    {

        if (userNum < comNum)
        {
            Console.WriteLine("User num is too Low try again");
            Console.WriteLine();
            game.attemp--;
        }
        else if(userNum > comNum)
        {
            Console.WriteLine("User num is too High try again");
            Console.WriteLine();
            game.attemp--;
        }
        else if(userNum == comNum)
        {
            Console.WriteLine("You got the answer!!");
            Console.WriteLine();
            game.attemp += 2;
            PlayGame(game);
        }

    }
    static int GenerateUserNum()
    {
        int userNum;
        Console.Write("Please enter the num btw 1-100\t\t");
        while (!Int32.TryParse(Console.ReadLine(), out userNum))
        {
            Console.WriteLine("Invalid num please input whole num");
        }
        return userNum;
    }
    static int GenerateComNum()
    {
        Random rnd = new Random();
        var comNum = rnd.Next(1,101);
        return comNum;
    }
    static int InputAttempt()
    {
        var attempt = 0;
        Console.Write("How many Attemps would you like?\t");

        while (!Int32.TryParse(Console.ReadLine(), out attempt))
            {
                Console.WriteLine("Invalid num please input whole num");
            }
        return attempt;
    }
}

1) Avoid using var if you want your code to be more readable.

2) Both CompareNum() and PlayGame() methods should be defined within the Game class as they rely on a game (which means they don't need to have a game as argument anymore)

3) In the CompareNum() method

Console.WriteLine("User num is too Low try again");
Console.WriteLine();

can become

Console.WriteLine("User num is too Low try again\n");

4) In the GenerateComNum() method you can shorten

var comNum = rnd.Next(1, 101);
return comNum;

to

return rnd.Next(1, 101);

and you should avoid creating a new Random() every time you have to generate a random number because it could create unexpected results. You should make a static Random rnd = new Random(); and only use rnd.Next(x, y) whenever you need a random number.

5) In the PlayGame() method you shouldn't have a do ... while loop because if the user chooses to have 0 attempts then he should lose instantly and that's not what will happen with a do ... while loop

6) The usual order for accessors is get then set, you did the opposite : public int attemp { set; get; } = 0; it'll work in both orders, it's just to make things more readable.

7) It's realy unlikely to happen but if the user would win during realy long time, your application could have a System.StackOverflowException because you are calling PlayGame() while it is already running when the user wins which will cause the current one running to wait for the new one to finish and those will stack up and take more and more memory, this is recursion, sorry and check internet if my explanations aren't clear.

To increase the range of possible numbers being generated, you can use a variable to determine what the max generated number is and put this variable as second argument instead of 101 when you do rnd.Next(1, 101);


Takes json input and builds dynamic json output based on values in input: Looking for a better way to accomplish this I have this specific scenario where I'm going to get a JSON that could have multiple Objects. From the Multiple Objects I will create a new Json that can have nested arrays growing by n amount depending on some json object values.

I hoping there is a better way to do this. Maybe my example below is the best way? I took a look at https://realpython.com/python-data-classes/ but I don't think that will help since it seems complicated to turn the dataclass object into another json.

Update: I think I will ingest the original json object into a dataclass. Then iterate over the objects.

Example Input: Note that 3 and 4 are break start and end times. Which is why you will see the start and end times show up in the break array.

[{
    "name": "Jay",
    "clock_type": 1,
    "start": "4am"
}, {
    "name": "Jay",
    "clock_type": 4,
    "start": "5am"
},
{
    "name": "Jay",
    "clock_type": 3,
    "end": "6am"
},
{
    "name": "Jay",
    "clock_type": 4,
    "start": "7am"
},
{
    "name": "Jay",
    "clock_type": 3,
    "end": "8am"
}]

Example output:

{
    "name": "Jay",
    "start": "4am",
    "break": [{
        "start": "5am",
        "end": "6am"
    }, {
        "start": "7am",
        "end": "8am"
    }]
}

What i'm doing right now is looping through the array, and building the punch dynamically based on the "clock_type".

import json

def write_new_json(data):

    request = {}
    break_count = 0

    for i in data:
        # 1 = New Object. First Clock In
        if i['clock_type'] in [1]:
            request.update({"name": i["name"],
                            "start": i["start"]})
        # 3 = Break End
        if i['clock_type'] in [3]:
            request["break"][break_count].update({"end": i["end"]})
            break_count += 1

        # 4 = Break start. If there is already a breaks object, then we do not create a breaks array,
              # we just append to the existing one.
        if i['clock_type'] in [4]:
            if break_count == 0:
                request.update({"break": []})

            request["break"].append({"start": i["start"]})

       # 2 = Not in the example, but a punch of 2 would reset break_count to 0.

    return request

with open('example.json') as f:
    api_request = write_new_json(json.load(f))

print(json.dumps(api_request))

There are a few points in the existing code that I think could be improved.
Confusing function name write_new_json
Naming is important. Good names make your code more readable, and easier to understand. Bad names make it less readable. Names are bad when they don't describe what the thing is/does, or conversely, if they seem to describe something that the thing isn't/doesn't do.
Usually, the verbs "read" and "write" in programming refer to fetching data from, and sending data to, some external/system resource. Especially a file. So the name write_new_json() sounds like you're writing some JSON to a file. But that's not what the function is doing at all. It's actually transforming some JSON into some other kind of JSON. Therefore write_new_json() is a misleading name.
I would pick a new name that avoids write and probably even avoids json; describing the input and output formats of a function as "JSON" does nothing to capture the purpose of the function. How about build_clock_punches? build_time_logs?
Confusing variable name i
    for i in data:

Another naming nitpick. In most programming languages, it is conventional to name your loop counter variable i. In Python, this would be something like for i in range(0, 1000):.1 But in your case, i is not a loop counter; it is not even a number. Therefore the name is confusing. Try using a more descriptive name, e.g. for clock_punch in data: or for clock_event in data:.
Unnecessary use of in operator
        if i['clock_type'] in [3]:

You're using in because [3] is a list, but there doesn't seem to be any reason that [3] needs to be a list. You can just compare the value directly using the equality operator, e.g. if clock_event['clock_type'] == 3:.
Unnecessary variable break_count
As far as I can tell, you only use break_count to keep track of the length of request["break"]. Persistently tracking the length of a list in a separate variable is a bad idea: it is unnecessary, and it opens you up to bugs. Suppose you go in later and add a new line of code that changes the length of request["break"], but forget to add that second line of code that updates break_count? Or vice versa?
To find the length of a list directly, use len().
        if clock_event['clock_type'] == 3:
            last_break_index = len(request["break"]) - 1
            request["break"][last_break_index].update({"end": clock_event["end"]})

But wait! There's an even better approach here. Python offers a handy syntax for getting the last item in a list: my_list[-1].
        if clock_event['clock_type'] == 3:
            request["break"][-1].update({"end": clock_event["end"]})

And for the if break_count == 0: line, it is better to check for the non-existence of request["break"] directly.
        if clock_event['clock_type'] == 4:
            if "break" not in request:
                request.update({"break": []})

Too trusting
Your program assumes the input data will always be well-formed. What happens if it's not? Suppose the system that produces your input goes haywire, and you receive a break-start, and then two break-ends? Or receive a break-end without receiving any break-starts first? Or data with a mix of two or more names?2 Depending on the nature of the nonsense input, your program would either crash entirely, or produce a nonsense output of its own.
What your program should do in these cases is detect that something is wrong, and then throw some kind of exception that explains what the problem was. Unfortunately I don't have time to elaborate on every possible case you might encounter, so I'll have to leave that as an exercise for you.
Of course, if this is a school assignment or other non-real-world project where you have a guarantee that the data will be well-formed, you may not want to go through all the extra trouble. But it's something you should keep in mind for future projects.

Thoughts on using a data class
You mentioned wanting to use a data class to ingest the input data. If all you are planning to do is replace all the dicts in the list data with equivalent data class objects, and then run those objects through the same logic you have now...then you don't gain that much. You do get the type annotations, and there's nothing wrong with having those, but it's not a huge benefit.

1Technically, Python doesn't even have "loop counters" like some other languages do. All Python for-loops are foreach loops rather than "traditional" for-loops. But it's close enough that the conventions around i still apply.
2Actually, it seems perfectly reasonable for the upstream system to send you data for two or more names at once. You may want to enhance your code to support that (or not, I don't know your circumstances).


Spearman correlations between Numpy array and every Pandas DataFrame row I want to efficiently calculate Spearman correlations between a Numpy array and every Pandas DataFrame row:

import pandas as pd
import numpy as np
from scipy.stats import spearmanr

n_rows = 2500
cols = ['a', 'b', 'c', 'd', 'e', 'f', 'g']

df = pd.DataFrame(np.random.random(size=(n_rows, len(cols))), columns=cols)
v = np.random.random(size=len(cols))
corr, _ = zip(*df.apply(lambda x: spearmanr(x,v), axis=1))
corr = pd.Series(corr)

For now, the calculation time of corr is:

%timeit corr, _ = zip(*df.apply(lambda x: spearmanr(x,v), axis=1))
>> 1.26 s ± 5.19 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

I found another good approach but it calculates only Pearson correlations:

%timeit df.corrwith(pd.Series(v, index=df.columns), axis=1)
>> 466 ms ± 1.12 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

Is there a way to calculate Spearman correlations faster?

Since Spearman correlation is the Pearson correlation coefficient of the ranked version of the variables, it is possible to do the following:

Replace values in df rows with their ranks using pandas.DataFrame.rank() function.
Convert v to pandas.Seriesand use pandas.Series.rank() function to get ranks.
Use pandas.corrwith() function to calculate Spearman correlation - Pearson correlation on ranked data.

import pandas as pd
import numpy as np
from scipy.stats import spearmanr

n_rows = 2500
cols = ['a', 'b', 'c', 'd', 'e', 'f', 'g']

df = pd.DataFrame(np.random.random(size=(n_rows, len(cols))), columns=cols)
v = np.random.random(size=len(cols))

# original implementation
corr, _ = zip(*df.apply(lambda x: spearmanr(x,v), axis=1))
corr = pd.Series(corr)

# modified implementation
df1 = df.rank(axis=1)
v1 = pd.Series(v, index=df.columns).rank()
corr1 = df1.corrwith(v1, axis=1)

Calculation time of the modified version:

    %%timeit
    v1 = pd.Series(v, index=df.columns).rank()
    df1 = df.rank(axis=1)
    corr1 = df1.corrwith(v1,axis=1)
    >> 495 ms ± 13.4 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

Checking corr and corr1 for equality proves that the results are the same:

    print(corr.var()-corr1.var(), corr.mean()-corr1.mean(), corr.median()-corr1.median())
    >> (0.0, 0.0, 0.0)


Model-View-Controller-Service shared flat expenses interface I'm new to php and OOP, and for learning I started writing an application that would help me and my flatmates to manage our expenses and debts to one another.

The full source is available at https://bitbucket.org/nicocool84/coloc-web/src. I tried to use the MVC(S) architecture without using a framework.

The project tree is as follows:

.
├── controllers
│   ├── DebtController.php
│   ├── DefaultController.php
│   └── UserController.php
├── models
│   ├── DebtList.php
│   ├── Debt.php
│   └── User.php
├── services
│   ├── Database.php
│   ├── DebtService.php
│   ├── RedirectService.php
│   └── UserService.php
├── views
│   ├── footer.php
│   ├── header_logged.php
│   ├── header_unlogged.php
│   ├── home_unlogged.php
│   ├── html_header.php
│   ├── jumbotron.php
│   └── list_debts.php
└── web
    ├── index.php
    └── .htaccess

.htaccess redirects all requests to index.php which decides which controller and which action to call based on the URI: http://mysite.my/controller/action.

To handle sessions, when a user is authenticated, I store a User instance with his details in $_SESSION. The related methods are stored in UserService, for instance getCurrentName. Is this a good practice? Should I create a SessionService? Should I do this completely differently?

Is creating a DebtList class to compute who owes what making any sense?

I know the HTML/CSS part is quite ugly, and I'm open to advice there too.

Thanks to anyone who will take the time to review it all.

Here is a start

I opened your code in php storm and it says
Non-static method Database::getPdo() should not be called statically

Where you have code like this

public function deleteAction()
{
    $id = $_REQUEST['debt_id'];
    if (UserService::isLogged()) {
        if (DebtService::getPaidByFromId($id) == UserService::getCurrentUserId() or UserService::isAdmin()) {
            DebtService::deleteById($id);
            return DebtController::listAction();
        } else {
            RedirectService::goHome();
        }
    } else {
        RedirectService::goHome();
    }
}

I prefer to have a guard clause at the top, so you can easily see the outcome of the if/else without having to scroll to the end of the code

public function deleteAction()
{
    if (!UserService::isLogged()) {
        RedirectService::goHome();
    }

    // TODO what if debt_id doesn't exist, always check isset()
    $id = $_REQUEST['debt_id'];

    if (DebtService::getPaidByFromId($id) == UserService::getCurrentUserId() or UserService::isAdmin()) {
        DebtService::deleteById($id);
        return DebtController::listAction();
    } else {
        RedirectService::goHome();
    }
}

Instead of this

$includes = array("controllers", "services", "models");

foreach ($includes as $dir) {
    foreach (glob("../" . $dir . "/*.php") as $file) {
        require $file;
    }
}

you might want to look at autoloading, it's not very hard to do
http://php.net/manual/en/function.spl-autoload-register.php

In your templates you are doing

<?php echo $debt->getDescription(); ?>

Unless you know for sure there is no html or user editable content,
you should really html escape it

<?php echo htmlentities($debt->getDescription()); ?>

Database service, take the config out of the service, and store it in a config file. otherwise you need to edit the code when you deploy to a different server.

Would a join work better here

// DebtService::getDebtsForMonth

SELECT date, amount, name AS description, paid_by, id, user_id
            FROM debts
JOIN debtors ON debts.id=debtors.debt_id
WHERE MONTH(date) = ? and YEAR(date) = ? ORDER BY date

You may need to use exit; here. setting the location doesn't stop the code from running

public static function goToURI($uri_suffix)
{
    header("Location:" . uri_prefix . $uri_suffix);
    exit;
}

It is not essential but I like to keep all my defines in uppercase

define('URI_PREFIX', "/coloc-web");

That should be enough to get your going for now.


Fast templated call back implementation Below is the code for my templated callback implementation. Currently it works for a single parameter. One thing I was going to try and do next was increase the argument to the function from 1..N arguments. Ideally this would be as simple as adding Args... everywhere using variadic templates. I don't have a lot of experience yet with variadic templates so any advice on this is helpful.

#include <iostream>
#include <vector>

template <typename R, typename Arg>
class Callback
{
  public:
  typedef R (*FuncType)(void*, Arg);
  Callback (FuncType f, void* subscription) : f_(f), subscription_(subscription) { }

  R operator()(Arg a)
  {
    (*f_)(subscription_,a);
  }

  private:
  FuncType f_;
  void* subscription_;
};

template <typename R, typename Arg, typename T, R (T::*Func)(Arg)>
R CallbackWrapper (void* o, Arg a)
{
  return (static_cast<T*>(o)->*Func)(a);
}

class Pricer
{
  public:
  typedef Callback<void,unsigned int> CbType;

  void attach ( CbType cb )
  {
    callbacks_.emplace_back(cb);
  }

  void receivePrice ( double price )
  {
    broadcast(static_cast<unsigned int>(price*100));
  }

  void broadcast (unsigned int price)
  {
    for ( auto& i : callbacks_)
    {
      i(price);
    }
  }

  private:
  std::vector<CbType> callbacks_;
};

class Strategy
{
  public:
   Strategy(Pricer*  p) :  p_(p)
   {
     p->attach(Callback<void,unsigned int>(&CallbackWrapper<void,unsigned int, Strategy, &Strategy::update>, static_cast<void *>(this)));
   }

   void update(unsigned int price)
   {
    //update model with price
     std::cout << "Received price: " << price / 100.0 <<  std::endl;
   }

  private:
  Pricer* p_;
};

int main ( int argc, char *argv[])
{
  Pricer p;
  Strategy s(&p);

  p.receivePrice(105.67);
}

The functionality you are trying to create already exists in std::function<> using std::bind<> to help.

Comments on the rest of the code:

Here you use emplace back:

  void attach ( CbType cb )
  {
    callbacks_.emplace_back(cb);
  }

Emplace back is usually used when you have the arguments and want to build the object in place using the arguments. By passing the object you are going to just invoke the copy constructor. As a result there is no benefit from using it. Though there is nothing wrong with using it either. Currently I am still working out when to use emplace_back() over push_back() but this is one situation where I would still use push_back().

Also because you pass the argument by value you are copying the object into the function then using the copy constructor to put it in the array resulting in another copy. So here I would pass by reference.

  void attach(CbType const& cb)
  {
    callbacks_.push_back(cb);
  }

Don't use unnecessary casts

broadcast(static_cast<unsigned int>(price*100));

// This is easier to read as:

broadcast(price*100); // double is auto converted to unsigned int

Use standard types:

typedef Callback<void,unsigned int> CbType;

// Replace with:

typedef std::function<void(unsigned int)> CbType;

If you use the standard function then the equivalent to creation becomes much simpler

 p.attach(std::bind(&Strategy::update,this, _1));

Don't use pointers where a reference is a better choice:

Strategy(Pricer*  p) :  p_(p)

You are passing a pointer and not checking for NULL. Actually the code never checks for NULL so you must use a valid pointer. In this case you may as well pass a reference. If you want to store this internally as a pointer then take its address inside your object.

Strategy(Pricer&  p) :  p_(&p)  // p can never be invalid.


Searching an element in a 2D sorted array I had to write a code (as an exercise) that receives a 2D row wise and col wise sorted array and an element, and return true is the element exists in the array.

The first thing that came to mind when i heard "sorted" is binary search, but than i realized that the last element in each row isn't necessarily smaller than the first one in the next line.

So, i figured out that the best complexity will be O(n), and wrote the following code:

 public static boolean findN(int[][] a, int x) {
        if (a.length == 0 || a[0].length == 0 || x > a[a.length - 1][a[0].length - 1] || x < a[0][0]) {
            return false;
        }
        int LastRow = a.length - 1, Lastcol = a[0].length - 1, row = 0, col = 0;

        while (row <= LastRow) {
            if (a[row][col] == x) {
                return true;
            } else if (col < Lastcol) {
                col++;
            } else {
                col = 0;
                row++;
            }
        }
        return false;
    }

    array example:

arr = {{1,2,7,30},
      {2,4,18,50},
      {3,6,19,90},
      {4,7,20,91}}

After realizing that the best complexity will be O(n), I googled this problem  so I'm almost certain that I'm right (although some people are claiming that they can do it in O(log(n))), but am I really?
Any other thoughts and improvements are welcomed, thank you all in advance!

Your function does a linear search over all elements of the nested array, until the given number is found, or all elements have been visited. The complexity is \$ O(mn) \$ for an \$ m \$-by-\$n \$ matrix. For a square \$ n \$-by-\$n \$ matrix that makes \$ O(n^2) \$, and not \$ O(n) \$ as you claimed.

This can be improved, but let's first do a

Review of your current implementation

The Java naming convention for variables is “lower camel case,” e.g. lastRow, lastCol instead of LastRow, LastCol.

The while-loop is a nested loop over all rows and columns in disguise, and can be more clearly written as nested for-loops:

int numRows = a.length;
int numCols = a[0].length;

for (int row = 0; row < numRows; row++) {
    for (int col = 0; col < numCols; col++) {
        if (a[row][col] == x) {
            return true;
        }
    }
}

This also restricts the scope of row and col to the minimum needed.

A better algorithm

Your implementation does not take advantage of the fact that the numbers are sorted in each row and in each column. A better algorithm is described in Search in a row wise and column wise sorted matrix on GeeksForGeeks:

Start with top right element.
Loop: compare this element e with x

if e = x, then return position of e, since we found x in the given matrix.
if e > x then move left to check elements smaller than e (if out of bound of matrix, then break and return false)
if e < x then move below to check elements greater than e (if out of bound of matrix, then break and return false)

repeat until you find the element, or return false

I do not want to deprive you of the satisfaction of implementing it yourself. Just note that any element is now found after at most \$ m+n \$ steps, so the complexity is reduced from \$ O(mn) \$ to \$ O(m+n) \$ for an \$ m \$-by-\$n \$ matrix.


Simple timer checking class This is a simple class that runs a timer every 10 seconds to update the title with some information like how long the application has been up, depending on the datetime that is set in another class when the application has finished booting up and is responsive. This application is part of a game server and it just displays how long the server has been up so I know when it was last launched.

In the future it will also include the number of players connected in the title. it is important to continuously reload it as this is a console application and there isn't really much room anywhere else to display such data.

I'm really not sure about the method name CheckServer but I was feeling very unmotivated at the time, if anyone could think of a better name, be sure to post that too.

I'm looking for basic improvements in overall performance.

internal sealed class ServerProcessWorker : IDisposable
{
    private Timer _serverTimer;
    private readonly int _timerInterval;
    private DateTime _lastUpdate;
    private bool _disposing;

    public ServerProcessWorker()
    {
        _timerInterval = 10000;
    }

    public void Start()
    {
        _serverTimer = new System.Threading.Timer(e => CheckServer(), null, TimeSpan.Zero, TimeSpan.FromMilliseconds(_timerInterval));
    }

    private void CheckServer()
    {
        if ((DateTime.Now - _lastUpdate).TotalMilliseconds <= _timerInterval )
        {
            return;
        }

        var serverUptime = DateTime.Now - Sirius.GetServer().GetServerInformation().ServerStarted;

        var days = serverUptime.Days + " day" + (serverUptime.Days != 1 ? "s" : "") + ", ";
        var hours = serverUptime.Hours + " hour" + (serverUptime.Hours != 1 ? "s" : "") + ", and ";
        var minutes = serverUptime.Minutes + " minute" + (serverUptime.Minutes != 1 ? "s" : "");
        var uptimeString = days + hours + minutes;

        Console.Title = Sirius.GetServer().GetServerInformation().ServerName + " - Uptime: " + uptimeString;

        _lastUpdate = DateTime.Now;
    }

    public void Dispose()
    {
        if (_disposing)
        {
            return;
        }

        _disposing = true;

        _serverTimer.Dispose();
        _serverTimer = null;
    }
}

This check

if ((DateTime.Now - _lastUpdate).TotalMilliseconds <= _timerInterval )
{
    return;
}

is superflous, because the timer fires at a defined interval.

Does the ServerName change over the time the application is running ? I guess not so it would be better to request the name once and store it for later usage.

The same applies to ServerStarted.

Personally I would create an extension method for the TimeSpan which would do the whole work with the day(s), hour(s) and minute(s). Something along these lines

public static string ToWords(this TimeSpan timeSpan)
{
    return  timeSpan.Days + (timeSpan.Days != 1 ? " days" : " day") + ", "
          + timeSpan.Hours  + (timeSpan.Hours != 1 ? " hours" : " hour") + ", and "
          + timeSpan.Minutes  + (timeSpan.Minutes != 1 ? " minutes" : " minute");
}

and calling like so

var uptimeString = serverUptime.ToWords();

Disclaimer: I don't like the name of the method.


Attemped Sieve of Eratosthenes I've just done Project Euler question 10:

  Find the sum of all the primes below two million.

I get the correct answer and it takes about 3.5-4 seconds to run.

I tried to use the Sieve of Eratosthenes to find the solution, but am not sure if my solution is using the method correctly, or if it could be improved in any way.

I was also wondering if anyone knew if for this case, is the Sieve of Eratosthenes the fastest way?

Any feedback would be very much appreciated.

import java.math.BigInteger;
import java.util.ArrayList;

public class Problem10 {

public static void main (String[] args) {
    double startTime = System.currentTimeMillis();
    run();
    double endTime = System.currentTimeMillis();
    System.out.println("Took "+((endTime - startTime) / 1000)+" seconds");
}

public static void run() {
    ArrayList<Boolean> numbers = new ArrayList<Boolean>();
    for (int i = 0; i < 2000000; i++) {
        if (i < 2) numbers.add(false); else numbers.add(true);
    }

    BigInteger count = BigInteger.ZERO;
    int index = 0;
    while (index < 2000000) {
        if (numbers.get(index) == true) { // if the index is true, i.e. prime
                count = count.add(BigInteger.valueOf(index)); // then add it's sum to the count
                for (int i = index; i < numbers.size(); i+=index) { // and then go through all of it's multiples (including itself)
                    if(numbers.get(i) == true) {
                        numbers.set(i, false); // and set them all to be false
                    }
                }
        }
        index++;
    }
    System.out.println("The sum of all primes below 2 million is:  " + count);
}
}

Boolean expressions evaluate to boolean. That means that can be used interchangeably with true and false.

if (numbers.get(index) == true)

is the same as

if (numbers.get(index))

if (i < 2) numbers.add(false); else numbers.add(true);

is the same as

numbers.add(i>=2);

It is also much easier to read and edit. Collapsing an if/else into one line is bad form. It forces the reader to look carefully at what is actually happening. Where as separate it across multiple lines, as you do with your other if statements allows each part to stand on its own. Additionally, dropping the curly braces means you will need to do extra work if you want to add another statement. Or, if you forget, the second line will execute unconditionally.

Why are you using a while loop for the main body? It does exactly what a for loop is designed to do and your code shows that you understand how to use them.

You should extract 2000000 into a constant. The name will make it clearer what the meaning of the number is. The constant will mean that the value clearly means the same thing in both places.

Comments are best used for describing why something is happening. Code is best for stating what is happening. If you feel that the code is complicated enough that you need to explicitly say  what is happening, that is an indication that extracting that piece of code into a function with a descriptive name would be helpful.


Arena memory allocator I'm not exactly sure if this is technically an "arena allocator" but it serves a similar purpose: providing a fast way to allocate a lot of objects that can all be freed at once.

#include <type_traits>
#include <utility>
#include <new>

template <std::size_t BlockSizeIn = 1024>
struct BlockProviderNewDelete
{
    static constexpr std::size_t BlockSize = BlockSizeIn;

    static void* allocateBlock()
    {
        return ::operator new(BlockSize);
    }
    static void freeBlock(void *block) noexcept
    {
        ::operator delete(block);
    }
};

template <class BlockProviderIn>
class ArenaAllocator
{
public:
    using BlockProvider = BlockProviderIn;

private:
    struct AllocBlock
    {
        AllocBlock *next;
        char mem[BlockProvider::BlockSize - sizeof(AllocBlock*)];
    };
    AllocBlock *base;
    AllocBlock *headBlock;
    std::size_t headIndex;

    void appendBlock()
    {
        AllocBlock *newBlock = static_cast<AllocBlock*>(BlockProvider::allocateBlock());
        newBlock->next = nullptr;
        headBlock->next = newBlock;
        headBlock = newBlock;
        headIndex = 0;
    }
    void nextBlock()
    {
        if(headBlock->next)
        {
            headBlock = headBlock->next;
            headIndex = 0;
        }
        else
            appendBlock();
    }

public:
    static constexpr std::size_t BlockSize = BlockProvider::BlockSize;
    static constexpr std::size_t MaxAllocationSize = sizeof(AllocBlock::mem);

    ArenaAllocator()
    {
        base = static_cast<AllocBlock*>(BlockProvider::allocateBlock());
        base->next = nullptr;
        headBlock = base;
        headIndex = 0;
    }
    ~ArenaAllocator()
    {
        AllocBlock *cur = base, *next;
        while(cur != nullptr)
        {
            next = cur->next;
            BlockProvider::freeBlock(cur);
            cur = next;
        }
    }

    void reset() noexcept
    {
        headBlock = base;
        headIndex = 0;
    }

    void* allocate(std::size_t s)
    {
        if(s == 0) s = 1;

        if(s > MaxAllocationSize)
            return nullptr;
        else if(s > MaxAllocationSize - headIndex)
            nextBlock();

        void *ptr = &headBlock->mem[headIndex];
        headIndex += s;
        if(headIndex >= MaxAllocationSize)
            nextBlock();

        return ptr;
    }

    template <typename T, typename... Args>
    T* construct(Args... args)
    {
        static_assert(std::is_trivially_destructible<T>::value, "Type must be trivially destructible!");
        static_assert(sizeof(T) <= MaxAllocationSize, "Type must not be larger than max allocation size!");

        T *result = static_cast<T*>(allocate(sizeof(T)));
        new (result) T(std::forward<Args>(args)...);
        return result;
    }
};

template <std::size_t BlockSize>
using ArenaAllocatorDefault = ArenaAllocator<BlockProviderNewDelete<BlockSize>>;

A caveat to this is that types allocated with the arena allocator must be trivially destructible (i.e. their memory must be reusable without destroying them first).

I'm already aware of one major issue: it doesn't consider alignment. However that's a fairly easy fix, so I'd like that to be ignored in the review. I'm mostly wondering whether this invokes any undefined behavior (other than due to ignoring memory alignment).

Here's an example of usage (this was to test if it leaked any blocks of memory or segfaulted. It didn't.):

// 2 MiB block size
ArenaAllocatorDefault<2097152> test;

for(size_t i = 0; i < 100000; ++i)
{
    for(size_t j = 0; j < 1000000; ++j)
    {
        test.construct<char[128]>();
    }
    test.reset();
}

Obviously you'd want to actually use the pointer returned by the construct() method for usual purposes but this was just for testing.

Missing copy constructor & assignment operator
Because you're handling raw pointers and owning memory through them, you must implement the rule of three. Otherwise you will get double deletes after some one copies your allocator.
Use of reset()
By the looks of it, this class is developed for a very specific purpose. OP hasn't told us what the purpose is unfortunately. And the class is kind of dangerous.
Calling reset() will conceptually invalidate any pointers returned by construct(...) but they will continue to be accessible and eventually overwritten with new data until the ArenaAllocator is destroyed. If some piece of code is wrongly holding a pointer to a data block after the reset this should be a bug but static and dynamic code analysis tools will likely not detect this because the pointer is still valid. Neither will you get a crash, just a quiet possibly wrong result.
Although safe if used correctly in a single threaded environment, I'm afraid that it is easy to make a mistake with this class.
Ewww! Raw new/delete
Instead of having a custom allocator policy (BlockProvider) you could use the standard std::allocator for example. By doing this you can now use std::shared_ptr with the allocator provided and get rid of the usage of raw new/delete which is bad, yet you still have the option of providing your own allocator. Don't worry, the overhead of using std::shared_ptr is minimal in this case because your blocks are large.
Overall
I don't see any obvious bugs besides the missing rule of three.
Although custom memory management has its uses, you really do want profiling data to back up the added complexity and failure points of rolling your own memory management. Unless you have a strong reason for custom management that seriously outweighs the reduction in efficacy of code analysis tools and increase in risk for bugs, I would not recommend rolling your own. You have to be sure the extra headache is worth it.
Also note, that if your type is a POD then operator delete will not call any destructors (there aren't any!). In fact I would expect various standard library functions to not call destructors on trivially destructible types any way. Apparently at least GCC does this.
If you have a piece of code that must rapidly allocate and throw away memory at a pace where the standard allocator isn't fast enough, then you might want to consider changing that code to re-use the same memory area instead. This is even faster than writing a custom allocator, solves the problem at the root and doesn't introduce near as much hair loss as custom memory management.
If you just need a lot of fixed size objects quickly and contiguously, I would consider using a fixed item size memory pool and overloaded new/delete for the class and make it transparent. You can do some very efficient reclamation and allocation when items are of fixed size.


Random String Generator using OpenSSL in PHP function GenerateRandomString($Length) {
 $Length /= 2;
 $cstrong = false;
 while ($cstrong != TRUE){
    $bytes = openssl_random_pseudo_bytes($Length, $cstrong);
 }
 $hex = bin2hex($bytes);
 return $hex;
}

Is this a good function for generating a cryptographically secure random string? Is there a chance, that it'll get into an infinite loop, because $cstrong stays 'False'?

The documentation says that crypto_strong is set depending on the algorithm used. So it will never go from false to true, so yes, this might result in an infinite loop.

Use it instead to see if you should trust the result. If it is true, the string should be secure.

(you should also never design your own crypto algorithms for anything other than academic interest)

because this is codereview, I also have a couple of notes regarding your code:

Why do you divide the length by 2? Seems unnecessary. If it does have a purpose, comment on it.
false TRUE: use case uniformly. either false and true or FALSE TRUE
$hex = bin2hex($bytes);
return $hex;
just make this one line: return bin2hex($bytes)
it is convention to use camelCase for variables, so Length should be length and the function name generateRandomString


Haskell - Pig Latin Translator What improvements can I make to the following Pig Latin translator? So far, I've been wondering about the FlexibleInstances declaration, but I can't think of how to remove it (since it's required for PigLatin String in instances, which is required for me to use " " in the Monoid instance; I need the extra type argument for Functor).

{-# LANGUAGE FlexibleInstances #-}

import Control.Monad(when)
import Data.Char (toLower, toUpper)
import System.IO (hFlush, stdout)

data PigLatin a = PigLatin a

instance Monoid (PigLatin String) where
  mempty = PigLatin ""
  mappend (PigLatin a) (PigLatin b) = PigLatin $ a ++ " " ++ b

instance Functor PigLatin where
  fmap f (PigLatin x) = PigLatin $ f x

instance Show (PigLatin String) where
  show (PigLatin s) = s

data English a = English a
  deriving (Eq)

safeTail :: [a] -> [a]
safeTail (_:xs) = xs
safeTail [] = []

quitCommand :: English String
quitCommand = English "\\q"

wordToPig :: English String -> PigLatin String
wordToPig (English word@(x:_)) = PigLatin $ if isVowel x
                                              then word ++ "yay"
                                              else rearrangedWord ++ "ay"
    where rearrangedWord = drop (length initialConsonants) word ++ initialConsonants
          initialConsonants = takeWhile isConsonant word
          isConsonant = not . isVowel
wordToPig (English "") = PigLatin ""

isVowel :: Char -> Bool
isVowel letter = letter `elem` ['a', 'e', 'i', 'o', 'u']

englishWords :: English String -> [English String]
englishWords (English input) = map English $ words input

sentenceToPig :: English String -> PigLatin String
sentenceToPig = fmap (capitalize . safeTail . map toLower) . foldl mappend mempty . map wordToPig . englishWords

capitalize :: String -> String
capitalize (x:xs) = toUpper x : map toLower xs
capitalize [] = ""

prompt :: String -> IO (English String)
prompt promptInput = do
  putStr promptInput
  hFlush stdout
  input <- getLine
  return $ English input

main :: IO ()
main = do
    input <- prompt "> "
    when (input /= quitCommand) $ do
      print $ sentenceToPig input
      main

You can let it derive the  Functor declaration:

data PigLatin a = PigLatin a deriving Functor

foldl mappend mempty <- You want fold.

break implements some of your stuff already.

I wouldn't introduce the types PigLatin a and English a in the first place. They just introduce wrapping without extra type-level structural info.

import Control.Monad (when)
import Data.Char (toLower, toUpper)
import System.IO (hFlush, stdout)
import Control.Cateogry ((>>>)) -- flip (.)

--This turns empty words to yay, but words doesnt send empty words, right?
wordToPig :: String -> String
wordToPig = break (`elem` "aeiou") >>> \(initialConsonants, rest) ->
  rest ++ initialConsonants ++ if null initialConsonants then "yay" else "ay"

-- Your pig latin rules drop the first character of each sentence at the end?
sentenceToPig :: String -> String
sentenceToPig = capitalize . drop 1 . unwords . map wordToPig . words

capitalize :: String -> String
capitalize (x:xs) = toUpper x : map toLower xs
capitalize [] = ""

main :: IO ()
main = do
  putStr "> "
  hFlush stdout
  input <- getLine
  when (input /= "\\q") $ do
    print $ sentenceToPig input
    main


Project Euler # 21 amicable numbers Python Let \$d(n)\$ be defined as the sum of proper divisors of \$n\$ (numbers less than \$n\$ which divide evenly into \$n\$).
If \$d(a) = b\$ and \$d(b) = a\$, where \$a ≠ b\$, then \$a\$ and \$b\$ are an amicable pair and each of \$a\$ and \$b\$ are called amicable numbers.

For example, the proper divisors of 220 are 1, 2, 4, 5, 10, 11, 20, 22, 44, 55 and 110; therefore d(220) = 284.
The proper divisors of 284 are 1, 2, 4, 71 and 142; so d(284) = 220.

Evaluate the sum of all the amicable numbers under 10000. Here's my implementation in Python. I need your feedback on how this can be improved.

Note: I'm a beginner in programming.

from time import time

def get_divisors(n):
    """Yields divisors of n."""
    divisor = 2
    while divisor * divisor <= n:
        if n % divisor == 0 and n // divisor != divisor:
            yield divisor
            if n // divisor != divisor:
                yield n // divisor
        divisor += 1
    yield 1

def get_sum_amicable(n, divs={2:1}):
    """Yields amicables below n."""
    for number1 in range(n):
        for number2 in range(number1):
            try:
                if divs[number2] == number1 and divs[number1] == number2:
                    yield number1
                    yield number2
            except KeyError:
                divs[number1] = sum(get_divisors(number1))
                divs[number2] = sum(get_divisors(number2))
                if divs[number2] == number1 and divs[number1] == number2:
                    yield number1
                    yield number2

if __name__ == '__main__':
    start = time()
    print(sum(get_sum_amicable(10000)))
    print(f'Time: {time() - start} seconds.')

Readability / maintainability

def get_sum_amicable(n, divs={2:1}):
    """Yields amicables below n."""
    for number1 in range(n):
        for number2 in range(number1):
            try:
                if divs[number2] == number1 and divs[number1] == number2:
                    yield number1
                    yield number2
            except KeyError:
                divs[number1] = sum(get_divisors(number1))
                divs[number2] = sum(get_divisors(number2))
                if divs[number2] == number1 and divs[number1] == number2:
                    yield number1
                    yield number2

This is awful. It violates the rule "Don't Repeat Yourself"; it expects exceptions as part of non-exceptional flow; and it relies on an obscure language feature/mis-feature.

All you need to do is sum numbers x in range(n) (or, perhaps better, range(2, n), to avoid complications around 0 and 1) for which d(x) < n and d(d(x)) == x. (And given the way the question is phrased, you might assume that there will be no amicable pair which is split around n, so the first half of the test is probably unnecessary). That's a one-liner.

If you want to cache d, instead of using default values you can use functools.lru_cache. But...

Performance

Whenever you're doing divisibility checks on every number from 1 to n, you should think about using a sieve. Either build an array of one prime factor of each number, and work from that, or (where appropriate) inline the calculation into the sieve processing.

It's fairly easy to adapt the sieve of Eratosthenes to generate an array of d efficiently using the inline approach.


Making an array editable by user What I want to do:

Let users add data to an array through an input field and add button.

HTML:

<input type="text" id="input">
<button type="button" id="btnAdd">Add Number</button>

JS:

var arrayList = [];

$(document).ready(function() {

    $('#btnAdd').on('click', mkArray);

    $('#outputBox').on('click', 'span', deleteNum);

});

Display the array to the user.

function mkArray(event) {

    event.preventDefault();

    var numberAdd = $('#input').val();

    arrayList.push(numberAdd);

    $('#outputBox').append(' ' + numberAdd + '');

}

If the user clicks on the data, confirm deletion and then delete the item and refresh the array and display.

function deleteNum() {

    var confirmation = confirm('Are you sure you want to delete?');

    var numberIndex = $(this).index();

        if (numberIndex > -1) {

        arrayList.splice(numberIndex, 1);

        };

    $(this).remove();

};

JS FIDDLE

Question

Any potential pitfalls with this way of modifying an array?

Any potential pitfalls with this way of modifying an array?

There certainly are for the user! Your code is not really confirming deletion: The number gets removed even if you press No/Cancel. You need to actually check the return value of the confirm() call:

var confirmation = confirm('Are you sure you want to delete?'); // returns true/false

if(confirmation) {
  // user confirmed, so proceed with removing the item
}

I'd also recommend a more descriptive confirmation message; tell me what I'm about to delete at least. Otherwise, I can't really be sure.

And there are further pitfalls.

For one, you're using the user's input as an element ID. Now, firstly, you don't need an ID. It's not used for anything. Second, IDs are really meant to be unique, but I can add the same thing as many times as I want, and I'll just get a bunch of elements with the same ID (it won't break anything, because - as mentioned - the ID isn't used for anything, but it only makes the IDs more pointless). And third, the input isn't being checked, so the user can input whatever they want, and have it appended directly to the page.

For instance, if you input , you'll get an alert when you click to add it, because you've just added a script element to the page. It'll also add some complete nonsense HTML to the page, because a script tag has suddenly become the ID for a span element, and... well, nonsense. Even if you weren't using the user's input as ID, it'd still behave as described, because you're inserting the input string as-is in the span. If you want to insert as text, use jQuery's .text() to set it. Otherwise it'll be interpreted as HTML.

E.g. you can input the string  and things get very zen.

You can do all this with the JS console of course, but the point remains: Always, always validate and sanitize user input, and insert it in a safe manner.

Also, your arrayList variable is global, meaning anything and everything can modify it in any way (for instance, you can input  and your list will get reset when it should just add something). It can end up not even being an array. In that case, your code will fail, because you can't splice() something that's not an array. Even if it is still an array, it may have been sorted, emptied, or who knows what else, and splice() might remove the wrong thing - or nothing at all.

Really, the trouble here is that the array isn't doing anything useful. You add numbers to the HTML directly, and you remove them from the HTML directly. The array is just a weak, error-prone copy of what's on the page. And you have to manually keep them in sync.

It's much better to have a single source for your data, so there's only one place to keep track of things. In this case, I'd say that that source should be the HTML. If you need to get an array of inputs, you can just use jQuery to fetch all the input so far:

$("#outputBox span").map(function () { return $(this).text() }).toArray()

That line will go through each of the spans, get their text, and return an array with the stuff that's been added so far (provided you remove the extra space in the span, which shouldn't really be there anyway - see below)

Other stuff:

arrayList is not a great name for a variable. Yeah, it's an array, so it's list of some sort... but what's in it?. I'd simply call it numbers or inputs, since that's what it actually represents.
Use jQuery to build the elements to avoid the error-prone string-concatenation
If you're making a list in HTML, make an actual
 or
 list (I'd pick ul as it's an unordered list of user input), and make the individual numbers list items (
).
Doesn't <button type="button" seem redundant to you? Because it is. You only need the type="button" if you're using an <input> element. A <button> element is - by definition - already a button.
You don't need event.preventDefault(), since clicking a button has no default behavior that needs preventing.
Use the $(function () { ... }); shorthand instead of $(document).ready(function () { ... });

In the end, you can do it all like this

HTML

<input id="input" type="text">
<button id="addItem">Add</button>

JS

$(function () {
  // keep the input and output elements around for later
  var input  = $("#input");
  var output = $("#output").on("click", "li", function () {
    if( confirm("Are you sure you want to delete this item?") ) {
      $(this).remove();
    }
  });

  $("#addItem").on("click", function () {
    var value = input.val();
    if(/^\d+$/.test(value)) { // check that the string contains only digits
      output.append($("

").text(value));
      input.val("");
    } else {
      alert("Please enter only numbers");
    }
  });
});

Here's a demo


Stable partition in Numpy In essence, I need to do a stable partition: All elements of the 1D np.array a that are present in b should be moved to the front of a but apart from that, the relative order of the elements in a should be preserved.

This code seems to do what I want:

from __future__ import print_function
import numpy as np

if __name__=='__main__':
    a = np.array([1, 2, 3, 4, 5], dtype=np.int32)
    b = np.array([4, 2, 6],       dtype=np.int32)
    pos = np.in1d(a, b, assume_unique=True)
    cnt = np.count_nonzero(pos)
    tmp = np.empty(a.size, dtype=np.int32)
    tmp[0:cnt] = a[pos]
    tmp[cnt: ] = a[np.invert(pos)]
    a = tmp
    print('after stable partition:', a)

Output:

after stable partition: [2 4 1 3 5]

Is there a cleaner / more efficient way of doing this in NumPy?

The actual creation of the pos mask looks perfect to me. You can streamline the rest of the code into a single-liner as:

a = np.concatenate((a[pos], a[~pos]))


Caliburn Micro communication between list and edit viewmodels I'm new at Caliburn Micro and the MVVM pattern.

I have 3 views (with its corresponding viewmodels):

AppView
ClientsView
EditClientView

AppView is my main page with a TabControl inside. The child views (WPF UserControls) are loaded in new tabs.

In ClientsView I have a grid with filtered clients. I select one, and when the Modify button is clicked I need to load a new tab with the editClientView and load its data. Here is my code:

public class EventAggregationProvider
{
    static EventAggregator _eventAggregator = null;
    public static EventAggregator EventAggregator
    {
        get
        {
            if (_eventAggregator == null)
                _eventAggregator = new EventAggregator();

            return _eventAggregator;
        }
    }
}

Function on ClientsViewModel:

public void ModificarCliente()
    {
        AppViewModel myParent = (AppViewModel)this.Parent;
        myParent.OpenTab(typeof(modClienteViewModel));
        EventAggregationProvider.EventAggregator.Publish(_selectedclient);
    }

MainViewModel OpenTab function:

public void OpenTab(Type TipoVista)
{
    bool bFound = false;
    if (TipoVista != null)
    {
        Screen myScreen = (Screen)Activator.CreateInstance(TipoVista as Type);
        myScreen.DisplayName = myScreen.ToString();
        foreach (Screen miItem in Items)
        {
            if (miItem.ToString() == myScreen.ToString())
            {
                bFound = true;
                ActivateItem(miItem);
            }
        }
        if (!bFound) ActivateItem(myScreen);
    }
}

EditClientViewModel:

class modClienteViewModel : Screen, IHandle<vw_ClientesFull>
{
    private OhmioService.OhmioServiceClient serviceClient =
        new OhmioService.OhmioServiceClient();

    public Clientes Cliente { get; set; }

    public modClienteViewModel()
    {
        EventAggregationProvider.EventAggregator.Subscribe(this);
        Cliente = new Clientes();
    }

    public void Handle(vw_ClientesFull myClient)
    {
        Cliente = serviceClient.Cliente_GetById(myClient.ID_Cliente);
        NotifyOfPropertyChange("Cliente");
    }
}

So what's your opinion on this? Especially about how I open the edit view (ClientsView becomes aware of the existence of MainWindowViewmodel and EditClientViewModel) from clients' viewmodel, and how I use eventaggregator to exchange the selected client between clientsViewModel and EditClientViewModel.

Naming

You should use PascalCase to name methods and types. You do this most of the time, but slip up with modClienteViewModel

Your parameters should be in camelCase. Again, you do this most of the time, but slip up with TipoVista in your OpenTab method.

Avoid Hungarian notation, with a modern IDE it's not necessary, and it just makes types harder to read.

bool bFound = false;

should be

var found = false;

The same probably goes for miItem which can just be item without any loss of meaning.

Var

Prefer to use var in a declaration when the right-hand side makes the type obvious.

e.g.

AppViewModel myParent = (AppViewModel)this.Parent;

is obvious because of the cast, so we write

var myParent = (AppViewModel)this.Parent;

The reasoning behind this is that if you wanted to change a type, you would have to do so in two places in one line. With var, it is only one change.

You should also use var when defining the loop variable in a foreach loop.


Conditional parsing, non-standard approach Conditional parsing (aka read some tokens and return true or false).

As a solution to this SO question, I wrote following code:

bool nectar_loader::resolve_conditional( const std::function<bool(const string&)> &config_contains )
{
    bool result = true;
    // My very own recursive implementation
    /*
      - each set of parenthesis is handled recursively
      - logical AND: +
      - logical OR:  |
      - logical NOT: !
      - two bools: "result" and "current"
      - "result" keeps global result, and is modified by "+"
      - "current" keeps results for "|" and "!"
      - syntax checking for invalid
    */
    bool previous_was_operator = true;
    bool current = false; // keep track of current state
    string token;
    while( next_token(token) )
    {
        conditional_operator op;
        if( map_value(conditional_operator_map, token, op) )
        {
            if( previous_was_operator )
            {
                if( op == conditional_operator::not_op )
                    current ^= current; // negate next
                else
                {
                    syntax_error( "Conditional operators \'+\', \'|\', \')\', and \'(\' must be followed by a CONFIG string." );
                    break;
                }
            }
            else
            {
                switch(op)
                {
                    case conditional_operator::right_parenthesis:
                        // TODO: allow conditionals without outer parenthesis of the form (a+b)|(c)
                        return result;
                    case conditional_operator::left_parenthesis: // recurse
                        return resolve_conditional( config_contains );
                    case conditional_operator::plus_op:
                        result = result && current; // "current" -> "result"
                        if( !result ) // negative when an element of a "+" expression
                        current = false; // reset "current"
                        break;
                    case conditional_operator::or_op: // combine with "current"

                    case conditional_operator::not_op: // unreachable
                        throw runtime_error( "Internal logic error in nectar_loader::resolve_conditional" );
                }
            }
            previous_was_operator = true;
        }
        else if( !previous_was_operator )
        {
            syntax_error( "In a conditional all CONFIG strings must be seperated by a conditional operator \'+\', \'|\', \')\', or \'(\'." );
            break;
        }
        else // previous was operator, so now we have a CONFIG string
        {
            // check CONFIG string, and perform a logical OR
            // TODO: check effect of "!"
            current = current || config_contains(token);
        }
    }
    return result;
}

next_token() reads the next token, and returns false if this fails. syntax_error sets a global error status with message which is checked after calling this function. map_value returns true if the token can be converted to an enum class value (so if it is a conditional operator as defined by me). This algorithm is more flexible than a shunting yard (due to my future plans).

I have not tested this extremely well, but what do you think?

A few comments:

bool nectar_loader::resolve_conditional( const std::function<bool(const string&)> &config_contains )

I'd generally prefer to turn this into a template, and pass the correct function type (and if I used std::function at all, use it only internally to store something):

template <class func>
bool nextar_loader::resolve_conditional(func &config_contains) {

    bool result = true;
    // My very own recursive implementation
    /*
      - each set of parenthesis is handled recursively
      - logical AND: +
      - logical OR:  |
      - logical NOT: !
      - two bools: "result" and "current"
      - "result" keeps global result, and is modified by "+"
      - "current" keeps results for "|" and "!"
      - syntax checking for invalid
    */

I'd prefer to move the block comment to just above the function header. I'd also prefer more descriptive names than result and current. Given their use, I'd consider names more like "expression_value" and "subexpression_value".

    bool previous_was_operator = true;
    bool current = false; // keep track of current state

This comment seems to add nothing to our understanding of the code. I'd consider something more like: "// Assume current sub-expression is invalid until parsed." (assuming that's really why you've initialized it to false).

    string token;
    while( next_token(token) )
    {
        conditional_operator op;
        if( map_value(conditional_operator_map, token, op) )
        {
            if( previous_was_operator )
            {
                if( op == conditional_operator::not_op )
                    current ^= current; // negate next

Again, the comment seems to do little to assist understanding of the code. I'd probably start by pointing out that an expression like a op1 op2 b is valid if and only if op2 is a unary operator (of which this grammar apparently only supports one).

                    case conditional_operator::plus_op:
                        result = result && current; // "current" -> "result"

Given that you're using it as an and, I'd name this conditional_operator::and_op (or something similar). Then name should reflect the logical meaning of the operator, not the fact that it happens to be (mis-)represented with the + character.

                        if( !result ) // negative when an element of a "+" expression
                        current = false; // reset "current"

This if statement appears redundant (at least to me). You've just set result to the logical and of current and result, so it will be true if and only if both inputs were already true.

result = current = (current && result);

So, both end up true if and only if both started out true. If either started out false, both end up false.

                        break;
                    case conditional_operator::or_op: // combine with "current"

                    case conditional_operator::not_op: // unreachable
                        throw runtime_error( "Internal logic error in nectar_loader::resolve_conditional" );

This looks rather like a bug. Did you really intend that or_op would throw an exception as unreachable, or should there be a break after the or_op case?


Shift and merge numbers as in 2048 game code I started to learn Rust last night. Given its parallelization capabilities, I thought a good starting point would be porting my C++ AI for 2048 to Rust.

This is the function implementing the shift of one line (from high to low indices) of the 2048 board, ported verbatim from C++. It is supposed to shift the numbers in the line and return the score made by that move.

static SCORE_MERGE_FACTOR: f32 = 1.2f32;

fn shift_line(line: [&mut u8, ..4]) -> u64 {
    let mut result: u64 = 0;
    let mut i = 0;
    while i < line.len() {
        let mut merged = false;
        if *line[i] != 0 &&
           i > 0 &&
           *line[i-1] == *line[i]
        {
            *line[i-1] += 1;
            *line[i] = 0;
            merged = true;
            result += *line[i-1] as u64;
        }
        if *line[i] == 0 {
            let mut shifted = false;
            let mut j = i+1;
            while j < line.len() {
                if *line[j] != 0 {
                    *line[i] = *line[j];
                    *line[j] = 0;
                    shifted = true;
                    break;
                }
                j += 1;
            }
            if !shifted || merged {
                i += 1;
            }
        } else {
            i += 1;
        }
    }

    (result as f32 * SCORE_MERGE_FACTOR).round() as u64
}

I would love a general review with respect to Rust's paradigms, but I feel especially uncomfortable for not using iterators and having to use mutable variables all over the place.

I also find these explicit casts quite cumbersome, are they really necessary?

With both scoring and merging

Oops! I did indeed neglect to include merging. This is my mistake; I'm not a big fan of returning values via mutable parameters, so I think my brain shut off. Fortunately, while working on the merging logic, I made the scoring logic shorter.

// Comment #1
fn compress_zeroes(line: [u8, ..4]) -> (u8, u8, u8, u8) {
    let mut nums: Vec<_> = line.iter().map(|x| *x).filter(|x| *x != 0).collect();
    nums.resize(4, 0);
    (nums[0], nums[1], nums[2], nums[3])
}

// Comment #2
fn score_line(line: [u8, ..4]) -> u64 {
    let line = compress_zeroes(line);
    let line = (line.0 as u64, line.1 as u64, line.2 as u64, line.3 as u64);

    let r = match line {
        (a, b, c, d) if a == b && c == d && a != 0 && c != 0 => a+1 + c+1,
        (a, b, _, _) if a == b && a != 0                     => a+1,
        (_, a, b, _) if a == b && a != 0                     => a+1,
        (_, _, a, b) if a == b && a != 0                     => a+1,
        _                                                    => 0,
    };

    (r as f32 * SCORE_MERGE_FACTOR).round() as u64
}

fn squish_line(line: [u8, ..4]) -> [u8, ..4] {
    let line = compress_zeroes(line);

    // Comment #3
    match line {
        (a, b, c, d) if a == b && c == d && a != 0 && c != 0 => [a+1, c+1, 0,   0],
        (a, b, c, d) if a == b && a != 0                     => [a+1, c,   d,   0],
        (a, b, c, d) if b == c && b != 0                     => [a,   b+1, d,   0],
        (a, b, c, d) if c == d && c != 0                     => [a,   b,   c+1, 0],
        (a, b, c, d)                                         => [a,   b,   c,   d],
    }
}

This is probably the best enhancement. Since we don't care about zero values, we can simply remove all the zeroes and then add them back at the end of the array. This removes 3 cases from each match statement!
Conceptually unchanged from the earlier solution, just smaller due to being able to ignore zeroes.
Similar to score_line, we use pattern matching to combine consecutive values that are the same (and non-zero). In this case, we need to track all of the values, so I changed up the naming a bit to leave each letter in the same position.

I definitely prefer having two distinct methods here. In the original, the method is called score_line, but it should actually be called score_and_merge_line. I think that added to my original confusion.

Original answer

There's a lot of code in the original to move things around, just to ignore all that and count how many squares would be merged. I made use of Rust's pattern matching to express the small number of interesting cases that exist in a given line.

fn score_line(line: [u8, ..4]) -> u64 {
    // Comment #1
    let a0 = line[0] as u64;
    let a1 = line[1] as u64;
    let a2 = line[2] as u64;
    let a3 = line[3] as u64;

    // Comment #2
    let r = match (a0, a1, a2, a3) {
        (a, b, c, d) if a == b && c == d && a != 0 && c != 0 => a+1 + c+1,
        (a, b, _, _) if a == b && a != 0 => a+1,
        (_, a, b, _) if a == b && a != 0 => a+1,
        (_, _, a, b) if a == b && a != 0 => a+1,
        (a, 0, b, _) if a == b && a != 0 => a+1, // Comment #3
        (a, 0, 0, b) if a == b && a != 0 => a+1,
        (_, a, 0, b) if a == b && a != 0 => a+1,
        _ => 0, // Comment #4
    };

    (r as f32 * SCORE_MERGE_FACTOR).round() as u64
}

We start by unpacking each of the 8-bit numbers and casting them to a larger type. When we add two squares together, it's possible to require more than 8 bits. A u16 would suffice here, but the original method returned a u64, so we will stick with that.
We've unpacked the array to separate variables, but we actually want to pattern match against all of them at once. We create a tuple to cram them all back together and match on that.
All the pattern match lines are similar, but this one has the most variety. a and b will be bound to the value of the number in the respective position in the tuple. _ means that we don't care about the value in that position, and 0 requires the literal value 0 in that position. We also use a pattern guard (if a == b...) to further restrict the pattern match to pairs of numbers that equal each other. The expression after the => will be the result of the match if that particular branch is chosen.
This is the catch-all branch, which is taken when none of the above branches are picked.

I think that this solution is improved from the original because:

Removed unneeded mutability. No variables are required to be mutated after they are created, which helps me as a programmer reason about the code better.
Removed unused functionality. The function is designed to return the score, it doesn't need to actually move numbers around into the future positions. This relates to the lack of mutability.
Pattern matching replaces what could be a complicated series of if-else checks with something that has more concise and has a visual component. The pattern matching looks like a line of numbers would.
There's no loops or nested loops. Again this helps me as a programmer see the logic clearer.

I tried to stay close to the original code in terms of types, but I would probably make a few more changes if I kept going:

Make a Line type or type alias.
Make a newtype or type alias to indicate that the u8 is to be interpreted as a power of two.
Use Option for missing values, instead of zero.
Remove the 1.2x factor, since I don't currently see the benefit of multiplying every value. If everything has the same weight, it doesn't affect anything.

Additionally, I believe there is a bug in the original code when two adjacent cells have 255. I found this because I compared this code against the original for every possible value. This only took a few minutes, so I believe my code is performant enough for the simple case.


Is this an efficient/correct way to get largest Prime factor? Is this an efficient way to get the largest prime factor of a given number? Other solutions I found involved nested algorithms.
function largestPrimeFactor(number) {
  const factors = []
  for(let i = 2; i <= number; i++){
    if(number % i === 0){
      factors.push(i);
      number = number / i;
      i = 1;
    }
  }
  return factors.pop()
}

Let's consider a few possible ways to speed this up.
Only look for prime factors
First of all, we know that even numbers (other than 2) are all composites, not primes. So, once we check for 2 as a factor, we can ignore all the other even numbers.
if (number % 2 == 0)
    factors.push(2);

for (int i=3; i<limit; i+=2) {
    if (number % i == 0)
        factors.push(i);
}

This will normally approximately double speed in return for minimal effort.
Limit the search for factors
Note that in the loop above, I've specified the upper limit on the loop as limit rather than number. There's a reason for that. If two numbers multiply to give a third number, at least one of the factors must be less than or equal to the square root of the result. If it's a perfect square, then both of those can be the square root of the number. Otherwise, one is smaller than the square root, and the other is larger than the square root.
But we don't have to conduct a long search for the second number--once we know the factor that's smaller than the square root, simple division tells us the other factor.
So, we can search only for factors less than or equal to the square root, and using them, we can directly find the factors that are larger than the square root.
Also note one other point: there can only be (at most) one prime factor larger than the square root.
Consider using a sieve
Taking the two preceding points together, we can see another possibility. In particular, we can start by searching for primes less than or equal to the input number. Then we can test whether the number is divisible by them (and not any composites).
There's a really fast algorithm for finding all the primes up to some limit called the Sieve of Eratosthenes. It doesn't involve doing an division (which is a slow operation for computers). Instead, consider starting with a table of all the numbers up to the limit you care about. Starting from 2, add 2 to your list of primes numbers, then cross off all the multiples of 2. The look for the next number that isn't crossed off (3 in this case). Add it to the list of primes, then cross off all the multiples of 3. Then repeat--look for the next number that isn't crossed off (5), add it to the list of primes, and cross off all of its multiples.
When you've gone through the whole table that way, you have a list of all the prime numbers up to the limit you care about. Then in your original loop turns into something like:
for (int i=0; i<numberOfPrimes; i++)
    if (number % primes[i] == 0)
        factors.push(primes[i]);

Although it's written in C++ rather than JavaScript, I posted some code to implement this algorithm on SO some years ago. Most of the code isn't very specific to C++ though, so converting to JavaScript should be fairly easy.
This may fall within what you're referring to as a "nested algorithm" though--I'm not entirely sure what you mean by that.


Simulation of animal skins This is a simulation of animal skins, like a cellular automat. I'd like it improved.

#Create Matrix with random numbers 0/1 with a 50% chance
createMatrix <- function(br,gen) {
mat <- matrix(rbinom(br*gen,1,0.5),br,gen)
    return(mat)
}

nextMatrix <- function(mat,w) {

    #Make that Matrix continuous
    wideMatrix <- cbind(mat,mat,mat)
    bigMatrix <- rbind(wideMatrix,wideMatrix,wideMatrix)

    newMat <- mat

    for(j in 1:nrow(mat)){
        for(i in 1:ncol(mat)){
            act <- sum(bigMatrix[(197+j):(203+j),(197+i):(203+i)]) #radius of 3
            inh <- sum(bigMatrix[(195+j):(206+j),(195+i):(206+i)])*w #radius of 6
            if(act>inh) { newMat[j,i] <- 1 }
            if(act<inh) { newMat[j,i] <- 0 }
        }
    }
    return(newMat)
}

x<- createMatrix(200,200)

for(i in 1:5) {
    x <- nextMatrix(x,0.33)
}

image(x, axes=FALSE,col = c("black","darkgoldenrod"))

In particular, the part where I sort out the part with the radius within a matrix:

 act <- sum(bigMatrix[(197+j):(203+j),(197+i):(203+i)])
 inh <- sum(bigMatrix[(195+j):(206+j),(195+i):(206+i)])*w

Is there any package I could use to get a "real radius" (it is a quadrat in my example)?

You can use roll_sum from the RcppRoll package to calculate rolling sums. That way I got an 80-fold speed increase.

Also from a memory perspective, your bigMatrix can be a lot smaller.

Below is my version of nextMatrix.

require(RcppRoll)
nextMatrix2 <- function(mat,w) {
  # Make continuous matrix only as large as necessary
  wideMatrix <- cbind(mat[, ncol(mat)-4:0], mat, mat[, 1:6])
  bigMatrix <- rbind(wideMatrix[nrow(mat)-4:0, ] , wideMatrix, wideMatrix[1:6, ])
  # use roll_sum from RcppRoll to get act/inh as matrices
  actMat <- roll_sum(roll_sum(bigMatrix[3:208, 3:208], 7), 7, by.column=FALSE)
  inhMat <- roll_sum(roll_sum(bigMatrix, 12), 12, by.column=FALSE)*w
  # create the new matrix
  newMat <- actMat > inhMat
  newMat[actMat == inhMat] <- mat[actMat == inhMat]
  # retrun newMat
  return(newMat)
}

And the benchmarktest:

# load benchmarking package
require(microbenchmark)
# create data
x <- createMatrix(200,200)
y <- x
# benchmarktests
microbenchmark(
  x <- nextMatrix(x, 0.33)
  ,
  y <- nextMatrix2(y, 0.33)
)
## Unit: milliseconds
##                      expr        min         lq       mean    median         uq       max neval cld
## x <- nextMatrix(x, 0.33)  545.519907 566.455440 584.396621 577.26897 590.373952 687.29806   100   b
## y <- nextMatrix2(y, 0.33)   5.858406   6.131756   7.214752   6.24489   6.453976  62.63813   100  a
#
# checking for equality
all.equal(x, y)
## [1] TRUE


Programming MySQLi prepared statements I'm programming code with MySQLi prepared statements. I'm a beginner and just want to ask to experts if my code is correct.

function getPaymentMethodName( $id ) {
    global $mysqli;
    if ( $stmt = $mysqli->prepare( 'SELECT name FROM `payment_methods` WHERE id = ?' ) ) {
        if ( $stmt->bind_param( 'i', $id ) ) {
            if ( $stmt->execute() ) {
                $stmt->bind_result( $name);
                if ( $stmt->fetch() ) {
                    $stmt->close();
                    return $name;
                }
            }
        }
    }
    $stmt->close();
    return false;
}

This is really pretty good.
I would do a few things slightly differently.

Don't use globals. Pass the $mysqli parameter into your function. Globals cause nightmares for maintainers.
Do an explicit error check after each mysqli method invocation, and get rid of the cascading if statements. Check for a false return, then issue a message.  In your logic you might also emit the error message and then return false. But you need to announce the error somehow or other or you'll have trouble debugging things.

For example: (Note, using die is a little sloppy, but not as bad as failing to report errors.)

 $stmt = $mysqli->prepare( 'SELECT name FROM `payment_methods` WHERE id = ?' );
 if ($stmt === false) {
         die "prepare failed:" . mysqli_connect_error();

 }


Iteration over an XDocument based on some existing elements and conditions I wrote this piece of code which works as expected:
XElement theContentWhichIncludeBuilds = xDocument.XPathSelectElement($"//Project/Group").Elements()
  .FirstOrDefault(e => e.Name== "Content" && e.Attribute("Included").Value == "buildings");

It targets all the Group elements under the Project nodes,
where its Content has an Include attribute with a value of buildings.
The base sample XML:
<Project>
  <Group>
    <Content Include="buildings"/>

I need to use the provided code on top in 2,3 places in my project, the problem with it is that some XML files that I will process will not have that structure, and I am checking if it has that elements based on the condition or not.
Obviously it may cause errors when it doesn't have that element or the needed architecture. So I need to put it in Try/Catch.
Isn't it better to check each level one-by one and if that existed then check for the next node? (That way also avoid using try/catch)
Does anyone confirm the used method can be good or bad on this case, or can provide any better alternative method for it?
Edit:
The XML files could be similar to the .CSProj structure which I altered a bit for my need.
basically, here we may have:
<Project>
Basic data under the project node, which might be the same in all documents
</Project>

only some may have:
  <Group>
    <Content Include="buildings"/>

Now we want to check if it doesn't have the missed part we be able to add it,
or if it contained the part we may alter its' values.
It is an upgrade process of those documents.

There are several ways how you can process an XML using Linq2XML.
As I can understand you want to process only those XML files that are matching a given format. You can check this with XML native tools, like XSD or DTD schema definitions and validation or manually via XLinq.
If you want to check that with XLinq I would suggest to use the following methods:  Descendants, Elements and Attributes. Each returns those items where the predicate is met. If there is no match, then it will return an empty collection.
If you combine this logic with the Any extension method then you can turn your query operators into existence operators. If the queried collection's length is zero then it returns false otherwise true.

I would suggest to break your logic into two sub-functions:

Check whether or not the given xml meets the desired format
Process only that xml, which satisfies the preconditions

EDIT: To reflect new requirements
Check the presence of the Group element and act based on its existence:
var xml = "<Project><Group><Content Include = \"buildings\" /></Group></Project>";
var xDocument = XDocument.Parse(xml);
var project = xDocument.Root;
var isGroupExist = project.Elements("Group").Any();
if (!isGroupExist)
{
    var group = new XElement("Group") { ... };
    project.AddAfterSelf(group);
}
//Process it


List comprehension for 7x7 board I have a 7x7 board and I want to select some elements from it. The elements I want to select are marked o, the other ones are marked x.

Here is the board:

xoxoxox
ooooooo
xooooox
ooooooo
xooooox
ooooooo
xoxoxox

I use indexing starting from 1. The point (1,1) is the top left.

The current list comprehension I have to select the o-elements is the following:

[(r,c) | r<-[1..7], c<-[1..7],
         (not (even r) && not (even c) && r/=1 && r/=7 && c/=1 && c/=7)
         || even r
         || even c]

I am wondering if I can make this list comprehension more simple. The code above is in Haskell, however, pseudo-code or Python list comprehensions are also fine with me.

Since 7 is prime, the following pseudo code works, although it involves some “thinking around the corner”.

[(r,c) | r<-[1..7], c<-[1..7], r * c not in [1, 3, 5, 7, 21, 35, 49]]

A more straightforward way would be

(r /= 1 && r /= 7 && c /= 1 && c /= 7) || even r || even c


Merge all text files in a directory and save a temp file I've coded this function where you read all text file in a directory and you save in a temporary file all values. The text files are x, y and z format. The function returns

the name of the temporary file
the bounding box
the origin (top-left corner)
and the bottom (bottom-right corner).

I wish for some comments or suggestion on how to improve my working code.

import os
import tempfile
import glob

class LaserException(Exception):
    """Laser exception, indicates a laser-related error."""
    pass

sepType = {
        "space": ' ',
        "tab": '\t',
        "comma": ',',
        "colon": ':',
        "semicolon": ';',
        "hyphen": '-',
        "dot": '.'
        }

def tempfile_merge(path,separator,wildcard= '*.txt'):
    file_temp = tempfile.NamedTemporaryFile(delete=False,dir=path)
    name = file_temp.name
    minx = float('+inf')
    maxx = float('-inf')
    miny = float('+inf')
    maxy = float('-inf')
    for file in glob.glob(os.path.join(path,wildcard)):
        for line in open(file, "r"):
            element = line.split(sepType[separator])
            if len(element) < 3:
                raise TypeError("not enough arguments: %s has only %s columns" % (inFile_name_suffix,len(element)))
            try:
                maxx = max(maxx, float(element[0]))
                minx = min(minx, float(element[0]))
                maxy = max(maxy, float(element[1]))
                miny = min(miny, float(element[1]))
            except ValueError:
                raise LaserException("x,y,z are not float-values")
            newelement = " ".join([str(e) for e in element])+ "\n"
            file_temp.write(newelement)
    file_temp.close()
    return(name, ((minx,maxy),(maxx,maxy),(maxx,miny),(minx,miny)),(minx,maxy),(maxx,miny))

Without going into implementation details, I would suggest looking into the following performance optimisations.

Use buffered reads. If you actually read a line at the time it's pretty time consuming.
Use buffered writes. Instead of writing each new line, collect in a buffer and write in chunks.

For coding review comments, this might be applicable.

The method does 3 different things - merging, syntax checking and bounding rectangle. It might be simpler to maintain and extend the code, if this was refactored into minor helper methods.
Based on the method name, I would be be able to guess what it does.
The initial comments say the method calculates the bounding box, but as the files contain 3d data, would it not be more correct to include the z value.
If the method only need to calculate the bounding rectangle (x,y), is there a need for the merged file to contain the third dimension data ?


Are nested fmap calls idiomatic Haskell? Given:

*Main> :t text
text
  :: (Selectable s, StringLike str, Ord str) => s -> Scraper str str

I need to perform "second level" fmap operation:

*Main> let z = fmap (fmap (:[])) text
*Main> :t z
z :: (Selectable s, StringLike a, Ord a) => s -> Scraper a [a]

On one hand z follows API structure, but otherwise I find such code hard to read. It feels like programming against stack-based machine.

Sometimes you can wrap the "nested functors" in a newtype which is itself a functor, use a single fmap on the newtype, and unwrap. In your case:

import Data.Functor.Compose

let z = getCompose . fmap (:[]) . Compose $ text

We are composing the ((->) s) and Scraper a functors.

The problem with this is that it trades newtype noise for nested fmap noise.