Locating a list of parts I have an ASP.NET MVC3 web application that displays a list of parts (a part is a simple entity with a number and a description).

I have updated the action method to support filtering and paging:

[HttpGet]
public ViewResult Index(int page = 1, int pageSize = 30, string filter = "All")
{
    IEnumerable<Part> parts;
    int totalParts;

    var acceptableFilters = new string[] {
        "All", "123", "A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "K", "L",
        "M", "N", "O", "P", "Q", "R", "S", "T", "U", "V", "W", "X", "Y", "Z" };

    // This checks that the filter value received by the method is an acceptable value.
    // E.G. If a user types in a value in the query string that is not contained in the
    // array above, then the filter is reset to 'All' and all records are queried.
    if (!acceptableFilters.Contains(filter))
    {
        filter = "All";
    }

    if (filter == "All")
    {
        parts = Database.Parts
            .OrderBy(p => p.Number)
            .Skip((page - 1) * pageSize)
            .Take(pageSize);

        totalParts = Database.Parts.Count();
    }
    else if (filter == "123")
    {
        var numbers = new string[]{"1","2","3","4","5","6","7","8","9","0"};

        parts = Database.Parts
            .OrderBy(p => p.Number)
            .Where(p=> numbers.Contains(p.Number.Substring(0,1)))
            .Skip((page - 1) * pageSize)
            .Take(pageSize);

        totalParts = Database.Parts
            .Count(p => numbers.Contains(p.Number.Substring(0, 1)));
    }
    else
    {
        parts = Database.Parts
            .OrderBy(p => p.Number)
            .Where(p => p.Number.StartsWith(filter))
            .Skip((page - 1) * pageSize)
            .Take(pageSize);

        totalParts = Database.Parts.Count(p => p.Number.StartsWith(filter));
    }

    PartsListViewModel viewModel = new PartsListViewModel()
    {
        Filter = filter,
        PageInfo = new PageInfo(page, pageSize, totalParts),
        Parts = parts,
    };

    return View(viewModel);
}

The idea is this:

If the filter is equal to 'All' then query all records.
If the filter is equal to '123' then query all records that start with a number.
If the filter is equal to a letter (A, B, C) then query all records that begin with said letter.

Once the required records have been queried I then need to do some calculations to determine how many pages I have and how many items to display on each page.

This works perfectly but I do not like the code I currently have, specifically the if statement that determines the Linq query to be used as the majority of the code is identical except for the where clause (or lack of where clause if all records are being pulled down). I also don't like the fact that I have to run each query twice: once to get the records and a second time to determine the total record set size.

So, is there a better way of achieving the same result? can the Linq queries be restructured in a more elegant way to reduce the redundant code or is this the best way?

You can construct LINQ queries step by step instead of writing them all at once

parts = Database.Parts;
if (filter == "123") {
    parts = parts.Where(p => p.Number[0].IsDigit())
} else if (filter != "All") { // Alphabetic filter
    parts = parts.Where(p => p.Number.StartsWith(filter))
}
int totalParts = parts.Count();
parts = parts
    .OrderBy(p => p.Number)
    .Skip((page - 1) * pageSize)
    .Take(pageSize);

I followed the DRY software design principle here. DRY = Don't Repeat Yourself. If you repeat the same (or almost the same) code over and over, the code is less maintainable and is more susceptible to mistake. The fact that you have to write more, if you repeat yourself, is rather secondary.


Count number of even and odd numbers from input I had to write a program that accepted five numbers from the user. Then it will display the number of odd and even numbers, and a total sum of all numbers.

#include <iostream>
using namespace std;

int main()
{
    int a;
    int b;
    int c;
    int d;
    int e;
    int sum;
    int remainder;
    int bremainder;
    int cremainder;
    int dremainder;
    int eremainder;
    int even;
    int odd;
    int total = 0;
    int Ototal = 0;

    cout << "Please enter the 5 numbers you would like calculated: \n";
    cin >> a;
    cin >> b;
    cin >> c;
    cin >> d;
    cin >> e;

    remainder = a % 2;
    bremainder = b % 2;
    cremainder = c % 2;
    dremainder = d % 2;
    eremainder = e % 2;

    sum = (a + b + c + d + e);

    if (remainder = 0)
    {
        total+=remainder;
    }
    else {
        Ototal += remainder;
    }
    if (bremainder = 0)
    {
        total += bremainder;
    }
    else {
        Ototal += bremainder;
    }
    if (cremainder = 0)
    {
        total += cremainder;
    }
    else {
        Ototal += cremainder;
    }
    if (dremainder = 0)
    {
        total += dremainder;
    }
    else {
        Ototal += dremainder;
    }
    if (eremainder = 0)
    {
        total += eremainder;
    }
    else {
        Ototal += eremainder;
    }

    cout << total << "\n";

    cout << Ototal << "\n";

    cout << sum << "\n";
    system("PAUSE");
        return 0;
}

Do not use system("pause"). Read
this
Stop using using namespace std. Read
this
You have an unneccessary amount of variables. You don't need to have
a variable to hold the remainder for each number.
Just have a function that determines whether the input number is odd
or even. I think you might have to rethink the logic of your
program.

//Your function should do the following:
//MAX is a const int. In your case it is 5
    for(int i=0; i<MAX; i++){
        if(numbers[i] % 2 == 0)
            evenNumbers++;
        else
            oddNumbers++;

        sum += numbers[i];
    }

Here is how I would have done it:

#include <iostream>
#include <vector>
#include <iterator>

using std::cin;
using std::cout;
using std::endl;
using std::vector;

int main(){

    int value = 0;
    int oddNumbers(0), evenNumbers(0), sum(0);

    std::istream_iterator<int> begin(cin);
    std::istream_iterator<int> end;

    //while(cin >> value)
    //  numbers.push_back(value);

    //Instead of a while loop, I use input stream iterators.
    vector<int> numbers;
    std::back_insert_iterator<std::vector<int>> iter (numbers);

    std::copy(begin,end,iter);

    //Range based for-loop (C++11)
    for(auto i : numbers){
        if(i % 2 == 0){
            evenNumbers++;
        }else {
            oddNumbers++;
        }

        sum += i;

    }

    cout << "# of odd numbers: " << oddNumbers << "\n";
    cout << "# of even numbers: " << evenNumbers << "\n";
    cout << "Sum of all numbers: " << sum << endl;

}


Simple c# MemoryCache mechanism to reduce number of db calls I have some legacy code which gets a list of Students and their corresponding School. It's LinqToSql and there is no lazy loading:

public class Student
{
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public School AttendedSchool { get; set; }

    public Student(int studentId) {
        // code to get the student from the database goes here, this
        // gets FirstName, LastName and SchoolId from the db.
        FirstName = dbResult.FirstName;
        LastName = dbResult.LastName;
        AttendedSchool = new School(dbResult.SchoolId);
    }
}

public class School
{
    public string SchooolName { get; set; }
    public School(int schoolId) {
        // go to database and call 'GetDbSchool' using schoolId
        SchoolName = resultFromDatabase.SchoolName;
    }
}

There are thousands of students but only a handful of schools. Because of the way the code is built we see a GetDbSchool call to the database every time a Student is created but this clearly isn't required, it's getting the same school details again and again. I want a simple cache of schools to prevent so many db calls. The cache doesn't need a mechanism to empty or update and if there is a race condition and the school is fetched from the database twice that's not a problem. This is the caching code I've written:

public static class SchoolCache
{
    public static School GetSchool(int schoolId)
    {
        if (MemoryCache.Default["school_" + schoolId] != null)
        {
            return (School)MemoryCache.Default["school_" + schoolId.ToString()];
        }

        var cachePolicy = new CacheItemPolicy { SlidingExpiration = TimeSpan.FromSeconds(100) };
        var schoolFromDb = // go to database to get the school details
        MemoryCache.Default.Add("school_" + schoolId, schoolFromDb, cachePolicy);
        return schoolFromDb;
    }
}

Then in the School class I can use the cache and we get one db call per school:

public School(int schoolId) {
    SchoolName = SchoolCache.GetSchool(schoolId).SchoolName;
}

It works as expected but I don't see many caches this simple, most examples and implementations are more complicated. Are there any obvious problems with this simple implementation?

This is a really bad pattern: neither your Student nor your School class should be responsible for retrieving themselves from the DB. Imagine retrieving a list of 100 Students: that would mean 100+ queries to the DB. You can't even apply filters (e.g. get all the Students who go to one School).

Your caching mechanism is solving the wrong problem. What you should do is:

Get all Schools and store them in a Dictionary<int, School> where the key is the Id of the School.
Get all Students, and then loop through them and attach the School based on the SchoolId.

Two queries instead of hundreds or thousands.


Haskell command line todo list I've implemented a command line todo list in Haskell. I've run hlint and build with -Wall, but is there anything else I can improve?

module Main where

import Control.Applicative ((<$>))
import Prelude.Unicode
import System.IO (IOMode(ReadMode, WriteMode), utf8, hClose, hGetContents, hPutStr, hSetEncoding, openFile)
import System.IO.Error (tryIOError)
import Text.Read (readMaybe)

type TodoList = [String]

options :: [(Char, String, Maybe (TodoList -> IO TodoList))]
options =
    [ ('c', "create new item", Just createItem)
    , ('d', "delete item", Just deleteItem)
    , ('x', "delete everything", Just . const $ return [])
    , ('l', "load file", Just loadFile)
    , ('s', "save to file", Just saveToFile)
    , ('q', "quit", Nothing)
    ]

main :: IO ()
main = do
    putStrLn "TODO-LIST\n"
    _ <- loop []
    return ()

loop :: TodoList -> IO TodoList
loop todos = do
    putStrLn "----------------------------"
    putStrLn "You have to do these things:"
    putStrLn ∘ unlines $ zipWith (\i e -> show i ++ ": " ++ show e) ([0..] :: [Integer]) todos
    input <- askForInput
    case filter (\(c, _, _) -> c == input) options of
        [(_, _, action)] -> case action of
            Nothing   -> return todos
            Just func -> func todos >>= loop
        _                -> putStrLn "action not found" >> loop todos

askForInput :: IO Char
askForInput = do
    putStrLn "Select an action"
    putStrLn ∘ unlines $ map (\(char, desc, _) -> char : ") " ++ desc) options
    line <- getLine
    case line of
        [c] -> return c
        _   -> askForInput

createItem :: TodoList -> IO TodoList
createItem todos = do
    putStrLn "enter a description of the new item:"
    (: todos) <$> getLine

deleteItem :: TodoList -> IO TodoList
deleteItem todos = do
    putStrLn "enter the number of the item you want to delete"
    line <- getLine
    case readMaybe line of
        Nothing -> deleteItem todos
        Just n  -> case removeAt n todos of
            Nothing  -> deleteItem todos
            Just val -> return val
    where
        removeAt :: Int -> [a] -> Maybe [a]
        removeAt n _ | n < 0 = Nothing
        removeAt _ []        = Nothing
        removeAt 0 (_:xs)    = Just xs
        removeAt n (x:xs)    = (x :) <$> removeAt (n-1) xs

loadFile :: TodoList -> IO TodoList
loadFile todos = do
    putStrLn "enter the file you want to load"
    line <- getLine
    eitherExOrFile <- tryIOError $ openFile line ReadMode
    case eitherExOrFile of
        Left _     -> loadFile todos
        Right file -> do
            hSetEncoding file utf8
            eitherExOrContents <- tryIOError $ hGetContents file
            case eitherExOrContents of
                Left _        -> loadFile todos
                Right content -> return (todos ++ lines content)

saveToFile :: TodoList -> IO TodoList
saveToFile todos = do
    putStrLn "enter the filename to save your todolist. The file will be overwritten."
    line <- getLine
    eitherExOrFile <- tryIOError $ openFile line WriteMode
    case eitherExOrFile of
        Left _     -> saveToFile todos
        Right file -> do
            hSetEncoding file utf8
            result <- tryIOError . hPutStr file $ unlines todos
            case result of
                Left _  -> putStrLn "couldn't write to file"
                Right _ -> hClose file
            return todos

I'm a little bit worried about the code duplication in readFile / saveToFile and the nested case of.

The general idea is to use library code, particularly to eliminate explicit recursion, and inline things used only once.

MaybeT abstracts computations that can fail and abort at some point, and allows you to bind into pattern matches that fail the computation if they don't match.

desperately abstracts retrying them until they work, using MaybeTs Alternative instance.

StateT abstracts computations that carry around a piece of state to read and write to.

forever defeats the need for loop to manually loop.

Contrary to that comment, abstraction is kinda Haskell's thing.

import System.Exit (exitSuccess)
import Control.Monad.State
import Control.Monad.Trans.Maybe
import Safe (fromJustNote)

type TodoList = [String]

main :: IO ()
main = do
    putStrLn "TODO-LIST\n"
    (`evalStateT` []) $ forever $ do
        say "----------------------------"
        say "You have to do these things:"
        say ∘ unlines ∘ zipWith (\i e -> show i ++ ": " ++ show e) ([0..] :: [Integer]) =<< get
        input <- desperately $ do
            say "Select an action"
            say ∘ unlines $ map (\(char, desc, _) -> char : ") " ++ desc) options
            [c] <- hear
            return c
        case find (\(c, _, _) -> c == input) options of
            Just (_, _, action) -> action
            _ -> say "action not found"

desperately :: Monad m => MaybeT m a -> m a
desperately = fmap (fromJustNote "desperately") . runMaybeT . asum . repeat

say = liftIO . putStrLn
hear = liftIO getLine

To bring main's last lines closer in method with the rest of your code, you could do Just (_, _, action) <- find (\(c, _, _) -> c == input) options and put another desperately to the right of forever. (Or runMaybeT, because the forever repeats everything anyway, but that's kinda incidental.)

When you implement a transformation of a simple partial algorithm into a safe one, chances are safe's already got something, here splitAtExactMay.

(,,) in its prefix form allows me to not close each option with a multi-line closing bracket, and shuddup indentation blocks split code into units just as well as names do, and then you don't need to choose names for everything. Each option even already lists a description of what it does!

import Safe.Exact (splitAtExactMay) -- goes at the top of the file, but relevant here

options :: [(Char, String, StateT TodoList IO ())]
options =
    [ (,,) 'c' "create new item" $ do
        say "enter a description of the new item:"
        hear >>= modify . (:)
    , (,,) 'd' "delete item" $ desperately $ do
        say "enter the number of the item you want to delete"
        Just n <- readMaybe <$> hear
        Just (before, _ : after) <- gets $ splitAtExactMay n
        put $ before ++ after
    , (,,) 'x' "delete everything" $ put []
    , (,,) 'l' "load file" $ desperately $ do
        say "enter the file you want to load"
        line <- hear
        Right file <- liftIO $ tryIOError $ openFile line ReadMode
        liftIO $ hSetEncoding file utf8
        Right content <- liftIO $ tryIOError $ hGetContents file
        modify (++ lines content)
    , (,,) 's' "save to file" $ desperately $ do
        say "enter the filename to save your todolist. The file will be overwritten."
        line <- hear
        Right file <- liftIO $ tryIOError $ openFile line WriteMode
        liftIO $ hSetEncoding file utf8
        result <- liftIO . tryIOError . hPutStr file . unlines =<< get
        case result of
            Left _  -> say "couldn't write to file"
            Right _ -> liftIO $ hClose file
    , (,,) 'q' "quit" exitSuccess
    ]


Finding all intervals that match predicate in vector I have a function find_all_intervals_below that iterates through a vector and finds all the index intervals of at least a given length where each element within the interval is below a given threshold.

struct Interval {
    int start;
    int end;
};

std::vector<Interval>
find_all_intervals_below(const std::vector<int> &v, const int &threshold,
                         const int &min_length) {
    auto start_position { 0 };
    auto end_position { 0 };
    std::vector<Interval> intervals;
    bool found_start { false };

    for (auto current_pos = 0; current_pos < v.size(); ++current_pos) {
        if (v[current_pos] <= threshold and not found_start) {
            start_position = current_pos;
            end_position = 0;
            found_start = true;
        } else if (found_start and v[current_pos] > threshold and end_position == 0) {
            end_position = current_pos;
            if (end_position - start_position >= min_length) {
                Interval interval;
                interval.start = start_position;
                interval.end = end_position;
                intervals.push_back(interval);
            }
            start_position = 0;
            found_start = false;
        }
    }
    if (found_start and end_position == 0 and v.size() - start_position >= min_length) {
        end_position = v.size();
        Interval interval;
        interval.start = start_position;
        interval.end = end_position;
        intervals.push_back(interval);
    }
    return intervals;
}

This function works perfectly fine, I would just like to get some input from others as I imagine there is likely much more succinct ways of doing this. search_n from STL looks like it might be a solution but I couldn't figure out how to use it for my case.

EDIT: I need the solution to be C++11 compatible, unfortunately.

Test example

const auto min_len { 3 };
const auto threshold { 3 };
const std::vector<int> v { 4, 2, 1, 1, 4, 1, 2, 4 };

const auto actual { find_all_intervals_below(v, threshold, min_len) };
const std::vector<Interval> expected { Interval(1, 4) };

assert(actual == expected);

Code Review

This piece

        Interval interval;
        interval.start = start_position;
        interval.end = end_position;
        intervals.push_back(interval);

can be transformed into

intervals.emplace_back(start_position, end_position);

Don't accept small objects by reference for read-only purposes. Although it usually doesn't hurt, in most implementations reference (which is implemented as pointer) will take up more space (compiler will probably inline the function or just pass by value though).
Algorithm. When there is a state which is represented by combination of flags and some metadata, flags usually go out of hand quickly. I would instead implement something like this:

1. Set previous, current to start of the input
2. previous = current
3. current = first index of element that is higher than threshold
4. if current - previous >= minlength, add to result
5. increment current
6. Go to 2

One could also create it the other way around, e.g. searching for those below threshold.

Alternative implementation

#include <vector>
#include <algorithm>
#include <type_traits>

using index_type = std::make_signed_t<std::size_t>;

struct interval {
    index_type first;
    index_type last;
};

bool operator==(const interval lhs, const interval rhs) {
    return lhs.first == rhs.first && lhs.last == rhs.last;
}

std::vector<interval> find_suitable_intervals(const std::vector<int>& input,
                                              const int threshold,
                                              const index_type min_length) {
    auto predicate = [threshold](int x) {
        return x <= threshold;
    };
    std::vector<interval> intervals;
    auto first = input.begin();
    auto previous = input.begin();
    auto current = first;
    while (current != input.end()) {
        previous = current;
        current = std::find_if_not(current, input.end(), predicate);
        if (current - previous >= min_length) {
            intervals.push_back({previous - first, current - first});
        }
        if (current == input.end()) {
            break;
        }
        ++current;
    }

    return intervals;
}

int main() {
    const int min_length = 3;
    const int threshold = 3;
    const std::vector<int> v { 4, 2, 1, 1, 4, 1, 2, 4 };

    const auto actual = find_suitable_intervals(v, threshold, min_length);
    const std::vector<interval> expected { {1, 4} };

    return actual != expected;
}

Wandbox Demo.

The logic got more "flat", but there are culprits of bridging STL style with more traditional style. Also, since incrementing iterator beyond end will cause undefined behavior, I had to put in the condition to check if the loop reached end. Mixing std::size_t and std::distance/difference will cause a warning and will require a cast to get rid of the warning, since one is unsigned and the other is not, thus I created index_type. There are rumors of std::index, but I wouldn't expect it in near future.

One could also make the condition an input into the function, e.g. predicate. Then it would look like this:

find_suitable_intervals(data, min_length, [threshold](auto x) {
    x < threshold;
});

Which is I believe is a bit more readable.


Recursive method to return a set of all combinations Can someone explain me how this code works or if it is possible to be written in another way? I tried it with just ArrayList but cannot figure it out.

public static Set<Set<Integer>> combinations(List<Integer> groupSize, int k) {

    Set<Set<Integer>> allCombos = new HashSet<Set<Integer>> ();
    // base cases for recursion
    if (k == 0) {
        // There is only one combination of size 0, the empty team.
        allCombos.add(new HashSet<Integer>());
        return allCombos;
    }
    if (k > groupSize.size()) {
        // There can be no teams with size larger than the group size,
        // so return allCombos without putting any teams in it.
        return allCombos;
    }

    // Create a copy of the group with one item removed.
    List<Integer> groupWithoutX = new ArrayList<Integer> (groupSize);
    Integer x = groupWithoutX.remove(groupWithoutX.size()-1);

    Set<Set<Integer>> combosWithoutX = combinations(groupWithoutX, k);
    Set<Set<Integer>> combosWithX = combinations(groupWithoutX, k-1);
    for (Set<Integer> combo : combosWithX) {
        combo.add(x);
    }
    allCombos.addAll(combosWithoutX);
    allCombos.addAll(combosWithX);
    return allCombos;
}

Basically the algorithm returns all subsets of groupSize (which should really be renamed to group), that have k elements.

The method works recursively. To do so it needs two things : establish primitive cases that end the recursion, and a way of formulating the problem in terms of itself, yet closer to the primitive form.

This algorithm has two primitive cases :

k == 0 : there is only one set that has 0 elements : the empty set.
k > size of the group : there are no subsets that have more elements than the original set

So there are two ways we can get to a primitive case : formulating it in terms of smaller sets (smaller group), or in terms of smaller subsets (smaller k)

The formulation in terms of itself for this algorithm is : given an element x from the group, its combinations are : all subsets that contain x, and all subsets that do not contain x.

All subsets that do not contain x, is really all subsets of the group without x of size k (moves closer to second primitive case)
All subsets that contain x, is all subsets of the group without x of size k-1 with x added to those subsets. (moves closer to both primitive cases)

The algorithm takes the last element of the group as x (but could just as well take another) : Integer x = groupWithoutX.remove(groupWithoutX.size()-1);

An example will make this clearer :

for groupSize = [1, 2, 3, 4, 5] and k = 3 the output would be

[[1, 2, 3], [1, 2, 4], [1, 2, 5], [1, 3, 4], [1, 3, 5], [2, 3, 4], [2, 3, 5], [1, 4, 5], [2, 4, 5], [3, 4, 5]]

Picking the last element x = 5

this is

all subsets of [1, 2, 3, 4] of size k = 3 : i.e. [[1, 2, 3], [1, 2, 4], [1, 3, 4], [2, 3, 4]]
and all subsets of [1, 2, 3, 4] of size k = 2 and 5 added i.e. [[1, 2, 5], [1, 3, 5], [1, 4, 5], [2, 3, 5], [2, 4, 5], [3, 4, 5]]


Check if an Array of Arrays contains a value in node js In C#, I could check if any of the nested List collection has a value. Example of such structure Check a List[]> contains a value using Linq

List<int> a= new List<int>(){ 2, 2, 3, 4, 5};
List<int> b= new List<int>() { 2, 2, 2, 6 };
List<List<int>[]> c = new List<List<int>[]>();
c.Add(new[]{a,b});

  // checking if the value is in any of the collections. for example, this will result to 2
  var result = c.SelectMany(x => x).SelectMany(x => x).Any(p => p == 2);

but since I'm exploring the LINQ capabilities in node js , I decided to extend this to an Array of Arrays

var data= [[2, 2, 3, 4, 5],[2, 2, 2, 6], [9], [13, 1,1,90]];
var LINQ = require('node-linq').LINQ;
var resultI=new LINQ(data).Any(function(row)
{
 return  checkElement(row);
});
 console.log(resultI);

function checkElement(array)
    {
     for(var i = 0 ; i< array.length; i++)
        {
           if(array[i]== 19){
             return true;
         }
     }
 }

Note: I installed the LINQ module  using
 npm install https://github.com/wearefractal/node-linq/tarball/master
Can this be improved? please any suggestions will be quite appreciated.

Style

Your braces are not styled consistently, and most JavaScript developers find sameline braces more idiomatic.
I think it is better form to place your requires all the way on the top

Approach

You basically rewrote Array.indexOf , except that indexOf gives more info (where is the element), so I would rewrite this with indexOf.
Searching for value 19 makes your hasElement function very limited, it would be a more flexible function if you pass the row and the search value.

All in all, I would counter-propose something like this:

var LINQ = require('node-linq').LINQ;

var data= [[2, 2, 3, 4, 5],[2, 2, 2, 6], [9], [13, 1,1,90]];

var resultI=new LINQ(data).Any(function(row){
 return hasElement(row, 19);
});
console.log(resultI);

function hasElement(array, value){
  return array.indexOf( value ) != -1;
}

The next level?

The flow of LINQ gets a bit broken with the inline function, it would be even cooler if you would call a function there that returns a function seeking for the right value:

var LINQ = require('node-linq').LINQ;

var data= [[2, 2, 3, 4, 5],[2, 2, 2, 6], [9], [13, 1,1,90]];

var resultI=new LINQ(data).Any( seekValue(19) );
console.log(resultI);

function seekValue( value) {
  return function boundSeekValue( row ){
    return row.indexOf( value ) != -1;
  };
}


Is my code Declarative (RxSwift) ? Please help me to explain why not? I'm newbie in Swift and Rx programming. Is my below code is declarative ? If not, please rewrite it and explain to help me more understanding about declarative :( (my English is so bad)

// MARK: RxSwift
func processPhotoApiService(target: PhotoService) -> Observable<[String: Any]> {
    return RxMoyaProvider<PhotoService>().request(target).map { try $0.mapJSON() as! [String: Any] }
}

.....

ApiServiceManager.shared.processPhotoApiService(target: .getNews(consumerKey: ApiConstants.CONSUMER_KEY, page: 1, imageSize: ApiConstants.MEDIUM_IMG_SIZE))
    .map {
        NewsJson(json: $0)!.photos
    } .flatMap { photos in
        Observable.from(photos).flatMap { photo in
            Observable.from(optional: PhotoItem(name: photo.name, URL: photo.imageUrl, viewCount: photo.viewCount, ratingCount: photo.ratingCount))
        }.toArray()
    }.subscribe { event in
    switch event {
    case .next(let dictionary):
        print("onNext:", dictionary)
        break
    case .error(let error):
        print("onOtherError:", error)
        break
    default:
        break
    }
}

In order for code to be declarative, it must express an invariant. Something that is true throughout the entire life of the variables involved. In other words, a direct chain from input to output. Think Excel. In Excel, every cell in the spreadsheet is either an input variable or a transformation of one or more cells which recursively go back to input variables and that's what makes it declarative.

Your code is not declarative because it doesn't specify what user action would cause processPhotoApiService to get called.

https://medium.com/@danielt1263/imperative-vs-declarative-programming-a74f6cceff0e


Polymorphism with overrides and base calls simulating an employee I've studied and worked a lot on the subject for a month or two to get closer to the concept in a cleaner way, i.e. no virtual tables and no bunch of functions with prefixes etc. and most importantly functions are to be defined within structs as pointers to simulate OOP classes as much as possible.

Only constructors with prefixes are to be available publically. I also wanted to have the ability to call the override super method.

I'm not sure if following example is a compile time or run time polymorphism but it's satisfying the runtime aspects of it but then isn't it what we need without the need to have a look at the type of object? Please enlighten.

I'm putting it out there for critical analysis and review or problem with the approach.

The idea is to introduce a pointer of the parent type that retains a reference to original methods, the sub structures can override the parent implementation of the methods but the call to original methods are available via base pointer.

main.c

int main() {
    Person* person = Person_new("John", "Doe");
    Employee* employee = Employee_new("Jane", "Doe", "Acme", 40000);
    person->display(person); // displaying Person object
    puts("------");
    employee->super.display(&employee->super); // displaying employee info
    // ((Person *)employee)->display(employee); // or this
    person->release(person);
    employee->super.release(&employee->super);
    return 0;
}

person.h

typedef struct Person Person;
struct Person {
    Person *base; // to keep access to original funcs in case of overrides
    char* first;
    char* last;
    void (*display)(const Person *self);
    void (*release)(Person *self);
};
Person* Person_new(char* first, char* last);

person.c

static void display(const Person *self) {
    printf("First: %s\n", self->first);
    printf("Last: %s\n", self->last);
}

static void release(Person *self) {
    free(self);
}

Person* Person_new(char* first, char* last) {
    Person* self = malloc(sizeof *self);
    if(self == NULL) return NULL;

    self->first = strdup(first);
    self->last = strdup(last);
    self->display = display;
    self->release = release;

    self->base = self; // assigning reference to self to retain original functions in case they get overridden

    return self;
}

employee.h

typedef struct Employee Employee;
struct Employee {
    Person super;
    char* company;
    int salary;
};
Employee* Employee_new(char* first, char* last, char* company, int salary);

employee.c

static void display(const Person* self) {
    self->base->display(self->base); // calling super class func

    printf("Company: %s\n", ((Employee *)self)->company);
    printf("Salary: %d\n", ((Employee*)self)->salary);
}

static void release(Person *self) {
    self->base->release(self->base); // calling super class func
    free(self);
}

Employee* Employee_new(char* first, char* last, char* company, int salary) {
    Employee* employee = malloc(sizeof(Employee));
    if(employee == NULL) return NULL;

    employee->super = *Person_new(first, last);
    employee->super.display = display; // override
    employee->super.release = release; // override

    employee->company = strdup(company);
    employee->salary = salary;

    return employee;
}

First: John
Last: Doe
---
First: Jane
Last: Doe
Company: Acme
Salary: 40000

Edit

Discussion with Pablo confirmed that the approach is correct. I'm still seeking some discussion on whether this approach falls under compile time or run time polymorphism.

There is one thing I don't line about Employee: you are leaking memory.

Person super; should be a pointer, not the object. You are creating the
object with malloc, by doing

    employee->super = *Person_new(first, last);

you are leaking memory because you lose the reference of the pointer returned
by malloc and you cannot free it when you destroy the object. And you are not
checking if malloc is returning 0. So I'd change it to:

typedef struct Employee Employee;
struct Employee {
    Person super;
    char* company;
    int salary;
    Person *super_ref;
};

Person* Employee_new(char* first, char* last, char* company, int salary) {
    Employee* employee = malloc(sizeof(Employee));
    if(employee == NULL)
        return NULL;

    employee->super_ref = *Person_new(first, last);
    if(employee->super_ref == NULL)
    {
        free(employee);
        return NULL;
    }

    employee->super = *employee->super_ref;
    employee->super.display = display; // override

    employee->company = company;
    employee->salary = salary;

    return (Person *)employee;
}

so now the destroy method can do free(obj->super_ref); to free the memory of
the base object.

edit

An alternative is that you create a init function for Person objects that gets
an object as an argument and does not allocate memory by itself. I'd do:

int Person_init(Person *self, char* first, char* last))
{
    if(self == NULL || first == NULL || last == NULL)
        return NULL;

    self->first = first;
    self->last = last;
    self->display = display;

    self->base = self; // assigning reference to self to retain original functions in case they get overriden

    return 1;
}

Person* Person_new(char* first, char* last) {
    Person* self = malloc(sizeof *self);

    if(self == NULL)
        return NULL;

    if(Person_init(self, first, last) == 0)
    {
        free(self);
        return NULL;
    }

    return self;
}

and for the Employee class

Person* Employee_new(char* first, char* last, char* company, int salary) {
    if(first == NULL || last == NULL || company == NULL)
        return NULL;

    Employee* employee = malloc(sizeof *employee);
    if(employee == NULL)
        return NULL;

    if(Person_init(&employee->super, first, last) == 0)
    {
        free(employee);
        return NULL;
    }
    employee->super.display = display; // override

    employee->company = company;
    employee->salary = salary;

    return (Person *)employee;
}

Then you don't have to worry about freeing the base object.

edit2

*OP said in the comments**

  I'm wondering what's wrong with the freeing the base pointer, I'm assigning it right away to the new value free(&employee->super); employee->super = *super;

Sorry, it seem to me you don't have a real understanding of memory managment and
what the * dereferencing operator does.

First of all, you have no guarantee that after free(ptr) you can access
the contents of ptr. After the free, ptr points to an invalid location and
accessing/dereferencing it is undefined behaviour.

Secondly

employee->super = *Person_new(first, last);

is the same as doing

Person *tmp = Person_new(first, last);
employee->super = *tmp;

*tmp is dereferencing tmp, the type is Person not Person*. In C when you
do an assigment with struct objects (not pointers) you are copying bit by bit
the bit pattern into a new object. That means that the bit pattern of
employee->super is the same as the bit pattern of *tmp, but they are not
the sam object, the reside at different places in memory.

You can only call free(ptr) when ptr stores the address returned by either
malloc or realloc. If you do free(&employee->super) you are passing a
completely different address to that one that malloc returned in Person_new,
this is undefined behaviour, you cannot do that.

There is no other way for you, you have to store the original pointer in the
struct, that's the reason why I added the Person *super_ref in the struct, so
that the original pointer is not lost.

  We have a major problem, this implementation is causing recursion at

I made small changes to your code, I don't have this problem:

#include <stdlib.h>
#include <stdio.h>
#include <stddef.h>

typedef struct Person Person;
typedef struct Person {
    Person *base; // reference to itself to retain reference to original function
    char* first;
    char* last;
    void (*display)(const Person *self);
    void (*destroy)(Person *self);
} Person;
Person* Person_new(char* first, char* last);

static void display(const Person *self) {
    printf("First: %s\n", self->first);
    printf("Last: %s\n", self->last);
}

static void destroy_person(Person *self)
{
    if(self == NULL)
        return;

    free(self);
}

Person* Person_new(char* first, char* last) {
    Person* self = malloc(sizeof(Person));
    self->first = first;
    self->last = last;
    self->display = display;
    self->destroy = destroy_person;

    self->base = self; // assigning reference to self to retain original functions in case they get overriden

    return self;
}

typedef struct Employee Employee;
struct Employee {
    Person super;
    char* company;
    int salary;
};
Person* Employee_new(char* first, char* last, char* company, int salary);

static void display2(const Person* self) {
    self->base->display(self->base);

    Employee *e = (Employee*) self;

    printf("Company: %s\n", e->company);
    printf("Salary: %d\n", e->salary);
}

static void destroy_employee(Person *self)
{
    if(self == NULL)
        return;

    if(self->base)
        self->base->destroy(self->base);

    free(self);
}

Person* Employee_new(char* first, char* last, char* company, int salary) {
    Employee* employee = malloc(sizeof(Employee));

    employee->super = *Person_new(first, last);
    employee->super.display = display2; // override
    employee->super.destroy = destroy_employee; // override

    employee->company = company;
    employee->salary = salary;

    return (Person*) employee;
}

int main(void)
{
    Person* person = Person_new("John", "Doe");
    Person* employee = Employee_new("Jane", "Doe", "Acme", 40000);
    person->display(person);
    puts("---");
    employee->display(employee);

    person->destroy(person);
    employee->destroy(employee);

}

It prints

$ ./b
First: John
Last: Doe
---
First: Jane
Last: Doe
Company: Acme
Salary: 40000

edit3

The fact that my little changes do not have the problem of endless recursion in
display2 kept me thinking about it and I've been running the code with the
debugger to see why I didn't have the problem as well. And now I understand why
my code does not have that problem, and your's shouldn't have that as well. In
fact, I realized that there is a way of freeing the memory correctly without
have a separate member like super_ref.

Let me explain:

In Employee_new you do:

employee->super = *Person_new(first, last);
employee->super.display = display2; // override

I told you that you would be leaking memory because you lose the original
pointer that malloc returns. And that is true, however I failed to realize
that you have in fact a pointer pointing to the original address that malloc
returns: the base member of the Person struct. In Person_new you do

self->base = self;

and this solves the problem with the overloading and the freeing, even though we
both didn't realize it at the time.

So again, when you do

employee->super = *Person_new(first, last);

you are dereferencing the pointer returned by malloc and making a copy of the
object into another object. But I failed to realize is that
employee->super->base points to the original location returned by malloc.
So when you do

employee->super.display = display2; // override

you are indeed setting a new value to employee->super.display, but because
employee->super is just a copy, the original does not change and with
employee->super->base you get the orginal object.

That's why

static void display2(const Person* self) {
    self->base->display(self->base);
    ...
}

shouldn't do end in a recursion, because self->display points to display2,
but self->base->display is still unchanged and points to display.

And now you can use the same behaviour for destroying the Employee object.
Let's say you have destroy function pointer in Person which points to the
destroy function:

void destroy_person(Person *person)
{
    if(person == NULL)
        return NULL;

    free(person);
}

and in Person_new you add:

self->destroy = destroy_person;

Now you can destroy a person object by doing person->destroy(person);.

The destroy for Employee will look similar to display2:

void destroy_employee(Person *self)
{
    if(self == NULL)
        return;

    if(self->base)
        self->base->destroy(self->base);

    free(self);
}

because self->base is still pointing to the original object,
self->base->destroy points to destroy_person. Now in
Employee_new you have to add

employee->super.display = display2; // override
employee->super.destroy = destroy_employee; // override

and in main you just do person->destroy(person) and
employee->destroy(employee);. I've checked the code above (I've updated with
the destroy functions) with valgrind and it told me that everything was freed
correctly.


Prime number finder in bash I need to learn bash at some point in the near future, so I decided to start playing around with it. This is really the first actual script that I've written and would like feedback.

This finds and prints all the prime numbers up to the argument provided to the script:

./primes.sh 100
2
3
5
7
11
13
17
19
23
29
31
37
41
43
47
53
59
61
67
71
73
79
83
89
97

Honestly, this is proving to be a challenging language with very finicky syntax and a lot of ways to do things half-incorrectly. This code works, but I'm sure it's far from optimal. I'd like any and all recommendations.

I intentionally included the mostly-unnecessary hasFactor function just to force use of functions. I wanted to see what complications it caused. The main issue with it is needing to return a number representing a boolean that indicates if the number has a factor or not, and cleanly handling that returned exit code at the "call-site". Needing to check against 1 seems poor. Any suggestions regarding use of functions would be appreciated.

And I know better algorithms exist, but I wanted to keep it simple.

#!/usr/bin/env bash

maxNumber=$1

# Returns if a number has factors other than 1 and itself
# Params: n - number to check
function hasFactor {
    n=$1
    for ((factor=2; factor<n; factor++)); do
        if [ $((n % factor)) -eq 0 ]; then
            true
            return
        fi
    done

    false
    return
}

for ((n=2; n<maxNumber; n++)); do
    hasFactor $n
    if [ $? = 1 ]; then
        echo $n
    fi
done

Firstly, the algorithm is poor.  Repeated trial division is one of the worst ways to generate primes.  Prefer a sieve of some sort (you can use an array to store the primes).

Instead of true; return we can return 0; similarly, return 1 can replace false; return.

Testing $? is usually a sign that you simply need to move the tested command:

if ! hasFactor $n
then
    echo $n
fi

Or simply:

hasFactor $n || echo $n


Concatenate two strings I am learning C and wrote a function to concatenate two strings in C. Please let help me improve it.

#include <stdio.h>
#include <string.h>
void concat (char result[], const char str1[], int n1, const char str2[], int n2)
{
    int i, j;
    // copy str1 to result
    for ( i = 0; i < n1; ++i )
        result[i] = str1[i];

    // copy str2 to result
    for ( j = 0; j < n2; ++j )
        result[n1 + j] = str2[j];
}
int main (void)
{

    const char s1[5] = { 'T', 'e', 's', 't', ' '};
    const char s2[6] = { 'w', 'o', 'r', 'k', 's', '.' };
    char s3[11];

    int i;
    concat (s3, s1, 5, s2, 6);

    for ( i = 0; i < 11; ++i )
        printf ("%c", s3[i]);

    printf ("\n");

    return 0;
}

This looks pretty good!  A few notes (some stuff you may still have to learn about since you are a beginner, such as pointers)

Your method isn't what I would consider "normal".  To see what I
mean, take a look at the function prototype of the standard library
function strcat():

char * strcat ( char * destination, const char * source );

As you can see, the standard library only takes in two parameters,
not 5.  This makes sense too: as the caller I want to throw in two strings and only get one returned back to me.  For a string concatenation function, I shouldn't have to manually enter in the length of the strings I want to concatenate, the function should be able to handle that.
So how do we find the length of a string exactly?  Well, we can iterate through a string until we find the \0 character (termination character).

while(*dest != '\0') /* find the end of the string */
{
     dest++;
}

All this loop does it start at the beginning of the dest string and find the end of it, very simple.  The next loop is a bit more complicated:

while((*dest++ = *source++) != '\0'); /* copy source */

This code iterates through the source string, and while it iterates through it, it appends the contents to dest (which we are doing by iterating dest at the same time as source).  We do this until we reach the \0 character.
We should be returning a value from the function.  This is more for a convenience to the caller.  This allows me to do stuff such as

puts(concat(strdup("test"), "this out"));

Rather than this longer piece:

char* str = strdup("test");
concat(str, "test");
puts(str);

strcat can be dangerous because if the string to be appended is too long to fit in the destination buffer, it will overwrite adjacent memory, invoking undefined behavior. Usually the program will simply cause a segmentation fault when this occurs, but a skilled attacker can use such a buffer overflow to break into a system.

To prevent buffer overflows, several alternatives for strcat() have been used. All of them take an extra argument which encodes the length of the destination buffer and will not write past that buffer end. All of them can still result in buffer overflows if an incorrect length is provided.

char* strncat(char *dst, const char *src, size_t n);

This will require a bit of modification to the copying for checking n, but not much.

Final Implementation (assuming no standard library functions can be used):

char* concat_s(char *dest, const char *src, size_t n)
{
    if (n != 0) // if n == 0 then we aren't concatenating anything...
    {
        while (*dest != 0)
        {
            dest++;
        }
        do
        {
            if (0 == (*dest = *src++)) break;
            dest++;
        } while (--n != 0);
        *dest = 0;
    }
    return dest;
}

Final Implementation (assuming all functions can be used except strncat()):

char* concat_s(char *dest, const char *src, size_t n)
{
    return memcpy(dest + strlen(dest), src, n);
}


check_prime.cpp (checks if input is a prime number) check_prime.cpp

Description: Accepts user input to check if the user entered a prime number.

Notes: If any text inputted after an integer will be ignored.

For example: 1234.5678 and 98abc will be interpreted as 1234 and 98, respectively.

Please provide any feedback (positive or negative).

// check_prime.cpp
// Author: Chris Heath
// Date: 12/9/2018
// Description: Accepts user input to check if the user entered a prime number.
// Notes: If any text inputted after an integer will be ignored.
// For example: 1234.5678 and 98abc will be interpreted as 1234 and 98, respectively.

#include <iostream>
#include <string>
#include <limits>

using namespace std;

int is_prime (unsigned long int num)
{
    unsigned long int n;
    // avoid loop;  0 or 1 are never prime
    if (num == 0 || num == 1)
        return 0;
    // loop through numbers 0..(n/2)+1, trying to
    // divide one into the other with no remainder.
    for (n=2; n < (num/2)+1; n++)
    {
        // if we had no remainder during a revision,
        // input number has a divisor... NOT PRIME!
        if ((num % n) == 0)
          return 0;
    }
    // made it through gauntlet...prime!
    return 1;
}

int main()
{
 unsigned long int i;
 cout<<"Enter an integer to check if it is a prime: ";
 cin>>i;
 while(1)
 {
  if(cin.fail())
  {
   cin.clear();
   cin.ignore(numeric_limits<streamsize>::max(),'\n');
   cout<<"You have entered wrong input.\nPlease try again: ";
   cin>>i;
  }
  if(!cin.fail())
  break;
 }
 if ( is_prime(i) )
  cout<<"\n"<<i<<" IS PRIME!"<<endl;
 else
  cout<<"\n"<<i<<" is NOT prime."<<endl;
return 0;
}

Don't use using namespace std;.

Since is_prime only has two possible return values, and given its name implies a binary result, it should return a bool, not an int.  Then replace the return statements with the proper bool values.

When checking for primes, you only need to go up to the square root of the number to check.  This value can be computed once then used in the loop condition, or you can check for i * i <= num, although this has the potential to overflow with sufficiently large num.

The check for 0 or 1 can be simplified to if (num < 2).

Before the loop, you can check num to see if it is divisible by 2 and return an appropriate value.  Then you loop can start at 3 and increment by 2 (so you only need to check the odd numbers).

You should declare your variables as close to the place you first use them as possible, so your loop could be

for (unsigned long n = 3; ...

(or for (auto n = 3UL;).

When getting input, rather than while (1) with repeated conditions in the loop, you can say while (!cin.fail()) and not check for that within the loop.


Spiral a matrix of size NxN filled with integers from 1 up to and including n^2 I have written a solution in Clojure for the following question:

  When starting from the number 1 and adding three numbers on each row
  a 3x3 matrix is formed as follows:

1 2 3
4 5 6
7 8 9

  Starting from the upper left and spiraling inwards in a clockwise
  direction the following series is obtained: 1 2 3 6 9 8 7 4 5.

  Create a method spiral that finds the spiraling series for a \$n\times n\$ matrix.

I created a long and short solution. They are inspired by the following Stack Overflow answer: ruby - Looping in a spiral outside-in

Version with descriptive names:

(ns spiral.core
  (:require [clojure.core.matrix :as m]))

(defn create-numbers-matrix
  "creates a size x size matrix of numbers from 1 up to and including size."
  [size]
  (let [numbers (range 1 (inc (* size size)))
        partitions (partition size numbers)
        matrix (m/matrix partitions)]
    matrix))

(defn get-first-row [m]
  "gets the first row (index 0) of matrix m."
  (m/get-row m 0))

(defn delete-first-row [m]
  "returns matrix with the first row of the matrix m removed."
  (drop 1 (m/rows m)))

(defn transpose [m]
  "transposes matrix m."
  (m/transpose m))

(defn reverse-rows [m]
  "returns matrix m with rows reversed."
  (m/matrix (reverse m)))

(defn rotate-left [m]
  "rotates the matrix m 90 degrees to the left."
  (-> m
      transpose
      reverse-rows))

(defn rotate-and-chop [m]
  "repeatedly rotates matrix m 90 degrees and cuts off the
   top till matrix is empty."
  (if (empty? m) nil
      (conj (rotate-and-chop (-> m
                                 delete-first-row
                                 rotate-left))
            (get-first-row m))))

(defn spiral [size]
  "creates list of values in spiral from grid of size size"
  (let [numbers-matrix
        (create-numbers-matrix size)]
    (flatten (rotate-and-chop numbers-matrix))))

Short version:

(ns spiral.core
  (:require [clojure.core.matrix :as m]))

(defn spiral-one-liner [size]
  (flatten
   ((fn loop [m]
      (if (empty? m) nil
          (conj (loop (m/matrix (reverse (m/transpose (drop 1 (m/rows m))))))
                (m/get-row m 0))))
    (m/matrix (partition size (range 1 (inc (* size size))))))))

The short version is still longer than the Ruby version from the answer (because tail recursions errors appear the loop and recur syntax can, I believe, not be used).

I would be grateful if someone can take a look and provide some suggestions.

First off, the docstring for a function always needs to come before the argument vector. You did this right in create-numbers-matrix, but in the rest of your functions, your docstring will be treated as part of the function body, evaluated, and ignored.

(require '[clojure.repl :refer [doc]])

(doc rotate-and-chop)
;; -------------------------
;; spiral.core/rotate-and-chop
;; ([m])
;;   nil
;=> nil

Some Clojure lint tools like Eastwood will catch this sort of error. Also, as you can see, the doc function indents the first line of the docstring by two spaces, so it's conventional to indent each line of the docstring after the first by two spaces to make everything line up:

(defn rotate-and-chop
  "repeatedly rotates matrix m 90 degrees and cuts off the
  top till matrix is empty."
  [m]
  (if (empty? m) nil
      (conj (rotate-and-chop (-> m
                                 delete-first-row
                                 rotate-left))
            (get-first-row m))))

(doc rotate-and-chop)
;; -------------------------
;; user/rotate-and-chop
;; ([m])
;;   repeatedly rotates matrix m 90 degrees and cuts off the
;;   top till matrix is empty.
;=> nil

For create-numbers-matrix, your current approach with let works fine. You could also avoid having to name each of the intermediate steps by using ->>:

(defn create-numbers-matrix
  "creates a size x size matrix of numbers from 1 up to and including size."
  [size]
  (->> (range 1 (inc (* size size)))
       (partition size)
       m/matrix))

Your get-first-row, delete-first-row, transpose, and reverse-rows functions are equivalent to the already available first, rest, transpose, and reverse functions, respectively, so I would just remove them; they add no value to your code.

It's conventional to indent the "then" branch (and the "else" branch, if present) of an if or when expression by two spaces. So in rotate-and-chop, since your first branch is nil, it would be more idiomatic to write it as (when-not (empty? m) ,,,) or just (when (seq m) ,,,). Also, it might be more readable to put the call to rotate-and-chop into the threading macro:

(defn rotate-and-chop
  "repeatedly rotates matrix m 90 degrees and cuts off the
  top till matrix is empty."
  [m]
  (when (seq m)
    (conj (-> m rest rotate-left rotate-and-chop)
          (first m))))

In spiral, you use flatten to concatenate the elements of the sequence returned by rotate-and-chop into a single sequence. While this does work in your case, I would generally advise against using flatten, because it can lead to unexpected behavior when you don't know what type of data is in the sequences you're concatenating. Usually you just need to flatten the outermost layer, in which case you can simply apply the concat function:

(defn spiral
  "creates list of values in spiral from grid of size size"
  [size]
  (let [numbers-matrix (create-numbers-matrix size)]
    (apply concat (rotate-and-chop numbers-matrix))))

As you noted in your question, though, this is still considerably longer than the Ruby solution. However, it's quite possible to write a short, idiomatic Clojure solution to this problem:

(defn spiral [m]
  (when (seq m) (concat (first m) (-> m rest m/transpose reverse spiral))))

This is basically a direct translation of the Ruby solution into Clojure, except without that ugly modification of the original matrix. When m is empty, I return an empty result (it's common to treat nil as an empty sequence in Clojure). Otherwise, I return the concatenation of the first row of m with the spiral sequence of the reversal of the transposition of the rest of m. The reason I have to call rest is that, in the original Ruby code, shift causes the side effect of removing the first row from the matrix, which I don't want to do here.

You could test this out with a function that generates an example matrix, similar to your create-numbers-matrix function:

(defn matrix [n]
  (partition n (range 1 (inc (* n n)))))

(spiral (matrix 3))
;=> (1 2 3 6 9 8 7 4 5)


How can I avoid repeated code using lambdas? I have a method which should accept a Function/Action and do something with that method (in my case I've added retry logic).

I'm wondering how I can remove the duplicated code (for loop, etc) from the last 2 methods. Is there any clever way to do this in C#?

public static class Retry {
    public static void Do(
    Type retryOnExceptionType,
    Action action,
    TimeSpan retryInterval =
    default (TimeSpan),
    int retryCount = 3) {
        if (retryInterval ==
        default (TimeSpan)) retryInterval = TimeSpan.FromSeconds(1);

        Do < object > (
        retryOnExceptionType, () = > {
            action();
            return null;
        }, retryInterval, retryCount);
    }

    public static T1 Do < T1, T2, T3 > (
    Type retryOnExceptionType,
    Func < T2, T3, T1 > action,
    T2 param1,
    T3 param2,
    TimeSpan retryInterval =
    default (TimeSpan),
    int retryCount = 3) {
        if (retryInterval ==
        default (TimeSpan)) retryInterval = TimeSpan.FromSeconds(1);

        var exceptions = new List < Exception > ();

        for (int retry = 0; retry < retryCount; retry++) {
            try {
                return action(param1, param2);
            } catch (Exception ex) {
                if (!retryOnExceptionType.IsInstanceOfType(ex)) throw;

                exceptions.Add(ex);
                Thread.Sleep(retryInterval);
            }
        }

        throw new AggregateException(exceptions);
    }

    public static T Do < T > (
    Type retryOnExceptionType,
    Func < T > action,
    TimeSpan retryInterval =
    default (TimeSpan),
    int retryCount = 3) {
        if (retryInterval ==
        default (TimeSpan)) retryInterval = TimeSpan.FromSeconds(1);

        var exceptions = new List < Exception > ();

        for (int retry = 0; retry < retryCount; retry++) {
            try {
                return action();
            } catch (Exception ex) {
                if (!retryOnExceptionType.IsInstanceOfType(ex)) throw;

                exceptions.Add(ex);
                Thread.Sleep(retryInterval);
            }
        }

        throw new AggregateException(exceptions);
    }
}

You should be able to wrap up your loops in a single method that takes a Func<T> and then do something like this:

public static T1 Do<T1,T2,T3>(Type retryOnExceptionType, Func<T2,T3,T1> action, T2 param1,
                              T3 param2, TimeSpan retryInterval, int retryCount)
{
    var del = new Func<T1>(() => action(param1, param2));
    return DoLoops<T1>(del, retryOnExceptionType, retryInterval, retryCount);
}

public static T Do<T>(Type retryOnExceptionType, Func<T> action,TimeSpan retryInterval,
                      int retryCount)
{
    return DoLoops<T>(action, retryOnExceptionType, retryInterval, retryCount);
}

private static T DoLoops<T>(Func<T> func, Type retryOnExceptionType,
                            TimeSpan retryInterval, int retryCount)
{
    if (retryInterval == default(TimeSpan))
        retryInterval = TimeSpan.FromSeconds(1);

    var exceptions = new List<Exception>();

    for (int retry = 0; retry < retryCount; retry++)
    {
        try
        {
            return func();
        }
        catch (Exception ex)
        {
            if (!retryOnExceptionType.IsInstanceOfType(ex))
                throw;

            exceptions.Add(ex);
            Thread.Sleep(retryInterval);
        }
    }

    throw new AggregateException(exceptions);
}

The key is in wrapping up your Func<T2, T3, T1> call in a separate Func<T1>, which allows you to pass a parameterless delegate to DoLoops regardless of how many type parameters the original delegate has, since that's the only difference between the original methods.

Even simpler, you can do away with DoLoops and just keep the for loop in the public static T Do<T> overload and Do<T1, T2, T3> can just pass the parameterless delegate it creates into that method:

public static T1 Do<T1,T2,T3>(Type retryOnExceptionType, Func<T2,T3,T1> action, T2 param1,
                              T3 param2, TimeSpan retryInterval, int retryCount)
{
    return Do<T1>(retryOnExceptionType, () => action(param1, param2),
                  retryInterval, retryCount);
}

public static T Do<T>(Type retryOnExceptionType, Func<T> action, TimeSpan retryInterval,
                      int retryCount)
{
    if (retryInterval == default(TimeSpan))
        retryInterval = TimeSpan.FromSeconds(1);

    var exceptions = new List<Exception>();

    for (int retry = 0; retry < retryCount; retry++)
    {
        try
        {
            return action();
        }
        catch (Exception ex)
        {
            if (!retryOnExceptionType.IsInstanceOfType(ex))
                throw;

            exceptions.Add(ex);
            Thread.Sleep(retryInterval);
        }
    }

    throw new AggregateException(exceptions);
}


Generate all possible permutations from a string with character repetitions I want to know if my code is good, with reasonable efficiency, and if it is possible to improve my code and how. What would you suggest?

import java.util.HashMap;
import java.util.HashSet;
import java.util.Set;

public class FindAllPermutationsWithRepetitions {

    public static void main(String[] args) {
        String str = "ferarri";
        Set<String> permutations = getPermutations(str);

        int i = 1;
        for(String s : permutations){
            System.out.println(i++ + ": " + s);
        }

        /* check correctness */
        System.out.println("\nThere must be " +  numOfArrangements(str) + " values");

    }

    public static Set<String> getPermutations(String remained){
        if(remained.length() <= 1){
            Set<String> set = new HashSet<String>();
            set.add(remained);
            return set;
        }

        char c = remained.charAt(0);
        Set<String> old_permutations = getPermutations(remained.substring(1));
        Set<String> new_permutations = new HashSet<String>();

        for(String old_s : old_permutations){
            for(int j = 0; j < old_s.length(); j++){
                new_permutations.add(old_s.substring(0, j) + c + old_s.substring(j));
            }
            new_permutations.add(old_s + c);
        }

        return new_permutations;
    }

    public static int factorial(int num){
        int result = 1;

        for (int i = 1; i <= num; i++) {
            result = result * i;
        }

        return result;
    }

    public static int numOfArrangements(String str){
        HashMap<Character, Integer> hm = new HashMap();

        for(char key : str.toCharArray()){
            if(hm.containsKey(key)) hm.put(key, hm.get(key) + 1);
            else hm.put(key, 1);
        }

        int divisor = 1;
        for(Integer value : hm.values()){
            divisor *= factorial(value);
        }

        return factorial(str.length()) / divisor;
    }
}

My solution below uses a custom radix counter and then maps the value to each character in a given String. I have used BigInteger as you will hit problems with ints once your String gets to about 10 characters long, and the same problem with longs once your String gets to around 16 characters.

I have wrapped it all in one class so you can see it in action, but given the Counter class is completely independent, you should have that in it's own file.

package permutationgenerator;

import java.math.BigInteger;
import java.util.Arrays;
import java.util.Iterator;

public class StringPermutationGenerator implements Iterable<String> {
    private final String word;

    public StringPermutationGenerator(String word) {
        this.word = word;
    }

    @Override
    public Iterator<String> iterator() {
        return new StringIterator(word);
    }

    private class StringIterator implements Iterator<String> {
        private final char[] alphabet;
        private final Counter counter;
        private final BigInteger max;
        private BigInteger count;

        private StringIterator(String word) {
            alphabet = word.toCharArray();
            counter = new Counter(alphabet.length, alphabet.length);
            max = BigInteger.valueOf(alphabet.length).pow(alphabet.length);
            count = BigInteger.ZERO;
        }

        @Override
        public boolean hasNext() {
            return count.compareTo(max) < 0;
        }

        @Override
        public String next() {
            StringBuilder sb = new StringBuilder();
            for (int i : counter.getCounter()) {
                sb.append(alphabet[i]);
            }
            counter.increment();
            count = count.add(BigInteger.ONE);
            return sb.toString();
        }

    }

    private class Counter {
        private final int radix;
        private final int length;
        private final int[] counter;

        public Counter(int radix, int length) {
            this.radix = radix;
            this.length = length;
            counter = new int[length];
        }

        public int[] getCounter() {
            return counter;
        }

        public void increment() {
            for (int index = length - 1; ; index--) {
                if (index == -1) {
                    resetCounter();
                    return;
                }
                if (counter[index] < radix - 1) {
                    counter[index]++;
                    return;
                }
                counter[index] = 0;
            }
        }

        private void resetCounter() {
            Arrays.fill(counter, 0);
        }
    }
}

Usage is as such:

for (String s : new StringPermutationGenerator("ferrari")) {
    System.out.println(s);
}


State machine implementation I have written a simple state machine. Comments and feedback are welcome.

#include <stdio.h>
#include <stdlib.h>

typedef enum Event
{
    eEVENT_X,
    eEVENT_Y,
    eEVENT_Z,
}Event;

typedef enum States
{
    eSTATE_X,
    eSTATE_Y,
    eSTATE_Z
}States;

int e = eEVENT_X;
int s = eSTATE_X;

struct state_transition
{
  Event EventId;
  States StateId;
  void (*function)(void); //call back utility
};

#define NSTATES 3
#define NEVENTS 3
#define STATE_END 3
#define EVENT_END 3

void function_eXsX(void)
{
    printf("\n xx \n");
    e = eEVENT_X;
    s = eSTATE_Y;

}
void function_eXsY(void)
{
    printf("\n xy \n");
    e = eEVENT_X;
    s = eSTATE_Z;
}

void function_eXsZ(void)
{
    printf("\n xz \n");
    e = eEVENT_Y;
    s = eSTATE_X;
}
void function_eYsX(void)
{
    printf("\n yx \n");
    e = eEVENT_Y;
    s = eSTATE_Y;
}
void function_eYsY(void)
{
    printf("\n yy \n");
    e = eEVENT_Y;
    s = eSTATE_Z;
}
void function_eYsZ(void)
{
    printf("\n yz \n");
    e = eEVENT_Z;
    s = eSTATE_X;
}
void function_eZsX(void)
{
    printf("\n zx \n");
    e = eEVENT_Z;
    s = eSTATE_Y;
}
void function_eZsY(void)
{
    printf("\n zy \n");
    e = eEVENT_Z;
    s = eSTATE_Z;
}
void function_eZsZ(void)
{
    printf("\n zz \n");
    e = EVENT_END;
    s = STATE_END;

}

struct state_transition stateTrans[NEVENTS][NSTATES] =
{
      {{eEVENT_X,eSTATE_X,function_eXsX},{eEVENT_X,eSTATE_Y,function_eXsY},{eEVENT_X,eSTATE_Z,function_eXsZ}},
      {{eEVENT_Y,eSTATE_X,function_eYsX},{eEVENT_Y,eSTATE_Y,function_eYsY},{eEVENT_Y,eSTATE_Z,function_eYsZ}},
      {{eEVENT_Z,eSTATE_X,function_eZsX},{eEVENT_Z,eSTATE_Y,function_eZsY},{eEVENT_Z,eSTATE_Z,function_eZsZ}},
};

int main(int argc,char *argv[])
{

    while(1)
    {
        stateTrans[e][s].function();
        if(e == STATE_END && s == EVENT_END)
            break;
    }
    return 0;
}

I see some things that may help you improve your code.

Avoid global variables

While you might only currently have a need for a single instance of your state machine, a better approach would eliminate the global variables.  Instead, it would make more sense to me to have each function return the new (end) state, or even better, have the state_transition contain the transitions implicitly.  Which you choose will depend on details of the real state machine.  For example, if it's a state machine for a communications protocol, the event might be "received a message header" and the next state may depend on whether the message header is valid or not.

Make your state table complete

The STATE_END and EVENT_END are not represented within the state table.  The problem with that is that anyone reading the code might easily overlook those two possibilities because they're not in the state table.  On the other hand, the EventId and StateId members are never used and should be omitted.

Use the appropriate types

The s and e variables are both declared int but they're really not.  They should instead be States and Event types.

Chose better names

The event enum is called Event (singular) but the state enum is called States (plural) which is confusing.  I'd recommend using the singular form for both names.

Simplify your typedefs

In this case, there is no need to repeat the name Event or the name State in the typedef.  You can make it an anonymous enum with a typedef:

typedef enum
{
    eEVENT_X,
    eEVENT_Y,
    eEVENT_Z,
    NEVENTS
} Event;

Note that this construction makes this line unnecessary:

#define NEVENTS 3

That makes it a little easier to maintain the tables.

Implement range checking

What happens if some energetic gamma ray flips a bit in your embedded device's RAM and the value for the state goes from 1 to 129?  The answer is that the code will use whatever arbitrary code is that far off the end of the state table.  It's prudent instead to have a range checking function wrapper so that bad values (which might more plausibly be a result of a program error somewhere) will not cause the system to crash.  Here's a range checking function that could be used:

bool applyEvent(State *s, Event *e)
{
    if (*s >= NSTATES || *e >= NEVENTS)
        return false;
    *s = stateTrans[*e][*s].function(e);
    return true;
}

Note that it also assumes that each function is of this form:

State function_eZsY(Event *e)
{
    printf("\n zy \n");
    *e = eEVENT_Z;
    return eSTATE_Z;
}

Using this, there are no longer any global variables and the main function can look like this:

int main()
{
    State s = eSTATE_X;
    Event e = eEVENT_X;
    while(applyEvent(&s, &e))
    {
    }
}

Which brings us to the last point:

Eliminate return 0 at the end of main

Since C99, the compiler automatically generates the code corresponding to return 0 at the end of main so there is no need to explicitly write it.


Optimizing my solution for problem #23 - project Euler The question -

  A perfect number is a number for which the sum of its proper divisors
  is exactly equal to the number. For example, the sum of the proper
  divisors of 28 would be 1 + 2 + 4 + 7 + 14 = 28, which means that 28
  is a perfect number.

  A number n is called deficient if the sum of its proper divisors is
  less than n and it is called abundant if this sum exceeds n.

  As 12 is the smallest abundant number, 1 + 2 + 3 + 4 + 6 = 16, the
  smallest number that can be written as the sum of two abundant numbers
  is 24. By mathematical analysis, it can be shown that all integers
  greater than 28123 can be written as the sum of two abundant numbers.
  However, this upper limit cannot be reduced any further by analysis
  even though it is known that the greatest number that cannot be
  expressed as the sum of two abundant numbers is less than this limit.

  Find the sum of all the positive integers which cannot be written as
  the sum of two abundant numbers.

My solution -

public class Problem23 {

    /**
     * A brute force solution -
     * 1. Calculate the abundant numbers till the limit.
     * 2. Check each number if it can be written as a sum of any two
     *    abundant numbers in the pre-calculated set
     *
     * @param args
     */
    public static void main(String[] args) {

        Set<Integer> abundantNumSet = new LinkedHashSet<Integer>(); //sorted set of abundant numbers

        // find all abundant numbers from 13 to 28123, since 12 is the smallest
        // abundant number and greatest number that cannot be expressed as the
        // sum of two abundant numbers is less than 28123
        abundantNumSet.add(12);
        for (int i = 13; i < 28124; ++i) {
            if (isAbundant(i)) {
                abundantNumSet.add(i);
            }
        }

        int total = 276; // sigma 23, since 24 = 12 * 2
        for (int i = 25; i < 28124; ++i) {
            boolean isRep = false;

            for (Integer num : abundantNumSet) {
                // iterating through half of the numbers in the set is enough
                if (abundantNumSet.contains(i - num)) {
                    // the number can be represented as a sum of two
                    isRep = true;
                    break;
                }
                if (i < num) {
                    // the abundant number should less than a given number to
                    // be part of the numbers forming the sum
                    break;
                }
            }

            if (!isRep) {
                // the number cannot be represented as sum of two abundant
                // numbers
                total += i;
            }
        }

        System.out.println(total);
    }

    public static boolean isAbundant(int n) {
        int sum = 1;
        int end = (int) Math.sqrt(n);

        for (int i = 2; i <= end; ++i) {
            if (n % i == 0) {
                int quotient = n / i;

                if (quotient != i) {
                    sum += i + quotient; // add the factors
                } else {
                    // avoid adding twice - factor in case of perfect squares
                    sum += quotient;
                }
            }
        }

        if (sum > n) {
            return true;
        }

        return false;
    }
}

I would like to optimize my solution.
Some pointers at the same would be appreciated.

I would like to optimize my solution.

Really? It's already pretty fast and there isn't much to gain, given that Java has some start up overhead. I'd save my power for later Eulers.
Set<Integer> abundantNumSet = new LinkedHashSet<Integer>();

Do you need "Linked"? I don't think so, so we've a tiny optimization.
    abundantNumSet.add(12);
    for (int i = 13; i < 28124; ++i) {
        if (isAbundant(i)) {
            abundantNumSet.add(i);
        }
    }

I really wouldn't do it. You're testing nearly 30k numbers, so is it worth to optimize the first 13? Moreover, the saved tests are the fastest.
You should use no magic constants like 12, 13, 28124. Define constants and give them proper names. This will help you to avoid unworthy optimizations. :D
    int total = 276; // sigma 23, since 24 = 12 * 2
    for (int i = 25; i < 28124; ++i) {

Another unworthy optimization, more magic numbers. What's worse, a repeated magic number. One day, in a big project you save a second by copying some numbers instead of defining constants. Another day, you change them all (it's easy, isn't it?). Yet another day, you'll be hunting a bug caused by having changed a completely unrelated number which by accident had the same value.
        boolean isRep = false;

The loop body is big enough for an own method. Then you can drop the variable, instead of spending time on naming it properly (isRep is not properly).
            // iterating through half of the numbers in the set is enough

This is a promise, you didn't keep. No problem, as this part takes much less time than the other, so speeding it twice helps nothing.
I'd go for a method like this (untested):
private boolean isSumOfTwoAbundants(int n) {
    for (int i : abundantNumSet) {
         if (i > n - i) {
             break;
         }
         if (abundantNumSet.contains(n - i)) {
             return true;
         }
    }
    return false;
}

When using things like this
    int end = (int) Math.sqrt(n);

be aware of how it get rounded. I always prefer Guava's
   IntMath.sqrt(int, RoundingMode)

as it's explicit about it.
    if (sum > n) {
        return true;
    }

    return false;

Do you really mean return sum > n?
I guess, isAbundant could be sped up by factorization:

Similarly to the Sieve of Eratosthenes, you can precompute one factor for all numbers up to 28124.
For every number n, you get one factor, which you can divide out and get another number and another factor etc., till you have the factorization of n.
Having all prime factors (with their exponents), you can trivially compute the number of divisors.

This would probably help a lot, however, your code is already more then fast enough. While I love optimizing, I did about nothing for this problem and it took one second only (yours is about 10 times faster, do I care?).
Summary
You can optimize, but now it's time to learn not to optimize. Otherwise you can get quickly lost in the root of all evil.


Securing access to PHP services from Javascript Ajax calls I am creating a generic PHP handler, or "dispatcher", for Ajax calls made from Javascript running on the page served by PHP. The handler will take posted data and, depending on a value, decide how to route the call further. WordPress is being used, but my question is not specific to WordPress (if it was I would have asked on the WordPress sub-site).

The PHP Ajax handler should be secured, not serving calls from anywhere other than the page that the visitor is viewing in the browser (assuming that Apache is not configured to restrict access to the handler). I have designed a security structure with this aim in mind, and would like to know if my approach is good or could be improved. Are there any weaknesses that could be exploited?

The design is quite simple. When the page is constructed for serving, a session cookie is created, and its value is an encrypted token, derived from the client's IP, and a random salt. The cookie's value, the encrypted token, is also used as the name of a temporary db record for 60 minutes, and the record's value is the salt that was used to generate the token.

$ajaxSalt = bin2hex(openssl_random_pseudo_bytes(30)); // Create random salt
$cookieAndTrans = md5(crypt($_SERVER['REMOTE_ADDR'], $ajaxSalt)); // We use cookie value as token
setCookie('qnrwp_ajax_cookie', $cookieAndTrans); // Set session cookie, for JS Ajax caller to echo back to us
set_transient('qnrwp_ajax_temp_salt_'.$cookieAndTrans, $ajaxSalt, 60 * MINUTE_IN_SECONDS); // Save salt for 60 mins

When the page is served and the client makes an Ajax request, the Javascript code will, as part of the data payload, send the value of the cookie to the handler.

Receiving the call, the handler will check that the transmitted cookie value matches the cookie value that was set initially. It then uses this value as the name of the temporary db record to call up (if 60 minutes haven't passed), and uses the value of the record, the salt, to confirm that the IP matches.

// ----------------------- Security check
if ($_POST['qnrwp_ajax_cookie'] !== $_COOKIE['qnrwp_ajax_cookie']) wp_die();
$ajaxTrans = get_transient('qnrwp_ajax_temp_salt_'.$_POST['qnrwp_ajax_cookie']);
if (!$ajaxTrans) wp_die();
if ($_POST['qnrwp_ajax_cookie'] !== md5(crypt($_SERVER['REMOTE_ADDR'], $ajaxTrans))) wp_die();

A couple of things to clarify to avoid confusion: md5() is primarily used as replacement for bin2hex(), its weak crypto security just an added bonus, but not being relied upon - crypt() is used for good encryption. crypt() is able to generate a salt, but I prefer creating my own as I'll store it in the temporary record (set/get_transient() in the code). The salt is quite long, helping avoid clashes between different clients accessing the site at the same time. A clash is still possible, but I think unlikely, and even if it happened, it would not be catastrophic - if the clashing clients share their IP, one of them may find up to 120 minutes available after page load rather than 60, no big deal.

I believe my question is at quite an advanced level and would prefer if those with good knowledge of the subject write answers rather than try to engage in discussion in the comments. That said, if anything is unclear, feel free to ask in the comments. If you see weaknesses in the code, please try to provide concrete suggestions for improvement, rather than merely pointing out the loopholes. The most constructive answer will be accepted. I will wait a few days before accepting.

Please note I'm not open to suggestions of third-party tools or WordPress plugins - this is a case of "rolling my own".

I consider myself a security nazi, and you're asking security-related questions, so I'm going to do something I don't normally do and be pretty blunt here.  Please don't take it personally.

You've completely missed the mark on security.  Not to say that your solution is "insecure" (you haven't given us enough information to really decide that), but simply because, from a security perspective, most of what you are doing just doesn't make sense.  Your code more-or-less looks like you threw together as many security-related functions as you could and called it secure.  Granted, these things aren't making things less-secure, but it also isn't making you more-secure.  I'm a firm believer that code that doesn't actually do anything should be thrown away.  That is the case with most of your code here.

I think the problem you have is that you've missed out on the big picture: what is your code actually trying to accomplish?  The answer, as near as I can tell, is to make sure that only the original viewer of a page can access the API calls related to that page.  In other words, you are trying to identify users via a secret.  In that context, here's the thing:

Don't use the remote address.  The client's IP address can change for any number of reasons, completely breaking your system.  This can happen as easily as someone opening your website on their phone with their home WIFI connection, and then stepping outside and switching to internet via their mobile carrier.  When that happens their IP changes and suddenly your javascript refuses all subsequent requests.  A system that breaks when the user moves 50 feet is definitely a problem.  Moreover, this is not a perfect security step anyway.  It is quite common for multiple users to share the same IP address, sometimes in contexts in which session stealing is most relevant (i.e. public wifi).  This means that identifying users by IP Address causes UI problems, and is also not secure.  Lose/Lose.
bin2hex is not the best choice.  If you want to encode random bytes as a string (which is perfectly normal: strings, especially ASCII, move back and forth from server-to-client easily), then don't use bin2hex.  For a given number of bytes, a hexadecimal string will be longer than an equivalent ASCII string.  Instead store your random string as an ASCII string using only numbers and letters.  You'll get shorter strings with the same amount of entropy.  Granted, this is really just a nitpick: this won't impact your security either way.  In essence, you are trying to generate a random string, and are doing that by taking random bytes and converting it to hex.  It's better to just generate a random string, IMO.
Neither crypt nor md5 are doing anything for you at all.  Again, focus on the big-picture goal here.  You are (as near as I can tell) trying to generate a random string to act as a "secret" to later identify users.  Encrypting things doesn't make them more random, and neither does hashing.  It's especially pointless to hash an encrypted string.  Encryption preserves data, while hashing destroys it.  The fact that you would follow up one with the other suggests that you don't really understand what you are doing.
Checking for the value both in the cookie and post data doesn't make your app more secure: it just gives you more code to maintain on both the front-end and back-end.  Moreover, it violates Kerckhoff's principle, which states that the only thing that should be a secret in any secure system is the actual secret (in this case, your key).  You're effectively trying to rely on an extra security step to keep out attackers, but that security step is already public (because it is present in your front-end javascript code).  As a result, it is pointless.  Just let the value come up in the cookie that you already set.

So what should you do?  I would suggest one of two things:

Best bet: authenticate users in this API call in exactly the same way that you authenticated them for the initial page load.  The only reason you can't do that is if:
Your system is anonymous and people are not authenticated at all.  In this case though, you don't need to do anything fancy at all: just use PHP's built in session system.  Its entire purpose is to securely identify users from one page-view to another, and everything you have here is basically a poor-man's session system.  You don't have to worry about errors in implementation if you use one that has been around and vetted for many years and by countless websites.  Don't reinvent the wheel, especially when it comes to security.

Also, standard things apply:

Make sure to flag your cookies as HTTP-only
Make sure to configure your cookies to only be transmitted over HTTPS.
This won't protect against session stealing, but then again neither will your solution.  That topic is a completely different discussion.

Forgot to say

If you don't want to use PHP sessions, then things can still be substantially simplified.  To reiterate the big picture goal, all you really want is a unique (and unguessable) key that lets you identify visitors from page-view to page-view.  You don't need crypt, md5, or salting to make that happen.  All you need is, literally, a long random string.  A 32 character string made from just numbers and letters (upper+lower case) gives you almost 200 bits of entropy, and is very easy to generate.  As long as your random string generator has no weaknesses, no one is going to guess such a string.  This becomes your session ID.  Store it in a cookie, and get it out of the cookie when it comes back up.  No need to encrypt or hash it: those don't increase your security at all.  So in the end your code basically just looks like:

$sessionId = randomString(32);
setCookie('qnrwp_ajax_cookie', $sessionId);
set_transient('qnrwp_ajax_temp_salt_'.$sessionId, 60 * MINUTE_IN_SECONDS);

Then:

$ajaxTrans = get_transient('qnrwp_ajax_temp_salt_'.$_COOKIE['qnrwp_ajax_cookie']);
if (!$ajaxTrans) wp_die();
// Also make sure and check session expiration

Also, you don't want to just die.  You should return some sort of API error that your front-end AJAX call can turn into a more user friendly "Your session has expired" error message.  Don't forget to check the session expiration.


Checking for many alertable conditions before saving customer information What's the best way to refactor this code? The method does a lot of checking before showing an alert. And the method is too long for my taste.

I'm thinking of extracting the checkers into their own methods, but I'm not sure how to handle multiple instances that call return.

- (IBAction)saveCustomer:(id)sender {
    @try {
        [self.view endEditing:YES];

        // Cannot save if customer is pending
        if ([self.service isWaitingForApproval]) {
            APAlert(WARNING_TITLE, NSLocalizedString(@"message.waiting-approval", nil));
            return;
        }

        // Check required field
        NSString *incompleteRequiredField = [self.service checkRequiredFields];
        if (incompleteRequiredField) {
            NSString *message = [NSString localizedStringWithFormat:NSLocalizedString(@"message.required field %@", nil), NSLocalizedString(incompleteRequiredField, nil)];
            APAlert(WARNING_TITLE, message);
            return;
        }

        if(![self.service isDistributorOrWholesalerInTW]) {
            incompleteRequiredField = [self checkRequireCellOTM];
            if(incompleteRequiredField) {
                NSString *message = [NSString localizedStringWithFormat:NSLocalizedString(@"message.required otm %@", nil), NSLocalizedString(incompleteRequiredField, nil)];
                APAlert(WARNING_TITLE, message);
                return;
            }
        }

        Country_Identifier countryID = (Country_Identifier)[self.service.customer.country_id integerValue];
        if (countryID == Country_Taiwan || countryID == Country_Thailand) {
            if (![self.service isHasContact]) {
                APAlert(WARNING_TITLE, ALERT_CONFRIM_HAVE_CONTACT);
                return;
            }
        }
        [[SomeManager alertViewManager] showAlertWithTitle:SAVING_TITLE
                                                                  message:SAVE_CUSTOMER_CONFIRM
                                                                   target:self
                                                               actionView:nil
                                                        cancelButtonTitle:YES_BUTTON_TITLE
                                                        otherButtonTitles:NO_BUTTON_TITLE
                                                                   andTag:AlertViewTag_Save];
    }
    @catch(NSException *exception) {
        [self sendException:exception];
    }
}

You can absolutely extract validation part out of this method because almost every validation is depending on self.service object.

1 Write a method in self.service class that will validate situation and return NSString message if not valid situation.

2 As only one validation is happening on self i.e. checkRequireCellOTM method, pass result of this method while validating self.service.

3 According to result of new method show an alert or proceed.

Here is how I'm trying to clean it-

New method in class of self.service:

- (NSString*)validateServiceReadiness:(NSString*)requiredCellOTM {
  // Cannot save if customer is pending
  if ([self isWaitingForApproval]) {
    return NSLocalizedString(@"message.waiting-approval", nil);
  }

  // Check required field
  NSString *incompleteRequiredField = [self checkRequiredFields];
  if (incompleteRequiredField) {
    NSString *message = [NSString localizedStringWithFormat:NSLocalizedString(@"message.required field %@", nil), NSLocalizedString(incompleteRequiredField, nil)];
    return message;
  }

  if(![self isDistributorOrWholesalerInTW]) {
    incompleteRequiredField = requiredCellOTM;
    if(incompleteRequiredField) {
        NSString *message = [NSString localizedStringWithFormat:NSLocalizedString(@"message.required otm %@", nil), NSLocalizedString(incompleteRequiredField, nil)];
        return message;
    }
  }

  Country_Identifier countryID = (Country_Identifier)[self.customer.country_id integerValue];
  if (countryID == Country_Taiwan || countryID == Country_Thailand) {
    if (![self isHasContact]) {
        return ALERT_CONFRIM_HAVE_CONTACT;
    }
  }
}

Then use this method for validation as:

- (IBAction)saveCustomer:(id)sender {
  @try {
    [self.view endEditing:YES];

    NSString *serviceReadinessMessage = [self.service validateServiceReadiness:[self checkRequireCellOTM]];
    if (serviceReadynessMessage != nil) {
        APAlert(WARNING_TITLE, serviceReadinessMessage);
        return;
    }
    [[SomeManager alertViewManager] showAlertWithTitle:SAVING_TITLE
                                               message:SAVE_CUSTOMER_CONFIRM
                                                target:self
                                            actionView:nil
                                     cancelButtonTitle:YES_BUTTON_TITLE
                                     otherButtonTitles:NO_BUTTON_TITLE
                                                andTag:AlertViewTag_Save];
  }
  @catch(NSException *exception) {
    [self sendException:exception];
  }
}

I'm not sure which method is throwing an exception here, but if it's belongs to our new method validateServiceReadiness then modify syntax accordingly.


Base64 encoder optimization Base64 is a widely used encoding mechanism to allow arbitrary binary content to be transferred as printable text.  This program is designed to take binary file as input and produce a Base64-encoded binary file as output.

#include <stdlib.h>
#include <stdio.h>

void enblock( int len, char *in, char *base64, FILE *outputFILE)
{
    fprintf(outputFILE, "%c", base64[in[0] >> 2]);
    fprintf(outputFILE, "%c", base64[((in[0] & 3) << 4) | ((in[1] & 240) >> 4)]);
    if (len > 1)
    {
        fprintf(outputFILE,"%c", base64[((in[1] & 15) << 2) | ((in[2] & 192) >> 6)]);
    }
    else
    {
        fprintf(outputFILE, "%c", '=');
    }
    if (len > 2)
    {
        fprintf(outputFILE, "%c", base64[(in[2] & 63)]);
    }
    else
    {
        fprintf(outputFILE, "%c",'=');
    }
}

void endecode (FILE *inputFILE, FILE *outputFILE)
{
    static char base64[]="ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";
    int len = 0;
    int i = 0;
    char buffer[3] = {0};
    while ((len = fread(buffer,sizeof(char),3,inputFILE)) >= 1)
    {
        for (i = len; i < 3; i++)
            buffer[i] = 0;
        enblock(len, buffer, base64 ,outputFILE );
    }
}

int main()
{
    FILE *file1 = NULL;
    FILE *file2 = NULL;
    if (NULL == (file1 = fopen("INPUT.txt", "r")))
    {
        printf("Can't open INPUT file!");
        return -1;
    }
    if (NULL == (file2 = fopen("OUTPUT.base64", "wb")))
    {
        printf("Can't create OUTPUT file");
        return -1;
    }
    endecode(file1,file2);
    fclose(file1);
    fclose(file2);
    return 0;
}

I see some things that could be improved.

Add comments to describe what the code is doing

It was not really immediately obvious what the code was intending to do.  In part this was because of some of the variable names, but also due to lack of comments.  Some comments describing what each function is attempting to do would have helped a lot.

Use mode rb for binary files

I found that the program did not work for me until I changed the file mode on the input from "r" to "rb".  This too made it not obvious which direction (base64 to text or vice-versa) was intended.  I'd expect the base64 file to be text mode and the binary file to be binary mode.

Allow the user to specify input and output files

The file names are currently hardcoded which certainly greatly restricts the usefulness of the program.  Consider using argc and argv to allow the user to specify file names on the command line.  Also OUTPUT.base64 is certainly a poor choice for a hardcoded file name since it is actually the input that is encoded in base64.

Avoid fprintf and friends for unformatted output

The code currently uses fprintf with a format string of "%c" for output of decoded byte values.  This isn't particularly efficient.  fputc would likely be a better choice for this.  Better still would likely be to decode multiple bytes and emit them all at the same time using fwrite.

Think carefully about where to put static data

In this code, the base64 static string is in the endecode() function, but it's only used within the enblock() function!  Wouldn't it make more sense to put it where it's being used and not have to pass the pointer?

Avoid magic numbers

One of the lines of code here is this:

fprintf(outputFILE, "%c", base64[((in[0] & 3) << 4) | ((in[1] & 240) >> 4)]);

All of those numbers, such as 240 and 4 no doubt mean something, but what?  I'm sure I could figure out that this is intended to mask and shift bits into place, but why make it a mystery?

Consider using buffering

In a program like this the typical bottleneck is I/O.  For that reason, one strategy for speeding up the program is to use memory (which is fast) in preference to disk I/O (which is triggered by such calls as printf and fread.  One way to apply that strategy in this case would be to read a larger chunk into memory, process it there and then write a large chunk of memory to disk at a time.  However, it's also often true that disk I/O is already buffered by the underlying operating system so the only way to find out for sure is to measure.