Game of Life rules in 14 lines of Python I've attempted a functional solution to Conway's Game of Life in Python.

The example code allows you to see the next generation of the universe by calling the step() function, passing the current generation of the universe. The universe is represented as a set of live cells. Live cells are represented as a tuple of x, y coordinates.

All suggestions for improvement are welcome. I'd especially like feedback on the following:

Approach - is it functional? If not, why not?
Test cases - are there any test case I've missed that highlight a bug? Can it be done with less test cases while maintaining the same code coverage?
Python idioms and conventions
Making use of built-in functions and datatypes

import unittest

def get_neighbours(x, y):
    '''
    Returns the set of the given cell's (x,y) 8 eight neighbours.
    :param x: x coordinate of cell
    :param y: y coordinate of cell
    '''
    return {(x + dx, y + dy) for dx, dy in [(-1, -1), (-1, 0), (-1, 1), (0, -1), (0, 1), (1, -1), (1, 0), (1, 1)]}

def is_survivor(universe, x, y):
    '''
    Returns True if given cell will survive to the next generation, False otherwise.
    :param universe: set of live cells in the universe. A live cell is the tuple (x,y)
    :param x: x coordinate of cell
    :param y: y coordinate of cell
    '''
    num_live_neighbours = len(get_neighbours(x, y) & universe)
    return num_live_neighbours == 2 or num_live_neighbours == 3

def is_born(universe, x, y):
    '''
    Returns True if given cell will be born in the next generation, False otherwise.
    :param universe: set of live cells in the universe. A live cell is the tuple (x,y)
    :param x: x coordinate of cell
    :param y: y coordinate of cell
    '''
    return len(get_neighbours(x, y) & universe) == 3

def step(universe):
    '''
    Returns the new universe after a single step in the game of life.
    :param universe: set of live cells in the universe. A live cell is the tuple (x,y)
    '''
    survivors = { (x, y) for x, y in universe if is_survivor(universe, x, y) }
    list_of_neighbour_sets = [get_neighbours(x, y) for x, y in universe]
    flattened_neighbour_set = {item for subset in list_of_neighbour_sets for item in subset}
    dead_neighbours = flattened_neighbour_set - universe
    births = { (x, y) for x, y in dead_neighbours if is_born(universe, x, y) }
    return survivors | births

class Test(unittest.TestCase):

    def test_get_neigbours(self):
        self.assertEqual({(-1, -1), (0, -1), (1, -1), (-1, 0), (1, 0), (-1, 1), (0, 1), (1, 1)}, get_neighbours(0, 0))
        self.assertEqual({(4, 5), (4, 6), (4, 7), (5, 5), (5, 7), (6, 5), (6, 6), (6, 7)}, get_neighbours(5, 6))

    def test_is_survivour_should_return_true_if_cell_has_2_live_neighbours(self):
        self.assertTrue(is_survivor({(0, 0), (1, 0), (2, 0)}, 1, 0))

    def test_is_survivour_should_return_true_if_cell_has_3_live_neighbours(self):
        self.assertTrue(is_survivor({(0, 0), (1, 0), (0, 1), (1, 1)}, 0, 0))
        self.assertTrue(is_survivor({(0, 0), (1, 0), (0, 1), (1, 1)}, 1, 0))
        self.assertTrue(is_survivor({(0, 0), (1, 0), (0, 1), (1, 1)}, 0, 1))
        self.assertTrue(is_survivor({(0, 0), (1, 0), (0, 1), (1, 1)}, 1, 1))

    def test_is_survivour_should_return_false_if_cell_is_underpopulated(self):
        self.assertFalse(is_survivor({(0, 0)}, 0, 0))

    def test_is_survivour_should_return_false_if_cell_is_overpopulated(self):
        self.assertFalse(is_survivor({(-1, -1), (0, -1), (1, -1), (-1, 0), (1, 0), (-1, 1), (0, 1), (1, 1)}, 0, 0))

    def test_is_born_should_return_false_if_dead_cell_doesnt_have_exactly_3_live_neighbours(self):
        self.assertFalse(is_born({(0, 0)}, 0, 0))

    def test_is_born_should_return_true_if_dead_cell_has_exactly_3_live_neighbours(self):
        self.assertTrue(is_born({(0, 0), (1, 0), (0, 1)}, 1, 1))

    def test_L_becomes_block_after_step(self):
        self.assertEqual({(0, 0), (1, 0), (0, 1), (1, 1)}, step({(0, 0), (0, 1), (1, 1)}))

if __name__ == "__main__":
    unittest.main()

For docstrings, if you're following the Sphinx documentation format (which it looks like you are), you can specify an explicit :return: field to document what exactly it is your function is returning. PEP-0257 has various other conventions for docstrings like leaving a blank line between the summary line and the rest of the docstring, if you're really interested.

In terms of functional programming, you could use some of Python's functional programming functions, e.g the builtins filter and map and also any functions from the functools module. Also, since you are already not mutating any of the variables in your program (as you wouldn't do when programming in a functional manner), you can use frozenset()s in place of where you're currently using sets, as frozen sets are explicitly immutable.

Here is an attempt at "functionifying" the step function. Since Python 3, you can put type hints in the signature of functions, and since Python 3.5 you can also use the typing module to define types in a formal way, which can allow for better static type-checking if you're using a typing-aware IDE. Since you're wanting to program in a functional way, I'm guessing this would be of more interest than usual.

from functools import reduce
from typing import Tuple, Set

Cell = Tuple[int, int]
Universe = Set[Cell]  # or even FrozenSet[Cell]

...

def step(universe: Universe) -> Universe:
    """ Evaluate the next step in the Game of Life.

    :param universe: set of live cells in the universe. A live cell is the tuple (x,y)
    :return: the new universe after a single step in the game of life.
    """
    survivors = filter(lambda cell: is_survivor(universe, *cell), universe)
    neighbours = reduce(set.union, map(lambda cell: get_neighbours(*cell), universe))
    dead_neighbours = filter(lambda cell: cell not in universe, neighbours)
    births = filter(lambda cell: is_born(universe, *cell), dead_neighbours)
    return frozenset(survivors) | frozenset(births)

To be honest though, functional programming does not seem idiomatic in Python, even though it can be done; I would say your current code is pretty clear and idiomatic as is, using set comprehensions and such.


Python Genetic Algorithm Implementation I've made this to pick the LQR parameters for my self balancing robots simulation. So to break down what I have done,
Randomly Create Population

I create a random population. The population is a list of lists having 4 parameters each of which is randomly generated. But I have applied bounds for them using the values I got from my manual tuning for my simulation.
 def population_create(p_size,geno_size,bounds):

 p = [ random_param(geno_size,bounds) for i in range(p_size)]
 return p

def random_param(g,b):
     return [random.uniform(b[i][0],b[i][1]) for i in range(g)]

Try it on Robot and Evaluate
2.Then I evaluate each parameter to see how much the wheels have rotated and how much the robot is inclined after one minute.
fitness = encoder_readings + tilt
So the closer the fitness is to zero the better the robot balances.
This is done for the whole population
Create next generation
3.Then I make the next generation with Mutation, Crossover and passing healthy individuals directly.
def population_reproduce(p,fitness):

    size_p = len(p)
    new_p = []

    dataframe = pd.DataFrame({"Param":p,"Fitness":fitness})
    dataframe = dataframe.sort_values(['Fitness'])
    dataframe = dataframe.reset_index(drop=True)

    sorted_p = dataframe['Param'].tolist()

    elite_part = round(ELITE_PART*size_p)
    new_p = new_p + sorted_p[:elite_part]

    for i in range(size_p-elite_part):
        mom = p[random.randint(0,size_p-1)]
        dad = p[random.randint(0,size_p-1)]
        child = crossover(mom,dad)
        child = mutate(child)
        new_p.append(child)

    return new_p

def crossover(p1,p2):

    crossover = []
    locii = [random.randint(0,8) for _ in range(len(p1))]

    for i in range(len(p1)):
        if locii[i]>4:
            crossover.append(p2[i])
        else:
            crossover.append(p1[i])

    return crossover

def mutate(c):
    size = len(c)
    for i in range(size):
        if random.random()< MUTATION_PROBABILITY:
            c[i] += random.gauss(c[i]/50,MUTATION_DEVIATION)

    return c

This continues for some generations.
These are the links to the full codes.
Genetic Algorithm Supervisor
Population Script
Robot Controller
I would very much appreciate if you could take a look and let me know if this is a correct implementation of a genetic algorithm.
One observation I made is that the fitness doesn't converge to a lower value after each iteration. It randomly goes up and down. Could this be because of some inconsistency in my stimulation on Webots or is it a mistake in my code thats causing this?

On the style of the code:
While I'm not saying the code is or isn't correct, there's some small tweaks you could make it to improve it's legibility:
Spacing
Write code like you would write English (in some sense :P). After commas you needs spaces, so do

def crossover(p1, p2): instead of def crossover(p1,p2):;
random.uniform(b[i][0], b[i][1]) instead of random.uniform(b[i][0],b[i][1])
same thing around binary ops: if locii[i] > 4: instead of if locii[i]>4:
etc.

List comprehensions often do not need a leading space, so you can go with
p = [random_param(geno_size, bounds) for i in range(p_size)]

instead of
p = [ random_param(geno_size,bounds) for i in range(p_size)]

crossover
You have written:
def crossover(p1,p2):

    crossover = []
    locii = [random.randint(0,8) for _ in range(len(p1))]

    for i in range(len(p1)):
        if locii[i]>4:
            crossover.append(p2[i])
        else:
            crossover.append(p1[i])

    return crossover

Consider this alternative definition:
def crossover(p1, p2):

    crossover = []
    for p1_elem, p2_elem in zip(p1, p2):
        if random.randint(0,8) > 4:
            crossover.append(p2_elem)
        else:
            crossover.append(p1_elem)

    return crossover

Notice I don't precompute all locii, although you could do that, of course. Also note the usage of zip which means I can traverse both p1 and p2, instead of having to use indices to later grab the values. (You can read more about zip and find other tips and tricks here.
random_param
In line with one of the things I showed above, instead of doing
def random_param(g,b):
     return [random.uniform(b[i][0],b[i][1]) for i in range(g)]

you could iterate directly over b:
def random_param(g, b):
     return [random.uniform(bs[0], bs[1]) for bs in b]

(More on better iterating in Python here.)
Then, take that and unpack bs with * instead of with indices:
def random_param(g, b):
     return [random.uniform(*bounds) for bounds in b]


Optimizing priority queue streaming algorithm in C++ This is a sorting algorithm that I wrote for finding the maximum \$n\$ elements at any time in a stream of IDs and integers with unknown length. I primarily do my coding in Python, so I'm not too familiar with C++ best practices, but I wanted to write this in C++ to gain some efficiency and speed.

For all of you seasoned C++ programmers; am I following best practices here or not? If not, what improvements can I make to make this more professional? Lastly, are there any optimizations that I can make to increase its speed?

#include <iostream>
#include <fstream>
#include <queue>
#include <vector>
#include <map>
#include <chrono>

// Overload comparison operator to create a min heap
struct compare
{
  bool operator()(const int& l, const int& r)
  {
    return l > r;
  }
};

int main()
{
  // Input filenames
  std::cout << "Enter a filename for the input stream: ";
  std::string filename;
  std::cin >> filename;
  std::ifstream infile(filename);

  std::cout << "Enter a filename for the output stream: ";
  std::string output;
  std::cin >> output;
  std::ofstream outfile(output);

  // Input a value for k
  std::cout << "Enter a value for k: ";
  int k;
  std::cin >> k;
  std::cout << std::endl;

  // Declare priority queue and map
  int elements = 0;
  int replacements = 0;
  int value;
  std::string id;
  std::priority_queue<int, std::vector<int>, compare> heap;
  std::map<int, std::vector<std::string>> ids;

  // Begin read/sort timer
  std::cout << "Beginning read and sort ..." << std::endl;
  auto readsortstart = std::chrono::high_resolution_clock::now();

  // Begin read and sort
  while (infile >> id >> value){
    elements++;
    if (heap.size() < k){
      ids[value].push_back(id);
      heap.push(value);
    } else if (heap.top() < value) {
      ids[value].push_back(id);
      heap.pop();
      heap.push(value);
      replacements++;
    }
  }

  // End timer
  auto readsortstop = std::chrono::high_resolution_clock::now();
  auto readduration = std::chrono::duration_cast<std::chrono::milliseconds>(readsortstop-readsortstart).count();
  std::cout << "Read and sort completed." << std::endl << std::endl;

  // Ensure that k is not larger than the total number of lines
  k = std::min(k,elements);

  // Begin write timer
  std::cout << "Beginning write ..." << std::endl;
  auto writestart = std::chrono::high_resolution_clock::now();

  // Pop k entries off of the sorted heap to retrieve the k largest results
  for (int i = 0; i < k; i++) {
    int key = heap.top();
    outfile << ids[key].back() << " " << heap.top() << std::endl;
    ids[key].pop_back();
    heap.pop();
  }

  // End write timer
  auto writestop = std::chrono::high_resolution_clock::now();
  auto writeduration = std::chrono::duration_cast<std::chrono::milliseconds>(writestop-writestart).count();
  std::cout << "Write completed." << std::endl << std::endl;

  std::cout << "----------------------- Statistics ------------------------" << std::endl
            << "  Total number of replacements: " << replacements << std::endl
            << "  Total number of elements:     " << elements << std::endl
            << "  Read/sort time elapsed:       " << readduration << "ms" << std::endl
            << "  Write time elapsed:           " << writeduration << "ms" << std::endl;
}

You could create a

struct Element {
   int value;
   std::string id;
};

and store that in the priority queue. Then you don't need the map. Your compare function would of course only look at the value component:

struct compare {
   bool operator()(const Element& l, const Element& r) {
       return l.value > r.value;
   }
};

Note that it is possible to use a naked function as comparator, it doesn't need to be a functor:

bool compare(const Element& l, const Element& r) {
   return l.value > r.value;
}

std::priority_queue<Element, std::vector<Element>, decltype(compare)> heap(compare);


Dynamically loading CSS files based on URL or URI in PHP I have written out the following code in the head section of my document,
which should load 3 separate style sheets for each page based on the URL/URI that the user is visiting. It is working as intended as tested with the comment in the code but I am wondering if there is a more efficient way to do this. I initially started writing out a switch statement, but chose to try if else statements before.

Also I have added this part:

 || $_SERVER['PHP_URL_PATH']

to each statement in case there is an error of some kind with the first expression. It seems to be working whether I use just ['PHP_URL_PATH']
or the full $_SERVER['PHP_URL_PATH'].

My questions are:

Which out of the two would be more efficient (switch or if else)?
Is the second declaration of $_SERVER actually necessary, or will it work without this specificity?

Any improvements that people could point me to would be greatly appreciated.

PS: I have removed all the echo's for including the actually <link>'s to the CSS files.

This has been tested and is showing to work in Firefox Developer Edition so guessing it's ok, but could be improved maybe; what about scalability, or any security concerns? I am new so please advise or help.

// Create Logic here to include various different style sheets.

if($_SERVER['PHP_SELF'] || $_SERVER['PHP_URL_PATH']  === "/mainHubV8.1.php"){
    echo "Loading Styles for MainHubV8.1.php";

    }
else if ($_SERVER['PHP_SELF'] || $_SERVER['PHP_URL_PATH'] === "/advSearchV8.1.php"){
    echo "Loading Styles for advSearchV8.1.php";

    }
else if ($_SERVER['PHP_SELF'] || $_SERVER['PHP_URL_PATH'] === "/loginOrSignUpV8.1.php"){
    echo "Loading Styles for loginOrSignUpV8.1.php";

    }
else if ($_SERVER['PHP_SELF'] || $_SERVER['PHP_URL_PATH'] === "/profilePageV8.1.php"){
    echo "Loading Styles for profilePageV8.1.php";

    }
else if ($_SERVER['PHP_SELF'] || $_SERVER['PHP_URL_PATH'] === "/chatApllicationV8.1.php"){
    echo "Loading Styles for chatApllicationV8.1.php";

    }
else if ($_SERVER['PHP_SELF'] || $_SERVER['PHP_URL_PATH'] === "/onlineUsersV8.1.php"){
    echo "Loading Styles for onlineUsersV8.1.php";

    }
else if ($_SERVER['PHP_SELF'] || $_SERVER['PHP_URL_PATH'] === "/index.php"){
    echo "Loading Styles for index.php";

    }
else if ($_SERVER['PHP_SELF'] || $_SERVER['PHP_URL_PATH'] === "/404"){
    echo "Loading Styles for error page";

    }

Is there a better alternative to achieving the same goal? If so could you please provide reference or articles/Question & answers anywhere on Stack Exchange or some other resource. Plus I do not want to use Javascript or JQuery really as these can be turned off and disabled. So PHP seems more appropriate.

Most of questions you are asking are not fit for this site and your code does not work as intended but this is an interesting case to review
The grave mistake
Adding anything to your code just in case there would be some imaginary "error of some kind" is the worst thing a programmer could do ever. You effectively ruined your code with adding that || $_SERVER['PHP_URL_PATH'] stuff:

there is no such thing $_SERVER['PHP_URL_PATH'] for starter: such a variable just doesn't exist
of course ['PHP_URL_PATH'] without the $_SERVER part makes no sense
neither the whole condition returns anything sensible due to PHP syntax rules you are yet to learn.

Yet this code works somehow, albeit not the way you expect.
The operator precedence
The way this code executes is a very interesting matter, once I wrote an article that explains it in detail, Operator precedence or how does 'or die()' work. Given you only started learning it could be too complex for you for the moment to wrap your head around, but in time you will find it very interesting to read.
$_SERVER['PHP_SELF'] || $_SERVER['PHP_URL_PATH']  === "/mainHubV8.1.php" doesn't mean "if either PHP_SELF or PHP_URL_PATH equal to /mainHubV8.1.php". It means "if PHP_SELF contains any value OR PHP_URL_PATH is equal to /mainHubV8.1.php".
In short, an expression $_SERVER['PHP_SELF'] || $_SERVER['PHP_URL_PATH']  === "/mainHubV8.1.php" evaluates to true when $_SERVER['PHP_SELF'] contains any value. Which means always.
Given PHP already has it's answer, it ceases to execute the further condition. This is why you can write either $_SERVER['PHP_URL_PATH'] or ['PHP_URL_PATH'] or don't even get a notice for the non-existent array index - this code is just never executed.
As a result, this code will always output just "Loading Styles for MainHubV8.1.php" no matter which page it is called on.
The right way
To elaborate on the Neoan's answer, given you have 3 styles to load, here is the proposed array structure
$stylesheets = [
    '/mainHubV8.1.php' => [
        'style1.css',
        'style2.css',
        'style3.css',
    ],
    '/advSearchV8.1.php' => [
        'style4.css',
        'style5.css',
        'style6.css',
    ],
    // and so on
];

given this structure, you will be able to get the correct styles right from $_SERVER['PHP_SELF']:
foreach($stylesheets[$_SERVER['PHP_SELF']] as $file) {
    echo "<link rel='stylesheet' type='text/css' href='$file'>\n";
}


Creates an employee record in a time and attendance database and returns the employee object I am writing a time and attendance system.  This is my idea of a simple elegant solution for the employee class. To me it makes the client very simple an encapsulates everything it needs, however I an new to Object Orientated Design and would appreciate a review.  I plan to create the other classes like this but fear there may be a better way.

My employee class...

 class Employee
    {
        public static Employee CreateNew(string forename, string surname)
        {
            Employee employee = new Employee(forename, surname);
            AddToDataBase(employee);
            return employee;
        }

        public string Forename { get; set; }
        public string Surname { get; set; }

        private Employee(string forename, string surname)
        {
            this.Forename = forename;
            this.Surname = surname;
        }

        private static void AddToDataBase(Employee employee)
        {
          //code to add employee to database

        }
    }

usage...

Employee.CreateNew("Bob", "Smith");

It is common practice to use static methods-creators instead of constructors. But this approach is used in cases where you need to convert some objects to the object of desired type. It is the main reason for such methods – conversion from one representation to another.

But you use your CreateNew method for side effects and it is not good in my opinion. If I call CreateNew I'll expect just to get a new instance of the Employee. I don't expect that this method will do something else.

I would create method AddNewEmployeeAndAddToDB but not in the Employee class. Adding to database is not a part of employee functionality.


Find the largest continuous descending sum in a 2d matrix Given a 2d array, the problem is to find the largest sum, where each subsequent is >= the previous digit but < all of the 'neighbour' digits. You can only move up, down, left and right.

My solution will run in \$O(N*M)\$ time with \$O(N*M)\$ space (please correct me if I'm wrong).

The solution is as follows: Start at an arbitrary point and look for the closest largest neighbor (as described above). Once found, append it to the path and mark as visited. Continue these steps until you have no where else to visit.

Code is below, with the the answer being [10, 9, 6, 4, 2]. Thank you

nums = [
  [10,8,4],
  [9,6,8],
  [2,4,3]
]

def get_largest_neighbour(i, j, previous, visited):
    max_location = -1, -1
    max_neighbour = -1

    # look up
    if i-1 >= 0 and nums[i-1][j] < previous and visited[i-1][j] == False:
        max_neighbour = nums[i-1][j]
        max_location = i-1, j

    # look down
    if i+1 < 3:
        if nums[i+1][j] > max_neighbour and nums[i+1][j] < previous and visited[i+1][j] == False:
            max_neighbour = nums[i+1][j]
            max_location = i+1, j

    # look left
    if j-1 >= 0:
        if nums[i][j-1] > max_neighbour and nums[i][j-1] < previous and visited[i][j-1] == False:
            max_neighbour = nums[i][j-1]
            max_location = i, j-1

    # look right
    if j+1 < 3:
        if nums[i][j+1] > max_neighbour and nums[i][j+1] < previous and visited[i][j+1] == False:
            max_neighbour = nums[i][j+1]
            max_location = i, j+1

    return max_location

def max_sum(i, j, path, visited):
    if i >= 3 or i < 0 or j >= 3 or j < 0:
        return

    if visited[i][j] == True:
        return

    # mark as visited and append to path
    visited[i][j] = True
    path.append(nums[i][j])

    # find the next largest path to search
    next_step_i, next_step_j = get_largest_neighbour(i, j, path[-1], visited)

    # there is no where to go, so stop iterating and backtrack
    if next_step_i == -1:
        return

    max_sum(next_step_i, next_step_j, path, visited)

    return path

def largest_continious_sum():
    if len(nums) <= 1:
        return 0

    visited = [[False for x in range(3)] for j in range(3)]
    path = []

    print max_sum(0, 0, path, visited)

largest_continious_sum()

get_largest_neighbour is a big confusing function. Let's try strip it down to avoid rewriting code.

First I would create a loop to run on each of the four directions. To loop, you want to make a list of the four positions and loop over each. To do that, you can just build a simple tuple of co-ordinates were you add and subtract one from i and j in turn:

coordinates = ((i - 1, j), (i + 1, j), (i, j - 1), (i, j + 1))
for x, y in coordinates:

Now you need to fold all your tests to make sure they're in the right range into one, which is simple enough and more readable in my opinion:

    if not 0 <= x < len(nums) or not 0 <= y < len(nums[0]):
        continue

To break it down a bit, the range we want is for x and y to be 0 or greater, and less than the length of the lists. Rather than hardcode 3, it's better to use len of the matrix. If you're concerned about performance for larger matrices, you could precalculate these values. If either of these values isn't valid for their range, then continue is called, with will skip onto the next co-ordinate and not run the rest of the loop.

Now all you need is your actual test and re-setting the max values. For convenience, I'd grab the value of nums[x][y] before the test:

    num = nums[x][y]
    if previous > num > max_neighbour and not visited[x][y]:
        max_neighbour = num
        max_location = x, y

You notice I also changed your if test, to make a single range test and to replace == False with not. Both make it easier to see what you're actually testing.

Now your code is much simpler, easier to adjust and easier to read:

def get_largest_neighbour(i, j, previous, visited):
    max_location = -1, -1
    max_neighbour = -1
    coordinates = ((i - 1, j), (i + 1, j), (i, j - 1), (i, j + 1))

    for x, y in coordinates:
        if not 0 <= x < len(nums) or not 0 <= y < len(nums[0]):
            continue

        num = nums[x][y]
        if previous > num > max_neighbour and not visited[x][y]:
            max_neighbour = num
            max_location = x, y

    return max_location


Simple TCP server using TPL I have a working C# code of a TCP server that runs inside a windows service. It accepts incoming connections and starts sending/receiving data based on some complex rules.

The top-level class for this server looks as follows:

public async void StartListen()
{
    var endpoint = new IPEndPoint(IPAddress.Parse(_host), _port);
    _serverSocket = new Socket(endpoint.Address.AddressFamily, SocketType.Stream, ProtocolType.Tcp);
    _serverSocket.Bind(endpoint);
    _serverSocket.Listen((int)SocketOptionName.MaxConnections);

    while (!_shutdown)
    {
        var socket = await AcceptClient();
        ProcessTransaction(socket);
    }
}

private async Task<Socket> AcceptClient()
{
    var socket = await Task<Socket>.Factory.FromAsync(_serverSocket.BeginAccept, _serverSocket.EndAccept, null);
    return socket;
}

private void ProcessTransaction(Socket socket)
{
    Task.Factory.StartNew(() => ProcessTransactionAsync(socket));
}

private void ProcessTransactionAsync(Socket socket)
{
    // complex logic involving socket.Send, socket.Receive, serialization, etc...
}

The server is started by calling StartListen, and then it should run indefinitely (as long as the Windows service it is hosted in is up and running).

This code works, but I have certain doubts about it. I don't have much experience with TPL, but, for example, I know that async void method is an anti-pattern (as stated in many TPL tutorials). What problems can it cause? Are there any other flaws in this code? This server is supposed to handle high load somewhere in the future, so its speed and efficiency is also a concern.

private void ProcessTransactionAsync(Socket socket)

You've mixed the naming conventions a little bit. We add the Async suffix to methods that are awaitable. This one isn't. You could however easily fix it.

private void ProcessTransaction(Socket socket)
{
    Task.Factory.StartNew(() => ProcessTransactionAsync(socket));
}

Rename it and mark it propertly with async. Let its return value be Task. In place of the Task.Factory.StartNew you can use the shortcut Task.Run(someAction):

private async Task ProcessTransactionAsync(Socket socket)
{
    await Task.Run(() => ProcessTransaction(socket));
}

now you can await it in StartListen:

while (!_shutdown)
{
    var socket = await AcceptClient();
    await ProcessTransaction(socket);
}

finally as Ron Beyer wrote you should change the return value of the StartListen method to Task.


Query to show each sites stock transfer history between all other sites The below query is used to return a dataset for a SSRS report. It works fine and is quick enough but I am looking for any improvements that could be made.

The client has asked to see all transfers between sites and the report should look like:

Site1

Site 2
Site 3

Site2

Site 1
Site 3

Each row should also show the value of the transfer to that site and from that site. Duplicated data is expected I.E If Site 1 transferred goods to Site 2 then Site 2 should display the same value in the from column. If no transfer takes place the site should be visible but with a value of 0 in both To and from columns.

The Exists clause must remain as well as it is used to workout descendants of selected site.

WITH Transfers_CTE(FromSiteNo
                  ,ToSiteNo
                  ,Value)
     AS (
     SELECT Transfers.FromSiteNo
           ,Transfers.ToSiteNo
           ,SUM(Transfers.[Value]) AS Value
     FROM dbo.Transfers
     WHERE Transfers.MoveDate BETWEEN @SessionDateFrom AND @SessionDateTo
     GROUP BY Transfers.FromSiteNo
             ,Transfers.ToSiteNo)
     SELECT CS.No AS SiteNo
           ,CS.Name AS SiteName
           ,JoinedSites.No AS OtherSiteNo
           ,JoinedSites.Name AS OtherSiteName
           ,ISNULL(ToTable.[Value],0) AS ToValue
           ,ISNULL(FromTable.[Value],0) AS FromValue
     FROM dbo.CfgSites AS CS
          FULL OUTER JOIN dbo.CfgSites AS JoinedSites ON CS.No != JoinedSites.No
          LEFT JOIN Transfers_CTE AS ToTable ON ToTable.FromSiteNo = CS.No
                                                AND ToTable.ToSiteNo = JoinedSites.No
          LEFT JOIN Transfers_CTE AS FromTable ON FromTable.ToSiteNo = CS.No
                                                  AND FromTable.FromSiteNo = JoinedSites.No
     WHERE EXISTS
(
    SELECT DescendantSites.Descendant
    FROM dbo.DescendantSites
    WHERE DescendantSites.Parent IN(@SiteNo)
    AND DescendantSites.Descendant = CS.No
)
     ORDER BY CS.No;

Subset DDL for CfgSites and Transfer View that only includes required fields

/****** Object:  Table [dbo].[CfgSites]    Script Date: 13/04/2018 08:47:32 ******/

CREATE TABLE [dbo].[CfgSites](
    [No] [smallint] NOT NULL,
    [Name] [varchar](50) NULL)
GO
/****** Object:  View [dbo].[Transfers]    Script Date: 13/04/2018 08:47:32 ******/

CREATE VIEW [dbo].[Transfers]
AS
SELECT [SiteNo] AS FromSiteNo
    ,[Site2No] AS ToSiteNo
    ,[Date] AS MoveDate
    ,[Value]
FROM PluMovement
WHERE MoveType = 4
    AND Processed = 1

Here is an attempt at a rewrite, but you didn't provide any DDL or DML so there is no way for me to test it.  I'll list reasons for the changes below.

WITH Transfers_CTE (FromSiteNo, ToSiteNo, Value) AS
  (
    SELECT      Transfers.FromSiteNo,
                Transfers.ToSiteNo,
                SUM( Transfers.Value ) AS Value
      FROM      dbo.Transfers
      WHERE     Transfers.MoveDate BETWEEN @SessionDateFrom AND @SessionDateTo
                AND FromSiteNo <> ToSiteNo
      GROUP BY  Transfers.FromSiteNo,
                Transfers.ToSiteNo
  )
  SELECT        FromCS.No AS SiteNo,
                FromCS.Name AS SiteName,
                ToCS.No AS OtherSiteNo,
                ToCS.Name AS OtherSiteName,
                ISNULL( Transfers.Value, 0 ) AS ToFromValue
    FROM        dbo.CfgSites AS FromCS
    LEFT JOIN   Transfers_CTE AS Transfers
      ON FromCS.No = Transfers.FromSiteNo
    LEFT JOIN   dbo.CfgSites AS ToCS
      ON Transfers.ToSiteNo = ToCS.No
    WHERE       EXISTS
        ( SELECT    DescendantSites.Descendant
            FROM    dbo.DescendantSites
            WHERE   DescendantSites.Parent IN ( @SiteNo )
                    AND DescendantSites.Descendant = FromCS.No)
    ORDER BY    FromCS.No
;

Since Cs.No and JoinedSites.No should not be equal, and they are joined to the From & To values of the CTE, I added the filter in the CTE to exclude any transfers where the To and From site matched.
Your Full Outer Join was effectively a left outer join because any results from the Right side table would have been excluded by the Left side reference of your EXISTS clause.
You attempt to join transfers to sites from both directions.  However, if A = B, then B = A.  So if we get all of the FromSite joins established to CS on the first pass, there is no reason to do a second pass by joining JoinedSites to the FromSite as they will already have found a match.  This change also eliminates the near Cartesian product of joining CfgSites to itself.
If we consider the CS table our anchor, understanding it has all of the From Sites included, our problem then becomes how to get the To sites.  The relationship between From and To is contained in the Transfers_CTE.  Thus we just need to join back to the CfgSites using the ToSiteNo field to find the site information.
The Sum aggregate was at the grain of the Transfer relationship, To & From.  This is maintained in the changes, but now there is only one value column, returning the Value that was transferred between From and To.

There is no need to join CfgSites to itself directly, unless you may also need to show any ToFrom combinations that did not get any transfers.

Hopefully this will return your expected results.  If not, provide some DDL/DML and I may take another look at it.


Matrix arithmetic operations I am currently attempting to implement Matrix Math for another project I am working on.

However, I am not sure whether this implementation will work. Can someone please tell me if there are any errors with my implementation?

#include <iostream>
#include <vector>
#include <cassert>

using namespace std;

typedef vector<vector<double> > Matrix;

Matrix add(Matrix a, Matrix b)
{
  assert(a.size() == b.size() && a[0].size() == b[0].size());

  int numRow = a.size(), numCol = a[0].size();
  Matrix output(numRow, vector<double>(numCol));

  for(int i = 0; i < numRow; i++)
  {
    for(int j = 0; j < numCol; j++)
    {
      output[i][j] = a[i][j] + b[i][j];
    }
  }

  return output;
}

Matrix subtract(Matrix a, Matrix b)
{
  assert(a.size() == b.size() && a[0].size() == b[0].size());

  int numRow = a.size(), numCol = a[0].size();
  Matrix output(numRow, vector<double>(numCol));

  for(int i = 0; i < numRow; i++)
  {
    for(int j = 0; j < numCol; j++)
    {
      output[i][j] = a[i][j] - b[i][j];
    }
  }

  return output;
}

Matrix multiply(Matrix a, double b)
{
  int numRow = a.size(), numCol = a[0].size();
  Matrix output(numRow, vector<double>(numCol));

  for(int i = 0; i < numRow; i++)
  {
    for(int j = 0; j < numCol; j++)
    {
      output[i][j] = a[i][j] * b;
    }
  }

  return output;
}

Matrix multiply(Matrix a, Matrix b)
{
  assert(a.size() == b.size() && a[0].size() == b[0].size());

  int numRow = a.size(), numCol = a[0].size();
  Matrix output(numRow, vector<double>(numCol));

  for(int i = 0; i < numRow; i++)
  {
    for(int j = 0; j < numCol; j++)
    {
      output[i][j] = a[i][j] * b[i][j];
    }
  }

  return output;
}

Matrix dotProduct(Matrix a, Matrix b)
{
  assert(a[0].size() == b.size());

  int numRow = a.size(), numCol = b[0].size();
  Matrix output(numRow, vector<double>(numCol, 0));

  for(int i = 0; i < numRow; i++)
  {
    for(int j = 0; j < numCol; j++)
    {
      for(unsigned int k = 0; k < a[0].size(); k++)
      {
        output[i][j] += a[i][k] * b[k][j];
      }
    }
  }

  return output;
}

Matrix transpose(Matrix a)
{
  int numRow = a[0].size(), numCol = a.size();
  Matrix output(numRow, vector<double>(numCol));

  for(int i = 0; i < numRow; i++)
  {
    for(int j = 0; j < numCol; j++)
    {
      output[i][j] = a[j][i];
    }
  }

  return output;
}

Matrix applyFunc(Matrix a, double (*f)(double))
{
  int numRow = a.size(), numCol = a[0].size();
  Matrix output(numRow, vector<double>(numCol));

  for(int i = 0; i < numRow; i++)
  {
    for(int j = 0; j < numCol; j++)
    {
      output[i][j] = (*f)(a[i][j]);
    }
  }

  return output;
}

int main()
{

}

Overview
Sure this is one way to represent a Matrix.
typedef vector<vector<double> > Matrix;

The problem here is that there is no enforcement that these are rectangular. Your code makes the assumption they are rectangles and things will go very wrong if the assumption is wrong.
You don't use encapsulation.
Matrix add(Matrix a, Matrix b)
Matrix subtract(Matrix a, Matrix b)
Matrix multiply(Matrix a, double b)
Matrix multiply(Matrix a, Matrix b)
Matrix dotProduct(Matrix a, Matrix b)

All these are standalone methods. Not an absolute no-no but using classes correctly you can enforce the rectangular size requirements (preferably at compile time) but you could do it at runtime. If you use these methods then these would normally be member functions.
Also these functions are just wrong:
Matrix multiply(Matrix a, Matrix b)
Matrix dotProduct(Matrix a, Matrix b)

Neither of these functions do what they advertise. You should check out wikipedia for the definitions.


Beautifulsoup and lxml (xpath) too slow with respect to regex when parsing HTML I agree that using regex to parse HTML is not a good way, in particular I am worried about their fragility with respect to change in the HTML.

The problem is that any alternatives are really too slow.

I am parsing this web page which is quite big (3MB). Somewhere in the html there is:

<form class="form-inline"  id="exportform" action="/references" class="hidden">
    <input type="hidden" name="format" value="" />

    <input type="hidden" name="item_id" value="678860" />

    <input type="hidden" name="item_id" value="656680" />

    <input type="hidden" name="item_id" value="678846" />

    <input type="hidden" name="item_id" value="650319" />

    <input type="hidden" name="item_id" value="651504" />

    <input type="hidden" name="item_id" value="678849" />

    <input type="hidden" name="item_id" value="649821" />

    <input type="hidden" name="item_id" value="649835" />

    <input type="hidden" name="item_id" value="651512" />

    <input type="hidden" name="item_id" value="651510" />

</form>

I need to extract the 10 numbers in the value field. I know they are exactly 10.

I tried different approaches:

BeautifulSoup

from bs4 import BeautifulSoup

def soup(html):
    soup = BeautifulSoup(html, 'lxml')
    form = soup.find("form", {'id': 'exportform'})
    if not form:
        return []
    entries = form.find_all("input", {'name': 'item_id'})
    result = [entry['value'] for entry in entries]
    return result

Regex

import re
import itertools

def regex(html):
    r = re.compile(r'<input type="hidden" name="item_id" value="([0-9]+)">')

    s = r.finditer(html)
    result = []
    for ss in itertools.islice(s, 10):
        result.append(ss.group(1))
    return result

xpath

import lxml.html
from lxml import etree # same performance with this

def xpath(html):
    root = lxml.html.fromstring(html)
    result = root.xpath('//form[@class="form-inline"]/*[@name="item_id"]')
    return [r.attrib['value'] for r in result]

I get the same results from the three, but very different timing:

BeautifulSoup: 3.3
regex: 0.00013
xpath: 0.57

In the regex case I am able to use the trick that I know there are exactly 10 numbers to find, gaining a factor 10. But also without that trick the regex approach is 400 times faster than xpath.

In addition asking xpath to give me the first 10:

(//form[@class="form-inline"]/*[@name="item_id"])[position() < 11]

is not improving

Well, your regex solution is obviously badly broken - it will fail if the attributes are in a different order, if they are separated by newlines, if they are delimited by single quotes, etc etc. If you try to replace it with a more correct regex (it will never be 100% correct of course) then you are quite likely to lose some of this speed - perhaps dramatically, if it starts backtracking.

The cost of the XPath solution is not actually in evaluating the XPath, it is (a) parsing the source document and building a tree representation, and (b) compiling the XPath expression. The key to getting a performance improvement is to make sure you are doing these operations only once. I don't know whether that's possible in your overall application scenario.


NIO2 and ByteBuffer use for reading variable length messages I am reading variable length messages where each message is an int + msg bytes. Messages could be any size.

Right now my strategy is to have a direct buffer that reads from the socket, which then transfers the bytes read into an array-backed nondirect buffer. However my solution has a lot of copying, some loops and just feels a bit "icky".

Is there a better solution?

Here is the read completion handler. I'm hoping it should be pretty obvious what is going on:

class DelimitedReadCompletionHandler implements CompletionHandler<Integer, Object> {

    private final AsynchronousByteChannel channel;
    private final Consumer<byte[]> onMessageCallback;
    private final ByteBuffer channelReadBuffer; // this is a direct buffer.

    private volatile ByteBuffer currentMessageBuffer;

    public DelimitedReadCompletionHandler(AsynchronousByteChannel channel,
                                          Consumer<byte[]> onMessageCallback,
                                          ByteBuffer channelReadBuffer) {

        this.channel = channel;
        this.onMessageCallback = onMessageCallback;
        this.channelReadBuffer = channelReadBuffer;
    }

    @Override
    public void completed(Integer result,
                          Object attachment) {

        channelReadBuffer.flip();

        do {
            if (currentMessageBuffer == null) {
                if (channelReadBuffer.remaining() < Integer.BYTES) {
                    channelReadBuffer.compact();
                    break;
                }

                currentMessageBuffer = ByteBuffer.allocate(channelReadBuffer.getInt());
            }

            int copyLength = Integer.min(currentMessageBuffer.remaining(), channelReadBuffer.remaining());

            for (int i = 0; i < copyLength; i++) {
                currentMessageBuffer.put(channelReadBuffer.get());
            }

            if (!currentMessageBuffer.hasRemaining()) {
                byte[] msg = currentMessageBuffer.array();
                ForkJoinPool.commonPool().execute(() -> onMessageCallback.accept(msg));

                currentMessageBuffer = null;
            }

            if (channelReadBuffer.hasRemaining()) {
                continue;
            }

            channelReadBuffer.clear();
            break;
        } while (true);

        channel.read(channelReadBuffer, null, this);
    }

    @Override
    public void failed(Throwable exc,
                       Object attachment) {

        exc.printStackTrace();
    }
}

Write some JavaDoc!

As stated above, I was very confused by your code. However, if your methods included JavaDoc, then I wouldn't be so confused by the names.

Here is an example of JavaDoc on a simple method:

/**
    Returns the sum of both of the parameters.
*/
public int add(int a, int b) {
    return a + b;
}

Note the /**? This is required for it to be valid JavaDoc.

In more complicated functions where the parameters aren't clear or the return value isn't clear, you can include something like this in your JavaDoc:

@param nameOfParameter -- description/purpose of parameter
@return -- description of return
        -- description of return
        -- description of return

You would have multiple -- description of returns if there were multiple potential return values.

There is no point in having a do/while loop where the condition being checked is true; if it were just a normal while loop, it would run atleast once anyway.

I recommend that you change your do/while to just a while as while loops are much more common.

I can not go into the way you tried to solve the problem because I am having trouble understanding your code. I believe this is just on me, but I think that having more documentation would help anyway.


Splitting up a list with datetimes I have this ordered list of datetimes that comprises of 30 minute slots:

availability = [
    datetime.datetime(2010, 1, 1, 9, 0),
    datetime.datetime(2010, 1, 1, 9, 30),
    datetime.datetime(2010, 1, 1, 10, 0),
    datetime.datetime(2010, 1, 1, 10, 30),
    datetime.datetime(2010, 1, 1, 13, 0), # gap
    datetime.datetime(2010, 1, 1, 13, 30),
    datetime.datetime(2010, 1, 1, 15, 30), # gap
    datetime.datetime(2010, 1, 1, 16, 0),
    datetime.datetime(2010, 1, 1, 16, 30)
]

And I would to split the ones with time gaps in between into a dictionary with start and end values, like so:

[
    {'start': datetime.datetime(2010, 1, 1, 9, 0), 'end': datetime.datetime(2010, 1, 1, 10, 30)},
    {'start': datetime.datetime(2010, 1, 1, 13, 0), 'end': datetime.datetime(2010, 1, 1, 13, 30)},
    {'start': datetime.datetime(2010, 1, 1, 15, 30), 'end': datetime.datetime(2010, 1, 1, 16, 30)}
]

So I threw this snippet of code together that does exactly that.

result = []
row = {}
for index, date in enumerate(available):
    exists = False
    if not row:
        row['start'] = date
    try:
        if date + timedelta(minutes = 30) != available[index + 1]:
            exists = True
    except IndexError:
        exists = True
    if exists:
        row['end'] = date
        result.append(row)
        row = {}

But needless to say this is super ugly and there just has to be a better, shorter and more elegant way to do this.

It is possible to utilize itertools.groupby() using indexes to calculate if there is a gap:

import datetime
from datetime import timedelta
from functools import partial
from itertools import groupby
from operator import itemgetter

def is_consecutive(start_date, item):
    """A grouping key function that highlights the gaps in a list of datetimes with a 30 minute interval."""
    index, value = item
    return (start_date + index * timedelta(minutes=30)) - value

available = [
    datetime.datetime(2010, 1, 1, 9, 0),
    datetime.datetime(2010, 1, 1, 9, 30),
    datetime.datetime(2010, 1, 1, 10, 0),
    datetime.datetime(2010, 1, 1, 10, 30),
    datetime.datetime(2010, 1, 1, 13, 0),  # gap
    datetime.datetime(2010, 1, 1, 13, 30),
    datetime.datetime(2010, 1, 1, 15, 30),  # gap
    datetime.datetime(2010, 1, 1, 16, 0),
    datetime.datetime(2010, 1, 1, 16, 30)
]

start_date = available[0]

result = []
for _, group in groupby(enumerate(available), key=partial(is_consecutive, start_date)):
    current_range = list(map(itemgetter(1), group))

    result.append({
        'start': current_range[0],
        'end': current_range[-1]
    })
print(result)

Prints:

[
    {'start': datetime.datetime(2010, 1, 1, 9, 0), 'end': datetime.datetime(2010, 1, 1, 10, 30)},
    {'start': datetime.datetime(2010, 1, 1, 13, 0), 'end': datetime.datetime(2010, 1, 1, 13, 30)},
    {'start': datetime.datetime(2010, 1, 1, 15, 30), 'end': datetime.datetime(2010, 1, 1, 16, 30)}
]


ASP.NET MVC Project Layering This is how I imagine my project's layering:

BLL

Should be independent and contain only pure business logic and properties
Sample Domain Model:

public class Role
{
    private UnitOfWork uow { get; set; }

    public Role()
    {
        uow = new UnitOfWork();
    }

    public int RoleId { get; set; }
    public string Name { get; set; }
    public string ID { get; set; }
    public string Description { get; set; }

    public int AddRole()
    {
        //Mapping from Business 'Role' type to Data Model 'Role' type
        AutoMapper.Mapper.CreateMap<SoC.BLL.Role, SoC.DAL.Role>();
        var model = AutoMapper.Mapper.Map<SoC.DAL.Role>(this);
        return uow.Roles.Add(model);
    }
}

DAL

Contain generic repository and Unit of work and data model meaning model that will reflect the database.
Sample Data Model:

public class Role
{
    [Key]
    public int RoleId { get; set; }

    [Required]
    [StringLength(50, ErrorMessage = "Name Cannot Be More Than 50 Characters")]
    public string Name { get; set; }

    [Required]
    [StringLength(6, ErrorMessage = "ID Cannot Be More Than 6 Characters")]
    public string ID { get; set; }

    [Required]
    [StringLength(250, ErrorMessage = "Description Cannot Be More Than 250 Characters")]
    public string Description { get; set; }
}

Web

This will be ASP.NET MVC app that will use BLL. (Should or should not be aware of DAL?)
Sample Controller Action:

public ActionResult Index()
{
    var role = new Role { RoleId = 1, ID = "ADMIN", Name = "Administrator", Description = "Hello World"  };
    int check = role.AddRole();

    return View();
}

So, from app I am communicating with the Domain Model which has a AddRole() method that will first map using automapper to the equivalent Data Model and then call Add() of repo method to persist the record.

This way my BLL is becoming a little dependent on AutoMapper and DAL but my DAL seems independent. I am trying to create just a simple layering for a simple yet well maintained project. Please suggest on its problems and improvements.

To start off, I have no idea what a SoC.DAL.Role is. Your namespaces are short abbreviations. This is quite hard to follow for people not really into your project.

Your role can add itself to a repository. This is not behaviour that I would expect. I would have a simple repository class (RoleManager?) that is capable of adding roles.

Role role = new Role(...);
new RoleManager().AddRole(role);

As such, you have 4 layers. One data access layer, one model layer, one domain layer and one GUI layer.


Combined looping and recursion when implementing Quicksort, using proper Clojure style In "The Joy of Clojure", 2nd edition, code for quicksort is introduced on page 133 that I found hard to digest. I have rewritten it to make it clearer (at least to myself).

Here is the original:

(defn sort-parts-joc [work]
  (lazy-seq
    (loop [[part & parts] work]
      (if-let [[pivot & xs] (seq part)]
        (let [smaller? #(< % pivot)]
          (recur (list*
                  (filter smaller? xs)
                  pivot
                  (remove smaller? xs)
                  parts)))
        (when-let [[x & parts] parts]
            (cons x (sort-parts-joc parts)))))))

Here is the rewritten code:

(defn sort-parts-explicit-lazy [work]
   (lazy-seq
      (loop [loopwork work]
         (let [[ part & partz ] loopwork ]
            (if-let [[pivot & valuez] (seq part)]
                  (let [ smaller? #(< % pivot)
                         smz      (filter smaller? valuez)
                         lgz      (remove smaller? valuez)
                         nxxt     (list* smz pivot lgz partz) ]
                        (recur nxxt))
                  (if-let [[oldpivot & rightpartz] partz]
                          (cons oldpivot (sort-parts-explicit-lazy rightpartz))
                          []))))))

Testing:

(require '( clojure test ))
(clojure.test/is (is-sorted (sort-parts-explicit-lazy [[]])))
(clojure.test/is (is-sorted (sort-parts-explicit-lazy [[1]])))
(clojure.test/is (is-sorted (sort-parts-explicit-lazy [[1 2]])))
(clojure.test/is (is-sorted (sort-parts-explicit-lazy [[1 2 3]])))
(clojure.test/is (is-sorted (sort-parts-explicit-lazy [[3 2 1]])))
(clojure.test/is (is-sorted (sort-parts-explicit-lazy [[3 3 3]])))

So, it works.

Both functions are essentially the same.

However I have a bad feeling about this. The simultaneous use of recur and full recursion, with the full recursion path "leaving" the loop in its middle in the same function feels weird, like unstructured code dropping a good, fat GOTO.

Here is the expression tree for reference:

We are far from from beta-reduction here. The "reduction" can only be understood using explicit instruction flow semantics. Okay, reality meets platonic ideals.

Is the loop-recur/full-recursion supposed to look like this? Is it a style problem that can be improved?

We are far from from beta-reduction here. The "reduction" can only be
  understood using explicit instruction flow semantics.

No. You have chosen to interpret loop and recur in this way, but that is not how Clojure understands them.

recur is simply a flag for a tail-recursive call, functional as you like.
loop is a gloss on defining and applying a function.

We could define loop as a macro in terms of fn:

(defmacro loop [bindings & expressions]
  (let [[bindees values] (->> bindings
                              (partition 2)
                              (apply map vector))]
    `((fn ~bindees ~@expressions) ~@values)))

The real loop macro checks the syntax of bindings. I've not bothered. And it handles destructuring explicitly. I've left it for fn to do.


What to do with the Exception when you fail to close() a resource? I am happy to receive all recommendations for improvement.  But below I am mostly interested in a review of my handling of the exception thrown by the close() method of RandomAccessFile.

The FileNotFoundException thrown by the opening of a file (in the constructor of RandomAccessFile) is declared as thrown by my method, because the caller can act on it.

But the exception thrown by the close() method on that file cannot be handled the caller, so I repackage it in a RuntimeException.

private boolean includeArticle(File articleFile, Map<String, String> conditionMap)
  throws FileNotFoundException
{
  if (doesFileQualifyAsArticle(articleFile)) {
    /* This can throw a FileNotFoundException, which the caller should know about. */
    RandomAccessFile raFile = new RandomAccessFile(articleFile, "r");
    if (this.isToInclude(raFile, conditionMap)) {
      return true;
    }
    try {
      raFile.close();
    } catch (IOException e) {
      throw new RuntimeException(e);
    }
  }
  return false;
}

At least in code that I've seen, it is very common to ignore exceptions when closing a resource. In fact Apache Commons IO has a number of closeQuietly functions just for this purpose.

Also, the work being done with raFile should be wrapped in a try, with the close/closeQuietly being called in the finally. This ensures that close/closeQuietly is called even if an exception occurs while working with the file.

RandomAccessFile raFile = new RandomAccessFile(articleFile, "r");
try {
  if (this.isToInclude(raFile, conditionMap)) {
    return true;
  }
}
finally {
  IOUtils.closeQuietly(raFile);
}

Also, if you are using Java 7, you might want to check out the new try-with-resources statement.


Counting occurrences of duplicate items in an array I have to make a simple program that reads N integers from the user and counts the occurrences of duplicate items. I know that the chosen sorting algorithm has poor performance, but I chose it for its simplicity.

I'm looking for suggestions for the last part, in which I count the duplicates.

#include <stdio.h>

#define N 10

int main(void) {
    int i, values[N];
    puts("Insert the values:");
    for (i = 0; i < N; i++) {
        scanf("%d", &values[i]);
    }

    /* Sorting */
    int j, tmp, minvalue, minindex;
    for (i = 0; i < N; i++) {
        tmp = values[i];
        minvalue = tmp;
        for (j = i + 1; j < N; j++) {
            if (values[j] < minvalue) {
                minvalue = values[j];
                minindex = j;
            }
        }
        values[i] = minvalue;
        values[minindex] = tmp;
    }

    printf("Array is: ");
    for (i = 0; i < N; i++) {
        printf("%d ", values[i]);
    }
    putchar('\n');

    /* Actual counting */
    int dupn = 0, duplicates[N], dupocc[N];
    for (i = 0; i < N; i++) {
        dupocc[i] = 0;
    }
    for (i = 1; i < N; i++) {
        if (values[i] == values[i - 1]) {
            duplicates[dupn] = values[i];
            if (dupocc[dupn] == 0) dupocc[dupn]++;
            dupocc[dupn]++;
        } else if (dupocc[dupn] > 0) dupn++;
    }
    for (i = 0; i < dupn; i++) {
        printf("Duplicate %d occurs %d times\n", duplicates[i], dupocc[i]);
    }
    return 0;
}

There are a few things that stand out to me immediately in your code. Some good, some are troubling.
Style
First up, your code is neat, and well structured. Indentation is good, and I even appreciate the fact that you have braced 1-liners and got the space right in your expressions.
Traditionally, C puts the open brace on the new line.... I am not a fan of that, but I don't write C very often, so I stick with a style I use in Java. Just thought I would point that out, though.
Sorting
Your sort algorithm is ... basic, but effective. I would agree that it is a problem. As a suggestion, you should use an insertion sort, and do the sort at the same time as the scanf. That way you load the data in an always-sorted way:
puts("Insert the values:");
for (i = 0; i < N; i++) {
    int val;
    scanf("%d", &val);
    int pos = i;
    while (pos > 0 && values[pos - 1] > val) {
        values[pos] = values[pos - 1];
        pos--;
    }
    values[pos] = val;
}

The above will do a shifting insertion sort. Just a suggestion.
Duplicates.
This is where there are the most significant problems. I would consider your algorithm to be a real overkill one. You can do it all with no extra data, no duplicates[] array, etc. A simple loop through the data should be fine.
int count = 1;
int previous = values[0] + 1; // something different to the start.
for (int i = 0; i < N; i++) {
    if (values[i] != previous) {
        // something changed... reset the counters
        if (count > 1) {
            // report the count of previous duplicates.
            printf("Duplicate %d occurs %d times\n", previous, count);
        }
        previous = values[i];
        count = 0;
    }
    count++;
}
if (count > 1) {
    // catch the last count, if any
    printf("Duplicate %d occurs %d times\n", previous, count);
}


Optimizing PowerShell string hasher method I am building a PowerShell class based tool that works with XML files of as much as a few hundred Kb size, and as many as a hundred of these files. I have extensive validation code that is time consuming, so I want to validate the XML and if it is valid hash it and write that hash as an attribute to the root element. My code can then hash on load, compare with that value and skip validation when possible. This is a pretty major speed improvement.
To that end, I have this static method of my xml class.
static [String] GetHash ([Xml.XmlDocument]$xmlDocument) {
    $stringBuilder = [Text.StringBuilder]::new()

    $hashBuilder = [Security.Cryptography.MD5CryptoServiceProvider]::new() #[Security.Cryptography.SHA384CryptoServiceProvider]
    $encoding = [Text.Encoding]::UTF8
    $xmlToHash = [Xml.XmlDocument]::new()
    $xmlToHash.AppendChild($xmlToHash.ImportNode($xmlDocument.DocumentElement, $true))
    $string = [pxXML]::ToString($xmlToHash)
    $hashIntermediate = $hashBuilder.ComputeHash($encoding.GetBytes($string))
    foreach ($intermediate in $hashIntermediate) {
        $stringBuilder.Append($intermediate.ToString('x2'))
    }

    return $stringBuilder.ToString()
}

As well as this static .ToString() method that is used by the hasher and also used for writing results to the console in testing.
static [String] ToString ([Xml]$Xml) {
    $toString = [System.Collections.Generic.List[String]]::new()

    $stringWriter = [IO.StringWriter]::new()
    $xmlWriter = [Xml.XmlTextWriter]::new($stringWriter)
    $xmlWriter.Formatting = "indented"
    $xmlWriter.Indentation = 4
    $Xml.WriteTo($xmlWriter)
    $xmlWriter.Flush()
    $stringWriter.Flush()
    $toString.Add($stringWriter.ToString())
    $toString.Add("`r" * 2)

    return [string]::Join("`n", $toString)
}

As you might notice from the commented part of the $hashBuilder line, I have tested this with different algorithms, and there is little difference in performance. Which is a bit of a surprise. So I wonder if there is something else her that I am missing, that is the the/an actual bottleneck? Or is this code actually reasonable close to optimal performance already? Given that I am totally new to classes and digging this deep into .NET, I suspect there is a bit to learn here beyond just optimizing this code.

In general I don't really see the value in not defining an XSD or DTD and validating against that when loading it with an XmlReader, which feels most correct. You should be able to instance one XmlSchemaSet and compile it. Then that can be repeatedly used to validate the documents without reloading the schema, and that should perform well. That feels like the most correct solution. If you want valid data the most correct thing to do is to validate it. That covers every situation where your goal is to have valid data.
What you're doing here just feels like a home grown XML digital signature, so your method just raises conceptual concerns to me. If I were to look at what you're doing, I'd be confused by what you were trying to accomplish.
If you just want to be able to verify what files you've previously validated, I'd prefer just maintaining a CSV file or database with the output of Get-FileHash and some file metadata. That seems much less complicated and doesn't require modifying the XML files.

However, taking the code as face value, you may see some performance improvement like this on the first method:
static [String] GetHash ([Xml.XmlDocument]$xmlDocument) {
    $stringBuilder = [System.Text.StringBuilder]::new(32)

    $hashBuilder = [System.Security.Cryptography.HashAlgorithm]::Create('md5')
    $xmlToHash = [Xml.XmlDocument]::new()
    $xmlToHash.AppendChild($xmlToHash.ImportNode($xmlDocument.DocumentElement, $true))
    $hashBuilder.ComputeHash([System.Text.StringBuilder]::UTF8.GetBytes([pxXML]::ToString($xmlToHash))).ForEach({
        $stringBuilder.Append($_.ToString('x2'))
    })

    return $stringBuilder.ToString()
}

When you know the length of the StringBuilder you can instance it with that initial capacity. MD5s are always 32 characters. That saves having to expand capacity.
The ComputeHash() method is an inherited method from the HashAlgorithm class. You don't gain anything by instancing a more complex child class because you don't use it for anything else. You're just calling a hash function; there's nothing inherently cryptographic about what you're doing.
The .ForEach({}) collection method is somewhat higher performant in most cases, although it makes for less readable code and is more obscure.
Eliminating variables that are only used once also tends to improve performance. Indeed, you could also just call [System.Security.Cryptography.HashAlgorithm]::Create('md5').ComputeHash(...), but it's getting a bit hard to read on StackOverflow.
Offhand, I'm not sure if $stringBuilder.AppendFormat('{0:x2}',$_) is better or worse than $stringBuilder.Append($_.ToString('x2')), but I believe the latter is better here.


Is this home page well organized and what can I do to improve performance here? My page has been loading a bit slower than I want it to.

The website is www.digitechlab.com

I segregated so many files because they were pretty big and I wanted to be able to work on just one section of that page at a time without running the risk of breaking the entire page. This way I could work on, say, the header on its own.

Here is what I have

<!DOCTYPE html>

  <title>DigiTech Dental Lab - Dental Lab Services- Digital Dental Restorations</title>
  <meta name="keywords" content="dental lab, dentist laboratory, dental restorations, digital dental, dental implants, dental bridges, implant system, implant abutments, Bruxzir, IPS.emax, Prismatik, cad/cam technology" />
  <meta name="description" content="DigiTech dental lab offers CAD/CAM technology and digital dentistry to provide quality dental restorations, crowns and bridges at a competitive price." />
  <meta name="google-site-verification" content="" />
  <meta name="y_key" content="" />
  <meta name="msvalidate.01" content="" />

  <link rel="stylesheet" href="/css/header.css" type="text/css" charset="utf-8" />
  <link rel="stylesheet" href="/css/font-museo500.css" type="text/css" charset="utf-8" />

CAD/CAM Digital Dental Restorations

At DigiTech Dental Restorations, we take great pride in offering you quality products at a competitive price. Our dedicated team uses state-of-the-art CAD/CAM processes and equipment to fabricate each restoration, ensuring each crown or bridge is created with an unsurpassed degree of precision. The DigiTech staff is specially trained in monolithic BruxZir material, and our experience is evident in every BruxZir case you prescribe. From crowns &amp; bridges to Inclusive&reg; Implant Abutments, DigiTech is committed to both dentist and patient satisfaction.

            Schedule a pickup

            888-336-1301

your structure looks good.

I am sure you have a reason for nesting so many
s

you know that you  can add classes to Tags as well, and you don't have to add them to only
 tags you can add them to other tags.

it just looks like you are surrounding some tags with Div tags because you think you can only style them that way.

like this

CAD/CAM Digital Dental Restorations

you can change to this

 CAD/CAM Digital Dental Restorations

or

CAD/CAM Digital Dental Restorations


Show/Hide button for a search field I have made a form where the submit button will show if the text box is focused and the button will hide if the text box is blurred.

These codes work fine. But I wonder if the jQuery code could be more simplified. Or is there any toggle function available?

var search = $("#search");
var button = $("#submit-button");
search.focus(function(){
    button.slideDown("slow");
});
search.blur(function(){
   button.slideUp("slow");
});
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
<form action="" method="get">
    <div>
        <input type="text" name="search" title="search" class="search" id="search" placeholder="Enter text here"/>
    </div>
    <button type="submit" class="submit-button" id="submit-button" name="submit" style="display: none">Click to Search</button>
</form>

Note: I can do it with click function. But I want to do it with focus and blur

Chaining is the power way in jQuery:

var button = $("#submit-button");
$("#search").focus(function(){
    button.slideDown("slow");
}).blur(function(){
   button.slideUp("slow");
});


A JS Tree Viewer in VueJS To learn Vue.js I have made a tree viewer with the code below. Can anyone give me some advice on how to improve the code?

  <tree v-bind:nodes="nodes"></tree>

This looks similar to the VueJS documentation Tree View example but it is different because of the unique selected aspect.

I would recommend not modifying the class names of elements based on the id, but instead use class bindings. So for the <node>:

What is selectedId? If you look at the example below, you will see I added a property to the <tree> and <node> components, and set that to a new item in the data of the root app.

var app = new Vue({
  el: "#app",
  data: {
    nodes: [/*...*/],
    selectedId: ''
  },
  methods: {
    setSelectedId: function(selectedId) {
      this.selectedId = selectedId;
    }
  }
});

Originally I was going to add properties to both the <tree> and <node> components and use traditional parent-child communication techniques but it got a little cumbersome with emitting from the nested children. Then I decided to use an Event bus to set the selected id (see Non Parent-Child Communication). One could use $root from the <node> component and call the method directly but some believe that goes against the traditional parent-child communication flow.

Also notice the code below uses the shorthand syntax for v-bind.

var selectedNode;
var bus = new Vue();
Vue.component("tree", {
  template: `

       <node
          :node="typeof node === 'string' ? node : Object.keys(node)[0]"
          :nestingTree="typeof node === 'string' ? undefined : Object.values(node)[0]"
          :id="index + '/' + (nestingLevel || 0)"
          :nestingLevel="nestingLevel || 0"
          :selected-id="selectedId"

        ></node>

  `,
  props: ["nodes", "nestingLevel", "selectedId"],
});
Vue.component("node", {
  template: `

        {{node}} {{
          nestingTree
            ? open ? "[-]" : "[+]"
            : ""}}

      <tree v-if="nestingTree && open" :nodes="nestingTree" :nestingLevel="nestingLevel + 1" :selected-id="selectedId" ></tree>

  `,
  props: ["node", "nestingTree", "nestingLevel", "id", "selectedId"],
  data: function() {
    return {
      open: false
    }
  },
  methods: {
    onClick: function(event) {
      this.toggleOpenClosedState(event);
      bus.$emit('setSelectedId', this.id);
    },
    toggleOpenClosedState: function(event) {
      this.open = !this.open;
    }
  }
});
var app = new Vue({
  el: "#app",
  data: {
    nodes: [
      "dfadsfa",
      "fkjsdafkj", {
        carrots: [
          "caveats", {
            moreNesting: [
              "bwah bwah bwah"
            ]
          }
        ]
      }
    ],
    selectedId: ''
  },
  mounted: function() {
      bus.$on('setSelectedId', this.setSelectedId);
  },
  methods: {
    setSelectedId: function(selectedId) {
      this.selectedId = selectedId;
    }
  }
});
.selected {
  font-weight: bold;
}

  <tree :nodes="nodes" :selected-id="selectedId"></tree>


Rotate image 90 degree clockwise * Time complexity: O(N)
* Memory complexity: \$O(1)\$

Is my calculation of complexity correct for this code?

#include <iostream>
#include <algorithm>
#include <vector>

using namespace std;

using Vector = vector<vector<int>>;

int main()
{
    const size_t SIZE = 7;

    Vector image(SIZE, vector<int>(SIZE));

    cout << "******* original image ******" << endl;
    int value = 0;
    for (auto& i : image)
    {
        for (auto& j : i)
        {
            cout << (j = value) << ' ';
        }

        value++;
        cout << '\n';
    }

    // rotate image 90 degree clockwise O(N)time & O(1)memory ???
    for (size_t i = 0; i < image.size(); ++i)
    {
        for (size_t j = 0; j < (image.size() - i); ++j)
        {
            swap(image[i][j], image[image.size() - j - 1][image.size() - i - 1]);
        }
    }

    cout << "\n******* rotated image ******" << endl;
    for (const auto& i : image)
    {
        for (const auto& j : i)
        {
            cout << j << ' ';
        }

        cout << '\n';
    }
}


Control game ticks/frames of updates and rendering separately I am working on a bigger project and want to separate game updates from rendering and not end up with a super complicated game loop. So I came up with the idea of isolating the problem and writing a controller for this. My game loop looks like:

public class Game {
    public bool Running;

    public void Loop()
    {
        var update = new FrameController(10, 30, true, Update);
        var render = new FrameController(1, 1000, false, Render);

        while (Running)
        {
            update.TryFrame();
            render.TryFrame();
        }
    }

    public void Update(double delta_time)
    {
        [...]
    }

    public void Render(double delta_time)
    {
        [...]
    }
}

And my tick/frame controller looks like:

public class FrameController
{
    protected double DeltaTime;
    protected readonly double MaxDeltaTime;
    protected readonly double MinDeltaTime;
    protected readonly bool SlowCatchUp;
    protected readonly Action<double> Action;
    protected readonly Stopwatch Watch;

    public FrameController(int min_fps, int max_fps, bool slow_catch_up, Action<double> action)
        : this(slow_catch_up, action)
    {
        MaxDeltaTime = 1.0 / min_fps;
        MinDeltaTime = 1.0 / max_fps;
    }

    public FrameController(double min_delta_time, double max_delta_time, bool slow_catch_up, Action<double> action)
        : this(slow_catch_up, action)
    {
        MaxDeltaTime = min_delta_time;
        MinDeltaTime = max_delta_time;
    }

    protected FrameController(bool slow_catch_up, Action<double> action)
    {
        SlowCatchUp = slow_catch_up;
        Action = action;
        Watch = new Stopwatch();
        Watch.Start();
    }

    public void TryFrame()
    {
        var delta_time = Watch.Elapsed.TotalSeconds;
        Watch.Restart();
        DeltaTime += delta_time;

        if (DeltaTime <= MinDeltaTime)
            // Skip
            return;
        else if (DeltaTime >= MaxDeltaTime)
            DoFrame(SlowCatchUp ? MaxDeltaTime : DeltaTime);
        else
            DoFrame(DeltaTime);
    }

    protected void DoFrame(double delta_time)
    {
        Action(delta_time);
        DeltaTime -= delta_time;
    }
}

The updates will fire 10 to 30 times per second and if the game hangs they will slowly catch up to the current state. Apart from this the rendering will fire 1 to 1000 times per second (just for testing) and if the game hangs they will catch up immediately. If the game is running to fast frame will be skipped.

Any input and critique is welcome!

I think sourcing the game-loop out is basically a good idea to improve separation of concerns. But I would go farther: I would implement a strategy pattern which makes the behavior of the game loop exchangeable. Futhermore I would source out the loop itself, too.

public interface IGameLoop{
    void Start();
    void Stop();
}
public interface IUpdateBahvior
{
    bool TryUpdate();
}
public interface IRenderBehavior
{
    bool TryRender();
}

Example implementation:

class SynchronGameLoop : IGameLoop
{
    private bool _isRunning;
    private readonly IUpdateBehavior _updateBehavior;
    private readonly IRenderBehavior _renderBehavior;

    public SynchronGameLoop(IUpdateBehavior updateBehavior, IRenderBehavior renderBehavior){
        _updateBehavior = updateBehavior;
        _renderBehavior = IRenderBehavior ;
    }

    public void Start(){
        if(_isRunning) return;
        while(_isRunning){
            _updateBehavior.TryUpdate();
            _renderBehavior.TryRender();
        }
    }
    public void Stop()
    {
        _running = false;
    }
}

class AsyncGameLoop : IGameLoop ...

class StatisticProvidingGameLoop: IGameLopp ...

This system offers you the possibility to exchange the game loop behavior at run-time. So If you decide to implement a game loop that offers statistic information(update count, update time, render count) for debugging purpose, you free to exchange this, without altering your code.

The separation into two *behavior interface provide the possibility to implement completely independent strategies, for this two functionality. But if you wish to, one class can implement both interfaces and can make the functionality dependent on each other. So while implementing your engine, you don't define what kind of game loop and update/render behavior is finally used. For example in XNA, you can switch between fixed time steps and variable one. All this is easily possible with the strategy pattern.


A simple drawer using CSS3 and jQuery I've made a simple drawer using CSS3 and jQuery that works with hover and click. It seems to be working fine but surely there's a simpler way of writing the code.

Fiddle: http://jsfiddle.net/73MpU/

$("#bottom-page").click(function () {
    $("#top-page").addClass("push");
});

$("#bottom-page").mouseover(function () {
    $("#top-page").addClass("nudge-right");
});

$("#bottom-page").mouseleave(function () {
    $("#top-page").removeClass("nudge-right");
});

$("#top-page").click(function () {
    $(this).removeClass("push");
    $(this).removeClass("nudge-left");
});

$("#top-page").mouseover(function () {
    if ($(this).hasClass("push")) {
        $(this).addClass("nudge-left");
    }
});

$("#top-page").mouseleave(function () {
    if ($(this).hasClass("push")) {
        $(this).removeClass("nudge-left");
    }
});

So, I simplified my code a bit. Is this valid?

Edit:

$("#bottom-page").hover(function () {
    $("#top-page:not(.push)").toggleClass("nudge-right");
});

$("#bottom-page").click(function () {
    $("#top-page:not(.push)").removeClass("nudge-right");
    $("#top-page:not(.push)").addClass("push");
});

$("#top-page").hover(function () {
    if ($(this).hasClass("push")) {
        $(this).toggleClass("nudge-left");
    }
});

$("#top-page").click(function () {
    if ($(this).hasClass("push")) {
        $(this).removeClass("push nudge-left");
    }
});

You could try to group the toggleClass inside one .hover, like this:

var $top=$("#top-page");             //save #top-page in $top var

$("#bottom-page").click(function () {
    $top.addClass("push");
}).hover(function(){                 //chain .hover to #bottom-page
    $top.toggleClass("nudge-right");
    if ($top.hasClass("push")) {     //if $top has the .push class then toggle .nudge-left
        $top.toggleClass("nudge-left");
    }
});

$top.click(function () {             //on $top click remove the .push and .nudege-left class
    $top.removeClass("push nudge-left");
});

http://jsfiddle.net/73MpU/1/


Fastest way to get a value from csv file using java 7 My code picks a value from a too large csv file (~300MB.) at a given index i and j (assuming the csv as 2D array because it contains just data without headers), it works but it is slow.
Is there other method to pick a value from csv file faster?

import java.io.BufferedReader;
import java.io.InputStreamReader;
import java.io.FileInputStream;
import java.io.File;
import java.io.IOException;
import java.util.List;
import java.util.ArrayList;

public class Main
{

    public static String r1(String input,int i, int j,int bufferSize){

        String[][] result = {};
        String row ="";

        try {
            BufferedReader r1 = new BufferedReader(new InputStreamReader(new FileInputStream(new File(input)), "UTF-8"), bufferSize);

            List<String[]> lines = new ArrayList<String[]>();

            while((row=r1.readLine())!=null){
                lines.add(row.split(","));
            }

            String[][] csv = new String[lines.size()][0];
            lines.toArray(csv);
            result = csv;
            r1.close();
        }

        catch(IOException e){
            e.printStackTrace();
        }
        return result[i][j];
    }

    public static void main(String[] args){
        int bufferSize = 10 * 1024;   //10 KB.
        String filename = "matrix.csv";
        int i = 744, j = 7000;
        String pickedValue = r1(filename,i,j,bufferSize);
        System.out.println(pickedValue);
    }

}

Spaces

Your use of spaces could stand improvement.  You should have one space after every comma in argument list, and around binary operators.  It is also customary to have a space between a closing parenthesis and an opening brace.

Names

Use descriptive names!

Is r1 a method name, or a BufferedReader?  Why are these named the same, anyway?

What is input?  It is a String, that much is clear, but what is it used for?  filename would be so much more descriptive than input.

Try-with-resources

If an IOException happens, is FileInputStream closed, or does it bypass the r1.close() statement?

Java 7 introduced the "try-with-resources" construct, which ensures resources are properly closed, even in the event of exceptions, and even if exceptions happen during the exception handling.

try (FileInputStream fis = new FileInputStream(new File(input)),
         InputStreamReader isr = new InputStreamReader(fir, "UTF-8"),
         BufferedReader br = new BufferedReader(isr)) {

    // use br here - see below

    // br, isr, and fis are automatically closed here, at the end of the try block.
} catch (IOException e) {
    e.printStackTrace();
}

Efficiency

You are doing a lot of unnecessary work, wasting time, to extract a single value from the file.

If you want the value from the first line, you still read in all of the lines
You split every line into columns, even when you don't need a value from that line.
You store all of the lines of data, split into columns, as an ArrayList
You store a second copy of of all of the lines of data, as a String[][]

In place of the // use br here - see below line, you could do the following:

Read in lines without storing them, until you reach the i-th row.
Split the i-th row into columns
Return the j-th value

Ie)

for(int row = 0;  row < i;  row++) {
    br.readLine();                    // Note: value is not saved
}

String line = br.readLine();
String[] columns = line.split(",");
return columns[j];                    // Resources are automatically closed!

Remove the original declaration of result, row, and lines.

  What about bufferSize, should I never use it?

It was an oversight on my part when writing the try-with-resources replacement, but in general, no.  The JVM will pick a buffer size that is optimal for the OS.

If you did want to specify a buffer size, 10KiB seems too arbitrary.  It should be related to (a multiple of) the size of the data being read, in this case, the length of the lines in the csv file.  The larger the value, the lower the odds that readline() will have to do string concatenation because the part of the line is in one buffer & part of the line is in another.  However, the larger the value, the higher the odds that too much data will be read (you want to stop at exactly the right line, remember?), memory pressure will increase causing other inefficiencies.  If you do try tweaking the value, profile, profile, profile (... and then go back to the default).


Find the missing number in a array I'm working on a personal project to practice my programming skills and I came to this problem. I tried to create a program that will find the missing number in a array.

I'm looking forward to upgrading this program and am trying to find the missing numbers in a array. Yes, with an 's'.

/********************************
*A sample program that will solve
*for a missing integer in a given sorted array.
********************************/

public class MissingInteger{

  public static void PrintArray(){
    int []fArrayDuplicate = {1,2,3,4,5,7,8,9,10};
    System.out.println("\nGiven the incomplete array:");
    System.out.print("[");
      for(int a = 0 ; a<fArrayDuplicate.length ; a++)
        System.out.print(" " + fArrayDuplicate[a] + " ");
    System.out.print("]");
    }

  public static void FindMissing(){
    int []finalArray = {1,2,3,4,5,7,8,9,10};
    int len = finalArray.length; //First, I took the length of the array.
    int sum = 0;
      for(int x=0;x<len;x++)
         sum+=finalArray[x]; //Second, I took the sum of the given array.
    int totalNumber = ((len + 1) * (len + 2)) / 2; //Third, I used this formula to get the total number.
    int missingInt = totalNumber - sum; // Fourth, I subtract the sum to total number to get missing integer.
    System.out.println("\nTherefore, the missing integer is " + missingInt + ".");
 }

  public static void main(String[] args){
      PrintArray();
      FindMissing();
    }
}

In Java, the convention wants that methods are written in camelCase. Which means PrintArray should be printArray and so on.

The indentation is a little off with the declaration of the int[]. Instead of this :

int []fArrayDuplicate

It should be like this

int[] fArrayDuplicate

It might look like nitpicking (and it might be!) but it improves readability by a ton.

Talking about nitpicking, I think your for loop could use some spacing :

for(int x=0;x<len;x++)

could be

for (int x = 0; x < len; x++)

When you comment your code, you should explain why you wrote what you did write, not what you wrote. I mean,

int len = finalArray.length; //First, I took the length of the array.

is pretty obvious, so the comment isn't neccessary. I think it is the same for all your comments, except the third. But what I need to understand, as a developper who reads your code, isn't that you used a formula to get the total, but to understand how your formula works so you can get the total.

Both your methods use the same int[], which makes me think it should be a parameter of both your methods, this way you can be sure they will never be different.

public static void main(String[] args){

    int[] fArrayDuplicate = {1,2,3,4,5,7,8,9,10};
    PrintArray(fArrayDuplicate);
    FindMissing(fArrayDuplicate);
}

I also think you should pull out the main method of this class and put it in a class that is used only to start the application with the good parameters. Doing so, you could use your MissingInteger class to only find missing integer. This way, if you ever want to reuse it, you will be able to without bringing the main method with it.

Also, you could make your MissingInteger class respect the OOP more. You could input the int[] in the constructor like so :

public class MissingInteger {

    private int[] fArrayDuplicate;

    public MissingInteger(int[] fArrayDuplicate){

        this.fArrayDuplicate = fArrayDuplicate;
    }

    public void PrintArray(){

        System.out.println("\nGiven the incomplete array:");
        System.out.print("[");
        for(int a = 0 ; a<fArrayDuplicate.length ; a++)
            System.out.print(" " + fArrayDuplicate[a] + " ");
        System.out.print("]");
    }

    public void FindMissing(){

        int len = fArrayDuplicate.length; //First, I took the length of the array.
        int sum = 0;
        for(int x=0;x<len;x++)
            sum+=fArrayDuplicate[x]; //Second, I took the sum of the given array.
        int totalNumber = ((len + 1) * (len + 2)) / 2; //Third, I used this formula to get the total number.
        int missingInt = totalNumber - sum; // Fourth, I subtract the sum to total number to get missing integer.
        System.out.println("\nTherefore, the missing integer is " + missingInt + ".");
    }
}

I think you might want to rename your class to MissingIntegerFinder since well, the class itself isn't about a missing integer, but about finding a missing integer. And the name fArrayDuplicate doesn't mean much to me. There's nothing talking about duplicates in your code, why is it prefixed with a f? I have a hard time to find a better name, someone might find one. But for now I'd name it integersWithOneMissing or... something like that.


Limit an integer to certain number of digits and suffix '+' I am having a problem describing what the following code does. Below is my attempt at its javadoc.

/**
 * Convert an integer so that the notion of its size fits within a certain number digits.
 * e.g.
 * Fit 123 to 2 digits -> 99+
 * @param in The integer
 * @param order The order of magnitude to fit it in.
 * @return The 'countified' String.
 */
public static String countify(int in, int order) {
    final int max = (int) Math.pow(10, order);

    if (in >= max) {
        return String.valueOf(max - 1) + "+";
    } else  {
        return String.valueOf(in);
    }
}

The idea is simple enough. For any value greater than 10^order it returns 10^order - 1 plus a + (e.g. 123, with order 2 becomes 99+), else it returns the number itself. How do I better describe  it? Thanks!

Use less powerful language constructs

You use if and else

if (in >= max) {
    return String.valueOf(max - 1) + "+";
} else  {
    return String.valueOf(in);
}

Using if and else duplicates return String.valueOf

Here the ternary operator comes to the rescue. It looks hard but really is very easy to use.

condition ? a : b

If condition is true, than a, else b. As easy as that.

You can now write return String.valueOf just one time:

return String.valueOf(in >= max ? (max - 1) + "+" : in)


Implementation of itoa() How can this solution be improved?

#include <limits.h>

void itoa(int n, char s[])
{
    int min_int = 0;
    int i, sign;
    if (INT_MIN == n) {
        min_int = 1;
        n++;
    }

    if ((sign = n) < 0)
        n = -n;
    i = 0;
    do {
        s[i++] = n % 10 + '0';
    } while ((n /= 10) > 0);

    if (sign < 0)
        s[i++] = '-';

    s[i] = '\0';

    if (min_int)
        s[0]++;

    reverse(s);
}

I think I'd break this up into a few separate routines, each with a single more specific intent. I think the easiest way to do this is probably to start by taking the absolute value of the input (represented as an unsigned type), something like this:

unsigned iabs(int input) {
    if (input >= 0)
        return (unsigned)input;
    return (UINT_MAX - (unsigned)input)+1U;
}

I believe C's requirements for converting a signed to unsigned ensure that this produce correct results for all inputs (including INT_MIN).

With that, nearly everything else can deal with an unsigned number. Personally, I also prefer to generate the result in order instead of generating in reverse, then reversing it. One fairly simple way to do that is with recursion, with each step first generating previous numerals (if any), then adding its own numeral to the end of the string. I'd have the recursive function deal only with an unsigned type (as produced above). Of course, like nearly everything that manipulates data in a buffer in C, you should also pass the maximum buffer length for it to use, to prevent buffer overruns. For the sake of generality, I've also made it accept the base for the conversion as a parameter instead of always assuming base 10.

char *itoa_internal(char *buffer, size_t len, unsigned input, int base) {
    static const char digits[] = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";
    char *pos = buffer;
    if (input >= base)
        pos = itoa_internal(buffer, len, input/base, base);

    if (pos-buffer < len-1)
        *pos++ = digits[input % base];
    return pos;
}

Then I'd have a small wrapper function that puts in a - if the input is negative, invokes the function above with the absolute value of the input, and terminates the string after the internal function returns.

char *itoa(char *buffer, size_t len, int input, int base) {
    char *pos = buffer;

    if (base < 2 || base > 36 || len < 1)
        return NULL;

    if (input < 0)
        *pos++ = '-';

    pos = itoa_internal(pos, len, iabs(input), base);
    *pos = '\0';
    return buffer;
}

This should also make it pretty trivial to write a utoa that handles unsigned inputs -- it would be like itoa without the code to deal with negative numbers. As a minor refinement, I'd probably also make iabs and itoa_internal static functions to minimize the chances of them interfering with other names.


Mnenomic Password Generator I have this code that creates unique passwords using the first letter of each word from the file. Before each password is created (written to a file) it is compared to all passwords that are currently in the file, so if the file has 50,000 passwords before a another is written it has to scan through all 50k.
A user can input any amount of passwords needed but the bigger the number the longer it takes. 100k will take a long time, so how can I optimize this to make the program run faster? (the password generation is not included code)

for mainLoop in range(passnum):

      user  = 0
      newpass = generatePassword() # New password generated each iteration of loop
      storePass = open("MnemPass.txt","a")
      print ("PASSWORD GENERATED ")

      #Checks if file is empty if True write first password
      fileEmpty = os.stat("MnemPass.txt").st_size == 0
      if fileEmpty == True:
          storePass.write(newpass)
          storePass.write("\n")
          storePass.close()
          print ("FIRST PASSWORD WRITTEN")

      #IF file is not empty Read line by line and compare each with new password generated returns boolean value
      elif  fileEmpty == False:
            storePass = open("MnemPass.txt","r")
            with open("MnemPass.txt") as f:
                fileData = f.read().splitlines()
                linelength = len(fileData).__int__()
                filemax = linelength
                num = linelength    #Number used to cycle through array set to size of list
                #print(linelength+10)

                for iterate in range(linelength):
                    num = num - 1 #Number decreases each pass
                    #print num
                    if fileData[num] != newpass: # The last element in the array is checked first decrementing each pass
                       go = True

                    if fileData[num]==newpass: #changed
                        print ("match found: PASSWORD : "+fileData[num])
                        passMatch = open("Matchpassword.txt","a")
                        passMatch.write(newpass)
                        passMatch.write("\n")
                        passMatch.close()
                        go = False
                        break

                    #places new password once it does not match and all elements prev passwords are compared
                    if go == True and num == 0:
                      storePass = open("MnemPass.txt","a")
                      storePass.write(newpass)
                      storePass.write("\n")
                      storePass.close()
                      print ("NEW WRITTEN")

            if linelength == filemax:

                num = num -1

user is redundant in your code, it's instantiated but never used at all so it should be removed. Another more wasteful redundancy is printing all the time, that actually slows down the code a surprising amount. You should strip them out. At most print once per loop and for exceptional circumstances (like errors).

Instead of checking if the file is empty it would be better test the lines in it and then afterwards write the password to the file. This way you write the password in the same way whether or not the file is empty, as it will just immediately find that the password isn't in the file anyway.

For the main body, you're reading in all the lines, then looping backwards over them by index to test them backwards. Is there a reason you're doing it backwards? I can't tell why you would do that. There's a lot of problems here, I would actually just start from scratch.

So, you need to test whether a password exists in the file, right? I'd make this a function first of all. So the function needs to take the password as an argument.

def exists(password):

Now we need to open the file and read each line in the file. Rather than read all the lines up front, it's much easier to read lines one by one. Python makes this incredibly easy, letting you iterate directly through the file, returning one line at a time.

    with open("MnemPass.txt") as f:
        for line in f:

Now, we can just test if the password is there and return True if it does, as this indicates that the password already exists.

            if password == line.strip():
                return True

I added .strip() to remove whitespace that might mess with the test. And then of course, if the whole file is read without this problem then you eventually reach the end of the function where you return False, indicating that it does not exist.

def exists(password):
    with open("MnemPass.txt") as f:
        for line in f:
            if password == line.strip():
                return True
    return False

Now. when we go back to the original code this makes things immensely tidier:

for mainLoop in range(passnum):
    newpass = generatePassword() # New password generated each iteration of loop
    if exists(newpass): #changed
        with open("Matchpassword.txt", "a") as passMatch:
            passMatch.write(newpass + "\n")
    else:
        with open("MnemPass.txt", "a") as storePass:
            storePass.write(newpass + "\n")

Now, this can be done even faster. If you were to go back to the function, you could use the any operator to lake the for loop and turn it into a generator expression. Generator expressions are basically like for loops condensed into a single expression. And if you pass one into any it can run a test on every element of a loop. It's faster than normal Python loops. So here's how it'd look:

def exists(password):
    with open("MnemPass.txt") as f:
        return any(password == line.strip() for line in f)

This means that if any ever finds a line that equals password then it will return True, but if it finds no such line it will return False.

After this, optimisation really ought to be done with storage. You could sort the passwords alphabetically and then intelligently detect where they should be. Maybe store in multiple files, corresponding to the starting character of the password. There's only so much that the exact code optimisation itself can do.