How to keep current HtmlElement active and change its properties? I have a simple class that listens click and draws two points on the page.
If first point is not exist it is drawn. When second point is present it should be changed after each click.
I dislike this part of code:
this.point2.element.style.left = `${e.x}px`;
this.point2.element.style.top = `${e.y}px`;

Full code:
export class Kizda {
    private point1: { element: HTMLElement; pointMeter: Point };
    private point2: { element: HTMLElement; pointMeter: Point };

    getPoints() {
        return { point1: this.point1, point2: this.point2 };
    }

    action(e: MouseEvent): void {
        const point = this.getMouseMPoint(e);

        function createPoint(color: string): HTMLElement {
            let pointSize = 12;
            let pointElement = document.createElement('div');
            pointElement.style.position = 'absolute';
            pointElement.style.width = `${pointSize}px`;
            pointElement.style.height = `${pointSize}px`;
            pointElement.style.left = `${e.x}px`;
            pointElement.style.top = `${e.y}px`;
            pointElement.style.background = color;
            pointElement.style.borderRadius = '50%';

            document.body.append(pointElement);

            return pointElement;
        }

        if (!this.point1) {
            this.point1 = { pointMeter: point, element: createPoint('red') };
            return;
        }

        if (!this.point2) {
            this.point2 = { pointMeter: point, element: createPoint('green') };
            return;

        } else {
            this.point2.element.style.left = `${e.x}px`;
            this.point2.element.style.top = `${e.y}px`;
        }
    }

There are some repetitions in the current implementation, which can be refactored and the code will become clearer. For example:

Use a custom type or interface for the private members.

interface MyPoint { // of course, choose a better name than MyPoint, which matches your context!
  element: HTMLElement;
  pointMeter: Point;
}

export class Kizda {
  private point1: MyPoint;
  private point2: MyPoint;

  ...
}

The createPoint function produces only a part of the point object, but it can indeed produce instances of MyPoint. And it does not need to be nested inside the action function:

private createPoint(e: MouseEvent, color: string): MyPoint {
  const size = '12px';
  const element = document.createElement('div');
  const style = element.style;

  style.position = 'absolute';
  style.width = size;
  style.height = size;
  style.background = color;
  style.borderRadius = '50%';

  this.setCoordinates(element, e);

  document.body.append(element);

  const pointMeter = this.getMouseMPoint(e);
  return { element, pointMeter };
}

private setCoordinates(point: HTMLElement, e: MouseEvent): void {
  const style = point.style;
  style.left = `${e.x}px`;
  style.top = `${e.y}px`;
}

Finally, adjust the implementation of the action method by reusing the refactored items:

public action(e: MouseEvent): void {
  if (!this.point1) {
    this.point1 = this.createPoint(e, 'red');
  }
  else if (!this.point2) {
    this.point2 = this.createPoint(e, 'green');
  }
  else {
    this.setCoordinates(this.point2.element, e);
  }
}


Turning a data class with too much functionality into a POD - SysLat Cleanup #2 This is a follow up question to Tuning the data class for the SysLat System Latency Testing hardware for gaming computers - SysLat Cleanup #1.
Since the program is too large and complex to put into one question I have also posted a question about the USB controller and the dialog that controls the entire program.
Here's my open source project: https://github.com/Skewjo/SysLat_Software
Today I'm attempting to clean up my "SysLatData" class, and the effort has gone very well so far, and I'm looking to pretty well finalize it now if possible.
Header file here: https://github.com/Skewjo/SysLat_Software/blob/master/SysLatData.h
Implementation here: https://github.com/Skewjo/SysLat_Software/blob/master/SysLatData.cpp
Definition and use here: https://github.com/Skewjo/SysLat_Software/blob/master/SysLat_SoftwareDlg.cpp
#pragma once

//Without the following 2 macros the date library spits out 50+ errors about min and max being undefined.
#undef max
#undef min
#include <date/date.h> //Should likely move this to StdAfx.h, but I'm not sure if I can because of the macro issue.
using namespace date;
using namespace std::chrono;

constexpr size_t MOVING_AVERAGE = 100;
constexpr size_t EVR_MIN = 3;
constexpr size_t EVR_MAX = 100;

struct SYSLAT_DATA {
    struct Statistics {
        std::size_t counter = 0;
        int total = 0;
        double average = 0.0;
        int median = 0;
        int max = 0;
        int min = 0;
        double approxMovingAvg = 0.0;
    };

    vector<int>         m_allResults;
    vector<string>      m_v_strRTSSWindow;
    vector<string>      m_v_strActiveWindow;

    Statistics          m_statistics;
    Statistics          m_statisticsEVR;
};

class CSysLatData
{
protected:
    SYSLAT_DATA         m_sld;
    Json::Value         m_JSONsld; //this is basically a second copy of the data... will probably eat up a BOATLOAD of memory for no reason. There's got to be a better way...
    int                 systemLatency = 0;

    std::mutex          m_Mutex;
    string              m_strError = "";

    const system_clock::time_point  m_startTime = system_clock::now();
    system_clock::time_point        m_endTime;
    duration<int>                   m_testDuration;

public:
    const SYSLAT_DATA&  GetData() {return m_sld;}
    const Json::Value&  GetJSONData() { return m_JSONsld; }
    const int&          GetSystemLatency() { return systemLatency; }

    double              CalculateMovingAverage(double currentAvg, int input);
    void                SetEndTime();
    void                UpdateSLD(unsigned int loopCounter, const string& sysLatResults, string RTSSWindow, string activeWindow, DWORD fgPID, DWORD rtssPID);
    void                AppendError(const string& error);

    void                CreateJSONSLD(); // I think I may need to make this and the following functions return bools or ints based on whether or not they failed.
    void                ExportData(int testNumber, string path = ".\\SysLat_Logs", int totalLogs = 10000);

    string              m_RTSSVersion = "0.0.0";
    string              m_targetApp = "Unknown";
    string              m_boxAnchor = "Unknown";

    bool                m_bDataExported = false;
    bool                m_bDataUploaded = false;
};

Suggestions:

Make export data a static method, pass in the object to be exported.
Do not have a member variable just to store JSON data; generate the JSON only as part of the export method.


Treat undef in Perl's  operator as +inf instead of 0 I have to sort some numbers with Perl. But some of the numbers are undefined. Perl's compare operator <=> treats undef as -inf 0. The result is, that the undefined numbers are at the beginning:

perl -MData::Dumper -e 'print Dumper(sort {$a <=> $b} 3, undef, 2, 1);'
$VAR1 = undef;
$VAR2 = 1;
$VAR3 = 2;
$VAR4 = 3;

I want them to occur at the end, because they represent new items, which should be added at the end. This means I am looking for a way to change the way Perl treats undef. In my case undef should be treated as +inf instead of -inf 0. I came up with the following expression.

($a <=> $b) * ((defined $a)*2-1) * ((defined $b)*2-1)

Is it always correct or did I miss anything?

And is there an easier way to achieve the same?

In fact, Perl treats undef as 0 in numeric contexts, and warns about that (why there's no -w in your one-liner?). You can use the defined-or operator to handle it:

sort { ($a // 'Inf') <=> ($b // 'Inf') } 3, undef, 2, 1

You need Perl v5.10+ for it to work, in older versions, you have to be more verbose:

(defined $a ? $a : 'Inf')


Calculating all prime palindromes from 0 to 1000 and printing the largest I have often read about how important clear and efficient code is. Also often people talk and write about 'beautiful' code. Some hints and critic from experienced developers for the following code would be extremely helpful to me. Another question that I often ask myself is about list comprehensions: I feel that in the first specific code (about prime palindromes), especially the second function that verifies palindrome characteristics could be expressed on one line in a list comprehension - how would that work, and would it even be an advantage of some sort?

The code calculates all the prime palindromes from 0 to 1000 and prints the largest.

#My solution to codeval challenge Prime Palindrome. AUTHOR: S.Spiess
#initial count
counting_list = [x for x in range(0,1001)]

#prime number check. make list prime_list containing only primes from 0 to 1000
def prime_check(a_list):
    prime_list = []
    for k in a_list:
        count = 2.0
        d_test = 0
        while count < k:
            if k % count == 0:
                d_test += 1
                count += 1
            else:
                count += 1
        if d_test < 1 and k > 1:
            prime_list.append(k)

    return prime_list

#check prime numbers from previous function for palindrome characteristic. append in new list.
def palindrome_check(num_list):
    palindrome_list = []
    for i in num_list:
        temp = str(i)
        if temp == temp[::-1]:
            palindrome_list.append(i)

    return palindrome_list

#print biggest palindrome prime from 0 to 1000
print max(palindrome_check(prime_check(counting_list)))

Here is another sample of code I wrote. It contains two functions that can change the base of a number.

def to_mod20(any_num):
    a_list = []

    if any_num < 20 and any_num >= 1:
        a_list.append(int(any_num))

    while any_num >= 20:
        a_list.append(any_num % 20)
        if any_num / 20 < 20:
            a_list.append(any_num / 20)
        any_num = any_num / 20
    #invert list for proper output
    return a_list[::-1]

def decimal_mod20(any_dec):
    count = 0
    a_list = []
    while any_dec < 1 and count < 4:
        a_list.append(int(any_dec * 17.0))
        any_dec = any_dec * 17.0 - int(any_dec * 17.0)
        count += 1
        #print any_dec

    return a_list

The list comprehension [x for x in range(0,1001)] is redundant, as range already returns a list in Python 2. In Python 3 one can use list(range(1001))
Write a docstring instead of a comment to describe a function.
Why is count a float (2.0) in prime_check? Should be an int.
Prefer for loops to while loops when they do the same thing. Eg. use for count in xrange(2, k) in prime_check.
if any_num < 20 and any_num >= 1: can be written as if 1 <= any_num < 20:
You may want to look for more efficient algorithms for generating primes.


C# AES256 CBC Implementation I was looking for a proper implementation of the AES256 encryption in C# but didn't find anything I was entirely satisfied with, so I decided to take this opportunity to write a class myself and maybe learn a thing or two.

After doing some research I managed to come up with this class :

class AES256
{

    private readonly byte[] _secretKey;

    public AES256(byte[] secretKey)
    {
        _secretKey = secretKey;
    }

    public byte[] Encrypt(byte[] plainBytes)
    {
        return Encrypt(plainBytes, GetRijndaelManaged(_secretKey));
    }

    public byte[] Decrypt(byte[] encryptedBytes)
    {
        return Decrypt(encryptedBytes, GetRijndaelManaged(_secretKey));
    }

    private byte[] Encrypt(byte[] plainBytes, RijndaelManaged rijndaelManaged)
    {
        byte[] transformFinalBlock = rijndaelManaged.CreateEncryptor().TransformFinalBlock(plainBytes, 0, plainBytes.Length);
        return transformFinalBlock;
    }

    private byte[] Decrypt(byte[] encryptedData, RijndaelManaged rijndaelManaged)
    {
        return rijndaelManaged.CreateDecryptor().TransformFinalBlock(encryptedData, 0, encryptedData.Length);
    }

    private RijndaelManaged GetRijndaelManaged(byte[] key)
    {
        var keyBytes = new byte[16];
        var secretKeyBytes = key;
        Array.Copy(secretKeyBytes, keyBytes, System.Math.Min(keyBytes.Length, secretKeyBytes.Length));
        var rijndaelManaged = new RijndaelManaged
        {
            Mode = CipherMode.CBC,
            Padding = PaddingMode.PKCS7,
            KeySize = 256,
            BlockSize = 128,
            Key = keyBytes,
            IV = keyBytes
        };
        return rijndaelManaged;
    }

}

It works, - but obviously that alone doesn't make the code good.
What can I improve in this class, is there anything I can optimize?
Feedback is welcome !

Peter already pointed out several important issues which I won't repeat, but here's what I've got:

Instead of verifying that the length of the key is correct, your code silently pads or trims the key. I would much rather see an exception being thrown instead. Especially security-related code should be easy to use correctly and difficult to use incorrectly.
Because the encryption key can only be specified when constructing an AES256 instance, your design 'encourages' key reuse. On its own that may not necessarily be a problem, but you're not using IVs correctly, and together that does introduce certain vulnerabilities.
RijndaelManaged and ICryptoTransform are both disposable, but they're not being disposed anywhere.
The private Encrypt/Decrypt methods are so short that merging them into the public methods would actually make the code easier to manage.
You may want to document which cipher and padding modes your code uses (and why), so other programmers (and future you) don't need to look at the implementation to find out (or wonder why certain decisions were made).
MSDN recommends using the Aes class instead of RijndaelManaged. It also appears that the Rijndael implementation isn't 'FIPS compliant', which apparently makes it unusable under certain security configurations.
As far as I understand there's no need to create a new RijndaelManaged instance for each operation, so why not create one instance and store it in a field? Or, instead of creating a very thin wrapper class, you could just create a factory method and some extension methods for encryption/decryption. By having these extension methods operate on SymmetricAlgorithm they can also be reused for other encryption algorithms, should the need ever arise.


Restucture code to wrap optional resources in a using block I have some C# I/O code that can take one or two input files.  I would like to wrap the Stream objects in a using block, but cannot find a way to succinctly express this in code.

Currently, I have two large files (>1GB), that contain concatenated TIFF files and PDF files respectively that need to be extracted into individual files.  The metadata of the individual files (TIFF Tags and PDF Keywords) cross-reference one another, so the processing rules are different if I receive both files at once, i.e. there is more information and verification logic.

I have a FileProcessor class than implements an IEnumerable<Range> to return the byte ranges of each individual file within the archive. My current implementation just wraps the main processing loop in a try/finally block and calls Dispose manually.

// Struct to represent the byte range of a file in the archive
struct Range
{
   public long start;
   public long end;
}

// Custom class that takes a Stream object in the constructor and implements IEnumerable<Range> to return the individual files in sequence
public class FileProcessor : IDisposable, IEnumerable, IEnumerable<Range>
{
    private Stream _stream;
    public FileProcessor(Stream stream) { this._stream = stream; }
    public virtual void Dispose()
    {
       if (_stream != null) { _stream.Dispose(); _stream = null; }
    }
}

public class TiffProcessor : FileProcessor { ... }
public class PdfProcessor : FileProcessor { ... }

// Snippet of the dispatching/processing logic
TiffProcessor infile1 = null,
PdfProcessor  infile2 = null;
try
{
    if (HasFirstInputFile)
    {
       infile1 = new TiffProcessor(File.OpenRead(FirstInputFileName));
    }

    if (HasSecondInputFile)
    {
       infile2 = new PdfProcessor(File.OpenRead(SecondInputFileName));
    }

    if (infile1 != null && infile2 == null)
    {
        foreach (var range in infile1) { ... }
    }

    if (infile1 == null && infile2 != null)
    {
        foreach (var range in infile2) { ... }
    }

    if (infile1 != null && infile2 != null)
    {
        foreach (var ranges in infile1.Zip(infile2, (tiff, pdf) => new Range[] { tiff, pdf })) { ... }
    }
}
finally
{
    if (infile1 != null) { infile1.Dispose(); }
    if (infile2 != null) { infile2.Dispose(); }
}

Is there any construct that can help clean up and organize this code structure.  It's not too bad now, but could become exponentially more complex if additional input streams are required in the future.

Edit

Based on the comments received, perhaps creating an intermediate Strategy object that managed resources would work?

using (var strategy = StrategyFactory.Create(CommandLineArgs))
{
    strategy.Process();
}

public static class StrategyFactory
{
    public static IStrategy Create(CommandLineArguments args)
    {
        if (args.FirstInputFile != null && args.SecondInputFile == null)
        {
            return new FirstStrategy(args.FirstInputFile);
        }

        if (args.FirstInputFile == null && args.SecondInputFile != null)
        {
            return new SecondStrategy(args.SecondInputFile);
        }

        if (args.FirstInputFile != null && args.SecondInputFile != null)
        {
            return new ThirdStrategy(args.FirstInputFile, args.SecondInputFile);
        }
    }
}

The only think that I would do different is that I wouldn't dispose streams inside your FileProcessor classes. Disposing of resources should be done at the same level (or scope) where they were initialized. Unfortunately, even BCL classes like StreamWriter and BinaryWriter dispose underlying streams when disposed, so I cannot claim that this is unexpected behavior either.

IMO, there is nothing wrong with your code. Going out of your way simply to save two lines of code might make your code less readable, so your first version might easily be the simplest one.

Having said that, if you really want to wrap it in a single using block, one idea (and I don't actually find it "better" than yours in any way) might be to wrap all your disposable resources into a single class:

public class InputFiles : IDisposable
{
    private readonly Dictionary<string, Stream> _files
        = new Dictionary<string, Stream>();

    public Stream GetStream(string type)
    {
        Stream input = null;
        _files.TryGetValue(type, out input);
        return input;
    }

    public InputFiles(string[] args)
    {
        // this is just an idea, you probably don't
        // use the extension to determine the type,
        // but it still seems a bit more general than
        // having strongly typed properties
        foreach (var path in args)
        {
            var ext = Path.GetExtension(path); // pdf or tif?
            _files[ext] = File.OpenRead(path);
        }
    }

    public void Dispose()
    {
        foreach (var stream in _files)
            stream.Value.Dispose();
    }
}

And then dispose the entire object when done:

static void Main(string[] args)
{
    using (var input = new InputFiles(args))
    {
        var pdf = input.GetStream("pdf");
        var tif = input.GetStream("tif");

        // use the appropriate strategy
        // (parsing strategy should not be concerned with
        // disposing of resources)
        Process(pdf, tif);
    }
}


Add a value to each of the array element between two given indices successively I am trying to solve the HackerRank: Array Manipulation problem using Java.

The problem that I am facing is that the program is running for half of the test cases, while for the other half it is showing "Execution couldn't be completed because of Timeout". It seems like there is a more efficient way to do the problem such that the program is executed within the given time frame. If anybody could guide me regarding the issue, it would be of great help.

Problem description

  Starting with a 1-indexed array of zeros and a list of operations, for
  each operation add a value to each of the array element between two
  given indices, inclusive. Once all operations have been performed,
  return the maximum value in your array.

  For example, the length of your array of zeros n=10. Your list of
  queries is as follows:

a b k
1 5 3
4 8 7
6 9 1

  Add the values of  between the indices  and  inclusive:

index->    1 2 3  4  5 6 7 8 9 10
          [0,0,0, 0, 0,0,0,0,0, 0]
          [3,3,3, 3, 3,0,0,0,0, 0]
          [3,3,3,10,10,7,7,7,0, 0]
          [3,3,3,10,10,8,8,8,1, 0]

  The largest value is 10 after all operations are performed.

  Function Description

  Complete the function arrayManipulation in the editor below. It must
  return an integer, the maximum value in the resulting array.

  arrayManipulation has the following parameters:

  n - the number of elements in your array
  queries - a two dimensional array of queries where each queries[i] contains three integers, a, b, and k.

  Input Format

  The first line contains two space-separated integers \$n\$ and \$m\$,
  the size of the array and the number of operations.

  Each of the next \$m\$ lines contains three space-separated integers
  \$a\$,\$b\$  and \$k\$, the left index, right index and summand.

  Constraints

  \$ 3 \le n \le 10^7 \$
  \$ 1 \le m \le 2 \cdot 10^5 \$
  \$ 1 \le a \le b \le n \$
  \$ 0 \le k \le 10^9 \$

  Output Format

  Return the integer maximum value in the finished array.

My Solution:

import java.io.*;
import java.math.*;
import java.security.*;
import java.text.*;
import java.util.*;
import java.util.concurrent.*;
import java.util.regex.*;

public class Solution {

    // Complete the arrayManipulation function below.
    static long arrayManipulation(int n, int[][] queries) {

        int queryLength = queries.length;

        System.out.println(queryLength);

        long[] p = new long[n];
        int a = 0 , b = 0 , k = 0;

        long max=0;

        // initialize p

        for(int i = 0 ; i < n ; i++)
        {
            p[i] = 0 ;
        }

        for(int i = 0 ; i < queryLength ; i++ )
        {
            a = queries[i][0];
            b = queries[i][1];
            k = queries[i][2];

            for(int j = (a-1) ; j <= (b-1) ; j++)
            {
                p[j] = p[j] + k ;
            }

        }

        for(int i = 0 ; i < n ; i++)
        {
            if(p[i]>max)
            {
                max = p[i];
            }
        }

        return max;

    }

    private static final Scanner scanner = new Scanner(System.in);

    public static void main(String[] args) throws IOException {
        BufferedWriter bufferedWriter = new BufferedWriter(new FileWriter(System.getenv("OUTPUT_PATH")));

        String[] nm = scanner.nextLine().split(" ");

        int n = Integer.parseInt(nm[0]);

        int m = Integer.parseInt(nm[1]);

        int[][] queries = new int[m][3];

        for (int i = 0; i < m; i++) {
            String[] queriesRowItems = scanner.nextLine().split(" ");
            scanner.skip("(\r\n|[\n\r\u2028\u2029\u0085])?");

            for (int j = 0; j < 3; j++) {
                int queriesItem = Integer.parseInt(queriesRowItems[j]);
                queries[i][j] = queriesItem;
            }
        }

        long result = arrayManipulation(n, queries);

        bufferedWriter.write(String.valueOf(result));
        bufferedWriter.newLine();

        bufferedWriter.close();

        scanner.close();

    }
}

Some general remarks:

System.out.println(queryLength);

The function should only return the result, but not print anything.

int a = 0 , b = 0 , k = 0;

The initial values are not used, and variables should be declared at the narrowest scope, in this case, inside the loop:

for(int i = 0 ; i < queryLength ; i++ )
{
    int a = queries[i][0];
    int b = queries[i][1];
    int k = queries[i][2];
    // ...
}

Iterating over all queries can also be done with an “enhanced for loop”:

for (int[] query: queries)
{
    int a = query[0];
    int b = query[1];
    int k = query[2];
    // ...
}

Here

for(int j = (a-1) ; j <= (b-1) ; j++)

the parentheses are not needed, and this

 p[j] = p[j] + k ;

can be shortened to

p[j] += k ;

The initialization

// initialize p
for(int i = 0 ; i < n ; i++)
{
    p[i] = 0 ;
}

is not needed because all elements are initialised to 0 by default.

It might be a matter of personal preference, but I would always put horizontal space around operators and keywords, e.g. in

if(p[i]>max)

There are also some unneeded import statements, but those are given by the submission template.

But your real problem is that the algorithm is too slow. The inner loop runs up to \$ 10^{7} \$ times, for each of the up to \$ 2 \cdot 10^{5} \$ queries.

The main idea to improve the performance is to represent the data differently. Instead of storing and updating the array with the actual numbers, one can store and update an array which holds the differences between successive numbers.

Initially, all differences are zero.

Then for each query \$ (a, b, k) \$ you only have to update two entries in the differences array: One difference increases by \$ k\$, and one difference decreases by \$ k \$.

Finally you compute the actual numbers by accumulating the differences, and determine the maximum value.

This should be much faster, because the inner loop has been eliminated.

Let's take the given example with \$n = 10\$ and the queries

a b k
1 5 3
4 8 7
6 9 1

Instead of maintaining an array of the \$ n \$ numbers

index:             1  2  3  4  5  6  7  8  9 10
initial array:    [0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
after (1, 5, 3):  [3, 3, 3, 3, 3, 0, 0, 0, 0, 0]
after (4, 8, 7):  [3, 3, 3,10,10, 7, 7, 7, 0, 0]
after (6, 9, 1):  [3, 3, 3,10,10, 8, 8, 8, 1, 0]

we maintain an array of \$ n+1 \$ differences, where each entry \$ d[i] \$  holds the difference \$a[i+1] - a[i] \$:

index:             0, 1  2  3  4  5  6  7  8  9 10
initial diffs:    [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
after (1, 5, 3):  [3, 0, 0, 0, 0,-3, 0, 0, 0, 0, 0]
after (4, 8, 7):  [3, 0, 0, 7, 0,-3, 0, 0,-7, 0, 0]
after (6, 9, 1):  [3, 0, 0, 7, 0,-2, 0, 0,-7,-1, 0]
final array:         [3, 3, 3,10,10, 8, 8, 8, 1, 0]

Do you see that only two entries are updated for each query? Can you figure out the pattern?

The final array is reconstructed by accumulating the final entries in the differences array.


F# function to concatenate some DSL scripts with indentation I'm rather new to F# and functional programming. I wrote the following method to concatenate some custom dsl scripts. The function works are expected without errors. I wanted to know if there was a better way to rewrite this function.

let foldScript (storeAppScripts : seq<string * string * string>) =
    storeAppScripts
       |> Seq.choose( fun (store, app, script) ->
                            match script with
                                | "" -> None
                                | _ -> Some (store, app, script) )
       |> Seq.groupBy(fun (store, app, script) -> store)
       |> Seq.map(fun (store, group) -> store, (group
                                                    |> Seq.groupBy(fun (_, app, _)-> app)
                                                    |> Seq.map(fun (app, group) -> app , (group |>  Seq.fold(fun accum (_,_, script) -> sprintf "%s\r\n%s" accum script) ""))
                                                    |> Seq.map(fun (app, script) -> sprintf "\r\n\tuse Application = %s  %s"  app (script.ToString()))))
        |> Seq.map(fun (store, script) ->
                                    sprintf "use Store=%s%s" store (script |> Seq.fold( fun accum appScript -> sprintf "%s%s" accum appScript) ""))
        |> Seq.fold(fun acc script -> sprintf "%s\r\n%s" acc script) ""

let s = [("s1", "a1", "script1"); ("s1", "a2", "script2");  ("s2", "a3", "script4")]
let r = foldScript s

//Expected result
// use Store=s1
//    use Application = a1
//      script1
//    use Application = a2
//      script2
// use Store=s2
//    use Application = a3
//      script1

I'll start with the code and explain after

let foldScript =
  let isScriptNonEmpty { Script = script } = script <> ""
  let foldAppScripts =
    let foldScripts = Seq.fold (fun acc { Script = script } -> sprintf "%s\r\n%s" acc script) ""
    Seq.groupBy (fun { App = app } -> app)
    >> Seq.map (fun (app, group) -> sprintf "\r\n\tuse Application = %s  %s" app <| foldScripts group)
    >> Seq.fold (sprintf "%s%s") ""

  Seq.filter isScriptNonEmpty
  >> Seq.groupBy (fun { Store = store } -> store)
  >> Seq.map (fun (store, group) -> sprintf "use Store=%s%s" store <| foldAppScripts group)
  >> String.concat "\r\n"

let s = [ { Store = "s1"; App = "a1"; Script = "script1" }; { Store = "s1"; App = "a2"; Script = "script2" }; { Store = "s2"; App = "a3"; Script = "script4" }]
let r = foldScript s

First I made inner "function" to ease readability and reused the Script type Reed talked about because it's a good idea
Then you can pattern match (deconstruct) on record almost anywhere so
(if there are multiple record type with same field name you'll have to qualify it but that's not the case here)
The foldAppScripts "function" have itself an inner "function"
I choose to favor function composition (and so made "function value") if it doesn't suit you, you can put back the argument and pipe over it
I used the backward pipe to avoid using parenthesis (also a matter of taste)
The last Seq.fold of foldAppScripts use eta-reduction (transforming fun arg -> fct arg in just fct)
Your script.ToString () isn't needed as script is already a string
I replace your last fold by a call to String.concat

Performance wise you could try to use a StringBuilder (and use Printf.bprintf) but it's not so easy to use

We could go further and remove each lambda by a named function (self documentation) but that's maybe overkill.

Edit

After toying a little more with this I think I managed to make the StringBuilder version working.
Not sure it was worth the effort my (simplistic) measurements show a quicker first version (some ms for 100k items so negligible) but the second has a little less pressure on GC gen 1
But that was mainly for the experiment (I finally used Printf.kbprintf)

here is the code :

let foldScript storeAppScripts =
  let builder = System.Text.StringBuilder (60 * Seq.length storeAppScripts)
  let bprintf format = Printf.bprintf builder format
  let kbprintf continuation format = Printf.kbprintf continuation builder format

  let isScriptNonEmpty { Script = script } = script <> ""
  let writeAppScripts storeAppScripts () =
    let writeScripts scripts () = Seq.iter (fun { Script = script } -> bprintf "\r\n%s" script) scripts

    storeAppScripts
    |> Seq.groupBy (fun { App = app } -> app)
    |> Seq.iter (fun (app, group) -> kbprintf (writeScripts group) "\r\n\tuse Application = %s  " app)
    bprintf "\r\n"

  storeAppScripts
  |> Seq.filter isScriptNonEmpty
  |> Seq.groupBy (fun { Store = store } -> store)
  |> Seq.iter (fun (store, group) -> kbprintf (writeAppScripts group) "use Store=%s" store)

string <| builder.Remove (builder.Length - 2, 2)

First I put back arguments because it's no more just function composition in the bodies.
I created a StringBuilder and gave him an initial capacity
The Seq.length means a first loop over the items so the diff in perf comes probably from there but without a proper capacity it'll suffer from many internal buffer resizing
Then I made some helper for bprintf and kbprintf embedding the builder
eta-reduction isn't possible for bprintf because it would then be a value and not a function so it wouldn't be generic
I renamed the inner function to reflect the change in code they now return unit
The extra unit parameter isn't strictly required it just allow me to do eta-reduction at call site as kbprintf continuation argument expect a unit -> 'a
The rest of the code is nearly the same ; some Seq.map changed in Seq.iter because of the unit return.
The Seq.fold vanished which probably means I could simplify the Seq.map (...) >> Seq.fold in just a fold in the first version
At the end I remove the trailing "\r\n" added by the last call to writeAppScripts ; I wanted to find a better way without luck.

Now a little explanation about how bprintf and kbprint works :

bprintf is "easy" it's a printf which write it's content into the Stringbuilder given (same principle as fprintf with a file)
kbprintf do the same but take a continuation (a function) which will be executed after (for example to transform the result into another object)
I could have not used it and just put multiple bprintf instead
So for example kbprintf (writeAppScripts group) "use Store=%s" store will first write "use Store=XXX" into the StringBuilder and then call writeAppScripts on the group value which will add it's content into the StringBuilder


Sticking to a working proxy generated by a rotating proxy script I've created a script in python to make proxied requests by picking working proxies from a list of proxies scraped from a free proxy site. The bot traverses few links to parse the urls of the different posts from a website. The script however uses a new working proxy every time it makes a new requests as there are multiple requests to make.

At this point I've rectified the logic within my script in such a way so that the script will first check whether the existing proxy is still working in making new requests. If it is still a working proxy then the script should stick to it otherwise It will pick a random one from the list to go on.

The logic to reuse the same working proxy in multiple requests (until it is invalid) is defined within this start_script() function.

The script eventually has got a weird look. I suppose there are rooms for improvement to make it more concise and less verbose.

This is what I've created so far (working one):

import random
import requests
from random import choice
from bs4 import BeautifulSoup
from urllib.parse import urljoin

test_url = 'https://stackoverflow.com/' #It is for testing proxied requests
base_url = 'https://stackoverflow.com'
main_urls = ['https://stackoverflow.com/questions/tagged/web-scraping?sort=newest&page={}&pagesize=50'.format(page) for page in range(2,5)]
cbool = False
usable_proxy = None

def get_proxies():
    response = requests.get("https://www.sslproxies.org/")
    soup = BeautifulSoup(response.text,"lxml")
    proxies = [':'.join([item.select_one("td").text,item.select_one("td:nth-of-type(2)").text]) for item in soup.select("table.table tr") if "yes" in item.text]
    return proxies

def get_random_proxy(proxy_vault):
    while proxy_vault:
        print("trying to get one----")
        random.shuffle(proxy_vault)
        proxy_url = proxy_vault.pop()
        proxy_dict = {
            'http': proxy_url,
            'https': proxy_url
        }
        try:
            res = requests.get(test_url, proxies=proxy_dict, timeout=10)
            res.raise_for_status()
            return proxy_url
        except:
            continue

def start_script(url):
    global cbool
    global usable_proxy
    if not cbool:
        proxy = get_proxies()
        random_proxy = get_random_proxy(proxy)
        if random_proxy:
            usable_proxy = {'https': f'http://{random_proxy}'}
            urls = make_requests(url,usable_proxy)
            cbool = True
            return urls
        else:
            return start_script(url)
    else:
        urls = make_requests(url,usable_proxy)
        if urls:
            return urls
        else:
            cbool = False

def make_requests(url,proxy):
    try:
        res = requests.get(url, proxies=proxy, timeout=10)
    except Exception:
        return start_script(url)
    print("proxy used in requests:",proxy)
    if res.status_code!=200:
        return start_script(url)
    soup = BeautifulSoup(res.text, "lxml")
    return [urljoin(base_url,item.get("href")) for item in soup.select(".summary .question-hyperlink")]

if __name__ == '__main__':
    for url in main_urls:
        print(start_script(url))

Do not recurse if the iterative solution is readily available. In Python it is particularly very important: Python does not optimize tail recursion, and there is a serious chance to hit the stack limit.

For example, make_request should look like

while True:
    try:
        res = requests.get(url, proxies=proxy, timeout=10)
    except Exception:
        continue
    print("proxy used in requests:",proxy)
    if res.status_code!=200:
        continue
    soup = BeautifulSoup(res.text, "lxml")
    return [urljoin(base_url,item.get("href")) for item in soup.select(".summary .question-hyperlink")]

Similarly, start_script shall also be converted into a loop. As a side benefit, there would be no need for the very alarming usable_proxy and cbool globals.
You shall not blindly retry on res.status_code!=200. Some status codes (e.g. a 400 family) guarantee that you will get the same error over and over again, resulting in the infinite loop.

Ditto for exceptions.


Search for everything at a tile in Minesweeper I'm writing a Minesweeper clone. I needed a function that returns everything located at a certain tile, and ended up with an atrocious mess. I'd like help making it cleaner and more idiomatic.

The main state of the board is represented by a Field record:

(defrecord Field [dimensions ; Pair: [width height] in cells
                  flags ; Set
                  bombs ; Set
                  hints ; Map: Position -> hint value
                  uncovered-cells]) ; Set

Cell contents are tracked using sets for flags and bombs (since I only care about whether or not they're there), and a map for the hints (since I need to track the value of the hint as well). The keys for the sets/map are [x y] pairs representing a coordinate position, while the value for the hint map is a number between 1 and 8.

As mentioned above, I needed a function that returns everything at a certain coordinate. I ended up with this mess:

(defn- get-by-key [field x y key]
  ((key field) [x y]))

(defn all-at
  "Returns a set of everything at the position.
  Possible contents are :bomb, :flag, or a number representing a hint."
  [field x y]
  (let [g #(get-by-key field x y %)
        flag? (if (g :flags) :flag nil)
        bomb? (if (g :bombs) :bomb nil)
        hint? (g :hints)
        hint'? (if hint? hint? nil)]
    (into #{} ; So it can be easily searched for membership
          (filter #(not (nil? %))
                  [flag? bomb? hint'?]))))

I'm thoroughly unhappy with this though. It's giant for something that superficially seemed like a simple job, and has a lot of copied parts. The need for the filter also seems wrong, especially since I immediately force it into a set, so all laziness is gone.

I need it to check :flags, :bombs, and :hints to see if they contain the given coordinate, and if they do, return a set of everything at the position.

How can I write this better? Any suggestions here would be appreciated.

A few things:

I'd suggest that get-by-key can more clearly be defined as an inline lambda using get-in instead of implicitly calling the keys. You can also inline the construction of the flag?-bomb?-hint? vector to cut down on definitions.

I wouldn't worry about the filter immediately going into a set. Sets do one thing and filter does another; the laziness doesn't need to come into play.

Also, this:

(if (g :flags) :flag nil)

is more idiomatic as

(when (g :flags) :flag)

and

#(not (nil? %))

is

some?

Here is a shorter version of your function, taking those into account and not declaring temporary bindings for flag?, bomb?, hint?. I think in this fairly simple scope, that's fine.

(defn all-at
  "Returns a set of everything at the position.
  Possible contents are :bomb, :flag, or a number representing a hint."
  [field x y]
  (let [get-at #(get-in field [% [x y]])]
    (->> [(when (get-at :flags) :flag)
          (when (get-at :bombs) :bomb)
          (get-at :hints)]
         (filter some?)
         (into #{}))))


Better practice to find the max/min element in a loop Is this the most elegant way to avoid declaring additional variables outside the       following_triggers loop?

      closest_trigger_utc_time = (2**(0.size * 8 -2) -1)
  following_triggers.each do |test|
    closest_trigger_utc_time = test.utc_time if test.utc_time < closest_trigger_utc_time
  end

Enumerable has many useful methods that solve problems like this. In this case you want map and min or min_by:

closest_trigger_utc_time = following_triggers.map{|test| test.utc_time}.min

2**(0.size * 8 -2) -1 is a bad way to write infinity. If you must have an infinity value, use Float::INFINITY instead, but it's better to avoid this. min returns nil for empty input, so it's convenient to use that as the default value, or replace it with something else by closest_trigger_utc_time || some_meaningful_default.


Asynchronous networking iOS API calls I have the code above in my iOS SDK that I am building that makes it easy for users of the SDK to make API calls.  Basically, users can specify an API endpoint and easily make API calls.

Is there any way to improve the code above to make it more maintainable and easy for users?  Is that the proper way to handle asynchronous networking in iOS?

+ (void) makeRequestToEndPoint:(NSString *) endpoint values:(NSMutableDictionary *) params onCompletion:(SDKCompletionBlock) responseHandler
{
    NSString * urlString = [self createApiUrlFromEndpoint: endpoint];
    NSURL * url = [NSURL URLWithString: urlString];

    NSMutableURLRequest *request = [[NSMutableURLRequest alloc] initWithURL: url];
    request.HTTPMethod = @"POST";
    [request setValue:@"application/json" forHTTPHeaderField:@"Accept"];
    [request setValue:@"charset" forHTTPHeaderField:@"utf-8"];
    [request setValue:@"application/json" forHTTPHeaderField:@"Content-Type"];
    request.HTTPBody = [[params urlEncodedString] dataUsingEncoding:NSUTF8StringEncoding];

    [NSURLConnection sendAsynchronousRequest:request
                                       queue:[NSOperationQueue mainQueue]
                           completionHandler:^(NSURLResponse *response, NSData *data, NSError *error)
    {

        NSDictionary * dictionary = nil;
        NSError * returnError = nil;
        NSString * errorCode = nil;
        NSString * errorText = nil;
        NSInteger newErrorCode = 0;

        if([data length] >= 1) {
            dictionary = [NSJSONSerialization JSONObjectWithData: data options: 0 error: nil];
        }

        if(dictionary != nil) {

             if([dictionary objectForKey: @"error_code"] != nil) {
                 errorCode = [dictionary objectForKey: @"error_code"];
                 errorText = [dictionary objectForKey: @"error_description"];
             }
        }

        NSInteger statusCode = [(NSHTTPURLResponse *)response statusCode];

        if(statusCode >= 400)
        {
            if(errorCode == nil)
            {
                newErrorCode = -1;
                errorText = @"There was an unexpected error.";
            }
            else {
                newErrorCode = [errorCode intValue];
            }

            NSMutableDictionary* details = [NSMutableDictionary dictionary];
            [details setValue: errorText forKey:NSLocalizedDescriptionKey];

            returnError = [NSError errorWithDomain: WPAPPErrorDomain code: newErrorCode userInfo: details];
        }

        responseHandler(dictionary, returnError);

    }];
}

NSDictionary * dictionary = nil;
NSError * returnError = nil;
NSString * errorCode = nil;
NSString * errorText = nil;

This is superfluous and unnecessary.  It's more than fine to just declare them.  They're nil by default.

NSDictionary *dictionary;
NSError *returnError;
NSString *errorCode;
NSString *errorText;

You should use modern syntax for your mutable dictionaries.

Rather than [someDictionary setValue:someValue forKey:someKey];, you can and should simply do:

someDictionary[someKey] = someValue;

And the same can be done for accessing the variable.  Rather than NSString *foo = [someDictionary objectForKey:someKey];, you can and should simply do:

NSString *foo = someDictionary[someKey];

Most Objective-C programmers I know (and certainly myself included) don't particularly like foo == nil or foo != nil.  We can instead use:

if (foo) // foo != nil

And

if (!foo) // foo == nil

I don't know what exactly createApiUrlFromEndpoint: does for you, as it's not included in the review.  But I'd argue, strictly speaking, that this method (the one posted for review) should probably take an NSURL for the endpoint argument and createApiUrlFromEndpoint: should be a public method.  If for no other reason, it could save multiple calls to createApiUrlFromEndpoint: if a user wants to use the same URL multiple times with a different params dictionary.

Moreover, as the method is called "create API URL", it should return an NSURL object, not an NSString object.

if([data length] >= 1)

I personally thing >= and <= are ugly and only use them when there's no alternative (or the alternative is even uglier).  In this case, length is an unsigned integer, so if([data length] > 0) is exactly the same... except less ugly in my opinion.

Your method is called makeRequestToEndPoint:values:onCompletion:

I'm going to argue that values: should be changed to params: since that's what you're naming the the variably internally.

I'm also going to argue that onCompletion: should be changed to completionHandler: or completion: so that it matches every other existing Apple method which takes a completion block as an argument.

dictionary = [NSJSONSerialization JSONObjectWithData: data options: 0 error: nil];

You've got an error object.  You call a method that could have an error, but you don't care at all about the possible error that could happen within this method?  You should send &returnError rather than nil as the last argument, and immediately follow this call with:

if (error) {
    responseHandler(dictionary,returnError);
    return;
}

There's no point in continuing to execute code in the method if all the other code relies on the dictionary being initialized correctly, AND if the dictionary wasn't initialized correctly AND we're going to return some sort of an error, we should at least care about the one that goes with this method and will give a decent description of what problem occurred.

To add to the previous point, this is all happening within a completion block which sends as an argument an NSError object.  So the absolute first thing we should do before we try to initialize the dictionary is check this error object, and just as before, if it exists, call responseHanlder(nil,error); and return out of the method.

As it stands, other than the points I made above, the code seems decent enough.  For completeness however, you might implement a way of accomplishing this within the same class with the delegate-protocol pattern, rather than completion blocks.

To do this, you'd need to create a protocol, add a delegate property (which conforms to the protocol and is a weak property), and add a instance method version of this class method--the primary difference being that the instance method doesn't take a completion block argument, and instead of executing a completion block, it calls the delegate methods.

Unlike Ravi D's answer, however, the delegate methods defined in your protocol need more arguments so they can send more detail.

You could create a single method that's called regardless of whether or not there's an error:

@required - (void)fooBarObject:(FooBarObject *)fooBarObject
            didCompleteRequest:(NSDictionary *)results
                         error:(NSError *)error;

Where the first part of the method is sending a self argument (similar to tableView:cellForRowAtIndexPath:, etc), the second argument is the results dictionary (though would be nil in the case of an error) and the third argument is the error (and would be nil when there's not an error.  These leaves it up to the delegate to handle checking whether or not there was an error in this method.

Alternatively, you can require two methods, one for success and one for failure.  You're checking that in your methods, and the delegate doesn't have to do the check--simply has to implement a method for each scenario:

@required - (void)fooBarObject:(FooBarObject *)fooBarObject
            didCompleteRequest:(NSDictionary *)results;

@required - (void)fooBarObject:(FooBarObject *)fooBarObject
                requestDidFail:(NSError *)error;


Using Task and actions for simple threading? My question is regarding a rather inelegant solution to a problem that I came up with a while ago.

I was making a Winform Application to access active directory among other things and needed to thread my application to stop my UI from freezing.

Although I have little knowledge of actual threading (I kept hitting cross-thread access issues), I've built a little function that allowed me to pass an object, act on it in a separate thread and then once it returned to the UI CurrentContext I could act on it again to update the view.

TL;DR

The below function sends a Generic Item to a separate thread, then once acted upon, returns the item and acts on it again.

public static void doThreadedQuery<T>(Func<T> processQuery, Action<T> onComplete)
{
    TaskScheduler scheduler = TaskScheduler.FromCurrentSynchronizationContext();
    Task<T> doQuery = new Task<T>(processQuery);
    doQuery.ContinueWith((results) => { if (onComplete != null) onComplete(results.Result); }, scheduler);
    doQuery.Start();
}

An example usage for something similar is pass an activeDirectory SearchResultCollection, parse through it and generate a TreeView/Grid. Then return the populated grid and UI updated without freezing.

Now I have done variants with a progress function/cancellation token etc, but I suppose my question is: on a base level, is there some horrible inefficiency or simple equivalent I am unaware of (aside from the Background Worker I am not overly fond of)?

I feel like I'm re-inventing the wheel here.

You shouldn't usually use Task constructor directly, use Task.Factory.StartNew (or Run).

I also don't understand why you want to have a separate method for starting a task. It is completely superficial. It's just as useless as adding Print method that calls Console.WriteLine instead of using Console.WriteLine directly.

Normally, you just do this:

void SomeUICode ()
{
    Task.Factory.StartNew (() => {
        // This will run on thread pool
        return DoSomeExpensiveWork ();
    }).ContinueWith (result => {
        // This will run on UI thread
        UpdateUI (result);
    }, TaskScheduler.FromCurrentSynchronizationContext ());
}

It's basically  the same thing you wrote, but without abstracting it away doThreadedQuery since there's nothing to abstract away here.

However, if you don't mind using C# 5, you can simplify things with async/await operators:

async void SomeUICode ()
{
    var result = await Task.Factory.Run (() => {
        // This will run on thread pool
        return DoSomeExpensiveWork ();
    });

    // The rest of this method will be scheduled on UI thread:
    UpdateUI (result);
}

Note the await keyword that “splits” the method and schedules the rest of it as a continuation on current synchronization context (exactly the same thing you did manually).

I declared the function as async void but you should only do this for event handlers (which have to be void). In all other cases, declare the function as async Task so the calling code has a chance to wait for it to finish:

async Task SomeUICode ()
{
    // ...
}

On a side note, doThreadedQuery is a rather unfortunate name because

.NET method names should be PascalCase, not camelCase;
There's no “query” being performed, you're just scheduling an operation.


Recursive spinlock for C using ASM My history of C/ASM locks here in codereview:

Simple spinlock for C using ASM
Simple spinlock for C using ASM, revision #1 (basis for this approach. If this one fails, then so will the one below.)

Once you have gotten taste of assembly, there is no turning back. I don't care if I
am not ready/worthy, I going to step things up a bit and be a little bit more adventures.
So I went ahead and made a lock that I hope is able to handle recursions.
I will without a doubt fail very painfully and the mistake is also going to be hard to spot.
No, I am not pessimistic. Just like gravity works to put things down, so does my reasoning skills to me.
It is up to you to give me tough love and spot my faulty reasoning and throw bananas at me!

The code in question.

// "gate_" is the namespace marker for higher level languages that might import this code... as if...
#define gate_Gate volatile int
#define gate_Pass volatile int

extern inline void
gate_Enter
(gate_Gate *gate, gate_Pass *pass)
{
    asm volatile (
        "jmp gate_Enter_check\n"     // Skip the line.
        "gate_Enter_wait:\n"         // Wait in line.
        "pause\n"                    // Wait a long time.
        "gate_Enter_check:\n"        // Now we are at the gate.
        "cmp %[lock], %[checkin]\n"  // See if our pass is any good.
        "jge gate_Enter_skip\n"      // Skip if pass >= lock.
        "mov %[lock], %%eax\n"       // eax = 1.
        "lock xchg %%eax, %[gate]\n" // Attempt to validate our pass and connect it to the gate.
        "test %%eax, %%eax\n"        // Hope for the best.
        "jnz gate_Enter_wait\n"      // There is no hope, go back in line.
        "gate_Enter_skip:\n"         // We are VIP!
        "add %[lock], %[checkin]\n"  // Checkin pass like pro!
        : [gate] "=m" (*gate), [checkin] "=m" (*pass)
        : [lock] "r" (1)
        : "eax"
    );
}

extern inline void
gate_Leave
(gate_Gate *gate, gate_Pass *pass)
{
    asm volatile (
        "cmp %[pass], %[isLast]\n"   // Have I checked in only once?
        "jg gate_Leave_skip\n"       // Skip next step if I have checked in more then once.
        "mov %[unlock], %[gate]\n"   // Close the gate because I am the last one to leave.
        "gate_Leave_skip:\n"         // Comment!
        "add %[checkout], %[pass]\n" // Checkout. This may or may not be the last time.
        : [gate] "=m" (*gate), [pass] "=m" (*pass)
        : [unlock] "r" (0), [checkout] "r" (-1), [isLast] "r" (1)
    );
}

// Example use
gate_Gate gate = 0;
gate_Pass exampleUsePass = 0;
void exampleUse(int count)
{
    printf("pass=%d\n", exampleUsePass);
    gate_Enter(&gate, &exampleUsePass);
    if (count == 3) {
        // exampleUsePass = 0; // force deadlock.
    }
    if (count < 5) {
        printf("Count = %d\n", count);
        exampleUse(++count);
    }
    gate_Leave(&gate, &exampleUsePass);
    printf("pass=%d\n", exampleUsePass);
}

exampleUse(0);

The question

The intent of this code is a lock that allows recursion.
Will this lock make exampleUse both threadsafe and reentrant if we allow our self to assume the pass, exampleUsePass is only used for that function?

If not? what is wrong?
If yes, it is still wrong; isn't it?... Give me a good rant about what to consider, what is missing or about better approach. If you want to suggest libraries, please restrict your self to C. I am not experienced enough to bind C++ stuff to other languages, so I often can't use the good stuff over there :'(

I don't believe your current code is thread safe.  Looking at your gate_Enter function:

The first thing you do is skip the wait loop, this is fine:

"jmp gate_Enter_check\n"
"gate_Enter_wait:\n"
"pause\n"

Then you check your reentry count.  If it's > 0 then you assume you are the the one holding the lock and skip the actual lock part.

"gate_Enter_check:\n"
"cmp %[lock], %[checkin]\n"
"jge gate_Enter_skip\n"
"mov %[lock], %%eax\n"
"lock xchg %%eax, %[gate]\n"
"test %%eax, %%eax\n"        // Hope for the best.
"jnz gate_Enter_wait\n"      // There is no hope, go back in line.

And increment the nesting level.

"gate_Enter_skip:\n"         // We are VIP!
"add %[lock], %[checkin]\n"  // Checkin pass like pro!

The issue is you don't actually know that the thread calling gate_Enter is the same one that originally entered the gate.  If another thread calls exampleUse function it's using the same pass. You could get around this by making use of thread local storage for your pass/checkin variable.  Or you could create exampleUsePass on the stack, then pass it in to recursive calls instead of using global vars.

You would have a similar issue with your gate_Leave method, where you unlock the gate, then decrement the nesting count.  It would be better to decrement the value, whilst you are within the locked section, something like this:

"add %[checkout], %[pass]\n"
"jnz gate_Leave_skip\n"       // If not zero, this is a nested call
"mov %[unlock], %[gate]\n"   // Close the gate because I am the last one to leave.
"gate_Leave_skip:\n"

As an aside, I'm not a huge fan of the way you've essentially renamed the pass parameter to your gate_Enter function to checkin.

[checkin] "=m" (*pass)

A slightly more robust version of your code, which will abort if there is an  unexpected exit from gate when you don't own the gate is shown below (along with a simple test harness).

#include <stdio.h>
#include <unistd.h>
#include <unistd.h>
#include <string.h>
#include <pthread.h>

#define gate_Gate volatile int
#define gate_Pass volatile int

/*extern*/ inline void
gate_Enter
(gate_Gate *gate, gate_Pass *pass)
{
    asm volatile (
        "jmp gate_Enter_check\n"     // Skip the line.
        "gate_Enter_wait:\n"         // Wait in line.
        "pause\n"                    // Wait a long time.
        "gate_Enter_check:\n"        // Now we are at the gate.
        "cmp %[lock], %[pass]\n"  // See if our pass is any good.
        "jge gate_Enter_skip\n"      // Skip if pass >= lock.
        "mov %[lock], %%eax\n"       // eax = 1.
        "lock xchg %%eax, %[gate]\n" // Attempt to validate our pass and connect it to the gate.
        "test %%eax, %%eax\n"        // Hope for the best.
        "jnz gate_Enter_wait\n"      // There is no hope, go back in line.
        "gate_Enter_skip:\n"         // We are VIP!
        "add %[lock], %[pass]\n"  // Checkin pass like pro!
        : [gate] "=m" (*gate), [pass] "=m" (*pass)
        : [lock] "r" (1)
        : "eax"
    );
}

/*extern*/ inline void
gate_Leave
(gate_Gate *gate, gate_Pass *pass)
{
    asm volatile (
       "add %[checkout], %[pass]\n"
       "js error_abort\n"            // Abort, underflow on *pass
       "jnz gate_Leave_skip\n"       // Skip next step if I have checked in more then once.
        "mov %[unlock], %%eax\n"
        "lock xchg %%eax, %[gate]\n"
        "test %%eax, %%eax\n"
        "jnz gate_Leave_skip\n"      // If zero, abort because we unlocked an unlocked gate!
        "error_abort:\n"
        "call abort\n"
        "gate_Leave_skip:\n"
        : [gate] "=m" (*gate), [pass] "=m" (*pass)
        : [unlock] "r" (0), [checkout] "r" (-1), [isLast] "r" (1)
        : "eax"
    );
}

// Example use
gate_Gate gate = 0;
__thread gate_Pass exampleUsePass = 0;  // notice this uses thread
                                        // local storage
void exampleUse(int count)
{
    printf("pass=%d\n", exampleUsePass);
    gate_Enter(&gate, &exampleUsePass);
    if (count == 3) {
        // exampleUsePass = 0; // force deadlock.
    }
    if (count < 5) {
        printf("Count = %d\n", count);
        exampleUse(++count);
    }
// uncomment this line to simulate Leaving twice when we owned the gate
//gate_Leave(&gate, &exampleUsePass);
    gate_Leave(&gate, &exampleUsePass);
    printf("pass=%d\n", exampleUsePass);
}

void *doSomeThing(void *arg) {
 // Uncomment lines in this method to force alternate thread to
 // try to leave gate, when it doesn't own it.
 //  if(arg == NULL)
        exampleUse(0);
 //   gate_Leave(&gate, &exampleUsePass);
    return 0;
}

pthread_t tid[2];

int main(void)
{
    int i = 0;
    int err;

    while(i < 2)
    {
        err = pthread_create(&(tid[i]), NULL, &doSomeThing, i?&err:0);
        if (err != 0)
            printf("\ncan't create thread :[%s]", strerror(err));
        else
            printf("\n Thread created successfully\n");

        i++;
    }

    sleep(5);
    return 0;
}


Minimal stack from unsigned int to track boolean values wtih bits This grew out of a need to track sequential ctrl+shift+Left/Right-arrow keypress events on a gtk editor. It is a small stack implementation for boolean values (for LEFT, RIGHT) that uses the bits of a predefined array of unsigned for storage. The amount of storage is determined by the constant STKMAX which simply specifies the number of unsigned elements in the array. The reason for using this as opposed to some builtin GtkStack, etc. was simply for storage efficiency where only 32+ bits were needed, compared to Gtk's traditional over-allocation for minimal data structures.

Here, I'm not that concerned with whether I used unsigned where uint32_t would be better, but more whether there is anything inherently wrong with packing bits and indexing the various array elements that make up the stack storage that I may be overlooking. I've thought through it and the logic and implementation seems sound -- but I could have easily missed something that will come back to bite me.

With that, here's the code and a short driver that intentionally attempts to add more than the stack will hold to validate stack-full behavior:

#include <stdio.h>

#ifndef CHAR_BIT
# define CHAR_BIT  8
#endif

enum { LEFT, RIGHT, STKMAX = 4 };

typedef struct {
    unsigned int kp[STKMAX], idx;   /* array for stack, bit-index */
} selstack;

/** zero the stack values */
void stack_clear (selstack *s)
{
    s->idx = 0;
    for (int i = 0; i < STKMAX; i++)
        s->kp[i] = 0;
}

/** stack_push set bit at index to 0 or 1 based on v.
 *  increment bit-index after setting bit.
 */
int stack_push (selstack *s, int v)
{
    /* total bits, bits per-array-element, array index */
    unsigned arrbits = (sizeof s->kp) * CHAR_BIT,
        elebits = (sizeof s->idx) * CHAR_BIT,
        arridx = s->idx/elebits;

    if (s->idx == arrbits) {
        printf ("stack full, index: %u\n", s->idx);
        return 0;
    }

    if (v)  /* set bit at index */
        s->kp[arridx] |= (1u << (s->idx % elebits));
    else    /* clear bit at index */
        s->kp[arridx] &= ~(1u << (s->idx % elebits));

    return ++s->idx;
}

/** stack_pop - decrement index, get bit at index, clear bit. */
int stack_pop (selstack *s)
{
    if (!s->idx) {
        printf ("stack empty, index: %u\n", s->idx);
        return -1;
    }

    /* number of bits per-array-element, decrement index */
    unsigned elebits = (sizeof s->idx) * CHAR_BIT,
        arridx = --s->idx/elebits,
        /* get bit value at index */
        v = (s->kp[arridx] >> (s->idx % elebits)) & 1;

    /* zero bit at index */
    s->kp[arridx] &= ~(1u << (s->idx % elebits));

    return v;
}

/** stack_last - get bit for last pushed value. */
int stack_last (selstack *s)
{
    if (!s->idx) {
        printf ("stack empty, index: %u\n", s->idx);
        return -1;
    }

    /* number of bits per-array-element, computed index */
    unsigned elebits = (sizeof s->idx) * CHAR_BIT,
        arridx = (s->idx - 1)/elebits;

    /* return bit at index */
    return (s->kp[arridx] >> ((s->idx - 1) % elebits)) & 1;
}

int main (void) {

    int v;
    selstack s;
    stack_clear (&s);

    for (int i = 0; i < 132; i++)
        if (i < 32 || (64 <= i && i < 96))
            stack_push (&s, RIGHT);
        else
            stack_push (&s, LEFT);

    while ((v = stack_pop (&s)) >= 0)
        printf ("popped[%3u] : %s\n", s.idx, v ? "RIGHT" : "LEFT");

    return 0;
}

Let me know if there are any gotchas I'm not picking up on.

Reading this:

enum { LEFT, RIGHT, STKMAX = 4 };

I expect to see use LEFT and STKMAX interchangeably however they're different things you packed together for convenience. Split it and readers won't need to browse all your code to understand that they can't use STKMAX where FALSE or TRUE are expected. Is stack_push (&s, STKMAX); valid and meaningful?

You already know you may use uint32_t instead of unsigned int but, at least, you should really use unsigned, the extra int in the declaration adds nothing to your code (but that's just an opinion then take it or leave it).

unsigned int kp[STKMAX], idx;

I prefer, for clarity, to have one single variable declaration for each line. I'd also change names to something more meaningful. For example, why idx can't be indexOfNextAvailableBit? We don't need to obfuscate our code and I do not neet to read stack_push() to understand how it's used to determine what it is.

stack_clear() can be simplified to use memset(), any decent compiler will (probably) recognize it and use the best algorithm to perform the task (loop, unrolled loop, plain memset(), something more esoteric/fancy).

memset(s->kp, 0, sizeof(unsigned)*STKMAX);

As noted by chux you better use memset(s->kp, 0, sizeof s->kp[0] *STKMAX) "as it reduces a dependency on matching a variable with a type - which may evolve".

In stack_push() you should again declare one variable each time, no need to save few keystrokes and it makes declaration easier to read. Also here you should use more meaningful names, ideally I would like to read the variable name and go to the next line, there should be no need to read and understand the expression to determine what it is going on. I'm stressing on this but it's a very important concept when after 1 year you go back to  your 500k/1M LOC codebase to fix a bug.

It's not a big compile-time protection but you have an enum, give it a name and use it for the parameter v (which may be value). Callers are still able to push 41 instead of TRUE and FALSE but you're documenting your function interface (and see next paragraph).

You may want to validate inputs.

Do not print error messages to stdout. For two reasons: first because stdout is for program output and errors should go to stderr, second because it's not your code responsibility to inform the user, you're just handling a stack of booleans and an appropriate return code is enough.

/* set bit at index */

If you feel you need a comment for this then you're missing the opportunity to introduce a separate self-explicative setBitAtIndex() method.

Do you really need to return the index of the next available bit? It's never a parameter for your functions then caller is probably not interested in it (and it's available in selstack). You may want to use return value as error code (eventually, from C11, consider to use errno_t).

Above applies to stack_pop() and stack_last() as well.


Player assignment in Tic-Tac-Toe I am building my first OOP program, Tic Tac Toe.  I want to conquer it in chunks, but the first part for me is the intialization of the program and the creation of the two players.  I want to get some feedback about how I structured this before I move on to build the board and the actual gameplay logic.

For instance, should I have made the player_names assignment method part of the Player class?  Or is the parent TicTacToe class good?

class TicTacToe

    def initialize
        @player1 = Player.new
        @player2 = Player.new
        game_sequence
    end

    def game_sequence
        puts "Let us play a nice game of Tic Tac Toe!"
        player_names

    end

    def player_names
        puts "Player 1, what is your name?"
        @player1.name = gets.chomp
        puts "Thanks, #{@player1.name}!  Would you like to be X or O?"

        answer = ""

        until ["X","O"].include?(answer)
            answer = gets.chomp
            if ["X","O"].include?(answer)
                @player1.mark = answer
                break
            end
            puts "You must enter in either X or O"
        end

        case @player1.mark
        when "X"
            @player2.mark = "O"
        when "O"
            @player2.mark = "X"
        end

        puts "#{@player1.name}\'s mark is #{@player1.mark} and Player 2\'s mark is #{@player2.mark}"
        puts "Player 2, what is your name?"
        @player2.name = gets.chomp

    end

    class Player

        attr_accessor :name, :mark

        def initialize(name = nil, mark = nil)
            @name = name
            @mark = mark
        end

    end

end

TicTacToe.new

When coming up with an OO design, always ask yourself "What is the responsibility of this class?" There should be only one. If you have to describe it with a bunch of ANDs, e.g. "it runs the game and prompts for player names and sets up the board and..." then it is a sign you may want to break it down further. This is called the Single Responsibility Principle. Resist the temptation to put all functionality into one class that does everything, as it is very easy to grow the class into the antipattern known as the God Object. Sometimes naming helps. If instead of TicTacToe, you called the object GameRunner (for example), you may be less tempted to put any and all Tic Tac Toe related things into it.

In your case, I'd try to keep TicTacToe as lean as possible. Prompting for a player's name should be something a Player object knows how to do, so I'd move that to the Player class. Also prompting for a mark can certainly be moved into the Player class. Possible interface:

marks = ["X", "O"] #this would be in the initialization
player1.mark = player1.select_mark(marks)
player2.mark = marks - player1.mark

Also, you'll want to create an object to represent the game board.


Matrix: Set a row/col to zero A practice interview question:

  Write an algorithm such that if an element in an MxN matrix is 0, its entire row and column is set to 0.

void SetZeroOnMatrix(int **matrix, int M, int N)
{
    bool *rowZeros = new bool[M];
    bool *colZeros = new bool[N];

    // Find the 0's
    for(int i=0; i<M; i++)
    {
        for(int j=0; j<N; j++)
        {
            if(matrix[i][j] == 0)
            {
                rowZeros[i] = false;
                colZeros[j] = false;
            }
        }
    }

    // Replace the values
    for(int i =0;i<M;i++)
    {
        for(int j=0;j<N;j++)
        {
            if(!rowZeros[i]||!colZeros[j])
                matrix[i][j]=0;
        }
    }

    delete[](rowZeros);
    delete[](colZeros);
}

If \$N = M\$, I'm assuming that this would have a time complexity of \$O(N^2)\$ and a space complexity of \$O(N)\$.

Don't dynamically allocate objects if you don't need to.

bool *rowZeros = new bool[M];
bool *colZeros = new bool[N];

If you do it this way you need to take control and do manual memory management (a bad idea).

Most specifically your code is not exception safe. If your code or anything you called throws an exception you leak memory (OK so this function is simplistic enough that it is unlikely but code has a tendency to evolve over time and you want to write code that is easy to maintain so writing it exception safe to begin with is a general goal).

Also C++ has a tendency to have early return backed into the code. This is because of automatic memory management that has been developed over the years. If somebody modifies your code to have an early return (say because they found no zero's on the first pass). Then your code is now likely to leak. So designing your code again t to use automatic memory management just makes your code more maintainable.

I would use a vector here:

std::vector<char> rowZeros(M, true);
std::vector<char> colZeros(N, true);

This allocates memory. But when the function exits all memory is correctly deallocated (even after early return or exception). Use standard container like automatic variables whenever you can.

Notice I did not use std::vector<bool>. Normally I would use the same type in the vector that I would have used in the array. But there is special consideration taken for std::vector<bool> that make it less efficient (everybody has concluded this was a mistake by the committee but it is not going to be fixed for backwards compatibility (at least not any time soon)).

Also note your original code contains a bug:

bool *rowZeros = new bool[M];  // This allocates the array but does not initialize.

Because the array is not initialized reading the elements is undefined behavior (until after they have been initialized). You could have initialized them to zero(false) with:

bool *rowZeros = new bool[M]();
                       //   ^^  forces zero initialization of all members.

But the vector constructor has a more obvious way of doing it.

std::vector<char> rowZeros(M, true);
                    //     ^               Size
                    //        ^^^^         Value copied into each cell of the vector.


Getting WiFi SSID on iOS in Swift I've seen a lot of Objective-C implementations but I'd like to do it in Swift.

I did it like this and it seems to work just fine. Does anyone have comments and/or improvements to make?

import SystemConfiguration

class EnvData: NSObject {

    class func getSSID() -> String {

        var currentSSID = ""

        let interfaces = CNCopySupportedInterfaces()

        if interfaces != nil {

            let interfacesArray = interfaces.takeRetainedValue() as [String]

            if interfacesArray.count > 0 {

                let interfaceName = interfacesArray[0] as String

                let unsafeInterfaceData = CNCopyCurrentNetworkInfo(interfaceName)

                if unsafeInterfaceData != nil {

                    let interfaceData = unsafeInterfaceData.takeRetainedValue() as Dictionary!

                    currentSSID = interfaceData["SSID"] as String

                } else {

                    currentSSID = ""
                }

            } else {

                currentSSID = ""
            }

        } else {

            currentSSID = ""
        }

        return currentSSID
    }
}

The first thing that stands out to me is that you've created an entirely new class that exists purely to have this class method.  This doesn't make a whole lot of sense.  At a minimum, this method could just exist on its own, outside of a class, but I'll do you one better: let's write it as an extension to UIDevice, that's where it makes the most sense, and there are already other similar properties on UIDevice...

extension UIDevice {
    public var SSID: String {
        get {
            // your code
        }
    }
}

Now, getting the SSID will be similar to getting the device name, or system version, or other useful things about the device you might need to know:

let ssid = UIDevice.currentDevice().SSID

Now then, on to the actual meat of your code...

For starters, I don't know much about getting SSIDs.  Is it at all possible that you wouldn't be able to fetch the SSID?  What is the likelihood of actually returning an empty string here?  Importantly, I think returning an empty string isn't actually the right thing to do, no matter how likely it is.

Instead, our return type should be a string optional: String? and we should return nil when something fails.

That way, it's easier on the end user to be certain this returned a valid value:

if let ssid = UIDevice.currentDevice().SSID {
    // do stuff
} else {
    // error fetching ssid
}

This seems easier than the alternative:

let ssid = UIDevice.currentDevice().SSID
if countElements(ssid) > 0 {
    // do stuff
} else {
    // error fetching ssid
}

So, with that out of the way, the only problem I have here is all of this unnecessary nesting and unnecessary variables:

public var SSID: String? {
    get {
        if let interfaces = CNCopySupportedInterfaces() {
            let interfacesArray = interfaces.takeRetainedValue() as [String]
            if let unsafeInterfaceData = CNCopyCurrentNetworkInfo(interfacesArray.firstObject) {
                let interfaceData = unsafeInterfaceData.takeRetainedValue() as Dictionary
                return interfaceData["SSID"]
            }
        }
        return nil
    }
}

Ultimately, there's no reason here to have the three layers of else blocks that all actually do nothing.  In your code you initialize currentSSID to an empty string, and then only change it to anything else if you can get the SSID.  Your three else blocks change currentSSID from an empty string to an empty string, so just omit them completely.


Read a txt file and split into 2 csvs based on regex So I have been told that my code is not good and needs refactoring. I'm struggling to see what I need to do. How do I specify the success and failure conditions for lambda? Also don't get what the method process_lines should contain. Any help is greatly appreciated!
These are the comments I was given:

Let's create a function (process _lines) that accepts 2 lambda functions (success, failure). The success will accept the function pointer that will write to good csv file, while failure to the bad csv file.
Something like:
def process_lines(success, failure):
...
def write_to_csv(csv_file):
...
process_lines(lambda: write_to_csv(good_csv), lambda: write_to_csv(bad_csv))

Currently this is my code:
import re
import csv

pattern = re.compile("([0-9]+) +([0-9\.-]+) +([0-9\.\-+Ee]+) +([0-9\.\-+Ee]+) +([0-9\.\-+Ee]+) +([0-9\.\-+Ee]+) +([0-9\.\-+Ee]+) +([0-9\.\-+Ee]+) +([0-9\.\-+Ee]+) +([0-9\.\-+Ee]+) +([0-9\.\-+Ee]+) +([0-9\.\-+Ee]+) +([0-9\.\-+Ee]+) +([0-9\.\-+Ee]+) +([0-9\.\-+Ee]+) +([0-9\.\-+Ee]+) +([0-9\.\-+Ee]+) +([0-9\.\-+Ee]+) +([0-9\.\-+Ee]+) +([0-9\.\-+Ee]+) +([0-9\.\-+Ee]+) +([0-9\.\-+Ee]+) +([0-9\.\-+Ee]+)")

good_csv = []
bad_csv = []
fname = "track_param_hist.txt"
good_csv_fname = "good_hist_csv.csv"
bad_csv_fname = "bad_hist_csv.csv"

class ConvertTxtToCsvs:
  def process(self):
    print("Script for splitting text file into two separate csvs...")
    # read in the text file and store the lines as a list
    with open(fname) as f:
      lines = f.readlines()

    # split the lines into good and bad
    for line in lines:
      good_csv.append(line) if pattern.match(line) else bad_csv.append(line)

    # create the csv for bad data
    with open(bad_csv_fname, 'w') as myfile:
      wr = csv.writer(myfile)
      for line in bad_csv:
        row = (' '.join(line.split())).split(' ')
        wr.writerow(row)
    print("Csv for the bad data has been created successfully")

    # create the csv for good data
    with open(good_csv_fname, 'w') as myfile:
      wr = csv.writer(myfile)
      for line in good_csv:
        row = (' '.join(line.split())).split(' ')
        wr.writerow(row)
    print("Csv for the good data has been created successfully")

# This can also be run as regular python script
if __name__ == "__main__":
  ConvertTxtToCsvs().process()

I really don't know why someone's recommending lambdas to you like that but it's not at all what I'd do. I do agree there's some refactoring that could be done here though.

Refactoring

Why is ConvertTxtToCsvs a class? It contains only one function, process, and no attributes. There's no need for it to be a class. Instead, have it function based, and you should split it up a bit. Notice how writing to the good and bad csvs are mirrors of each other? You should have one write_csv function. Since all the code is the same except the filename and the data, it's very easy to turn into a function:

def write_csv(filename, data):
    with open(filename, 'w') as myfile:
        wr = csv.writer(myfile)
        for line in data:
            row = (' '.join(line.split())).split(' ')
            wr.writerow(row)

And then it can be called for both like this:

write_csv(good_csv_fname, good_data)
print("Csv for the bad data has been created successfully")
write_csv(bad_csv_fname, bad_data)
print("Csv for the good data has been created successfully")

Which is much neater. Also notice I changed from using a 2 space indent to 4 spaces. That's suggested by the Python style guide and is much more readable. This is especially important since whitespace affects program flow in Python.

Also, you read all the lines of the file up front just to iterate over them anyway. It'd be easier and less resource hogging to just read it line by line. Python makes it very easy, as you can directly iterate over a file-like object.

with open(fname) as f:
    for line in f:
        good_csv.append(line) if pattern.match(line) else bad_csv.append(line)

And lastly, you create your two lists before the function is called. This is bad, because you couldn't call the function more than once without accidentally including all the old data. Instead create the two lists inside the function so with each function call you create new empty lists.

Style Notes

I linked the style guide just above and it's a good read, but there's some other stuff I'd like to specifically point out. Some of your namìng isn't helpful. good and bad are often poor descriptions. Why are they good? Does good mean valid data, matching data, useful data? Instead, consider a name more relevant to what that means. matched and unmatched perhaps? Similarly, I'd avoid the use of csv. A csv is a filetype, your lists are not csvs (yet), so you don't need to include that name.

And don't shorten filename to fname. That shortened three characters isn't worth the reduced clarity here. And instead of just filename it'd be better to use source or source_filename.

source_filename = "track_param_hist.txt"
matched_data_file = "good_hist_csv.csv"
unmatched_data_file = "bad_hist_csv.csv"
pattern = re.compile("([0-9]+) +([0-9\.-]+)" + "+([0-9\.\-+Ee]+)" * 21)
matched = []
unmatched = []

You have a lot of comments that you don't really need. Comments should explain complex code or intent that's confusing to follow. There's no need comment about using with open and readlines. These are simple to follow functions, Python is highly readable that way. This could be a good line to comment:

            row = (' '.join(line.split())).split(' ')

It took me a moment to realise that it's splitting and rejoining the sentence to ensure that there's only a one space gap instead of any multiple spaces. You should write a comment to clarify this as it's a slightly confusing solution.

Using a ternary in your for loop works fine, but is a little hard to read. Ternaries are really best used when they can remove duplicate code and shorten things. Your case is less lines, but just as many words. This might be a better use of a ternary:

good_csv.append(line if pattern.match(line) else "")

I'd personally think it's better to write the loop this way:

for line in lines:
    if pattern.match(line):
        matched.append(line)
    else:
        unmatched.append(line)

This is how I'd rewrite your whole script in the end:

import re
import csv

source_filename = "track_param_hist.txt"
matched_data_file = "good_hist_csv.csv"
unmatched_data_file = "bad_hist_csv.csv"
pattern = re.compile("([0-9]+) +([0-9\.-]+)" + "+([0-9\.\-+Ee]+)" * 21)

def write_csv(filename, data):
    with open(filename, 'w') as myfile:
        wr = csv.writer(myfile)
        for line in data:
            # Make sure all whitespace is reduced to one space character
            row = (' '.join(line.split())).split(' ')
            wr.writerow(row)

def process(source):
    print("Script for splitting text file into two separate csvs...")

    matched = []
    unmatched = []

    with open(source) as f:
        for line in f:
            if pattern.match(line):
                matched.append(line)
            else:
                unmatched.append(line)

    write_csv(matched_data_file, matched)
    print("Csv for the good data has been created successfully")
    write_csv(unmatched_data_file, unmatched)
    print("Csv for the bad data has been created successfully")

if __name__ == "__main__":
    process(source_filename)


Approximating pi/4 using Wallis Product - SICP exercise 1.31 From SICP

  The sum procedure is only the simplest of a vast number of similar
  abstractions that can be captured as higher-order procedures. Write an
  analagous procedure called product that returns the product of the
  values of a function at points over a given range.  ...  "Also
  use product to compute approximations to  pi using the formula
  pi/4 = 2*4*4*6*6*8*.../3*3*5*5*7*7*...

Here is a procedure called product:

(define (product term a next b)
  (if (> a b)
      1
      (* (term a) (product term (next a) next b))))

I have made two solutions:

Here is the first solution. This one navigates through the series by a period of 2 (E.G. if n is 1, it will evaluate 2*4/3*3). I did this because I wanted to find a pattern from the series and what I got was 2*4 and 4*6 and 6*8, and the denominators 3*3 and 5*5 and 7*7.

(define (pi-product2 n)
    (define (inc x) (+ x 1))
    (define (numer-term x)
      (* (* 2.0 x)
         (+ (* 2.0 x)
            2.0)))
    (define (denom-term x)
      (square (+
               (* 2.0 x)
               1.0)))
    (define (term x)
      (/ (numer-term x) (denom-term x)))
    (product term 1.0 inc n))

Here is my second solution. I just recently thought of how to evaluate them by each term (E.G. if n is 1, it will evaluate 2/3).

  (define (pi-product n)
    (define (inc x) (+ x 1))
    (define (term x)
      (if (even? x)
          (/ (+ x 2.0) (+ x 1.0))
          (/ (+ x 1.0) (+ x 2.0))))
    (product term 1.0 inc n))

Is my first solution still a correct representation of the formula even if it evaluates two terms on each approximation? Or is it the first solution with the correct representation?
I would think my first solution, though very ugly, is at least as twice as fast as the second solution. Is that right? Is the performance difference huge?

Whitch of my two solution is better? How can I improve them?

Is my first solution still a correct representation of the formula even if it evaluates two terms on each approximation? Or is it the first solution with the correct representation?

Although both solutions may be mathematically equivalent in theory, they are vastly different in practice.  The first uses floating-point constants, which leads to inexact math.  The second one uses exact math.

  How can I improve them?

In general, you should avoid using recursion directly when there are higher-level abstractions that fit.  For example, here are some ways you could restructure your code around streams:

Generate one stream for terms in the numerator and one stream for terms in the denominator.  Then use stream-map to divide successive terms.  Take the first n terms of the result, and reduce with +.
Produce a stream of successive approximations of \$π\$.  There are several ways to do this.  For example, you could use cons-stream, but it would also be possible (if you define a few auxiliary functions) to tweak the strategy outlined in the preceding point.  Then take the nth item of this stream.


Partitioning array elements into two subsets Problem:

  Given a randomly ordered array of n-elements, partition the elements
  into two subsets such that elements <= x are in one subset and elements

    x are in the other subset.

  One way to do this is to sort the array in ascending order. But with
  this solution we actually ordered the array in addition to partition.
  Also sort is costly.

  For example, let's take this array \$[37,36,27,11,15,12,34,29,4,10]\$
  and partition value is 17.

  So the array after partition should be
  \$[10,4,12,11,15,27,34.29.36,37]\$

  Comparing these two data sets we can see that elements from left side
  greater than 17 must be moved to right side and at the right side
  elements less than 17 to left side.

  Initially we do not know how many elements are less than < 17. One
  way to solve this, pass through the array and count the number of
  elements less than 17. Once we know the partition value (here it is
  15), then make another pass through the array to make the exchange.
  But this is \$0(n^2)\$.

  We need to find a solution where this can be done with only pass
  through the array. Looking at these data sets we can find that even if
  we do not know the partition value(here it is 17), 37 can be exchanged
  with 10, 36 can be exchanged with 4 and so on.

  This is something like left partition is growing to right and right is
  growing to left.

| Left Partition ->    ||   <- Right Partition |
^                    ^                       ^
i=0            partition point           j=n (length of array)

  So we get the basic mechanism to exchange, while two partitions have
  not met do

  Extend the left and right partitions inward exchanging wrongly  placed elements.
  Exchange can be done by the standard technique

t = a[i];
a[i] = a[j];
t = a[j];

  Moving inwards from left and right can be done by,

  from left to right: while(a[i] <= x) i = i + 1;

  from right to left: while(x < a[j]) j = j - 1;

  where a is the array, i is the starting index 0, j is last index
  that is the length of the array.

  One case is, initially we don't know a[i] and a[j] should be
  exchanged since there is no initial move inwards from the left and
  right to check if an exchange is really necessary. we can overcome
  this by placing a check before passing through the array:

while(a[i] <= x) i = i + 1;
while(x < a[j]) j = j - 1;

  Algorithm (it is based on R. G. Dromey's How to solve it by computer):

  Establish an array \$a[1...n]\$ and partition value x.
  Move the two partitions towards each other until a wrongly placed pair of    elements is encountered.
  While two partitions have not met or crossed do

  exchange wrongly placed pairs and extend both partitions inwards    by one element.
  extend left partition while elements less than or equal to x
  extend right partition while elements greater than x

  Print partitioning index p and partitioned array.

Here is my solution:

public class PartitionArray {

  public static void main(String args[]) {

    int[] elements = {37, 36, 27, 11, 15, 12, 34, 29, 4, 10};

    int x = 17;
    int p = 0;

    int i = 0;
    int j = elements.length - 1;

    while (elements[i] <= x && i < j) i = i +1;
    while (x < elements[j] && i < j) j = j - 1;

    int tmp;

    while (i < j) {
      tmp = elements[i];
      elements[i] = elements[j];
      elements[j] = tmp;

      i = i + 1;
      j = j - 1;

      while (elements[i] <= x && i < j) i = i +1;
      while (x < elements[j] && i < j) j = j - 1;

      p = j;
    }

    print("P: " + elements[p]);
    print("Partitioned Array: " + Arrays.toString(elements));
  }
}

Unless it is a homework for sort algorithm study :

Manage ArrayList and load your list of number,
put all number<=x in a new ArrayList,
delete them from the original ArrayList,
finish with .addAll(..), to concatenate if needed.

Knowing that VM is optmized, it would be interesting to compare performance of this approach with your one.


Simple animation method with requestAnimationFrame code structure I'm writing a animate method to perform DOM animations. I know there are a lot of libraries out there that already does that (and I use some ideas of them) but I usually prefer writing my own code to pinpoint my problem.

The code below is working perfectly, but something with the structure is making me uncomfortable. I think it is because I use references to itself just before the objects dies, so I'm not sure if I'm reusing the animation object or creating new ones. Apparently there is no memory leak.

Can someone give it a look? Fell free to suggest code modifications even if they are unrelated with the memory leak.

window.animate = function(options) {

    var fps = options.fps || 30;
    var x = options.x || null;
    var y = options.y || null;
    var duration = Math.ceil((options.duration / 1000) * fps);
    var onProgress = options.onProgress || null;
    var onComplete = options.onComplete || null;
    var easing = easing || 'easeInOutQuad';

    var animation = {
        fps: fps,
        x: x,
        y: y,
        duration: duration,
        onProgress: onProgress,
        onComplete: onComplete,
        easing: easing,
        _id: null,
        _iteration: 1,
        _lastFrameOccurrence: 0,
        _draw: function() {

            var $this = this;

            var now = Date.now();
            var delta = now - $this._lastFrameOccurrence;
            var interval = 1000 / $this.fps;

            if (delta > interval) {

                $this._lastFrameOccurrence = now - (delta % interval);

                var response = {};
                var factor = window.w.easing[$this.easing]($this._iteration, $this.duration);
                for (var i in $this.x)
                    if ($this.x.hasOwnProperty(i))
                        response[i] = $this.x[i] + ($this.y[i] - $this.x[i]) * factor;

                if ($this.onProgress instanceof Function)
                    $this.onProgress(response);

                $this._iteration++;

                if ($this._iteration > $this.duration)
                    if ($this.onComplete instanceof Function)
                        $this.onComplete();

            };

            if ($this._iteration <= $this.duration) {
                $this._id = requestAnimationFrame(function() {
                    $this._draw();
                });
            } else {
                cancelAnimationFrame($this._id);
            };

        }
    };

    animation._draw();

};

                w.animate({
                    x: { opacity: 1 },
                    y: { opacity: 0 },
                    duration: 500,
                    onProgress: function(r) { console.log(r); },
                    onComplete: function() {}
                });

Firstly, the usage example at the end doesn't really make sense. You're passing 5 arguments to the function, but it seems to only take 1 options object.

Structurally, it seems overwrought to create the animation object. Everything can be handled with closures, as far as I can tell. Somewhat in the same vein, I see no reason for the $ and _ prefixes on half the stuff.

Speaking of naming, the x and y options should probably be called from and to or something. It took me a few moments to realize that's what they are - not coordinates or something similar.

Your setup also seems conflicted, or at any rate complicated. You're using frame count, FPS, and duration (time) to track things, though these may contradict each other. You can't really dictate FPS; FPS is a measured value. If the browser is busy, FPS will drop, meaning each iteration will take longer. But you're calculating animation progression based on _iteration versus duration (which has since been redefined as the target number of frames, not time).

So I can say "I want 600,000 FPS!" but I won't get that kind of performance (requestAnimationFrame aims for 30 to 60'ish FPS, usually). But I can also say I want a duration (time-wise) of 1 second, yet the animation will still run 600,000 iterations. However this will not complete in 1 second.

It makes slightly more sense if you want low FPS, since your function just skips some onProgress calls. But low FPS is usually not considered a thing to strive for, really. In case you do want it, it might be easier to use setTimeout instead of requestAnimationFrame and ignoring updates.

Typically, animations have a set duration (in time), and FPS is whatever it is. You can't control it anyway. And so counting frames usually isn't very useful. Instead you do:

animationProgress = (now - startTime) / totalDuration

And you clamp that to 0...1. That's the input to the easing function.

Lastly, there's no need to call cancelAnimationFrame unless you've already called requestAnimationFrame and want to undo that before the next repaint.

Here's a pretty basic implementation:

function animate(options) {
  options = options || {};

  // defaults
  var duration = options.duration || 1000,
      ease = options.easing || function (a) { return a }, // basic linear easing
      onProgress = options.onProgress || function () {},
      onComplete = options.onComplete || function () {},
      from = options.from || {},
      to = options.to || {};

  // runtime variables
  var startTime = Date.now();

  function update() {
    var deltaTime = Date.now() - startTime,
        progress = Math.min(deltaTime / duration, 1),
        factor = ease(progress),
        values = {},
        property;

    for(property in from) {
      if(from.hasOwnProperty(property) && to.hasOwnProperty(property)) {
        values[property] = from[property] + (to[property] - from[property]) * factor;
      }
    }

    onProgress(values);

    if(progress === 1) {
      onComplete(deltaTime);
    } else {
      requestAnimationFrame(update);
    }
  }

  requestAnimationFrame(update);
}

// ==============================

// Example
animate({
  from: { a: 0 },
  to:   { a: 100 },
  duration: 500,
  onProgress: function (values) {
    document.getElementsByTagName("pre")[0].innerHTML += values.a + "\n";
  },
  onComplete: function (actualDuration, averageFps) {
    document.getElementsByTagName("pre")[0].innerHTML += "Done in " + actualDuration + "ms";
  }
});

I've opted to pass the easing function as an option, rather than a name. Thus you can supply your own easing function on the fly, or pass in a predefined function.