React app that interacts with CoinMarketCap API I've started to fill my GitHub profile with small applications to educate and showcase my skills. However, I feel like I'm missing something essential, while creating these projects.

Could you take a look at this component, and advice me on how to refactor it? Its purpose is to get Coin Data from an API, and display the chart if the data is present :

import React from 'react';
import {connect} from 'react-redux';

import {formatCrypto} from '../../common/formating';

import ChartContainer from './chartContainer';

class Liquidity extends React.Component {
    constructor(props) {
        super(props);

        this.state = {
            chartData: [],
            minmaxValues: {},
            isLinear: false,
        };
    };

    /***
     * @method getDerivedStateFromProps - defines if the chart should be updated, since useEffect
     * hook was giving a warning for async component rendering
     * @param props - nextProps of the app
     * @param state - nextState of the app
     * @return {null} - returns null when there is no updates for the current state
     */
    static getDerivedStateFromProps({coinsData}, state) {
        if (!coinsData) return null;

        let parsedChartData = [];

        if (coinsData) {
            parsedChartData = [{
                id: 'CoinMarketCap API response',
                data: [...coinsData.map((el) => {
                    return {
                        id: el.id,
                        name: el.name,
                        marketCap: '$' + formatCrypto(el.quote.USD.market_cap, 3),
                        volume: '$' + formatCrypto(el.quote.USD.volume_24h, 3),
                        priceChange: formatCrypto(Math.abs(el.quote.USD.percent_change_24h)) + '%',
                        x: el.quote.USD.market_cap,
                        y: el.quote.USD.volume_24h,
                        z: Math.abs(el.quote.USD.percent_change_24h)
                    }
                })]
            }];

        }

        return {chartData: parsedChartData};
    }

    swapChart = () => {
        this.setState((prevState) => ({isLinear: !prevState.isLinear}));
    };

    render() {
        const {chartData, isLinear} = this.state;

        return (
            <>

Liquidity

                    <button
                        className={'btn btn-primary'}
                        type="button"
                        onClick={() => this.swapChart()}
                    >
                        Switch to {isLinear ? 'logarithmic' : 'linear'}
                    </button>

                <ChartContainer chartData={chartData} isLinear={isLinear}/>
            </>
        )
    }
}

const mapStateToProps = (state) => {
    const {coinsData} = state.cmcListCallReducer;

    return {coinsData};
};
export default connect(mapStateToProps)(Liquidity);

This component is a part of the largest app I've created so far. The app repo is here, in case you might be interested in a big picture. I will appreciate any kind of critics related to the project in general, since maybe my structure sucks more, than code styling.

You're using redux for state management. The problem with mixing local state and redux state is that component render cycles are asynchronous, which oftentimes can cause finicky race conditions. If you're going to use redux, commit to it. If you aren't using any middlewares like redux-thunk, I highly suggest you do so. Dispatching actions and making api calls are ensured to be synchronous when using thunk actions

Your component is "responsible" for too much. It's a violation of the single responsibility principle, makes the component harder to reason, and is [far] more difficult to test. One of the main selling points of redux is the ability to tuck away how data is being passed. It lets you create a separation of concerns and will make your components much easier to reason and maintain. If I were in your position, I would create the following:

Make your api call inside a redux thunk action, and store the raw values in the store

Data calls and transfer are ensured to be synchronous. I would place the data in its entirety, just in case you need the raw data elsewhere

getCoinsData = () => async dispatch => {
    coinsData = await // api call
    const parsedCoinsData = coinsData.map((el) => { // Array#map already creates a new array. No need to spread it
        const {
            market_cap,
            volume_24h,
            percent_change_24h,
            percent_change_24h
        } = el.quote.USD
        return {
            id: el.id,
            name: el.name,
            marketCap: market_cap,
            volume24h: volume_24h,
            percentChange24h: percent_change_24h
        }
    })
    dispatch({
        type: SET_COINS_DATA,
        data: parsedCoinsData
    })
}

Create a selector to create the chart data

Creating a selector to parse/structure the data for your needs is an easy way to hide how the data is being created for your component to use and be able to hold all of the data in its unadulterated state in your redux store.

getChartData = state => {
    const { coinsData } = state.cmcListCallReducer;
    return [
        {
            id: 'CoinMarketCap API response',
            data: coinsData.map((el) => {
                const {
                    id,
                    name,
                    marketCap,
                    volume24h,
                    percentChange24h,
                } = el
                return {
                    id,
                    name,
                    marketCap: '$' + formatCrypto(marketCap, 3),
                    volume: '$' + formatCrypto(volume24h, 3),
                    priceChange: formatCrypto(Math.abs(percentChange24h)) + '%',
                    x: marketCap,
                    y: volume24h,
                    z: Math.abs(percentChange24h)
                }
            })
        }
    ]
}

And then your component would just look like this.

import React from 'react';
import { connect } from 'react-redux';
import ChartContainer from './chartContainer';

class Liquidity extends React.Component {
    constructor(props) {
        super(props);

        this.state = {
            minmaxValues: {},
            isLinear: false,
        };
    };

    swapChart = () => {
        this.setState((prevState) => ({ isLinear: !prevState.isLinear }));
    };

    render() {
        const { isLinear } = this.state;
        const { chartData } = this.props
        return (
            <>

Liquidity

                    <button
                        className={'btn btn-primary'}
                        type="button"
                        onClick={() => this.swapChart()}
                    >
                        Switch to {isLinear ? 'logarithmic' : 'linear'}
                    </button>

                <ChartContainer chartData={chartData} isLinear={isLinear} />
            </>
        )
    }
}

const mapStateToProps = (state) => {
    return {
        chartData: getChartData(state)
     };
};

export default connect(mapStateToProps)(Liquidity);
```


Is this a sensible way to implement a model? I've created the following model for an academic project, and I'm wondering if this is a sensible way to manage a model:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.IO;
using System.Runtime.Serialization.Formatters.Binary;
using System.Runtime.Serialization;
using System.Collections.ObjectModel;
using System.Windows.Forms;

namespace IceCreamShop
{
    [Serializable]
    public class Customer : ISerializable
    {
        private const string SAVE_FILE_NAME = @".\Customers.bin";

        #region static model accessors

        private static List<Customer> customers = new List<Customer>();

        public static ReadOnlyCollection<Customer> Customers
        {
            get
            {
                return customers.AsReadOnly();
            }
        }

        public static int Cardinality { get; private set; }

        public static void saveCustomers(List<Customer> customers)
        {
            IFormatter formatter = new BinaryFormatter();

            Stream stream = File.Open(SAVE_FILE_NAME, FileMode.Create);

            formatter.Serialize(stream, customers);

            formatter.Serialize(stream, Customer.Cardinality);

            stream.Close();
        }

        public static void loadAllCustomers()
        {
            IFormatter formatter = new BinaryFormatter();

            List<Customer> customers = new List<Customer>();

            try
            {
                Stream stream = File.Open(SAVE_FILE_NAME, FileMode.OpenOrCreate);

                if (stream.Length > 0)
                {
                    customers = (List<Customer>)formatter.Deserialize(stream);

                    Customer.Cardinality = (int)formatter.Deserialize(stream);
                }

                stream.Close();
            }
            // Shouldn't happen during normal operation, pending any permissions/file write issues
            catch (Exception ex)
            {
                throw ex;
            }

            if(customers.Count == 0)
            {
                // Dummy customer for patrons who don't want their data tracked
                customers.Add(new Customer("nobody", "nobody"));

                Customer.Cardinality = 1;
            }

            Customer.customers = customers;
        }
        #endregion

        #region individual model data

        private string firstName;
        public string FirstName { get { return firstName; } }

        private string lastName;
        public string LastName { get { return lastName; } }

        private int id;
        public int ID { get { return id; } }

        //Used externally to generate a new customer
        public Customer(string firstName, string lastName)
        {
            this.id = Customer.Cardinality++;
            this.firstName = firstName;
            this.lastName = lastName;
        }

        #endregion

        #region serialization

        Customer(SerializationInfo info, StreamingContext context)
        {
            this.id = info.GetInt32("i");
            this.firstName = info.GetString("j");
            this.lastName = info.GetString("k");
        }

        void ISerializable.GetObjectData(SerializationInfo info, StreamingContext context)
        {
            info.AddValue("i", this.id);
            info.AddValue("j", this.firstName);
            info.AddValue("k", this.lastName);
        }

        #endregion
    }
}

An individual Customer consists of an id, first name and a last name. I realize there are still some issues to be ironed out, but I'm primarily wondering if it makes sense to use statics in this way.

This is part of an MVP-based project, which focuses on putting application logic in the controllers and data-based operations in the model with dumb views.

This smells slightly odd to me. Why would the class of customers know about what customers are actually participating in the model at the moment? Is there a need to even have knowledge of all the customers in the model overall? (Even in a simulation, you'd have that knowledge as part of the harness, not the customers themselves.)

No, I'd instead model the collection of all potential customers as an instance of another class (the catchment? the clientele?) and would avoid having public static methods in any class at all. That collection might well just be a simple wrapper around a core collection class instance, but I'd be making it a proper member of the model simply because the collection of customers is a model participant. (Well, if it isn't then you have no reason to keep a list of all customers at all.) The other advantage of working this way is that you  can then easily adapt to a growing business that opens a second shop far enough away from the first that virtually nobody goes to both shops…


Reading strings into a vector, without using namespace std I have been told that using namespace std is a bad practice (especially in header files). But wouldn't this make the program less readable?

int main()
{
    std::string text;
    std::vector<std::string> svec;

    while(std::cin >> text)
        svec.push_back(text);

    for(std::vector<std::string>::size_type i = 0; i < svec.size(); i++)
        std::cout << svec[i]  + " " << svec[i].size() << "\n";

    return 0;
}

Does this not make the for loop difficult to read, even for a simple program as this? Is there a better way to do this?

I'll assume that you've read Why is using namespace std considered bad practice? so you understand the reasons that this can cause problems for your code.  I'm not going to address that any further in this answer.

You have alternatives, that reduce the risk compared to using namespace std; at file scope:

Selectively import names you're going to use, in the scope you're going to use them:

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

int main()
{
    using std::string;
    using std::vector;

    string text;
    vector<string> svec;

    while (std::cin >> text)
        svec.push_back(text);

    for (vector<string>::size_type i = 0; i < svec.size(); i++)
        std::cout << svec[i]  + " " << svec[i].size() << "\n";

    return 0;
}

This is an important technique when using free functions such as begin() and end() in generic code, where you want the std:: implementations to be available, but for argument-dependent lookup to prefer a more specific (local namespace) override if available.
Reduce the need to actually write typenames, with greater use of auto.  For example, your output loop can be

for (const auto& element: svec)
    std::cout << element << " " << element.size() << "\n";

Using auto more liberally can be very helpful when you decide that a method needs to be generic; you won't need to trawl through it updating the types to match the new signature.

As an aside, note that some namespaces (e.g. std::literals and its contained namespaces) are intended to be imported with a using namespace directive; you may still want to restrict their scope to just that part of your code that needs it, though - and never in a header file!


Rebuilding WAR files in webapps directory This script cleans up destination directory and copies file into it with new name. Can this intent be expressed more cleanly with shelly?

main = shellyFailDir $ verbosely $ do
  escaping False $ rm_rf $ webapps </> "*"
  cp (m2repo
       </> groupPath
       </> artifact
       </> version
       </> (intercalate "-" [artifact, version] `append` ".war")
     ) $ webapps </> (finalName `append` ".war")
  echo "done"

Alternate take:

main = shellyFailDir $ verbosely $ do
  ls webapps >>= mapM_ rm_rf
  cp (m2repo <> groupPath <> (mconcat . map fromText $ [artifact, version, artifactFileName]))
     $ webapps <> fromText (finalName <> extension)
  echo "done"

artifactFileName = intercalate "-" [artifact, version] <> extension

Having (types match only second version because they help pick correct Monoid):

m2repo = ".m2/repository" :: FilePath
version = "1.0-SNAPSHOT" :: Text
webapps = "tomcat/webapps" :: FilePath
groupPath = "com/example" :: FilePath
artifact = "app" :: Text
finalName = artifact
extension = ".war" :: Text

I think your first version looks pretty slick. You could use a let-binding to do a little reordering and naming.

import Data.Monoid

main = shellyFailDir $ verbosely $ do
  escaping False $ rm_rf $ webapps </> "*"
  let sourceName = artifact <> "-" <> version <> ".war"
      source = m2repo </> groupPath </> artifact </> version </> sourceName
      destination = webapps </> finalName <> ".war"
  cp source destination
  echo "done"


Powershell script to save user credentials in a xml file I have a power shell script that when run will prompt the user to:

Select a folder location
select a file name
enter credentials

This then exports those credentials in a xml file. Although this requires 3 prompts to the user before is complete. Looking to find if there is a better way to approach this. For example, having a single prompt for the user.

###########################################################
# Create a file containing encrypted credentials
###########################################################

clear

Function Get-Folder($initialDirectory)
{
    #User selected folder location
    [System.Reflection.Assembly]::LoadWithPartialName("System.windows.forms")|Out-Null

    $path = New-Object System.Windows.Forms.FolderBrowserDialog
    $path.Description = "Select a folder"
    $path.rootfolder = "MyComputer"

    if($path.ShowDialog() -eq "OK")
    {
        $location += $path.SelectedPath
    }

    #User selected file name
    [void][Reflection.Assembly]::LoadWithPartialName('Microsoft.VisualBasic')

    $title = 'File Name'
    $msg   = 'Please enter your file name'

    $text = [Microsoft.VisualBasic.Interaction]::InputBox($msg, $title)

    #Open prompt for user to enter credentials and export to xml file
    Get-Credential | Export-Clixml "$location\Credentials-$text.xml"

}

Get-Folder

Instead of letting the user (1) choose a folder and (2) choose a file name, you can just present the user with a Save File Dialog. It also warns the user if he tries to overwrite an existing file.
If the user cancels one of your dialogs, you should cancel the script rather than continue with invalid data.

$dialog = New-Object System.Windows.Forms.SaveFileDialog
$dialog.FileName = "Credentials.xml"
$dialog.Filter = "XML files (*.xml)|*.xml|All files|*.*"

if ($dialog.ShowDialog() -ne "OK")
{
    exit
}

...

Get-Credential | Export-Clixml $dialog.FileName


Obtaining all repeated characters' position in a string using Java Stream API Input is a string, and the method should return repeated characters in that string and the position of these characters.

I'm currently using Java 8 Stream. The problem is I have to do it by calling 2 times collectors. I am trying to find a way to combine all these operations in 1 single collector operations if possible.

Furthermore, the operation of mapping of characters and their position in a string is still performed in another method since I have not found a way to perform that.

Please give suggestion of improvement for the code:

public static void main(String args[]) {
    String str = "acbdaghfb";

    Map<Character,List<Integer>> map=charMaps(str);
    map.forEach((k,v)->System.out.println(k+"_"+v));
}

public static Map<Character,List<Integer>> charMaps(String str) {

    Map<Character, List<Integer>> maps = encodeString(str)
            .stream()
            .collect(Collectors.groupingBy(Pair::getKey, Collectors.mapping(Pair::getValue, Collectors.toList())));

    maps = maps.entrySet()
                .stream()
                .filter(e -> e.getValue().size() > 1)
                .collect(Collectors.toMap(e -> e.getKey(), e -> e.getValue()));

    return maps;
}

public static List<Pair<Character, Integer>> encodeString(String str) {
    List<Pair<Character, Integer>> pairs = new LinkedList<Pair<Character, Integer>>();
    char chars[] = str.toCharArray();

    for (int a = 0; a < chars.length; a++) {
        pairs.add(new Pair<Character, Integer>(chars[a], a));
    }
    return pairs;
}

You do not need encodeString and you do not even need a Pair class. You only use this method to build a list of each index with the character of the string at that index. Instead, you can directly use an IntStream that will go over the indexes of each character of the String.

Map<Character, List<Integer>> map =
    IntStream.range(0, str.length())
             .boxed()
             .collect(Collectors.groupingBy(str::charAt, HashMap::new, Collectors.toList()));

This creates an IntStream going from 0 to the length of the String (excluded). We turn this primitive Stream into a Stream<Integer> using boxed: we will need to box each index to store them in a list anyway, and having a Stream of objects enable us to use the built-in groupingBy(classifier, mapFactory, downstream) collector.

This collector groups each element of the Stream by the classifier given. In this case, the classifier is a function returning the character at the index considered. It is using the method-reference str::charAt (which would be equivalent to the lambda expression i -> str.charAt(i)). The downstream collector is used to collect all elements that are classified to the same key; in this case, it is simply toList() which will collect the indexes into a list. By default, groupingBy isn't specified to return a specific Map implementation (the current Oracle JDK returns a HashMap but that cannot be relied upon). Since we're going to post-process that map, giving HashMap::new as the map supplier makes sure that we end up with a HashMap.

The Stream API doesn't offer a way to do a GROUP BY HAVING, like we would like here (in SQL jargon), so 2 passes are needed. The second one can be simplified though: instead of creating a new Stream pipeline, we can use

map.values().removeIf(l -> l.size() <= 1);

to remove every entry from the map where the value is a list that has a size less than 1. This works because values() returns a view of the map, so changes to it reflect through the map. And then removeIf gives us the ability to remove the elements matching the given predicate.

So all in all, you can have:

public static Map<Character,List<Integer>> charMaps(String str) {
    Map<Character, List<Integer>> map =
        IntStream.range(0, str.length())
                 .boxed()
                 .collect(Collectors.groupingBy(str::charAt, HashMap::new, Collectors.toList()));
    map.values().removeIf(l -> l.size() <= 1);
    return map;
}


Bash function to produce variants of images at 2X and 3X resolutions I use ImageMagick to crop image, encapsulate the related code to functions.
Here is the code:
convertc2(){
  convert $1 -crop $2 "two.png"
}

converto3(){

  Delimeter="placeHolder"

  if [[ $2 == *"x"* ]]; then
    Delimeter="x"
  else
    Delimeter="X"
  fi

  First="${2%$Delimeter*}"
  Fourth="${2##*+}"
  Lhs="${2%%+*}"
  Rhs="${2#"$Lhs+"}"

  Second="${Lhs##*$Delimeter}"
  Third="${Rhs%+*}"

  One=$(echo "scale=0 ; $First * 1.5" | bc)
  One=${One%.*}
  Two=$(echo "$Second * 1.5" | bc)
  Two=${Two%.*}
  Three=$(echo "$Third * 1.5" | bc)
  Three=${Three%.*}
  Four=$(echo "$Fourth * 1.5" | bc)
  Four=${Four%.*}

  Final=$One"X"$Two"+"$Three"+"$Four

  echo $Final

  convert $1 -crop $Final "three.png"
}

Usage like this:

convertc2 /Users/dengjiangzhou/Desktop/Simulator\ Screen\ Shot\ -\ iPhone\ 8\ -\ 2019-02-20\ at\ 14.39.02.png   300X300+0+0

converto3 /Users/dengjiangzhou/Desktop/Simulator\ Screen\ Shot\ -\ iPhone\ 8\ -\ 2019-02-20\ at\ 14.39.02.png    300X300+0+0

I need the image size of @2X and @3X. So the function converto3  use convertc2 's size and transform to its size.
Delimeter="placeHolder"

if [[ $2 == *"x"* ]]; then
    Delimeter="x"
else
    Delimeter="X"
fi

I really care about how to improve the above code. The "x" , I may type "X" or "x".
how to use this "${2%$Delimeter*}" insensitive?
And the calculation code in  converto3() is very ugly.

Double-quote variables used as command line arguments

Instead of this:

convert $1 -crop $2 "two.png"

Write like this:

convert "$1" -crop "$2" "two.png"

This is to protect from word splitting and glob expansion.

Double quote... naturally

This is hard to read and confusing:

Final=$One"X"$Two"+"$Three"+"$Four

I suggest to write like this:

Final="${One}X$Two+$Three+$Four"

Use better variable names

It's really hard to make sense of a program that uses variable names like First, Fourth, Second, Lhs, that don't reveal their purpose.

Also, the convention is to use lowercase names, without capitalizing the first letters.

Chopping off characters case insensitively

Instead of detecting if the delimiter is x or X and storing it in a variable,
you could use the pattern [xX], for example:

width=${spec%%[xX]*}

For example if you have 200x300 or 200X300 in spec, width becomes 200.
(Do take note of the meaningful names.)

Floating point math in the shell

Unfortunately Bash doesn't do floating point math.
It would give you a syntax error if you tried to multiply something by 1.5.
On the other hand, if you want to multiply by 1.5, and you don't mind truncating decimal points (as is the case here), you could multiply by 3 and divide by 2.

That is, instead of this:

width=$(bc <<< "$width * 1.5")

You could write:

((width = width * 3 / 2))

Use here strings

Instead of echo ... | cmd, write cmd <<< "...".


All-purposes AJAX handler made with PHP and .htaccess that speaks JSON - Part 2 Earlier, I've posted a question about a file used to handle AJAX requests.

I've followed some of the advices given and made some changes on the code itself.

You can go on "All-purposes AJAX handler made with PHP and .htaccess that speaks JSON" to read more information about it's working.

This is simply a follow-up, and I will only present here the PHP code:

<?php
    define( 'ROOT', $_SERVER['DOCUMENT_ROOT'] ); //define the root beforehand

    //if the cookie isn't set or the session failed or you aren't an admin or your level is inferior to 2
    if( !isset( $_COOKIE[session_name()] ) || !@session_start() || !isset($_SESSION['admin']) || $_SESSION['admin']['level']<2 )
    {
        //you aren't allowed here
        header( 'HTTP/1.1 403 Forbidden' );
        echo '{"error":403}';
    }
    //if the file exists and the name has the proper format (to avoid stuff like ../../../../../killall$\.php)
    else if( @is_file($file = ROOT.'/ajax/'.preg_replace('@^/([a-z_]+)\.php@', '$1', $_SERVER['ORIG_PATH_INFO']).'.php') )
    {
        ob_start(); //avoid 'leaked' non-JSON garbage
        session_start();

        //database file, returns the result of mysqli_connection()
        if( $link = include ROOT.'/bd/bd.php' )
        {
            //returns a massive array with the translations
            $text = include ROOT.'/lang/'.$_SESSION['admin']['lang'].'.php';

            //the included files return an array or nothing (requiring the (array) there)
            $out = (array)include $file;
        }
        else
        {
            //indicates which error was it (in this case, a communication error)
            $out = array('error' => true, 'type' => 'link');
        }

        if( isset($_REQUEST['debug']) && $_SESSION['admin']['level']>=5)
        {
            //debug informations sent when the parameter ?debug is added
            //only allowed for administrators with level higher than 5
            $out['__debug'] = array(
                'mysql' => array(
                    'n' => mysqli_errno( $link ),
                    'desc' => mysqli_error( $link )
                ),
                '$_POST' => print_r( $_POST, true ),
                '$_SERVER' => print_r( $_SERVER, true ),
                '$_SESSION' => print_r( $_SESSION, true ),
                'last_error' => error_get_last(),

                'output' => ob_get_contents()
            );
        }

        ob_end_clean(); //clears the errors

        //outputs the JSON code to the browser
        echo json_encode( $out );
    }
    else
    {
        //if none of the conditions was met, send the error 404
        header( 'HTTP/1.1 404 Not Found' );
        echo '{"error":404}';
    }

Since the code looks a little more difference, I've made a new question instead of an edit in the older one.

Considering the new code, What else can I change?

What else can I improve?

There are two things I would refactor.

The first if statement (now) checks if the session has been initialized. I think this can be done in a more readable way by using the native PHP function session_status(). You can also write the admin check into a variable. This small refactor will make the if-statement more readable.

$session_name = session_name();

/*
 * Checks if a session exists and that the session was initialized
 * properly.
 */
$session_active = (session_status() == PHP_SESSION_ACTIVE);

/*
 * Checks if an administrator is logged in and that his
 * access level is greater than 2.
 */
$admin_authorized = (isset($_SESSION['admin'] && $_SESSION['admin']['level'] > 2)

if(!isset($_COOKIE[$session_name]) || !$session_active || !$admin_authorized) {

    header('HTTP/1.1 403 Forbidden');
    echo '{"error":403}';

    /*
     * Remember to exit to ensure execution is stopped.
     */
    exit;

}

It doesn't hurt to write an extra variable if it helps readability. A too long or complex if-statement-body can hurt readability, because it can be hard to keep track of what conditions has been checked or what the condition actually does. By extracting the logic into variables you can also better document the specific piece of logic. Consider writing all documentation before the whole statement or before each variable definition.

Then there are the part where you check if the requested file exists. I think this should be refactored as well. You should also remove the silence operator. According to documentation is_file also checks if the file actually exists.

I would refactor this to:

/*
 * This may seem redundant, but experience has taught me that you should only
 * perform one manipulation on a variable per line. Its easier to read what is
 * actually happening and to modify it later.
 */
$file = preg_replace('@^/([a-z_]+)\.php@', '$1', $_SERVER['ORIG_PATH_INFO']);
$file = ROOT . '/ajax/' . $file . '.php';

if(!is_file($file)) {

    /*
     * The requested file does not exist.
     */
    header('HTTP/1.1 404 Not Found');
    echo '{"error":404}';
    exit;

}

// Continue execution as the file exists.

In the last post you also mentioned security. As said in that post I'm not security expert, but one thing that I have made a habit is to extract values from superglobals using the filter_input() function. It helps you filter out the input to better match your exact requirements. Imagine an input that was expected to provide a string, but was in fact an integer, due to a malicious user. This would break your code. As an example when extracting the ORIG_PATH_INFO you would write.

/*
 * There might be more/better filters. To add more filters delimit them
 * using the pipe-character: |
 */
$original_path_info = filter_input(INPUT_SERVER, 'ORIG_PATH_INFO', FILTER_SANITIZE_STRING);

This helps make sure the variable is actually a string. Its not bulletproof (no where near actually), but it could remove malicious code from a potential attacker.

Hope this helps, happy coding!


Implementing a date validator using enum First and second post on the same subject. This is such a great way to learn.

Once again I'm open to suggestions. I have implemented almost all of the suggestions from before, which have resulted in the code looking like this:

import java.util.Set;
import java.util.HashSet;
import java.util.Arrays;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

enum DateValidator {
    DDMMYYYY(0, 2, 4),
    MMDDYYYY(2, 0, 4),
    YYYYMMDD(6, 4, 0);

    private final int dayIndex;
    private final int monthIndex;
    private final int yearIndex;

    private DateValidator(int dayIndex, int monthIndex, int yearIndex) {
        this.dayIndex = dayIndex;
        this.monthIndex = monthIndex;
        this.yearIndex = yearIndex;
    }

    private int getDay(String date){ return Integer.parseInt(date.substring(dayIndex, dayIndex + 2)); }
    private int getMonth(String date){ return Integer.parseInt(date.substring(monthIndex, monthIndex + 2)); }
    private int getYear(String date){ return Integer.parseInt(date.substring(yearIndex, yearIndex + 4)); }

    private static boolean isLeapYear(int y) {
        return (y % 4 == 0) && ((y % 100 != 0) || (y % 400 == 0));  }

    private boolean isDateLegal(String date) {
        final int day = getDay(date);
        final int month = getMonth(date);
        final int year = getYear(date);
        boolean isValid = false;

        final Set<Integer> values = new HashSet<Integer>(Arrays.asList(1, 3, 5, 7, 8, 10, 12));
        final int daysInFebruary = (month == 2 && isLeapYear(year)) ? 29 : 28;

        if (day > 0) {
            if (values.contains(month)) { isValid = day <= 31; }
            else if (month > 0 && month < 13 && month != 2) { isValid = day <= 30; }
            else if (month == 2) isValid = day <= daysInFebruary;
        }

        return isValid;
    }

    private static boolean isDateFormatValid(String date) {

        final String DATE_PATTERN = "(^" + "[0-9]{2}\\.[0-9]{2}\\.[1-2][0-9]{3}|" +
                                     "[0-9]{2}\\-[0-9]{2}\\-[1-2][0-9]{3}|" +
                                     "[0-9]{2}\\/[0-9]{2}\\/[1-2][0-9]{3}|" +
                                     "[0-9]{2}\\s[0-9]{2}\\s[1-2][0-9]{3}|" +
                                     "[0-9]{2}[0-9]{2}[1-2][0-9]{3}|" +
                                     "[1-2][0-9]{3}\\.[0-9]{2}\\.[0-9]{2}|" +
                                     "[1-2][0-9]{3}\\-[0-9]{2}\\-[0-9]{2}|" +
                                     "[1-2][0-9]{3}\\/[0-9]{2}\\/[0-9]{2}" +
                                     "[1-2][0-9]{3}\\s[0-9]{2}\\s[0-9]{2}" +
                                     "[1-2][0-9]{3}[0-9]{2}[0-9]{2}" +")$";

        Pattern pattern = Pattern.compile(DATE_PATTERN);
        Matcher matcher = pattern.matcher(date);

        return matcher.matches();
    }

    public boolean validate(String date) {
        return this.isDateLegal(date.replaceAll("[\\.\\-\\/\\s]", "")) && isDateFormatValid(date);
    }
}

class Main {
    public static void main(String[] args) {
        System.out.print(DateValidator.DDMMYYYY.validate("16.10.1990") ? "Date is valid" : "Date is NOT valid");
    }
}

Handling exceptions

Integer.parseInt throws a NumberFormatException when there is a parsing error, you may want to consider catching that and then just output the validation status as "Invalid".

Field names

final Set<Integer> values = new HashSet<Integer>(Arrays.asList(1, 3, 5, 7, 8, 10, 12));

These aren't just any values, these represent months with 31 days in them. :) Probably call this as monthsWith31Days? I also understand you want to make it clear that these months are obviously unique in the collection (in layman terms), but considering this is only used internally within the method, I don't think there's much harm leaving it as just a List.

Day validation logic

Personally, I will start from the most constrained case and work my way down. The slight advantage is that your 'wider' cases will not need to check that is it not any of the constrained cases (e.g. look at how you do a month != 2 for the first else if branch). In conclusion, this means you can validate for the month of February first, then the 31-day validation, and finally for all remaining months. You can also return immediately from each of the branch, instead of one return statement at the end.

Putting it altogether for DateValidator.isDateLegal()

    private boolean isDateLegal(final String date) {
        final int day = getDay(date);
        final int month = getMonth(date);
        final int year = getYear(date);
        final List<Integer> monthsWith31Days = Arrays.asList(1, 3, 5, 7, 8, 10, 12);
        if (day > 0) {
            if (month == 2) {
                // BTW no need to check if month = 2 here
                return day <= isLeapYear(year) ? 29 : 28;
            } else if (monthsWith31Days.contains(month)) {
                return day <= 31;
            } else if (month > 0 && month < 13) {
                return day <= 30;
            }
        }
        return false;
    }

Regex

Now onto the big one... your regular expression is ideal for simplification by applying the following rules:

[1-2] can be represented as [12].
Metacharacters (e.g. .) are interpreted literally in character classes, so there is no need to escape them.
To match - inside a character class, put it at the last so that it doesn't get interpreted as a range syntax.
Just as you have used a character class to do your separator replacement for date, you should do likewise for your DATE_PATTERN too.
In addition to the previous point, you can then apply capture groups to check that the same separator is used.

Putting it altogether for regex

private static boolean isDateFormatValid(final String date) {
    final String DATE_PATTERN = "^([0-9]{2}([./\\s-]?)[0-9]{2}([./\\s-]?)[12][0-9]{3}|"
            + "[12][0-9]{3}([./\\s-]?)[0-9]{2}([./\\s-]?)[0-9]{2})$";
    final Matcher matcher = Pattern.compile(DATE_PATTERN).matcher(date);
    return matcher.matches() && (Objects.equals(matcher.group(2), matcher.group(3)))
            && (Objects.equals(matcher.group(4), matcher.group(5)));
}

(BTW, Objects.equals() is a Java 7 method and it is a simple null-safe implementation of calling the first argument's equals() with the second argument. This is important as matcher.group() can return null for non-matches.)

Pro-tip: Actually, you may want to go a step further by grouping the day and year values, and make sure that the length of both parts (i.e. DD,YYYY or YYYY,DD) are different:

private static boolean isDateFormatOk(final String date) {
    final String DATE_PATTERN = "^([0-9]{2}|[12][0-9]{3})([./\\s-]?)[0-9]{2}([./\\s-]?)"
            + "([0-9]{2}|[12][0-9]{3})$";
    final Matcher matcher = Pattern.compile(DATE_PATTERN).matcher(date);
    return matcher.matches() && (matcher.group(1).length() != matcher.group(4).length())
            && (Objects.equals(matcher.group(2), matcher.group(3)));
}

(Pro-pro-tip: The Pattern can be a private static final field...)

For the validate() method, you can check for a valid format first before validating the entire string:

public boolean validate(final String date) {
    return isDateFormatValid(date) && isDateLegal(date.replaceAll("[./\\s-]", ""));
}


Cumulative transition matrix Markov Chain simulation I have a cumulative transition matrix with probabilities for all the possible states from 1 to 5. Now the algorithm for simulating future states is following: the initial state is selected randomly, and a random value between 0 and 1 is then produced by uniform random number generator.To determine the next state in the Markov process the value of the random number is is compared with the elements of of the *i-*th row of the cumulative transition matrix determined from the preceding state. If the value of the random number is greater than the cumulative probability of the preceding state but less then or equal to the cumulative probability of the succeeding state, the succeeding state is chosen to represent the next state.

cum_trans <- matrix(c(0.1686747,0.4337349,0.6265060,0.7289157,1,
              0.2765957,0.5053191,0.6648936,0.7659574,1,
              0.2518519,0.4740741,0.6518519,0.7407407,1,
              0.1911765,0.4705882,0.6617647,0.7941176,1,
              0.2096774,0.4892473,0.6827957,0.7419355,1
              ), nrow = 5, ncol = 5, byrow = TRUE)

       [,1]      [,2]      [,3]      [,4]      [,5]
 [1,] 0.1686747 0.4337349 0.6265060 0.7289157    1
 [2,] 0.2765957 0.5053191 0.6648936 0.7659574    1
 [3,] 0.2518519 0.4740741 0.6518519 0.7407407    1
 [4,] 0.1911765 0.4705882 0.6617647 0.7941176    1
 [5,] 0.2096774 0.4892473 0.6827957 0.7419355    1

state<-matrix(NA, nrow=1,ncol=25)
i<-sample(1:5,1)
state[1]<-i
for (k in 2:25){
 rr<-runif(1)
 state[k]<-findInterval(rr,cum_trans[i,])+1
 i<-state[k]
 }
 state

So I'm not sure if I implemented the algorithm correctly. Can someone suggest modifications or improvements.

I see nothing wrong with your implementation, in the sense it does exactly what you described in plain English, and somewhat efficiently. Here are however a few pieces of advice, mostly about improving your coding standards.

Add some spaces to your code to make it more readable. Have a look at the body of basic functions (e.g. lm) to see what clean code should look like. In particular, spaces after commas and spaces on both sides of <-, =, and all binary operators.
Replace your hardcoded values (5 and 25) with variables. 5 can be derived from the number of rows in cum_trans and 25 is an input to your process. By assigning it to a variable (or a function input), and reusing that variable throughout your code, your code becomes easier to read and maintain, and more robust to changes. Imagine for example what would be required of you if you wanted to change the size of the transition matrix or wanted to have 50 iterations instead of 25.
Using a 1-row matrix is overkill and error-prone: use a vector.
Choose well how you name your variables and comment your code. Again, to make it easier to read and understand.
Learn the difference between numerics and integers. Here your states are obviously integers, yet your code stores numerics. Granted, it is not critical here, but using integers use less memory and can make your code faster. Integers are also not subject to floating point errors so they can make your code more robust in some instances.
Marginal speed improvement. The 25 random calls to runif(1) being independent, you can make a single call to runif(25) and store the results.
Think of your code in terms of inputs of outputs, then write a function.

Updated code taking some of these into account:

random.walk <- function(cum_trans, length.out = 25L) {
   num.states <- nrow(cum_trans)
   states  <- vector(mode = "integer", length = length.out)
   # pick the initial state randomly
   states[1L] <- sample(num.states, 1L)
   # jump randomly using the cum_trans matrix
   num.jumps <- length.out - 1L
   randoms <- runif(num.jumps)
   for (i in seq(num.jumps)) {
      current.state  <- states[i]
      current.prob   <- cum_trans[current.state, ]
      states[1L + i] <- findInterval(randoms[i], current.prob) + 1L
   }
   return(states)
}

Next, think of the assumptions your code is making: cum_trans must be a square matrix with each row being an increasing vector of positive values ending with 1. length.out must be an integer vector of length 1. All of these assumptions can be checked at the top of the function's body; I am only implementing a few of them here:

stopifnot(nrow(cum_trans) == ncol(cum_trans),
          all(cum_trans >= 0),
          is.integer(length.out),
          length(length.out) == 1L)

This way, there is no way you or your end-user can mess up with the inputs.

I hope you find this useful and it encourages you to write better code.


Abstract Factory Design Java I am new to Design Pattern implementation and would like to strongly solidify my knowledge to become as good of a programmer as I can. Is this a proper implementation of the Abstract Factory design? The idea here is that I can interchangeably utilize either a ConsoleLogger or a FileLogger for three different "levels" of logging (Execution, Output, and Error). I want the client to be able to log Execution and Output to console, but Error logs to a file if necessary.

Logger Interface:

public interface ILogger {
    public void log(String content);
}

Base Logging Factory:

import java.util.HashMap;

public abstract class LoggingFactory {
    static {
        factoryMap = new HashMap<String, LoggingFactory>();
    }

    private static HashMap<String, LoggingFactory> factoryMap;

    public abstract ILogger getLogger();

    public static void register(String type, LoggingFactory factory) {
        factoryMap.put(type, factory);
    }

    public static LoggingFactory getFactory(String type) {
        return factoryMap.get(type);
    }
}

ConsoleOutputLoggingFactory:

public class ConsoleOutputLoggingFactory extends LoggingFactory {
    static {
        LoggingFactory.register("ConsoleOutputLoggingFactory", new ConsoleOutputLoggingFactory());
    }

    @Override
    public ILogger getLogger() {
        return new ConsoleOutputLogger();
    }
}

ConsoleOutputLogger:

public class ConsoleOutputLoggerimplements ILogger {
    @Override
    public void log(String content) {
        System.out.println("ConsoleOutputLogger " + content);
    }
}

FileOutputLoggingFactory:

public class FileOutputLoggingFactory extends LoggingFactory {
    static {
        LoggingFactory.register("FileOutputLoggingFactory", new FileOutputLoggingFactory());
    }

    @Override
    public ILogger getLogger() {
        return new FileOutputLogger();
    }
}

FileOutputLogger:

public class FileOutputLogger implements ILogger {
    @Override
    public void log(String content) {
        //write output to file
    }
}

ConsoleErrorLoggingFactory:

public class ConsoleErrorLoggingFactory extends LoggingFactory {
    static {
        LoggingFactory.register("ConsoleErrorLoggingFactory", new ConsoleErrorLoggingFactory());
    }

    @Override
    public ILogger getLogger() {
        return new ConsoleErrorLogger();
    }
}

ConsoleErrorLogger:

public class ConsoleErrorLogger implements ILogger {
    @Override
    public void log(String content) {
        System.out.println("ConsoleErrorLogger" + content);
    }
}

FileErrorLoggingFactory:

public class FileErrorLoggingFactory extends LoggingFactory {
    static {
        LoggingFactory.register("FileErrorLoggingFactory", new FileErrorLoggingFactory());
    }

    @Override
    public ILogger getLogger() {
        return new FileErrorLogger();
    }
}

FileErrorLogger:

public class FileErrorLogger implements ILogger {
    @Override
    public void log(String content) {
        //write error to file
    }
}

Factory Initializer:

public class LoggingInitializer {
    public LoggingInitializer() {
        LoggingFactory.register("ConsoleOutputLogger", new ConsoleOutputLoggingFactory());
        LoggingFactory.register("FileOutputLogger", new FileOutputLoggingFactory());
        LoggingFactory.register("ConsoleErrorLogger", new ConsoleErrorLoggingFactory());
        LoggingFactory.register("FileErrorLogger", new FileErrorLoggingFactory());
    }
}

Main:

public class AbstractFactoryExample {
    public static void main(String[] args) {
        new LoggingInitializer();

        ILogger outputLogger = LoggingFactory.getFactory("OutputLogger").getLogger();
        if (outputLogger != null)
            outputLogger.log("Hello World");
    }
}

static {
    factoryMap = new HashMap<String, LoggingFactory>();
}

private static HashMap<String, LoggingFactory> factoryMap;

can be simplified to the following without any changes in functionality:

private static final Map<String, LoggingFactory> factoryMap = new HashMap<>();

public class LoggingInitializer {
    public LoggingInitializer() {
        LoggingFactory.register("ConsoleOutputLogger", new ConsoleOutputLoggingFactory());
        LoggingFactory.register("FileOutputLogger", new FileOutputLoggingFactory());
        LoggingFactory.register("ConsoleErrorLogger", new ConsoleErrorLoggingFactory());
        LoggingFactory.register("FileErrorLogger", new FileErrorLoggingFactory());
    }
}

The initialization of a class only happens in it's initializer blocks. Nowhere else.
What you're doing here is wiring up dependencies. Use  a DI framework for that ...

If you were going to set up the factories this way in the first place, you could've just not have them register themselves. It's weird that they register themselves anyways. That results in the following changes:

LoggerFactory implementations don't need to be derived from LoggingFactory to have access to the factoryMap, only to conform to the getLogger interface.
Actually the getLogger interface is what you'd have in a functional interface in modern Java.

This results in simplifications to your API. Instead of explicitly constructing factories and using getFactory, you're off much better hiding the implementation detail of Factories and instead using a Facade, like so:

public static final class LoggingFactory {
    private static final Map<String, Supplier<ILogger>> factoryMap = new HashMap<>();
    static {
        ILogger consoleOutLoggerSingleton = new ConsoleOutputLogger();
        factoryMap.put("ConsoleOutputLogger", () -> consoleOutputLoggerSingleton);

        ILogger fileOutputLoggerSingleton = new FileOutputLogger();
        factoryMap.put("FileOutputLogger", () -> fileOutputLoggerSingleton);

        ILogger consoleErrorLoggerSingleton = new ConsoleErrorLogger();
        factoryMap.put("ConsoleErrorLogger", () -> consoleErrorLoggerSingleton);

        ILogger fileErrorLoggerSingleton = new FileErrorLogger();
        factoryMap.put("FileErrorLogger", () -> fileErrorLoggerSingleton);
    }

    public static ILogger getLoggerFor(String name) {
        return factoryMap.getOrDefault(name, () -> new NoOpLogger()).get();
    }
}

This cuts away the "man in the middle" of your factory, not to mention the initialization outside the class. It also is more in line with usual Java logging APIs, where you directly obtain the logger.

In closing you should be aware that reinventing logging is usually a "Bad Idea™". If this hadn't been prefixed, I'd have washed your head for it :)

Finally: Just because there is a factory, the client of you API doesn't need to know- Generally the less indirection your API has, the easier it will be to use. Strive to minimize ways through your API.


!empty() check before a foreach statement Is this the best way to prevent that foreach loop from happening if $terms doesn't exist?

$terms = get_the_terms( $postid, 'fecha' );
if(!empty($terms)) {
    foreach($terms as $term) {
        $myterm = $term->slug;
        if(in_array($myterm, $queried_terms)) {continue;}
        $queried_terms[] = $myterm;
    }
}

The loop won't be executed if $terms is false/null/an empty array.

As such: you don't need the empty check. It doesn't hurt, but it's simply not required.

If $terms is sometimes not an array

You can just cast it to an array before use:

$terms = get_the_terms( $postid, 'fecha' );
foreach((array) $terms as $term) {
    $myterm = $term->slug;
    if(in_array($myterm, $queried_terms)) {
        continue;
    }
    $queried_terms[] = $myterm;
}

The code would work with or without casting to an array - the only difference is that if $terms is not an array, a notice would be thrown without it.

if this is part of another function

you can simply return-early to keep your code concise:

function foo() {
    $terms = get_the_terms( $postid, 'fecha' );
    if (!$terms) {
        return array();
    }

    $queried_terms = array();
    foreach($terms as $term) {
        $myterm = $term->slug;
        if(in_array($myterm, $queried_terms)) {
            continue;
        }
        $queried_terms[] = $myterm;
    }
    return $queried_terms;
}

But, any way you slice it - the empty check isn't strictly required, and deeper code nesting is something to be aware of and avoid.


Getting a path from list of arcs in C++ We are given a vector of std::pair<int, int>s, start int and stop int, and I need to extract the path out of it.

For instance:
given start = 2, stop = 8 and list of std::pairs as [(0, 5) (6,8), (5,6), (2,0)]. I am ordering the vector into:
[(2,0), (0,5), (5,6), (6,8)]. Now I need to isolate the nodes to get the path as [2,0,5,6,8]. (i.e. (2,0) and (0,5) become [2,0,5] and so on).

I have the following code. It works but I feel that it is needlessly long. Any idea to make it more compact and "safe"?

std::vector<int> get_pretty_path(const std::vector<std::pair<int,int>>& path_arcs, int s, int t){

    std::vector<std::pair<int,int>> ordered_arcs;

    assert(path_arcs.size() > 0);

    int _s = s;
    int _t = t;

    bool l_flag= true;

    std::vector<int> path;

    while (l_flag) {
        for(auto &arc: path_arcs){
            if (arc.first == _s){
                ordered_arcs.push_back(arc);
                _s = arc.second;
                if(arc.second == _t){
                    l_flag = false;
                    break;
                }
            }
        }
    }

    if(ordered_arcs.size() == 1){
        path.push_back(ordered_arcs.begin()->first);
        path.push_back(ordered_arcs.begin()->second);
    }
    else if (ordered_arcs.size() == 2){
        for (auto it = ordered_arcs.begin(); it != ordered_arcs.end()-1; ++it){
            if (it->second == (it+1)->first){
                path.push_back(it->first);
                path.push_back(it->second);
            }

            if((it+1) == ordered_arcs.end()-1){
                path.push_back((it+1)->second);
            }
        }
    }
    else{

        for (auto it = ordered_arcs.begin(); it != ordered_arcs.end()-1; ++it){
            if (it->second == (it+1)->first){
                path.push_back(it->first);
            }
            if((it+1) == ordered_arcs.end()-1){
                path.push_back((it+1)->first);
                path.push_back((it+1)->second);
            }
        }
    }

    return path;
}

This function is called in the order of 10,000 times. I was wondering if it can be as fast as possible. The max length of path_arcs is around 7-8 but is is usually 4-5 or less.

Note that the paths are not weighted. There is only one path. I just need it to be ordered from start to end. Some more examples:

input = [(2,0)] // given by the user
start = 2       // given by the user
stop = 0        // given by the user
output = [2,0]  // This should be the output

input = [(1,4), (2,1)] // given by the user
start = 2              // given by the user
stop = 4               // given by the user
output = [2,1,4]

input = [(4,5),(7,4),(2,3),(5,2)] // given by the user
start = 7                         // given by the user
stop =  3                         // given by the user
output = [7,4,5,2,3]

The key is to create a mapping for the edges:

vector<int>
get_pretty_path(const vector<pair<int,int>>& edges, int s, int t) {
    map<int,int> edgeMap = {begin(edges), end(edges)};
    vector<int> path;
    while (s != t) {
        path.push_back(s);
        s = edgeMap[s];
    }
    path.push_back(s);
    return path;
}
int main() {
    const vector<pair<int,int>> edges{{4, 5}, {7, 4}, {2, 3}, {5, 2}};
    const vector<int> path = get_pretty_path(edges, 7, 3);
    for (auto i : path) {
        cout << i << ' ';
    }
    cout << endl;
    return EXIT_SUCCESS;
}

The code assumes using namespace std; which is sometimes frowned
upon but which I think is fine for small examples. The code will bug
out if you can't walk from s to t or if more than one edge starts
at a given node index. If such situations can happen, you have to decide
how to best handle them.


Object cache storage for Rails I want to have fast cache in which I want to keep all my nomenclature data.
I don't want to go with Memcached because I have to do serialize/de-serialize on each object which is slow.

So I choose to be less effective in memory and keep the cache in each server instance.
I am sure I am doing it in the wrong way because the NoCache module - which skips my cache and hits the Rails cache is faster than mine.

Here is how it is initialized

$cache = Cache.new

Here is the example usage

property_types = $cache['PropertyType']

and here is the source

module DirectCache
  def init_cache
  end

  def reload_model(model_class)
    key = get_key(model_class)
    klass = key.constantize
    object = klass.scoped
    puts "Loading cache for #{key}..."
    if klass.respond_to?(:translates) and klass.translates?
      puts "  Adding translation in the model for #{klass}"
      object = object.includes(:translations)
      self.storage[key] = object.send("all")
    end
  end

  def [] class_name_or_object
    puts "Hiting cache for #{class_name_or_object}"
    key = get_key(class_name_or_object)
    reload_model(class_name_or_object) if self.storage[key].blank? or self.storage[key].empty?
    raise "#{key} is missing in the cache #{@cache.keys.join ', '}" unless key? key
    self.storage[key]
  end

  def init_cache
  end

end

module NoCache
  def reload_model(model_class)
    key = get_key(model_class)
    self.storage[key] = key.constantize
  end

  def [] class_name_or_object
    key = get_key(class_name_or_object)
    raise "#{key} is missing in the cache #{@cache.keys.join ', '}" unless key? key
    klass = self.storage[key]
    object = klass.scoped
    if klass.respond_to?(:translates) and klass.translates?
      puts "Adding translation in the model for #{klass}"
      object = object.includes(:translations)
    end
    object.send("all")
  end

  def init_cache
  end

end

class Cache
  include DirectCache
  #  include NoCache
  #  include OpenStructCache

  @@models = [
    PropertyFunction,
    PropertyCategoryLocation,
    ConstructionType,
 ....20 more ....
    ExposureType,
  ]

  cattr_reader :models

  def initialize
    @cache = {}
    begin
      init_cache
    rescue
      puts "missing tables"
    end
  end

  def storage
    @cache
  end

  # returns the the key - aways string
  def get_key class_name_or_record
    case class_name_or_record
    when Class
      key = class_name_or_record.to_s
    when String
      key = class_name_or_record
    else
      key = class_name_or_record.class.to_s
    end
    key
  end

  def key? class_name_or_object
    key = get_key(class_name_or_object)
    self.storage.keys.include? key
  end

end

The ActiveSupport::Cache is not mandatory in Memcached, there are a cache in memory if you use the :memory_store ( http://api.rubyonrails.org/classes/ActiveSupport/Cache/MemoryStore.html )

config.cache_store = :memory_store

Maybe can be a good start to implement your own Cache in memory


Memory mapped classpath HttpServlet I have a single page application where the resources sit on the classpath. Any request for a resource that doesn't exist should redirect to the index.html page. Resources should be mapped to memory on first request and then all subsequent requests should load from memory.

Have I made any mistakes with how I should be using direct ByteBuffers, it feels overly laborious, but that could just be the API?

class MemoryMappedClasspathHttpServlet extends HttpServlet {

    private final ConcurrentMap<String, ByteBuffer> mappedResources = new ConcurrentHashMap<>();

    @Override
    protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
        String requestPath = req.getPathInfo();

        if (requestPath.equals("/")) {
            requestPath = "/web/index.html";
        } else {
            requestPath = "/web" + requestPath;

            if (MemoryMappedClasspathHttpServlet.class.getResource(requestPath) == null) {
                requestPath = "/web/index.html";
            }
        }

        ByteBuffer buffer = mappedResources.computeIfAbsent(requestPath, MemoryMappedClasspathHttpServlet::loadResourceFromClasspath);

        Channels.newChannel(resp.getOutputStream())
            .write(buffer.duplicate());
    }

    private static ByteBuffer loadResourceFromClasspath(String classpathItem) {
        try (ByteArrayOutputStream os = new ByteArrayOutputStream();
             InputStream is = MemoryMappedClasspathHttpServlet.class.getResourceAsStream(classpathItem)) {

            byte[] buffer = new byte[8092];

            for (int len; (len = is.read(buffer)) != -1; ) {
                os.write(buffer, 0, len);
            }

            os.flush();

            byte[] bytes = os.toByteArray();

            ByteBuffer directBuffer = ByteBuffer.allocateDirect(bytes.length);
            directBuffer.put(bytes);
            directBuffer.flip();

            return directBuffer;
        } catch (IOException e) {
            throw new UncheckedIOException(e);
        }
    }
}

Possible improvement:

return directBuffer;

with:

return directBuffer.asReadOnlyBuffer();

Wanted to find a one-liner to help https://code.google.com/p/guava-libraries/wiki/IOExplained but ByteBuffer, streams nontrivial. Notice defining mutable ByteBuffer references in the method, placing each into an instance collection, one never flushed or refreshed...Servlet web container would need a restart in order to reflect changes to the files or other resources.


Longest contiguous Increasing Subset in java I have tried to come up with a solution to the longest increasing subsequence problem.Can this solution be optimized further?

Example 1:

Input: {5,4,3,7,8,9,11}
Output: 3, 7, 8, 9, 11

Example 2:

Input: {1,2,1,2,1}
Output: 1, 2 (the latest one)

Implementation:

 public class LongestIncreasingSubsequence {

        public static ArrayList<Integer>[] getLongestIncreasingSubsequence(int[] array, ArrayList<Integer>[] list,
                int index) {

            list[index] = new ArrayList<Integer>();

            if (index == 0) {
                list[index].add(array[index]);

            }

            else {

                if (array[index] > array[index - 1]) {
                    list[index].addAll(list[index - 1]);

                }

                    list[index].add(array[index]);

               }

            if (index == array.length - 1)
                return list;

            return getLongestIncreasingSubsequence(array, list, index + 1);

        }

        public static List<Integer> findLIS(int[] array) {

            ArrayList<Integer>[] arrayList = getLongestIncreasingSubsequence(array, new ArrayList[array.length], 0);

            int maxLength = arrayList[array.length - 1].size();
            int maxIndex = array.length - 1;
            for (int i = array.length - 2; i >= 0; i--) {
                if (arrayList[i].size() > maxLength) {
                    maxLength = arrayList[i].size();
                    maxIndex = i;
                }

            }

            return arrayList[maxIndex];

        }

        public static void main(String args[]) {

            int[] array = new int[] { 7, 1, 3, 8, 11 };

            List<Integer> LIS = findLIS(array);

            for (Integer i : LIS)
                System.out.print(i + ",");

        }
    }

For an input array of N values, the current implementation creates N instances of ArrayList. But you only need 2:

One to store the longest sequence seen so far
One to store the currently tracking sequence

For example the input 5, 6, 7, 1, 2, 3, 4, walking through the elements from the start:

5: the first element -> add it to the current sequence
6: greater than the previous -> add it to the current sequence
7: greater than the previous -> add it to the current sequence
1: the current sequence is broken -> the current sequence is longer than the longest sequence -> update the longest sequence = [5, 6, 7]
2: greater than the previous -> add it to the current sequence
3: greater than the previous -> add it to the current sequence
4: greater than the previous -> add it to the current sequence
Reached the end -> the current sequence is longer than the longest sequence -> the longest sequence is [1, 2, 3, 4]

The implementation can be simplified to:

public static List<Integer> findLIS(int[] array) {
    List<Integer> longest = new ArrayList<>();
    List<Integer> current = new ArrayList<>();

    int previous = Integer.MAX_VALUE;
    for (int value : array) {
        if (value <= previous) {
            if (longest.size() < current.size()) {
                longest = current;
            }
            current = new ArrayList<>();
        }
        current.add(value);
        previous = value;
    }

    return longest.size() < current.size() ? current : longest;
}

Some other tips on the original code:

Avoid arrays of generic types such as ArrayList<Integer>[]
Prefer interface types in declarations and method return types, such as List instead of ArrayList
When initializing generic types, Java 7 can guess the correct type parameter, so instead of new ArrayList<Integer>() you can write new ArrayList<>() (also known as the diamond operator <>)
The indentation and formatting was inconsistent, use an IDE to format the code nicely
Instead of printing elements of LIS one by one, System.out.println(LIS) would easily produce an output that's just as readable
A more compact writing style to initialize an array: int[] array = { 7, 1, 3, 8, 11 };


Pixelating black and white images I am doing a function that pixelates black and white images. The function must be called pixelate(img, (pixel_width, pixel_height), mode).

The parameter (pixel_width, pixel_height) is a tuple which indicates the size of the resulting pixel values. the function must return a result in any event, even if they are higher than the image dimensions. If the image size is not a multiple of the pixel size,some of the resulting pixels will have a smaller size than (pixel_width, pixel_height). This smaller pixels will be located in the right and bottom edge of the image as shown in the examples below. In other words, the pixels always start from the top left corner of the image.

The parameter mode, is an string, 'min', 'max' or 'mean'. We will write 'mean', if we want to calculate the mean value of the area where a pixel of the new image is going to be. We will write 'max' if we want the brightest value or 'min' for the darkest. If the mean value has decimals, we will only use the integer part.

Now I have the program done, but I want to do it writing the shortest possible code, to use as little CPU memory and time as possible.

from PIL import Image

import numpy as np

def pixelate_mode(mode):
    if mode == 'mean':
        return np.mean
    elif mode == 'min':
        return min
    elif mode == 'max':
        return max

def pixelate(img, (pixel_width, pixel_height), mode):
    width, height = img.size
    W = 0
    while W * pixel_width < width:
        H = 0
        while H * pixel_height < height:
            l = []
            for x in xrange(pixel_width * W, pixel_width * (W + 1)):
                for y in xrange(pixel_height * H, pixel_height * (H + 1)):
                    if x < width and y < height:
                        l.append(img.getpixel((x, y)))
            color = pixelate_mode(mode)(l)
            for x in xrange(pixel_width * W, pixel_width * (W + 1)):
                for y in xrange(pixel_height * H, pixel_height * (H + 1)):
                    if x < width and y < height:
                        img.putpixel((x, y), color)
            H += 1
        W += 1
    return img

And these are some examples applying the program to this image: it has 200×200 pixels, and each square of the image is 10×10 pixels.

pixelate(img, (10, 10), 'mean'), pixelate(img, (5, 5), 'max'),
pixelate(img, (2, 2), 'min') and pixelate(img, (1, 1), 'mean') return the same image.

pixelate(img, (11, 11), 'mean') returns this.

Simple optimisation

Instead of doing color = pixelate_mode(mode)(l) in the nested loop, you could compute modefunc = pixelate_mode(mode) once at the beginning of the function then call modefunc(l) as you need it.

Other optimisation

Instead of checking if x and y are in the range, you could just generate them so that they always are in the range :

l = []
for x in xrange(pixel_width * W, min(width, pixel_width * (W + 1))):
    for y in xrange(pixel_height * H, min(height, pixel_height * (H + 1))):
        l.append(img.getpixel((x, y)))
color = modefunc(l)
for x in xrange(pixel_width * W, min(width, pixel_width * (W + 1))):
    for y in xrange(pixel_height * H, min(height, pixel_height * (H + 1))):
        img.putpixel((x, y), color)

Also, for a more optimised version, you could compute limits only once :

minx, maxx = pixel_width * W, min(width, pixel_width * (W + 1))
miny, maxy = pixel_height * H, min(height, pixel_height * (H + 1))
l = []
for x in xrange(minx, maxx):
    for y in xrange(miny, maxy):
        l.append(img.getpixel((x, y)))
color = modefunc(l)
for x in xrange(minx, maxx):
    for y in xrange(miny, maxy):
        img.putpixel((x, y), color)

List expressions

You could use a list expression to define l. Even better you do not need l as a list at all and could just use a generator expression (Edit: Apparently this does not work for np.array so you have to build the string anyway, may it be with multiple appends or with list comprehension).

minx, maxx = pixel_width * W, min(width, pixel_width * (W + 1))
miny, maxy = pixel_height * H, min(height, pixel_height * (H + 1))
color = modefunc(img.getpixel(x, y)
                for x in xrange(minx, maxx)
                for y in xrange(miny, maxy))
for x in xrange(minx, maxx):
    for y in xrange(miny, maxy):
        img.putpixel((x, y), color)

More optimisations

You could compute the x limits outside the nested loop.

Your while loops are actually for loop in disguise and you could make things more efficient by using for W in range(width/pixel_width) or you could use the step argument of range to use the values you'll actually need : for w in range(0, width, pixel_width): (in both cases, there might be an off-by-one issue but I'll let you check and fix).

Final (untested) version of the code

def pixelate(img, (pixel_width, pixel_height), mode):
    width, height = img.size
    modefunc = pixelate_mode(mode)
    for w in range(0, width, pixel_width):
        minx, maxx = w, min(width, w + pixel_width)
        for h in range(0, height, pixel_height):
            miny, maxy = h, min(height, h + pixel_height)
            color = modefunc(img.getpixel(x, y)
                    for x in xrange(minx, maxx)
                    for y in xrange(miny, maxy))
            for x in xrange(minx, maxx):
                for y in xrange(miny, maxy):
                img.putpixel((x, y), color)
    return img


Replace characters using multiple dependent regular expression substitutions I've written the following Python module to iterate over field values in the following two fields: "NAME_LABEL", "CATEGORY". I'm using regular expressions in the following function: "correct_invalid_char", to substitute characters over three expressions that are dependent on each other. The first expression replaces the following characters "/-space" with an underscore, the result is then passed onto the second expression to remove non-alphanumeric and non-numeric characters. The last regular expression removes duplicate underscores that were originally in the field values or generated due to the first regular expression.

Is there more Pythonic way of writing the following regular expressions to achieve the same result, that would allow me to write better unit tests for the following function?

Original field values:

OBJECTID,NAME,NAME_LABEL,FACILITY_TYPE,CATEGORY,SHAPE_Length,SHAPE_Area
1,Ward 27 - New 1,Ward 27 - New 1,Settlements,Settlements 2,533.176039669,12288.746516
2,429 (Block R),(Block R) 429,Settlements,Settlements 4,508.411033555,9622.22635621
3,Kondelelani (Block 1),Kondelelani (Block 1),Settlements,Settlements 4,738.203751902,22815.0234794
4,Yebo Gogo,Yebo \ Gogo,Settlements,Settlements 1,674.979301727,23413.6988572
5,Tebogo Bottle Store,Tebogo / Bottle Store,Settlements,Settlements 1,329.239037836,7157.39741934
6,Block 1 Y,Block 1 [Y],Settlements,Settlements 2,1893.89651205,82883.9076782
7,Stand 1427, Ga-Rankuwa X25,Stand_ 1427, Ga-Rankuwa X25,Settlements,Settlements 3,1209.46585836,66852.9597381
8,Stand 1719, Ga-Rankuwa X23,Stand 1719, Ga-Rankuwa X23,Settlements,Settlements 3,997.901714538,51299.0275644

Original values: CSV

Updated field values:

"""
Created on 23 Oct 2018

Remove invalid characters

found in SAA fields values:

NAME_LABEL; CATEGORY

@author: Peter Wilson
"""
# import site-packages and modules
import re
import argparse
import arcpy

# set environment settings
arcpy.env.overwriteOutput = True

def sites_fields_list(sites):
    """
    Validate fields found
    and create list for
    update cursor.
    """
    fields = ['NAME_LABEL', 'CATEGORY']
    sites_fields = [f.name for f in arcpy.ListFields(sites) if f.name in fields]
    return sites_fields

def correct_invalid_char(field_value):
    """
    Correct field values
    by replacing characters,
    removing non-alphanumeric,
    non-numeric characters,
    duplicate underscores,
    and changing to title case.
    """
    try:
        underscore = re.sub('[/\- ]', '_', field_value)
        illegal_char = re.sub('[^0-9a-zA-Z_]+', '', underscore)
        dup_underscore = re.sub(r'(_)+', r'\1', illegal_char)
        update_value = dup_underscore.title()
        return update_value
    except TypeError as e:
        print("There's no value in the field: {0}".format(e))
        raise

def update_field_values(sites):
    """
    Iterate over field values
    in: NAME_LABEL; CATEGORY
    and correct field values
    by replacing characters.
    """
    sites_fields = sites_fields_list(sites)
    with arcpy.da.UpdateCursor(sites, sites_fields) as cursor:
        for row in cursor:
            for idx, val in enumerate(row):
                row[idx] = correct_invalid_char(val)
            cursor.updateRow(row)

if __name__ == '__main__':
    description = 'Remove invalid characters in SAA fields NAME_LABEL and CATEGORY'
    parser = argparse.ArgumentParser(description)
    parser.add_argument('--sites', metavar='path', required=True,
                        help='path to input sites feature class')
    args = parser.parse_args()
    update_field_values(sites=args.sites)

I can't say it would be more Pythonic, but we can reduce your number of re.sub() calls from 3 down to 2.

First, we just eliminate all of the invalid letters:

valid_chars = re.sub('[^-/_ 0-9a-zA-Z]', '', field_value)

Then, replace occurrences of one or more '-', '/', '_' and ' ' characters with a single underscore, convert to title case, and return:

return re.sub('[-/_ ]+', '_', valid_chars).title()

Since the try ... except block is unconditionally re-raising the raised exception, it is not really adding much, and could probably be eliminated.

When using a regex over and over, it is usually more efficient to compile the regular expression once, and then reuse the compiled regular expression object.

INVALID_CHARS = re.compile('[^-/_ 0-9a-zA-Z]')
UNDERSCORE_CHARS = re.compile('[-/_ ]+')

def correct_invalid_char(field_value):
    """
    Correct field values by replacing characters, removing non-alphanumeric,
    non-numeric characters, duplicate underscores, and changing to title case.
    """

    valid_chars = INVALID_CHARS.sub('', field_value)
    return UNDERSCORE_CHARS.sub('_', valid_chars).title()

A quick "Hello world"-ish test:

>>> correct_invalid_char('Hello EMPEROR / ***WoRlD_-=-_lEaDeR***')
'Hello_Emperor_World_Leader'


Python - Making A Valid Date Checker - using Regular Expressions Date Detection:
Write a regular expression that can detect dates in the DD/MM/YYYY format.
Assume that the days range from 01 to 31, the months range from 01
to 12, and the years range from 1000 to 2999. Note that if the day or month
is a single digit, it’ll have a leading zero.
Then store these strings into variables named month, day, and
year, and write additional code that can detect if it is a valid date.
April,
June, September, and November have 30 days, February has 28 days, and
the rest of the months have 31 days. February has 29 days in leap years.
Leap years are every year evenly divisible by 4, except for years evenly divisible by 100, unless the year is also evenly divisible by 400. Note how this calculation makes it impossible to make a reasonably sized regular expression
that can detect a valid date.
import re
def check_date(day, month, year):
    # April, June, September, November = 30 days/ February = 28 days, unless leapyear so 29/ rest has 31 days
    month_dict = {4: 30, 6: 30, 9: 30, 11: 30, 2: 28}
    day_bound = month_dict.get(month, 31)

    # month is february
    if day_bound == 28:
        # checks if the year is a leap year
        if year % 4 == 0:
            if year % 100 == 0:
                if year % 400 == 0:
                    day_bound = 29
            else:
                day_bound = 29

    # if the day is in the range of possible days
    if day <= day_bound:
        return True
    return False

# DD/MM/YYYY
string = "31/02/2020"
date_regex = re.compile(r"([0-2]\d|3[01])/(0\d|1[0-2])/([12]\d{3})")
match = date_regex.search(string)
valid = False
if match:
    day = int(match.group(1))
    month = int(match.group(2))
    year = int(match.group(3))
    valid = check_date(day, month, year)

if valid:
    print(f'Day: {day}, Month: {month}, Year: {year}')
else:
    print('Invalid Date!')

Docstrings / Type hints
These allow you to describe how your code works in a pythonic manner. Docstrings allow IDE's and other documentation tools to see what your function/class does. Type hints allow you to show what types of parameters are accepted, and what types of values are returned.
check_date
Instead of calculating a leap year yourself, you can use calendar.isleap from the calendar module.
Return comparisons, not raw booleans
Instead of
if day <= day_bound:
    return True
return False

do this
return day <= day_bound

Does the exact same thing, but looks a lot nicer.
Split code into functions
You've done a good job splitting your code into functions, but I think you could use one more. Instead of parsing the date in the "main" code, put that code in another function, and pass it the date string.
def get_date_values(...) -> ...:
   ...

With all these changes, your final code would look something like this:
import re
import calendar
from typing import Tuple, Union

def check_date(day: int, month: int, year: int) -> bool:
    """
    Returns a bool based on if the date passed is a valid date.

    :param int day: Day.
    :param int month: Month.
    :param int year: Year.

    :return: True if a valid date, False otherwise.
    """
    # April, June, September, November = 30 days/ February = 28 days, unless leapyear so 29/ rest has 31 days
    month_dict = {4: 30, 6: 30, 9: 30, 11: 30, 2: 28}
    day_bound = month_dict.get(month, 31)

    if day_bound == 28:
        if calendar.isleap(year):
            day_bound = 29

    return day <= day_bound

def get_date_values(date: str) -> Union[Tuple[int, int, int], None]:
    """
    Returns a tuple containing the day, month, and year of the passed date.

    :param str date: Date to parse and retrieve values.

    :return: Either a Tuple, or for an invalid date, None.
    """
    date_regex = re.compile(r"([0-2]\d|3[01])/(0\d|1[0-2])/([12]\d{3})")
    match = date_regex.search(date)
    if match:
        return (int(match.group(1)), int(match.group(2)), int(match.group(3)))
    return None

if __name__ == "__main__":
    date = "31/02/2020" #DD/MM/YYYY
    if check_date(*get_date_values(date)):
        print('Valid Date!')
    else:
        print('Invalid Date!')

I'll explain a bit more since I made some changes I haven't mentioned already.
Unpacking
if check_date(*get_date_values(date)):

This line unpacks each item from the tuple returned by get_date_values and passes them to the function. Since the tuple has three values, and check_date accepts three parameters, the * unpacks the tuple and passes each value to the function.


sum, range and rangeSum functions Forgive me if this is too simple, but I'm a beginner trying to learn and understand. I came across an exercise to create these three basic functions, and this is what I came up with.

Any glaring errors?

This program defines three functions:

sum(x,y)
range(x,y,step)
rangeSum (start, end)

sum will give you the sum of two numbers.

range will give you an array of numbers from x to y, with an optional step argument that dictates the increment to use when listing the range. If no step is given, the default value is 1.

rangeSum will give you the sum of a range of numbers from start to end.

// sum(x,y) takes the sum of two numbers
var sum = function(x,y){
  return x + y;
};
// range(x,y,step) gives an array of the range of x and y, with an optional step argument
var range = function(x, y, step){
    if (step == null) step = 1;
  var arr = [];

  if (step > 0) {
    for (var i = x; i <= y; i += step)
      arr.push(i);
  } else {
        for (var i = x; i >= y; i += step)
        arr.push(i);
  }
    return arr;
};
// rangeSum(start,end) gives the sum of the range from start to end.
var rangeSum = function(start,end){
  var arr = [];
  for (var i = start; i <= end; i++)
    arr.push(i);
  return arr.reduce(function(a,b){
    return a + b;
  });
};

The code looks fine but has a few flaws and can be improved. I didn't solve the flaws and change the code: I've left that as an excercise for you.

In general:

Why do you use function expressions instead of function declarations?
I recommend to always use braces, even for single line if statements or for loops. Omitting braces will bite you eventually.
What happens when the parameters of your functions are not numbers or are undefined?

Your code, with my comments added as review points:

// sum(x,y) takes the sum of two numbers
var sum = function(x,y){
    return x + y;
};

// range(x,y,step) gives an array of the range of x and y, with an optional step argument
var range = function(x, y, step){
    if (step == null) step = 1;

    // why do you call this variable arr?
    // It's better to use a meaningful name like result.
    var arr = [];

    // what happens when y is less than x?
    // what happens when step equals zero?

    if (step > 0) {
        for (var i = x; i <= y; i += step)
            arr.push(i);
    } else {
        for (var i = x; i >= y; i += step)
            arr.push(i);
    }
    return arr;
};

// rangeSum(start,end) gives the sum of the range from start to end.
// why do you use other argument names than in the range function?
// it's better to use consistent names.
// By the way: the names of these argument are better than the arguments
// of the range function, so I'd change those to start and end.
var rangeSum = function(start,end){

    // why so complex? You can iterate over the range once and keep a running total.

    var arr = [];
    for (var i = start; i <= end; i++)
        arr.push(i);
    return arr.reduce(function(a,b){
        return a + b;
    });
};


Exposing WCF Service with dynamic search operation I have this WCF service providing search functionality - to be used by our client in his web application. There can be any number of search parameters and possible nesting of AND/OR operations.

Could you please have a look and share some ideas on if I can make it more client-friendly?

  <con:Search>
     <con:clientID>xxx</con:clientID>
     <con:propertyQueries>
        <wil:PropertyQuery Id="0">
            <wil:PropertyID>DocumentType</wil:PropertyID>
            <wil:Value>Statement</wil:Value>
            <wil:Operator>EQUALS</wil:Operator>
        </wil:PropertyQuery>
        <wil:PropertyQuery Id="1" AND="0">
            <wil:PropertyID>BANK</wil:PropertyID>
            <wil:Value>HSBC</wil:Value>
            <wil:Operator>EQUALS</wil:Operator>
        </wil:PropertyQuery>
        <wil:PropertyQuery Id="2" OR="1">
            <wil:PropertyID>BANK</wil:PropertyID>
            <wil:Value>BARCLAYS</wil:Value>
            <wil:Operator>EQUALS</wil:Operator>
        </wil:PropertyQuery>
        <wil:PropertyQuery Id="3" AND="0">
            <wil:PropertyID>Date</wil:PropertyID>
            <wil:Value Start="1">11/01/2014</wil:Value>
            <wil:Value End="1">11/02/2014</wil:Value>
            <wil:Operator>BETWEEN</wil:Operator>
        </wil:PropertyQuery>
    </con:propertyQueries>
  </con:Search>

The values in AND and OR attribute indicates the Id of PropertyQuery to which the condition to be added. For example:

<wil:PropertyQuery Id="2" OR="1">

performs an OR operation between Property Queries with ID of 1, 2.

Any better ideas out there to improve the interface?

Without knowing its intended audience (Intranet / Internet), I'm assuming Intranet.

In my opinion, you are giving a lot of flexibility to this service in one method call, which isn't immediately clear how a user would structure a call to this. The caller has to have knowledge of a "propertyId" which doesn't make too much sense on a glance. I hope your API documentation describes this well, and how the user would retrieve what these IDs are.

So here is my assumption of the coded interface:

public interface ISearchService
{
    List<object> Search(string clientId, IEnumerable<PropertyQuery> propertyQueries);
}

// Element
public class PropertyQuery
{
   // Attribute
   public string PropertyID { get; set; }

   // Array
   public IEnumerable<Value> Values { get; set; }

   // Attribute
   public byte AND { get; set; }

   // Attribute
   public byte OR { get; set; }

   // Element
   public string Operator { get; set; }
}

// Element
public class Value
{
   // Attribute
   public int Id { get; set; }

   // Attribute
   public byte Start { get; set; }

   // Attribute
   public byte End { get; set; }

   // Text
   public string Text { get; set; }
}

To make this a little simpler, I would add a single class that takes all parameters and a class returning the results, following the Request/Response pattern. This means that the interface can change over time, and should not affect backwards compatibility.

public interface ISearchService
{
   // Gets the search criteria for the screen
   SearchCriteriaResponse GetSearchCriterias(SearchCriteriaRequest request);

   // Searches by the search criteria
   SearchResponse SearchBy(SearchRequest searchCriteria);
}

// Base elements for a response
public abstract class ResponseBase
{
   public bool Success { get; set; }
   public string Message { get ; set; }
}

public class SearchCriteriaResponse : ResponseBase
{
   public string[] DocumentTypes { get; set; }
   public string[] BankName { get; set; }
}

public class SearchResponse : ResponseBase
{
   public IEnumerable<SearchResult> Results { get; set; }
}

public class SearchRequest
{
   public string ClientId { get; set; }
   public string[] BankNamesToMatchExactly { get; set; }
   public string[] BankNamesToPartiallyExactly { get; set; }
   public string[] DocumentTypesToMatchExactly { get; set; }
   public string[] DocumentTypesToPartiallyExactly { get; set; }
   public DateTime? StartDate { get; set; }
   public DateTime? EndDate { get;set; }
   public bool CheckBetweenDates { get; set; }
}

This interface is a more clean on its approach. Its slightly larger, but it makes the ORing and ANDing much more obvious. It also makes the code cleaner at the service side as each of these properties have well-named properties and should be easy to implement:

public List<SearchResponse> SearchBy(SearchRequest searchCriteria)
{
   IQueryable dbQuery = DbContext.BankNames;

   foreach(var bankName in searchCriteria.BankNamesToMatchExactly)
   {
      dbQuery = dbQuery.Where(bn => bn.BankName == bankName);
   }

   foreach(var bankName in searchCriteria.BankNamesToMatchPartially)
   {
      dbQuery = dbQuery.Where(bn => bn.BankName.Contains(bankName);
   }

   // etc..
}


Matrix rotation efficiency I am not sure if I should be using recursion or not or if it even helps in increasing the performance of my algorithm. I feel as though I am creating and replacing too many arrays by calling my inner rotate() function.

Should I add a check to convert rotation -270 to 90 and vice-versa so that I am rotating less often?

Please refer to the JSFiddle I have provided for details and clarification on the functions involved in the code below: JSFiddle Demo

var rotateMatrix = function (matrix, n, direction) {
    var ret = matrix.slice();

    var rotate = function(direction, matrix) {
        var r = zeroArr(n, n);
        for (var i = 0; i < n; i++) {
            for (var j = 0; j < n; j++) {
                if (direction < 0) {
                    r[i][j] = matrix[n - j - 1][i];
                } else {
                    r[i][j] = matrix[j][n - i - 1];
                }
            }
        }
        return r;
    };

    for (var turn = Math.abs(direction); turn > 0; turn -= 90) {
        ret = rotate(direction, ret);
    }

    return ret;
};

var tile = [
    ['A', 'B', 'C'],
    ['D', 'E', 'F'],
    ['G', 'H', 'I']
];

trace2('Rotate +180', printMatrix(rotateMatrix(tile, 3, 180)));
trace2('Rotate +90', printMatrix(rotateMatrix(tile, 3, 90)));
trace2('Orginal', printMatrix(tile));
trace2('Rotate -90', printMatrix(rotateMatrix(tile, 3, -90)));
trace2('Rotate -180', printMatrix(rotateMatrix(tile, 3, -180)));

Output:

Rotate +180:

 I| H| G
--+--+--
 F| E| D
--+--+--
 C| B| A

Rotate +90:

 C| F| I
--+--+--
 B| E| H
--+--+--
 A| D| G

Orginal:

 A| B| C
--+--+--
 D| E| F
--+--+--
 G| H| I

Rotate -90:

 G| D| A
--+--+--
 H| E| B
--+--+--
 I| F| C

Rotate -180:

 I| H| G
--+--+--
 F| E| D
--+--+--
 C| B| A

The n parameter is redundant, as it should be possible to deduce the matrix dimensions from matrix itself, using matrix.length and matrix[0].length.  You seem to have made the assumption that matrix is square — you should either document or relax the restriction.

I suggest writing three separate inner functions for the three possible rotations.  One code-reuse technique you could use for the ±90° rotations is to combine transpose and row-swap operations.


malloc and free of a char-array I wrote a function to reverse a char-array (string).
Since I'm beginner and didn't work with malloc and stuff before, maybe someone could take a look, if this is fine, what I'm doing here?

char* reverse_string(char* string)
{
    // getting actual length of the string
    size_t length = strlen(string);

    // allocate some space for the new string
    char* new_string = malloc(sizeof(char)*(length+1));

    // index for looping over the string
    int actual_index = 0;

    // iterating over the string until '\0'
    while(string[actual_index] != '\0')
        new_string[length-actual_index-1] = string[actual_index++];

    // setting the last element of string-array to '\0'
    new_string[length] = '\0';

    // free up the allocated memory
    free(new_string);

    // return the new string
    return new_string;
}

Here are some things that may help you improve your code.

Fix the bug

Once memory is freed, it should not be referenced again.  Unfortunately, your code allocates memory and then frees it and then returns a pointer to the freed memory.  That's a serious bug!  To fix it, simply omit the free within the function and make sure the caller calls free instead.  Alternatively, you could avoid all of that by reversing the passed string in place.

Use the required #includes

The code uses strlen which means that it should #include <string.h> and malloc and free which means that it should #include <stdlib.h>.  It was not difficult to infer, but it helps reviewers if the code is complete.

Use const where practical

In your revere_string routine, the string passed into the function is not and should not be altered.  You should indicate that fact by declaring it like this:

char* reverse_string(const char* string)

Check for NULL pointers

The code must avoid dereferencing a NULL pointer if the call to malloc fails.  The only indication that it has failed is if malloc returns NULL; if it does, it would probably make most sense to immediately return that NULL pointer.

Learn to use pointers instead of indexing

Using pointers effectively is an important C programming skill.  This code could be made much simpler by doing an in-place reversal of the passed string and by using pointers:

char* reverse_string(char* string) {
    if (string == NULL)
        return string;
    char *fwd = string;
    char *rev = &string[strlen(string)-1];

    while (rev > fwd) {
        char tmp = *rev;
        *rev-- = *fwd;
        *fwd++ = tmp;
    }
    return string;
}


In need to speed up this code that filters the data of XML using XMLReader I am using XMLReader and avoided SimpleXML because I must handle a huge file and for memory issues, SimpleXML is not the ideal solution. However, even I coded the below script in SimpleXML, it gives me the result really much faster.

Because speed and memory is a must for this project, is there any way to speed up this code? What it does, it filters the books and show those that contains the word jQuery.

error_reporting(~0);

$xml = <<<EOD
<?xml version="1.0" encoding="ISO-8859-1"?>
<library>
    <book isbn="781">
        <name>SCJP 1.5</name>
        <info>Sun Certified Java Programmer book</info>
    </book>
    <book isbn="194">
        <name>jQuery is Awesome!</name>
        <info>jQuery Reference Book</info>
    </book>
    <book isbn="199">
        <name>jQuery 101</name>
        <info>All you need to know about jQuery</info>
    </book>
</library>
EOD;

$file = 'data://text/plain;base64,'.base64_encode($xml);

class XMLBookIterator implements iterator
{
    private $file;
    private $reader;
    private $state;
    private $key = 0;
    private $book;
    private $valid;
    public function __construct($file)
    {
        $this->file = $file;
    }
    public function current()
    {
        return $this->book;
    }
    public function key()
    {
        return $this->key;
    }
    public function next()
    {
        $reader = $this->reader;
        while ($next = $reader->read())
        {
            switch ($reader->nodeType)
            {
                case (XMLREADER::ELEMENT):
                    $case = $reader->localName.'|'.$this->state;
                    switch($case)
                    {
                        case 'library|0':
                            break;
                        case 'book|1':
                            $this->book = new stdClass;
                            $this->book->isbn = $reader->getAttribute('isbn');
                            break;
                        case 'name|2':
                            $this->book->name = $reader->readInnerXML();
                            break;
                        case 'info|3':
                            $this->book->info = $reader->readInnerXML();
                            $this->state=0;
                            break;
                        default:
                            throw new Exception(sprintf('Invalid State: %s.', $case));
                    }
                    $this->state++;
                    if ($this->state === 1) break 2;
            }
        }
        $this->valid = $next;
        $this->key++;
    }
    public function rewind()
    {
        if ($this->reader) $this->reader->close();
        $this->reader = new XMLReader();
        $this->reader->open($this->file);
        $this->state = 0;
        $this->next(); // move to first element or invalidate
        if ($this->valid) $this->next(); // get first book
        $this->key = 0;
    }
    public function valid()
    {
        return $this->valid;
    }
}

class BookFilterIterator extends FilterIterator
{
    private $term;
    public function __construct($iterator, $term)
    {
        parent::__construct($iterator);
        $this->term = $term;
    }
    public function accept() {
        $book = parent::current();
        $name = $book->name;
        return false !== stripos($name, $this->term);
    }
}

// filtered books:

$books = new XMLBookIterator($file);

$filtered = new BookFilterIterator($books, 'jQuery');

foreach($filtered as $key => $book)
{
    echo 'book (', $key, '):', "\n";
    print_r($book);
}

Your code seems quite tidy and easy to understand, congratulations. But maybe you can check for built in functionality. XPath can search items for you:

http://www.php.net/manual/en/simplexmlelement.xpath.php
http://www.w3schools.com/xpath/xpath_syntax.asp