User-tracking history query I have a query consisting of UNIONs.

 SELECT CONCAT("You sent track request to ",u.username) AS ACTION, h.`dateTime`  FROM login_user u
LEFT JOIN
history h ON u.id = h.`toUserId` WHERE h.`fromUserId`=28 AND h.`Action`="Request"
UNION
SELECT CONCAT("You Accepted track request from ",u.username) AS ACTION, h.`dateTime`  FROM login_user u
LEFT JOIN
history h ON u.id = h.`toUserId` WHERE h.`fromUserId`=28 AND h.`Action`="Accept"
UNION
SELECT CONCAT("You Denied track request from ",u.username) AS ACTION, h.`dateTime`  FROM login_user u
LEFT JOIN
history h ON u.id = h.`toUserId` WHERE h.`fromUserId`=28 AND h.`Action`="Deny"
UNION
SELECT CONCAT(u.username, " sent you Track Request") AS ACTION, h.`dateTime`  FROM login_user u
LEFT JOIN
 history h ON u.id = h.FromUserId WHERE h.`toUserId`=28 AND h.`Action`="Request"
  UNION
 SELECT CONCAT(u.username, " accepted your track request") AS ACTION, h.`dateTime`  FROM login_user u
 LEFT JOIN
 history h ON u.id = h.`fromUserId` WHERE h.`toUserId`=28 AND h.`Action`="Accept"
 UNION
 SELECT CONCAT(u.username, " Denied your track request") AS ACTION, h.`dateTime`  FROM login_user u
 LEFT JOIN
 history h ON u.id = h.`fromUserId` WHERE h.`toUserId`=28 AND h.`Action`="Deny"

Since all the constituent queries are almost same, I was wondering if I can use CASE or something to shorten and optimize the query?

These constituent queries will never generate overlapping results, since their ACTION columns are all distinct.  Therefore, you should be using UNION ALL instead of UNION to avoid the deduplication effort.

Also, I suspect you should be using INNER JOIN instead of LEFT JOIN because I doubt that you are interested in historical events that didn't occur.

For portability, string literals should be written using single quotes, like 'Request', rather than "Request".  Allowing double quotes for string literals is a MySQLism — standard SQL uses double quotes for identifiers, the way MySQL uses backticks.

Take care to be consistent in your indentation and capitalization.  You wrote h.FromUserId instead of h.`fromUserId` in one case.  You also call the result column ACTION, which is capitalized differently from history.Action.  The result also had "Accepted", "Denied", and "Track Request" oddly capitalized in various places.

Anyway, to address your specific concern, I think that this formulation using CASE could work for you.

SELECT Action, dateTime
    FROM (
        SELECT CASE
               WHEN h.Action = 'Request' THEN CONCAT('You sent track request to ', u.username)
               WHEN h.Action = 'Accept'  THEN CONCAT('You accepted track request from ', u.username)
               WHEN h.Action = 'Deny'    THEN CONCAT('You denied track request  from ', u.username)
               END CASE AS Action
             , h.dateTime
             , h.fromUserId AS self
            FROM history h
                INNER JOIN login_user u
                    ON h.toUserId = u.id
            WHERE h.Action IN ('Request', 'Accept', 'Deny')    -- possibly superfluous
        UNION ALL
        SELECT CASE
               WHEN h.Action = 'Request' THEN CONCAT(u.username, ' sent you track request')
               WHEN h.Action = 'Accept'  THEN CONCAT(u.username, ' accepted your track request')
               WHEN h.Action = 'Deny'    THEN CONCAT(u.username, ' denied your track request')
               END CASE AS Action
             , h.dateTime
             , h.toUserId AS self
            FROM history h
                INNER JOIN login_user u
                    ON h.fromUserId = u.id
            WHERE h.Action IN ('Request', 'Accept', 'Deny')    -- possibly superfluous
    ) AS msg
    WHERE self = 28
    ORDER BY dateTime;


Securing an Image Upload Script How well does this code limit security problems when uploading images to the server?

<?php

$filename = $_FILE['inputfile']['name'];
$upload_path = 'images/';
//extract extension
$ext = strtolower(substr($filename, strrpos($filename, '.') + 1));

// return mime type ala mimetype extension
$finfo = finfo_open(FILEINFO_MIME_TYPE);

//get MIME type of the given file
$mime = finfo_file($finfo, $filename);
//close finfo
finfo_close($finfo);

if (is_uploaded_file($_FILES['inputfile']['tmp_name'])) {
    //first check: file extension and mime type
    if(!in_array($ext, array('jpeg', 'jpg', 'gif', 'png')) && !in_array($mime, array('image/jpeg', 'image/gif', 'image/png')) ){
     die("Error1: Invalid Image type");
    }

    //create image according to MIME type

    //first: JPEG and JPG
    if($ext == 'jpeg' || $ext == 'jpg'){
       //move_uploaded_file ignored because no need to use it
       $im = @imagecreatefromjpeg($_FILE['inputfile']['tmp_name']);
       if($im){
          $createimage = imagejpeg($im, $upload_path.$_FILE['inputfile']['name']);
          if(!$createimage){
            die("Error3: Can't create image!");
          }
          //last check
          $filecontent = file_get_contents($upload_path.$_FILE['inputfile']['name']);

          //clean the file from any php code
          //changed
          while(preg_match('#<\?php|\?>|<\?#i', $filecontent)){
            $filecontent = str_replace(array("<?php", "<?", "?>"), "", $filecontent);
          }
          $handle = fopen($upload_path.$_FILE['inputfile']['name'], "wb");
          fwrite($handle, $filecontent);
          fclose($handle);
       }
       else{

        die("Error2: Invalid Image Detected");

       }
    }

}
?>

Edit:

          while(preg_match('#<\?php|\?>|<\?#i', $filecontent)){
            $filecontent = str_replace(array("<?php", "<?", "?>"), "", $filecontent);
          }

This is a bad example of correcting user input:

$filecontent = str_replace(array("<?php", "<?", "?>"), "", $filecontent);

I could still upload an image with the following comment in it:

<?<?phpphp echo "this code gets executed"; ??>>

because after your str_replace the comment would be

<?php echo "this code gets executed"; ?>

Instead of correcting user input, you should simply search for these strings reject and the image if it contains <?. Why should you save the image anyway if you suspect an hacker attack? ;-)

Edit: This would be better

// test for opening php tags
// including asp-style opening tags (http://www.php.net/manual/en/ini.core.php#ini.asp-tags)
if(strpos($filecontent, "<?") !== false || strpos($filecontent, "<%") !== false) {
  // stop upload
}

Edit:
And one other thing I forgot to mention. Never use the original filename of the upload without validating it!

Because you could receive a request like this:

POST /upload.php HTTP/1.1
Host: localhost
Content-Length: 318
Content-Type: multipart/form-data; boundary=----WebKitFormBoundaryePkpFF7tjBAqx29L

------WebKitFormBoundaryePkpFF7tjBAqx29L
Content-Disposition: form-data; name="uploadedfile"; filename="../hello.jpg"
Content-Type: image/jpeg

<?php echo "evil code"; ?>
------WebKitFormBoundaryePkpFF7tjBAqx29L

And the "image" file gets saved in the directory above.

Better generate your own file names.


Parsing a slow-loading webpage with scrapy in combination with selenium I've written a scraper in Python scrapy in combination with selenium to scrape 1000 company names and their revenue from a website. The site has got lazy-loading method enabled so it is not possible to make the site load all the items unless the scraper is able to scroll that page downmost. However, my scraper can reach the lowest portion of this webpage and parse the aforesaid category flawlessly. I've set explicit wait in my scraper instead of any hardcoded delay so that it doesn't take longer than necessary.

As this is my first time to work with selenium along with scrapy, there might be scopes to do betterment of this script to make it more robust.

import scrapy
from selenium import webdriver
from selenium import webdriver
from selenium.webdriver.common.by import By
from selenium.webdriver.support.ui import WebDriverWait
from selenium.webdriver.support import expected_conditions as EC

class ProductSpider(scrapy.Spider):
    name = "productsp"
    start_urls = ['http://fortune.com/fortune500/list/']

    def __init__(self):
        self.driver = webdriver.Chrome()
        self.wait = WebDriverWait(self.driver, 10)

    def parse(self, response):
        self.driver.get(response.url)
        check_height = self.driver.execute_script("return document.body.scrollHeight;")
        while True:
            self.driver.execute_script("window.scrollTo(0, document.body.scrollHeight);")
            try:
                self.wait.until(lambda driver: self.driver.execute_script("return document.body.scrollHeight;")  > check_height)
                check_height = self.driver.execute_script("return document.body.scrollHeight;")
            except:
                 break

        for item in self.driver.find_elements_by_css_selector(".row"):
            name = item.find_element_by_css_selector(".company-title").text
            revenue = item.find_element_by_css_selector(".company-revenue").text
            yield {"Title":name,"Revenue":revenue}

The spider is readable and understandable. I would only extract some of the things into separate methods for readability. For example, the "infinite scroll" should probably be just defined in a separate method.

And, the bare except can be replaced with handling a more specific TimeoutException:

def scroll_until_loaded(self):
    check_height = self.driver.execute_script("return document.body.scrollHeight;")
    while True:
        self.driver.execute_script("window.scrollTo(0, document.body.scrollHeight);")
        try:
            self.wait.until(lambda driver: self.driver.execute_script("return document.body.scrollHeight;")  > check_height)
            check_height = self.driver.execute_script("return document.body.scrollHeight;")
        except TimeoutException:
             break

def parse(self, response):
    self.driver.get(response.url)
    self.scroll_until_loaded()

    for item in self.driver.find_elements_by_css_selector(".row"):
        name = item.find_element_by_css_selector(".company-title").text
        revenue = item.find_element_by_css_selector(".company-revenue").text
        yield {"Title":name,"Revenue":revenue}


Trying to improve a working regex I am extracting data from a text file. Some of the lines from which I want to extract the data consist of a text description with single spaces, followed by a multiple-space gap preceding four fields containing the data, each separated by multiple spaces. A field might either contain the indicator "N/A" or else it will contain an integer < 10,000 (possibly comma-ed) such as 15 or 7,151 followed by a valid percentage in parentheses. The percentage will always have a single decimal point; for example, (0.0%) or (19.8%) or (100.0%) . If the field contains "N/A", then I want to write out "NA,NA" and if the field contains a number and percentage, then I want to write those two values out separated by a comma.

At the moment, I use the following regex to describe a single field:

$naNumberGroup = qr/(N\/A|(([0-9]{1,3}(,[0-9]{3})*) \(([0-9]{1,2}\.[0-9])\%\)))/

and then the following code to get the various pieces from the current line, which is $line :

if( $line =~ m/$naNumberGroup +$naNumberGroup +$naNumberGroup +$naNumberGroup/ ) {
 if("N/A" eq $1) {
  print "NA,NA";
 } else {
  print ",$3,$5";
 }

 if("N/A" eq $6) {
  print ",NA,NA";
 } else {
  print ",$8,$10";
 }

 if("N/A" eq $11) {
  print ",NA,NA";
 } else {
  print ",$13,$15";
 }

 if("N/A" eq $16) {
  print ",NA,NA";
 } else {
  print ",$18,$20";
 }
 print "\n";
}

It seems horribly clumsy; for example, it's easy to make a mistake in counting the parentheses and getting the pairs of fields correctly referenced ... but I am unsure of even what sorts of things I should be looking at to improve it (assuming that's possible). I would appreciate some guidance or comment. Even, "it seems fine" would at least indicate that I shouldn't waste time on improving it!

An example line of text is:

Adults who actively pursue work opportunities
             197 (82.8%)              30 (12.6%)                  N/A                     N/A

The description at the beginning of the line changes depending on the data. The output that I want for this line is:

197,82.8,30,12,6,NA,NA,NA,NA

Similarly, if the line were:

Adults who actively pursue work opportunities
             197 (82.8%)              N/A              30 (12.6%)                  N/A

then I want the output:

197,82.8,NA,NA,30,12.6,NA,NA

Using "split" is an option as other people have mentioned.  However, using a regex has the added benefit of validating the input data while parsing, so is still worth considering depending on your use-case.

A regex is better used in loop here since we're matching the same pattern repeatedly.  And you should use non-capturing parentheses for the bits you're not interested in capturing. E.g. changing nothing else your code would look like this:

$naNumberGroup = qr/(N\/A|(?:([0-9]{1,3}(?:,[0-9]{3})*) \(([0-9]{1,2}\.[0-9])\%\)))/;
my @outFields;
while ($line =~ m/\s\s+$naNumberGroup/g) {
    if("N/A" eq $1) {
        push @outFields, 'NA', 'NA';
    } else {
        push @outFields, $2, $3;
    }
}
print join(',', @outfields),"\n";

It's worth noting that your code as-is would preserve any commas in the input, therefore breaking your output.  And "100.0%" isn't handled.

If you're wanting to improve readability and maintainability of your regexes, here are some additional things worth changing:

Use the /x modifier to improve readability/maintainability.
Use more intermediate variables to build your regexes.
Avoid having to escape slashes by using qr{...} instead of qr/.../

E.g.

my $numberGroup = qr{
    (?<number> [0-9,]+ )                # Number with optional commas
    [ ]                                 # Single space
    \( (?<percent> [0-9]+\.[0-9] ) %\) # Percentage in parens
}x;
my $naNumberGroup = qr{
    [ ]{2,}                             # Two or more spaces
    (?: $numberGroup | N/A )            # No need to capture "N/A"
}x;
my @outFields;
while ($line =~ m/$naNumberGroup/xg) {
    my $number  = $+{number}  // 'NA';
    my $percent = $+{percent} // 'NA';
    $number =~ tr/,//d;             # Strip commas
    push @outFields, $number, $percent;
}
if (scalar @outFields == 8) {
    print join(',', @outFields),"\n";
} else {
    # Description line, or invalid line.  You may be able to use
    # another regex to determine which.
}


Excel VBA: Implementing Box Muller, Zigurrat and Ratio of Uniforms Algorithm This is a very specific question mixing up stochastic knowledge and VBA skills. So very exciting!

I'm trying to compare several methods for generating standard, normally distributed numbers given a source of uniformly distributed random numbers. Therefore I'm implementing the Box Muller Algorithm, Ziggurat Algorithm and Ratio of Uniforms Algorithm. Every single implementation works great in terms of generating a clean standard, normally distribution. (checked by Shapiro-Wilk-Test).

What I want to find out: which is the quickest method?

Testing every single program with a total of 10^7 generated numbers these are the run times:

Box-Muller: 3,7 seconds
Ziggurat: 1,28 seconds
Ratio of Uniforms: 10,77 seconds

Actually I am very happy about those readings, because I didn't expect it to be that fast. Of course the run time of every single method also depends on my programming skills and VBA knowledge.

My problem: after doing some research I found out that the Ratio of Uniforms Algorithm should be the quickest (about 3 to 4 times quicker than Box Muller). This information just leans on this stack:

I am curious if this is just a wrong claim of this user or (what I do expect more) if my code is not perfectly implemented. Therefore I'll post my code and hope someone could help me with my question, if my code is just not good enough or if the Ratio of Uniforms just doesn't work that quick as mentioned.

Sub RatioUniforms()

Dim x(10000000) As Double
Dim passing As Long
Dim amount As Long: amount = 10000000
Dim u1 As Double
Dim u2 As Double
Dim v2 As Double

    Do While passing <= amount
        Do
            u1 = Rnd    'rnd= random number(0,1)
        Loop Until u1 <> 0  'u1 musn't become 0
        v2 = Rnd
        u2 = (2 * v2 - 1) * (2 * exp(-1)) ^ (1 / 2)
        If u1 ^ 2 <= exp(-1 / 2 * u2 ^ 2 / u1 ^ 2) Then
            x(passing) = u2 / u1
            passing = passing + 1
        End If
    Loop

 End Sub

Thank you very much helping me on this topic. Maybe some of you have tried those algorithms in VBA or other languages and can help me with there experience about the run time? If you need something else to know about my other implementations just let me know. Have a great day!

There are a few optimizations that can be made to the code to speed it up, mainly to do with how some mathematical operations are performed in VBA. Not sure if I have it implemented 100% accurately so please review for accuracy. Also, I'm sure there are more optimizations to be had, but this is hopefully a starting point for further conversation.

A lot of my changes may be PC specific. It may ultimately depend on your CPU and instruction set available. On my computer, this runs in about 2.5 seconds.

A list of changes:

I replaced all instances of raising something to a power of two, instead, I just multiplied the item by itself.
I pre-computed this part Sqr((2 * Exp(-1))) as it appears to always be the same, so it isn't calculated for each loop and put it into a constant.
I removed the variable v2, it wasn't really needed, you just needed to introduce Rnd in one more spot
General cleanup of the code, and renamed a few variables for clarity

Code

Sub RatioUniforms()
    Const NumberOfIterations        As Long = 10000000
    Const u2CalculationSecondHalf   As Double = 0.857763884960707 'Caching this part Sqr((2 * Exp(-1)))
    Dim Results(NumberOfIterations) As Double
    Dim PassCounter                 As Long
    Dim u1                          As Double 'Define a better name if possible
    Dim u2                          As Double 'Define a better name if possible
    Dim MyTimer                     As Double

    MyTimer = Timer
    Do While PassCounter <= NumberOfIterations
        Do: u1 = Rnd: Loop Until u1 > 0
        u2 = (2 * Rnd - 1) * u2CalculationSecondHalf

        If u1 * u1 <= Exp(-1 / 2 * (u2 * u2) / (u1 * u1)) Then
            Results(PassCounter) = u2 / u1
            PassCounter = PassCounter + 1
        End If

    Loop
    Debug.Print "Process took: " & Timer - MyTimer

 End Sub


Grid column size calculation I have a one function to calculate all the possible widths plus the margins for the gutters.

In this same function, the grid extensions such as the pull, pushand offset functions are also calculated.

@function calculate-one-column() {
  $a: 100%;
  @if ($gutter-width-px > 0px) {
    $a: ($a - ($gutter-width-procent * ($column-count - 1)));
  }
  @return $a / $column-count;
}

@function calculate-each-gutter() {
  @if ($gutter-width-px > 0px) {
    @return $gutter-width-procent;
  }
  @return 0;
}

@function calculate-function($function, $size, $first-child: false, $responsive: false) {
  $each-column: calculate-one-column() * $size;
  $each-gutter: calculate-each-gutter() * ($size - 1);
  @if ($function == 'size') {
    @return $each-column + $each-gutter;

  } @else if ($function == 'offset' and $first-child == false) and ($responsive == false) {
    @return $each-column + $each-gutter + ($gutter-width-procent * 1.5);

  } @else if ($function == 'push' or $function == 'pull') and ($responsive == false) or ($function == 'offset' and $first-child == true) {
    @return $each-column + $each-gutter + $gutter-width-procent;

  } @else if($function == 'offset' or 'push' or 'pull') and ($responsive == true)  {
    @return auto !important;
  }
}

The calculate-function @function is getting called by this piece of code:

@mixin make-grid-sizes() {
  @for $index from 1 through $column-count {
    &.#{$column-size}#{$index} {
      @include calculate-property('width', $index);
    }
  }
}

All of these functions are getting hold together with these variables:

$column-count: 12;
$row-max-width: 1024px;

$gutter-width-px: 15px !default;
$gutter-width-procent: percentage($gutter-width-px / $row-max-width);

The code works perfectly, but in my opinion it looks very messy.

Warning: I am far from an expert with Sass!

I'll list a few things I think could use some re-factoring.

We can DRY out your code. In the main function, I see $each-column + $each-gutter three times. Create a variable and assign the variable the sum.

This line: if ($function == 'offset' and $first-child == false) and ($responsive == false) can all be in one pair of parentheses. Breaking it apart actually reduces clarity.

I find that this line:

if ($function == 'push' or $function == 'pull') and ($responsive == false) or ($function == 'offset' and $first-child == true)

is difficult to read. Note my changes in the code below.

A couple instances, things seem mistaken. For example, this line:

if ($function == 'offset' or 'push' or 'pull')

doesn't seem to be correct. I tried to copy this conditional, and I'm surprised you haven't noticed the error.

This statement says: If $function is equal to 'offset', or either 'push' is non-null or 'pull' is non-null. I believe it should be:

if ($function == 'offset' or $function == 'push' or $function == 'pull') and ($responsive == true) {

I've written your code to take into account all of these modifications. I think it looks neater, however it's not meant to be completely superficial. Since my Sass isn't very strong I hope someone else will be able to help break down your code with you more thoroughly.

@function calculate-function($function, $size, $first-child: false, $responsive: false) {
    $each-column: calculate-one-column() * $size;
    $each-gutter: calculate-each-gutter() * ($size - 1);

    $sum-width: $each-column + $each-gutter;

    $push-or-pull: ($function == 'push' or $function == 'pull');
    $offsetting: $function == 'offset';

    @if ($function == 'size') {
        @return $sum-width;

    } @else if ($offsetting and $first-child == false and $responsive == false) {
        @return $sum-width + ($gutter-width-percent * 1.5);

    } @else if $push-or-pull and ($responsive == false) or ($offsetting and $first-child) {
        @return $sum-width + $gutter-width-percent;

    } @else if ($offsetting or $push-or-pull) and $responsive {
        @return auto !important;
    }
}

Also, you misspelled percent as procent. I suggest you change that in all occurrences.


Image resizing class How does this class to resize an image look?

using System;
using System.Collections.Generic;
using System.Web;
using System.Drawing;
using System.IO;

/*
 * Resizes an image
 **/
public static class ImageResizer
{
    // Saves the image to specific location, save location includes filename
    private static void saveImageToLocation(Image theImage, string saveLocation)
    {
        // Strip the file from the end of the dir
        string saveFolder = Path.GetDirectoryName(saveLocation);
        if (!Directory.Exists(saveFolder))
        {
            Directory.CreateDirectory(saveFolder);
        }
        // Save to disk
        theImage.Save(saveLocation);
    }

    // Resizes the image and saves it to disk.  Save as property is full path including file extension
    public static void resizeImageAndSave(Image ImageToResize, int newWidth, int maxHeight, bool onlyResizeIfWider, string thumbnailSaveAs)
    {
        Image thumbnail = resizeImage(ImageToResize, newWidth, maxHeight, onlyResizeIfWider);
        thumbnail.Save(thumbnailSaveAs);
    }
    // Overload if filepath is passed in
    public static void resizeImageAndSave(string imageLocation, int newWidth, int maxHeight, bool onlyResizeIfWider, string thumbnailSaveAs)
    {
        Image loadedImage = Image.FromFile(imageLocation);
        Image thumbnail = resizeImage(loadedImage, newWidth, maxHeight, onlyResizeIfWider);

        saveImageToLocation(thumbnail, thumbnailSaveAs);
    }

    // Returns the thumbnail image when an image object is passed in
    public static Image resizeImage(Image ImageToResize, int newWidth, int maxHeight, bool onlyResizeIfWider)
    {
        // Prevent using images internal thumbnail
        ImageToResize.RotateFlip(System.Drawing.RotateFlipType.Rotate180FlipNone);
        ImageToResize.RotateFlip(System.Drawing.RotateFlipType.Rotate180FlipNone);

        // Set new width if in bounds
        if (onlyResizeIfWider)
        {
            if (ImageToResize.Width <= newWidth)
            {
                newWidth = ImageToResize.Width;
            }
        }

        // Calculate new height
        int newHeight = ImageToResize.Height * newWidth / ImageToResize.Width;
        if (newHeight > maxHeight)
        {
            // Resize with height instead
            newWidth = ImageToResize.Width * maxHeight / ImageToResize.Height;
            newHeight = maxHeight;
        }

        // Create the new image
        Image resizedImage = ImageToResize.GetThumbnailImage(newWidth, newHeight, null, IntPtr.Zero);

        // Clear handle to original file so that we can overwrite it if necessary
        ImageToResize.Dispose();

        return resizedImage;
    }
    // Overload if file path is passed in instead
    public static Image resizeImage(string imageLocation, int newWidth, int maxHeight, bool onlyResizeIfWider)
    {
        Image loadedImage = Image.FromFile(imageLocation);
        return resizeImage(loadedImage, newWidth, maxHeight, onlyResizeIfWider);
    }
}

PascalCase the method names and method params if you are feeling overly ambitious.

    // Set new width if in bounds
    if (onlyResizeIfWider)
    {
        if (ImageToResize.Width <= newWidth)
        {
            newWidth = ImageToResize.Width;
        }
    }

FindBugs barks in Java for the above behavior... refactor into a single if since you are not doing anything within the first if anyways...

    // Set new width if in bounds
    if (onlyResizeIfWider && ImageToResize.Width <= newWidth)
    {
        newWidth = ImageToResize.Width;
    }

Comments here could be a bit more descriptive; while you state what the end result is I am still lost as to why that would resolve the issue.

    // Prevent using images internal thumbnail
    ImageToResize.RotateFlip(System.Drawing.RotateFlipType.Rotate180FlipNone);
    ImageToResize.RotateFlip(System.Drawing.RotateFlipType.Rotate180FlipNone);

Maybe something similar to what is stated on this blog...

    // Prevent using images internal thumbnail since we scale above 200px; flipping
    // the image twice we get a new image identical to the original one but without the
    // embedded thumbnail
    ImageToResize.RotateFlip(System.Drawing.RotateFlipType.Rotate180FlipNone);
    ImageToResize.RotateFlip(System.Drawing.RotateFlipType.Rotate180FlipNone);


Draw an ASCII checkerboard I'm a newcomer to C++, from C#. I wrote a program that prints an ASCII checkerboard with a side length of the number passed to it (link to original Code Golf question).

stdafx.h

#include "targetver.h"

#include <stdio.h>
#include <tchar.h>

#include <iostream>
#include <sstream>
#include <string>

Checkers.cpp

#include "stdafx.h"

using std::cin;
using std::cout;
using std::endl;
using std::stoi;
using std::string;
using std::stringstream;

string header(int num);

template <bool white>
string body(const string &head, int num);

template <bool white>
stringstream& append_body(stringstream &stream, int rem, int approx);

int _tmain(int argc, _TCHAR* argv[])
{
    int num;

    if (argc == 1 || ((num = stoi(argv[1])) < 0))
    {
        cout << "Usage: ./Checkers [nonnegative int]" << endl;
        return 1;
    }

    switch (num)
    {
    case 0:
        return 0;
    case 1:
        cout << "+--+" << endl << "|  |" << endl << \
            "|  |" << endl << "+--+" << endl;
        return 0;
    }

    int rem = num % 2;
    int approx = num - rem;

    const string& head = header(num);
    string& wtail = body<true>(head, num);
    string& btail = body<false>(head, num);

    string& block = wtail + btail;

    stringstream stream;

    for (int i = 0; i < approx / 2; i++)
        cout << block;

    if (rem == 1)
        cout << wtail;

    cout << head << endl;

#ifdef _DEBUG
    cin.get();
#endif
}

string header(int num)
{
    string result;

    for (int i = 0; i < num; i++)
        result += "+--";

    return result + '+';
}

template <bool white>
string body(const string &head, int num)
{
    int rem = num % 2;
    int approx = num - rem;

    stringstream stream;
    stream << head << endl;

    append_body<white>(stream, rem, approx) << endl;
    append_body<white>(stream, rem, approx) << endl;

    return stream.str();
}

template <bool white>
stringstream& append_body(stringstream &stream, int rem, int approx)
{
    const char *const block =
        white ? "|  |##" : "|##|  ";

    for (int i = 0; i < approx / 2; i++)
        stream << block;

    stream << '|';

    if (rem == 1)
        stream << (white ? "  |" : "##|");

    return stream; // not a local so this is OK
}

Questions:

I intentionally had a little fun optimizing prematurely here, i.e. generating the header ("+--+") and body ("|##|  |") strings only once instead of every loop. What could I do to increase readability without getting rid of each-loop optimizations like this?
When is it okay to use non-type template parameters, like I did in body? This means that you always have to rewrite your code when you want to offer a non-template alternative, correct?

To appreciate a power of non-type templates, try to implement body and append_body in plain C. You will see immediately why your approach is an abuse of templates.
Most important

body() returns a temporary object:

    stringstream stream;
    ...
    return stream.str();

and clang expectedly complains:

error: non-const lvalue reference to type
  'basic_string<[3 * ...]>' cannot bind to a temporary of type
  'basic_string<[3 * ...]>'
string& wtail = body<true>(head, num);
        ^       ~~~~~~~~~~~~~~~~~~~~~


Reversing an array recursively Implementation:

void ReverseArray<T>(T[] a)
{
    ReverseArray(a,0,a.Length-1);
}

void ReverseArray<T>(T[] a,int lo,int hi)
{
    if(lo >= hi) return;

    // swap
    var temp = a[lo];
    a[lo] = a[hi];
    a[hi] = temp;

    ReverseArray(a,lo+1,hi-1);
}

Usage:

int[] arr = {1,2,3,4,5,6,7,8};
ReverseArray(arr); // reverses the array

How can I improve this?

You can improve it by making it a loop instead of a recursive call. There is no need for the recursion, and it limits you in ways that a loop would not (for example, you are limited by your stack size with recursion).

Additionally, the loop implementation is simpler, and probably a bunch faster too.

If you do stick with the recursive call, you should make the recursive method private too.

The loop could be as simple as:

void ReverseArray<T>(T[] a)
{
    for (int lo = 0, hi = a.Length - 1; lo < hi; lo++, hi--)
    {
        var temp = a[lo];
        a[lo] = a[hi];
        a[hi] = temp;
    }
}

Normally I would take issue with the short variable names, but, in this context, with the generic array data type, a is not too bad of a name.

The only style issue I have other than the a, is that you need space around the operators in your conditions and assignments. The code:

ReverseArray(a,0,a.Length-1);
....
ReverseArray(a,lo+1,hi-1);

should be:

ReverseArray(a, 0, a.Length - 1);
....
ReverseArray(a, lo + 1, hi - 1);

Note, that if the recursive call is required as part of an exercise, then it is implemented correctly/functionally, and I can't see any problems other than the limited size of the array (I would guess about 40,000 elements is too many to handle).


Improve pandigital algorithm First of all, I am a beginner in Python trying to learn optimizations and proper coding standards.

Here is a snippet of code that checks if a number follows the rule: first AND last N digits contain all unique numbers from 1 to N, but in any order. This also means that in either first N or last N digits, each number appears exactly once.

import time

start_time = time.time()

def is_pandigital(nr, n):
    digits = ''.join(map(str, range(1, n + 1)))
    nr = str(nr)
    for i in digits:
        if str(i) not in nr[0:9]:
            return False
        if str(i) not in nr[-9:]:
            return False

    return True

assert is_pandigital(1423, 4) is True
assert is_pandigital(1423, 5) is False
assert is_pandigital(14235554123, 4) is True
assert is_pandigital(14235552222, 4) is False # !important
assert is_pandigital(1444, 4) is False
assert is_pandigital(123564987, 9) is True

pandigitals = []
# this loop is strictly for benchmarking is_pandigital
for i in range(100000, 999999):
    if is_pandigital(i, 6):
        pandigitals.append(i)

print pandigitals

print time.time() - start_time, "seconds"

When running this, the result is:

[123456, .......]
2.968 seconds

Process finished with exit code 0

The code seems to work fine, but it doesn't appear to be very efficient. Would you have any tips to improve it?  Any piece of code and/or idea would be highly appreciated.

PS: I chose this loop so that any improvements to the is_pandigital function would be immediately obvious.

Example of numbers I am looking for:

321XXXXXXXXXXXXXXX132 - good

321XXXXXXXXXXXXXXX133 - not good, because the last 3 digits dont contain 1, 2 and 3

231 - good

You can rewrite your asserts without comparing to True and False:

assert is_pandigital(1423, 4)
assert not is_pandigital(1423, 5)
assert is_pandigital(14235554123, 4)
assert not is_pandigital(14235552222, 4) # !important
assert not is_pandigital(1444, 4)
assert is_pandigital(123564987, 9)

You can rewrite your benchmark with list comprehension:

# this loop is strictly for benchmarking is_pandigital
pandigitals = [i for i in range(100000, 999999) if is_pandigital(i, 6)]

Now for the algorithm itself, I must confess that I have troubles understanding what you want to do as you seem to be calling "pandigital" two different things. In any case, I have the feeling that something is wrong in your code, it seems like :

   if str(i) not in nr[0:9]:
        return False
    if str(i) not in nr[-9:]:
        return False

should be

    if str(i) not in nr[0:n]:
        return False
    if str(i) not in nr[-n:]:
        return False

and

assert not is_pandigital(9999912399999, 3)

should provide you some hints.

I'll go deeper in the code once you confirm that my understanding is correct.

Edit : I have to go, no time to run benchmarks but here are the improvements. I kept different versions to that you can take ideas out of it.

def is_pandigital(nr, n):
    nr = str(nr)
    beg=nr[0:n]
    end=nr[-n:]
    for i in map(str, range(1, n + 1)):
        if i not in beg or i not in end:
            return False
    return True

def is_pandigital(nr, n):
    nr = str(nr)
    beg=set(nr[0:n])
    end=set(nr[-n:])
    return beg==end and beg==set(map(str, range(1, n + 1)))


Repository Pattern Review I have following code for a library management system.

Is the following code a proper implementation of the Repository Pattern?
How can I adapt this code to use generics?
Should I let MyOracleReservationRepository and MyOracleBookRepository be  DataContracts if I am consuming this code as a WCF service (business Layer will be called by service layer)?

Business Layer

namespace LibraryBL
{
    public class ReservationManager
    {
        //LibraryDAL.ReservationDAL resDAL = new LibraryDAL.ReservationDAL();
        //LibraryDAL.BookDAL bookDAL = new LibraryDAL.BookDAL();

        LibraryRepository.IReservationRepository reservationRepository;
        LibraryRepository.IBookRepository bookRepository;

        public ReservationManager(LibraryRepository.IReservationRepository resRepositroy, LibraryRepository.IBookRepository bookRepositroy)
        {
            reservationRepository = resRepositroy;
            bookRepository = bookRepositroy;
        }

        public List<LibraryDTO.Reservation> GetAllReservations()
        {

            List<LibraryDTO.Reservation> allReservations = reservationRepository.GetAllReservations();

            //Book object inside allReservations has two values as NULL (author and BookTitile).
            //These values need to be set using foreach loop
            foreach (LibraryDTO.Reservation reservation in allReservations)
            {
                int bookID =reservation.ReservedBook.BookID;
                LibraryDTO.Book book = bookRepository.GetBookByID(bookID);
                reservation.ReservedBook = book;
            }
            return allReservations;
        }

    }

}

Repository

namespace LibraryRepository
{
    public interface IReservationRepository
    {
        List<LibraryDTO.Reservation> GetAllReservations();
    }

    public interface IBookRepository
    {
        LibraryDTO.Book GetBookByID(int bookID);
    }

    public class MyOracleReservationRepository : IReservationRepository
    {
        public List<LibraryDTO.Reservation> GetAllReservations()
        {
            int databaseValueResID1 = 1;
            DateTime databaseValueResDate1 = System.Convert.ToDateTime("1/1/2001");
            int databaseValueResBookID1 = 101;

            List<LibraryDTO.Reservation> reservations = new List<LibraryDTO.Reservation>();

            LibraryDTO.Reservation res = new LibraryDTO.Reservation();
            res.ReservationID = databaseValueResID1;
            res.ReservedDate = databaseValueResDate1;
            res.ReservedBook = new LibraryDTO.Book();
            res.ReservedBook.BookID = databaseValueResBookID1;
            res.ReservedBook.Author = null; //Set as null as we don't have values in Reservation DAL
            res.ReservedBook.BookTitle = null; //Set as null as we don't have values in Reservation DAL

            reservations.Add(res);
            return reservations;
        }
    }

    public class MyOracleBookRepository : IBookRepository
    {
        public LibraryDTO.Book GetBookByID(int bookID)
        {
            LibraryDTO.Book book = null;
            if (bookID == 101)
            {
                book = new LibraryDTO.Book();
                book.BookID = 101;
                book.Author = "First Author";
                book.BookTitle = "Book 1";
            }
            return book;
        }
    }
}

That is a very good start.  However, look into moving the loop to get all the books into the GetAllReservations() method in the concrete class that implements IReservationRepository.  When I see the method name "GetAllReservations" in the interface IReservationRepository I would expect it would return all the reservation DTOs fully populated.  You can still keep the logic separate in the repository, it would just have a call to GetByBookId method.  This means the constructor on MyOracleReservationRepository would have to change, but you can hide that in a factory.

namespace LibraryRepository
{
    public interface IReservationRepository
    {
        List<LibraryDTO.Reservation> GetAllReservations();
    }

    public interface IBookRepository
    {
        LibraryDTO.Book GetBookByID(int bookID);
    }

    public class LibraryRepositoryFactory
    {
        public virtual IReservationRepository GetReservationRepository()
        {
            IBookRepository bookRepo = GetBookRepository();

            return new MyOracleReservationRepository(bookRepo);
        }

        public virtual IBookRepository GetBookRepository()
        {
            return new MyOracleBookRepository();
        }
    }

    public class MyOracleReservationRepository : IReservationRepository
    {
        private IBookRepository m_BookRepo;

        public MyOracleReservationRepository(IBookRepository bookRepo)
        {
            m_BookRepo = bookRepo;
        }

        public List<LibraryDTO.Reservation> GetAllReservations()
        {
            List<LibraryDTO.Reservation> reservations = GetReservationsFromDatabase();

            foreach (LibraryDTO.Reservation reservation in allReservations)
            {
                int bookID =reservation.ReservedBook.BookID;
                LibraryDTO.Book book = bookRepository.GetBookByID(bookID);
                reservation.ReservedBook = book;
            }
            return allReservations;
        }

        private List<LibraryDTO.Reservation> GetReservationsFromDatabase()
        {
            int databaseValueResID1 = 1;
            DateTime databaseValueResDate1 = System.Convert.ToDateTime("1/1/2001");
            int databaseValueResBookID1 = 101;

            List<LibraryDTO.Reservation> reservations = new List<LibraryDTO.Reservation>();

            LibraryDTO.Reservation res = new LibraryDTO.Reservation();
            res.ReservationID = databaseValueResID1;
            res.ReservedDate = databaseValueResDate1;
            res.ReservedBook = new LibraryDTO.Book();
            res.ReservedBook.BookID = databaseValueResBookID1;
            res.ReservedBook.Author = null; //Set as null as we don't have values in Reservation DAL
            res.ReservedBook.BookTitle = null; //Set as null as we don't have values in Reservation DAL

            reservations.Add(res);
            return reservations;
        }
    }

    public class MyOracleBookRepository : IBookRepository
    {
        public LibraryDTO.Book GetBookByID(int bookID)
        {
            LibraryDTO.Book book = null;
            if (bookID == 101)
            {
                book = new LibraryDTO.Book();
                book.BookID = 101;
                book.Author = "First Author";
                book.BookTitle = "Book 1";
            }
            return book;
        }
    }
}

Now your ReservationManager would look something like this:

namespace LibraryBL
{
    public class LibraryBLFactory
    {
        public ReservationManager GetReservationManager()
        {
            var repoFactory = new LibraryRepository.LibraryRepositoryFactory();
            LibraryRepository.IReservationRepository reservationRepo = repoFactory.GetReservationRepository();

            return new ReservationManager(reservationRepo);
        }
    }

    public class ReservationManager
    {
        LibraryRepository.IReservationRepository reservationRepository;

        public ReservationManager(LibraryRepository.IReservationRepository resRepositroy)
        {
            reservationRepository = resRepositroy;
        }

        public List<LibraryDTO.Reservation> GetAllReservations()
        {
            return reservationRepository.GetAllReservations();
        }

    }

}

I am sure there is a bit of refactoring that can be done but hopefully you get the idea.

The idea of the repository is to hide how the data is stored and retrieved from other components.  In this particular example ReservationManager.  How would the ReservationManager know that after it got all the reservations it would need to go back to the repository and load all the books?  What if there was another consumer of the repository?  How would it know to do that?  The fact you added a comment to indicate why a for loop is needed should be an indication the loop was not in the right place.

As for your other questions.  I am not sure what your intention is for Generics, I cannot see a good place where it could be used.  As for using data contracts, I would not be in favor of that.  The LibraryBL namespace should exist in it's own set of class libraries another service library should invoke through interfaces.  The service library should be the one consumed directly by the WCF service.  And it would map your DTOs to the Data Contracts.  This will keep your code decoupled, because if you ever had to change your logic in LibraryBL you wouldn't really have to worry about how it affects the WCF service, unless you alter a property in the DTOs, but then you would only have to worry about the mapping.


Conditional logic to determine if popup menu should be visible I have this small piece of code written in a class. I am looking for a nicer way to write this, preferably avoiding nested if statements like what I currently have:

@Override
  public void popupMenuWillBecomeVisible(PopupMenuEvent e)
  {
    TheatreListDataWrapper[] selectedOperations = getSelectedDatas();
    boolean isOperationFromMulitpleSoc = TheatreListDataWrapper.operationsOfMultiplePatients(selectedOperations);
    if (!isOperationFromMulitpleSoc)
    {
      //passing boolean literal value 'false' instead of variable 'isOperationFromMultipleSoc' since that,
      //at this point it is certain that the operations selected are from a single patient
      CambioAction attentionSignalAction = AttentionSignalDelegator.getAttentionSignalAction(false,selectedOperations);
      if (attentionSignalAction != null)
      {
        if (Framework.getInstance().getActiveSubjectOfCare().id == null
            || !getAcuteListTable().getTableDataByRow(getAcuteListTable().getSelectedRow()).socId.equals(Framework
                .getInstance().getActiveSubjectOfCare().id))
        {
          attentionSignalAction.setEnabled(false);
        }
        else
        {
          attentionSignalAction.setEnabled(true);
        }
      }
    }

    if(selectedOperations != null && selectedOperations.length > 1){
        listRegistraionOpenMenuItem.setEnabled(false);
    }else{
        listRegistraionOpenMenuItem.setEnabled(true);
    }
  }

For me, using that last if checking seems like, I could do it in a better way, using a switch or some other control structure. Is there a way to avoid the null checks?

You may not need another if else to setEnabled. The expression within if returns a boolean. Just set the inverse of it to setEnabled. You can pull it into a variable in case of better readability.

Updated:

if (attentionSignalAction != null) {
  bool isActiveRowSelected = (Framework.getInstance().getActiveSubjectOfCare().id == null||!getAcuteListTable().getTableDataByRow(getAcuteListTable().getSelectedRow()).socId.equals(Framework.getInstance().getActiveSubjectOfCare().id));
  attentionSignalAction.setEnabled(!isActiveRowSelected);
}

Update 2

if(selectedOperations != null && selectedOperations.length > 1){
    listRegistraionOpenMenuItem.setEnabled(false);
}else{
    listRegistraionOpenMenuItem.setEnabled(true);
}

can also be reduced to

bool operationsSelected = (selectedOperations != null && selectedOperations.length > 1);
listRegistraionOpenMenuItem.setEnabled(!operationsSelected);


Redistributing times Original problem:
Given \$x\$ number of tests that should run in \$y\$ number of seconds shift the time per test so that each test takes a little longer than the next.
Example: If running 10 tests over 10 seconds, the equal time per test would be 1 second each.
Instead, have the first test take .5 seconds and the last test take 1.5 seconds while each subsequent test take a progressively longer duration.  We shift the time per test around while still maintaining the original duration of 10 seconds for the 10 tests.
I am slightly ignorant of what math principles are occurring.  I knew how I wanted to solve the problem but I can't describe the problem. I've made it possible to adjust how much shifting can be done.  Since the array of times is linear (and we can't really have negative times) I put a limit on that 'shifting' value.

How do you describe this function / math problem?  (which would hopefully result in a better name for it).
What should the pct variable be called?  I originally thought it to be a percentage of each second, but as I developed the function I realized this was inaccurate; since after the first value, the percentage of shifting per time slice goes down then back up again.

def duration_spread(tests: float, seconds: float, pct: float = .75):
    assert -1 <= pct <= 1, "pct must be within +/-100% otherwise it will push times into negative"
    pct *= 2
    original_velocity = seconds/tests
    seconds_per_tests = original_velocity  # couldn't decide on naming convention
    average_velocity = original_velocity
    diff_average_velocity = pct * average_velocity
    acceleration = diff_average_velocity/tests

    multiplier_list = get_multipliers(tests)
    results = []
    for i, multiplier in enumerate(multiplier_list):
        newadjustment = (multiplier * acceleration)
        newtime = original_velocity + newadjustment
        print(f"{original_velocity} -> {newtime} :: {abs(newadjustment)}")
        results.append(newtime)

    return results

def get_multipliers(tests: float):
    odd = int(tests % 2)
    half_tests = int(tests // 2)
    result = list(range(-half_tests, half_tests+1))
    if not odd:
        result.pop(half_tests)
    return result

How do you describe this function / math problem?

The input is a list of tests with the same duration. For example, \$input=[2,2,2,2]\$. Given the indexes from 0 to 3, we can see it in a graph:

So we can define the input as a (discrete) function \$f(x)=2\$, or in general \$f(x) = seconds/tests\$. Where \$0<=x<=tests,x∈Z\$.
The function \$durationSpread\$ can be described as \$durationSpread = g(x) * f(x)\$. Where \$g(x)\$ is a function that depends on \$pct\$ and \$tests\$. In the following I am going to refer it as \$g(pct,tests)\$.
Function analysis
For pct=0 the input remains the same: \$g(pct,tests) * input = g(0,4) * [2,2,2,2] = [2, 2, 2, 2]\$. Meaning that all elements of the input are multiplied by 1. So \$g\$ should look like this:

For pct=-1 the result "should be": \$g(-1,4) * [2,2,2,2] = [4.0, 2.67, 1.33, 0.0]\$. The first element of of the input is multiplied by 2, the last one by 0, and the elements in between by a descending factor.
I said "should be" because your code outputs \$[4.0,3.0,1.0,0.0]\$, which is not possible if we consider a constant factor (the gap betweenn 4.0 and 3.0 is 1, but between 3.0 and 1.0 is 2).
So \$g\$ should look like this:

For pct=1 the result is: \$g(1,4) * [2,2,2,2] = [0.0, 1.33, 2.67, 4.0]\$. In this case, the first element is multiplied by 0 and the last one by 2. So the function \$g\$ is:

How to find \$g\$? From the three examples, we know that \$g\$ is a linear function. A linear function can be represented as y = mx + b. How to find m and b? They can be found given two points.
Let's choose the first point (x0,y0) in the origin. x0 is always 0. While y0 depends on pct. Looking at the examples, we can tell that it's equal to abs(pct - 1). Explanation:

In the first example pct=0 and y0=1. In fact, y0 = abs(0 - 1) = 1
In the second example pct=-1 and y0=2. In fact, y0 = abs(- 1 - 1) = abs(-2) = 2
In the third example pct=1 and y0=0. In fact, y0 = abs(1 - 1) = abs(0) = 0

For the second point (x1,y1), we can choose the center. Where y1 is always 1. While x1 can be found as x1 = (tests-1)/2. (You can double-check in the three examples).
Given two points we can find the slope m as m = (y1 - y0) / (x1 - x0). Once we have m, we calculate the constant b as b = y0 - (m * x0).
Now that we found \$g\$, we can multiply the input by \$g\$.
def duration_spread(tests, seconds, pct):
    x0, y0 = 0, abs(pct - 1)
    x1, y1 = (tests - 1) / 2, 1
    m = (y1 - y0) / (x1 - x0)
    b = y0 - (m * x0)
    unit_time = seconds / tests
    return [unit_time * (m * x + b) for x in range(tests)]

Some tests:
times = duration_spread(4, 8, 1)
print(' '.join(f'{x:.1f}' for x in times))
# output: 0.0 1.3 2.7 4.0

times = duration_spread(5, 10, -1)
print(' '.join(f'{x:.1f}' for x in times))
# output: 4.0 3.0 2.0 1.0 0.0

times = duration_spread(5, 10, .5)
print(' '.join(f'{x:.1f}' for x in times))
# output: 1.0 1.5 2.0 2.5 3.0

Code review
Very short since @Reinderien covered most of the ground.

Input type: tests represents the number of tests, so I am not sure why it's a float. It should be an integer.
Missing return type hint: the return type hint is missing in both functions.


Reversed Binary Numbers in JavaScript I'm trying it out just to see if I could solve it. It would be great if I could receive tips on how to improve it.

//input
var decimal = prompt("Please enter a decimal number");
if (decimal >= 1 && decimal < 1000000000) {
    //convert to binary
    var binConvert = parseInt(decimal, 10).toString(2);

    //reverse binary number
    var makeString = binConvert.toString();
    var srj = makeString.split("").reverse().join("");
    var makeNumber = Number(srj);

    //convert back to decimal
    alert(parseInt(makeNumber, 2));

} else {
    alert("Input number should be: 1 <= N < 1 000 000 000. Please try again.");
}

jsFiddle

prompt("Please enter a decimal number");

is a little confusing to me. I thought you wanted a number with a few decimal digits on it.

It might make more sense to say "Please enter a base 10 number"

You are a little incosistent in how you turn your string numbers into number numbers.

In the beginning, you use parseInt:

var binConvert = parseInt(decimal, 10).toString(2);

In the end, you use parseInt:

alert(parseInt(makeNumber, 2));

But in the middle, you use Number:

var makeNumber = Number(srj);

It is better to be consistent and stick to one function (in my opinion, you should use parseInt).

If you take the above advice in using parseInt, you can then get rid of the last parseInt call like so:

var makeNumber = parseInt(srj, 2); // I moved the parseInt call up here

//convert back to decimal
alert(makeNumber);

This might just be me, but your comments are a little confusing.

Here, you wrote:

//convert back to decimal
alert(parseInt(makeNumber, 2));

Aren't you technically converting to binary? (The radix parameter of parseInt is 2).


Improving and optimizing Tic-Tac-Toe game written in Python 3 I've been teaching myself Python3 for about a week now and I decided to put my skills to the test by writing a simple Tic-Tac-Toe game. Here is what I came up with:

#!/usr/bin/env python3

import random

board = [0]*9

def resetBoard():
    for index, i in enumerate(board):
        board[index] = 0

def printBoard():
    counter = 0
    s = ""
    print("|-----|-----|-----|")
    for index, i in enumerate(board):
        s += "|  "
        if(board[index] == 0):
            s += str(index)
        if(board[index] == 1):
            s+= "x"
        if(board[index] == 2):
            s += "o"
        s += "  "
        counter += 1
        if(counter == 3):
            s += "|"
            print(s)
            print("|-----|-----|-----|")
            s = ""
            counter = 0

def getPlayers():
    return input("1 or 2 Players?")

def moveSquare(xOro, ai):
    square = 0
    flag = 0
    while(board[square] != 0 or flag == 0):
        if(ai == 0):
            if(xOro == 1):
                square = eval(input("Choose a square to place X: "))
            if(xOro == 2):
                square = eval(input("Choose a square to place O: "))
        if(ai == 1):
            square = random.randint(0, 8)
        flag = 1
    board[square] = xOro
    print("\n\n")

def checkForWin():
    if(board[0] == board[1] and board[1] == board[2] and board[2] != 0):
        return 1
    if(board[3] == board[4] and board[4] == board[5] and board[5] != 0):
        return 1
    if(board[6] == board[7] and board[7] == board[8] and board[8] != 0):
        return 1
    if(board[0] == board[3] and board[3] == board[6] and board[6] != 0):
        return 1
    if(board[1] == board[4] and board[4] == board[7] and board[7] != 0):
        return 1
    if(board[2] == board[5] and board[5] == board[8] and board[8] != 0):
        return 1
    if(board[0] == board[4] and board[4] == board[8] and board[8] != 0):
        return 1
    if(board[2] == board[4] and board[4] == board[6] and board[6] != 0):
        return 1
    for index, i in enumerate(board):
        if(i  == 0):
            break
        if(index == 8):
            return 2
    return 0

def gameOver():
    return eval(input("\nPress 0 and then ENTER to play again!"))

# Main
random.seed()
quit = 0
while(quit == 0):
    players = 0
    win = 0
    while(players != 1 and players != 2):
        players = eval(getPlayers())
    resetBoard()
    game = 0 #game = 1 when game is finished
    while(game == 0): #game loop
        printBoard()
        moveSquare(1, 0)
        game = checkForWin()
        if(game > 0):
            win = game
            break
        printBoard()
        if(players == 1):
            moveSquare(2, 1)
        if(players == 2):
            moveSquare(2, 0)
        game = checkForWin()
        if(game > 0):
            win = game + 1
            break
    if(win == 1):
        print("\nX Wins!")
    if(win == 2):
        print("\nO Wins!")
    if(win == 3):
        print("\n The game is a draw!")
    quit = gameOver()
exit()

I am interested in the following points about my code:

How can I make my code smaller and more optimized?
The game displays "O Wins" instead of "The game is a draw!" and I'm not sure why.
How can I sanitize input to prevent crashes if a string is entered instead of an integer (for example when choosing a square on the board)?
Any additional points to improve my code or general tips that will help me become a better Python programmer.

#!/usr/bin/env python3

import random

board = [0]*9

Having global variables that change is considered a bad idea. You'd be better off putting this variable and the methods inside a class.

def resetBoard():
    for index, i in enumerate(board):
        board[index] = 0

You could also do board[:] = [0] * 9

def printBoard():
    counter = 0

In python you rarely need to keep track of counters like this

    s = ""

In python, you should avoid building strings by adding different pieces together

    print("|-----|-----|-----|")
    for index, i in enumerate(board):
        s += "|  "
        if(board[index] == 0):

The point of enumerate is that i is the value in board. Use it rather then refetching it here.

            s += str(index)
        if(board[index] == 1):
            s+= "x"
        if(board[index] == 2):
            s += "o"
        s += "  "

By building the string this way its hard to follow exactly what the string is.

        counter += 1
        if(counter == 3):

Instead of making the counter use `if index % 3 == 2:' Also you don't need the parens

            s += "|"
            print(s)
            print("|-----|-----|-----|")
            s = ""
            counter = 0

Your whole algorithm here is rendered more complicated because you are iterating over cells rather the rows. Your algorithm would be much more natural with rows

def getPlayers():
    return input("1 or 2 Players?")

Shouldn't this return an int, not a string?

def moveSquare(xOro, ai):

variable names are recommended to follow x_or_o as a naming convetion

    square = 0
    flag = 0

If this is a boolean flag, use False not 0

    while(board[square] != 0 or flag == 0):

No need for the parens around the while expression

        if(ai == 0):

Again use True/False for boolean flags. No need for parens

            if(xOro == 1):
                square = eval(input("Choose a square to place X: "))
            if(xOro == 2):
                square = eval(input("Choose a square to place O: "))

Avoid magic numbers like 1 and 2 to denote X and O. In this case, I'd probably pass 'x' or 'o' strings.

        if(ai == 1):

Use an else:

            square = random.randint(0, 8)
        flag = 1

Flag is really terrible variable name because I don't know what it means. Boolean flags are usually an ugly solution

    board[square] = xOro
    print("\n\n")

def checkForWin():
    if(board[0] == board[1] and board[1] == board[2] and board[2] != 0):
        return 1

Don't use magic numbers like that. Instead create some global constants to return or something

    if(board[3] == board[4] and board[4] == board[5] and board[5] != 0):
        return 1
    if(board[6] == board[7] and board[7] == board[8] and board[8] != 0):
        return 1
    if(board[0] == board[3] and board[3] == board[6] and board[6] != 0):
        return 1
    if(board[1] == board[4] and board[4] == board[7] and board[7] != 0):
        return 1
    if(board[2] == board[5] and board[5] == board[8] and board[8] != 0):
        return 1
    if(board[0] == board[4] and board[4] == board[8] and board[8] != 0):
        return 1
    if(board[2] == board[4] and board[4] == board[6] and board[6] != 0):
        return 1

A lot duplication here.

    for index, i in enumerate(board):
        if(i  == 0):
            break
        if(index == 8):
            return 2
    return 0

def gameOver():
    return eval(input("\nPress 0 and then ENTER to play again!"))

Eval is a bit dangerous because the user can put in anything they want.

# Main

You should your main logic in a function

random.seed()
quit = 0
while(quit == 0):

Make quite a boolean variable

    players = 0
    win = 0
    while(players != 1 and players != 2):
        players = eval(getPlayers())
    resetBoard()
    game = 0 #game = 1 when game is finished

Game becoming 1 when the game is finished seems backwards

    while(game == 0): #game loop
        printBoard()
        moveSquare(1, 0)
        game = checkForWin()
        if(game > 0):
            win = game
            break

Not much point checking game == 0 above if you are just going to break anyways

        printBoard()
        if(players == 1):
            moveSquare(2, 1)
        if(players == 2):
            moveSquare(2, 0)
        game = checkForWin()
        if(game > 0):
            win = game + 1
            break

See how the two parts of the loop are very similar? Suggests that you should get rid of the duplication.

    if(win == 1):
        print("\nX Wins!")
    if(win == 2):
        print("\nO Wins!")
    if(win == 3):
        print("\n The game is a draw!")
    quit = gameOver()

Having gameOver() return whether or not you should quit seems confusing. The name doesn't suggest that

exit()

Not much point in calling quit, the program is about to terminate anyways

And just to demonstrate how I'd do this:

#!/usr/bin/env python3
import random

WINNING_LINES = [
    [0, 1, 2],
    [3, 4, 5],
    [5, 6, 7],

    [0, 3, 5],
    [1, 4, 6],
    [2, 5, 7],

    [0, 4, 7],
    [2, 4, 5]
]

STALEMATE = '!'

class TicTacToe(object):
    def __init__(self):
        self.board = [' '] * 9

    def print(self):
        # make a copy of the board
        cells = self.board[:]

        # put a digit in every blank cell
        for index, cell in enumerate(cells):
            if cell == ' ':
                cells[index] = str(index)

        # put spaces around the cells
        cells = [' %s ' % cell for cell in cells]

        # print the actual board
        print('|---|---|---|')
        for start in range(0, 9, 3):
            row = cells[start:start+3]
            print('|', '|'.join(row), '|', sep = '')
            print('|---|---|---|')

    def move_square(self, player, ai):
        square = -1
        while square < 0 or square >= 9 or self.board[square] != ' ':
            if ai:
                square = random.randrange(9)
            else:
                question = "Choose a square to place {}:".format(player.upper())
                square_text = input(question)
                print(square_text)
                try:
                    square = int(square_text)
                except ValueError:
                    # user didn't enter a number
                    square = -1
        self.board[square] = player
        print("\n\n")

    def status(self):
        # check for a win
        for a, b, c in WINNING_LINES:
            if self.board[a] == self.board[b] and self.board[b] == self.board[c] and self.board[a] != ' ':
                return self.board[a]

        if not any(cell == ' ' for cell in self.board):
            return STALEMATE
        return ' '

def get_players():
    while True:
        text = input("1 or 2 players")
        try:
            value = int(text)
        except ValueError:
            pass
        else:
            if 0 < value < 3:
                return value

def play_again():
    return input("\nPress 0 and then ENTER to play again!") == '0'

GAME_OVER_MESSAGES = {
    'X' : 'X wins!',
    'O' : 'O wins!',
    '!' : 'The game is a draw!'
}

def main():

    while True:

        # create a list to denote whether
        # a player is AI
        ai_players = [False, False]
        if get_players() == 1:
            ai_players[1] = True

        tic_tac_toe = TicTacToe()
        player_turn = 0
        player_codes = 'XY'
        while tic_tac_toe.status() == ' ':
            tic_tac_toe.print()
            tic_tac_toe.move_square(player_codes[player_turn], ai_players[player_turn])
            player_turn = (player_turn + 1) % 2

        print(GAME_OVER_MESSAGES[tic_tac_toe.status()])

        if not play_again():
            break

if __name__ == '__main__':
    main()


Multithreading C++ loop I've made a small skeleton for a larger project that will include cross-platform multithreading (by using Boost) and thread-safe random numbers (by using GNU scientific libraries and mutexes).  My goal is to run a simulation (probably over 100,000 loops) in which each thread will utilize a random number and read from a library that the threads will not change the value of (thus, I don't bother putting the library in a mutex)!

Is this code alright?

#include <stdio.h> // Standard in out for c
#include <vector> // Vectors
#include <boost/thread.hpp> // Boost threads
#include <boost/signals2/mutex.hpp> // Boost mutexes
#include <gsl/gsl_rng.h> // Gnu Scientific Library, thread safe rand

const gsl_rng_type * T = gsl_rng_default; // Select random generator
gsl_rng * r = gsl_rng_alloc (T); // Allocate the range
boost::mutex mtx; // Create a mutex
std::vector<unsigned long> data; // Create a vector

int count=23; // Have a total number of threadFunctions you want to loop through
int CORES = static_cast<int>(boost::thread::hardware_concurrency());

void threadFunction(int id);

int main()
{
    printf("Number of cores = %d\n-------------------\n\n",CORES);
    gsl_rng_env_setup(); // Setup the random variable environment
    gsl_rng_set (r,time(NULL)); // Seed the r generator
    std::vector<boost::thread *> t; // Create vector of boost::thread pointers
    while(count>0){ // While you are looping
        for(int i=0;i<((CORES>count?count:CORES));i++) // Create 4 threads if more than 4 loops are left, else create the appropriate number of threads
            t.push_back(new boost::thread(threadFunction,i)); // Push threads objects back
        for(int i=0;i<t.size();i++){ // Loop through all threads
            t[i]->join(); // Wait to join.  Note, these are pointers so use -> instead of .
            count--; // Reduce count by 1
        }
        t.resize(0); // Empty the array
    }

    printf("\nEnd of threads...");
    FILE *out = fopen("RandomNumbers.txt","w+"); // Create a file to hold values
    for(int i=0;i<data.size();i++) // Print out values
        fprintf(out,"%lu\n",data[i]);
    fclose(out); // Close file
    printf("\nPress Enter to exit...");
    getchar();
    return 0;
}

void threadFunction(int id)
{
    { // By scoping in, the lock_guard can destroy itself upon exit
        boost::lock_guard<boost::mutex> ownlock(mtx); // Create lock_guard
        data.push_back(gsl_rng_get(r)); // Put a random number in data
    }
    printf("Working on %d...\n",id);
}

You're overdoing it with comments. A lot. And trivial ones at that. Every programmer knows that std::vector<boost::thread *> t; creates a vector of pointers to boost::thread, or that you have to use -> with pointers. Such comments are really just clutter and actually make the code harder to read. Save them for the non-obvious bits.
It's unclear what mtx protects. Does it protect data? Or r? Or both? This is one of the places where you should comment, but you didn't (//Create a mutex is absolutely pointless). If possible, rename the mutex to match its purpose. It would also be a good idea to wrap the mutex and the data it protects in a class, to ensure that unsafe access is impossible.
((CORES>count?count:CORES)) requires the reader to parse it (and what's with the double parentheses?) The meaning of std::min(CORES, count), on the other hand, is immediately obvious. And how does it create 4 threads? You could just as well rewrite the entire loop to make it more concise (and perhaps more efficient too):

while (count > 0) {
  std::vector<boost::thread *> threads(std::min(CORES, count));
  for (size_t i = 0; i < threads.size(); ++i) { // Start appropriate number of threads
    threads[i] = new boost::thread(threadFunction, i);
  }
  for (size_t i = 0; i < threads.size(); ++i) { // Wait for all threads to finish
    threads[i]->join();
    delete threads[i];
    --count;
  }
}

Also note that I added a call to delete into the waiting loop. Your previous code was leaking the threads.
You're writing C++. Don't use (type unsafe!) C I/O. Do this instead:

{
  std::ofstream out("RandomNumbers.txt");
  for (size_t idx = 0; idx < data.size(); ++idx) {
    out << data[idx] << '\n';
  }
}

The same holds for all other uses of C I/O (printf(), getchar()), of course.
count and CORES would be better declared with type size_t, as they represent the number of some objects.
If you have access to C++11, all the above could be simplified even further using range-based for loops, lambdas in standard algorithms, or at the very least iterator-based loops. I didn't bother with iterators here (even though they could be used), because their type has to be spelled out explicitly.

In such case, it would also make more sense not to allocate the threads dynamically, but create them directly in the vector. No need for manual deallocation, no dynamic allocation overhead.


Farm tractor implementation I have a code which was not refactored at all. I refactored it to some extent, but stuck at a point where I cannot think of anything further.

Tractor.java:

package com.farm;

public class Tractor implements MethodsInterface {

    private int[] position;

    private int[] field;

    private String orientation;

    public Tractor(){
        position  = new int[]{0,0};
        field = new int[]{5,5};
        orientation = "N";
    }

    public void move(String command) {
        if(command=="F"){
            moveForwards();
        }else if(command=="T"){
            turnClockwise();
        }
    }

    private void moveForwards() {
        if(orientation=="N"){
            position = new int[]{position[0], position[1]+1};
        } else if(orientation == "E"){
            position = new int[]{position[0]+1, position[1]};
        } else if(orientation == "S"){
            position = new int[]{position[0], position[1]-1};
        }else if(orientation == "W"){
            position = new int[]{position[0]-1, position[1]};
        }
        if(position[0] > field[0] || position[1] > field[1]) {
            try {
                throw new TractorInDitchException();
            } catch (TractorInDitchException e) {
                e.printStackTrace();
            }
        }

    }

    private void turnClockwise() {
        if(orientation=="N"){
            orientation = "E";
        }else if(orientation == "E"){
            orientation = "S";
        }else if(orientation == "S"){
            orientation = "W";
        }else if(orientation == "W"){
            orientation = "N";
        }
    }

    public int getPositionX() {
        return position[0];
    }

    public int getPositionY() {
        return position[1];
    }

    public String getOrientation() {
        return orientation;
    }
}

TractorInDitchException.java:

package com.farm;

public class TractorInDitchException extends Exception{

}

MethodsInterface.java:

package com.farm;

public interface MethodsInterface {

    public int getPositionX();
    public int getPositionY();
    public String getOrientation();
}

What else could be refactored?

Instead of magic constants like "N", "S" etc. use an enum which contains named constants and their values are checked on compile time.

public enum Orientation {
  NORTH, SOUTH, WEST, EAST
}

With plain Strings it's too easy to set an invalid value to the orientation field.
You can move the turnClockwise method to an enum:

public enum Orientation {
  NORTH {
    @Override
    public Orientation turnClockwise() {
      return EAST;
    }
  },
  SOUTH {
    @Override
    public Orientation turnClockwise() {
      return WEST;
    }
  },
  WEST {
    @Override
    public Orientation turnClockwise() {
      return NORTH;
    }
  },
  EAST {
    @Override
    public Orientation turnClockwise() {
      return SOUTH;
    }
  };

  public abstract Orientation turnClockwise();

}

It increases cohesion and replaces conditional with polymorphism.

(I'd also try to create a Position class with x and y coordinates and move the statements from the moveForwards method to this class like goNorth(), goWest() etc. Then a new Position moveForward(Position current) method in the Orientation enum would call one of the Position.go*() methods.)
I'd change this array:

position = new int[] {0, 0};

to two ints:

private int positionX;
private int positionY;

Indexes here (0 and 1) are magic constants.
The moveForwards and the the Tractor class looks unfinished. Noone writes the field array.
This:

try {
  throw new TractorInDitchException();
} catch (final TractorInDitchException e) {
  e.printStackTrace();
}

could be written as

new TractorInDitchException().printStackTrace();

without the try-catch structure. A logger framework (for example SLFJ and Logback) would be better here.
What should happen when the tractor reach the border of the field? (When the index is negative or greater than or equal to the size of the array.)


Opening files using fopen only if user enters correct password I wrote this program to just get some practice in C, but I have a question:

Do you see anything that I am doing that will create bad habits in my future programming?

#include <stdio.h>

char password[]="ps"; // set the password that user must match
char input[20];          // user input buffer
char filename[32];
FILE *fp;
int c,i,count=3;

int main()
{

printf("Please enter the password:");   //ask for password
fgets(input,20,stdin);                  //store user entry into input buffer
sscanf(input,"%s",input);               //format

    if(strcmp(password,input)==0)   //if the input is equal to the password then open the file and print contents
    {
            if((fp=fopen("text","r")) != NULL)
            {
                    while((c=getc(fp)) != EOF)
                    {
                            putchar(c);
                    }
                    fclose(fp);                     //close the file once done
            }
            else
            {
                    perror("Error:");
                    return(-1);
            }
    }
    else if(strcmp(password,input) != 0)    //if the input is wrong then give the user 3 more tries
         {
                    for(i=0;i<3;i++)
                    {
                            printf("\nThe password you entered is incorrect!");
                            printf("\n%d tries left, password:",count);
                            fgets(input,20,stdin);
                            sscanf(input,"%s",input);

                            --count;

                            if(strcmp(password,input)==0)   //if out of those 3 tries entry is correct--open file/print contents
                            {
                                    if((fp=fopen("text","r")) != NULL)
                                    {
                                            while((c=getc(fp)) !=EOF)
                                     {
                                                    putchar(c);
                                            }
                                            fclose(fp);     //close file
                                            break;          //break out of for loop
                                    }
                                    else
                                    {
                                            perror("Error:");
                                            return(-1);
                                    }
                            }
                    }
            }
}

Don't repeat yourself. This block of code appears twice. It would be better to extract it to a separate function and reuse it:

        if((fp=fopen("text","r")) != NULL)
        {
                while((c=getc(fp)) != EOF)
                {
                        putchar(c);
                }
                fclose(fp);                     //close the file once done
        }
        else
        {
                perror("Error:");
                return(-1);
        }

Btw, is there a reason to read and print the file contents character by character? It would be more efficient to do it buffer by buffer.

Also, the comment is pointless, that line is self-explanatory.

In this code:

if(strcmp(password,input)==0)
{
        // ...
}
else if(strcmp(password,input) != 0)

The else if condition can be replaced with a simple else. That's less typing, and again, don't repeat yourself!


How easy is it to crack this encryption algorithm? When I was about 13 or 14 years old, I was a little interested in cryptography (which is, after all, an interesting field). I learnt quite a lot since that time (it has been about 8 years since then), but I'm still very far away from considering myself an expert in cryptography.

Whatever, when I was at that age, I wrote this little Perl script which I just found on an old HD. (Saved as MyEncrypt.pm)

package MyEncrypt;

use strict;
use warnings;

my @_ALPHABET = ('a' .. 'z', 'A' .. 'Z', 0 .. 10, ' ');
my $_i = 0;
my %_ALPHABET = map { $_i++; $_ => $_i } @_ALPHABET;
$_i = 0;
my %_ALPHABET_REVERSE = map { $_i++; $_i => $_ } @_ALPHABET;

sub new {
    bless +{}, shift
}

sub set_password {
    my $self = shift;
    my $password = shift;
    $self->{hashed_password} = _hash_password($password);
    return $self;
}

sub encrypt {
    my $self = shift;
    my $input = shift;

    my $hashed_password = $self->{hashed_password};
    die "No password set.\n" unless $hashed_password;

    my @split = split(//, $input);

    my $output = shift;
    for (0 .. $#split) {
        my $new_number .= ($_ALPHABET{$split[$_]} + ($hashed_password ** ($_ + 1))) % $#_ALPHABET;
        $output .= $_ALPHABET_REVERSE{$new_number};
    }
    return $output;
}

sub _hash_password {
    my $password = shift;
    my $hash = 1;
    my $i = 1;
    for (split(//, $password)) {
        my $power = length($password) / (2 ** $i);
        $power = 1 if $power < 1;
        $hash *= int($_ALPHABET{$_} ** $power);
        $i++;
    }

    if(is_multiple_of_two($hash)) {
        $hash += 1;
    }

    while (length($hash) != 10) {
        no warnings;
        $hash *= $hash | join('', map { $_ALPHABET{$_} } split(//, $password));
        $hash =~ s/\.//g;
        $hash = substr($hash, 0, 10);
    }
    return $hash;
}

sub is_multiple_of_two {
    my $n = shift;
    my $log = log($n) / log(2);
    if($log == int($log)) {
        return 1;
    } else {
        return 0;
    }
}

1;

(It probably has the worst hashing-algorithm ever. I know).

This is my "test-program" for it:

use strict;
use warnings;
use MyEncrypt;

my $enc = MyEncrypt->new();
$enc->set_password('abc');
die $enc->encrypt("hello world");

My idea was this:
We give it a password and it somehow generates a "hash" of it (here in a way that, I admit, I don't quite understand anymore. For some reason, it should be 10 digits long and these are generated by, until the password is 10 digits long, multiplying the password with the OR'd characters of the alphabet (where a = 1, ... A = 27, ...) and then cutting it to 10 characters or less, until this leads to a "hash"), which is then later used for this letter-by-letter-substitution:

x = (number of the actual letter + password_hash to the power of (position of this character + 1)) modulo the size of the alphabet

and then x is used to look for the letter numbered x, which will be added to the output-string. When gone through all the text, the output will have a completely different form of what it had as input. And when one letter changes in the password, the whole string is different.

E.g.:

string: "hello world", pass: "abc" => 9bkpXY1H0oR
string: "hello world", pass: "abcd" => HS4gVkuWX4U
string: "hello world", pass: "abcda" => DhAqIeHn9cr

and so on.

Of course, the code is terrible, the hashing-algorithm is probably the worst one ever and the idea is not really new, but I came up with it myself and at that time I was quite proud of myself for this. But how secure would this have been? How much time would one need to decrypt this and how would this be done? As I've said, I developed in many fields, but for some reason after that script I did not care so much about cryptography anymore and my knowledge there is quite limited.

Your encryption scheme basically takes a string and does a one-to-one mapping of characters to 63 numbers (0 to 62).  Ignoring this straightforward mapping of characters, your scheme basically is the i-th character of the ciphertext (1-indexed) is c[i] = (p[i] + H**i) % N, where N is the size of the alphabet (63), p[i] is i-th character of the plaintext, and H is the hash calculated from the password.

Decryption should be straightforward as p[i] = (c[i] - H**i) % N.

But the code doesn't work -- decryption is impossible

First, the code is seriously flawed as perl handles integer overflow by switching to floating point numbers rather than moving to a big integer library.

For example, for pw="abc" the hash is 8486571168 (as seen by inserting a print statement).  Note for say the second character in the array you calculate 8486571168**2 in perl is 7.20218901895289e+19 -- an double precision floating point number.  Note adding any number 0 to 63 to 7.20218901895289e+19 leaves the value unchanged in a double -- hence your code doesn't work -- it loses the original value of the plaintext due to rounding errors from using floating point math.

You can easily demonstrate this by using the same password (abc) to encrypt say "hBROKEN1234" or "hNOTWORKING" (or anything else starting with an h of this length).  Regardless of the plaintext, the generated "ciphertext" will always be "9bkpXY1H0oR".  You could fix this by using modular exponentiation library or a big-integer math library, or switching to a language like python that has big integer math built-in.  Note, it suffices to show that an encryption method is broken if the same key encrypts two different plaintexts to the exact same ciphertext.  If that happens clearly there is no decryption function, as the decryption function takes the key and the ciphertext and needs to recover the original plaintext.

Furthermore the algorithm is trivially attackable

Second, this scheme if fixed would be trivial to attack.  You merely have to try to decrypt with all N (length of your alphabet) different values of the hash (modulo the alphabet size) and at least one of them should work.

If you understand modular arithmetic (if not see below) you will see your encryption method for each character c[i] = (p[i] + H**i) % N is exactly equivalent to c[i] = p[i] + (H % N)**i) % N.  This is bad as there are only N possible values of H % N (and in our case N=63), so its trivial to try decrypting with H=0 to H=62 until you find something that is meaningful data.

Background on modular arithmetic

To see this note we are doing modular arithmetic.  In modular arithmetic, there's the concept of congruent numbers; e.g., on a clock face (modulo 12) there's no difference between 3 o'clock or 15 o'clock, or 51 o'clock (48+3), so we say \$3 \equiv 15 \equiv 51 \;(\bmod 12)\$ -- the notation is to only write \$(\bmod N)\$ in parenthesis at the end, even though it applies to the whole equation (all sides).

When you do modular arithmetic for the most part, it doesn't matter if you take the modulus step early.  This is easy to see for addition.  if you add 7 hours to 3 o'clock, you get 10.  Similarly if you add 31 hours (\$31 = 7 \bmod 12\$) to 15 o'clock, you again get 10 on the clock face.  To formalize this notation, let's define two pairs of congruent numbers modulo N, specifically \$a \equiv A\;(\bmod N)\$ which means \$A = a + m N\$ for some integer \$m\$ (\$m\$ could be positive, negative or zero), and similarly let \$b \equiv B \;(\bmod N)\$ and \$B = b + n N\$ for some integer \$n\$.  Doing the addition we get \$A + B = a + b + (m + n) N\$, and since \$m + n\$ is also an integer we just showed the \$A + B \equiv a + b\; (\bmod N)\$.  You can do similar proofs for subtraction, multiplication, and exponentiation.
works.  The proof for multiplication: $$\begin{eqnarray}
A B &=& (a + m N)(b + n N)\\
&=& a b + m b N + n a N + m n N N \\
&=& a b + (m b + n a + m n N) N \\
\therefore A B &\equiv& a b\; (\bmod N)
\end{eqnarray}$$

Similarly for the base in exponentiation, we can show we can take the modulus (of the base) can be done early as

$$\begin{eqnarray}
A^k &=& (a + m N)^k\\
&=&  \sum_{i=0}^k {k \choose i}a^{k-i} (m N)^i \\
&=& a^k + \sum_{i=1}^k {k \choose i}a^{k-i} m^i N^i \\
&=& a^k + N \left(\sum_{i=1}^k {k \choose i}a^{k-i} m^i N^{i-1} \right) \\
\therefore A^k &\equiv& a^k\; (\bmod N)
\end{eqnarray}$$

where the binomial theorem was used.  Thus we are allowed to take an expression like (a + b**x) % N and change it to (a + (b % N)**x) % N and it won't change the result and thus only need to try all the values of (h % N) when brute-force decrypting.


Optimizing the performance of a nested for loop over millions of messages from SQLite Query - v2 I've posted and gotten help to optimize my code before here and here. Now I have implemented the following changes, after very helpful advice:

I have implemented a Dictionary instead of using list when looking into the extracted messages.
I have implemented index on the userid and unixtime tables in my database.

Both these changes combined has sped up this program a tenfold, but I still need to make it quicker.

I have a very large number (millions) of messages, each labeled with the unixtime it was sent in a SQLite database. Each message has its own userid, for the user that have sent it. I want to know what's the max number of messages that is sent within an 24hr timeslot for each user. The 24hr timeslot is defined as the time from one message to another. So if there are five messages, where the 5th one is sent 24 hours after the first one, 5 is the number I want.

After these changes the query time of my code is small compared to the total runtime of the program, so I think the biggest gains is in optimizing the algorithm itself.

con = lite.connect(databasepath)
    messageFrequency = []
    start = time.time()
    with con:
        cur = con.cursor()
        #Get all UserID
        distincttime = time.time()
        cur.execute('SELECT DISTINCT userid FROM MessageType1 limit 100000')
        deltadistincttime = time.time()-distincttime
        userID = cur.fetchall()
        userID = {index:x[0] for index,x in enumerate(userID)}
        #For each UserID
        for index in range(len(userID)):
            messageFrequency.append(0)
            #Get all MSG with UserID = UserID sorted by UNIXTIME
            cur.execute('SELECT unixtime FROM MessageType1 WHERE userID ='+str(userID[index])+' ORDER BY unixtime asc')
            AISmessages = cur.fetchall()
            AISmessages = {index:x[0] for index,x in enumerate(AISmessages)}
            #Loop through every MSG
            for messageIndex in range(len(AISmessages)):
                frequency = 0
                message = AISmessages[messageIndex]
                for nextMessageIndex in range(messageIndex+1, len(AISmessages)):
                #Loop through every message that is within 24 hours
                    nextmessage = AISmessages[nextMessageIndex]
                    if  nextmessage < message+(86400):
                    #Count the number of occurences
                        frequency += 1
                    else:
                        break
                #Add best benchmark for every message to a list that should be plotted.
                if messageFrequency[-1]<frequency:
                    messageFrequency[-1] = frequency

What I think can be done:

Instead of looping through every message in the third loop, I can implement some kind of binary-search to find the first message that is 24hours away.
Instead of looping through every message in the second loop, I can loop through every message that is \$x\$ seconds/minutes away; but maybe the gain is lost by trying to skip forward \$x\$ seconds?

When I run with 2000 userID's the total running time is 8.1 seconds, the cumulative time of the innermost loop is 6.9 seconds. Most optimal I will run this code with 80 000 userIDs and 200M messages.
I would love other suggestions on what to be done to bettering the performance of this code.

You could save a lot of comparisons if you wouldn't throw away the information where the last comparison ended (at 'break'). What you do is a sort of sliding window: you start at msg[k] and compare at index k+1, k+2,...k+f until the timestamp leaves the 24h range. You do this sequentially, or you could do that with a binary search - the time reduction depends on how big the 'frequency' is on average.

My suggestion to improve deals with the next iteration: instead of comparing at least f-1 values again, do this:

drop the last starting point, i.e. frequency -= 1
skip all the values k+1...k+f as you have counted them already in the last iteration
start comparing at index k+f+1, either sequentially or with binary search. Increment the last value of 'frequency' for each valid comparison.

f in this case is available in 'frequency'.
Think of a sliding window where you advance the window by 1 to the right: drop the leftmost and start comparing one after the rightmost element. This should save you a lot of time (the more, the higher the frequency is on average).


Pythonic quicksort algorithm Is my implementation of quicksort efficient, in-place, pythonic ?

def quicksort(array, lo, hi):
    if hi - lo < 2:
        return
    key = random.randrange(lo, hi)
    array[key], array[lo] = array[lo], array[key]
    y = 1 + lo
    for x in xrange(lo + 1, hi):
        if array[x] <= array[lo]:
            array[x], array[y] = array[y], array[x]
            y += 1
    array[lo], array[y - 1] = array[y - 1], array[lo]
    quicksort(array, lo, y - 1)
    quicksort(array, y, hi)

a = map(int, raw_input().split())
quicksort(a, 0, len(a))

Looks good in general. Few comments.

Missing import random.
Random pivot selection is just one of many possible strategies. It may or may not be a good strategy depending on the nature of data. Only the client knows what a suitable strategy will be; let him chose.
No Raw Loops mantra: Every loop possible represents an algorithm, often very important itself. In this case the algorithm is partition. It is worthy to be factored out in a function of its own.


Leftist Tree in Dyalog APL -- can it be made more compact/idiomatic? Following great feedback given in response to my previous question, I was encouraged to consider a different type of heap structure, the Leftist Tree. It lends itself better to a functional implementation. It's already shorter, but is there anything else I should consider in order to make this more idiomatic APL? Many thanks.

⍝ APL implementation of a leftist tree.
⍝
⍝ https://en.wikipedia.org/wiki/Leftist_tree
⍝ http://typeocaml.com/2015/03/12/heap-leftist-tree/

⎕io←0
Insert←{ ⍝ Insert item into leftist tree, returning the resulting tree
    (tree item)←⍵
    1 item ⍬ ⍬ Merge tree
}

Pop←{ ⍝ Pop off smallest element from a leftist tree
    0=≢⍵:⍬
    (v l r)←1↓⍵                 ⍝ value left right
    (l Merge r) v               ⍝ Return the resulting tree and the value
}

Merge←{ ⍝ Merge two leftist trees, t1 and t2
    t1←⍺ ⋄ t2←⍵
    0=≢t1:t2 ⋄ 0=≢t2:t1                          ⍝ If either is a leaf, return the other
    (key1 left right)←1↓t1 ⋄ key2←1⌷t2
    key1>key2:t2∇t1                              ⍝ Flip to ensure smallest is root of merged
    merged←right∇t2                              ⍝ Merge rightwards
    (⊃left)≥⊃merged:(1+⊃merged) key1 left merged ⍝ Right is shorter
    (1+⊃left) key1 merged left                   ⍝ Left is shorter; make it the new right
}

⍝ Example heap merge from http://typeocaml.com/2015/03/12/heap-leftist-tree/
h←Insert ⍬ 2
h←Insert h 10
h←Insert h 9

s←Insert ⍬ 3
s←Insert s 6

h Merge s
┌→─────────────────────────────────────────────────────────────┐
│     ┌→────────────────────────────────────┐ ┌→─────────────┐ │
│ 2 2 │     ┌→────────────┐ ┌→────────────┐ │ │      ┌⊖┐ ┌⊖┐ │ │
│     │ 2 3 │     ┌⊖┐ ┌⊖┐ │ │     ┌⊖┐ ┌⊖┐ │ │ │ 1 10 │0│ │0│ │ │
│     │     │ 1 6 │0│ │0│ │ │ 1 9 │0│ │0│ │ │ │      └~┘ └~┘ │ │
│     │     │     └~┘ └~┘ │ │     └~┘ └~┘ │ │ └∊─────────────┘ │
│     │     └∊────────────┘ └∊────────────┘ │                  │
│     └∊────────────────────────────────────┘                  │
└∊─────────────────────────────────────────────────────────────┘

I think your code generally looks good.

Comments

I recommend annotating functions with what the structure of their argument(s) and result are, especially when not just simple arrays, at the top of the function, rather than relying on code comments to reveal this.

Take benefit of dyadic functions

If you define Insert and Pop as dyadic functions both code and usage can be simplified. You can even let ⍬ be the default left argument, allowing easy initialisation of a tree.

Insert←{ ⍝ Insert item ⍵ into leftist tree ⍺, returning the resulting tree
    ⍺←⍬              ⍝ default to init
    1 ⍵ ⍬ ⍬ Merge ⍺
}

h←Insert 2
h Insert←10
h Insert←9

s←Insert 3
s Insert←6

Full variable names or not?

This is a personal style thing. Some people prefer mathematical-looking single-letter variables, others like full variable names that obviate comments. However, at least be consistent. (I've also moved the first element of ⍵ to become ⍺, as per above.)

Pop←{ ⍝ Pop off smallest element from a leftist tree
    0=≢⍺:⍬
    (value left right)←⍵
    (left Merge right) value
}

Unnecessary naming

⍺ and ⍵ are well understood to be the left and right arguments. I don't think renaming them t1 and t2 brings much, other than the ability to create matching keyN variables. However, here you only ever use key2 once, and its definition is very simple, and in fact as short or shorter than any appropriate name, so you might as well use it inline, freeing up key to only apply to ⍵:

Merge←{ ⍝ Merge leftist trees ⍺ and ⍵
    0=≢⍺:⍵ ⋄ 0=≢⍵:⍺                              ⍝ If either is a leaf, return the other
    (key left right)←1↓⍺
    key>1⌷⍵:⍵∇⍺                                  ⍝ Flip to ensure smallest is root of merged
    merged←right∇⍵                               ⍝ Merge rightwards
    (⊃left)≥⊃merged:(1+⊃merged) key left merged  ⍝ Right is shorter
    (1+⊃left) key merged left                    ⍝ Left is shorter; make it the new right
}