Postfix evaluation using a stack in c I have written a program to evaluate a postfix expression using a stack. I had my stack implementation using a linked list reviewed here, so I am only including the header file here.

I have taken the postfix expression in form of a string where the operators and operands are delimited by spaces and have used a sentinel at the end to mark the end of the expression.

stack.h

#ifndef STACK_H
#define STACK_H

#include <stdbool.h>

typedef int StackElement;

typedef struct stack_CDT *Stack;

Stack stack_init();

void stack_destroy(Stack s);

bool stack_is_empty(Stack s);

void stack_push(Stack s, StackElement elem);

StackElement stack_pop(Stack s);

#endif

eval_postfix.h

#ifndef EVAL_POSTFIX_H
#define EVAL_POSTFIX_H

// space is the delimiter between the different tokens

extern const char *sentinel;

int eval_postfix(char *exp);

#endif

eval_postfix.c

#include "eval_postfix.h"
#include "stack.h"
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>

#define MAX_TOKEN_LEN 25

const char *sentinel = "$";

static char *get_token(char *token, char *exp, int idx)
{
    sscanf(exp + idx, "%s", token);
    return token;
}

static bool is_operator(char *token)
{
    return (strcmp(token, "/") == 0 || strcmp(token, "*") == 0 ||
            strcmp(token, "%") == 0 || strcmp(token, "+") == 0 ||
            strcmp(token, "-") == 0);
}

static int eval(int a, int b, char *op)
{
    if (strcmp(op, "/") == 0)
        return a / b;
    if (strcmp(op, "*") == 0)
        return a * b;
    if (strcmp(op, "%") == 0)
        return a % b;
    if (strcmp(op, "+") == 0)
        return a + b;
    if (strcmp(op, "-") == 0)
        return a - b;

    return 0;
}

int eval_postfix(char *exp)
{
    Stack s = stack_init();
    char token[MAX_TOKEN_LEN + 1];
    int i = 0;
    while (strcmp(get_token(token, exp, i), sentinel) != 0) {
        if (is_operator(token)) {
            int operand1 = stack_pop(s);
            int operand2 = stack_pop(s);
            stack_push(s, eval(operand2, operand1, token));
        } else {
            stack_push(s, (int)strtol(token, NULL, 0));
        }

        i += strlen(token) + 1; // one extra for the space
    }

    int res = stack_pop(s);
    stack_destroy(s);
    return res;
}

test.c

#include "eval_postfix.h"
#include <stdio.h>
#include <string.h>

#define MAX_EXPRESSION_LEN 100

int main()
{
    printf("Enter postfix expression(no more than %d characters): ",
            MAX_EXPRESSION_LEN);
    char exp[MAX_EXPRESSION_LEN + 3];
    fgets(exp, sizeof exp, stdin);
    char *pos;
    if ((pos = strchr(exp, '\n')) != NULL)
        *pos = '\0';
    strcat(strcat(exp, " "), sentinel);

    printf("Result = %d\n", eval_postfix(exp));
    return 0;
}

Optimize strcmp chains
Some of your functions use strcmp() repeatedly.  You could make those functions faster by eliminating the multiple calls to strcmp() and using switch statements instead.  This function:

static bool is_operator(char *token)
{
    return (strcmp(token, "/") == 0 || strcmp(token, "*") == 0 ||
            strcmp(token, "%") == 0 || strcmp(token, "+") == 0 ||
            strcmp(token, "-") == 0);
}

would become:
static bool is_operator(const char *token)
{
    if (token[1] != '\0')
        return false;

    switch (token[0]) {
        case '/':
        case '*':
        case '%':
        case '+':
        case '-':
            return true;
        default:
            return false;
    }
}

This function:

static int eval(int a, int b, char *op)
{
    if (strcmp(op, "/") == 0)
        return a / b;
    if (strcmp(op, "*") == 0)
        return a * b;
    if (strcmp(op, "%") == 0)
        return a % b;
    if (strcmp(op, "+") == 0)
        return a + b;
    if (strcmp(op, "-") == 0)
        return a - b;

    return 0;
}

could become:
static int eval(int a, int b, const char *op)
{
    switch (*op) {
        case '/':
            return a / b;
        case '*':
            return a * b;
        case '%':
            return a % b;
        case '+':
            return a + b;
        case '-':
            return a - b;
        default:
            return 0;
    }
}

Notice I also added a const to your string arguments because they aren't modified by your functions.
Confusing variable names
This code confused me a little:

        int operand1 = stack_pop(s);
        int operand2 = stack_pop(s);
        stack_push(s, eval(operand2, operand1, token));

Here, operand1 is actually the second operand, and operand2 is actually the first operand, as you can see from the call to eval().  I would have rewritten it like this, because otherwise it would get confusing if someone were stepping through it with a debugger and examining variable values:
        int operand2 = stack_pop(s);
        int operand1 = stack_pop(s);
        stack_push(s, eval(operand1, operand2, token));


Count words in string and tally up the repetitive words I'm an elixir beginner.  I'm doing the word count exercise of exercism website.  The word count exercise returns a map with the word as the key and the number of instances as value.  The exercise excludes special characters, and ignores underscores. My code works, but I'm sure there is a way to make it cleaner.  I've attached the test file so that you can see the criteria that it needs to pass.

word_count.exs

defmodule Words do
  @doc """
  Count the number of words in the sentence.

  Words are compared case-insensitively.
  """
  @spec count(String.t) :: map
  def count(sentence) do
    String.downcase(sentence)                                           |>
      String.split(~r/[\s_]/)                                           |>
      Enum.filter(fn(word) -> String.match?(word, ~r/[a-zA-z\d]/) end)  |>
      Enum.map(fn(word) -> String.replace(word, ~r/[:!&@$%^,]/, "") end)|>
      Enum.group_by(fn(word) -> word end)                               |>
      Enum.reduce(%{}, fn({k, v}, acc) -> Map.put(acc, k, Enum.count(v)) end)
  end
end

word_count_test.exs

if !System.get_env("EXERCISM_TEST_EXAMPLES") do
  Code.load_file("word_count.exs")
end

ExUnit.start
ExUnit.configure exclude: :pending, trace: true

defmodule WordsTest do
  use ExUnit.Case

  test "count one word" do
    assert Words.count("word") == %{ "word" => 1 }
  end

  test "count one of each" do
    expected = %{ "one" => 1 ,  "of" => 1 ,  "each" => 1 }
    assert Words.count("one of each") == expected
  end

  test "count multiple occurrences" do
    expected = %{ "one" => 1 ,  "fish" => 4 ,  "two" => 1 ,  "red" => 1 ,  "blue" => 1 }
    assert Words.count("one fish two fish red fish blue fish") == expected
  end

  test "ignore punctuation" do
    expected = %{"car" => 1, "carpet" => 1, "as" => 1, "java" => 1, "javascript" => 1}
    assert Words.count("car : carpet as java : javascript!!&@$%^&") == expected
  end

  test "include numbers" do
    expected = %{"testing" => 2, "1" => 1, "2" => 1}
    assert Words.count("testing, 1, 2 testing") == expected
  end

  test "hyphens" do
    expected = %{"co-operative" => 1}
    assert Words.count("co-operative") == expected
  end

  test "ignore underscores" do
    expected = %{"two" => 1, "words" => 1}
    assert Words.count("two_words") == expected
  end

  test "normalize case" do
    expected = %{"go" => 3}
    assert Words.count("go Go GO") == expected
  end

  test "German" do
    expected = %{"götterfunken" => 1, "schöner" => 1, "freude" => 1}
    assert Words.count("Freude schöner Götterfunken") == expected
  end
end

Just finished that exercise too.
I think you can extract some of that logic to anonymous functions.
It isolates a pieces of behaviour that you can work on.

defmodule Words do
  @doc """
  Count the number of words in the sentence.

  Words are compared case-insensitively.
  """
  @spec count(String.t) :: map
  def count(sentence) do
    filter_word = fn(word) ->
      String.match?(word, ~r/[a-zA-z\d]/)
    end

    remove_special_characters = fn(word) ->
      String.replace(word, ~r/[:!&@$%^,]/, "")
    end

    count_words = fn({k, v}, acc) ->
      Map.put(acc, k, Enum.count(v))
    end

    sentence
    |> String.downcase
    |> String.split(~r/[\s_]/)
    |> Enum.filter(filter_word)
    |> Enum.map(remove_special_characters)
    |> Enum.group_by(fn(word) -> word end)
    |> Enum.reduce(%{}, count_words)
  end

end

And actually, I've created a function that returns a list of words to count: http://exercism.io/submissions/e26667aff3734da79ac0b6c6fedfd3da

Hope that helps!


Doubly linked list implementation in Python Here's my implementation of doubly linked list in Python:

 class DList:

    def __init__(self):
        self.head = None
        self.size = 0

    def insert(self, data):
        self.size += 1
        if self.head is None:
            self.head = Node(data)
        else:
            p = Node(data)
            p.next = self.head
            self.head.previous = p
            self.head = p

    def remove(self, index):
        if self.head is None:
            raise ValueError('Removing off an empty list')
        if index < 0 or index >= self.size:
            raise IndexError("Index is either negative or greater than the list size.")
        current = self.head
        if index == 0:
            self.head = self.head.next
            self.head.previous = None
        else:
            for _ in range(index -1):
                current = current.next
            p = current.next.next
            if p is None:
                current.next = None
            else:
                current.next = p
                p.previous = current

    def __sizeof__(self):
        return self.size

    def __repr__(self):
        res = '[ '
        current = self.head
        while current is not None:
            res += str(current.data)
            res += ' '
            current = current.next
        res += ']'
        return res

class Node:
    def __init__(self, data):
        if data is None:
            raise ValueError('Node value cannot be None')
        self.data = data
        self.previous = None
        self.next = None

OK, a few things:

I would make head and size private, you don't want them changed from outside the class.
You should use __len__ instead of __sizeof__ - they have different meanings.
Implement __iter__
After doing #3, format the repr using:

"[{}]". format(", ". join(map(str, self)))


Pass pointer data across multiple functions I would like review on the following dynamic memory allocation process and suggestions on whether there are any memory leaks. Following code is not real code in use, but I'm trying to understand concepts of memory allocations and de-allocation in different ways.

class ObjMapData
{
private:
//Header Info fields to describe each object actual data type, total elements of actual data type, and lot more describing the object common fields
int itsDataTypeID; // 1 to 20+, each represent one type of data being used
int itsDataType; // 1 to 8, which could be UINT8, INT8, UINT16, INT16, UINT32, INT32
int itsDataSize; // 1 or 2 or 4 bytes
int itsTotalElements; // original data type elements, but not the total bytes of the binary file
//...... other common header information
// body data, (actual data could be UINT8, INT8, UINT16, INT16, UINT32, INT32 type)
char* itsMapData; // size = itsTotalElements*itsDataSize; type cast to various actual data types later to access/populate the data
........
public:
ObjMapData();
ObjMapData& operator = (const ObjMapData& objMyMap);
ObjMapData(const ObjMapData& objMyMap);
~ObjMapData(){if(itsMapData!= NULL) delete[] itsMapData;}
//Following two functions are used to allocate/free memory within the scope of the object so that it can be re-used again for a different data type object
ClearMemory() {if(itsMapData!= NULL) {delete[] itsMapData; itsMapData= NULL}}
void AllocateMemory( unsigned long  nSizeInBytes = 0)
{
if(itsMapData!= NULL)   {delete[] itsMapData; itsMapData= NULL;}
itsMapData= new (std::nothrow) INT8[nSizeInBytes];
}
.......
void SetMapData(char* ptrData) { itsMapData = ptrData;} // or should I use char*&ptrData??
char* GetMapData() const { return itsMapData;}
}

Now can I do the following without any memory leaks?

//There are 30+ functions like Function1() to populate the map data of different type of maps using different sources of input data or algorithms
bool Function1(ObjMapData& objMyMap)
{
//populate the ptrData with some data values using many different ways
int* ptrData = new int[1000]; // usually a variable from binary file header

......................
objMyMap.SetMapData((char*)ptrData);
//don't want to delete the memory yet as this map will be used along with map objects to create a new mapdata
return true;
}

bool Function2(ObjMapData& objMyMap)
{
int* ptrData = (int*) objMyMap.GetMapData();
//do some work such as writing updated data and then write into a binary file
}

bool Function3(ObjMapData& objMyMap)
{
//populate the data using different method for different map data types
//there could be 30+ other functions like Function1() to populate the map data
bool bStatus = Function1(objMyMap);

int* ptrData = (int*)objMyMap.GetMapData();
//update the map data by modifying ptrData variable
..........
objMyMap.SetMapData((char*)ptrData); // Do I need to set again or member pointer get updated automatically?
bStatus = Function2(objMyMap); // write data to a binary file
objMyMap.ClearMemory(); // not real code  in use, but for understanding the concept
bStatus = Function1(objMyMap); // re-allocate memory and assign a new Map Data
short* ptrData2 = (short*) objMyMap.GetMapData();
//update the map data using ptrData variable
objMyMap.SetMapData((char*)ptrData2); // Do I need to set again or member pointer get updated automatically?
return true
}
int main()
{
ObjMapData objMyMap;
bool bStatus = Function3(objMyMap);
short* ptrData = (short*)objMyMap.GetMapData();
//do something with ptrData
//default destructor of objMyMap can take care of the allocated memory cleanup
return 0;
}

Don't do your own memory management. Use the tools provided for you unless you have good reason to do otherwise.

This:

objMyMap.SetMapData((char*)ptrData);

causes undefined behavior. This is because when objMyMap is deleted you delete the pointer as a char pointer (it was created as an int pointer). Thus the runtime memory management will probably get the size of the block wrong.

If you ever use a C-Cast then you are probably doing something wrong. Always use C++ style casts. Then it is easy to spot when you are doing things wrong.

const_cast<>()          // Bad if you casting away constness.
reinterpret_cast<>()    // Bad unless used to interact with C libraries.
static_cast<>()         // Bad if used to cast pointers around
dynamic_cast<>()        // Safe but slow as it is a runtime cast.
                        // Note: Only useful for casting down class hierarchy
                        //       But it is also a sign of bad design.

Looking at the class.

class ObjMapData
{

There is no reason do your own memory management.

private:
char* itsMapData;

This should probably be a std::vector<char>. Once you do that the rest becomes irelavant and all memory management issues are solved.

........
public:
ObjMapData();

Don't bother to test for NULL. Just delete.

~ObjMapData(){if(itsMapData!= NULL) delete[] itsMapData;}
ClearMemory() {if(itsMapData!= NULL) {delete[] itsMapData; itsMapData= NULL}}
.......

This obviously leaks the memory that you had before.

void SetMapData(char* ptrData) { itsMapData = ptrData;} // or should I use int*&ptrData??

Sure. But only if you need to expose that data.
I would rather encapsolate the manipulation of the data inside the class so you don't need to expose it.

char* GetMapData() const { return itsMapData;}
}

You don't show it so I have to assume you are not following the "Rule of Three" (look it up).

I would have done this:

class ObjMapData
{
    private:
        std::vector<char> itsMapData;

    public:
        clearMemory() // lower case first letter of non type.
        {
            itsMapData.swap(std::vector<char>());
        }
        void setMapData(std::vector<char>& ptrData)
        {
             itsMapData.swap(ptrData);
        }
        char* getMapData() const { return &itsMapData[0];}
};

Addition based on comment:

  (could be INT8, UINT8, INT16, UINT16, UINT32, INT32)

// Note the above data is not correctly aligned on all systems.

// INT8   INT8      INT16            INT16

01234567  01234567  0123456701234567 0123456701234567  Not a 32 Bit Boundary
|         |         |       |        |       |         |
8/16/32 Boundary    8/16 Boundary    8/16/32 Boundary  8/16 Boundary
          8 Boundary        8 Boundary       8 Boundary

But lets assume you just made a typo and you know what you are doing.

// Need room for
// INT 8 | INT 8 | INT 16 | INT 32 | INT 32
std::vector<char>  data(96);
::read(fd, &data[0], 96);    // Wow look at that fits neatly.

INT8  d1 = *reinterpret_cast<INT8 *>(&data[ 0]);  // The reinterpret cast is required
INT8  d2 = *reinterpret_cast<INT8 *>(&data[ 8]);  // It also shows its really dangerious
INT16 d3 = *reinterpret_cast<INT16*>(&data[16]);
INT32 d4 = *reinterpret_cast<INT16*>(&data[32]);
INT32 d5 = *reinterpret_cast<INT32*>(&data[64]);


Checking for differences between two (large) files I want to write a relatively simple program, that can backup files from my computer to a remote location and encrypt them in the process, while also computing a diff (well not really...I'm content with seeing if anything changed at all, not so much what has changed) between the local and the remote files to see which ones have changed and are necessary to update.

I am aware that there are perfectly good programs out there to do this (rsync, or others based on duplicity). I'm not trying to reinvent the wheel, it's just supposed to be a learning experience for myself

My question is regarding to the diff part of the project. I have made some assumptions and wrote some sample code to test them out, but I would like to know if you see anything I might have missed, if the assumptions are just plain wrong, or if there's something that could go wrong in a particular constelation.

Assumption 1: If files are not of equal length, they can not be the same (ie. some modification must have taken place)
Assumption 2: If two files are the same (ie. no modification has taken place) any byte sub-set of these two files will have the same hash
Assumption 3: If a byte sub-set of two files is found which does not result in the same hash, the two files are not the same (ie. have been modified)

The code is written in Java and the hashing algorithm used is BLAKE-512 using the java implementation from Marc Greim.
_File1 and _File2 are 2 files > 1.5GB of type java.io.File

public boolean compareStream() throws IOException {
    int i = 0;
    int step = 4096;
    boolean equal = false;

    FileInputStream fi1 = new FileInputStream(_File1);
    FileInputStream fi2 = new FileInputStream(_File2);

    byte[] fi1Content = new byte[step];
    byte[] fi2Content = new byte[step];

    if(_File1.length() == _File2.length()) { //Assumption 1
        while(i*step < _File1.length()) {

            fi1.read(fi1Content, 0, step); //Assumption 2
            fi2.read(fi2Content, 0, step); //Assumption 2

            equal = BLAKE512.isEqual(fi1Content, fi2Content); //Assumption 2

            if(!equal) { //Assumption 3
                break;
            }

            ++i;
        }
    }

    fi1.close();
    fi2.close();
    return equal;
}

The calculation for two equal 1.5 GB files takes around 4.2 seconds. Times are of course much shorter when the files differ, especially when they are of different length since it returns immediately.

Thank you for your suggestions :)

There are some basic issues here, as well as some algorithmic complexities, and then some advanced suggestions.

Basic issues relate to Java code conventions, etc.

Basics

Use try-with-resources. You have code which may fail, and leave open files lying around to be garbage collected. Consider the following code:

try (FileInputStream fi1 = new FileInputStream(_File1);
    FileInputStream fi2 = new FileInputStream(_File2);) {

    // do stuff with the files - they will be auto-closed.

}

The next thing, is why open the files if they are different lengths?

FileInputStream fi1 = new FileInputStream(_File1);
FileInputStream fi2 = new FileInputStream(_File2);

byte[] fi1Content = new byte[step];
byte[] fi2Content = new byte[step];

if(_File1.length() == _File2.length()) { //Assumption 1

The code above, should be:

if(_File1.length() == _File2.length()) { //Assumption 1
    FileInputStream fi1 = new FileInputStream(_File1);
    FileInputStream fi2 = new FileInputStream(_File2);

    byte[] fi1Content = new byte[step];
    byte[] fi2Content = new byte[step];

Use the power of the force, ... I mean parameters, Luke... I mean Daniel.

Your method should take the two files as parameters, not as class-level fields. As it stands, your code is not "reentrant", and it should be. Your method is:

public boolean compareStream() ....

but it should be

public boolean compareStream(File filea, File fileb) ....

Algorithm

Since you are comparing two files byte-by-byte, the hashing will make no difference. If the two files were on different machines, and you have a slow network between them, and if you could run the hashing algorithm remotely, then it probably makes sense to hash the two files on each side, and then just compare the small, and easy to transfer, hash result. Something like SHA-256.

So, there's no need to hash, just do byte-by-byte comparisons.

For large files like yours, why have such a small step size? Use something much larger like 4MB, not 4KB. It will make it much faster.

Alternatives

File IO is always slower than you want. Java has the NIO framework for higher-performance IO using Channels and Buffers. This would be a great time to learn how to use them, because, a 4MB Memory-Mapped IO operation on the two files will likely give you the best performance.

See the MemoryMapped IO JavaDoc

I ran up a test using NIO, and produced the following code:

public static final boolean compareFiles(final Path filea, final Path fileb) throws IOException {
    if (Files.size(filea) != Files.size(fileb)) {
        return false;
    }

    final long size = Files.size(filea);
    final int mapspan = 4 * 1024 * 1024;

    try (FileChannel chana = (FileChannel)Files.newByteChannel(filea);
            FileChannel chanb = (FileChannel)Files.newByteChannel(fileb)) {

        for (long position = 0; position < size; position += mapspan) {
            MappedByteBuffer mba = mapChannel(chana, position, size, mapspan);
            MappedByteBuffer mbb = mapChannel(chanb, position, size, mapspan);

            if (mba.compareTo(mbb) != 0) {
                return false;
            }

        }

    }
    return true;
}

private static MappedByteBuffer mapChannel(FileChannel channel, long position, long size, int mapspan) throws IOException {
    final long end = Math.min(size, position + mapspan);
    final long maplen = (int)(end - position);
    return channel.map(MapMode.READ_ONLY, position, maplen);
}

Note, the guts could be rewritten more concisely too:

            if (!mapChannel(chana, position, size, mapspan)
                   .equals(mapChannel(chanb, position, size, mapspan))) {
                return false;
            }

On my laptop, this is comparing 1.5GB files in under 2 seconds. Obviously, your milage may vary, and my laptop is an unknown beast.... but things that may play in to the equation:

I have 16GB mem
it's a 4 year old laptop
it has an SSD
there is file-system encryption
it runs linux.


Concisely adding values in a loop to a column In my data frame I assign values to the column Register by checking another column (Source) for specific values. Based on those values, the output in Register changes.

I am relatively new to R, but I have some coding knowledge. The following code contains lines that are all so similar that I can't help but wonder if it can't be done way simpler with a condition. In JavaScript, for instance, I'd do this with a case-function. I do not know, however, if something similar is possible in R.

d$Register <- "Banana placeholder"
d$Register[grep(".*/comp-a/.*", d$Source, perl=TRUE)] <- "a: spont-conv"
d$Register[grep(".*/comp-b/.*", d$Source, perl=TRUE)] <- "b: interv-ler-nl"
d$Register[grep(".*/comp-(c|d)/.*", d$Source, perl=TRUE)] <- "c/d: telefoon"
d$Register[grep(".*/comp-e/.*", d$Source, perl=TRUE)] <- "e: onderhandeling"
d$Register[grep(".*/comp-f/.*", d$Source, perl=TRUE)] <- "f: interv-radio-tv"
d$Register[grep(".*/comp-g/.*", d$Source, perl=TRUE)] <- "g: debat"
d$Register[grep(".*/comp-h/.*", d$Source, perl=TRUE)] <- "h: les"
d$Register[grep(".*/comp-i/.*", d$Source, perl=TRUE)] <- "i: spont-comm-radio-tv"
d$Register[grep(".*/comp-j/.*", d$Source, perl=TRUE)] <- "j: reportage-radio-tv"
d$Register[grep(".*/comp-k/.*", d$Source, perl=TRUE)] <- "k: nieuws-radio-tv"
d$Register[grep(".*/comp-l/.*", d$Source, perl=TRUE)] <- "l: comm-radio-tv"
d$Register[grep(".*/comp-m/.*", d$Source, perl=TRUE)] <- "m: misviering"
d$Register[grep(".*/comp-n/.*", d$Source, perl=TRUE)] <- "n: college"
d$Register[grep(".*/comp-o/.*", d$Source, perl=TRUE)] <- "o: voorgelezen"

It would be better to make a list of the parameters,
and then use a loop to perform the assignments.
That will reduce the duplication of logic,
for example in the long grep(...).

params <- list(
  c("/comp-a/", "a: spont-conv"),
  c("/comp-b/", "b: interv-ler-nl"),
  c("/comp-(c|d)/", "c/d: telefoon"),
  c("/comp-e/", "e: onderhandeling"),
  c("/comp-f/", "f: interv-radio-tv"),
  c("/comp-g/", "g: debat"),
  c("/comp-h/", "h: les"),
  c("/comp-i/", "i: spont-comm-radio-tv"),
  c("/comp-j/", "j: reportage-radio-tv"),
  c("/comp-k/", "k: nieuws-radio-tv"),
  c("/comp-l/", "l: comm-radio-tv"),
  c("/comp-m/", "m: misviering"),
  c("/comp-n/", "n: college"),
  c("/comp-o/", "o: voorgelezen")
)

placeholder <- "PLACEHOLDER"
d$Register <- placeholder

for (i in 1:length(params)) {
  pattern.i <- params[[i]][1]
  label.i <- params[[i]][2]
  d$Register[grep(pattern.i, d$Source)] <- label.i
}

if (nrow(d[d$Register == placeholder]) > 0) stop('Register is not assigned in some rows')

Some other minor improvements:

Simplified the regular expressions: pattern will match the same things as .*pattern.*
No need for the perl = T parameter in grep


Idiomatic loop and break condition I am calling a C library via P/Invoke. That library needs to be called in sucession with an increasing unsigned int, and returns a pointer. That pointer is then mapped to a managed type, until it returns a NULL pointer.

In C, the idiomatic way to write it, is probably for(i=0;;i++), but what is the most idiomatic way to write it in C#?

Currently it is using a do {} while loop, as in my opinion, this is the clearest way to show that this loop will repeat until newPort is null.

static IEnumerable<Port> GetPorts ()
{
    List<Port> ports = new List<Port>();
    uint i = 0;
    Port newPort;
    do {
        // This calls an C library, and maps the returned pointer
        // to the Port class
        newPort = GetPortData (i);
        if (newPort != null){
            ports.Add (newPort);
        }
        i++;
    } while(newPort != null);
    return ports;
}

On the other hand, the variables i and newPort are only used inside the loop, so using for would be another solution, but it does not clearly show the breaking condition.

    for (uint i = 0; ; i++) {
        Port newPort = GetPortData (i);
        if (newPort == null) {
            break;
        }
        ports.Add (newPort);
    }

Which version should I use?

I think the for loop can show the "breaking condition" like this:

for (uint i = 0; (newPort = GetPortData(i)) != null; i++) {
    ports.Add (newPort);
}


Reading a text file that may have embedded nulls I've always used fgets to read a file. However, I want to read a file that may have embedded \0. I thought of using ftell to query the size, but it doesn't seem to work on all files.

I've got a test file,

31 32 33 00 34 35 36 0A E2 82 AC 0D 0A 61

Here is my fgets.

#include <stdlib.h> /* EXIT */
#include <stdio.h>  /* printf perror fputc fread */
#include <string.h> /* strlen */
#include <errno.h>  /* errno */
#include <assert.h> /* assert */

int main(void) {
    char file[1000], *f = file, *a;
    const int granularity = 80;
    int is_done = 0;

    for( ; ; ) {
        /* Fail when contents bigger than the size;
         would be a good spot to use realloc. */
        if(granularity > sizeof file - (f - file))
            { errno = ERANGE; break; }
        if(!fgets(f, granularity, stdin))
            { if(ferror(stdin)) break; is_done = 1; break; }
        f += strlen(f);
    }

    for(a = file; a < f; a++) printf("%02hhX ", *a);
    fputc('\n', stdout);
    return is_done ? EXIT_SUCCESS : (perror("stdin"), EXIT_FAILURE);
}

Running this, (I'm on a UNIX-like machine,)

$ bin/fgets < test
31 32 33 E2 82 AC 0D 0A 61

Here is my fread.

#include <stdlib.h> /* EXIT */
#include <stdio.h>  /* printf perror fputc fread */
#include <errno.h>  /* errno */
#include <assert.h> /* assert */

int main(void) {
    char file[1000], *f = file, *a;
    const int granularity = 80;
    size_t read;
    int is_done = 0;

    for( ; ; ) {
        if(granularity > sizeof file - (f - file))
            { errno = ERANGE; break; }
        read = fread(f, 1, granularity, stdin);
        if(ferror(stdin)) break;
        assert(read >= 0 && read <= granularity);
        f += read;
        if(read != granularity) { is_done = 1; break; }
    }

    for(a = file; a < f; a++) printf("%02hhX ", *a);
    fputc('\n', stdout);
    return is_done ? EXIT_SUCCESS : (perror("stdin"), EXIT_FAILURE);
}

Running this,

$ bin/fread < test
31 32 33 00 34 35 36 0A E2 82 AC 0D 0A 61

I would like to know if this is pedantically correct and how I improve.

I find your code hard to read, for the following reasons:

the variable file is not really a file but a buffer
the variable f (which usually stands for file) is a pointer into that buffer
the variable a has a name which does not convey any meaning at all
if the first if statement you indent the brace in the next line, and in the very last if statement the body is in the same line
there are spaces missing in the code at places where I expect them, such as after an if or for
the forever loop is usually written as for (;;), not for( ; ; )
the main block of the code is inside the for loop, and there's not a single empty line in that part. This suggests that the whole block is doing a single thing with no possible interruption or logical break
the comma operator is generally frowned upon
is_done is not really about the work being done, it's more about being successful

Because of all the above reasons, I would write the code differently:

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

int main(void) {
    char buf[1000];
    size_t buflen = 0;
    const size_t granularity = 80;

    while (true) {
        if (granularity > sizeof buf - buflen) {
            errno = ERANGE;
            break;
        }

        size_t nread = fread(buf + buflen, 1, granularity, stdin);
        if (ferror(stdin))
            break;

        buflen += nread;
    }

    for (size_t i = 0; i < buflen; i++)
        printf("%02hhX ", buf[i]);
    fputc('\n', stdout);

    if (ferror(stdin)) {
        perror("stdin");
        return EXIT_FAILURE;
    }
}

And here is what I changed:

I renamed all variables to match their purpose
I replaced the various pointers into the buffer with indexes
I removed the is_done variable since the program should not print an error just because the file is a multiple of 80 bytes
I moved the variable a to a smaller scope by declaring it inside the for loop where it is used; I also renamed it to i, since it is now an index instead of a pointer
I replaced the comma operator with an if statement, since that is the form that is commonly used
I included <stdbool.h> to have a boolean type and the constants true and false
I replaced for (;;) with while (true), which is less magic
I sorted the included headers alphabetically since for headers from the C standard library, the order doesn't matter
I renamed the variable read to nread, to avoid possible conflicts with the POSIX function of the same name


Subclass instance recorder in Java I've written a relatively small abstract class in Java which keeps track of all instances of it's sublasses. Any class which extends this abstract class will automatically have all its instances stored (via a weak reference) in a Set, which can be retrieved if a subclass ever wants to get references to all it's instances.

Although this class is relatively short and simple, I have never made anything like it and so I want to ensure that I'm not doing something terribly wrong or giving in to any bad practices:

import java.util.*;

public abstract class InstanceRecorder {
    private static Map<Class<? extends InstanceRecorder>, Set<InstanceRecorder>> instances;

    static {
        instances = new HashMap<>();
    }

    {
        instances.putIfAbsent(this.getClass(), Collections.newSetFromMap(new WeakHashMap<>()));
        instances.get(this.getClass()).add(this);
    }

    protected static int countInstances(Class<? extends InstanceRecorder> key) {
        if (instances.containsKey(key))
            return instances.get(key).size();
        return 0;
    }

    @SuppressWarnings("unchecked")
    protected static <T extends InstanceRecorder> Set<T> getInstances(Class<T> key) {
        if (instances.containsKey(key))
            return Collections.unmodifiableSet((Set<T>) instances.get(key));
        return new HashSet<>();
    }
}

As I said above, I've never made a class like this before and so any feedback or insight would be greatly appreciated.

Your code is not thread-safe. You should use ConcurrentHashMap instead of HashMap, if this code is ever run in a multithreaded environment.

In Java 8 there was a terrible performance bug in ConcurrentHashMap.computeIfAbsent, which would lock the whole map even if the key already exists. That bug has been fixed in Java 9. See also this question.


Update only modified fields in Entity Framework I'm working on a website on ASP.NET MVC4 and EF5. I want to ensure that only modified values are updated in the database.  I'm using a unit of work pattern and repositories for data work. Here's the code for a generic repository. I've two questions, and need a code review for the following code.

Is the code to update only modified fields good? It works, but I don't know if this is the best way. I especially don't like that I have to do this loop through the properties myself (can't EF do this?). I also don't like the cast to object and the comparison.
How are my Add and Update methods? I'm planning the clients (MVC/API controllers) to use a service (passing in a unit of work) to get the work done. They will not directly use the repository. The repository will not have a Save() method, only the UnitOfWork will have one. I can ensure within the service that I will use the same context to update/add entities. Given this, are there any scenarios where my Add/Update code is going to fail?

public class EFRepository<T> : IRepository<T> where T : class
{
    protected DbContext DbContext { get; set; }
    protected DbSet<T> DbSet { get; set; }

    public EFRepository(DbContext dbContext)
    {
        if (dbContext == null)
            throw new ArgumentNullException("dbContext");
        DbContext = dbContext;
        DbSet = DbContext.Set<T>();
    }

    public virtual void Add(T entity)
    {
        DbContext.Entry(entity).State = EntityState.Added;
    }

    public virtual void Update(T entity)
    {
        //dbEntityEntry.State = EntityState.Modified; --- I cannot do this.

        //Ensure only modified fields are updated.
        var dbEntityEntry = DbContext.Entry(entity);
        foreach (var property in dbEntityEntry.OriginalValues.PropertyNames)
        {
            var original = dbEntityEntry.OriginalValues.GetValue<object>(property);
            var current = dbEntityEntry.CurrentValues.GetValue<object>(property);
            if (original != null && !original.Equals(current))
                dbEntityEntry.Property(property).IsModified = true;
        }
    }

    //... Other methods ...
}

Why bother checking if the value/property has been changed? You are just adding additional complexity for no reason. Sure, it might be slightly! faster to only update modified values, but in return you are adding an overhead to keep track of the modified values, which will diminish whatever performance boost you got.

Just invoke Update on the entire object... All values that has not been changed will remain the same and the values that has been modified will, well, update.

If it's speed you're looking for you are most likely looking at the wrong place to squeeze ms from.


Print all permutations of a given string in Java I want to print all permutations of a given string in Java. To do this I create one auxiliary array boolean used[] to check if I have used some character or not.

private static void permute(String s) {
        int n = s.length();
        boolean[] used = new boolean[n];
        char[] in = s.toCharArray();
        StringBuffer output = new StringBuffer();
        doPermute(in, output, used, n, 0);
    }

//helper method
private static void doPermute(char[] in, StringBuffer output,
            boolean[] used, int n, int level) {
        // TODO Auto-generated method stub
        if(n == level){
            System.out.println(output.toString());
            return;
        }
        for (int i = 0; i < in.length; i++) {
            if(used[i])
                continue;
            output.append(in[i]);
            used[i] = true;
            doPermute(in, output, used, n, level+1);
            used[i] = false;
            output.setLength(output.length()-1);
        }
    }

//main() method
public static void main(String[] args) {
        String s = "dog";
        permute(s);
    }

My main question: is there more elegant/better solution? Can I avoid using this auxiliary array (I tried to but it didn't work)?

I'd bet you're right with used being somehow strange. The standard algorithm I recall doesn't need it. As I tried to improve your code, I ended up with the standard algorithm, so it's pointless to paste it here (someone else will surely do this better).

So some unsolicited general advice instead(*):

When you start with a main, call a static method, which uses another static method, then you're programming purely procedurally. I always recommend to stop it ASAP, e.g., by something like

public static void main(String[] args) {
    new MyClass().go(args);
}

Then you need no more static and can start putting some fields into your first object.

The other thing is printing instead of doing some reusable work.

(*) I consider this more important the good algorithmization as it took me long to grasp.


Filtering a List based on a Suffix and avoid duplicates I was wondering if there is a better way to solve this issue.
I have a list like this:
imp_list=["aa","daa","ab","as","aem",
      "aum","aw","aa","acm","at",
      "ar","aa_imp","daa_imp","ab_imp",
      "as_imp"]

I want to select all the strings that have the _imp suffix, plus all the the strings that have no _imp partner.
That's because aa_imp it's just a modified version of aa, and for me, in a large sense, it's a replica.
So I created this function:
def imputed_handler(my_list):
  imp=[x for x in my_list if "imp" in x]

  cleaned_imp=set(map(lambda x: x.replace("_imp",""),imp))

  not_imp=[x for x in my_list if "imp" not in x]
  set_list=set(my_list)
  no_replica=list(set_list-cleaned_imp)
  print(no_replica)
  return no_replica

Running the code as described above
test=imputed_handler(imp_list)

I get the following output:
['at', 'acm', 'aw', 'ar', 'daa_imp', 'aem', 'as_imp', 'ab_imp', 'aa_imp', 'aum']

Do better solutions exist? Thanks for your time, and let me know if something is not clear :)

First off, since you're looking for strings that end with _imp, x.endswith("_imp") is probably more reliable than "imp" in x.
Second, since we only want to remove _imp when it's a suffix, and since we know how long it is, we can use string slicing to remove the last 4 characters. That means we don't accidentally turn x_impabc_imp into xabc instead of x_impabc or something. Assuming that's not what we want to do. Maybe it is, maybe it isn't, maybe we'll never get an input like that so it won't matter either way, I have no idea.
I'm also thinking it might be nice to pass the suffix as a parameter. Maybe it might change at some point in the future. Maybe another part of the program uses a different suffix in a similar way. Doesn't hurt to be prepared.
In general, though, your approach is good. You find the _imp ones, you figure out what they are replacing, you remove the ones that have been replaced.
The neat thing is, you can do the first two steps all at once in a single pass through the input list. For example, you can do this:
def imputed_handler(my_list, suffix="_imp"):
    my_set = set(my_list)
    cleaned_imp = set(item[:-len(suffix)] for item in my_set if item.endswith(suffix))

    return list(my_set - cleaned_imp)

Though a loop might be clearer at that point:
def imputed_handler(my_list, suffix="_imp"):
    my_set = set(my_list)

    for item in my_list:
        if item.endswith(suffix):
            my_set.discard(item[:-len(suffix)])

    return list(my_set)


Function to count rows in a database query result I have written this function that surprisingly works. (I didn't really know what I was doing; just a little experiment.) It is supposed to count rows selected by a PDO query.

Is it well written? It works, but I am just asking, as it is my first more-complex function.

function countRows($sql, $parameters){
            require("include/db.php");

            $result = $db->prepare($sql);
            foreach ($parameters as $key => $parameter) {
                    $result->bindParam($key, $parameter);
            }

            $result->execute();
            $number_of_rows = $result->fetchColumn();

            return $number_of_rows;

            }

require

Incorporating code by file inclusion is a bad idea — especially code that has side-effects.  At the very least, db.php seems to have the side-effect of defining $db.  It makes me suspicious of what other things happen in there.

Recommendations:

move the require() call outside countRows()
change it to require_once()
make the side-effects explicit

require_once('include/db.php');

function countRows($sql, $parameters) {
    $db = connect();    # connect() is defined in db.php
    …
}

Even that is problematic, though: you shouldn't open a new connection for each query.  It's probably better to have the caller pass a database connection as a parameter.

function countRows($db, $sql, $parameters) {
    …
}

Purpose

This function is probably a bad idea to begin with.  The database sends back a lot of data just so that your PHP code can retrieve the row count.

If you just want to know how many rows that satisfy some criterion exist, then you should run a SELECT count(…) FROM … query, and the database will tell you just the count.

On the other hand, if you are also interested in the data itself, then you should just note the size of the result set when you run a "real" query.


Simple Blackjack game in Python 2.6.9 Like the title says, I made a very simple game of Blackjack using Python version 2.6.9. I am relatively new to python and programming so I would really appreciate some constructive criticism to help me improve.

from random import shuffle #this imports shuffle which shuffles the     deck

deck = list('234567890JQKA'*4)
for shu in range(0,3):
    shuffle(deck)
value = {'2':2, '3':3, '4':4, '5':5, '6':6, '7':7, '8':8,
         '9':9, '0':10, 'J':10, 'Q':10, 'K':10, 'A':1}      # Creates the shuffled deck

player = [deck.pop() for _ in range(2)]
AI = [deck.pop() for _ in range(2)]     # Deals the cards to the players

stay = False

def player_game():
    global stay
    print "Your hand: %s" % player
    hit = raw_input("Hit or Stay? ")
    if hit == "Hit":
        player.append(deck.pop())
    elif hit == "Stay":
        stay = True
        return stay
    else:
        print "I'm sorry. you need to input 'Hit or Stay'."

while 1:
    player_game()
    total = sum(map(value.get, player, AI))
    if total > 21:
        print "Hand: %s" % player
        print "You busted with a score of: %s" % total
        break
    elif total == 21:
        print player
        print "You win!"
        break
    if stay == True:
        print "You stayed."
        print "Final score: %s" % total
        break

It is common practice to shorten statements like

if stay == True:

down to

if stay:

This'll make your code easier to read (more English-like).


Parallel foreach with configurable level of concurrency The purpose of this code is to let me loop over 100 items (up to MAX_CONCURRENT at a time), performing some action on them, and then return only once all items have been processed:

/// <summary>Generic method to perform an action or set of actions
/// in parallel on each item in a collection of items, returning
/// only when all actions have been completed.</summary>
/// <typeparam name="T">The element type</typeparam>
/// <param name="elements">A collection of elements, each of which to
/// perform the action on.</param>
/// <param name="action">The action to perform on each element. The
/// action should of course be thread safe.</param>
/// <param name="MaxConcurrent">The maximum number of concurrent actions.</param>
public static void PerformActionsInParallel<T>(IEnumerable<T> elements, Action<T> action)
{
    // Semaphore limiting the number of parallel requests
    Semaphore limit = new Semaphore(MAX_CONCURRENT, MAX_CONCURRENT);
    // Count of the number of remaining threads to be completed
    int remaining = 0;
    // Signal to notify the main thread when a worker is done
    AutoResetEvent onComplete = new AutoResetEvent(false);

    foreach (T element in elements)
    {
        Interlocked.Increment(ref remaining);
        limit.WaitOne();
        new Thread(() =>
        {
            try
            {
                action(element);
            }
            catch (Exception ex)
            {
                Console.WriteLine("Error performing concurrent action: " + ex);
            }
            finally
            {
                Interlocked.Decrement(ref remaining);
                limit.Release();
                onComplete.Set();
            }
        }).Start();
    }
    // Wait for all requests to complete
    while (remaining > 0)
        onComplete.WaitOne(10); // Slightly better than Thread.Sleep(10)
}

I include a timeout on the WaitOne() before checking remaining again to protect against the rare case where the last outstanding thread decrements 'remaining' and then signals completion between the main thread checking 'remaining' and waiting for the next completion signal, which would otherwise result in the main thread missing the last signal and locking forever. This is faster than just using Thread.Sleep(10) because it has a chance to return immediately after the last thread completes.

Goals:

Ensure thread safety - I want to be sure I won't accidentally return too early (before all elements have been acted on), and be sure that I don't become deadlocked or otherwise stuck.
Add as little overhead as possible - minimizing amount of time that fewer than MAX_CONCURRENT threads are executing action, and returning as soon as possible after the final action has been performed.

I've made use of a CountdownEvent signal to avoid the use of the remaining integer and avoid the busy waiting involved in polling it with the unreliable AutoResetEvent onComplete:

public static void PerformActionsInParallel<T>(IEnumerable<T> elements, Action<T> action)
{
    int threads = MaxConcurrent ?? DefaultMaxConcurrentRequests;
    // Ensure elements is only enumerated once.
    elements = elements as T[] ?? elements.ToArray();
    // Semaphore limiting the number of parallel requests
    Semaphore limit = new Semaphore(MAX_CONCURRENT, MAX_CONCURRENT);
    // Count of the number of remaining threads to be completed
    CountdownEvent remaining = new CountdownEvent(elements.Count());

    foreach (T element in elements)
    {
        limit.WaitOne();
        new Thread(() =>
        {
            try
            {
                action(element);
            }
            catch (Exception ex)
            {
                Console.WriteLine("Error performing concurrent action: " + ex);
            }
            finally
            {
                remaining.Signal();
                limit.Release();
            }
        }).Start();
    }
    // Wait for all requests to complete
    remaining.Wait();
}


Does my image upload look safe enough? I have an image upload script that uploads images to the server:

<?php
define("UPLOAD_DIR", "uploads/");

$fileType = exif_imagetype($_FILES["myFile"]["tmp_name"]);
$allowed = array(IMAGETYPE_GIF, IMAGETYPE_JPEG, IMAGETYPE_PNG);
if (!in_array($fileType, $allowed)) {
    header('Location: edit.php');
    exit();
}

if (!empty($_FILES["myFile"])) {
    $myFile = $_FILES["myFile"];

    if ($myFile["error"] !== UPLOAD_ERR_OK) {
        echo "
An error occurred.
";
        exit;
    }

    // ensure a safe filename
    $name = preg_replace("/[^A-Z0-9._-]/i", "_", $myFile["name"]);

    // don't overwrite an existing file
    $i = 0;
    $parts = pathinfo($name);
    while (file_exists(UPLOAD_DIR . $name)) {
        $i++;
        $name = $parts["filename"] . "-" . $i . "." . $parts["extension"];
    }

    // preserve file from temporary directory
    $success = move_uploaded_file($myFile["tmp_name"],
        UPLOAD_DIR . $name);
    if (!$success) {
        echo "
Unable to save file.
";
        exit;
    }

    // set proper permissions on the new file
    chmod(UPLOAD_DIR . $name, 0644);
}

Is it safe? Are there any good tips to protect myself against attacks via the image upload?

Here's my markup, if that counts:

<form action="upload.php" method="post" enctype="multipart/form-data">
<input type="file" name="myFile" id="file" capture="camera" accept="image/*">

<input type="submit" name="submit" value="Submit">
</form>

The Security Aspect
Good to see that you're not just checking for a file extension in the name!!
Basically, yes, it's the minimal security needed. There aren't too many steps needed to make an image uploading script safe, however, doing it wrong could present some future issues.
Here's some notes though:

When renaming the file, what's the point in changing the name to something similar but with characters replaced? You could (not recommended) keep the original name, or you could completely alter the name to something unpredictable, and then use a look-up table (i.e. database) to point the user in the correct path. Instead of simply swapping out illegal characters with underscores, give the file a name that has no resemblance.

The permissions on your directory do look okay. It is however, not a bad idea to simply remove the images from the public directories, place them in say var/www/uploads/ and let a script get them for you. You don't want these images being in a directory where PHP (or any other server scripts) may run.

Simply appending $parts["extension"] onto the file seems risky to me. You did check the image type, however I'm not sure how it responds to files with multiple extensions. Someone with more knowledge here could extend this...

Non-Security Related

exit(); and exit; are used throughout. Please don't! Two things: be consistent, they're aliases (along with die); don't use them, they leave the end-user hangin'! These kill the page with no clean way to display an error.

Using names such as myFile are quite book/tutorial copy-and-pasted. Choose names that have meaning to your application.

 echo "
An error occurred.
"; Lot's of things I could say here. 1) An error!? No kidding! You kill the page right after this without even saying what the user should do to correct the error. (Doesn't matter if you're the only person using this)

$name, $parts, $success - more unhelpful variable names!

Other than that, it's a simple piece of code, and there's already tons of people who have done this, plus more. It's usually safer to work with a community run library than to create your own!


Regex
The time regular expression can be simplified to
regexp_time = re.compile(r'\d{10}\.\d{6}')

which is a bit more readable. The second one does not acount for letters in hexadecimal numbers so a line starting with 0x00a0: won't be parsed… Since you are using a file that is extremely regularly formatted, I suggest you drop this regular expression and rely on the fact that, if regexp_time does not match, then you have a line of data. (At least on my tcpdump output, I only get a single line.) But if you want to use a second regular expression to filter out lines that may be between the timestamp and the first line of data, I'd suggest simply using
regexp_hex = re.compile(r'\t0[xX][\da-fA-F]+:')

and not worry about the rest of the line.
Matches
When using the match method on a compiled regular expression, you receive a match object or None. This match object let you easily extract out the matched bits using its group method. So there is little use of splitting a line to parse stuff:
timestamp = regexp_time.match(line):
if timestamp:
    output.write("%s\n" % timestamp.group())

In fact, you can so easily extract out the groups that I would use a second regex for the hexadecimal data anyway:
regexp_hex = re.compile(r'\t0x([\da-fA-F]+):  ([\da-fA-F ]+?)\s{2,}(.*)')

So that, the first group is the hexadecimal address, the second one is the hexadecimal payload and the third one is the ASCII payload. You then just need to format the output:
payload = regexp_hex.match(line)
if payload:
    address = payload.group(1)
    hex_data = payload.group(2).replace(' ', '')
    ascii_data = payload.group(3)
    output.write('\t{0>6}:  {:<47}  {}\n'.format(address, hex_data, ascii_data))

The only thing left is to split the hex_data in groups of 2 digits. This can be done using the itertools recipe grouper.
Proposed improvements
import re
import itertools

TIMESTAMP = re.compile(r'\d{10}\.\d{6}')
PAYLOAD = re.compile(r'\t0x([\da-fA-F]+):  ([\da-fA-F ]+?)\s{2,}(.*)')

def grouper(iterable, count, fillvalue=None):
    args = [iter(iterable)] * count
    return itertools.zip_longest(*args, fillvalue=fillvalue)

def convert_tcpdump_to_text2pcap(in_filename, out_filename):
    with open(in_filename) as input, open(out_filename, 'w') as output:
        for line in input:
            time = TIMESTAMP.match(line)
            if time:
                output.write('{}\n'.format(time.group()))
                continue
            payload = PAYLOAD.match(line)
            if payload:
                address = payload.group(1)
                hex_data = payload.group(2).replace(' ', '')
                ascii_data = payload.group(3)

                hex_data = ' '.join(''.join(part) for part in grouper(hex_data, 2, ' '))
                output.write('\t{:0>6}:  {:<47}  {}\n'.format(address, hex_data, ascii_data))

if __name__ == '__main__':
    convert_tcpdump_to_text2pcap('../Traffic/traffic1.txt', '../Traffic/txt2.txt')


Finding the Nth prime I'm going through some of the problems over on exercism and we were asked to write a program that finds the nth prime. I know Ruby has a Prime class that handles that but the exercise doesn't allow us to use that.

My code works fine but it's very, VERY slow when testing very large numbers (e.g.: the 10,001st prime number). I'm not entirely sure of this is because of inefficient math or bad code.

  def nth(nth_prime)
    raise ArgumentError, 'Invalid Input' if nth_prime == 0

    list_of_primes = []
    results = []

    (1..2000).each do |i|
      (2..(i-1)).each do |j|
        results << i%j
      end
      list_of_primes << i unless results.include? 0
      results.clear
    end

    list_of_primes[nth_prime]
  end

Note: I've limited it to listing the primes from 2 to 2000 for now because that's fast enough for testing purposes. I've also deliberately included 1 in  list_of_primes because the tests (written and provided by exercism.io) test that asking for the 1st prime will give you the number 2 so I needed 2 to be at list_of_primes[1].

Using @RubberDuck's reference to the Sieve of Eratosthenes, I came up with the following code for listing prime numbers:

def list_primes up_to
  primes = (2..up_to).to_a
  primes.each {|num| primes.delete_if {|i| i > num && (i % num) == 0} }
  primes
end

The code is fairly simple. We build an array of all the numbers up to up_to and start deleting from that list every number that is devisable by any of it's predecessors (not a prime).

This promises less iteration, since we don't have to review numbers more than once and all the predecessors we check against are prime numbers, so we don't have to check against numbers that are irrelevant (i.e. checking against 6 when we already checked agains 2 and against 3).

Benchmarking this code in comparison to your code showed that listing the prime numbers up to 10,000 took 0.09 seconds using this application for the Sieve of Eratosthenes vs. 3.65 seconds using your code... That's about 40 times faster(!)*.

I would say, although my code might not be the best way to write the Sieve of Eratosthenes in Ruby, that @RubberDuck's advice is pure gold.

Reading about it in Wikipedia, it seems that n * log(n) would offer a number close to the prime number... so this will help up determine up to what number we should search....

Here's my version of the code, which assumes the first prime number to be 2:

def my_nth_prime n
  up_to = n * (Math.log(n) + 2)
  primes = (2..up_to).to_a
  primes.each {|num| primes.delete_if {|i| i > num && (i % num) == 0} }
  primes[n-1]
end

up_to will designate the upper limit for the prime numbers (prime numbers up to that number will be calculated). It is calculated using the rule of n * log(n) suggested by Wikipedia...

...BUT, that rule might brake down, as it's only an approximation. So, just to be on the safe side, we add another 2 ns to the up_to, making sure we don't get caught by a lower approximation than the actual nth prime.

Once these two things are done, we build an array of all the primes up to up_to and select the n-1 prime (arrays are 0 based, meaning the first item is at [0] so the nth prime is actually [n-1]).

To build the array we use the same code as before, collecting all the numbers up to up_to and deleting any number that is devisable by any of it's predecessors (leaving the predecessors to be only prime numbers, this saves us a lot of testing and iterations).

Good Luck!

although the Ruby code I provided might be faster, it's extremely slow compared to the Prime core class... Getting the 10,000th prime using my code took my computer 7.52 seconds, vs. 0.002925 seconds using the native module(!).


Simple networking layer in Swift using Strategy design pattern Does the following code demonstrate proper use of Strategy design pattern for a simple networking layer in swift 3?

Some code smells I'm unsure about:

violates Single responsibiility principle. Each strategy class such as Find, has a method for a different type of implementation. This is because I could want to find an image, or a user, or a chatroom. which are stored at different nodes in Firebase. all these different find methods are clumped together in Find class.
At the call sight of a request, if I need to make multiple async request, I nest the next request call inside the closure of the call back. Is this Ok?
The request object allows access to every type of insert, and find method. so in my signup VC I could i have the option to download a chatroom. Is even having access to that kind of implementation bad?

I have posted the code below, and left out all the actual implementation for brevity.

Any tips or guidance is much appreciated!

// USE CASE: Would go in viewDidLoad of ViewController
func testMyRequest () {

    let myRequest = Request(insert: Insert(), find: Find())

    myRequest.find?.user(with: "id", handler: { (user) in

        myRequest.find?.nearbyUsers(user: user, handler: { (users) in

            // update collectionView datasource
        })
    })
}

// Is this protocol necessary?
protocol RequestProtocol {
    // - Family of algorithms, related actions.
    var insert: Insert? { get set }
    var find: Find? { get set }
}

// ---------------------------

class Request: RequestProtocol {

    var insert: Insert?
    var find: Find?

    init(insert: Insert?, find: Find?) {
        self.insert = insert
        self.find = find
    }
}

// Use a singleton maybe for the classes below? Why wouldn't I?

class Insert {

    init() { }

    func user(_ user: User) {
        // insert user to firebase implementation
    }

    func message(_ message: Message) -> Void  {
        // insert message to firebase impelmentation
    }

    func image(data: Data, user: User)  {
        // insert image to firebase impelmentation
    }
}

class Find {

    init() { }

    func user(with id: String, handler: @escaping (_ user: User) -> Void ) {
        // find user implementation
    }

    func allChatrooms(handler: @escaping ([Chatroom]) -> Void) {
        // find all chatrooms implementation
    }

    func nearbyUsers(user: User, handler: @escaping ([User]) -> Void ) {
        // find nearby Users relative to current User location implementation
    }

    // Private helper method
    private func findChatPartners (currentUser: User, chatrooms: [Chatroom] ) -> Set<String> {
    }

}

What you have is not a Strategy pattern... The key features of the Strategy pattern is that once the Strategy object is inserted into the Context, it is not referenced from outside the Context. Your view controller is still rooting around inside the Context object for the strategy object.

The simplest way to solve this is to have the view controllers be the context. Then you would have a protocol:

protocol API {
    func insert(user: User)
    func insert(message: Message)
    func insert(imageData: Data, toUserID: String)
    func findUser(withID: String, completion: @escaping (User) -> Void)
    // etc...
}

Then you would have a FirebaseAPI that would implement this protocol and a TestAPI that would implement it. The TestAPI would just return canned data so you can test all the possible permutations.


Processing delimiter characters in C? Write a C program that will implement the basic operations of table processors.
The input will be text data read from a .txt file, operations will be defined using terminal arguments, and the output will be in a .txt file as well.
Program should be run the following way:
./main [-d delimiter] [name of the function for the table] <in.txt >out.txt

Where the -d argument determines which symbols can be interpreted as separators of single cells, by default the delimiter is a space character.
Multiple cases of the same sign in the delimiter are ignored.
The first sign in the delimiter character will be used as the separator of output values.
name of the function is the identifier of the function that will be called to perform certain tasks on the table. <in.txt redirects reading from stdin to reading from in.txt, >out.txt redirects outputting to stdout to outputting to out.txt.
Here's what I wrote:
#include <stdio.h>
#include <string.h>

int main(int argc, char* argv[])
{
    if((argc > 2) && (strcmp(argv[1], "-d") == 0)) {
        char delim = *argv[2];

        for (int i; (i = getchar()) != EOF; ) {
            if(i == '\n')
                putchar(i);

            if(!((i >= '0' && i <= '9') || (i >= 'a' && i <= 'z') || (i >= 'A' && i <= 'Z')) ){
                putchar(delim);
                continue;
            }
            putchar(i);
        }

    }

    else if((argc == 2) && strcmp(argv[1], "-d") == 0) {
        char delim = ' ';

        for (int i; (i = getchar()) != EOF; ) {
            if(i == '\n')
                putchar(i);

            if(!((i >= '0' && i <= '9') || (i >= 'a' && i <= 'z') || (i >= 'A' && i <= 'Z')) ){
                putchar(delim);
                continue;
            }
            putchar(i);
        }

    }

    return 0;
}

The code works and does what it's supposed to, but I am not sure about the effectivity of the implementation. The requirements are: input table can't be an empty file, maximum length of a row (both input & output) is 10KiB, otherwise an error message should be shown. Using global variables isn't allowed, preprocessor macro #define is. Functions for working with files and dynamic memory allocation aren't allowed as well.
What are the ways to change my code?

It would be helpful for you to break up the code into functions, that will simplify each task.
Process the command line arguments first, then once you have the delimiter process the text files.

Declare the variable delim above any logic and initialize it to the default value
Process the command line to get the actual delimiter.
Process the input file.

This should shorten the code because you only need one loop to process the input file rather than 2.


Grid layout component for VueJS I'm working on a vue.js / nuxt.js project, applying the Atomic Design methodology, I need to do one to set the grid layout and using CSS Grid Layout.

I already did the component, but I think I did not do it the best way ... the code is great and not very smart, lots of manual work, I would like your opinion and help to make this component better

GridLayout.vue

<template>

    <slot />

</template>

using component on any page.vue

<template>
  <GridLayout
    columns="1fr 1fr 1fr 1fr"
    rows="auto"
    gap="10px"
    :medium="{
      columns: '1fr 1fr',
      rows:'auto auto'
    }"
    :small="{
      columns: '1fr',
      rows: 'auto auto auto auto',
    }"
  >

1

2

3

3

  </GridLayout>
</template>

problems

  1 - the style tag

Never heard of Atomic design till today.Read the blog and a few chapters of the book. Nothing really new, just another analogy (re-branding) for the standard modular design methods.

To your code.

Its not at all DRY but the various settings are consistent and can easily be changed so that you minimize the repeated code and literals.

The following example reduces the repeated code but without knowing more about how these properties change per client/instance I cant add add anything regarding the design methods.

css() {
    const rules = {
        columns: "grid-template-columns: ##SET##;",
        rows: "grid-template-rows: ##SET##;",
        areas: "grid-template-areas: \"##SET##\";",
        gap: "grid-gap: ##SET##;",
        columnGap: "grid-column-gap: ##SET##;",
        rowGap: "grid-row-gap: ##SET##;",
    };
    const sizes = {
        default:" .grid { ##STYLE## }",
        small: "@media (max-width: 600px) { .grid { ##STYLE## } } ",
        medium: "@media (min-width: 600px) and (max-width: 992px) { .grid { ##STYLE## } } ",
    };
    var src, css = "";
    const rule = name => src[name] ? rules[name].replace("##SET##", src[name]) : "";
    for (const size of Object.keys(sizes)) {
        src = size === "default" : this : this[size];
        if (src) {
            css += sizes[size].replace("##STYLE##", Object.keys(rules).map(rule).join(""));
        }
    }
    return css;
}


ArrayBlockingQueue - blocking drainTo() method This is a follow-up question to this question.

Is this a reasonable implementation for a blocking version of method ArrayBlockingQueue.drainTo(Collection)?

/**
 * This blocking method blends {@link #take()} with {@link #drainTo(Collection)}.
 * <br>
 * Chiefly, {@code take()} blocks and {@code drainTo(Collection)} does not block.
 */
public int drainAtLeastOneTo(Collection<? super E> c)
throws InterruptedException {
    final ReentrantLock lock = this.lock;
    lock.lockInterruptibly();
    try {
        while (count == 0) {
            notEmpty.await();
        }
        final int x = drainTo(c);
        return x;
    }
    finally {
        lock.unlock();
    }
}

To clarify: This code comes from the Sun JDK source code of ArrayBlockingQueue.  I simply blended the source from take() with drainTo(Collection).

I am considering adding it to a modified version of ArrayBlockingQueue.

Your problem is easily solved without having to delve in to the internals of ArrayBlockingQueue, and without needing a new ReEntrantLock either. SImply use the methods that are currently available, and combine them....

public int drainAtLeastOneTo(Collection<? super E> c)
        throws InterruptedException {
    c.add(take());
    return 1 + drainTo(c);
}


Django Model field naming convention for lookup tables I'm currently in the process of refactoring a Django app to use Django REST Framework. Within the existing code there are 15 models for lookup tables. The structure of each model is as follows (using gender as an example - there are valid reasons in the context of the application for including a gender model rather than as hardcoded CHOICES on a Person model):
class Gender(models.Model):
    """
    This model stores genders
    """
    name = models.CharField(max_length=64)
    sort = models.IntegerField()

    class Meta:
        ordering = ['sort']

    def __unicode__(self):
        return self.name

(I've already made some changes that are included in the above code to bring it in line with Django's coding style guidelines)
All of the 15 models use the same field names that I am concerned should be more explicit.
My concern with using the name field-name is that it does not explicitly state what data is in the relevant table.
To add to this, I might have the following code in my views:
class PersonByGenderList(generics.ListAPIView):
    """
    This endpoint lists all people by gender
    """
    serializer_class = PersonSerializer

    def get_queryset(self):
        """
        :return: queryset of people restricted by gender.
        """
        queryset = Person.objects.all()
        gender = self.request.QUERY_PARAMS.get('gender', None)
        if gender is not None:
            queryset = queryset.filter(gender__name=gender)
        return queryset

Again, gender__name (for me) is not explicit enough. Also, in a SlugRelatedField the slug name is always slug_name='name'.
So my questions are:

Are there any database design conventions that recommend using name as a generic field name on models, or should I be more explicit (following the Zen of Python) and prefer (albeit with some repetition):
queryset = queryset.filter(gender__gender=gender)
Or is it preferable to use name and be explicit with the model name in the case of gender?

Is the arbitrary integer field used for sorting explicit enough when effectively it will sort by first entry to last? I would think it is generally better to sort by something that is relevant to the use case, but as with question 1 are there any conventions/style guides I should be aware of when naming fields?

Some info on my research:
I've looked for other questions on this site and Stack Overflow, Django's coding style guidelines, PEP8 but not found anything that directly answers this question. A question on Programmer's SE comes closest Should model/table names be as specific as possible. The points I take from the answers are:

naming should be as specific as possible
Table and field names should be descriptive of the domain entity that they capture

You could have an abstract base model to lock in how the lookup tables all should work:

class LookupModel(models.Model):
    name = models.CharField(max_length=64)
    sort = models.IntegerField()

    class Meta:
        abstract = True
        ordering = ['sort']

    def __unicode__(self):
        return self.name

.. ditto for a DRF serializer, you just swap out which model is being connected. When you do this you cannot go the "gender__gender=gender" route though. It's a matter of how lazy you are, really (as in laziness, impatience, hubris). One advantage is that you can have a single class in the admin if you use that:

class LookupAdmin(admin.ModelAdmin):
    list_display = ('name', sort')
    ...

admin.site.register(Gender, LookupAdmin)
admin.site.register(Location, LookupAdmin)
admin.site.register(Status, LookupAdmin)

As for naming conventions, if sort is chosen/changeable and an integer I would call it position. If a string, sortkey or something like that. If automatic: Django, when sorting on foreign key with order_with_respect_to, adds the invisible column "_order", so you could do that, and find a way to set the column automatically. Or, you could sort on the automatically added auto-incrementing primary keyid-field (which I admit I've never done). I don't like implicitness though so would have a manually added column and manually set "ordering". Fewer surprises that way, laziness again.

But, and here's why I write this in 2020, long after you are hopefully done with the project you needed input on: Django 3.0 can use enums in Field.choices. So, in your case, with just a single name-field and a sort you won't change, you could forego the lookup table and instead:

LifeStatus =  models.IntegerChoices(
    'LifeStatus',
    'ALIVE MISSING DEAD UNDEAD'
)

class Human(models.Model):
    status = models.IntegerField(choices=LifeStatus.choices)
    ..

You can then do things like:

# zombieapocalypse.py

if human_looking.status == LifeStatus.UNDEAD:
    run_away()

Which IMO is very readable. Typos would also be a syntax error, bonus!

(Unfortunately this doesn't fix my problem though, I'd like a lookup table that also works like an enum...)


First Linked List in Java I just started learning Java, and wrote up a linked list. I'm coming from just having learned Haskell, so I've been out of the mindset of stateful computations for awhile.

I'd like to know how it could be improved in any way. Please be critical. Mainly though, I'd like suggestions on readability, and safe practices.

It's very simple right now, only allowing additions at the end and deletions; but I figured it would be sufficient for a review.

class Node<T> {
    T cargo;
    Node<T> tail;

    public Node(T nCargo) {
        cargo = nCargo;
        tail = null;
    }

    public Node<T> getNext() {
        return tail;
    }

    public T getCargo() {
        return cargo;
    }

    public boolean isLast() {
        return tail == null;
    }

    public Node<T> getLast() {
        Node<T> curNode = this;
        while (!curNode.isLast()) {
            curNode = curNode.getNext();
        }
        return curNode;
    }

    public void add(T nCargo) {
        tail = new Node<T>(nCargo);
    }

    public boolean canLink() {
        return this.getNext().getNext() != null;
    }

    public void deleteNext() {
        if (this.canLink()) { tail = tail.getNext();
            } else { tail = null; }
    }

}

public class Vect<T> {
    Node<T> head;
    int vSize;

    public Vect() {
        head = null;
        vSize = 0;
    }
    public Vect(T cargo) {
        head = new Node<T>(cargo);
        vSize = 1;
    }

    public void pushBack(T cargo) {
        Node<T> last = head.getLast();
        vSize += 1;
        last.add(cargo);
    }

    private Node<T> getNodeAt(int i) {
        Node<T> curNode = head;
        for (int cI = 0; i > cI; cI++) {
            curNode = curNode.getNext();
        }
        return curNode;
    }

    public T getAt(int i) {
        return getNodeAt(i).getCargo();
    }

    public int length() {
        return vSize;
    }

    public void deleteHead() {
        if (head.getNext() != null) { head = head.getNext();
            } else { head = null; }
        vSize -= 1;
    }

    public void deleteAt(int i) {
        if (i == 0) {
            deleteHead();
        } else {
            Node<T> curNode = this.getNodeAt(i - 1);
            curNode.deleteNext();
            vSize -= 1;
        }
    }
}

and its use:

public class Main {

    public static void main(String[] args) {
        Vect <Integer> v = new Vect<Integer>(2);
        v.pushBack(3);
        v.pushBack(4);
        v.pushBack(5);
        v.pushBack(6);

        for (int i = 0; i < v.length(); ++i) {
            System.out.println(v.getAt(i));
        }

        v.deleteAt(2);

        for(int i = 0; i < v.length(); ++i) {
            System.out.println(v.getAt(i));
        }

    }
}

Disclaimer: This review is reviewing this as work-in-process, and not as "finished".

On a quick glance this looks mostly correct. I still got a few points, that definitely need to be fixed before proceeding:

Index handling:
You don't do anything with negative indexes. vector.getAt(-1); results in an infinite loop, as well as vector.deleteAt(-1);. I suggest you just check and throw an InvalidArgumentException for any \$index < 0\$
Empty list:
Your empty list throws null-pointer exceptions all around. Don't do that. You should check your head before doing stuff with it, and if it's null, throw IllegalStateException  or similar.
Optimizing pushBack():
There is a relatively simple and quick-to-implement optimization for your pushBack. Your Vect<T> should maintain a "pointer" to the last node. On deleting the last node you need to check for the special-case just like when deleting head, but after that your pushBack becomes dead-simple:

 public void pushBack(T cargo) {
     tail.add(cargo);
     tail = tail.getNext();
     vSize++;
 }


Conversion of hexadecimal string to string Given a C# string which is a set of hexadecimal numbers such as:

string HexString = "202048656c6c6f20576f726c64313233212020";

Where those hexadecimal numbers represent the ASCII text:

"  Hello World123!  "

I need to convert the HexString to a String and I have the following code which works, but looks so excessive!

string HexStringToString(string HexString) {
    string stringValue = "";
    for (int i = 0; i < HexString.Length / 2; i++) {
        string hexChar = HexString.Substring(i * 2, 2);
        int hexValue = Convert.ToInt32(hexChar, 16);
        stringValue += Char.ConvertFromUtf32(hexValue);
    }
    return stringValue;
}

Am I missing some elegant method?

I don't think there is a built-in method. Yours is pretty good but we could make some improvements:

Parameters should be camelCase => hexString.
You should favour StringBuilder when building up strings.
You should step through the string in increments of 2 to cut down on the maths.
You should validate the argument.
You should prefer var when the type is obvious.

Result of those points:

string HexStringToString(string hexString)
{
    if (hexString == null || (hexString.Length & 1) == 1)
    {
        throw new ArgumentException();
    }
    var sb = new StringBuilder();
    for (var i = 0; i < hexString.Length; i += 2) {
        var hexChar = hexString.Substring(i, 2);
        sb.Append((char)Convert.ToByte(hexChar, 16));
    }
    return sb.ToString();
}


Sherlock and The Beast I have recently written the program for the Sherlock and The Beast' HackerRank challenge. That's working fine, but the problem is that it takes too much time if a big number is given as a input.  I would like you to help me out in optimizing my code so that it could run under 16 seconds, which is needed.

from collections import Counter
def isDecent(n):
    digits = list(map(int, str(n)))
    if not set(str(n)).issubset('35'): return False
    threes = Counter(digits)[3]
    fives = Counter(digits)[5]
    if threes % 5 == 0 and fives % 3 == 0: return True
    else: return False

inps = []
decents = []
t = int(input())
for i in range(t): inps.append(int(input()))
for inp in inps:
    if inp == 1:
        decents.append(-1)
        continue
    n=2
    while True:
        if(isDecent(n) and len(str(n)) == inp): break
        n+=1
    if n != 2: decents.append(n)
    else: decents.append(-1)
for decent in decents: print(decent)

Is there any thing that could be used to optimize it?

Your algorithm is way off.... ;-)

Let's consider the solution to a decent number.

For any decent number, the more 5's we put at the front, the better.

So, let's break it down to some maths....:

d => number of digits in the decent number
f => number of fives in the decent number
t => number of threes in the decent number

also

d = f + t
f % 3 == 0
t % 5 == 0

We have:

d = f + t

Algorithm:

// tmp number of five values is the number of digits
ftmp = d
// decrease the number of fives (by the number of threes in a batch)
// until both the rules f % 3 == 0 and t % 5 == 0 are satisfied
while ftmp % 3 != 0 : ftmp -= 5

check the ftmp is a valid value
if ftmp % 3 != 0 : return -1;

f = ftmp;
t = d - f

return "5" x f + "3" x t

Writing it in Java, I have the following:

private static String sherlock(final int target) {
    int threes = 0;
    int fives = 0;
    int digits = target;
    while (digits > 0) {
        if (digits % 3 == 0) {
            fives = digits;
            break;
        }
        digits -= 5;
    }
    threes = target - digits;
    if (digits < 0 || threes % 5 != 0) {
        return "-1";
    }
    StringBuilder sb = new StringBuilder(target);
    while (fives-- > 0) {
        sb.append("5");
    }
    while (threes-- > 0) {
        sb.append("3");
    }
    return sb.toString();
}

For me, on my laptop, this solves the 100000 digit problem in less than 1 millisecond. First I 'warm up' Java with the first 10,000 solutions....

Then I run some big ones....

public static void main(String[] args) {
    int cnt = 0;
    long ms = System.currentTimeMillis();
    for (int i = 0; i < 10000; i++) {
        cnt += sherlock(i).length();
    }
    ms = System.currentTimeMillis() - ms;
    System.out.println("Warmup created " + cnt + " characters in " + ms + " Milliseconds");
    for (int i : new int[] { 1, 3, 5, 11, 19, 100000 }) {
        long nanos = System.nanoTime();
        String val = sherlock(i);
        nanos = System.nanoTime() - nanos;
        System.out.printf("    request digits %d : actual digits %d Value %s in (%.3fms)%n",
                i, val.length(), val.length() > 20 ? "too long" : val, nanos / 1000000.0);
    }
}

This produces the output:

Warmup created 49995011 characters in 703 Milliseconds
request digits 1 : actual digits 5 Value 33333 in (0.004ms)
request digits 3 : actual digits 3 Value 555 in (0.012ms)
request digits 5 : actual digits 5 Value 33333 in (0.003ms)
request digits 11 : actual digits 11 Value 55555533333 in (0.002ms)
request digits 19 : actual digits 19 Value 5555555553333333333 in (0.002ms)
request digits 100000 : actual digits 100000 Value too long in (0.622ms)


Encrypt and decrypt a serializable object I want to make sure that the code I have for encrypting and decrypting a serializable object makes sense and is proper. Does it look right too?

Here's what I have so far:

public static void encrypt(Serializable object, String path) throws IOException, NoSuchAlgorithmException,
            NoSuchPaddingException, InvalidKeyException {

          try {

            // Length is 16 byte
            SecretKeySpec sks = new SecretKeySpec("MyDifficultPassw".getBytes(),
                    "AES/ECB/PKCS5Padding");

            // Create cipher
            Cipher cipher = Cipher.getInstance("AES/ECB/PKCS5Padding");
            cipher.init(Cipher.ENCRYPT_MODE, sks);
            SealedObject sealedObject = new SealedObject(object, cipher);
            // Wrap the output stream
            CipherOutputStream cos = new CipherOutputStream(new BufferedOutputStream(new FileOutputStream(path)), cipher);
            ObjectOutputStream outputStream = new ObjectOutputStream(cos);
            outputStream.writeObject(sealedObject);
            outputStream.close();
        }
        catch(IllegalBlockSizeException e){

            e.printStackTrace();

        }
        }

public static void decrypt(Serializable object, String path) throws IOException, NoSuchAlgorithmException,
            NoSuchPaddingException, InvalidKeyException {

        SecretKeySpec sks = new SecretKeySpec("MyDifficultPassw".getBytes(),
                "AES");

        Cipher cipher = Cipher.getInstance("AES");
        cipher.init(Cipher.DECRYPT_MODE, sks);

        CipherInputStream cipherInputStream = new CipherInputStream( new BufferedInputStream( new FileInputStream(path) ), cipher );
        ObjectInputStream inputStream = new ObjectInputStream( cipherInputStream );
        SealedObject sealedObject = null;
        try {
            sealedObject = (SealedObject) inputStream.readObject();
            TransferData td = (TransferData) sealedObject.getObject( cipher );

        }
          catch (ClassNotFoundException e) {
            e.printStackTrace();
        } catch (IllegalBlockSizeException e) {
            e.printStackTrace();
        } catch (BadPaddingException e) {
            e.printStackTrace();
        }

    }

Some obvious problems that jump into the eye:

Don't use duplicate string literals, like "MyDifficultPassw" and "AES/ECB/PKCS5Padding". Put them into constants and define them near the top.
Don't e.printStackTrace(). It's considered bad practice.
The encrypt and decrypt methods violate the single responsibility principle, because they encrypt / decrypt and at the same time do file I/O. Instead of writing to / reading from a filesystem path, it would be better to work with streams. That would open make them testable too (see below).
The decrypt method takes a Serializable object that's not used at all. Also, the initialization SealedObject sealedObject = null; is pointless, as the variable is always assigned before use anyway.

It would be slightly better this way:

private static final byte[] key = "MyDifficultPassw".getBytes();
private static final String transformation = "AES/ECB/PKCS5Padding";

public static void encrypt(Serializable object, OutputStream ostream) throws IOException, NoSuchAlgorithmException, NoSuchPaddingException, InvalidKeyException {
    try {
        // Length is 16 byte
        SecretKeySpec sks = new SecretKeySpec(key, transformation);

        // Create cipher
        Cipher cipher = Cipher.getInstance(transformation);
        cipher.init(Cipher.ENCRYPT_MODE, sks);
        SealedObject sealedObject = new SealedObject(object, cipher);

        // Wrap the output stream
        CipherOutputStream cos = new CipherOutputStream(ostream, cipher);
        ObjectOutputStream outputStream = new ObjectOutputStream(cos);
        outputStream.writeObject(sealedObject);
        outputStream.close();
    } catch (IllegalBlockSizeException e) {
        e.printStackTrace();
    }
}

public static Object decrypt(InputStream istream) throws IOException, NoSuchAlgorithmException, NoSuchPaddingException, InvalidKeyException {
    SecretKeySpec sks = new SecretKeySpec(key, transformation);
    Cipher cipher = Cipher.getInstance(transformation);
    cipher.init(Cipher.DECRYPT_MODE, sks);

    CipherInputStream cipherInputStream = new CipherInputStream(istream, cipher);
    ObjectInputStream inputStream = new ObjectInputStream(cipherInputStream);
    SealedObject sealedObject;
    try {
        sealedObject = (SealedObject) inputStream.readObject();
        return sealedObject.getObject(cipher);
    } catch (ClassNotFoundException | IllegalBlockSizeException | BadPaddingException e) {
        e.printStackTrace();
        return null;
    }
}

This is only slightly better, it still looks quite messy. But it has the big advantage that now this is testable, for example:

@Test
public void testEncryptDecryptString() throws InvalidKeyException, NoSuchAlgorithmException, NoSuchPaddingException, IOException {
    String orig = "hello";
    ByteArrayOutputStream baos = new ByteArrayOutputStream();
    encrypt(orig, baos);
    ByteArrayInputStream bais = new ByteArrayInputStream(baos.toByteArray());
    assertEquals(orig, decrypt(bais));
}

@Test
public void testEncryptDecryptPerson() throws InvalidKeyException, NoSuchAlgorithmException, NoSuchPaddingException, IOException {
    Person orig = new Person("Jack", 21);
    ByteArrayOutputStream baos = new ByteArrayOutputStream();
    encrypt(orig, baos);
    ByteArrayInputStream bais = new ByteArrayInputStream(baos.toByteArray());
    assertEquals(orig, decrypt(bais));
}

static class Person implements Serializable {
    private static final long serialVersionUID = 0;
    private final String name;
    private final int age;

    Person(String name, int age) {
        this.name = name;
        this.age = age;
    }

    @Override
    public boolean equals(Object o) {
        if (this == o) {
            return true;
        }
        if (o == null || getClass() != o.getClass()) {
            return false;
        }

        Person person = (Person) o;

        if (age != person.age) {
            return false;
        }
        if (!name.equals(person.name)) {
            return false;
        }

        return true;
    }
    @Override
    public int hashCode() {
        int result = name.hashCode();
        result = 31 * result + age;
        return result;
    }
}


Avoid Split to reduce memory press (on servers) I have a Component that is used for all Active Directory queries.

The function below is used heavily perhaps even milions of times each day.

Performance is a feature!

It's used on servers and I want to reduce memory press on these servers (when the GC is collecting the server websites etc are not responding... well of course ther are perhaps even bigger sinners in that case - but I write this in VB.NET and because of that the code has to be better than ordinary).

I can trust the format so I don't have to check.
dn = distinguishedName
Data: "CN=joedoe0101,OU=User,OU=Cardiff Electric,OU=Company,DC=root,DC=usa,DC=net"
Result: "Cardiff Electric"

Below I have two examples that not quite does what I want:

Friend Shared Function GetOrganization(dn As String) As String
    Return dn.Split(","c)(2).Substring(3)
End Function

Or this

Friend Shared Function GetOrganization(dn As String) As String
    Return dn.Substring(dn.IndexOf(","c) + 1).Split(","c)(1).Substring(3)
End Function

Is there a better solution?

Is there a solution to also get the "Company" ?

You basically have 3 other possibilities

Using Split() like you did, but different

Private Const CommaOUEqual As String = ",OU="
Friend Shared Function GetOrganization(dn As String) As String
    Return dn.Split({CommaOUEqual}, StringSplitOptions.None)(2)
End Function

Using IndexOf() similiar like you did, nut without Split()

Private Const CommaOUEqual As String = ",OU="
Friend Shared Function GetOrganization(dn As String) As String
    Dim index As Integer = dn.IndexOf(CommaOUEqual) + 4
    index = dn.IndexOf(CommaOUEqual, index) + 4
    Return dn.Substring(index, dn.IndexOf(CommaOUEqual, index) - index)
End Function

Using Regex

Private Shared regex As Regex = New Regex(",OU=(.+?),OU=(.+?),OU=", RegexOptions.Compiled)
Friend Shared Function GetOrganization(dn As String) As String
    Return regex.Match(dn).Groups(2).ToString()
End Function

Timing these 3 methods together with the 2 methods fromn you results after 500.000 iteration in

  yours

  ~340 ms (Split())
  ~330 ms (IndexOf() + Split())

  mine

  ~460 ms (Split())
  ~250 ms (IndexOf())
  ~900 ms (Regex)

So in the worst case for assuming 5.000.000 calls in 24 hours this will result for the worst case in 9 seconds and for the best case in 2.5 seconds.

But this are still only micro gain in performance.

But wait, we can do faster

Friend Shared Function GetOrganization(dn As String) As String
    Dim firstHit As Boolean = False
    Dim secondHit As Boolean = False
    Dim startIndex As Integer = 0
    For i As Integer = 6 To dn.Length - 3
        If dn.Chars(i) = "="c AndAlso dn.Chars(i - 1) = "U"c AndAlso dn.Chars(i - 2) = "O"c Then
            If Not secondHit AndAlso firstHit Then
                startIndex = i + 1
                secondHit = True
                Continue For
            End If
            If secondHit Then
                Return New String(dn.ToCharArray(startIndex, i - startIndex - 3))
            End If
            firstHit = True
        End If
    Next
    Return String.Empty
End Function

This will run for 500.000 iterations in

  ~70 ms

Using

Return dn.Substring(startIndex, i - startIndex - 3)

instead of

Return New String(dn.ToCharArray(startIndex, i - startIndex - 3))

will improve the speed to about

  ~60 ms

if you need to parse more than one item of the given string we should use this to its own class and remove the Shared.

Friend Class ParsedLDAP
    Friend Enum LDAPComponent
        OU = 1
        DC = 2
        CN = 4
        ALL = 7
    End Enum
    Public Property CommonNames As IList(Of String)
    Public Property OrganisationalUnits As IList(Of String)
    Public Property DomainComponents As IList(Of String)
    Private Sub New()
        CommonNames = New List(Of String)
        OrganisationalUnits = New List(Of String)
        DomainComponents = New List(Of String)
    End Sub
    Public Sub New(dn As String)
        Me.new(dn, LDAPComponent.ALL)
    End Sub
    Public Sub New(dn As String, parseComponent As LDAPComponent)
        Me.New()
        ParseString(dn, parseComponent)
    End Sub

    Private Const reversedOU As String = "UO"
    Private Const reversedDC As String = "CD"
    Private Const reversedCN As String = "NC"
    Private Const equalSign As Char = "="c
    Private Sub ParseString(dn As String, parseComponent As LDAPComponent)
        Dim isNotOU As Boolean = ((parseComponent And LDAPComponent.OU) <> LDAPComponent.OU)
        Dim isNotDC As Boolean = ((parseComponent And LDAPComponent.DC) <> LDAPComponent.DC)
        Dim isNotCN As Boolean = ((parseComponent And LDAPComponent.CN) <> LDAPComponent.CN)

        If (isNotOU AndAlso isNotDC AndAlso isNotCN) Then Return

        Dim currentComponent As LDAPComponent
        For i As Integer = 2 To dn.Length - 1
            If dn.Chars(i) <> equalSign Then Continue For

            Dim startIndex As Integer = i + 1
            Select Case dn.Chars(i - 1)
                Case reversedOU.Chars(0)
                    If isNotOU Then Continue For
                    If dn.Chars(i - 2) <> reversedOU.Chars(1) Then Continue For
                    currentComponent = LDAPComponent.OU
                Case reversedDC.Chars(0)
                    If isNotDC Then Continue For
                    If dn.Chars(i - 2) <> reversedDC.Chars(1) Then Continue For
                    currentComponent = LDAPComponent.DC
                Case reversedCN.Chars(0)
                    If isNotCN Then Continue For
                    If dn.Chars(i - 2) <> reversedCN.Chars(1) Then Continue For
                    currentComponent = LDAPComponent.CN
                Case Else
                    Continue For
            End Select

            While (i < dn.Length - 1 AndAlso dn.Chars(i + 1) <> equalSign)
                i += 1
            End While

            If i = dn.Length - 1 Then i += 3 ' This is needed, because otherwise the last item can't be found

            Dim currentValue As String = dn.Substring(startIndex, i - startIndex - 2)
            Select Case currentComponent
                Case LDAPComponent.OU
                    OrganisationalUnits.Add(currentValue)
                Case LDAPComponent.DC
                    DomainComponents.Add(currentValue)
                Case LDAPComponent.CN
                    CommonNames.Add(currentValue)
            End Select

        Next
    End Sub
End Class

Because I assume that you maybe have millions of calls where you need e.g the desired OU but for a much smaller number of distinct dn's I would suggest to store the results in a Dictionary(Of String, ParsedLDAP) to first lookup if the dn has been parsed by calling Dictionary(Of TKey,TValue).TryGetValue().


HackerRank - Birthday Cake Candles and the counting loop I was having fun on solving one of the HackerRank simple challenges
- Birthday Cake Candles.

Description

  Colleen is turning n years old! Therefore, she has n candles of various heights on her cake, and candle i has heighti. Because the taller candles tower over the shorter ones, Colleen can only blow out the tallest candles.

Given the heighti for each individual candle, find and print the number of candles she can successfully blow out.

Input Format

The first line contains a single integer, , denoting the number of candles on the cake.
The second line contains  space-separated integers, where each integer  describes the height of candle .

Output Format

Print the number of candles Colleen blows out on a new line.

Sample Input 0

4
3 2 1 3

Sample Output 0

2

Explanation 0

We have one candle of height 1, one candle of height 2, and two candles of height 3. Colleen only blows out the tallest candles, meaning the candles where height=3. Because there are 2 such candles, we print 2 on a new line.

My solution to the exercise above. You can find full code in gist with full code

function birthdayCakeCandles(n, ar) {
  var counts = {};

  ar.forEach(function(x) {
      counts[x] = (counts[x] || 0) + 1;
  });

  var toAr = Object.keys(counts).map(function(key){ return counts[key]; });
  var result = Math.max.apply(Math, toAr);

  return result;
}

n - number of candles
ar - array with heights of the candles

I would like to get a feedback on my part of the code, as I guess I might do a lot of unnecessary steps.

The algorithm is to count the number of elements in the array that are equal to the max of the array.  Let the code express that.  Both the original code and the procedural accepted answer are a clutter of bookkeeping details.

function birthdayCakeCandles(n, ar) {
    const max = Math.max(...ar)
    return ar.filter(x => x === max).length
}