Custom encryption algorithm, "SBC" I decided to roll out my own encryption algorithm, which I am calling "SBC" or Simple Byte Cipher. Essentially, I just take the bytes of the string data, do a Caesar cipher like shift to it, increment through an array of password bytes for each byte in the data (this enables the same letters such as "T" to not always have the same output, which would be easily crackable). And in the end I just reverse the process to glean the original data.

My code seems to work just fine; decryption works like a dream. I just want to know if there is a better way, or any hidden errors that may occur with the code as it currently stands.

I am aware that the length of the password and data are not length tested, or null tested. I will address that later.

public static class SimpleByteCipher
    {
        public static byte[] EncryptStringToByteArray( string data , string password )
        {
            byte[] bytes = Encoding.ASCII.GetBytes( data );
            byte[] passwordBytes = Encoding.ASCII.GetBytes( password );
            int passwordShiftIndex = 0;
            for( int i = 0; i < bytes.Length; i++ )
            {
                bytes[ i ] = ( byte )( bytes[ i ] + passwordBytes[ passwordShiftIndex ] );
                passwordShiftIndex = ( passwordShiftIndex + 1 ) % passwordBytes.Length;
            }
            return bytes;
        }

        public static string DecryptByteArrayToString( byte[] data , string password )
        {
            byte[] bytes = data;
            byte[] passwordBytes = Encoding.ASCII.GetBytes( password );
            int passwordShiftIndex = 0;
            for( int i = 0; i < bytes.Length; i++ )
            {
                bytes[ i ] = ( byte )( bytes[ i ] - passwordBytes[ passwordShiftIndex ] );
                passwordShiftIndex = ( passwordShiftIndex + 1 ) % passwordBytes.Length;
            }
            return Encoding.ASCII.GetString( bytes );
        }

        public static byte[] EncryptStringToByteArray( string data , string password , uint seed)
        {
            byte[] bytes = Encoding.ASCII.GetBytes( data );
            byte[] passwordBytes = Encoding.ASCII.GetBytes( password );
            int passwordShiftIndex = 0;
            for( int i = 0; i < bytes.Length; i++ )
            {
                bytes[ i ] = ( byte )( bytes[ i ] + passwordBytes[ passwordShiftIndex ] + seed );
                passwordShiftIndex = ( passwordShiftIndex + 1 ) % passwordBytes.Length;
            }
            return bytes;
        }

        public static string DecryptByteArrayToString( byte[] data , string password , uint seed)
        {
            byte[] bytes = data;
            byte[] passwordBytes = Encoding.ASCII.GetBytes( password );
            int passwordShiftIndex = 0;
            for( int i = 0; i < bytes.Length; i++ )
            {
                bytes[ i ] = ( byte )( bytes[ i ] - passwordBytes[ passwordShiftIndex ] - seed );
                passwordShiftIndex = ( passwordShiftIndex + 1 ) % passwordBytes.Length;
            }
            return Encoding.ASCII.GetString( bytes );
        }
    }

And here is a practice main:

class Program
    {
        static void Main( string[] args )
        {
            string data = "This is but a test.";
            byte[] encrypted = SimpleByteCipher.EncryptStringToByteArray( data , "Test" , 5000 );
            string decrypted = SimpleByteCipher.DecryptByteArrayToString( encrypted , "Test" , 5000 );
            Console.WriteLine( decrypted );
            Console.ReadLine();
        }
     }

The output correctly prints:

  "This is but a test."

And here is a visualization of what the encrypted bytes look like in a text file:

Reduce Duplication

There appears to be some significant code duplication between the encrypt and decrypt methods.

Method Overloads

The first and most obvious spots we can refactor are the overloads for your decrypt and encrypt functions:

public static byte[] EncryptStringToByteArray( string data , string password )
{
    byte[] bytes = Encoding.ASCII.GetBytes( data );
    byte[] passwordBytes = Encoding.ASCII.GetBytes( password );
    int passwordShiftIndex = 0;
    for( int i = 0; i < bytes.Length; i++ )
    {
        bytes[ i ] = ( byte )( bytes[ i ] + passwordBytes[ passwordShiftIndex ] );
        passwordShiftIndex = ( passwordShiftIndex + 1 ) % passwordBytes.Length;
    }
    return bytes;
}

public static byte[] EncryptStringToByteArray( string data , string password , uint seed)
{
    byte[] bytes = Encoding.ASCII.GetBytes( data );
    byte[] passwordBytes = Encoding.ASCII.GetBytes( password );
    int passwordShiftIndex = 0;
    for( int i = 0; i < bytes.Length; i++ )
    {
        bytes[ i ] = ( byte )( bytes[ i ] + passwordBytes[ passwordShiftIndex ] + seed );
        passwordShiftIndex = ( passwordShiftIndex + 1 ) % passwordBytes.Length;
    }
    return bytes;
}

The only difference between these two is that one takes in a seed and adds it to the bytes within the loop.  The overload without a seed can be rewritten as follows:

public static byte[] EncryptStringToByteArray(string data, string password)
{
    return EncryptStringToByteArray(data, password, 0);
}

A similar refactor can be done with the two decrypt overloads.  I will leave that step as an exercise for the reader.

Symmetric Encryption

With what's left, I cannot help but notice that this appears to be a symmetric encryption algorithm.  As a result, the encrypt and decrypt steps are nearly identical to one another:

public static byte[] EncryptStringToByteArray( string data , string password , uint seed)
{
    byte[] bytes = Encoding.ASCII.GetBytes( data );
    byte[] passwordBytes = Encoding.ASCII.GetBytes( password );
    int passwordShiftIndex = 0;
    for( int i = 0; i < bytes.Length; i++ )
    {
        bytes[ i ] = ( byte )( bytes[ i ] + passwordBytes[ passwordShiftIndex ] + seed );
        passwordShiftIndex = ( passwordShiftIndex + 1 ) % passwordBytes.Length;
    }
    return bytes;
}

public static string DecryptByteArrayToString( byte[] data , string password , uint seed)
{
    byte[] bytes = data;
    byte[] passwordBytes = Encoding.ASCII.GetBytes( password );
    int passwordShiftIndex = 0;
    for( int i = 0; i < bytes.Length; i++ )
    {
        bytes[ i ] = ( byte )( bytes[ i ] - passwordBytes[ passwordShiftIndex ] - seed );
        passwordShiftIndex = ( passwordShiftIndex + 1 ) % passwordBytes.Length;
    }
    return Encoding.ASCII.GetString( bytes );
}

They do, however, have some subtle differences:

Encrypt adds the seed, while Decrypt subtracts it
Encrypt takes in string and returns byte[], while Decrypt takes in byte[] and returns string.

However, neither are particularly large differences.  We can refactor further if we extract the following method:

private static byte[] Encrypt(byte[] bytes, string password, uint seed, int factor)
{
    var passwordBytes = Encoding.ASCII.GetBytes(password);
    var passwordShiftIndex = 0;
    for(int i = 0; i < bytes.Length; i++)
    {
        bytes[i] = (byte) (bytes[i] + factor * (passwordBytes[passwordShiftIndex] + seed));
        passwordShiftIndex = (passwordShiftIndex + 1) % passwordBytes.Length;
    }
    return bytes;
}

Note the following changes:

It takes in and returns byte[]
I added an integer factor variable which is used to control whether passwordBytes[passwordShiftIndex] and seed are added or subtracted

With those changes, we can refactor our seeded encrypt and decrypt methods to call the above method, changing the class as follows:

public static class SimpleByteCipher
{
    public static byte[] EncryptStringToByteArray(string data, string password)
    {
        return EncryptStringToByteArray(data, password, 0);
    }

    public static byte[] EncryptStringToByteArray(string data, string password, uint seed)
    {
        var bytes = Encoding.ASCII.GetBytes(data);
        return Encrypt(bytes, password, seed, 1);
    }

    private static byte[] Encrypt(byte[] bytes, string password, uint seed, int factor)
    {
        var passwordBytes = Encoding.ASCII.GetBytes(password);
        var passwordShiftIndex = 0;
        for(int i = 0; i < bytes.Length; i++)
        {
            bytes[i] = (byte) (bytes[i] + factor * (passwordBytes[passwordShiftIndex] + seed));
            passwordShiftIndex = (passwordShiftIndex + 1) % passwordBytes.Length;
        }
        return bytes;
    }

    public static string DecryptByteArrayToString(byte[] data, string password)
    {
        return DecryptByteArrayToString(data, password, 0);
    }

    public static string DecryptByteArrayToString(byte[] data, string password, uint seed)
    {
        var bytes = Encrypt(data, password, seed, -1);
        return Encoding.ASCII.GetString(bytes);
    }
}


One-handed Solitaire algorithm My attempt at a simulation of one-handed Solitaire. Any comments welcome!

// Basic implementation of one handed solitaire by Andrew Ewen.
//
// Not sure if working correctly as it wins roughly 1/90 games whereas wikipedia states it
// is closer to 1/140.

function Card (suit, rank) {
    this.suit = suit;
    this.rank = rank;
}

Card.suits = ["Spades", "Hearts", "Clubs", "Diamonds"];
Card.prototype.toString = function () {
    var suit = Card.suits[this.suit];
    var rank = (this.rank + 1).toString();

    switch (this.rank) {
        case 0:
            rank = "Ace";
            break;
        case 10:
            rank = "Jack";
            break;
        case 11:
            rank = "Queen";
            break;
        case 12:
            rank = "King";
            break;
    }

    return rank + " of " + suit;
}

function debugLog(msg) {
    var enabled = false;
    if (enabled) console.log(msg);
}

var deck = function () {
    cards = [];
    // Create deck of cards
    for (var suit = 0; suit < 4; suit++) {
        for (var rank = 0; rank < 13; rank++) {
            cards.push(new Card(suit, rank));
        }
    }

    // Shuffle cards (Fisher-Yates)
    for (var i = cards.length - 1; i > 0; i--) {
        var j = Math.floor(Math.random() * (i+1));
        var t = cards[j];
        cards[j] = cards[i];
        cards[i] = t;
    }
    return cards;
};

// Stats
var wins = 0;
var losses = 0;

for(var i = 0; i < 1000000; i++) {
    var pile = deck();
    var hand = [];
    var discard = [];

    var turns = 0;
    var gameOver = false;
    while (!gameOver) {
        debugLog("turn: " + turns++);

        Array.prototype.push.apply(hand, pile.splice(pile.length - 1, 1));
        // Keep drawing cards until there is either non left or we have enough
        while (hand.length < 4 && pile.length > 0) {
            hand.push(pile.pop());
        }

        // Check if we're on our last 2 cards.
        var last2 = hand.length < 4;
        // Always check last card in hand
        var cardA = hand[hand.length - 1];
        // If we're on the last two compare with the first, otherwise compare
        // with 4th from the end.
        var cardB = last2 ? hand[0] : hand[hand.length - 4];

        // Check for matches and discard accordingly
        var match = false;
        if(cardA.suit === cardB.suit) {
            var removed;
            if (last2) {
                removed = hand.splice(0, hand.length);
            } else {
                removed = hand.splice(hand.length - 3, 2);
            }
            Array.prototype.push.apply(discard, removed);
            match = true;
        } else if (cardA.rank === cardB.rank) {
            var removed;
            if (last2) {
                removed = hand.splice(0, hand.length);
            } else {
                removed = hand.splice(hand.length - 4, 4);
            }
            Array.prototype.push.apply(discard, removed);
            match = true;
        }

        if (match) { debugLog("Match: " + cardA + " -> " + cardB); }

        debugLog("hand: " + hand.length);
        debugLog("pile: " + pile.length);
        debugLog("discard: " + discard.length);
        debugLog("");

        // Detect win and loss conditions
        if (pile.length === 0 && !match) {
            debugLog("You lose...");
            losses += 1;
            gameOver = true;
        }
        if (pile.length === 0 && hand.length === 0) {
            debugLog("You win!");
            wins += 1;
            gameOver = true;
        }
    }
    console.log("Win rate: " + (wins/losses));
}

Overall it's pretty good, but there might be something iffy going on with the rules you use.

I had honestly never heard of one-handed solitaire, but if I understand it correctly, there are no special rules for the last cards. You either have exactly 4 cards left, and win if you can discard them all, or you lose.

But you have a last2 rule, where you change the rules to look at the 1st and 2nd card if there are less than four cards left. I imagine this skews the results.

Anyway, in terms of code:

Don't mix plain console.log and a custom logging function.
Put the code for playing 1 round into a function. Right now it's all inside a loop that runs 10000 times. It'd be a lot simpler to just call play() and get a won/lost boolean back. You can always loop that.
It'd be simpler to unshift cards onto the hand. Then you'd always be comparing hand[0] to hand[3] instead of having to subtract stuff from hand.length. Splicing also becomes just splice(0, 4) or splice(1, 2)
You can use concat instead of Array.prototype.push.apply (or the slightly shorter alternative [].push.apply). The difference, however, is that concat returns a new array instead of modifying the receiver like push does.
You can simplify Card and Deck a little by using a list of values for rank as well as suit:

function Card(suit, rank) {
  this.suit = suit;
  this.rank = rank;
}

Card.prototype = {
  toString: function () {
    return this.rank + this.suit;
  }
};

Card.suits = ["♠", "♥", "♣", "♦"];
Card.ranks = ["A", 2, 3, 4, 5, 6, 7, 8, 9, 10, "J", "Q", "K"];

function Deck() {
  var cards, shuffled, i;

  cards = Card.suits.reduce(function (cards, suit) {
    return cards.concat(Card.ranks.map(function (rank) {
      return new Card(suit, rank);
    }));
  }, []);

  shuffled = [];
  while(cards.length) {
    i = Math.random() * cards.length | 0;
    shuffled.push(cards.splice(i, 1)[0]);
  }

  return shuffled;
}

I've named Deck with PascalCase as though it's a constructor, which it isn't quite. But you can call it as new Deck or just Deck() and either thing'll work, as it explicitly returns something.

You could also take a different tack altogether. You don't really need a hand array: You can just examine the shuffled deck directly. For instance:

function play() {
  var deck = new Deck,
      i = deck.length,
      a, b;

  while(true) {
    // decrement if we're too close to the end of the deck
    i = Math.min(i, deck.length-4);

    // if we had to decrement past zero, game's over
    if(i < 0) {
      return deck.length === 0;
    }

    // the cards to examine
    a = deck[i],
    b = deck[i+3];

    // match cards or decrement
    if(a.rank === b.rank) {
      deck.splice(i, 4);
    } else if(a.suit === b.suit) {
      deck.splice(i + 1, 2);
    } else {
      i--;
    }
  }
}

(Edit: I updated the code based on @Jonah's comment below to examine the deck from the end, rather than the beginning.)

Running 300,000 games, I get a 0.730% chance of winning, which is pretty close to 1 in 140.

Edit: It makes for a nice animation - see snippet below

var frameTime = 100;
var button = document.getElementsByTagName("button")[0];
var span = document.getElementsByTagName("span")[0];

function Card(suit, rank) {
  this.suit = suit;
  this.rank = rank;
}

Card.suits = ["♠", "♥", "♣", "♦"];
Card.ranks = ["A", 2, 3, 4, 5, 6, 7, 8, 9, 10, "J", "Q", "K"];

function Deck() {
  var cards, shuffled, i;

  cards = Card.suits.reduce(function (cards, suit) {
    return cards.concat(Card.ranks.map(function (rank) {
      return new Card(suit, rank);
    }));
  }, []);

  shuffled = [];
  while(cards.length) {
    i = Math.random() * cards.length | 0;
    shuffled.push(cards.splice(i, 1)[0]);
  }

  return shuffled;
}

function play() {
  var deck = new Deck,
      i = deck.length - 4;

  button.disabled = true;
  span.innerHTML = "";

  function step() {
    var a, b;

    i = Math.min(i, deck.length - 4);
    drawState(deck, i);

    if(i < 0) {
      if(deck.length) {
        span.innerHTML = "Lost (" + deck.length + " cards remain)";
      } else {
        span.innerHTML = "Won!";
      }
      button.disabled = false;
      return;
    }

    a = deck[i];
    b = deck[i+3];

    if(a.rank === b.rank) {
      deck.splice(i, 4);
    } else if(a.suit === b.suit) {
      deck.splice(i+1, 2);
    } else {
      i--;
    }

    setTimeout(step, frameTime);
  }

  step();
}

function drawState(deck, offset) {
  var canvas = document.getElementsByTagName("canvas")[0],
      ctx = canvas.getContext("2d");

  canvas.width = 520;
  canvas.height = 35;

  function drawTriangle(x, y) {
    ctx.beginPath();
    ctx.moveTo(x, y);
    ctx.lineTo(x + 3, y + 5);
    ctx.lineTo(x - 3, y + 5);
    ctx.closePath()
    ctx.fill();
  }

  if(offset >= 0) {
    drawTriangle(offset * 10 + 5, 25);
    drawTriangle((offset + 3) * 10 + 5, 25);
    ctx.fillStyle = "rgba(0, 0, 0, 0.1)";
    ctx.fillRect(offset * 10, 0, 40, 22);
  }

  ctx.fillStyle = "#000";
  ctx.font = "10px serif";
  ctx.textAlign = "center";

  deck.forEach(function (card, x) {
    x = x * 10 + 5;
    ctx.fillText(card.rank, x, 10);
    ctx.fillText(card.suit, x, 20);
  });
}

button.addEventListener("click", play, false);
play();
<canvas></canvas>

<button>Play again</button>


Displaying alerts for blank fields I'm trying to make my code easier to understand by making it more correct and maintainable.

Here's the code I'm trying to simplify:

       $(document).ready(function(){
  $('.opretbruger').on('click', function(){
    if (opretbruger() === true){

    }
  });
});

var opretbruger = function(){
  if($('#brugernavn').val() === ''){
     alert('remember Email');
      return false;
  }

  if($('#pass1').val() === ''){
    alert('remember password');
    return false;
  }

  if($('#pass2').val() === ''){
    alert('remember password');
    return false;
  }

  if($('#fornavn').val() === ''){
    alert('remember your name');
    return false;
  }

  if($('#efternavn').val() === ''){
    alert('remember your last name');
    return false;
  }

  return true;
};

Here's my attempt at simplifying it:

       $(function () {
     var $login = $("#login"),
         $pwd = $("#pass"),
         $usr = $("#brugernavn");

     $login.on("submit", function (event) {
         var msg = "Husk ",
             usr = ($usr.val().trim() !== ""),
             pwd = ($pwd.val().trim() !== "");

         msg += !usr ? " brugernavn" : "";
         msg += !usr && !pwd ? " og " : "";
         msg += !pwd ? " kodeord" : "";

         (pwd && usr) || alert(msg);

         return (pwd && usr);
     });
 });

Let me make sure I understand the problem.  You have a number of fields on the page where a value is required; when the user submits the page, and any of the fields are blank, you'd like the page to display a Javascript alert instead of submitting the form.  Yes?

I'd suggest you use an unobtrusive technique as follows:

If a field is required, it is marked with a "required" css class.  This can be a dummy class if you like.
Each field that is required is associated with a data attribute which specifies its human-readable name in the appropriate language
The submit button inspects all fields that are marked with the style and displays the names.

So to mark up a required field, use this pattern:

<input ID="login" class="required" data-name="login"/>

Then include this bit of code in your common script.  There's nothing page- or field- specific in it, so you can include it in every page if you want (e.g. in "common.js" or whatever):

$(document).ready(function(){
    var validate = function(e){
        var emptyFields = [];
        $(".required").each(function(){
            if ($(this).val().trim() == "") emptyFields.push($(this).data("name"));
        }
        if (emptyFields.length != 0) {
            e.preventDefault();
            var message = "You forgot these fields: " + emptyFields.join(",");
            alert(message);
        }
    }

    $(":submit").click(validate);
})

This ought to cover any additional fields or really any additional pages that require this sort of validation.


Speed up CSV reading code (vector of doubles) I am trying to read a single-columned CSV of doubles into Java with a string header. It is 11 megabytes and takes over 15 minutes to read, which is clearly unacceptable. In R this CSV would take about 3 seconds to load.

This CSV file may contain strings so I am parsing it with this in mind.

The CSV reading method needs to return Vector<Double> due to reliance on this output by other parts of the application.

The issue is not due to the isNumber static method, since each call to that is taking 200 nanoseconds, thus contributing approximately 0.2 seconds to the 15 minutes of parsing time.

The Double.valueOf() only takes about 500 nanoseconds, so it is not that either.

csvData.add() is only taking 80 nanoseconds so it is not that.

private static Vector<Double> readTXTFileSingle(String csvFileName) throws IOException {
    String line = null;
    BufferedReader stream = null;
    Vector<Double> csvData = new Vector<Double>();

    try {
        stream = new BufferedReader(new FileReader(csvFileName));
        while ((line = stream.readLine()) != null) {
            String[] splitted = line.split(",");
            if( ! NumberUtils.isNumber(splitted[0])) {

                continue;
            }

            Double dataLine = Double.valueOf(splitted[0]);
            csvData.add(dataLine);

        }
    } finally {
        if (stream != null)
            stream.close();
    }
    return csvData;
}

As per Bobby's comment, Vector is your problem, but not for the reason he says...

Vector is a synchronized class. Each call to any method on Vector will lock the thread, flush all cache lines, and generally waste a lot of time (in a situation where it's usage is in a single thread only).

The fact that you use Vector indicates that you are running some really old code, or you have not properly read the JavaDoc for it.

A secondary performance problem is that each value is being converted to a Double Object. In cases where you have large amounts of data, and where there is a primitive available for you to use, it is always faster to use the primitive (in this case, double instead of Double).

You should also be using the Java7 try-with-resources mechanism for your stream.

My recommendation is to change the signature of your method to return a List... actually, no, my recommendation is to return an array of primitive double[].... if you are interested in speed, this will be a significant improvement:

private static double[] readTXTFileSingle(String csvFileName) throws IOException {
    double[] csvData = new double[4096]; // arbitrary starting size.
    int dcnt = 0;

    try (BufferedReader stream = new BufferedReader(new FileReader(csvFileName))) {
        String line = null;
        while ((line = stream.readLine()) != null) {
            String[] splitted = line.split(",");
            if( ! NumberUtils.isNumber(splitted[0])) {

                continue;
            }

            double dataLine = Double.parseDouble(splitted[0]);
            if (dcnt >= csvData.length) {
                // add 50% to array size.
                csvData = Arrays.copyOf(csvData, dcnt + (dcnt / 2));
            }
            csvData[dcnt++] = dataLine;

        }
    }
    return Arrays.copyOf(csvData, dcnt);
}

Edit:

One other thing, if you want another tweak in performance, use:

String[] splitted = line.split(",", 2);

since you never access more than the first field in the record, you do not need to look for comma's beyond the first comma


Optimizing Project Euler #12 (Python 3) I just figured out Project Euler problem #12. I'm still really new to Python, so I'm putting my code up here in the hopes that someone will be able to give me some tips for optimizing my code to run faster. It took an incredibly long time to run, so I'm hoping that someone will be able to give me pointers.

If anyone is willing, would you mind looking over my solution and letting me know how I can improve on it, either through optimization or style?

The link to the problem is here: Project Euler #12

run = True
p = 2
numberInUse = 3

def checkDivisors(number):
    numberOfDivisors = 0
    for x in range(1, int(number+1)):
        if number%x ==0:
            numberOfDivisors += 1
    return(numberOfDivisors)

while run == True:
    if checkDivisors(numberInUse) > 500:
        run = False
        print(numberInUse, "is the answer!")
    else:
        p += 1
        numberInUse += p

Project Euler problems are more about math than programming.

You must prove (or at least observe) the following:

An nth triangular number is \$\dfrac{n(n+1)}{2}\$. One of the numerator's factors is obviously even.
The number_of_divisors is a multiplicative function (known as \$\sigma\$), meaning that if \$m\$ and \$n\$ are coprime, then \$\sigma(mn) = \sigma(m)\cdot\sigma(n)\$
\$n\$ and \$n+1\$ are necessarily coprime.
Once you computed \$\sigma(n)\$ and \$\sigma(n+1)\$ on a certain iteration, on the next iteration you'd only need to compute \$\sigma(n+2)\$. Using memoization, you may even avoid this when \$n+2\$ is even.

This should be enough to build an efficient solution.


Showing the contents of a file This program reads a filename from standard input and then prints its content. Please review:

#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>
#include <errno.h>

int main()

{
    char fileName[20];

    // Get the filename from the user:
    int fileNameLength = read(0,fileName,19);
    // We need to get rid of the new line character caused by terminal
    // Replace the new line character with \0
    fileName[fileNameLength-1] = fileName[fileNameLength];

    printf("You want to see the contents of: %s\n", fileName);

    // open the file:
    int fd = 0;
    fd = open(fileName,O_RDONLY);

    if(fd == -1) {
        // Something went wrong, perhaps no such file:
        perror(NULL);
        printf("%s\n", fileName);
    } else {
        // Read until the read method returns 0 bytes.
        char buf[20];
        int numBytesRead = read(fd,buf,20);
        while(numBytesRead) {
            write(1,buf,numBytesRead);
            numBytesRead = read(fd,buf,20);
        }
        close(fd);
    }
    puts("");
}

And in action:

Korays-MacBook-Pro:~ koraytugay$ gcc koray.c
Korays-MacBook-Pro:~ koraytugay$ ./a.out
k.txt
You want to see the contents of: k.txt
Hello Code Review!
This is the contents of k.txt.
Have a good day!
Korays-MacBook-Pro:~ koraytugay$

Magic numbers 20 and 19?

Instead of this:

char fileName[20];

// Get the filename from the user:
int fileNameLength = read(0,fileName,19);

Define MAX_FILENAME_LENGTH somewhere, and use that as the buffer size parameter, and MAX_FILENAME_LENGTH + 1 as the array size containing the buffer, to account for the terminating null character.

Write in code your intention exactly

Write in code more explicitly what you want.
For example here you say in comment that you want to write a \0 to the final position of the char[], but the code doesn't do exactly that:

// We need to get rid of the new line character caused by terminal
// Replace the new line character with \0
fileName[fileNameLength-1] = fileName[fileNameLength];

Don't assume what might be at an uninitialized memory location.
If you want to set the terminating character to \0,
then do exactly that:

fileName[fileNameLength-1] = '\0';

More magic number 20

All those 20 everywhere:

char buf[20];
int numBytesRead = read(fd,buf,20);
while(numBytesRead) {
    write(1,buf,numBytesRead);
    numBytesRead = read(fd,buf,20);
}

Why not introduce a variable so that you can change it later if you want to:

int bufsize = 20;
char buf[bufsize];
int numBytesRead = read(fd,buf,bufsize);
while(numBytesRead) {
    write(1,buf,numBytesRead);
    numBytesRead = read(fd,buf,bufsize);
}

And why use a buffer of size 20? Why read a file 20 byte at a time? It would be faster to read in larger chunks. Surely you have enough memory to load 4 kbyte at a time. Luckily, now it's easy to change that:

int bufsize = 4096;

Avoid code duplication

In the previous code snippet, read(fd,buf,bufsize); appears twice,
which is not pretty. It can be rewritten without such duplication:

int numBytesRead;
while ((numBytesRead = read(fd, buf, bufsize)) > 0) {
    write(1, buf, numBytesRead);
}
close(fd);

On the other hand, some people find this writing style potentially error prone or confusing. In my opinion code duplication is the bigger evil,
so I still prefer this writing style, which is also shorter.

Comments stating the obvious

// open the file:
int fd = 0;
fd = open(fileName,O_RDONLY);

Is that comment really necessary there? Or is it just noise?

Pointless variable initialization

int fd = 0;
fd = open(fileName,O_RDONLY);

If you're going to set fd to something else, why set it to 0?

Usability

When you run the program,
it prints nothing,
it's just waiting for user input.
It would be better to print a prompt,
for example:

puts("Enter file name:");


Optimizing Java Anagram checker (compare 2 strings) An anagram is like a mix-up of the letters in a string:

  pots is an anagram of stop

  Wilma is an anagram of ilWma

I am going through the book Cracking the Coding Interview and in the basic string manipulation there's the problem:

  write a method to check if two strings are anagrams of each other.

My method uses StringBuffer instead of String because you can .deleteCharAt(index) with StringBuffer/StringBuilder.

public boolean areAnagrams(StringBuffer s1b, StringBuffer s2b) {

    for (int i=0; i<s1b.length(); ++i) {
        for (int j=0; j<s2b.length(); ++j) {

            if (s1b.charAt(i) == s2b.charAt(j)) {

                s1b.deleteCharAt(i);
                s2b.deleteCharAt(j);

                i=0;
                j=0;
            }
        }
    }

    if (s1b.equals(s2b)) {
        return true;
    } else
        return false;

}

I iterate over every character in s1b and if I find a matching char in s2b I delete them both from each string and restart the loop (set i and j to zero) because the length of the StringBuffer objects changes when you .deleteCharAt(index).

I have two questions:

Should I use StringBuilder over StringBuffer (in Java)?
How can I make this faster?

In regards to fasterness:

This method is good in that it doesn't require any additional space, but it sorta destroys the data as you're working on it.  Are there any alternatives that I have overlooked that could potentially preserve the strings but still see if they are anagrams without using too much external storage (i.e. copies of the strings are not allowed -- as a challenge)?

And, if you can use any sort of storage space in addition to this, can one lower the time complexity to \$O(n)\$ (technically \$O(2n)\$) instead of \$O(n^2)\$?

Also, the above code might not compile because I just wrote it from scratch in here; sorry if it's bugg'd.

Start with a simple, easy to understand version. Try to reuse API functions.

import java.util.Arrays;
...

public boolean areAnagrams(String s1, String s2) {
    char[] ch1 = s1.toCharArray();
    char[] ch2 = s2.toCharArray();
    Arrays.sort(ch1);
    Arrays.sort(ch2);
    return Arrays.equals(ch1,ch2);
}

Of course this is not the fastest way, but in 99% it is "good enough", and you can see what's going on. I would not even consider to juggle with things like deleted chars in StringBuilder if there were no serious performance problem.


Reducing cyclomatic complexity in a simple string concatenation I have written the following function that takes an address object and builds a string using the object properties. It will later be used to display a tooltip

var getAddressTooltip = function (address) {

    var tooltip = '';

    if (address.MasterAddress !== undefined &&
        address.MasterAddress !== null &&
        address.MasterAddress.Id !== undefined &&
        address.MasterAddress.Id !== null) {
        tooltip += '[' + address.MasterAddress.Id + '] ';
    }
    if (address.PurchasingContact !== null &&
        address.PurchasingContact !== undefined &&
        address.PurchasingContact.Name !== null &&
        address.PurchasingContact.Name !== undefined &&
        address.PurchasingContact.Name.length > 0) {
        tooltip += $.trim(address.PurchasingContact.Name) + ',\n';
    }
    if (address.ShipTo !== null && address.ShipTo !== undefined && address.ShipTo.length > 0) {
        tooltip += $.trim(address.ShipTo) + ',\n';
    }
    if (address.Line1 !== null && address.Line1 !== undefined && address.Line1.length > 0) {
        tooltip += $.trim(address.Line1) + ',\n';
    }
    if (address.Line2 !== null && address.Line2 !== undefined && address.Line2.length > 0) {
        tooltip += $.trim(address.Line2) + ',\n';
    }
    if (address.City !== null && address.City !== undefined && address.City.length > 0) {
        tooltip += $.trim(address.City) + ',\n';
    }
    if (address.State !== null &&
        address.State !== undefined &&
        address.State.Name !== null &&
        address.State.Name !== undefined &&
        address.State.Name.length > 0) {
        tooltip += $.trim(address.State.Name) + ',\n';
    }
    if (address.ZIP !== null && address.ZIP !== undefined && address.ZIP.length > 0) {
        tooltip += $.trim(address.ZIP) + ',\n';
    }

    //replace trailing comma / line break
    if (tooltip.substr(tooltip.length - 2, 2) === ',\n') {
        tooltip = tooltip.substr(0, tooltip.length - 2);
    }

    return tooltip;
};

As you can probably tell, I'm rather fastidious about ensuring the values are there before concatenating them, as we have had problems in the past with properties coming back as undefined and bringing the whole application crashing to a halt.

However, the cyclomatic complexity of this function is through the roof because of all the null / undefined / length > 0 checks that I'm doing. It is entirely possible that I'm taking things too far.

What I'm wondering is if anybody knows a faster, smarter and tidier way of doing the same thing? Do I really need all these checks? It appears that if I want valid, safe and linted JavaScript, all of them are necessary.

What about this:

var getAddressTooltip = function (address) {

    var tooltip = '';

    if (address.MasterAddress &&
        address.MasterAddress.Id) {
        tooltip += '[' + address.MasterAddress.Id + '] ';
    }
    if (address.PurchasingContact &&
        address.PurchasingContact.Name) {
        tooltip += $.trim(address.PurchasingContact.Name) + ',\n';
    }
    if (address.ShipTo) {
        tooltip += $.trim(address.ShipTo) + ',\n';
    }
    if (address.Line1) {
        tooltip += $.trim(address.Line1) + ',\n';
    }
    if (address.Line2) {
        tooltip += $.trim(address.Line2) + ',\n';
    }
    if (address.City) {
        tooltip += $.trim(address.City) + ',\n';
    }
    if (address.State &&
        address.State.Name) {
        tooltip += $.trim(address.State.Name) + ',\n';
    }
    if (address.ZIP) {
        tooltip += $.trim(address.ZIP) + ',\n';
    }

    //replace trailing comma / line break
    if (tooltip.substr(tooltip.length - 2, 2) === ',\n') {
        tooltip = tooltip.substr(0, tooltip.length - 2);
    }

return tooltip; };

Since JavaScript considers null, undefined, and the empty string to be falsy, the value of the variable itself can be used to substitute for all the tests. Further, as any object is automatically truthy, the tests like address.PurchasingContract will be true if the object is there.

References:

MDN: A Reintroduction to JavaScript (just the first thing I came across)


Simple c++ triple class to mimic pair for use with uniform initialization I'm trying to create a simple triplet class, but the more I look at the implementation of std::pair the more I feel like I'm missing important details.  What I have feels "too simple".  I am aware of std::tuple and other options, but the use case is to be able to use uniform initialization -- so we can make certain assumptions / ignore complications (?)

I am restricted to c++11, none of those beautiful c++14/17 magical unicorns added.

/// keeping class and test in self-contained file {test.cpp}
/// Compile: g++ -o test -std=c++11 test.cpp
#include <iostream>
#include <vector>

/// class definition
template <class T1, class T2, class T3>
struct triple {
    /// q1
    typedef T1 first_type;
    typedef T2 second_type;
    typedef T3 third_type;

    T1 first;
    T2 second;
    T3 third;

    /// q2
    triple() : first(), second(), third() {}

    triple(const T1 &f, const T2 &s, const T3 &t)
        : first(f), second(s), third(t) {}

    /// q3
    template<class U1, class U2, class U3>
    triple(const triple<U1, U2, U3> &t)
        : first(t.first), second(t.second), third(t.third) {}

    triple(const triple<T1, T2, T3> &t) = default;
    triple(triple<T1, T2, T3> &&t) = default;
};

/// simple function to "use" these
void print(const std::vector<triple<int, const std::string, const std::string>> &vals) {
   for(auto &tup : vals) {
        std::cout << "First:  " << tup.first  << std::endl
                  << "Second: " << tup.second << std::endl
                  << "Third:  " << tup.third  << std::endl;
        std::cout << "------------------" << std::endl;
   }
}

/// simple main, goal: uniform initialization
int main(int argc, char **argv) {
    print({
        {1, "hi", "billy"},
        {2, "you", "cool"},
        {3, "no", "not"}
    });
}

q1: the purpose of typedef

I know it's kind of old, but it's easier to explain from this implementation -- I'm looking at the implementation for clang.  Anyway, they all do this

typedef _T1 first_type;    ///<  @c first_type is the first bound type
typedef _T2 second_type;   ///<  @c second_type is the second bound type

and subsequently never use them.  What is the purpose, just to be able to do something like typename std::pair<int, float>::second_type f = 0.012f;?

q2: default constructors

My ultimate purpose is to just enable uniform initialization of something instead of a (large) number of std::make_tuple calls.  In this context, it seems reasonable to include some static_assert for

is_trivially_constructible
is_trivially_default_constructible
is_trivially_move_constructible
is_trivially_destructible

Does adding these make sense or is that over-limiting for no real reason?

q3: reflection construction?

Concerning the constructor with U1, U2, and U3, the purpose of this is to allow for automatic conversions?  Say automatically converting a triple<unsigned int, float, bool> -> triple<size_t, double, int>?

q4: any alarms going off?

Like I said, this is intended to be a simple and non-robust class.  I've been playing around with it, no memory leaks I can find, etc.  The operator overloads would be easy to put in, but other than that is there anything I should be more concerned with?  The stl implementation of pair is much more complex, and being that I ditched std::tuple so that I can do uniform initialization, this makes me think I'm missing something.

While the code you have appears to work as you expect, your stated reason for using it doesn't make sense to me, because one could use std::tuple with uniform initialization as well.

Use std::tuple

With only a relatively minor change to your print routine, the main you have could be used just as easily with std::tuple:

void print(const std::vector<std::tuple<int, const std::string, const std::string>> &vals) {
   for(auto &tup : vals) {
        std::cout << "First:  " << std::get<0>(tup) << std::endl
                  << "Second: " << std::get<1>(tup) << std::endl
                  << "Third:  " << std::get<2>(tup) << std::endl;
        std::cout << "------------------" << std::endl;
   }
}

Answer 1

The reason those typedefs are there is that they're required by the standard.  That way, it's easy to be able to create other variables of the same type as, say, the second argument, without having to jump through a lot of hoops.  In other words, it's a convenience for the user of the template rather than for the author of the template.

Answer 2

I wouldn't bother with static asserts.  If you did want to restrict the kinds of things that can be stored, it's probably better to use std::enable_if.

Answer 3

I'm not sure, but I suspect that the other types U1 and U2 are required for convertable but non-identical classes that might be used to construct the triple (as you've surmised).

Answer 4

Only one alarm -- why aren't you using std::tuple?  :)

Try it live.


Solution to Project Euler problem 5 (LCM of 1 to 20) I'm an experienced C# developer trying to teach myself F#. I spent a day or 3 reading throught the F# wikibook trying to get to know the syntax and F# fundamentals.

As an exercise I'm trying to go through the Project Euler problems to get a better feeling of the syntax and working with the language.

I've just solved problem 5 (finding the least common multiple of 1, 2, 3, …, 20). But I'm not so happy about the hoops I had to jump through to get a data structure that represents my solution.
I've used this blogpost to get to the algorithm for solving the problem.

I was wondering if anyone could give me some pointers as to how this code could be improved? My guess is that the inherent immutability of F# values is causing me to have to perform a lot of steps to get the exact data I want...

This is my code:

let main argv =
//calculates the prime factors of a number
let findPrimeFactors x =
    let primes = [|2I;3I;5I;7I;11I;13I;17I;19I|]
    let rec loop acc counter = function
        | x when x = 1I -> failwith "A PRIME IS BY DEFINITION GREATER THAN 1"
        | x when primes |> Array.contains x -> x :: acc
        | x when counter = primes.Length -> failwith "MY LIST OF KNOWN PRIMES IS NOT BIG ENOUGH"
        | x when x%primes.[counter]=0I-> loop (primes.[counter]::acc) (counter) (x/primes.[counter])
        | x -> loop acc (counter + 1) x

    let primeFactor = loop [] 0 x |> List.rev
    primeFactor

//calculates the prime factors for each of the numbers between 2 and n
//then, for each of the prime factorizations it tries to find the highest power for each occurring prime factor
let findPrimeFactorsPowers n =
    //builds a map of all the prime factor powers for all prime factorizations
    let rec addCounterFactorPowers factorPowers = function
        | counter when counter = n -> factorPowers
        | (counter : int) -> addCounterFactorPowers ((findPrimeFactors (counter|>bigint) |> List.countBy (fun x-> x)) @ factorPowers) (counter + 1)
    let allFactorPowers = addCounterFactorPowers [] 2
    //group all the powers per prime factor
    let groupedFactorPowers = allFactorPowers |> List.groupBy (fun (factor, power) -> factor)
    //get the highest power per prime factor
    let maxFactorPowers = groupedFactorPowers |> List.map (fun (key, powers) -> (key, powers |> List.map (fun (factor, power) -> power) |> List.max))

    //return the result
    maxFactorPowers

let n = 20;
let primeFactorSet = findPrimeFactorsPowers n
printfn "%A" primeFactorSet

let smallestNumberDivisableByAllNumbersBelown =  (primeFactorSet |> List.fold (fun state (factor, power) -> state * pown factor power) 1I)
printfn "Result %A" smallestNumberDivisableByAllNumbersBelown

System.Console.ReadKey(true)|>ignore
0 // return an integer exit code

Let's start with the loop inside findPrimeFactors. You have the right idea, but it's complicated because you're flattening two loops into one: looping over the primes, and looping over the factors for your target number.

So let's do those one at a time. First, let's create a function that finds the next prime factor:

let rec findNextFactor candidates n =
    match candidates with
    | x::xs when n%x=0I -> x
    | x::xs -> findNextFactor xs n
    | _ -> failwith "No factor found"

The first line matches when a factor is found, the second when no factor is found, and the third when the collection is empty. Note how matching on the collection makes it very easy to recursively loop through it by matching against x:xs. This helps avoid your issue of looping by index.

Now that we know how to find the next prime factor, it's easy to find all of them:

let rec loop acc n =
    match findNextFactor primes n with
    | n -> n:acc
    | p -> findPrimeFactors p:acc n/p

We're not passing primes in here as we're not iterating it here. The primes referred to is the outer one which always contains all the primes up to 20. By splitting out the loop, we only need to deal with these two simple cases.

Just glue those together and you get your findPrimeFactors implementation.

For findPrimeFactorsPowers, you're again increasing the complexity by completely changing your data type, from a list of factors, to a map of factors to the count of those factors. This isn't actually that hard. For a single number n it might look like:

let findPrimeFactorsPowers n =
    findPrimeFactors n
    |> Seq.groupBy i -> i
    |> Seq.map i -> (i.Key, i.Count)

I'm honestly not quite sure what your version is doing, so I can't compare very closely.

But actually, there's something even simpler we can do. Really all we want to do is merge two lists in a special way: for each value, the number of times that value appears in the merged list should equal the maximum number of times it appears in each individual list.

By taking advantage of the fact that our lists are sorted, we can do that very easily:

let rec merge a b acc =
match a,b with
| x::xs, y::ys when x = y -> merge xs ys (x::acc)
| x::xs, y::ys when x < y -> merge xs b (x::acc)
| x::xs, y::ys -> merge a ys (y::acc)
| _, [] -> List.append (List.rev acc) a
| [], _ -> List.append (List.rev acc) b

We loop over the lists, take the head of each one. If they're equal, remove both heads and add it to the accumulator, otherwise take just the lowest and add that. Finally the last lines let us terminate once a list is empty.

This is the last piece we need. Now we just need to get the prime factor lists for each number under 20, and merge each one in turn into an empty list. Finally find the product of the numbers in the resultant list, and you have your answer.

For more general comments, there isn't much I can find to fault. There's a couple of times you declare a variable, then only use it once in the next line. Not doing that means you can get rid of the cumbersome name smallestNumberDivisableByAllNumbersBelown.

Also, you should probably trim out some unnecessary comments. If you're just learning F#, the idea that the last expression of a function is what gets returned might cause enough cognitive friction for you that you want to leave yourself a comment, but those shouldn't be in part of a finished piece of code.

Generally, though, stylistically the code is good, including both naming and organization.


Is this a valid and correct skeleton class/struct I have been watching Alexander Stepanov programming C++ with components and he, as usual, has some strong opinions. I did like one of his assertions that we use regular, semi-regular and fully ordered types. So I decided to pull together a skeleton class (pair in this case) and wonder if there are any valid reasons for using this skeleton or something different. I realise there is no silver bullet and am not looking for one, but I am interested in the basics being programmatically consistent and correct. Alex seemed to loosely suggest that such a start position would be valid.

I hope this is a valid question, I think some of the areas of concern are:

Is it valid to refer to class as regular/semiregular/fullyordered?
(It's hard to find examples.)
Are there strong reasons for non-member friend/non-friend member functions? (I'm thinking here of the Dr.
Dobbs article showing non-member (non-friend) functions improving encapsulation.)
Should friend requirements define member/non member functions?
Is it a good rule that each of these groups are implemented fully if at least one member of that group is? I.e. == must also be implemented with != etc. Sort of as an extension to the big 4 or 5 or 6 construction or regular methods :-)

This is C++11 that MSVC 2012 can handle. Clang, gcc etc. can improve this loads, but for now I am using MSVC 2012 as the baseline. friend is not required in these functions. I have included it to try and cover a normal "template" (if I create a template/snippet) for use where there are private data members. My intent is not to use this as a snippet, though.

#ifndef MY_CLASS_H
#define MY_CLASS_H

#include <tuple>
#include <utility>

template <typename T, typename U>
struct MyClass {
    T first;
    U second;

    // semiregular

    MyClass(const MyClass& other)
        : first(other.first),
          second(other.second)
    {}

    MyClass(MyClass&& other)
        : first(std::move(other.first)),
          second(std::move(other.second))
    {}

    MyClass()
        : first(),
          second()
    {}

    ~MyClass()
    {}

    MyClass& operator=(MyClass other) {
        swap(*this, other);
        return *this;
    }

    // regular
    friend
    bool operator==(const MyClass& lhs, const MyClass& rhs) {
        return std::tie(lhs.first, lhs.second)
                   == std::tie(rhs.first, rhs.second);
    }

    friend
    bool operator!=(const MyClass& lhs, const MyClass& rhs) {
        return !operator==(lhs, rhs);
    }

    // fully ordered
    friend
    bool operator<(const MyClass& lhs, const MyClass& rhs) {
        return std::tie(lhs.first, lhs.second)
                   < std::tie(rhs.first, rhs.second);
    }

    friend
    bool operator>(const MyClass& lhs, const MyClass& rhs) {
        return operator<(rhs, lhs);
    }

    friend
    bool operator<=(const MyClass& lhs, const MyClass& rhs) {
        return !operator>(lhs, rhs);
    }

    friend
    bool operator>=(const MyClass& lhs, const MyClass& rhs) {
        return !operator<(lhs, rhs);
    }
};

// swap
void swap(MyClass& lhs, MyClass& rhs) /* noexcept */ {
    using std::swap;
    swap(lhs.first, rhs.first);
    swap(lhs.second, rhs.second);
}

#endif // MY_CLASS_H

It is kind of hard to tell what you're really asking. Without knowing what you're asking, it's hard to provide a helpful and meaningful answer. I will try regardless. Feel free to point me in the right direction if I'm missing the point.

  Is it valid to refer to class as regular/semiregular/fully ordered?

While I'm not familiar with the terminology, the distinctions are useful. These classifications let you reason about a class. For example, std::list<T>::sort() requires that operator< is defined for T. This is part of what Java and C# interfaces and the suggested C++ concepts feature is about. Note that they aren't necessarily accumulative: A type can be "fully ordered" without being "semi-regular".

However, I can't see a good reason for sorting types into only these categories. Stepanov may have some deeper insight here.

  Are there strong reasons for non-member friend/non-friend member functions?

Like Scott Meyers points out in the article you link to, non-member non-friendfunctions improve encapsulation, which leads to looser coupling. (Looser coupling in turn leads to better complexity handling, replaceable components, more testable interfaces and so on. These are good things.)

Regarding non-member friend functions, I personally consider them a code smell. It doesn't necessarily mean that the code is dirty, but in most cases you don't want friends. (Even without friends, you can still throw party;!) Declaring a friend breaks encapsulation and leads to tighter coupling, which is the opposite of what we want.

(If you do have friends, make sure they interact with an interface (i.e. member function) and not directly with private variables/implementation details.)

  Should friend requirements define member/non-member functions?

I am reading the question as: "Should functions that need to be declared friend be member functions instead?"

For functions that are not part of the class' public interface, the answer is yes, definitely. They should be private member functions.

For functions that are part of the public interface: Unless you have a specific reason to make that function non-member, it should be a member function. The point Scott Meyers is making, however, is this: If the function can use the class' public interface instead of being non-member friend or a member function, it should do so: It should be a non-member, non-friend function that interacts with the type through the type's public interface.

My basic guideline is this: Can the function make use of only the public interface? Then make it non-member, non-friend. If not, make it a member function. However, don't extend the public interface more than otherwise necessary just to allow a function to be non-member, non-friend. If you have to extend the public interface for just a single function, then that function should probably be a member function instead.

  Is it a good rule that each of these groups are implemented fully if at least one member of that group is?

When defining one of assignment operator, destructor or copy constructor, the Rule of Three comes into play and you should probably define the others.

For the categories Stepanov talks about: Yes, if you implement one part of the groups of operators, you should probably implement the others. This is because you should follow the Principle of Least Astonishment. Users of a type expect that if you can say a == b, then you should be able to say a != b as well.

The exception is what he refers to as regular: A type that is constructable may not need to be copyable or movable.


Two script completing the same task, which is the best way? I have the following two methods of completing the same task, however I'd like to know which is the preferred method and WHY? Or if there are any suggestions to complete the task in an even better, cleaner way.

The task

Find part 1 of $string in the example $string = '1/2/3/4'; so the first part would be '1'.

Match that value to the corresponding 'id' from entries in the database, then return the 'filename' that corresponds to the 'id'.

Then loop again until each part of the initial string has a corresponding value.

The 'filename' values are each then output to a string delimited by a '/'.

EG. filename1/filename2/filename3/filename4

Option 1

$string = '1/2/3/4';

$change = str_replace('/',',',$string);

$result = $connection->query("SELECT id, filename FROM pages WHERE id IN($change) ORDER BY FIELD($change)");

while($row = $result->fetch_assoc()) {
    $match[] = $row;
}

foreach ($match as $array) {
    echo $array['filename'].'/';
}

UPDATE
This works fine if there are more than one parts to the string eg. '1/2/3/4' but I get the following error if just '1' is used.

Fatal error: Call to a member function fetch_assoc() on a non-object in C:\xampp\htdocs\admin\pages\test2.php on line 22

OR

Option 2

$string = '1/2/3/4';

$split = explode('/', $string);

$split = array_flip($split);

$result = $connection->query("SELECT id, filename FROM pages");

while ($row = $result->fetch_assoc()) {
    $new_array[$row['id']] = $row['filename'];
}

$matched = array_intersect_key($new_array, $split);

$join = implode('/', $matched); // join $matched as string.

echo $join;

$string = '1/2/3/4';

$change = str_replace('/',',',$string);

$result = $connection->query("SELECT id, filename FROM pages WHERE id IN($change) ORDER BY FIELD(id,$change)");

$new_string = "";
while($row = $result->fetch_assoc()) {
     $new_string .= $row['filename']."/";
}

$new_string = substr($new_string,0,-1);

echo $new_string;

Less looping - the match[] = $row was unnecessary IMO because you want a string back only...so you concatenate the file name with a / delimiter and then the last slash is removed with sub_str.

Updated the fixes including single query match.


iOS4 alternatives for dismissViewControllerAnimated:completion: My inspiration for creating this protocol came from iOS 5's "dismissViewControllerAnimated:completion:" addition to UIViewController.  I wanted this functionality in iOS 4.3.
I find using the modal view's -viewDidDissapear to invoke methods on the presenting view controller work's very well.  One benefit is the ability to delay deallocation on the modal view controller.  Please post any improvements I can make.

PresentorDelegateProtocol.h

@protocol PresentorDelegateProtocol <NSObject>
@optional

/*

Extra protocol methods defined in protocol for flexibility.
Main methods are:
- (void)dismissPresentingModalViewController:(id)modalView animated:(BOOL)animated;
- (void)modalViewDissapeared:(id)modalView;  //used in modal view's -viewDidDissapear

*/

- (void)dismissPresentingModalViewController:(id)modalView animated:(BOOL)animated;
- (void)modalViewDissapeared:(id)modalView;

// use the block in this method send messages to save state, etc.  This is the one I like to use.
- (void)dismissPresentingModalViewController:(id)modalView animated:(BOOL)animated withBlock:(void(^)())block;

// use in other classes that are not controlling dismissal of the modal view
- (void)executeBlockOnModalDissapearance: (void(^)())block;

@end

PresentingViewController.h

#import "PresentorDelegateProtocol.h"
@interface PresentingViewController : UIViewController <PresentorDelegateProtocol>
- (void)showModalVC;
@end

ModalViewController.h

#import "PresentorDelegateProtocol.h"
@interface ModalViewController : UIViewController
@property (nonatomic, assign) id <PresentorDelegateProtocol> presentorDelegate;
- (void)close;
@end

PresentingViewController.m

#import "PresentingViewController.h"
#import "ModalViewController.h"
@implementation PresentingModalViewController
- (void)showModalVC
{
    ModalViewController *modalVC = [[ModalViewController alloc] initWithNibName:@"ModalViewController" bundle:nil];
    modalVC.presentorDelegate = self;
    [self presentModalViewController:modalVC animated:YES];
}
- (void)dismissPresentingModalViewController:(id)modalView animated:(BOOL)animated
{
    if ([modalView isKindOfClass:[ModalViewController class]]) {
        NSLog(@"Can invoke based on class");
    }
    [self dismissModalViewControllerAnimated:animated];
}
- (void)dismissPresentingModalViewController:(id)modalView animated:(BOOL)animated withBlock:(void(^)())block
{
    block();
    /* execute block before or after calling to dismiss modal view */
    [self dismissPresentingModalViewController:modalView animated:animated];
    //block();
}
- (void)modalViewDissapeared:(id)modalView
{
    if ([modalView isKindOfClass:[ModalViewController class]]) {
        NSLog(@"Do stuff based on class.");
    }
}
- (void)executeBlockOnModalDissapearance: (void(^)())block
{
    block();
    NSLog(@"This delay's dealloc on modal view until block completes");
}
@end

ModalViewController.m

#import "ModalViewController.h"
@implementation ModalViewController
@synthesize presentorDelegate;

- (void)close
{
    if (1 == 0 /*need to do something before dealloc*/){
        [self.presentorDelegate dismissPresentingModalViewController:self animated:YES withBlock:^{
            NSLog(@"Do stuff with block.  Save, animate, etc");
        }];

    } else {
        [self.presentorDelegate dismissPresentingModalViewController:self animated:YES];
    }
}

- (void)viewDidDisappear:(BOOL)animated
{
    if (1 == 0 /*stuff to do*/){
        [self.presentorDelegate executeBlockOnModalDissapearance:^{
        // do stuff before modal view is deallocated
        }];
    }
    [self.presentorDelegate modalViewDissapeared:self];

    presentorDelegate = nil;
    [super viewDidDisappear:animated];
}
@end;

Two light suggestions:

disappear has one s and two ps.
the word "block" to describe a block is accurate but not very descriptive. OK, so I need to pass a block, but I can see that from the parameter list. What the block is for should be explained in the method name: this is why Apple often uses phrases like "completion handler" or "comparator" to describe block parameters.


Loading JavaScript files asynchronously I need to get some scripts from the different social networking site and cache them.  I want the function to be asynchronous, and non-blocking.

I have settled with a script, but I am not really sure if it is not overly complicated to do the job.

I did replace a normal for loop, and this method is much faster. I do not know why, but it just is.

// Cached scritps
var cachedScriptPromises = {};

// Location of the scripts
var Scripts : {
    twitter    : '//platform.twitter.com/widgets.js',
    facebook   : 'http://static.ak.fbcdn.net/connect.php/js/FB.Share',
    googleplus : 'https://apis.google.com/js/plusone.js',
    linkedin   : 'http://platform.linkedin.com/in.js'
};

// When the button is clicked all the scripts
// are loaded. The conainers are already
// on the website. They will be filled
// as soon as the script is loaded.
jQuery('a.share-link').click(function(){
    for( var script in Scripts ){
        jQuery.cachedGetScript( Scripts[script] );
    }
});

// Function that get and caches the scripts
jQuery.cachedGetScript = function( script ) {
    if ( !cachedScriptPromises[ script ] ) {
        cachedScriptPromises[ script ] = jQuery.Deferred(function( defer ) {
            jQuery.getScript( script ).then( defer.resolve, defer.reject );
        }).promise();
    }
    return cachedScriptPromises[ script ];
};

Eventually this function can be expanded by adding some before and after actions. The thing is that the files are loaded so quickly that I have removed these features, but I am planning to do this as soon as the project becomes bigger.

Does anyone have any suggestions on making this better?  Maybe some adjustments to the code, or go a complete other way with this.

I believe you don't need to use jquery deferred objects for this task. Just use script tags with sources - they will be downloaded in parallel from CDNs (try to use CDN, its always a good idea). The amount of parallel requests will be limited by browser restrictions.

Since your code just replaces default browser technique, for more sophisticated features (like dependent components, libraries that you really want to defer, because they are not used until some action on the page), use

http://jsload.net/
http://developer.yahoo.com/yui/get/
deferred objects as you did


Python 3 - Fibonacci Implementation I wrote a function returning the n-th Fibonacci number in Python 3:

# Functions returns the nth Fibonacci number F_n
# Parameters: n >= 0, (Optional: F_0, F_1)
def fibonacci(n, f0=0, f1=1):
    if n == 0:
        return f0

    while n > 1:
        f0, f1 = f1, f0 + f1
        n -= 1

    return f1

Here are a few things I noticed and want to get a review on:

Is the notation clear? The functions returns the n-th Fibonacci number, but n=0 returns the first Fibonacci number (this is a special case)
I'm new to Python and any general style guide is welcome.
Performance: The function isn't intended to be used under incredible performance pressure, but I always try to look what's at the horizon. This implementation should run way faster than an recursive (or even memoized recursive) implementation, but what can be done to improve performance? I tried using the explicit formula including the golden ratio, but accuracy lost in using floating point numbers resulted in wrong return values after some n.

I don't see why you need to special case n == 0. The

def fib(....):
    for _ in range(n):
        f0, f1 = f1, f0 + f1
    return f0

seems cleaner, even though it does one extra addition.
The Fibonacci are well defined in the negative domain as well. For example, \$F_{-1}\$ can be derived from the same recurrence (\$F_1 = F_0 + F_{-1}\$), etc, so handling negative n in this particular problem is a bit trickier.
Performance wise, yes it is possible to compute the nth number in \$O(\log{n})\$ time, either by matrix exponentiation, or by exploiting the

\$F_{2n-1}=F_{n}^{2}+F_{n-1}^{2}\\F_{2n}=(F_{n-1}+F_{n+1})F_{n}\$

identities.


A directive allowing to make a whole DOM subtree readonly What do you think about this directive? For each input, it traverses the DOM towards the root and if it finds an element with the class readonly, it makes the input readonly.
An ng-readonly directive on the input itself get honored: The input becomes readonly whenever ng-readonly evaluates to true or any enclosing element has the class readonly.
.directive("input", function($parse) {
    return {
        restrict: "E",
        link: function($scope, element, attr) {
            if (attr.type === "radio" || attr.type === "checkbox") {
              return; // for simplicity, let's ignore them
            }

            var org = attr.ngReadonly;
            if (!org && attr.readonly) {
              return; // readonly seems to be set manually, so let's not touch it
            }

            $scope.$watch(function() {
                var readonly = $parse(org)($scope);
                for (var e = element; e.length && !readonly; e=e.parent()) {
                    readonly = e.hasClass("readonly");
                }
                console.log(element.attr("ng-model"), readonly)
                attr.$set("readonly", !!readonly);
            });
        },
    };
})

The purpose is to allow to make a whole subtree readonly, no matter what's inside. It works, but I can imagine there can be performance problems and/or interferences with ng-readonly setting the value independently of this directive.
An explanation why I'm not simply binding to a scope variable
While I really appreciate Thomas' answer, I disagree with this part:

Again, I highly recommend that you simply bind to a scope variable instead of checking for the presence of a class further up the tree.

I really find it both complicated and error-prone: I'm having a big form in which many fields already have their readonly logic (e.g., bank name is read only when a known BIC is entered). Now, I'd have to add is_readonly to all existing ng-readonly directives and add the directive to every input field missing it.
Moreover, the form includes some partials, which have no idea about the outer scope. Again, I'd have to add change the partial as above and make it to get is_readonly from the outer scope.

Generally speaking, you should avoid looking outside the current directive for anything that would affect your internal state (bar events). The preferred method of communication is scopes. Here is how I would recommend you do this.

        <input ng-readonly="is_readonly" />

            <input ng-readonly="is_readonly" />

Simply toggling $scope.is_readonly from MyCtrl is enough to disable all child inputs that have ng-readonly="is_readonly", and it's bindable too, so you can add/remove readonly status at runtime.

Now, if there is a genuine need to have the behaviour you described, then you have 2 options.

Stick with your current implementation, but this will not be performant, since your DOM traversal will fire any time the scope changes, for any reason, anywhere up the scope tree.
Rewrite to use directive's require property.

Here is how you would do the latter.

.directive("readonly", function() {
    return {
        restrict: "C",
        link: function() {},
        controller: function() {},
    }
})
.directive("input", function()
    return {
        restrict: "E",
        // search for optional parent directive named `readonly`
        require: "^?readonly",
        // 4th parameter is `readonly` controller or null.
        link: function($scope, element, attr, has_readonly) {
            var org = attr.ngReadonly;
            if (org) {
                return; // readonly seems to be set manually, so let's not touch it
            } else if (has_readonly) {
                attr.$set("readonly", true)
            }
        },
    };
})

This tells angular that it should pass you a reference to the readonly controller if found, or null otherwise. In this way, you can tell at link time whether the parent node exists. This saves you from performing the DOM traversal yourself, and should be faster too.

Again, I highly recommend that you simply bind to a scope variable instead of checking for the presence of a class further up the tree.


Sonar: Move constants to a class or enum I found one project on Internet and after I downloaded it, I got this message from SonarQube:

Move constants to a class or enum.

But, when I downloaded SonarLint, it did not report any code smell.
This is the code:
@Repository
public interface TaskRepository extends JpaRepository<Task, Long> {

    String QUERY = "update Task t set t.isDeleted = true where t.id = :taskId";

    @Modifying
    @Query(QUERY)
    void deleteTaskById(@Param(value = "taskId") long taskId);
}

My question is: Should I remove constant QUERY and write it directly into @Query or I can leave it like this?
Note: This is a short query and it can easily be put inside @Query. But I am also interested what would be the best practice if this query was longer(ex. if it had 300 characters or more).

Personally, I do not see the reason why this needs to be a variable in the first place. If I were you I would either move this into the @Query annotation or better yet I would create a @NamedQuery under your Task entity class.
Also on a side note, I would avoid naming the repository method deleteTaskById as this name implies that a matching object will be deleted from the database. Since this is not the case here, I would rename the method to markTaskAsDeleted to avoid ambiguity.


Find the maximum possible summation of differences of consecutive elements Array A contains the elements, \$A_1,A_2, \ldots, A_N\$.
   And array B contains the elements, \$B_1,B_2, \ldots, B_N\$.

  There is a relationship between \$A_i\$ and \$B_i\$: any element \$A_i\$ lies between \$1\$ and \$B_i\$.

  Let the cost S of an array A be defined as:

  $$ S = \sum_{i=2}^N \lvert A_i - A_{i-1} \rvert$$

  Given B, find and print the largest possible value of S.

The problem can be found here

Example:

size of array:5
array: 10 1 10 1 10
output : 36
(since the max value can be derived as |10 - 1| + |1 - 10| + |10 - 1| + |1 - 10|)

Approach:
The only approach I could think of was brute force. I thought I would make an overlapping recursive equation so that I could memoize it, but was not able to.

CODE:

public static void func(int pos,int[] arr,int[] aux,int n)
    {
        /*
         * pos is current index in the arr
         * arr is array
         * aux is temp array which will store one possible combination.
         * n is size of the array.
         * */

        //if reached at the end, check the summation of differences
        if(pos == n)
        {
            long sum = 0;
            for(int i = 1 ; i < n ; i++)
            {
                //System.out.print("i = " + i + ", arr[i] = " + aux[i] + " ");
                sum += Math.abs(aux[i] - aux[i - 1]);
            }
            //System.out.println();
            //System.out.println("sum = " + sum);
            if(sum > max)
            {
                max = sum;
            }
            return;
        }

        //else try every combination possible.
        for(int i = 1 ; i <= arr[pos] ; i++)
        {
            aux[pos] = i;
            func(pos + 1,arr,aux,n);
        }
}

NOTE:
The complexity of this is \$\mathcal{O}(n*2^n)\$

The above can be optimized by using two things.

Memorizing the result of the func().  To implement memorizing you need to send some response from the func which can be memorized for certain parameters. In your cases it wouldn't be possible. You need to change the func defination to implement memorizing.
Pre-processing on array B to calculate minimum till any i and maximum till any i.

  Explaining 2nd point in detail, at any point of three possible
  combination of \$A_{i-1}, A_i\$ and \$A_{i+1}\$

  \$A_i > A_{i-1}, A_{i+1}\$, In this case it makes sense to choose \$A_i\$ as the max value in B till i

  \$A_i < A_{i-1},A_{i+1}\$, In this case it makes sense to choose \$A_i\$ as the min value in B till i

  \$A_{i-1} < A_i < A_{i+1}\$, In this case you can see \$A_i\$ would have any involvement in final sum. It is getting cancelled out. So even if we choose \$A_{i-1}\$ or \$A_{i+1}\$ as \$A_i\$ it will also give same result.
  Hence it makes sense to pick either min or max value at any point from B.

You can try below solution with both of the above optimization.

possible_sum[][] = new Integer[n][2] // An array which memorize maximum possible sum till any n
initially possible_sum to some default value '-1'

int find_possible_sum(int i, int next, int minORmax){
    if(possible_sum[i][minORmax] != -1)return possible_sum[i][minORmax];
    possible_sum[i][minORmax] =  max(find_possible_sum(i-1, max_till_i, 0) + abs(max_till_i - next), find_possible_sum(i-1, minTill_i, 1)) + abs(min_till_i - next));
   // max_till_i and min_till_i can obtained in O(n) during pre-calculation
   return possible_sum[i][minORmax];
}

int max_value = -1 , min_value = MAX_VALUE;
   int min_till_i[], max_till_i[];
   for(int i=1;i<=n;i++){
        min_till_i[i] = min(B[i],min_value);
        min_value = min(min_value, B[i]);

        max_till_i[i] = max(B[i],max_value);
        max_value = min(max_value, B[i]);
    }

Total complexility of this solution should be around: \$O(n)\$ + \$O(2*n)\$


Stateful User tracking I have a class that takes a string or an integer, which stand for username or user id in the database.  I am not comfortable with my constructor taking "a string or an integer" and want to modify it to be more type-specific.  But how do I do so?

class User
{

    /**
     *
     * @var string|int
     */
    private $id;

    /**
     *
     * @var array
     */
    private $udata;

    /**
     *
     * @param string|int $var
     */
    function __construct($var)
    {
        $this->id = 0;
        $this->lookup($var);
    }

    /**
     *
     * @param string|int $var
     */
    function lookup($var)
    {
        $sql = "SELECT * FROM user WHERE "
            . (is_numeric($var) ? 'id' : 'username') . " = " . db_input($var);
        $row = query($sql);
        $this->udata = $row;
        $this->id = $row['id'];
        return $this->id;
    }
}

/**
 * Uses
 */
$user = new User(51);
$user = new User("JohnDoe");

Create static functions findById and findByUsername and clean-up User constructor or modify it to accept $id and $udata; you can also make it private, if you don't need it outside.

class User {
    // ...

    public static function findById (id) {
        // TODO
    }

    public static function findByName (name) {
        // TODO
    }
}

$user = User::findById(51)
$user2 = User::findByName("JohnDoe")


Secure login and authentication in PHP This seems to work for me, but I want to make it more secure. I want to see how far I can go without pre-built packages/bundles, so please don't suggest any.

How secure is this? What are some steps I can take to improve it?

In a controller:

public function loginUser($request, $response) {
    if (Security::isUser()) { return $this->redirect('home'); }
    $username = $request->getParam('username');
    $password = $request->getParam('password');
    $user = Security::authenticate($username, $password);
    if ($user) {
        Security::login($user->id);
        return $this->redirect('home');
    } else {
        $this->flash->addMessage('error', 'Invalid username and/or password.');
        return $this->redirect('login');
    }
}

Here is part of the Security class:

public static function hash($string) {
    return password_hash($string, PASSWORD_DEFAULT);
}

public static function authenticate($username, $password) {
    $user = User::where('username', $username)->orWhere('email', $username)->first();
    if (password_verify($password, $user->password) && $user->banned === 0) { return $user; }
    else { return false; }
}

public static function getUser() {
    if (isset($_SESSION['uid'])) {
        $user =  User::where('id', $_SESSION['uid'])->first();
        if ($user->banned === 1) { return false; }
        else { return $user; }
    } else { return false; }
}

public static function isUser() {
    if (!self::getUser()) { return false; } else { return true; }
}

public static function isAdmin() {
    $user = self::getUser();
    if (!$user) { return false; }
    else if ($user->role === 'admin') { return true; }
    else { return false; }
}

public static function login($uid) {
    session_regenerate_id();
    $_SESSION['uid'] = $uid;
}

public static function logout() {
    session_unset();
    session_destroy();
}

Then, to secure something:

public function loginUser($request, $response) {
    if (!Security::isUser()) { // or use ::isAdmin()
        // deny access
    }
    // allow access
}

Csrf tokens are checked by Slim's Guard. This is all over https.

This isn't a matter of security, but mashing things onto a single line

else { return $user; }

is considered poor form in most style guidelines.

public static function isUser() {
    if (!self::getUser()) { return false; } else { return true; }
}

reads a bit more naturally if you don't use an inverted condition:

public static function isUser() {
    if (self::getUser()) { return true; } else { return false; }
}

And then it's easy to see that you can simplify it to just

public static function isUser() {
    return (bool)(self::getUser());
}

Similarly,

else if ($user->role === 'admin') { return true; }
else { return false; }

can be simplified to

return $user->role === 'admin';

$user = User::where('username', $username)->orWhere('email', $username)->first();

is a bit strange.  Users can log in via either email or username?  Are they both unique?  It will simplify your life (and theirs) a lot if there's only one unique identifier for each user.

You're using password_hash and password_verify, which is good.  You may want to consider setting the cost parameter to password_hash to something other than the default to get something that makes sense for your hardware.


Display user detail utility I currently have a utility that will display details about a list of specified users. In its current state I don't use any functions, which I've noticed make it a little more repetitive than I would like.
I'm specifically interested in how to make this more functional, but I'd be interested in any suggestions for how to make this better.
Details.py
class userdetails:

    def __init__(self, Userdetails, Education, Work_history, Bank_history,
    Credit_history):

        self.Userdetails = Userdetails
        self.Education = Education
        self.Work_history = Work_history
        self.Bank_history = Bank_history

My other file
from Details import userdetails

pin_no = ('1111', '2222', '3333')

while True:

   pin_no = input("Input the no : ")

   if pin_no == '1111' or pin_no == '2222' or pin_no == '3333':

       print ("\n Hello and welcome to my program.  Please choose from one of the following
options:")

       break

else:

    print ("please try again ")

Tom_Watts = userdetails ('Tom Watts \n' , '\nSidney Stringer School \n\n' 'GCSE English: Grade A \n'
'GCSE Maths:   Grade B \n' 'GCSE Physics: Grade A \n', 'GSA LTD', 'Barclays Bank', 'Good')

Bill_Gates = userdetails ('Bill Gates \n ', '\nBinley Woods School \n\n' 'GCSE Maths: Grade A \n',
'MI6', 'Bank of England', 'Good' )

Steve_McQueen = userdetails ('Steve McQueen \n', '\nArizona High School \n\n' 'GCSE English: Grade A
\n' 'GCSE Maths:   Grade A \n', 'GSA LTD', 'Lloyds Bank ', 'Average')

dict = {

    '1111': Tom_Watts,
    '2222': Bill_Gates,
    '3333': Steve_McQueen,

}

user =  input("\n\n 1. Userdetails \n 2. Education \n 3. Work History \n 4. Bank History \n" ' 5.
Credit History \n\n  '  )

a = '1'
b = '2'
c = '3'
d = '4'
e = '5'

if user == a:

    print (dict[pin_no].Userdetails)

elif user == b:

    print (dict[pin_no].Education)

elif user == c:

    print (dict[pin_no].Work_history)

elif user == d:

    print (dict[pin_no].Bank_history)

elif user == e:

    print (dict[pin_no].Credit_history)

The code could be simpler and more Pythonic. For example:
pin_no = ('1111', '2222', '3333')

while True:

   pin_no = input("Input the no : ")

   if pin_no == '1111' or pin_no == '2222' or pin_no == '3333':

could simply be:
valid_pins = ('1111', '2222', '3333')
pin_no = input("Input the no : ")
if pin_no in valid_pins:

Thus avoiding repetition.
This code:
user =  input("\n\n 1. Userdetails \n 2. Education \n 3. Work History \n 4. Bank History \n" ' 5.
Credit History \n\n  '  )

could become:
str = ("Userdetails", "Education", "Work History", "Credit History")
for num, value in enumerate(str, start=1):
    print(f"{num}) {value}")

user = input("Choose an option: ")

which gets you output like this: (note that I use enumerate for automatic line numbering)

1) Userdetails
2) Education
3) Work History
4) Credit History
Choose an option:

The problem with this code is that your data is polluted with linebreaks. Just don't. The formatting can be done in your code. For example the join function can easily break down a tuple:
print("\n".join(str))

Userdetails
Education
Work History
Credit History

As it stands your class is underutilized. Instead of this:
Steve_McQueen = userdetails ('Steve McQueen \n', '\nArizona High School \n\n' 'GCSE English: Grade A
\n' 'GCSE Maths:   Grade A \n', 'GSA LTD', 'Lloyds Bank ', 'Average')

I would strongly advise to use keywords arguments like this:
Steve_McQueen = userdetails (education='Arizona High School', work_history='GSA LTD', bank_history='Lloyds Bank')

First of all, it is more clear what the values relate to. And then you can provide arguments in the order you want (so your code won't break if you insert new arguments in the future, or change their order).
Reference: Positional and Keyword Arguments
Note that by convention Python variables should be lowercase.
I am not sure the class is beneficial here. You could simply make up a dict of all users. That depends on the ultimate purpose. As an example:
people = {
    '1111': {
        'name': 'Tom Watts',
        'education': 'Sidney Stringer School',
        'grade': 'GCSE English: Grade A'
    },
    '2222': {
        'name': 'Bill Gates',
        'education': 'Binley Woods School',
        'grade': 'GCSE English: Grade A'
    },
    '3333': {
        'name': 'Steve McQueen',
        'education': 'Arizona High School',
        'grade': 'GCSE Maths:   Grade A'
    }
}

Then:
>>> people['2222']['name']
'Bill Gates'
>>> people['2222']['education']
'Binley Woods School'

And then, provided that your dict is defined beforehand, the list of valid 'pins' can be built automatically:
# list comprehension
valid_pins = [k for k in people.keys()]

# what the list comprehension returns:
[k for k in people.keys()]
['1111', '2222', '3333']

No need for hardcoding numbers.
Thus, you could do something like this:
# list comprehension: get the list of valid pins from dict 'people' defined above
valid_pins = [k for k in people.keys()]

while True:
    pin_no = input("Input the no : ")
    if pin_no not in valid_pins:
        print ("please try again ")
    else:
        # show details of matching user
        print(people[pin_no])


Starting a fire and forget async method Sonar/FxCop are telling us that we shouldn't use void async methods. That's ok.
This is the current implementation:

private async void InitMethod(ServiceControl serviceControl)
{
    if (serviceControl != null)
    {
        await Task.Factory.StartNew(() => serviceControl.Execute());
    }
}

// fire and forget ... calling from a non-async method!
InitMethod(serviceControl);

Is this a good way or how would you refactor this method?

private async Task InitMethod(ServiceControl serviceControl)
{
    if (serviceControl != null)
    {
        await Task.Factory.StartNew(() => serviceControl.Execute());
    }
}

// fire and forget ... calling from a non-async method!
// when calling without Start then VS is complaining about "not awaiting" this method
InitMethod(serviceControl).Start();

Just fire the task without async/await.

private void InitMethod(ServiceControl serviceControl)
{
    if (serviceControl != null)
    {
        Task.Factory.StartNew(() => serviceControl.Execute());
    }
}

In a proper F&F task all exception handling (including a final catch, logging, notifications) is done by the task itself, so you don't need exception handling that async/await provides.

Make the method name reflect what it does. I wouldn't mind a name like FireAndForgetXyz


Mutations and Combinations I have recently answered one question on Stack Overflow.

Problem statement:

  Input sequence:

PEACCEL

  User mutation input file:

E2R C4W E6G

  The algorithm is for an input sequence of, say, PEACCEL and mutation E2R.  We
  need to replace second character in sequence, that is, E to R, and so on for
  the combinations of mutations.

I provided this answer:

from itertools import combinations,chain
from collections import Counter
input_sequence='PEACCEL'
mutations=map(lambda x:x.strip(),open('a.txt','r').readlines())
 #['E2R', 'C4W', 'E6G']
mutation_combinations= chain.from_iterable([list(combinations(mutations,i))for i in range(1,4)])
 #[('E2R',), ('C4W',), ('E6G',), ('E2R', 'C4W'), ('E2R', 'E6G'), ('C4W', 'E6G'), ('E2R', 'C4W', 'E6G')]

for i in mutation_combinations:
    print "combination :"+'_'.join(i)
    c=Counter({})
    temp_string=input_sequence
    for j in i:
        c[j[1]]=j[2].lower()
    for index,letter in c.items():
        temp_string=temp_string[:int(index)-1]+letter+temp_string[int(index):]
    print temp_string

Output:

combination :E2R
PrACCEL
combination :C4W
PEAwCEL
combination :E6G
PEACCgL
combination :E2R_C4W
PrAwCEL
combination :E2R_E6G
PrACCgL
combination :C4W_E6G
PEAwCgL
combination :E2R_C4W_E6G
PrAwCgL

Algorithm I followed:

Read the mutation sequences like E2R.... from a file using

mutations=map(lambda x:x.strip(),open('a.txt','r').readlines())

Made the combinations of the mutations

mutation_combinations= chain.from_iterable([list(combinations(mutations,i))for i in range(1,4)])

If you have 4 mutations you want all four change the range value to 5.
For each combination I replaced them with specified character:

for j in i:
    c[j[1]]=j[2].lower()

I used the above counter to keep track of which character was to be replaced during the mutation combination.

How can I make this more efficient?  I also need to know of any valuable pointers I should know while coding.

There are several things that I would change in your code:

Use functions

This is a good example of spaghetti code where everything is meshed together.
You should provide functions to load the mutation list, one to execute one mutation and one that executes a list of mutations.

Printing the results should also be an extra function.

Use a better mutation representation

Your internal representation for the replacements is too leaky (and maybe too easy).

What if the requirements change and you want to replace multiple character strings?

Instead you should use a structure that already does the separation of the search term, the counter and replace term.

Another problem here is that you will not be able to pass replacement indices bigger than 9 because you only take one character instead of matching a regex for the number in the middle and taking the whole number as index.

Missing error handling

Talking of indices, your program will fail when the given index is out of bounds for the input string.

Unclear input

Neither you nor the OP on SO state what the first element in the mutation string is for. I don't see it being used. It seems to duplicate some of the information of the replacement index.

Code formatting

Please improve the code formatting. Take for example this:

temp_string=temp_string[:int(index)-1]+letter+temp_string[int(index):]

I had a hard time finding the boundaries of the [] because everything is so close together. Instead try to leave space around operators/operands:

temp_string = temp_string[:int(index) - 1] + letter + temp_string[int(index):]

Misused collection

I don't see why you are using a Counter in your code. You don't seem to count anything with it and use it like an ordinary dict.

The line

for j in i:
        c[j[1]]=j[2].lower()

Looks dubious, what is it for? If there are several mutation strings with the same offset only the last one will be recorded (because it overwrites the previous ones). Is this the intended behavior?

If you do not want this behavior then why use the dict (or rather Counter) at all? You could have iterated through the mutations and directly applied them with the same result.


Changing the environment if it sees the incorrect environment in the setting I am using the function below to change the environment. If it sees the wrong environment values, it clicks on it and it changes the environment.
In this function, I am just using if and else statement. Is this a good code practice? This does the job however it does not seem clean.
class SettingsPage extends Page {

changeEnvironment(appName) {
        if (appName.$(PreferenceScreen.environmentProduction).isDisplayed()) {
            appName.$(PreferenceScreen.environmentProduction).click();
        }
        else if
            (appName.$(PreferenceScreen.environmentDevelopment).isDisplayed()) {
             appName.$(PreferenceScreen.environmentDevelopment).click();
        }
    }
}

The built in JS array transformation functions could help you reduce duplicated code.
const environments = ['environmentProduction','environmentDevelopment'];

class SettingsPage extends Page {

    changeEnvironment(appName) {
        const all_buttons = environments.map(
            env=>appName.$(PreferenceScreen[env])
        );
        const displayed_buttons = all_buttons.filter(button=>button.isDisplayed());
        displayed_buttons.forEach(button=>button.click());
    }
}


Mergesort Criticism in C I have written a merge sort in C and would like any advice on how to improve it. Any advice helps to make the code better!
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <string.h>

static void mergesort_helper(void *array, uint32_t index, uint32_t length, size_t size_element, int32_t compare(void *, void *), void *storage)
{
   uint32_t i, j, k;
   if(length == 1)
   {
      /*printf("\n%d, %d\n", index, length);*/
      return;
   }

   mergesort_helper(array, index, length / 2, size_element, compare, storage);

   if(length % 2 == 0)
      mergesort_helper(array, (index + (index + length)) / 2, length / 2, size_element, compare, storage);
   else
      mergesort_helper(array, (index + (index + length)) / 2, length / 2 + 1, size_element, compare, storage);

   /*sorting part*/
   i = index;
   j = (index + (index + length)) / 2;
   k = 0;
   while(i < (index + (index + length)) / 2 || j < (index + length))
   {
      if(i < (index + (index + length)) / 2 && (compare(array + i * size_element, array + j * size_element) <= 0 || j >= (index + length)))
      {
          memcpy(storage + k * size_element, array + i * size_element, size_element);
          i++;
      }
      else if(j < (index + length) && (compare(array + i * size_element, array + j * size_element) > 0) || i >= (index + (index + length)) / 2)
      {
          memcpy(storage + k * size_element, array + j * size_element, size_element);
          j++;
      }
      k++;
   }
   memcpy(array + index * size_element, storage, length * size_element);
}

/*
   array -  to be sorted
   length - exclusive
   size_element - how big is one element
*/
void mergesort(void *array, uint32_t length, size_t size_element, int32_t compare(void *, void *))
{
   void *storage;
   if(length == 0)
      return;

   storage = malloc(size_element * length);
   mergesort_helper(array, 0, length, size_element, compare, storage);
   free(storage);
}

/*user code*/
int32_t compare(void *ptr1, void *ptr2)
{
   int32_t *temp_ptr1, *temp_ptr2;
   temp_ptr1 = (int32_t *)ptr1;
   temp_ptr2 = (int32_t *)ptr2;

   if(*temp_ptr1 < *temp_ptr2)
      return -1;
   else if(*temp_ptr1 > *temp_ptr2)
      return 1;

   return 0;
}

int main(void)
{
   uint32_t i;
   int32_t array[] = {6, 4, 3, 2, 7, -100, 1, 0, 5, 8, -100, 8};

   for(i = 0; i < sizeof(array) / sizeof(array[0]); i++)
   {
      printf("%d ", array[i]);
   }
   printf("\n");

   mergesort(array, sizeof(array) / sizeof(array[0]), sizeof(array[0]), &compare);

   for(i = 0; i < sizeof(array) / sizeof(array[0]); i++)
   {
      printf("%d ", array[i]);
   }
   printf("\n");
   return 0;
}

Avoid code duplication
Your code is more verbose than necessary, because you are repeating a lot of things unnecessarily, or write things in a more complex way than necessary. For example:
(index + (index + length)) / 2

This looks really weird, and is actually equivalent to:
index + length / 2

Which makes much more sense. Even with that change, that expression is used a lot in the code, so it's best to give it a name:
uint32_t middle = index + length / 2;

You also duplicated a call to mergesort_helper() in order to handle odd and even cases of length. However, you don't have to if you write it like so:
uint32_t middle = index + length / 2;
uint32_t end = index + length;

mergesort_helper(array, index, middle - index, size_element, compare, storage);
mergesort_helper(array, middle, end - middle, size_element, compare, storage);

You might also want to consider renaming index to start.
Use const for pointer arguments in compare()
Since the function compare() should not modify the elements it is comparing, the arguments should be pointers to const:
int32_t compare(const void *ptr1, const void *ptr2) {
   const int32_t *temp_ptr1 = ptr1;
   const int32_t *temp_ptr2 = ptr2;
   ...
}

Optimizing the while-loop
There are a few minor changes that can be made to speed up the while-loop a little bit. First, calling compare() is probably going to be the most expensive operation, so you would want to avoid calling it if possible. Rearrange the order in which you check things to make use of short-circuiting:
if(i < middle && (j >= end || compare(...)))

In the above, compare() will only be called if i < middle is true and j >= end is false.
Another possible optimization is realizing that if i >= middle || j >= end, you know you only need to copy the remainder of one side of the midpoint. Instead of doing that element by element, you could do that in one large memcpy(). It also simplifies the if-statements in the while-loop, like so:
while (i < middle && j < end) {
    if (compare(array + i * size_element, array + j * size_element) <= 0) {
        memcpy(storage + k * size_element, array + i * size_element, size_element);
        i++;
    } else {
        memcpy(storage + k * size_element, array + j * size_element, size_element);
        j++;
    }

    k++;
}

/* Copy the remainder */
if (i < middle) {
    memcpy(storage + k * size_element, array + i * size_element, size_element * (middle - i));
} else {
    memcpy(storage + k * size_element, array + j * size_element, size_element * (end - j));
}

The above has also the advantage that compare() is only called once every iteration, whereas in your original code it could have been called twice.
Avoid void pointer arithmetic
Lau G mentioned void pointers, and indeed doing arithmetic on void pointers is illegal. The simple workaround is to cast it to a char * first:
static void mergesort_helper(void *array_ptr, ...)
{
    char *array = array_ptr;
    ...

To catch these sort of issues, enable strict compiler warnings. For GCC and Clang, use -Wall -pedantic. Fix all the warnings the compiler produces.