Capturing and taking a screenshot of a window in a loop I am capturing a window of my own choice, and taking a screenshot of it constantly in a loop. Latency and speed is everything.

It's very capable, but I hope it can be improved.

var proc = Process.GetProcessesByName(prcname)[0];
IntPtr hwnd = proc.MainWindowHandle;
RECT rc;
NativeMethods.GetWindowRect(hwnd, out rc);
Bitmap bmp = new Bitmap(rc.Width, rc.Height, System.Drawing.Imaging.PixelFormat.Format32bppPArgb);
Graphics gfxBmp = Graphics.FromImage(bmp);
IntPtr hdcBitmap = gfxBmp.GetHdc();
NativeMethods.PrintWindow(hwnd, hdcBitmap, 0);
gfxBmp.ReleaseHdc(hdcBitmap);
gfxBmp.Dispose();
MemoryStream ms = new MemoryStream();
bmp.Save(ms, System.Drawing.Imaging.ImageFormat.Bmp);

return new MemoryStream(Lz4.CompressBytes(ms.GetBuffer()));

I will set a name prcname which will be the name of a process, for example, Internet Explorer. It will then hook on to that and save it as a bitmap and then .bmp (or other).

I then return the memory stream , and then I can do what I want with it (lz4 is used here to compress, which is very fast and lossless so it works pretty well).

Here are the Imports used:

[DllImport("user32.dll")]
internal static extern bool GetWindowRect(IntPtr hWnd, out RECT lpRect);
[DllImport("user32.dll")]
internal static extern bool PrintWindow(IntPtr hWnd, IntPtr hdcBlt, int nFlags);

Then there is the RECT class, which is enormous, and I did not write it. I took it and reused this code for my own purposes, but RECT has been untouched, as I don't feel safe messing around with it.

Now, is there a way to improve the speed? Preferably I would like to improve the latency, meaning the frame it lags behind.

I am willing to try something else if user32 is not enough for this, so I'm open for suggestions and examples on that.

Can this code be improved?

NetSerializer.Serializer.Serialize(tcpcap.GetStream(), PrintWindow(process));
PrintWindow(process).Dispose();

panel1.BackgroundImage = Image.FromStream(new Lz4DecompressionStream((MemoryStream)NetSerializer.Serializer.Deserialize(tt1.GetStream())));

Well, yes, you're holding on to a bunch of IDisposable objects which should be dealt with by employing the using statement:

IntPtr hwnd;

using (var proc = Process.GetProcessesByName(prcname)[0])
{
    hwnd = proc.MainWindowHandle;
}

RECT rc;
NativeMethods.GetWindowRect(hwnd, out rc);
using (Bitmap bmp = new Bitmap(rc.Width, rc.Height, System.Drawing.Imaging.PixelFormat.Format32bppPArgb))
{
    using (Graphics gfxBmp = Graphics.FromImage(bmp))
    {
        IntPtr hdcBitmap = gfxBmp.GetHdc();
        try
        {
            NativeMethods.PrintWindow(hwnd, hdcBitmap, 0);
        }
        finally
        {
            gfxBmp.ReleaseHdc(hdcBitmap);
        }
    }

    using (MemoryStream ms = new MemoryStream())
    {
        bmp.Save(ms, System.Drawing.Imaging.ImageFormat.Bmp);
        return new MemoryStream(Lz4.CompressBytes(ms.GetBuffer()));
    }
}

Plus, the MemoryStream you return should be Dispose()'d be the calling code.


Setter use for dependency injection, passing interface through too many layers I have an Offers class which is basically a seller which has specific skills, such as law knowledge.
It needs a method isLawyer. Offers has a parent_cat_id column, which references the id of a Category.

Here's Offers :

<?php

namespace App\Models;

use App\Services\CategoryService\AbstractCategory;
use LogicException;

class Offers
{
    /** @var AbstractCategory $category */
    private $categoryService;

    public function isLawyer(): bool
    {
        return $this->getCategoryService()->isLawyer($this->getParentCatId());
    }

    public function setParentCatId(int $catId)
    {
        $this->parent_cat_id = $catId;
    }

    public function getParentCatId(): int
    {
        return (int)$this->parent_cat_id;
    }

    public function setCategoryService(AbstractCategory $categoryService)
    {
        $this->categoryService = $categoryService;
    }

    public function getCategoryService(): AbstractCategory
    {
        if (is_null($this->categoryService)) {
            throw new LogicException('Category service should be set beforehand.');
        }

        return $this->categoryService;
    }
}

AbstractCategory is an abstract class that different categories will extend. I don't care about the implementation of this as of now.

Now in tests, it's all good and fun :

/** @test */
public function is_lawyer_defers_to_category_is_lawyer_method()
{
    $category = Mockery::mock(AbstractCategory::class);
    $category->shouldReceive('isLawyer')->once()->with(1)->andReturn(true);

    $offer = new Offers;
    $offer->setParentCatId(1);
    $offer->setCategoryService($category);

    $this->assertTrue($offer->isLawyer());
}

But as for the real implementation, it gets tricky, as I have 3-4 layers before I'm using $offers->isLawyer().

<?php

namespace App\Models\Product\Chat;

use App\Models\Offers;

class Chat
{
    /** @var Offers $offer */
    private $offer;

    /**
     * @param Offers $offers
     */
    public function setOffer(Offers $offers)
    {
        $this->offer = $offers;
    }

    /**
     * @return Offers
     */
    public function getOffer()
    {
        return $this->offer;
    }

    /**
     * @return bool
     */
    public function byLawyer(): bool
    {
        return $this->getOffer()->isLawyer();
    }
}

Chat uses isLawyer of Offers, but byLawyer is itself called by something else.

I still need to set the AbstractCategory before I use this method.

So, should I pass it through as a parameter to every step ? This sounds ridiculous.

Should I "setup" Offers and set the AbstractCategory even before it's assigned to the Chat ?

Should I give up and instantiate a new Category right in the Offers class ? Setting the abstraction felt like a better design.

Thanks.

Your class names are really throwing me off.  Perhaps because they are so generically named.  By looking at these classes I have a hard time understanding the interfaces you are trying to expose.

What is a Chat?  Why does a chat hold references to Offers (which seems to only be a single "offer" so should perhaps be named Offer)?  Why would a Chat object need a method to check if the Offers it holds is byLawyer (which perhaps should be named isLawyer() since it returns boolean.)?

I just honestly can't make heads or tails of it because I don't think your classes are defined upon real-world lines. Make sure you name your classes meaningfully and specifically, and most importantly with relevance to real-word concepts.  9 times out of 10, if you find yourself thinking of you objects in real-world terms, you will come up with a better design.  In the real world, does a chat have the ability to determine if it has something to do with a lawyer?  No, a chat may need to know about the parties chatting, have some send/receive message functionality, maybe store messaging history, etc.  What does it have to do with lawyers or offers?

What is a CategoryService (ServiceCategory?) and what does it have to do with lawyers (are lawyer services one category of service?)

Lots of questions around Offers:

Why would an Offer(s) object hold information on categories? Wouldn't this be an opposite relationship in real world terms (where categories hold offers)?
If you require AbstractCategory (or CategoryService) as a dependency for Offers why not pass that dependency in constructor?  Why let your class be set up in a bad state?
Do you really need Offers to be mutable, or will it always have a 1-to-1 relationship with CategoryService it wraps?  If so, why the need for the getters/setters?
Does Offers even provide any meaningful functionality above and beyord the AbstractCategory object it holds.
Should AbstractCategory truly be an abstract class or an interface?  Usually when you pass a generalized object like this, you are depending on that object having a given interface, not that it inherits from some class. I would seem a bit odd to be type hinting against an abstract class.

$this->parent_cat_id

You don't have parent_cat_id defined as property on this class.

return (int)$this->parent_cat_id;

There should be no reason for type casting here.

Chat can be set up in a bad state.  byLawyer() method can perform unexpectedly if offer property has not been set.  Should Offers be passed as dependency on constructor?

Why inconsistent use of doc blocks?

Don't mix camelCase and snake_case in your own code.

Why inconsistent use of return type hinting?


Excessive use of lambda with variant of Sieve of Eratosthenes? This is a function which produces the sum of primes beneath (not including) a certain number (using a variant on the sieve of Eratosthenes).

erastosum = lambda x: sum([i for i in xrange(2, x) if i == 2 or i == 3 or reduce(lambda y,z: y*z, [i%j for j in xrange(2,int(i ** 0.5) + 1)])])

Excessive use of lambda? Perhaps. Beautification would be nice, but I'm looking for performance optimizations. Sadly, I'm not sure if there is any further way to optimize the setup I've got right now, so any suggestions (on how to, or what else to do) would be nice.

As much as I love anonymous functions, they can be a nightmare to debug. Splitting this code up piece wise (into an actual function or otherwise) shouldn't and wouldn't decrease it's performance while improving maintenance and portability for you later on.

This class is quite efficient for determining primes. Despite being quite lengthy is more efficient than the more usual approach.


speed up Elias gamma compression I wrote some functions to compress and decompress a list of ints using Elias gamma encoding:
def _reverseBits(number: int) -> int:
    rev = 0
    while number:
        rev = rev << 1
        if number & 1:
            rev = rev ^ 1 # bitwise ^ 1 is faster than arithmetic + 1
        number = number >> 1
    return rev

def compress(numbers: List[int]) -> int:
    r'''Calculate the elias gamma encoded bitstream representation of the input list of numbers.

    >>> compress([5]) == 0b10100
    True
    >>> compress([15]) == 0b1111000
    True
    >>> compress([5, 15]) == 0b10100010100
    True
    '''
    bitstream = 0
    bitstream_length = 0
    previous_number = 0
    for number in numbers:
        delta = number - previous_number # store deltas, much smaller numbers
        N = delta.bit_length() - 1
        encoded_number = _reverseBits(delta) << N # reversed bitstream won't have leading zeroes problem in int container
        bitstream += encoded_number << bitstream_length
        bitstream_length += (N * 2 + 1)
        previous_number = number
    return bitstream

def decompress(bitstream: int) -> List[int]:
    r'''Convert the elias-encoded bitstream into a list of numbers.

    >>> decompress(0b10100010100)
    [5, 15]
    '''
    numbers = []
    cumulative_sum = 0
    while bitstream:
        N = 1
        while bitstream & 1 == 0: # count Elias leading zeroes
            bitstream = bitstream >> 1
            N += 1
        mask = (1 << N) - 1
        number = _reverseBits(bitstream & mask)
        number = number << N - number.bit_length()
        cumulative_sum += number
        numbers.append(cumulative_sum)
        bitstream = bitstream >> N
    return numbers

But it's very slow on huge input lists
start = 5000
size = int(4e6)
numbers = [i for i in range(start, start + size + 1)]

I started a test with a four-million size input list, all elements sequential, it took 13 minutes to encode and decode on my laptop.
I've thought about parallelising what I already have, but I don't think there's anywhere I can actually work it in.
Please could you help me review my algorithm and see if there are ways to improve or rework it to make my compress and decompress methods faster?
Or potentially, this might be the limit of Elias gamma encoding in Python...

I have also noticed a problem with mine, that memory does not seem to be freed after it finishes. tracemalloc.get_traced_memory() after running it shows a memory usage higher than the encoded int.

Elias gamma encoding encodes numbers like this
+--------+---------------+
| Number |  γ encoding   |
+--------+---------------+
|      1 |             1 |
|      2 |          0 10 |
|      3 |          0 11 |
|      4 |        00 100 |
|      5 |        00 101 |
|      6 |        00 110 |
|      7 |        00 111 |
|    ... |               |
|     15 |      000 1111 |
|     16 |   0000 1 0000 |
|    ... |               |
|     31 |   0000 1 1111 |
|     32 | 00000 10 0000 |

The leading zeroes inform the decoder how many of the following bits are part of the current number. So numbers of the input list are concatenated into a bitstream, and can be unambiguously decoded back into the same list of numbers. [1, 4, 3] encodes to 1 00100 011.
I am storing the bitstream in reverse inside of a python int, reversed so that there's no problems with the leading zeroes of the int container.
Even though the bit sequences in the table look large because of the leading zeroes, my compression only encodes the difference between the current list element and the previous (delta encoding), so that most of the time the number to encode and concatenate is much smaller than the actual list element.
My methods intentionally only work with sorted lists.

Rather strange to encode as a number instead of a (bit)string. And the way you do it takes quadratic time in the size of your list (because both the + and the << in bitstream += encoded_number << bitstream_length are linear). You could do it in linear time by building a string and then turning it into a number at the end.
This should take only a few seconds for your large test (removing the docstring just for brevity here):
def compress(numbers: List[int]) -> int:
    bitstream = []
    previous_number = 0
    for number in numbers:
        delta = number - previous_number # store deltas, much smaller numbers
        N = delta.bit_length() - 1
        encoded_number = bin(delta)[:1:-1] + '0' * N
        bitstream.append(encoded_number)
        previous_number = number
    bitstream.reverse()
    return int(''.join(bitstream), 2)

For decompression, I'd accordingly first turn the number into a string and then work with that.
Optimized version should take less than a second:
def compress(numbers: List[int]) -> int:
    bitstream = []
    append = bitstream.append
    memo = {}
    lookup = memo.get
    previous_number = 0
    for number in numbers:
        delta = number - previous_number # store deltas, much smaller numbers
        if not (encoded_number := lookup(delta)):
            N = delta.bit_length() - 1
            memo[delta] = encoded_number = bin(delta)[:1:-1] + '0' * N
        append(encoded_number)
        previous_number = number
    bitstream.reverse()
    return int(''.join(bitstream), 2)


Non-Contiguous Substrings Problem:

  A non-contiguous substring of string \$s\$ is a sequence of \$k \geq 0\$ characters in \$s\$, in the order in which they occur in \$s\$. For instance, the set of all non-contiguous substring of "abcd" are \$\{a,cd\}, \{ab,d\}, \{a,c\}, \{a,d\}, \{b,d\}\$.

I want to mention this is from an assignment (which I completed and submitted more than a week before asking this) and the reason I'm asking is because I'm curious if there is a better, more elegant method than mine.

This is supposed to be done recursively.

My solution:

Find all possible substring (powerset) of the given string (recursive function). Test each possible substring to see if it is non-contiguous (iterative function).

import java.util.Arrays;

public class HWK3_2_hoodav {

    public static void main(String[] args) {
        /** Parses input and calls powerSet method with input */
        String input = Arrays.toString(args);
        input = input.substring(1, input.length()-1);
        powerSet(input, "", input);
    }

    public static void powerSet(String original, String base, String substring) {
        /** This method recursively finds all possibile combinations of
        substrings and then invokes the nonContiguous method to check
        if a specific combination is non-contiguous */

        if (substring.length() == 1) {
            nonContiguous(original, base + substring);
            nonContiguous(original, base + " ");
        }
        else {
            powerSet(original, base + substring.charAt(0), substring.substring(1));
            powerSet(original, base + " ", substring.substring(1));
        }
    }

    public static void nonContiguous(String original, String candidate) {
        /** This method verifies a given candidate is non-contiguous */
        if (candidate.contains(" ")) {
            for (int i=0; i < original.length(); i++) {
                if (candidate.charAt(i) != ' ') {
                    for (int j=i+1; j < original.length(); j++) {
                        if (candidate.charAt(j) == ' ') {
                            for (int k=j+1; k < original.length(); k++) {
                                if (candidate.charAt(k) != ' ') {
                                    printSet(candidate);
                                    return;
                                }
                            }
                        }
                    }
                }
            }
        }
    }

    public static void printSet(String orgString) {
        /** This method takes in a string containing whitespaces seperating
        non-whitespace charcters and output a string enclosed by braces and
        with whitespaces replaced by commas */
        String[] splitString = orgString.split(" ");
        String string = "{";
        for (int i=0; i < (splitString.length-1); i++) {
            if (!splitString[i].equals("")) {
                string += splitString[i] + ", ";
            }
        }
        string += splitString[splitString.length-1] + "}";
        System.out.println(string);
    }
}

The input is any string, where ALL characters are treated equally.

The output of the input "abcde" is:

{abc, e}
{ab, de}
{ab, d}
{ab, e}
{a, cde}
{a, cd}
{a, c, e}
{a, c}
{a, de}
{a, d}
{a, e}
{bc, e}
{b, de}
{b, d}
{b, e}
{c, e}

Bugs in nonContiguous

If your input string contains spaces then your nonContiguous function produces arbitrary output regardless of whether you are correct or not.

False positive: As mentioned in the comments, input string "X Z" will print as a non-contiguous subset of itself.
False negative: What if the input string is "   " (three spaces)?  When you build a substring of the first space with the third space this should still count as a non-contiguous subset.

Other problems

Base case: If input="" then your base case of recursion (substring.length() == 1) does not work.
Naming: substring is not a good name for the parameter in your function.  While the name does indicate what the value represents, its name may confuse someone into thinking that it holds the non-contiguous substring when really it just holds the unvisited portion of the original string.  For a lack of a better name, I will simply call this value str.

Another approach

This is a fairly poor analogy but here it goes anyway.  What if I told you to flip through a deck of cards and tell me if there are any Jokers?  Which approach do you think is better?

A) When you see a Joker you insert a Queen into the deck after the Joker and look for that Queen afterwards (not knowing if there were any Queens in the original deck or not).
B) When you see a Joker you write down that you saw a Joker on a scratch sheet of paper.

I hope we can agree that B is the simpler approach.  In this case the Jokers are simply gaps between characters chosen from the input string.

The problem with integrating this logic with your approach is the order in which the recursion is performed.  You know when you skip a character (currently when you are adding a " ") but you need to know that another character is added after the skip for you to declare the string as non-contiguous.  The method is shown below:

public static void main(String[] args) throws java.lang.Exception
{
    /** Parses input and calls powerSet method with input */
    String input = "abcde";
    nonContiguousSubstrings("", input, true, false);
}

public static void nonContiguousSubstrings(String base, String str, boolean contiguous, boolean skipped)
{
    // This stop condition is simpler and safer.
    // If the original input is empty it stops the recursion here
    if (str.length() == 0)
    {
        // only print the string if it is not contiguous
        if(!contiguous)
        {
            System.out.println(base);
        }
    }
    else
    {
        // This is the recursion where we skip a character here
        // Set skipped only if we already had a character in base
        // Even if we already had a character in base, we cannot say this skip makes the string non-contiguous unless we add a character to it later
        nonContiguousSubstrings(base, str.substring(1), contiguous, base.length() != 0);

        // Below is the recursion where we add a character here
        // Since we are adding a character to the string, set contiguous to false if we skipped a character previously, otherwise retain its value.
        contiguous = contiguous && !skipped;
        nonContiguousSubstrings(base + str.charAt(0), str.substring(1), contiguous, skipped);
    }
}

Yet Another Approach

By restructuring the recursion we can remove the skipped parameter.  The trick is to use recursion only when needed.  Iteration suffices to skip indices but we "must" recurse to choose an arbitrary number of elements from input.

Here is a simple substring printer (powerset) using this method:

public static void main(String[] args) throws java.lang.Exception
{
    String input = "abcde";

    // Initially the substring is empty
    allSubstrings(input, "");
}

public static void allSubstrings(String str, String substring)
{
    for (int i = 0; i < str.length(); i++)
    {
        // For each i, we skip i characters when generating the substring
        allSubstrings(str.substring(i + 1), substring + str.charAt(i));
    }

    System.out.println(substring);
}

Now you just want to be able to detect if there is a gap in each substring.  This can be done quite simply if you keep track of whether you ever add a gap.  Use a boolean instead of manipulating the string.

public static void main(String[] args) throws java.lang.Exception
{
    String input = "abcde";

    // Initially the substring is empty and deemed contiguous
    nonContiguousSubstrings(input, "", true);
}

public static void nonContiguousSubstrings(String str, String substring, boolean contiguous)
{
    for (int i = 0; i < str.length(); i++)
    {
        // There is a gap here iff i doesn't equal 0 (we skip characters) AND we are not creating the first character of the substring.
        boolean gapHere = (i != 0) && (substring.length() != 0);

        // The substring is still contiguous iff it was already contiguous and there is no gap here
        nonContiguousSubstrings(str.substring(i + 1), substring + str.charAt(i), contiguous && !gapHere);
    }

    if(!contiguous)
    {
        System.out.println(substring);
    }
}


Recursive Determinant The following code I devised to compute a determinant:

module MatrixOps where

determinant :: (Num a, Fractional a) => [[a]] -> a
determinant [[x]] = x
determinant mat =
 sum [s*x*(determinant (getRest i mat)) | i <- [0..n-1], let x = (head mat) !! i
                                                             s = (-1)^i]
 where n = length $ head mat

getRest :: (Num a, Fractional a) => Int -> [[a]] -> [[a]]
getRest i mat = removeCols i (tail mat)

removeCols :: (Num a, Fractional a) => Int -> [[a]] -> [[a]]
removeCols _ [] = []
removeCols i (r:rs) = [r !! j | j <- [0..n-1], j /= i] : removeCols i rs
 where n = length r

I have a few general questions about the style of my code and practices:

Is a very "Haskell" solution? I come from an OOP background and I am still learning functional programming.
Is there a better way to space this out? I feel like some of the code is not very readable (this may just be because I am new), especially the definition of determinant mat = ...
Is this code considerably "clean?"

Just a few thoughts:

Your method (IIUC) is conceptually correct, but has computation complexity O(n!) where n is the dimension of a given matrix. If you need better complexity (polynomial in n), you have to use another solution, such as using PLU decomposition described here.
Be aware that since Haskell lists are essentially linked lists, getting i-th element using (!!) takes O(i). So your code

[r !! j | j <- [0..n-1], j /= i]

has O(n2) complexity. You could express it in O(n) for example using splitAs as

let (left, right) = splitAt i r
 in left ++ (tail right)

The same applies for i <- [0..n-1], let x = (head mat) !! i. You could do something like

(i, x) <- zip [0..] (head mat)

instead.


Cosine similarity computation I have a matrix of ~4.5 million vector [4.5mil, 300] and I want to calculate the distance between a vector of length 300 against all the entries in the matrix.

I got some great performance time using the answers from the following post: Efficient numpy cosine distance calculation.

from scipy import spatial

def cos_matrix_multiplication(vector, matrix):

    v = vector.reshape(1, -1)
    scores=spatial.distance.cdist(matrix_1, v, 'cosine').reshape(-1)
    return scores

Here, vector is a NumPy array [300,1] and matrix_1 is a NumPy matrix [4.5mil, 300]

Using this I can calculate scores for the entire matrix (4.5 mil records) in about 19secs. I have been trying to optimize it even further but don't seem to make any progress.

I want to know if it is feasible to convert this code into a cython code or use Process/Thread pool to speed up even more.

I split my matrix into smaller chunks (500K each) and used ThreadPoolExecutor as follows:

matrix_list=[mat_small1,mat_small2,....,mat_small9]

import concurrent.futures
def cos_thread():
    neighbors=[]

    with concurrent.futures.ThreadPoolExecutor(max_workers=8) as executor:

        future_to_list = {executor.submit(cos_matrix_multiplication,vec,mat_col): mat_col for mat_col in matrix_list}

        for future in concurrent.futures.as_completed(future_to_list):

            data = future.result()

            neighbors.extend(data)

    return neighbors

I can currently compute the entire ~4.5 million cosines in ~5 seconds.

Does scipy.spatial.distance.cdist run multiple cores in parallel on your machine ?
On my mac with Accelerate framework, it runs all 4 cores,
but equivalent numpy seems to run only 1.
(This is regardless of VECLIB_MAXIMUM_THREADS, which I don't understand.)

If the big matrix doesn't change very often, normalize it once outside the loop:

A /= np.linalg.norm( A, axis=1 )  # <-- once

def cosdist( A, b, dtype=np.float32 ):
    """ (A . b) / |b| -- A normalized """
    Adotb = A.dot(b) /
    Adotb /= np.linalg.norm(b)
    return Adotb

Since A.dot(b) and norm(A) take roughly the same time,
this runs about twice as fast -- on 1 core.

How much memory do you have ? 1M x 300 x 8 bytes is 2.4 Gbytes,
which may be a reasonable chunk size; 4.5M, 11Gbytes, will be memory-bound. Can you monitor memory usage / swapping ?

Using np.float32 instead of the default np.float64
might be faster, or allow bigger chunks.
But scipy cdist seems to convert float32 to float64, slower.

Numpy and scipy link to libraries for
BLAS,
Basic Linear Algebra Subprograms.
These libs are usually vendor-tuned, faster than C or Cython loops.
To see what numpy / scipy link to:

from numpy import __config__
print "numpy blas:\n", __config__.get_info( "blas_opt" )

from scipy import __config__
print "scipy blas:\n", __config__.get_info( "blas_opt" )

Even though these are the same (Accelerate framework on my mac),
numpy and scipy have different wrappers around BLAS
(numpy/linalg/linalg/lapack_lite.so, scipy/linalg/cython_blas.so)
with different overheads.

Summary: numpy / scipy dot and norm to BLAS
to multiple cores is a messy business.
Don't trust any runtimes that you haven't run yourself.


Utility function to split a number into n parts in the given ratio I wrote a small utility function that divides a total into n parts in the ratio given in load_list. Numbers in each part should be positive integers. It supports an optional min_num argument that, when specified, puts at least min_num in each of the n parts before distributing the rest.  Please review.

def reduce_ratio(load_list, total_num, min_num=1):
  """
  Returns the distribution of `total_num` in the ratio of `load_list` in the best possible way
  `min_num` gives the minimum per entry
  >>> reduce_ratio([5, 10, 15], 12)
  [2, 4, 6]
  >>> reduce_ratio([7, 13, 30], 6, 0)
  [1, 2, 3]
  >>> reduce_ratio([7, 13, 50], 6)
  [1, 1, 4]
  >>> reduce_ratio([7, 13, 50], 6, 3)
  Traceback (most recent call last):
    ...
  ValueError: Could not satisfy min_num
  >>> reduce_ratio([7, 13, 50], 100, 15)
  [15, 18, 67]
  >>> reduce_ratio([7, 13, 50], 100)
  [10, 19, 71]
  """
  num_loads = [ [load, min_num] for  load in load_list ]
  yet_to_split = total_num - ( len(num_loads) * min_num )
  if yet_to_split < 0:
    raise ValueError('Could not satisfy min_num')
  for _ in range(yet_to_split):
    min_elem = min(num_loads, key=lambda (load, count): (float(count)/load, load))
    min_elem[1] += 1
  reduced_loads = map(itemgetter(1), num_loads)
  if sum(reduced_loads) != total_num:
    raise Exception('Could not split loads to required total')
  return reduced_loads

Because the problem is not quite formalised and is mostly defined by the way your code currently works, I must confess that I find it quite hard to make thinds more efficient/concise.

A few point however:

using ( len(num_loads) * min_num ) un-necessarly relies on the way you have populated num_loads. You could something more generic like : yet_to_split = total_num - sum(num for _,num in num_loads) so that things work no matter what.
at the moment, I don't think it is possible for the condition sum(reduced_loads) != total_num to be true. Indeed, at the beginning, the definition of yet_to_split leads to the following property : yet_to_split + sum_of_loads = total_num. Then as we iterate over yet_to_split, sum_of_loads gets increased one by one. Because the condition cannot be true, it's not really wise to expect anyone to expect and catch this exception. If you want to perform the verification, assert is what you should be using : assert(sum(reduced_loads) == total_num).
in order to avoid any troubles while processing the input, you might want to add more checks.

My suggestions are the following :

if not load_list:
    raise ValueError('Cannot distribute over an empty container')
if any(l <= 0 for l in load_list):
    raise ValueError('Load list must contain only stricly positive elements')

A bit of a naive question but couldn't the min_num logic be extracted out by processing the load_list normally and only at the end (in a potentially different function) check that min(return_array) >= min_num ? This makes performance slightly worse for cases leading to failures but makes this usual case a bit clearer.

For instance, the code would be like :

num_loads = [ [load, 0] for  load in load_list ]
for _ in range(total_num):
    min_elem = min(num_loads, key=lambda (load, count): (float(count)/load, load))
    min_elem[1] += 1
reduced_loads = [num for _,num in num_loads]
assert(sum(reduced_loads) == total_num)
return reduced_loads

Just a suggestion but instead of recomputing the minimal element by reprocessing the whole array every-time, maybe you could use an appropriate data structure like heapq to get the minimal element quickly and then re-add it to the structure once updated.


HTML downloader and parser for CR This program downloads a Code Review HTML file and parses it.

Could you review my program?

Main.java

import java.net.URL;

public class Main {
    public static void main(String[] args) throws Exception {
        final String site1 = "http://codereview.stackexchange.com/questions/";
        String site2;
        String site3;
        URL url;
        HtmlGetter htmlGetter = new HtmlGetter();

        while(true) {
            site2 = "69";
            site3 = "is-this-implementation-of-shamos-hoey-algorithm-ok";
            url = new URL(site1 + site2 + "/" + site3);
            htmlGetter.setFileName("[" + site2 + "]" + site3 + ".html");
            htmlGetter.download(url);
            htmlGetter.parse();
            break;
        }
    }
}

HtmlGetter.java

import java.io.File;
import java.io.InputStream;
import java.io.OutputStream;
import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.net.URL;
import java.util.Scanner;

import org.apache.commons.lang3.StringEscapeUtils;
import org.apache.commons.io.IOUtils;

public class HtmlGetter {
    private String fileName;

    public void setFileName(String fileName) {
        this.fileName = fileName;
    }

    public String getFileName() {
        return fileName;
    }

    public void download(URL url) throws Exception {
        final InputStream in = url.openStream();
        final OutputStream out = new FileOutputStream(fileName);
        IOUtils.copy(in, out);
        in.close();
        out.close();
    }

    public void parse() throws Exception {
        Scanner scanner = new Scanner(new File(fileName));
        String oneLine;
        String htmlTagRegex = "<(/)?([a-zA-Z]*)(\\s[a-zA-Z]*=[^>]*)?(\\s)*(/)?>";

        while(scanner.hasNextLine()) {
            oneLine = scanner.nextLine();
            if(oneLine.matches(".*<code>.*")) {
                while(scanner.hasNextLine()&&(!oneLine.matches(".*</code>.*"))) {
                    oneLine = StringEscapeUtils.unescapeHtml4(oneLine.replaceAll(htmlTagRegex, ""));
                    System.out.println(oneLine);
                    oneLine = scanner.nextLine();
                    // This cannot handle such kind of a line which has <code> and </code> together.
                }
                System.out.println("-------------------------------------------");
            }
        }
    }
}

public static void main(String[] args) throws Exception {
    final String site1 = "http://codereview.stackexchange.com/questions/";
    String site2;
    String site3;

Why do define site1 as a final constant, but delay the other assignments until later? They should really be static finals for the class. Also some clearer names would not go amiss.

    URL url;

Declare them where they are used, don't declare them ahead of time.

    HtmlGetter htmlGetter = new HtmlGetter();

    while(true) {
        site2 = "69";
        site3 = "is-this-implementation-of-shamos-hoey-algorithm-ok";
        url = new URL(site1 + site2 + "/" + site3);
        htmlGetter.setFileName("[" + site2 + "]" + site3 + ".html");
        htmlGetter.download(url);
        htmlGetter.parse();
        break;

Why do you have a loop if you are just going to break out of it?
        }
    }

public void parse() throws Exception {

You shouldn't throw Exception, your specification should really only include the exceptions that'll actually be thrown

    Scanner scanner = new Scanner(new File(fileName));
    String oneLine;
    String htmlTagRegex = "<(/)?([a-zA-Z]*)(\\s[a-zA-Z]*=[^>]*)?(\\s)*(/)?>";

Parsing HTML by regex is asking for trouble. Instead, you'll find the task much easier if you grab a html parsing library and use that.

    while(scanner.hasNextLine()) {
        oneLine = scanner.nextLine();
        if(oneLine.matches(".*<code>.*")) {

You are just searching for a substring here, there isn't much point in using regex to do that.

            while(scanner.hasNextLine()&&(!oneLine.matches(".*</code>.*"))) {

If you insist on regex, use a single regex to capture the entire code tag.

                oneLine = StringEscapeUtils.unescapeHtml4(oneLine.replaceAll(htmlTagRegex, ""));
                System.out.println(oneLine);
                oneLine = scanner.nextLine();
                // This cannot handle such kind of a line which has <code> and </code> together.
            }
            System.out.println("-------------------------------------------");
        }
    }
}

Using something like jsoup: http://jsoup.org/

Document question = Jsoup.connect("http://codereview.stackexchange.com/questions/69").get();
Elements codes = question.select("code")
for(Element code: codes)
{
   System.out.println(code.text())
}


Combining two 32-bit integers into one 64-bit integer long long combine(unsigned int high, unsigned int low) {
    return ((unsigned long long) high) << 32 || low;
}

Is there a better name for the operation?
Is the implicit cast from unsigned long long to long long a reinterpret_cast, or what happens if the unsigned number is too large for the signed data type?

You should return unsigned long long and let the user decide what they want to do with the cast, especially if you want this to be generic.

I'd prefer a name such as u32tou64, uinttoull, or something more descriptive than combine.  A lot of this will depend on your own naming standards, though.

Also, I'd consider being more pedantic:

return (((uint64_t) high) << 32) | ((uint64_t) low);

It's unlikely to make a difference because the code is essentially the same as yours, but it's easier to read and avoids extremely rare (but very troublesome to debug) casting issues.  It may require a custom types header if your compiler doesn't support this type notation.

Also, consider making it a macro.  There's little benefit to having it as a function - the operation itself will take far less time than the function call setup, call, and return, so the performance of a macro will be much higher.  Further, it isn't going to take up much more program space than a function call, so there's little to gain by leaving it a real function.


Make a git repository read-only over ssh depending on the key used I want a git repository, accessible over ssh, to be read-only when used with certain keys. With other keys access to the full system is okay.

Here is my solution.

git-readonlyshell:

if echo "$2" | egrep -q ^git-upload-pack; then
  sh -c "$2"
else
  echo Error: read only access 1>&2
fi

.ssh/authorized_keys:

command="./git-readonlyshell -c \"$SSH_ORIGINAL_COMMAND\"" ...

Thoughts?

Any scenario in which this would break?

Should be

printf "%s\n" "$2" | grep -E "^git-upload-pack" && sh -c "$2"

Notes:

sh not bash for portability
The printf instead of echo is for safety.  What happens if someone puts  "-n foo" as "$2"?


Blinking LED code at a particular frequency I wrote a function for interacting with my PIC16F1788. The code sends pulses out the RC4 port at a specific frequency, to make an LED blink according to that frequency. So the LED will blink faster or slower depending on the magnitude of the frequency. I decided to take a blinking code and vary the delay time according to my input frequency as follows.

void blink(int frequency){
/* starts the LED blinking at a particular frequency */

while(1)
{
 PORTCbits.RC4=0XFF;  // RC4 on
 __delay_ms(1000/frequency);   // time in ms dealing with frequency values in kHz
 PORTcbits.RC4=0X00;   // RC4 off
 __delay_ms(1000/frequency);   // time in ms dealing with frequency values in kHz
}

}

would this code work, is this a good idea ,and is there anyway that I could improve it?

Your code:

void fault_blink(int frequency){
/* starts the LED blinking at a particular frequency */

while(1)
{
 PORTCbits.RC4=0XFF;  // RC4 on
 __delay_ms(1000/frequency);   // time in ms dealing with frequency values in kHz
 PORTcbits.RC4=0X00;   // RC4 off
 __delay_ms(1000/frequency);   // time in ms dealing with frequency values in kHz
}
}

Indentation

The body of the fault_blink function should be indented, along with the body of the while loop.

The code now:

void fault_blink(int frequency){
/* starts the LED blinking at a particular frequency */

    while(1)
    {
         PORTCbits.RC4=0XFF;  // RC4 on
         __delay_ms(1000/frequency);   // time in ms dealing with frequency values in kHz
         PORTcbits.RC4=0X00;   // RC4 off
         __delay_ms(1000/frequency);   // time in ms dealing with frequency values in kHz
    }
}

Spacing

To enhance readability, there should be spaces around arithmetic operators and = signs.

The code now:

void fault_blink(int frequency) {
/* starts the LED blinking at a particular frequency */

    while(1)
    {
         PORTCbits.RC4 = 0XFF;  // RC4 on
         __delay_ms(1000 / frequency);   // time in ms dealing with frequency values in kHz
         PORTcbits.RC4 = 0X00;   // RC4 off
         __delay_ms(1000 / frequency);   // time in ms dealing with frequency values in kHz
    }
}

0X vs 0x

This isn't the biggest thing, but most people have a lower-case 'x' when defining a number as a hexadecimal number.

The code now:

void fault_blink(int frequency) {
/* starts the LED blinking at a particular frequency */

    while(1)
    {
         PORTCbits.RC4 = 0xFF;  // RC4 on
         __delay_ms(1000 / frequency);   // time in ms dealing with frequency values in kHz
         PORTcbits.RC4 = 0x00;   // RC4 off
         __delay_ms(1000 / frequency);   // time in ms dealing with frequency values in kHz
    }
}

Unnecessary comments

Don't repeat your comments; especially when the original comment is very near.

In this part of your code:

PORTCbits.RC4=0XFF;  // RC4 on
__delay_ms(1000/frequency);   // time in ms dealing with frequency values in kHz
PORTcbits.RC4=0X00;   // RC4 off
__delay_ms(1000/frequency);   // time in ms dealing with frequency values in kHz

You can remove the comment on the second __delay_ms call as the comment is identical to the call right above it.


Factory pattern for creating sub classes derived from interface with generic type Would like to know if the factory pattern I've implemented for my stat library is the correct way to go. I have an interface IPlayerStatsManager where the type parameter is an interface as well (IPlayerStatsModel). The IPlayerStatsManager is an interface that lists the common operations that can be performed on any class that implements IPlayerStatsModel. I've only completed the implementation for basketball and it seems to work correctly but would like to know if the way I currently have it is the most efficient way. Here's the code (some of the implementation removed for brevity)

Context

public interface IPlayerStatsModel
{
    string type {get; set;}
}

// concrete implementation
public class  BasketballPlayerStats : IPlayerStatsModel
{
    // various class members with getters and setters
}

public interface IPlayerStatsManager<T> where T : IPlayerStatsModel
{
    Task<T> GetPlayerGameStats(....);

    Task<List<T>> GetPlayersGameStats(...);

    // insert and update methods
}

//concrete implementation
public class BasketballPlayerStatsManager : IPlayerStatsManager<BasketballPlayerStats>
{
    public BasketballPlayerStatsManager(DbContext context)
    {
    }

    public async Task<BasketballPlayerStats> GetPlayerGameStats(...)
    {
    }
}

Factory

//Factory class
public class PlayerStatsFactory<IPlayerStatsManager>
{
    public static IPlayerStatsManager Create(DbContext context, int sportType)
    {
        if (sportType == (int)Utils.SportType.Basketball)
        {
             return (IPlayerStatsManager)Activator.CreateInstance(typeof(BasketballPlayerStatsManager));
        }
        return default(IPlayerStatsManager);
     }
}

I feel like there might be a better way but I'm not sure what that would look like. Any pointers would be greatly appreciated.

You have:

public static IPlayerStatsManager Create(DbContext context, int sportType)

And then:

if (sportType == (int)Utils.SportType.Basketball)

Why not just take a SportType parameter and remove that cast?

An abstract factory is a great tool, especially with Dependency Injection. The problem is that yours is a static method, which makes it available just about anywhere in the code, effectively hiding dependencies and defeating the purpose of IoC and DI: that factory becomes some sort of ambient context that's there waiting to be used by anyone, and your constructors don't need to document the fact that you're depending on an abstract factory: that smells IMO.

The factory implementation has one purpose: instantiate types. It has to know what it's creating. Your usage of reflection is nothing but a very convoluted way of saying...

return new BasketballPlayerStatsManager();

The cast to interface is also superfluous - if BasketballPlayerStatsManager does implement IPlayerStatsManager, then that's all the client code will see anyway (of course it implements that interface).

"Manager" sounds like a very general and not-so-precise way of referring to what other programmers would expect to be called a Repository; I'd rename it as such.

return default(IPlayerStatsManager);

You're returning a reference type - the default value can only be null. Your intent would be much clearer like this:

return null;


Many if statements - refactoring Yesterday I found some nice video from some IT conference about removing if statements from code. I saw a very nice example of refactoring. I am curious what do you think about it, especially what weak points do you see in such approach.

1) We have a class with very "nice" method:

public int inferCategoryID(Component c) {
        if(c.hasOneChannel() && c.hasValidCertificate() && c.isTested() && c.frequentlyUsed()) {
            return 24;
        }
        if(c.hasOneChannel() && c.hasValidCertificate() && !c.isTested() && c.frequentlyUsed()) {
            return 95;
        }
        if(c.hasOneChannel() && !c.hasValidCertificate() && c.isTested() && c.frequentlyUsed()) {
            return 221;
        }
        if(c.hasTwoChannels() && c.hasValidCertificate() && c.isTested() && c.frequentlyUsed()) {
            return 2004;
        }
        if(c.hasTwoChannels() && c.hasValidCertificate() && !c.isTested() && c.frequentlyUsed()) {
            return 20044;
        }
        if(c.hasTwoChannels() && c.hasValidCertificate() && !c.isTested() && !c.frequentlyUsed()) {
            return 2003;
        }
        // etc...
    }

As you can see all conditions are mutually exclusive.

2) The idea was, to create a separate class for each condition - under a common interface:

interface CategoryIDCondition  {
        public boolean isTrue(Component c);
        public int getID();
    }

class Cat24Condition implements CategoryIDCondition {
    public boolean isTrue(Component c){
        return c.hasOneChannel()
                && c.hasValidCertificate()
                && c.isTested()
                && c.frequentlyUsed();
    }

    public int getID(){
        return 24;
    }
}

// etc...

3) Usage example:

public int inferCategoryID(Component c) {
        List<CategoryIDCondition> conditions = new ArrayList<>();
        conditions.add(new Cat24Condition);
        conditions.add(new Cat95Condition);
        // etc... (btw. not a good place for list init but never mind...)

        for (CategoryIDCondition cond : conditions) {
            if(cond.isTrue(c)){
                return cond.getID();
            }
        }
    }

What weak points do you see in such approach apart from that, that conditions have to be mutually exclusive, or have to be added to conditions list in a very specific order? Would you decide to use such strategy in your code?

Personally, I wouldn't use that approach, because of the amount of boilerplate code and complexity it introduces. Adding N+1 classes, where N is the number of conditions, just seems like overkill to me.

First off, I don't think that the method at hand must necessarily be refactored. I agree that is not a terribly "nice" method from a aesthetic point of view, but the amount of complexity it contains might not justify a refactor. Often, a bit of reformatting is already a good approach:

public int inferCategoryID(Component c) {
    if( c.hasOneChannel() &&
        c.hasValidCertificate() &&
        c.isTested() &&
        c.frequentlyUsed()) {
        return 24;
    }

    [...]
}

If that is not satisfactory, my next approach would've been to simply used separate methods for the conditions. This doesn't imply creating lots of classes, but still clean up the inferCategoryID substantially. While I'm not a fan of giving names like isCat24, I will use it here since the example refactoring technique using classes does so too and I can't infer more useful naming from the conditions:

public int inferCategoryID(Component c) {
    if(isCat24(c)) {
        return 24;
    }

    [...]
}

public boolean isCat24(Component c) {
    return c.hasOneChannel() &&
           c.hasValidCertificate() &&
           c.isTested() &&
           c.frequentlyUsed();
}

Last but not least, in cases where lots of boolean conditions are used, bit-lists are often applicable. The following code isn't tested, but it should give an idea of what I mean:

const int CATEGORY_24 = 15;
const int CATEGORY_95 = 13;

public int inferCategoryID(Component c) {
    int properties = componentProperties(c);

    if(properties == CATEGORY_24) {
        return 24;
    }

    if(properties == CATEGORY_95) {
        return 95;
    }

    [...]
}

public int componentProperties(Component c) {
    return ((int) c.hasOneChannel() << 3) &&
           ((int) c.hasValidCertificate() << 2) &&
           ((int) c.isTested() << 1) &&
           ((int) c.frequentlyUsed());
}

The integer returned from componentProperties is a dense representation of the components properties in form of a bit list, that is each bit represents one boolean property. For example, if all properties are true, the resulting integer (in binary) would be 1111, if only hasOneChannel was true, the integer would be 1000 and so on. This has the advantage that you don't have to create a large amount of methods or classes, but still cleans up the inferCategoryID method quite nicely. The downside of this method is that it is arguably the most cryptic (while I also find the multiple-class approach relatively cryptic :D).

Since I said that adding lots of classes introduces too much complexity to be really worth it (in this case), I can't say that the last solution is any different in terms of adding unnecessary complexity, so I probably wouldn't use either of them for the use case at hand. I just wanted to give you an idea of a different approach that, in my opinion, is preferable to creating a class per condition :)


Find first non-repetitive character in a string The original task was to find the first non-repetitive character in a string.

Every now and then I read questions here on Code Review and try answering them myself, and as such I stumbled upon this one.

Now, I have two solutions:

The first one:

struct ch {
    int num;
    char c;
} characters[127];

char find_first_non_repetitive(char *string)
{
    int i = 0;

    for(i = 0; i < strlen(string); i++) {
        characters[string[i]].num++;
        characters[string[i]].c = string[i];
    }

    int index = characters[string[0]].num;
    char c = characters[string[0]].c;

    for(i = 1; i < 127; i++)
    {
        if(characters[i].num != 0 && first > characters[i].num)
        {
            first = characters[i].num;
            c = characters[i].c;
        }
    }

    return c;
}

About this one, if using C# instead of C, my idea was to replace struct with an ArrayList<int>.

The second one:

char find_first_non_repetitive(char *string)
{
    const int NUM_CHARACTERS = 127;

    int i = 0, cha[NUM_CHARACTERS];

    for(i = 0; i < NUM_CHARACTERS; i++) cha[i] = 0;

    for(i = 0; i < strlen(string); i++) cha[string[i]]++;

    int index = cha[string[0]];
    char c = string[0];

    for(i = 1; i < strlen(string); i++)
    {
        if(index > cha[string[i]])
        {
            index = cha[string[i]];
            c = string[i];
        }
    }

    return c;
}

Which one is more efficient, and is there an even more efficient way?

Common characteristics

Asymptotic complexity

Both algorithms perform θ(n) operations for strings of length n.  Since the task always requires examining every character of the input, there can be no alternative that provides better asymptotic complexity than these do.  Additionally, each has memory overhead that does not vary with the input, which affords no asymptotically better implementation in that respect either.  Of course, implementations with the same asymptotic complexity are not all equivalent by either measure.

See also below, however: your implementations of these algorithms are potentially flawed and less efficient.

Wrong range for char

Your codes assume that the string elements all have numeric value between 1 and 126, inclusive, but char can be a signed type, and whether signed or unsigned it can have more than 7 value bits.  Also, (char) 127 is always a valid char (from C's perspective).  As a result, on any conforming C implementation there are valid C strings which, if provided as input to either of your functions, will cause that function to exhibit undefined behavior.

To solve this problem I suggest declaring your arrays with dimension UCHAR_MAX + 1, and casting input chars to type unsigned char before using them as indexes.

Failure behavior

Each function exhibits somewhat surprising behavior for non-empty inputs that contain no unrepeated characters.  They return that character among those with the minimum number of repeats whose first appearance is earliest in the string.  It's unclear what the function is supposed to do in that case, but the the actual behavior is potentially misleading.  It would be better to return an error code.

You could use 0 as an error code, as it is the only char value that cannot appear inside a string.  Your functions will already return 0 when presented with an empty string as input, though I'm uncertain whether that's by design.  Alternatively, the functions could return type int, with the result being -1 when there is no unrepeated character, else the first non-repeated one cast to unsigned char.  This is the scheme the getchar() function uses.

Loop termination conditions

Both codes contain loops with i < strlen(string) for their termination condition.  The compiler can lift the evaluation of strlen() out of the loop only if it makes several assumptions about that function.  Inasmuch as that's a standard library function, it is conceivable that that would happen, but by no means certain.  If it doesn't happen then your implementations become θ(n2) instead of θ(n); I therefore recommend lifting the strlen() call out of the loop manually.

Alternatively, consider avoiding strlen() altogether -- since you need to iterate through the whole string anyway, just break the iteration when you reach the terminator.  That should be more efficient.

Using type int for character counters

Each code uses objects of type int as character counters, but

that's a signed type, even though character counts should never be negative, and
its maximum value can be as low as 32767, which is a relatively low limit

I would suggest size_t instead, which should be sufficient for any representable string, but if you prefer something that may be a bit smaller (at the cost of restricting its range) then you could go with uint_least32_t (from stdint.h) for the smallest an unsigned integer type with at least 32 value bits, or uint_fast32_t for the fastest unsigned integer type with at least 32 value bits (these might or might not be the same type).

First implementation

Use of a file-scope variable

This version of the function relies on file-scope variable characters.  Being declared a file scope gets it free initialization, but relying on that makes your function a one-shot.  Even if you reset the contents of this array on each function call, relying on it makes your function non-thread-safe.

I recommend switching to a local (array) variable inside the function.  If (and only if) you continue to use a file-scope variable then at least declare it static to give it internal linkage.

Questionable data structure

The struct ch type on which this version of the function relies seems a bit of a miss to me, for, as it is used, the value of member c is completely determined by the index of a given instance within array characters.  This member is therefore redundant.

See also my previous comments about using type int for your character counters.

Second implementation

Make use of initialization

You declare array cha without an initializer, and then manually loop to set its initial contents.  Given the data type and the initial values you want to set, it would be clearer and potentially more efficient to instead declare it with an initializer:

int cha[NUM_CHARACTERS] = { 0 };

... and skip the initialization loop.  (Elements not explicitly designated in the initializer get initialized to 0.)

Comparative efficiency

It's hard to be sure what to compare here.  The two codes comprise slightly-different versions of the same underlying algorithm.  Implementing fixes for the various shortcomings I discussed would bring them to pretty much the same place, which would look more like the second code than the first.

There are a number of possible variations on this approach, some of which may run slightly faster than others, but I don't see room for anything substantially more efficient.


Array of linked lists in C This is from an exercise I've done:

We want to implement a priority queue system to plan the execution of the system processes. An array of MAXPRI represents the priorities of the system, where 0 is the highest priority and MAXPRI-1 is the lowest priority.  Each position i in the array will contain a dynamic linked list where the first element of this is the priority i process that must be executed (provided there are no processes available in the previous i-1 positions). Implement the following operations:

Create. Initialize array.

AddProcess. Given a priority and a process identifier, it adds the process to the end of the corresponding list.

ExecuteProcess. Remove from the list the most priority process. If there are no processes to execute, it will be indicated with a warning message.

Search. Given a process identifier it returns the priority of this one. If the id of the process does not exist, -1 will be returned.

Display. Go through the structure to show the existing processes that are available for execution sorted by priority (highest first).

main.c, linkedListArray.c, linkedListArray.h

#include "linkedListArray.h"

void Create(Node **queue)
{
    for (int i = 0; i < MAXPRI; i++) {
        queue[i] = NULL;
    }
}

void Push(Node **queue, int pri, int id)
{
    Node *new_node = malloc(sizeof(Node));
    if (new_node != NULL) {
        new_node->id = id;
        new_node->next = NULL;
        if (queue[pri] == NULL) {
            queue[pri] = new_node;
        }
        else {
            Node *aux = queue[pri];
            while (aux->next != NULL) {
                aux = aux->next;
            }
            aux->next = new_node;
        }
    }
}

void Execute(Node **queue)
{
    bool end = false;
    int i = 0;

    while (!end && i < MAXPRI) {
        if (queue[i] != NULL) {
            Node *aux = queue[i];
            queue[i] = queue[i]->next;
            free(aux);
            end = true;
        }
        i++;
    }
    if (!end) {
        printf("There are no processes\n");
    }
}

int Search(Node **queue, unsigned int id)
{
    int pri = -1;
    Node *aux;

    for (int i = 0; i < MAXPRI  &&  pri == -1; ++i) {
        aux = queue[i];
        while (aux != NULL  &&  pri == -1) {
            if (aux->id == id) {
                pri = i;
            }
            else
                aux = aux->next;
        }
    }
    return pri;
}

void Output(Node *head)
{
    for (Node *current = head; current != NULL; current = current->next) {
        if (current->next != NULL)
            printf ("%d -> ", current->id);
        else
            printf ("%d", current->id);
    }
}

void Display(Node **queue)
{
    for (int i = 0; i < MAXPRI; i++) {
        printf ("Priority queue %d: ", i);
        Output(queue[i]);
        putchar('\n');
    }
    putchar('\n');
}

An if clause of Output is only executed once, on the very first node. You may avoid this logic altogether by making it explicit:

if (!head)
    return;
printf("%d", head->id);

for (head = head->next; head; head = head->next) {
    printf(" -> %d", head->id);
}

A missing

printf("\n");

seems like a bug (I hope it is a copy-paste error).
An early return helps to avoid unnecessary variables. Consider Search:

for (int i = 0; i < MAXPRI; i++)
    for (Node * aux = queue[i]; aux; aux = aux->next) {
        if (aux->id == id) {
            return i;
        }
    }
}
return -1;

A perk benefit is that now you can rename meaningless i to a more reasonable pri.

Similarly, if Execute returns as soon as a process is found, you wouldn't need bool end.
I strongly recommend to factor the

    while (aux->next != NULL) {
        aux = aux->next;
    }

loop into a function (say, get_tail), and streamline Push:

Node * node = new_node(id);
Node * tail = get_tail(pri);
if (tail) {
    tail->next = node;
} else {
    queue[pri] = node;
}

Using extra memory may significantly reduce the run time. In your case, the

struct list {
    Node * head;
    Node * tail;
};

would reduce looking for tail complexity from linear to \$O(1)\$.
linkedListArray.h shall only export the names defined in linkedListArray.c. The #include files required for linkedListArray.c compilation should be included directly in linkedListArray.c.


Chi Square Independence Test for Two Pandas DF columns I want to calculate the scipy.stats.chi2_contingency() for two columns of a pandas DataFrame. The data is categorical, like this:

var1    var2
0       1
1       0
0       2
0       1
0       2

Here is the example data: TU Berlin Server

The task is to build the crosstable sums (contingency table) of each category-relationship. Example:

         var1
         0    1
---------------------
     0 | 0    1
var2 1 | 2    0
     2 | 2    0

I'm not really a coder, but this is what I got (working):

def create_list_sum_of_categories(df, var, cat, var2):
    list1 = []
    for cat2 in range(int(df[var2].min()), int(df[var2].max())+1):
            list1.append( len(df[ (df[var] == cat) & (df[var2] == cat2) ]))
    return list1

def chi_square_of_df_cols(df,col1,col2):
    ''' for each category of col1 create list with sums of each category of col2'''
    result_list = []
    for cat in range(int(df[col1].min()), int(df[col1].max())+1):
        result_list.append(create_list_sum_of_categories(df,col1,cat,col2))

    return scs.chi2_contingency(result_list)

test_df = pd.read_csv('test_data_for_chi_square.csv')
print(chi_square_of_df_cols(test_df,'var1','var2'))

My question gears towards two things:

Can you confirm that this actually does what I want?
If you have suggestions to make this code more beautiful (e.g. include everything in one function), please go ahead!

I would try to use existing pandas features where possible to keep this code minimal - this aids readability and reduces the possibility of bugs being introduced in complicated loop structures.

import pandas
from scipy.stats import chi2_contingency

def chisq_of_df_cols(df, c1, c2):
    groupsizes = df.groupby([c1, c2]).size()
    ctsum = groupsizes.unstack(c1)
    # fillna(0) is necessary to remove any NAs which will cause exceptions
    return(chi2_contingency(ctsum.fillna(0)))

test_df = pandas.DataFrame([[0, 1], [1, 0], [0, 2], [0, 1], [0, 2]], columns=['var1', 'var2'])
chisq_of_df_cols(test_df, 'var1', 'var2')


Singleton design pattern I am a beginner in PHP-OOP and design patterns. I have got this basic piece of code in Singleton Pattern and I changed it in order to understand its behavior. I can see that it works as expected.

However, it is not clear for me exactly how it works. Can anyone explain me? So it is working! My doubt is "Whis $inst is not always null in the if since I am defining it null before entering the if"?

<?php

final class UserFactory // "final" prevents child classes from overriding a method by prefixing the definition
{
    private $state=1;

    /**
     * Call this method to get singleton
     *
     * @return UserFactory
     */
    public static function Instance()
    {
        static $inst = null;
        if ($inst === null) {
            $inst = new UserFactory();
        }
        return $inst;
    }

    private function __construct()
    {

    }

    public function addToState(){
        $this->state=$this->state+1;
    }
    public function getState(){
        return $this->state;
    }
}

// $myFactory=new UserFactory(); //Throws an error

$myFactory=UserFactory::Instance();
print_r($myFactory);

echo "
";
echo 'myState: '.$myFactory->getState()."
";

$myFactory->addToState();

echo 'myState: '.$myFactory->getState()."
";

$myFactory2=UserFactory::Instance();
print_r($myFactory2);

?>

The output is:

UserFactory Object ( [state:UserFactory:private] => 1 )
myState: 1
myState: 2
UserFactory Object ( [state:UserFactory:private] => 2 ) UserFactory Object ( [state:UserFactory:private] => 2 )

You're misunderstanding how the static keyword works.

The first time it is encountered for a specific variable in the method it defines the $inst variable as null, then checks to see if it is null (it is :p) and then sets it to a new object of that class.

The second time, the static definition isn't used because the static variable has already been defined. It then checks to see if it is null, it isn't, it has already been set to a new object of the class, so it skips the if and returns the object.


Detecting over-/underflow for uint64+int64 I am trying to write a function in Go which adds an uint64 value and an int64 value and which detects whether the result can be represented as an uint64.

func addUintInt64(x uint64, y int64) (uint64, bool) {
    if y >= 0 {
        z := x + uint64(y)
        if z < x {
            return 0, false
        }
        return z, true
    } else if y == -(1 << 63) {
        if x < 1<<63 {
            return 0, false
        }
        return x - 1<<63, true
    } else {
        negY := uint64(-y)
        if x < negY {
            return 0, false
        }
        return x - negY, true
    }
}

(Also on the Go Playground)

The second argument indicates whether the result is in the range of an int64.  If this is true, the first argument is the sum of x and y.

My questions:

Is this code correct?  I tested all boundary cases I could think of, but maybe I overlooked something?
Is there a nicer or more obvious looking way to write this code?  The fact that I'm not sure whether the code is correct probably indicates that it is not well written.
Is there a more efficient way to perform this task?  In particular, I would love to get rid of the special case for y == -(1<<63).  Note that I don't care about the first result, in cases where the second result is false.  Maybe this offers some potential for optimisations?

I really like that you put those tests together in the playground, thanks!

I am not a fan of losing information that may be useful. You will always need to check 0-value results to see if they indicate an overflow/underflow condition, so it's just simpler to return a useful result for those times when overflow is OK.... why return 0 when the actual remaining result is just as good?

More specifically, you will always have code like:

value, ok := addUintInt64(x, y)
if !ok {
   ....
}

so why force the 0 in to the value for !ok?

In addition, the magnitude of an int64 is never going to exceed half the magnitude of a uint64, so a double-overflow is not possible, and the logic required to check an overflow condition is a case of checking the change, or the direction of the change, of the sign-bit of the equivalent int64 value.

Perhaps this is best described if you handle the complete calculation in int64 space, and convert the result to uint64 - and realize that the MSB of the uint is the sign bit of the int.

The following conditions are all OK:

the sign bit does not change after the addition, then there's no overflow condition (no possibility of wrap)
the sign bit changes, but the int value was positive and the source sign bit was not set
the sign bit changes, but the int value was negative and the source sign bit was set

This is easy to codify in Go as:

result := uint64(int64(x) + y)
before := x >> 63
after := result >> 63
ok := before == after || (before == 0 && y >= 0) || (before == 1 && y < 0)

The above calculates in int64 space, wraps back to uint64 space, and computes the overflow conditions....

Using your same test cases (with the 0 return value for !ok) you can see this in the playground.

I would rather return the result though even if !ok, to avoid the additional check, so it would be the function:

func addUintInt64(x uint64, y int64) (uint64, bool) {
    result := uint64(int64(x) + y)
    before := x >> 63
    after := result >> 63
    ok := before == after || (before == 0 && y >= 0) || (before == 1 && y < 0)
    return result, ok
}


Create secret messages The problem is stated in Encrypt this!:

Your message is a string containing space separated words. You need to
encrypt each word in the message using the following rules:

The first letter needs to be converted to its ASCII code.

The second letter needs to be switched with the last letter

Keepin' it simple: There are no special characters in input.

Kata.encryptThis("Hello") => "72olle"
Kata.encryptThis("good")` => "103doo"
Kata.encryptThis("hello world") => "104olle 119drlo"

class NewEnpc{
    public static String encryptThis(String text) {
        StringBuilder sb=new StringBuilder();
        String[] arry=text.split(" ");
        if(text!=""){
            for(int j=0;j<arry.length;j++){
                char[] sd=arry[j].toCharArray();
                int number=(int)sd[0];
                if(sd.length>1){
                    for(int k=0;k<sd.length;k++){
                        if(k==0){
                            sb.append(number);
                        }
                        else if(k==1){
                            int len=sd.length-1;
                            char c=sd[len];
                            sb.append(c);
                        }
                        else if(k==sd.length-1){
                            char c=sd[1];
                            sb.append(c);
                        }
                        else{
                            char c=sd[k];
                            sb.append(c);
                        }
                    }
                }
                else{
                    sb.append(number);
                }
                sb.append(" ");
            }
        }
        return sb.toString().trim();
    }
}

public class EncryPt {
    public static void main(String[]args){
        System.out.println("Hello");

      System.out.println(NewEnpc.encryptThis("A
        wise old owl lived in an oak"));
    }
}

Can someone help me improve my coding standards. I have solved the problem.

I would suggest following:

Do not compare strings by ==, use String.equals(). And in Your case is better String.isEmpty()

text != "" → !text.isEmpty()
Use constants for numbers and strings. It increases readability and futher refactoring.

private static final String WORD_SEPARATOR = " ";
There is no need to use indexed iterations

for(int j=0;j<arry.length;j++) → for (String s : arry)
Use IDE with spelling checker:

arry → array
Use meaning variable names.

String s → String word
No need to use cast:

int number = (int) sd[0]; → int number = sd[0];
Decrease level of visibility

public static String encryptThis → static String encryptThis
Use method extraction to decrease nesting level

private static StringBuilder encrypt(char[] wordChars, int number)
You add first char in any case so condition can be omitted.
You could use chain of method .append(...) calls for StringBuilder
It is nice to move conditions like k==1 to meaningful named method:

See isSecondChar(...)
It is nice to validate input params early and prevent any actions if it is not valid:

if(text!="") → if (text.equals("")) then return empty string;

I would strongly suggest you to read great Joshua Bloch 'Effective Java' book.

Have a look at the refactored code:

class NewEnpc {

    private static final String WORD_SEPARATOR = " ";

    static String encryptThis(String text) {
        if (text.isEmpty()) {
            return text;
        }
        StringBuilder result = new StringBuilder();
        String[] words = text.split(WORD_SEPARATOR);
        for (String word : words) {
            char[] wordChars = word.toCharArray();
            result.append((int) wordChars[0])
                    .append(encrypt(wordChars))
                    .append(" ");
        }
        return result.toString().trim();
    }

    private static StringBuilder encrypt(char[] wordChars) {
        StringBuilder encrypted = new StringBuilder();
        for (int index = 1; index < wordChars.length; index++) {
            if (isSecondChar(index)) {
                int len = wordChars.length - 1;
                encrypted.append(wordChars[len]);
            } else if (isLastChar(index, wordChars.length - 1)) {
                encrypted.append(wordChars[1]);
            } else {
                encrypted.append(wordChars[index]);
            }
        }
        return encrypted;
    }

    private static boolean isLastChar(int currentIndex, int prevCharIndex) {
        return currentIndex == prevCharIndex;
    }

    private static boolean isSecondChar(int currentIndex) {
        return isLastChar(currentIndex, 1);
    }
}

public class EncryPt {
    public static void main(String[] args) {
        System.out.println("Hello");
        System.out.println(NewEnpc.encryptThis("A wise old owl lived in an oak"));
    }
}


Check if a string is a permutation of a palindrome I see a couple of these on codereview but I was hoping my way hasn't yet been encountered:

def is_palindrome_permutation(strng):
    cache = set()
    for char in strng.lower():
        if not char.isalpha():
            continue
        if char in cache:
            cache.remove(char)
        else:
            cache.add(char)

    return len(cache) <= 1

Instead of maintaining any counts of characters to check for oddness or evenness, simply add and remove characters from a set. A character repeated an even number of times will be removed from the set; a character repeated an odd number of times will remain in the set. If the final set has a length less than or equal to one, it is a permutation of a palindrome.

Documentation

You should definitely add a docstring. It took me a few seconds to figure out was was going. Make sure to add some examples in the docstring as well.

Naming

The name cache may not be the best name. Why? Let's look at the definition of cache:

  is a hardware or software component that stores data so future requests for that data can be served faster

I don't really believe you use cache to serve stuff faster. I believe odd_occuring_letters is a better idea. (I don't like long names, maybe challenge yourself to a shorter name? Maybe odd_letters? Think about it.)

(More) Comments

Even then, odd_occuring_letters isn't the most descriptive name, so you should include some comments as to what your code does. Don't over do it though.

Remember saying something like:

# Check the length of odd_occuring_letters is less than or equal to one.
return len(odd_occuring_letters) <= 1

Is not good. But saying:

# A word can have at most 1 odd character and still be palindrome.
return len(odd_occuring_letters) <= 1

explains to me why you return the length compared to one.


Finding Permutations of Numbers I'm trying to solve this challenge, which consists of finding the next biggest number formed by the digits of the passed in number. If it is impossible to find a number larger than the passed in parameter by rearranging the digits, return -1.

EX: nextBigger(12) === 21
    nextBigger(2017) === 2071
    nextBigger(987654321) === -1

My solution works, but they are testing a bunch of really long numbers that are causing it to time out. I don't even get to see what these really long numbers are because it times out and doesn't return any test cases. I was wondering what could be done to optimize this further.

function nextBigger(n){

      let numString = n.toString();
      let permutations = []
      let smallest = numString[0];
      let descending = true;
      let same = true;

      for(let i = 1; i < numString.length;i++){
        if(numString[i-1] < numString[i]){
            descending = false;
        }
        if(numString[i-1] != numString[i]){
            same = false;
        }

        if(smallest > numString[i]){
          smallest = numString[i]
        }
      }
      if(descending || same){
        return -1;
      }

      const permute = (prefix, str) => {
        let index = String(str).length;
        if(index == 1){
          permutations.push( prefix + str );
        }

        for(let i = 0; i < index; i++){
          permute(prefix + str[i], str.slice(0,i) + str.slice(i+1,index))
        }
      }

      let nextBiggest = Infinity;
      for(let i = numString.length - 2; i >= 0; i--){
          let start = numString.slice(0,i);
          let end = numString.slice(i,numString.length)
          permute(start,end)
          for(let j = 0; j < permutations.length; j++){
            if(permutations[j] > n && permutations[j] < nextBiggest){
              nextBiggest = permutations[j];
            }
          }
          if(smallest == numString[i]){
            return nextBiggest
          }
          permutations = [];
      }
      return -1
    }

    console.log(nextBigger(28554))
    //42558
    console.log(nextBigger(1961821))
    //1962118
    console.log(nextBigger(12))
    //21
    console.log(nextBigger(1111))
    //-1
    console.log(nextBigger(987654321))
    //-1

I'm not entirely sure what this code does, but it certainly seems too long for the problem. First some general review:

      let numString = n.toString();

This is a matter where opinions will certainly differ, but I would prefer to instead assign n = n.toString(); as n is never used again, and the real purpose of this line is to coerce the input to the type which you want to receive.

      let permutations = []

This is a bit surprising: you only need to generate one permutation, so why do you need an array with many? A comment explaining the purpose would be nice.

      let smallest = numString[0];

This is also surprising. "Smallest" in what sense? It's unlikely that the smallest digit will coincidentally be the first one.

      if(descending || same){
        return -1;
      }

Is there any reason for using two variables here? I think it could be simplified by having one variable which tracks whether there's any increase over the previous digit (although the complete rewrite will eliminate this anyway).

      const permute = (prefix, str) => {

What does this function do? Comment!

        let index = String(str).length;

Earlier the code used toString(); here it uses String(...) to do the same thing. Be consistent.

        if(index == 1){

Although index will always be a number, consistently using === reduces the cognitive load in working out whether == should really have been ===.

      let nextBiggest = Infinity;

WTF? This method was working with strings: where does Infinity come into it?

      return -1

This should be unreachable, so why not throw an exception to alert you to the fact that if it's reached you have a bug?

Now, the part you're really interested in. The algorithm.

The question of finding the next permutation in lexicographic order is a classical one (going back 7 centuries) and well documented in the literature. E.g. Wikipedia gives the following algorithm:

  Find the largest index k such that a[k] < a[k + 1]. If no such index exists, the permutation is the last permutation.
  Find the largest index l greater than k such that a[k] < a[l].
  Swap the value of a[k] with that of a[l].
  Reverse the sequence from a[k + 1] up to and including the final element a[n].


Processing files retrieved through FTP in parallel I am busy creating a file processor that needs to get some files from an Ftp client download the file and save the data into the database. I have a two part question.

How can i refactor the code to look a bit cleaner? Feels like the
    method ProcessFiles is doing a bit much. I can extract some of the
    methods but then i am just sitting with a lot of small methods and
    not really gaining anything.
What will be the best way the handle exceptions? I need to be able
to capture the file that caused an error.

Code Sample

public void Run()
        {
            var ftpFileManager = new FtpFileManager("ftp.someurl.net", "username", "password");
            var files = ftpFileManager.GetDirectory("somepath");

            try
            {
                ProcessFiles(files, ftpFileManager);
            }
            catch (AggregateException ae)
            {
                foreach (var ex in ae.InnerExceptions)
                {
                    if (ex is ArgumentException)
                        Console.WriteLine(ex.Message);
                    else
                        throw ex;
                }
            }
        }

private void ProcessFiles(IEnumerable<FtpListItem> files, IFtpFileManager ftpFileManager)
        {
            var exceptions = new ConcurrentQueue<Exception>();
            files.AsParallel().ForAll(async _ =>
            {
                try
                {
                    var dataFileService = _container.GetExportedValue<IDataFileHandler>();
                    var progress = new ProgressReporter(_container.GetExportedValue<IDataFileHandler>());

                var attr = FtpFileAttributes.Parse(_.Name);
                attr.DateOfFile = _.Modified;
                var dataFileId = await dataFileService.AddAsync(attr);

                await progress.Report(Status.Started, dataFileId);

                var file = ftpFileManager.DownloadFile(_.FullName);
                var fileHandler = FileHandlerFactory.Create(attr.FileType, attr);
                var list = fileHandler.ProcessFile(file);

                var handler = CreateHandler(attr.FileType);
                await handler.AddAsync(list, dataFileId);

                var dataFile = await dataFileService.GetAsync(dataFileId);
                dataFile.EndDate = DateTime.Now;
                dataFile.TotalRecords = list.Count();
                await dataFileService.UpdateAsync(dataFile);

                await progress.Report(Status.Completed, dataFileId);
            }
            catch (Exception e)
            {
                exceptions.Enqueue(e);
            }
        });

        if (exceptions.Count > 0)
            throw new AggregateException(exceptions);
    }

Exception order changes processing

This looks really odd to me:

catch (AggregateException ae)
{
    foreach (var ex in ae.InnerExceptions)
    {
        if (ex is ArgumentException)
            Console.WriteLine(ex.Message);
        else
            throw ex;
    }
}

It's basically saying write ArgumentExceptions to the Console, but only until you find an error that isn't an ArgumentException.  Then throw that exception.

I don't like it firstly because it could have different results based on order that the exceptions have been added to the queue and secondly because it discards any exceptions after the first one.

Exceptions for program flow

Having ProcessFiles which is a private method return void and throw an aggregate exception, which is immediately caught in the public caller, feels wrong to me.  You've already collected the exceptions into a queue, why not just have the method return the queue?  The caller can then decide what to do based on the collection being empty or not.


Tic Tac Toe game in haskell follow up This is a follow-up question of this one: Tic Tac Toe game in Haskell

I have revised my code. How does it look now?

import System.Random (randomRIO)
import System.IO (hFlush, stdout, getLine)
import Data.List (intercalate)
import Data.Array

data Tile   = EmptyTile | X | O deriving (Eq)
data Player = Player1 | Player2

showTile :: Tile -> String
showTile EmptyTile = " "
showTile X         = "X"
showTile O         = "O"

showBoard :: Board -> String
showBoard b = let bstr = fmap showTile b
                  blist = boardAsList bstr
              in  unlines [intercalate "|" row | row <- blist]
              where
                boardAsList b = [[b!(x,y) | y <- [1,2,3]] | x <- [1,2,3]]

type Board = Array (Int,Int) Tile

emptyBoard :: Board
emptyBoard = array ((1,1),(3,3)) [((x,y), EmptyTile) | x <- [1,2,3], y <- [1,2,3]]

put :: Board -> Tile -> Int -> Int -> Maybe Board
put b t x y = case b!(x,y) of
                EmptyTile -> Just (b // [((x,y), t)])
                _         -> Nothing

p1wins, p2wins :: Board -> Bool
p1wins b = tileWins b X
p2wins b = tileWins b O

tileWins :: Board -> Tile -> Bool
tileWins b t =
   any (\row -> all (\col -> b!(row,col) == t) [1..3]) [1..3] ||
   any (\col -> all (\row -> b!(row,col) == t) [1..3]) [1..3] ||
   all (\rc -> b!(rc,rc) == t) [1..3] ||
   all (\rc -> b!(rc,4-rc) == t) [1..3]

checkFull :: Board -> Bool
checkFull b = all (\row -> all (\col -> b!(row, col) /= EmptyTile) [1..3]) [1..3]

compMove :: Board -> IO (Board)
compMove b = do
    (row, col) <- getRandomEmpty b
    let (Just b') = put b O row col
    return b'

getRandomEmpty :: Board -> IO (Int, Int)
getRandomEmpty b = do
    col <- randomRIO (1,3)
    row <- randomRIO (1,3)
    case b!(row, col) of
        EmptyTile -> return (row, col)
        _         -> getRandomEmpty b

prompt :: String -> IO String
prompt s = do
    putStr s
    hFlush stdout
    getLine

showTileNumbers :: String
showTileNumbers  = (unlines
                   [intercalate "|" ["(" ++ show x ++ "," ++ show y ++ ")" |
                   y <- [1,2,3]] | x <- [1,2,3]])

main :: IO ()
main = do
    putStrLn "This is classic tic tac toe game."
    putStrLn "In order to play, you need to put a number between 0 and 8"
    putStrLn "This table shows tile numbers"
    putStrLn showTileNumbers
    putStrLn $ showBoard emptyBoard
    playTurn emptyBoard Player1
    where
        playTurn b Player1 = do
            row <- prompt "Row: "
            col <- prompt "Col: "
            let newboard = put b X (read row) (read col)
            case newboard of
                Nothing -> do
                            putStrLn "Invalid move."
                            playTurn b Player1
                Just b' -> if p1wins b' then
                                putStrLn "You win"
                            else
                                playTurnIfNotTie b' Player2
        playTurn b Player2 = do
            b' <- compMove b
            putStrLn $ showBoard b'
            if p2wins b' then
                putStrLn "You Lose!"
            else
                playTurnIfNotTie b' Player1

        playTurnIfNotTie b p = if checkFull b then
                                  putStrLn "Tie."
                               else
                                  playTurn b p

With the new board representation, the brute-force pattern matching is gone, which is good.  Still, I personally prefer the 1–9 numbering for simpler user interface — perhaps a translation function could be implemented.  Note that you forgot to adjust the introductory message for the new coordinate system.

Instead of the showTile function, you should use the standard Show typeclass:

instance Show Tile where
    show EmptyTile = " "
    show X         = "X"
    show O         = "O"

In showBoard you used both let … in and where.  Pick one or the other (I prefer where).

As mentioned in my previous answer, all of your functions that accept a Board should place the board parameter last, since the board is what they all operate on.  With tileWins :: Tile -> Board, for example, you can define p1wins = tileWins X using the point-free style.

data Player = Player1 | Player2 isn't doing anything useful for you.  You are hard-coding the Player1 ↔ X and the Player2 ↔ O correspondence all over the place, in main, p1wins, p2wins, and compMove.

With this second version, then, I would say that the greatest improvement can be made in handling the two players.  Here is my suggestion:

humanTurn :: Tile -> Board -> IO Board
humanTurn t b = do
    putStrLn $ showBoard b
    move
    where
        move = do
            row <- prompt "Row: "
            col <- prompt "Col: "
            let newboard = put b t (read row) (read col)
            case newboard of
                Nothing -> do
                            putStrLn "Invalid move."
                            move
                Just b' -> return b'

computerTurn :: Tile -> Board -> IO Board
computerTurn t b = do
    (row, col) <- getRandomEmpty b
    let (Just b') = put b t row col
    return b'

main :: IO ()
main = do
    putStrLn "This is the classic tic tac toe game."
    putStrLn "Enter the coordinates for each move according to this scheme:"
    putStrLn showTileNumbers
    play player1 player2 emptyBoard
    where
        player1 = (X, humanTurn, "You win!")
        player2 = (O, computerTurn, "You lose.")
        play p@(tile, turn, winMsg) p' b = do
            b' <- turn tile b
            if tileWins tile b' then do
                putStrLn $ showBoard b'
                putStrLn winMsg
            else if checkFull b' then do
                putStrLn $ showBoard b'
                putStrLn "Tie."
            else
                play p' p b'

This turns main into a leaner state machine.  As evidence of the superiority of this design, observe that you can make a human-vs.-human game simply by changing two lines to…

       player1 = (X, humanTurn, "X wins.")
       player2 = (O, humanTurn, "O wins.")


Replace the content of a file by zero-bytes and keep the same filesize Here is a little tool for Windows that replaces the content of a file with zero-bytes, keeping the same filesize. It works. The goal is to be sure that the old bytes-content of the file is no more recoverable on disk.

Question: how to be 100% sure that it replaces the content in-place and that it doesn't allocate new sectors on disk to write the zero bytes? (thus leaving the old content somewhere else, still possibly accessible, which would be a security hole!)

Note: I know that freewares exist for this, but I would like to try a simple implementation first

Note2: I know that some more advanced wiping techniques exist (random content and/or multi-pass, etc.), but for me zeroing the content is enough if at least I know that the zeroing is done in-place, i.e. if the new zero bytes replace the old content at the same location on disk (and not moved to another sectors on disk!)

Could you review the following code?

#include <stdio.h>

int main(int argc, char *argv[])
{
    FILE *f;

    if (argc != 2) { fprintf (stderr, "Usage: %s <file>\n", argv[0]); exit(1); }

    if ((f = fopen(argv[1], "rb+")) == NULL) { perror("Error opening the file"); exit(1); }

    fseek(f, 0L, SEEK_END);
    unsigned long size = ftell(f);
    fseek(f, 0L, SEEK_SET);

    char buf[1024] = {0};
    unsigned long num = size / 1024;
    int remaining = size % 1024;

    for (unsigned long i = 0; i < num; i++)
        fwrite(buf, 1, 1024, f);

    fwrite(buf, 1, remaining, f);

    fclose(f);
    exit(0);
}

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

Use return rather than exit()

The effect of using exit(0) or exit(1) is going to be identical to return 0 or return 1 but with one difference: exit() is a library call and would therefore require #include <stdlib.h> (which this program does not have) while return does not require any particular #include.  Also note that exit(0) implicitly flushes and closes files, as does return 0.  Further, the equivalent to return 0; is provided by the compiler if omitted (since C99), so technically, the last two lines of the program:

fclose(f);
exit(0);

could both be omitted with no change to the function of the program.  If, however, you decided to move this part of the function to a separate function, I'd suggest keeping fclose(f); and using return 0; rather than exit(0);.  Tastes vary on this point.

Don't cram multiple statements per line

Lines such as this:

if ((f = fopen(argv[1], "rb+")) == NULL) { perror("Error opening the file"); exit(1); }

tend to be more readable if formatted as separate lines:

if ((f = fopen(argv[1], "rb+")) == NULL) {
    perror("Error opening the file");
    return 1;
}

Check return values

The code does check for the return value of fopen, which is good, but does not check the return values for fseek, ftell and fwrite which can also fail.

Use named constants

The number 1024 appears multiple times in the code.  If one wanted, for instance, a 2k buffer instead, one would have to change it all three places.  Using a named constant instead simplifies maintenance and makes the code easier to read as well.

Understand the bounds of standard C

The C language is very expressive and has a great number of handy library functions for doing things like reading and writing files.  However, what exactly the underlying operating system does to manage files on disk is outside the domain of C and squarely in the domain of the OS and hardware designers.  There is, in short, no guarantee that this code will physically overwrite the same sectors of physical media, and more importantly, no provision in standard C for doing so.  To take just one example, if I run a virtual Windows machine on a system that actually runs on a cluster of Linux machines, there might not actually even be any physical media.


Validating initial inputs then a joint validation test The code works as intended but I believe it is very inefficient. The calculation of weights is simple, but I imagine this would be silly, for example, a 100 asset portfolio. The first problem is that my code returns to the beginning of the loop if the weight for y is entered incorrectly. Help with this would be amazing.
Secondly, as alluded to above, this is an inefficient approach. Someone entering 100 weights would not repeat this process. Any ideas where I could find some help for this approach would be great.
r_x=0.15
r_y=0.21
while True:
    try: w_x=float(input("Enter the weight of stock x in your portfolio: "))
    except ValueError:
        print("Please enter the weight using numbers")
        continue
    else:
        if w_x<0 or w_x>1: #seperating it instead of doing it altogether corrected this error for me
            print("Please enter the number as a decimal. Remember, your maximum investment is 100%.")
            continue
    try: w_y=float(input("Enter the weight of stock y in your portoflio: "))
    except ValueError:
        print("Please enter the weight using numbers")
    else:
        if w_y<0 or w_y>1:
            print("Please enter the numbers as a decimal. Remember, your maximum investment is 100%.")
            continue
        else:
            if w_x+w_y<0 or w_x+w_y>1:
                print("Remember, the total weight of the portfolio is 100%. The weights should add to 1.")
                continue
            else:
                r_p=r_x*w_x+r_y*w_y
                r_p=r_p*100
                print("The total return on your portoflio is %.2f"%r_p +"%")
                break

You should write a function that takes care of asking the user for input. It should be able to assign a type and validate the input. This way you can continue asking for a valid input for each of the variables independently. It also allows you to validate the second variable using the value of the first.
def ask_user(message, type_=str, valid=lambda x: True, wrong_type="Wrong type!", invalid="Invalid!"):
    while True:
        try:
            x = type_(input(message))
        except (TypeError, ValueError):
            print(wrong_type)
            continue
        if valid(x):
            return x
        else:
            print(invalid)

Which you can use like this:
if __name__ == "__main__":
    r_x = 0.15
    r_y = 0.21
    w_x = ask_user("Enter the weight of stock x in your portfolio: ",
                   type_=float,
                   wrong_type="Please enter the weight using numbers",
                   valid=lambda w_x: 0 <= w_x <= 1,
                   invalid="Please enter the number as a decimal. Remember, your maximum investment is 100%.")
    w_y = ask_user("Enter the weight of stock y in your portoflio: ",
                   type_=float,
                   wrong_type="Please enter the weight using numbers",
                   valid=lambda w_y: 0 <= w_x + w_y <= 1,
                   invalid="Remember, the total weight of the portfolio is 100%. The weights should add to 1.")
    r_p = r_x * w_x + r_y * w_y
    print(f"The total return on your portoflio is {r_p:.2%}")

I also used the newer f-strings for string formatting, with the % format code, which takes care of that multiplication by one hundred. I also added a if __name__ == "__main__": guard to allow importing from this script without the code being run.
Python has an official style-guide, PEP8. It recommends using spaces around = when using it for assignment, but not when using it for keyword arguments. Similarly, operators like + and * should generally also be surrounded by spaces.

If you want to be able to input n weights, I would add another function:
def ask_n_weights(message, n, type_=str, wrong_type="Wrong type!", invalid="Invalid!"):
    weights = []
    for i in range(1, n + 1):
        weights.append(ask_user(message.format(i),
                                type_=type_,
                                wrong_type=wrong_type,
                                valid=lambda w: 0 <= w + sum(weights) <= 1,
                                invalid=invalid))
    return weights

This uses the fact that sum([]) = 0 to also be correct for the first variable. In order not to loose the ability to know which weight you are entering I made it so that message should have one placeholder:
ask_n_weights("Enter weight {}: ", 4, type_=float)
# Enter weight 1: 0.25
# Enter weight 2: 0.25
# Enter weight 3: 0.25
# Enter weight 4: 0.25
# [0.25, 0.25, 0.25, 0.25]

You could also take an iterable of variable names instead of n.