Recursive function for listing directories The below code is recursive function that list directories in 2 levels:

static  Dictionary<string,string> pList=new Dictionary<string, string>();
static void ListDirec(string path, int start, int end)
{
    var dirInfo = new DirectoryInfo(path);
    var folders = dirInfo.GetDirectories().ToList();

    foreach (var item in folders)
    {
        Console.WriteLine("".PadLeft(start * 4, ' ') + item.Name);
        if (start < end) {
            ListDirec(item.FullName, start + 1, end);
        }
    }
}

Now how can optimize that in this case?

For example, I've these directories with their sub-directories:

folder-A
    folder-A-1
    folder-A-2
    folder-A-3
folder-B
    folder-B-1
    folder-B-2
    folder-B-3
folder-C
    folder-C-1
    folder-C-2
    folder-C-3

Now I want save parents as key and children as value in dictionary, with their relation for example folder-A has these children: folder-A-1, folder-A-2, folder-A-3 and so on.

What's the best way to do that in this function?

To relate your parent directories to their children, you could have plist declared like this:

static Dictionary<String, List<String>> plist = new Dictionary<string, List<string>>();

Each time you hit a child folder, you add its name to

Console.WriteLine("".PadLeft(start * 4, ' ') + item.Name);
string parent = path.Split('\\').Last();
if (!pList.Keys.Contains(parent))
    pList.Add(parent, new List<string>());
pList[parent].Add(item.Name);

Sorry, I don't have access to a C# compiler at the moment so I can't verify this 100%, but it's a starting point at least.

Edit: After thinking about it, I don't think C# likes it if you use [] before an element is in the Dictionary.  I edited the above code to include how to get the parent and included the Console.WriteLine so you know where I would put it.


Singly linked list in Scala I've written a basic implementation of a singly linked list in Scala. I would appreciate any feedback and improvements (especially the remove() method, I feel like it could be implemented better).

Node class

class Node[T](data: T, var next: Option[Node[T]] = None) {
  override def toString: String = data.toString
}

Singly linked list implementation:

class SinglyLinkedList[T](var head: Node[T], var size: Int = -1){
  if(size == -1) size = calculateSize

  private def calculateSize: Int = {
    @scala.annotation.tailrec
    def traverse(count: Int, curNode: Option[Node[T]]): Int = curNode match {
      case None => count
      case Some(node) => traverse(count + 1, node.next)
    }
    traverse(0, Some(head))
  }

  override def toString: String = {
    @scala.annotation.tailrec
    def traverse(sb: StringBuilder, curNode: Option[Node[T]]): String = curNode match {
      case None => sb.append("None").toString
      case Some(node) => traverse(sb append (node + " --> "), node.next)
    }
    traverse(new StringBuilder, Some(head))
  }

  def append(d: T): Unit = {
    @scala.annotation.tailrec
    def traverse(curNode: Option[Node[T]]): Unit = curNode.flatMap(_.next) match {
      case None => curNode foreach (_.next = Some(new Node[T](d)))
      case next => traverse(next)
    }
    traverse(Some(head))
    size += 1
  }

  def remove(index: Int): Unit = {
    @scala.annotation.tailrec
    def traverse(elementsRemaining: Int, prevNode: Option[Node[T]], curNode: Option[Node[T]]): Unit = (elementsRemaining, curNode) match {
      case (_, None) => throw new NoSuchElementException
      case (0, n) =>
        if(prevNode.isEmpty) head.next.foreach(head = _)
        else prevNode.foreach(_.next = n.flatMap(_.next))
      case (i, n) => traverse(i - 1, n, n.flatMap(_.next))
    }
    traverse(index, None, Some(head))
    size -= 1
  }
}

Deficiencies

What's this piecemeal approach to functional/imperative programming? It makes code difficult to understand. In a particular part of a program, try to stick to a particular paradigm (I can't believe I had to say that. Multiparadigm languages have come a long way!).

You seem to not have noticed the glory of import. The glorious import scala.annotation.tailrec at the start of SinglyLinkedList would make all the tail-recursion annotations in your methods to simply @tailrec.

Node[T] should ideally be a case class. That would give you proper, compiler-implemented equals, toString and hashCode methods, alongside having easy pattern matching support, and not requiring new to instantiate them. By the way, did you know that Some is a case class?

You seem to be using Option as a replacement for null. IMHO, it complicates your code. Just use null if you have to. You're implementing the singly-linked list in classic reference/pointer based way after all. Don't get me wrong here, using Option is definitely better than using null, but I think it overcomplicates your code.

Do take a look at cons lists. I guess that's what you actually want.

Finally, a reference based list is not idiomatic Scala. A cons list (as above) is. Try to use that, it's not too far from what you're currently doing (I do hope that you want to do it the functional programming way, as you have not stated otherwise). As this is already not idiomatic Scala, you could go the full imperative route with while loops and null, as Scala allows you to do so. Or you could go the functional route with full immutability and cons lists. The choice is yours, but in this case, I and most other Scala programmers would recommend you choose the latter option. But whatever you do, keep this Frankenstein's monster imperative/functional hybrid from most people.

Suggestions

For remove(Int):Int, an algorithm is to do it the normal, C (pointer-based way). Traverse the list maintaining references to 2 elements, the current element and it's previous element. At the element to be removed, set the next field of the previous reference to refer to the element following the current element. This would work far better with while loops than flatMap and forEach. Again, imperative and functional don't really mix well in the same part of the program.

Take a look here for a reasonable functional singly linked list in Scala.


Calculating a page size to execute a subset of jobs at a time We have to execute a bunch of jobs , each have an ID and they are passed on to the processor . Since the processor can only work on a limited number of jobs (denoted by deleteBatchSizeMax in the code snippet),we are using a paged approach so that only a subset is picked up by the processor.To implement this we have a functionality which calculates the Page Size. It takes in the total number of jobs, and the max page size. The code handles the situation where the page size may not be an exact multiple , example 22 items have a deleteBatchSizeMax of 10. This translates to 22/10  so it gives us a page size of 3 where the jobs are executed in chunks of 10,10,2.
Hence wrote the below described function!
    private int CalculatePageSize(int totalNumberOfJobs, int deleteBatchSizeMax)
=> totalNumberOfJobs % deleteBatchSizeMax != 0
            ? (totalNumberOfJobs / deleteBatchSizeMax) + 1
            : totalNumberOfJobs / deleteBatchSizeMax;

I am hearing complaints that apparently its not readable and I should do the if-else block.  I think that the variable names are well defined , code is well commented (omitted here) and self explanatory.
I wanted to know the feedback of the community.

There are a few ways which this method could be made more readable.

The first if-statement is negated, making the control flow slightly more complicated; the statement can be negated (again) to clarify the flow:

private int CalculatePageSize(int totalNumberOfJobs, int deleteBatchSizeMax)
            => totalNumberOfJobs % deleteBatchSizeMax == 0
            ?  totalNumberOfJobs / deleteBatchSizeMax
            : (totalNumberOfJobs / deleteBatchSizeMax) + 1;

Secondly, the statement is currently checking if totalNumberOfJobs is an even divisible of deleteBatchSizeMax. The computation totalNumberOfJobs / deleteBatchSizeMax is used three times. When (double)totalNumberOfJobs / (double)deleteBatchSizeMax doesn't contain a decimal place, then it implies totalNumberOfJobs % deleteBatchSizeMax == 0, and vice-versa.

To simplify the pattern, consider the following: when A divides B with no remainder, then divide it, otherwise divide it and add one. Therefore, Math.Ceiling can be used:

private int CalculatePageSize(int totalNumberOfJobs, int deleteBatchSizeMax)
    => (int)Math.Ceiling((double)totalNumberOfJobs / (double)deleteBatchSizeMax);

The Math.Ceiling function "rounds" up to positive infinity; so 4.5 -> 5, 4.00001 -> 5 and so on. This emulates the original loop condition.

Edit: the second double conversion is not required as it is upcasted to a double.

private int CalculatePageSize(int totalNumberOfJobs, int deleteBatchSizeMax)
    => (int)Math.Ceiling((double)totalNumberOfJobs / deleteBatchSizeMax);


Remove multiple array elements by indices I prepared a method to make possible to select elements of the array except for some indexes.

def array_except(array, *indexes)
  indexes =
    indexes
      .flat_map { |e| e.is_a?(Range) ? e.to_a : e }
      .map { |e| e.negative? ? e + array.length : e }

  array.values_at(*((0...array.length).to_a - indexes))
end

It works in such a way:

> a = ["a", "b", "c", "d", "e", "f", "g", "h"]
array_except(a, 1, -1, 2..3, -4..-2)
=> ["a"]
> array_except(a, 1, 2)
=> ["a", "d", "e", "f", "g", "h"]
> array_except(a, -2..-1)
=> ["a", "b", "c", "d", "e", "f"]

I feel like it might be done with a lower level of complexity.
I spent a day playing with #each_with_object([]) but can't find the proper way.
Maybe someone has any thoughts?

The main concern with this implementation is not its complexity but the fact that you transform ranges into arrays of indices. It works fine for basic cases, but what if the range is huge? For example,

array_except(a, 1..100000000)

works with the significant delay on my laptop already, and infinite range will obviously cause your implementation to never terminate.

But you don't need to iterate over all the numbers within the range to check if the index belongs to it, right? It's enough to just check the boundaries. So, my suggestion is

def array_except(array, *indexes)
  array.reject.with_index do |_, i|
    indexes.any? do |index|
      index.is_a?(Range) ? index.cover?(i) : index == i
    end
  end
end

pry(main)> array_except(a, 1..Float::INFINITY) # => ["a"]

UPD. Please, note: it doesn't work with the negative ranges as is, but can be adjusted if you decide to go in this direction - for example, via converting ranges with the negative boundaries into "equivalent" ones with the proper positive boundaries:

def array_except(array, *indexes)
  array.reject.with_index do |_, i|
    indexes.any? do |index|
      if index.is_a?(Range)
        start = index.begin > 0 ? index.begin : array.length + index.begin
        stop = index.end > 0 ? index.end : array.length + index.end
        start <= i && i <= stop
      else
        index == i
      end
    end
  end
end

The final version is more verbose than your initial implementation, but its performance doesn't depend on the size of the ranges provided as an input...

UPD2. If you don't care about the predictable performance and is just interested in a concise implementation, it can be made as short as

def array_except(array, *indexes)
  array.reject.with_index { |_, i| indexes.any? { |ind| Array(ind).include?(i) } }
end

UPD3. Good point raised in the comment. The second implementation works terribly bad in the cases when indexes parameter is a huge list of something (integers or ranges) applied to a large list. In this case we get stuck with O(m*n)... To beat this we need something more sophisticated. For example, we can first iterate over all indexes and fill some auxiliary data structure (with O(1) read access) with some "flags" (defining what to keep/delete) and then iterate over the array itself and clean it up. Very dirty implementation:

def array_except4(array, *indexes)
  to_delete = Array.new(array.size)

  indexes.each do |index|
    case index
    when Range
      start, stop = [index.begin, index.end].sort
      start = start < 0 ? [0, start + array.length].max : [start, array.length - 1].min
      stop = stop < 0 ? [stop + array.length, array.length - 1].min : [stop, array.length - 1].min

      (start..stop).each { |k| to_delete[k] = true }
    else
      to_delete[index] = true
    end
  end

  array.reject.with_index { |_, i| to_delete[i] }
end
```


Python class to manage a table in SQLite Module name:    database.py

Code:

import sqlite3

class Database(object):
    """sqlite3 database class that holds testers jobs"""
    DB_LOCATION = "/root/Documents/testerJobSearch/tester_db.sqlite"

    def __init__(self):
        """Initialize db class variables"""
        self.connection = sqlite3.connect(Database.DB_LOCATION)
        self.cur = self.connection.cursor()

    def close(self):
        """close sqlite3 connection"""
        self.connection.close()

    def execute(self, new_data):
        """execute a row of data to current cursor"""
        self.cur.execute(new_data)

    def executemany(self, many_new_data):
        """add many new data to database in one go"""
        self.create_table()
        self.cur.executemany('REPLACE INTO jobs VALUES(?, ?, ?, ?)', many_new_data)

    def create_table(self):
        """create a database table if it does not exist already"""
        self.cur.execute('''CREATE TABLE IF NOT EXISTS jobs(title text, \
                                                            job_id integer PRIMARY KEY,
                                                            company text,
                                                            age integer)''')

    def commit(self):
        """commit changes to database"""
        self.connection.commit()

I have a few questions here:

I named my python module as database.py and my class as Database, which is perhaps not the best way, as they are too generic and somewhat less informative and they are almost identical. Is there a better way to name them?
I exposed the database connection and database cur as instance variables. Is that a good practice?

I exposed the database connection and database cur as instance variables. Is that a good practice?

Having a cursor defined as an instance variable is a common practice. The problem though here is that you don't have a way to close the cursor and the connection itself in case of an error. In situations like this, having "context manager" capabilities on your class would be quite handy:

def __enter__(self):
    return self

def __exit__(self, ext_type, exc_value, traceback):
    self.cursor.close()
    if isinstance(exc_value, Exception):
        self.connection.rollback()
    else:
        self.connection.commit()
    self.connection.close()

Then, the Database class could be used with with:

with Database() as db:
    db.create_table()


Repeatedly check URL for change on iOS I am wanting to constantly check a URL http://www.example.co.uk/untitled.php which will either return 1 or 0 on my app.

Is the most efficient way to do it by repeatedly requesting the URL every second like this?

Function to return content of URL

-(void)getOnline{
    if(connected==0){
        NSString *uuid = @"59xxxfb6d-4659-gdsf-sfsdf";

        NSString *URL = [NSString stringWithFormat:@"%@%@", @"http://www.example.co.uk/untitled.php?=", uuid];
        NSURLSession *aSession = [NSURLSession sessionWithConfiguration:[NSURLSessionConfiguration defaultSessionConfiguration]];
        [[aSession dataTaskWithURL:[NSURL URLWithString:URL] completionHandler:^(NSData *data, NSURLResponse *response, NSError *error) {
            if (((NSHTTPURLResponse *)response).statusCode == 200) {
                if (data) {
                    NSString *contentOfURL = [[NSString alloc] initWithData:data encoding:NSUTF8StringEncoding];
                    if([contentOfURL isEqual: @"1"]){
                        //do stuff
                    }
                }
            }
        }] resume];
    }
}

Function on load to repeatedly call the function above

[NSTimer scheduledTimerWithTimeInterval:1.0f target:self selector:@selector(getOnline) userInfo:nil repeats:YES];

If you want a UUID, generate one.  What you have isn't a UUID.  It's just a string of random numbers.

If you want to reuse a once-generated UUID multiple times, you can do that.

static NSString * const uuid = [NSUUID UUID].UUIDString;

If you're calling a variable URL, it should actually be a NSURL object and not an NSString object.

And nesting the method call that actually wraps it into an NSURL doesn't do anything but reduce readability.

So perhaps, we really want something like this:

static NSString * const uuid = [NSUUID UUID].UUIDString;
static NSString * urlString = [NSString stringWithFormat:@"%@%@", @"http://www.example.co.uk/untitled.php?=", uuid];
static NSURL * url = [NSURL URLWithString:URL];

Finally, we have a problem with our timer here.

Here's the timer code you show:

[NSTimer scheduledTimerWithTimeInterval:1.0f target:self selector:@selector(getOnline) userInfo:nil repeats:YES];

The problem is that this timer repeats, and it's firing an asynchronous task.

Now, I'd certainly look into JavaProphet's comment about long polling versus sending repeated requests, but I do want to address the issue you have here.

What happens if it takes the second more than one second to reply to a request?  More importantly, what if the server replies to three requests in a row in under one second, the fourth takes over two seconds, and the fifth goes back to taking less than a second?

We can easily get the responses out of order because we're not waiting around for the server to tell us.

Rather than a repeating timer, we need a timer that is rescheduled after every fire.  We don't want to ask the server every single second.  We simply want to wait for one second since the last poll before we ask again.

Any time we're using a repeating timer, we must stop and answer this question:

  Do we really want this event to fire every , or do we actually want this event to fire  since the end of the last event?

Importantly, when we pick the latter, it makes it impossible for two (or several more) of the events to be going at the same time.  So when we stop the timer, the most amount of asynchronous events that could be out in some background waiting to come back is just one.  Using your approach, the number of events that could still fire back after we invalidate the timer is only a function of however long the timeout is.

If the server suddenly stopped responding and you had a 5 minute time out, and you waited until the first "The server timed out" response before you stopped the timer, there'd be 5 minutes-worth of these events still off in a background thread waiting for the server's reply, so you'd get hit with the "The server timed out" response 300 times before it was actually fully stopped.


Batch program to xcopy from host PC to remote destination with multi-processing This is a batch program to xcopy from host PC to remote destination with multi-processing.

Kepler, Python-3.x are my environment.

#!/usr/bin/env python
# -*- coding: utf-8 -*-
# based on Carnival http://ask.python.kr/users/6970/carnival/

import os
import os.path
import csv
import re

from multiprocessing import Process, Lock

class Server:
    def __init__(self, addr, path):
        self.addr = addr
        self.path = path

def distribute_file(server_list, pathname):
    print("Read from {}".format(pathname))

    with open(pathname, 'rb') as inFile:
        buffer = inFile.read()

    for server in server_list:
        remotepath = "//%s/%s/%s" % (server.addr, server.path, pathname)
        print ("Write to {}".format(remotepath))
        with open(remotepath, 'wb') as outFile:
            outFile.write(buffer)

def multi_distribute_file(lo, server, dirpath, filename, path):
    lo.acquire()

    pathname = os.path.join(dirpath, filename)
    print("Read from {}".format(pathname))

    with open(pathname, 'rb') as inFile:
        buffer = inFile.read()

    if(path == ''):
        remotepath = "//%s/%s/%s" % (server.addr, server.path, filename)
    else:
        remotepath = "//%s/%s/%s/%s" % (server.addr, server.path, path, filename)

    print ("Write to {}".format(remotepath))
    with open(remotepath, 'wb') as outFile:
        outFile.write(buffer)

    lo.release()

def make_dir(server_list, path):
    for server in server_list:
        dirpath = "//%s/%s/%s" % (server.addr, server.path, path)
        d = os.path.dirname(dirpath)
        if not os.path.exists(d):
            os.makedirs(d)
            print ("Make dir {}".format(d))
        else:
            print ("dir {} already exists.".format(d))

        if not os.path.exists(dirpath):
            os.makedirs(dirpath)
            print ("Make dir {}".format(dirpath))
        else:
            print ("dir {} already exists.".format(dirpath))

def dist_files(server_list, subdir):
    for dirpath, dirnames, filenames in os.walk(subdir):

        l = re.findall(r"[\w']+",subdir)
        m = re.findall(r"[\w']+",dirpath)
        path = ''
        if(l != m):
            path = m[len(m)-1]

        make_dir(server_list, path)
        for filename in filenames:
            for server in server_list:
                lock = Lock()
                Process(target=multi_distribute_file,args=(lock, server, dirpath, filename, path)).start()

def get_server_list(filename):
    mydictionary = []
    csvFile = csv.reader(open(filename, "r"))
    for row in csvFile:
      mydictionary.append(Server(row[0], row[1]))
      print("{}, {}".format(row[0], row[1]))

    return mydictionary

if __name__ == '__main__':
    server_list = get_server_list('client_list.csv')
    dist_files(server_list, 'C:\\Users\\Public\\Test')

client_list.csv

  192.168.10.100, Test
  192.168.10.101, Test
  192.168.10.102, Test

Technical issues:

You are reading whole files into memory at once. Consider what happens if the directory contains huge files.
You use multiprocessing, apparently in an attempt to parallelize things, and use locks to synchronize.
First, why do you need a lock anyway? Which data do you want to protect? The only shared data are in the filesystems, which could be changed by other processes at any time anyway.
Next, each call to multi_distribute_file gets its own lock instance, that is only used once by this call. So there is no synchronization happening at all.
On the other hand, if you would use a single lock for all calls to multi_distribute_file, the processes will execute one after another, as each acquires the lock for its entire runtime. Your parallelizing efforts would be in vain.
You are spawning a new process for each file being copied. This can create a huge number of processes very quickly. Consider what this will do to your system performance.
If something goes wrong in multi_distribute_file and raises an exception, the lock will never be released. And things will go wrong occassionaly as you are communicating over a network. Use a with statement instead.
Don't use string manipulation (format and re) to manipulate paths. Use os.path instead. This is saver and more reliable.

Style issues:

There are no docstrings and no comments (apart from the header)
Avoid single-letter and other abbreviated names like d, l, m, and lo.
mydictionary is a meaningless and misleading name. Meaningless, as it doesn't tell us anything about the purpose of the variable, and misleading as it is actually a list, not a dictionary. A better name might be server_list (like you do elsewhere).


Encoding on Dropdownlist items I am currently doing Encoding on Dropdownlist in our project.

Could you please review and provide your comments about the following approach?  After the encoding on dropdown, if there are any special characters it is displaying in the form of encoded characters, please let me know is there any better approach than this.

// raising the databound event to to encoding
protected void ddlCountry_DataBound(object sender, EventArgs e)
{
    if (ddlCountry.Items.Count > 0)
    {

        foreach (ListItem list in ddlCountry.Items)
        {
            list.Text = EncodeDropDownItem(list.Text);
        }
    }
}

//below are the methods to do encoding
//encoding with antixss's htmlencode method
private string EncodeDropDownItem(string DropdownText)
{
    return Replacecharacters(AntiXss.HtmlEncode(DropdownText));
}

//below method will replace the antixsstags with normal character.
private string Replacecharacters(string value)
{
    string dropdowntext;
    StringBuilder sb = new StringBuilder(value);

    sb.Replace("&#38;", "&");
    sb.Replace("&#60;", "<");
    sb.Replace("&#62;", ">");
    sb.Replace("&#34;", "\"");
    sb.Replace("&#92;", "\\");
    return dropdowntext = sb.ToString();
}

A few comments -

It seems that while your code should work fine as is, it could be improved in the following ways:

A. It seems like you have three methods here when only one or two would be necessary. You may have your reasons for doing this, but the "middle-man" method "EncodeDropDownItem" seems superfluous.

B. The way you are hard-coding the calls to sb.replace, one at a time and with target/replacement values "inlined" might be more effective if they were preformed within a looping structure, with the Axss tags and replacement values retrieved from a custom dictionary. The hard-coded Axss tags and related character representations could then be encapsulated within that custom dictionary class, and re-used in other contexts if necessary.

You may have your reasons for structuring your methods the way you have. However, I make the below suggestions for your code. Note that I am not able to test this, but hopefully you or some other forum readers will catch anything I screw up here . . .

FIRST - a custom dictionary for the Axss tags/values:

    class AntiAxssDictionary : Dictionary<String, String>
    {
        public void AntiAxssDictionary()
        {
            this.Add("&#38;", "&");
            this.Add("&#60;", "<");
            this.Add("&#62;", ">");
            this.Add("&#34;", "\"");
            this.Add("&#92;", "\\");
        }
    }

THEN, a minor rearrangement of your methods (this might be a matter of taste, or function, but this is how I would do it, given what little I know about your code):

    // raising the databound event to to encoding
    protected void ddlCountry_DataBound(object sender, EventArgs e)
    {
        if (ddlCountry.Items.Count > 0)
        {
            //Initialize an AntiAxssDictionary object:
            AntiAxssDictionary tokenProvider = new AntiAxssDictionary();

            foreach (ListItem list in ddlCountry.Items)
            {
                // Encode the AntiAxss string here:
                StringBuilder sb = new StringBuilder(AntiXss.HtmlEncode(list.Text));

                // Iterate through the Axss tags stored as keys in the dictionary:
                foreach (String token in tokenProvider.Keys)
                {
                    // Attempt a replacement for each of the possible
                    // values in the Axss dictionary:
                    sb.Replace(token, tokenProvider[token]);
                }

                // Assign the new value to the list.text property:
                list.Text = sb.ToString;
            }
        }
    }

Hopefully I didn't butcher anything too badly here. I am in the midst of migrating from vb.net to C#, and learning Java at the same time. Hope that helps, and I will be interested to hear commentary and critique from other forum members.


NewSequentialId - pure net core 2.2 implementation of sql server function i'm currently trying to rewrite sql server newSequentialId() function in pure c# that is performing as fast as possible and eats minimum memory.

Question is can I it be done better - current benchamark shows it is at most 2x slower than native function used by SqlServer (p/invoke)?

using System;
using System.Buffers;
using System.Linq;
using System.Net.NetworkInformation;
using System.Threading;

namespace PerfBenchmarkDotNet
{
    internal static class FastGuid
    {
        private static readonly byte[] macBytes;
        private static long baseDateTicks;

        private static long ClockSequenceNumber = 0;

        static FastGuid()
        {
            macBytes = GetDefaultMacAdress();
            baseDateTicks = new DateTime(1582, 10, 15, 0, 0, 0, DateTimeKind.Utc).Ticks;
        }

        public static Guid NewGuid()
        {
            var guidArray = ArrayPool<byte>.Shared.Rent(16);
            var nowTicks = DateTime.UtcNow.Ticks;
            var sequenceNumber = Interlocked.Increment(ref ClockSequenceNumber) % 65536;

            var ticksDiff = nowTicks - baseDateTicks;
            var ticksDiffBytes = BitConverter.GetBytes(ticksDiff);

            // Copy the bytes into the guid

            // mac
            guidArray[15] = macBytes[5];
            guidArray[14] = macBytes[4];
            guidArray[13] = macBytes[3];
            guidArray[12] = macBytes[2];
            guidArray[11] = macBytes[1];
            guidArray[10] = macBytes[0];

            // sequence number
            guidArray[8] = (byte)(sequenceNumber >> 8);
            guidArray[9] = (byte)(sequenceNumber);

            // time
            guidArray[7] = ticksDiffBytes[0];
            guidArray[6] = ticksDiffBytes[1];
            guidArray[5] = ticksDiffBytes[2];
            guidArray[4] = ticksDiffBytes[3];
            guidArray[3] = ticksDiffBytes[4];
            guidArray[2] = ticksDiffBytes[5];
            guidArray[1] = ticksDiffBytes[6];
            guidArray[0] = ticksDiffBytes[7];

            var guid = new Guid(guidArray);

            ArrayPool<byte>.Shared.Return(guidArray);

            return guid;
        }

        private static byte[] GetDefaultMacAdress()
        {
            var nic = NetworkInterface.GetAllNetworkInterfaces().FirstOrDefault();

            if (nic != null)
            {
                return nic.GetPhysicalAddress().GetAddressBytes();
            }

            var fallback = Guid.NewGuid().ToByteArray();
            return fallback.Take(6).ToArray();
        }
    }
}

You want to reduce both computation time and memory allocation. Memory allocation itself takes time, so that's a good place to start -- try to find all the places where you're allocating memory and see if they can be avoided.

ArrayPool<byte>.Shared.Rent(16); can avoid an allocation, but not always. Can we do better? Turns out we can; we'll see how.

BitConverter.GetBytes(ticksDiff) makes an allocation. ticksDiff is just an integer so it's quite easy to extract its binary representation using binary operators. We'll see how we can use this later on.

But first:

Profiling

Profilers are your friend! You should always start the optimization journey by asking "Where are the bottlenecks?", which you can promptly answer with cold hard data from a profiling tool.

As suspected, dotTrace reports that the ArrayPool<byte>.Shared.Rent/Return and BitConverter.GetBytes(ticksDiff) calls take a significant chunk (~21%) of the total running time. Let's get rid of those.

Guid constructor

You're using the Guid constructor that takes a byte[], which has perpetuated an implementation that necessarily allocates a byte array somewhere. You can get clever with this to allocate on the stack instead of the heap (Span<byte> with stackalloc) or to minimize the number of allocations that actually need to happen (ThreadLocal<byte[]>), but the best solution is to not allocate anything at all. To accomplish this, you can use one of the other Guid constructors that takes its values directly, instead of taking a byte array.

There's a constructor overload that takes an int, two shorts, and eight bytes. You can pack these values directly (see the reference source for endianness on the int and shorts):

int a = ticksDiffBytes[7] | (ticksDiffBytes[6] << 8) | (ticksDiffBytes[5] << 16) | (ticksDiffBytes[4] << 24);
short b = (short)(ticksDiffBytes[3] | (ticksDiffBytes[2] << 8));
short c = (short)(ticksDiffBytes[1] | (ticksDiffBytes[0] << 8));

var guid = new Guid(
    a,
    b,
    c,
    (byte)(sequenceNumber >> 8),
    (byte)(sequenceNumber),
    macBytes[0],
    macBytes[1],
    macBytes[2],
    macBytes[3],
    macBytes[4],
    macBytes[5]);

Binary stuff

You can extract bits directly from ticksDiff to avoid the call to BitConverter.GetBytes:

var ticksDiff = nowTicks - baseDateTicks;

int a = (int)(
    ((ticksDiff >> 56) & 0xFF) |
    ((ticksDiff & 0x00FF000000000000) >> 40) |
    ((ticksDiff & 0x0000FF0000000000) >> 24) |
    ((ticksDiff & 0x000000FF00000000) >> 8));

short b = (short)(
    ((ticksDiff & 0x00000000FF000000) >> 24) |
    ((ticksDiff & 0x0000000000FF0000) >> 8));

short c = (short)(
    ((ticksDiff & 0x000000000000FF00) >> 8) |
    ((ticksDiff & 0x00000000000000FF) << 8));

If you can tolerate a change in endianness, the following is a lot cleaner to read (but provides no measurable change in runtime for me):

var ticksDiff = nowTicks - baseDateTicks;

int a = (int)(ticksDiff);
short b = (short)(ticksDiff >> 32);
short c = (short)(ticksDiff >> 48);

Remainder

You don't need to do a remainder when computing sequenceNumber because we're extracting the remainder bits directly anyhow when we pass (byte)(sequenceNumber >> 8), (byte)(sequenceNumber) to the Guid constructor.

My numbers below make it hard to conclude if this helps at all. This is the kind of thing that might be optimized away by the jitter anyway.

Benchmarks

With these changes, my benchmarks look as follows:

|                                    Method |     Mean |   Error |  StdDev | Ratio |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------------------------------------ |---------:|--------:|--------:|------:|-------:|------:|------:|----------:|
|                          NewGuid_Original | 224.2 ns | 4.37 ns | 4.49 ns |  1.00 | 0.0041 |     - |     - |      20 B |
|                  NewGuid_WithPackedValues | 153.6 ns | 2.51 ns | 2.35 ns |  1.00 | 0.0041 |     - |     - |      20 B |
| NewGuid_WithPackedValuesAndNoBitConverter | 148.6 ns | 2.43 ns | 2.28 ns |  1.00 |      - |     - |     - |         - |
|                NewGuid_WithReorderedBytes | 148.9 ns | 2.92 ns | 3.00 ns |  1.00 |      - |     - |     - |         - |
|  NewGuid_WithReorderedBytesAndNoRemainder | 146.4 ns | 2.11 ns | 1.98 ns |  1.00 |      - |     - |     - |         - |

Final code:

public static Guid NewGuid_WithBinaryOptimizationsAndWithoutRemainder()
{
    var nowTicks = DateTime.UtcNow.Ticks;
    var sequenceNumber = Interlocked.Increment(ref ClockSequenceNumber);

    var ticksDiff = nowTicks - baseDateTicks;

    int a = (int)(ticksDiff);
    short b = (short)(ticksDiff >> 32);
    short c = (short)(ticksDiff >> 48);

    var guid = new Guid(
        a,
        b,
        c,
        (byte)(sequenceNumber >> 8),
        (byte)(sequenceNumber),
        macBytes[0],
        macBytes[1],
        macBytes[2],
        macBytes[3],
        macBytes[4],
        macBytes[5]);

    return guid;
}


single array or many single argument to call a constructor with params? I am seeing it more and more.

Which do you think works best and why?

This:

$config = array(
'param1'=>'abc'
'param2'=>'dfg'
);

new Class($config);

Or:

new Class('abc','dfg');

I'd always use the second version, since one could use type-hinting. The array style is less transparent.

At least the array style should read like this:

$config = array(
 Class::PARAM_1_INFO =>'abc'
 Class::PARAM_1_SOMETHING =>'dfg'
);

This way, Class is the single point, where the keys get defined.


Inter Thread Communication Even Odd Example I have implemented a simple example for inter thread communication which have 2 threads to print even and odd numbers in sequence.

I am looking for a feedback and points for improving the example.

My code looks like:

public class InterThreadCommunicationEvenOddExample
{
    public static void main(String[] args)
    {
        NumberResource numberResource = new NumberResource();
        new EvenThread(numberResource);
        new OddThread(numberResource);
    }
}

class EvenThread implements Runnable
{

    NumberResource numberResource;

    EvenThread(final NumberResource numberResource)
    {
        this.numberResource = numberResource;
        new Thread(this, "Even").start();
    }

    public void run()
    {
        while (true)
        {
            System.out.println("Even Thread::" + this.numberResource.getNextEven());
            try
            {
                Thread.sleep(1000);
            }
            catch (InterruptedException e)
            {

            }
        }
    }

}

class OddThread implements Runnable
{

    NumberResource numberResource;

    OddThread(final NumberResource numberResource)
    {
        this.numberResource = numberResource;
        new Thread(this, "Odd").start();
    }

    public void run()
    {
        while (true)
        {
            System.out.println("Odd Thread::" + this.numberResource.getNextOdd());
            try
            {
                Thread.sleep(1000);
            }
            catch (InterruptedException e)
            {

            }
        }
    }
}

class NumberResource
{

    private int number = 0;

    public synchronized int getNextEven()
    {
        while (isEven(number))
        {
            try
            {
                wait();
            }
            catch (InterruptedException e)
            {

            }

        }
        number++;
        notify();
        return number;
    }

    public synchronized int getNextOdd()
    {
        while (!isEven(number))
        {
            try
            {
                wait();
            }
            catch (InterruptedException e)
            {

            }

        }
        number++;
        notify();
        return number;
    }

    private boolean isEven(int number)
    {
        return number % 2 == 0;
    }

}

There is some refactoring that could be made.

Naming

EvenThread and OddThread aren't really threads since they don't implement the Thread class. There are two options:

Rename the two classes to something more appropriate/fitting with its purpose

e.g., EvenRunner/OddRunner

Move the thread management to the InterThreadCommunicationEvenOddExample.main method. This is more standard.

Redundancy

OddThread and EvenThread have almost the same functionality; a NumberThread could be created that paramaterizes the parity (e.g., a Parity enum parameter). This would be ideal since the logic in run() would only need to be changed once if needed.
The above point could be applied to getNextOdd and getNextEven in a similar manner.

Modifiers

Some variables could use tighter modifiers to better convey their purpose.

NumberResource numberResource could be final in all of its occurrences.
private boolean isEven(int number) could be made static since it is constant with respect to each NumberResource instance; furthermore, a semantic argument could be made that isEven(number) is not asking whether the NumberResource instance is even but rather asking if the provided number is even.

Threads

There is no need to call Thread.sleep() inside EvenThread.run() or OddThread.run(), since there is a wait() and notify() statement in the NumberResource's block. A single thread at a time will "wake up" from the wait() call after notify() is called in another thread, so removing the Thread.sleep() calls won't burn CPU cycles.
If the Thread.sleep() call is there for the sole purpose of text not flashing quickly onto the screen, then that is fine. Though moving the Thread.sleep() into the NumberResource method(s), before the notify() call, would cause text to be printed more evenly.

Empty catch statements

Either log something here or write a comment explaining why no action is taken here.
Allowing getNextOdd/getNextEven to throw an InterruptedException exception might even be cleaner.
A javadoc string for each method/class could clarify this as well.

Spacing

Overall, the spacing is fine and consistent, but 3 empty lines between each class method seems like too much, but this is subjective.

It is interesting to note that the overall output order of the messages is not guaranteed since notify() is called inside of getNextOdd/getNextEven; however, it is guaranteed that each Even Thread::{num} message will be strictly increasing (by 2). The same goes for each Odd Thread::{num} message.
Below is an example of some of the refactoring comments made above (except adding Javadocs).

public class InterThreadCommunicationEvenOddExample {
    public static void main(String[] args) throws InterruptedException {
        final NumberResource numberResource = new NumberResource();
        final Thread[] numberThreads = new Thread[] {
            new Thread(new NumberRunner(numberResource, Parity.EVEN)),
            new Thread(new NumberRunner(numberResource, Parity.ODD)),
        };

        for (Thread numberThread : numberThreads) {
            numberThread.start();
        }

        for (Thread numberThread : numberThreads) {
            numberThread.join();
        }
    }
}

class NumberResource {
    private int number = 0;

    public synchronized int getNextNumber(Parity parity) throws InterruptedException {
        while (getParity(number) != parity) {
            wait();
        }
        final int nextNumber = number++;
        notify();
        return nextNumber;
    }

    private static Parity getParity(final int number) {
        return number % 2 == 0 ? Parity.EVEN : Parity.ODD;
    }
}

class NumberRunner implements Runnable {
    private final NumberResource numberResource;
    private final Parity parity;

    NumberRunner(final NumberResource numberResource, final Parity parity) {
        this.numberResource = numberResource;
        this.parity = parity;
    }

    @Override
    public void run() {
        while (true) {
            try {
                System.out.printf("%s Thread::%d\n", parity.name(), numberResource.getNextNumber(parity));
            } catch (InterruptedException e) {
                System.out.println("Interrupted while trying to retrieve next number.");
                return;
            }
        }
    }
}

enum Parity {
    EVEN,
    ODD,
}


Quadrilaterals containing a square number of lattice points I am working on Project Euler problem 504, which states:

  Let \$ABCD\$ be a quadrilateral whose vertices are lattice points lying on the coordinate axes as follows:

  \$A(a, 0), B(0, b), C(−c, 0), D(0, −d)\$ where \$1 ≤ a, b, c, d ≤ m\$ and \$a, b, c, d, m\$ are integers.

  It can be shown that for \$m = 4\$ there are exactly 256 valid ways to construct \$ABCD\$. Of these 256 quadrilaterals, 42 of them strictly contain a square number of lattice points.

  How many quadrilaterals \$ABCD\$ strictly contain a square number of lattice points for \$m = 100\$?

My code is working fine, but the issue is optimization. I am already using memoization.

from fractions import gcd
from math import fabs
def PolygonArea(corners):
    n = len(corners)
    area = 0.0
    for i in range(n):
        j = (i + 1) % n
        area += corners[i][0] * corners[j][1]
        area -= corners[j][0] * corners[i][1]
    area = abs(area) / 2.0
    return area

def isSquare(n):
    sqrt = n ** 0.5
    if sqrt == int(sqrt):
        return True
    return False

memo = {}
def lattice(p1, p2):
    t = (fabs(p2[0] - p1[0]), fabs(p2[1] - p1[1]))
    g = gcd(t[0], t[1]) + 1
    if not t in memo:
        memo[t] = g
    return memo[t]

def boundary(p1, p2, p3, p4):
    return (lattice(p1, p2) + lattice(p2, p3) + lattice(p3, p4) + lattice(p4, p1) - 4)

def interior(p1, p2, p3, p4):
    corners = [p1, p2, p3, p4]
    A = PolygonArea(corners)
    B = boundary(p1, p2, p3, p4)
    I = A - B/2 + 1
    return I

squares = set([x**2 for x in xrange(1000)])
count = 0
for a in xrange(1, 101):
    for b in xrange(1, 101):
        for c in xrange(1, 101):
            for d in xrange(1, 101):
                I = interior((a, 0),(0, b),(-1*c, 0),(0, -1*d))
                if I in squares:
                    count += 1
print count

1. Review

There are no docstrings. What do all these functions do? How do I call them? What do they return?
The code is not portable to Python 3 because it uses xrange and the print statement.
The function isSquare is not used and could be removed.
The function lattice does not actually make use of the memoization (it doesn't look at the dictionary memo until it has already computed the result).
There's no need to use math.fabs as the vertices are all at integer points. You could use abs instead.
The program is hard to test because it has code that runs at the top level. This means that you can't run it from the interactive interpreter, or using timeit. It would be better to organize the code into functions to make it easier to test.
It's inconsistent for the function PolygonArea to have a name using CamelCase but other functions (lattice, boundary, interior) to have names using lower case.
The nested loops can be replaced by a single loop using itertools.product.
In order to work on the performance, it would be handy if the parameter \$m=100\$ were not hard-coded. We'd like to be able to run the code with smaller values of \$m\$ and see how the runtime behaves.

2. Revised code

from fractions import gcd
from itertools import product

def polygon_area(corners):
    """Return the area of the polygon with the given vertices."""
    n = len(corners)
    area = 0.0
    for i in range(n):
        j = (i + 1) % n
        area += corners[i][0] * corners[j][1]
        area -= corners[j][0] * corners[i][1]
    area = abs(area) / 2.0
    return area

_memo = {}
def lattice(p1, p2):
    """Return the number of lattice points on the line from p1 to p2."""
    t = abs(p2[0] - p1[0]), abs(p2[1] - p1[1])
    if t not in _memo:
        g = gcd(t[0], t[1]) + 1
        _memo[t] = g
    return _memo[t]

def boundary(p1, p2, p3, p4):
    """Return the number of lattice points on the boundary of the
    quadrilateral with vertices p1, p2, p3, p4.

    """
    return lattice(p1, p2) + lattice(p2, p3) + lattice(p3, p4) + lattice(p4, p1) - 4

def interior(p1, p2, p3, p4):
    """Return the number of lattice points in the interior of the
    quadrilateral with vertices p1, p2, p3, p4.

    """
    corners = [p1, p2, p3, p4]
    A = polygon_area(corners)
    B = boundary(p1, p2, p3, p4)
    I = A - B/2 + 1
    return I

def problem504(m=100):
    """Return the number of quadrilaterals with vertices lying at lattice
    points on the coordinate axes, no more than m units from the
    origin, whose interior contains a square number of lattice points.

    """
    squares = set(i ** 2 for i in range(2 * m))
    count = 0
    for a, b, c, d in product(range(1, m + 1), repeat=4):
        I = interior((a, 0), (0, b), (-c, 0), (0, -d))
        if I in squares:
            count += 1
    return count

3. Measurement

We expect the runtime to be proportional to \$m^4\$, so let's see how this performs on smaller values of \$m\$ and then fit a curve to figure out the projected runtime for \$m = 100\$. Note that because lattice is memoized, we have to be careful to reset the memoization each time.

>>> from timeit import timeit
>>> _memo={}; timeit(lambda:problem504(10), number=1)
0.10902838699985296
>>> _memo={}; timeit(lambda:problem504(20), number=1)
1.7153344419784844
>>> _memo={}; timeit(lambda:problem504(30), number=1)
8.644499261979945
>>> _memo={}; timeit(lambda:problem504(40), number=1)
28.569149894057773

Let's fit these results to the equation \$t = am^4\$:

Gnuplot finds \$a = 1.11 × 10^{-5}\$, so we predict that when \$m=100\$, the runtime will be about 1110 seconds (about 19 minutes).

4. Performance improvement

The code here is general-purpose — for example polygon_area computes the area of any polygon. But in this problem we don't need the general case — we only have quadrilaterals whose vertices are at lattice points on the coordinate axes. So the obvious thing to do is to reduce the amount of work by only computing the bare minimum, and reusing as many results as we can.

The key observation is that we can compute the number of interior points in the quadrilateral from the number of interior points in the four right-angled triangles in the four quadrants, like this:

If I write \$T(a, b)\$ for the number of interior points in the triangle with vertices \$(0, 0)\$, \$(a, 0)\$ and \$(0, b)\$, then this diagram shows that the value we want is $$Q(a, b, c, d) = T(a, b) + T(b, c) + T(c, d) + T(d, a) + a + b + c + d - 3.$$ The value of \$T(a, b)\$ can be computed in various ways, for example using Pick's theorem. I like to use this diagram:

in which there are \$(a - 1)(b - 1)\$ dots altogether, of which \$\gcd(a, b) - 1\$ lie exactly on the hypotenuse (the grey dot), and the remainder divide into two equal halves (the blue and red dots), so $$T(a, b) = {(a - 1)(b - 1) - \gcd(a, b) + 1 \over 2}.$$ In this example, \$a=10\$ and \$b=12\$ so \$\gcd(a, b) = 2\$ and \$T(a, b) = 49\$.

There are now some micro-optimizations we can make:

Let \$U(a, b) = T(a, b) + a\$. Then $$Q(a, b, c, d) = U(a, b) + U(b, c) + U(c, d) + U(d, a) - 3$$ which avoids four additions.
Instead of creating a set of squares, create a set of squares plus 3. Then we can test \$U(a, b) + U(b, c) + U(c, d) + U(d, a)\$ to see if it is a square plus 3. This saves a subtraction.
Instead of maintaining a set of squares plus 3, create a list whose \$i\$th entry is True if \$i\$ is a square plus 3, and False otherwise. Fetching a list entry is faster than determining if an item is in a set.

That results in the following code:

def problem504a(m=100):
    """Return the number of quadrilaterals with vertices lying at lattice
    points on the coordinate axes, no more than m units from the
    origin, whose interior contains a square number of lattice points.

    """
    square_plus_three = [False] * (4 * m * m + 3)
    for i in range(2 * m):
        square_plus_three[i * i + 3] = True
    R = range(m + 1)
    U = [[((a - 1) * (b - 1) - gcd(a, b) + 1) // 2 + a for a in R] for b in R]
    count = 0
    for a, b, c, d in product(range(1, m + 1), repeat=4):
        if square_plus_three[U[a][b] + U[b][c] + U[c][d] + U[d][a]]:
            count += 1
    return count

After all those micro-optimizations, we had better check that we haven't made a mistake. So let's test the new code against the old code:

>>> all(problem504(i) == problem504a(i) for i in range(20))
True

And see what the performance is like:

>>> timeit(lambda:problem504a(10), number=1)
0.007875622948631644
>>> timeit(lambda:problem504a(20), number=1)
0.09821957419626415
>>> timeit(lambda:problem504a(30), number=1)
0.49982553208246827
>>> timeit(lambda:problem504a(40), number=1)
1.6008274049963802
>>> timeit(lambda:problem504a(50), number=1)
4.018431055126712

The runtime is still \$Θ(m^4)\$ so here's the best fit to \$t = am^4\$ as before:

Gnuplot finds \$a = 6.4 × 10^{-7}\$ and so we predict that when \$m=100\$, the runtime will be about 64 seconds. In practice it takes a little longer than that:

>>> timeit(lambda:problem504a(100), number=1)
69.0101949761156


Login using VBScript I was tasked with re-writing a login VBScript we use on about 50 machines. The original script was hacked together by someone who clearly had no idea what they were doing (multiple lines that literally did nothing, including creating persistent shares, deleting them, then recreating them 4+ times in a ForEach loop)

I don't have any scripting experience in vbs, so I'm assuming there's a good amount of stuff here that should be cleaned up. The ultimate goal is to create up to three mapped drives (P:, Q:, R:) that point at registers in the location. Every site should have an identical set up other than the first two Consts, for ease of re-use.

Option Explicit

Const STORE_NUMBER = "29"
Const NUM_REGS = 3

Const EVENT_SUCCESS = 0
Const EVENT_FAIL = 1

Class Mapping
    Public strLocalDrive
    Public strUNCPath
    Public strPersistent
    Public strUsr
    Public strPas
End Class

Dim alMappings : Set alMappings = CreateObject("System.Collections.ArrayList")

Dim objNetwork : Set objNetwork = WScript.CreateObject("WScript.Network")
Dim oShell     : Set oShell = CreateObject("WScript.Shell")

' ## Mapping Objects ##

' POS 1
If NUM_REGS >= 1 Then
    Dim udtPOSOne : Set udtPOSOne = New Mapping
    With udtPOSOne
         .strLocalDrive = "P:"
         .strUNCPath = "\\10.0." & STORE_NUMBER & ".101\dpalm"
         .strPersistent = "False"
         .strUsr = "somename"
         .strPas = "somepass"
    End With
    alMappings.add(udtPOSOne)
End If

' POS 2
If NUM_REGS >= 2 Then
    Dim udtPOSTwo : Set udtPOSTwo = New Mapping
    With udtPOSTwo
         .strLocalDrive = "Q:"
         .strUNCPath = "\\10.0." & STORE_NUMBER & ".102\dpalm"
         .strPersistent = "False"
         .strUsr = "somename"
         .strPas = "somepass"
    End With
    alMappings.add(udtPOSTwo)
End If

' POS 3
If NUM_REGS >= 3 Then
    Dim udtPOSThree : Set udtPOSThree = New Mapping
    With udtPOSThree
         .strLocalDrive = "R:"
         .strUNCPath = "\\10.0." & STORE_NUMBER & ".103\dpalm"
         .strPersistent = "False"
         .strUsr = "somename"
         .strPas = "somepass"
    End With
    alMappings.add(udtPOSThree)
End If

oShell.LogEvent EVENT_SUCCESS, "Mappings built"

Dim udtMapping
For Each udtMapping in alMappings
    ' Unmap the drive if it's currently mapped
    On Error resume Next
    objNetwork.RemoveNetworkDrive udtMapping.strLocalDrive, True
    On Error goto 0

    ' Map the drive
    objNetwork.MapNetworkDrive udtMapping.strLocalDrive, udtMapping.strUNCPath, _
                               udtMapping.strPersistent, udtMapping.strUsr, _
                               udtMapping.strPas

    ' Windows event logging
    if err.number <> 0 then
        oShell.LogEvent EVENT_FAIL, "Login script failed" & vbcrlf &_
                "Error #: " & err.number & vbcrlf & "Error: " & err.description &_
                "Failed to map " & udtMapping.strUNCPath & " to " & udtMapping.strLocalDrive
    else
        oShell.LogEvent EVENT_SUCCESS, "Login script successfully mapped " &_
                udtMapping.strUNCPath & " to " & udtMapping.strLocalDrive
    end if

Next

Just a quick little note: that chunk here, should be parameterized and moved into a separate procedure:

' POS 1
If NUM_REGS >= 1 Then
    Dim udtPOSOne : Set udtPOSOne = New Mapping
    With udtPOSOne
         .strLocalDrive = "P:"
         .strUNCPath = "\\10.0." & STORE_NUMBER & ".101\dpalm"
         .strPersistent = "False"
         .strUsr = "somename"
         .strPas = "somepass"
    End With
    alMappings.add(udtPOSOne)
End If

...instead of being copied over and repeated 3 times with a different strLocalDrive and strUNCPath.


Refactoring observer pattern interface I have a observer pattern implemented and I'm using two base classes called Subject and Observer. - The Subject class is an abstract class that has methods to insert the observers. And the Observer is just an interface.

What I want is to provide a safer interface to classes that inherit to access the vector of observers and that is the way I implemented...

What would you do to make this better? Is this a bad design? Anything wrong?

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

class Observer
{
public:
    virtual ~Observer() {}

    virtual void notifyHuman() = 0;
    virtual void notifyRobot() = 0;
};

class Subject
{
    typedef std::vector<std::unique_ptr<Observer>> vector_type;
public:
    virtual ~Subject() {}

    void attachObserver(vector_type::value_type observer)
    {
        container.push_back(std::move(observer));
    }

    vector_type::const_iterator begin() const
    {
        return container.begin();
    }

    vector_type::const_iterator end() const
    {
        return container.end();
    }

    virtual void notifyHuman() = 0;
    virtual void notifyRobot() = 0;
private:
    vector_type container;
};

class FooSubject : public Subject
{
public:
    FooSubject() {}
    ~FooSubject() {}

    void notifyHuman()
    {
        std::for_each(begin(), end(), [] (auto & observer) {
            observer->notifyHuman();
        });
    }

    void notifyRobot()
    {
        std::for_each(begin(), end(), [] (auto & observer) {
            observer->notifyRobot();
        });
    }
};

class BarObserver : public Observer
{
public:
    BarObserver() {}
    ~BarObserver() {}

    void notifyHuman()
    {
        std::cout << "received notification of human." << std::endl;
    }

    void notifyRobot()
    {
        std::cout << "received notification of robot." << std::endl;
    }
};

int main()
{
    FooSubject subject;
    subject.attachObserver(std::make_unique<BarObserver>());
    subject.notifyHuman();
    subject.notifyRobot();

    return 0;
}

Looks good.

This is a little confusing:

void attachObserver(vector_type::value_type observer)

To see what I have to pass to this function. I need to look up vector_type (which is in the class). I would make it more explicit so that I can see exactly what is expected:

void attachObserver(std::unique_ptr<Oberver> observer)

Don't see the need for:

class FooSubject : public Subject

I would just put all methods in Subject (no need for a FoodSubject). But this is a pattern not a design and you may have a reason that are not obvious from the current question.

There is no need to define methods that are covered by the default versions.

// I would not bother with this
FooSubject() {}
~FooSubject() {}


Configure AspNet.Identity to allow for either username OR email address on login I am wondering if there is a more efficient route to take here.  Using AspNet.Identity I would like to allow the user to sign in to the same text box using either their usernameor email.  I went ahead and addressed this issue in theAccountController Login ActionResult`.

I run the check before calling:

var result = await SignInManager.PasswordSignInAsync(model.Username,
                                                     model.Password,
                                                     model.RememberMe,
                                                     shouldLockout: false);

My method for checking username or email:

// Determine if user has entered their UserName or Email address.
// TODO: research if there is a more efficient way to do this.
using (var db = new ApplicationDbContext())
{
  model.Username = (db.Users.Any(p => p.UserName == model.Username)) ?
  model.Username :
  (db.Users.Any(p => p.Email == model.Username)) ?
  db.Users.SingleOrDefault(p => p.Email == model.Username).UserName :
  model.Username;
}

My concerns here:

Is there a more efficient practical way to do this?
Am I introducing any new security risks or performance risks by doing it this way?

I am including the entire ActionResult below for reference:

//
// POST: /Account/Login
[HttpPost]
[AllowAnonymous]
[ValidateAntiForgeryToken]
public async Task<ActionResult> Login(LoginViewModel model, string returnUrl)
{
  if (!ModelState.IsValid)
  {
    return View(model);
  }

  // Determine if user has entered their UserName or Email address.
  // TODO: research if there is a more efficient way to do this.
  using (var db = new ApplicationDbContext())
  {
    model.Username = (db.Users.Any(p => p.UserName == model.Username)) ?
    model.Username :
    (db.Users.Any(p => p.Email == model.Username)) ?
    db.Users.SingleOrDefault(p => p.Email == model.Username).UserName :
    model.Username;
  }

  // This doesn't count login failures towards account lockout
  // To enable password failures to trigger account lockout, change to shouldLockout: true
  var result = await SignInManager.PasswordSignInAsync(model.Username, model.Password, model.RememberMe, shouldLockout: false);
  switch (result)
  {
    case SignInStatus.Success:
      return RedirectToLocal(returnUrl);
    case SignInStatus.LockedOut:
      return View("Lockout");
    case SignInStatus.RequiresVerification:
      return RedirectToAction("SendCode", new { ReturnUrl = returnUrl, RememberMe = model.RememberMe });
    case SignInStatus.Failure:
    default:
      ModelState.AddModelError("", "Invalid login attempt.");
      return View(model);
  }
 }

I added some formatting to that nested ternery to try to understand what it's doing, and it still took me a while!

model.Username = (db.Users.Any(p => p.UserName == model.Username))
    ? model.Username
    : (db.Users.Any(p => p.Email == model.Username))
        ? db.Users.SingleOrDefault(p => p.Email == model.Username).UserName
        : model.Username;

It seems to have 3 possible paths, and only one of them actually does anything (since the other two just set model.Username to model.Username).

So it looks like this is actually trying to say:

if(!db.Users.Any(p => p.UserName == model.Username)
    && db.Users.Any(p => p.Email == model.Username))
{
    model.Username = db.Users.Single(p => p.Email == model.Username).UserName;
}

(Not that I've changed SingleOrDefault to Single. This practically shouldn't have an effect but is a slightly more appropriate method for the situation)

Okay, that's a little better, and we could extract the conditional to its own method for readability. But it's still problematic:

It's making 3 database calls (via EF) to get one piece of information
It's having to use some pretty roundabout logic to do this. For example, it relies on the fact that no valid email address could also be a valid username. This is domain logic, which shouldn't be encoded (or repeated) here.

The first point could be improved on with some further rejigging:

var user = db.Users.Where(p => p.UserName == model.Username || p.Email = model.Username)
    .SingleOrDefault();
if(user != null)
{
    model.Username = user.UserName.
}

Here we're relying on:

Uniqueness of usernames
Uniqueness of email addresses
No string can be valid as both a username and email address

Of those, only the first one is a new assumption compared to the original code

The second point is rather more difficult to deal with. While it would be possible to move the logic for matching against an individual username or email to a User class, it would be hard to do that in a way which EF would still be able to translate to SQL. To avoid adding a lot of complexity, it's probably best not to try to do this. Do make sure your validation won't allow for usernames and email addresses which could match, though!


Character and word counter A program that will count up the number of words in a text, as well as counting how many times a certain character shows up in the text. Only ignore spaces.
This gave me a lot of trouble as I grappled with list comprehension syntax errors.

class TextAnalyzer:
    def __init__(self, txtFile):
        self.txtContent = open(txtFile)
        self.txtStream = self.txtContent.read()

    def __repr__(self):
        return self.txtStream

    def countChars(self):
        characters = {}
        for char in self.txtStream:
            if char != ' ' and char != '\n':
                if char.lower() not in characters:
                    characters[char.lower()] = 1
                else:
                    characters[char.lower()] += 1
        return [x for x in characters.items()]

    def countWords(self):
        totalWords = 0
        for b, a in enumerate(self.txtStream):
            if a != ' ' and a != '\n' and b != len(self.txtStream)-1 and (self.txtStream[b+1] == ' ' or self.txtStream[b+1] == '\n'):
                totalWords += 1
        return totalWords+1

analyze = TextAnalyzer('Darwin.txt')
totalChars = 0;
print(analyze)
for x, y in sorted(analyze.countChars()):
    totalChars += y
    print('{0}: {1}'.format(x, y))
print('Total Characters: ', totalChars)
print('Total Words: ', analyze.countWords())

Incorrect counting

For a text file that ends with a space or newline character,
the countWords function counts 1 more word than there really are.

Before giving away the fix, I would point out a few things about this implementation:

totalWords = 0
for b, a in enumerate(self.txtStream):
    if a != ' ' and a != '\n' and b != len(self.txtStream)-1 and (self.txtStream[b+1] == ' ' or self.txtStream[b+1] == '\n'):
        totalWords += 1
return totalWords+1

The names b and a make this a bit hard to read.
The value of a is a character,
the value of b is the position (or index) of the character.
Better names would have been c and i (or pos or index),
and the code would be a bit easier to read.

Processing the text character by character like this is a bit complicated,
and it will get even more complicated if you add the special treatment to fix the processing at the end of the content.

A simpler way would be to split the input, and use the length of the resulting list, for example:

return len(self.txtStream.split())

However, this is not exactly the same original code, and it has a drawback too.
It's not the same, because the split() function of strings splits the input by any whitespace,
including TAB,
while your code splits strictly on SPACE and NEWLINE.
If you really want to use the same logic and not split on TAB,
then you would have to use the split function from the re package:

import re

# ...

return len(re.split('[ \n]', self.txtStream))

This preserves the behavior of your original code,
and fixes the off-by-1 error,
because just like the split function of strings,
it ignores whitespace at the end of the content.

The splitting technique has an important drawback though:
it consumes more memory,
because it creates a list of words from the content,
effectively doubling the memory use.
Your solution is harder to implement correctly and to read,
but it's more memory-efficient.

Always close resources

There are several issues here:

def __init__(self, txtFile):
    self.txtContent = open(txtFile)
    self.txtStream = self.txtContent.read()

def __repr__(self):
    return self.txtStream

The biggest issue is that a filehandle is not properly closed.
When you open something, remember to close it!

Since it's so easy to forget closing filehandles,
Python has a syntax to do this automatically for you:

with open(txtFile) as fh:
    self.content = fh.read()

Class attributes vs local variables

In the with example in the previous section,
I renamed self.txtContent to fh,
for two reasons:

It's a more appropriate name: txtContent implies some text content, which is not true and misleading. Likewise, I renamed txtStream to content, because that's what it really is. A "stream" is usually something that doesn't occupy much memory, doesn't contain the full content, but a data structure from which you can stream the content gradually without consuming much memory.
Even more importantly, self.txtContent is not used anywhere outside the __init__ function,
so there's no need to make it an attribute of the class, it can be a local variable.

Avoid repeated operations

In this code, char.lower() is performed 3 times:

if char.lower() not in characters:
    characters[char.lower()] = 1
else:
    characters[char.lower()] += 1

Since all 3 times it will give the same result,
it would be better to do this once,
store the value in a variable and use that variable instead of repeating the call.

Avoid repeated operations -- the bigger picture

An even more important repeated operation could be performed by users of the TextAnalyzer class.
Once you create an instance of this class,
the text content it read from the file will normally not change,
but calling countChars and countWords will recompute the same values over and over again.

If the class is not designed to change the value of the text content after construction,
then it would be better to not compute the character and word counts only once.

Python conventions

There are common conventions for variable and function naming in Python,
explained in PEP8.
It's strongly recommended to follow that when coding in Python.

Simplifications

Instead of this:

if char != ' ' and char != '\n':

You can write like this:

if char not in ' \n':

Instead of this:

return [x for x in characters.items()]

You can write like this:

return characters.items()

Instead of this:

if char.lower() not in characters:
    characters[char.lower()] = 1
else:
    characters[char.lower()] += 1

You can write like this:

c = char.lower()
characters[c] = characters.get(c, 0) + 1


MySQL limit across multiple tables in multiple queries I have a website that keeps track of 'changes' that users make. These changes come in all shapes and sizes and each shape and size has a different MySQL table to store them in. In total there are 8 different tables. Each table has completely different columns, so I don't think a simple JOIN would be possible, however they all have a time column.

What I would like to do is show the last x number of changes made across all tables (i.e. sorted by time). How I am doing it now is querying all tables like this:

foreach ($tables as $table) {
    $stmt = $this->_dbh->prepare("SELECT $table.*,
            realname,
            time,
            reservations.conf_num,
            reservations.conf_letters
        FROM
            $table
        LEFT JOIN users ON $table.user_id = users.id
        LEFT JOIN reservations
            ON $table.conf_num = reservations.conf_num
        ORDER BY
            time DESC
        LIMIT ".$this->_num_values);
    $stmt->execute();

    $changes[$table] = $stmt->fetchAll();
}

Then I sort and slice the resultant array:

ksort($changes);
$changes = array_reverse($changes);

// Make sure to limit it if we have a maximum number of values
if ($this->_num_values > 0) {
    $changes = array_slice($changes, 0, $this->_num_values);
}

This works fine, however it just seems extremely inefficient to me since we are retrieving the latest x values from all tables instead of just returning the exact number so that the sum of the number of rows from the individual queries equals x. Is there a better way to do this, or am I just trying to micro-optimize too much here?

Yes, there is: create a stored procedure and UNION the result sets then sort the records and return them to PHP.


Monty hall python simulation I have the following code that simulates the monty hall problem (see google for more details). I used sets to do it but is there a more intuitive or efficient way to do it?

import random as r
from sets import Set
def montysim(N):
  K = 0
  for i in range(N):
    s = Set([1,2,3])
    doorstoswitch = Set([1,2,3])
    cardoor = r.randint(1,3)
    chosendoor = r.randint(1,3)
    doorstoswitch.remove(chosendoor)
    if chosendoor != cardoor:
        s.remove(chosendoor)
    s.remove(cardoor)

    montydoor = r.sample(s, 1)[0]
    doorstoswitch.remove(montydoor)
    newdoor = r.sample(doorstoswitch, 1)[0]
    if newdoor == cardoor:
        K+=1
  return float(K) / float(N)

print montysim(10000)

Two things stick out to me:

First, Python has an official style-guide, PEP8. It recommends consistently using 4 spaces as indentation (you use a mix of two and four). It also recommends using lower_case_with_underscores for names.

Second, the sets module has been deprecated. You can just use the built-in set, instead of sets.Set.

import random

def monty_sim(n):
    k = 0
    for i in range(n):
        s = set([1, 2, 3])
        doors_to_switch = set([1, 2, 3])
        car_door = random.randint(1, 3)
        chosen_door = random.randint(1, 3)
        doors_to_switch.remove(chosen_door)
        if chosen_door != car_door:
            s.remove(chosen_door)
        s.remove(car_door)

        monty_door = random.sample(s, 1)[0]
        doors_to_switch.remove(monty_door)
        newdoor = random.sample(doors_to_switch, 1)[0]
        if newdoor == car_door:
            k += 1
      return float(k) / float(n)

if __name__ == "__main__":
    print monty_sim(10000)

This even comes with a nice speed-boost for free!

In [2]: %timeit montysim(10000)
10 loops, best of 3: 109 ms per loop

In [4]: %timeit monty_sim(10000)
10 loops, best of 3: 54 ms per loop


Find first SO chat message in Haskell I have made some simple functions to find the first SO chat message. How can I improve my code?

Even if there is a stackoverflow link that will get the job done (I'd be glad to know), I'd like to improve upon my already written code.

import Network.HTTP
import Text.Regex.Posix

type UserID      = String
type MessageLink = String
type PageNumber  = Integer

findLast :: UserID -> IO MessageLink
findLast = searchLast 1 "Not found"

searchLast :: PageNumber -> MessageLink -> UserID -> IO MessageLink
searchLast pg lnk id = do

    let link = "http://chat.stackoverflow.com/users/" ++
               id ++ "?tab=recent&page=" ++ show pg

    c <- simpleHTTP (getRequest link) >>= getResponseBody

    let match = c =~ "/transcript/[^\"]+" :: [[String]]

    if null (c =~ "monologue" :: String)
        then return $ "http://chat.stackoverflow.com" ++ lnk
        else searchLast (pg + 1) (head . last $ match) id

Avoid partial functions like head and last.
http://chat.stackoverflow.com is repeated twice in searchLast. Maybe throw it into its own let-binding to make the function more DRY.


Python deploy function using Fabric Simple task - deploy war-file from TeamCity agent to remote web-server.

I realized it with Fabric, because of it make readable output, and give opportunity to login via SSH with RSA-key. Also - during deploy - build agent must make some additional steps - create backup, stop-start Tomcat service etc.

URL to deploy to can be passed via variable on build-server.

import os
import sys
import time

from fabric.api import run, env, put

VAR_PREFIX = 'bamboo_'

"""Cloudlibrary functions"""

def deploy(logger, rds_basedir):

    """Deploy cloudlibrary.war to remote box, passed via BASE_URL"""

    logger.logger.info('Running cloudlibrary.war deploy')

    # file to deploy
    local_file = 'target/cloudlibrary.war'
    # local_file = 'd:\\RDS\\rdsmanager\\file.txt'

    # file will be created during backup command
    back_file = '/home/ec2-user/backups/cloudlibrary/cloudlibrary_%s.war' % time.strftime('%Y-%m-%d')

    # commands list
    status = 'sudo service tomcat7 status'
    backup = 'cp /var/lib/tomcat7/webapps/cloudlibrary.war %s' % back_file
    check_bkp = 'file %s' % back_file
    tomcat_kill = 'sudo kill -9 $(cat /var/run/tomcat7.pid)'
    tomcat_start = 'sudo /etc/init.d/tomcat7 start'

    try:
        base_url = os.environ[VAR_PREFIX + 'BASE_URL']
        logger.logger.info('\nURL found %s' % base_url)
    except KeyError:
        base_url = 'www.dev.domain.com'
        logger.logger.info('URL variable not found, will default - %s' % base_url)

    # Fabric connection settings
    env.host_string  = base_url
    env.key_filename = [os.path.join(rds_basedir, '.ssh', 'rdsmanager_priv.openssh')]
    env.user = 'user'
    env.project_root = '/var/lib/tomcat7/webapps'

    logger.logger.info('Using URL: %s, user: %s, RDS-key: %s, Tomcat webapps: %s' % (env.host_string, env.user, env.key_filename[0], env.project_root))

    if os.path.isfile(local_file):
        # make backup of current cloudlibrary.war to ~/backup/cloulibrary
        run(backup)

        # if new file doesn't found - Fabric will exit
        run(check_bkp)

        # stop Omcat before put new war-file
        run(tomcat_kill)

        # put new builded war-file
        put(local_file, env.project_root)

        # start Tomcat
        run(tomcat_start)

        # cgeck it's status
        run(status)
    else:
        logger.logger.error('ERROR: file %s not found. Exit.' % local_file)
        sys.exit(1)

Using % for string formatting is deprecated. You should be using str.format() instead. Here's how one would use str.format(): print "Hello {0}.".format("world"). str.format() also supports using keyword arguments. Here's an example of that: print "Hello {word}.".format(word="world").
Near the end of the function deploy, underneath the if block with the condition os.path.isfile(local_file), you have comments above every line. These are unnecessary and can be removed.
Finally, I'm not sure why you have square brackets, [], around the value of the variable env.key_filename. If these aren't needed, they can be removed.


Round Robin implementation in java I tried to implement round robin in java please check and help me to
improve it Thanks:

package trial.java;

import java.util.ArrayList;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;

public class Roundrobin1 {
//common interval for all process.
static int commonInt=3;
//to store a list of queued process
Map<String, Integer> waitingMAp=new LinkedHashMap<>();
List<Integer>waitingTimeList=new ArrayList<>();
//to Store elapsed time.
int timepassed=0;

public  void mainL(){
    //input process name and time in order.
    waitingMAp.put("p0", 3);
    waitingMAp.put("p1", 3);
    waitingMAp.put("p2", 3);
    waitingMAp.put("p3", 9);
    //first time getting the list of process.
    List<String>process=new ArrayList<>();
    //it will call recursively till no process is waiting.
    while(!waitingMAp.isEmpty()){
        process.addAll(this.getRoundRobin(waitingMAp));
    }
    System.out.print(process);
    System.out.println();
    System.out.print(waitingTimeList);
}

    public List<String> getRoundRobin(Map<String, Integer>
    processMap){
    Map<String, Integer> Local=new LinkedHashMap<>();
    Local.putAll(processMap);
    waitingMAp.clear();
    List <String>curr=new ArrayList<>();
    for(Map.Entry<String, Integer> entry :Local.entrySet()){
        curr.add(entry.getKey());

   waitingTimeList.add(entry.getValue()>3?timepassed=timepassed+3:     (timepassed=timepassed+entry.getValue()));

        if(entry.getValue()>commonInt){
            waitingMAp.put(entry.getKey(),entry.getValue()-commonInt);
        }
        }

    return curr;

    }
    public static void main(String[] a){
    new Roundrobin1().mainL();

    }

}

it will return below result:
first row is order and second row is time elapsed.
[p0, p1, p2, p3, p3, p3]
[3, 6, 9, 12, 15, 18]

I would like to add couple of questions here

Do I need to take priorities of  the process as well in the input.If yes how it going to solve starvation problem.
How to scale up this algorithm for multiprocessor or multi-core systems

Naming

Your variables, class and methods aren't very well named.

The class Roundrobin1 should be Pascal case. It's also discouraged from using numbers in class names. If you are thinking of doing implementations of Round Robin, perhaps DefaultRoundRobin would be okay. If not, just RoundRobin is fine.

You have a local variable named Local. Variables should always be camel case to distinguish them from classes.

You also have a few inconsistencies such as timepassed, which should be timePassed, and waitingMAp which should be waitingMap.

It's not clear what the method mainL is supposed to do. Give it a more specific name.

Fields

Fields should be declared with the lowest access possible. Your fields can be private as they are not required outside of the class. Fields should be declared as final if you do not expect them to change.

Whitespace

Consider using it more liberally, especially between operators.

entry.getValue() > commonInt

is easier to read than

entry.getValue()>commonInt

Compound Statements

Specifically this line:

waitingTimeList.add(entry.getValue()>3?timepassed=timepassed+3:     (timepassed=timepassed+entry.getValue()));

has way too much going on. You could refactor it to:

timepassed += (entry.getValue() > 3) ? 3 : entry.getValue();
waitingTimeList.add(timepassed);

which is much more readable.

Magic Numbers

You have a few seemingly random 3s and 9s. Consider using constants (static final fields) with appropriate names.


Counting combinations of consecutive symbols in Java Let's assume we have an array that contains the following elements:

-1, 0, 0, 1, 0, -1, 0, -1, 1

I would like to count the combinations of consecutive elements such that

-1,0 & 0,0 & 0,1 & 1,0 & 0,-1 & -1,0 & 0,-1 & -1&1

In this case:

The count of -1,0 = 2
The count of 0,1 = 1
The count of 1,0 = 1
and so on

I implemented the following:

List<Double> sixProbabilites = new ArrayList<Double>();

        int count_neg10 = 0, count_neg11 = 0, count_10 = 0, count_1neg1 = 0, count_0neg1 = 0, count_01 = 0;

        for(int i=0; i<slopeChanges.size(); i++) {
            if(i+1 != slopeChanges.size()) {
                if((slopeChanges.get(i) == -1) && (slopeChanges.get(i) == 0)) {count_neg10++;}
                else if((slopeChanges.get(i) == -1) && (slopeChanges.get(i+1) == 1)) {count_neg11++;}
                else if((slopeChanges.get(i) == 1) && (slopeChanges.get(i+1) == 0)) {count_10++;}
                else if((slopeChanges.get(i) == 1) && (slopeChanges.get(i+1) == -1)) {count_1neg1++;}
                else if((slopeChanges.get(i) == 0) && (slopeChanges.get(i+1) == -1)) {count_0neg1++;}
                else if((slopeChanges.get(i) == 0) && (slopeChanges.get(i+1) == 1)) {count_01++;}
            }
        }

There is no problem with the code but I am just wondering if there is a way to enhance the code especially the if statement. Is there any way to reduce the code size to few lines?

I would build a Map with a custom keying function and just iterate over the length (minus one) of the array of values.

import java.util.*;
import java.util.function.Function;
import java.util.stream.Collectors;

public class GroupFrenquency {
    public static <T> Map<String, Integer> groupFrenquencies(T[] arr, Function<List<T>, String> keyFn) {
        Map<String, Integer> frequency = new HashMap<>();
        for (int i = 0; i < arr.length - 1; i++) {
            String key = keyFn.apply(Arrays.asList(arr[i], arr[i + 1]));
            frequency.put(key, frequency.containsKey(key) ? frequency.get(key) + 1 : 1);
        }
        return frequency;
    }

    public static void main(String[] args) {
        Integer[] sequence = { -1, 0, 0, 1, 0, -1, 0, -1, 1 };
        Function<List<Integer>, String> keyFn = (items) -> String.format("%s,%s", items.get(0), items.get(1));

        System.out.println(groupFrenquencies(sequence, keyFn).entrySet().stream()
                .map(e -> String.format("The count of %-4s = %d", e.getKey(), e.getValue()))
                .collect(Collectors.joining(System.lineSeparator())));
    }
}

Output

The count of 0,0  = 1
The count of 0,1  = 1
The count of 1,0  = 1
The count of 0,-1 = 2
The count of -1,1 = 1
The count of -1,0 = 2

Using reflection, you can create a copy of the array instead of creating an entire list object, when creating the key. The only downside is that you need to pass in the class type of the array, because you cannot initialize a generic array without knowing the type.

import java.lang.reflect.Array;
import java.util.*;
import java.util.function.Function;
import java.util.stream.Collectors;

public class GroupFrenquency {
    public static <T> Map<String, Integer> groupFrenquencies(Class<T> type, T[] arr, Function<T[], String> keyFn) {
        Map<String, Integer> frequency = new HashMap<>();
        for (int i = 0; i < arr.length - 1; i++) {
            String key = keyFn.apply(copyArray(type, arr, i, 2));
            frequency.put(key, frequency.containsKey(key) ? frequency.get(key) + 1 : 1);
        }
        return frequency;
    }

    @SuppressWarnings("unchecked")
    private static <T> T[] copyArray(Class<T> type, T[] source, int startPos, int length) {
        T[] copy = (T[]) Array.newInstance(type, length);
        System.arraycopy(source, startPos, copy, 0, length);
        return copy;
    }

    public static void main(String[] args) {
        Class<Integer> type = Integer.class;
        Integer[] sequence = { -1, 0, 0, 1, 0, -1, 0, -1, 1 };
        Function<Integer[], String> keyFn = (items) -> String.format("%s,%s", items[0], items[1]);

        System.out.println(groupFrenquencies(type, sequence, keyFn).entrySet().stream()
                .map(e -> String.format("The count of %-4s = %d", e.getKey(), e.getValue()))
                .collect(Collectors.joining(System.lineSeparator())));
    }
}


Class for median filter I'm new to ruby, later I asked about ways to refactor code (Parsing GPS data), one man gave me direction, so I made class for Median filter and it will be great to read review for my work.

What guys are you thinking about it? Is it still smells?

class MedianFilter
  attr_accessor :step

  def initialize(step, keys)

    raise ArgumentError.new('Step must be odd and higher than 3') if step < 3 && step % 2 != 1
    raise ArgumentError.new('Keys must be array of symbols') unless keys.kind_of?(Array) && keys.count > 0

    @step = step
    @keys = keys

  end

  def filter(param_values)
    process_values param_values.clone
  end

  def filter!(param_values)
    process_values param_values
  end

  private

  def process_values(values)

    half_step = (@step - 1) / 2

    values.each_index do |i|

      item = values[i]

      median_points = [ values[ [0, i - half_step].max ],
                        item,
                        values[ [values.count - 1, i + half_step].min ] ]

      @keys.each do |key|
        item[key] = median_points.map { |x| x[key] || 0 }.sort[half_step]
      end

    end

    values

  end

end

Your code has some bad side-effects. There's no real difference between filter and filter! since they both modify the input values directly.

It seems that your input is something like:

[{foo: 2}, {foo: 80}, {foo: 6}, {foo: 3}]

in other words, an array of hashes.

When you call filter, you duplicate the array - but the hash objects are still the same objects. So when you calculate the median, and set item[key] = ..., you're modifying the same hash objects. Duplicating the array has no real effect; you're not modifying the array itself, only the objects inside the array. And those objects are the same even if you duplicate the array.

An example:

filter = MedianFilter.new(3, [:foo])
original = [{foo: 2}, {foo: 80}, {foo: 6}, {foo: 3}]

modified = filter.filter(values)

modified # => [{:foo=>2}, {:foo=>6}, {:foo=>6}, {:foo=>3}]
original # => [{:foo=>2}, {:foo=>6}, {:foo=>6}, {:foo=>3}]

In the example, modified is a different array object than original, but the contents of both arrays have been changed. In order words: Your filter method has side-effects.

And I doubt that's what you want.

Anyway, about the code:

Another bug: Your exception says that step "must be odd and higher than 3", but that's not what you actually check. You code checks whether step is below 3 and even. So I can pass in 4 and not get an error, even though it's an even number. Or I can pass in -3 and not get an error, even though is less than 3.
Your error message is also a little misleading. It says "higher than 3" when it fact it can be 3.
You check of keys only checks for a non-empty array - it doesn't check that the array only contains symbols. I could pass in ["foo", 42, Date.new] without the initializer raising an error.
There are a few built-in methods that would make the checks more idiomatic and readable. For instance, step % 2 != 1 can be written as step.even?, and keys.count > 0 can be written as !keys.empty?
I'd probably rename step to window_size, since I think that's a better name. Or you might simply redefine it as the number of neighbors to include - i.e. what you currently call half_step.
I'd simply skip the keys argument, and instead provide the input data in a different way. It would keep this class simple and generic.
I'm not sure this actually needs to be a class, since it doesn't really need to maintain state. It could just be a method.
As for the process method, you can use each_cons to get the "windows" you want to find the median for. You just need to pad the array a little. Here's a simplified example:

# example input
values = [2, 80, 6, 3]
window_size = 3
neighbors = (window_size / 2).floor

# create an array with extra elements at the start and end
temp = ([values.first] * neighbors) + values + ([values.last] * neighbors)

# calculate medians
medians = temp.each_cons(window_size).map { |window| window.sort[neighbors] }

medians # => [2, 6, 6, 3]


Is this a good structure for my website? I have been building a site using php and would like advice to improve the structure of my site and the best way to lay out my code for SEO. I'm also a little concerned about echoing large chunks of HTML and how SEO will work with my structure.

Ok here's my structure...

My site contains many pages with only the body content changing for each page. So I have created template functions in PHP for echoing HTML. I use $_GET['page_id'] in a switch statement to determine what page content to load. .

my pages look something like this....

Header();                            //echo header HTML

 switch ($_GET['page_id'])            //Echo body content
 {
   default: homePage(); break;
   case 1:  servicesPage(); break;
   case 2:  galleryPage(); break;
   case 3:  contactPage(); break;
 }

 Footer();                           //Echo footer HTML

And my php functions look like this

 function header() {
      echo "

Header

";
 }

 function footer() {
      echo "

Header

";
}

 function homePage()
 {
    echo "

 Home

 Home1

 Home1

This is home page

";

 }

I'm really looking for any pointers to improve my code but my main concerns are...

Is it ok to echo large chucks of HTML like that from php? especially with my site growing?

Does Google crawler bot still crawl my site ok using $_GET['page_id'] for all my pages?

P.S I'm new here, so sorry if any code formatting is a little off!

Looking at your switch statement from an SEO point of view, I'd have to say: it's awful (sorry).
One of the first things to look at, when you're interested in SEO, is mod_rewrite. Search engines don't like urls that say nothing:

http://www.my-site.com/1
http://www.my-site.com/2
http://www.my-site.com/3
http://www.my-site.com/3

Doesn't say anything, whereas

http://www.my-site.com/home
http://www.my-site.com/services
http://www.my-site.com/gallery
http://www.my-site.com/contact

If anything, contains a sort-of meaningful keyword in the url from the off. Not only to crawlers love that, so do people browsing your site, for that matter.
Naturally, you have to keep this up, take the services page for example. I'm assuming that this is the page where you list what you do/offer, then all sub-nav urls should look like:

http://www.my-site.com/services/custom/cms
http://www.my-site.com/services/portfolio

And so on...
As to what Topener suggested: require_once 'pages/'.$_GET['page'].'.php'; is a terrible, terrible idea. Using data from the network, data the client can change to determine what files should be loaded is a huuuuge security hole!

From a CodeReview viewpoint:
A switch is all well and good, but it won't be long untill you find yourself scrolling up and down through your code, because the switch-case has grown so large. You'll have to invent hacky ways of dealing with form validation, and other types of client requests (pagination, ajax, file uploads, ...) in a manageable, safe and maintainable way.
A switch can be useful to shorten a lot of if-elseif-elseif-else branches, but not for something as crucial as to determine what page you have to present the client with. Most modern sites would go with a framework that implements the MVC pattern (Symfony2, ZendFW, Yii, ...)
As AD7Six said in his comment: if all you're after is a simple routing solution, there are micro-fw's, too, that might be a better fit for your problem. Even so, most frameworks are more or less modular, and with a dependency manager like composer, you can use the routing module(s) from any framework.

Your functions are also in violation of the PSR standards as defined by PHP-Fig (look for them on github). Classes (objects) start with an upper-case, functions/methods don't. Try to adhere to what little standards/conventions you have

Lastly, this just looks wrong to me:

switch($foo)
{
    default: homePage(); break;
    case 1: ...
}

The defaut case is the fallback case. To me, logic dictates that the default/fallback procedure is the last resort, and it should therefore be written as the last case label. Maybe that's just me, and my (sometimes over eager) tendency to use case fallthroughs:

switch ($foo)
{
    case 1:
    case 2:
        bothDoTheSame();
        break;
   case 3:
       threeSpecific();
   case 4:
       threeAndFour();
       break;
   default: defaultCase();//no break required
}


Bubble, selection, and insertion sorting The following programs are bubble sorting, selection sorting and insertion sorting. As most of the programs online or in books are with Arrays and not ArrayList, I am unsure if they are correct.

BubbleSort:

    public class BubbleSort {

      List<Integer> unsortedList;

      private BubbleSort() {
        unsortedList = new ArrayList<Integer>();
        unsortedList.add(0, 6);
        unsortedList.add(1, 3);
        unsortedList.add(2, 7);
        unsortedList.add(3, 4);
        unsortedList.add(4, 2);
        unsortedList.add(5, 3);
        unsortedList.add(6, 8);
    }

    private void sortData() {
        ArrayList<Integer> tempList = (ArrayList<Integer>) this.unsortedList;
        int size = unsortedList.size();
        int counter = size;
        do {

            for (int i = 0; i < size-1; i++) {

                if (unsortedList.get(i) > unsortedList.get(i + 1)) {

                    Integer temp1= unsortedList.get(i + 1);
                    Integer temp2= unsortedList.get(i);
                    unsortedList.set(i,temp1);
                    unsortedList.set(i+1,temp2);

                }
            }
            size = size - 1;
        } while (size != 1);
    }

    public static void main(String arg[]) {

        BubbleSort obj = new BubbleSort();
        obj.sortData();

        for(Integer i:obj.unsortedList){
            System.out.println(i);
        }
    }}

SelectionSort:

    public class SelectionSort {

    List<Integer> unsortedList;

    private SelectionSort() {
        unsortedList = new ArrayList<Integer>();
        unsortedList.add(0, 16);
        unsortedList.add(1, 3);
        unsortedList.add(2, 7);
        unsortedList.add(3, 4);
        unsortedList.add(4, 12);
        unsortedList.add(5, 3);
        unsortedList.add(6, 8);
        unsortedList.add(7, 18);
        unsortedList.add(8, 81);
        unsortedList.add(9, 2);
    }

    public void sortData() {

    int totalPass = unsortedList.size();
    int listLength = totalPass;

    for (int i = 0; i <= totalPass - 1; i++) {
        int pointerSmallPosition=i;
        for (int j = i; j < listLength-1; j++) {

            if (unsortedList.get(pointerSmallPosition) > unsortedList.get(j + 1)) {
                pointerSmallPosition=j + 1;
            }
        }

        int temp1= unsortedList.get(i);
        int temp2= unsortedList.get(pointerSmallPosition);
        unsortedList.set(i,temp2);
        unsortedList.set(pointerSmallPosition,temp1);
    }

}

public static void main(String arg[]) {

    SelectionSort obj = new SelectionSort();
    obj.sortData();

    for (Integer i : obj.unsortedList) {
        System.out.println(i);
    }
}}

InsertionSort:

 public class InsertionSort {

    List<Integer> unsortedList;

    public InsertionSort(){
        unsortedList = new ArrayList<Integer>();
        unsortedList.add(0, 6);
        unsortedList.add(1, 3);
        unsortedList.add(2, 7);
        unsortedList.add(3, 4);
        unsortedList.add(4, 2);
        unsortedList.add(5, 3);
        unsortedList.add(6, 8);
    }

    public static void main(String arg[]){

        InsertionSort obj=new InsertionSort();
        obj.sortData();

        for(Integer i:obj.unsortedList){
            System.out.println(i);
        }

    }

    private void sortData() {

        int size = unsortedList.size();
        int sortedSize=0;
        for(int i=1;i<size-1;i++){
            int j=i;
            do{
                if(unsortedList.get(j)<unsortedList.get(j-1)){
                    int small=unsortedList.get(j);
                    int large=unsortedList.get(j-1);
                    unsortedList.set(j-1, small);
                    unsortedList.set(j, large);
                }
                j--;
            }while(j>sortedSize);

        }}}

They seem right to me. A few minor comments:

You don't need to construct your lists by adding .add(pos, value). You can build them from an array as:

int[] array = {6,12,9,-14};
List<Integer> list = new ArrayList<>(Arrays.asList(array);

which is more concise and readable.
In your bubblesort, you do:

Integer temp1= unsortedList.get(i + 1);
Integer temp2= unsortedList.get(i);

accessing unsortedList twice. If you assign outside the if, you can recycle it and make it more readable. Also, consider using better names than temp1 and temp2: not much difference here, but it's a good habit to get into, same for insertion.