Custom operator new and operator delete In our embedded application, we have to replace ::operator new and ::operator delete to call a special malloc(), to ensure that the memory is allocated correctly. The code, written with this guide in mind, looks like this:

#include <new>
#include <stdexcept>

// Regular scalar new
void* operator new(std::size_t n) throw(std::bad_alloc)
{
    using namespace std;

    for (;;) {
        void* allocated_memory = ::operator new(n, nothrow);
        if (allocated_memory != 0) return allocated_memory;

        // Store the global new handler
        new_handler global_handler = set_new_handler(0);
        set_new_handler(global_handler);

        if (global_handler) {
            global_handler();
        } else {
            throw bad_alloc();
        }
    }
}

// Nothrow scalar new
void* operator new(size_t n, std::nothrow_t const&) throw()
{
    if (n == 0) n = 1;

    return malloc(n);
}

// Regular array new
void* operator new[](size_t n) throw(std::bad_alloc)
{
    return ::operator new(n);
}

// Nothrow array new
void* operator new[](size_t n, std::nothrow_t const&) throw()
{
    return ::operator new(n, std::nothrow);
}

// Regular scalar delete
void operator delete(void* p) throw()
{
    free(p);
}

// Nothrow scalar delete
void operator delete(void* p, std::nothrow_t const&) throw()
{
    ::operator delete(p);
}

// Regular array delete
void operator delete[](void* p) throw()
{
    ::operator delete(p);
}

// Nothrow array delete
void operator delete[](void* p, std::nothrow_t const&) throw()
{
    ::operator delete(p);
}

// Driver function to make the sample code executable.
int main()
{
    int* x = new int;
    delete x;

    int* y = new int[10];
    delete[] y;
}

Is this code correct?

In particular:

Do I need any special alignment handling?
Should there be any other differences between operator new and operator new[]?

I am surprised this compiles and works (it does I test it)

 global_handler();

As global handler is a pointer to a function  I was expecting to see this:

 (*global_handler)();

Comments:

Throw specifiers (except no throw) have been deprecated in the latest standard.
So you should probably follow their lead in that

void* operator new(std::size_t n) throw(std::bad_alloc)

The array version of new calls the no-throw version of new.

void* operator new[](size_t n, std::nothrow_t const&) throw()

Thus you are not getting the benefits of any registered out of memory handlers. Maybe you should just call the normal version of new so that the out of memory handlers are called for arrays as-well.

  Do I need any special alignment handling?

No. malloc() provides memory that is aligned for any type.
http://pubs.opengroup.org/onlinepubs/009695399/functions/malloc.html

  Should there be any other differences between operator new and operator new[]?

No. Apart from not using the out of memory handlers it looks fine.


Add item to an Array 2D using LINQ I've wrote a generic function to add an item to an Array 2D

This is what I have:

Private Sub Add_Item_Array_2D(ByRef Array_2D As String(,), _
                              ByVal Items As String())

    Dim tmp_array(Array_2D.GetUpperBound(0) + 1, Array_2D.GetUpperBound(1)) As String

    For x As Integer = 0 To Array_2D.GetUpperBound(0)
        tmp_array(x, 0) = Array_2D(x, 0)
        tmp_array(x, 1) = Array_2D(x, 1)
    Next

    For x As Integer = 0 To Items.Count - 1
        tmp_array(tmp_array.GetUpperBound(0), x) = Items(x)
    Next

    Array_2D = tmp_array

End Sub

The code works, but I want to know if can be improved using Array.Resize method of LINQ extensions.

This is an usage example:

Private Sub Form1_Load(sender As Object, e As EventArgs) Handles MyBase.shown

    ' Create Array 2D (2,2)
    Dim MyArray As String(,) = _
    {{"Item 0,0", "Item 0,1"}, {"Item 1,0", "Item 1,1"}, {"Item 2,0", "Item 2,1"}}

    ' Add Item
    Add_Item_Array_2D(MyArray, {"Item 3,0", "Item 3,1"})

    ' Loop over the Array 2D
    For x As Integer = 0 To MyArray.GetUpperBound(0)
        MsgBox(String.Format("Array 2D {1},0: {2}{0}Array 2D {1},1: {3}", Environment.NewLine, _
                            x, MyArray(x, 0), MyArray(x, 1)))
    Next

End Sub

Couple nitpicks:

Try to stick to PascalCasing for method names. Using _ underscores not only makes your method names look rather messy, the underscore is conventionally used for event handler methods (like Form1_Load, i.e. object_event).
Consistency is king. Your two snippets aren't consistent as far as parameter naming goes: you're using PascalCasing in your top example, and camelCasing in the bottom one. Pick one, stick to it.
The term generic has a specific meaning in the .net world. That's not a generic function. General-purpose, maybe?

A quick Google search didn't yield much interesting results in VB.net, but this StackOverflow answer has a decent C# version of the code - and it just happens to be an actual generic method:

  Most methods in the array class only work with one-dimensional arrays, so you have to perform the copy manually:

T[,] ResizeArray<T>(T[,] original, int rows, int cols)
{
    var newArray = new T[rows,cols];
    int minRows = Math.Min(rows, original.GetLength(0));
    int minCols = Math.Min(cols, original.GetLength(1));
    for(int i = 0; i < minRows; i++)
        for(int j = 0; j < minCols; j++)
           newArray[i, j] = original[i, j];
    return newArray;
}

Now I know this isn't VB, but an array is an array regardless of the language, so I strongly recommend you take a look at the SO answer, as it goes in much more details than I'd care to quote over here.

Bottom line: You cannot use Array.Resize() with multidimensional arrays.


Sort List By Boolean I have a class called Task that contain a member boolean variable Completed.

I created a list of Task objects in a variable called Tasks.

I have the following code, which sorts the objects depending on whether the Task is completed. (I want the incompleted tasks to be in the list first)

private List<Task> GetSortedTask(List<Task> tasks)
{
    List<Task> completedTaskList = new List<Task>();
    List<Task> incompleteTaskList = new List<Task>();

    for (int i = 0; i < tasks.Count; i++)
    {
        HelperObject T = task[i].getHelperObject();

        if (T != null && T.Completed)
            completedTaskList.Add(tasks[i]);
        else
            incompletetaskList.Add(tasks[i]);
    }

    //merge the two lists together
    imcompleteTaskList.AddRange(completedTaskList);

    return incompleteTaskList;
}

Is there a way to do this without creating so many lists? Thanks in advance

EDIT: Made my question a bit more specific since orderBy seems to be the correct answer, however my problem may be a bit more complicated than that.

I think my original comment in regards to using OrderBy could still possibly still even with your latest edits (and BTW that code does not compile, there is no declared task object :)).

How about something like:

var query = from task in tasks
            let helperTask = task.getHelperObject()
            select new
            {
               Completed = helperTask != null && helperTask.Completed,
               Task = task
            };

return query.OrderBy(p => p.Completed).Select(p => p.Task).ToList();

UPDATE:
Another excellent option suggested by svick to further refine and remove the anonymous object selection is something along the lines of:

var query = from task in tasks
            let helperTask = task.getHelperObject()
            orderby helperTask != null && helperTask.Completed
            select task;

return query.ToList();

I guess if I could I would do away with ToList() altogether and return IEnumerable but that depends on the context of the operation.  I also personally like using an intermediate variable here (query) as I think it just helps readability.  All to their own on that front though.


Using fetch() and a new Promise object to get API results I've written an ES6 function using the new fetch() API and returning a new resolved promise with a data object or a rejected promise with an error subclass.

I'm pretty new to both ES6 and Promises, and it looks a little unwieldy, so I'm wondering if there's a better way. I'm also wondering about how this relates to the "explicit promise construction antipattern".

Any suggestions for how to improve or simplify?

function get(url) {
  console.log('Making fetch() request to: ' + url);

  let promise = new Promise((resolve, reject) => {
    fetch(url).then(response => {
      if (response.ok) {
        const contentType = response.headers.get('Content-Type') || '';

        if (contentType.includes('application/json')) {
          response.json().then(obj => {
            resolve(obj);
          }, error => {
            reject(new ResponseError('Invalid JSON: ' + error.message));
          });
        } else if (contentType.includes('text/html')) {
          response.text().then(html => {
            resolve({
              page_type: 'generic',
              html: html
            });
          }, error => {
            reject(new ResponseError('HTML error: ' + error.message));
          });
        } else {
          reject(new ResponseError('Invalid content type: ' + contentType));
        }
      } else {
        if (response.status == 404) {
          reject(new NotFoundError('Page not found: ' + url));
        } else {
          reject(new HttpError('HTTP error: ' + response.status));
        }
      }
    }, error => {
      reject(new NetworkError(error.message));
    });
  });

  return promise;
}

A few comments:

1) fetch already returns a promise, which means this:

new Promise((resolve, reject) => {
  return fetch(url).then(response => {
    if (response.ok) {
      resolve(response)
    } else {
      reject(new Error('error'))
    }
  }, error => {
    reject(new Error(error.message))
  })
})

Is pretty much the same as:

fetch(url).then(response => {
  if (response.ok) {
    return response
  }
  return Promise.reject(Error('error'))
}).catch(error => {
  return Promise.reject(Error(error.message))
})

2) Keeping this in mind you can simplify your code with early returns and fewer branches:

function get(url) {
  return fetch(url).then(response => {
    if (response.ok) {
      const contentType = response.headers.get('Content-Type') || '';

      if (contentType.includes('application/json')) {
        return response.json().catch(error => {
          return Promise.reject(new ResponseError('Invalid JSON: ' + error.message));
        });
      }

      if (contentType.includes('text/html')) {
        return response.text().then(html => {
          return {
            page_type: 'generic',
            html: html
          };
        }).catch(error => {
          return Promise.reject(new ResponseError('HTML error: ' + error.message));
        })
      }

      return Promise.reject(new ResponseError('Invalid content type: ' + contentType));
    }

    if (response.status == 404) {
      return Promise.reject(new NotFoundError('Page not found: ' + url));
    }

    return Promise.reject(new HttpError('HTTP error: ' + response.status));
  }).catch(error => {
    return Promise.reject(new NetworkError(error.message));
  });
}


c++ is it worth preprocessor optimization for for loops while i < 1 I'm writing some code to take high precision timings of a function call

for (int i = 0; i < iterations; i++) {
    beginTime = high_resolution_clock::now();

    CallFunc(functionPtr, otherParamaters ...); // inline function call to do the timed function

    endTime = high_resolution_clock::now();
    elapsedTime = duration_cast<duration<double>>(endTime - beginTime);
    timings.push_back(elapsedTime.count());
}

However some of the functions I'm timing run so fast the clock can't pick them up, it ends up just writing a load of 0s
I Had the idea to do several repetitions of the function call per timing, to try to get a numerical time I could then divide down past the resolution of the clock, but I don't want to penalize the accuracy of the timing for functions that don't need these repetitions
My solution was to use preprocessor #ifs

for (int i = 0; i < iterations; i++) {
    beginTime = high_resolution_clock::now();

#if repetitions > 1
    for (int i = 0; i < repetitions; i++) {
#endif
        CallFunc(functionPtr, otherParamaters ...); // inline function call to do the timed function
#if repetitions > 1
    }
#endif
    endTime = high_resolution_clock::now();
    elapsedTime = duration_cast<duration<double>>(endTime - beginTime);
    timings.push_back(elapsedTime.count());
}

My question is, is this kind of optimization silly? Is the compiler likely to do this for me anyway?
EDIT: repetitions is a const int

Notational suggestion: if insisting on a bracketed controlled statement, favour
#if 0 < repetitions
# if 1 < repetitions
    for (int i = 0; i < repetitions; i++)
# endif
    {
        CallFunc(functionPtr, otherParamaters …); // inline call to timed function
    }
#endif


Gradient function that returns a list of all colors in order I've made this Function gradient() that returns a list of rbg of all the colors in an order.
Issues:-

What should be the condition for while loop to stop the loop. Though b == 0 in the last elif block works fine.

How can I reduce the code of the function keeping the functionality the same. Like if there is any better approach to doing this.

Also, How can I calculate the length of the rbg_list just with the value of gap parameter without running the function. Let's say the length of the rbg_list will be 1529 if gap=1 so I can iterate it with range(1529).

gradient() function:-
def gradient(gap=1):
    r, g, b = 255, 0, 0

    if gap<=0:
        return (r, g, b)

    rbg_list = []
    while True:
        if  r == 255 and g >= 0 and g < 255 and b == 0:           # 1
            g += gap
            if g > 255:
                g = 255

        elif r <= 255 and g == 255 and r > 0 and b == 0:          # 2
            r -= gap
            if r < 0:
                r = 0

        elif r == 0 and g == 255 and b < 255 and b >= 0:          # 3
            b += gap
            if b > 255:
                b = 255

        elif r == 0 and g <= 255 and g > 0 and b == 255:          # 4
            g -= gap
            if g < 0:
                g = 0

        elif r >= 0 and g == 0 and r < 255 and b == 255:          # 5
            r += gap
            if r > 255:
                r = 255

        elif r == 255 and g == 0 and b > 0 and b <= 255:          # 6
            b -= gap
            if b < 0:
                b = 0

            if b == 0: break

        # print(r, g, b)
        rbg_list.append((r, g, b))
    return rbg_list

print(gradient(5))

Note:- I want the function to be compatible with both Python 2 and Python 3

I rarely do python nowadays, but cleanup of the conditions would be:
def add(c, gap):
    return min(255, c + gap)

def sub(c, gap):
    return max(0, c - gap)

def gradient(gap=1):
    r, g, b = 255, 0, 0

    if gap<=0:
        return (r, g, b)

    rbg_list = []
    while True:
        if  r == 255 and g < 255 and b == 0:         # 1
            g = add(g, gap)

        elif r > 0 and g == 255 and b == 0:          # 2
            r = sub(r, gap)

        elif r == 0 and g == 255 and b < 255:        # 3
            b = add(b, gap)

        elif r == 0 and g > 0 and b == 255:          # 4
            g = sub(g, gap)

        elif r < 255 and g == 0 and b == 255:        # 5
            r = add(r, gap)

        elif r == 255 and g == 0 and b > 0:          # 6
            b = sub(b, gap)

            if b == 0: break

        # print(r, g, b)
        rbg_list.append((r, g, b))
    return rbg_list

Now let me understand what happens. Let us on paper name the 3 states for a color component x (r, g or b):
    a = x == 0
    b = 0 < x < 255
    c = x == 255

    a :   == 0
    bc :  > 0
    c :   == 255
    ab :  < 255

    start: c a a

    c   ab  a    .+.  -->>  c c a
    bc  c   a    -..  -->>  a c a
    a   c   ab   ..+  -->>  a c c
    a   bc  c    .-.  -->>  a a c
    ab  a   c    +..  -->>  c a c
    c   a   bc   ..-  -->>  c a a

A very limited kind of gradient, restricted to one color component.
This  could be written as 6 loops:
        while g < 255:         # 1
            g += gap
            rbg_list.append((r, g, b))

        g = 255
        rbg_list.append((r, g, b))

        while r > 0:           # 2
            r -= gap

        r = 0
        rbg_list.append((r, g, b))

        ...

However you have just increasing from 0 by gap, and decreasing from 255 by gap.
These would give two component value arrays which can then be used with the two other components 0 or 255. That should be exploitable in Python.
You might also calculate how many gap steps are needed for 255, times 6, and have a function that gives for the ith step the (r, g, b). That would be the simplest.
I admit I might be mistaken in what gradient calculation you want to achieve.


Incrementing time in a discrete-event simulation of a supermarket queue A method has been written that increments time in minutes that simulates how much time it takes to process item in a supermarket queue and if there is an empty queue, skip it and loops around to find the number of items processed in each interval (each time a minute has passed). I was wondering if there are ways to improve the code I have written??

private Queue[] lines;
private double time; // Time measured in minutes
private static double item_per_min;
private static double time_inc;
private static Random rand = new Random();
static int x = 0;
public static final double lambda = item_per_min * time_inc;
public static double target_Probability = rand.nextDouble();
public static final double target_ProbabilityabilityOfX = Math.pow(lambda, x) * Math.exp(-lambda) / factorial(x); // Poisson
                                                                                // distribution

public void add_time(double time_inc)
{
    for (int i = 0; i < lines.length; i++) // Loops
    {
        if (lines[i].empty())
        {
            continue;
        }

        while (true)
        {
            time += time_inc;
            if (target_ProbabilityabilityOfX > target_Probability)
            {
                break;
            }

            x++;
            target_Probability -= target_ProbabilityabilityOfX;
        }

        while (!lines[i].empty() && x > 0)
        {
            lines[i].decrement();

            x--;
        }
    }

    this.display();
}

if (probK <= prob) { // increment k until cumulative probability, prob, reached
             k++;
             prob -= probK;
          } else {
             break;
          }

With this pattern, it's often better to write

          if (probK > prob) {
              break;
          }

          k++;
          prob -= probK;

This saves some indent and makes it clear what is happening.  We check for the end condition and then proceed normally if it was not reached.  The other way, it's not obvious what the end condition is, since it is hidden in an else.

But I would actually rewrite the whole loop.

      int k = 0;
      for (double probabilityOfK = Math.exp(-lambda);
              probabilityOfK <= targetProbability;
              probabilityOfK *= lambda / k) {
          targetProbability -= probabilityOfK;
          k++;
      }

The for loop declaration is on three lines to eliminate side scroll on this site.  It might fit on one line in actual code.

I prefer probabilityOfK to probK as being more readable.  I prefer targetProbablity to prob as being more descriptive.

I stopped using Math.pow as a rather expensive calculation.  It is replaced with a simple multiplication.  And you were multiplying anyway, so that's effectively free.

Now it doesn't use a loop forever pattern with a break.  It just loops normally until the condition fails.

This works because you never use cumulativeProbability outside the loop, but each value relates to the previous one.

The result may change slightly due to double rounding.  It's up to you if that matters.  And if it does matter, if the original was better.

    for (int i = 0; i < lines.length; i++) {

I would do this with a range-based for loop.  Something like

    for (Queue<?> line : lines) {

Then you could replace all the lines[i] with just line.  Replace Queue<?> as necessary.

       while (!lines[i].isEmpty() && k > 0) {

Consider something like

        if (k >= line.size()) {
            line.clear();
            continue;
        }

        while (k > 0) {

Now you don't have to check if line is empty, as you'll run out of k before you run out of line.

You could move that check into the previous loop.  Then if you reach the maximum possible k, you can stop immediately rather than continuing.  That might be more efficient.  Something like

        for (Queue<?> line : lines) {
            if (line.isEmpty()) {
                continue;
            }

            updateLine(line);
        }

and

    public void updateLine(Queue<?> line) {
        int k = 0;
        for (double probabilityOfK = Math.exp(-lambda);
                probabilityOfK <= targetProbability;
                probabilityOfK *= lambda / k) {

            targetProbability -= probabilityOfK;
            k++;

            if (k >= line.size()) {
                line.clear();
                return;
            }
        }

        while (k > 0) {
            line.decrement();
            k--;
        }
    }


Optimal algorithm to move a a rope from one position to another I'm an engineer working with a deformable membrane that is attached to actuators.  The goal is to move the membrane from one shape to another, without ripping the membrane.  This imposes "neighbor rules" which state the maximum deviation between neighboring actuators cannot exceed some value, or the membrane will rip.

For the purposes of the problem, let's consider a 1-d membrane or "rope" that is attached to actuators:

The goal is to move the rope from one set of actuator positions to another in the minimum number of moves.

The rules are

Only one actuator may be moved at a time
An actuator may be moved any distance, but
An actuator must respect the neighbor rule, so the distance between it and subsequent actuators cannot exceed a fixed distance

The code below presents two algorithms, neither of which is optimal.  The first algorithm, get_max_deviation_index picks the actuator which is furthest from its final position, then moves it the maximum distance possible.  This algorithm gets stuck on a simple test of moving from position [2, 4, 6, 3] to [-2, -4, -6, -3].  The next algorithm, get_next_index, starts at the first actuator, moves it the maximum distance possible, and then moves to the subsequent actuator and does the same, until it is converged.  This algorithm works, but I do not believe it is optimal.

The code below implements both algorithms; please comment in/out the line beginning with ind = to switch between them.  My question is:

What is the optimal algorithm to move between one position and another?

import numpy as np

def check_legal(y, max_dist=None):
    #checks if a position is legal
    return np.all(np.diff(y)<=max_dist)

def get_neighbors(index, array):
    #gets neighbors of an element in a  list,
    #respecting end nodes
    max_index = len(array)-1
    if index == 0:
        neighbors = [array[index+ 1]]
    elif index == max_index:
        neighbors = [array[index- 1]]
    else:
        neighbors = list(array[[index-1, index + 1]])
    return neighbors

def get_max_deviation_index(current_pos, final):
    diff = np.abs(final-current_pos)
    max_ind = np.where(diff == np.max(diff))[0][0]
    #find move amt necessary to get to final position
    return max_ind

def get_next_index(current_pos, current_ind):
    if current_ind == len(current_pos)-1:
        next_ind = 0
    else:
        next_ind = current_ind+1
    return next_ind

def solve(init, final, max_dist=None):
    assert check_legal(init, max_dist=max_dist)
    assert check_legal(final, max_dist=max_dist)
    assert len(init) == len(final)
    print init
    current_pos = init
    idx = 0
    ind = 0
    while not np.allclose(current_pos, final):
        #find index that has maximum offset from where it should be
        #ind = get_max_deviation_index(current_pos, final)
        ind = get_next_index(current_pos, ind)
        delta = final[ind]-current_pos[ind]
        if delta == 0:
            continue
        sign = np.sign(delta)
        #find neighbors of that point
        neighbors = get_neighbors(ind, current_pos)
        #find the maximum allowable move
        pos_options = [final[ind]]\
                      +[n+sign*max_dist for n in neighbors]
        if sign<0:
            move = max(pos_options)
        else:
            move = min(pos_options)
        current_pos[ind] = move
        print current_pos
        idx+=1
    return idx

if __name__ == "__main__":
    initial_state = np.array([2,4,6, 3])
    final_state = -initial_state
    max_dist = 3 #max distance between neighboring actuators
    ans = solve(initial_state, final_state, max_dist=max_dist)
    print "Completed in ", ans, " moves"

Your bolded question What is the optimal algorithm is off-topic for Code Review. If you're really interested in original-research answers to that question, I think it might plausibly be on-topic for Puzzling SE or Computer Science SE.

Certainly if you post in either of those places, looking for "optimal" algorithms, you will have to define what you mean by "optimal." The simplest way of doing that would also be useful on CodeReview — and also in general in your programming career! Provide test cases.

assert solve([2, 4, 6, 3], [-2, -4, -6, -3], 3) == 42

This test case was constructed by looking at your sample program and then filling in an utterly random number 42 in the one place that matters. When you post the question elsewhere (and/or here again), make sure to have that number filled in.
And then give some other test cases!

assert solve([2, 4, 6, 3], [-2, -4, -6, -3], float('inf')) == 4
assert solve([1, 1, 1], [0, 0, 0], 1) == 3

And then consider what should happen in cases of "invalid input":

assert solve([1, 1, 1], [0, 0, 0], 0) is None
assert solve([1, 2, 1], [1, 3, 1], 1) is None
assert solve([1, 3, 1], [1, 2, 1], 1) is None
assert solve([1, 3, 1], [1, 3, 1], 1) is None

On your code itself, try to write out full identifiers instead of abbreviations: index (index of what?) instead of ind, for example.

When writing a "predicate" function that returns bool, give it a name that indicates the predicate, rather than an action verb: instead of check_legal (which should really be check_legality), prefer is_legal, so that you can write

assert is_legal(init, max_dist)

Notice that max_dist has no reason to be a keyword argument, and no reason to be optional. Instead of passing None and special-casing it, just pass float('inf') and don't write any special cases.

pos_options = [final[ind]]\
                  +[n+sign*max_dist for n in neighbors]

This line of code is impenetrable, due to the lack of whitespace and the backslash in the middle. I'd at least recommend

pos_options = [final[ind]] + [n + sign * max_dist for n in neighbors]

and then look for a way to refactor that or at least put a code comment explaining the logic behind the expression.


Extremely simple timer class in C++ A very bare-bones timer class created using the std::chrono library. I would like to know if any optimisations are possible and just to clarity, I am using MSVC (this code will not compile with certain compilers).

Timer.h:

#pragma once

#include <chrono>

class Timer
{
private:
    std::chrono::time_point<std::chrono::steady_clock> m_StartTime;

public:
    void Start();
    float GetDuration();
};

Timer.cpp:

#include "Timer.h"

void Timer::Start()
{
    m_StartTime = std::chrono::high_resolution_clock::now();
}

float Timer::GetDuration()
{
    std::chrono::duration<float> duration = std::chrono::high_resolution_clock::now() - m_StartTime;
    return duration.count();
}

Keep solid picture of usage in your head

When creating a library, it is extremely important to always keep the intended usage in mind. If the usage syntax is disgusting, it is better to think of a new one before writing the library (of course within reasons). In this case, I believe it is not what you wanted.

Make interfaces easy to use correctly and hard to use incorrectly

Start() function could be built into constructor. If user didn't use Start(), they'll get wrong results. Constructor, on the other hand, will enforce initialization. Never write non-fully-initializable objects, unless it is not possible to do otheriwse (extremely rarely). Then, users will only need to create object at the right place.

Don't mix clocks

Doing this:

std::chrono::time_point<std::chrono::steady_clock> m_StartTime;

and then

m_StartTime = std::chrono::high_resolution_clock::now();

is extremely bad idea. Not all standard libraries alias std::chrono::high_resolution_clock to steady_clock. Though I guess this is a feature of the code rather than a bug. The above will trigger compilation error in libc++ at least.

Know the subject

I know this is extremely simple clock, but since std::chrono::high_resolution_clock is used, I'd expect at least some mechanism to prevent compiler/execution unit optimizations, like std::atomic_thread_fence around it to prevent compiler optimizations and in general, make the execution point in which the time is taken observable. If none of what I said makes any sense to you, then your usage cases don't care about this.

Make it more reusable

I'd expect something like this:

template <typename Clock = std::chrono::high_resolution_lock>
class stopwatch
{
    /*...*/
};

Both time_point and duration are inferrable from Clock. I'd also like to see this:

template <typename Units = std::chrono::nanoseconds, typename Rep = double>
Rep elapsed_time()
{
    //ns chrono omitted for brevity
    return static_cast<Rep>(duration_cast<Units>(/*...*/).count());
}

(The static_cast is just to remove warnings when narrowing is done, which is intentional in this case, as default doesn't perform narrowing).

The project might suggest to change ordering of the template parameters, as Rep might be used more compared to Units.

Minor

Naming

CamelCase is usually used for type names in C++, the rest is camelCase. I personally prefer snake_case for everything, except template parameter and concept names, for which I use CamelCase. It is a matter of preference and consistency though.

Make header only if feasible

In this case, adding .cpp file makes it harder to use. .cpp file doesn't seem to make any difference, as any linkage nuances are not mentioned in the post. Also, making it header-only will make it even more simpler, which is I guess, the intention.

Putting it all together

Final code roughly looks like this:

#include <chrono>
#include <atomic>

namespace shino
{
    template <typename Clock = std::chrono::high_resolution_clock>
    class stopwatch
    {
        const typename Clock::time_point start_point;
    public:
        stopwatch() :
            start_point(Clock::now())
        {}

        template <typename Rep = typename Clock::duration::rep, typename Units = typename Clock::duration>
        Rep elapsed_time() const
        {
            std::atomic_thread_fence(std::memory_order_relaxed);
            auto counted_time = std::chrono::duration_cast<Units>(Clock::now() - start_point).count();
            std::atomic_thread_fence(std::memory_order_relaxed);
            return static_cast<Rep>(counted_time);
        }
    };

    using precise_stopwatch = stopwatch<>;
    using system_stopwatch = stopwatch<std::chrono::system_clock>;
    using monotonic_stopwatch = stopwatch<std::chrono::steady_clock>;
}

As you probably noticed, I changed default template arguments in elapsed_time(), and also incorporated const correctness from @JDługosz answer. The defaults change is to support all possible clocks and still avoid lossy conversionsby default.

Lets use this:

Measuring scheduler overhead using stopwatch

The measurements are rather naive:

#include <iostream>
#include <thread>

int main()
{
    std::cout << "testing scheduler overhead\n";
    using namespace std::literals;
    for (auto wait_time = 100ms; wait_time <= 1s; wait_time += 100ms)
    {
        shino::precise_stopwatch stopwatch;
        std::this_thread::sleep_for(wait_time);
        auto actual_wait_time = stopwatch.elapsed_time<unsigned int, std::chrono::microseconds>();
        std::cout << "Scheduler overhead is roughly " << actual_wait_time - (wait_time + 0us).count() << " microseconds"
                  << " for " << wait_time.count() << " milliseconds of requested sleep time\n";
    }
}

(Weird +0us is to let compiler handle the conversion from ms to us)

On my system gives:

testing scheduler overhead
Scheduler overhead is roughly 114 microseconds for 100 milliseconds of requested sleep time
Scheduler overhead is roughly 110 microseconds for 200 milliseconds of requested sleep time
Scheduler overhead is roughly 75 microseconds for 300 milliseconds of requested sleep time
Scheduler overhead is roughly 111 microseconds for 400 milliseconds of requested sleep time
Scheduler overhead is roughly 111 microseconds for 500 milliseconds of requested sleep time
Scheduler overhead is roughly 107 microseconds for 600 milliseconds of requested sleep time
Scheduler overhead is roughly 110 microseconds for 700 milliseconds of requested sleep time
Scheduler overhead is roughly 110 microseconds for 800 milliseconds of requested sleep time
Scheduler overhead is roughly 112 microseconds for 900 milliseconds of requested sleep time
Scheduler overhead is roughly 93 microseconds for 1000 milliseconds of requested sleep time

Full code to copy and run

#include <chrono>
#include <atomic>

namespace shino
{
    template <typename Clock = std::chrono::high_resolution_clock>
    class stopwatch
    {
        const typename Clock::time_point start_point;
    public:
        stopwatch() :
            start_point(Clock::now())
        {}

        template <typename Rep = typename Clock::duration::rep, typename Units = typename Clock::duration>
        Rep elapsed_time() const
        {
            std::atomic_thread_fence(std::memory_order_relaxed);
            auto counted_time = std::chrono::duration_cast<Units>(Clock::now() - start_point).count();
            std::atomic_thread_fence(std::memory_order_relaxed);
            return static_cast<Rep>(counted_time);
        }
    };

    using precise_stopwatch = stopwatch<>;
    using system_stopwatch = stopwatch<std::chrono::system_clock>;
    using monotonic_stopwatch = stopwatch<std::chrono::steady_clock>;
}

#include <iostream>
#include <thread>

int main()
{
    std::cout << "testing scheduler overhead\n";
    using namespace std::literals;
    for (auto wait_time = 100ms; wait_time <= 1s; wait_time += 100ms)
    {
        shino::precise_stopwatch stopwatch;
        std::this_thread::sleep_for(wait_time);
        auto actual_wait_time = stopwatch.elapsed_time<unsigned int, std::chrono::microseconds>();
        std::cout << "Scheduler overhead is roughly " << actual_wait_time - (wait_time + 0us).count() << " microseconds"
                  << " for " << wait_time.count() << " milliseconds of requested sleep time\n";
    }
}

Wandbox demo.


Count words that are longer than a set Length This is working, but I want to know if is this a place where IEnumerable can be used?

public static int LongerThanStingCompair(string s, int n)
    {

        int counter = 0;
        // splits based on nextLine break
        var sCount = s.Split('\n');
        // Cast to list
        sCount.ToList();
        foreach (var item in sCount)
        {
            //checks to see if words are a certain charLength
            if (item.Length > n)
            {
                counter++;
            }
        }

        return counter;
    }

Test class gives it the input like

  Lion, Ape, Tiger, Zebra and 4

So it would return 2 since there are two words with more than 4 letters

Signature:

public static int LongerThanStingCompair(string s, int n)

Compair is misspelled, as is Sting - they should be Compare and String, respectively. Although, a better name might be CountLinesLongerThan.

s and n aren't great names for parameters.

On to your code:

// splits based on nextLine break
var sCount = s.Split('\n');

The comment is unnecessary and slightly wrong. The code splits on all new line characters.

// Cast to list
sCount.ToList();

This comment is also incorrect - that doesn't simply cast it creates a new list from an array. It's unnecessary because you only need to enumerate the lines - you don't need the functionality of the list.

Here's a Linq based alternate for you:

public static int CountLinesLongerThan(string source, int characterLimit)
{
    if (source == null)
    {
        throw new ArgumentNullException("source");
    }
    return source.Split('\n')
        .Count(line => line.Length > characterLimit);
}


Array indexing with copy constructor, assignment operator and index operator overload The code below is a custom array class that handles indexing, copying, printing, etc. explicitly.
Is there a better approach for specification (declaration and definition) of the assignment operator (with swap function) and the index [] operator?
Furthermore, are there any improvements that can be made to the other methods to improve functionality?

// Overloading operators for Array class
#include<iostream>
#include<cstdlib>

using namespace std;

// A class to represent an integer array
class Array{
private:
    int *ptr;
    int size;
public:
    Array(int *p = NULL, int s = 0);
    Array(const Array&);
    ~Array();
    Array& operator= (Array);

    // Overloading [] operator to access elements in array style
    int &operator[] (int);

    // Utility function to print contents
    void print() const;

    friend void swap(Array& first, Array& second);};

// Implementation of [] operator.  This function must return a
// reference as array element can be put on left side
int &Array::operator[](int index){
    if (index >= size || index < 0){
       throw out_of_range("Index out of Range error");
    }
    return ptr[index];
}

// constructor for array class
Array::Array(int *p, int s){
    size = s;
    ptr = NULL;
    if (s != 0){
        ptr = new int[s];
        for (int i = 0; i < s; i++)
            ptr[i] = p[i];}
}

// destructor for array class
Array::~Array(){
    delete[] ptr;
    ptr = NULL;}

// copy constructor for array class
Array::Array(const Array& A) {
    size = A.size;
    ptr  = new int[size];
    for (int i = 0; i < size; i++)
        ptr[i] = A.ptr[i];}

//swap friend function of assignment operator
void swap(Array& first, Array& second){
    using std::swap;
    swap(first.size, second.size);
    swap(first.ptr, second.ptr);}

//Assignment operator for array class
Array& Array::operator=(Array other){
    swap(*this, other);
    return *this;}

//print function for array elements
void Array::print() const{
    cout << "{";
    for(int i = 0; i < size; i++)
        cout<<ptr[i]<<" ";
    cout<<"}"<<endl;}

// Driver program to test above methods
int main()
{
    int a[] = {1, 2, 3, 4, 5, 6};
    Array arr1(a, 6);
    arr1[0] = 7;
    arr1.print();
    Array arr2 = arr1;
    arr2.print();
    arr1[-1] = 4;
    return 0;
}

This isn't really a code review so much as help on how to get this to compile.

Array::Array(int *p = NULL, int s = 0){ default parameters go on the declaration, not the definition.*
Array::Array& operator=(Array other){ Qualify the operator with Array::, not the return type. You're not returning an Array::Array**, you're defining the operator= member function of Array
void Array::swap(Array& first, Array& second){ swap is not a member of array, it's a friend. Remove the Array::

*This is because the client (caller) to the method needs to know what the defaults are supposed to be, since default parameters are mostly a syntactic shortcut. If you put the defaults into the definition (e.g. a cpp/.o file), a client which includes the header cannot see the definition and thus cannot use them. In other words, there exists no Array::Array() constructor right now, there's only the Array::Array(int *, int), but through C++ magic it can look as though Array::Array() is being called.

** An Array::Array type is impossible because you can't name a child type the same as its parent type. It would be ambiguous with the constructor.


Render dynamically vertical tabs and set classes depends on if statement I would like to refactor code below and get rid of if statements for classes from view.

.container
  .tabbable.tabs-left
    %ul.nav.nav-tabs
      - @related_tasks.each do |task|
        %li{class: ("active" if @task == task)}
          = link_to task.name, "#tab#{task.id}", data: { toggle: 'tab' }, class: @current_user_tasks.include?(task.id) ? '' : 'inactive'

    .tab-content
      - @related_tasks.each do |task|
        .tab-pane{id: "tab#{task.id}", class: ("active" if @task == task)}
          = render 'task_tab', task: task

Helper methods are good for this.  In your view's helper file:

def task_link_class(task)
  if @current_user_tasks.include?(task)
    ''
  else
    'inactive'
  end
end

Some would prefer the trinary operator instead:

def task_link_class(task)
  @current_user_tasks.include?(task) ? '' : 'inactive'
end

And in your view:

= link_to task.name, "#tab#{task.id}", data: { toggle: 'tab' }, class: task_link_class(task)


Divide an alphanumeric string into a map of numbers and strings Below is a bit of code which divides an alphanumeric string to a map of numbers and strings (assume numbers are single digits and non-repeating).

Example input:

String alphaNumericStr = "1abcdef2xyz9aaa6xxx";

Output:

Map<Integer, String> map = {1=abcdef, 2=xyz, 9=aaa, 6=xxx}

This is the code i wrote. While it gets the work done, this does not look like a clean solution to me. How can I make this better?

    String alphaNumericStr = "1abcdef2xyz9aaa6xxx";

    Map<Integer, String> map = new LinkedHashMap<Integer, String>();

    Character key = null;
    StringBuffer s = new StringBuffer();

    for (int i = 0; i < alphaNumericStr.length(); i++) {
        Character c = alphaNumericStr.charAt(i);

        if (Character.isDigit(c)) {
            if (s.length() > 0) {
                map.put(Character.getNumericValue(key), s.toString());
                s = new StringBuffer();

            }
            key = c;

        } else {
            s.append(c);
            if (i == alphaNumericStr.length() - 1) {
                map.put(Character.getNumericValue(key), s.toString());
            }
        }
    }

    System.out.println(map);

You can use this regex (\d+)(\D+) which will match two groups, one is to match the number and the second to match the non digit part, beside If you are using Java 9+ Pattern, you can use simply :

String input = "1abcdef2xyz9aaa6xxx";
String regex = "(\\d+)(\\D+)";

Map<Integer, String> map = Pattern.compile(regex)
        .matcher(input)
        .results()
        .collect(Collectors.toMap(
                m -> Integer.parseInt(m.group(1)), m -> m.group(2)
        ));

System.out.println(map);

Result

{1=abcdef, 2=xyz, 9=aaa, 6=xxx}

If you are using Java 8 or lower, you can use :

String alphaNumericStr = "1abcdef2xyz9aaa6xxx";
String regex = "(\\d+)(\\D+)";
Matcher matcher = Pattern.compile(regex).matcher(alphaNumericStr);

Map<Integer, String> map = new HashMap<>();
while (matcher.find()) {
    map.put(Integer.parseInt(matcher.group(1)), matcher.group(2));
}
System.out.println(map);

But you will lose some information if the input contain the same number, in this case I would like to create a class which hold a number and a String and use a list in this case instead.


Split Collection to two list depend on criteria Let's say I have a List contains numbers:

1,2,3,4,5,6,7,8,9

and I would like to split this to two lists:

odd numbers
even numbers

So I can do this two ways:

Solution 1:

Create two methods:

List<Integer> filterOdd(List<Integer> numbers){
    List<Integer> result  = new ArrayList<Integer>();
    for(Integer n : numbers){
        if(n % 2 != 0){
            result.add(n);
        }
    }
    return result;
}

List<Integer> filterEven(List<Integer> numbers){
    List<Integer> result  = new ArrayList<Integer>();
    for(Integer n : numbers){
        if(n % 2 == 0){
            result.add(n);
        }
    }
    return result;
 }

and in the code I will call:

List<Integer> numbers = Arrays.asList(new Integer[] { 1,2,3,4,5,6,7,8,9 });

List<Integer> oddNumbers = filterOdd(numbers);
List<Integer> evenNumbers = filterEven(numbers);

and do something with these lists.

Disadvantage:

I do two loops over one collection.

Solution 2:

Create one method:

public void filter(List<Integer> numbers, List<Integer> oddNumbers, List<Integer> evenNumbers){
    for(Integer n : numbers){
        if(n % 2 != 0){
            oddNumbers.add(n);
        }else{
            evenNumbers.add(n);
        }
    }
}

and in the code I will call:

List<Integer> numbers = Arrays.asList(new Integer[] { 1,2,3,4,5,6,7,8,9 });
List<Integer> oddNumbers  = new ArrayList<Integer>();
List<Integer> evenNumbers = new ArrayList<Integer>();
filter(numbers, oddNumbers, evenNumbers);

Disadvantage:

I hear that assigning results for parameters is bad practice.

Which solution is better?

Here is a solution which replicates Guava's Predicate interface. Since this interface is really easy, here is how it is done:

public interface Predicate<T>
{
    // returns true if input obeys the predicate
    boolean apply(T input);
}

Given this interface, implement it for your types; and then do a method like this:

public static <T> List<List<T>> filteredLists(final List<T> list,
    final List<Predicate<T>> predicates)
{
    final int size = predicates.size();
    final List<List<T>> ret = new ArrayList<List<T>>(size);

    // Fill ret with initial lists
    for (int i = 0; i < size; i++)
        ret.add(new ArrayList<T>());

    // Now walk the predicates and add to the necessary lists

    for (final T element: list)
        for (int i = 0; i < size; i++)
            if (predicates.get(i).apply(element))
                ret.get(i).add(element);

    // Done! Return...
    return ret;
}

Now, if you had a utility class, say MyPredicates, with two methods even() and odd(), you could write:

final List<List<Integer>> filtered = filteredList(inputList,
    Arrays.asList(MyPredicates.even(), MyPredicates.odd()));

The "drawback" here is that it is up to the caller to remind what predicates where in what order in the calling lists, of course.

As to your MyPredicates hypothetical class:

public static final class MyPredicates
{
    // No instansiation
    private MyPredicates()
    {
    }

    public static Predicate<Integer> even()
    {
        return new Predicate<Integer>()
        {
            @Override
            public boolean apply(final Integer input)
            {
                return input.intValue() % 2 == 0;
            }
        }
    }

    public static Predicate<Integer> odd()
    {
        return new Predicate<Integer>()
        {
            @Override
            public boolean apply(final Integer input)
            {
                return input.intValue() % 2 == 1;
            }
        }
    }
}


Is it necessary to replace this cursor in SQL Server 2005? The Problem

I have a cursor that I am trying to replace (perhaps unnecessarily) in an attempt to clean up a stored procedure. Essentially what it is doing is counting each note for each member in a temp table and then assigning each of those notes a sequential value into the field NOTE_COUNTER (int) of the temp table. Here is the original code for it with the variables:

@lUPI_current          CHAR(32),
@lCASE_SEQ_current     INT,
@lDIFF_ENCOUNTER_DATE  INT,
@lcounter              INT

SET @lUPI_current = ''
SET @lCASE_SEQ_current = ''
SET @lDIFF_ENCOUNTER_DATE = 0
SET @lcounter = 0

DECLARE CURSOR_NOTE_COUNTER CURSOR FOR
SELECT
    MEMBER,
    CASE_SEQ,
    ENCOUNTER_DATE
FROM
    #TMP_NOTES
WHERE
    TOTAL_CALLS > 1
ORDER BY
    MEMBER,
    CASE_SEQ,
    ENCOUNTER_DATE

OPEN CURSOR_NOTE_COUNTER

FETCH NEXT FROM CURSOR_NOTE_COUNTER
INTO
  @lUPI_current,
  @lCASE_SEQ_current,
  @lDIFF_ENCOUNTER_DATE

WHILE (@@FETCH_STATUS = 0)
BEGIN
    SET @lcounter = (
                        SELECT
                            MAX(NOTE_COUNTER)
                        FROM
                            #TMP_NOTES
                        WHERE
                            MEMBER_UPI = @lUPI_current
                            AND CASE_SEQ = @lCASE_SEQ_current
                    )
UPDATE #TMP_NOTES
    SET
        NOTE_COUNTER = @lcounter + 1
    FROM #TMP_NOTES
    WHERE
        MEMBER_UPI = @lUPI_current
        AND CASE_SEQ = @lCASE_SEQ_current
        AND DIFF_ENCOUNTER_DATE = @lDIFF_ENCOUNTER_DATE

    FETCH NEXT FROM CURSOR_NOTE_COUNTER
    INTO
        @lUPI_current,
        @lCASE_SEQ_current,
        @lDIFF_ENCOUNTER_DATE
END

What I Have Tried

I tried replacing this with a CTE because of the notion that cursors should be avoided in favor of working on sets of data. Here was my attempt at that, the stumbling block being the ambiguity of the column names since it is working on the same table.

WITH RecordsBeingUpdated AS
    (
        SELECT
            MEMBER_UPI,
            CASE_SEQ,
            DIFF_ENCOUNTER_DATE,
            ROW_NUMBER() OVER (PARTITION BY MEMBER_UPI, CASE_SEQ, DIFF_ENCOUNTER_DATE ORDER BY DIFF_ENCOUNTER_DATE ASC) AS RowNum
        FROM
            #TMP_NOTES
        WHERE
            TOTAL_CALLS > 1
    )

UPDATE #TMP_NOTES
    SET
        NOTE_COUNTER = R.RowNum
    FROM
        RecordsBeingUpdated AS R
    WHERE
       MEMBER_UPI = R.MEMBER_UPI
       AND CASE_SEQ = R.CASE_SEQ
       AND DIFF_ENCOUNTER_DATE = R.DIFF_ENCOUNTER_DATE

Please tell me if there is a cleaner way to doing this. I am grateful for any time that the community can provide.

So, you can simplify the CTE with some SQL magic:

WITH RecordsBeingUpdated AS
(
    SELECT
        NOTE_COUNTER,
        MEMBER_UPI,
        CASE_SEQ,
        DIFF_ENCOUNTER_DATE,
        ROW_NUMBER() OVER (PARTITION BY MEMBER_UPI, CASE_SEQ ORDER BY DIFF_ENCOUNTER_DATE ASC) AS RowNum
    FROM
        #TMP_NOTES
    WHERE
        TOTAL_CALLS > 1
)

UPDATE RecordsBeingUpdated
SET    NOTE_COUNTER = RowNum

Essentially, the virtual table built for the CTE has direct reference back to the concrete tables in the selection. This means you can update the CTE directly and SQL will update the corresponding concrete table within, provided the target table is the FROM table (not a joined) and there is no chance of ambiguity in what needs to be updated.

(Removed DIFF_ENCOUNTER_DATE from partition per comments).


Array of date by each week I have two dates, example :

date1 = "4-02-2017".to_date
date2 = "4-02-2017".to_date + 29

I try to get array of date by each week with this :

(date1..date2).select{|x| Date::DAYNAMES[x.wday] == Date::DAYNAMES[date1.wday] }

Rersult :

[Sat, 04 Feb 2017,
 Sat, 11 Feb 2017,
 Sat, 18 Feb 2017,
 Sat, 25 Feb 2017,
 Sat, 04 Mar 2017]

Is there a better way to solve the case other than the way as above?

There's no reason to look up the day's name in the DAYNAMES array. You already have the wday values, so you can just compare those directly.

I.e. if

Date::DAYNAMES[x] == Date::DAYNAMES[y]

it follows that

x == y

so:

(date1..date2).select { |date| date.wday == date1.wday }

Or, simpler, you can specify a step for the range and just skip one week at a time:

(date1..date2).step(7).to_a

Now, a week is typically 7 days long, but there have been a few cases of nations choosing to officially move to the other side of the international dateline, causing a week to not be 7 days long in their calendar. It's rare of course, but something to be aware of.


Wrap email quotes with html element I'm wondering if I could handle the following situation with a single function or regex call. Given the folowing input:

Some text

> Foo
> Bar
>> Baz
> Foo

The output should be:

Some text

Foo
Bar

        Baz

Foo

The following two functions do handle this situation, but I think there must be a more elegant way to handle it. I thought about regular expressions, but could not think of any pattern that replaces the input with the output (see above).

<?php
function toArray($str, $currLevel = 0, $i = 0) {
  $return = array();
  $lines = explode("\n", $str);
  for($j = count($lines); $i < $j; $i++) {
    $line = trim($lines[$i]);
    $level = preg_replace('/^((?:>|\s)+).*/','\1', $line);
    $level = substr_count($level, '>');
    if($level == $currLevel) {
      array_push($return, preg_replace('/^((?:>|\s)+)(.*)/','\2', $line));
    }
    else if($level > $currLevel) {
      array_push($return, toArray(join("\n", $lines), $currLevel  + 1, &$i));
    } else if($level < $currLevel) {
      $i--;
      return $return;
    }
  }

  return $return;
}

function toQuote($lines) {
  $return = "
\n";
  foreach($lines as $line) {
    if(is_array($line)) {
      $return .= toQuote($line);
    }
    else {
      $return .= $line . "\n";
    }
  }
  $return .= "
\n";
  return $return;
}

$str = <<<INPUT
  Some text

  > Foo
  > Bar
  >> Baz
  > Foo
INPUT;

echo toQuotes(toArray($str));

This isn't much more elegant, but maybe you like it:

$text = "Some text

> Foo
> Bar
>> Baz
> Foo
";

function clean($text) {
  $result = "";
  $lines = explode("\n", $text);
  foreach($lines as $key => $line) {
    $lines[$key] = preg_replace('/(>)+ /', '', $line);
  }
  return join("\n", $lines);
}

function quotes($text, $level = 1) {
  $search = str_repeat(">", $level);
  $fstpos = strpos($text, $search);
  $sndpos = strrpos($text, $search);
  $sndpos = strpos($text, "\n", $sndpos);
  $middle = substr($text, $fstpos, $sndpos + 1);
  if($fstpos === false or $sndpos === false)
    return clean($middle, $search);
  $fst = clean(substr($text, 0, $fstpos), $search);
  $snd = clean(substr($text, $sndpos), $search);
  return $fst . "<blockquote>\n" . quotes($middle, $level + 1)  . "</blockqoute>" . $snd;
}

echo quotes($text);


C++ determinant calculator I've wrote a C++ program to calculate the determinant of a matrix:

#include <iostream>
#include <string>

double getDeterminant(double arr[], int dimension);

int main() {

    //First, the user has to enter the dimension of the matrix
    int dimension;
    std::cout << "Please enter dimension of Matrix: ";
    std::cin >> dimension;
    std::cout << std::endl;
    double matrix[dimension][dimension];

    //Now, the user has to enter the matrix line by line, seperated by commas
    std::string str;
    for(int i = 1; i <= dimension; i++) {
        std::cout << "Enter line " << i << " only seperated by commas: ";
        std::cin >> str;
        std::cout << std::endl;
        str = str + ',';
        std::string number;
        int count = 0;
        for(int k = 0; k < str.length(); k++) {
            if(str[k] != ',') {
                number = number + str[k];
            }
            else {
                matrix[i - 1][count] = std::stod(number);
                number = "";
                count++;
            }
        }
    }

    //Conversion to a onedimensional matrix to be able to give it over as a parameter
    double array[dimension * dimension];
    int k = 0;
    for(int i = 0; i < dimension; i++) {
        for(int j = 0; j < dimension; j++) {
            array[k] = matrix[i][j];
            k++;
        }
    }

    //Output
    for(int i = 0; i < dimension; i++) {
        for(int j = 0; j < dimension; j++) {
            std::cout << matrix[i][j] << " ";
        }
        std::cout << std::endl;
    }

    std::cout << "Determinant of the matrix is : " << getDeterminant(array, dimension) << std::endl;
    return 0;
}

double getDeterminant(double array[], int dimension) {

    //Formula for 2x2-matrix
    if(dimension == 2) {
        return array[0] * array[3] - array[1] * array[2];
    }

    //Conversion back to 2D-array
    double matrix[dimension][dimension];
    int k = 0;
    for(int i = 0; i < dimension; i++) {
        for(int j = 0; j < dimension; j++) {
            matrix[i][j] = array[k];
            k++;
        }
    }

    double result = 0;
    int sign = 1;
    for(int i = 0; i < dimension; i++) {

        //Submatrix
        double subMatrix[dimension - 1][dimension -1];
        for(int m = 1; m < dimension; m++) {
            int z = 0;
            for(int n = 0; n < dimension; n++) {
                if(n != i) {
                    subMatrix[m-1][z] = matrix[m][n];
                    z++;
                }
            }
        }

        //Conversion of the submatrix to 1D-array
        double array2[(dimension - 1) * (dimension - 1)];
        int k = 0;
        for(int x = 0; x < dimension - 1; x++) {
            for(int y = 0; y < dimension - 1; y++) {
                array2[k] = subMatrix[x][y];
                k++;
            }
        }

        //recursive call
        result = result + sign * matrix[0][i] * getDeterminant(array2, dimension -1);
        sign = -sign;
    }

    return result;
}

I would appreciate any suggestions on improving the code!

You can find the follow-up question here.

double matrix[dimension][dimension];

double array[dimension * dimension];

double matrix[dimension][dimension];

    double subMatrix[dimension - 1][dimension -1];

    double array2[(dimension - 1) * (dimension - 1)];

None of those are legal C++, because dimension isn't a constant-expression.

We have a signed/unsigned comparison here:

    for(int k = 0; k < str.length(); k++) {

We can easily eliminate the compiler warning by using a more appropriate type:

    for (std::size_t k = 0;  k < str.length();  ++k) {

But a better fix, given we only use k to index str, is to use a range-based loop:

    for (auto const c: str) {

When using operator>> on a stream, we must always check that it succeeded, before we depend on the result:

std::size_t dimension;
std::cin >> dimension;
if (!std::cin) {
    std::cerr << "Input failed\n";
    return EXIT_FAILURE;   // needs <cstdlib>
}
if (dimension == 0) {
    std::cout << "1\n";    // empty matrix determinant
}


AngularJS, Directive improvement I wrote my very first directive and I'd like to get some suggestions for improvement.
My code - plnkr.

webApp.controller('AppCtrl', function ($scope) {

$scope.expandAdd = function() {
    $scope.hiddenAdd = true;

};

$scope.addWeb = function() {
    $scope.hiddenAdd = false;
};

});

webApp.directive('enter',function(){
return {

    link: function (scope, element, attrs){

        scope.hiddenAdd = false;   //hides the site details at first
        //Invoke expender on the controller on mouse click
        element.bind('click', function(){
            console.log('clicked');
            scope.$apply(attrs.enter);
        });
    }
}
});

I have three points I think I can improve here but I'm not sure how:

My directive is not modular enough, the line scope.hiddenAdd = false is bound to hiddenAdd. Is it possible to make it more flexible for future use?
Though it seems two be picking to different bindings I think that is because they are both start with hidden.
Inside the controller I'm doing DOM manipulation  $scope.hiddenAdd = true; & $scope.hiddenAdd = false; from what I've read it all best be inside directives and I can't find the way to make it a pure directive.
I read somewhere that it is not recommended to use $apply too often, is there a way to avoid using apply in my case?

Option 1: Don't use a directive.
What you are trying to do does not actually require a directive. It is simple enough to play with the scope.

  $scope.expandAdd = function() {
    $scope.hiddenAdd = true;
  };

  $scope.addWeb = function() {
    $scope.hiddenAdd = false;
  };

plnkr


Speed up query to rank team sales by aggregating data from two databases On my machine it runs in about 60 seconds and about 2-3 minutes live.

The criteria needed are:

Current month only (from first of month until current day)
I need the rank of the currently logged in person based on total sales.
I have two tables across two databases where this data is to be gathered:

Database 1: ProPit_User table (this houses the logged in users).
Database 2: This houses the sales in the Orders table.

Both tables will be linked on the SalesRepID column.
There are around around 352 users in the ProPit_User table.
There are a lot of orders in the orders table.

This code does what I need it to do; it is just dreadfully slow.

public static int GetNationalRank(int teamId, int CurrentUserId)
{
    List<ProPit_User> lstProPit_User = new List<ProPit_User>();
    List<TeamSales> lstTeamSales = new List<TeamSales>();
    int currentUserRank = 0;

    using (ProsPitEntities ppdb = new ProsPitEntities())
    using (TraegerEntities db = new TraegerEntities())
    {
        lstProPit_User = ppdb.ProPit_User.Where(x => x.SalesRepID != null).ToList();

        foreach (var user in lstProPit_User)
        {

            var strStartDate = DateTime.Now.AddDays(-(DateTime.Now.Day - 1)).ToString("M/d/yyyy");
            var dtStartDate = Convert.ToDateTime(strStartDate);

            var strEndDate = DateTime.Now.AddDays(1).ToString("M/d/yyyy");
            var dtEndDate = Convert.ToDateTime(strEndDate);

            var orders = db.Orders.Where(o => o.DateCompleted >= dtStartDate
                                        && o.DateCompleted < dtEndDate
                                        && (o.Status == 1 || o.Status == 2)
                                        && o.Kiosk != 0).AsEnumerable();

            var total = (from o in orders
                         where o.SalesRepID == user.SalesRepID
                         select o.OrderTotal).Sum();

            TeamSales TeamSales = new TeamSales();

            TeamSales.userId = user.userID;
            TeamSales.TotalSales = total;

            lstTeamSales.Add(TeamSales);

            }

        }

        int rank = lstTeamSales.OrderByDescending(x => x.TotalSales).FindIndex(x => x.userId == CurrentUserId);

        if (rank == -1)
        {
            currentUserRank = 0;
        }
        else
        {
            // Zero based index need to add one to rank
            currentUserRank = rank + 1;
        }

    }

    return currentUserRank;
}

I have .FindIndex added as an extension method to IEnumerable.

All I really need is to get the rank of the person logged in in comparison to everyone else sales.

Try what's below on for size. The date calculation is moved out of the loop, the TeamSales list capacity is pre-allocated and finally, the two LINQ statements are combined into one and rolled into AddRange rather than the explicit loop.

public static int GetNationalRank(int teamId, int CurrentUserId)
{
    int currentUserRank;
    var startDate = Convert.ToDateTime(DateTime.Now.AddDays(-(DateTime.Now.Day - 1)).ToString("M/d/yyyy"));
    var endDate = Convert.ToDateTime(DateTime.Now.AddDays(1).ToString("M/d/yyyy"));
    List<ProPit_User> proPitUsers;

    using (var ppdb = new ProsPitEntities())
    {
        proPitUsers = ppdb.ProPit_User.Where(x => x.SalesRepID != null).ToList();
    }

    var teamSales = new List<TeamSales>(proPitUsers.Count());

    using (var db = new TraegerEntities())
    {
        teamSales.AddRange(proPitUsers
            .Select(user => new
            {
                user,
                total = db.Orders.Where(o => o.DateCompleted >= startDate
                    && o.DateCompleted < endDate
                    && (o.Status == 1 || o.Status == 2)
                    && o.Kiosk != 0
                    && o.SalesRepID == user.SalesRepID).Select(o => (decimal?)o.OrderTotal).Sum() ?? 00.0M
            })
            .Select(userTotal => new TeamSales { userId = userTotal.user.userID, TotalSales = userTotal.total }));

        var rank = teamSales
            .OrderByDescending(x => x.TotalSales)
            .FindIndex(x => x.userId == CurrentUserId);

        currentUserRank = rank == -1 ? 0 : rank + 1;
    }

    return currentUserRank;
}


Search utility that searches through source code files I'm working on a search utility that searches through source files. It's only the beginning of the project. It lacks a lot of features, but a backbone of the project works and it would be great if you can take a look at it.

path, keyword = ARGV if ARGV.length == 2

if ARGV.length != 2
  puts "Not enough argumetns. Type 'finder --help' for help"
  abort if ARGV.length != 2
end

IGNORED_DIRS = ['..', '.', '.git', 'blib', '_build', '.bzr', '.cdv', 'cover_db',
                '__pycache', 'CVS', '_darcs', '~.dep', '~.dot', '.hg', '~.nib', '.pc', '~.plst', 'RCS', 'SCCS',
               '_sgbak', '.svn', '.tox','.metadata', '.cover']

# colorize output
class String
  def black;          "\033[30m#{self}\033[0m" end
  def red;            "\033[31m#{self}\033[0m" end
  def green;          "\033[32m#{self}\033[0m" end
  def brown;          "\033[33m#{self}\033[0m" end
  def blue;           "\033[34m#{self}\033[0m" end
  def magenta;        "\033[35m#{self}\033[0m" end
  def cyan;           "\033[36m#{self}\033[0m" end
  def gray;           "\033[37m#{self}\033[0m" end
  def bg_black;       "\033[40m#{self}\033[0m" end
  def bg_red;         "\033[41m#{self}\033[0m" end
  def bg_green;       "\033[42m#{self}\033[0m" end
  def bg_brown;       "\033[43m#{self}\033[0m" end
  def bg_blue;        "\033[44m#{self}\033[0m" end
  def bg_magenta;     "\033[45m#{self}\033[0m" end
  def bg_cyan;        "\033[46m#{self}\033[0m" end
  def bg_gray;        "\033[47m#{self}\033[0m" end
  def bold;           "\033[1m#{self}\033[22m" end
  def reverse_color;  "\033[7m#{self}\033[27m" end
end

# convert given path to full path
# which can be used in 'Dir.chdir()'
def expand_path(path)
  case path
  when '.'
    Dir.pwd
  when /~(\/[a-zA-Z\w]*)+/
    File.expand_path(path, __FILE__)
  when /\.\/[a-z]*/
    Dir.pwd + path[1..-1]
  when /\/[a-z]*/
    Dir.pwd + path
  else
    puts "Wrong path name"
  end
end

def search_in_file(path_to_file, keyword)
  f = open(path_to_file)

  flag = true

  f.each_with_index do |line, i|
    if line.include? keyword

      # print file name and path
      if flag
        puts ""
        puts path_to_file.bold.blue
        flag = false
      end
      puts "#{i+1}:".bold.gray + " #{line}".sub(keyword, keyword.bg_red)
    end
  end
end

def file_finder(path, keyword)
  Dir.chdir(path)

  entries = Dir.entries('.')

  entries.each do |item|
    unless IGNORED_DIRS.include?(item)
      if File.file?(path + "/" + item)
        search_in_file(path + "/" + item, keyword)
      else
        file_finder(path + "/" + item, keyword)
      end
    end
  end
end

file_finder(expand_path(path), keyword)

The POSIX grep command accepts the keyword as the first parameter, then the paths to search as subsequent parameters.  That has the advantage of letting the user specify any number of paths to search.

You misspelled "argumetns".  Also, I recommend writing "#{File::basename($0)}" rather than hardcoding "finder" as the program name.

It is customary to put all of the class / method definitions first, then all of the "main" code at the end.  You've put the argument-handling code at the top, and the file_finder(expand_path(path), keyword) call at the bottom.  Curiously, you test ARGV.length in three places — is it really that important?

file_finder

Avoid using Dir#chdir, as changing the current directory alters the global state of the process.  One consequence of Dir#chdir is that you need to use absolute paths.  For portability, use File::join(path, item) instead of path + "/" + item.

For efficiency, IGNORED_DIRS should be a Set.

def file_finder(path, keyword)
  Dir.foreach(path) do |item|
    next if IGNORED_DIRS.include?(item)

    item_path = File::join(path, item)
    if File.file?(item_path)
      search_in_file(item_path, keyword)
    else
      file_finder(item_path, keyword)
    end
  end
end

expand_path

Your expand_path incorrectly treats an absolute path as a relative path.

When expanding a path starting with ~, I don't see any reason to provide __FILE__ as the second argument to File.expand_path.

In case of an error, you should raise an exception instead of printing a message here.  However, I don't see any reason why there should be an error in the first place.

If you avoid changing directories altogether, then you wouldn't need to generate absolute paths at all.  Then you could just eliminate this function and call File::expand_path instead.

search_in_file

Opening a file without closing it causes a file descriptor leak.

flag is a poor variable name.

I'm not a fan of augmenting the String class, especially for code that is, strictly speaking, related more to terminal control than strings.

def search_in_file(path_to_file, keyword)
  seen = false
  File::open(path_to_file) do |f|
    f.each_with_index do |line, i|
      if line.include?(keyword)
        if not seen
          puts path_to_file.bold.blue
          seen = true
        end
        puts "#{i+1}:".bold.gray + " #{line}".sub(keyword, keyword.bg_red)
      end
    end
  end
  puts "" if seen
end


Checking if a tree is balanced I'm trying to implement balanced tree checking by python like following.

# Balanced tree definition:
# if tree's left and right sub tree are balanced, and their height difference is not bigger than 1, then the tree is balanced else not.

class Node:
    def __init__(self, val):
        self.left = None
        self.right = None
        self.val = val

def is_balanced(root):
    # root: Node
    # return: boolean
    ret, h = is_balanced_helper(root)
    return ret

def is_balanced_helper(root):
    if not root:
        return True, 0
    else:
        retl, hl = is_balanced_helper(root.left)
        retr, hr = is_balanced_helper(root.right)
        h = max(hr, hl)+1
        if retl and retr:
            return abs(hl-hr) < 2, h
        else:
            return False, h

Please help me review above code.

And I feel returning 2 return values are not elegant. And return -1 when the tree is not balanced is also not elegant. Or shall I raise exception when tree is not balanced? What's your suggestion when handle such case?

Refer here for the definition of a balanced binary tree. If you don't like having two return values, you can always refactor and write a separate height() function to get the height of a tree. This would allow you to implement it without having to return two values and would improve the readability of your code as well. This is the suggested way to do it.

I would say avoid raising exceptions unless you're certain you know you want to. You should never raise an exception if you don't have to.


Power utility for starting processes I was looking for a way to start elevated (i.e. administrator permissions) and/or invisible processes via batch in Windows. It turns out that this is kind of impossible with batch only, so I googled some vbs stuff and hacked it all together into a neat command line power utility.

The interface is as follows:

exec [/action: elevate, open, read, print] [/display: hide, show] exe [arguments]

/action defaults to open. /display defaults to show. exe is the path to an executeable, could be a script, image, whatever as well. arguments are applied to the exe.

Example: This starts a background node.js deamon with admin rights.

exec /action:elevate /display:hide node myDeamon.js

Everything works just fine, but I'm pretty sure that it's not perfect yet, as I basically have no idea about VBScript. I'd like to hear your opinions or ideas!

Set objShell = CreateObject("Shell.Application")

intOffset = 0

' get action
strAction = "open"
If (WScript.Arguments.Named.Exists("action")) Then
  intOffset = intOffset + 1
  Select Case WScript.Arguments.Named.Item("action")
        Case "elevate" strAction = "runas"
        Case "open" strAction = "open"
        Case "read" strAction = "read"
        Case "print" strAction = "print"
        Case Else strAction = "open"
    End Select
End If

' get display mode
intMode = 1
If (WScript.Arguments.Named.Exists("display")) Then
    intOffset = intOffset + 1
    Select Case WScript.Arguments.Named.Item("display")
        Case "0" intMode = 0
        Case "false" intMode = 0
        Case "hide" intMode = 0
        Case "hidden" intMode = 0
        Case Else intMode = 1
    End Select
End If

' get application to execute
strApplication = WScript.Arguments(intOffset)

' get arguments
strArguments = ""
For i = intOffset+1 To WScript.Arguments.Count-1
    strArguments = strArguments & WScript.Arguments(i)
Next

' execute application
If (WScript.Arguments.Count >= 1) Then
    objShell.ShellExecute strApplication, strArguments, "", strAction, intMode
Else
    WScript.Quit
End If

Gist

The number one improvement you can make is to include Option Explicit as the first line of your script and explicitly declare your variables using Dim. This will save you a lot of heartache in the future (e.g., if you make a typo in a variable name, it becomes a runtime error, instead of just silently creating a new variable).


Upload an image to a folder through a form I am using this code to upload an image. Please review it and give your feedback regarding performance, security, and quality. This also works in PDO project.

<!DOCTYPE html>

<meta charset="UTF-8">

<form action="" method="post" enctype="multipart/form-data">

Upload File

Filename1:
<input type="file" name="photo" id="fileSelect">

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

<?php
include 'home.php';
if(isset($_FILES["photo"]["error"])){
if($_FILES["photo"]["error"] > 0){
echo "Error: " . $_FILES["photo"]["error"] . "
";
} else{
$allowed = array("jpg" => "image/jpg", "jpeg" => "image/jpeg", "gif" => "image/gif", "png" => "image/png");
$filename = $_FILES["photo"]["name"];
$filetype = $_FILES["photo"]["type"];
$filesize = $_FILES["photo"]["size"];

// Verify file extension
$ext = pathinfo($filename, PATHINFO_EXTENSION);
if(!array_key_exists($ext, $allowed)) die("Error: Please select a valid file format.");

// Verify file size - 5MB maximum
$maxsize = 5 * 1024 * 1024;
if($filesize > $maxsize) die("Error: File size is larger than the allowed limit.");

// Verify MYME type of the file
if(in_array($filetype, $allowed)){
// Check whether file exists before uploading it
if(file_exists("upload/" . $_FILES["photo"]["name"])){
echo $_FILES["photo"]["name"] . " is already exists.";
} else{
move_uploaded_file($_FILES["photo"]["tmp_name"], "upload/" . $_FILES["photo"]["name"]);
echo "Your file was uploaded successfully.";
}
}
else
{
echo "Error: There was a problem uploading your file - please try again.";
}
}

} else{
echo "Error: Invalid parameters - please contact your server administrator.";
}
?>

You should invert the order of your PHP processing and your HTML output.  When working in PHP, always strive to adhere to this approach, as it will help you as you start working on more complex logic.  You will inevitably find yourself needing to set HTTP response readers, cookie values, etc. in your responses.  You need to do this work BEFORE sending any output to the browser - either that or use a hacky output buffering approach.

If you need to set conditional output (i.e. success/error messages, dynamic content, variable output, etc.), consider setting these values into variables and having only basic PHP insertions in the HTML portion of the code.  You should avoid any complex PHP calculations or logic once output to browser has started.

Other thoughts:

You need work on your coding style. Primarily in your case that is in the area of using indentations in your code (both HTML and PHP) to better convey nesting of code blocks.
Don't output system-generated error messages directly to the user.  You may want to log these, but you should always consider end user error messages separately from system errors.  Oftentimes, that may require mapping of PHP-generated errors to user-friendly error messaging. Here is good example: echo "Error: " . $_FILES["photo"]["error"] . "<br>";
Things like file size that are too large may best end up with server responding with 400 or 413 HTTP response code in response header rather than just simple error message. You should actually not handle this at all in your PHP code, but rather in your PHP configuration, so that file upload limits are applied at PHP level, not in your code logic.  If for some reason you need to change the file size limit for a particular script, you could make runtime change to configuration.  There is no reason to defer this file size check into your code. The PHP file upload common pitfalls page has some good additional info in this area - http://php.net/manual/en/features.file-upload.common-pitfalls.php
From a security standpoint, you are doing nothing to prevent against cross-site request forgery (CSRF) attacks.  Typically, this is handled with comparison of hidden form input field with value in session. This really should be a standard part of any form your create in any web application.
You are doing nothing to validate $_FILES and some of the values it holds before operating against the information.  The file name, in particular is something you might want to validate, to make sure the user isn't passing a filenames like ../../index.php which could end up writing files in arbitrary locations in your web directory.


Better way to code this code snippet (copy paste) I am trying to improve upon a VBA code created through recording a macro. Can the code below be optimized (I was informed to avoid .Select as much as possible)? The code itself works fine for me though.
    Sheets("DataExport").Select
    Range("A1").Select
    Range(Selection, Selection.End(xlToRight)).Select
    Range(Selection, Selection.End(xlDown)).Select
    Selection.ClearContents

    Sheets("Analysis").Select
    Range("BX1:CI4").Select
    Selection.Copy
    Sheets("DataExport").Select
    Range("B1").Select
    Selection.PasteSpecial Paste:=xlPasteValues, Operation:=xlNone, SkipBlanks _
        :=False, Transpose:=False

Let me give you the following answer/advise:
As far as your first piece of code is concerned, this can be re-written as:
Sheets("DataExport").Range("A1").CurrentRegion.ClearContents

As far as the second piece of code is concerned (about the copying), I'd like to refer you to this very complete answer. (I might give you the answer but I believe you'd better try to create it, based on this URL. If you have any issues, please edit your question, we'll have another look).


Comma separated string for list I do not have an intention to ask for an answer, but I want to know if my answer is good for a two-week Python beginner?

Question: (Automate the Boring Stuff with Python/ page 102)

Comma Code

Say you have a list value like this:

spam = ['apples', 'bananas', 'tofu', 'cats']

  Write a function that takes a list value as an argument and returns a
  string with all the items separated by a comma and a space, with and
  inserted before the last item. For example, passing the previous spam
  list to the function would return:

    apples, bananas, tofu, and cats

  But your function should be able to work with any list value passed to
  it.

My Answer:

spam = ['apples', 'bananas', 'tofu', 'cats']
def sentence(i):
    for n in range(0, len(spam) - 1):
        print(spam[n], end=', ')
    print('and ' + spam[-1])
sentence(spam)

Since testing falls under the general heading of code review, I'm just going to review your testing strategy.

The question says:

  your function should be able to work with any list value passed to it.

but you are not testing this; you're only testing the single example that was given to you (and which, as it happens, you've hard-coded into your function).  Here's how you could test it:

print(sentence(['eggs', 'bacon', 'spam', 'bacon']))

Does this print the string eggs, bacon, spam, and bacon?  If not, why?  (Try to understand the bug before you fix it -- don't just change things randomly until it works!)

An even better test would be:

assert sentence(spam) == "apples, bananas, tofu, and cats"
assert sentence(['eggs', 'bacon', 'spam', 'bacon']) == "eggs, bacon, spam, and bacon"

Add this to your program, and then fix the sentence function so that neither line of code causes your program to fail.  This is called "test driven development" -- write the test first that says how you want the function to work, then write the function until the test passes!


Sorted linked-list in C (self-compiling) I needed a sorted-list that I can iterate over to render elements in layers. Right now, delete_item deletes the first node with a matching priority it finds but I intend to delete specific list items in my implementation.

I didn't know if I needed the list to be doubly linked, but it turns out it makes deleting an item from the list easier. I've also seen people initializing the memory with calloc, is it important in this case?

There's also probably a way to give each list item a unique identifier (apart from its pointer) so that I can delete a specific item instead of deleting the first occurrence. For now, I'll just use the pointer...

Also, what do you think of my self-compiling trick?

//usr/bin/gcc ${0##*/} -o temp && ./temp ; rm temp 2>/dev/null ; exit

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

struct node {
    struct node *next;
    struct node *prev;

    int priority;
};

struct node *head;

void add_item(int priority)
{
    struct node *curr = head;
    struct node *prev = NULL;

    while((curr != NULL) && (curr->priority < priority)) {
        prev = curr;
        curr = curr->next;
    }

    struct node *new = malloc(sizeof(struct node));
    new->priority = priority;
    new->next = curr;
    new->prev = prev;

    if(prev != NULL) {
        prev->next = new;
    } else {
        head = new;
    }
}

void remove_item(int priority)
{
    struct node *curr = head;
    while((curr != NULL) && (curr->priority != priority)) {
        curr = curr->next;
    }

    if(!curr)
        return;

    if(curr == head) {
        head = curr->next;
    } else {
        curr->prev->next = curr->next;
        if(curr->next)
            curr->next->prev = curr->prev;
    }
    free(curr);
}

void delete_list()
{
    struct node *curr;
    while(head != NULL) {
        curr = head;
        head = head->next;
        free(curr);
    }
}

void print_items()
{
    int member = 0;
    for(struct node *iter = head; iter != NULL; iter = iter->next)
    {
        printf("Member: %d -> priority: %d\n", member, iter->priority);
        member++;
    }
}

int main(int argc, char** argv)
{
    printf("This file is self-compiling\n");

    add_item(1);
    add_item(1);
    add_item(2);
    add_item(3);
    add_item(1);
    add_item(5);
    add_item(4);
    add_item(3);
    remove_item(1);
    remove_item(4);
    remove_item(5);
    add_item(5);

    print_items();
    delete_list();
}

All in all there's not much to criticize. Be sure to fix the bugs mentioned in the other answer, though!

Fail fast

struct node *new = malloc(sizeof(struct node));
new->priority = priority;

You do not check whether allocation succeeded.  Thus the next line will probably result in a crash in case malloc returned NULL.  But it could also continue to seem to be working, failing at a potentially completely unrelated point in your program.  That's the problem with undefined behavior. Thus, if you want an allocation failure to be a fatal failure, then you have to actually code such a failure mode:

void * malloc_or_die(size_t size) {
  void * pointer = malloc(size);
  if (pointer) {
    return pointer;
  }
  fprintf(stderr, "Fatal: failed to allocate memory of size %zu.\n", size);
  exit(EXIT_FAILURE);
}

The trick

  Also, what do you think of my self-compiling trick?

//usr/bin/gcc ${0##*/} -o temp && ./temp ; rm temp 2>/dev/null ; exit

Path to compiler hard coded
Compiler hard coded
Deletes a possibly existing file temp without asking

It's a "trick" that's fun to show once, but really don't use it.  If you want to avoid the sequence of commands to compile, run and possibly delete, then write a shell function to do that for you.

Global variable

struct node *head;

You could make your code more general (as in usable for more than one single list instance) if you pass (a pointer to) such a head pointer in to your functions.

Use blocks

    if(curr->next)
        curr->next->prev = curr->prev;

Just always use { /* ... */ }.  Saves you from potential headaches when you later add a line of code without looking too closely.

Consistency

Just some small oddities, nothing of real importance:

You have struct node *next; but also char** argv.  Decide whether you want to keep the * next to the type or the declared name.
You test for (curr != NULL) but also use !curr in a condition later one.  Use either curr != NULL and curr == NULL or curr and !curr.


Another Scheme Mergesort Here's a merge sort I wrote trying to learn Scheme.

(define mergesort
  (lambda (l cmp)
    (define merge
      (lambda (left right)
        (cond
          ((null? left) right)
          ((null? right) left)
          ((cmp (car left) (car right)) (cons (car left) (merge (cdr left) right)))
          (else (merge right left)))))
    (define split
      (lambda (l)
        (define splitr
          (lambda (l left right)
            (if (null? l)
                (list left right)
                (splitr (cdr l) right (cons (car l) left)))))
        (splitr l '() '())))
    (cond
      ((null? l) l)
      ((null? (cdr l)) l)
      (else (let* ((parts (split l))
                   (left (car parts))
                   (right (cadr parts)))
              (merge (mergesort left cmp)
                     (mergesort right cmp)))))))

I know there's already a merge sort question on code review. Frankly, that approach has some nuances I'm not sure I'm prepared to understand yet. So here's my much more naïve approach.

So, keeping in mind I'm pretty much still an FP newbie (so if I don't understand your critique, I'll ask followup questions) and I may have missed some opportunities to do things "the scheme way", how did I do?

Code Review questions don't have a concept of duplicates per se. You're looking for people to look at your code, not my code! :-)

In so saying, here are my comments:

The most important thing I noted is that your function doesn't seem to maintain sort stability. At least, I don't see any active effort to keep the sorting stable.
Comments on merge:

Your merge uses "right-folding" (whereas my merge uses "left-folding"), which is why it looks so different, but both are legitimate strategies.
Your code doesn't correctly handle the case where two elements compare "equal" though (where (cmp a b) and (cmp b a) both return false). In particular, it will loop endlessly.

Comments on split:

Your split zigzags the elements between the two output lists rather than splitting midway, which was what made me doubt the sort stability.
The inner function splitr could be reformulated using a named let, which is more idiomatic in Scheme:

(define (split l)
  (let loop ((l l)
             (left '())
             (right '()))
    (if (null? l)
        (list left right)
        (loop (cdr l) right (cons (car l) left)))))

Personally, I'd use (values left right) instead of (list left right), to return the sublists as two values. I'll explain why in the final point.

In the outer mergesort function:

You should merge the first two cond tests into a single (or (null? l) (null? (cdr l))) test.
Instead of the sequential let* binding of parts then left then right, you should use either this:

(let ((parts (split l)))
  (let ((left (car parts))
        (right (cadr parts)))
    ...))

or, if you took my suggestion to have split return (values left right), you could do:

(let-values (((left right) (split l)))
  ...)

which is much more succinct.

In summary, I'd suggest two fixes:

You should make the function handle "equal" elements correctly.
You should make the function provide stable sorting, namely that if two elements in the input list are "equal" to each other, you should keep their relative order in the output list (see my test case below).

You can use this as a test case:

(mergesort '((3 . 1) (1 . 4) (4 . 1) (1 . 5) (5 . 9)
             (9 . 2) (2 . 6) (6 . 5) (5 . 3) (3 . 5))
           (lambda (x y) (< (car x) (car y))))

The correct result should be:

((1 . 4) (1 . 5) (2 . 6) (3 . 1) (3 . 5)
 (4 . 1) (5 . 9) (5 . 3) (6 . 5) (9 . 2))


Prepend data to stream in C# I'm working on an XNA project that loads several resources that are packed into a single file. XNA can take a WAV file stream and turn that into a SoundEffect (seen below in "case ".wav"").

Since WAV files are large, I want to enable MP3 audio as one of the resources, so I found MP3Sharp, a cross-platform library that converts mp3 file data into PCM data.(just the wavform data, no headers.) Unfortunately, the method I have to use to integrate this in, "SoundEffect.FromStream(Stream stream)", is expecting both the WAV file headers and the PCM data (waveform) To do this, I am using several MemoryStreams and my instinct tells me that it is horribly inefficient.(noticeably too)  Below you will find the code, the important part being the "case ".mp3":" portion. How can I do this better?

tl;dr: Prepend/combine streams in C#, how? (also note that nChannels and nSamplesPerSec need to be discovered from decoding the mp3 file)

I am worried that I am taking the wrong approach to reading from the stream and

byte[] data = reader.ReadBytes(reader.ReadInt32());
files[path] = data;
string extension = Path.GetExtension(path);
switch (extension)
{
    case ".png":
        string texturePath = Path.ChangeExtension(path, null);
        using (MemoryStream buffer = new MemoryStream(data))
        {
            textures[texturePath] = Texture2D.FromStream(Main.instance.GraphicsDevice, buffer);
        }
        break;
    case ".wav":
        string soundPath = Path.ChangeExtension(path, null);
        using (MemoryStream buffer = new MemoryStream(data))
        {
            sounds[soundPath] = SoundEffect.FromStream(buffer);
        }
        break;
    case ".mp3":
        // TODO, better way to do this?
        string mp3Path = Path.ChangeExtension(path, null);
        MemoryStream wavData = new MemoryStream();
        MemoryStream wavFile = new MemoryStream();
        ushort wFormatTag = 1;
        ushort nChannels;
        uint nSamplesPerSec;
        uint nAvgBytesPerSec;
        ushort nBlockAlign;
        ushort wBitsPerSample = 16;
        using (MemoryStream buffer = new MemoryStream(data))
        {
            using (MP3Sharp.MP3Stream s = new MP3Sharp.MP3Stream(buffer))
            {
                s.CopyTo(wavData);
                nChannels = (ushort)s.ChannelCount;
                nSamplesPerSec = (uint)s.Frequency;
            }
        }
        nBlockAlign = (ushort)(nChannels * (wBitsPerSample / 8));
        nAvgBytesPerSec = (uint)(nSamplesPerSec * nChannels * (wBitsPerSample / 8));
        using (BinaryWriter bw = new BinaryWriter(wavFile))
        {
            bw.Write("RIFF".ToCharArray());
            bw.Write((UInt32)(wavData.Length + 36));
            bw.Write("WAVE".ToCharArray());
            bw.Write("fmt ".ToCharArray());
            bw.Write(16);
            bw.Write(wFormatTag);
            bw.Write(nChannels);
            bw.Write(nSamplesPerSec);
            bw.Write(nAvgBytesPerSec);
            bw.Write(nBlockAlign);
            bw.Write(wBitsPerSample);
            bw.Write("data".ToCharArray());
            bw.Write((UInt32)(wavData.Length));
            bw.Write(wavData.ToArray());
        }
        using (MemoryStream buffer = new MemoryStream(wavFile.ToArray()))
        {
            sounds[mp3Path] = SoundEffect.FromStream(buffer);
        }
        break;
    }

The full method is available on GitHub.

I agree with Heslacher, should be fairly easy to do with NAudio.

var output = new MemoryStream();
using (var reader = new Mp3FileReader(mp3File))
using (var writer = new WaveFileWriter(output, reader.WaveFormat))
{
    //read from reader, write to writer, like you would do with regular streams
    //header is appended automatically to output stream in WaveFileWriter constructor
}

You should also consider encapsulating your resource loading logic to separate class (or classes: one class per resource type). Handling textures and sound in single method looks messy. Single resposibility.

EDIT: If you have to stick to the library of your choosing, then you should just use single output stream.

var output = new MemoryStream();
using (var input = new MP3Sharp.MP3Stream(yourMp3FileStream))
using (var writer = new BinaryWriter(output, Encoding.UTF8, true))
{
    var headerSize = 44; //or w/e it is for RIFF, I might have forgotten
    output.Position = headerSize;
    input.CopyTo(output);
    output.Position = 0;
    //wtite your header
    writer.Write("RIFF".ToCharArray());
    ...
}

Also notice, that you can stack usings on top of each other without adding extra braces.


Comparing list elements in an effective way I have a list of opened nodes, with each step about 3000 nodes are opened and I want to put to the opened list only those nodes that are not already there. For now I'm just comparing each new one to all of the ones already in the opened list and only add it if it's not there. But that is around n*3000^2 comparisons for the nth step and makes the progress slower and slower  each step. Is there a way to do it faster. Or at least is there a structure that would be better than ArrayList.

This is part of the bigger code, but basically the method I use:

public void addEverything() {
    List<gameState> list = current.neighbours;
    for (gameState state : list) {
        if (!isOpened(state)) {
            opened.add(state);
        }
    }
}
public boolean isOpened(gameState state) {
    for (int i = 0; i < opened.size(); i++) {
        if (state.isSame(opened.get(i)))
            return true;
    }
    return false;
}

As for the isSame() method from the gameState class, it just returns true when the two gameStates are considered the same.

Instead of implementing isSame implement equals and hashCode. After that you could use a HashSet (or LinkedHashSet if the order of elements is important). Set is a collection that contains no duplicate elements and inserting/searching is much faster than searching in a list.

Set<GameState> opened = new LinkedHashSet<GameState>();

public void addEverything() {
    List<GameState> list = current.neighbours;
    for (GameState state : list) {
        opened.add(state);
    }
}

If you need a list, you can convert it back after the loop:

List<GameState> openedList = new ArrayList<GameState>(opened);

See also: Overriding equals and hashCode in Java
According to the Java Code Conventions, class names in Java usually starts with uppercase letters.


Retrieve and cache web SPI results, then display on page I'm writing some code to return results from a web service and display them on screen. If the service is down I want to use Lawnchair to serve cached results.

Test

Click to load!

<button class="btn" type="button" onclick="loadData()">Load</button>

@section scripts{

}

As you can probably tell I'm very new to JavaScript and concepts of variable scope and callback functions.

I'd like some general JavaScript feedback but more specifically how to avoid globals. I understand they're evil in every language but do they seem to be unavoidable here?

While this code works and retrieves data, the approach to getting the data from my Lawnchair cache seems wrong. Is there a better way than in the method retrieveFromStore()?

Closures in JavaScript

  I'd like some general JavaScript feedback but more specifically how to avoid globals. I understand they're evil in every language but do they seem to be unavoidable here?

You can easily avoid globals in JavaScript using closures.
I suggest to read more about the subject,
for example in this article by Douglas Crockford.

In fact you have already used the concept here:

function retrieveFromStore() {
    var data;
    window.store.get('data', function (d) { data = d.value; });
    return data;
}

The anonymous function in the call to store.get uses the data variable defined outside of the function, but within its closure.

As another example, and a simple solution to avoid window.store,
you could define all your functions within the outer $(function () { ... });, and then store will be visible to them, for example:

$(function () {
    var store = new Lawnchair(function () { });

    function storeData(data) {
        store.save({ key: 'data', value: data });
    }

    // ... other functions
});

Append directly instead of accumulating text

This function accumulates text in a variable before appending to the dom:

function drawData() {
    var data = retrieveFromStore();
    var text = '';
    for (var i = 0; i < data.length; i++) {
        text += '
' + data[i].Name + '
';
    }
    $('#results').append(text);
}

This is unnecessary,
you could append directly:

function drawData() {
    var data = retrieveFromStore();
    var results = $('#results');
    for (var i = 0; i < data.length; i++) {
        results.append('
' + data[i].Name + '
');
    }
}

Note that the reason for var results = $('#results'); is to avoid repeated dom lookups, which can be expensive.
It's a good practice to save repeatedly used dom lookups in variables.