Caesar cipher in i686 assembly + MMX I have written the following implementation of the Caesar cipher for an old Linux computer with a Pentium MMX chip.  The code has been written with the following design goals in mind:

the code should perform well
to avoid timing side-channel attacks, the runtime should be independent of the data used
the code should run on a Pentium MMX

Please let me know of possible improvements and performance pitfalls I fell in.

    # MMX caeasar cipher implementation
    # for i586 with MMX
    # signature:
    # caesar(out, in, len, key)
    # key is between 0 and 25

    .section .text
    .globl caesar
    .type caesar,@function
    .align 16
caesar:
    push %ebp
    mov %esp,%ebp
    push %ebx
    push %edi
    push %esi

    mov 8(%ebp),%edi        # edi: destination
    mov 12(%ebp),%esi       # esi: source
    mov 16(%ebp),%edx       # edx: length
    and $~7,%edx            # only process full qwords
    test %edx,%edx
    jz 1f

    xor %ecx,%ecx           # ecx: index
    movd 20(%ebp),%mm5
    punpcklbw %mm5,%mm5
    punpcklwd %mm5,%mm5
    punpckldq %mm5,%mm5     # mm5: key bytes

    movq %mm5,%mm4
    psubb twentysix,%mm4        # mm4: key - 26

    movq Amask,%mm6
    psubb %mm4,%mm6         # mm6: 'A' - 1 - (key - 26)

    .align 16
0:  movq (%esi,%ecx),%mm0
    movq %mm0,%mm1
    pand ucmask,%mm1        # mm1: xmm0 in upper case

    movq %mm1,%mm3
    movq %mm1,%mm2

    pcmpgtb Amask,%mm2      # mm2: 0xff where 'A' <= buf[i]
    pcmpgtb Zmask,%mm3      # mm3: 0xff where 'Z' < buf[i]
    pcmpgtb %mm6,%mm1       # mm1: 0xff where 'A' + key <= buf[i]

    pandn %mm1,%mm3
    pand %mm4,%mm3          # mm3: key-26 where 'A' + (26 - key) <= buf[i] <= 'Z'

    pandn %mm2,%mm1
    pand %mm5,%mm1          # mm1: key where 'A' <= buf[i] < 'A' + (26 - key)

    por %mm3,%mm1           # mm1: 0/key/key-26
    paddb %mm1,%mm0
    movq %mm0,(%edi,%ecx)

    add $8,%ecx
    cmp %edx,%ecx
    jb 0b
    emms
    add %ecx,%edi
    add %ecx,%esi

1:  mov 16(%ebp),%edx       # length
    mov 20(%ebp),%ecx       # key
    and $7,%edx         # loop tail

    # process remaining bytes
    add %edx,%edi       # end of output buffer
    add %edx,%esi       # end of input buffer
    neg %edx        # index counts up to 0

    lea -26(%ecx),%eax  # al: key - 26
    mov $'A'+26-1,%ah
    sub %cl,%ah     # ah: threshold ('A' - 1 - (key - 26))

    .align 16
0:  mov (%esi,%edx),%bl
    and $~0x20,%bl      # bl: uppercase c
    cmp $'Z'+1,%bl      # cf: uc <= 'Z'
    sbb %ch,%ch     # ch: 0xff if uc <= 'Z'
    cmp %bl,%ah     # cf: thresh < uc
    sbb %bh,%bh     # bh: 0xff if thresh < uc
    and %bh,%ch
    and %al,%ch     # ch: key - 26 & isleZ & isgttthres
    not %bh         # bh: ~isgtthresh
    cmp $'A',%bl
    cmc         # cf: 'A' <= uc
    sbb %bl,%bl     # bl: 0xff if 'A' <= uc
    and %bh,%bl
    and %cl,%bl     # bl: key & isgeA & ~isgtthresh
    or %ch,%bl
    add (%esi,%edx),%bl
    mov %bl,(%edi,%edx)
    inc %edx
    jnz 0b

1:  pop %esi
    pop %edi
    pop %ebx
    leave
    ret
    .size caesar,.-caesar

    .section .rodata.cst8,"aM",@progbits,8
    .align 8
    .type ucmask,@object
ucmask: .fill 8, 1, ~0x20
    .size ucmask, 8
    .type Amask,@object
Amask:  .fill 8, 1, 'A' - 1
    .size Amask, 8
    .type Zmask,@object
Zmask:  .fill 8, 1, 'Z'
    .size Zmask, 8
    .type twentysix,@object
twentysix:
    .fill 8, 1, 26
    .size twentysix, 8

You can use this harness to test the code:

#define _POSIX_C_SOURCE 200809L
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

extern void caesar(const char *out, char *in, size_t len, int key);

extern int
main(int argc, char *argv[])
{
    char *buf = NULL;
    size_t len = 0;
    ssize_t count;
    int key = 13;

    if (argc > 1)
        key = atoi(argv[1]) % 26;

    while (count = getline(&buf, &len, stdin), count != EOF) {
        caesar(buf, buf, strlen(buf), key);
        fputs(buf, stdout);
    }

    return (EXIT_SUCCESS);
}

A big danger.

When the len parameter happens to be a muliple of 8 and all of the full qwords have been processed, there will be no remaining bytes to process.
Your code calculates the number of remaining bytes using

mov 16(%ebp),%edx     # length
...
and $7,%edx           # loop tail

but forgets to exit if this produces zero. The loop that follows is started anyway and at the bottom where it reads

inc %edx
jnz 0b

the increment of %edx will keep producing NZ for a very long time!

Resolve this by exiting if the and instruction sets ZF=1.

mov 16(%ebp), %edx    # length
...
and $7, %edx          # loop tail
jz  1f

A small optimization

and $~7,%edx        # only process full qwords
test %edx,%edx
jz 1f

Because the and instruction defines the zero flag (ZF), there's no need to write test %edx, %edx.

Allow me

mov $'A'+26-1,%ah

This struck me as being a bit too much of a complication. Why not simply write

mov $'Z', %ah

Now if it was your intent to come close to the comment that follows in the next line
# ah: threshold ('A' - 1 - (key - 26))
then writing

lea -26(%ecx), %eax   # al: key - 26
mov $'A'-1, %ah
sub %al, %ah          # ah: threshold ('A' - 1 - (key - 26))

would have nailed it.


Basic timer from one minute to 0 I'm a beginner in JavaScript and I am sure that there is a better way to do it (basic one minute timer from 01:00 to 00:00 running from pageload):

JSFiddle

window.onload=function(){
(function(){
     setInterval(function(){
     cro();
     document.getElementById('unminut').innerHTML="0"+jocminut+":"+jocsecunda1+jocsecunda2;
    },1000);
    }());
}
var jocminut = 1;
var jocsecunda1 = 6;
var jocsecunda2 = 10;
function cro(){
    if(jocsecunda1!=0||jocsecunda2!=0){
        jocsecunda2 -=1;
        while(jocminut==1){
            jocminut-=1;
            jocsecunda1 -=1;
        }
            while(jocsecunda2==-1){
                jocsecunda1-=1;
                jocsecunda2=9;

            }

    }
}

You want to be careful with setInterval() and setTimeout(), because javascript runs in only a single thread if the event loop is busy it won't fire the code in setInterval() or setTimeout() exactly as expected.  This means your timer could take longer than you expected.

Instead of starting with a counter and decrementing on each interval, you can calculate a time difference and then use setInterval() to refresh the frontend.  This will be more accurate.

The following code calculates a completion date and then can calculate how many seconds are remaining.

    //sets a completion date 60 seconds into the future
    // Date.getTime() returns the time in milliseconds
    var completion = new Date(new Date().getTime() + 60000),
    function calculateSecondsRemaining() {
        var now = new Date(),
            //Math.max is used to prevent negative numbers from being returned
            differenceInMilliseconds = Math.max(0, completion.getTime() - now.getTime()),
            differenceInSeconds = Math.floor(differenceInMilliseconds / MILLISECONDS_IN_SECOND);
        return differenceInSeconds;
    }

calculateSecondsRemaining() handles the complex logic that was once in cro(), but I feel that is is more readable.  When dealing with time and numbers, tracking each digit can be tedious.  I prefer to not deal with it at that level.

I prefer to split logic into multiple functions so that the code is more readable.  In your code what jumped out at me is three different bits of logic, calculateSecondsRemaining(), updateDisplay(), and formatNumberTo2Places().  By spliting the functions up, the logic within your setInterval() is very small and very easy to understand.

As a final note, once your timer hits zero, the interval no longer needs to fire.  The following code cancel the interval using clearInterval() once the timer has reached zero.

  var refreshIntervalId = setInterval(function () {
        var secondsRemaining = calculateSecondsRemaining();
        updateDisplay(secondsRemaining);
        if (secondsRemaining <= 0) {
            //clear timer once time expired
            clearInterval(refreshIntervalId);
        }
    }, 1000);

All Together

The following is wrapped in a self-invoking anonymous function to prevent spilling code into the global scope.

(function (window, document, undefined) {
    var MILLISECONDS_IN_SECOND = 1000,
        SECONDS_IN_MINUTE = 60,
        timerLength = 60, //seconds
        start = new Date(),
        completion = new Date(start.getTime() + (timerLength * MILLISECONDS_IN_SECOND)),
        pentruminut = document.createElement('span');

    // add elements
    document.body.appendChild(pentruminut);

    function formatNumberTo2Places(n) {
        //this assumes 1 or 2 digit positive numbers
        var buffer = (n > 9) ? "" : "0";
        return buffer + n;
    }

    function calculateSecondsRemaining() {
        var now = new Date(),
            //Math.max is used to prevent negative numbers from being returned
            differenceInMilliseconds = Math.max(0, completion.getTime() - now.getTime()),
            differenceInSeconds = Math.floor(differenceInMilliseconds / MILLISECONDS_IN_SECOND);
        return differenceInSeconds;
    }

    function updateDisplay(secondsRemaining) {
        var minutes = Math.floor(secondsRemaining / SECONDS_IN_MINUTE),
            seconds = Math.floor(secondsRemaining % SECONDS_IN_MINUTE);
        pentruminut.innerHTML = formatNumberTo2Places(minutes) + ":" + formatNumberTo2Places(seconds);
    }

    window.onload = function () {
        var refreshIntervalId = setInterval(function () {
            var secondsRemaining = calculateSecondsRemaining();
            updateDisplay(secondsRemaining);
            if (secondsRemaining <= 0) {
                //clear timer once time expired
                clearInterval(refreshIntervalId);
            }
        }, 1 * MILLISECONDS_IN_SECOND);
    };
}(window, document));


Initializing multiple variables with different types using parallel calls Consider the pattern below, where multiple async calls are made in parallel:

var thisTask = ThisAsync();
var thatTask = ThatAsync();

await Task.WhenAll(thisTask, thatTask);

var this = await thisTask;
var that = await thatTask;

While it works fine, I find it a bit too verbose for what it is doing.

While having some fun with tuple types, I came up with the following approach that results in the same behavior:

var (this, that) = await (ThisAsync(), ThatAsync()).ResultsAsync();

It relies on an extension method on a tuple of Tasks, like below:

static class ParallelTasksExtensions
{
    static async Task<(T1, T2)> ResultsAsync<T1, T2>(this (Task<T1>, Task<T2>) tasks)
    {
        await Task.WhenAll(tasks.Item1, tasks.Item2);

        return (await tasks.Item1, await tasks.Item2);
    }
}

With a couple overloads (wish C# supported variadic generic types like C++ does in templates) this can be extended to an arbitrary number of parallel calls, so that we can have something like this:

var (one, two, three, four) = await (OneAsync(), TwoAsync(), ThreeAsync(), FourAsync()).ResultsAsync();

I like it quite a bit because of the following reasons:

Is is dramatically shorter. Usually shorter code leads to less chance of problems (not a golden rule of course, but I feel it applies here)
It requires less state to be maintained (less variables)
It avoids people calling .Result instead of await on the original tasks (a common bad practice)

Another option that would result in the same behavior would be to create an extension class instead of coding this as an extension method.

For example:

var (this, that) = await TaskEx.WhenAll((ThisAsync(), ThatAsync()));

This approach would be more inline with existing, non-tuple based overloads of Task.WhenAll.

Thoughts?

Is it "too clever" in your opinion? I don't think there is any similar native construct that achieves this so it could be seen as clever by some I'm sure.
Do you consider this over-engineering?
Is it more readable/intuitive to you?
Do you have other suggestions for the method name?

I've been doing something similar to your await Task.WhenAll, though not often enough to think about a shorter way of doing it. However, I've recently come to doubt the necessity of it.

Note that there is almost zero difference in the effective behavior of this code:

var t1 = SomethingAsync();
var t2 = OtherAsync();
await Task.WhenAll(t1, t2);
var r1 = await t1;
var r2 = await t2;

and this code:

var t1 = SomethingAsync();
var t2 = OtherAsync();
var r1 = await t1;
var r2 = await t2;

In both cases, the tasks are started. In both cases, the tasks run in the background as much as possible. In both cases, the method won't finish until both tasks are done, assuming they succeed.

If only task t1 fails, both cases will throw its exception. The first version will first wait for task t2 to finish, the second version will not. This may make a difference, but usually won't.

If only task t2 fails, both cases will throw its exception, although the second version will execute a little more code first; this difference is very unlikely to make a practical difference.

If both tasks fail, both cases will throw the exception of t1 and swallow the one of t2, see here: https://blogs.msdn.microsoft.com/seteplia/2018/02/14/one-user-scenario-to-rule-them-all/ down at "Exception Handling".

So if I don't mind the swallowed exceptions, I would simply omit the WhenAll completely, which makes your utility method redundant.

That said, it would make a good place to attach code that prevents the swallowing of the exception, if that is what you want. Given that, I like your idea, but it can be improved: you can make a tuple of tasks directly awaitable. The GetAwaiter method that the compiler implicitly calls may be an extension method. That would result in this code:

static class ParallelTasksExtensions
{
    static TaskAwaiter<(T1, T2)> GetAwaiter<T1, T2>(this (Task<T1>, Task<T2>) tasks)
    {
        return AwaitAndPackage().GetAwaiter();

        async Task<(T1, T2)> AwaitAndPackage()
        {
            await Task.WhenAll(tasks.Item1, tasks.Item2);
            return (await tasks.Item1, await tasks.Item2);
        }
    }
}

(Warning: untested.)

Now you can just write await (t1, t2) directly. You can then proceed to overload for more arguments, of course.

Or instead you can install the TaskTupleAwaiter NuGet package, which contains exactly this code already. See also the enhancement request that led to this.


LREM command for redis-like plugin I had some difficulties implementing a redis-like plugin basil.js, particularly the LREM method. The complexity was to loop only once (and not the whole array every time if we have 0 !== count) ascending or descending. My code below works and passes tests, but I was wondering if another smarter implementation (fewer intermediary vars) was possible.

Here is the whole source.

lrem: function (key, count, value) {
    if ('undefined' === typeof count || 'undefined' === typeof value)
        throw new Error('ERR wrong number of arguments for \'lrem\' command');

    var list = this.get(key) || [];

    if (0 === list.length)
        return 0;

    // added here for Basil, even if objects are supported
    // we to not support (yet?) object comparison, too heavy
    if ('number' !== typeof value && 'string' !== typeof value)
        throw new Error('ERR syntax error');

    var index = count >= 0 ? 0 : list.length - 1,
        length = list.length,
        iteration = 0,
        removed = 0;

    // iterate on whole list or stop if we found exactly the right count occurences
    while (iteration < length && (0 === count || (0 !== count && removed !== Math.abs(count)))) {

        if (value === list[index]) {
            list.splice(index, 1);
            removed++;

            // we removed an element, we need to decrease index if we are
            // looping from 0 to length otherwise we'll skip some values..
            if (count >= 0)
                index--;
        }

        iteration++;
        count >= 0 ? index++ : index--;
    }

    if (0 === list.length)
        this.remove(key);
    else
        this.set(key, list);

    return removed;
}

First some style notes: The "Yoda notation" is rare to see in JavaScript. Sure, it works, but you won't find many people using it, and just for that reason I'd recommend not using it, but it's your call. Just a heads-up in case others have to work on this code too.

I'd also recommend always using braces, even when they're not strictly necessary. Again, my recommendation, based on what I consider best practices.

In terms of functionality, it appears to match the docs for redis' LREM exactly - nice. Still I see some areas of improvement.

I don't see any need to avoid declaring local variables, though. You code seems just fine as-is on that point. Variables aren't a horrible performance drain or anything like that; there's no reason to avoid them. On the contrary, if you think adding a local variable will make the code more readable, then it's worth it.

Anyway, code review. First a simple thing: Use double quotes, and you don't have to escape single quotes in the string

throw new Error("Wrong number of arguments for 'lrem' command");

For that particular error, it's also easier to simply check arguments.length, and you could make it more informative - what is the correct number of arguments? (see below for an example of what I mean)

I've also removed the "ERR" prefix. It's already an Error object you're throwing, so it's kinda redundant.

Next, you can skip the || [] fallback, and instead do:

var list = this.get(key);

// if list is falsy or empty, or count is zero
if (!list || !list.length || !count) {
  return 0;
}

Next, another error. I've merely changed the wording a little, since it's not a syntax error; it's a semantic error. Besides, if it actually were a syntax error, you ought to be throwing a SyntaxError object. Here however, what we really want is a TypeError

if ('number' !== typeof value && 'string' !== typeof value) {
    throw new TypeError("'value' must be a string or a number");
}

But it might be simpler just to do all these checks upfront, instead of doing it in two different parts of the functions:

if(arguments.length < 3) {
  throw new Error("Wrong number of arguments for 'lrem' function (" + arguments.length + " for 3)");
}

if(typeof value !== 'number' && typeof value !== 'string') {
  throw new TypeError("'value' must be a string or a number");
}

And you might want to check the count argument's type too, while you're at it, but I'll leave that to you.

As for the actual meat of the function, I'd suggest simply using indexOf and lastIndexOf for simplicity.

var limit = Math.abs(count),
    removed = 0,
    indexOf = count < 0 ? list.lastIndexOf : list.indexOf,
    i;

while( removed < limit && (i = indexOf.call(list, value)) !== -1 ) {
  list.splice(i, 1);
  removed++;
}

if(!list.length) {
  this.remove(key);
}

return removed;

I've left out the this.set(key, list) line because I'm not sure it's necessary. While I'm not 100% sure (I just quickly skimmed the code on github), it seems that get returns a reference to the stored array, and, if so, you're removing items directly on that stored array already. No need to set it again.

Of course, if I'm wrong, just add the else-line back in.

All in one block:

lrem: function (key, count, value) {
  if(arguments.length < 3) {
    throw new Error("Wrong number of arguments for 'lrem' function (" + arguments.length + " for 3)");
  }

  if(typeof value !== 'number' && typeof value !== 'string') {
    throw new TypeError("'value' must be a string or a number");
  }

  // TODO: check 'count' type?

  var list = this.get(key);

  if (!list || !list.length || count == 0) {
    return 0;
  }

  var limit = Math.abs(count),
      removed = 0,
      indexOf = count < 0 ? list.lastIndexOf : list.indexOf,
      i;

  while( removed < limit && (i = indexOf.call(list, value)) !== -1 ) {
    list.splice(i, 1);
    removed++;
  }

  if(!list.length) {
    this.remove(key); // remove list entirely, if it's empty
  }

  return removed;
}


Calculate the arclength I am trying to estimate a good approximation for the arclength of the curve \$ y = \sqrt{1-x^2}\$ for \$x \in [0, 1]\$. I did this using a Bezièr curve, and made it so that the area under the two curves are alike. In order to estimate the error I want to calculate the differences in arclength. The arclength of a quarter circle is \$\frac14\pi\$ so it boils down to estimating

$$\mathrm{Difference}= \frac{L - \frac12\pi}{\frac12\pi}$$

Where \$L\$ is the arclength of my Bezièr curve. The problem, or rather my question is how to find a good method for estimating this arclength.

I tried using recursion and the Pythagorean theorem. I calculate the distance between \$P\$ and \$Q\$. Then I calculate the distances \$PR\$ and \$QR\$, where \$R\$ lies between \$P\$ and \$Q\$. If \$\mathrm{abs}( d(PR) + d(QR) - d(PQ)) < \mathrm{Error}\$ I return the length \$d(PR) + d(QR)\$.

This is painfully slow. Setting digits = 15 takes about 30 seconds on my computer and digits = 16 takes about a minute.

Have I implemented this method incorrectly, or in a highly inefficient way?

from decimal import *
import decimal
from math import pi
getcontext().prec = 100

nu = 2-(66-15*pi)**0.5/float(3)

def bezier(t):
    f = (1-t)**3 + 3*(1-t)**2*t + 3*(1-t)*t**2*nu
    g = 3*(1-t)**2*t*nu + 3*(1-t)*t**2 + t**3
    return (f, g)

def arc_length(start_t = 0, stop_t = 1, error = 10**(-6), length = 0):
    mean_t = 0.5*(start_t + stop_t)
    P = bezier(start_t)
    Q = bezier(mean_t)
    R = bezier(stop_t)

    length_one = decimal.Decimal(pyt(P, Q))
    length_two = decimal.Decimal(pyt(Q, R))
    if abs(length_one + length_two - length) < error:
        return length_one + length_two
    else:
        return arc_length(start_t, mean_t, error, length_one) + arc_length(mean_t, stop_t, error, length_two)

def pyt(P, Q):
    return ((P[0] - Q[0])**2 + (P[1] - Q[1])**2)**0.5

if __name__ == '__main__':

    digits = 5
    error = 10**(-digits)
    t1 = 0
    t2 = 1
    length = arc_length(0, 1, error)
    print length
    diff = (length - decimal.Decimal(pi)/decimal.Decimal(2))/(decimal.Decimal(pi)/decimal.Decimal(2))
    print diff

weird function name pyth

Did you mean pythagoras? Oh, no, you meant distance.

what does bezier do?

What are f,g? It looks like it returns two values based on t, but it is not clear to me what the expressions signify. How did you get those expressions?

It probably derives somehow from f(x) = sqrt(1-x^2) and its derivatives.

Maybe it would help if you could show us what the code looks like for the arclength of the arc formed by f(x) = 1 between -1 and 1 (it should return 2).

arc length algorithm

Also, the arc-length runs the risk of running into a recursion-error. And it's slow due to function calls.

Suggested implementation (typed on phone, untested):

def arclength(f, low, high, error):
    p_low, p_high = f(low), f(high)
    length = 0
    todo = [(low, high, p_low, p_high, distance(p_low, p_high))]

    while todo:
        segment = todo.pop()
        middle = (segment[0] + segment[1])/2
        p_middle = f(middle)
        d_left = distance(p_middle, segment[2])
        d_right = distance(p_middle, segment[3])

        if abs(d_left + d_right - segment[4]) < error:
            length += d_left + d_right
        else:
            todo.append((low, middle, p_low, p_middle, d_left))
            todo.append((middle, high, p_middle, p_high, d_right))

    return length

This has the following invariants on todo

all tuples are of the form (t0, t1, f(t0), f(t1), distance(f(t0), f(t1))).
at each iteration, a segment is split in two
if the change in value from splitting up is small enough, the resulting length is added to length
else: the separate segments are both added back to todo.

(The f is in this case bezier, this allows you to replace it with other curve-defining functions like (cos(t), sin(t)))


Linq query performance that use ToList() This code written by @Rahul Singh in this post:

var result = _dbContext.ExtensionsCategories.ToList().GroupBy(x => x.Category)
                .Select(x =>
                    {
                        var files = _dbContext.FileLists.Count(f => x.Select(z => z.Extension).Contains(f.Extension));
                        return new
                        {
                            Category = x.Key,
                            TotalFileCount = files
                        };
                    });

This code has a problem when used inside database context and we should use ToList() like this to fix "Only primitive types or enumeration types are supported in this context" error:

 var result = _dbContext.ExtensionsCategories.ToList().GroupBy(x => x.Category)

The problem of this is that ToList() fetches all records and reduces performance. Now I wrote my own code:

    var categoriesByExtensionFileCount =
        _dbContext.ExtensionsCategories.Select(
            ec =>
                new
                {
                    Category = ec.Category,
                    TotalSize = _dbContext.FileLists.Count(w => w.Extension == ec.Extension)
                });

    var categoriesTotalFileCount =
        categoriesByExtensionFileCount.Select(
            se =>
                new
                {
                    se.Category,
                    TotalCount =
                        categoriesByExtensionFileCount.Where(w => w.Category == se.Category).Sum(su => su.TotalSize)
                }).GroupBy(x => x.Category).Select(y => y.FirstOrDefault());

The performance of this code is better but it has many lines of code. Any idea about improving the performance of the latter, or reducing the length of the former?

You can indeed improve the query by changing the strategy.

If you first GroupBy file extension and Count them inside the GroupBy element selector you can remove ToList and the query will be executed on the server and it won't be necessary to download all the data to group it later on your machine. You will then only get a few groups and their number of elements.

Now for the few file extensions it's very easy and quick to get the category names:

var result =
    Files
    .GroupBy(x => x.Extension, (ext, fs) => new
    {
        Extension = ext,
        Category = Categories.Single(c => c.Extension == ext).Name,
        FileCount = fs.Count()
    })
    // the second GroupBy sums categories with multiple file extensions like pdf, doc etc.
    .GroupBy(x => x.Category, (cat, counts) => new
    {
        Category = cat,
        FileCount = counts.Sum(y => y.FileCount)
    });

This will produce the following query:

SELECT SUM([t3].[value2]) AS [FileCount], [t3].[value] AS [Category]
FROM (
    SELECT (
        SELECT [t2].[Name]
        FROM [Category] AS [t2]
        WHERE [t2].[Extension] = [t1].[Extension]
        ) AS [value], [t1].[value] AS [value2]
    FROM (
        SELECT COUNT(*) AS [value], [t0].[Extension]
        FROM [File] AS [t0]
        GROUP BY [t0].[Extension]
        ) AS [t1]
    ) AS [t3]
GROUP BY [t3].[value]

I've tested it with 1.000.000 rows:

and compared with the ToList version:

In the last rows of the statistics you can see that short version is about 3 times faster then the original one.


Consolidating a dictionary from nested dictionary Currently learning from "LPTHW - Zed Shaw" and "Automating the boring stuff".

There is a nested dictionary of the type {'string1': {inner dict 1}, 'string2':{inner dict 2},...}.

I want a non nested consolidated dictionary of all the keys and values from all of the {inner dicts}. I have written the code below, but I am not convinced it is the most efficient way. I am looking for suggestions on how the code can be simplified by utilising logic within a single loop or by making use of some other modules and methods.

Is that new method efficient from a processing point of view?

allGuests = {'Alice': {'apples': 5, 'pretzels': 12},
           'Bob': {'ham sandwiches': 3, 'apples': 2},
           'Carol': {'cups': 3, 'apple pies': 1}}

item_list = []
count_list = []
consolidated = {}
all_items_list = list(allGuests.values()) #This is a list of sub dictionaries

#Loop for the number of guests who brought items
for guest in range(0, len(allGuests)):

#Creating 2 separate lists for 'items' and 'their count'

    item_list = item_list + list(all_items_list[guest].keys())
    count_list = count_list + list(all_items_list[guest].values())

#Creating a consolidated dictionary
for index in range(0, len(item_list)):

    if not consolidated.get(item_list[index]):
        consolidated.setdefault(item_list[index], count_list[index])

    else:
        consolidated[item_list[index]] = consolidated[item_list[index]] + count_list[index]

    pprint.pprint(consolidated)

Expected output:

{'apple pies': 1, 'apples': 7, 'cups': 3, 'ham sandwiches': 3, 'pretzels': 12}

If your dictionary always has the structure {key: {key: int}}, i.e. the values of the inner dictionaries are all integers, this can be easily achieved using collections.Counter.update:

from collections import Counter

def consolidate(d):
    consolidated = Counter()
    for sub_dict in d.values():
        consolidated.update(sub_dict)
    return dict(consolidated)

if __name__ == "__main__":
    all_guests = {'Alice': {'apples': 5, 'pretzels': 12},
                  'Bob': {'ham sandwiches': 3, 'apples': 2},
                  'Carol': {'cups': 3, 'apple pies': 1}}
    print(consolidate(all_guests))
    # {'apple pies': 1, 'apples': 7, 'cups': 3, 'ham sandwiches': 3, 'pretzels': 12}

Note that Python has an official style-guide, PEP8, that recommends using lower_case both for functions and for variables. I also put this code into a function, so it is reusable, and only called it with the test case under a if __name__ == "__main__": guard to allow importing from this module without executing the test case.


Implementation of quick sort in C# I'm currently learning programming for the first time and I've created the following implementation of quick sort in C#. I basically read a very basic overview of how quick sort works recursively and implemented it without looking at any concrete examples of its implementation so that I could learn the most from it.

Could you let me know what you think and where it could be optimized?

class Program
{
    static void Main(string[] args)
    {
        var input = new int[] {  10, 3, 20, 5, 200 };
        QuickSort(0, 0, input.Length-1, input);
        Console.WriteLine("Sorted output: :");
        new List<int>(input).ForEach(x => Console.Write(x + ","));
        Console.ReadLine();
    }

    /// <summary>
    /// sample input: 5 7 3 9 2
    /// same output: 2 3 5 7 9
    /// </summary>
    /// <param name="numbers"></param>
    public static void QuickSort(int pivotIndex, int startIndex, int endIndex, int[] numbers)
    {
        if (startIndex == endIndex) return;

        int pivotNumber = numbers[pivotIndex];

        var lowerThanPivot = new List<int>();
        var higherThanPivot = new List<int>();

        for(int i = startIndex; i <= endIndex; i++)
        {
            if(i != pivotIndex)
            {
                if( numbers[i] <= pivotNumber )
                    lowerThanPivot.Add(numbers[i]);
                else
                    higherThanPivot.Add(numbers[i]);
            }
        }

        // now lets put the pivot number in the correct place
        numbers[startIndex + lowerThanPivot.Count] = pivotNumber;

        // reinsert the numbers lower than pivot in the right place
        int z = startIndex;
        lowerThanPivot.ForEach(x => numbers[z++] = x);

        z++; // *** skip over the new pivot index ***

        // reinsert the numbers higher than pivot in the right place
        higherThanPivot.ForEach(x => numbers[z++] = x);

        // recursively call to sort each remaining elements

        int newLeftPivot = startIndex;
        int newEndLeftIndex = newLeftPivot + lowerThanPivot.Count - 1 ;

        int newRightPivot = lowerThanPivot.Count + 1;
        int newEndRightIndex = newRightPivot + higherThanPivot.Count - 1;

        if(lowerThanPivot.Count> 0)
            QuickSort(newLeftPivot, newLeftPivot, newEndLeftIndex, numbers);

        if(higherThanPivot.Count > 0)
            QuickSort(newRightPivot, newRightPivot, newEndRightIndex, numbers);

    }
}

new List<int>(input).ForEach(x => Console.Write(x + ","));

I see absolutely no reason to do this. In general, List.ForEach() is considered bad form by some and you should use normal foreach instead.

/// <summary>
/// sample input: 5 7 3 9 2
/// same output: 2 3 5 7 9
/// </summary>
/// <param name="numbers"></param>

This is not a very good documentation comment. The comment should describe in words what your code does. Example input and output might help to clarify that further, but I think that's not necessary for sorting.

Also, you should either describe your parameters using <param>, or don't use it (if you think the meaning of the parameters is clear). But empty <param> is not useful.

var lowerThanPivot = new List<int>();
var higherThanPivot = new List<int>();

You don't need these two lists. QuickSort can be implemented as an in-place algorithm. That means you don't need any additional collections to use it.

The way this is done is that you iterate the array from the start and at the same time from the end, switching elements between the two positions if the left one is larger than the pivot and the right one is smaller.

This will make your code somewhat more complicated, but it also potentially saves a lot memory.

int newLeftPivot = startIndex;

int newRightPivot = lowerThanPivot.Count + 1;

This is not a very good choice for a pivot. If the array is already sorted (or mostly sorted, with often isn't that uncommon), then your code will run in O(n^2) time, which will make it very slow on large inputs. You should consider a better way to choose the pivot.


Boolean enums: improved clarity or just overkill? Suppose we are writing a GUI toolkit in C++ (though this question may also apply to other languages). We have a button class with a member function hide, which hides the button. This member function takes a Boolean parameter animated to control if the button should be hidden with an animation.

class Button {
public:
  // Rule of three, etc…

  void hide(bool animated);
};

When invoking this member function, it may not be clear what it does.

Button button;
button.hide(false); // well, does it hide the button or not?
                    // what does "false" even mean here?!

We could rewrite this using a Boolean enum.

class Button {
public:
  // Rule of three, etc…

  enum Animated : bool {
    Animate = true,
    DoNotAnimate = false,
  };
  void hide(Animated animated);
};

Now if we call it, everything becomes more clear.

Button button;
button.hide(Button::DoNotAnimate);

Is this a good thing to do? Does it improve clarity of the code, or is it just overkill and should we use separate documentation (Doxygen-like) for this instead?

I think the enum is a very nice solution here. And in a way I disagree with Johannes, even for single-use the enum improves readability and discoverability of the API, and writing it is a negligible effort; and I’d be wary of using comments as in his example, they scream “hack”.


Check if three arrays contains the same element Do 3 arrays contain a common element, if they contain output it.
For example: [1,2,3], [2,3], [2,4] -> answer = 2

let arr1 = [1, 3, 11, 32, 44, 99];
let arr2 = [4, 12, 15, 99];
let arr3 = [4, 11, 13, 15, 23, 43];

function searchThreeSameNum(arr1, arr2, arr3) {
    let i = 0;
    let j = 0;

    while (1) {
        if (i == arr1.length || j == arr2.length) {
            return 'No equal numbers';
        }

        if (arr1[i] < arr2[j]) {
            i++;
            continue;
        } else if (arr1[i] > arr2[j]) {
            j++;
            continue;
        } else if (arr1[i] == arr2[j])
            for (let k = 0; k < arr3.length; k++) {
                if (arr1[i] == arr3[k])
                    return arr1[i];
            }

        return 'No equal numbers';
    }
}

I will be glad if you give me any tips to improve my code. Thanks in advance.
Sorry, I'm not an English speaker.

Your code assumes that each of the 3 arrays is sorted. Otherwise the < operator would not work. It's ok to assume this. You should have mentioned this in your question.

You use the == operator for comparing the numbers and the lengths. You should better use the === since the == operator considers 0 and "0" equal, which is not good in most cases.

It does not matter which of the 3 arrays comes first. The result will always be the same. Therefore it would be nice if the code looked the same for each of the 3 arrays. Your current code looks different for arr3.

I would write the code differently:

function smallestCommonElement(a, b, c) {
    let i = 0, j = 0, k = 0;

    while (i < a.length && j < b.length && k < c.length) {
        const max = Math.max(a[i], b[j], c[k]);

        let same = true;
        while (i < a.length && a[i] < max) i++, same = false;
        while (j < b.length && b[j] < max) j++, same = false;
        while (k < c.length && c[k] < max) k++, same = false;

        if (same)
            return a[i];
    }
    return null;
}

The idea is to start at the beginning of the arrays. In each step, look at the current values and find the maximum number. Advance each array to this maximum number. If none of the 3 arrays has been advanced, this means that the current values from all the arrays must be the same. In that case, return this value. Otherwise the values must be different, so try again. Do all this until one of the arrays is at the end, in which case there is no common element.

Looking again at your code, there is a bug. Given the arrays [2, 3], [2, 3], [3], your code will return 'No equal number' even though the 3 appears in each array. Using a debugger (or pen and paper), you should step through your code to see where the bug is.

It's an edge case, and it happens in the part of the code that differs from the other parts. That's why I suggested that the code for all 3 arrays should look the same. It's one less chance of introducing bugs.


Naive Bayes classifier for OKCupid profiles I tried to write a naive Bayes classifier to classify OkCupid profiles, and I was wondering if you could give me feedback on my code.  The classifier performs no better than chance and the coding style is probably not great either.  I had a training set of 20 good profiles and 20 bad profiles, and a test set of 16 good and 29 bad, so I don't know if I am classifying poorly or if I just don't have enough data.

#!/usr/bin/python

# Naive Bayes classifier for OkCupid data.  The features are the words in
# the profile and the classes are HIGH and LOW.  This is used to
# predict the rating I would give a profile.

# TODO: tweak the classifier so it works better.

from subprocess import call
from math import sqrt

# File containing training data in the form "<rating> <username>"
TRAINING_DATA = "train.dta"

# File containing test set in the form "<rating> <username>"
TEST_DATA = "test.dta"

# Dictionary
dictionary = "TWL06.txt"

# Hash table containing words in highly-ranked profiles.
HIGH_WORDS = {}

# Hash table containing words in low-ranked profiles.
LOW_WORDS = {}

# Number of users in each category
hi = 0
low = 0

# Add training data to the hash tables.
t = open(TRAINING_DATA, 'r')
for user in t:
    rating = int(user.split(" ")[0])
    name = user.split(" ")[1]

    # Count users
    if rating == 1:
        hi += 1
    else:
        low += 1

    # Get data for each user
    call(["curl", "-o", "tmp.dta", "http://www.okcupid.com/profile/" + name])
    d = open("tmp.dta", 'r')

    # Get word list from user
    words = {}
    for line in d:
        for word in line.split(" "):
            if "/" in word or "=" in word or "<" in word or ">" in word or "()" in word or "&" in word or len(word) > 10:
                continue
            words[word.rstrip().lower()] = None
    d.close()

    # Add words in this word list to our master word lists
    for word in words:
        if rating == 1:
            if word in HIGH_WORDS:
                HIGH_WORDS[word] += 1
            else:
                HIGH_WORDS[word] = 1
        else:
            if word in LOW_WORDS:
                LOW_WORDS[word] += 1
            else:
                LOW_WORDS[word] = 1
t.close()

print HIGH_WORDS
print LOW_WORDS

# Classify a point, assuming training has already happened.
# P(C | F1 ... Fn) proportionate to P(C) * P(F1 | C) * ... * P(Fn | C)
def classify(username):
    # P(C)
    Phi = float(hi) / float(hi + low)
    Plow = float(low) / float(hi + low)
    ratio = Phi / Plow
    # Get data
    call(["curl", "-o", "tmp.dta", "http://www.okcupid.com/profile/" + username])
    d = open("tmp.dta", 'r')
    for line in d:
        for word in line.split(" "):
            # Calculate the probability of the feature given a class
            if word in HIGH_WORDS:
                numHi = float(HIGH_WORDS[word])
            else:
                numHi = 1
            if word in LOW_WORDS:
                numLow = float(LOW_WORDS[word])
            else:
                numLow = 1

            ratio = ratio * (numHi / float(hi)) / (numLow / float(low))
#            if word in HIGH_WORDS:
##                Phi = Phi * float(HIGH_WORDS[word]) / float(hi)
##            else:
##                Phi = Phi * (1 / float(hi))
#
#            if word in LOW_WORDS:
##                Plow = Plow * float(LOW_WORDS[word]) / float(low)
##            else:
##                Plow = Plow * (1 / float(low))

    print ratio
    if ratio > 1:
        return 1
    else:
        return 0

# Compute out of sample error on test set.
def getError():
    t = open(TEST_DATA, 'r')
    numUsers = 0.0
    total = 0.0
    for line in t:
        user = line.split(" ")[1]
        rating = int(line.split(" ")[0])
        prediction = classify(user)
        numUsers += 1.0
        if rating != prediction:
            total += 1.0
        print str(rating) + " " + str(prediction) + " " + user

    return total / numUsers

print getError()

#!/usr/bin/python

# Naive Bayes classifier for OkCupid data.  The features are the words in
# the profile and the classes are HIGH and LOW.  This is used to
# predict the rating I would give a profile.

# TODO: tweak the classifier so it works better.

from subprocess import call
from math import sqrt

# File containing training data in the form "<rating> <username>"
TRAINING_DATA = "train.dta"

# File containing test set in the form "<rating> <username>"
TEST_DATA = "test.dta"

# Dictionary
dictionary = "TWL06.txt"

Why is this not in ALL_CAPS like the others?

# Hash table containing words in highly-ranked profiles.
HIGH_WORDS = {}

# Hash table containing words in low-ranked profiles.
LOW_WORDS = {}

These aren't constants, so they shouldn't be global or in ALL_CAPS

# Number of users in each category
hi = 0
low = 0

Once you actually start logic, its best to do it inside a function except for really simply scripts.

# Add training data to the hash tables.
t = open(TRAINING_DATA, 'r')

Don't use single letter variable names, it results in difficult to follow code. Also, you should open a file using the with statement, to make sure it closes in all circumstances.

for user in t:
    rating = int(user.split(" ")[0])
    name = user.split(" ")[1]

Why are you passing the " " to slit instead of trusting the default. This approach is a little wasteful because you split the input twice.

    # Count users
    if rating == 1:
        hi += 1
    else:
        low += 1

    # Get data for each user
    call(["curl", "-o", "tmp.dta", "http://www.okcupid.com/profile/" + name])

Use urllib.urlopen to fetch urls. That you can avoid the temporary file.

    d = open("tmp.dta", 'r')

    # Get word list from user
    words = {}
    for line in d:
        for word in line.split(" "):
            if "/" in word or "=" in word or "<" in word or ">" in word or "()" in word or "&" in word or len(word) > 10:

Make a list of blocked characters rather then duplicating your logic so much here. And I suspect that "()" in word wasn't what you wanted.

                continue

I try to almost always avoid continue. I suggest rewriting the logic so the following line in the if block.

            words[word.rstrip().lower()] = None

You seem to be using words as a set. Use a set.

    d.close()

    # Add words in this word list to our master word lists
    for word in words:
        if rating == 1:
            if word in HIGH_WORDS:
                HIGH_WORDS[word] += 1
            else:
                HIGH_WORDS[word] = 1

Look at collections.Counter to simplify this.

        else:
            if word in LOW_WORDS:
                LOW_WORDS[word] += 1
            else:
                LOW_WORDS[word] = 1
t.close()

print HIGH_WORDS
print LOW_WORDS

# Classify a point, assuming training has already happened.
# P(C | F1 ... Fn) proportionate to P(C) * P(F1 | C) * ... * P(Fn | C)
def classify(username):
    # P(C)
    Phi = float(hi) / float(hi + low)
    Plow = float(low) / float(hi + low)

Add from __future__ import division so that all division produce floats. Then you don't have to cast to float. I also suggest calling the variable: probaility_high for greater clarity.

    ratio = Phi / Plow
    # Get data
    call(["curl", "-o", "tmp.dta", "http://www.okcupid.com/profile/" + username])
    d = open("tmp.dta", 'r')
    for line in d:
        for word in line.split(" "):

There is some duplication here. You should be able to write a function that returns the words from the url.

            # Calculate the probability of the feature given a class
            if word in HIGH_WORDS:
                numHi = float(HIGH_WORDS[word])
            else:
                numHi = 1
            if word in LOW_WORDS:
                numLow = float(LOW_WORDS[word])
            else:
                numLow = 1

            ratio = ratio * (numHi / float(hi)) / (numLow / float(low))
#            if word in HIGH_WORDS:
##                Phi = Phi * float(HIGH_WORDS[word]) / float(hi)
##            else:
##                Phi = Phi * (1 / float(hi))
#
#            if word in LOW_WORDS:
##                Plow = Plow * float(LOW_WORDS[word]) / float(low)
##            else:
##                Plow = Plow * (1 / float(low))

Don't leave dead code. Delete it.

    print ratio
    if ratio > 1:
        return 1
    else:
        return 0

# Compute out of sample error on test set.
def getError():
    t = open(TEST_DATA, 'r')
    numUsers = 0.0
    total = 0.0
    for line in t:
        user = line.split(" ")[1]
        rating = int(line.split(" ")[0])
        prediction = classify(user)
        numUsers += 1.0
        if rating != prediction:
            total += 1.0

Total seems a confusing name, because its only the mismatches.

        print str(rating) + " " + str(prediction) + " " + user

    return total / numUsers

print getError()

You may want to look into numpy. It'll allow much more efficient operations on large amounts of data like this.


Codility "max slice swap" challenge Problem Description:

  Find the maximum sum of a compact subsequence of array elements after
  performing a single swap operation.

Solution:

def maxsum(lst): # Get maximum sum of all possible slices in list
    max_ending = max_slice = lst[0]
    for i in xrange(1,len(lst)):
        max_ending = max(lst[i], max_ending + lst[i])
        max_slice = max(max_slice, max_ending)
    return max_slice

def solution(A):
    msum = A[0]
    for i in xrange(0, len(A)-1):
        for j in xrange(i+1, len(A)):
            A[i], A[j] = A[j], A[i] # swap
            msum = max(maxsum(A),msum)
            A[i], A[j] = A[j], A[i] # swap back
    return msum

I am swapping all possible pairs in the list to get the maximum sum after a particular swap and then the maximum of all those maximum sums. Given the \$O(N^3)\$ complexity, it times out for larger sequences/values. Is there a better way to implement this solution? Codility says \$O(N)\$ is possible.

After reading through the article linked in Josay's comment, I have translated the code into Python, removed extra fluff and added a bunch of comments to make it more readable then the version in the article. It also ran faster than the solution offered, probably due to Python's highly optimized Lists vs Java's ArrayLists.

This solution is \$O(N)\$ in both space and time, and for more details on why it works, refer to the article.

Here it is for Python 2.7 (the version required for Codility):

import operator

def solution(A):
    # calculate best swap forwards
    result1 = calculate(A)
    # calculate best swap backwards
    A.reverse()
    result2 = calculate(A)

    result = max(result1, result2)

    # If list is negative, choose least negative value
    if result == 0:
        return max(data)

    return result

def calculate(data):
    """Calculates the best partial sum swapping from front of data to back"""
    n = len(data)
    total = sum(data)

    # rolling sum: [0, 1, 2, 3] -> [0, 1, 3, 6]
    partialSum = accumulate(data, operator.add)
    # rolling max: [4, 3, 2, 1] -> [4, 4, 4, 4]
    partialMax = accumulate(data, max)

    differences = [partialSum[i] - partialMax[i] for i in range(n)]
    # rolling minimum of differences
    minDifferences = accumulate(differences, min)

    # Calculate best forward partial sums
    list1 = [min(data[0], 0)]
    for i in range(1, n):
        list1.append(min(minDifferences[i-1] + data[i], list1[i-1]))

    # Calculate best reverse partial sums
    data.reverse()
    reversedPartialSum = accumulate(data, operator.add)
    list2 = accumulate(reversedPartial, min)
    list2.reverse()

    # Calculate best swap result

    best = 0
    for i in range(n-1):
        best = max(best, total - list1[i] - list2[i+1])

    return max(best, total - list1[n-1])

def accumulate(values, function):
    """Accumulates the results of the function into a list"""
    result = [values[0]]
    for i in range(1, len(values)):
        result.append(function(values[i], result[i-1]))
    return result

And just for fun, here's a Python3.x version with the new accumulate() function from itertools:

from itertools import accumulate

def solution(A):
    # calculate best swap forwards
    result1 = calculate(A)
    # calculate best swap backwards
    A.reverse()
    result2 = calculate(A)

    result = max(result1, result2)

    # If list is negative, choose least negative value
    if result == 0:
        return max(data)

    return result

def calculate(data):
    """Calculates the best partial sum swapping from front of data to back"""
    n = len(data)
    total = sum(data)

    # rolling sum: [0, 1, 2, 3] -> [0, 1, 3, 6]
    partialSum = list(accumulate(data))
    # rolling max: [4, 3, 2, 1] -> [4, 4, 4, 4]
    partialMax = list(accumulate(data, max))

    differences = [partialSum[i] - partialMax[i] for i in range(n)]
    # rolling minimum of differences
    minDifferences = list(accumulate(differences, min))

    # Calculate best forward partial sums
    list1 = [min(data[0], 0)]
    for i in range(1, n):
        list1.append(min(minDifferences[i-1] + data[i], list1[i-1]))

    # Calculate best reverse partial sums
    data.reverse()
    reversedPartialSum = list(accumulate(data))
    list2 = list(accumulate(reversedPartial, min))
    list2.reverse()

    # Calculate best swap result

    best = 0
    for i in range(n-1):
        best = max(best, total - list1[i] - list2[i+1])

    return max(best, total - list1[n-1])


Save to a MySQL database a temporary row with all information about a receipt The Code
The Save Function:

public void SaveProducts()
{
    TempSave _Save = new TempSave();
    _Save.Localization = _LocationLabel.Text + "," + _LocNumbLab.Text;
    string items = "";
    for (int i = 0; i < _ProductsGrid.Rows.Count; i++)
    {
        items += _ProductsGrid.Rows[i].Cells[1].Value.ToString();
        items += ",";
    }
    string Qnt = "";
    for (int i = 0; i < _ProductsGrid.Rows.Count; i++)
    {
        Qnt += _ProductsGrid.Rows[i].Cells[0].Value.ToString();
        Qnt += ",";
    }
    try
    {
        items = items.Remove(items.Length - 1, 1);
        Qnt = Qnt.Remove(Qnt.Length - 1, 1);
    }
    catch{}
    _Save.Items = items;
    _Save.Quantity = Qnt;

    IEnumerable<TempSave> _CheackTem;

    string Cheak_TempSave = "SELECT * FROM TempSave WHERE Localization='" + _Save.Localization + "'";

    using (MySqlConnection Con = new MySqlConnection("server=localhost; database=wintestbeta;user=root; password=;"))
    {
        Con.Open();
        _CheackTem = Con.Query<TempSave>(Cheak_TempSave);
        Con.Close();
    }

    if (_CheackTem.Count() == 0)
    {
        string _TempSave = "INSERT INTO TempSave (Localization,items,Quantity) VALUES('" + _Save.Localization + "','" + _Save.Items + "','" + _Save.Quantity + "')";

        using (MySqlConnection Con = new MySqlConnection("server=localhost; database=wintestbeta;user=root; password=;"))
        {
            Con.Open();
            Con.Query(_TempSave);
            Con.Close();
        }
    }
    else
    {
        string _TempDelete = "DELETE FROM TempSave WHERE Localization='" + _Save.Localization + "'";

        using (MySqlConnection Con = new MySqlConnection("server=localhost; database=wintestbeta;user=root; password=;"))
        {
            Con.Open();
            Con.Query(_TempDelete);
            Con.Close();
        }
        string _TempSave = "INSERT INTO TempSave (Localization,items,Quantity) VALUES('" + _Save.Localization + "','" + _Save.Items + "','" + _Save.Quantity+ "')";

        using (MySqlConnection Con = new MySqlConnection("server=localhost; database=wintestbeta;user=root; password=;"))
        {
            Con.Open();
            Con.Query(_TempSave);
            Con.Close();
        }

    }
}

The TempSave class

public class TempSave
{
    public int id { get; set; }
    public string Localization { get; set; }
    public string Items { get; set; }
    public string Quantity { get; set; }
}

I don't use the int id but I left it there because I can became necessary in the future

How It Works
What the code does is gather 3 information's from a grid

Get the location (this is from a label)
Get all items from a grid that have been selected and put it into a string array
Get the quantity of all items from a grid and put it into a string array

The grid looks like this

+---------------------+
|Qnt|Description|Value|
+---------------------+

Then for each string in the items string array it will join all string together with a "," between them.

Then it does the same to the quantity string array

After that it checks if in the MySQL table already contains any row in the same location

If it doesn't it will insert the row,
if it does it will delete and insert the row.

This function is activated every time a new item is add to the grid no matter if it exists or not, if it exists it will just change the quantity.

I want to know if there is anyway I can make this function faster because it is taking to much time, about 2 seconds.

With this code:

string items = "";
 for (int i = 0; i < _ProductsGrid.Rows.Count; i++)
 {
     items += _ProductsGrid.Rows[i].Cells[1].Value.ToString();
     items += ",";
 }

items = items.Remove(items.Length - 1, 1);

You're concatenating a list of values into a string. Let me first say a word about that try/catch. Exceptions are exceptional conditions, if you can avoid an exception then you should:

if (items.Length > 1)
    items = items.Remove(items.Length - 1, 1);

However you do not need it, both items and Qnt can be created with like this (assuming _ProductsGrid is a DataGrid but same logic applies in any case):

var rows = _ProductsGrid.Rows.Cast<DataRow>();
string items String.Join(";",
    rows.Select(x => Convert.ToString(x.Cells[1].Value)));

Now let's see you SQL. You're building SQL commands into strings, it makes your code hard to debug, open to SQL injection attacks and fragile because invalid and unexpected user inputs will crash the application. Use SQL parameters:

string query = "SELECT * FROM TempSave WHERE Localization=@Localization";

Now you have to build the command:

using (var cmd = new MySqlCommand(query, connection))
{
    cmd.Parameters.AddWithValue("@Localization", _Save.Localization);

    // ...
}

And you have the reader:

using (var reader = cmd.ExecuteReader())
{
    // ...
}

Note that using parameters also the string concatenation will go away (because you can dynamically create named parameters as @param1, @param2 and so on).

You do not need to call MySqlConnection.Close() if (correctly!) you're already wrapped the object inside a using statement.

Please follow usual C# naming guidelines, they will make your code easier to read for everyone (for example local variables should be camelCase). Stick to one convention and follow it (to have items and Qnt is confusing, why they have different casing?)

Also try to use meaningful names for your variables. Code should be a fluent sentence to read like a book.

Catch exceptions. Things may go wrong and you do not want to crash entire application.

Validate user input. What if user didn't enter anything in one of your input boxes?

Split code into separate methods, it helps because to understand whole logic you need to read just few lines, not the entire code. Think (names are weird because I do not know your domain but you must do better):

if (CheackTemDataAlreadyExists())
    DeleteCheackTemData();

InsertCheackTemData();

Note that  you should avoid code duplication (INSERT was repeated twice).

Do not hard-code constants (like connection string). Ideally it should be in your configuration file but, at least, move it to a private const string field.


EmailSession in Java Webapp I have written an EmailSession class to connect and get (unread) mail messages. It is all working, but I would like to see your opinions on any enhancements or improvements, as I understood there are lots of bad examples on the Internet and I copy-pasted most of it together.

One issue I already noticed:

If there is no internet connection it throws a java.net.UnknownHostException instead of handling the issue gracefully.

import java.util.*;
import java.util.logging.*;
import javax.mail.*;

public class EmailSession {

    private Session session;
    private Store store;
    private Folder inbox;
    private int messageCount;
    private int unreadMessageCount;
    private List<Message> unreadMessages;

    public EmailSession() {
        Properties props = new Properties();
        props.setProperty("mail.store.protocol", MyProps.getProperty("mail.store.protocol"));
        try {
            session = Session.getInstance(props, null);
            store = session.getStore();
            store.connect(MyProps.getProperty("mail.host"), MyProps.getProperty("mail.user"), MyProps.getProperty("mail.password"));
            inbox = store.getFolder(MyProps.getProperty("mail.foldername"));
            inbox.open(Folder.READ_ONLY);
            messageCount = inbox.getMessageCount();
            List<Message> messages;
            if (messageCount > 100) {
                Logger.getLogger(EmailSession.class.getName()).log(Level.WARNING,
                        "More than 100 messages");
                messages = Arrays.asList(inbox.getMessages(1, 100));
            } else {
                messages = Arrays.asList(inbox.getMessages());
            }
            unreadMessages = new ArrayList<>();
            unreadMessageCount = 0;
            for (Message message : messages) {
                boolean isUnreadMessage = true;
                for (Flags.Flag flag : message.getFlags().getSystemFlags()) {
                    if (flag == Flags.Flag.SEEN) {
                        isUnreadMessage = false;
                        break;
                    }
                }
                if (isUnreadMessage) {
                    unreadMessages.add(message);
                    unreadMessageCount++;
                }
            }
            inbox.close(true);
        } catch (NoSuchProviderException ex) {
            Logger.getLogger(EmailSession.class.getName()).log(Level.SEVERE, null, ex);
        } catch (MessagingException ex) {
            Logger.getLogger(EmailSession.class.getName()).log(Level.SEVERE, null, ex);
        }
    }

    public Folder getInbox() {
        return inbox;
    }

    public int getMessageCount() {
        return messageCount;
    }

    public int getUnreadMessageCount() {
        return unreadMessageCount;
    }

    public List<Message> getUnreadMessages() {
        return unreadMessages;
    }

    public boolean close() {
        try {
            store.close();
            return true;
        } catch (MessagingException ex) {
            Logger.getLogger(EmailSession.class.getName()).log(Level.SEVERE, null, ex);

            return false;
        }
    }

}

Pointless member variables

session could be a local variable in the constructor. You use it once to get store and you don't reference it again.

unreadMessageCount is pointless, because you could replace it with unreadMessages.size() instead.

In the constructor, you assign inbox, but you also close it. I'm not really familiar with how javax.mail works, but usually, after you close something it tends to become unusable. If that's the case, then the inbox member is pointless.

Pointless member variables make your class design confusing, hard to read, and potentially buggy. Try to be minimalistic, and eliminate anything from your class that it doesn't really need.

Extract logger

The logging in your code can be simplified if you extract the Logger to a constant:

private static final Logger LOGGER = Logger.getLogger(EmailSession.class.getName());

// ...
LOGGER.warning("More than 100 messages");
// ...
LOGGER.log(Level.SEVERE, null, ex);

Extract methods

The constructor is too long. It's hard to read large chunks of code. You could extract some of the functionality there to private methods to make it more readable:

private List<Message> getMessages(Folder inbox, int messageCount) throws MessagingException {
    if (messageCount > 100) {
        LOGGER.warning("More than 100 messages");
        return Arrays.asList(inbox.getMessages(1, 100));
    }
    return Arrays.asList(inbox.getMessages());
}

private List<Message> getUnreadMessages(List<Message> messages) throws MessagingException {
    List<Message> unreadMessages = new ArrayList<>();
    for (Message message : messages) {
        boolean isUnreadMessage = true;
        for (Flags.Flag flag : message.getFlags().getSystemFlags()) {
            if (flag == Flags.Flag.SEEN) {
                isUnreadMessage = false;
                break;
            }
        }
        if (isUnreadMessage) {
            unreadMessages.add(message);
        }
    }
    return unreadMessages;
}

With these helper methods and the other suggestions above, you could simplify the constructor:

public EmailSession() {
    Properties props = new Properties();
    props.setProperty("mail.store.protocol", MyProps.getProperty("mail.store.protocol"));
    Session session = Session.getInstance(props, null);
    try {
        store = session.getStore();
        store.connect(MyProps.getProperty("mail.host"), MyProps.getProperty("mail.user"), MyProps.getProperty("mail.password"));
        inbox = store.getFolder(MyProps.getProperty("mail.foldername"));
        inbox.open(Folder.READ_ONLY);
        messageCount = inbox.getMessageCount();
        List<Message> messages = getMessages(inbox, messageCount);
        inbox.close(true);
        unreadMessages = getUnreadMessages(messages);
    } catch (NoSuchProviderException ex) {
        LOGGER.log(Level.SEVERE, null, ex);
    } catch (MessagingException ex) {
        LOGGER.log(Level.SEVERE, null, ex);
    }
}

I also moved inbox.close higher to close it immediately when you don't need it anymore.

Avoid wildcard imports

Wildcard imports like import java.util.* are considered bad practice. Try to avoid it, it's better to clean up all of these:

import java.util.*;
import java.util.logging.*;
import javax.mail.*;


Tone Mapping image manipulation This code is extremely slow and ideally I would appreciate someone walking me through getting this going on the GPU, but if nobody has time for that which is understandable, any and all optimisations would be most welcome!

Private Sub Picture(source As Image, picturebox As PictureBox)
    Dim r As Double
    Dim g As Double
    Dim b As Double

    Dim bmp As New Bitmap(source)
    Dim rect As New Rectangle(0, 0, bmp.Width, bmp.Height)
    Dim bmpData As System.Drawing.Imaging.BitmapData = bmp.LockBits(rect, System.Drawing.Imaging.ImageLockMode.[ReadOnly], bmp.PixelFormat)
    Dim ptr As IntPtr = bmpData.Scan0
    Dim bytes As Integer = bmpData.Stride * bmp.Height
    Dim rgbValues As Byte() = New Byte(bytes - 1) {}
    System.Runtime.InteropServices.Marshal.Copy(ptr, rgbValues, 0, bytes)

    Dim linWhiteMult = 1 / (((LinearWhiteTextBox.Text * ((ShoulderStrengthTextBox.Text * LinearWhiteTextBox.Text) + (LinearAngleTextBox.Text * LinearStrengthTextBox.Text)) + (ToeStrengthTextBox.Text * ToeNumeratorTextBox.Text)) _
    / (LinearWhiteTextBox.Text * ((ShoulderStrengthTextBox.Text * LinearWhiteTextBox.Text) + LinearStrengthTextBox.Text) + ToeStrengthTextBox.Text * ToeDenominatorTextBox.Text) - (ToeNumeratorTextBox.Text / ToeDenominatorTextBox.Text)))

    'precalculations for optimisation

    Dim LaLs = LinearAngleTextBox.Text * LinearStrengthTextBox.Text
    Dim TsTn = ToeStrengthTextBox.Text * ToeNumeratorTextBox.Text
    Dim TsTd = ToeStrengthTextBox.Text * ToeDenominatorTextBox.Text
    Dim Tn_Td = ToeNumeratorTextBox.Text / ToeDenominatorTextBox.Text

    Dim list As New List(Of String)
    For x As Integer = 0 To bmp.Width - 1
        For y As Integer = 0 To bmp.Height - 1
            Dim position As Integer = (y * bmpData.Stride) + (x * Image.GetPixelFormatSize(bmpData.PixelFormat) / 8)
            b = rgbValues(position)
            g = rgbValues(position + 1)
            r = rgbValues(position + 2)

            '^2.2 lookup table (for optimisation) for gamma correction

            b = table.Item(b)
            g = table.Item(g)
            r = table.Item(r)

'the following is the filmic tone map formula, i.e.
'x*(A*x+C*B)+D*E)/(x*(A*x+B)+D*F)) - E/F

            r = (((r * ((ShoulderStrengthTextBox.Text * r) + (LaLs)) + (TsTn)) _
                    / (r * ((ShoulderStrengthTextBox.Text * r) + LinearStrengthTextBox.Text) + TsTd) - (Tn_Td))) * linWhiteMult

            g = (((g * ((ShoulderStrengthTextBox.Text * g) + (LaLs)) + (TsTn)) _
                    / (g * ((ShoulderStrengthTextBox.Text * g) + LinearStrengthTextBox.Text) + TsTd) - (Tn_Td))) * linWhiteMult

            b = (((b * ((ShoulderStrengthTextBox.Text * b) + (LaLs)) + (TsTn)) _
                    / (b * ((ShoulderStrengthTextBox.Text * b) + LinearStrengthTextBox.Text) + TsTd) - (Tn_Td))) * linWhiteMult
            If r > 1 Then
                r = 1
            End If
            If g > 1 Then
                g = 1
            End If
            If b > 1 Then
                b = 1
            End If

            rgbValues(position) = Math.Round((b ^ 0.45) * 255)
            rgbValues(position + 1) = Math.Round((g ^ 0.45) * 255)
            rgbValues(position + 2) = Math.Round((r ^ 0.45) * 255)

        Next
    Next

    System.Runtime.InteropServices.Marshal.Copy(rgbValues, 0, ptr, bytes)

    bmp.UnlockBits(bmpData)

    ' apply the bmp to the avatars and dispose old

    Dim OldImage = picturebox.Image
    picturebox.Image = bmp
    OldImage.Dispose()

End Sub

First, I have to say that you are using good variable names.

Turn Option Strict On

This setting is important, it'll help you see errors and needed conversion.

You'll notice that this isn't possible

Dim LaLs = LinearAngleTextBox.Text * LinearStrengthTextBox.Text

But instead you need to convert it to integer.

Dim LaLs = Int32.Parse(LinearAngleTextBox.Text) * Int32.Parse(LinearStrengthTextBox.Text)

Don't repeat yourself

You'll then notice that some conversions are done often.

Int32.Parse(LinearStrengthTextBox.Text)

Put these in proper variable right at the begining.

Dim linearStrength As Integer = Int32.Parse(LinearStrengthTextBox.Text)

There are formula like the below that are done in the inner loop, which can be moved outside of the loop.

(x * Image.GetPixelFormatSize(bmpData.PixelFormat) / 8)


Function to find sum of digits in the number a^b where a, b are positive integers I was solving Project Euler problem 16, which asks to find the sum of digits of the number 2 raised to the power 1000. Using Python, it can be solved in one line of code:

sum(map(int, list(str(2**1000))))

It felt too easy so I decided to write a function in C to do the same job. Here's my program. In what ways can I improve it?  Please suggest good practices also.

#include <stdio.h>
#include <stdlib.h>
#include <math.h>
#include <assert.h>

int sumdigit(int a, int b);

int main(void) {
    int a = 2;
    int b = 10000;
    printf("%d\n", sumdigit(a, b));
    return 0;
}

int sumdigit(int a, int b) {
    // numlen = number of digit in a^b
    // pcount = power of 'a' after ith iteration
    // dcount = number of digit in a^(pcount)

    int numlen = (int) (b * log10(a)) + 1;
    char *arr = calloc(numlen, sizeof *arr);
    int pcount = 0;
    int dcount = 1;
    arr[numlen - 1] = 1;
    int i, sum, carry;

    while(pcount < b) {
        pcount += 1;

        sum = 0;
        carry = 0;

        for(i = numlen - 1; i >= numlen - dcount; --i) {
            sum = arr[i] * a + carry;
            carry = sum / 10;
            arr[i] = sum % 10;
        }

        while(carry > 0) {
            dcount += 1;
            sum = arr[numlen - dcount] + carry;
            carry = sum / 10;
            arr[numlen - dcount] = sum % 10;
        }
    }

    int result = 0;
    for(i = numlen - dcount; i < numlen; ++i)
        result += arr[i];

    free(arr);
    return result;
}

The problem calls for computing the sum of digits in 21000.  You did 210000.  Overachiever!

There are a few things you could do to improve readability.

char *arr looks a lot like a string.  One might initially think that you are storing the number as a string (i.e., ASCII '0'… '9').  You could dispel that impression by changing it to unsigned char *arr.  To be even clearer, I would use a typedef unsigned char digit; so that you can declare digit *arr.  Then it is clear that you are using binary-coded decimal.

As long as you are doing binary-coded decimal, there is no advantage to putting the most-significant digit first.  With the least-significant digit first, the code is simpler, and there is the nice property that the number is sum(arr[i] * 10**i for i in range(len(arr))).

For further readability, I would define a struct to represent the big decimal:

typedef unsigned char digit;

typedef struct {
    int capacity;
    int dcount;
    digit *digits;  /* Unsigned binary-coded decimal. The number is
                       sum(arr[i] * 10**i for i in range(dcount)) */
} bigdecimal;

There are a lot of variables in your sumdigit() — not quite a mess, but enough to start being concerned.  The function name sumdigit is also a bad code smell.  I would also decompose the operations for readability.  (Defining the bigdecimal type makes that practical.)

void mult(unsigned int a, bigdecimal *b) {
    int carry = 0;

    for (int i = 0; i < b->dcount; ++i) {
        int sum = a * b->digits[i] + carry;
        carry = sum / 10;
        b->digits[i] = sum % 10;
    }

    while (carry > 0) {
        int sum = b->digits[b->dcount] + carry;
        carry = sum / 10;
        b->digits[b->dcount] = sum % 10;
        b->dcount++;
    }
}

bigdecimal *exponentiate(unsigned int base, unsigned int power) {
    bigdecimal *result = malloc(sizeof(bigdecimal));
    result->capacity = 1 + (int) (power * log10(base));
    result->digits = calloc(result->capacity, sizeof(digit));

    /* base**0 == 1 */
    result->digits[0] = 1;
    result->dcount = 1;

    for (int p = 0; p < power; p++) {
        mult(base, result);
    }

    return result;
}

unsigned int sumdigit(bigdecimal *b) {
    unsigned int sum = 0;
    for (int i = b->dcount - 1; i >= 0; --i) {
        sum += b->digits[i];
    }
    return sum;
}

void free_bigdecimal(bigdecimal *b) {
    free(b->digits);
    free(b);
}

int main(void) {
    bigdecimal *n = exponentiate(2, 1000);
    printf("%u\n", sumdigit(n));
    free_bigdecimal(n);
    return 0;
}

I've used C99 variable scoping in the for-loops because it's nicer.


Wildcard string search with capture My goal was to create a function as performant as possible with results being:

True/False as to whether wild matches str.
Optional capture of certain patterns.

My purposes required:

Case sensitive match.
Pattern must match entire string to succeed.
Assumption that * and ? do not exist in str.
* matches zero or more characters.
? matches exactly one character.
Lazy * in this case meaning that going from left to right, * would match as little as possible while still ensuring a match if such were possible.

It's been a challenge as many times I almost had it but then found an edge case that didn't work.  This is basically globbing, but I need such to work for strings rather than files and I also hoped to make something much faster than using a regex which IMHO would be overkill for my needs.

I have tested the following to work on many patterns but I can't be certain yet if there isn't a permutation where it fails.  I would be very interested if anyone can point out a situation where it fails the terms set above.  I'm also interested to hear if there are any ways the code can be improved (made faster).

Hacks like (unsigned)(*wild-40)<=1 are probably unnecessary given the ability of gcc to optimize, but I put it in there anyway.

static __attribute__ ((noinline)) int globSearch(char *wild,char *str,char **c)
  {
  int N=-1,n=0,nm=0;

  while(*str)
      {
      if(*wild == '*'){ N=n; wild++; }
      else if((unsigned)(*wild-40)<=1) // faster version of if(*wild=='('||*wild==')')
          {
          c[n++]=str;
          wild++;
          }
      else if(*wild == '?'){ wild++; str++; }
      else if(*wild == *str)
          {
          if(nm){while(N < n){ c[N++]+=nm; }}
          nm=0;
          N=-1;
          wild++;
          str++;
          }
      else if(N > -1){ str++; nm++; }
      else return 0;
      }

    while((unsigned)(*wild-40) <= 2) // while( *wild == '(' || *wild == ')' || *wild == '*' )
        {
        if(*wild++ != '*')c[n++]=str;
        }
    if(nm){while(N < n){ c[N++]+=nm; }}

    return !*wild;
    }

Testing function

int main()
    {
    char tmp[100]={0};
    char *c[20]={0};
    char *m="(hello) (*)(?*?)?(?)?(*)(*)";
    char *str="hello world";

    int t=globSearch(m,str,c);

    if(t)
        {
        for(int x=0;c[x]&&c[x+1];x+=2)
            {
            int z=c[x+1]-c[x];
            memcpy(tmp,c[x],z);
            tmp[z]=0;
            printf("%s:%d\n",tmp,z);
            }
        }
    return 0;
    }

The above code returns successful match with patterns captured: (hello:5) (0) (wo:2) (l:1) (0) (0)

Other patterns and results:

*(hello*)* -> (hello:5)
hello *(d) -> (d:1)
(hello) (*)(d) -> (hello:5) (worl:4) (d:1)
(hello) *(*)r(*)? -> (hello:5) (wo:2) (l:1)
(hello) *(*)r(*)*? -> (hello:5) (wo:2) (0)

All the results are exactly what I was hoping given the parameters.  Can it be done faster?

I see a number of things that may help you improve your code.

Use a switch instead of long if ...else chain

The pattern matching logic is much easier to see if a swtich statement is used instead of the long if...else chain.  The default case can then be used solely for the non-pattern characters and a relatively short if...else chain would go there.  On my machine, this also makes the code slightly faster.

Use longer, more meaningful names

Names like n and N are not very descriptive.  Instead, you might use capturegroup and groupnum.

Use const where practical

The strings should not be altered by the search and the captured array c is simply a list of pointers into the string.  For that reason, I would advise changing the signature of the function to this:

int globSearch(const char *pattern, const char *str, const char **capture)

Consider checking for NULL pointers

If speed is the primary consideration and if the caller checks, you don't need this, but I'd normally recommend having the called function check for NULL pointers before attempting to dereference them.

Differentiate between '(' and ')'

Right now, your program produces the same output for this pattern:

char *m = "(hello) (*)(?*?)?(?)?(*)(*)";

as this pattern:

char *m = ")hello) )*))?*?)?)?)?)*))*)";

Maybe that's OK, but it also produces the same for this pattern:

char *m = "(hello) (*((?*?)?)?)?(*)(*)";

That's a problem because it appears to those familiar with regular expressions that this is pattern contains nested capture patterns, which is not how the code actually interprets the string.

Consider changing the interface

Instead of an array of pointers, it might be more useful to have pointers and counts.  Otherwise, any time the values are returned, you'll have to do things like this:

int z = c[x + 1] - c[x];
memcpy(tmp, c[x], z);
tmp[z] = 0;
printf("%s:%d\n", tmp, z);

Pass the size of the array of pointers

Any time you pass an array to a function, you should either use a sentinel value (such as the terminating '\0' character to mark the end of a string) or pass a size with the pointer.  Otherwise, the called routine has no way to assure that it won't overrun the end of the array.

Avoid buffer overflow vulnerabilities

I realized it's just demo code, but any time you use memcpy it's important to make sure that the destination buffer has enough space for the number of bytes being copied.  That's the case here with these particular fixed strings, but it's better generally to dynamically check that at runtime, especially when the length parameter is not a constant.

Eliminate the return 0; in main

For many years, the C standard has specified that the compiler must automatically generate the equivalent of return 0; at the end of main so there is no reason to specfically include it manually.


Wanted to write my own form slider, Jquery/JS I would like a fair minded critique of this code to get some feedback. I am not trying to re-invent the mold here, and I know there are plugins galore I could have used to do what I am doing here, but the point was to do it myself so that I can learn more and become better.

I have included comments to try and best explain my train of thought through the process, please don't implicitly critique the comments, they are just to explain my thinking as I made this code.

code below does exactly what I want..functions great in all modern browsers, rip me apart and make it better so I can get better:

$(document).ready(function(){

var sliding = false

    $(function () {
        var pos = new Array(); //certainly a way to automate this more but I didn't mind putting exact numbers in for interval postions...please critique.
         pos[0] = 0;
         pos[1] = 67;
         pos[2] = 133;
         pos[3] = 199;
         pos[4] = 267;
         pos[5] = 333;

        var target = $('span.slider')
        var targetToMove = null;
        var sliderCoordStart = null;
        var mouseMovement = null;

        target.mousedown(function(c) {
              c.preventDefault();//prevent chrome from turning cursor to text when dragging
              sliding = true
              targetToMove = $(this);
              targetToMove.addClass('noselect');//make sure we cant drag  untargeted things all over the screen
              mouseCoordStart =  c.clientX;
              sliderCoordStart=  parseInt(targetToMove.css('marginLeft'));
              oncontextmenu = function() {
              return false
              }

        });
        $(document).mouseup(function(e) {
             oncontextmenu = function() {
             return true
             }
             sliding = false
             document.onselectstart = function(){
             return true;
             }
             if(targetToMove !=null){
             targetToMove.removeClass('noselect');//no need to have the non select class anymore
             }
             var compare = new Array();
             if(mouseMovement !=null ){
             for(c=0;c<=5;c++){//do some math to make sure the slider ends up going to the closest interval postion
                 compare[c] = Math.abs(pos[c] - mouseMovement);
                }
             var goTo = Math.min.apply(Math, compare)
             var finalPos = compare.indexOf(goTo);
             targetToMove.css('marginLeft', pos[finalPos])

             }
        })
        $(document).mousemove(function(e) {
            if(sliding){
            document.onselectstart = function(){ //probably a better/smoother way to chain that right?
            return false;
            }
            sliderPos = targetToMove.css('marginLeft');
            mouseMovement = (e.pageX - mouseCoordStart)+sliderCoordStart;
                if(mouseMovement <= 0){//make sure we never exceed the left boundary
                    mouseMovement = 0;
                }
                if(mouseMovement >= 333){//make sure we never exceed the right boundary
                    mouseMovement = 333;
                }
              targetToMove.css('marginLeft', mouseMovement)
            }
        })
    })

});

Here are some suggestions to simplify the syntax:

For pos, use an array literal: var pos = [0, 67, 133, 199, 267, 333]

Similar thing for compare: var compare = [];

You're missing semi-colons after some statements
You can make initializing compare simpler using Array.prototype.map (doesn't work on IE < 9):

var compare = pos.map(function (e) {
    return e - mouseMovement;
});

If you need to support IE < 9, I'd either implement map, or do basically what you're doing, but use Array.prototype.push instead:

for(var c=0;c<=5;c++){//do some math to make sure the slider ends up going to the closest interval postion
    compare.push(Math.abs(pos[c] - mouseMovement));
}

You've got a lot of undefined variables that will default to the global scope:

sliderPos- not used
mouseCoordStart- dangerous because it might not be defined when it's used
oncontextmenu- not used anywhere

333 in your mousemove handler is magic, perhaps it should be pos[pos.length-1]?

I'm not going to comment on jQuery plug-in style (I don't use jQuery) or your implementation of a "form slider" (which I assume is an implementation of an HTML5 input range).

Hopefully this was useful!


Jvector Map add/remove function shorten code I am achieving a hover state per continent through:

var continentId =""
function getID(continentId){
    jQuery.each(mapObject.mapData.paths, function(i, val) {
            if (val.continent == continentId){
                continentCodes[i] = "#3e9d01";
                mapObject.series.regions[0].setValues(continentCodes);
            }
    });
}

function removeGetID(continentId){
    jQuery.each(mapObject.mapData.paths, function(i, val) {
            if (val.continent == continentId){
                continentCodes[i] = "#128da7";
                mapObject.series.regions[0].setValues(continentCodes);
            }
    });
}

//LIST COUNTRIES & CONTINENTS TEMP
jQuery('.continentLink').hover(function(e) {
    continentId = this.id;
    getID(continentId);
}, function(){
    removeGetID(continentId);
});

Is there any way to shorten this so that I don't have to have multiple each statements?  I'm really trying to learn to write efficient code.

Here's the full code if it helps: JSFIDDLE

jQuery(function(){
//JSON MARKERS
var markers = [{latLng: [-34.033333300000000000, 23.066666700000040000], name: 'Knysna', info:'its got a lake...'},
    {latLng: [-33.924868500000000000, 18.424055299999963000], name: 'Cape Town', info:'its nice...'}];
//JSON MARKERS

//JSON STYLING
var markerStyle = {initial: {fill: '#F8E23B',stroke: '#383f47'}};
var regionStyling = {initial: {fill: '#128da7'},hover: {fill: "#A0D1DC"}};
//JSON STYLING

//GLOBAL VARIABLES
var countryList = "", continentList = "";
var resultsDup = {};
var continentCodes = {};
//GLOBAL VARIABLES

//INIT MAP PLUGIN
jQuery('#world-map').vectorMap({
    map: 'world_mill_en',
    normalizeFunction: 'polynomial',
    markerStyle:markerStyle,
    regionStyle:regionStyling,
    backgroundColor: '#383f47',
    series: {regions: [{values: {},attribute: 'fill'}]},
    markers: markers,
    onRegionClick:function (event, code){
        jQuery('#world-map').vectorMap('set', 'focus', code);
    },
    onMarkerClick: function(events, index){
        jQuery('#infobox').html(markers[index].name);
    }
});
//INIT MAP PLUGIN

var mapObject  = jQuery('#world-map').vectorMap('get', 'mapObject');

//LIST COUNTRIES & CONTINENTS
jQuery.each(mapObject.mapData.paths, function(i, val) {

    countryList += '
'+val.name+'
';

    //remove duplicate continents
    var resultsList = val.continent;
    if (resultsDup[resultsList]) {
        jQuery(this).remove();
    }else{
        resultsDup[resultsList] = true;
        continentList += '
'+val.continent+'
';
    }
    //remove duplicate continents

});
//display countries
jQuery('#countryList').html(countryList);

//display continents
jQuery('#continentList').html(continentList);

var continentId =""
function getID(continentId){
    jQuery.each(mapObject.mapData.paths, function(i, val) {
            if (val.continent == continentId){
                continentCodes[i] = "#3e9d01";
                mapObject.series.regions[0].setValues(continentCodes);
            }
    });
}

function removeGetID(continentId){
    jQuery.each(mapObject.mapData.paths, function(i, val) {
            if (val.continent == continentId){
                continentCodes[i] = "#128da7";
                mapObject.series.regions[0].setValues(continentCodes);
            }
    });
}

//LIST COUNTRIES & CONTINENTS TEMP
jQuery('.continentLink').hover(function(e) {
    continentId = this.id;
    getID(continentId);
}, function(){
    removeGetID(continentId);
});

//Zoom to Country Function
jQuery('.countryLink').click(function(e) {
    jQuery('#world-map').vectorMap('set', 'focus', this.id);
});

//Continent Hover function

});

You are looking to remove the copy pasted code, to make it more DRY.

Beyond that, it seems that really whether you getID or removeGetID, you set a number of ID's and then update mapObject. Furthermore, it seems that you do not need to update mapObject within the loop, which is slow.

I would counter-propose

var helpfullyNamedConstant1 = '#3e9d01';
var helpfullyNamedConstant2 = '#128da7';

function setContinentCodes(continentId, continentCodesValue ){
    jQuery.each(mapObject.mapData.paths, function(i, val) {
            if (val.continent == continentId){
                continentCodes[i] = continentCodesValue ;
            }
    });
    mapObject.series.regions[0].setValues(continentCodes);
}

jQuery('.continentLink').hover(function(e) {
    setContinentCodes(this.id, helpfullyNamedConstant1);
}, function(){
    setContinentCodes(this.id, helpfullyNamedConstant2);
});


Assignement on reading from stdin in C I am an experienced Python developer learning C. I solved this assignement:

  Write a program that reads a word and some sentences from stdin. The word is separated by space from a sentences. The sentences are separated by one of the following characters: .!?. Words in sentences are separated by any char for which one of library functions isspace and ispunct returns nonzero result.

  Your program should output average usage of the word in the sentences (a number of matched words divided by a number of sentences).

  Example input:

  two one four two seven eight two four two. two, one, nine, two, two, three, two!

  Example expected output: 3.5

  Explanation: The word is "two". Then we have two sentences: "one four two seven eight two four two." and "two, one, nine, two, two, three, two!". The word occurs 7 times, so result is 7/2 = 3.5.

Here is my solution:

# include <ctype.h>
# include <stdbool.h>
# include <stdio.h>
# include <stdlib.h>
# include <unistd.h>

void check_malloc_result(void* malloc_result) {
    if (!malloc_result) {
        perror("malloc failed");
        exit(EXIT_FAILURE);
    }
}

int main(void) {
    char stdin_ch;

    size_t buff_size = 8;
    size_t word_len = 0;
    bool reading_word = false;

    char* word = malloc(buff_size);
    check_malloc_result(word);

    // reading the word
    while(read(STDIN_FILENO, &stdin_ch, 1) > 0) {
        if (isspace(stdin_ch)) {
            if (reading_word) {
                break;
            } else {
                continue;
            }
        }
        reading_word = true;
        ++word_len;

        if (word_len > buff_size) {
            buff_size *= 2;
            word = realloc(word, buff_size);
            check_malloc_result(word);
        }

        word[word_len - 1] = stdin_ch;
    }

    // counting matches
    size_t sen_count = 0, matched_count = 0, cur_word_pos = 0;
    bool matching = true;
    reading_word = false;

    while(word_len && read(STDIN_FILENO, &stdin_ch, 1) > 0) {
        if (ispunct(stdin_ch) || isspace(stdin_ch)) {
            if (reading_word) {
                // end of a word
                if (matching) {
                    ++matched_count;
                }
                if (stdin_ch == '.' || stdin_ch == '?' || stdin_ch == '!') {
                    ++sen_count;
                }
                reading_word = false;
                matching = true;
                cur_word_pos = 0;
            }
            continue;
        }

        reading_word = true;
        if (matching) {
            if (cur_word_pos >= word_len || word[cur_word_pos] != stdin_ch) {
                matching = false;
            }
            ++cur_word_pos;
        }
    }
    free(word);

    // printing the result
    double result = 0.0;
    if (sen_count) {
        result = (double)matched_count / (double)sen_count;
    }
    printf("%f", result);

    return EXIT_SUCCESS;
}

Some notes:

I don't want to use fixed max length for the word so I use dynamic allocation here.
The reading of sentences is done using two boolean flags. reading_word means are actualy reading a word, and if it's false, we are somewhere in between. It's used for dealing with consecutive separators. matching becomes false when we know that current scanned word doesn't match, so we just scan to the end of the word, not checking anything.

What could be better in this code? Is it "idiomatic" C, or is it obvious that it's written by someone without much language experience?

First, some positive notes. I got no warnings when compiled with gcc using -Wall -Wextra -pedantic. The code seems to work correctly, although the user has to signal EOF from the keyboard, or use file redirection (some warning about this in the question would have been nice.) It is good to see you using size_t for array indices, good that you are checking the results of your functions (i.e., malloc() and realloc()), and good that you are remembering to free() allocated memory.

I like that you do not cast the result of malloc(), though there are those that would disagree on that point.

Concerning realloc(), you are directly assigning the result of realloc() to the pointer to the memory which is being reallocated. This is bad, since realloc() can return a NULL pointer in the event of an error. This would cause a memory leak because you would no longer have a pointer to the previous allocation. You should instead store the result of realloc() in a temporary variable, and test that before reassigning:

char *temp;
...
temp = realloc(word, buff_size);
check_malloc_result(temp);
word = temp;

I don't really understand the need for dynamic allocation here. You only allocate storage for one word; this is not a lot of memory. The method of allocating a reasonable space, and then growing it as needed is sound, but note that if the first word is 17 characters, then you will have to allocate for 32 characters. This is a lot of waste, if you are concerned about space at this scale. You could allocate for one extra character at a time, as needed. I would recommend allocating for a larger initial value (say, 100 chars); if you want to handle possible very large first words, then you can grow this allocation dynamically. Then, if you really want to trim the memory, you can realloc() to the actual length of the word after you have read it.

It would be more idiomatic to use getchar() instead of read() to read from stdin. And, since the specification says that the only separators are spaces and the three punctuation characters, I would consider using '\n' as a way to terminate input to make keyboard entry more friendly:

while((stdin_ch = getchar()) != '\n' && stdin_ch != EOF) {
...
while(word_len && (stdin_ch = getchar()) != '\n' && stdin_ch != EOF) {

Without the test for newlines, this looks much simpler than the read() version of the same code:

while((stdin_ch = getchar()) != EOF) {
...
while(word_len && (stdin_ch = getchar()) != EOF) {

When you calculate result, you don't need to use two casts; the first will do:

result = (double)matched_count / sen_count;

Or, you can do away with the cast altogether by multiplying by 1.0; Note that a floating constant without a suffix is of type double:

result = (1.0 * matched_count) / sen_count;

I personally lean towards the second approach as I prefer to avoid casts whenever possible.

I also added a newline to the format string of your final printf statement, since this is nicer for the user; you might have a reason not to do this, though.

Finally, a note on the indirection operator: opinions vary, but it seems most common to use "right-leaning" asterisks. This is purely a matter of style, but I prefer:

int *var;

to the "left-leaning" version:

int* var;

The only place that I make an exception is in function declarations:

int * func(int *ptr);

There are good reasons for both styles; consistency trumps all in this case.


String pattern matching - method needs improving I'm trying to build simple pattern matching in a string.

ie., Given a template of: "Hello ${1}" and an input of "Hello world", I'd like to be able to return a map of 1='world'.

This test demonstrates what I'm trying to acheive:

@Test
public void detectsTokenValues()
{
    Properties properties = formatter.getTokenValues("${1}/andThen/${2}/followedBy/${3}","start/andThen/middle/followedBy/end");
    assertThat((String) properties.get("1"), equalTo("start"));
    assertThat((String) properties.get("2"), equalTo("middle"));
    assertThat((String) properties.get("3"), equalTo("end"));
}

Here's my implementation.  It works, but I feel like it's convoluted, and I suspect there's Regex or other JDK native features I could be using to make it simpler.

// Matches ${0} - ${9999} etc.,  But not ${variblesWithWords}
private static final Pattern NUMERIC_VARIABLES = Pattern.compile("\\$\\{([0-9]+?)\\}");

Properties getTokenValues(String template, String input) {
    Properties properties = new Properties();
    Matcher matcher = NUMERIC_VARIABLES.matcher(sourcestring);
    final int variableWrapperLength = "${}".length();
    int accumulativeOffset = 0;
    while (matcher.find())
    {
        String variableName = matcher.group(1);
        String value = null;
        if (matcher.end() >= template.length())
        {
            value = input.substring(matcher.start() + accumulativeOffset);
        } else {
            // Find the character after the variable in the template.
            // This serves as the end-delimeter in the input string.
            String endDelimiter = template.substring(matcher.end(),matcher.end()+1);
            int variableEndIndex = findEndDelimiterIndex(input, matcher.start() + 1,
                    endDelimiter);
            value = input.substring(matcher.start() + accumulativeOffset, variableEndIndex);

            // Adjust the accumulative offset to account for the difference
            // in lengths between the variableName (eg., ${1})
            // and the the actual value (eg., 'foo')
            accumulativeOffset += value.length() - (variableWrapperLength + variableName.length());
        }

        if (value != null)
            properties.put(variableName, value);
    }
    return properties;
}

private int findEndDelimiterIndex(String input, int fromIndex,
        String endDelimiter) {

    // Find the first instance of the delimiter in the input string
    // that occurs after the start of our  matches variable.
    if (fromIndex > input.length())
    {
        return Integer.MAX_VALUE;
    }
    int variableEndIndex = input.indexOf(endDelimiter,fromIndex);
    return variableEndIndex;
}

How can I improve this method?

The function getTokenValues is actually a pattern matching function. It just uses a different syntax than the pattern format of java.util.regex.

I suggest you to convert your pattern string (the one with $ signs) into a valid Pattern.

${1}/andThen/${2}/followedBy/${3}

becomes

(.+?)/andThen/(.+?)/followedBy/(.+?)

And then you can use Pattern.matcher to find the content of the groups. There is still one thing missing : the fact that the numbering of the original pattern is lost. For that, you can parse the original pattern string to extract those numbers, to create a mapping table.

When you convert your pattern string into a Pattern, be careful to escape the substrings between the $ using Pattern.quote().

I don't know if this solution would be an improvement of yours, but it's an alternative to consider.

Edit

The implementation of my solution :

Map<String, String> getTokenValues(String pattern, String str) {
    Map<String, String> res = new HashMap<String, String>();
    List<String> identifiers = new ArrayList<String>();

    Pattern dollarRgx = Pattern.compile("\\$\\{([0-9]+?)\\}");
    Matcher dollarMatcher = dollarRgx.matcher(pattern);
    StringBuilder convRgxBd = new StringBuilder();
    int lastEnd = 0;
    while(dollarMatcher.find()) {
        String before = pattern.substring(lastEnd, dollarMatcher.start());
        convRgxBd.append(Pattern.quote(before));
        convRgxBd.append("(.+?)");
        lastEnd = dollarMatcher.end();
        identifiers.add(dollarMatcher.group(1));
    }
    String after = pattern.substring(lastEnd, pattern.length());
    convRgxBd.append(Pattern.quote(after));

    Pattern convRgx = Pattern.compile(convRgxBd.toString());
    Matcher convMatcher = convRgx.matcher(str);
    if(convMatcher.matches()) {
        for(int i = 0; i < convMatcher.groupCount(); i++) {
            String id = identifiers.get(i);
            res.put(id, convMatcher.group(i+1));
        }
    }

    return res;
}