Type-agnostic BubbleSort in C I'm trying out different sorting algorithms for learning purposes, here I'm doing a "type-agnostic" bubbleaort.

I modeled the function signature after the standard library's qsort. I know the bubblesort algorithm in general is not that fast, but I'm looking for a review in particular about the memory management.

In my implementation I'm accepting a void pointer to an array, the size of the array, the size of the single elements and the pointer to the comparison function. Then I convert the void pointers to char pointers, and I do manually the pointer arithmetics (I avoided nonstandard arithmetics on void *).

The part I'm not very happy about is the need for a buffer (dynamically allocated), and the use of memcpy to swap the memory of the two elements. I'd love to know if that part can be improved even slightly.

I'm using a pointer to the beginning and the end of the array, instead of using indexes with [] bracket notation.

Also, what about the if (buffer == NULL) check and "halting" with exit()? Is there a more elegant way to fail here?

I also could use one variable instead of two (current and left), but doing one more pointer arithmetic in the inner loop.

Here's my current bubblesort implementation:

/**
 * Bubblesort algorithm. Compatible with any data type.
 * @param base A void pointer to the array's first element.
 * @param num The number of elements in the array.
 * @param size The size of the data type.
 * @param compar A pointer to the comparison function.
 */
void bubblesort(void* base, size_t num, size_t size,
    int (*compar)(const void*, const void*)
) {
    const char *arr = (const char *) base;
    char *buffer, *left, *current, *right;

    if (arr != NULL && size > 0 && num > 1) {
        buffer = (char *) malloc(size);

        if (buffer == NULL) {
            fprintf(stderr, "Out of memory.");

            exit(EXIT_FAILURE);
        }

        right = (char *) arr + num * size;
        while (right > arr) {
            left = (char *) arr;
            current = left + size;

            while (current < right) {
                // Compare the two elements and move the bigger to the right
                if (compar(left, current) > 0) {
                    memcpy(buffer, left, size); // Temporary variable
                    memcpy(left, current, size); // Swap pt.1
                    memcpy(current, buffer, size); // Swap pt.2
                }

                left += size;
                current += size;
            }

            right -= size;
        }

        free(buffer);
    }
}

Error handling

Some would argue that it's unacceptable to exit() in case of an "out of memory" error occurring in a library function. I would invite them to tell me how to actually recover in those situations, because I doubt many "well-designed" applications do so.

That said, many libraries indeed have the courtesy of telling the caller that they can't complete their task, rather than killing the process. For a language like C (which lacks support for sum types and exceptions), I'm currently undecided between a few approaches, but I'd always make sure that the caller can distinguish between success and failure by a simple inspection of the return value.

The easiest approach would be to change the return value of bubblesort to a library-specific enum of error codes, which includes exactly one 'success' value (in C libraries, the common value would be 0) and at least one 'generic failure' value. If you want your users to be able to distinguish between failure reasons, just add more reason to the enum.

Once you have done this, you can also explicitly signal for other error conditions (arr == 0, size == 0), if you consider them errors.

Note: for some pet projects, I used to just return a bool, where true would mean success. Since I realised that this runs counter to the UNIX tradition of "zero means success", I no longer use that in public APIs.

Swapping

By introducing a memswap function

void memswap(void *restrict left, void *restrict right, size_t length);

we can move a lot of logic out of bubblesort itself. This also means we would move the malloc and free to it, which seems like a bad idea for a function in the inner loop.

But don't worry: you can implement memswap in any way you like. For instance, memswap could swap the data between aaaa and bbbb byte-by-byte:

void memswap(
    void *restrict left,
    void *restrict right,
    size_t length
) {
    char *left2 = (char *)left;
    char *right2 = (char *)right;

    for (size_t cursor = 0; cursor < length; cursor++) {
        char tmp = left2[cursor];
        right2[cursor] = left2[cursor];
        left2[cursor] = tmp;
    }
}

Now that you have no more malloc, you won't have to deal with the possibility of it returning a null pointer.

Of course, a robust memswap would not ever cause any UB (which could imply exit()), so you should add some checks for null pointers here. Whether that's an error condition, or that it causes memswap to do nothing is up to you as a library designer. The same goes for the check that left and right don't overlap.

Other functional remarks

You forgot a trailing newline in your error message.
Checking for the cases num == 0 or num == 1 is an attempt at optimisation (right?). I wouldn't do this, unless a benchmark shows that clients of the library are getting significant poor performance because of it (but then it might be a better idea to figure out why these calls are made to begin with).
In the line

right = (char *) arr + num * size;

you should definitely protect yourself against a potential integer overflow in num * size.

Stylistic remarks

Don't cast malloc's return value.
I assume that this isn't all code, but I want to clarify that the structure of files matters. My specific remark here would be that I always make a forward declaration to the functions I define.
Positive conditions, even when negated, are easier to understand than negative ones. For instance, the condition in

if (arr != NULL && size > 0 && num > 1) {

takes me more time to understand than

if (!(arr == NULL || size == 0 || num <= 1)) {

In terms of code real estate, vertical size is cheaper than horizontal size. With that, I mean that I think a failure check like

if (!(arr == NULL || size == 0 || num <= 1)) {

is more readable (and more VCS-friendly ;)) when it's split into multiple blocks, or that the clauses in the condition at least get their own line. Yes, your code grows in length, but I'd prefer boring-looking code that's obvious over "elegant" short code that takes effort to get through (and I say that as a Haskell user).
As of C99, 0 is the (canonical) null pointer constant (meaning that when you convert a constant integer expression with value 0 to a pointer, you get a null pointer; the actual "address" of that null pointer is implementation-defined, though). NULL is still valid, but I wouldn't use it.
const char *arr is confusing, as you know for a fact that you will modify arr. Just use char *arr, avoiding pointer casts later on.
The comments around the swap are not particularily helpful. I prefer many functions and named constants over comments and magic.
You could turn either loop into a for loop, but I recommend to keep this as a while here. The for-notation only helps readability when it concerns a single iterator, and has no performance benefit over while.
Thank you for using EXIT_SUCCESS and EXIT_FAILURE :)

Putting it together (without memswap itself)

enum your_library_status_code {
    your_library_success = 0,
    your_library_failure = 1,
};

/**
 * Bubblesort algorithm. Compatible with any data type.
 * @param base A void pointer to the array's first element.
 * @param num The number of elements in the array.
 * @param size The size of the data type.
 * @param compar A pointer to the comparison function.
 */
enum your_library_status_code bubblesort(
    void* base,
    size_t num,
    size_t size,
    int (*compar)(const void*, const void*)
) {
    if (base == 0) {
        // TODO: introduce error code for "invalid argument"
        return your_library_failure;
    }

    if (size == 0) {
        // TODO: introduce error code for "invalid argument"
        return your_library_failure;
    }

    size_t total_size = num * size;

    if (total_size / size != num) {
        // TODO: introduce error code for "invalid argument (due to overflow)"
        return your_library_failure;
    }

    char *arr = base;
    char *left, *current, *right;

    right = (char *) arr + total_size;

    while (right > arr) {
        left = (char *) arr;
        current = left + size;

        while (current < right) {
            if (compar(left, current) > 0) {
                memswap(left, current, size);
            }

            left += size;
            current += size;
        }

        right -= size;
    }

    return your_library_success;
}

(Caveat: not tested.)


355 - The Bases Are Loaded (math numbers conversion) Recently I found this task, which is to convert numbers between arbitrary bases.

My code is working fine, but time limit exceeded.

#include <cstring>
#include <cstdio>

int broj[50], n;
char uzorak[] = {'0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z'};

long long ToDec(char a[], long long base)
{
    long long dec = 0, b = 1;
    int l = strlen(a);
    for(int i = l-1; i >= 0; i--)
    {
        if(a[i] >= '0' && a[i] <= '9')
            dec += (a[i]-'0') * b;
        else
            dec += (a[i] - 'A' + 10) * b;
        b *= base;
    }

    return dec;
}

void ToB2(long long a, int b2)
{
    n = 0;
    while(a > 0)
    {
        broj[n] = a%b2;
        a = (a-broj[n])/b2;
        n++;
    }
    for(int i = n-1; i>=0; i--)
        printf("%c", uzorak[broj[i]]);
}

int main()
{
    char a[50];
    int b1, b2, l;
    while(scanf("%i %i %s", &b1, &b2, &a))
    {
        int base = 1;
        l = strlen(a);
        for(int i = 0; i < l; ++i)
        {
            if(a[i] >= '0' && a[i] <= '9')
                    base = (base > a[i]-'0') ? base : a[i]-'0';
            else if(a[i] >= 'A' && a[i] <= 'Z')
                    base = (base > a[i]-'A'+10) ? base : a[i]-'A'+10;
            else
            {
                base = -1;
                goto Break;
            }
        }
        base++;
        Break:
        if((base <= b1)&&(base != -1))
        {
            printf("%s base %i = ", a, b1);
            ToB2(ToDec(a, b1), b2);
            printf(" base %i\n", b2);
        }
        else
            printf("%s is an illegal base %i number\n", a, b1);
    }
    return 0;
}

I would appreciate any suggestions on optimizing my code, thanks!

Is there any faster methods to convert numbers from one base to another or you have to go over base 10?

No, you don't need to convert to base 10. Base 10 is in fact quite slow to operate with, as it is in no way a "native" base for computers, which store integers in binary format.

Let's do the math. Assume you need to convert four digit number from base a to base b. Let x_i denote the digit of the number in base a and y_i the digit in base b:

In base a the number is:

x1*a^3 + x2*a^2 + x3*a^1 + x4*a^0 = ((x1*a + x2)*a + x3)*a + x4

In base b it becomes:

y1*b^3 + y2*b^2 + y3*b^1 + y4*b^0 = ((y1*b + y2)*b + y3)*b + y4

The right side of these equations suggests an algorithm idea: to convert a number to binary format from a string s in base a, start with zero, and in each step multiply the number so-far with a and add the next digit. This requires only one multiplication per loop, not two, like in your approach (although that's not likely the reason why your code is too slow.)

int len = strlen(s);
int i;
unsigned long long number = 0;

for (i = 0; i < len; ++i) {
        unsigned digit;
        if (s[i] >= '0' && s[i] <= '9') {
                digit = s[i] - '0';
        } else {
                digit = s[i] - 'A' + 10;
        }
        number = number * a + digit;
}

You had the rest of it already: adding for completeness. For conversion to base b, in each step divide by b.

const char digits[] = {'0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z'};

char result[50];
/* Note the end condition here: stop when the number becomes zero */
for(i = 0; number != 0; ++i) {
        unsigned digit = number % b;
        number /= b;
        result[i] = digits[digit];
}

Now result contains the digits backwards. To print them in order:

for (; i >= 0; --i) {
        putchar(result[i]);
}

Other minor performance notes

Don't call strlen() twice - there's no need to do it more than once anyway, but if there was, you could cache the result and pass as a parameter.
The code in main that determines whether the input string is acceptable is too complex: no need to do the conversion here, just check if each digit is in the acceptable range.

Unrelated notes about readability:

instead of this:

base = (base > a[i]-'0') ? base : a[i]-'0';

suggest using this:

base = max(base, a[i] - '0');

Avoid variable names like l - easy to confuse with 1.
Use const before read-only variabes (such as the array of digits).


Extending functionality of org.springframework.batch.item.file.transform.DefaultFieldSet I would like to be able to set token values (defaultFieldSet.tokens) and names (defaultFieldSet.names) on org.springframework.batch.item.file.transform.DefaultFieldSet using a java.util.Properties object. Specifically, the keys of the Properties object will serve as the names and the corresponding Properties values will serve as the tokens. Here's the code that I have to do this:

import java.util.Properties;
import java.util.Set;

import org.springframework.batch.item.file.transform.DefaultFieldSet;
import org.springframework.batch.item.file.transform.FieldSet;

/**
 * PropertiesFieldSetFactory is a factory to create
 * a {@link FieldSet} from a {@link Properties} object.
*/
public class PropertiesFieldSetFactory {

    /**
     * Creates a {@link FieldSet} by setting its token values equal to the {@link Properties} values
     * and its token names equal to the {@link Properties} keys.
     * Note: Passing a null argument to this method will cause a {@link NullPointerException}
     * to be thrown.
     *
     * @param properties used to populate the {@link FieldSet}
     * @return {@link FieldSet} that has token values and names from the passed in {@link Properties} object
     */
    public static FieldSet create(Properties properties) {
        final Set<String> tokenNamesSet = properties.stringPropertyNames();
        final int numberOfTokens = tokenNamesSet.size();
        final String[] tokenNames = tokenNamesSet.toArray(new String[numberOfTokens]);
        final String[] tokenValues = new String[numberOfTokens];
        for (int tokenPosition = 0; tokenPosition < numberOfTokens; tokenPosition++) {
            String tokenName = tokenNames[tokenPosition];
            tokenValues[tokenPosition] = properties.getProperty(tokenName);
        }
        return new DefaultFieldSet(tokenValues, tokenNames);
    }

}

My concern with this approach is in regards to unit testing. For example, I have the following method (in another class) that needs to be unit tested:

public class CarFieldSetMapper extends BeanWrapperFieldSetMapper<Car> {

    @Override
    public Car mapFieldSet(FieldSet fieldSet) throws BindException {
        Properties fieldProperties = fieldSet.getProperties();

        fieldProperties.put("modelDescription", fieldProperties.get("model"));

        removeDummyIndicator(fieldSet, fieldProperties);

        // build field from properties derived / transformed from the original field set
        FieldSet domainObjectFieldSet = PropertiesFieldSetFactory.create(fieldProperties);

        return super.mapFieldSet(domainObjectFieldSet);
    }

    private void removeDummyIndicator(FieldSet fieldSet, Properties fieldProperties) {
        // dummyIndicator is not a field of any domain object
        final String dummyIndicatorTokenName = "dummyIndicator";
        final String isDummyIndicator = fieldSet.readString(dummyIndicatorTokenName);
        if (isDummyIndicator.equalsIgnoreCase("Y")) {
            fieldProperties.put("model", "");
        }
        fieldProperties.remove(dummyIndicatorTokenName);
    }
}

I do not know of a great way to prevent PropertiesFieldSetFactory.create() from being called when running the unit test ala http://misko.hevery.com/2008/12/15/static-methods-are-death-to-testability/. Also, I'm really trying to extend the DefaultFieldSet functionality so I question using a factory. Note: On the other hand, Joshua Bloch seems to promote use of static factory methods in Effective Java (Chapter 2: Item 1).

Should I change PropertiesFieldSetFactory to PropertiesFieldSet extending DefaultFieldSet? If so, how can I set the FieldSet tokens and names. Both fields are private on the DefaultFieldSet without any way to set the values other than during construction.

Edit: Added code for removeDummyIndicator and correction of line fieldProperties.put("model", fieldProperties.get("model")); to fieldProperties.put("modelDescription", fieldProperties.get("model"));

The real issue here is that you do not need a PropertiesFieldSetFactory. Take a look at how you currently use it:

@Override
public Car mapFieldSet(FieldSet fieldSet) throws BindException {
    Properties fieldProperties = fieldSet.getProperties();

    fieldProperties.put("modelDescription", fieldProperties.get("model"));

    removeDummyIndicator(fieldSet, fieldProperties);

    // build field from properties derived / transformed from the original field set
    FieldSet domainObjectFieldSet = PropertiesFieldSetFactory.create(fieldProperties);

    return super.mapFieldSet(domainObjectFieldSet);
}

This code is taking as input the FieldSet that was tokenized by the LineTokenizer you are using. It adds car-specific values to the Properties of that FieldSet, removes a value in case a dummy parameter is set, and reconstructs a FieldSet back from those properties. Those hoops are needed because FieldSet is immutable.

But look at this again: you don't need to fiddle with the FieldSet, only to have everything set-up automagically by the BeanWrapperFieldSetMapper. Store the result of super.mapFieldSet (invoked with the fieldSet given to the mapper) to a local car and do the necessary logic on that car:

public class CarFieldSetMapper extends BeanWrapperFieldSetMapper<Car> {

    @Override
    public void afterPropertiesSet() throws Exception {
        setStrict(false); // <-- ignore the non-existant "dummyIndicator"
        setTargetType(Car.class);
        super.afterPropertiesSet();
    }

    @Override
    public Car mapFieldSet(FieldSet fieldSet) throws BindException {
        Car car = super.mapFieldSet(fieldSet);
        car.setModelDescription(car.getModel());
        if (fieldSet.readString("dummyIndicator").equalsIgnoreCase("Y")) {
            car.setModel(null);
        }
        return car;
    }

}

The Spring Batch approach to reading a file is:

Take a LineMapper which is supposed to read a line and map it into your domain objects.
This mapper tokenizes the line into a FieldSet with a LineTokenizer and maps this FieldSet into your domain object with a FieldSetMapper. As you can see, it is the tokenizer role to make a FieldSet out of the line, which is then used by the mapper to create the final domain object.
By default, the line tokenizer creates FieldSet instances using a FieldSetFactory. It gets the names you configured and the values come from the parsed line.

So once the FieldSet has been constructed by the LineTokenizer, it should not be re-created: it contains the data as specified in the line that was read. This data is then transformed into your domain object; hence the only place to implement domain-specific logic is in the mapper, directly with the instance to be returned, and not in the FieldSet.


Video frame to Minecraft map colors I'm still somewhat new to C, so pardon me for any silly mistakes I've made.

I'm trying to convert video frames into Minecraft map colors using JNI and FFmpeg's av (and swscale) libraries. The code works below but it's sluggish and I'm going to assume it's prone to the branch prediction problem.

The Minecraft map data is stored in ctx->data; it's a 16384 byte array that's fetched from a direct ByteBuffer via GetDirectBufferAddress.

convertRow is called from a thread pool (taken from here); the thread pool has 4 threads to use.

Current specs of the computer:

  8GB of RAM

  i5 650 @ 3.19GHz

// http://www.codeproject.com/Articles/69941/Best-Square-Root-Method-Algorithm-Function-Precisi
float fast_sqrt(const float x){
    union
    {
        int i;
        float x;
    } u;

    u.x = x;
    u.i = (1<<29) + (u.i >> 1) - (1<<22);
    return u.x;
}

// forgot where this was from; TODO: find source
double dist(unsigned char r, unsigned char g, unsigned char b, Color e2) {
    int rmean = (r + e2.r ) / 2;
    int rDiff = r - e2.r;
    int gDiff = g - e2.g;
    int bDiff = b - e2.b;
    return fast_sqrt((((512+rmean)*rDiff*rDiff)>>8) + 4*gDiff*gDiff + (((767-rmean)*bDiff*bDiff)>>8));
}

char matchColor(unsigned char r, unsigned char g, unsigned char b){
    int index = 0;
    double best = -1;
    double distance;
    for (int i = 4; i < ARR_LENGTH(mapColors); ++i) {
        distance = dist(r, g, b, mapColors[i]);
        if (distance < best || best == -1.0f) {
            best = distance;
            index = i;
        }
    }

    return (char)(index < 143 ? index : -144 + (index - 143));
}

static void convertRow(void* ptr){
    param* params = (param*)ptr;
    int p;
    unsigned char r, g, b;

    for (int y = 0; y < params->ctx->height; y++) {
        p = params->x * 3 + y * params->ctx->rgbFrame->linesize[0];
        r = params->ctx->rgbFrame->data[0][p];
        g = params->ctx->rgbFrame->data[0][p + 1];
        b = params->ctx->rgbFrame->data[0][p + 2];
        params->ctx->data[(params->x / 128) + (params->ctx->width / 128) * (y / 128)][((params->x % 128) + 128 * (y % 128))] = matchColor(r, g, b);
    }

    latch_countdown(params->ctx->latch);
}

JNIEXPORT jboolean JNICALL Java_ga_nurupeaches_vivianmap_natives_NativeVideo_n_1readFrame(JNIEnv* env, jobject jthis, jlong ptr){
    Context* ctx = (Context*)ptr;

    while((ctx->read = av_read_frame(ctx->formatCtx, &(ctx->packetIn))) >= 0){
        if(ctx->packetIn.stream_index == ctx->vtrackId){
            avcodec_decode_video2(ctx->codecCtx, ctx->rawFrame, &(ctx->finished), &(ctx->packetIn));
            if(ctx->finished){
                sws_scale(ctx->sws, (const uint8_t* const*)ctx->rawFrame->data,
                          ctx->rawFrame->linesize, 0, ctx->codecCtx->height,
                          ctx->rgbFrame->data, ctx->rgbFrame->linesize);

                break;
            }
        }
    }

    if(ctx->read < 0){
        // bad. don't do this.
        // stop doing this me.
        Java_ga_nurupeaches_vivianmap_natives_NativeVideo_n_1close(env, jthis, ptr);
    }

    // lock count mutex so that we don't have a race condition
    latch_lockCount(ctx->latch);
    for(int x=0; x < ctx->width; x++){
        param* params = malloc(sizeof(param));
        params->ctx = ctx;
        params->x = x;
        threadpool_add(ctx->pool, convertRow, params, 0);
    }
    // unlock mutex after all tasks have been submitted
    latch_unlockCount(ctx->latch);

    // wait for count to hit 0
    latch_wait(ctx->latch);
    // reset latch
    latch_reset(ctx->latch);
    return true;
}

Any suggestions (especially performance improvements) would be appreciated! I'm willing to edit more code into the question if you need it; I just didn't because I didn't want it to become a needle in a haystack problem.

Iterate on x, not y
This computation loop:

for (int y = 0; y < params->ctx->height; y++) {
    p = params->x * 3 + y * params->ctx->rgbFrame->linesize[0];
    r = params->ctx->rgbFrame->data[0][p];
    g = params->ctx->rgbFrame->data[0][p + 1];
    b = params->ctx->rgbFrame->data[0][p + 2];
    params->ctx->data[(params->x / 128) + (params->ctx->width / 128) *
          (y / 128)][((params->x % 128) + 128 * (y % 128))] = matchColor(r, g, b);
}

is inefficient because you are looping over y whereas your arrays are stored in x order.  For example, from the first iteration to the next, you will be reading from:
(y = 0) params->ctx->rgbFrame->data[0][0]
(y = 1) params->ctx->rgbFrame->data[0][linesize]

If you had iterated over x, you would be reading from:
(x = 0) params->ctx->rgbFrame->data[0][0]
(x = 1) params->ctx->rgbFrame->data[0][3]

Similarly, you are writing to:
(y = 0) params->ctx->data[0][0]
(y = 1) params->ctx->data[0][128]

If you iterated over x, you would be writing to:
(x = 0) params->ctx->data[0][0]
(x = 1) params->ctx->data[0][1]

There are two reasons why accessing nearby addresses is faster:

You will be utilizing the CPU cache efficiently by reusing the same cache lines instead of causing lots of cache misses by accessing memory all over the place.
Since your program is multithreaded, your threads will stop contending on the same cache lines.  In other words, right now each of your threads is writing to the same cache lines as each other thread, because they are writing to neighboring addresses in the data array.  By switching to x order, your threads will each write to their own separate rows in the data array, which means they will be writing to different cache lines from each other (for the most part).  This will reduce cache contention and speed things up.


A linear search implementation for any data type in C I performed an "experiment" to test my understanding of pointers in C. I decided to write a linear search function that can work on any array provided there is an existing function to compare the elements of the array. Here are the files:

linear_search.h

#ifndef _LINEAR_SEARCH_H
#define _LINEAR_SEARCH_H

/*
 * Summary:
 *  Performs linear search on the given array using the given
 * comparator function. Returns the index of the matching array
 * element (< len), and -1 if the element is not found.
 *
 *  Parameters are independent of type and hence this function
 * can be used for arrays of all data-types, provided that the width
 * (i.e. size of the datatype in terms of bytes) is properly specified.
 *
 * Parameters:
 *  1. *base - the address of the array from whence we start searching.
 *  2. *x    - the address of the element to find in the array
 *  3. len   - the maximum number of elements we are willing to search against.
 *  4. width - the width of each element of the array (in bytes)
 *  5. compare(void*, void*)
 *           - the function which is used for comparison. Zero is taken to be equality.
 */
long long linear_search
    (
        const void *base, const void *x, const long long len, const int width,
        const int (*compare)(const void *, const void *)
    );

#endif

linear_search.c

long long linear_search
    (
        const void *base, const void *x, const long long len, const int width,
        const int (*compare)(const void *, const void *)
    )
{
    long long index = 0;
    // convert to a char * pointer in order to make the pointer arithmetic in terms of 1 byte
    char *base_ptr = (char *) base;
    for (; index < len; index++) {
        if ((*compare)((base_ptr + index * width), x) == 0)
            return index;
    }
    return -1;
}

main.c

#include <stdio.h>
#include <stdlib.h>
#include "linear_search.h"

const int compare(const void *a, const void *b)
{
    double *p = (double *) a;
    double *q = (double *) b;
    return *p - *q;
}

int main(int argc, char const *argv[])
{
    double *a;
    double x;
    int width;
    long long int index, len, i;
    width = sizeof(double);

    printf("Enter how many elements : ");
    scanf("%lld", &len);

    a = malloc(len * width);
    if (a == NULL) {
        printf("Error while trying to allocate memory.\n");
        return EXIT_FAILURE;
    }

    printf("Enter elements one-by-one:\n");
    for (i = 0; i < len; i++) {
        printf("%d : ", (i + 1));
        scanf("%lf", (a + i));
    }

    printf("Enter the element to look for : ");
    scanf("%lf", &x);
    index = linear_search(a, &x, len, width, compare);

    printf("Index is %lld\n", index);
    return EXIT_SUCCESS;
}

I would like to know if there's a better way to accomplish this task, or if I'm making any gross errors. While it's true that my program compiled without any errors or warnings, I am never sure when it comes to C. Suggestions (however small) and critique is welcome.

I think you have chosen some of your types poorly. You write:

long long linear_search
    (
        const void *base, const void *x, const long long len, const int width,
        const int (*compare)(const void *, const void *)
    );

If you take a look at the definition of bsearch, you would do well to be inspired by that function. Obviously, someone thought about how it will work, and also there's the "principle of least surprise" to consider: why should two search functions have different argument types or return different results? Or even pass their arguments in different orders?

Let's change the parameter names, and compare:

long long linear_search(const void *ptr, const void *key, const long long count, const int size, const int (*comp)(const void*, const void*));
void*     bsearch      (const void *key, const void *ptr,       size_t    count,    size_t size,       int (*comp)(const void*, const void*));

Parameter Order

The search function puts the key first, then the details of the array to be searched. Why?

Because the key isn't related to anything else in the parameter list, and it's the "important" thing in the search expression:  Find me a needle! In this haystack, of such and such size. Use this comparator.

I'd suggest you reorder your parameters to conform with this. Not only is it a more sensible arrangement, it keeps your search function consistent with the "standard" search function. That means less to learn, and less chance for a silly bug.

Parameter Names

base, x, len, width, compare

versus

key, ptr, count, size, comp

I actually prefer base to ptr, but len and width aren't super clear. count and size are better in that regard.

Parameter Types

Your parameters are pointers, an array size, and a memory size. You got void * correct as the base type of the pointers (const because no changes). The array size and memory size should be size_t.

The numeric parameters, and the compare-function return type, do not need to be declared const. Declaring them const is a promise not to change them - but they're function arguments (& a return value)! Who cares if you change them, it's not like you're going to give them back! Get rid of those const declarations, since they provide no value to the caller.

Return Type

The bsearch function returns a pointer, rather than an index. Why?

Because it just found a pointer, and it's easier and faster to return the result that it has, instead of doing any math to maintain both a pointer and an index, or doing math to convert from pointer to index.

Also, because the likely next operation for the user who calls a search function will be to use the result of the call, which will require computing a pointer.

If you choose not to use a pointer, you will need a signed integral type that is adequate to store any array index. That would be ptrdiff_t, since it is signed and can express any value of the form (&a[i] - a). So long long should be replaced, either by void * or by ptrdiff_t.

Note that the return type of bsearch is declared as void * rather than const void *. Why? Because the data being searched is probably not constant, and that's the "most compatible" type - you can assign it to pretty much anything with less required casting than other options (char *, const void *, etc.).


Turning a 2D array into a tree I have the following data structure that's obtained from a third party:

var data = [
    ["Parent1", "Parent1.1", "Parent1.1.1"],
    ["Parent2", "Parent2.1", "Parent2.1.1"],
    ["Parent3", "Parent3.1", "Parent3.1.1"],
    ["Parent1", "Parent1.2", "Parent1.2.1"],
    ["Parent1", "Parent1.2", "Parent1.2.2"]
];

Where child nodes may or may not be present.
I'm turning it into a tree to represent the data logically:

var tree = {
    "Parent1": {
        "Parent1.1": ["Parent1.1.1"],
        "Parent1.2": ["Parent1.2.1", "Parent1.2.2"]
    },
    "Parent2": {
        "Parent2.1": ["Parent2.1.1"]
    },
    "Parent3": {
        "Parent3.1": ["Parent3.1.1"]
    }
};

What I'm currently doing seems rather straightforward to myself, but somewhat convoluted due to the numerous return statements:

var tree = data.reduce(function(tree, item) {
    if (!item[0]) return tree;
    tree[item[0]] = tree[item[0]] || {};
    if (!item[1]) return tree;
    tree[item[0]][item[1]] = tree[item[0]][item[1]] || [];
    if (!item[2]) return tree;
    tree[item[0]][item[1]].push(item[2]);
    return tree;
}, {}); //Returns as shown above

Is there a better way to approach this problem, what improvements could be made?
The depth of the data is always fixed at three.

I was also facing similar issue where I needed to convert the 2-D array to a tree structure. This code might help.:)

var tree = data.reduce(function(tree, item) {
    var tempTree = tree;
    for(var i=0;i<item.length;i++){
        if(!tempTree[item[i]])
            tempTree[item[i]] = {};
        tempTree = tempTree[item[i]];
    }
    return tree;
}, {});


Parsing HTML table into Pandas DataFrame There is a text (link clickable) file with HTML table. The table is a bank statement. I'd like to parse it into pandas DataFrame. Is there a way to do it more gracefully? I've started to learn Python recently so there is a good chance you guys can give me a good advice.

from bs4 import BeautifulSoup
import pandas as pd
with open("sber2.txt", "r", encoding = "UTF8") as f:
    context = f.read()
    soup = BeautifulSoup(context, 'html.parser')

rows_dates = soup.find_all(attrs = {'data-bind':'momentDateText: date'})
rows_category = soup.find_all(attrs = {'data-bind' : 'text: categoryName'})
rows_comment = soup.find_all(attrs = {'data-bind' : 'text: comment'})
rows_money = soup.find_all(attrs = {'data-bind' : 'currencyText: nationalAmount'})

dic = {
    "dates" : [],
    "category" : [],
    "comment": [],
    "money" : []
    }
i = 0
while i < len(rows_dates):
    dic["dates"].append(rows_dates[i].text)
    dic["category"].append(rows_category[i].text)
    dic["comment"].append(rows_comment[i].text)
    dic["money"].append(rows_money[i].text)
    '''
    print(
        rows_dates[i].text, rows_category[i].text,
        rows_comment[i].text, rows_money[i].text)
    '''
    i += 1

df = pd.DataFrame(dic)
df.info()
print(df.head())

Output:

RangeIndex: 18 entries, 0 to 17
Data columns (total 4 columns):
category    18 non-null object
comment     18 non-null object
dates       18 non-null object
money       18 non-null object
dtypes: object(4)
memory usage: 656.0+ bytes
       category                                   comment       dates    money
0  Supermarkets  PYATEROCHKA 1168         SAMARA       RU  28.12.2017  -456,85
1  Supermarkets  KARUSEL                  SAMARA       RU  26.12.2017  -710,78
2  Supermarkets  PYATEROCHKA 1168         SAMARA       RU  24.12.2017  -800,24
3  Supermarkets  AUCHAN SAMARA IKEA       SAMARA       RU  19.12.2017  -154,38
4  Supermarkets  PYATEROCHKA 9481         SAMARA       RU  16.12.2017  -188,80

zip() with a list comprehension to the rescue:

rows_dates = soup.find_all(attrs={'data-bind': 'momentDateText: date'})
rows_category = soup.find_all(attrs={'data-bind': 'text: categoryName'})
rows_comment = soup.find_all(attrs={'data-bind': 'text: comment'})
rows_money = soup.find_all(attrs={'data-bind': 'currencyText: nationalAmount'})

data = [
    {
        "dates": date.get_text(),
        "category": category.get_text(),
        "comment": comment.get_text(),
        "money": money.get_text()
    }
    for date, category, comment, money in zip(rows_dates, rows_category, rows_comment, rows_money)
]

Or, you can do it a bit differently - zipping the lists of texts and specifying the dataframe headers via columns argument:

rows_dates = [item.get_text() for item in soup.find_all(attrs={'data-bind': 'momentDateText: date'})]
rows_category = [item.get_text() for item in soup.find_all(attrs={'data-bind': 'text: categoryName'})]
rows_comment = [item.get_text() for item in soup.find_all(attrs={'data-bind': 'text: comment'})]
rows_money = [item.get_text() for item in soup.find_all(attrs={'data-bind': 'currencyText: nationalAmount'})]

data = list(zip(rows_dates, rows_category, rows_comment, rows_money))

df = pd.DataFrame(data, columns=["dates", "category", "comment", "money"])

df = pd.DataFrame(data)


Proper usage of "using" statement in base64 encode method I used the example on here, but with little changes in order to write a base64 encoder method. I have a few points about the code:

I would like to use using statement, but I do not want to use inner "using" statements, so I use multiple variable declaration in a single "using" statement. Is that suitable in order to benefit from "using" statement for more than one object. Moreover, what are alternatives?
Inside the using statement block, I used different try-catch blocks for each operation that can be throw an exception. I did not catch specific exceptions first an just use Exception class, but my goal is just return true or false which indicates the success status of the method. Is it a good practice to use different try-catch blocks and use "return" statement inside these blocks for resource handling?
Also, I would like to use an extra try-catch block which surrounds the using blocks for the classes which have Dispose() methods that can throw exceptions also. This is also discussed here. Is that a common approach, or should I completely stop using using statements, for these cases.

 public static bool ConvertToBase64(string inputFile, string outputFile)
 {
     try
     {
         using (FileStream inputFileStream = new FileStream(inputFile, FileMode.Open),
                outputFileStream = new FileStream(outputFile, FileMode.Create))
         {
             try
             {
                 ToBase64Transform base64Transform = new ToBase64Transform();
                 //Buffers for read/write operations
                 Byte[] outputBuffer = new byte[base64Transform.OutputBlockSize];
                 Byte[] inputBuffer = new byte[inputFileStream.Length];
                 //Offset to count the number of bytes transformed so far
                 int inputOffset = 0;

                 try
                 {
                     inputFileStream.Read(inputBuffer, 0, (int)inputFileStream.Length);
                 }
                 catch(Exception ex)
                 {
                     return false;
                 }

                 if (false == base64Transform.CanTransformMultipleBlocks)
                 {
                     while (inputBuffer.Length - inputOffset > base64Transform.InputBlockSize)
                     {
                         //Transform a block of input data
                         base64Transform.TransformBlock(inputBuffer, inputOffset, inputBuffer.Length - inputOffset, outputBuffer, 0);
                         inputOffset += base64Transform.InputBlockSize;

                         try
                         {
                             outputFileStream.Write(outputBuffer, 0, base64Transform.OutputBlockSize);
                         }
                         catch(Exception ex)
                         {
                             return false;
                         }

                         //Insert a new line after 76 characters
                         if (inputOffset % 19 == 0)
                         {
                             byte[] newline = Encoding.ASCII.GetBytes(Environment.NewLine);
                             outputFileStream.Write(newline, 0, newline.Length);
                         }
                     }
                 }

                 //Final block transform
                 try
                 {
                     byte[] lastBlock = base64Transform.TransformFinalBlock(inputBuffer, inputOffset, inputBuffer.Length - inputOffset);
                     outputFileStream.Write(lastBlock, 0, base64Transform.OutputBlockSize);
                 }
                 catch(Exception ex)
                 {
                     return false;
                 }

                 if (false == base64Transform.CanReuseTransform)
                 {
                     base64Transform.Clear();
                 }
             }
             catch(Exception ex)
             {
                 return false;
             }
         }
     }
     catch(Exception ex)
     {
         return false;
     }

     return true;
 }

SRP

Your method is responsible of to many things, like

reading from a file
writing to a file
converting to base64

So I would suggest to add some more methods

public static bool ConvertToBase64(string inputFile, string outputFile)
{
    try
    {
        byte[] input = System.IO.File.ReadAllBytes(inputFile);
        byte[] base64 = ConvertToBase64(input);
        System.IO.File.WriteAllBytes(outputFile,base64);
        return true;
    }
    catch(Exception)
    {
        return false;
    }
}

private byte[] ConvertToBase64(byte[] input)
{

}

As already stated in the comment, the inner try..catch statements are useless. Your outer try..catch would handle the exceptions in the same way.

About the using statements

If you don't need them, don't use them. As you are reading/writing to/from a file, you can just use the above methods, which internally using a using statement. Always use the right tool to do the job.

If you need to use using statements, you can also stack them like

using (disposable1)
using (disposible2)
{

}

Comments

Don't use comments to describe what you are doing, but why you are doing something. Especially if it is crystal clear what you are doing like

//Buffers for read/write operations
Byte[] outputBuffer = new byte[base64Transform.OutputBlockSize];
Byte[] inputBuffer = new byte[inputFileStream.Length];

this comment is just useless.

Magic numbers

As you need a comment to describe

if (inputOffset % 19 == 0)

you should better use a const for that 19 (but don't ask me how to name it)

If you still want the inner try..catch you can simply use catch(Exception) as there is no need for ex.

Otherwise your code looks good. The naming of your fields is also meaningful.


Pandas - 3 iterations per second on simple conditional The function removes rows from a pandas df if that row doesn't have the value of important_1 inside of important_2. For example if important_1 is "blue" and important_2 is "M" then that row would be removed, but if important_2 were "redbluegreen" then the row would be kept.

On my ~125mb files this code runs really slow. Only about 3 iterations a second. In this example it runs faster maybe because it is a much smaller csv.
How can I speed this up?

import pandas as pd
import numpy as np
from io import StringIO

df = """
valid,important_1,important_2,unrelated_1,unrelated_2
2015-07-10 01:47:00,blue,,blabla,foobar56
2015-07-10 01:51:00,blue,M,blabla,foobar32
2015-07-10 02:37:00,blue,M,blab004la,foobar
2015-07-10 02:51:00,blue,M,blabla,foobar343
2015-07-10 03:19:00,blue,blue green,blabla,foobar
2015-07-10 03:51:00,blue,,blabla1,foobar6543
2015-07-10 04:11:00,blue,green red,blabla,foobar
2015-07-10 04:51:00,blue,red,blabla,foobar2466
2015-07-10 05:51:00,blue,blue,blabla,foobar
2015-07-10 06:27:00,blue,,blabla4,foobar
2015-07-10 06:51:00,blue,,blab605la3,foobar6543
2015-07-10 07:27:00,blue,M,blabla,foobar
2015-07-10 07:51:00,blue,M,blab445la2,foobar2334
2015-07-10 08:51:00,blue,blue green red,blabla,foobar7666
2015-07-10 09:51:00,blue,blue green,blabla,foobar
"""
df = StringIO(df)
df = pd.read_csv(df)

def remove_rows_that_dont_have_important_1_in_important_2(df):
    """
    Removes all rows of the dataframe that do not have important_1 inside of important_2.
    Note: All rows have the same value for important_1.
    Note: Every row can have a different value for important_2.
    Note: There are unrelated columns in the df that cannot be dropped from the df. But rows still can.
    """
    important_1 = df.at[0, 'important_1']  # get the icao fro the df from the first ob
    important_2_column_index = df.columns.get_loc('important_2')  # column order won't always be the same, so find the right column first
    iter_index = len(df)  # the start point of the loop

    while iter_index > 0:  # start from the end of the df and work forwards
        print(iter_index)  # DEBUG printing
        iter_index -= 1  # every loop move forward one item. This is at the top of the loop because that way every loop we ensure to move down one (or up a row depending on how you look at it).

        value_of_important_2_for_this_row = df.iat[iter_index, important_2_column_index]  # the value of important_2 for the row we are currently on

        # check for np.NaN first because it is cheaper then checking str in str
        if value_of_important_2_for_this_row is np.NaN:  # if the value of important_2 is np.NaN then there is now way important_1 can be in it
            pass

        # str in str check
        elif important_1 not in value_of_important_2_for_this_row:  # safe to assume the important_2 is off type str
            pass

        else:  # safe to assume that important_1 is indeed in important_2
            continue  # skip deletion because important_1 is in important_2 Yay!

        df.drop([iter_index], inplace=True)  # delete the row

    return df  # reset the index before returning the df

df = remove_rows_that_dont_have_important_1_in_important_2(df=df)
print(df)

OUTPUT

                  valid important_1     important_2 unrelated_1 unrelated_2
4   2015-07-10 03:19:00        blue      blue green      blabla      foobar
8   2015-07-10 05:51:00        blue            blue      blabla      foobar
13  2015-07-10 08:51:00        blue  blue green red      blabla  foobar7666
14  2015-07-10 09:51:00        blue      blue green      blabla      foobar

I don't really know the libraries that you're using, but maybe the overhead of dropping rows from the CSV one by one is significant?  You could try batching the drop (it looks like that drop function takes a list of indices but you're only passing it one at a time) and see if that speeds things up:

from pandas import DataFrame
from typing import List

def remove_rows_that_dont_have_important_1_in_important_2(df: DataFrame) -> DataFrame:
    """
    Removes all rows of the dataframe that do not have important_1 inside of important_2.
    Note: All rows have the same value for important_1.
    Note: Every row can have a different value for important_2.
    Note: There are unrelated columns in the df that cannot be dropped from the df. But rows still can.
    """
    important_1 = df.at[0, 'important_1']  # get the icao fro the df from the first ob
    important_2_col = df.columns.get_loc('important_2')  # column order won't always be the same, so find the right column first

    indices_to_drop: List[int] = []

    for i in range(len(df), 0, -1):
        print(i)  # DEBUG printing

        important_2  = df.iat[i, important_2_col]
        if important_2 == np.NaN or important_1 not in important_2:
            indices_to_drop.append(i)

    df.drop(indices_to_drop, inplace=True)
    return df


Implement Fibonacci with `iterate` I implemented the Fibonacci sequence using iterate.

import Data.List

fib :: Int -> Maybe Int
fib n
 | n < 0     = Nothing
 | otherwise = (fmap (snd . fst)) $ find (\(_,i) -> n == i) (zip res [0..])

res :: [(Int, Int)]
res = iterate f (0,1)

f :: (Num a, Num b) => (a, a) -> (a, a)
f (x, y) = (y,x+y)

Testing

> fib (-1)
Nothing
> fib 0
Just 1
> fib 2
Just 2
> fib 3
Just 3
> fib 4
Just 5
> fib 5
Just 8
> fib 6
Just 13

Please review it.

In my opinion, Maybe is too cumbersome here.  I'd consider fib (-1) to be ill-defined, and I'd let it just crash with an error.

By your convention, the Fibonacci sequence is

  1, 2, 3, 5, 8, 13, …

I would prefer to see it start earlier, such that fib 0 is 0:

  0, 1, 1, 2, 3, 5, 8, 13, ….

The outputs can get pretty big.  I suggest using Integer instead of Int.

You're using a very convoluted way to extract the nth item from a list.

Suggested solution

import Data.List (iterate)

fib :: Int -> Integer
fib n = fst $ sequence !! n
  where
    sequence = iterate (\(x, y) -> (y, x + y)) (0, 1)

You could also use the point-free style:

fib :: Int -> Integer
fib = (!!) $ map fst $ iterate (\(x, y) -> (y, x + y)) (0, 1)

For that matter, you could just define the infinite sequence

fibonacciSequence = map fst $ iterate (\(x, y) -> (y, x + y)) (0, 1)

… and let the caller do whatever they want with it, including extracting specific entries using !!.

Behaviour

*Main> fib (-1)
*** Exception: Prelude.!!: negative index
*Main> fib 0
0
*Main> fib 1
1
*Main> fib 2
1
*Main> fib 3
2
*Main> fib 4
3
*Main> fib 5
5


Counting characters inside of a textarea using Angular I have a textarea and need to calculate how many characters have been typed into it.

Both of the following solutions work correctly—which would be preferable, and why? Is there an even better way to implement it?

Option A:

<textarea
   id="descriptionEducation"
   name="description"
   rows="5"
   [(ngModel)]="fruit.description"></textarea>

    You have written {{ fruit.description.length }} characters

Option B:

<textarea
   id="descriptionEducation"
   name="description"
   rows="5"
   [ngModel]="fruit.description" #descriptionFruit></textarea>

    You have written {{ descriptionFruit.value.length }} characters

You don't need to declare an extra reference variable. I would suggest the first approach i.e. using [(ngModel)] for two-way data binding.

However, in the first approach, you must initialize the model value before using it in your template i.e. fruit.description should not be undefined otherwise you'll get the following TypeError:

  Cannot read property 'length' of undefined

Using the second approach, you wont get that error.


Getting a value from a query string I have a URL in this format:

http://localhost:57279/AssociateCaseDetails?UId=176948&caseID=1123

I need the values of UId and CaseID:

var UId =window.location.href.split("?")[1].split("&")[0].split("=")[1];

It’s working, but is there a better way to access the same, given the same URL?

Instead of splitting from the ?, you can use location.search. It returns everything from (and including) the ? until the end of the query string. If there's a hash, it stops right before #.

Since it includes the ?, you can do location.search.slice(1) to get the string without the ?.

Then you can do:

var queries = location.search
                      .slice(1)
                      .split('&')
                      .reduce(function(carry, query){
                        var queryPair = query.split('=');
                        carry[queryPair[0]] = queryPair[1];
                        return carry;
                      },{});

From ?foo=foovalue&bar=barvalue, you can get something like:

var queries = {
  foo: 'foovalue',
  bar: 'barvalue',
}

I have to add that this only works for 1-level query string. Stuff like arrays (foo[]=foo0&foo[]=foo1) and key-value pairs (foo[bar]=bar&foo[baz]=baz) isn't covered in this logic.


Write a message to a log I have started to read Robert C. Martin Clean Code book.

To learn and gain more expreiences I wrote a single Log class. So I want some suggestions to improve this code..
This is a part of the Log class:

public class FileLogger : ILog
{
    private string directoryPath = string.Empty;
    private string fileName = string.Empty;

    public FileLogger(string logDirectoryPath, string logFileName)
    {
        if (string.IsNullOrEmpty(logDirectoryPath))
        {
            throw new ArgumentException(nameof(logDirectoryPath), "Write some error message...");
        }

        if (string.IsNullOrEmpty(logFileName))
        {
            throw new ArgumentException(nameof(logFileName), "Write some error message...");
        }

        this.directoryPath = logDirectoryPath;
        this.fileName = logFileName;
    }

    public void LogMessage(string message)
    {
        try
        {
            if (!string.IsNullOrEmpty(message))
            {
                if (!LogDirectoryExists(directoryPath))
                {
                    CreateLogDirectory(directoryPath);
                }

                WriteToLogFile(message);
            }
        }
        catch (Exception ex)
        {
            //Catch exception, do something...
        }
    }

    private bool LogDirectoryExists(string logDirectoryPath)
    {
        return Directory.Exists(logDirectoryPath);
    }

    private void CreateLogDirectory(string logDirectoryPath)
    {
        Directory.CreateDirectory(logDirectoryPath);
    }

    private void WriteToLogFile(string message)
    {
        using (var logFile = new FileStream(Path.Combine(directoryPath,fileName), FileMode.Append, FileAccess.Write))
        {
            using (var logFileWriter = new StreamWriter(logFile))
            {
                logFileWriter.Write(message);
            }
        }
    }
}

I wrote 3 different function to write a message to a log. I try to use SRP and Command Query Separation, but I don't sure this is a good in this way. Maybe there are another problems with this code, example naming or variable declarations.

After discussion in comments your code became much better.

Also if directoryPath and fileName will not be changed after initialization in constructor you can define these fields as readonly:

private readonly string directoryPath = string.Empty;
private readonly string fileName = string.Empty;

This code

using (var logFile = new FileStream(Path.Combine(directoryPath,fileName), FileMode.Append, FileAccess.Write))
{
    using (var logFileWriter = new StreamWriter(logFile))
    {
        logFileWriter.Write(message);
    }
}

can be replaced with

File.AppendAllText(Path.Combine(directoryPath, fileName), message);

Also ArgumentException's constructor takes message as first argument and paramName as second, so you need to call it like this:

throw new ArgumentException("Write some error message...", nameof(logDirectoryPath));


Am I interfacing in a secure manner with rijndael? I have been working to create an easy-to-use set of methods to encrypt configuration objects for my client application. It will contain username and passwords to databases and similar vaults of data, so it's rather important that I've got the fundamentals correct.

I'm fairly new to this form of encryption, and I certainly haven't done this in  .NET before. So I am curious - is there any given, open caveats in the attached code? I'll greatly appreciate your comments!

Also, please don't mind the comments in the code. This is a late-night side-project for me, and I'll look into sprucing it all up once I've got it all examined.

 public byte[] CalculatePasswordHash(byte[] salt)
 {
     var hash = Encoding.UTF8.GetBytes(Password);
     var hashing = SHA256.Create();
     var iterations = 10000;

     while (iterations > 0)
     {
         // Compute the hash
         hash = hashing.ComputeHash(hash);

         // Apply the salt
         for (int position = 0, cursor = 0; position < hash.Length; position += 1)
         {
             hash[position] ^= salt[cursor];

             cursor += 1;
             if (cursor >= salt.Length)
                 cursor = 0;
         }

         iterations -= 1;
     }

     return hash;
 }

 public byte[] CreateSalt()
 {
     return CreateSalt(SaltLength);
 }

 public byte[] CreateSalt(int size)
 {
     var random = new RNGCryptoServiceProvider();
     var buf = new byte[size];
     random.GetBytes(buf);

     return buf;
 }

 public void Lock(string filePath, byte[] source)
 {
     // Ensure that the path is rooted
     filePath = RootPath(filePath);

     // Generate salt as well as the encryption key
     var salt = CreateSalt();
     var key = CalculatePasswordHash(salt);

     // Initialize the Rijndael encryption tool
     var crypto = Rijndael.Create();
     var iv = CreateSalt(crypto.BlockSize / 8);

     using (var mem = new MemoryStream())
     {

         // Encrypt the source array
         crypto.Key = key;
         crypto.IV = iv;

         var desc = crypto.CreateEncryptor();
         using (var cryptoStream = new CryptoStream(mem, desc, CryptoStreamMode.Write))
         {
             cryptoStream.Write(source, 0, source.Length);
         }

         desc.Dispose();

         // Append the salt and iv to the end of the file
         using (var fs = new FileStream(filePath, FileMode.Create))
         {
             var encrypted = mem.ToArray();
             fs.Write(encrypted, 0, encrypted.Length);
             fs.Write(salt, 0, salt.Length);
             fs.Write(iv, 0, iv.Length);
         }
     }
 }

CalculatePasswordHash is inadvertently reimplementing (and probably not as well) a standard primitive. You should use System.Security.Cryptography.Rfc2898DeriveBytes instead.
The code

    var desc = crypto.CreateEncryptor();
    ...
    desc.Dispose();

raises a red flag. Why isn't it using using?
You write to a memory stream and then write the contents of the memory stream unmodified to a file. It seems that you could simplify this just skipping the middle-man.


Writing an interface for networking in an embedded system I'm writing a driver in C for a WiFi module attached to a PIC18 microcontroller. I want to implement some functions that I'm familiar with in computer application level programming like Window's API recv function for WinSock.
The module itself already implements the WiFi communication protocol and the TCP/IP stack, and communicate with the PIC18 microcontroller through UART.

Is this the correct way to achieve this? Efficiency is very important here (It's an embedded system too).

#define RECEIVE_TIMEOUT 200
#define RECEIVE_RETRY_COUNT 4

UINT16 ReceiveData(void* _data, UINT16 length)
{
    UINT16 received = 0;
    UINT8 retry_count = 0;

    while(received < length)
    {
        if(UART1_Data_Ready())
        {
            ((BYTE*)_data)[received] = ReceiveByte();
            received++;
            continue;
        }

        if(retry_count == RECEIVE_RETRY_COUNT)
            break;

        retry_count++;
        Delay_ms(RECEIVE_TIMEOUT/RECEIVE_RETRY_COUNT);
    }

    return received;
}

Just a small note on sleeping when waiting for input.  As others have said
your loop is inefficient in that it uses a simple busy loop.  This has at
least two potential problems:

it prevent other activities from executing;
it uses power.

But these are not necessarily problems for your application.  And if in
your application there really is nothing else to be done until bytes arrive
and if power consumption is not a problem, then a busy loop is by far the
simplest solution.

Using interrupts will inevitably complicate the rest of your code and should
be avoided unless really necessary (for example if there are other
computing tasks that need to run while waiting for input).

If 1 above is not important but 2 is (ie if you need to minimise power use)
you could put the core to sleep during waiting and have the byte-receive wake
it.  This would retain the benefit of avoiding interrupt handling. You would
need to set a timer to wake the core too if you need a timeout.

On the details of your code, I dislike the use of Windows types in an embedded
system.  What is wrong with int and char or their unsigned varieties?

Your leading underscore on _data is reserved by something or other and is anyway
pointless.

I don't object to passing in a void * (after all, C stdlib functions recv
and read do this), but I would assign it to a local variable of appropriate
type and avoid casts.  This makes absolutely no difference when the buffer is
used just once, as here and is just a personal preference.

And using continue is usually frowned on and often unnecessary.

int ReceiveData(void *data, int length)
{
    unsigned char *p = data;
    int received = 0;
    int retry = 0;

    while (received < length)
    {
        if (UART1_Data_Ready())
        {
            p[received++] = ReceiveByte();
        }
        else if (++retry <= RECEIVE_RETRY_COUNT)
        {
            Delay_ms(RECEIVE_TIMEOUT/RECEIVE_RETRY_COUNT);
        }
        else break;
    }
    return received;
}


Tile map movement by touch I am currently developing a game for Android. One of the things I am working on is moving the map by touch.

This is the code that handles getting speed, which is then added to the screens position and the next update draws the corresponding portion of the map.

@Override
public boolean onTouchEvent(MotionEvent event) {
    switch(event.getAction()) {
    case MotionEvent.ACTION_DOWN:
        if (released) {
            touchX = event.getX();
            touchY = event.getY();
            released = false;
        }
        else {
            float newX = event.getX();
            float newY = event.getY();
            speedX = (int) (newX - touchX);
            speedY = (int) (newY - touchY);
            touchX = newX;
            touchY = newY;
        }
        return true;
    case MotionEvent.ACTION_UP:
        released = true;
        return true;
    }
    return false;
}

It works fine, it does. But it seems like it is not enough of a fluid movement.

Does anyone have any idea of how I can make it more like a seamless motion?

EDIT:

And just for info, my fps is currently 80. So I do not believe it is caused by the bitmap drawing itself.

My experience with Android and handling touch events is that it's very hard to look at the code and understand how it will work when you're running the application. It can even be a difference with different Android devices, so make sure that you test your app on an Android 2.3.5 device as well as an Android 4+ device (assuming you're targeting Android 2.3.5 as well).

From what I understand, "moving the map by touch" you mean that you are scrolling through the map, like a HorizontalScrollView combined with an ordinary (vertical) ScrollView. It is said to be possible to combine these two together to create a "two dimensional scroll view", I myself have not been able to accomplish that in a way that creates a smooth user experience (I actually think this is a bad way of doing it). I have however, managed to find a quite easy-to-use Android view class that takes care of all this scrolling. It seems however like the link has disappeared but you could try this GitHub project, if that doesn't work for you then let me know either by a comment or find me in The 2nd Monitor chatroom and I can provide the implementation I'm using.

As for your existing code, it's clean, it's straight-forward. But where/how is speedX/speedY and touchX/touchY used? Could the problem be there?

I only have one suggestion for improvement of your code:

switch(event.getAction()) {

Should be (see here)

switch(event.getActionMasked()) {

A general suggestion when working with touch events:

Log, Log, and Log Use Log.d("YOUR_TAG", "A whole bunch of information") like a maniac! The more you log, the more you will hopefully be able to understand about MotionEvents, and the better code you will be able to write.


Basic cross-validation in R We would like to perform n-fold non-stratified cross-validation on a model, say an lm or glm.

We have a list of quality metrics we would like to evaluate, which we wrap in a function:

fn_rmse <- function(v_pred,v_true) sqrt(mean((v_true-v_pred)^2))
fn_mae <- function(v_pred,v_true) mean(abs(v_true-v_pred))
l_q <- list(fn_rmse, fn_mae, fn_smdape)
fn_q <- function(v_pred, v_true) {
  return(sapply(l_q, function(f)f(v_pred,v_true)))
}

Now we set up a cross-validation routine:

# Set up n-fold non-stratified cross-validation routines

# fn_fold: Run cross validation on a fold
# Input:
#   df_train = training data
#   df_test = testing data
#   mdl = model to test
#   fn_q = quality metrics to try
# Output: list of qualities for this fold
fn_fold <- function(df_train, df_test, mdl, fn_q) {
    mdl_fold <- update(mdl, data=df_train)
    v_predictions <- predict(mdl_fold, newdata=df_test, type="response")[[1]]
    fmla <- formula(mdl$call["formula"])
    s_regressand <- all.vars(fmla[[2]])
    v_quality <- sapply(v_predictions, fn_q, v_true=df_test[[s_regressand]])
    return(v_quality)
}

# fn_cv: Perform n-fold cross validation
# Input:
#   df = sample
#   mdl = model
#   n_folds = number of folds
#   fn_q = quality metrics to try
# Output: ([# of metrics] x n_folds)-matrix containing cross validation results
fn_cv <- function(df, mdl, n_folds, fn_q) {
    v_folds <- cut(seq(1,nrow(df)), breaks=n_folds, labels=FALSE)
    v_folds <- sample(v_folds)
    v_results <- sapply(1:n_folds,
        function(n_f) fn_fold(
            df[-which(v_folds==n_f,arr.ind=TRUE),],
            df[which(v_folds==n_f,arr.ind=TRUE),],
            mdl,
            fn_q)
        )
    return(v_results)
}

To run 10-fold cross-validation on a model, I call:

mtx_cv_results <- fn_cv(df=df_my_sample,
                        mdl=mdl_my_model,
                        n_folds=10,
                        fn_q=fn_q)

This feels a bit like reinventing the wheel. Can anything be done to make this code nicer?

My advice is to use the caret-package in which you can easily define your own metrics. The use of CV is described here in a clear manner:

https://machinelearningmastery.com/how-to-estimate-model-accuracy-in-r-using-the-caret-package/

The ingredients to your metrics requirements should be deriveable from the output via str(model). Here's a small example that should help you to get to your results:

library(caret)
library(ModelMetrics)

# Generate some data
Xs<- matrix(rnorm(300*20), nrow = 300, ncol = 20)
Yvec<- rnorm(300)

# Define your metrics
maeSummary <- function (data,
                        lev = NULL,
                        model = NULL) {
  mae <- mae(data$obs, data$pred)
  names(mae) <- "MAE"

  mse <- rmse(data$obs,data$pred)
  names(mse) <- "RMSE"

  out = c(mae,mse)
  out
}

# Initialize
train= trainControl(method="CV", number=10, summaryFunction = maeSummary, returnResamp="all", savePredictions="all")

# Train
model = train(x= Xs, y = Yvec, trControl=train, method="lm")

# Overall results
print(model)

# Metrics per fold
model$resample

Good luck!


Beginner stopwatch app I'm learning Swift and would appreciate any code review suggestions (the more nit-picky, the better).  In particular, I'm wondering if there's a more efficient way to format the time in the timeToString function I wrote, since I'm repeating myself three times.

import UIKit
import Foundation

class ViewController: UIViewController {

    @IBOutlet var timerLabel: UILabel!

    var timer = NSTimer()
    var count = 0

    @IBAction func start(sender: AnyObject) {
      timer = NSTimer.scheduledTimerWithTimeInterval(1, target: self, selector: Selector("result"), userInfo: nil, repeats: true)
    }

    @IBAction func stop(sender: AnyObject) {
      timer.invalidate()
    }

    @IBAction func reset(sender: AnyObject) {
      timer.invalidate()
      count = 0
      let (h, m, s) = convertSeconds(count)

      timerLabel.text = timeToString(h, m: m, s: s)
    }

    func result(){
      count++
      let (h, m, s) = convertSeconds(count)

      timerLabel.text = timeToString(h, m: m, s: s)
    }

    func convertSeconds(totalSeconds: Int) -> (h:Int, m:Int, s:Int){
      var seconds = totalSeconds % 60
      var minutes = (totalSeconds / 60) % 60
      var hours = totalSeconds / 3600
      return (hours, minutes, seconds)
    }

    func timeToString(h:Int, m:Int, s:Int) -> String {

      var h = String(format: "%02d", h)
      var m = String(format: "%02d", m)
      var s = String(format: "%02d", s)
      return "\(h)h \(m)m \(s)s"
    }

    override func viewDidLoad() {
      super.viewDidLoad()
      let (h, m, s) = convertSeconds(count)

      timerLabel.text = timeToString(h, m: m, s: s)
    }

    override func didReceiveMemoryWarning() {
      super.didReceiveMemoryWarning()
    }

}

The primary problem I see here is the way we're keeping track of the time passed.  NSTimer is not guaranteed to tick with the exact amount of time you passed it.  It will try getting as close as possible, but the most specific problem is when the thread has a lot of activity on it.

If between two ticks, the thread the timer is one gets a lot of activity stuck on it, then our timer can start displaying a strange time.

So, instead of using a count variable as an int, let's use a startTime as an NSDate object.  We may also need an NSTimeInterval variable in the case when the timer is starter, then stopped, then restarted without being reset.

Once this is done, the start method will do something like this:

func start() {
    self.startTime = NSDate()
    self.updateUI()
    timer.invalidate()
    timer = NSTimer.scheduledTimerWithTimeInterval(1.0f, target: self, selector: Selector("updateUI"), userInfo: nil, repeats: true)
}

while the stop method looks more like this:

func stop() {
    timer.invalidate()
    self.accumulatedTime += NSDate().timeIntervalSinceDate(self.startTime)
    self.updateUI()
}

and the reset method looks something like this:

func reset() {
    timer.invalidate()
    self.accumulatedTime = 0
    self.start()
}

We can replace convertSeconds and timeToString with the following method:

func timeIntervalToString(ti: NSTimeInterval) -> String? {
    let dcf = NSDateComponentsFormatter()
    dcf.zeroFormattingBehavior = .Pad
    dcf.allowedUnits = (.CalendarUnitHour | .CalendarUnitMinute | .CalendarUnitSecond)
    return dcf.stringFromTimeInterval(ti)
}

And now our updateUI method looks something like this:

func updateUI() {
    var time = self.accumulatedTime + NSDate().timeIntervalSinceDate(self.startTime)
    self.timerLabel.text = self.timeIntervalToString(time)
}

Now our label should always give us an accurate representation of the time passed in hours, minutes, and seconds no matter how busy the thread is.  A busy thread would only mean that the UI isn't updated as often, but in the instant it is updated, it is perfectly accurate.

But more importantly, we're able to track more precisely the exact amount of time passed in case we ever decide we want to display more precision.

Moreover, if we ever decide we want to change the rate at which the UI is updated, we can do that, because our timer doesn't rely on the update method being ticked once per second in order to be accurate.  The time could be calculated in any instant, and we will get a precise time.

As one final note, rather than putting all of this code in our view controller, I'd probably abstract this into a stop watch struct.  Something like this:

func timeIntervalToString(ti: NSTimeInterval) -> String? {
    let dcf = NSDateComponentsFormatter()
    dcf.zeroFormattingBehavior = .Pad
    dcf.allowedUnits = (.CalendarUnitHour | .CalendarUnitMinute | .CalendarUnitSecond)
    return dcf.stringFromTimeInterval(ti)
}

public struct StopWatch {
    private var startTime: NSDate?
    private var accumulatedTime: NSTimeInterval = 0.0

    public var elapsedTimeInterval: NSTimeInterval {
        get {
            return self.accumulatedTime + NSDate().timeIntervalSinceDate(self.startTime ?? NSDate())
        }
    }

    public var elapsedTimeString: String {
        get {
            return timeIntervalToString(self.elapsedTimeInterval) ?? "0:00:00"
        }
    }

    public mutating func start() {
        self.startTime = NSDate()
    }

    public mutating func stop() {
        self.accumulatedTime += NSDate().timeIntervalSinceDate(self.startTime ?? NSDate())
        self.startTime = nil
    }

    public mutating func reset() {
        self.accumulatedTime = 0.0
        self.start()
    }
}

And this allows us to minimize the amount of code that actually goes into our view controller, which can now look something more like this:

class StopWatchViewController: UIViewController {
    @IBOutlet var timerLabel: UILabel!
    var stopWatch: StopWatch = StopWatch()
    var timer: NSTimer?

    func viewDidAppear(animated: Bool) {
        super.viewDidAppear(animated)
        self.timer = NSTimer.scheduledTimerWithTimeInterval(1.0, target: self, selector: Selector("updateUI"), userInfo: nil, repeats: true)
    }

    func viewDidDisappear(animated: Bool) {
        super.viewDidDisappear(animated)
        self.timer?.invalidate()
    }

    func updateUI() {
        self.timerLabel.text = self.stopWatch.elapsedTimeString
    }

    @IBAction func start(sender: UIButton) {
        self.stopWatch.start()
    }

    @IBAction func stop(sender: UIButton) {
        self.stopWatch.stop()
    }

    @IBAction func reset(sender: UIButton) {
        self.stopWatch.reset()
    }
}

The main point here being that we've abstracted all of the non-UI code out of the view controller class.


Defining array contents using multiple statements on one line I'm writing a sub which is going to determine which type of business is being detailed on a particular line of data. It's going to use column positions to check if the data exists, and then output the corresponding string if it does.

I've decided that the best way to organise a group of my arguments is using a 2D array. Now, for readability, clarity and general good coding practice, which is better:

This:

arrAmountColumns(1, 1) = lngInvestmentAmountColumn: arrAmountColumns(1, 2) = "Investment Amount"
arrAmountColumns(2, 1) = lngSinglePremiumColumn: arrAmountColumns(2, 2) = "Single Premium"
arrAmountColumns(3, 1) = lngMonthlyPremiumColumn: arrAmountColumns(3, 2) = "Monthly Premium"

or this:

arrAmountColumns(1, 1) = lngInvestmentAmountColumn
arrAmountColumns(1, 2) = "Investment Amount"

arrAmountColumns(2, 1) = lngSinglePremiumColumn
arrAmountColumns(2, 2) = "Single Premium"

arrAmountColumns(3, 1) = lngMonthlyPremiumColumn
arrAmountColumns(3, 2) = "Monthly Premium"

I'd go with the second snippet. They're instructions, and instructions read more naturally from top to bottom, rather than from left to right.

Cramming multiple instructions into a single line is a bad habit IMO - it's not because the language supports it that it's necessarily a good idea; the array has 2 columns.. what if it were 10? Would you consider cramming 10 instructions on the same line?

Your indices seem to go (row,column), which amounts to (y,x), which is a bit counter-intuitive, and if you wanted to write that array into a worksheet range, you'd have to transpose it:

Range("A1:B" & UBound(arrAmountColumns,2)) = arrAmountColumns

wouldn't produce the expected result.

By using the first index as your "column" (X) and the second as you "row" (Y), you eliminate that ambiguity and the code would read (x,y), which is a much more intuitive way of visualizing 2D arrays.


Generating arithmetic quiz questions I have created a quiz in Python that will generate a series of 10 questions to test the basic arithmetic skills of primary school children. To do this, I have used a while loop and random functions to generate random numbers and operations.

name = input("What is your name? ")
print("Hello, "+name+". You will be completing a quiz that will ask you 10 questions which will test you on adding, subtracting and multiplying two numbers together. Try your best at each question and good luck!")

import random
from operator import add, sub, mul

count = 0
score = 0
while count <= 9:
    ops = (add, sub, mul)
    op = random.choice(ops)
    x = random.randint(1,10)
    y = random.randint(1,10)

    if op == add:
        print("What is", x, "+",y, "? ")
        question_add = int(input())
        answer_add = op(x,y)
        if question_add == answer_add:
            print("Well done, this is correct!")
            score = score + 1
            count = count + 1
        else:
            print("Sorry, but this is incorrect.")
            count = count + 1

    elif op == sub:
        print("What is", x, "-", y, "? ")
        question_sub = int(input())
        answer_sub = op(x,y)
        if question_sub == answer_sub:
            print("Well done, this is correct!")
            score = score + 1
            count = count + 1
        else:
            print("Sorry, but this is incorrect.")
            count = count + 1

    elif op == mul:
        print("What is", x, "x", y, "? ")
        question_mul = int(input())
        answer_mul = op(x,y)
        if question_mul == answer_mul:
            print("Well done, this is correct!")
            score = score + 1
            count = count + 1
        else:
            print("Sorry, but this is incorrect.")
            count = count + 1

    if count == 10:
        print("Well done "+name+"! You have completed the quiz. Your final score out of 10 is "+str(score)+".")

I have done it, however, I do not think that this is the most efficient way to write this program. Can this program be condensed down further into fewer lines of code?

Looping

You want to perform an action 10 times. The loop counter never matters for your code, you just need to ask 10 questions. The way you wrote the code, you have:

count = count + 1

in 6 places, all of which are necessary, otherwise you would loop too many times. That is very error prone. If you added support for division, you would have to write it two more times. Good rule of thumb is never repeat yourself!

The best way to loop 10 times in Python, when you don't care about the loop count, is:

for _ in range(10):
    # stuff

No need to worry about incrementing count. Let range() do it for you.

Don't Repeat Yourself

The code for adding, subtracting, and multiplying is the same. The only difference is what operation you do and what operation you print. You mostly already have that refactored. You do use op() to perform the operation. So let's just go one step further:

ops = (('+', add), ('-', sub), ('*',mul))
op_name, op = random.choice(ops)

And then, I'm pretty much copying your logic for add with just one real change (and renaming some variables):

print("What is", x, op_name ,y, "? ")  # op_name, not +
actual = int(input())
expected = op(x,y)
if actual == expected:
    print("Well done, this is correct!")
    score += 1
else:
    print("Sorry, but this is incorrect.")

I also renamed your variables to actual and expected, instead of question and answer - since question didn't really refer to a question. Also += 1 is a more concise way of expressing incrementing by one.

Tell the user the answer?

If the user gets it wrong, do you want to tell them the correct answer?

Finishing up

You don't need to check count (or, now, _) to know when you're done. Just wait for the loop to end, at which point you can print the score. So the flow looks like:

score = 0
for _ in range(10):
    # prompt a problem and potentially increment score

print("Well done "+name+"! You have completed the quiz. Your final score out of 10 is "+str(score)+".")

Though typically you'd express the score as X/10, not out of 10 is X. That just reads funny.


Chaining multiple predicates I have a some filters that chains around 30 Expression<Func<T, bool>> using linq to entities. Currently this is how I am managing them

        ...
        //project filter
        Expression<Func<Project, bool>> projectFilter = FilterEnabled();
        projectFilter = projectFilter.And(GetProjectByOrganization())
             .And(GetProjectByProductLine())
             .And(GetProjectByProjectType())
        ...

        //subproject filter
        Expression<Func<SubProject, bool>> subProjectFilter = FilterEnabled();
        subProjectFilter = subProjectFilter .And(...)
        ...

        //activity filter
        Expression<Func<Activity, bool>> activityFilter = FilterEnabled();
        activityFilter = activityFilter .And(...)
        ...

The problem is .And(Expression<Func<T, bool>>) extension method goes on for another 30 lines or so. How can I manage this another way instead of having to add .And 30+ times for each filter critera. Each filter method looks something like this.

public Expression<Func<Project, bool>> GetProjectByProjectId()
{
    return prj => FilterCriteria.ProjectId == null || prj.ProjectID == FilterCriteria.ProjectId.Value;
}

This is also my predicate builder class where I created the .And()

 public static class PredicateBuilder
    {

        public static Expression<Func<T, bool>> And<T>(this Expression<Func<T, bool>> a, Expression<Func<T, bool>> b)
        {
            ParameterExpression p = a.Parameters[0];

            SubstExpressionVisitor visitor = new SubstExpressionVisitor();
            visitor.subst[b.Parameters[0]] = p;

            Expression body = Expression.And(a.Body, visitor.Visit(b.Body));
            return Expression.Lambda<Func<T, bool>>(body, p);
        }

        public static Expression<Func<T, bool>> Or<T>(this Expression<Func<T, bool>> a, Expression<Func<T, bool>> b)
        {
            ParameterExpression p = a.Parameters[0];

            SubstExpressionVisitor visitor = new SubstExpressionVisitor();
            visitor.subst[b.Parameters[0]] = p;

            Expression body = Expression.Or(a.Body, visitor.Visit(b.Body));
            return Expression.Lambda<Func<T, bool>>(body, p);
        }
    }

    internal class SubstExpressionVisitor : System.Linq.Expressions.ExpressionVisitor
    {
        public Dictionary<Expression, Expression> subst = new Dictionary<Expression, Expression>();

        protected override Expression VisitParameter(ParameterExpression node)
        {
            Expression newValue;
            if (subst.TryGetValue(node, out newValue))
            {
                return newValue;
            }
            return node;
        }
    }

To answer the direct question, the obvious way is to have an All method which takes a params Expression<Func<T, bool>>[] expressions argument. You'll still have a 30-line call, but you won't have And on each line. There are potential side-benefits:

It should reduce the amount of wrapping an expression just to unwrap it immediately.
If you decide that you want AndAlso instead of And (is there any reason for not using it?) then it's much simpler to change.

I would also be tempted to favour a refactor which allows null expressions. If instead of

public Expression<Func<Project, bool>> GetProjectByProjectId()
{
    return prj => FilterCriteria.ProjectId == null || prj.ProjectID == FilterCriteria.ProjectId.Value;
}

you have

public Expression<Func<Project, bool>> GetProjectByProjectId()
{
    return FilterCriteria.ProjectId == null ? null : (prj => prj.ProjectID == FilterCriteria.ProjectId.Value);
}

(with whatever type coercion is necessary to make that compile - I haven't tested it) then you can filter out null expressions in All. This both simplifies the expression constructed and the SQL generated from it, which might improve performance and might also make debugging it easier.

Finally, two comments about SubstExpressionVisitor:

    public Dictionary<Expression, Expression> subst = new Dictionary<Expression, Expression>();

On the basis of coding to the interface rather than the implementation, the type should be IDictionary<Expression, Expression>. I personally would also prefer to hide the field and take the substitutions as arguments to the constructor rather than by modifying a public field after construction, but that's a subjective issue of style. If making that change, the type can become IReadOnlyDictionary<Expression, Expression> instead.

    protected override Expression VisitParameter(ParameterExpression node)
    {
        Expression newValue;
        if (subst.TryGetValue(node, out newValue))
        {
            return newValue;
        }
        return node;
    }

Too many people use Contains and then [key] rather than TryGetValue, so thumbs up for getting that right. I would observe, though, that foo.TryGetValue(key, out val) ? val : defaultVal is a common enough pattern that you could consider factoring it out as an extension method to IDictionary<K,V> and IReadOnlyDictionary<K,V>.


Dynamic Array Problem (Hacker rank) I am trying to solve the Dynamic Array problem on HackerRank:

  Create a list, seqList, of N empty sequences, where each sequence is indexed from 0 to N-1. The elements within each of the N sequences also use 0-indexing.
  Create an integer, lastAnswer, and initialize it to 0.
  The 2 types of queries that can be performed on your list of sequences (seqList) are described below:

  Query: 1 x y

  Find the sequence, seq, at index ((x ^ lastAnswer) % N) in seqList.
  Append integer y to sequence seq.

  Query: 2 x y

  Find the sequence, seq, at index ((x ^ lastAnswer) % N) in seqList.
  Find the value of element y % size in seq (where size is the size of seq) and assign it to lastAnswer.
  Print the new value of lastAnswer on a new line.

However, I am getting a “time out error” for very large inputs.

Below is my code in Swift so far:

func dynamicArray(n: Int, queries: [[Int]]) -> [Int] {
    var sequences: [Int: [Int]] = [:]
    var lastAnswer = 0
    var answerlist = [Int]()
    for query in queries {
        let queryType = query[0]
        let x = query[1]
        let y = query[2]
        switch queryType {
        case 1:
            let seq = (x ^ lastAnswer) % n
            if var sequence = sequences[seq] {
                sequence.append(y)
                sequences[seq] = sequence
            } else {
                sequences[seq] = [y]
            }
        case 2:
            let seq = (x ^ lastAnswer) % n
            let index = y % (sequences[seq])!.count
            lastAnswer = sequences[seq]![index]
            answerlist.append(lastAnswer)
        default: break
        }
    }
    return answerlist
}

Can it be further optimised?

You chose to represent the “list of sequences” as a dictionary

var sequences: [Int: [Int]] = [:]

and here you test if a sequence for the given index already exists, and then either append a new element or create the initial sequence for that index:

if var sequence = sequences[seq] {
    sequence.append(y)
    sequences[seq] = sequence
} else {
    sequences[seq] = [y]
}

That can be greatly simplified by using a subscript with a default value:

sequences[seq, default: []].append(y)

But actually, instead of representing the “list of sequences” as a dictionary I would represent it as an array (of arrays) instead:

var sequences: [[Int]] = Array(repeating: [], count: n)

The number of sequences is a-priori known, so that there is no advantage of using a dictionary. Appending a new element to a sequences (query type 1) then becomes

sequences[seq].append(y)

and for query type 2 we get

lastAnswer = sequences[seq][index]
answerlist.append(lastAnswer)

without the need of forced-unwrapping for the dictionary subscripting.

This should also be more efficient, because (array) index lookups are faster than dictionary lookups.

Some more remarks:

The type of a variable should not be part of the variable name, e.g. answers instead of answerList.
Multiple (related) assignments can be combined to a tuple assignment, e.g.

let (queryType, x, y) = (query[0], query[1], query[2])

We know that the query type can only be 1 or 2, but a fatalError() in the default case helps to find programming errors.

Putting it together, the function could look like this:

func dynamicArray(n: Int, queries: [[Int]]) -> [Int] {
    var sequences: [[Int]] = Array(repeating: [], count: n)
    var lastAnswer = 0
    var answers = [Int]()
    for query in queries {
        let (queryType, x, y) = (query[0], query[1], query[2])
        switch queryType {
        case 1:
            let seq = (x ^ lastAnswer) % n
            sequences[seq].append(y)
        case 2:
            let seq = (x ^ lastAnswer) % n
            let index = y % sequences[seq].count
            lastAnswer = sequences[seq][index]
            answers.append(lastAnswer)
        default:
            fatalError("Bad query type")
        }
    }
    return answers
}

In addition, it seems that most of the time is spent while reading the input data into the queries array, done by the (HackerRank supplied template) code

guard let queriesRowTemp = readLine()?.replacingOccurrences(of: "\\s+$", with: "", options: .regularExpression) else { fatalError("Bad input") }

let queriesRow: [Int] = queriesRowTemp.split(separator: " ").map {
    if let queriesItem = Int($0) {
        return queriesItem
    } else { fatalError("Bad input") }
}

Instead of removing trailing whitespace with a regular expression search we can split the input row and ignore empty components:

guard let queriesRowTemp = readLine() else { fatalError("Bad input") }
let queriesRow: [Int] = queriesRowTemp.split(separator: " ",
                                             omittingEmptySubsequences: true).map {
    if let queriesItem = Int($0) {
        return queriesItem
    } else { fatalError("Bad input") }
}

In my test that cut the time to read 100000 queries down from 3.7 to 1.7 seconds.


isPermutation method I wrote an isPermute method. Can I get some tips and advice on better coding style?

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

bool isPermute(char*, char*);
char* sort(char*);

int main(int argv, char **argc)
{
  printf("%s and %s are %spermutations of each other. \n", argc[1], argc[2], isPermute(argc[1], argc[2]) ?
    "" : "not ");

  return 0;
}

bool isPermute(char* s1, char* s2)
{

  int size1 = strlen(s1);
  int size2 = strlen(s2);

  //default cases
  if((size1 != size2) || (size1 == 0) || (size2==0))
    return false;

  if(strcmp(sort(s1), sort(s2)))
    return false;

  else
    return true;
}

char* sort(char* str1)
{
  int d=0, size = strlen(str1);
  char character;
  char *original = str1;

  char *result = (char *)malloc(size);

  for ( character = 'a' ; character <= 'z' ; character++ )
  {
     int i;
     for ( i = 0 ; i < size ; i++ )
     {
        if ( str1[i] == character )
        {
           result[d] = str1[i];
           d++;
        }
     }
     str1 = original;
  }
  return result;
}

First off:

bool isPermute(char*, char*);
char* sort(char*);

You could just move the function implementation over the main method, avoiding to have these declarations; however, that isn't really a big deal.

The convention for int main isn't:

int main(int argv, char **argc)

It normally is the opposite: argc is for the arg count and argv is for the arguments passed to the program.

So it really should be:

int main(int argc, char **argv)

Next:

printf("%s and %s are %spermutations of each other. \n", argc[1], argc[2],
    isPermute(argc[1], argc[2]) ? "" : "not ");

You never checked if the program was actually passed two parameters, and so instead it should be (argc is the arg count from now on, and argv is the arguments)

if(argc == 3)
{
    printf("%s and %s are %spermutations of each other. \n", argv[1], argv[2],
        isPermute(argv[1], argv[2]) ? "" : "not ");
}
else
{
    printf("Incorrect amount of parameters passed!\n");
    return -1;
}

The following:

if(strcmp(sort(s1), sort(s2)))
    return false;
else
    return true;

can be trimmed down to be just:

return !strcmp(sort(s1), sort(s2));

The following:

char *result = (char *)malloc(size);

You don't need to have the explicit cast (char *) as malloc returns a void*, so it can just be

char *result = malloc(size);

Which doesn't detract from clarity in any real way.

Also, I've noticed that you are declaring variables for the for loops outside the loop, even though that isn't necessary (unless you are using C89, in which case, ignore this)

So this:

for ( character = 'a' ; character <= 'z' ; character++ )
{
     ...
}

would become

for (char character = 'a'; character <= 'z'; character++)
{
    ...
}

The same thing essentially applies for the other for loop.

for ( i = 0 ; i < size ; i++ )
{
    ...
}

would become

for (int i = 0; i < size; i++)
{
    ...
}


Sum of multiples of 3 or 5 using functional programming I wanted to practice functional programming (fp) without using any library. So I took the 1st problem from project euler:

If we list all the natural numbers below 10 that are multiples of 3 or 5, we get 3, 5, 6 and 9. The sum of these multiples is 23.
Find the sum of all the multiples of 3 or 5 below 1000.

My solution in fp looks like this:
const isDivisibleBy = i => number => i % number === 0;
const selected = criteria => (...numbers) => i => numbers.filter(criteria(i))
    .length > 0;
const getSumOf = conditionFor => (sum, _, i) => conditionFor(i) ? sum + i :
    sum;
const solution = Array(1000)
    .fill()
    .reduce(getSumOf(selected(isDivisibleBy)(3, 5)), 0);

I have questions regarding efficiency and parametization:

Is there a more efficient way to write the function selected that is still functional? Because right now the filter functions runs through all elements of numbers - even if it finds an element that is not divisible for the given condition. Also .length > 0 makes it look like a hack. Is there a more declarative way?

I assume getSumOf can't be refactored any more?

This is one of those programming problems where there’s a special trick that can give you a better algorithm, in this case constant-time instead of linear.

If you add the multiples of 3 and the multiples of 5, you’ll count the multiples of 15 twice. So subtract those out. The answer becomes (3+6+9+...+999) + (5+10+15+...+995) - (15+30+45+...+990).

Then, to get all the multiples of m from 1 to N, factor the series m+2·m+... to m· (1 + 2 + ... + ⌊N/m⌋). There’s a simple formula for the series from 1 up to some number, called the Gaussian formula after its discoverer, Carl Friedrich Gauss.  The story goes that he found it as a child, and when his math teacher ordered the whole class to add the numbers from 1 to 100, just to keep them busy so he could take a nap, young Carl shocked him by finding the answer in seconds.  It works on computers, too!  So, substitute this formula into the previous equation to get a constant-time formula for the sum of all multiples of m up to N.  Then substitute that formula into the paragraph above this one, with m = 3, 5 and 15.

Maybe you know all that.  The point of the exercise might have been to practice the specific idioms you used here, like list comprehension and reduction.  Now that you have, you might want to compare an optimal algorithm in functional style.  The last time I posted a better algorithm for a Project Euler problem, someone informed me that the point of Code Review is not to write a better answer myself, so I’ll leave that to you if you feel like it.


Sinewaves and FFT plotter I am trying to code a sin wave with 5 sliders: frequency, amplitude, phase change, number of data points, and time. I am currently trying to link my phase change slider with my graph so that it updates it when I move the slider. I realize that my code is probably quite messy and could be more concise but I can adjust that later.
I think adding a phase change slider and length of time the function is plotted for would be good too, like the frequency and amplitude sliders (which do work).
For those who aren't familiar with the physics terms:
amplitude: height of the wave (in y-axis)

frequency: number of times a wave passes a certain point per unit time. 1 Hertz = 1 wave passing per second
Phase difference: the change in the starting position of the current wave where amplitude = 0 at t = 0. A 90-degree phase change would shift the function the left or right, then representing a cos function rather than sine.

Any advice for my current code would be much appreciated!
My code so far:
import tkinter
import numpy as np
from scipy import signal
from scipy.fft import fft, ifft as sc
from matplotlib import pyplot as p, animation
# Implement the default Matplotlib key bindings.
from matplotlib.backend_bases import key_press_handler
from matplotlib.backends.backend_tkagg import (FigureCanvasTkAgg, NavigationToolbar2Tk)
from matplotlib.figure import Figure
from matplotlib.widgets import Slider

root = tkinter.Tk()
root.wm_title("Embedding in Tk")
fig = Figure(figsize=(6, 5), dpi=100)
t = np.linspace(0, 5, 200)
f = 1
time = np.linspace(0, 100, 101)
y = np.sin(f*t)

fig.subplots_adjust(left=0.15, bottom=0.45)  # Adjust subplots region leaving room for the sliders
and buttons

canvas = FigureCanvasTkAgg(fig, master=root)  # A tk.DrawingArea.
canvas.draw()
canvas.get_tk_widget().pack(side=tkinter.TOP, fill=tkinter.BOTH, expand=1)

toolbar = NavigationToolbar2Tk(canvas, root)
toolbar.update()
canvas.get_tk_widget().pack(side=tkinter.TOP, fill=tkinter.BOTH, expand=1)

def on_key_press(event):
    print("you pressed {}".format(event.key))
    key_press_handler(event, canvas, toolbar)

canvas.mpl_connect("key_press_event", on_key_press)

def _quit():
    root.quit()  # stops mainloop
    root.destroy()

button = tkinter.Button(master=root, text="Quit", command=_quit)
button.pack(side=tkinter.BOTTOM)

# Amplitude slider
initiala = 1  # starting value of amplitude on graph
asliderax = fig.add_axes([0.25, 0.15, 0.65, 0.03])  # setting amplitude slider position
aslider = Slider(asliderax, 'Amp', 0.1, 10.0, valinit=initiala)  # setting amplitude slider values

# Frequency slider
initialf = 1  # starting value of frequency on graph
fsliderax = fig.add_axes([0.25, 0.1, 0.65, 0.03])  # setting frequency slider position
fslider = Slider(fsliderax, 'Freq', 0.1, 3.0, valinit=initialf)  # setting frequency slider values

# Time slider
initialt = 1  # starting value of time on graph
tsliderax = fig.add_axes([0.25, 0.2, 0.65, 0.03])  # setting time slider position
tslider = Slider(tsliderax, 'Time', 0.1, 100.0, valinit=initialt)  # setting time slider values

# Number of Points slider
initialp = 99  # starting value of number of points on graph
psliderax = fig.add_axes([0.25, 0.25, 0.65, 0.03])  # setting points slider position
pslider = Slider(psliderax, 'Points', 3, 30.0, valinit=initialp)  # setting frequency points values

# Phase slider
initialph = 0  # starting value of number of points on graph
phsliderax = fig.add_axes([0.25, 0.3, 0.65, 0.03])  # setting points slider position
phslider = Slider(phsliderax, 'Phase Change', 0, np.pi*2, valinit=initialph)  # setting frequency
points values

def update(val):
    amp = aslider.val
    freq = fslider.val
    t = np.linspace(0, 5, 200)
    y.set_ydata(amp*np.sin(2*np.pi*freq*t))

fslider.on_changed(update)
aslider.on_changed(update)
tslider.on_changed(update)
pslider.on_changed(update)

tkinter.mainloop()

Seems to me you just need to add the phase to the angle in update().
def update(val):
    amp = aslider.val
    freq = fslider.val
    phase = phslider.val
    t = np.linspace(0, 5, 200)

    y.set_ydata(amp*np.sin(2*np.pi*freq*t + phase))  # << add phase here

fslider.on_changed(update)
aslider.on_changed(update)
tslider.on_changed(update)
pslider.on_changed(update)
phslider.on_changed(update)  # update on slider change