Initialize a List's Capacity I received my code review comments for the following method:

private RequiredDocumentEntity GenerateRequiredDocumentMapping(string documentTypeCode, params string[] alternativeDocumentIds)
{
    int alternativeDocumentCount = alternativeDocumentIds?.Length ?? 0;
    List<DocumentTypeEntity> satisfyingDocuments = new List<DocumentTypeEntity>(alternativeDocumentCount + 1);  // A document can always satisfy itself, hence the + 1
    DocumentTypeEntity documentType = null;

    if (documentTypeCode != null)
    {
        documentType = GetDocumentType(documentTypeCode);
        satisfyingDocuments.Add(documentType);
    }

    alternativeDocumentIds.Select(GetDocumentType)
                          .ForEach(satisfyingDocuments.Add);

    return new RequiredDocumentEntity() { SatisfiedBy = satisfyingDocuments };
}

The issue the reviewer had with this method was that the initialization of the list's capacity made the code difficult to read. It was also stated that it was a micro-optimization to save the list from doubling upon resize. This I agree with, to an extent. The list will have, at most 6, elements in it upon return. My argument against that would be that while it is a micro-optimization, if you can calculate how many elements will be in a list, why wouldn't you simply initialize the list's capacity?

Therefore, my question is:  Does the initialization as written make this code difficult to read?

You don't have to initialize the list's capacity and with only 6 items you won't notice any difference anyway so why making the code overcomplicated? As a matter of fact you need only a single query to get what you need so it's not only the capacity that makes it unecessarily more difficult to read by the entire implementation.

You can just Concat the alternativeDocumentIds to the optional documentTypeCode:

private RequiredDocumentEntity GenerateRequiredDocumentMapping(string documentTypeCode, params string[] alternativeDocumentIds)
{
    var satisfyingDocuments =
        from docId in
            string.IsNullOrEmpty(documentTypeCode)
                ? Enumerable.Empty<string>()
                : new string[] { documentTypeCode }).Concat(alternativeDocumentIds)

        select GetDocumentType(docId);

    return new RequiredDocumentEntity()
    {
        SatisfiedBy = satisfyingDocuments.ToList()
    };
}

but if the the documentTypeCode is optional anyway why don't you simply include it in the params array? It doesn't make any difference for the result and the method becomes really simple:

private RequiredDocumentEntity GenerateRequiredDocumentMapping(params string[] documentIds)
{
    var satisfyingDocuments =
        from docId in documentIds
        where !string.IsNullOrEmpty(docId)
        select GetDocumentType(docId);

    return new RequiredDocumentEntity
    {
        SatisfiedBy = satisfyingDocuments.ToList()
    };
}

or

private RequiredDocumentEntity GenerateRequiredDocumentMapping(params string[] documentIds)
{
    return new RequiredDocumentEntity
    {
        SatisfiedBy =
            documentIds
                .Where(docId => !string.IsNullOrEmpty(docId))
                .Select(GetDocumentType).ToList()
    };
}

whatever you prefer.


Pulling data from different JSON files I have a controller that pulls gets data from two different JSON files. One of the calls loadProducts happens when the controller is initiated. The other one, loadPrices, is called when a user clicks a button on the interface.

I have noticed that the controller is starting to get bigger and I was worried about the state of the code. As I am knew to this I was hoping for suggestions about how I can clean it up or make it more efficient.

'use strict';
(function () {

var app = angular.module('priceQuoteApp');

var quoteBuilderController = function ($scope, $http, $timeout) {

    //set the variabls
    $scope.listOfProducts = null;
    $scope.selectedProduct = null;
    $scope.listOfProductVariants = null;
    $scope.formData = {};
    $scope.showTable = 'none';
    $scope.selectedProductId = null;
    $scope.loadingProducts = true;
    $scope.loadedProducts = false;
    $scope.loadingPrices = false;
    $scope.loadedPrices = false;

    // get all the portfolios and the products
    // the timer is to simulate network latency
    var loadProducts = function () {
      $http.get('scripts/json/sample-products.json')
        //$http.get('http://www.thesite/api/Pricing/GetProductCatalogue')
            .then(function (allProducts) {
                $scope.listOfProducts = allProducts.data.Connectivity;
            })
            .finally(function () {
                $scope.loadingProducts = false;
                $scope.loadedProducts = true;
            });
    };
    //loadProducts();
   $timeout(loadProducts, 2000);

    // get all the prices
    // the timer is to simulate network latency
    var loadPrices = function () {
        $http.get('scripts/json/sample-product-prices.json')
            .then(function (res) {
                $scope.selectedProductPrices = res.data.ProductVariants;
                $scope.selectedProductAddOns = res.data.product_addons;
            })
            .finally(function () {
                $scope.loadingPrices = false;
                $scope.loadedPrices = true;
            });
    };

    // get the product details
    $scope.getProductDetails = function (item) {
        $scope.selectedProduct = item.product_name;
        $scope.selectedProductId = item.product_id;
        $scope.listOfProductVariants = item.product_variants;
        $scope.formFields = item.product_price_attributes;
    };

    // select the product and get the price
    $scope.selectProduct = function (item) {
        $scope.selectedRow = item;
        $scope.selectedProductPrice = item.RecurringCost;
    };

    // replace this with api calls to get the prices
    $scope.getProductVariants = function () {
        $scope.loadingPrices = true;
        $timeout(loadPrices, 2000);
    };
};

app.controller('quoteBuilderController', ['$scope', '$http', '$timeout', quoteBuilderController]);

}());

One this for sure is that I should probably break it up as its becoming long. Is there a need to define the variables at the top? Should I be using the This keyword?

There is no need using the 'this' keyword, or such as angularjs provides for those scenario. One thing you can do is to abstract the loadProducts functions and load prices function to a factory and then pass the factory as a parameter to your controller as shown below.

var priceQuoteApp = angular.module('priceQuoteApp', [])
    .constant("loadProductUrl", "scripts/json/sample-products.json")
    .constant("loadPricesUrl", 'scripts/json/sample-product-prices.json');

priceQuoteApp.factory("ProductsFactory", function ($http, loadProductUrl, loadPricesUrl) {
        return {
            loadProducts: function () {
                //Create an object to hold your result
                var result = {};

                $http.get(loadProductUrl)
                .then(function (allProducts) {
                    result.listOfProducts = allProducts.data.Connectivity;
                })
                    .finally(function () {
                        result.loadingProducts = false;
                        result.loadedProducts = true;
                    });
            },

            loadPrices: function () {
                $http.get(loadPricesUrl)
                    .then(function (res) {
                        result.selectedProductPrices = res.data.ProductVariants;
                      result.selectedProductAddOns = res.data.product_addons;
                    })
                    .finally(function () {
                        result.loadingPrices = false;
                        result.loadedPrices = true;
                    });
            }
        });

Then you inject the factory into your controller as shown below

priceQuoteApp.controller("quoteBuilderController", function ($scope, $http, $timeout, ProductsFactory) {
        var products = ProductsFactory.loadProducts();
        //Your other codes

        //When the user clicks the button
        $scope.clickAction = function () {
            var prices = ProductsFactory.loadPrices;
            //Then do other things with it.
        }
    });

One good thing is that this increases code return and thereby making every part of your code to focus on a single responsibility. Hope this helps.


Escaping characters in a WMI Query I've knocked up this code to allow me to query for specific service names using PowerShell.

I'm after the executable (PathName) for the service, so the Get-Service cmdlet is not appropriate here. I don't actually have any services with special characters in their names; but I like to build robust code where I can foresee future gotchas.

I'm posting here as want to know if my ConvertTo-WmiEscapedQuery is the right way to escape a WMI Query.

function ConvertTo-WmiEscapedQuery {
    [CmdletBinding()]
    param (
        [Parameter(Mandatory = $true)]
        [string]$Query
        ,
        [Parameter()]
        [string[]]$Parameters = @()
    )
    begin {
        [string]$EscapeCharatersRegex = '([\\''"])'
    }
    process {
        [string[]]$EscapedParameters = $Parameters | %{$_ -replace $EscapeCharatersRegex, '\$1'}
        $Query -f $EscapedParameters
    }
}

function Get-WmiService {
    [CmdletBinding()]
    param (
        [Parameter(Mandatory = $true)]
        [string]$ServiceName
    )
    begin {
        [string]$Query = 'select * from win32_service where name = "{0}"'
    }
    process {
        Get-WmiObject -Query (ConvertTo-WmiEscapedQuery -Query $Query -Parameters $ServiceName)
    }
}

Get-WmiService 'John''s Example Service'

I realise some people don't like the | %{...} shorthand for a foreach statement.  I've deliberately opted to use such shorthand options only for foreach/% and where/? statements, since these are pretty universally known conventions.

(I've also posted here as this question falls into the grey area between question and code-review.)

Per @Dangph's comment on the question, it seems that whether to escape ' or " depends on the context; i.e. if within quotes of the same type these characters need escaping; if not however, they must not be escaped.

The below version only escapes apostrophes (' / single quotes) and backslashes (\); not quotation marks (" / double quotes).

It assumes that the query will only use single quotes for surrounding parameter names.

It has a bit of extra logic to capture the more common cases where double quotes are used (i.e. where it's simply quotes around the placeholder, or quotes and wildcards (for "starts with" / "ends with" / "contains") matching around the placeholders.  I don't really like this as it's inconsistent / may lead to unpredictable behaviour, so will likely not keep that line / rather will state that only single quotes should be used... but included here to illustrate one possible option.

function ConvertTo-WmiEscapedQuery {
    [CmdletBinding()]
    param (
        [Parameter(Mandatory = $true)]
        [string]$Query
        ,
        [Parameter()]
        [string[]]$Parameters = @()
    )
    begin {
        [string]$EscapeCharatersRegex = '([\\''])' #Doesn't escape " as assumes strings are enclosed using apostrophes; i.e. "... where field = '{0}'"
    }
    process {
        [string[]]$EscapedParameters = $Parameters | %{$_ -replace $EscapeCharatersRegex, '\$1'}
        [string]$escapedQuery = $Query -replace '(")(%?{\d+}%?)\1', '''$2''' #enforce the above assumption by replacing double quotes around parameterised input with apostrophes.  Not guaranteed to work, but better than nothing.  Remove this line & replace the below with `[string]$escapedQuery = $Query -f $EscapedParameters` if you don't like this behaviour.
        $escapedQuery = $escapedQuery -f $EscapedParameters
        Write-Verbose "Escaped Query: $escapedQuery"
        $escapedQuery
    }
}

function Get-WmiService {
    [CmdletBinding()]
    param (
        [Parameter(Mandatory = $true)]
        [string]$ServiceName
    )
    begin {
        [string]$Query = 'select * from win32_service where name = ''{0}'''
    }
    process {
        Get-WmiObject -Query (ConvertTo-WmiEscapedQuery -Query $Query -Parameters $ServiceName)
    }
}

Get-WmiService '"John''s Example Service\String"' -Verbose
Get-WmiObject -Query (ConvertTo-WmiEscapedQuery -Query 'select * from win32_service where name = ''{0}'' or displayname like "%{1}%"' -Parameters 'bits', 'transfer' -Verbose) | select name, displayName #just to prove it works
Get-WmiObject -Query (ConvertTo-WmiEscapedQuery -Query 'select * from win32_service where name = ''{0}'' or displayname like "%{1}%"' -Parameters '''bits"', '"transfer''' -Verbose) | select name, displayName #to test escaping logic


I want to take away WebService call from UIViewController I am using Moya to make network calls in Swift.

In my ViewController I am calling the WebService and creating a model from response.

import UIKit
import Moya

class LoginViewController: UIViewController {
  let provider = MoyaProvider<Api>()

  override func viewDidLoad() {
    super.viewDidLoad()
    generateAccessToken()
  }

  private func generateAccessToken() {
    provider.request(.generateAccessToken(
        mobile: phoneTextField.text!,
        countryCode: "91",
        name: "Saurabh",
        otp: "9612")
    ) { result in
        switch result {
        case .success(let response):
            let data = response.data
            let user = try? JSONDecoder().decode(User.self, from: data)
            if let authToken = user?.authToken {
                AuthManager.setToken(token: authToken)
            }
        case .failure(let error):
            print(error.response?.statusCode as Any)
        }
    }
  }

I want to move the WebService call to other file, say NetworkManager.swift

import Foundation
import Moya

struct NetworkManager {
  public static let shared = NetworkManager()
  private let provider = MoyaProvider<Api>()

  func generateAccessToken(with mobile: String, countryCode: String, name: String, otp: String)  {
    // Get result and return to calling class
  }
}

But it is strongly recommended not to use Singleton Class.

How should I call my WebService and return the response to the LoginViewController?

I would recommend looking into MVVM architecture. You can see a simple example in this repo.
One problem with it is that its author declares and assigned view models directly in a ViewController which can be a bit problematic when writing tests. A better approach would be to inject view model so it can be easily replaced with mock. To improve that approach you can take a look at coordinators patterns, a bit more advanced example can be found here
In most cases the WebService should go in the ViewModel in the MVVM pattern. Services can be injected to ViewModels, which are responsible for managing data provided by these services. Then when they perform all logical transformations (filtering, ordering, multiplying etc.) they provide this information to a view (ViewController in our case) which just consumes it. It allows us to easily write mocks and tests. There are of course more approaches like state machine, but ViewModel as content provider and meeting point for services is most common and most universal one in my opinion.


Auto-complete in JavaScript I recently got interviewed in a company for Senior level position. They asked me to develop auto-complete component (2 hours time limit) using plain JavaScript similar to this.

I wrote below code, which works as expected, but they responded saying that the solution doesn't match expectations. Please help me figure out what I could improve in this.

Here is the link to working example.

function customAutoComplete(input,sourceList){

if(!input){
    console.error('No input box provided');
}
if(!sourceList ||   sourceList.length<=0){
    console.error('No sourceList provided for customAutoComplete');
}

var suggList;
input.addEventListener('keyup',function(e){
    suggList=populateSourceList(input.value);
    if(suggList){
        input.parentNode.appendChild(suggList);
    }
    if(e.keyCode==40){
        suggList.focus();
    }
});

input.addEventListener('blur',function(e){
    if(e.relatedTarget.id!=input.id + '_autoComplete'){
        setTimeout(clearSuggestionList,200);
    }
});

function populateSourceList(inputText){

    if(!inputText){
        return null;
    }
    var suggList=getSuggestList();
    var size=1;
    sourceList.forEach( function(itm){
        if(itm.indexOf(inputText)>=0) {
            var listItem=document.createElement("option");
            listItem.appendChild(document.createTextNode(itm));
            suggList.appendChild(listItem);
            size++;
        }
    });
    suggList.setAttribute('size',size);
    if(size>1){
        return suggList;
    }
    return null;
}

function getSuggestList(){
    var acListId=input.id + '_autoComplete';
    var acList= document.getElementById(acListId)
    if(!acList){
        acList = document.createElement("select");
        acList.setAttribute('class','suggestion-list');
        acList.setAttribute('id',acListId);
        var stl='left:' + input.offsetLeft + 'px;top:' + (input.offsetTop + input.offsetHeight) + 'px;min-width:' + input.offsetWidth + 'px';
        acList.setAttribute('style',stl);
        acList.addEventListener('keydown',function(e){autoCompleteItemSelected(e,acList)});
        acList.addEventListener('blur', function(){clearSuggestionList(suggList)} );
    }

    else{
        acList.innerHTML='';
    }

    return acList;
}
function clearSuggestionList(suggList){
    if(!suggList){
        suggList=getSuggestList();
    }
    suggList.outerHTML='';
}
function autoCompleteItemSelected(e,list){
    if(e.keyCode==13){
        input.value=list.selectedOptions[0].text;
        clearSuggestionList();
    }
}
}

There's a few things that can be reviewed:

Your indentation and spacing are incorrect, use JSFiddle's TidyUp function to clean it up.

All of the e.keyCode == matches shouldn't be matched to magic numbers, but moved to a kind of enum or something similar:
var KEYS = {
    Enter: 13,
    DownArrow: 40,
    UpArrow: 38
};
// ...
if (e.keyCode == KEYS.Enter){

suggList:
Don't sacrifice readability for a few characters: suggestedList is more than fine.

console.error:
Instead of console.error, use throw instead: console.error('No sourceList provided for customAutoComplete') into: throw 'ACError: No sourceList provided for customAutoComplete'


Join and map command line arguments in Rust Is there a more natural way to do this in rust? I'm having a lot of difficulty trying to make this look clean. Type mismatches and borrowing have me stumped. What are some refactors that I could make?

Input

Take in list of space-separated, command-line arguments

Output

Book (join all arguments except the last)
Reference (the last argument)
File (looked up via the book)

I abbreviated the books_to_files mapping to only include a couple examples of books with different word lengths.

Examples

In:                             Out:

Genesis 1:1                     Genesis, 1:1, mhc1.txt
Song of Solomon 2:2-4           Song of Solomon, 2:2-4, mhc3.txt
Acts of the Apostles 3:3-9      Acts of the Apostles, 3:3-9, mhc6.txt

Source Code

use std::env;
use std::collections::HashMap;

fn main() {
    let args: Vec<String> = env::args().collect();

    let book: String;
    if args.len() == 3 {
        book = args[1].clone();
    } else if args.len() == 4 {
        book = args[1].clone() + " " + &args[2].clone();
    } else if args.len() == 5 {
        book = args[1].clone() + " " + &args[2].clone() + " " + &args[3].clone();
    } else if args.len() == 6 {
        book = args[1].clone() + " " + &args[2].clone() + " " + &args[3].clone() + " " + &args[4].clone();
    } else {
        panic!("Wrong number of arguments");
    }
    let reference: String = args[args.len()-1].clone();
    let file: String = get_file(&book);

    println!("{}, {}, {}", book, reference, file);
}

fn get_file(book: &String) -> String {
    let books_to_files: HashMap<&str, &str> = [
        ("Genesis", "mhc1.txt"),
        ("First Samuel", "mhc2.txt"),
        ("Song of Solmon", "mhc3.txt"),
        ("Acts of the Apostles", "mhc6.txt"),
        ("First Corinthians", "mhc6.txt"),
        ("Galatians", "mhc6.txt"),
    ].iter().cloned().collect();

    books_to_files.get::<str>(&book.to_string()).unwrap().to_string()
}

```

I've collected a few thoughts about possible refactors you could consider below.

Your if chain isn't the most efficient way

As you've written it, your code will only work for titles between one and four words long. This isn't a disaster, and it will probably work for most titles, but we can rewrite your code to be shorter and handle any length of title. A win-win!

We can use Iterator::skip() to throw away the executable name, as we don't care about it anyway. See skip() in the documentation.

let args: Vec<String> = env::args().skip(1).collect();

It'd be a good idea to check that we've been given enough arguments before continuing.

let arg_len: usize = args.len();
if arg_len < 2 {
    panic!("At least two arguments must be supplied!");
}

Now, we can be a bit clever and avoid your if chain all together. Using a range we can get a slice of our args vector corresponding to all but the last element. We can then use join() to turn that slice back into a String. For example, if we started with vec!["Song", "of", "Solomon", "2:2-4"] in args, we would take a slice to get ["Song", "of", "Solomon"] and rejoin them with " " in the middle to get "Song of Solomon".

let book = args[0..(arg_len - 1)].join(" ");

Try to avoid clone() unless necessary

The first thing I noticed when I took a look at your code was that you've used clone() in quite a few places. We've already got rid of a lot of them with the tweak above, and you should try to avoid them all together as allocating when you don't need to is a waste of time and memory. If you can use an &str, do that instead of insisting on all strings being String. As an aside, there is rarely any point bothering with &String — just use &str there.

Putting the above into action, we can let reference be an &str and avoid a clone.

let reference: &str = &args[args.len() - 1];

Let's also change get_book's method signature as suggested above.

fn get_file(book: &str) -> &str {

Now we can get rid of the unnecessary to_string() below.

books_to_files.get::<str>(&book.to_string()).unwrap()

And tweak this line in main() to accept an &str.

let file: &str = get_file(&book);

Avoiding unwrap

Ideally, get_book ought to return an Option instead, so the caller can choose how they want to handle the error. As it is fatal anyway, unwrapping makes little difference, but it's worth bearing in mind as a future improvement.

Try it Online


Optimizing the function in R by replacing for loop I am trying to create a function that I want to use in my real data. Is there any way to optimize the following function? This is a function I created to filter the data.

filter <- function(x, m, delt) {
  series <- x
  series_filtered <- rep(0,length(series))
  delta_t <- delt # time step between data points (hours)
  tau_crit <- 3 # critical period (hours)
  theta_crit <-  2*pi*delta_t/tau_crit # critical frequency
  M <- m # number of time steps before and after available in filtering window
  h <- rep(0,(2*M+1)) # initialize the weights
  # get the lanczos coefficients
  for (n in (-1*M):M) {
    h[M+n+1] <- ifelse(n == 0,theta_crit*delta_t/pi,
                       (sin(n*theta_crit*delta_t)/(pi*n)) * (sin(n*pi/M)/(n*pi/M)))
  }
  # need to adjust the weights so that they add up to 1
  h_unbiased <-  h/sum(h)
  h <- h_unbiased
  # step through each time for which we'd like to create a filtered value
  for (t in (1+M):(length(series)-M)) {
    for (n in (-1*M):M) {
      # apply the cofficients to create the filtered series
      series_filtered[t] = series_filtered[t] + series[t+n] * h[M+n+1]
    }
  }
  # Filtered data
  y2 <- series_filtered[(M+1):(length(series)-M)]
  return(y2)
}

Any suggestions on removing for loop and replacing by apply or any other function is appreciated.

In the above function, I have one single for loop and one double for loop.

Edit:

A testing can be done by the following data:

x <- 1:100
series <-  sin(2*x)+sin(x/4) + sin(x/8) + rnorm(100)
jd2 <- filter(x=series,m=4, delt=(40/60))

I have tried this, but I don't know, how much (and even if) faster it is. Anyway, you used a lot of redundant code (creating variables M, h_unbiased, delta_t etc.).

filter2 <- function(x, m, delt) {
  series <- x
  tau_crit <- 3 # critical period (hours)
  theta_crit <-  2*pi*delt/tau_crit # critical frequency
  # get the lanczos coefficients
  h <- sapply(-m:m, FUN=function(x)ifelse(x == 0,theta_crit*delta_t/pi,
    (sin(x*theta_crit*delt)/(pi*x)) * (sin(x*pi/m)/(x*pi/m))))
  # need to adjust the weights so that they add up to 1
  h <- h/sum(h)
  # step through each time for which we'd like to create a filtered value
  y2 <- sapply((1+m):(length(series)-m), function(t){
    sum(unlist(sapply(-m:m, FUN=function(x)(series[t+x]*h[m+x+1]))))
  })
  # return
  return(y2)
}


Extracting the timestamp, minimum score, and maximum score from hundreds of arrays So this is what I'm doing:

I have hundreds of arrays, each of them filled with a date in milliseconds in the first position, and the rest of the array is filled with user game scores.

Let's just call this array above var scoresDates

The ultimate goal is to have each of those arrays contain only the date in milliseconds, the smallest score and then the largest score, in that order. To do this, I set up for loop within a for loop, that will loop the larger array and the systematically make several operations until the end result is achieved. I would like to see if there are better ways to do it or optimize this process.

This is what I have...

// Logic that stacks all the scores per unique day from all the user's game scores. All unique dates with subsequent scores are stored in the uniqueDaysScores array.

uniqueDaysScores = [];
uniqueDaysScores.push(scoresDates[0]);

for(var i = 0, ii = scoresDates.length; i < ii; i++){
  var tempArray = [];

  if(scoresDates[i][0] == uniqueDaysScores[0][0]){
    uniqueDaysScores[0].push(scoresDates[i][1]);
  } else{
    tempArray.push(scoresDates[i][0]);
    tempArray.push(scoresDates[i][1]);
    uniqueDaysScores.unshift(tempArray);
  }
}

// Storted scores from smallest to largest for each unique date for user games played.

scoreSorted = [];

// Logic to take all the scores, sort them from decreasing to increasing order, then package the date, lowest and shortest score into the finished array, scoreSorted.

for(var i = 0, ii=uniqueDaysScores.length; i < ii; i++){
  var scoreSorted1 = [];
  var scoreSortedPrimer = [];

// First level for loop that goes through the unique scores array and ignores the date, while pushing all the scores into the scoreSorted1 array.

  for(var z = 0, zz = uniqueDaysScores[i].length; z < zz; z++){
    if(uniqueDaysScores[i][z] < 1000){
      scoreSorted1.push(uniqueDaysScores[i][z]);
    }
  }

  // Next step is sorting the scoreSorted1 array from lowest score to highest score.
  scoreSorted1.sort(function(a,b){
    return a-b;
  });

  // Find maximum and minimum scores in the array and finally push them into the scoreSortedPrimer array.
  var primerMin = Math.min.apply(Math, scoreSorted1);
  scoreSortedPrimer.push(primerMin);
  var primerMax = Math.max.apply(Math,scoreSorted1);
  scoreSortedPrimer.push(primerMax);

  // Finally, find the date and push it into the scoreSortedPrimer array, so it can be pushed into the main scoreSorted array.

  for(var x = 0, xx = uniqueDaysScores[i].length; x < xx; x++){
    if(uniqueDaysScores[i][x] > 1000){
      scoreSortedPrimer.unshift(uniqueDaysScores[i][x]);
    }
  }
  uniqueDaysScores.push(scoreVarianceSortedPrimer);
}

The uniqueDaysScores array will contain all the arrays with the date first and the lowest and highest score thereafter. I have added comments to make it easier to follow. It kind of seems like a long process and I'm just curious if there is a more experienced programmer that can see where this script can be optimized or improved.

I'm a little unsure of why scoresDates starts with a 56-item array and the rest are 2-item arrays.

Just to be sure, this is your data model, correct? Please let me know if I've totally misread things.

scoresDates is [
    [date, score, score, score...],
    [date, score],
    [date, score],
    ...
]

Working forwards from your current code, here are some suggestions. I've used some functions like Array.filter that require IE > 9; if you need to support old Internet Explorer then a utility library like Lodash would provide polyfills for these.

In general, you're doing more loops through the data than should be necessary. For example:

Sorting is extra work

  // Next step is sorting the scoreSorted1 array from lowest score to highest score.
  scoreSorted1.sort(function(a,b){
    return a-b;
  });

  // Find maximum and minimum scores in the array and finally push them into the scoreSortedPrimer array.
  var primerMin = Math.min.apply(Math, scoreSorted1);
  scoreSortedPrimer.push(primerMin);
  var primerMax = Math.max.apply(Math,scoreSorted1);
  scoreSortedPrimer.push(primerMax);

scoreSorted1 seems to only be used for calculating min and max. Sorting is more work than min and max, each of which go through the entire array anyway. Here are a few faster options:

keep the sorting, in which case you will know that min is the first item and max is the last
don't sort, just call Math.min and Math.max.
write your own function that calculates both min and max in just one pass of the array, as opposed to two (Math.min + Math.max)

Array.filter

Using higher-level functions like forEach and filter can simplify some of your loops, by letting you ignore array indexes.

For example, this:

var scoreSorted1 = [];

// First level for loop that goes through the unique scores array and ignores the date, while pushing all the scores into the scoreSorted1 array.

for(var z = 0, zz = uniqueDaysScores[i].length; z < zz; z++){
  if(uniqueDaysScores[i][z] < 1000){
    scoreSorted1.push(uniqueDaysScores[i][z]);
  }
}

becomes

var scoreSorted1 = uniqueDaysScores[i].filter(function(score) {
  return score < 1000;
});

And, at a higher level, your entire program becomes two parts: merging all the scores for each date, then applying a function [scores] -> [min(scores), max(scores)] to each item in that merged object.

Comments

Comments should explain why, rather than describe the code, unless it is particularly hairy. This is a little subjective, but, for example, you can easily read scoreSorted1.sort(... and see that it is sorting the scores, comment or not.

Instead, I recommend saving comments for more confusing passages and for higher-level overviews of your functions or logic. Using descriptive variable and function names is often enough of a comment.

// Logic to take all the scores, sort them from decreasing to increasing order, then package the date, lowest and shortest score into the finished array, scoreSorted.

for(var i = 0, ii=uniqueDaysScores.length; i < ii; i++){
  var scoreSorted1 = [];
  var scoreSortedPrimer = [];

// First level for loop that goes through the unique scores array and ignores the date, while pushing all the scores into the scoreSorted1 array.

  for(var z = 0, zz = uniqueDaysScores[i].length; z < zz; z++){
    if(uniqueDaysScores[i][z] < 1000){
      scoreSorted1.push(uniqueDaysScores[i][z]);
    }
  }

  // Next step is sorting the scoreSorted1 array from lowest score to highest score.
  scoreSorted1.sort(function(a,b){
    return a-b;
  });

  // Find maximum and minimum scores in the array and finally push them into the scoreSortedPrimer array.
  var primerMin = Math.min.apply(Math, scoreSorted1);
  scoreSortedPrimer.push(primerMin);
  var primerMax = Math.max.apply(Math,scoreSorted1);
  scoreSortedPrimer.push(primerMax);

  // Finally, find the date and push it into the scoreSortedPrimer array, so it can be pushed into the main scoreSorted array.

  for(var x = 0, xx = uniqueDaysScores[i].length; x < xx; x++){
    if(uniqueDaysScores[i][x] > 1000){
      scoreSortedPrimer.unshift(uniqueDaysScores[i][x]);
    }
  }
  uniqueDaysScores.push(scoreVarianceSortedPrimer);
}

becomes, more sparsely,

// Calculate min and max score for each date, appending to uniqueDaysScores.

uniqueDaysScores.forEach(function(dateAndScores) {

  var date = dateAndScores.filter(function(score) {
    return score >= 1000;
  })[0];

  var scores = dateAndScores.filter(function(score) {
    return score < 1000;
  });

  uniqueDaysScores.push([
      date,
      Math.min.apply(Math, scores),
      Math.max.apply(Math, scores)
  ]);
});

and would be even simpler if the date was stored somewhere specific so that you don't need to use loops or filter to find it.

Let's rewrite the initial score-merging step to accommodate this.

Merging/grouping scores for each date

I think this is where a lot of the extra complexity is coming from.

Using an object, with the dates as keys and the arrays of scores as values, would be natural, but your array of [date, scores...] arrays would also work.

The important thing is to make sure the date is easy to find, not mixed into your scores, so you don't need to search for it later (by checking what score is > 1000).

// Logic that stacks all the scores per unique day from all the user's game scores. All unique dates with subsequent scores are stored in the uniqueDaysScores array.

uniqueDaysScores = [];
uniqueDaysScores.push(scoresDates[0]);

for(var i = 0, ii = scoresDates.length; i < ii; i++){
  var tempArray = [];

  if(scoresDates[i][0] == uniqueDaysScores[0][0]){
    uniqueDaysScores[0].push(scoresDates[i][1]);
  } else{
    tempArray.push(scoresDates[i][0]);
    tempArray.push(scoresDates[i][1]);
    uniqueDaysScores.unshift(tempArray);
  }
}

becomes, using an object of {date: scores},

// Group scores by date

var scoresByDate = {};

scoresDates.forEach(function(scores) {
  var date = scores[0];
  scores.shift();  // Remove date

  if (!(date in scoresByDate)) {
    scoresByDate[date] = scores;
  } else {
    scoresByDate[date].concat(scores);
  }
}

I've used forEach here, but your original loop-with-index would also work just fine.

With this scoresByDate structure, finding min and max for each date becomes very easy:

var maxMinByDate = Object.keys(scoresByDate).map(function(date) {
  return scoreMinMax(date, scoresByDate[date]);
};

or you could just not bother with a separate scoreMinMax function.

Altogether

With fewer, more high-level comments and a better data model, you end up with something more like this:

var scoresDates = [...];

// Group scores by date.

var scoresByDate = {};

scoresDates.forEach(function(scores) {
  var date = scores[0];
  scores.shift();  // Remove date

  if (!(date in scoresByDate)) {
    scoresByDate[date] = scores;
  } else {
    scoresByDate[date].concat(scores);
  }
}

// Calculate min and max score for each date.

var minMaxByDate = Object.keys(scoresByDate).map(function(date) {
  var scores = scoresByDate[date];
  return [
    date,
    Math.min.apply(Math, scores),
    Math.max.apply(Math, scores)
  ];
});

which could be further accelerated by writing a extrema(arr) function that would return the pair [min(arr), max(arr)] in one pass.


JavaScript Regex Test and Replace I need to sanitize some chars.  I have a solution that works but I am wondering if there is a better solution that may be faster or better or if I flat out should be approaching this differently?

function escapeStr(_str){
    if (/\"|\'|\%/g.test(_str)) {
        _str = _str.replace(/"/g, "%22");
        _str = _str.replace(/'/g, "%27");
        _str = _str.replace(/%/g, "%25");
    }
    return _str;
}

And vice versa:

function unescapeStr(_str){
    if (/\%22|\%27|\%25/g.test(_str)) {
        _str = _str.replace(/\%22/g, '"');
        _str = _str.replace(/\%27/g, "'");
        _str = _str.replace(/\%25/g, "%");
    }
    return _str;
}

Instead of hardcoding the characters, and their escape values specifically in your .replace call, you can pass a callback function for the replacement. I suggested that you dive into encodeURI:

So, something like:

function escapeStr(_str) {
  return _str.replace(/([%'"])/g, encodeURI)
}

function unescapeStr(_str) {
  return _str.replace(/(%(?:2[257]))/g, decodeURI)
}
const e = escapeStr, u = unescapeStr;
console.log( e("string") );
console.log( e("some\" char%s") );
console.log( u("string") );
console.log( u("some%22 char%25s") );


Abstract Factory Experiment While I should probably be using Dependency Injection via Ninject or a similar project, I have been attempting to implement an abstract factory design that would provide me with the following easy to read and use syntax:

UniversalFactory.Factory<IFoo>.Create<FooImpl>(... parameters ...);

I'm throwing this experiment on here in order to solicit some constructive feedback both with implementation logic and design reasoning.

Here is my current implementation:

public class UniversalFactory
{
    private readonly IFactory<IFoo> FooFactoryEx;

    public UniversalFactory()
    {
        FooFactoryEx = new FooFactory();
    }

    public IFactory<T> Factory<T>()
    {
        if (typeof (T) == typeof (IFoo))
            return (IFactory<T>)FooFactoryEx;
        return null;
    }
}

public interface IFactory<in TF>
{
    T Create<T>(params object[] p) where T : TF;
}

public class FooFactory : IFactory<IFoo>
{
    public T Create<T>(params object[] p) where T : IFoo
    {
        if (typeof(T) == typeof(FooImpl))
        {
            var id = Convert.ToInt32(p[0]);
            return (T)FooImpl.Create(id);
        }
        return default(T);
    }
}

public interface IFoo
{
    // Some interface
}

public class FooImpl : IFoo
{
    public int Id { get; set; }

    private FooImpl(int id)
    {
        Id = id;
    }

    public static IFoo Create(int id)
    {
        var foo =  new FooImpl(id);
        return foo;
    }
}

This code allows a UniversalFactory to be instantiated and the following to be invoked successfully:

UniversalFactory.Factory<IFoo>().Create<FooImpl>(1);

My reasoning for this approach:

All construction is easily identifiable via the call to UniversalFactory.
Factories are invoked based on the interface implementation being constructed, hiding all details about the specific factory being used from the new object request.
The object type and its construction parameters are clearly identifiable in the request.
Object construction stays in each class, and any additional updates to other objects at construction time can take place in the factory method.

My current implementation has (at a minimum) the following flaws:

There is no compile-time safety that ensures IFoo is implemented by an IFactory. Only that FooImpl implements IFoo is ensured.
UniversalFactory.Factory has to add a check for every interface supported by a factory
Individual factories have to invoke static Create() methods within each class. It would be better if the static create could be invoked in a generic way, and Create(params object[] p) somehow mapped to Create(int a, string b, ...) as needed.

Thoughts? Feedback? Is this approach unnecessary or too complex? Hopefully this experiment is, at the least, interesting to a couple of other SO folk out there.

Well what you are trying to do is not strictly speaking an abstract factory. For it to be an abstract factory the use case should look like this:

UniversalFactory.Factory<IFoo>  // client has no idea about IFactory<IFoo> implementation
                .Create(... parameters ...); // client has no idea about IFoo implementation, returns IFoo

Ideally, you should not even need to pass parameters. That is why you are building factories after all. To delegate object creation.

Other minor remarks:

where T : TF - those are not very descriptive names for generic arguments. Perhaps you can come up with better naming.
var id = Convert.ToInt32(p[0]); - i'm not sure i like Convert here. It might hide bugs and misusages. I think that strong cast is better in that case.
public static IFoo Create(int id) - i don't see any reason to use factory method.

public class FooImpl : IFoo
{
    public int Id { get; private set; }

    public FooImpl(int id)
    {
        Id = id;
    }
}

this looks a lot cleaner. Also i think Id setter should be always private.

Overall, i think unless you are doing it for studying purposes, you should just grab an existing DI library, as you've metioned yourself :)


Vector (physics) implementation I recently started learning Java, and I decided to implement a basic vector system for another particle system I was building.

import java.util.*;

class Vector {
    int x;
    int y;

    public Vector(int x, int y) {
        this.x = x;
        this.y = y;
    }

    public void add(Vector vector) {
        this.x += vector.x;
        this.y += vector.y;
    }

    public void sub(Vector vector) {
        this.x -= vector.x;
        this.y -= vector.y;
    }

    public void mul(Vector vector) {
        this.x *= vector.x;
        this.y *= vector.y;
    }

    public void div(Vector vector) {
        this.x /= vector.x;
        this.y /= vector.y;
    }
}

What can be improved? I'm new to Java, so constructive criticism on anything would be nice.

Immutable objects are awesome. They are robust, predictable, and inherently thread-safe.
Make the x, and y fields final,
and change the operations to return the resulting Vector,
for example:

class Vector {
    private final int x;
    private final int y;

    public Vector(int x, int y) {
        this.x = x;
        this.y = y;
    }

    public Vector add(Vector other) {
        return new Vector(x + other.x, y + other.y);
    }

    public Vector sub(Vector other) {
        return new Vector(x - other.x, y - other.y);
    }

    public Vector mul(Vector other) {
        return new Vector(x * other.x, y * other.y);
    }
}

As in the example above, I recommend changing the name of the Vector parameter from vector to something else, like other, to make it perfectly clear that the method is dealing with another vector.

This way you'll be able to chain operations like this:

new Vector(1, 2).add(new Vector(3, 4)).sub(new Vector(5, 6)).mul(new Vector(7, 8));

It will be also useful to implement a custom toString method to make it easy to print a String representation of the vector, for example:

@Override
public String toString() {
    return String.format("(%s, %s)", x, y);
}


Reload pickled objects into dict - assigned to variable name from file name I'm doing a bunch of work with echonest audio objects (as returned by the echonest API) in the Python CLI and rather than having to recreate them each time, they can be saved.

This bit of code reloads them all, referenced via a dict:

def get_saved(directory = 'audio/', extension = 'en'):
    return glob.glob(directory + '*.' + extension)

def file_queue(files):
    """
    Get list of files, add them to a queue and return the queue.
    """
    q = Queue()

    for f in files:
        q.put(f)

    return q

def lazarus(filename):
    """
    Get pickled (*.en) filename (path) and return echonest analysis object
    """
    with open(filename) as f:
        return pickle.load(f)

def lazarus_queue(q):
    container = {}
    while not q.empty():
        track = q.get()
        filename = track.replace('.mp3.analysis.en', '')
        filename = filename.replace('audio/', '')
        container[filename] = lazarus(track)
    return container

def resurrect():
    files = get_saved()
    q = file_queue(files)
    return lazarus_queue(q)

It's part of a little helper module:

>>> import mtu
>>> audio_objs = mtu.resurrect()
>>> audio_objs
{'claudia': <echonest.remix.audio.LocalAudioFile object at 0x10712e910>, 'GlitchBit_BJanoff': <echonest.remix.audio.LocalAudioFile object at 0x10712e690>, 'livingstonleo': <echonest.remix.audio.LocalAudioFile object at 0x10f52e0d0>, 'amandathorpe': <echonest.remix.audio.LocalAudioFile object at 0x10712ef90>, 'cucumbers': <echonest.remix.audio.LocalAudioFile object at 0x10712e990>}

(Note that pickle itself does not work on the echonest LocalAudioFiles, which have their own save() method.

Here's a couple pieces of style advice, sourced from Python's style guide (PEP0008).

Keyword arguments shouldn't have space on either side of the equals sign:

def get_saved(directory='audio/', extension='en'):

Good that you're including a docstring, but one line docstrings should be on one line.

"""Get list of files, add them to a queue and return the queue."""

Also there may be a reason, but why not use the os module's os.path.join function instead of concatenation when making paths? It checks the OS you're on to ensure it's using the right syntax. It's generally a safer way than concatenation.

return glob.glob(os.path.join(directory, '*.' + extension))


Sorting strings in a one dimensional array What are the better solutions possible?

I have a character array with different strings. There is a max size for the strings. In case a string is smaller than max size remaining places are filled with '-'.
Sorting has to be done on this.

  Example:

  maxStringSize = 5

  totalStrings = 4

  InputArray: w,a,t,e,r, c,a,t,-,-, f,i,v,e,-, b,a,l,l,-,-

  outputArray: b,a,l,l,-,-, c,a,t,-,-, f,i,v,e,-, w,a,t,e,r

-

public class QuestionOne {

char[] mInput = {'w','a','t','e','r','c','a','t','-','-','f','i','v','e','-','b','a','l','l','-','-'};
int mBlockSize = 5;
int mTotalBlocks = 4;

public void init(char[] input, int blockSize, int totalBlocks)
{
    mInput = input;
    mBlockSize = blockSize;
    mTotalBlocks = totalBlocks;
}

public char[] sort()
{

    for(int i=0; i < mTotalBlocks; i++)
    {
        for(int j=i+1; j<mTotalBlocks; j++)
        {
            for(int count = 0; count < mBlockSize; count++ )
            {
                if(mInput[i*mBlockSize+count]>mInput[j*mBlockSize+count])
                {
                    swapBlocks(i,j);
                    break;
                }
                else if(mInput[i*mBlockSize+count]<mInput[j*mBlockSize+count])
                    break;
            }
        }
    }
    return mInput;
}

private void swapBlocks(int blockOne, int blockTwo)
{
    for(int i=0; i<mBlockSize; i++)
    {
        swap(blockOne*mBlockSize+i,blockTwo*mBlockSize+i);
    }
}

private void swap(int one, int two)
{
    char temp = mInput[one];
    mInput[one] = mInput[two];
    mInput[two] = temp;
}

}

What are the better solutions possible?

How about the simple process of converting the chars to separate strings, sorting the Strings, then copying the results back to an array?

It would look something like:

String source = new String(input);
String[] values = new String[input.length / blocksize];
for (int i = 0; i < values.length; i++) {
    values[i] = source.substring(i * blocksize, (i + 1) * blocksize);
}
Arrays.sort(values);
char[] result = new char[values.length * blocksize];
for (int i = 0; i < values.length; i++) {
    System.arraycopy(values[i].toCharArray(), 0, result, i* blocksize, blocksize);
}
return result;


Improving the BigPicture I'm currently working on the library BigPicture.js with live demo here.

There surely are lots of little improvements possible in the main .js file (< 300 lines of code), here are some little questions :

Is it common to use something like :

(function() {
  // code here
} ) ();

to launch the code and to be able that return; will work ? Or are there more common schemes ?
Should I place this at the end of the <body> in the mail .html file or in the <head> part?  Should I launch this code on something like DOMContentLoaded ?
When zooming to much (using the zoom feature I provide with mousewheel or double-click or PGUP/DOWN etc.) with Chrome (for example when a single letter uses full browser height, i.e. the text is really big), then panning (with CLICK+DRAG ) is really slow.  It is not true at all on Firefox, not on IE. What could be the reason (and solution) for this slowness with Chrome only?
Are there some other obvious improvements? I taught myself JavaScript (but with help of SO questions/answers of course), so there are surely some bad coding habits in my code, right?

Code for panning (a little bit shortened, see the project's page for real code)

  dragging = false;

  bpContainer.onmousedown = function(e) {
    dragging = true;
    previous_mouse = {x: e.pageX, y: e.pageY};
  }

  window.onmouseup = function() {
    dragging = false;
  }

  bpContainer.ondragstart = function(e) {
    e.preventDefault();
  }

  bpContainer.onmousemove = function(e) {
    if (dragging) {
      bp.style.transitionDuration = "0s";
      bp.x += e.pageX - previous_mouse.x;
      bp.y += e.pageY - previous_mouse.y;
      bp.style.left = bp.x + 'px';
      bp.style.top = bp.y + 'px';
      current.x -= (e.pageX - previous_mouse.x) * current.zoom;
      current.y -= (e.pageY - previous_mouse.y) * current.zoom;
      previous_mouse = {x: e.pageX, y: e.pageY};
    }
  }

I've used that before in my code. I find it is often used in libraries as an "initializer" that sets up the code and DOM for the library rather than forcing the user of the library to have to call an initialization function.
It depends. If you need to interact with the DOM right away(as in, find elements, change properties, or set element events), the code has to be at the bottom of the body. If it's just function declarations and definitions, you can keep your code in the head
Is this a question?
There is nothing that really pops out. You have very nice spacing in your code(everything isn't bunched together) and your formatting is perfect(in my opinion).


Custom database Lock - implemented with IDisposable I have a lock class, that handels a database lock. It's not important how.
It is used in the beginning of a large operation, here is how it is used today:

public void LargeOperation()
{
    try
    {
        MyLock.DoLock("SomeId");
        // Do lots of stuff that might thorw exceptions
    }
    finally
    {
        MyLock.ReleaseLock("SomeId");
    }
}

I would like to use it like this instead:

public void LargeOperation()
{
  using(new MyLock("someId"))
  {
    // Do lots of stuff that might thorw exceptions
  }
}

this works fine, but im not sure wether it is a bad idea. I have implemented IDisposable on my class MyLock but it doesn't dispose.. instead it releases the lock.
Here is how the class look:

public class MyLock : IDisposable
{
    private string _id;
    public MyLock(string id)
    {
       DoLock(id);
    }

    public static void DoLock(string id)
    {
      //..code omitted
    }

    public static void ReleaseLock(string id)
    {
      //..code omitted
    }

    public void Dispose()
    {
      ReleaseLock(_id);
    }
}

Is this abuse of the IDisposable interface?

The IDisposable was primarily added in order to be able to release acquired unmanaged resources in a deterministic fashion. However it also provides a nice way of releasing other resources in a deterministic fashion as you have found.

I would not say that this is an abuse of the disposable pattern: You acquire a lock which is a resource and you need to release it at a given point in time. For me this is good enough especially since you said it's some kind of db lock which I assume lives outside of the application (that is unmanaged for me). It provides you with native support through using to make sure it's released in case the executing code throws an exception.

However I don't particularly like your two public static methods on there. Now you all of a sudden have two interfaces into the class. If you want a factory like interface then make the DoLock method a proper factory method returning the lock which was acquired (in which case the ctor should become private):

public class MyLock : IDisposable
{
    private string _id;

    private MyLock(string id)
    {
        _id = id;
        DoLock();
    }

    public static MyLock Acquire(string id)
    {
        return new MyLock(id);
    }

    private void DoLock()
    {
        // get the lock
    }

    public void Release()
    {
        // release it
    }

    public void Dispose()
    {
        Release();
    }
}

While you could do without a public Release method having Dispose calling Release is semantically useful in case you can't use using and need to release the lock explicitly. In this case the code will be clearer when you call Release rather than Dispose. The .NET stream classes do a similar thing (where Dispose calls Close).

Some things to consider:

Add an "object is disposed" flag so you don't try to release the same lock twice.
Add a finalizer to try and release the lock in case it was not properly released. Add  a Debug.Assert at least for that case. Note that adding a finalizer will make all MyLock objects go through the finalizer queue which will add overhead for the GC. You could wrap the finalizer into a #ifdef DEBUG to only do that in debug builds - depends how many of these locks you create (although I guess if you create lots of them you have another problem altogether).


Shuffling function using rand() I would like to hear your suggestions about my shuffling function in C++. I've decided to take advantage of using rand() to build a shuffling function.

#include <iostream>
#include <vector>
#include <ctime>

bool find(const std::vector<int>& vec, int index)
{
    for (int i = 0; i < vec.size(); ++i){
        if ( index == vec[i] )
            return true;
    }

    return false;
}

void shuffle(int array[], const int size)
{
    int temp[size];
    std::vector<int> indices;

    for ( int i(0); i < size; ++i)
        temp[i] = array[i];

    int index = rand() % size;
    indices.push_back(index);

    for ( int i = 0; i < size; ++i){
        if ( i == 0 )
            array[i] = temp[index];
        else{
              while( find(indices, index) )
                index = rand()%size;

            indices.push_back(index);
            array[i] = temp[index];
        }
    }

}

int main(void)
{
    srand((unsigned int)time(NULL));

    int a[7] = {1,2,3,4,5,6,7};

    for ( int i = 0; i < 7; ++i)
        std::cout << a[i] << " ";
    std::cout << std::endl;

    shuffle(a, 7);

    for ( int i = 0; i < 7; ++i)
        std::cout << a[i] << " ";
    std::cout << std::endl;

    return 0;
}

The result:

1 2 3 4 5 6 7
3 7 1 6 4 5 2

Algorithm

This algorithm is already provided by the standard library.
std::random_shuffle()
If you want to implement it yourself then this is not very optimal (as @rolfl mentioned this is \$O(N^2)\$ at best). @rolfl suggests using Fisher-Yates I call it the Knuth shuffle but basically it is the same thing (and Knuth is a cooler name to throw around :-)

The Knuth shuffle is basically:

Pick a random value in your container. Swap it with the last element.
Reduce the size of the elements you consider for swapping by 1.
if size > 0 goto 1

Rand

The rand algorithm is known to be bad so consider using the new random number generators provided by the standard. If you want to stick to rand() then at least try and use it in a way that does not have bias (or worse bias).

index = rand()%size;

Does not give an even distribution (unless RAND_MAX % size == 0). If the modulus is not zero then a couple options have a slightly higher probability. The correct way to do it is:

// To make sure you have an even distribution you
// must discard all values that contribute to it being uneven.
// So first calculate the max value where we can evenly distribute
// input values [0..max)
//
// If rand() generates a value greater or equal to max then discard it
// as this is part of the unevenness. Otherwise do your modules and return.
int randEven(int max)
{
     maxRange = RAND_MAX / max * max; // Note division is integral
                                      // So result is not necessarily RAND_MAX
     for(;;)
     {
         int r = rand();
         if (r < maxRange)
         {    return r % max;
         }
     }
}

Interface

void shuffle(int array[], const int size);

This is a very C like interface. It also limits you to using C-Arrays (In C++ there are lots of container types). So I would change this to use iterators.

template<typename T>
void shuffle(T begin, T end);

You can still use the same function with your C array (as pointers implement the iterator interface).

shuffle(a, 7);

// becomes

shuffle(&a[0], &a[7]);

Code Review

Try to always use braces.

    for ( int i(0); i < size; ++i)
        temp[i] = array[i];

Its a good habit to get into as there are a few corner cases where it matters and if you just do it by habit you will not hit them.

    for ( int i(0); i < size; ++i) {
        temp[i] = array[i];
    }

Prefer to never use C cast style.

Use a C++ cast. They are easy to spot and the compiler can check most of them for correctness. The dangerous ones are then easy to find during code review.

    srand((unsigned int)time(NULL));

    // The C++ way.
    srand(static_cast<unsigned>(time(nullptr)));

    // Alternatively you can use the constructor syntax (not official name)
    // Personally don't like this.
    srand(unsigned(time(nullptr)));

If you are initializing arrays like this then don't specify a size:

    int a[7] = {1,2,3,4,5,6,7};

    //  Don't say seven
    int a[] = {1,2,3,4,5,6,7};

In this version the compiler calculates the correct size. If you had accidentally used an incorrect size then the compiler can generate incorrect code. If you had used 8 then the compiler fills the array with zeros. If the you had used 6 then you overrun the size of the array. So prefer to let the compiler work it out.

Prefer to use "\n" rather than std::endl. This is because std::endl adds '\n' and then flushes the stream. Unless you really want to flush the stream use "\n". You rarely want to do this as the streams will do it automatically when they fill up.

    std::cout << std::endl;


Making prime (sieve) code faster and able to run on big numbers For an input triple (a, b, c), the task is to count the number of integers in the inclusive range [a, b] which have c distinct prime factors. This entails factoring each of the numbers in the range [a, b] and establishing how many distinct prime factors each has.

I need faster code so my program runs faster and works on big numbers.

#include<bits/stdc++.h>
using namespace std;

bool f[100000001];
void siv()
{
    f[1] = 1;
    for (long long i = 2; i * i <= 100000001; ++i)
    {
        if (!f[i])
            for (long long j = i * i; j <= 100000001; j += i)
                f[j] = 1;
    }
}

int main()
{
    siv();
    vector<long long>V;
    long long a,b,c;
    int t,counter=1;
    cin >> t;
    while (t--)
    {
        int count=0;
        cin >> a >> b >> c;

        for (long long i=a; i <= b; ++i)
        {
            V.clear();
            for(long long q = 1; q <= i; ++q)
            {
                if(i % q == 0)
                {
                    if (!f[q])
                        V.push_back(q);
                }
            }
            if (V[0] == 1)
                V.erase(V.begin());
            if (V.size() == c)
            {
                ++count;
            }
        }
        cout << "Case #" << counter++ << ": " << count << endl;
    }
    return 0;
}

Example input for what I need:

5 15 2
2 10 1
24 42 3

The output should be like this:

5
7
2

\$2 \le a \le b \le 10^7\$

\$1 \le c \le 10^9\$

The output shows how many integers in the inclusive range [a, b] have a prime of exactly c.

In the first test case, the numbers in the inclusive range [5, 15] with c= 2 are 6, 10, 12, 14, and 15. All other numbers in this range have c=1.

using namespace std;

It won't matter much with this program, but this can be a bad habit to start.  See Why is using namespace std bad practice?

Sieve of Eratosthenes

bool f[100000001];
void siv()

You're asking for a code review, so you want to encourage people to read your code.  You could make it easier on us by picking self-commenting names.  For example, I think these would be more recognizable as

const int MAXIMUM_VALUE = 100000001;
bool is_not_prime[MAXIMUM_VALUE];
void sieve_of_erastothenes()

Now I can easily see what the upper bound is, what is_not_prime holds, and know what the function does.  I can also reuse MAXIMUM_VALUE and if I type it incorrectly, I'll get a compiler error.  If I miscount the zeroes in 100000001, it will just silently do the wrong thing.

for (long long i = 2; i * i <= 100000001; ++i)
{
    if (!f[i])
        for (long long j = i * i; j <= 100000001; j += i)

You do i * i twice.  You could save it the first time and avoid that.

for ( long long i = 2, j = i * i; j < MAXIMUM_VALUE; ++i, j = i * i )
{
    if ( ! f[i] )
    {
        for ( ; j < MAXIMUM_VALUE; j += i )
        {

Not much of an improvement but some.  Although the compiler might do this for you already.

Note that I also changed j <= to j <.  This prevents us from writing past the end of the array.  The array is 0-indexed, so its maximum index is one less than its size.

Even better might be

int *prime_factor_counts = NULL;
void sieve_of_erastothenes(const size_t maximum_value) {
    prime_factor_counts = calloc(maximum_value+1, sizeof(int));

    if ( NULL == prime_factor_counts )
    {
        std::cerr << "Failed to allocate array." << std::endl;
        exit(EXIT_FAILURE):
    }

    prime_factor_counts[1] = 1;
    for ( size_t i = 2; i*i <= maximum_value; ++i )
    {
        if ( ! prime_factor_counts[i] )
        {
            for ( size_t j = i; j <= maximum_value; j += i )
            {
                ++prime_factor_counts[j];
            }
        }
    }
}

Switching from prime_factor_counts[j] = 1 to ++prime_factor_counts[j]; is about the same work but saves additional information.  Now we have a record of the count of the prime factors of each number.  Note that we are doing more work now (since j starts at i rather than i * i), but it's work that we probably needed to do anyway.

I dropped long long in favor of size_t.  The reason is that we can't create an array with more than size_t elements.  So there's no point in creating an index variable of any other type.

In C++, elements are undefined unless you specifically set them.  That was a bug in your original program.  It's fixed here by using calloc which explicitly zero-initializes the memory it allocates.

main

I'm going to simplify main a bit.

int main()
{
    int input_row_count;
    std::cin >> input_row_count;
    std::cout << process_input(input_row_count, 1, 2);
}

This gets most of the logic outside main.  We only get the number of rows of inputs directly.  Everything else comes through the process_input function.

Technically speaking, we should probably check for an input error.  However, if you're willing to live with the consequences of not checking, then I'm not going to insist.

process_input

std::string process_input(int input_remaining_count, int iteration_count, size_t maximum_value)
{
    if ( 0 >= input_remaining_count )
    {
        sieve_of_eratosthenes(maximum_value);
        return;
    }

    size_t start, end, desired_factor_count;
    std::cin << start << end << desired_factor_count;

    if ( maximum_value < end )
    {
        maximum_value = end;
    }

    std::string output = process_input(input_remaining_count - 1, iteration_count + 1, maximum_value);

    size_t count = count_matching_numbers(start, end, desired_factor_count);

    std::ostringstream out;
    out << "Case #" << iteration_count << ": " << count << '\n' << output;

    return out.str();
}

The big point to this is to find the maximum value for which we need a factor count before running the sieve_of_eratosthenes.  This way we don't have to always calculate a large number.

The second point is that this allows us to only calculate factor counts once across the entire input.  The other way, we had to recalculate the factor counts once for each range.  This puts the calculation work entirely in the sieve_of_eratosthenes function which had to do most of it anyway.

The original algorithm was probably better for short ranges with high start and end values.  It also used less space, since it just saved whether or not a number was prime and the prime factors of a particular number.  I don't know if the space increase matters or not given your usage.

count_prime_factors

size_t count_matching_numbers(size_t start, size_t end, size_t desired_factor_count)
{
    size_t count = 0;
    for ( size_t i = start; i < end; i++ )
    {
        if ( prime_factor_counts[i] == desired_factor_count )
        {
            ++count;
        }
    }

    return count;
}

Other issues with original

vector<long long>V;

It's pretty common to use ALL_CAPS to mean a constant.  This is not a constant, so it might be better named prime_factors.

        for(long long q = 1; q <= i; ++q)
        {

But later you say

        if (V[0] == 1)
            V.erase(V.begin());

It would be easier to replace both statements with just

        for ( size_t j = 2; j <= i; ++j )
        {

Now there's no chance of adding 1 to the vector, so you'll never need to remove it.

I'm also unclear on why the variable is named q.  The more common inner loop index variable would be j.

            if(i % q == 0)
            {
                if (!f[q])
                    V.push_back(q);
            }

It may be better if you did the checks in the reverse order.

            if (!f[q])
            {
                if(i % q == 0)
                    V.push_back(q);
            }

Checking if a value is zero should be simpler than checking if the remainder from a division is zero.  Of course, the pointer arithmetic may mess that up.  It may be worth timing both ways.

vector<long long>V;
        V.clear();
                    V.push_back(q);
        if (V.size() == c)
        {
            ++count;
        }

Since you only ever check the size of the vector, you don't actually need it.  You could replace it with just a counter variable:

size_t prime_factor_count;
        prime_factor_count = 0;
                    ++prime_factor_count;
        if ( prime_factor_count == c )
        {
            ++count;
        }

OOP

I didn't try to change things, but instead of making prime_factor_counts a global variable, you could have made it an object variable.  Then both count_matching_numbers and sieve_of_eratosthenes could have been object methods.  You could even make prime_factor_counts and sieve_of_eratosthenes static so as to share them across multiple objects.  As it is, this code is rather procedural.  Why use C++ to write C?


Attempting to build template but Having Issue with gets and posts I am building my first real php web app. as many you know this requires building LOTS of pages.

in attempting to streamline the repetitive stuff i placed most of my stuff within a content.php page which looks like this

?php include_once ('config3.inc');?>
<?php if(isset($_GET['p'])){
    $page = $_GET['p'];

}else {
    $page ='';
}
?>
<!DOCTYPE html>

<meta name="viewport" content="width=device-width, initial-scale=1.0">
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
    <title><?php echo $_SERVER['PHP_SELF'];?></title>
    <?php include('styles.inc');?>

   <?php include('api.inc'); ?>
   <?php include('scripts.inc');?>

    <?php include ('header.inc');?>

    <?php include  ('./content/'.$page);?>

<?php include ('footer.inc');?>

This way when I want to call up a page I go to www.domain.com/content.php?p=pagename.php and it will "wrap" everything in the correct style sheets container and footer etc.

had been working well until I started making my CRUD pages requiring that pagename use POST data.

Is there a better approach for this or do I need to scrap this approach entirely?

Well a more elegant approach would be to use an MVC pattern as a starter framework I would suggest you to use Laravel it's very clear written and also an easy catch for people who are general new to MVC. The way your currently writing the code, you include to much business logic inside of it.

Which in the long term is terrible because the code becomes un-maintainable, for that the MVC Pattern is the best way to write clear and maintainable code, but check it out yourself. An good article to start with should be this one by Nettuts, called MVC for Noobs don't get irritated by the name the article is pretty good and should get you a basic understanding of the MVC pattern.

Hope I could be a little help :)


Using if/else by checking page URL You can place the script where you want the content to be checked and loaded in your design.

urlCheck is the name given to the function.  You can change it, and if you do, change all urlCheck references within the code.
window.location.href returns the current address as http://....  Use this to identify certain pages on which you want the content to be loaded.  Make as many if/else combinations as necessary.
document.write builds the content to be loaded first.  Type with later in one unique line or create multiple document.writes for each line of your content.

Or run your code when the document is ready, by putting it at the end of the page (or use a framework which supports something like $(document).ready()):

//-->

As I already said, that's a relic, stop doing it.


Comma Code - Ch. 4 Automate the Boring Stuff I am new to programming and currently learning Python using "Automate the Boring" by Al Sweigart. I've just completed the "Comma Code" task in Chapter 4 and would like some feedback on my solution. The task is:

Say you have a list value like this:
spam = ['apples', 'bananas', 'tofu', 'cats']

Write a function that takes a list value as an argument and returns a string with all the items separated by a comma and space, with and inserted before the last item. For example, passing the previous spam list to the function would return 'apples, bananas, tofu, and cats'. But your function should be able to work with any list value passed to it.

I tried to come up with something that mostly sticks to what you learn by this point in the book, but I did use .join() which is from a few chapters later. I also tried to make sure my code works with lists containing strings and/or integers, and for the output to be correct independent of list size (no comma when the list has two items, etc.).
your_list = ['item1', 'item2', 3, 'item4', 'item5', 'item6', 7, 'item8', 9]

def list_concatenator(your_list):
    items = len(your_list)
    if items == 0:
        return 'Your list is empty'

    elif items == 1:
        return "'" + str(your_list[0]) + "'"

    elif items == 2:
        return "'" + str(your_list[0]) + ' and ' + str(your_list[1]) + "'"

    else:
        your_list_split1 = (', '.join((map(str,your_list[:-1]))))
        return "'" + your_list_split1 + ', and ' + str(your_list[-1]) + "'"

list_concatenator(your_list)

I realize these exercises have been answered before, I'm just looking for some feedback on my solution.

Your code is well-built and covers everything stated in the task, but could be made much shorter by using the .format() function.

  str.format() is one of the string formatting methods in Python 3, which
  allows multiple substitutions and value formatting. This method lets
  us concatenate elements within a string through positional formatting.

Your code can, therefore, be made shorter and more concise this way. Here is what your code might look like -

def list_concatenator(your_list):

     if not your_list:
         return "Your list is empty!"

     elif len(your_list) == 1:
         return str(your_list[0])

     elif len(your_list) == 2:
         return '{0} and {1}'.format(your_list[0], your_list[1])

     else:
         body = ", ".join(map(str, your_list[:-1]))
         return '{0}, and {1}'.format(body, your_list[-1])

I have used {0} and {1} so that positional arguments can be placed in the order you want them to be in, though it is not necessary to do so.

OR

Another way to format in Python 3 is to use f-strings.

  The idea behind f-strings is to make string interpolation simpler. To create an f-string, prefix the string with the letter “ f ”. The string itself can be formatted in much the same way that you would with str.format(). f-strings provide a concise and convenient way to embed python expressions inside string literals for formatting.

Here's how your code would look like with f-strings (much shorter) -

def list_concetenator(your_list):

    if not your_list:
        return "Your list is empty!"

    elif len(your_list) == 1:
        return str(your_list[0])

    elif len(your_list) == 2:
        return f'{your_list[0]} and {your_list[1]}'

    else:
        body = ", ".join(map(str, a_list[:-1]))
        return f'{body}, and {your_list[-1]}'

Here are some example outputs -

your_list = ['item1', 'item2', 3, 'item4', 'item5', 'item6', 7, 'item8', 9]

print(list_concatenator(['item1', 'item2', 3, 'item4', 'item5', 'item6', 7, 'item8', 9]))
>>> item1, item2, 3, item4, item5, item6, 7, item8, and 9

print(list_concatenator(['item1', 'item2', 3]))
>>> item1, item2, and 3

print(list_concatenator(['item1', 3]))
>>> item1 and 3

print(list_concatenator(['item1']))
>>> item1

print(list_concatenator([]))
>>> Your list is empty!

The program also takes care of the Oxford comma, where lists with two items do not have commas (e.g. apples and bananas) but lists with more than two items are separated with commas (apples, bananas, and cakes). Here are some useful links that help us understand what Oxford commas really are and when they should be used -

https://www.grammarly.com/blog/comma-before-and/
https://en.wikipedia.org/wiki/Serial_comma

Overall, I believe formatting was the main problem in your code.

EDIT -

In the comments section above, @Ilmari Karonen correctly mentions that the usage of " ' " in the book is the book's way of writing string literals. Therefore, I have edited my answer to make this change (i.e. remove unnecessary quotes). But if you require these quotes, then you could always wrap the string in double quotes, like this - f"'{your_list[0]} and {your_list[1]}'" or this - "'{0} and {1}'".format(your_list[0], your_list[1]).

Another let-down in your code is the use of inconsistent parentheses here,

(', '.join((map(str,your_list[:-1]))))

which could just be written as -

', '.join(map(str,your_list[:-1])).

To remain concise and immaculate in writing programs, I suggest you have a look at PEP 8, which is Python's official style guide.

Hope this helps!


Basic Backbone application using d3 I've started my first application using backbone and have something basic up and running, backbone seems to give a lot of freedom to application design and I don't know what I'm doing right and wrong in that regard. I'm just after some tips and criticism.

The first view is for the details which should be visible when the page loads:

var DetailsView = Backbone.View.extend({

    el: $(".details"),

    initialize: function(options) {
        // nothing here yet
    },

    render: function() {
        // nothing here yet
    },

    events: {
        "click .btn": "setDetails"
    },

    setDetails: function(e) {
        this.$el.hide();
        // will do validation here...
        this.options.dataEditorView.$el.show();
        this.options.finalView.render();

    }

});

Once a user has completed the details form for that view, the data edit view is displayed:

var DataEditorView = Backbone.View.extend({
    el: $(".data-editor"),

    initialize: function(options) {

    },

    events: {
        "keypress .point-x": "addSeriesPoint",
        "keypress .point-y": "addSeriesPoint"
    },

    addSeriesPoint: function(e) {
        if (e.keyCode != 13) return;
        //if (!this.input.val()) return;

        console.log('adding point');

        var point = [];
        point[0] = $("#input-point-x").val();
        point[1] = $("#input-point-y").val();

        Series.create({
            x: $("#input-point-x").val(),
            y: $("#input-point-y").val()
        });

        var points = this.options.seriesCollection.toJSON();
        this.options.summaryView.addPoint(points);

        $(".point-x").val("");
        $(".point-x").focus();

    }

});

And the summary view which is visible while the date editor view is visible:

var SummaryView = Backbone.View.extend({
    initialize: function() {

        console.log('initialising...');

        this.canvas = d3.select(this.el)
                        .append("svg")
                        .attr("width", 600)
                        .attr("height", 500);

    },

    className: "summary",

    render: function() {
        $("body").append(this.el);
    },

    addPoint: function(points) {
        console.log(points);
        this.canvas.selectAll("circle")
                    .data(points)
                    .enter()
                    .append("circle")
                    .attr("r", 42)
                    .attr("cx", function(d) { return d.x })
                    .attr("cy", function(d) { return d.y })
                    .transition().selectAll('circle')
                    .ease('bounce').duration(900)
                    .attr('r', function (d) {
                        console.log(d);
                        return 99; //ugh
                    });
    }

});

DetailsView

You should pass jquery selector to the el — Backbone will handle it. Also, note that you should pass DOM element to the el property.

Injecting one view into another is often a bad practice (if relation is not like parent and subview): is.options.dataEditorView.$el.show(); this.options.finalView.render();

Consider triggering event like validation-success from DetailsView. Subscriber to this event should be in the route or other main object.

DataEditorView

var point = {
    x: $("#input-point-x").val(),
        y: $("#input-point-y").val()
};

Series.create(point);
// Probably you want to add new point to existing collection this.options.seriesCollection?

Another view injection: this.options.summaryView.addPoint(points);

Just add new point into collection and subscribe to event in the summaryView (see next block).

// Micro optimization
var pointX = $(".point-x");
pointX.val("");
pointX.focus();

SummaryView

Add view.listenTo(seriesCollection, 'add', addPoint) into initialize step. addPoint should accept parameters: model, collection, options, where model is a new point which was added to the collection.

In this function you should run collection.toJSON() to prepare points for D3.data().


A better way of always dumping Gedit out of Terminal? I use terminal alot, so it was frustrating to me to deal with gedit's inconsistent behavior between having a window open and when one was not. I ended up searching around, and finding a way of getting around this. I've created this bash script, and aliased it to gedit, and I've slowly been expanding it.

Basically the script does these things:

Checks to see if arguments exist, if not, it opens gedit with no document
If arguments do exist, it iterates over each and opens them all.

This creates consistent behavior where gedit won't stick in terminal dumping log data.

#!/bin/bash

if [[ -z $1 ]]; then
    /usr/bin/gedit &
else
    for var in "$@"
    do
        /usr/bin/gedit "$var" &
    done
fi

My question is if there are any glaring errors here, or if there is a cleaner, faster way of doing this? I'm specifically interested in the loop, as I feel there should be an easier way of passing list data as a single string. The reason I didn't just use $* for that, is when I tried the fact that it's wrapped in quotes caused it to open a single document with all parameters listed as the file name.

I added the quotes for a reason that I should have documented, but I currently do not remember.

If you just want to make gedit operate silently and in the background, I would create a function (save in ~/.bashrc)

gedit() { command gedit "$@" &>/dev/null & }

Also, Accessing bash command line args $@ vs $*


Determining which octant has a specific point I am interested in octants in plane geometry, which basically involves dividing a plane into 8 equal parts according to a certain point.

Octants are particularly useful while implementing the Bresenham's algorithm.

Problem

Given two points A = (x1, y1) and B = (x2, y2), how to determine in which octant belongs B according to A?

Counting octants is performed as shown: starting from the upper right and iterating anticlockwise.

Naive implementation

def get_octant(pA, pB):
    AX = pA[0]
    AY = pA[1]
    BX = pB[0]
    BY = pB[1]

    if BX > AX:
        if BY > AY:
            if (BY - AY) < (BX - AX):
                octant = 0
            else:
                octant = 1
        else:
            if (AY - BY) < (BX - AX):
                octant = 7
            else:
                octant = 6
    else:
        if BY > AY:
            if (BY - AY) < (AX - BX):
                octant = 3
            else:
                octant = 2
        else:
            if (AY - BY) < (AX - BX):
                octant = 4
            else:
                octant = 5

    return octant

Would it not possible to find a cleverer implementation?

Using the symmetry properties, I am convinced that it is possible to make a shorter and more readable code. Unfortunately, I do not find it myself, so does anyone have an idea, please?

I am convinced that it is possible to make a shorter and more readable code.

To implement the discrete analog of atan2((B.y - A.y), (B.x - A.x)):

x, y  = (B.x - A.x), (B.y - A.y)
octant = [([1, 2], [8, 7]), ([4, 3], [5, 6])][x < 0][y < 0][abs(x) < abs(y)]

It is shorter, I don't know about readability. It is easy to write tests that check all possible variants including the behavior on the boundaries.


MySQL connection using a static connection I have been using a static class to connect and retrieve information from a MySQL database and also reading about how static works in php. I'm still not sure if the code is bad or not and why (besides the usual static are dangerous). Also, code improvements suggestions will be appreciated.

class Database {

    const STOREDPROCEDURE = 1;
    const ADHOC = 2;

    private static $instance;

    private static function call() {

        if (!isset(self::$instance)) {
            self::$instance = new MySQLi('localhost', 'root', '', 'alfred');
            if (self::$instance -> connect_error) throw new Exception(self::$instance -> connect_error);
            self::$instance -> set_charset('utf8');
        }

        return self::$instance;

    }

    public static function query() {

        //
        $arguments = func_get_args();
        $count = count($arguments);

        //
        $references = array();
        for ($i = 2; $i <= count($arguments) - 1; $i++) $references[$i] = &$arguments[$i];

        //
        $query = $count === 0 ? '' : str_repeat('?, ', $count - 4) . '?';
        $query = 'CALL' . ' ' . $arguments[1] . '(' . $query . ');';

        //
        $stmt = self::call() -> prepare($query);
        if ($count > 2) call_user_func_array(array($stmt, 'bind_param'), $references);

        //
        $stmt -> execute();
        $stmt -> store_result();

        //
        $result = array();
        while ($metadata = $stmt -> result_metadata()) {

            //
            $fields = array();
            foreach ($metadata -> fetch_fields() as $field) {
                $reference[$field -> name] = '';
                $fields[$field -> name] = &$reference[$field -> name];
            }

            //
            call_user_func_array(array($stmt, 'bind_result'), $fields);

            //
            $rows = array();
            while ($stmt -> fetch()) {
                $row = new stdClass();
                foreach ($fields as $name => $value) $row -> $name = $value;
                array_push($rows, $row);
            }

            //
            array_push($result, empty($rows) ? null : $rows);

            //
            $stmt -> next_result();

        }

        //
        $stmt -> free_result();
        $stmt -> close();

        //
        return $result;

    }

}

You can use the code by doing:

        $response = Database::query(
            Database::STOREDPROCEDURE,
            'UserRead',
            'sss',
            $useremail,
            $userpassword,
            $userhash
        );

Right now, it does not seem possible to run every type of query. For example: "SHOW FULL PROCESSLIST" or just "SELECT id FROM xyz LIMIT 0,10". Apart from this, there are a lot of things which might possibly go wrong.

What if I call the function with just one argument? arguments[1] would not exist and the query would become CALL (?). This statement is prepared and executed, but there is nothing executed.

I would suggest keeping things more easy instead of trying to merge several things together.

create a single query method which just runs "ADHOC" queries (no prepare)
create a prepare statement where you can bind parameters to the query (and execute)


Two similar functions for writing data to binary file using BinaryWriter I have two functions for writing data to binary file using BinaryWriter.
The first function has single data object as argument and looks as below:

    // write single data object to binary file
    public void AddHeaderToIndexer(string path, RollHeader header)
    {
        bool isDone = false;
        bool error = false;

        DateTime quitTime = DateTime.Now.AddMilliseconds(500);

        while (!isDone && DateTime.Now < quitTime)
        {
            try
            {
                BinaryWriter writer;
                using (writer = new BinaryWriter
                    (File.Open(path, FileMode.OpenOrCreate)))
                {
                    writer.Seek((int)writer.BaseStream.Length, new SeekOrigin());
                    WriteSingleHeader(writer, header);

                    isDone = true;
                    if (error)
                        Program.logger.Write(LogLevel.Error, "Done after timeout.");
                }

                SetReadRightToFileForEveryone(path);
            }
            catch (IOException ex)
            {
                if (!error)
                    Program.logger.Write(LogLevel.Error, ex.Message);
                error = true;
            }
        }
    }

The second is very similar, with only difference on accepting List of data objects and writing all List members to binary file. Actually, BinaryWriter is opened in Create unlike the first function, which uses OpenOrCreate.

    // write list of fdata objects to binary file
    public void WriteHeadersToIndexer(string path, List<RollHeader> headers)
    {
        bool isDone = false;
        bool error = false;

        DateTime quitTime = DateTime.Now.AddMilliseconds(500);

        while (!isDone && DateTime.Now < quitTime)
        {
            try
            {
                BinaryWriter writer;
                using (writer = new BinaryWriter
                    (File.Open(path, FileMode.Create)))
                {
                    foreach (RollHeader header in headers)
                        WriteSingleHeader(writer, header);

                    isDone = true;
                    if (error)
                        Program.logger.Write(LogLevel.Error, "Done after timeout.");
                }

                SetReadRightToFileForEveryone(path);
            }
            catch (IOException ex)
            {
                if (!error)
                    Program.logger.Write(LogLevel.Error, ex.Message);
                error = true;
            }
        }
    }

I'm not able to find some elegant solution, how to separate all duplicate code, which is around the using BinaryWriter block. Also, you will maybe find more elegant solution of my error handling in this case. I'll be glad for such comments, too.

Both methods are public so I would like to suggest doing proper argument validation up front which enables you to throw the correct exceptions and return early.
Use the var type if the right hand side of an assignment makes the type obvious.

For instance

var isDone = false;
var quitTime = DateTime.Now.AddMilliseconds(500);

instead of

bool isDone = false;
DateTime quitTime = DateTime.Now.AddMilliseconds(500);

use braces {} although they might be optional. This makes your code less error prone and the intent of the code is more clear.
Don't talk to Program but let Program pass any needed parts to your class and use it then. A child object should only talk to its parent by using events.
you should pass the 500 ms as a optional parameter to that method. This will make your code more flexible in case you need to adjust the timeout.
you should always try to code against interfaces rather than against concrete implementations. So either use IList<RollHeader> or better IEnumerable<RollHeader> as the passed in type of WriteHeadersToIndexer(). This enables you to pass for instance either an List<RollHeader> or RollHeader [] to that method. In fact each type which implements IEnumerable<T> will be good to pass.

From the comments of the question

  I am using different FileModes and Seek() because in the first case, I write single data object to existing file, in the second case, I create new file and write all data objects from List to this file.

Based on the above I would like to suggest adding another method which is called from the former 2 methods. This new method will have one method argument being a FileMode value like so

private void WriteToIndexer(string path, IEnumerable<RollHeader> headers, FileMode mode, double timeOutMs = 500d)
{
    if (path == null) { throw new ArgumentNullException($"{path}"); }
    if (headers == null) { throw new ArgumentNullException($"{headers}"); }
    if (string.IsNullOrWhiteSpace(path)) { throw new ArgumentException("Parameter may not be empty", $"{path}"); }

    if (!headers.Any()) { return; }

    bool isDone = false;
    bool error = false;

    DateTime quitTime = DateTime.Now.AddMilliseconds(timeOutMs);

    while (!isDone && DateTime.Now < quitTime)
    {
        try
        {

            using (var stream = File.Open(path, mode))
            using (var writer = new BinaryWriter(stream))
            {
                writer.Seek((int)writer.BaseStream.Length, new SeekOrigin());
                foreach (RollHeader header in headers)
                {
                    WriteSingleHeader(writer, header);
                }

                isDone = true;

                if (error)
                {
                    Program.logger.Write(LogLevel.Error, "Done after timeout.");
                }
            }

            SetReadRightToFileForEveryone(path);
        }
        catch (IOException ex)
        {
            if (!error)
            {
                Program.logger.Write(LogLevel.Error, ex.Message);
            }
            error = true;
        }
    }
}

this takes advantage of the string interpolation of C# 6.0.

No you can call this like so

public void AddHeaderToIndexer(string path, RollHeader header)
{
    WriteToIndexer(path, new RollHeader[] { header }, FileMode.OpenOrCreate);
}
public void WriteHeadersToIndexer(string path, IEnumerable<RollHeader> headers)
{
    WriteToIndexer(path, headers, FileMode.Create);
}


Haskell union list implementation pt 2 This is my revision from another post

union' :: (Eq a) => [a] -> [a] -> [a]
union' [] list = list
union' (x:xs) list
    | not $ x `elem` list = x : union' xs list
    | otherwise = union' xs list

Any suggestions welcome.

If this is a reinventing-the-wheel question, let's check whether you actually reinvented union:

import Data.List (union)
import Test.QuickCheck

union' :: (Eq a) => [a] -> [a] -> [a]
union' = ...

prop_union :: [Int] -> [Int] -> Property
prop_union xs ys = union' xs ys === union xs ys

main = quickCheck prop_union

It will fail after some tests:

*** Failed! Falsifiable (after 4 tests and 2 shrinks):
[]
[1,1]
[1,1] /= [1]

While your union' returned [1,1] on [] [1,1], Data.List.union returned [1]. That's because it's left-biased. In order to actually reinvent the wheel, you need to get rid of duplicates on the second list:

union' :: Eq a => [a] -> [a] -> [a]
union' xs [] = xs
union' [] ys = nub ys

But it gets weirder. While the left hand side is allowed to have duplicates, elements on the right hand side that were already in the left hand side need to get discarded:

union "Hello" "World" == "HelloWrd"

However, your current version discards elements from the left hand side:

union' "Hello" "World" == "HeWorld"

Your version is right-biased. The original list xs should be contained completely:

union' xs ys = xs ++ deleteDuplicatesAndXSElements ys

The deleteDuplicatesAndXSElements is left as an exercise. If you define it correctly, you can get rid of all the other cases.

By the way, I don't agree with the previous answer concerning ++. Appending is a problem if you do it over and over again, since (a ++ b) ++ c will have quadratic runtime instead of linear with a ++ (b ++ c), but here you're doing it once.

Last but not least, don't use not .. elem. notElem is a function in Prelude.


Locating an image file in a pyGame I am writing a simple game with pyGame.

Structure-wise, I have separated my game into several files:

main.py # the main runner, it loads up the environment
class_tank.py # it defines the player-controlled tank class
class_shell.py # it defines the shells that shoot from the player's tank
class_mob.py # it defines the enemies

The relationship of the modules above is:

main.py imports class_tank.py, class_shell.py and class_mob.py
all of class_tank.py, class_shell.py and class_mob.py are subclasses of pygame.Sprite.sprite.

Currently, I noticed that I am repeating myself by having the following code snippets in all four files:

current_path = os.path.dirname(os.path.realpath(__file__))
resource_path = os.path.join(current_path, 'Resources')
img_folder_path = os.path.join(resource_path, 'Images')
bg_img_path = os.path.join(img_folder_path, "xxxx.png")

xxxx.png is the image I need to load for the background (main.py), for the player (class_tank.py), for bullets (class_shell.py) and for enemies (class_mob.py).

Logically, this code snippet does belong to each of the file, but is there any elegant way to not repeat myself? The only way I can think of is to define an additional helper class.

You don't necessarily need classes in Python. The helper class approach is the same direction as I would go, just with a plain module.
Basically, your resource_path and img_folder_path are constants, so I would simply create a module constants.py with the following:

import os

RESOURCE_PATH = os.path.join(os.path.realpath(__file__), 'Resources')
IMG_FOLDER_PATH = os.path.join(resource_path, 'Images')

You can then simply import constants.py everywhere you need to know those paths.