JavaScript - Comparing 2 arrays - Would like to check if I'm reinventing the wheel or am I good to go? I would like to be able to use it with IE8 or earlier, so I'm staying away from newer array methods. I'm not an expert so my code may not be the most efficient choice, but it works.

var data = [
["John", 2],
["Jenny", 3],
["John", 4],
["Beavis", 5]]

var newarr = [["John",5]];

for (var x=0; x < data.length; x++) {

    for (var y=0; y < newarr.length+1; y++) {
        if (typeof newarr[y] !== 'undefined') {
            if (newarr[y][0] == data[x][0]) {
                newarr[y][1] = (newarr[y][1] + data[x][1]);
                break;
            }
        }
        else {
            newarr.push([data[x][0]]);
            newarr[y][1] = data[x][1];
            break;
        }
    }

}

alert("newarr is: " + newarr);

//if elements are similar, merge them.

Would like to find out if this is a decent way of doing it, or maybe there are other easier/less code ways of doing it. Maybe somebody sees an issue with this code that I do not. Thanks.

You should take a look at the Underscore javascript library, which contains a number of methods for comparing and manipulating arrays and objects.

I'm using the _.intersection and _.keys methods to compare your two objects. I've also taken the liberty of restructuring your arrays into objects, which I think makes more sense for what you're trying to do.

// Adds the values of keys that are the same between any two objects
function addObjects(a, b) {
  var sameKeys = _.intersection(_.keys(a), _.keys(b)),      // Creates array of similary keys. eg ["John", "Jenny", "Amanda"]
      sumObj = {}

  // Loop through sameKeys, and add values from both objects
  for(i=0; i < sameKeys.length; i++) {
    var thisKey = sameKeys[i];
    sumObj[thisKey] = a[thisKey] + b[thisKey];              // eg. sumbObj["John"] = a["John"] + b["John"] = 7
  }

  return sumObj;
}

var firstObj = {
  "John" : 2,
  "Jenny" : 3,
  "Beavis" : 5,
  "Toby": 6,
   "Amanda": 12
}

var secondObj = {
  "John": 5,
  "Jenny": 18,
  "Amanda": 2
}

mySumObj = addObjects(firstObj, secondObj);
/*
mySumObj = {
    "Amanda": 14,
    "Jenny": 21,
    "John": 7
}
*/

I hope this helps you. It may help if you could explain a bit more what your end goal here is.


Seed to flower/mushroom I've already done quite a bit of cleaning up. For example, the strings dryCheck and blueCheck no longer exist, instead they're replaced with "else". But I think this could be done 10x better, so if you know how (no advanced C++ lingo etc), that would be great!

  A red seed will grow into a flower when planted in soil temperatures
  above 75 degrees, otherwise it will grow into a mushroom. Assuming the
  temperature meets the conditions for growing a flower, planting a red
  seed in wet soil will produce a sunflower and planting a red seed in
  dry soil will produce dandelion. A blue seed will grow into a flower
  when planted in soil temperatures ranging from 60 to 70 degrees,
  otherwise it will grow into a mushroom. Assuming the temperature meets
  the conditions for growing a flower, planting a blue seed in wet soil
  will produce a dandelion and planting the blue seed in dry soil will
  produce a sunflower. Write a program that will ask the user to input
  the seed color, the soil temperature and whether the soil is wet or dry
  and then output the plant that will grow.

#include <iostream>
#include <string>
using namespace std;

int main()
{
    string seedColor, soilMoist, redCheck, wetCheck;
    int soilTemp;

    redCheck = "Red";
    wetCheck = "Wet";
    cout << "Enter the seed type (Blue/Red): ";
        getline(cin, seedColor);

    cout << "Enter the soil temperature: ";
        cin >> soilTemp;

    cout << "Is the soil dry or wet? ";
        cin >> soilMoist; // I tried another "getline(cin, soilMoist), but that conflicts with the cin >> soilTemp; (line 30)

    if (seedColor == redCheck)
    {
        if (soilTemp > 75)
            {
            if (soilMoist == wetCheck)
                cout << "It's a sunflower!";
            else
                cout << "It's a dandelion!";
            }
        else
            cout << "It's a mushroom!";
    }

    else
    {
        if (60 < soilTemp && soilTemp < 70)
            {
            if (soilMoist == wetCheck)
                cout << "It's a dandelion!";
            else
                cout << "It's a sunflower!";
            }
        else cout << "It's a mushroom!";
    }
}

Just some quick pointers here to get the more obvious details out of the way.  First, you should not use using namespace std;, but rather explicitly state which namespace you are using like this std::cout.  This prevents namespace clashes, and is discussed in great detail here.

Second, you should use braces around your if/else statements and loops even when there is only one statement after it:

if (soilMoist == wetCheck)
    cout << "It's a sunflower!";
else
    cout << "It's a dandelion!";

Should be:

if (soilMoist == wetCheck)
{
    std::cout << "It's a sunflower!";
}
else
{
    std::cout << "It's a dandelion!";
}

Right here, you should not indent your braces:

if (soilTemp > 75)
    {
    if (soilMoist == wetCheck)
        cout << "It's a sunflower!";
    else
        cout << "It's a dandelion!";
    }

You do not do this in other areas, so I don't know why you did it here.

You should also not indent your cins:

cout << "Enter the soil temperature: ";
    cin >> soilTemp;

You should have your prompts always specify exactly what the input should be.  Here, it looks as if you expect lowercase input: "Is the soil dry or wet? "  However, the program checks for uppercase input.  The best way to fix this would be to allow both "wet" or "Wet".

Otherwise, this looks pretty good.  However, one way you may want to consider expanding your program in is ensuring the user put in the right values instead of merely assuming the right values will be input.  You could do this with a do-while loop, among other ways.

This code is an example of what I mean:

string input = "";

do {
    if (input == "")
    {
        std::cout << "Is the soil dry or wet? ";
    }
    else
    {
        std::cout << "Invalid response.\nIs the soil dry or wet? ";
    }

    std::cin >> input;

} while (input != "Wet" && input != "wet");


Is there a better way to write insertion sort? Background

I am doing an insertion sort and I would like it to be as efficient as the algorithm allows.

Code

After much research, this is what I made:

const isFunction = require("lodash.isfunction"); //copy pasta from lodash :D

const insertion = ( array ) => {

    if(!Array.isArray(array))
        throw new Error("array must be an Array");

    if (array.length === 0)
        return [];

    //shallow copy
    const clonedArray = array.slice();

    //Optimized iterative version: https://en.wikipedia.org/wiki/Insertion_sort
    let i = 1, j, temp;
    while( i < clonedArray.length ){
        temp = clonedArray[i];
        j = i - 1;
        while( j >= 0 && clonedArray[ j ] > temp ){
            clonedArray[ j + 1 ] = clonedArray[ j ];
            j = j - 1;
        }
        clonedArray[ j + 1 ] = temp;
        i = i + 1;
    }

    return clonedArray;
};

module.exports = insertion;

Questions

I tested the code and I know it works. But I wonder if the algorithm available in wikipedia is the best insertion sort algorithm.

Having in mind I wish this function's API to be pure ( I don't want to change the original array, I want to return a new one), I have the following questions:

Is there a better way to code insertion sort?
What improvements can be made to this code?

Other than rewriting it in a less pseudo-code-like way:

const insertion = ( array ) => {

    if(!Array.isArray(array))
        throw new Error("array must be an Array");

    if (array.length === 0)
        return [];

    //shallow copy
    const clonedArray = array.slice();

    //Optimized iterative version: https://en.wikipedia.org/wiki/Insertion_sort
    let temp;
    for (let i = 1; i < clonedArray.length; i++) {
      temp = clonedArray[i];
      for (let j = i-1; j >= 0 && clonedArray[j] > temp; j--) {
        clonedArray[j + 1] = clonedArray[j];
      }
      clonedArray[j + 1] = temp;
    }

    return clonedArray;
};

module.exports = insertion;

There isn't much room for improvement from an algorithm perspective, since you're already using the optimized algorithm and you aren't doing anything unnecessary from a JavaScript perspective.

Rewriting it to use for() instead of while() won't have any real effect, other than it just looks less like pseudo-code and more like typical JS. It is really just a code style thing though, and open to opinion.

(Also, dropped isFunction() since you don't seem to use it, but that might just be copy-pasta from the rest of your code.)


Finding the Greatest Integer and Ties I'm trying to find the greatest integer of four integers and its ties.  I used the following code.  Is there anyway to simplify this?

int winner = MAX(self.playerOneScore, MAX(self.playerTwoScore, MAX(self.playerThreeScore, self.playerFourScore)));

if (winner == self.playerOneScore && winner != self.playerTwoScore && winner != self.playerThreeScore  && winner != self.playerFourScore) {
    NSLog(@"1");
} else if (winner == self.playerTwoScore && winner != self.playerOneScore && winner != self.playerThreeScore && winner != self.playerFourScore) {
    NSLog(@"2");
} else if (winner == self.playerThreeScore && winner != self.playerOneScore && winner != self.playerTwoScore && winner != self.playerFourScore) {
    NSLog(@"3");
} else if (winner == self.playerFourScore && winner != self.playerOneScore && winner != self.playerTwoScore && winner != self.playerThreeScore) {
    NSLog(@"4");
} else if (self.playerOneScore == self.playerTwoScore || self.playerOneScore ==  self.playerThreeScore || self.playerOneScore == self.playerFourScore || self.playerTwoScore == self.playerThreeScore || self.playerTwoScore == self.playerFourScore || self.playerThreeScore == self.playerFourScore) {
    NSLog(@"Tie");
}

Your code looks correct (apart from the typo in case #4). Note that the final else if is not necessary.
If none of the first 4 tests passes then you necessarily have a tie,
so a simple else is sufficient here.

The winner variable is named badly, as it contains the maximal score and
not the winning player.

But you are using far too many comparisons, and the code does not scale well for a larger number of players. What you need to know is

Which player has the maximum score?
How many players have the maximum score?

Both can more easily determined if all scores are stored in an array.

Since you
are working with Objective-C, I would suggest to take advantage of the
classes and methods that the Foundation framework provides.

First create an NSArray of all player scores. Objective-C collections can only
contain objects, therefore the integers have to be boxed into NSNumber
with @(...):

NSArray *scores = @[@(score1), @(score2), @(score3), @(score4)];

Use Key-Value Coding to find the maximum element:

NSNumber *maxScore = [scores valueForKeyPath:@"@max.self"];
NSLog(@"maximum score is %@", maxScore);

Use indexesOfObjectsPassingTest: to find all objects in the scores array
that are equal to the maximum score:

NSIndexSet *bestPlayers = [scores indexesOfObjectsPassingTest:^BOOL(NSNumber *obj, NSUInteger idx, BOOL *stop) {
    return [obj isEqual:maxScore];
}];

If the index set has only one element then there is a unique winner, otherwise a tie:

if ([bestPlayers count] == 1) {
    NSLog(@"Best player is #%ld", [bestPlayers firstIndex] + 1);
} else {
    NSLog(@"Tie between");
    [bestPlayers enumerateIndexesUsingBlock:^(NSUInteger idx, BOOL *stop) {
        NSLog(@"  Player #%ld", idx + 1);
    }];
}

(Array indices are 0-based, therefore you add one to get the player number.)


Iter 1: Reusable, robust c++ std::hash for GMP's big integer type This Q&A was iteration 1. Now there is
Iter 2: Reusable, robust c++ std::hash<mpz_class> for GMP's big integer type
1. Goal
My intention is to provide a fast hashing algorithm  to hash GMP's big integer type mpz_class and mpz_t so I can use these types as keys for an unordered_map. The code shall be reusable for others.
cf. my stackoverflow question about this topic
2. My Approach
I've written the C++ glue code to use MurmurHash3 to hash a GMP big integer.
The questions about the code are at the bottom of this post.
3. Code
File hash_mpz.h:
#ifndef HASH_MPZ_H_
#define HASH_MPZ_H_

#include <gmpxx.h>

namespace std {

template<> struct hash<mpz_srcptr> {
    size_t operator()(const mpz_srcptr x) const;
};

template<> struct hash<mpz_t> {
    size_t operator()(const mpz_t &x) const;
};

template<> struct hash<mpz_class> {
    size_t operator()(const mpz_class &x) const;
};

}

#endif /* HASH_MPZ_H_ */

File hash_mpz.cpp:
#include "hash_mpz.h"
#include "MurmurHash3.h"

size_t MurmurHash3_size_t(const void *key, int len, uint32_t seed) {

#if SIZE_MAX==0xffffffff
    size_t result;
    MurmurHash3_x86_32(key, len, seed, &result);
    return result;

#elif SIZE_MAX==0xffffffffffffffff
    size_t result[2];
    MurmurHash3_x64_128(key, len, seed, &result);
    return result[0] ^ result[1];

#else
#error cannot determine correct version of MurmurHash3, because SIZE_MAX is neither 0xffffffff nor 0xffffffffffffffff
#endif

}

namespace std {

size_t hash<mpz_srcptr>::operator()(const mpz_srcptr x) const {
    // found 1846872219 by randomly typing digits on my keyboard
    return MurmurHash3_size_t(x->_mp_d, x->_mp_size * sizeof(mp_limb_t),
            1846872219);
}

size_t hash<mpz_t>::operator()(const mpz_t &x) const {
    return hash<mpz_srcptr> { }((mpz_srcptr) x);
}

size_t hash<mpz_class>::operator()(const mpz_class &x) const {
    return hash<mpz_srcptr> { }(x.get_mpz_t());
}

}

File main.cpp:
#include <iostream>
#include <gmpxx.h>
#include <unordered_map>

#include "hash_mpz.h"

using namespace std;

int main() {
    mpz_class a;

    mpz_ui_pow_ui(a.get_mpz_t(), 168, 16);

    cout << "a      : " << a << endl;
    cout << "hash(a): " << (hash<mpz_class> { }(a)) << endl;

    unordered_map<mpz_class, int> map;
    map[a] = 2;
    cout << "map[a] : " << map[a] << endl;

    return 0;
}

(click to view MurmurHash3.h)

4. Questions
4.1. In MurmurHash3_size_t(), I check SIZE_MAX to tell whether I'm on a 32 bit system or a 64 bit system. Does this check make sense, are there alternatives which are better?
4.2. Near the end of the file hash_mpz.cpp, I have the statement return hash<mpz_srcptr> { }(x.get_mpz_t());. Does this create an object of type hash<mpz_srcptr> for every calculated hash value?
4.3. I'm not sure whether I'm using the references const mpz_class &x and const mpz_t &x in the most efficient way. This is partly due to GMP being a C library, mpz_t is a 1 element array with element type __mpz_struct and mpz_class being something rather obscure to me.
Excerpt from gmp.h:
typedef struct
{
  int _mp_alloc;        /* Number of *limbs* allocated and pointed
                   to by the _mp_d field.  */
  int _mp_size;         /* abs(_mp_size) is the number of limbs the
                   last field points to.  If _mp_size is
                   negative this is a negative number.  */
  mp_limb_t *_mp_d;     /* Pointer to the limbs.  */
} __mpz_struct;

//...

typedef __mpz_struct mpz_t[1];

Excerpt from gmpxx.h:
typedef __gmp_expr<mpz_t, mpz_t> mpz_class;

4.4. Last but not least, do you see anything else which can be improved?

Answers to your questions

In MurmurHash3_size_t(), I check SIZE_MAX to tell whether I'm on a 32 bit system or a 64 bit system. Does this check make sense, are there alternatives which are better?

I think it's not worth it to write a generic version of this that avoids checking SIZE_MAX, even though that is possible. Machines with a different size_t size than 32 or 64 bits are uncommon, unless you're talking about 8 and 16 bit machines, but I'm assuming you are not interested in using bigints on those.
But don't forget to #include <cstdint>, otherwise SIZE_MAX might not be defined! Alternatively, use sizeof(size_t) == 4 and == 8, in combination with if constexpr (...) instead of #if ....

Near the end of the file hash_mpz.cpp, I have the statement return hash<mpz_srcptr> { }(x.get_mpz_t());. Does this create an object of type hash<mpz_srcptr> for every calculated hash value?

Technically, yet. But those objects don't have any member variables, just a member function operator(), which is resolved at compile-time, so there is no need to worry about inefficiencies here.

I'm not sure whether I'm using the references const mpz_class &x and const mpz_t &x in the most efficient way. This is partly due to GMP being a C library, mpz_t is a 1 element array with element type __mpz_struct and mpz_class being something rather obscure to me.

Using const references to the objects is perfectly fine.

Last but not least, do you see anything else which can be improved?

Yes, see below.
Your functions don't handle negative bigints correctly
As it mentions in the excerpt from gmp.h you posted, _mp_size will be negative for negative bigints. You need to handle this. One way would just be to write:
return MurmurHash3_size_t(..., abs(x->_mp_size) * sizeof(mp_limb_t), ...);

However, this means that for a given positive number, that number and its negative will get the same hash value. It may or may not be an issue for your application. One possible solution is to use a different seed value for negative numbers.
Consider using std::hash to calculate the hash
Yes, you can use std::hash instead of implementing your own. Of course, you need to pick an appropriate existing specialization std::hash. You could iterate over the limbs in the bignum, which are either 32 or 64 bit integers, so you could std::hash those individually, but then the questions is how to combine them. Just adding or XORing them might not result in a good final hash value if there are any patterns in the input. But in C++17, there is a specialization of std::hash for std::string_view. So you can create a view for the limb array, and hash that:
size_t hash<mpz_srcptr>::operator()(const mpz_srcptr x) const {
    std::string_view view{reinterpret_cast<char *>(x->_mp_d), abs(x->_mp_size) * sizeof(mp_limb_t)};
    return std::hash<std::string_view>{}(view);
}

There is a good chance that the standard library's hash function for strings is a variant of MurmurHash, see for example this question.
Use the appropriate cast operator instead of C-style casts
You should use static_cast<>() if possible to cast similar types, so the compiler will be able to generate an error if you are doing incorrect casts. For example:
size_t hash<mpz_t>::operator()(const mpz_t x) const {
    return hash<mpz_srcptr>{}(static_cast<mpz_srcptr>(x));
}


Copy data to another worksheet I have the following but it takes long time to run, it is a simple wherein the user select a file and data from Sheet1 is copied to another workbook.

Sub ImportApp(ByVal filepath_Report As String, file_name1 As String, wsOutput As Worksheet)
    Application.ScreenUpdating = False
    Set wbReport = Workbooks.Open(file_name1)
    Set wsReport = wbReport.Sheets(1)
    Select Case wsOutput.Name
        Case Is = "Downtilt Tracker"
            wsReport.Activate
            With Rows(1)
                Set d = .Find("Completed Date")
                If d Is Nothing Then
                    MsgBox "Invalid Down-Tilt Tracker." & vbNewLine & "Please import a valid Down-Tilt Tracker."
                    wbReport.Close False
                    Sheets("Control").Activate
                    Application.ScreenUpdating = True
                    Exit Sub
                End If
            End With
    End Select

    wsReport.Activate: wsReport.Cells.Copy: wsOutput.Activate: Cells(1, 1).Select: ActiveSheet.Paste: Cells.EntireColumn.AutoFit: wsOutput.Range("A1:AB" & 1048576).HorizontalAlignment = xlCenter
    wsOutput.Cells(1, 1).Select
    Application.CutCopyMode = False
    wbReport.Close False
End Sub

I like your indentation. It's nice to see well-indented VBA code, it needs to be pointed out!

Application.ScreenUpdating = False

That's generally a good idea, but all by itself, it makes a poor UX - and if calculation mode remains xlAutomatic, it might be a missed opportunity to gain some more performance by turning off calculations while you're working on the worksheet.

Thing is, that's a whole concern all by itself, and as such it deserves its own function. Almost all my macro-enabled Excel workbooks have this function somewhere:

Private Sub ToggleWaitMode(Optional ByVal wait As Boolean = True)
    With Excel.Application

        .Calculation = IIf(wait, xlCalculationManual, xlCalculationAutomatic)
        .Cursor = IIf(wait, xlWait, xlDefault)
        .StatusBar = IIf(wait, "Please wait...", False)

        .DisplayAlerts = Not wait
        .ScreenUpdating = Not wait

    End With

End Sub

So instead of Application.ScreenUpdating = False, you can say ToggleWaitMode, and then do ToggleWaitMode False when you're done processing.

The problem with turning ScreenUpdating off, is that if anything goes wrong, it stays off and the user (and sometimes even the dev) is left thinking Excel is frozen: whenever you play with ScreenUpdating, you must handle runtime errors:

Private Sub DoSomething()
    On Error GoTo CleanFail

    'do something

    CleanExit:
        'clean up code
        ToggleWaitMode False
        Exit Sub
    CleanFail:
        'error-handling code
        Resume CleanExit
End Sub

This way you're always sure that all exit paths involve resetting ScreenUpdating and calculation mode.

...which is a good idea, because the next line can actually blow up whenever a bad file_name1 is passed to the procedure:

Set wbReport = Workbooks.Open(file_name1)

Set wsReport = wbReport.Sheets(1)

I like that: by assigning an object reference, you can work against that object. There are a few issues:

Where's the declaration? Always use Option Explicit and declare all variables! If the variable is declared at module level, then the declaration belongs inside the procedure's scope - move it there, to the smallest possible scope.
The Sheets collection contains chart sheets and actual worksheets. You probably intend to query the Worksheets collection here; if you had declared the wsReport variable As Worksheet and the Sheets(1) object were actually a chart sheet, you'd have a runtime error here.

Select Case wsOutput.Name
    Case Is = "Downtilt Tracker"

Several things here:

You're clearly using the wrong construct here - this should definitely be an If...Then block; what's wrong with this?

If wsOutput.Name = "Downtilt Tracker" Then
    '...
End If

I'm actually surprised Case Is = "string literal" actually compiles and works as intended; it's a pretty convoluted way of doing Case "string literal"...

And then things get a bit out of hand:

        wsReport.Activate
        With Rows(1)
            Set d = .Find("Completed Date")
            If d Is Nothing Then
                MsgBox "Invalid Down-Tilt Tracker." & vbNewLine & "Please import a valid Down-Tilt Tracker."
                wbReport.Close False
                Sheets("Control").Activate
                Application.ScreenUpdating = True
                Exit Sub
            End If
        End With

You have a reference to the wsReport object - you don't need to .Activate it.
The With block adds unnecessary indentation and confusion. I'd do wsReport.Rows(1).Find instead.
d is a meaningless name that doesn't tell anything about what you're doing here.
You need a reference to the "Control" sheet. Declare and assign another Worksheet variable, and use it instead of Selecting and Activateing and using implicit-context Cells.
If d isn't Nothing, you don't appear to be setting ScreenUpdating back to True. The control flow described in my introduction would solve that.

Avoid this - AT ALL COSTS:

wsReport.Activate: wsReport.Cells.Copy: wsOutput.Activate: Cells(1, 1).Select: ActiveSheet.Paste: Cells.EntireColumn.AutoFit: wsOutput.Range("A1:AB" & 1048576).HorizontalAlignment = xlCenter

There are 7 instructions on that line of code; there's no reason to do this. Ever.

And here's where I'd think your bottleneck is:

wsReport.Cells.Copy

You're copying the entire worksheet.

wsOutput.Range("A1:AB" & 1048576)

Why bother with the concatenation here? It's hard-coded anyway, and besides, A1 has its row specified inside the string literal.

wsOutput.Range("A1:AB1048576")

This might work a bit better - if not, it's at least certainly much easier to read:

wsReport.UsedRange.Copy
wsOutput.Range("A1").Paste
wsOutput.UsedRange.Columns.EntireColumn.AutoFit
wsOutput.UsedRange.HoriontalAlignment = xlCenter


Detecting a certain amount of violet in an image - follow-up I'm trying to write a program to detect the colour violet in an image.As discussed in my previous Post @Johnbot suggested that I try using HSL / HSV to determine if the pixel is some shade of violet.

this.button2.Click += async (sender, e) =>
{
    Task task = Task.Run(() =>
    {
      ProcessImage(pictureBox1.Image);
    });
      Task.WaitAll(task);
      await task;
    };
}

    private void ProcessImage(Image image)
    {
        if (image == null)
        {
            return;
        }
        using (var selectedImage = new Bitmap(image))
        {
            double total = 0;
            double infected = 0;

            Color violet = Color.FromKnownColor(KnownColor.Violet);
            Color selectedPixel;

            for (int x = 0; x < selectedImage.Width; x++)
            {
                for (int y = 0; y < selectedImage.Height; y++)
                {
                    selectedPixel = selectedImage.GetPixel(x, y);

                    if (selectedPixel.GetHue() == (violet.GetHue()))
                    {
                        infected++;
                    }
                    total++;
                }
            }
            //TODO: Needs to be rounded
            //Change the value eg: 0.0030, 0.0010, 0.0050
            try
            {
                double percentage = (infected / total);
                Console.WriteLine(percentage);

                double result;
                if (Double.TryParse(Convert.ToString(percentage).Substring(0, 6), out result))
                {
                    if (result > 0.0017)
                    {
                        MessageBox.Show(string.Format("Image is infected: {0:P}", percentage));
                    }
                    else
                    {
                        MessageBox.Show("Threshold not reached");
                    }
                }
            }
            catch
            {
                MessageBox.Show("No Violet Pixels Whatsoever were found");
            }
            finally
            {
                selectedImage.Dispose();
            }
        }
    }

Here are two of the images I'm working with.
The first one has the colour violet in it quite a lot with a percentage of 0.64% or 0.00636199367565126The second one from what I can tell doesn't have any violet in it, however the application believes it does with 0.36% violet pixels or 0.00355871886120996.The code above is working so to speak, however is there something I could be doing to improve it for reliability and performance? I did see a post about Bitmap locking however because its only going to be scanning a image every hour or so for any violet pixels, it didn't implement it, should i still do it?

this.button2.Click += async (sender, e) =>
{
    Task task = Task.Run(() =>
    {
      ProcessImage(pictureBox1.Image);
    });
      Task.WaitAll(task);
      await task;
    };
}

Is the indentation like that in the original, or did it get corrupted by the tab-to-space conversion of StackExchange?

What's going on with the task? I don't know much about async event handlers, but as far as general async/await usage you only need one of Task.WaitAll(task) or await task.

What stops me from mashing the button and spinning off loads of tasks? Normally you would want to prevent a second click by opening a modal dialog (denying access to the button) or disabling the button, and then using ContinueWith to undo the block in the GUI thread when the task finishes.

    private void ProcessImage(Image image)
    {
        if (image == null)
        {
            return;
        }
        using (var selectedImage = new Bitmap(image))

If the image is already a bitmap, this is unnecessary and slow. It's probably worth having two cases and factoring out the code which works with Bitmap to a separate method:

    if (image is Bitmap) Process(image as Bitmap);
    else using (var bitmap = new Bitmap(image)) Process(bitmap);

            double total = 0;
            double infected = 0;

That's an odd choice for counters. Why not use an integer type and coerce to double when dividing?

            for (int x = 0; x < selectedImage.Width; x++)
            {
                for (int y = 0; y < selectedImage.Height; y++)
                {
                    selectedPixel = selectedImage.GetPixel(x, y);

This is slow. A loop over GetPixel is always slow, and it's worse when the outer loop is over x because you lose cache locality. If you care about speed, use LockBits and switch the loops.

                    if (selectedPixel.GetHue() == (violet.GetHue()))

GetHue() returns a float. == comparisons on non-integer numeric types are generally inadvisable. You should almost certainly test whether selectedPixel.GetHue() is within a small distance of violet.GetHue(). How wide a range is a parameter which you may need to tune to get the results you expect.

                if (Double.TryParse(Convert.ToString(percentage).Substring(0, 6), out result))

??! Why? And if the specs require you to round to an exact number of decimal places and that's non-negotiable, why use double instead of decimal?

            catch
            {
                MessageBox.Show("No Violet Pixels Whatsoever were found");
            }

Under what circumstances can this catch anything? The only one I can see is if total == 0, in which case the error message should really be that the image was empty.

            finally
            {
                selectedImage.Dispose();
            }

This is unncessary: it's already in a using statement.


C++ Split string into a vector While profiling parts of my code, I noticed that my Split( ... ) function is rather slow, (about 50% of the time).
I was wondering if there was a more efficient way of Splitting a string into a vector.

My subject string can be anything between 2 words to well over 1000 words, hence the reason why I would like to speed it up.

Here is an example program with the function.

#include <iostream>
#include <string>
#include <cstring>
#include <vector>

void Split(const std::string& subject, std::vector<std::string>& container)
{
  container.clear();
  size_t len = subject.length() + 1;
  char* s = new char[ len ];
  memset(s, 0, len*sizeof(char));
  memcpy(s, subject.c_str(), (len - 1)*sizeof(char));
  for (char *p = strtok(s, " "); p != NULL; p = strtok(NULL, " "))
  {
    container.push_back( p );
  }
  delete[] s;
}

int main()
{
  std::vector<std::string> container;
  Split( "Hello World", container );

  for( std::vector<std::string>::const_iterator it = container.begin(); it != container.end();++it)
  {
    std::cout << *it << "!\n";
  }
}

Any suggestions on how I could speed it up.

Of course any other general comments are more than welcome.

Edit profiling is very simple.

...
clock_t t = clock ();
std::vector<std::string> container;
Split( "actus non facit reum nisi mens sit rea", container );
t = clock() - t;
std::cout << (((float)t)/CLOCKS_PER_SEC) << "ms\n";
...

I haven't attempted to measure the timing of this, in part because I don't know how you're planning to use it, but here are some ideas for improving your code.

Consider returning something useful from functions

Your Split function doesn't return anything, which isn't an error, but it's an odd design.  I'd expect it to be declared like this instead:

std::vector<std::string> Split(const std::string& subject)

Understand sizeof

The sizeof operator is defined as always returning 1 for sizeof(char), so having that expression as part of your code probably isn't useful.

Use "range for" to simplify your code

The code to print the vector could be made much simpler and shorter by using a "range for" that was introduced in C++11:

for(const auto &item : container) {
    std::cout << item << "!\n";
}

Use standard library algorithms

I'd be inclined to write this using C++ rather than C functions.  One way to do this is to use std::copy and std::stringstream:

std::vector<std::string> Split(const std::string& subject)
{
    std::istringstream ss{subject};
    using StrIt = std::istream_iterator<std::string>;
    std::vector<std::string> container{StrIt{ss}, StrIt{}};
    return container;
}

I've also been playing with std::sregex_token_iterator lately.  Here's how that might look:

std::vector<std::string> Split(const std::string& subject) {
    static const std::regex re{"\\s+"};
    std::vector<std::string> container{
        std::sregex_token_iterator(subject.begin(), subject.end(), re, -1),
        std::sregex_token_iterator()
    };
    return container;
}


Make a given number by adding given numbers I have created a code that checks if a number can be made by adding different quantities of given numbers and if so returns the quantities. Is this the best way to do this or is there a more efficient way?

Here is my function:

def sumFinder(quantities, final_num):
    if len(quantities) == 1:
        for i in range(final_num//quantities[0]+1):
            num = quantities[0] * i
            if num == final_num:
                return [i]
        return False
    for i in range(final_num//quantities[0]+1):
        num = quantities[0]*i
        found_nums = sumFinder(quantities[1:], final_num-num)
        if found_nums:
            return [i] + found_nums
    return False

This is an example of it working:

for num in range(100):
    answer = sumFinder([4, 6, 9, 20], num)
    if answer:
        a, b, c, d = answer
        answer = "4({}) + 6({}) + 9({}) + 20({})".format(a, b, c, d)
    print(num, "=", answer)

Avoid repeated values

In your testing code, the values 4, 6, 9, 20 appear in multiple places. You could re-write your code to generate a formatted answer from the list:

num_list = [4, 6, 9, 20]
for num in range(100):
    answer = sumFinder(num_list, num)
    if answer:
        answer = " + ".join("{} * {}".format(a, b) for a, b in zip(num_list, answer))
    print(num, "=", answer)

Document your function and use the correct type

Your function returns both lists and boolean. Maybe it's be clearer to make it return a list (when a list is found) and None otherwise. This can be documented accordingly in a docstring.

def sumFinder(quantities, final_num):
    """Return a list such that dot-product(list, quantities) == final_num. Return None when no list is found."""
    if len(quantities) == 1:
        for i in range(final_num//quantities[0]+1):
            num = quantities[0] * i
            if num == final_num:
                return [i]
        return None
    for i in range(final_num//quantities[0]+1):
        num = quantities[0]*i
        found_nums = sumFinder(quantities[1:], final_num-num)
        if found_nums:
            return [i] + found_nums
    return None

Handle empty input

You access the first element of the input without checking the length first. Maybe it'd be clearer to document this as an invalid input or you could handle it properly by returning [] if final_sum == 0 else None.

Removing duplicated logic

When the empty case is handled, the case with 1 element doesn't need to be handled in a special way.

With a few minor changes, you can write:

def sumFinder(quantities, final_num):
    """Return a list such that dot-product(list, quantities) == final_num. Return None when no list is found."""
    if not quantities:
        return [] if final_num == 0 else None
    first, remain = quantities[0], quantities[1:]
    for i in range(final_num//first+1):
        found_nums = sumFinder(remain, final_num - first * i)
        if found_nums is not None:
            return [i] + found_nums
    return None

Names

Python names are usually snake_cased. Also, find_sum would be a better name that sum_finder which sounds like a class name.

final_num could be renamed target, n, etc.

quantities could be renamed lst


Random Number Generator game #include <iostream>
#include <cstdlib>
#include <ctime>
#include <string>
using namespace std;
void wrapUp() {
    cout << "-------------------------------------------------" << endl;
    system("CLS");

}

int main() {
    srand(time(0));
    string name;
    int player;
    int livesTracker = 5;
    int x = 1 + (rand() % 10);

    cout << "Hello, what is your name?" << endl;
    cin >> name;
    cout << "Welcome to guess the number, " << name << "!" << endl;
    wrapUp;
    cout << '\n' << "Number of Lives: " << livesTracker << endl;

    for (int round = 1; round <= 3; round++) {

        if (livesTracker == 0) {
            system("CLS");
            cout << "Game Over! Actual Answer: " << x << endl;
            system("Color 4");
            break;
        }

        cout << "Guess the number that will come up on the screen from 1 to 10" << endl;
        cin >> player;

        if (player == x) {
            cout << "You won round " << round << "!" << endl;
            x = (rand() & 10);
        }
        else if(player > x){

            cout << "Your guess was too high, round" << round << "!" << endl;
            wrapUp;
            round--;
            livesTracker--;
            cout << '\n' << "Number of Lives: " << livesTracker << endl;
        }
        else {
            cout << "Your guess was too low, round" << round << "!" << endl;
            wrapUp;
            round--;
            livesTracker--;
            cout << '\n' << "Number of Lives: " << livesTracker << endl;
        }

        if (round == 3) {

            cout << "For the hard level, you have to guess a number from 15!" << endl;
            for (int roundh = 1; roundh <= 2; roundh++) {

                int xh = 1 + (rand() % 15);

                    cout << "Guess the number that will come up on the screen from 1 to 15" << endl;
                    cin >> player;

                if (player == xh) {

                    cout << "You won round " << roundh << "!" << endl;
                    cout << "-------------------------------------------------" << endl;
                    xh = 1 + (rand() % 15);
                }
                else if (player > xh) {

                    cout << "Your guess was too high, round " << round << "!" << endl;
                    wrapUp;
                    roundh--;
                    cout << '\n' << "Number of Lives: " << livesTracker << endl;
                }
                else {
                    cout << "Your guess was too low, round " << round << "!" << endl;
                    wrapUp;
                    roundh--;
                    cout << '\n' << "Number of Lives: " << livesTracker << endl;
                }

            }

        }
    }

    system("pause");

}

This program first creates a random number before going into the loop. A livesTracker variable is then created to control the player's lives. The first loop runs until the player finishes the first round/ level. If the player gets the randomly generated number correct, a new random number will be assigned to x for the next round to start, until the player runs out of lives or wins the game, this will happen.

Just wanted to explain the program because its probably too messy to read, appreciate any help!

EDIT: I know its not good to clear the screen using system("CLS"), but couldn't find any other options, any help on that would be helpful as well!

I have some suggestions to improve your code:

Formatting

You should use empty lines consistently. E.g. you have some ifs/else ifs blocks where you put an empty line at the beginning but not everywhere. I suggest you to not use empty lines there at all.
You should not use more than two empty lines to seperate things. It's harder to read.
You should put empty lines between logically grouped things. E.g. your #include statements, the using statement and the wrapUp function are glued together.
In general try to group things visually that belong together. In the wrapUp function the two empty lines are seperating the ending curly bracket from the function for no reason.
You should not put a newline as first thing you print in a line, e.g. cout << '\n' << "Number of Lives: " << livesTracker << endl;. And you should not mixup '\n' and endl.

Naming

You should name variables more precisely. player should be named playerGuess. This will make it much clearer what the variable is for. You may ask why this is relevant because you know it already but be aware of other developers. Just by putting that Guess at the end will remove the amount of time someone has to put in finding out stuff. Some other variables could use better naming as well, like roundh or livesTracker.
I think wrapUp does not describe well enough what it's doing. (That one is kinda personal taste.)

Functional

It's strange to put the game logic in a for loop and decrement the iterator to continue with the game. I know, I know, it works, but this is not the usual way to use a for loop. A while loop does fit here much better. When you connect the while loop to the amount of lifes left, you will have a much more readable code, e.g. while (amountOfLifes > 0) {// continue with the game}.
The game logic for the normal game and the hard mode is very similar. You can put it in a method and use parameters, e.g. to individualize the range for the random number to guess.

Here is an example of how the program could look like when you apply my suggestions. It doesn't need to be exactly like that. Please note that I didn't double check everything and I changed some of the logic but I hope you get an overall idea about what is improvable:

#include <iostream>
#include <cstdlib>
#include <ctime>
#include <string>

using namespace std;

void printSeperator() {
    cout << "-------------------------------------------------" << endl;
}

bool guess(int& lifesCounter, int numberToGuess, int round, int upperRandomLimit) {
    cout << "Guess the number that will come up on the screen from 1 to " << upperRandomLimit << ": ";
    int playerGuess;
    cin >> playerGuess;

    if (playerGuess == numberToGuess) {
        cout << "You won round " << round << "!" << endl;
        printSeperator();
        return true;
    }
    else if (playerGuess > numberToGuess) {
        cout << "Your guess was too high, round " << round << "!" << endl;
        printSeperator();
        lifesCounter--;
        cout << "Number of Lives: " << lifesCounter << endl;
    }
    else {
        cout << "Your guess was too low, round " << round << "!" << endl;
        printSeperator();
        lifesCounter--;
        cout << "Number of Lives: " << lifesCounter << endl;
    }
    return false;
}

bool playRounds(int& lifesCounter, int upperRandomLimit) {
    int numberToGuess;
    for (int round = 1; round < 3; round++) {
        numberToGuess = 1 + (rand() % upperRandomLimit);
        while (!guess(lifesCounter, numberToGuess, round, upperRandomLimit)) {
            if (lifesCounter == 0) {
                cout << "Game Over! Actual Answer: " << numberToGuess << endl;
                return false;
            }
        }
    }
    return true;
}

int main() {
    srand(time(0));
    string playerNamer;
    int lifesCounter = 5;

    cout << "Hello, what is your name? ";
    cin >> playerNamer;
    cout << "Welcome to guess the number, " << playerNamer << "!" << endl;
    printSeperator();
    cout << "Number of lifes you have: " << lifesCounter << endl;

    if (!playRounds(lifesCounter, 10)) {
        return 0;
    }
    lifesCounter += 5;
    cout << "You got more lifes! Now you have: " << lifesCounter << endl;
    cout << "For the hard level, you have to guess a number from 15!" << endl;
    playRounds(lifesCounter, 15);
    return 0;
}

P.S.: It's very hard to win when you have just 5 lifes for all 4 guesses. ¯\_(ツ)_/¯


Search every char from a string for every char in another I am adding a feature to a program that will let the user add new dictionaries as a file to the file folder to be used as a note taking repository. I just got the code finished for creating the file name from user input, however it feels as if there should be a smarter way to perform the task than what I have done.

The following is what I have come up with that does work but I don't know if I should be doing this in a shorter method with either a built in function or a smarter one-liner.

Can you explain any ways that you think this could be improved? I have searched for other solutions but they were not well explained enough that I could understand them very clearly.

EDIT: Added my imports to code. Note: the print statement at the bottom was just to test the result of the code and see it in console. It will be changed to return when I finish up the rest of the program.

from tkinter import *
import sys
import time
import tkinter.messagebox
import tkinter.simpledialog
import json

finalFilename =''
def new_lib_prompt(event=None):
    finalFilename =''
    name_of_new_library = tkinter.simpledialog.askstring('Create New Note Library', 'Alphanumeric lowercase and "_" only', initialvalue = "Name_Here")
    from string import ascii_letters,digits
    validFilenameChars = "-_.() %s%s" %(ascii_letters, digits)
    stringofnewname = str(name_of_new_library)
    stringofvalid = str(validFilenameChars)
    for i in stringofnewname:
        if i in stringofvalid:
            if i == ' ':
                new_i = '_'
                finalFilename += ''.join(new_i)
            else:
                lower_i = i.lower()
                finalFilename += ''.join(lower_i)
        else:
            pass
    print (finalFilename)

EDIT: 04/21/2017 2:30 PM

Thanks for all the great feedback everyone and thanks to holroy for his very useful and thought out response. If anyone is interested here is the solution I was looking for after taking advice from a few answers.

valid_filename = '' # will be using this in another function later
def new_lib_prompt(event=None):
    global valid_filename
    a_name = tkinter.simpledialog.askstring('Create New Note Library',
                                            'Alphanumeric lowercase and "_" only',
                                            initialvalue = "Name_Here")
    VALID_CHARS = '-_.() {}{}'.format(ascii_letters, digits)
    valid_filename = (''.join(c for c in a_name if c in VALID_CHARS)).replace(' ', '_').lower()

First of all let's address some of the more prominent style issues of your code, before diving into an alternate approach to solving it.

Have imports at top of code – At first I didn't see the from string import ... line as it was hidden in the code, but this is not good coding practice. Put this at the top of the file, so it is easy to see.
Have more vertical space – Please add an extra line here and there, to increase readability in your code. Typically I would add spaces in front of for loops or if block to accentuate these blocks.
Be consistent in variable naming – Naming variables is hard but choose one style for variable naming, and don't mix it like name_of_new_library, validFilenameChars and stringofnewname. This make the code a whole lot harder to read, which in turns makes it harder to maintain in the long run.
Functions should return, not print their result – To have a function print its output is usually a code smell, and it would be better to have it return the result. If by some reasoning it's required to print it out, I would maybe indicate that in the function name.
Using the % operator is depreceated in favor of str.format – See format examples for the newer and better method of adding arguments into strings.
Why the event=None – I haven't used tkInter a lot, so you might need it for some reason, but it kind of looks superfluous to me as you don't use the event in the function at all.
Why the str(some_variable)? – Why do you do this? Is this related to tkInter somehow as well? Do you really need to "stringify" the return from tkInter?
Only join when needed! – The join command is mainly used to join lists, so joining a list of one character is extra work, and not needed. To append a single character you could do txt += char. See below for a better use of join.

Algorithm comments

When summarized your code does the following things:

Reads a suggestion using tkInter
Builds a constant of validFilenameChars
Converts any spaces into underscores, and
lowercases all other characters

As covered before the import should be done at top, and so should also declaration of constants. When you start validifying the name you split the string into single characters, and use a somewhat strange += ''.join(new_i) to add onto the new filename.

There does however exist functions which does both of your required operations. See str.lower() and str.replace(). Do however note that, the latter replace substrings. In your case str.replace(' ', '_') would work, but if replacing multiple chars see how to replace multiple characters in a string?

The only part not covered then is the valid characters part, and this is the slightly ugly part of the new alternative. My proposed solution is to do:

''.join(c for c in suggested_name if c in VALID_CHARS)

And this possibly requires a little explanation:

''.join(_some list_) – This expects a list of something, which it will join together by the separator in front, that is the empty string.
c ... – If the for loops is valid, then return the c
for c in suggested_name ... – Split the suggested_name into single characters
if c in VALID_CHARS – Only do the action (that is return c) from the for loop if the if expression is true. And the expression verifies that the character c is within the string VALID_CHARS.

In other words, the previous statements joins all characters from suggested_filename using the empty string separator, for those characters who are present in VALID_CHARS. That is quite a mouthful, but it is a very useful expression!

A longer version to write the same is something like (with a lot of edge cases removed):

my_list = []
for c in suggested_name:
  if c in VALID_CHARS:
    my_list.append(c)

my_separator = ''
my_txt = my_list[0]
for c in my_list[1:]:
  my_txt += my_separator
  my_txt += c

I'm also accustomed to separating how you get stuff (aka the tkInter dialog) from validation/transformation of the result, so I would make a function to handle this.

Code refactored

So what would the code look like if applying all of this? Maybe something like this (untested as I don't have tkinter available):

from string import ascii_letters, digits
VALID_CHARS = '-_.() {}{}'.format(ascii_letters, digits)

def get_valid_library_name(suggestion):
  valid_filename = ''.join(c for c in suggestion if c in VALID_CHARS)
  valid_filename = valid_filename.replace(' ', '_').lower()
  return valid_filename

a_name = tkinter.simpledialog.askstring(
                 'Create New Note Library',
                 'Alphanumeric lowercase and "_" only',
                 initialvalue = "Name_Here")

print('{} into {}'.format(a_name, get_valid_library_name(a_name))

filename = get_valid_library_name("MY someWHAT ugly suggestion#$!&&")
print(filename)    # Would output "my_somewhat_ugly_suggestion"

Note that one could join the valid_filename transformation into fewer lines, but keeping it readable is also a good thing. For real code I would most likely have the valid_filename = ''.join(..) on one line, and then do a return valid_filename.replace().lower() instead of the two next lines.

As a final note, also see Turn a string into a valid filename which I found when researching for this answer. It uses some alternate stuff from unicode, and has some other related ideas on how to handle validation of filenames.


Is this the way to use HibernateCompositeUser type for handling localized contents? I am trying to use HibernateCompositeUser type to handle i18n specific data in my application. I am trying to use the below approach.

I have a table named master table which contains all locale independent data, while I have created another table master_translation which contains all locale sensitive information. Master table contains a reference to the master_translation.

Here is the detailed table structure:

master
 ---ID
 ---master_translation
 ---Other locale independent fields.

master_translation
 ---ID
 ---language_id
 ---Other locale sensitive fields

Now I am using HibernateCompositeUser type to handle this internal process.  Here is the mapping for master class:

<hibernate-mapping>
    <class name="Master"
        table="MASTER">
        <id name="uuid" type="java.lang.String">
            <column name="UUID" />
            <generator class="uuid" />
        </id>

        <property name="master_i18n" type="masteri18n">
           <column name="NAME"/>
           <column name="SHORTDESCRIPTION"/>
           <column name="LONGDESCRIPTION"/>
    </class>
</hibernate-mapping>

where type="masteri18n" is hibernate CompositeUserType.now.

When I am fetching the master content, it is correctly fetching the data based on the language and my Master class contains a reference to the Master_translation which holds locale specific data.

I am handling this case:

public Object nullSafeGet(ResultSet rs, String[] arg1,
        SessionImplementor arg2, Object arg3) throws HibernateException,
        SQLException {

    Master_i18n master_i18n=Master_i18n.getInstance();
    master_i18n=dao.getMaster_i18n(rs.getString(arg1[0]), "nl_NL");;

    //return Master_i18n.getName();
    return Master_i18n;
}

@Override
public void nullSafeSet(PreparedStatement ps, Object arg1, int index,
            SessionImplementor arg3) throws HibernateException, SQLException {

    if(arg1==null){
        ps.setNull(index, Hibernate.STRING.sqlType());
        //ps.setNull(index+1, Hibernate.STRING.sqlType());
        //ps.setNull(index+2, Hibernate.STRING.sqlType());set
    }
    else{
         Master_i18n des=(Master_i18n)arg1;
         des=dao.saveMaster_i18n(des);
         ps.setString(index, des.getUuid());
         ps.setString(index+1, des.getUuid());
         ps.setString(index+2, des.getUuid());
         ps.setString(index+3, des.getUuid());
    }
}

Somehow I am not feeling confident about my approach.  Am I implementing it correctly or not?

I'm not too familiar with Hibernate, so just some generic notes:

DBMSs usually have case-insensitive attribute names, so a lot of people use underscore to separate words in attribute and table names.

<column name="SHORTDESCRIPTION"/>
<column name="LONGDESCRIPTION"/>

In the case above SHORT_DESCRIPTION and LONG_DESCRIPTION would be easier to read.

Is SQL syntax case sensitive?

    Master_i18n master_i18n=Master_i18n.getInstance();
    master_i18n=dao.getMaster_i18n(rs.getString(arg1[0]), "nl_NL");;

Master_i18n.getInstance() call seems unnecessary, since on the next line the local variable is overwritten. The following is the same (and the first line might not be required at all):

Master_i18n.getInstance();
Master_i18n master_i18n=dao.getMaster_i18n(rs.getString(arg1[0]), "nl_NL");;

<id name="uuid" type="java.lang.String">

java.lang.String could be simply string here.

public Object nullSafeGet(ResultSet rs, String[] arg1,
        SessionImplementor arg2, Object arg3) throws HibernateException,
        SQLException {

    Master_i18n master_i18n=Master_i18n.getInstance();
    master_i18n=dao.getMaster_i18n(rs.getString(arg1[0]), "nl_NL");;

    //return Master_i18n.getName();
    return Master_i18n;
}

I think currently it does not compile, since return Master_i18n is uppercased Master_i18n which seems a class, not a local variable.

master_i18n=dao.getMaster_i18n(rs.getString(arg1[0]), "nl_NL");;

nl_NL could be a parameter/constant. (Does every caller want to use this locale?) (One of the semicolons is unnecessary at the end of the line.)
I'd use longer and more descriptive variable names than ps, rs, arg1, arg3. Longer names would make the code more readable since readers don't have to decode the abbreviations every time when they read the code and don't have to guess which abbreviation the author uses.

(Clean Code by Robert C. Martin, Avoid Mental Mapping, p25)
Master_i18n is rather an ugly class name. It does not follow the usual CamelCase naming convention.
It also looks like a singleton which is rather an antipattern nowadays. They make testing harder and often hide dependencies which leads to spaghetti code which is really hard to work with.

What is so bad about singletons?
TotT: Using Dependency Injection to Avoid Singletons
Eliminating static helper classes
The Singleton Pattern


Random missile generator and air defense system My task in C#:

Random missile generator generating 2 types of missiles each second: ThermoNuclearBomb and FacetBomb

Missiles have properties Serial Number, Type

Air Defense System, that is identifying the 2 types of missiles and neutralizes each of them with a different gun (AntiThermoGun and AntiFacetGun)

Air Defense system has a method AcceptMissile that accepts a Missile as a parameter
Every Gun has a method Neutralize that accepts an appropriate Missile as a parameter and neutralizes only such types of missiles.
The Air Defense system must decide which missile will be neutralized with which gun.

I've got two interfaces:

public interface IMissile
    {
         int SerialNumber { get; set; }
         string Type { get; set; }

    }

public interface IGun
    {
         string Name { get; set; }
         void Neutrilize(IMissile missile);
    }

Of course FacetBomb and ThermoNuclearBomb class implement this interface IMissile:

public class FacetBomb : IMissile
    {
        public int SerialNumber { get; set; }
        private string type;
        public string Type { get; set; }
    }

    public class ThermoNuclearBomb:IMissile
    {
        public int SerialNumber { get; set; }
        public string Type { get ; set ;  }

    }

Here are my two classes AntiThermoGun and AntiFacetGun that implement IGun:

public class AntiThermoGun  : IGun
    {
        private string name;
        public string Name
        {   get {return name;}
            set { name = "AntiThermoGun"; }
        }
        public  void Neutrilize(ThermoNuclear gun)
        {
            Console.WriteLine(this.Name + "neutrilizes" + gun.Type);

        }
    }

   public class AntiFacetGun : IGun
    {
        private string name;
        public string Name
        {
            get { return name; }
            set { name = "AntiFacetGun"; }
        }
        public void Neutrilize(FacetBomb missile)
        {
            Console.WriteLine(this.Name + "neutrilizes" + missile.Type);
        }
    }

Here is my AirDefence class which accepts a missile and decides which missile will be neutralized with which gun:

public class AirDefenceSystem
    {

        public void InterceptMissile(IMissile missile)
        {
            IGun igun = null;

            switch (missile.Type)
            {
                case "ThermoNuclear":

                    igun = new AntiThermoGun();
                    break;

                case "FacetBomb":
                    igun = new AntiFacetGun();
                    break;
            }
            igun.Neutrilize(missile);
        }
    }

In my program.cs:

 static void Main(string[] args)
        {
            FacetBomb facetBomb = new FacetBomb { SerialNumber = 100, Type = "FacetBomb" };
            FacetBomb facetBomb2 = new FacetBomb { SerialNumber = 200, Type = "FacetBomb" };
            ThermoNuclear thermoBomb = new ThermoNuclear { SerialNumber = 500, Type = "ThermoNuclear" };

            IMissile[] objects = new IMissile[] { facetBomb, facetBomb2, thermoBomb };
            AirDefenceSystem system = new AirDefenceSystem();
            foreach (var item in objects)
            {
                system.InterceptMissile(item);

            }

        }

Consider var

Using the var keyword when the right-hand-side of an assignment makes the type obvious can shorten your code and make changing types much easier.

The missile type is unused.

You use Type, but not type. Remove that.

Use constructors

For your two missile types, the properties don't change after they've been set, so make them getter-only properties and set them in the missile constructors.

public interface IMissile
{
     int SerialNumber { get; }
     string Type { get; }
}

public class FacetBomb : IMissile
{
    public int SerialNumber { get; private set; }
    public string Type { get; private set; }

    public FacetBomb(string type, int serialNumber)
    {
        Type = type;
        SerialNumber = serialNumber;
    }
}

Avoid using strings for type information

This is what types are for, so remove the Type property entirely. When you need to check a type, you can use type checks.

public void InterceptMissile(IMissile missile)
    {
        IGun igun = null;

        if(missile is ThermoNuclearBomb)
        {
           igun = new AntiThermoGun();
        }
        else if(missile is FacetBomb)
        {
           igun = new AntiFacetGun();
        }

        igun.Neutrilize(missile);
    }

It's "Neutralize" or "Neutralise"

Naming

Your interface is IMissile but your concrete types all end in Bomb. You have a variable called igun which belies its type. If you change the type you'd then have to go around changing the name of the variable too. Consider instead a variable name that specifies its purpose, not its type.

What is the serial number for?

It is never used, as far as I can tell.

Use a getter to return a specific value, not a setter

This doesn't make much semantic sense.

    private string name;
    public string Name
    {
        get {return name;}
        set { name = "AntiThermoGun"; }
    }

If I call .Name = "foo" .Name will next return "AntiThermoGun". Instead, stop me from setting .Name at all:

    public string Name
    {
        get {return "AntiThermoGun";}
    }


Parallel Job Consumer Edit: A second iteration of this problem here.

I need to provide a service (either a Windows Service or an Azure Worker Role) which will handle the parallel execution of jobs.  These jobs could be anything from importing data to compiling of reports, or to sending out mass notifications, etc.  They are generally long-running and are not suitable to run on the web server.

Important factors

The solution needs to be a very extensible and maintainable solution, as new job implementations will be created by developers all the time.
The execution of these jobs is to be abstracted away from the client code.  All developers need to do is to create the new job and add it to the job data source (database, MSMQ, Azure Queue, etc).

Solution

Deriving from AbstractJob and overriding the abstract Execute() method to provide implementation for the new job:

public abstract class AbstractJob
{
    public JobState State { get; protected set; }

    public AbstractJob()
    {
        State = JobState.New;
    }

    protected abstract bool Execute();

    public sealed void ExecuteJob()
    {
        State = JobState.Executing;

        try
        {
            if (Execute())
            {
                State = JobState.Successful;
            }
            else
            {
                State = JobState.Failed;
            }
        }
        catch (Exception)
        {
            State = JobState.FailedOnException;
        }
    }
}

public class TestJob : AbstractJob
{
    protected override bool Execute()
    {
        Debug.WriteLine("Test job!");
        return true;
    }
}

The following JobConsumer would reside on a Windows Service or Azure Worker Role.  IJobSource provides the consumer with jobs.  These could be from the database, a MSMQ, an Azure Queue, etc (I have left this implementation out).

Every 5 seconds (this can obviously be configurable), the JobConsumer executes new jobs when there are available cores.

public class JobConsumer
{
    public List<AbstractJob> Jobs { get; set; }

    public IJobSource JobSource { get; private set; }

    public int ParallelExecutionCount { get; set; }

    protected List<Task> Tasks { get; set; }

    public JobConsumer(IJobSource jobSource, int parallelExecutionCount)
    {
        JobSource = jobSource;
        ParallelExecutionCount = parallelExecutionCount;
    }

    public void StartConsumer()
    {
        Task.Factory.StartNew(() =>
        {
            TaskScheduler currentScheduler = TaskScheduler.Current;

            while (true)
            {
                int availableCores = ParallelExecutionCount - Tasks.Count();

                IEnumerable<AbstractJob> newJobs =
                    JobSource.ReadNewJobs().Take(availableCores);

                foreach (var job in newJobs)
                {
                    System.Threading.Tasks.Task task = null;

                    task = Task.Factory.StartNew(() =>
                    {
                        Tasks.Add(task);
                    }, CancellationToken.None,
                       TaskCreationOptions.None,
                       currentScheduler)
                    .ContinueWith(t =>
                    {
                        // Execute job on a new thread
                        job.ExecuteJob();
                    })
                    .ContinueWith(t =>
                    {
                        Tasks.Remove(task);
                    }, currentScheduler);
                }

                Thread.Sleep(5000);
            }
        });
    }
}

public interface IJobSource
{
    public IEnumerable<AbstractJob> ReadNewJobs();
}

Input, critique and suggestions very welcome!

EDIT:  I am using .NET 4.5

The way you're using TaskScheduler.Current to try to synchronize access to Tasks won't work (unless this code runs under a very specific custom TaskScheduler).

With the default scheduler, a Task can be executed on any ThreadPool thread, which means that multiple threads could access Tasks at the same time. The simplest fix for that is to use lock instead of trying to use TaskScheduler (which will also simplify your code a bit).

Since you seem to be using Tasks just for counting, another option would be to replace it with int field and then use Interlocked.Increment()/Decrement() to modify it.
I think you should save the exception that's caught in ExecuteJob() somewhere, so that when you encounter an issue, you can actually diagnose it.

Also, it looks like you're not saving the results of jobs anywhere. I think that's a bad idea, you should have some sort of logging, to see where the problem was when something doesn't work right. (Was the job even scheduled? Did it complete successfully? Etc.)

And I'm not sure you want to differentiate between "failed" and "failed on exception". When something fails, you usually want to know why it failed, which is what exceptions are for.
Why are most of the properties of JobConsumer public? If you want to allow adding a new job to Jobs, but nothing else, then create a method for that.

And why is Tasks protected, when you don't have any virtual methods? The default accessibility should be private (which means you don't need a property anymore and you can use just a field) and you should change that only when you have good reason.
The way you use ReadNewJobs() seems to indicate that items are actually consumed only when the enumerable is iterated. That's not a bad idea (BlockingCollection.GetConsumingEnumerable() does the same thing), but it should be documented very clearly that that's what is expected from the method.
Since you're on .Net 4.5, you can use await Task.Delay() instead of Thread.Sleep(). This means you would need to make the lambda async and you should probably also use Task.Run() instead of Task.Factory.StartNew() (which is a good idea anyway, because it's shorter).
There is no need to spell the name System.Threading.Tasks.Task in full, use just Task.
I don't think that the name AbstractJob needs to emphasize that the class is abstract, calling it just Job would work just as well. Another possible name is JobBase.


Parse and format date I have the following code for taking a date in the form of a string yyyy-MM-dd HH:mm:ss (UTC timezone) and turning it into EEEE d(st, nd, rd, th) MMMM yyyy HH:mm (devices default timezone).

However, my problem with the way I have done it is the code looks messy and inefficient. Is there a way to achieve what I want without formating and parsing the same date so many times to make it more efficient? Or any other improvements?

Preferably supporting Android API level 14.

String inputExample = "2017-06-28 22:44:55";

//Converts UTC to Device Default (Local)
private String convertUTC(String dateStr) {
    try {
        SimpleDateFormat df = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
        df.setTimeZone(TimeZone.getTimeZone("UTC"));
        Date temp = df.parse(dateStr);
        df.setTimeZone(TimeZone.getDefault());
        String local = df.format(temp);
        Date localDate = df.parse(dateStr);
        SimpleDateFormat outputDF1 = new SimpleDateFormat("EEEE ");
        SimpleDateFormat outputDF2 = new SimpleDateFormat(" MMMM yyyy HH:mm");
        return outputDF1.format(temp) + prefix(local) + outputDF2.format(temp);
    } catch(java.text.ParseException pE) {
        Log.e("", "Parse Exception", pE);
        return null;
    }
}

private String prefix(String dateStr) {
    try {
        SimpleDateFormat outputDF = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
        Date temp = outputDF.parse(dateStr);
        SimpleDateFormat df = new SimpleDateFormat("d");
        int d = Integer.parseInt(df.format(temp));
        if(1 <= d && d <= 31) {
            if(11 <= d && d <= 13)
                return d + "th";
            switch (d % 10) {
                case 1: return d + "st";
                case 2: return d + "nd";
                case 3: return d + "rd";
                default: return d + "th";
            }
        }
        Log.e("", "Null Date");
        return null;
    } catch(java.text.ParseException pE) {
        Log.e("", "Parse Exception", pE);
        return null;
    }
}

Parsing twice

Instead of parsing dateStr twice, consider passing the Date object you have already constructed to the local time zone into prefix(String).

Calendar vs Date

A Calendar object has arguably better semantics than a Date, so it may be easier to access the individual chronological fields (actually, in your case, just the day component).

Lengthy try-catch block

Your try-catch block in convertUTC(String) spans the entirety of the method, but it's generally recommended to keep them short and to-the-point so that you effectively get a fail-fast mechanism. It's also easier to understand what's the exact code that might plausibly throw the checked Exception, so that it can be easier to refactor in the future.


Handling nil: Trying to avoid #try Yesterday, I introduced a bug into our codebase by calling #titleize on a string that could possibly come in as nil:

def route
  medication.route_name.titleize
end

This threw an error on production and a coworker fixed this by adding try:

def route
  medication.route_name.try(:titleize)
end

I'm not totally happy with this solution, after being really influenced by Avdi's article on #try, and I've been trying to come up with something I like better. Here are two possibilities.

Firstly, in Avdi's talk "Confident Code", he talks about asking for the data type you need: if you need a string, ask for a string. So I thought about this:

def route
  medication.route_name.to_s.titleize
end

This way, nil will be converted to empty string, and #titleize will be called, which will not error. The obvious issue with this is that you're calling #to_s on something which is only ever an instance of String or NilClass.

The only other approach I could think of was this:

def route
  medication.route_name.present? medication.route_name.titleize : ''
end

The issue here is the if/else parading around as prettier code through the use of a ternary operator, and the repetition of medication.route_name. This could be minified by delegating route_name to medication, or some other work around. But there's still the if/else conditional. And that's more code.

The bigger question here is how to handle nil values. This method is a pretty typical example of how this comes up. How would you handle it?

I introduced a bug into our codebase by calling #titleize on a string that could possibly come in as nil

When a problem is so pervasive, it's usually a sign that there is a conceptual problem with the language itself, some call it void safety, also dubbed "the billion-dollar mistake". Some languages, specially the functional ones, use an Option type. Let's see the options in Ruby:

medication.route_name.try(:titleize). This is probably the most idiomatic approach in Rails. You say you're not totally happy, yes, it's ugly that a method name ended up being a symbol. Note, however, that if you want an empty string as fallback, you should write medication.route_name.try(:titleize) || "".
Very similar to try, I prefer the proxy approach of andand/maybe: medication.route_name.maybe.titleize || "".
medication.route_name.to_s.titleize. IMO, that's dubious. You are calling to_s to a nil object. Yes, it happens to return "", but it could as well return "nil" (see for example Python: str(None) #=> 'None'). Too implicit.
medication.route_name.present? medication.route_name.titleize : ''. You are protecting yourself against nil objects, not empty strings, so there is no need to use present?: medication.route_name ? medication.route_name.titleize : ''. As you say, this is simple but verbose. Also, imagine you have a chain of  nullable values, it becomes a real mess. That's why andand and similar libraries were created.

By the way, I've wrote about this subject at RubyIdioms.


Making modern (RAII) C++-code with Qt5 I am fully aware that Qt is quite good with handling ownership in its object trees. The usual pattern seems to be to use a raw new, assigning the result to a raw pointer and then giving up the ownership with some widget->add(...) call.

This does not seem C++11/C++14, or modern, or RAII-style to me -- especially not in the light of the new C++ Core guidelines and the Guidelines Support library (GSL).

I started using unique_ptr to be on the safe side during set-up. The widget->add(...) then  gets the unique_ptr::release()-result. So if anything goes awry it is still sure there is no leak. But because after release() the content of the unique_ptr is nullptr I stored its content non-owning (an "alias") in a separate variable for later use (like conntect and other method calls).

That looks quite ok to me in simple cases. But when I designed my first own Widget class I had a lot of boiler-plate code around all this unique_ptr/non-owning-alias.

Here is my header file:

// 005-my-window/my_window.hpp
#ifndef MY_WINDOW_HPP
#define MY_WINDOW_HPP
#include <QWidget>
#include <QVBoxLayout>
#include <QPushButton>
#include <QLabel>
#include <QObject>
class MyWindow : public QWidget {
    Q_OBJECT
public:
    MyWindow(QWidget* parent = nullptr);
private: // nicht selbst Besitzer dieser QWidgets:
    QLabel* label_ = nullptr;
    QPushButton* button0_ = nullptr;
    QPushButton* button1_ = nullptr;
    QVBoxLayout* layout_ = nullptr;
    int cnt_ = 0; // zählt die Klicks mit
private slots:
    void setText();
};
#endif // MY_WINDOW_HPP

And here the implementation:

// 005-my-window/my_window.cpp
#include "my_window.hpp"
#include <QApplication>
#include <memory>
MyWindow::MyWindow(QWidget *parent)
    : QWidget{parent}
{
    auto label = std::make_unique<QLabel>("old text");
    label_ = label.get();
    auto button0 = std::make_unique<QPushButton>("Increment");
    button0_ = button0.get();
    auto button1 = std::make_unique<QPushButton>("Quit");
    button1_ = button1.get();
    auto layout = std::make_unique<QVBoxLayout>(this);
    layout_ = layout.get();
    layout->addWidget(button0.release());
    layout->addWidget(button1.release());
    layout->addWidget(label.release());
    setLayout(layout.release());
    connect( button0_, SIGNAL( clicked() ), this, SLOT( setText() ) );
    connect( button1_, SIGNAL( clicked() ), qApp, SLOT( quit() ) );
}
void MyWindow::setText() {
    QString msg{"Label updated "};
    msg.append(QString{}.setNum(++cnt_));
    msg.append(" times");
    label_->setText(msg);
}

Well, I need the non-owning raw pointer members to the QWidgets for later use, obviously. But in the constructor I use my safe RAII unique_ptr-machinery to initialize them. Disadvantages:

quite some boiler-plate per item for initialization
initialization not done in c'tors init-section
members are first ini'd with nullptr and later re-assigned with actual values.

Therefore I am only 70% satisfied with this code.

Is there a more comprehensive way to init Qt5 Widgets in a safe, modern RAII manner in C++14?

When in Qt, act like Qt.

Qt already has an ownership model, and it works by passing the parent object (as a raw pointer) in the constructor (so there's no window of exception-triggered leakage).  And Qt has its own weak pointers (including QObjectPointer).

I wouldn't try to outsmart Qt on pointer lifetime and ownership - but keep watching development, and if a future version of the library starts accepting smart pointers to transfer ownership, then reach out and grab it at that time!

In my personal opinion, the code you show is less clear than the Qt-native equivalent:

#include <QWidget>
#include <QVBoxLayout>
#include <QPushButton>
#include <QLabel>
#include <QObject>

class MyWindow : public QWidget {
    Q_OBJECT
public:
    MyWindow(QWidget* parent = nullptr);
private:
    QLabel *const label;
    QPushButton *const button0;
    QPushButton *const button1;
    QVBoxLayout *const layout;
    int count = 0;
private slots:
    void setText();
};

#include <QApplication>
MyWindow::MyWindow(QWidget *parent)
    : QWidget{parent},
      label{new QLabel{"old text", this}},
      button0{new QPushButton{"Increment", this}},
      button1{new QPushButton{"Quit", this}},
      layout{new QVBoxLayout{this}}
{
    layout->addWidget(button0);
    layout->addWidget(button1);
    layout->addWidget(label);
    setLayout(layout);
    connect(button0, &QPushButton::clicked, this, &MyWindow::setText);
    connect(button1, &QPushButton::clicked, qApp, &QCoreApplication::quit);
}
void MyWindow::setText() {
    label->setText(QString{"Label updated %0 times"}.arg(++count));
}

Here, I've made the widget pointers const for safety, so we know that they will be valid for the lifetime of the MyWindow object.

I've also used the method-pointer version of connect() - I recommend this where practicable, to get more checking at compile time.  Connect by name only when you really have to, such as when you can't know the complete types of the objects you're connecting.  See also Connecting overloaded signals and slots in Qt 5 if you have trouble with overloads.

Your code and comments suggest that you're concerned about leaking memory if an exception is thrown in an initializer.  For example, consider a failure to allocate/construct button0 after label was successfully created; will label then be left in limbo?  Although our ~MyWindow() destructor is not called (and we don't even need a non-default one), its superclass destructors will be.  And that includes the ~QObject destructor, which deletes its children.  We know that label will be deleted, because its constructor returned (and adding itself as a child is among the first things it does).

None of this would be true (and we would not be exception-safe) if we forgot to include the optional parent parameter (this) when constructing label; if we were to initialize it without a parent and then its setParent() method in the body of the constructor, we would have a gap where Qt wouldn't own it properly.

We can demonstrate this by wrapping one or more of the allocations in a function.  The following code allows you to experiment with throwing exceptions for the different child objects:

int alloc_succeeds = 3;
#include <stdexcept>
template<typename T, typename... Args>
T *createWidget(Args... args)
{
    if (--alloc_succeeds < 0)
        throw std::bad_alloc();
    return new T{args...};
}

#define PARENT this
//#define PARENT nullptr

#include <QApplication>
MyWindow::MyWindow(QWidget *parent)
    : QWidget{parent},
      label{createWidget<QLabel>("old text", PARENT)},
      button0{createWidget<QPushButton>("Increment", PARENT)},
      button1{createWidget<QPushButton>("Quit", PARENT)},
      layout{createWidget<QVBoxLayout>(PARENT)}
{

Change the value of alloc_succeeds to determine how many constructors are successful.  If we run the code under Valgrind, we see that by changing PARENT to nullptr, we can get our objects to leak (we also see some of the leaks of Qt itself, so it's helpful to run both to eliminate false positives).


Flex app for quizzing This is from a flex app that is for quizzing. It calls the database for the question and then checks to see if it has been tagged (bookmarked) by a user already. "Question" is a value object and "Tag" is also a value object.

I just wanted to post it to see how it could be better. It works fine, but I feel that since I am new to this, it probably isn't best.

public function getQuestionByNumber($itemNum, $userID) {
    $stmt = mysqli_prepare($this->connection,
          "SELECT
                        questions.id, questions.question_number, questions.chapter_id, questions.section_id,
                        questions.question_txt, questions.answers, questions.feedbacks, questions.correct_answer,
                        questions.question_type, questions.module, questions.shuffle_answer,
                        questions.tags
           FROM questions where questions.question_number=?");
      $this->throwExceptionOnError();

      mysqli_stmt_bind_param($stmt, 'i', $itemNum); $this->throwExceptionOnError();

      mysqli_stmt_execute($stmt); $this->throwExceptionOnError();

      $row = new Question();

      mysqli_stmt_bind_result($stmt, $row->id, $row->question_number, $row->chapter_id, $row->section_id,
                                        $row->question_txt, $row->answers, $row->feedbacks, $row->correct_answer,
                                        $row->question_type, $row->module, $row->shuffle_answer, $row->tags);

          if (mysqli_stmt_fetch($stmt)) {

              } else {
                          return null;
                  }
            mysqli_stmt_free_result($stmt);
            mysqli_stmt_close($stmt);

    $stmt2 = mysqli_prepare($this->connection,
          "SELECT
                        id
           FROM tagged where user_id =? && question_number=?");
      $this->throwExceptionOnError();

      mysqli_stmt_bind_param($stmt2, 'ii', $userID, $itemNum);
      $this->throwExceptionOnError();

      mysqli_stmt_execute($stmt2);
      $this->throwExceptionOnError();

      $tag = new Tag();

      mysqli_stmt_bind_result($stmt2, $tag->id);

      if (mysqli_stmt_fetch($stmt2)) {
                  $row->tagged = $tag->id;
                  return $row;
      } else {
                  $row->tagged = 0;
                  return $row;
          }

      mysqli_stmt_free_result($stmt2);
      mysqli_close($this->connection);

}

You could combine your two SQL queries so you don't have to hit the database twice.

The new SQL statement would be something like this:

SELECT
    q.id, q.question_number, q.chapter_id, q.section_id,
    q.question_txt, q.answers, q.feedbacks, q.correct_answer,
    q.question_type, q.module, q.shuffle_answer,
    q.tags, COALESCE(t.id, 0) As tagged
FROM questions q
     LEFT JOIN tagged t ON q.question_number = t.question_number AND t.user_id = ?
WHERE q.question_number=?

The COALESCE function would return the id from the tagged table if one exists, otherwise it returns 0.  This eliminates the need for your final if/else statement and gives you a direct mapping for $row->tagged.

You should also release all result sets and close all connections before issuing a return statement.  Any code after a return statement will not be executed.


Scraping PubMed query results How can I make the following PHP code better, more efficient, shorter, elegant, etc?  While it already works, I am still learning PHP and want to improve upon my current code:

<?php

$query = 'psoriasis';

$eSearchQueryParameters = array(
    'db' => 'pubmed',
    'term' => $query,
    'retmode' => 'xml',
    'retstart' => '0',
    'retmax' => '500',
    'usehistory' => 'y',
);

$eSearchQueryResults = simplexml_load_file('http://eutils.ncbi.nlm.nih.gov/entrez/eutils/esearch.fcgi?' . http_build_query($eSearchQueryParameters));

$eFetchQueryParameters = array(
    'db' => 'pubmed',
    'retmax' => '500',
    'query_key' => (string) $eSearchQueryResults->QueryKey,
    'WebEnv' => (string) $eSearchQueryResults->WebEnv,
);

$eFetchURL = 'http://eutils.ncbi.nlm.nih.gov/entrez/eutils/efetch.fcgi?' . http_build_query($eFetchQueryParameters);
$matches = array();
preg_match_all('/[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,4}/', file_get_contents($eFetchURL), $matches);

foreach ($matches[0] as $key => $value) {
    echo $value . "<br />";
}

?>

In my humble opinion, the version below is more elegant, easier to read and to maintain.

It uses a class to keep things more organized.

I would consider the code below as a starting point to start being improved, with things like error checking, etc.

I hope you like, and maybe learn something from it.

<?php

class MyQuery
{
    public $query = '';

    public $search_url = 'http://eutils.ncbi.nlm.nih.gov/entrez/eutils/esearch.fcgi?';
    public $fetch_url  = 'http://eutils.ncbi.nlm.nih.gov/entrez/eutils/efetch.fcgi?';

    public $search_parameters = array(
        'db'         => 'pubmed',
        'term'       => '',
        'retmode'    => 'xml',
        'retstart'   => '0',
        'retmax'     => '500',
        'usehistory' => 'y'
    );

    public $fetch_parameters = array(
        'db'        => 'pubmed',
        'retmax'    => '500',
        'query_key' => '',
        'WebEnv'    => ''
    );

    public $search_results;
    public $fetch_results;
    public $matches = array();

    public $match_regex = '/[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,4}/';

    public __constructor( $query )
    {
        $this->query = $query;
    }

    public search()
    {
        $this->search_parameters['term'] = $this->query;
        $url = $this->search_url . http_build_query( $this->search_parameters );
        $this->search_results = simplexml_load_file( $url );
    }

    public fetch()
    {
        $this->fetch_parameters['query_key'] = (string) $this->search_results->QueryKey;
        $this->fetch_parameters['WebEnv']    = (string) $this->search_results->WebEnv
        $url = $this->fetch_url . http_build_query( $this->fetch_parameters );
        $this->fetch_results = file_get_contents( $url );
    }

    public match()
    {
        $matches = array();
        preg_match_all( $this->match_regex, $this->fetch_results, $matches );
        $this->matches = array_values( $matches[0] );
    }

    public get()
    {
        $this->search();
        $this->fetch();
        $this->match();
        return $this->matches;
    }
}

$query  = new MyQuery( 'psoriasis' );
$result = $query->get();
echo implode( '<br />', $result );

?>


Generating a sequence number based on last item of a list I wrote this method in order to generate a sequence number based on a specific pattern:

public string LatestNumber(Guid id = default(Guid))
{
    var number = "";
    if (id == Guid.Empty)
    {
        var items = myList.ToList();
        var lastNumber = items.Any() ? items[items.Count - 1].Number : "";

        var intitialNumber = "Number#000001";
        if (!items.Any())
            number = intitialNumber;
        else
        {
            var nextNumber = int.Parse(lastNumber.Split('#')[1]) + 1;
            number = $"Number#{nextNumber:D6}";
        }
    }
    else
    {
        number = myList.FirstOrDefault(p => p.Id == id).Number;
    }

    return number;
}

This code works as expected in production but I wonder if there's a better way to make this code clean or more testable. What do you think?

There are a couple of things you could do differently.

number - this variable is not necessary, you can return anytime
myList - if this is already a list then you don't need to copy it
lastNumber - this is initialized too early. It should be placed inside else
if - don't ommit the {}
!items.Any() and items.Any() - you don't need to check it twice and even with different results
: "" - this value is never used because you don't enter the first else if myList.Any() - in this case you use .Number and the only place where you use the resulting lastNumber is the first else
[1] - you should name this index
intitialNumber - define this as a constant

number = myList.FirstOrDefault(p => p.Id == id).Number;

You know that this might blow? If the list is empty then the Number cannot be read. You need either C# 6 for this or one more line of code.

Putting everything together gives:

const string intitialNumber = "Number#000001";

if (id == Guid.Empty)
{
    if(myList.Any())
    {
        const int someMagicValue = 1;
        var lastNumber = myList.Last().Number; // Last is O(1) for IList
        var nextNumber = int.Parse(lastNumber.Split('#')[someMagicValue]) + 1;
        return = $"Number#{nextNumber:D6}";
    }
    else
    {
        return intitialNumber;
    }
}
else
{
    var first = myList.FirstOrDefault(p => p.Id == id);
    return first != null ? first.Number : intitialNumber;

    // C# 6
    // myList.FirstOrDefault(p => p.Id == id)?.Number ?? intitialNumber;
}


Randomly Permute Elements in a List I want to randomly permute a finite list in the most effective and efficient way in C#. My attempt is as follows.

/*===================================*
 * Compile it to produce Shuffle.exe *
 * ==================================*/
using System;
using System.Collections.Generic;
using System.IO;

namespace Shuffle
{
    class Program
    {
        static void Main(string[] args)
        {
            int Columns = int.Parse(args[0]);
            int Rows = int.Parse(args[1]);
            int Seed = int.Parse(args[2]);
            string OutputFilename = args[3];

            List<string> OrderedList = new List<string>();
            for (int x = 0; x < Columns; x++)
                for (int y = 0; y < Rows; y++)
                    OrderedList.Add(string.Format("{{{0},{1}}}", x, y));

            Random rnd = new Random(Seed);
            List<string> ShuffledList = new List<string>();

            for (int i = 0; i < Columns * Rows; i++)
            {
                int x = rnd.Next(OrderedList.Count);
                ShuffledList.Add(OrderedList[x]);
                OrderedList.RemoveAt(x);
            }

            using (StreamWriter sw = new StreamWriter(OutputFilename))
            {
                foreach (string s in ShuffledList)
                    sw.WriteLine(s);
            }

        }
    }
}

This program will be used in my production. Could you review whether or not my code is already the most efficient and effective in C#?

Don't waste time on micro-optimizations. Use the simplest possible techniques that are known to perform well. In your specific case, it means replacing your multiple lists with one single array and using the standard fisher-yates-shuffle.
Naming conventions
In good C# code, local variables are camelCase beginning with a lower case letter. I renamed most of your variables to better express intent.

Generating the strings
You'll get a performance increase of about 30%-40% by using an array instead of a list (not much to be gained here).
Don't bother with string concatenation instead of string.Format, there is hardly any measurable difference in speed, but a huge difference in readability and maintainability.
string[] elements = new string[columns * rows];
for (int column = 0; column < columns; column++)
    for (int row = 0; row < rows; row++)
        elements[columns * row + row] = string.Format("{{{0},{1}}}", column, row);

Shuffling them
Whenever you remove from a list, all the remaining elements have to be shifted to fill the gap. Use your array instead and use the fisher-yates-shuffle; don't attempt inventing your own.
for (int i = elements.Length - 1; i > 0; i--)
{
    int swapIndex = random.Next(i + 1);
    string tmp = elements[i];
    elements[i] = elements[swapIndex];
    elements[swapIndex] = tmp;
}

This runs about 160 times as fast as your shuffling implementation.

Don't reinvent the wheel
When saving the results to disk, use File.WriteAllLines which does exactly what you were doing with the StreamWriter - in a single line.
File.WriteAllLines(outputFilename, elements);

Complete code (argument parsing omitted)
Once you've realized that a few milliseconds more or less don't matter, it would be good to split this up into several methods that each do exactly one thing. I'll leave that as an exercise to you.
string[] elements = new string[columns * rows];
for (int column = 0; column < columns; column++)
    for (int row = 0; row < rows; row++)
        elements[column*rows + row] = string.Format("{{{0},{1}}}", column, row);

Random random = new Random(seed);
for (int i = elements.Length - 1; i > 0; i--)
{
    int swapIndex = random.Next(i + 1);
    string tmp = elements[i];
    elements[i] = elements[swapIndex];
    elements[swapIndex] = tmp;
}

File.WriteAllLines(outputFilename, elements);


Double Inclusive Range step by step I've written a method to obtain an IEnumerable from a start point, end point and step. Similar to Enumerable.Range() but for doubles and not fixed step.

public static IEnumerable<double> InclusiveRange(double start, double end, double step = 1)
    {
        while (start <= end)
        {
            yield return start;
            start += step;
        }
    }

Am I missing something, or there is a better approach?

As this is a quit nice approach, it is missing an important part.

Validation of input parameter

You should check at least the step parameter if it is a positive number greater 0.

InclusiveRange(1d, 5d,-1);

the above call is by your definition a valid call, but can lead to a System.OutOfMemoryException if combined with the .ToList() extension method.

You need to decide if you want to enable negative step. If you don't want to allow negative values for step a simple guard clause can handle this.

if (step <= 0) { throw new ArgumentOutOfRangeException("step", "The value has to be greater than 0"); }

But if you want to enable negative step values you need to add more checks and a different implementation.

A negative step means that start > end so you would need to introduce 2 more methods which you then call based on step beeing positive or negative.

private static IEnumerable<double> InclusiveRangeAscending(double start, double end, double step)
{

}

private static IEnumerable<double> InclusiveRangeDescending(double start, double end, double step)
{

}

public static IEnumerable<double> InclusiveRange(double start, double end, double step = 1)
{
    if (step == 0) { throw new ArgumentOutOfRangeException("step", "The value has to be unequal to 0"); }
    if (step > 0)
    {
        return InclusiveRangeAscending(start, end, step);
    }
    else
    {
        return InclusiveRangeDescending(start, end, step);
    }
}


Python Basic Calculator Hello I made this calculator in Python, I know it's pretty basic but I want your opinions if I made it well or not, and if you find any mistakes or not.
P.S ( I know I could do another better than this one if I looked in internet but I want to evolve that's why I'm here )

#Commands
print("Operations that you can use:")
print("Addition: +")
print("Subtraction: -")
print("Multiplication: *")
print("Division: /")
print("Exponentiation: **")
print("Radicalization: **(1/")
print("\n")

#Variables
x = None
y = None
z = None
ce = 0

while ce < 1:

# Inputs
    x = float(input("First Value: "))
    y = input("Operation: ")
    z = float(input("Second Value: "))

# If
    if '+' == y:
        add = float(x) + float(z)
        print(add)
    elif '-' == y:
        sub = float(x) - float(z)
        print(sub)
    elif '/' == y:
        div = float(x) / float(z)
        print(div)
    elif "**(1/" == y:
        rad = float(x) **(1/float(z))
        print(rad)
    elif "**" == y:
        exp = float(x) ** float(z)
        print(exp)
    elif '*' == y:
        mul = float(x) * float(z)
        print(mul)
    print("Continue or Exit?")
    ce = int(input("    0    or   1  : "))

Let me offer the following rewrite of your program and then let's walk through it:

names = {'Addition' : '+',
         'Subtraction' : '-',
         'Division' : '/',
         'Exponentation': '**',
         'Radicalization': '**(1/'
        }

print("Operations that you can use:")
for op in names:
    print(op, names[op])

ops = {
       '+' : (lambda x, y: x + y),
       '-' : (lambda x, y: x - y),
       '*' : (lambda x, y: x * y),
       '/' : (lambda x, y: x / y),
       '**' : (lambda x, y: x ** y),
       '**(1/' : (lambda x, y: x ** 1.0 / y),
       'na' : (lambda x, y: 'Unknown binary operator')
      }

ce = 0
while ce != 1:
    x = float(input("First Value: "))
    y = input("Operation: ")
    z = float(input("Second Value: "))

    op = ops.get(y, 'na')
    print(ops[op](x, z))

    print("Continue or Exit?")
    ce = int(input("    0    or   1  : "))

It seems unnecessary to define x, y and z to be None so we just define them inside the main loop.
The ops dictionary stores a symbol of the operator and the actual implementation in a lambda function. It also stores a special 'na' value to represent any unknown operator; this is useful in the main loop where we return that key if we don't find what the user gave us as input.
The key difference and main idea here is a data-driven solution. That is, notice how we've put the supported operators into data structures. This way it's easier to make changes and to maintain the program, e.g., when you add a new binary operator, you don't have to touch the logic of your calculator, just add a new element into your data structure.
There's no explicit if-elif chain anymore, yay! Such chains are often annoying to write and it's always a potential code smell. No matter how experienced you are, it's one of those things where it's easy to make a human error and write something you didn't mean to, but it can be tough to discover the bug.