Bubble Sort in C: Array vs Linked List A few weeks ago I posted a code review of a linked list so I decided to implement bubble sort on my journey of learning C. Here is the referenced linked list library, though this isn't really what's being reviewed (feel free if you'd like!).

Linked lists seem inefficient for bubble sort as deletes and gets both have to walk the list. So it's probably something like \$ O\left( n^{3} \right) \$?

I implemented it with arrays too, which is faster because the swapping can be done in place.

LinkedList.h

LinkedList.c

BubbleSort.c:

#include "../linked_list/ll.h"

void sort_linked_list(linked_list *list) {
  assert(list);
  int sorted = 1;
  for (int x = 0; x < list->size; x++) {
    /* don't do it if this is the last value */
    if (x + 1 == list->size) {
      continue;
    }
    int first = linked_list_get(list, x);
    int second = linked_list_get(list, x + 1);
    if (first > second) {
      sorted = 0;
      int temp = linked_list_get(list, x);
      /* {1, 7, 3}, 1 -> {1, 3} */
      linked_list_delete_at(list, x);
      /* {1, 3}, 7, 1 -> {1, 3, 7} */
      linked_list_insert_after(list, temp, x);
    }
  }
  /* do it again if it's not fully sorted */
  if (!sorted) {
    sort_linked_list(list);
  }
}

void sort_array(int *array, int size) {
  assert(size >= 0);
  int sorted = 1;
  for (int x = 0; x < size; x++) {
    /* don't do it if this is the last value */
    if (x + 1 == size) {
      break;
    }
    /* get int address */
    int *first = array + x;
    /* get int address after that one */
    int *second = first + 1;
    if (*first > *second) {
      /* we did some sorting so not sorted */
      sorted = 0;
      int temp = *first;
      int temp2 = *second;
      /* swap values */
      *first = temp2;
      *second = temp;
    }
  }
  /* do it again if it's not fully sorted */
  if (sorted == 0) {
    sort_array(array, size);
  }
}

void print_array(int *array, int size) {
  printf("{ ");
  for (int x = 0; x < size; x++) {
    printf("%d ", *array);
    array++;
  }
  printf("}\n");
}

int main() {
  linked_list *list = new_linked_list();
  linked_list_append(list, 7);
  linked_list_append(list, 5);
  linked_list_append(list, 3);
  linked_list_append(list, 8);
  linked_list_append(list, 1);
  linked_list_print(list);
  sort_linked_list(list);
  linked_list_print(list);
  int a[5] = {7, 5, 3, 8, 1};
  print_array(a, 5);
  sort_array(a, 5);
  print_array(a, 5);
  return 0;
}

Makefile:

    all: ../linked_list/ll.h bubble.c
        gcc -std=c99 -Wall -o bubble bubble.c ../linked_list/ll.c
        ./bubble

As you can see, linked_list is in another directory. Is this the correct way to include this in my project? I don't fully understand Makefiles quite yet.

Also, any general style tips for my bubble sort would be appreciated. I use pointer arithmetic with my arrays because it helps me understand what's really going on (which is why I'm learning C) but let me know if that's messy. I'd just like to keep learning how to write good C code.

Things you did well

You initialize your variables when you declared them.
You initialize variables inside of your for loops.(C99)
Your pointer arithmetic looks pretty sound.

Things you can improve

Efficiency/Algorithm

Unfortunately, bubble sort is not a very good sorting algorithm if you are looking for speed.  Its average case performance is \$ O\left(n^{2}\right) \$.
If you are looking into greater speed with your sorting function, look at a quicksort algorithm.  Its average case performance is \$ O\left( n \log n \right) \$.

User experience

Is shutting down the program really necessary when your linked list doesn't actually exist, or that the size of your array is less than one?  Do something more useful than assert(), and return a specific error code that you can handle later on in your code.  This will greatly help you in debugging too.

Makefile

Makefile variables - Right now your Makefile isn't very expandable.  Variables help out a lot with that.

Put your compiler names into variables.

CXX = g++
CC = gcc

This is something I like to do, however, some variables like CC are declared already, so you generally don't need to.
Put your final executable into a variable name.

EXECUTABLE = bubble

Put your sources into a variable name.

SOURCES = bubble.c

Put your objects into a variable name.

OBJECTS = $(SOURCES:.c=.o)

.c.o:
    $(CC) $(CFLAGS) -o $@ $<

All of your compiler flags should be in one variable.  And tack on a linker flag variable while we are at it, for futures sake.

CFLAGS = -std=c99 -Wall
LDFLAGS =

Calling the variables is now a lot more easy and flexible.

all: $(EXECUTABLE)

$(EXECUTABLE): $(OBJECTS)
    $(CC) $(LDFLAGS) -o $@ $<

Compiler flags - get the most out of your compiler so that you can code better.

You should change the standards you are using to C11 with --std=c11
-g adds symbols for debugging.  Without it, your debugger won't be able to give you variable or function names.  The don't slow down the program, and we don't care if the program is a kilobyte larger, so there's little reason to not use this.
-O3 indicated optimization level three, which tries every trick known to build faster code.  If, when you run the debugger, you find that too many variables have been optimized out for you to follow what's going on, then change this to -O0.

If you want to go above and beyond, here are all of the compiler flags I use.

CFLAGS = -std=gnu11 -c -s -O3 -Werror -Wall -Wextra -Wformat=2 -Winit-self -Wswitch-enum -Wstrict-aliasing=2 -Wundef -Wshadow -Wpointer-arith -Wbad-function-cast -Wcast-qual -Wcast-align -Wwrite-strings -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-declarations -Wredundant-decls -Wnested-externs -Winline -Wdisabled-optimization -Wunused-macros -Wno-unused

Standards

You don't have to return 0 at the end of main(), just like you wouldn't bother putting return; at the end of a void-returning function. The C standard  knows how frequently this is used, and lets you not bother.

  C99 & C11 §5.1.2.2(3)

  ...reaching the } that terminates the main() function returns a
  value of 0.

Syntax/Styling

Use Yoda conditions to prevent future bugs.

if (0 == sorted)

Placing the constant value in the expression does not change the behavior of the program. A common mistake is to accidentally assign a value instead of writing a conditional statement.  Using Yoda conditions will throw a syntax error if you accidentally do that, so your code won't compile.
You should return some status indicators from your sorting functions, so we can check that the sorting went okay.  This also makes for easier debugging.
You can simplify some of your test conditionals

if (sorted == 0)

That is actually harder for me to understand over my version.

if (!sorted)

If I were to read my version out loud, it would read "if not sorted", whereas your version is "if sorted is equal to 0".  It's odd, since you have this in one place, but not the other (always be consistent).
Declare your method parameters as void if you don't take any arguments in.

int main(void)


Optimizing the performance of html5 video manipulation I've been working on an html5 canvas video player with a lot of fun little extras and ui toys, the main one being a chroma key (green screen) effect that allows the user to key out different colors or ranges of color while the video is playing.

I seem to have hit a snag in the recommended chroma key method. It works, but it is a really heavy process and can tax the cpu to the point where the video will look choppy if the user is running other applications in the background.

Here is the stripped down code I'm using for the chroma key effect:

   <video id="sourceVid" controls="true">
        <source src="http://s3.amazonaws.com/dfc_attachments/public/documents/3185373/AAA.webm" />
    </video>
    <canvas id="hCanvas"></canvas>
    <canvas id="dCanvas"></canvas>

    <script type="text/javascript">
        var doc = document;
        var sourceVid = doc.getElementById("sourceVid");
        var hCanvas = doc.getElementById("hCanvas");
        var dCanvas = doc.getElementById("dCanvas");
        var hContext = hCanvas.getContext("2d");
        var dContext = dCanvas.getContext("2d");

                    sourceVid.addEventListener('loadeddata', function() {
                        hCanvas.setAttribute('width', sourceVid.offsetWidth);
                        dCanvas.setAttribute('width', sourceVid.offsetWidth);
                        hCanvas.setAttribute('height', sourceVid.offsetHeight);
                        dCanvas.setAttribute('height', sourceVid.offsetHeight);
                    }, false);

        sourceVid.addEventListener('play', function() {
            runAnalysis();
        });

        var runAnalysis = function() {
            if (sourceVid.paused || sourceVid.ended) {
                return
            }
            frameFix();
            if (window.requestAnimationFrame) {
                requestAnimationFrame(runAnalysis);
            } else {
                setTimeout(runAnalysis, 0);
            }
        };

        var frameFix = function() {

            hContext.drawImage(sourceVid, 0, 0, sourceVid.videoWidth, sourceVid.videoHeight);

            var frame = hContext.getImageData(0, 0, sourceVid.videoWidth, sourceVid.videoHeight);

            var length = frame.data.length;
            for (var i = 0; i < length; i++) {
                var r = frame.data[i * 4 + 0];
                var g = frame.data[i * 4 + 1];
                var b = frame.data[i * 4 + 2];

                if (g >= 0 && g < 100 && r >= 0 && r < 100 && b >= 0 && b < 100) {
                    frame.data[i * 4 + 3] = 0;
                }
            }
            dContext.putImageData(frame, 0, 0);
            return
        };

Live Demo

Hoping to find ways to further optimize this for performance.

After back and forth with ChrisW and yourself, it seems I found a glaring bug

    var frameFix = function() {

        hContext.drawImage(sourceVid, 0, 0, sourceVid.videoWidth, sourceVid.videoHeight);

        var frame = hContext.getImageData(0, 0, sourceVid.videoWidth, sourceVid.videoHeight);

        var length = frame.data.length;
        for (var i = 0; i < length; i++) {
            var r = frame.data[i * 4 + 0];
            var g = frame.data[i * 4 + 1];
            var b = frame.data[i * 4 + 2];

            if (g >= 0 && g < 100 && r >= 0 && r < 100 && b >= 0 && b < 100) {
                frame.data[i * 4 + 3] = 0;
            }
        }
        dContext.putImageData(frame, 0, 0);
        return
    };

Since in frame.data you have 4 data points per pixel, you only need to go as far as frame.data.length / 4, this should make your code 4 times faster.

This observation with ChrisW's common sense observations makes for:

var frameFix = function()
{
  hContext.drawImage(sourceVid, 0, 0, sourceVid.videoWidth, sourceVid.videoHeight);

  var frame  = hContext.getImageData(0, 0, sourceVid.videoWidth, sourceVid.videoHeight),
      data   = frame.data,
      length = data.length, i;

  for (i = 0; i < length; i += 4)
    if (data[i] < 100 && data[i + 1] < 100 && data[i + 2] < 100)
      data[i + 3] = 0;

  dContext.putImageData(frame, 0, 0);
};


Plotting an input curve and its FFT using gnuplot in C++ I'm currently working on a project1 which will result in millions of FFTs being solved time and time again. I have a finished prototype in Python (using numpy, scipy, and the pyfftw3 package), but there are significant bottlenecks in calculation speed. I have been writing a GPU version using arrayfire, but there's still a large amount of number crunching which can't be done in parallel. I'm hoping that translating the project to C++, either the entire thing or just the compute-intensive parts, will provide an extra boost over Python where the GPU isn't appropriate.

As a starting point, I have written a very basic C++ script which defines a Gaussian curve, takes the FFT using the FFTW3 library, and plots the input curve with its FFT using gnuplot. As this is my first script in C++ (neglecting 'hello world') I'd like to make sure I'm not wandering severely off track when it comes to optimizing efficiency and speed. I haven't benchmarked this yet, but was hoping that someone with more experience in using these libraries and the language could scan over it and point out any howlers.

The code:

#include "gnuplot-iostream.h"
#include <iostream>
#include <vector>
#include <cmath>
#include <fftw3.h>
#include <complex>

typedef std::complex<double> doubleComp;

int main()
{
    Gnuplot gp;

    doubleComp i;
    i = -1;
    i = sqrt(i);

    int gridOrder = 9;
    int nPoints = pow(2, gridOrder);
    int halfPoints = pow(2, gridOrder-1);
    float tau = 18;
    float inTemp, outTemp;

    fftw_complex in[nPoints], out[nPoints], pArr1[nPoints], pArr2[nPoints];
    fftw_plan p;

    for (int idx=0; idx<nPoints; idx++) {
        in[idx][0] = exp(-pow((idx-halfPoints)/tau, 2));
        in[idx][1] = 0;
    }

    p = fftw_plan_dft_1d(nPoints, pArr1, pArr2, FFTW_FORWARD, FFTW_MEASURE);
    fftw_execute_dft(p, in, out);

    for (int idx=0; idx<nPoints; idx++) {
        out[idx][0] *= (1/double(nPoints));
        out[idx][1] *= (1/double(nPoints));
    }

    std::vector<std::pair<double, double> >inPlot;
    std::vector<std::pair<double, double> >outPlot;
    for (int idx=0; idx<nPoints; idx++) {
        inTemp = std::abs(in[idx][0] + i*in[idx][1]);

        // Deal with fftshifts needed for visuals:
        if (idx > halfPoints)
            outTemp = std::abs(out[idx-halfPoints][0] +
                                i*out[idx-halfPoints][1]);
        if (idx < halfPoints)
            outTemp = std::abs(out[idx+halfPoints][0] +
                                i*out[idx+halfPoints][1]);

        inPlot.push_back(std::make_pair(idx-halfPoints, inTemp));
        outPlot.push_back(std::make_pair(idx-halfPoints, outTemp));
    }

    gp << "set xlabel 'Grid points' font 'Helvetica, 12'\n" <<
        "set ylabel 'Intensity' font 'Helvetica, 12'" << std::endl;
    gp << "set tics font 'Helvetica, 10'" << std::endl;
    gp << "set title 'Gaussian pulse example' font 'Helvetica, 13'" << std::endl;
    gp << "set grid xtics lt 0 lw 1 lc 'gray'\n" <<
        "set grid ytics lt 0 lw 2 lc 'gray'" << std::endl;

    gp << "set term wxt '0'" << std::endl;
    gp << "plot '-' lt 1 lw 2 linecolor 'blue' with line title 'Time domain'\n";
    gp.send1d(inPlot);

    gp << "set term wxt '1'" << std::endl;
    gp << "plot '-' lt 11 lw 2 with line title"
        "'Spectral domain'\n";
    gp.send1d(outPlot);

    fftw_destroy_plan(p);

    return 0;
}

To compile (Ubuntu 16.04):

g++ -O3 GaussianPulseDefinition.cpp -o GaussianPulseDefinition -lboost_system -lboost_iostreams -lboost_filesystem -lfftw3 -lm

1 For those who like to know the application: The project is basically a coupled PDE solver (with Qt GUI) aimed at simulating the nonlinear dynamics of optical pulse propagation in optical fibre, fibre laser systems, optical amplifiers, etc.

Firstly, does the program give correct results?

If so, how do you know?

Without going into extensive unit testing, checking your results against a Matlab prototype/your Python implementation is probably good enough for now.

I can't comment on the Gnuplot functions as I don't have it installed, also but some general C++ tips are...

std::vectors are your friend! Try use them instead of C-style arrays. They 'know' their size and accessing out-of-bounds values are a major problem with C-style arrays and definitely for fiddly calculations like FFT. (You can use my_vector.data() to pass it into C-style functions).
Reduce reliance on a 'global' nPoints constant by using the vectors .size() member. (Unfortunately this outputs an unsigned integer size_t so there are some casts in the code to silence the compiler warnings). This is so that if nPoints goes out of sync with the size of the array/vector, you're not going to access an out-of-bounds value.
Always try to define a variable's value as soon as you declare it. Undefined variables are a major problem as C++ doesn't (usually) auto-define say, a double to zero. (double x; would refer to some garbage value in memory).
Always try to define variables when you need them i.e. as late as possible (see code for examples)
Try get into the habit of spitting out little functions right from the get-go, instead of one big block of code. It will break the problem up and be easier to debug. Also this will limit some variables scope so its easier to see what parts of the program change what variables.
For FFTs probably best to stick to doubles all round for the extra precision (removed the floats)
Use C++11 if you can and turn up the compiler warning level g++ -std=c++11 -Wall ...

Hopefully a bit to get you started!

I whole heartedly recommend this book if you're coming from Python, it's a short in length but really comprehensive and does a much better job than I could at explaining how to write good C++, and by the man himself - Stroustrup's "A Tour Of C++"

//#include "gnuplot-iostream.h"
#include <iostream>
#include <vector>
#include <cmath>
#include <fftw3.h>
#include <complex>
#include <cassert>

using doublePairs = std::vector<std::pair<double, double> >; // using (instead of typedef) for long named vector of pairs

void makeGaussian(std::vector<fftw_complex>& v, double tau) { // pass-by-reference indicates v is going to be altered
    for (size_t idx=0; idx<v.size(); idx++) {   // in.size() so we don't go out of range
        const int halfPoints = v.size() / 2;
        v[idx][0] = exp(-pow((idx-halfPoints)/tau, 2));
        v[idx][1] = 0;
    }
}

void performFft(std::vector<fftw_complex>& in,
                std::vector<fftw_complex>& out) {

    fftw_plan p;

    assert(in.size() == out.size()); // test same sizes
    const int nPoints = static_cast<int> (in.size());

    std::vector<fftw_complex> pArr1 (nPoints);
    std::vector<fftw_complex> pArr2 (nPoints);
    p = fftw_plan_dft_1d(nPoints, pArr1.data(), pArr2.data(), FFTW_FORWARD, FFTW_MEASURE);
    fftw_execute_dft(p, in.data(), out.data()); // access raw pointers to vectors for c-style function call

    fftw_destroy_plan(p);

    for (auto& n : out) {                           // nicer c++11 range based for loop
        n[0] *= (1/static_cast<double> (nPoints));  // static_cast<T> is preferred to c-style casts
        n[1] *= (1/static_cast<double> (nPoints));
    }
}

void makePlotPairArrays(const std::vector<fftw_complex>& in,
                        const std::vector<fftw_complex>& out,
                        doublePairs& inPlot,
                        doublePairs& outPlot) {

    assert(in.size() == out.size()); // test same sizes
    const std::complex<double> i {std::sqrt(-1)}; // typedef not necessary? (it's only used once)
    const int halfPoints = in.size() / 2;

    for (size_t idx=0; idx<in.size(); idx++) {
        // these Temps should be doubles?? since it's a pair of doubles not floats?
        // would lead to less precise FFTs
        double inTemp = std::abs(in[idx][0] + i*in[idx][1]); // keep these local to the loop scope

        // Deal with fftshifts needed for visuals:
        double outTemp {0};

        const int idx_ = static_cast<int> (idx); // for signed/unsigned compiler warnings
        if (idx_ > halfPoints)                              // what about when idx == halfPoints ???
            outTemp = std::abs(out[idx-halfPoints][0] +     // try make this an if/else statement so
                                i*out[idx-halfPoints][1]);  // at least one branch is always touched
        if (idx_ < halfPoints)
            outTemp = std::abs(out[idx+halfPoints][0] +
                                i*out[idx+halfPoints][1]);

        inPlot.push_back(std::make_pair(idx-halfPoints, inTemp));
        outPlot.push_back(std::make_pair(idx-halfPoints, outTemp));
    }
}

int main()
{
    constexpr int gridOrder = 9;
    constexpr int nPoints = std::pow(2, gridOrder);
    constexpr double tau = 18;
    // 'halfPoints' not needed here by declaring in function's scope
    // std::pow(2, gridOrder-1); BTW pow is heavy why not nPoints / 2?

    // our fft arrays
    std::vector<fftw_complex> in (nPoints);
    std::vector<fftw_complex> out (nPoints);

    makeGaussian(in, tau);

    performFft(in, out);

    doublePairs inPlot;
    doublePairs outPlot;
    makePlotPairArrays (in, out, inPlot, outPlot);

    // ... do the GnuPlotting

    return 0;
}


Simple simulated annealing template in C++11 This simulated annealing program tries to look for the status that minimizes the energy value calculated by the energy function.

The status class, energy function and next function may be resource-intensive on future usage, so I would like to know if this is a suitable way to code it.

#pragma once

#include <cmath>
#include <random>
#include <utility>
#include <algorithm>

// To find a status with lower energy according to the given condition
template<typename status, typename count, typename energy_function, typename temperature_function, typename next_function, typename generator>
status simulated_annealing(status i_old, count c, const energy_function& ef, const temperature_function& tf, const next_function& nf, generator& g){

    auto   e_old  = ef(i_old);

    status i_best = i_old;
    auto   e_best = e_old;

    std::uniform_real_distribution<decltype(e_old)> rf(0, 1);

    for(; c > 0; --c){
        status i_new = nf(i_old, g);
        auto   e_new = ef(i_new);

        if(e_new < e_best){
            i_best =           i_new ;
            e_best =           e_new ;
        }

        if( e_new < e_old || std::exp( (e_old - e_new) / tf(c) ) > rf(g) ){
            i_old  = std::move(i_new);
            e_old  = std::move(e_new);
        }
    }
    return(i_best);
}

Example usage:

#include <cmath>
#include <random>

#include <chrono>
#include <cstdlib>
#include <iostream>

#include "simulated_annealing.hpp"

template<typename Clock = std::chrono::system_clock>
class timer{
private:
    typedef std::chrono::time_point<Clock> time_point;
    time_point _start;
    time_point _finish;
public:
    void start(){
        _start = Clock::now();
    }
    void finish(){
        _finish = Clock::now();
    }
    template<typename rtype>
    rtype count(){
        std::chrono::duration<rtype> elapsed = _finish - _start;
        return elapsed.count();
    }
};

auto ef = [](long double x){
    return std::abs( (x - 10)*(x + 20)*(x - 30) );
};

auto tf = [](long double k){
    return std::exp( -20 * k );
};

template<typename generator>
long double nf(long double x, generator& g){
    std::normal_distribution<decltype(x)> d(0, 1);
    return x + d(g);
}

int main(int argc, char const *argv[])
{
    std::random_device rd;
    std::mt19937_64 g(rd());
    timer<std::chrono::high_resolution_clock> t;

    long double root   = std::atof(argv[1]);
    std::size_t count  = std::atoi(argv[2]);

    t.start();
    root = simulated_annealing(root, count, ef, tf, nf<decltype(g)>, g);
    t.finish();

    std::cout << "Result: " << root << std::endl;
    std::cout << "Time elapaed: " << t.count<double>() << '\n';

    return 0;
}

I think the algorithm is well abstracted and the code is nicely written, so there is not all too much you can do in order to make it more efficient. Still I have some minor suggestions for improvement:

First, regarding the algorithm itself:

When the check if(e_new < e_best) is successful, you can directly treat the step as accepted. Although this might be negligible, you avoid one more check.

In your check whether to accept the new step, you can think about choosing a cutoff (say ~ 10.0) parameter to bypass the relatively expensive calculation of std::exp as well as the random number generation.

Summarizing 1. and 2., one gets a slightly more verbose but hopefully more efficient code:
    if(e_new < e_best){
        i_best =           i_new ;
        e_best =           e_new ;
        i_old  = std::move(i_new);
        e_old  = std::move(e_new);
        continue;
    }

    auto t = tf(c);
    auto delta_e = e_new - e_old;

    if( delta_e > 10.0 * t) continue;  //as std::exp(-10.0) is a very small number

    if( delta_e <= 0.0 || std::exp( - delta_e / t ) > rf(g) ){
        i_old  = std::move(i_new);
        e_old  = std::move(e_new);
    }

Next, regarding your choice of functions: Here I would try to avoid expensive functions as well. I know, this might often not be possible as the functions are determined by your problem at hand. However, for example, in your temperature function you can think about replacing the std::exp(-20*k) by something more efficient such as 1/(k*k).

Regarding the language:

You can get a bit more of a C++11 touch if you use universal references instead of lvalue references.
template<typename status, typename count, typename energy_function
       , typename temperature_function, typename next_function, typename generator>
status simulated_annealing(status&& i_old, count&& c, energy_function&& ef
                         , temperature_function&& tf, next_function&& nf, generator&& g)
{
    //...
}

This is maybe also a bit more consistent with your std::move's of your status and count-type variables (which, as a side node, will resolve to simple copies when fundamental types such as double or size_t are used).
Further, you can think  about replacing your type count by size_t, since that is what this type is for. But this just because I personally try to avoid template parameters when possible.

Regarding the design:

The nf function can be made more generic by defining a static generator inside
long double nf(long double x){
    static std::mt19937_64 g(std::random_device());
    std::normal_distribution<decltype(x)> d(0, 1);
    return x + d(g);
}

I wouldn't pass a generator here, as it doesn't fit well into a generic desing. (For instance, the next state could also be found deterministically).

The same holds for your main simulated_annealing function. I wouldn't bother the user to define a random number generator, but just define it inside -- moreover since it is used "only" to create random numbers in [0,1] (once you've followed suggestion 4.).

        template<typename status, typename count, typename energy_function
               , typename temperature_function, typename next_function>
        status simulated_annealing(status&& i_old, count&& c, energy_function&& ef
                             , temperature_function&& tf, next_function&& nf)
        {
            std::mt19937_64 g(std::random_device());
            //...
        }

This you can simply call the routine via
    root = simulated_annealing(root, count, ef, tf, nf);

which is more concise and better reflects the main components of a simulated annealing algorithm.


Updating images in a weather app Right now in my weather app, I have one section of code in my View Controllers to set up the condition image and the background image. It has about 400 lines of if-else statement at the end.

The app's performance is fine, but is it bad to have this? Would it be something that, say, Apple would consider rejecting? The code is very easy to read, and makes perfect sense in my opinion.

- (void)updateImages:(ICB_WeatherConditions *)weather {
    if ([condition isEqualToString:@"113"]) {
        conditionsImageView.image = [UIImage imageNamed:@"Sun.png"];
        BGView.image = [UIImage imageNamed:@"Sun.jpg"];
    } else {
        if ([condition isEqualToString:@"116"]) {
            conditionsImageView.image = [UIImage imageNamed:@"Mostly_Sunny.png"];
            BGView.image = [UIImage imageNamed:@"Partly_Cloudy.jpg"];

        } else {
            if ([condition isEqualToString:@"119"]) {
                conditionsImageView.image = [UIImage imageNamed:@"Overcast.png"];
                BGView.image = [UIImage imageNamed:@"Overcast.jpg"];
            }
            else {
                if ([condition isEqualToString:@"122"]) {
                    conditionsImageView.image = [UIImage imageNamed:@"Overcast.png"];
                    BGView.image = [UIImage imageNamed:@"Overcast.jpg"];
                }
                else {
                    if ([condition isEqualToString:@"143"]) {
                        conditionsImageView.image = [UIImage imageNamed:@"Mist.png"];
                        BGView.image = [UIImage imageNamed:@"Foggy.jpg"];
                    }
                    else {
                        if ([condition isEqualToString:@"176"]) {
                            conditionsImageView.image = [UIImage imageNamed:@"Scattered_Thunderstorms.png"];
                            BGView.image = [UIImage imageNamed:@"Scat_Tstorms.jpg"];
                    }

It keeps going on after that. As you can see, I have a lot of nearly identical statements, which is because I have a lot of conditions. Is it bad practice to do this? If it is, then how can this become more efficient?

In this case, your indentation makes the code drift over to the right so does make the code difficult to read so does need some changes.

First I would note that a lot of code is repeated and also as noted a switch after converting the condition to an int is possible.

However in this case your code is effectively doing multiple lookups so I would look at using NSDictionaries (or NSArrays if the conditions are 0 to a number) and then do a straight lookup

e.g.
setup

NSDictionary* conditionsImages = [NSDictionary dictionaryWithObjectsAndKeys:
                @"113", @"Sun.png",
                @"116", @"Mostly_Sunny.png",
     ...
                nil];

Or read the dictionaries from a file

Access them by

- (void)updateImages:(ICB_WeatherConditions *)condition {
    conditionsImageView.image = [UIImage imageNamed:
                   [conditionsImages objectForKey:condition]];
    BGView.image = [UIImage imageNamed:[otherImages objectForKey:condition]];

  ...
}


Changing the formatting of sheets This my program to change the formatting of sheets in different folders. I have over 5000 files in one folder and it is taking up to three hrs for one folder and there are multiple subfolders in the main folder. I want to cut down this time to 10 mins or as fast as possible.

Sub loopAllSubFolderSelectStartDirectory()

'Another Macro must call LoopAllSubFolders Macro to start to procedure
Call LoopAllSubFolders("D:\HTTP\")

End Sub
'Don’t run the following macro, it will be called from the macro above:

'List all files in sub folders
Sub LoopAllSubFolders(ByVal folderPath As String)

Dim fileName As String
Dim fullFilePath As String
Dim numFolders As Long
Dim folders() As String
Dim i As Long

Set wc = ActiveWorkbook
If Right(folderPath, 1) <> "\" Then folderPath = folderPath & "\"
fileName = Dir(folderPath & "*.*", vbDirectory)

While Len(fileName) <> 0

    If Left(fileName, 1) <> "." Then

        fullFilePath = folderPath & fileName

        If (GetAttr(fullFilePath) And vbDirectory) = vbDirectory Then
            ReDim Preserve folders(0 To numFolders) As String
            folders(numFolders) = fullFilePath
            numFolders = numFolders + 1

        Else

Set wb = Workbooks.Open(fullFilePath)
Dim sht As Worksheet
Dim fnd As Variant
Dim rplc As Variant

fnd = "<*>"
rplc = ""

For Each sht In wb.Worksheets
On Error GoTo 0
  sht.Cells.Replace what:=fnd, Replacement:=rplc, _
    LookAt:=xlPart, SearchOrder:=xlByRows, MatchCase:=False, _
    SearchFormat:=False, ReplaceFormat:=False
    Next sht

Workbooks("New file to be saved.xlsm").Sheets("Sheet1").Range("A1:T1").Copy

'Now, paste to y worksheet:
wb.ActiveSheet.Range("A1").PasteSpecial
Range("G2:H100").Cut Range("Q2:R100")
Range("B2:F100").Cut Range("F2:J100")
wb.ActiveSheet.Cells.EntireColumn.AutoFit

'With wb.ActiveSheet.Range("A1:T1").Interior
'        .Pattern = xlSolid
'        .PatternColorIndex = xlAutomatic
'        .ThemeColor = xlThemeColorDark1
'        .TintAndShade = -0.149998474074526
'        .PatternTintAndShade = 0
'    End With
'    With wb.ActiveSheet.Range("A1:T1").Font
'        .ThemeColor = xlThemeColorLight2
'        .TintAndShade = 0
'    End With
'    Range("A1:T1").Font.Bold = True

    wb.SaveAs Replace(wb.FullName, ".csv", ".xls"), FileFormat:=xlExcel8
wb.Close True
Kill fullFilePath
            'Insert the actions to be performed on each file
            'This example will print the full file path to the immediate window
            'Debug.Print folderPath & fileName

        End If

    End If
fileName = Dir()

Wend

For i = 0 To numFolders - 1

    LoopAllSubFolders folders(i)

Next i

End Sub

I use the following sub to speed up processing:

Public Sub AppSpeed(Optional iWhat As Long = xlDown)

    Dim bDir As Boolean

    bDir = True
    If iWhat = xlUp Then bDir = False

    On Error Resume Next

    With Application
         .Calculation = IIf(bDir, xlCalculationAutomatic, xlCalculationManual)
         .ScreenUpdating = bDir
         .DisplayStatusBar = bDir
         .EnableEvents = bDir
         .DisplayPageBreaks = bDir
         .PrintCommunication = bDir
   End With
End Sub

and I call it like

AppSpeed xlUp ' at the beginning of the app

and

AppSpeed xlDown  ' az the end of processing

The inconvenience of this solution is that you can't monitor what's happening so Dim a counter like filecounter and put the following snippet somewhere in the loop e.g. after wb.SaveAs to see that something is happening

    filecounter = filecounter + 1
    If 100 * (filecounter \ 100) = filecounter Then
        AppSpeed xlDown
        Application.StatusBar = folderpath & " " & CStr(filecounter)
        DoEvents
        AppSpeed xlUp
    End If

Next advice is to reduce the number of files in one folder. The documenations do not really help in this matter. My experience is that it is worth to keep the number of files under 1000 in one folder because access time increases enormously over that. You have a flexible structure so can quickly test it by dividing the files to multiple folders.


shared_ptr code implementation I have never used std::shared_ptr before and I don't really know how this smart pointer actually works but I decided to create one to verify my knowledge. Here is my code :

namespace myStd
{
    template<class T>
    class shared_ptr
    {
        public :

        shared_ptr() : refCount(nullptr), t(nullptr) {}
        shared_ptr(T *p) : refCount(new int(1)), t(p) {}

        ~shared_ptr()
        {
            this->destroy();
        }

        shared_ptr(const shared_ptr &p) : refCount(nullptr), t(nullptr)
        {
            if(p.isvalid())
            {
                refCount = p.refCount;
                t = p.t;

                if(isvalid())
                (*this->refCount)++;
            }
        }

        void destroy()
        {
            if(isvalid())
            {
                --(*refCount);
                if((*refCount) <= 0)
                {
                    delete refCount;
                    delete t;
                }

                refCount = nullptr;
                t = nullptr;
            }
        }

        shared_ptr& operator =(const shared_ptr &p)
        {
            if(this != &p)
            {
                if(!p.isvalid())
                {
                    this->destroy();
                    this->refCount = p.refCount;
                    this->t = p.t;

                    if(isvalid())
                    (*this->refCount)++;

                    return (*this);
                }

                if(!isvalid())
                {
                    this->refCount = p.refCount;
                    this->t = p.t;

                    if(isvalid())
                    (*this->refCount)++;

                    return (*this);
                }
                else
                {
                    this->destroy();
                    this->refCount = p.refCount;
                    this->t = p.t;

                    if(isvalid())
                    (*this->refCount)++;
                }
            }
            return (*this);
        }

        bool isvalid() const {return (t != nullptr && refCount != nullptr);}
        int getCount() const {if(refCount != nullptr) return (*refCount); else return 0;}

        T* operator ->() {return t;}
        T& operator *() {return *t;}

        private :
        int *refCount;
        T *t;
    };
}

What am I missing? Did I implement this smart pointer right?
Your suggestions would be appreciated.

What am I missing?

Several things.

constructor that takes object of type std::nullptr_t
constructor that takes object of type shared_ptr<U> where U is derived from T
boolean conversion operator.
swap operator
move semantics

Sure there is more. In Smart pointers you always forget something.

  Did I implement this smart pointer right?

YOu implemented the rule of three. So barring any bugs it should be good.

  Your suggestions would be appreciated.

I go into a lot of detail in my articles:

Unique Pointer
Smart Pointer
Constructors

Don't use this.

It is not very normal to see this-> in C++ code (unlike Java).

I actually consider the use of this-> as bad practice. You only need to use this->member if there is some ambiguity between local/parameters and member names.

If you have some ambiguity then you have done a bad job at naming and you are using this-> to cover up the problem. Prefer to fix the problem as some poor maintainer is going to come along later and use the wrong variable because of your laziness.

I normally turn on the compiler warning that tells my if a local variable is shadowing another thus forcing me to have unique (and meaningful names).

One Line One Statement

shared_ptr(const shared_ptr &p) : refCount(nullptr), t(nullptr)

To me this looks exactly like multiple assignments packed onto the same line.

varOne = 5; varTwo = 6; theOtherOne = doStuff(5);

I think it is well accepted that one line should have one statement. Why not apply this to your parameter list. It will make reading the code much easier and also validating order of initialization problems.

shared_ptr(const shared_ptr &p)
    : refCount(nullptr)
    , t(nullptr)
{
    /* Stuff */
}

Construct with a null ptr

    shared_ptr(T *p) : refCount(new int(1)), t(p) {}

What happens if you I build with a nullptr?

X*            ptr = nullptr;
shared_ptr<X> data1(ptr);
shared_ptr<X> data2(nullptr); // should have its own constructor.

Double Check of isvalid() is wastefull

If p.isvalid() is true then isvalid() will be true after the copy.

    shared_ptr(const shared_ptr &p) : refCount(nullptr), t(nullptr)
    {
        if(p.isvalid())
        {
            refCount = p.refCount;
            t = p.t;

            if(isvalid())
            (*this->refCount)++;
        }
    }

Also this could be much simplified.

    shared_ptr(const shared_ptr &p)
        : refCount(p.refCount)
        , t(p.t)
    {
        if(isvalid()) {
            (*refCount)++;
        }
    }

Prefer Copy/Swap Idiom for assignment.

    shared_ptr& operator =(const shared_ptr &p)
    {
        if(this != &p)
        {
            /* This LOOKS LIKE A BUG */
            /* Don't think the ! should be there. */
            if(!p.isvalid())
            {
                this->destroy();
                this->refCount = p.refCount;
                this->t = p.t;

                if(isvalid())
                (*this->refCount)++;

                return (*this);
            }

            if(!isvalid())
            {
                this->refCount = p.refCount;
                this->t = p.t;

                if(isvalid())
                (*this->refCount)++;

                return (*this);
            }
            else
            {
                this->destroy();
                this->refCount = p.refCount;
                this->t = p.t;

                if(isvalid())
                (*this->refCount)++;
            }
        }
        return (*this);
    }

Sorry that function is big and clumsy and hard to read. It also does not provide the strong exception guarantee. You modify the content of this object doing exception dangerous work (call destroy) before updating the content of this object.

To do any work in exception safe way you need to follow a three step processes.

Make a copy into temp. -> Dangerious as exceptions can fly.
Swap the temp and current. -> Safe as swap should not throw exceptions.
Destroy then temp. -> Safe as the state of current is now good.

This 3 step process is ipitimized by the copy and swap idiom.

 shared_ptr& operator =(const shared_ptr &p)
 {
     shared_ptr  temp(p);    // Constructor copies. Destructor destoryes.
     temp.swap(*this);       // Perform an exception safe transfer of state.
     return *this;
 }

Lots of people take this one step further. And do the copy in the parameter:

 shared_ptr& operator =(shared_ptr p)  // Notice the pass by value.
 {                                     // this gets your copy.
     p.swap(*this);
     return *this;
 }

Const correctness

These two functions do not mutate the state of the object.

    T* operator ->() {return t;}
    T& operator *() {return *t;}

So they should probably be marked as const.


Basic Go HTTP router I know Gorilla and others have packages that accomplish this, but I'm trying to learn Go by making stuff. In the router below, the first route / is for all http methods, the second route /hello is only for GET, and the third route hello/:name has one parameter for all HTTP methods.

func main() {
    http.HandleFunc("/", route)
    http.ListenAndServe(":8080", nil)
}

func match(re string, route string) bool {
    match, err := regexp.MatchString(re, route)
    if err != nil {
        return false
    }

    return match
}

func route(w http.ResponseWriter, r *http.Request) {
    route := r.URL.Path
    switch {
    case route == "/":
        home(w, r)
    case route == "/hello" && r.Method == "GET":
        hello(w, r)
    case match("^/hello/([^/]+)$", route) == true:
        var re = regexp.MustCompile("^/hello/([^/]+)$")
        match := re.FindStringSubmatch(route)

        helloName(w, r, match[1])
    default:
        notFound(w, r)
    }
}

// func home, hello, helloName, and notFound

How can I make this more efficient? Is it reliable?

Instead of case x == true you can use boolean expressions directly,
and write simply:

case x:

The same regex is written twice here.
What's worse is that the pattern matching is also evaluated twice:

case match("^/hello/([^/]+)$", route) == true:
    var re = regexp.MustCompile("^/hello/([^/]+)$")

I'm not sure if there's an elegant solution for this situation in Go.
But at the minimum you could move the regex pattern into a variable to avoid typing it twice.

Another alternative is to create a matcher structure,
with a match function that will not only match a pattern and return a boolean,
but at the same time also store the sub-string you want to extract.

type matcher struct {
    value string
}

func (m *matcher) match(pattern string, route string) bool {
    var re = regexp.MustCompile(pattern)
    matches := re.FindStringSubmatch(route)
    if len(matches) < 2 {
        return false
    }

    m.value = matches[1]

    return true
}

With the help of this, you could rewrite the route function as:

func route(w http.ResponseWriter, r *http.Request) {
    route := r.URL.Path
    m := &matcher{}
    switch {
    case route == "/":
        home(w, r)
    case route == "/hello" && r.Method == "GET":
        hello(w, r)
    case m.match("^/hello/([^/]+)$", route):
        helloName(w, r, m.value)
    default:
        notFound(w, r)
    }
}


Find the noun phrase before and after a number in a text Given a text I have to find the preceding words to all numbers up to a stop word belonging to a check_words list (kind of stopwords).
My code:
check_words = ['the', 'a', 'with','to']
mystring = 'the code to find the beautiful words 78 that i have to nicely check 45 with the snippet'
list_of_words = mystring.split()

In that particular text I would check before '78' and '45' and will go backwards up to the point where I find any of the words in check_words (but not more than 8 words).
The code for doing that might be:
preceding_chunks = []
for i,word in enumerate(list_of_words):
    if any(char.isdigit() for char in word):

        # 8 precedent words (taking into account that I can not slice with 8 back at the beginning)
        preceding_words = list_of_words[max(0,i-8):i]
        preceding_words[::-1]

        # I check from the end of the list towards the start
        for j,sub_word in enumerate(preceding_words[::-1]):
            if  sub_word in check_words:
                # printing out j for checking
                myposition = j
                print(j)
                real_preceding_chunk = preceding_words[len(preceding_words)-j:]
                print(real_preceding_chunk)
                preceding_chunks.append(real_preceding_chunk)
                break

This code works.
basically I check every word tha
But I have the impression (perhaps I am wrong) that it can be achieved with a couple of one liners and hence without loops.
Any idea?

NOTE:
This question is about improvement the readability of the code, trying to get rid of the loops to make the code faster, and trying to make the code nicer, which is part of the Zen of Python.

NOTE 2: Some previous checks that I did:

Finding the position of an item in another list from a number in a different list
Finding the index of an item in a list
Find in list

I came up with this:
import itertools
import re

chunks = (grouped_chunk.split() for grouped_chunk in re.split("\\s+\\d+\\s+", mystring))
preceding_chunks = []

for reversed_chunk in map(reversed, chunks):
    preceding_chunk = list(itertools.takewhile(lambda word: word not in check_words, reversed_chunk))[::-1]
    preceding_chunks.append(preceding_chunk)

We apply itertools.takewhile to the reversed_chunk which gives us the preceding chunk in reversed order. We then obtain the correctly ordered preceding_chunk by reversing at the end with [::-1].
The regex splits mystring based on a number (the escaped \d+). The surrounding escaped \s+s represent any padding around the number. This causes this code to have different behavior than yours if digits and letters are mixed in the same words (for example, a1).
For your original code, I'd make a couple suggestions:

Follow PEP 8. For example, add spacing after the comma in i,word.
Remove the redundant expression preceding_words[::-1]. While this does evaluate to the reversed preceding_words, because it is not in-place, the evaluation has no side-effects. Plus, you already perform this reversal in enumerate(preceding_words[::-1]).


Getting value from session if available, if not, take from the database This method is a constructor that has to decide which URL to add to the response header as Allow-Origin. i am actually adding the Allow-Origin later according to the value of PermittedOrigion variable. I am using Session to eliminate database access.

Could i write it more efficient?  I am working with asp.net webform

public ResponseAsJson(HttpRequest request, int affiliateId)
    {
        string cacheKeyName = $"PermittedOrigion_{affiliateId.ToString()}";
        const string notDefined = "N";
        object ValueFromCache = HttpContext.Current.Cache[cacheKeyName];

        if (!String.IsNullOrWhiteSpace(ValueFromCache?.ToString()))
        {
            if (ValueFromCache.ToString() == notDefined)
                return;

            PermittedOrigion = ValueFromCache.ToString();
            return;
        }

        string actualOrigion = null;

        if (request.UrlReferrer != null)
           actualOrigion = request.UrlReferrer.GetLeftPart(UriPartial.Authority);

        string[] strFromDB = RM.BL.Affiliate.Instance(affiliateId).LUAffiliate.AccessControlAllowOriginJsonResult.Split(';');

        if (!String.IsNullOrWhiteSpace(actualOrigion) && strFromDB.Contains(actualOrigion))
        {
            PermittedOrigion = actualOrigion;
        }
        else
        {
            string strLocalhost = strFromDB.Where(x => x.ToLower().StartsWith("http://localhost")).FirstOrDefault();

            if (!String.IsNullOrWhiteSpace(strLocalhost))
                PermittedOrigion = strLocalhost;
        }

        if (PermittedOrigion == null)
        {
            HttpContext.Current.Cache[cacheKeyName] = notDefined;
        }
        else
        {
            HttpContext.Current.Cache[cacheKeyName] = PermittedOrigion;
        }

    }

Some recommendations:

1) Give your method a better name. ResponseAsJson is not what it does. Proposal: AppendAllowOriginHeader

2) Your method relies on the affiliateId inputparameter. I would change the parameter to int? affiliateId and then add a short guard:

if(!affiliateId.HasValue) {
   throw new ArgumentNullException(nameof(affiliateId));
}

Also keep in mind, that the id may be invalid in which I would prefere to throw an InvalidArgumentException

3) Let's clean up: Extract the method in two subfunctions:TryAddFromCache(string key) and TryAddFromAllowedOrigins

TryAddFromCache could look like:

if(String.IsNullOrWhiteSpace(key)) {
   return false;
}

...
return true;

And in AppendAllowOriginHeader:

if (TryAddFromCache()) {
    return;
}

4) As a rule: Use const or var wherever possible.

5) Extract a second method:

bool TryAddFromAllowedOrigins(IEnumerable<string> allowedOrigins) { ... }

6) Performance: In this case we have the database access as bottleneck.
The party starts at:

RM.BL.Affiliate.Instance(affiliateId).LUAffiliate.AccessControlAllowOriginJsonResult.Split(';');

.. which has the most impact.

Your array strFromDB will probably not hold references to several thousand strings. So filtering or sorting (which should be the work of the db anyway) has no impact.

 string strLocalhost = strFromDB.Where(x => x.ToLower().StartsWith("http://localhost")).FirstOrDefault();

can be changed to:

strFromDB.FirstOrDefault(x => x.ToLower().StartsWith("..."));

7) Consider using better names: for example strFromDB could be changed to allowedOrigins


Check if a site is indexed by google and stores status in Database This is the script I need to speed up.

Things that I have already done to speed up are:

Using single quotes instead of double quotes wherever possible.
Created Database Index
Replaced queries containing 'select' * with 'select column_names'

It takes around 1.7 seconds per URL on average. I want to know if this can be made any faster by any optimizations in PHP or SQL.

<?php
    $servername = '127.0.0.1';
    $username = 'root';
    $password = '';
    $dbname = 'cron';
    $conn = mysqli_connect($servername, $username, $password, $dbname);
    if (!$conn)
    {
        die('Connection failed: ' . mysqli_connect_error());
    }
    $sql = 'SELECT Name FROM domain';
    $result = mysqli_query($conn, $sql);
    while ($row = mysqli_fetch_row($result))
    {
        $line = (string)$row[0];
        $auth = base64_encode('ABC:XYZ');
        $aContext = array('http' => array('proxy' => 'tcp://IP_ADDRESS', 'request_fulluri' => true, 'header' => 'Proxy-Authorization: Basic'.' '. $auth,),);
        $cxContext = stream_context_create($aContext);
        if (strstr(file_get_contents('http://www.google.com/search?q=site:'.$row[0], False, $cxContext), 'did not match any documents'))
        {
            $status = FALSE;
        }
        else
        {
            $status = TRUE;
        }
        date_default_timezone_set('Asia/Kolkata');
        $date = date('Y-m-d H:i:s');
        $sql = "UPDATE domain SET Index_Status = '$status', Last_Checked = '$date' WHERE Name = '$line' ";
        $result1 = mysqli_query($conn, $sql);
    }
    mysqli_close($conn);
    header( 'Location: /Cron/php/display.php');
?>

First of all, changing between double and single quotes is going to make such a trivial difference, you would probably need to measure to microsecond precision to see any performance impact. Use quote styles that make your code most readable in whatever context you are forming strings.

The main thing you can do to speed things up is to parallelize the requests against Google and then batch your update queries for each batch of requests.

Using curl_multi may be a good approach. I actually have a simple REST library that leverages curl_multi that you are free to use if you find it helpful - https://github.com/mikecbrant/php-rest-client

Obviously, making requests in parallel will consume more memory and CPU on your server, so you might need to play around with number of concurrent requests to get your desired compromise of run time vs. resource utilization.

You also need to make sure you have an index on your Name field in the DB.

Don't use root user in MySQL, and especially don't leave root user with empty password.


opening a text file and building a dictionary Need to write a function that takes an open file as the only parameter and returns a dictionary that maps a string to a list of strings and integers.

each line in the text will have a username, first name, last name, age, gender and an e-mail address. The function will insert each person's information into a dictionary with their username as the key, and the value being a list of [last name, first name, e-mail, age, gender].

basically what im trying to do is open a text file that contains this:

           ajones Alice Jones 44 F alice@alicejones.net

and return something like this:

          {ajones: ['Jones', 'Alice', 'alice@alicejones.net', 44, 'F']}

so far i have done this, but is there any other easier way?

def create_dict(file_name):
    '''(io.TextIOWrapper) -> dict of {str: [str, str, str, int, str]}

    '''
    newdict = {}
    list2 = []
    for line in file_name:
        while line:
            list1 = line.split() #for a key, create a list of values
    if list2(0):
        value += list1(1)
    if list2(1):
        value += list1(2)
    if list2(2):
        value += list1(3)
    if list2(3):
        value += list1(4)
    newdict[list1(0)] = list2

    for next_line in file_name:
        list1 = line.split()
        newdict[list1(0)] = list1
    return newdict

def helper_func(fieldname):
    '''(str) -> int
    Returns the index of the field in the value list in the dictionary
    >>> helper_func(age)
    3
    '''

    if fieldname is "lastname":
        return 0
    elif fieldname is "firstname":
        return 1
    elif fieldname is "email":
        return 2
    elif fieldname is "age":
        return 3
    elif fieldname is "gender":
        return 4

There are more efficient ways to implement this:

1) If you only need a dictionary to keep your data together without semantics, you could split as follows:

>> line = "ajones Alice Jones 44 F alice@alicejones.net"
>> identifier, *userdata=line.split()

results in:

>> identifier
'ajones'
>> userdata
['Alice', 'Jones', '44', 'F', 'alice@alicejones.net']

That's a simple point to start, but as mentinoed, the resulting array is very dumb, i.e. you have no semantics about what is what. You end up with a simple array.

2) Another way is using named tuples

from collections import namedtuple

[...]
Userdata = namedtuple("Userdata", "first_name last_name age gender email")
[...]
key, firstname, lastname, age, gender, email = a.split()
user=Userdata(firstname, lastname, age, gender, email)

Now your user contains the data in a meaningful way:

>> user.last_name
'Jones'

Besides:
Your code looks a little bit messy. It's really hard, to get ones head around what you wrote. If you come again in 3 months i doubt, you understand anything you did above.

Your naming of variables is not only poor (list,value,newdict) but terribly misleading:

for line in file_name:

You are not iterating over lines in file_name you are iterating over lines, which are perhaps from a file; in the function (and for python) it doesn't matter what the source is, but you should choose an apropriate name.

You make your and your peers life easier, if you try to write better, i.e. cleaner code.


Python Knapsack greedy Given a dictionary of cows and their weight:

cows = {'Betsy': 9, 'Oreo': 6, 'Herman': 7, 'Florence': 2, 'Maggie': 3, 'Moo Moo': 3, 'Milkshake': 2, 'Lola': 2, 'Millie': 5, 'Henrietta': 9}

I want to get a list of lists to move all cows, each nested list of cow's a total weight <= 10, hence to first two trips will have just Betsy and Henrietta:

The answer for GREEDY COW TRANSPORT:

[['Betsy'], ['Henrietta'], ['Herman', 'Maggie'], ['Oreo', 'Moo Moo'], ['Millie', 'Florence', 'Milkshake'], ['Lola']]

Here is my code that took too long in a net grader:

def greedy_cow_transport(cows,limit=10):
    train = []
    cart = []

# get tuples from dictionary key ordered by value high to low
    CowTupleList = sorted(cows.items(), key=lambda x: x[1], reverse = True)

# put cow names in list order high to low to loop over
    names = []
    for i in range(len(cows)):
        names.append(CowTupleList[i][0])
    train = []
    while sum(cows.values()) > 0:
        cart = []
        total = 0

        for cow in names:
            if cows[cow] != 0 and cows[cow] + total <=limit:
                cart.append(cow)
                total += cows[cow]
                cows[cow] = 0
        train.append(cart)
    return train

This took too much time for the online grader, so I failed. Can this be done in only a few lines?

You sort the cows, but you don't take advantage of the fact that they're sorted.  Instead of iterating over the cows multiple times (which leads to \$\mathcal{O}(n^2)\$ time w.r.t. the number of cows), iterate over the sorted list once.

Unfortunately, I can't think of an easy way to do this using Python's built-in data structures.  However, if we assume that CowTupleList is some list-like datastructure that has \$\mathcal{O}(\log{n})\$ or better performance for all operations (including del), then we can use binary search to find the largest cow that will fit in a cart's remaining capacity:

def greedy_cow_transport(cows,limit=10):
  # Sort cows from largest to smallest
  CowTupleList = sorted(cows.items(), key=lambda x: x[1], reverse = True)

  while CowTupleList:
    # Add first (largest) cow to a new cart
    name,weight = CowTupleList[0]
    cart = [name]
    remaining_capacity = limit - weight

    # Remove first cow from list
    del CowTupleList[0]

    # Find largest remaining cow that fits in remaining capacity (if any)
    idx = find_largest_fitting(CowTupleList, remaining_capacity)
    while idx is not None:
      # Add the cow at idx to the cart
      name,weight = CowTupleList[idx]
      cart.append(name)
      remaining_capacity -= weight

      # Remove cow at idx from list
      del CowTupleList[idx]

      # Find largest remaining cow that fits (if any)
      idx = find_largest_fitting(CowTupleList, remaining_capacity)

    # No more cows fit => yield the cart
    yield cart

Assuming find_largest_fitting is implemented as a binary search over CowTupleList (and an appropriate data structure is chosen for CowTupleList), this should take \$\mathcal{O}(n\log{n})\$ time.  If linear search is used for find_largest_fitting and/or Python's build-in list type is used for CowTupleList (so that del operates in \$\mathcal{O}(n)\$), then this algorithm will operate in \$\mathcal{O}(n^2)\$ time.


Dynamically display relevant form elements Here is a group of questions (hidden by default except the first one) with yes|no buttons

Q with 1 more option?

    <input type="button" value="true" data-value="web" />
    <input type="button" value="no" />

Q with 2 more option?

    <input type="button" value="yes" data-value="analy,seo" />
    <input type="button" value="no" />

Q3?

    <input type="button" value="yes" data-value="qwe" />
    <input type="button" value="no" />

Q4?

    <input type="button" value="yes" data-value="asd" />
    <input type="button" value="no" />

and the script to reveal the next relevant question (or multiple questions):

$('.form-item:nth-child(1)').slideDown('fast');

$('input[value="yes"],input[value="true"]').click(function () {
    next_id = $(this).attr('data-value');
    matches = [next_id];
    rx = /[^a-z|^0-9]/g;
    if (patt = next_id.match(rx)) {
        matches = next_id.split(patt[0]);
    }

    i = 0;
    while (matches[i]) {
        $('#' + matches[i]).slideDown('fast');
        i++;
    }
});

Sandbox: http://jsfiddle.net/eapo/pu53rx31/

There is any way to simplify this code?

Comments:

The idea of splitting your data-values with different isolation characters seems silly, just using a comma seems much better, as @76484 suggested, and removing the Regular Expression altogether.

However stepping around that, I can see a few things you could fix up:

Yes and No buttons:

You use yes and no for testing the input, why not use true and false, that way instead of $('input[value="yes"],input[value="true"]'), you can just use $('input[value]').

<input type="button" value="true" data-value="foo" />
<input type="button" value="false" />

Regular Expression declaration:

Assuming entire removal of the regular expression isn't possible: you only use rx once, meaning you can just place the expression in the loop:

if (patt = next_id.match(/[^a-z|^0-9]/g)) {

While loops:

while loops are notorious for causing recursion to occur, so you ought to replace it with a for loop.

It also removes the need for two extra lines for declaring the variable and incrementing the variable.

for (var i = 0; matches[i]; i++) {
  $('#' + matches[i]).slideDown('fast');
}


Factorize All Numbers Up to a Given Number this post is sort of a continuation of my answer on the following question: Fast Algorithm to Factorize All Numbers Up to a Given Number. As this post explains - We need to factorize all the numbers up to a large N.
At first I gave a python solution which was pretty slow (since - you know, python), than I decided to write it in C++. I am not that good with C++ and I would like to have a code review about that answer:
#include <math.h>
#include <unistd.h>
#include <list>
#include <vector>
#include <ctime>
#include <thread>
#include <iostream>
#include <atomic>

#ifndef MAX
#define MAX 200000000
#define TIME 10
#endif

std::atomic<bool> exit_thread_flag{false};

void timer(int *i_ptr) {
    for (int i = 1; !exit_thread_flag; i++) {
        sleep(TIME);
        if (exit_thread_flag) {
            break;
        }
        std::cout << "i = " << *i_ptr << std::endl;
        std::cout << "Time elapsed since start: "
                  << i * TIME
                  << " Seconds" << std::endl;
    }
}

int main(int argc, char const *argv[])
{
    int i, upper_bound, j;
    std::time_t start_time;
    std::thread timer_thread;
    std::vector< std::list< int > > factors;

    std::cout << "Initiallizating" << std::endl;
    start_time = std::time(nullptr);
    timer_thread = std::thread(timer, &i);
    factors.resize(MAX);
    std::cout << "Initiallization took "
              << std::time(nullptr) - start_time
              << " Seconds" << std::endl;

    std::cout << "Starting calculation" << std::endl;
    start_time = std::time(nullptr);
    upper_bound = sqrt(MAX) + 1;
    for (i = 2; i < upper_bound; ++i)
    {
        if (factors[i].empty())
        {
            for (j = i * 2; j < MAX; j += i)
            {
                factors[j].push_back(i);
            }
        }
    }
    std::cout << "Calculation took "
              << std::time(nullptr) - start_time
              << " Seconds" << std::endl;

    // Closing timer thread
    exit_thread_flag = true;

    std::cout << "Validating results" << std::endl;
    for (i = 2; i < 20; ++i)
    {
        std::cout << i << ": ";
        if (factors[i].empty()) {
            std::cout << "Is prime";
        } else {
            for (int v : factors[i]) {
                std::cout << v << ", ";
            }
        }
        std::cout << std::endl;
    }

    timer_thread.join();
    return 0;
}

I would especially like a review about my usage of threads (I am afraid it might slow down the code). The performance are reaching 6619 which is the 855th (out of 1662 primes up to 14140 ~ square root of 200000000) in 1.386111 hours, if you find any way to make it faster I will be amazing! A more semantic review is also very welcome (Like #include order?).
Just for fun and a point of reference if you are trying to run the code yourself:

Where X is time and Y is the prime reached (i). The orange tradeline is y = 13 * 1.00124982852632 ^ x. The graph is exponential since indeed the inner loop time is getting shorter.
The orange tradeline says I will reach 14107 (The highest prime before the square root) at x ≈ 5595.842803197861 seconds which is 1.554 hours!

Timing
The timer thread is unnecessary and an inaccurate way to measure time.

I do now know about this system_clock, system calls might slow down the process (maybe the context switch even more though)

Querying the time costs a bit of time, even if it does not involve an actual system call - which it really may not, there are lots of clever tricks for example clock_gettime uses vDSO on modern Linux and reads from a shared memory location and QueryPerformanceCounter reads the TSC on typical Windows systems, there is no transition into and out of kernel mode. It's never a lot of time relative to what this program is doing, the overhead of getting the time is only an issue when timing very short spans of time. Even if getting the time costs a milisecond (which would be unacceptable and considered a bug in the implementation), it would still be OK for this program.
Performance
Storing the factors in explicit linked lists is a major performance problem, and unlike usual, using vectors wouldn't be great either. There is an alternative: store only one factor of a number. That still gives a complete factorization for any number, because if a number N has a factor factors[N], then you can divide N by that factor and look up a factor of the new (smaller) number and so on, until 1 is reached.
That way the inner loop of the sieve only does a bunch of stores into a vector, nothing heavy-weight like dynamically allocating nodes of a linked list, and the memory usage does not get out hand.
As a convention, I'll use that the lowest factor of a prime is the prime itself. That is the mathematical definition, and it makes iterating over the implicit factor lists easy.
Other
Defining MAX by macro definition and putting local variable declarations at the top of the function are very C things to do. Even C has moved away from "all locals at the top". As general guidelines, I recommend using const variables instead of defines, and limiting local variables with the smallest possible scope. That does not repeatedly incur a cost for "making a variable" because that's not how that happens, any fixed space a function needs is allocated all at once at function entry. Besides, most local variables spend their entire lifetime in registers.
Avoid #include <unistd.h> if possible/reasonable, it does not exist on all platforms.
Pick a brace style and stick to it. There were "same line"-braces and "next line"-braces. There are various opinions on which should be used, but at least they shouldn't be mixed.
In total, the code might come out like this:
#include <iostream>
#include <vector>
#include <math.h>
#include <chrono>

int main() {
    const int MAX = 200000000;
    std::vector<int> factors;

    std::cout << "Initiallizating" << std::endl;
    auto start_time = std::chrono::steady_clock::now();
    factors.resize(MAX);
    std::cout << "Initiallization took "
        << std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::steady_clock::now() - start_time).count()
        << " ms" << std::endl;

    std::cout << "Starting calculation" << std::endl;
    start_time = std::chrono::steady_clock::now();
    int upper_bound = sqrt(MAX) + 1;
    for (int i = 2; i < upper_bound; ++i) {
        if (factors[i] == 0) {
            for (int j = i; j < MAX; j += i) {
                factors[j] = i;
            }
        }
    }
    std::cout << "Calculation took "
        << std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::steady_clock::now() - start_time).count()
        << " ms" << std::endl;

    std::cout << "Validating results" << std::endl;
    for (int i = 2; i < 20; ++i) {
        std::cout << i << ": ";
        if (factors[i] == i) {
            std::cout << "Is prime";
        }
        else {
            for (int N = i; N > 1; N /= factors[N]) {
                std::cout << factors[N] << ", ";
            }
        }
        std::cout << std::endl;
    }

    return 0;
}

On my PC the sieving takes about 2.5 seconds now. Ideone is a bit slower but the program is fast enough to run there too.


Swift append an array when it's been declared as Any I'm learning Swift and I have a following problem: I create a dictionary containing some data, one of which is stored as an array. I want make an operation on this array to add some info. Currently I have managed to achieve this by writing this code:

var myDictionary = [String: Any]()

myDictionary = [
    "name": "Wiktor",
    "age": 25,
    "scores": [1, 2, 3, 4, 5, 6],
]

var l = myDictionary["scores"] as? [Int] ?? [Int]()
l.append(100)
myDictionary["scores"] = l
print("\(myDictionary["scores"]!)")

This works, meaning an array with value 100 is printed out, but this solution seems a little bit over engineered to me. Can I do it easier, more like:

myDictionary["scores"].append(100)

in python?

Since myDictionary values are declared as Any, you could have anything (or even nothing) assigned at "scores", so you need to safe-cast the value in to a variable first and then reassign it.

I don't know about Python, but Swift doesn't check dictionary values at compile time.


I've written remainder(x,y) in OCaml. Is there more efficient than O(n)? Here's my code:

let rem x y =
  let rec aux acc i n =
    if i=n then acc else
    if acc+1=y then aux 0 (i+1) n else
    aux (acc+1) (i+1) n in
  aux 0 0 x;;

I'm just learning OCaml and I wonder:

Is this tail recursive?
Is there a more efficient algorithm, i.e., operating in better than linear time?

I don't know OCaml, but I know enough similar languages that I think I can read that well enough to answer.  Still, take this with a grain of salt.

That is tail recursive.  You either return a value, or return the result of a recursive call that depends only on its input parameters that are known at the time of the call.

There is a more efficient algorithm.  Hint: repeated subtraction.  (That may not be the most efficient algorithm.)

You don't actually use the parameter n, you could just use x directly in the same way you use y directly.  The name rem isn't the most descriptive; remainder would be better, but that particular function is mostly known as mod or modulus.

I would find this formatting more readable:

if i = n
then acc
else if acc+1 = y
     then aux      0  (i+1) n
     else aux (acc+1) (i+1) n


Rails 6: Attaching Devise current_user.id to form submissions In my Ruby on Rails 6 app:

I have a User class built with Devise, and I scaffolded a List class where each List should belong_to a User.

I want Lists created through my app to be automatically assigned to the logged-in user. This should happen on the back end, after the rest of the new List fields have been posted to the server.

I was able to make this happen by adding a line to the default list-params definition in the scaffolded controller:

def list_params
  params[:list].merge!(:user_id => current_user.id.to_s)
  params.require(:list).permit(:title, :active, :user_id)
end

My questions are:

Am I reinventing the wheel somehow with this approach?
Does modifying the list_params in this way affect the application outside of creating and updating Lists?
Have I introduced any potential security issues by doing things this way?

For reference, here is the rest of the controller:

class ListsController < ApplicationController
  before_action :authenticate_user!
  before_action :set_list, only: [:show, :edit, :update, :destroy]

# GET /lists
# GET /lists.json
def index
  @lists = List.all
end

# GET /lists/1
# GET /lists/1.json
def show
end

# GET /lists/new
def new
  @list = List.new
end

# GET /lists/1/edit
def edit
end

# POST /lists
# POST /lists.json
def create
  @list = List.new(list_params)

  respond_to do |format|
    if @list.save
      format.html { redirect_to @list, notice: "List was successfully created." }
      format.json { render :show, status: :created, location: @list }
    else
      format.html { render :new }
      format.json { render json: @list.errors, status: :unprocessable_entity }
    end
  end
end

# PATCH/PUT /lists/1
# PATCH/PUT /lists/1.json
def update
  respond_to do |format|
    if @list.update(list_params)
      format.html { redirect_to @list, notice: "List was successfully updated." }
      format.json { render :show, status: :ok, location: @list }
    else
      format.html { render :edit }
      format.json { render json: @list.errors, status: :unprocessable_entity }
    end
  end
end

# DELETE /lists/1
# DELETE /lists/1.json
def destroy
  @list.destroy
  respond_to do |format|
    format.html { redirect_to lists_url, notice: "List was successfully destroyed." }
    format.json { head :no_content }
  end
end

private

# Use callbacks to share common setup or constraints between actions.
def set_list
  @list = List.find(params[:id])
end

# Only allow a list of trusted parameters through.
def list_params
  params[:list].merge!(:user_id => current_user.id.to_s)
  params.require(:list).permit(:title, :active, :user_id)
end
end

And here is the form template:

<%= form_with(model: list, local: true) do |form| %>
    <% if list.errors.any? %>

<%= pluralize(list.errors.count, "error") %> prohibited this list from being saved:

          <% list.errors.full_messages.each do |message| %>

<%= message %>

          <% end %>

    <% end %>

      <%= form.label :title %>
      <%= form.text_field :title %>

      <%= form.label :active %>
      <%= form.check_box :active %>

      <%= form.submit %>

  <% end %>

Why do we need user_id in params at all? Probably, there is a need to add some context to a current scope. And yes, you're trying to write straightforward logic in a different way.

Here is some suggested refactoring:

class ListsController < ApplicationController
  def new
    build_resource
  end

  def create
    build_resource

    respond_to do |format|
      if update_resource
        # ...
      else
        # ...
      end
    end
  end

  def update
    update_resource
    # ...
  end

  def destroy
    destroy_resource
  end

  private

  def permitted_params
    params.require(:list).permit(:title, :active)
  end

  def resource
    @resource ||= current_user.lists.find(params[:id])
  end

  def build_resource
    @resource = current_user.lists.new
  end

  def update_resource
    resource.update(permitted_params)
  end

  def destroy_resource
    resource.destroy
  end
end

For those pure-CRUD controllers common logic can be easily moved into concern. We've created a gem some time ago https://github.com/cimon-io/unobtrusive_resources


Testing Depth First Search Using Pytest I have an implementation of depth first search, which I want to test using the pytest framework.
I would be making test cases for other graph algorithms, and would like to know if this approach is good enough to be replicated on similar programs.
Here is what it looks like currently:
(This may not be a very exhaustive set of test cases, but my goal is to separate simple checks that are done in the actual implementation)
import pytest
from dfs import dfs_recursive, dfs_iterative

def test_data():
    test_graph = {
        'A' : ['B','S'],
        'B' : ['A'],
        'C' : ['D','E','F','S'],
        'D' : ['C'],
        'E' : ['C','H'],
        'F' : ['C','G'],
        'G' : ['F','S'],
        'H' : ['E','G'],
        'S' : ['A','C','G']
    }

    assert dfs_iterative(test_graph, 'A') is not None
    assert len(dfs_iterative(test_graph, 'A')) == len(list(test_graph.keys()))
    assert dfs_recursive(test_graph, 'A') is not None
    assert len(dfs_recursive(test_graph, 'A')) == len(list(test_graph.keys()))

def test_graph():
    test_graph = {
        1: [2, 3]
    }

    if len(test_graph.keys()) < 2:
        print("\nA graph has to have atleast 2 vertices")

def all_unique(x):
    """
    Check if all elements in a list are unique; if not exit erly
    """
    seen = set()
    return not any(i in seen or seen.add(i) for i in x)

def test_unique():
    test_graph = {
        'A': ['B', 'C'],
        'B': ['A', 'D', 'E'],
        'C': ['A', 'F'],
        'D': ['B'],
        'E': ['B', 'F'],
        'F': ['C', 'E']
    }

    assert all_unique(dfs_iterative(test_graph, 'A')) is True
    assert all_unique(dfs_recursive(test_graph, 'A')) is True

def test_vertex1():
    test_graph = {
        'A': ['B', 'C'],
        'B': ['A', 'D', 'E'],
        'C': ['A', 'F'],
        'D': ['B'],
        'E': ['B', 'F'],
        'F': ['C', 'E']
    }

    with pytest.raises(KeyError) as excinfo:
        dfs_iterative(test_graph, 'Z')

    assert 'Z' in str(excinfo.value)
    assert 'KeyError' in str(excinfo.type)
    print('\nVertex does not exist')

def test_vertex2():
    test_graph = {
        1: [2, 3],
        2: [2, 3, 4],
        3: [],
        4: [],
    }

    for key, value in test_graph.items():
        if key in value:
            pytest.fail("A vertex cannot point to itself")    # explicitly fails this test

Please suggest any changes, additions or improvements.

Three observations:

There are two many assert statements in a single test function. Ideally each function should only test one thing. That way when the test fails you know exactly what is wrong.
Instead of initializing test data in the test function, you can write fixtures. Each fixture can be used in multiple test functions.
You don't need to assert 'KeyError' in str(excinfo.type) since you already has with pytest.raises(KeyError).


Need to clean up my code and add the ability to resize the image I have the following code:

<?php
include '../../inc/config.php';

$size = isset($_REQUEST['size']) ? $_REQUEST['size'] : 'full';

$image = isset($_REQUEST['image']) ? $_REQUEST['image'] : FALSE;

if(!$image) {
    print 'Nenhuma imagem definida!';
    exit;
}

$img_hash = $PDO->prepare("SELECT url FROM missionary_photos WHERE hash = :hash");
$img_hash->bindValue(':hash', $image);
$img_hash->execute();
$img_hash = $img_hash->fetchAll(PDO::FETCH_ASSOC);
$img_url = SITE_URL.$img_hash[0]['url'];

// Get the image mime type
function get_mime_type($url)
{

$ch = curl_init($url);
curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
curl_setopt($ch, CURLOPT_FOLLOWLOCATION, true);
curl_setopt($ch, CURLOPT_HEADER, 1);
curl_setopt($ch, CURLOPT_NOBODY, 1);
curl_exec($ch);

return curl_getinfo($ch, CURLINFO_CONTENT_TYPE);

}

$mime_type = get_mime_type($img_url);

header('Content-type: '.$mime_type);

readfile($img_url);

I want to clean up the code a little bit and add the ability to resize the image using a GET or POST. Any ideas?

You can use PHP's getimagesize() function to get the MIME type.

<?php
include '../../inc/config.php';

if(!isset($_REQUEST['image'])) die('Nenhuma imagem definida!');

$size = isset($_REQUEST['size']) ? $_REQUEST['size'] : 'full';
$image = $_REQUEST['image'];

$img_hash = $PDO->prepare("SELECT url FROM missionary_photos WHERE hash = :hash");
$img_hash->bindValue(':hash', $image);
$img_hash->execute();
$img_hash = $img_hash->fetchAll(PDO::FETCH_ASSOC);
$img_url = SITE_URL.$img_hash[0]['url'];

$info = getimagesize($img_url);
header('Content-type: '.$info['mime']);

readfile($img_url);


Convert binary string to decimal number I'm finishing the book C Primer Plus by Stephen Prata and I wrote as a part of the homework program to convert binary string to decimal number:

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

typedef unsigned char byte;
typedef unsigned int uint;

int strbin_to_dec(const char *);

int main(void) {
    char * wbin = "01001001";
    printf("%s to dziesietnie %d.\n", wbin, strbin_to_dec(wbin));
    return 0;
}

int strbin_to_dec(const char * str) {
    uint result = 0;
    for (int i = strlen(str) - 1, j = 0; i >= 0; i--, j++) {
        byte k = str[i] - '0'; // we assume ASCII coding
        k <<= j;
        result += k;
    }
    return result;
}

Questions:

How's the efficiency of this program?
Are byte operations faster than
normal?

Well, first of all the description is wrong.
strbin_to_dec() should be strbin_to_i(), because it parses a base-2-number, instead of converting it to base-10.

Next, refrain from using your own defines as much as possible, if there is an appropriate type already.

Third, assuming correct input is generally naive. Assuming corrupted or malicious input is far wiser.
Your code breaks if any character is neither 0 nor 1, or if the number is too big for an unsigned.

Finally, it's more straight-forward to parse from beginning to end, only needs a single pass, and is probably due to both reasons slightly more efficient, if you really care. Still, what input you get might be important to the determination.

Regarding your question whether byte-operations are faster than wider operations, probably only on 8bit-microcontrollers or the like.
Still, micro-optimisations are hard to do correctly, and have hardly any significant effect, especially if you don't know what to optimise for: Source clarity (should be the default), source size, compiled code size, code throughput, memory bandwidth, ...
Let's hope you chose the right optimisation-options, or the point is moot anyway.


Mathematical matrices implementation As a Java programmer who's trying his hand at C++, I thought I'd start building a library for working with matrices as my first C++ project. I've got very little C++ experience and it's been a long time since I've even so much as dabbled in it.

So far I've only implemented an addition function, but I think this should be enough code to establish my current coding style.

I was wondering if anyone would be able to give my code a look over and see if it conforms to standards and if there are any cleaner ways of doing anything.

Here is my matrix.h file:

#ifndef MATRIX_H
#define MATRIX_H

#include <iostream>

using namespace std;

class Matrix {

    private:
        int** array;

    public:
        int m, n;
        Matrix(int m, int n);

        static Matrix* add(Matrix* m1, Matrix* m2);
        int get(int i, int j);
        void set (int i, int j, int e);

        ~Matrix();

};

#endif

and my matrix.cpp file:

#include <iostream>
#include "matrix.h"

using namespace std;

Matrix::Matrix(int var_m, int var_n) {
    m = var_m;
    n = var_n;
    array = new int*[n];
    for (int i = 0; i < n; i++)
        array[i] = new int[m];
    cout << "foo" << endl;
    cout << m << " " << n << endl;
}

Matrix* Matrix::add(Matrix* m1, Matrix* m2) {

    if (m1->m != m2->m or m1->n != m2->n) {
        cerr << "Error: Mismatched sizes in Matrix.add()" << endl;
        throw;
    }

    Matrix* res = new Matrix(m1->m, m1->n);

    for (int i = 0; i < res->m; i++) {
        for (int j = 0; j < res->n; j++) {
            res->array[i][j] = m1->array[i][j] + m2->array[i][j];
        }
    }

    return res;
}

int Matrix::get(int i, int j) {
    return array[i][j];
}

void Matrix::set(int i, int j, int e) {
    array[i][j] = e;
}

Matrix::~Matrix() {
    delete array;
}

and just some test code thrown together in my main.cpp:

#include <iostream>
#include "matrix.h"

using namespace std;

int main() {

    // --- Test code ---
    int m, n;
    Matrix* mat1;
    Matrix* mat2;
    Matrix* mat3;

    cout << "m: ";
    cin >> m;
    cout << "n: ";
    cin >> n;

    mat1 = new Matrix(m, n);
    mat2 = new Matrix(m, n);

    mat1->set(0,2,5);

    mat3 = Matrix::add(mat1, mat2);

    for (int i = 0; i < mat3->m; i++) {
        for (int j = 0; j < mat3->n; j++) {
            cout << mat3->get(i,j);
        }
        cout << endl;
    }

    delete mat1, mat2, mat3;

    return 0;
}

There is a bug in your Matrix destructor. delete array is not correct. It was allocated using an array form of the new operator, so it must be deleted using an array form of delete(that is, delete[]). The current version of your code invokes undefined behavior. But simply changing it to delete[] array is not enough to fix it. There is still a memory leak here. A correct version of the destructor should look this way:

~Matrix() {
    for (int row = 0; row < n; row++)
        delete[] array[row];
    delete[] array;
}

It is necessary because deleting an array of pointers does not delete the objects pointed by this pointers.
It looks like the size of the array is not correct, either. You allocate an array of pointers of size n, but then you use it as if it had a size m.

array = new int*[n];
for (int i = 0; i < n; i++)
    array[i] = new int[m];
...
for (int i = 0; i < res->m; i++) {
    for (int j = 0; j < res->n; j++) {
        res->array[i][j] = m1->array[i][j] + m2->array[i][j];
    }
}

This code invokes undefined behavior in case n != m.
You should probably implement a copy constructor and a copy-assignment operator. A default copy constrcutor(and assignment operator) just copies the pointer, not the content of the Matrix(I'm not sure if you it is what you want).
A good way to get rid of many bugs in your code is to use standard containers instead of dynamic memory allocation with raw pointers. For example, you can use a std::vector to store the elements of the Matrix:

 class Matrix {
     std::vector<std::vector<int>> array;
     ...
 };

It allows you to avoid manual memory management issues.
Using raw pointers and the new operator is not common in modern C++ in general.
C++ has operator overloading, and it is usually used in C++ programs. For example, matrix addition is a good candidate for overloading the + operator.

class Matrix {
    ...
public:
    Matrix operator+(const Matrix& rhs) const {
        // Check the validity of input parameters.
        Matrix res(n, m);
        // Addition goes here.
        return res;
    }

    ...
};

This example also shows a conventional way to pass arguments without copying them: by const reference(passing a pointer to them is not common in modern C++).
Member-functions that do not change the instance should have a const qualifier.
It is a good practice to write unit tests instead of ad-hoc testing(it is not C++ specific, of course).


Haskell#concat Implementation Here's how I implemented concat - flattens a list of lists into just a list of elements.

concat' :: [[a]] -> [a]
concat' ys = foldl (\acc x -> acc ++ x) [] ys

Is it a problem that I'm using the ++ function?

What's a better way to write this?

You could eta-contract the definition in two places: \acc x -> acc ++ x is the eta-expanded version of (++) and concat' ys = (...) ys is the eta-expanded version of concat' = (...). So that would take you to:

concat' :: [[a]] -> [a]
concat' = foldl (++) []

Now, in that case it turns out that it's better to use foldr rather than foldl and there is a good write-up about concat on the haskell wiki.


Loop to copy values from one sheet and paste the values 68 times in another, time efficiency I have created a large VBA program to automate creation of a data table that is needed to run slicers in an Excel file.
While the loop works well in creating what I need, the main loop take an hour to create the list of company names that I need. I was wondering if there is a way to improve the time it takes for the loop to complete.
I have 191 rows that need to be copied and then pasted 68 times each into the new sheet. I have tried a few different approaches to improve the time and have only reduced the time required to about 50 minutes. Any help would be much appreciated. I know that using select is horrible for time efficiency but all the other options I have tried have not worked well.

Application.ScreenUpdating = False

Dim rng As Range, cell As Range
For Each cell In rng

    Sheets("Input Data").Select
    cell.Select
    cell.Copy
    Sheets("TrialSheet").Select
    For i = 1 To 68
        LastRow = ActiveSheet.Cells(Rows.Count, "A").End(xlUp).Row + 1
        Range("A" & LastRow).Select
        ActiveSheet.Paste
    Next i

    Sheets("Input Data").Select

Next cell

Application.ScreenUpdating = True

Since no information was available about the size of source range being copied
Following grey area in the question is assumed as follows

Since 191 Rows X 68 copy X 3 columns take around 10 minutes only (with you code), the range is about 191 Rows X 15 Columns in size

since it has been claimed that code is working Correctly. The Cells of the range (irrespective of their row or column positions) is being copied in column A only (below one another). Though it contradicts the statement "automate creation of a data table"

Since the cells of the ranges are being copied and pasted. In test case only formulas are copied.
So the code below will just replicate what your code is doing with some increased efficiency. As I personally don't prefer to keep calculations, event processing and screen updating off (in normal cases) i haven't added that standard lines. However you may use these standard techniques, depending on the working file condition. Make necessary changes regarding Range etc

Code takes only 2-3 seconds to complete with  191 Rows X 15 columns X 68 copies:
Sub test()
Dim SrcWs As Worksheet, DstWs As Worksheet, SrcArr As Variant
Dim Rng As Range, cell As Range, DstArr() As Variant
Dim X As Long, Y As Long, Z As Long, i As Long, LastRow As Long
Dim Chunk60K As Long
Dim tm As Double
tm = Timer
Set SrcWs = ThisWorkbook.Sheets("Input Data")
Set DstWs = ThisWorkbook.Sheets("TrialSheet")

Set Rng = SrcWs.Range("A1:O191")
SrcArr = Rng.Formula

LastRow = DstWs.Cells(Rows.Count, "A").End(xlUp).Row + 1
Chunk60K = 0
Z = 1
    For X = 1 To UBound(SrcArr, 1)
    For Y = 1 To UBound(SrcArr, 2)
    For i = 1 To 68
        ReDim Preserve DstArr(1 To Z)
        DstArr(Z) = SrcArr(X, Y)

        If Z = 60000 Then  ' To Overcome 65K limit of Application.Transpose
        DstWs.Range("A" & Chunk60K * 60000 + LastRow).Resize(UBound(DstArr, 1), 1).Formula = Application.Transpose(DstArr)
        Chunk60K = Chunk60K + 1
        Z = 1
        ReDim DstArr(1 To 1)
        Debug.Print "Chunk: " & Chunk60K & " Seconds Taken: " & Timer - tm
        Else
        Z = Z + 1
        End If

    Next i
    Next Y
    Next X

If Z > 1 Then DstWs.Range("A" & Chunk60K * 60000 + LastRow).Resize(UBound(DstArr, 1), 1).Formula = Application.Transpose(DstArr)

Debug.Print "Seconds Taken: " & Timer - tm
End Sub

Edit: With full credit to @TinMan's brilliant suggestion in his comment, the code has been modified (efficiency improved around 65%). The inherent deficiency in the above code is use of ReDim Preserve within the loop has been removed. This enabled use of 2D array and eliminates necessity of transposing the array. Again with many thanks to TinMan and acknowledging full credit to TinMan, The final simplified code as follows.
Sub test()
Dim SrcWs As Worksheet, DstWs As Worksheet, SrcArr As Variant
Dim Rng As Range, cell As Range, DstArr() As Variant
Dim X As Long, Y As Long, Z As Long, i As Long, Lastrow As Long
Dim Repeat As Long
Dim tm As Double
tm = Timer
Set SrcWs = ThisWorkbook.Sheets("Input Data")
Set DstWs = ThisWorkbook.Sheets("TrialSheet")
Set Rng = SrcWs.Range("A1:O191")
SrcArr = Rng.Formula
Repeat = 68
ReDim DstArr(1 To (Repeat * Rng.Count), 1 To 1)
Lastrow = DstWs.Cells(Rows.Count, "A").End(xlUp).Row + 1
Z = 1

    For X = 1 To UBound(SrcArr, 1)
    For Y = 1 To UBound(SrcArr, 2)
    For i = 1 To Repeat
    DstArr(Z, 1) = SrcArr(X, Y)
    Z = Z + 1
    Next i
    Next Y
    Next X

DstWs.Range("A" & Lastrow).Resize(UBound(DstArr, 1), 1).Formula = DstArr
Debug.Print "Seconds Taken: " & Timer - tm
End Sub


Instantiating objects with many attributes I have a class with quite a few attributes, most of which are known when I create an instance of the object.  So I pass all the values in the constructor:

$op = new OpenIdProvider($imgPath . $name . $ext, 'openid_highlight',
                         0, 0, 108, 68, 6, $info[0], $info[1], $name);

I'm finding that having this many parameters makes it confusing both when writing and reading the code, as it's not easy to determine which attribute each value corresponds to.  Also, this has a bit of a code smell to it - seems like there should be a better way.  Any suggestions?

Martin Fowler's bible book Refactoring does identify a smell called "Long Parameter List" (p78) and proposes the following refactorings:

Replace Parameter with Method (p292)
Introduce Parameter Object (295)
Preserve Whole Object (298)

Of these I think that "Introduce Parameter Object" would best suit:

You'd wrap the attributes up in their own object and pass that to the constructor.  You may face the same issue with the new object if you choose to bundle all the values directly into its' constructor, though you could use setters instead of parameters in that object.

To illustrate (sorry, my PHP-fu is weak):

$params = new OpenIDParams();
$params->setSomething( $imgPath . $name . $ext );
$params->setSomethingElse( 'openid_highlight' );

$params->setName( $name );

$op = new OpenIdProvider( $params );

This is a little more verbose but it addresses your concern about not being clear about the attributes' purpose / meaning.  Also it'll be a little less painful to add extra attributes into the equation later.


AsyncTask for handling server api calls Got this design for getting rid of checking network availability and informing user of errors. Is it good at all? Please, help.

Overall point is to fire server call as short as possible:

new DefaultServerCallTask<LoginResponse>(){

    @Override
    protected void onSuccess(LoginResponse response){
       //go to nextActivity}
    }

    @Override void callServer() throws ConnectionException, InternalServerException, JSONException {
        return ServerApi.login(/*enclosed params from outer variables */);
    }
}.execute();

AsyncResult is a wrapper class, it's main purpose to be returned from AsyncTask.doInBackground.

CheckableResult is an inteface for POJOs, returned from server.

class AsyncResult<T>{

    T mResponse;
    boolean mOk;
    String mErrorMessage;

    T getResponse(){
        return mResponse;
    }

    boolean isOk(){
    return mOk;
}

String getErrorMessage(){
    return mErrorMessage;
}

public AsyncResult (T response){
    mErrorMessage = "";
    mOk = true;
    mResponse = response;
}

public AsyncResult (String errorMessage){
    mErrorMessage = errorMessage;
    mOk = false;
}

}

AbstractServerCallTask handles connection and server-side exceptions^

abstract class AbstractServerCallTask<Result extends CheckableResponse> extends AsyncTask<Void, Void, AsyncResult<Result>>{

@Override
protected AsyncResult<Result> doInBackground(Void... arg0) {
    if (!AppUtils.isNetworkAvailable())
                return new AsyncResult("No network available. Please, check your connection");
    try {
        return new AsyncResult<Result>(callServerMethod());
    } catch (ConnectionException e) {
        return new AsyncResult<Result>("Can't connect to server, chek your network state");
    } catch (InternalServerException e) {
        return new AsyncResult<Result>("Server-side fault. Please, try again later");
    } catch (JSONException e) {
        return new AsyncResult<Result>("Wrong data format. Please try again later");
    }
}

abstract Result callServerMethod() throws ConnectionException, InternalServerException, JSONException ;

@Override
protected void onPostExecute(AsyncResult<Result> asyncResult){
    if (asyncResult.isOk()){
        Result r = asyncResult.getResponse();
        if (r.isOk())
            onSuccess(r);
        else
            onRequestFailed(r); // we got to server, but somehing is wrong
    }
    else
        onError(asyncResult.getErrorMessage());
}
protected abstract void onError(String errorMessage);
protected abstract void onRequestFailed(Result r) ;
protected abstract void onSuccess(Result r);
}

DefaultServerCallTask leaves onSuccess for derived innerclasses to override:

abstract class DefaultServerCallTask<Result extends CheckableResponse> extends AbstractServerCallTask<Result>{

@Override
protected void onError(String errorMessage) {
    AppUtils.showToast(errorMessage);

}

@Override
protected void onRequestFailed(Result r) {
    AppUtils.showToast(r.getErrorMessage()); // show server-side generated message
}
}

There are many things that you do well here. Overall the code is fine.
I have some points that you should seriously consider and some things that you should think about. In the end, this is your code and I don't know everything that you know about it's purpose, usage and further plans.
Seriously consider

Android provides a lovely way to handle String Resources. I suggest you make use of that. Not only is it very handy to keep all your strings stored in .xml-files but it is also impossible to internationalize your messages if you hardcode string values within your code.

As AsyncResult seems to be a good, flexible, generic (generic as in both Java-generics and English generic) utility class, it wouldn't hurt to put getters as public and the fields as private final

Think about

What if the server provides both an error message and more detailed information as a response? I suggest that you create a constructor for your AsyncResult which accepts both T response and String errorMessage. You can then use constructor chaining in your class. Constructor chaining is a good way to reduce code duplication in constructors.

I find the naming of DefaultServerCallTask to be a bit misleading, the usage of the word Default together with the keyword abstract on the same class doesn't make sense to me. If it really is Default, then why is it abstract? (Although I understand your reasons for having it this way, I question the naming of the class).

Your AbstractServerCallTask extends AsyncTask<Void, Void, AsyncResult<Result>>. It is functional and I understand that you can specify data to be sent to the server by calling a constructor and storing it in a field, or creating an inner anonymous class which you actually do when you call new DefaultServerCallTask<LoginResponse>(){ ... }.execute();.
I think that you should consider allowing change of the first Void generics, which is the Param class. If you would allow changing that, then you can pass arguments to your execute method, which can be passed on to callServerMethod instead of having to get variables from outside into your new DefaultServerCallTask<LoginResponse>(){...

It is OK to have a AsyncResult<T> class, but will you have any use for the generics in that class outside your server calls? Your server calls only makes use of AsyncResult<Result>. As hinted by rolfl in his comment, a user currently can't use a AsyncResult<String> along with your classes for server calls.

I want to leave you with something positive though: Overall, your code is fine. So for your question "Is it good at all?" I have to say "yes".