Search key in a rotated sorted array without repetition of elements Is there any improvement possible in this implementation?

#include<iostream>

int search( int array[], int N, int key )
{
    int first_index = 0;
    int last_index = N - 1;

    while ( last_index >= first_index )
    {
        int middle_index = last_index + first_index / 2;

        if( array[ middle_index ] == key )
           return middle_index;

        if ( array[ first_index ] < array [ middle_index ] )
        {
            if( key >=  array[ first_index ] && key < array [ middle_index ] )
                last_index = middle_index - 1;
            else
                first_index = middle_index + 1;
        }
        else
        {
            if( key > array [ middle_index ] && key <= array [ last_index ])
                first_index = middle_index + 1;
            else
                last_index = middle_index - 1;
        }
    }
    return -1;
}

int main()
{
    int array[] = { 2, 4, 5, 6, 7, 0, 1 };
    int index = search( array, 7, 0 );

    std::cout<<"index :"<<index;

}

Generally speaking, well-designed algorithms in C++ work with iterators, not with arrays and size or with full collections. By using iterators, you can make your algorithm work with any standard collection. Since you heavily rely on the indices and make math with them, I guess that you algorithm is only meant to work with random-access iterators, but we can still generalize it with iterators to make it work with arrays, std::array, std::vector and std::deque with trivial changes.

Make it work with any random-access iterator

First, let's change the signature:

template<typename RandomAccessIt>
int search(RandomAccessIt begin, RandomAccessIt end, int key);

We don't have the size N anymore, but we can still compute last_index with the iterators:

int last_index = std::distance(begin, end) - 1;

Now, all you have to do is replace array by begin everywhere and change your main:

int main()
{
    int array[] = { 2, 4, 5, 6, 7, 0, 1 };
    int index = search(std::begin(array), std::end(array), 0);

    std::cout<<"index :"<<index;
}

Return the iterator, not the offset

Searching algorithms in the standard library tend to return the iterator where the value was found - if it was found -, and the end iterator otherwise. Once again, you can implement that with only a few changes:

Change the return type to RandomAccessIt:

template<typename RandomAccessIt>
RandomAccessIt search(RandomAccessIt begin, RandomAccessIt end, int key);

Instead of returning middle_index, return the corresponding iterator:

return std::next(begin, middle_index);

Instead of returning -1 when the value has not been found, return the end iterator:

// The key was not found
return end;

Make it fully iterator-based

By using the functions in <iterator>, you can make your algorithm have a more "iterator-oriented" feel. I almost converted it to work with bidirectional iterators (with the additional cost than std::distance is \$O(n)\$ instead of \$O(1)\$ for bidirectional iterators) but stopped myself since I would have to alter a bit the logic of your algorithm and comparing the original and the new versions would have been trickier. Let's say that the final steps are left as an exercise to the reader :p

template<typename RandomAccessIt>
RandomAccessIt search(RandomAccessIt begin, RandomAccessIt end, int key)
{
    RandomAccessIt last = std::prev(end);

    while ( last >= begin )
    {
        RandomAccessIt middle = std::next(begin, std::distance(begin, last) / 2);

        if( *middle == key )
           // The key was found
           return middle;

        if ( *begin < *middle )
        {
            if( key >= *begin && key < *middle )
                last = std::prev(middle);
            else
                begin = std::next(middle);
        }
        else
        {
            if( key > *middle && key <= *last )
                begin = std::next(middle);
            else
                last = std::prev(middle);
        }
    }
    // The key was not found
    return end;
}

Make it work for any comparable type

Turning this index-based algorithm to an iterator-based algorithm made me realize that there weren't any tricky things making use of the value of key to be more efficient and that the key was only used for comparison. Therefore, you can also make the searched type generic instead of restricting it to int. Actually, it is as simple as merely changing its signature and it works with any LessThanComparable type:

template<typename RandomAccessIt, typename LessThanComparable>
RandomAccessIt search(RandomAccessIt begin, RandomAccessIt end, LessThanComparable key);


A function to convert FASTA .txt to a dictionary I'd like feedback on my function to convert FASTA data to a dictionary in Python. Here's an example of FASTA text:

>gi|33112219|sp|O93279.1|A4_TAKRU RecName: Full=Amyloid beta A4 protein; AltName: Full=ABPP; Short=APP; AltName: Full=Alzheimer disease amyloid A4 protein homolog; Contains: RecName: Full=Beta-amyloid protein; AltName: Full=A-beta; AltName: Full=Beta-APP; Flags: Precursor
MGETTAFVLLLVATLTRSSEIPADDTVGLLTEPQVAMFCGKLNMHINVQNGKWESDPSGTKSCLNTKEGI
LQYCQEVYPELQITNVVEANQPVSIQNWCKKGRKQCRSHTHIVVPYRCLVGEFVSDALLVPDKCKFLHQE
RMNQCESHLHWHTVAKESCGDRSMNLHDYGMLLPCGIDRFRGVKFVCCPAETEQETDSSEVEGEESDVWW
GGADPEYSENSPPTPSRATYVAGDAFERDENGDGDEDEEDDEDVDPTDEQESDERTANVAMTTTTTTTTE
SVEEVVRAVCWAQAESGPCRAMLERWYFNPKKRRCVPFLFGGCGGNRNNFESEEYCLAVCSSSLPTVAPS
PPDAVDQYFEAPGDDNEHADFRKAKESLEAKHRERMSQVMREWEEAERQAKNLPRADKKAVIQHFQEKVE
ALEQEAAGERQQLVETHMARVEALLNSRRRLTLENYLGALQANPPRARQVLSLLKKYVRAEQKDRQHTLK
HYEHVRTVDPKKAAQIRPQVLTHLRVIDERMNQSLALLYKVPSVASEIQNQIYPAAGSDCKDPVEHCVCP
QVDGLVSYGNDALMPDQAYSSAPMDMGVDGLGSIDQSFNQANTENHVEPVDARPIPDRGLPTRPVSSLKL
EEMPEVRTETDKRQSAGYEVYHQKLVFFADDVGSNKGAIIGLMVGGVVIATVIVITLVMLRKKQYTSIHH
GVIEVDAAVTPEERHLARMQQNGYENPTYKFFEQMQN

FASTA lines to be used as a key begin with '>'
FASTA lines to be used as a value follow the key line.
Repeat

Is this the best way to remove whitespace, tabs, newlines, etc? I suspect it isn't because I can't tell which escape characters my .txt has.

def txt2dict(filename, key_id, \
    replace_id_list = ['\r','\n','\w','\s','\t','\v','\f'], min_len = 1):

    """A .txt file is opened as lines. Every line containing 'key_id' is added
    to the dictionary as a key. Every line which doesn't contain 'key_id' is
    added to the dictionary as a value for the previous key."""

    dictionary = dict()
    open_file = open(filename, 'r')
    lines = open_file.readlines()
    open_file.close()
    for line in lines:
        if key_id in line:
            key_line = line
            value_line = str()
        if key_id not in line:
            value_line = value_line + line
            dictionary.update({key_line:value_line})
    copy = dictionary.copy()
    for key, value in dictionary.items():
        if len(value) <= min_len:
            del copy[key]
        else:
            for replace_id in replace_id_list:
                value = value.replace(replace_id, '')
            copy.update({key:value})
    return copy

Here's an example returned dictionary. I have truncated the keys and values.

{'>gi|33112219|sp|O93279.1|A4': 'MGETTAFVLLLVATLTRSSEIPADDTVG', '>gi|another protein': 'ANDITSSEQUENCE'}

The above dictionary could have come from the text:

>gi|33112219|sp|O93279.1|A4\n
MGETTAFVLLLVATLTRSSEIPADDTVG\n
>gi|another protein\n
ANDITS\n
SEQUENCE\n

Here are my suggestions:

It's a good practice to use a with statement to handle the opening and closing of files for you. Not only does it keep you from having to worry about closing every file that you open, it also simplifies your code.
You might consider taking the file I/O part out of your function altogether, thereby simplifying what your function does. Your function would still work exactly the same way, you would just be passing it the actual FASTA text (including linebreaks) as a string, rather than a filename. Your function call could like something like this:

with open("some_fasta.txt", "r") as f:
    txt = f.readlines()
    d = parse_fasta(txt, "this_is_a_key_id")

(I also took the liberty of renaming the function parse_fasta so as to better describe what it does.)
A simpler way to update a dictionary entry is dictionary["key"] = "new value" (as opposed to dictionary.update({"key": "new value"}).
Instead of adding all of the keys and values to the dictonary, and then going through them one by one and deleting them or replacing escape characters, you could simplify things by validating the entries before adding them to the dictionary. You can also use str.translate() as a simpler way to remove any escape characters from the string. Conveniently, you can specify the escape characters as a string, allowing you to represent them more concisely (see my revision at the end -- I managed to get your argument list down to one line!)

"fix\n\r\n\r\t me".translate(None, escape_chars)
#=> "fix me"

(I would also rename replace_id_list to escape_chars.)
You might consider changing if key_id not in line: to simply else:, to better reflect that your code either does one thing or the other, depending on whether or not key_id is in line.
Instead of updating the value of value_line and then updating the value in the dictionary to that updated value, you can just update the value in the dictionary directly.
The way your code works currently, a KeyError will be thrown if the first line doesn't contain the given key_id. A simple way to fix this would be to initially (before the for loop) define key_line to be something like "key_id not found" -- that way any lines at the beginning of the file that do not contain the key_id will be collected as the value of that key. A better solution would involve exception handling.
Another possible problem with your function is that you are deleting the entry for a key altogether if any subsequent "value" line has less than the minimum required length of characters. This might be the behavior you want, but I have a hunch that what you really want to do is to ignore any lines that are not of the minimum required length. I've taken that approach with my revision below.
As you currently have it, you are checking the length of each line, including any escape characters. You should check after removing the escape characters.
You should also remove the escape characters from the key lines.

Here is your code with my suggested revisions:

def parse_fasta(txt, key_id, escape_chars="\r\n\w\s\t\v\f", min_len=1):
    """Returns a dictionary representation of FASTA text. Every line containing the
       supplied key_id is added to the dictionary as a key, with every subsequent
       line until the next key_id line as its value."""
    d = dict()
    key = "key_id not found"
    for line in txt:
        line = line.translate(None, escape_chars)
        if key_id in line:
            key = line
            d[key] = ""
        else:
            if len(line) >= min_len:
                d[key] += line   # (or optionally, d[key] += line + "\n"
                                 # if you want to keep the lines separate)
    return d


Merge/Insertion sort program taking longer than an hour to finish - C++ My program uses merge and insertion sort to sort a set of numbers.
It works perfectly fine for small inputs, less than 2 seconds for 1,000 ints. But I need to sort 1,000,000 ints. When I try with 1 million, it takes over 2 hours to sort. Can someone please help me optimize this program so it works faster?
FYI: I have several versions of this program because I have been trying to make it faster. Originally, my program would read 1 million ints from a text file, sort, then output the sorted numbers to a text file. But I assumed that was taking a large portion of the running time so I took that out completely.
Instead, I initialized the vector with 1,000,000 ints in the main program, like so:
vector<int> vec {7000, 8090, 189, 19, 0, 29032, ... ,90};

Again, my program works great with small input sizes. Any advice? Thank you!
#include<iostream>
#include<vector>
#include <time.h>
#include <stdio.h>
#include<fstream>

using namespace std;

void merge(vector<int> &vec, int left, int center, int right, int originalsize)
{
   int leftVsize, rightVsize; //left and right vector sizes
   vector<int> vec1;

   leftVsize = center-left+1;            //calculating left and right temporary vector sizes
   rightVsize = right - center;

   vector<int> tempLeft(leftVsize);     //initializing temp left and right vectors
   vector<int> tempRight(rightVsize);

   for(int i = 0; i < tempLeft.size(); ++i)
   {
     tempLeft.at(i) = vec.at(left+i);      //initializing left vector
   }

   for(int i = 0; i < tempRight.size(); ++i)
   {
      tempRight.at(i) = vec.at(i+center+1);   //initializing right vector
   }

   int i = left, j = 0, k = 0;

   while((j < tempLeft.size()) && (k < tempRight.size()))      //while left and right vector have elements
   {
      if(tempLeft.at(j) <= tempRight.at(k))     //if left element is smaller
      {
        vec.at(i) = tempLeft.at(j);   //add value to original vector
        j++;
      }
      else      //else
      {
        vec.at(i) = tempRight.at(k);      //add value to original vector
        k++;
      }
     i++;
   }

 while(j < tempLeft.size())      //while left vector has elements
 {
    vec.at(i++) = tempLeft.at(j++);
 }

 while(k < tempRight.size())     //while right vector has elements
 {
    vec.at(i++) = tempRight.at(k++);
 }

}

void insertionSort(vector<int> &vec, int originalsize)
{
   for(int i = 1; i < originalsize; ++i)     //starting from 1 for original vector size
  {
    int tempval = vec[i];      //set tempval to vector value at 1
    int j = i;                    //j now equals i

    for(j = i; ((j > 0)&&(tempval < vec[j-1])); --j)    //for j=i while j is greater than 0 and tempval is less than the number before it
    {
      vec[j] = vec[j-1];    //set vector[j] to vector[j-1]
 }
      vec[j] = tempval;   //tempval now holds vec[j]
}
}

void sort(vector<int> &vec, int left, int right, int originalsize)
{
  int insertion = right - left;
  if(insertion <= 8)    //if righ-left is less than or equal to 8
  {
     insertionSort(vec, originalsize);       // call insertion sort
  }
  if(left < right)
  {
    int center = (left+right)/2;    //calculating center of vector
    sort(vec, left, center, originalsize);    //calling sort for temp vector
    sort(vec, center+1, right, originalsize);   //calling sort for temp vector
    merge(vec, left, center, right, originalsize);    //calling merge to merge two vectors together
  }
 }

int main()
{
  vector<int> vec { 1 million ints };
  int temp;
  clock_t q, q1, q2,t;

  int orgsize = vec.size();

  q=clock();

  sort(vec, 0, (vec.size()-1), orgsize);    //calling sort function

  q=clock()-q;

  cout << "Total Time: "<< ((float)q)/CLOCKS_PER_SEC <<"\n";
 return 0;
}

Performance
For the moment, let's look at only one small part:
void sort(vector<int> &vec, int left, int right, int originalsize)
{
  int insertion = right - left;
  if(insertion <= 8)    //if righ-left is less than or equal to 8
  {
     insertionSort(vec, originalsize);       // call insertion sort
  }
  if(left < right)
  {
    int center = (left+right)/2;    //calculating center of vector
    sort(vec, left, center, originalsize);    //calling sort for temp vector
    sort(vec, center+1, right, originalsize);   //calling sort for temp vector
    merge(vec, left, center, right, originalsize);    //calling merge to merge two vectors together
  }
 }

This has a couple of problems. The first has already been pointed out: when you call insertionSort, you're telling it to sort the entire array, rather than just the small section you're currently dealing with.
From there, however, it just gets worse. Because after you do an insertion sort on the entire array, you don't just return can call it good. You continue to call sort recursively as long as left < right.
So that means when you get a partition down to 8 elements, you do insertion sort on the entire array. Then you create two partitions of 4 elements--and for each of them, you insertion sort the whole array again. Then from each of those you create two partitions of 2 elements--and for each of them...yup, you insertion sort the whole array yet again.
So not only are you do an \$O(N^2)\$ insertion sort on the whole array--with the array of a million elements, you're doing it 500,000 + 250,000 + 125,000 = 875,000 times!
Comments
Some of your comments are quite good:
 while(j < tempLeft.size())      //while left vector has elements
 {
     vec[i++] = tempLeft[j++];
 }

That's a real help in showing what your intent was. Some of the other comments aren't nearly so helpful though. For example, none of these strikes me as being very helpful at all:
   for(int i = left; i < right; ++i)     //starting from 1 for original vector size
  {
    int tempval = vec[i];      //set tempval to vector value at 1
    int j;

    for(j = i; ((j > 0)&&(tempval < vec[j-1])); --j)    //for j=i while j is greater than 0 and tempval is less than the number before it
    {
      vec[j] = vec[j-1];    //set vector[j] to vector[j-1]
     }
      vec[j] = tempval;   //tempval now holds vec[j]
}

The last of those goes beyond useless and into misleading (sounds like you think it assigned from vec[j] to tempval, when you actually did the reverse).
Indentation
Indentation is an important tool in keeping code understandable. The precise parameters you choose don't seem to matter a whole lot (within reason), but whatever you do, do it consistently. This code is sadly lacking in that respect.


C++ Calculator for complex numbers - follow-up After following the suggestions from the first question on that topic (link), I'd like to show you the result now:

#include <iostream>

class ComplexNumber {

    private:
        double real;
        double imaginary;

    public:
        ComplexNumber operator+(ComplexNumber b) {

            //Just add real- and imaginary-parts
            double real = this->real + b.real;
            double imaginary = this->imaginary + b.imaginary;
            ComplexNumber c = ComplexNumber(real, imaginary);
            return c;
        }

        ComplexNumber operator-(ComplexNumber b) {

            //Just subtract real- and imaginary-parts
            double real = this->real - b.real;
            double imaginary = this->imaginary - b.imaginary;
            ComplexNumber c = ComplexNumber(real, imaginary);
            return c;
        }

        ComplexNumber operator*(ComplexNumber b) {

            //Use binomial theorem to find formula to multiply complex numbers
            double real = this->real * b.real - this->imaginary * b.imaginary;
            double imaginary = this->imaginary * b.real + this->real * b.imaginary;
            ComplexNumber c = ComplexNumber(real, imaginary);
            return c;
        }

        ComplexNumber operator/(ComplexNumber b) {

            //Again binomial theorem
            double real = (this->real * b.real + this->imaginary * b.imaginary) / (b.real * b.real + b.imaginary * b.imaginary);
            double imaginary = (this->imaginary * b.real - this->real * b.imaginary) / (b.real * b.real + b.imaginary * b.imaginary);
            ComplexNumber c = ComplexNumber(real, imaginary);
            return c;
        }

        void printNumber(char mathOperator) {
            std::cout << "a " << mathOperator << " b = " << this->real << " + (" << this->imaginary << ") * i" << std::endl;
        }

    /*
     * Constructor to create complex numbers
     */
    ComplexNumber(double real = 0.0, double imaginary = 0.0) {
        this->real = real;
        this->imaginary = imaginary;
    }
};

int main() {

    /*
     * Variables for the real- and imaginary-parts of
     * two complex numbers
     */
    double realA;
    double imaginaryA;
    double realB;
    double imaginaryB;

    /*
     * User input
     */
    std::cout << "enter real(A), imag(A), real(B) and imag(B) >> ";
    std::cin >> realA >> imaginaryA >> realB >> imaginaryB;
    std::cout << std::endl;

    /*
     * Creation of two objects of the type "ComplexNumber"
     */
    ComplexNumber a(realA, imaginaryA);
    ComplexNumber b(realB, imaginaryB);

    /*
     * Calling the functions to add, subtract, multiply and
     * divide the two complex numbers.
     */
    ComplexNumber c = a + b;
    c.printNumber('+');

    c = a - b;
    c.printNumber('-');

    c = a * b;
    c.printNumber('*');

    c = a / b;
    c.printNumber('/');

    return 0;
}

If you have any suggestions on further improving the code, I would really appreciate it if you share them with me.

Use field initialization lists:

So your constructor

ComplexNumber(double real = 0.0, double imaginary = 0.0) {
    this->real = real;
    this->imaginary = imaginary;
}

Can become:

ComplexNumber(double real = 0.0, double imaginary = 0.0)
    : real(real), imaginary(imaginary) { }

Simplify your returns

I could see an argument for making an extra ComplexNumber to hold your return value if you need to further modify it or if the name of that variable is explanatory in showing what the return means, but as it stands, your c is neither of those.

Simplify

ComplexNumber c = ComplexNumber(real, imaginary);
return c;

To just

return ComplexNumber(real, imaginary);

Make your operator functions const

Since you (correctly) don't modify a when you do a + b, the operator function can (and should) be declared const. That way, even if you have a const object, you'll still be able to call it (and if you accidentally try to modify the member variable, you'll know immediately in the form of a compilation error).

That'd look like:

ComplexNumber operator+(const ComplexNumber &b) const {

Notice I've also declared b as const here since you shouldn't be modifying it either. I've also passed it by reference to save you some overhead.

Make your class printable with std::cout

Your printNumber is very specific. In fact, if you ever want to use this class for anything other than simply showing arithmetic results, that print may not be what you want. Instead, I'd make a generic str() that just returns a string version of the complex number. Something like:

std::string str() {
    std::ostringstream oss;
    oss << this->real << " + (" << this->imaginary << ") * i";
    return oss.str();
}

And then in the global scope, you can overload the << operator for std::cout:

std::ostream& operator<<(std::ostream &os, const ComplexNumber &cn) {
    return os << cn.str();
}

And now when you want to print it in main(), you can say:

std::cout << "a + b = " << a + b << std::endl;
std::cout << "a - b = " << a - b << std::endl;
std::cout << "a * b = " << a * b << std::endl;
std::cout << "a / b = " << a / b << std::endl;

Look at how easy that becomes to read and understand!


Stack data structure implementation I have implemented in C# a stack data structure, without using any existing C#/.NET containers (data structures). I have used TDD technique to implement this requirements, so you can find tests, too. All my tests are passing, so I guess stack is behaving correctly.

I would be very pleased to have a code review for this implementation.

Please do not recommend a generic implementation :-)

using System;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using StackDataStructure;

namespace StackTesting
{
    [TestClass]
    public class Tests
    {
        [TestMethod]
        public void TestEmptyStackSize()
        {
            var stack = new Stack();
            Assert.IsTrue(stack.IsEmpty());
        }

        [TestMethod]
        [ExpectedException(typeof(InvalidOperationException))]
        public void EmptyStackPopException()
        {
            var stack = new Stack();
            stack.Pop();
        }

        [TestMethod]
        public void PushingAndPoppintItemsToStack()
        {
            var stack = new Stack();
            stack.Push(1);
            Assert.AreEqual(1, stack.Size);
            stack.Push(2);
            Assert.AreEqual(2, stack.Size);
            Assert.AreEqual(2, stack.Pop());
            Assert.AreEqual(1, stack.Size);
            Assert.AreEqual(1, stack.Pop());
            Assert.IsTrue(stack.IsEmpty());
            Assert.IsTrue(stack.Size == 0);
            stack.Push(10);
            Assert.IsTrue(stack.Size == 1);
            Assert.IsTrue(10 == stack.Pop());
            Assert.IsTrue(stack.IsEmpty());
        }
    }
}

using System;

namespace StackDataStructure
{
    internal class Node
    {
        internal int value;
        internal Node underTop;
    }

    public class Stack
    {
        private int size;
        private Node top;

        public int Size { get { return size; } }

        public bool IsEmpty()
        {
            return top == null;
        }

        public int Pop()
        {
            if (IsEmpty())
                throw new InvalidOperationException("Stack is empty");

            int value = top.value;
            top = top.underTop;
            size--;
            return value;
        }

        public void Push(int v)
        {
            top = new Node{
                value = v,
                underTop = top
            };
            size++;
        }
    }
}

The code and the test look very good. I only found these nit-picks:

There's a typo in the method name Poppint.
The test sometimes uses Assert.IsTrue(a == b), which should be Assert.IsEqual(a, b).
The name underTop does not sound perfect to me, since it refers to a top, which is meaningless for a single node. A single node doesn't have a top, therefore I would call it next or below.
The property Size should be renamed to Count to align with the other collection types.
I prefer to have the same namespace for the main code and the testing code. But I don't know the official convention for organizing tests.


SQL update statement I have a SQL update statement which does what it's supposed to. However, I have a feeling that the way I wrote it isn't the best. I am not highly experienced in SQL so any pointers or hints on how can I rewrite this statement and possibly get rid of the duplicate joins (joining on same tables again in the where statement is what bothers me about this update statement) would be helpful.

UPDATE Item SET AttachementID = NULL
    FROM Item i
    JOIN BidItem ebi ON i.BidItemID = ebi.BidItemID
    JOIN Group eg ON ebi.GroupID = eg.GroupID
    WHERE eg.EstimateID = @JobID and i.AttachementID NOT IN (
        SELECT i.ItemID FROM Item i
            JOIN BidItem ebi ON i.BidItemID = ebi.BidItemID
            JOIN Group eg ON ebi.GroupID = eg.GroupID
            WHERE eg.EstimateID = @JobID
    )

I don't recommend the use of a FROM clause with UPDATE, as it is non-standard SQL.  Neither Oracle nor MySQL supports UPDATE … FROM ….  PostgreSQL does support FROM, but the documentation states that it is a PostgreSQL extension.  Even worse, it interprets the FROM clause differently from Microsoft SQL Server!  (PostgreSQL treats the Item i in the FROM clause as a self-join with the Item table in the main clause of the UPDATE; SQL Server treats it as an alias of the same table.)

I suggest using a Common Table Expression to reduce the redundancy.  CTEs with UPDATE are still non-standard, but at least PostgreSQL and SQL Server interpret the query in the same way.

WITH ItemsForEstimate AS (
    SELECT ItemID
        FROM Item
            INNER JOIN BidItem
                ON BidItem.BidItemID = Item.BidItemID
            INNER JOIN [Group]
                ON [Group].GroupID = BidItem.GroupID
        WHERE EstimateID = @EstimateID
)
UPDATE Item SET AttachementID = NULL
    WHERE
        ItemID IN (SELECT ItemID FROM ItemsForEstimate)
        AND AttachementID NOT IN (SELECT ItemID FROM ItemsForEstimate);

Since GROUP is an SQL keyword, it should be quoted as an identifier.  (I'm surprised that your query worked at all.)

AttachementID should be spelled AttachmentID.


Local (In-Process) Publish/Subscribe I have a handful of objects that don't have references to each-other that all need to receive an update.

From what I've read, it seems like a Publish-Subscribe Messaging Channel is an appropriate pattern. Searching for an existing solution mainly turned up messaging libraries for application integration (between processes). Those solutions were much more than what I needed, so I wrote this simple Publisher.

Questions:

Is there already something simple like this in the .NET framework that I overlooked? Or another light-weight library?
Is this an appropriate implementation and application of the Pub/Sub pattern? What might be done to improve it?
What gotchas are there? For example, I presume I need to Unsubscribe in the subscriber's finalizer.

public interface ISubscriber
{
    void SubscriptionUpdate(object message);
}

public interface IPublisher
{
    void Subscribe(ISubscriber subscriber, string channel);
    void Unsubscribe(ISubscriber subscriber, string channel);
    void Publish(string channel, object message);
}

public class Publisher : IPublisher
{
    static Publisher _instance = new Publisher();
    public static Publisher Instance { get { return _instance; } }
    Dictionary<string, List<ISubscriber>> _subscribers = new Dictionary<string, List<ISubscriber>>();

    public void Subscribe(ISubscriber subscriber, string channel)
    {
        if (!Exists(channel))
            _subscribers[channel] = new List<ISubscriber>();
        if (!_subscribers[channel].Contains(subscriber))
            _subscribers[channel].Add(subscriber);
    }

    public void Unsubscribe(ISubscriber subscriber, string channel)
    {
        if (Exists(channel))
            _subscribers[channel].Remove(subscriber);
    }

    public void Publish(string channel, object message)
    {
        if (!Exists(channel)) return;
        foreach (var subscriber in _subscribers[channel])
            subscriber.SubscriptionUpdate(message);
    }

    bool Exists(string channel)
    {
        return _subscribers.ContainsKey(channel);
    }
}

Don't think it's a good approach.

Singletons are evil by themselves
Subscribers are likely to expect a specific type of message on specific channel
Magic strings that specify the channel name are also potential source of bugs
Incorrect messages or posts to wrong channels are detected at run-time only.
Logical dependency between objects still exist, but is hidden.

.NET already has a notion of in-process publish/subscribe, and (not that surprisingly) this functionality is covered by events... All you need to do is to wire up all your entities properly, and that's your actual challenge is.

I would recommend using any of available IoC frameworks for that (I use Autofac at the moment).

You will have a strictly-typed communication protocol between publisher and subscriber
It will allow you to explicitly declare dependency on a certain external functionality in your classes.
It will simplify unit-testing of your classes

Unfortunately you haven't described specific use cases for your pub/sub requirement so it's hard to show this solution on specific example


Prints Books from a HashMap that are complete or incomplete This code is meant to display book titles that are incomplete. I have a HashMap with the book title and a number that represents the amount of the book that has been read.

I want the program to:

Check to see if there are any books in the HashMap
Check to see if there are any started, yet incomplete books
If there are any Started, yet incomplete books, it will print "You have x incomplete books. They are:"
Finally, it will print out a list of incomplete books

The code works, but I feel like it probably has too many loops and could be cleaned up a lot.

import java.util.HashMap;

public class Library {
  public Library () {

  }

public void getIncompleteBooks(HashMap<String, Integer> library) {
    int unfinished = 0;
    if (library.size() < 1) {
      System.out.println("Please add some books to your library.");
    } else {
      for (String book : library.keySet()) {
        if (library.get(book) < 100 && library.get(book) > 0) {
          unfinished++;
        }
      }
      if (unfinished > 0) {
          System.out.println("You have " + unfinished + " unfinished books. They are:");
        }
      for (String book : library.keySet()) {
        if (library.get(book) < 100 && library.get(book) > 0) {
          System.out.println(book);
        }
      }
    }
  }

Prefer interfaces

public void getIncompleteBooks(HashMap<String, Integer> library) {

As a general rule, you should favor interfaces over implementations as types.

public void getIncompleteBooks(Map<String, Integer> library) {

Now you can change to another Map implementation without changing this method at all.

Object fields

And of course, it would be more common to make library an object field.

private Map<String, Integer> library = HashMap<String, Integer>();

Then you wouldn't have to pass it as a parameter to objects of the class.

public void getIncompleteBooks() {

If you don't do that, you should declare the methods static so people don't expect them to use data from the object.

entrySet()

      for (String book : library.keySet()) {
        if (library.get(book) < 100 && library.get(book) > 0) {
          unfinished++;
        }
      }

While it will work, a keySet is the wrong tool here.  Consider

      for (Map.Entry<String, Integer> book : library.entrySet()) {
        if (book.getValue() < 100 && book.getValue() > 0) {
          unfinished++;
        }
      }

Now you aren't doing a hash operation just to iterate over the collection.

Separate logic and display

You mix your data operations with your display operations.  This can be fragile and hard to maintain, as you end up with many similar methods.  Consider how else you could write this.

public static List<Map.Entry<String, Integer>> getIncompleteBooks(Map<String, Integer> library) {
    List<Map.Entry<String, Integer>> results = new ArrayList<>();

    for (Map.Entry<String, Integer> book : library.entrySet()) {
        int percentFinished =  book.getValue();
        if (0 < percentFinished && percentFinished < 100) {
            results.add(book);
        }
    }

    return results;
}

I made this static because it uses none of the object fields of the class, just the parameters.

Adding the percentFinished variable does two things here.  One, it means that we only call the getValue() function once.  The compiler probably does that anyway.  Two, it makes clear what the value actual means.  This is what is called "self-commenting" code.  It replaces a comment that says, // getValue returns the percent of the book that is finished.

I switched to four column indent because that's more common in Java code.

This is what I would expect a getIncompleteBooks method to do.  It just filters out the books that you don't want.  You can then use it in multiple ways.

public static void displayTitles(Collection<Map.Entry<String, Integer>> books) {
    for (Map.Entry<String, Integer> book : books) {
        System.out.println(book.getKey());
    }
}

This is the basic display method.

public static void displayTitlesOfIncompleteBooks(Map<String, Integer> library) {
    if (library.isEmpty()) {
        System.out.println("Please add some books to your library.");
        return;
    }

    List<Map.Entry<String, Integer>> incompleteBooks = getIncompleteBooks(library);

    if (incompleteBooks.isEmpty()) {
        return;
    }

    System.out.println("You have " + incompleteBooks.size() + " unfinished books. They are:");
    displayTitles(incompleteBooks);
}

This should produce the same output as your original method.

Notice that it uses early return on exceptional conditions like an empty library or no unfinished books.  This avoids heavy nesting.

I used isEmpty rather than comparing the size to 0.  Functionally I can't think of a way that this will matter, but I find this version easier to read.

While this is more code for this specific problem, this allows you to solve other problems easily.  For example,

public static void displayTitles(Map<String, Integer> library) {
    displayTitles(library.entrySet());
}

or

public static double calculateFractionIncomplete(Map<String, Integer> library) {
    if (library.isEmpty()) {
        // consider throwing an exception instead
        System.out.println("Please add some books to your library.");
        return 0.0;
    }

    return getIncompleteBooks(library).size() / (double) library.size();
}

I cast the size to double so that Java wouldn't use integer arithmetic and truncate the result.


Computing the name of the month I created the following program that transforms a numerical month into the corresponding textual month. I initially calculated the starPoint and endPoint in the class constructor; then I changed that calculation to the getName method. Which is a better option? Calculating in the class constructor, or in a separate method like shown in the code?

/**
 * Created by Robert Holland on 25/11/2016.
 *
 * A class that accepts a number and calculates
 * the corresponding month. Once the month has
 * been calculated it will then be returned.
 *
 */

public class Month
{
    public static final String MONTHS = "January   " + "February  " + "March     " +
                                        "April     " + "May       " + "June      " +
                                        "July      " + "August    " + "September " +
                                        "October   " + "November  " + "December  ";
    private int monthNumber;

    //Construct a month class that has a value equal to the numerical month
    //@param uMonthNumber the number of the month to find the correspondance.
    public Month(int uMonthNumber)
    {
        monthNumber = uMonthNumber;
    }

    //Determine the name that corresponds to the month number.
    //@return month the name of the corresponding month.
    public String getName()
    {
        String month;
        int startPoint;
        int endPoint;

        endPoint = monthNumber * 10;
        startPoint = endPoint - 10;
        month = MONTHS.substring(startPoint, endPoint);

        return month;
    }
}

This exists already:

It's not the prettiest to use but it's there.

import java.time.*;
import java.time.format.*;
import java.util.*;

public class Example {

    public static void main(String[] args) {
        Month month = Month.of(2);
        System.out.printf("%s", month.getDisplayName(TextStyle.FULL, new Locale("en")));
        // outputs:
        // February
    }
}

Don't re-write code that's in your standard libraries :)


Make a beautiful binary string This is the "Beautiful Binary String" problem at HackerRank:

Alice has a binary string. She thinks a binary string is beautiful if and only if it doesn't contain the substring 010.
In one step, Alice can change a 0 to a 1 or vice versa. Count and print the minimum number of steps needed to make Alice see the string as beautiful.
For example, if Alice's string is 010 she can change any one element and have a beautiful string.

This is my Python code:
def beautifulBinaryString(b):
    temp = list(b)
    count,i = 0,0
    if len(b) == 3 and b == "010": count += 1
    elif len(b) == 3 and b != "010": count = count
    else:
        while (i+3 <= len(temp)):
            if temp[i:i+3] == ['0','1','0']:
                count += 1
                del temp[i:i+3]
            else: i += 1
    return count

It seems to me as having too many conditionals (though readable, I guess). Is there a more concise way to accomplish this?
Some test cases:
0101010
01100

2
0

Style

Read the PEP8 style guide!

Functions and variables should be snake_case
Conditions should be on the next line if a: ... is bad style
Conditions don't need parenthesis while (a) is the same as while a:
Avoid temp variables

Algorithm

Your first 2 guard clauses seem very unnecessary

When I remove them, the code still works.
Consider writing docstring/tests or both with the doctest module

Alternative Code

You could use re.findall(substring, string) for counting the occurrence,

OR string.count(substring) making this practically a one-line

import doctest

def beautiful_binary_string(b):
    """
    Returns the steps to make a binary string beautiful

    >>> beautiful_binary_string("0101010")
    2

    >>> beautiful_binary_string("01100")
    0

    >>> beautiful_binary_string("0100101010100010110100100110110100011100111110101001011001110111110000101011011111011001111100011101")
    10
    """
    return b.count("010")

if __name__ == '__main__':
    doctest.testmod()


Is the "current connected user" an information that should be shared with the domain? I'm working right now in a readers social network. The main points are this:

When an user connects with his account I save his user id in the
session variable.
My MVC is working with DTO's all the way, filling the necessary
variables for the presentation in the model.

So, everything was ok, until today that I had to do a function that returns a DTO that has all the information of an user profile page. User profile pages can be accessed by anyone who has an account (Like Facebook user pages).

This profile page, shows a button "Follow" if the profile is not the profile of the current user (to avoid you following yourself).
If we are following that user, it will display an unfollow button.

So I was thinking that the UserProfileDTO must have two boolean:

isTheCurrentUser and isFollowedByTheCurrentUser.

public class UserProfileDTO {
    private Long id;
    private String username;

    private Boolean isCurrentUser;
    private Boolean isfollowedByCurrentUser;
...

And the Domain Controller

public User getUserProfile(String username, Long currentUserId) throws UserNameDoesntExist {
         UserProfileDTO user = dataController.getUserByUsername(username);
         if(user != null) {
               user.setIsCurrentUser(currentUserId == user.getId());
           user.setIsFollowedByCurrentUser(dataController.isFollowed(user.getId(), currentUserId));
          ...

But I thought, "This DTO will be filled with information at the domain model. So the domain must have information that the presentation had first. That information is the current connected user, thus that will create a dependency between the two layers.

Am I correct? Should the domain model be ignorant about who the current connected user is? Or is what I am doing okay?

I'd create an UserProfilePageDTO to separate that information:

public class UserProfilePageDTO {

    private UserProfileDTO userProfile;

    private Boolean isCurrentUser;
    private Boolean isfollowedByCurrentUser;
    ...
}

UserNameDoesntExist - it seem unusual a little bit because of the nt. I'd call it UsernameDoesNotExists or UserNotFoundException (same pattern as ClassNotFoundException and FileNotFoundException from JDK).
I'd consider using a guard clause:

public User getUserProfile(String username, Long currentUserId)
        throws UserNotFoundException {
    UserProfileDTO user = dataController.getUserByUsername(username);
    if (user == null) {
        throw new UserNotFoundException(...);
    }
    user.setIsCurrentUser(currentUserId == user.getId());
    user.setIsFollowedByCurrentUser(dataController.isFollowed(user.getId(), currentUserId));
    ...
}


Pronounceable Name Generator I'm learning C# and have written a function which generates an easily-pronounceable fictional name by alternating between taking one vowel and one consonant. The one parameter, halfLength, will be the amount of vowels added and the amount of consonants added, meaning that the length of the returned string is double the given halfLength.

static string GeneratePronounceableName(int halfLength)
{
    char[] vowels = "aeiou".ToCharArray();
    char[] consonants = "bcdfghjklmnpqrstvwxyz".ToCharArray();
    List<char> chosenVowels = new List<char>();
    List<char> chosenConsonants = new List<char>();

    Random r = new Random();

    for (int i = 0; i < halfLength; i++)
    {
        chosenVowels.Add(vowels[r.Next(0, 5)]);
        chosenConsonants.Add(consonants[r.Next(0, 21)]);
    }

    return string.Join("", chosenConsonants.Zip(chosenVowels, (v, c) => v.ToString() + c.ToString()));
}

Is there a way this can be made shorter, more idiomatic or more efficient?

int halfLength

Don't make the user think to much when specifying parameters if this can be avoided. Calculate the target length and let the user specify the length he needs.

If the name should always have an even length then you could check if the given one is odd and if so then add 1.

length = length % 2 == 0 ? length : length + 1;

The loop just divides this by 2:

for (int i = 0; i < length / 2; i++)

I do this because I assumed a longer string is better then a short one so I add 1 to odd lengths but this is actually not necessary as C# will round the number to the next lower int when dividing. This means that 5 / 2 == 2 as far as int division is concerned. If you're ok with shorter strings then you don't need the line with length + 1

char[] vowels = "aeiou".ToCharArray();
char[] consonants = "bcdfghjklmnpqrstvwxyz".ToCharArray();

You don't need to ToCharArray these because strings are already arrays. You can just define two constants:

const string vowels = "aeiou";
const string consonants = "bcdfghjklmnpqrstvwxyz";

List<char> chosenVowels = new List<char>();
List<char> chosenConsonants = new List<char>();

You don't need two lists that you join later. You can use the StringBuilder and create the name right away.

chosenVowels.Add(vowels[r.Next(0, 5)]);
chosenConsonants.Add(consonants[r.Next(0, 21)]);

Don't use magic numbers. Use the constants you have to get the length:

r.Next(0, 5)

You can use the overload that takes just the maxValue.

v.ToString() + c.ToString()

When building longer strings with lot's of concatenations use specialized methods or objects. Concatenating strings with the + operator is the slowest possible way to create a string. The StringBuilder is such an object. Highly optimized for string creation.

With all those optimizations the final result could look like this:

static string GeneratePronounceableName(int length)
{
    const string vowels = "aeiou";
    const string consonants = "bcdfghjklmnpqrstvwxyz";

    var rnd = new Random();
    var name = new StringBuilder();

    length = length % 2 == 0 ? length : length + 1;

    for (var i = 0; i < length / 2; i++)
    {
        name
            .Append(vowels[rnd.Next(vowels.Length)])
            .Append(consonants[rnd.Next(consonants.Length)]);
    }

    return name.ToString();
}

Alternatively to the StringBuilder you could use in this example a char[] array and build the string yourself:

static string GeneratePronounceableName(int length)
{
    const string vowels = "aeiou";
    const string consonants = "bcdfghjklmnpqrstvwxyz";

    var rnd = new Random();

    length = length % 2 == 0 ? length : length + 1;

    var name = new char[length];

    for (var i = 0; i < length; i += 2)
    {
        name[i] = vowels[rnd.Next(vowels.Length)];
        name[i+1] = consonants[rnd.Next(consonants.Length)];
    }

    return new string(name);
}


Perform instruction in loop every time except the last time? The specific issue is this: I am writing to a file and want to output a new line after each line written.  If I use a normal loop without any further checks, this will create a blank line at the very end of the file for no reason.  So I need to do this every time except the last one.

Despite the fact that this is a specific problem, I'm actually looking for a better general coding practice to deal with scenarios like this, since it isn't the first and won't be the last time I have to deal with it.

Here's my unsatisfying solution:

//Write contents to the file
BufferedWriter writer = new BufferedWriter(new FileWriter(file));
for(int i = 0; i < lines.size(); i++) {
    writer.write(lines.get(i));
    if(i < lines.size() - 1) writer.newLine();
}

It seems wasteful to check the conditions twice through each iteration of the loop, and I feel like there ought to be a better way to accomplish what I want without the vague code smell.  Anyone have any cool tips or tricks to make this more elegant?

It also prevents me from using an enhanced for loop, which makes me sad.

lines is a List<String>.

Also, for those saying I should just join all the Strings with \n, that's not an adequate solution.  Firstly, it doesn't actually address the general coding practice.  Secondly, when writing to a file with a BufferedWriter, it's important to use newLine() rather than writing a \n.

I presume lines is a Collection of some sort. One option that has slightly less of a smell (although it still is odoriferous), is to use an iterator, which will essentially do the same work, but will be more readable:

for (Iterator<String> it = lines.iterator(); it.hasNext();) {
    writer.write(it.next());
    if (it.hasNext()) {
        writer.newline();
    }
}

As I say, all this does is make it more readable....

Other options are to duplicate the write - once in the loop, and then the last one outside the loop:

if (!lines.isEmpty()) {
    int limit = lines.size() - 1;
    for (int i = 0; i < limit; i++) {
        ....
    }
    writer.write(lines.get(limit));
}

EDIT: @tomdemuyt suggested reversing the newline to happen only after the first line as follows:

if (!lines.isEmpty()) {
    writer.write(lines.get(0));
    // start index at 1 instead of 0.
    for (int i = 1; i < lines.size(); i++) {
        writer.newline();
        writer.write(lines.get(limit));
    }
}


Printing an hourglass pattern In a recent interview I have been asked to write a program to generate the following output:

* * * * *
 * * * *
  * * *
   * *
    *
   * *
  * * *
 * * * *
* * * * *

I had written the following code in java

public class Triangle
{
    public static void main( String[] args )
    {
        show( 5 );
    }

    public static void show( int n )
    {
        for ( int i = 0; i < n - 1; i++ )
        {
            for ( int j = 0; j < i; j++ )
            {
                System.out.print( " " );
            }
            for ( int k = n - i; k > 0; k-- )
            {
                System.out.print( "* " );
            }
            System.out.println();
        }
        for ( int i = 0; i < n; i++ )
        {
            for ( int j = n - i; j > 1; j-- )
            {
                System.out.print( " " );
            }
            for ( int k = 0; k < i + 1; k++ )
            {
                System.out.print( "* " );
            }
            System.out.println();
        }
    }
}

But the interviewer was not happy. He asked me minimize the number variables I had used. I didn't found a way. Anyone could help? Also, could there be a possibility to optimize the iteration here?

The biggest performance boost you could make would be to issue fewer print() and println() calls.  Each call involves a lot of overhead for locking, which you normally don't think about.  Coalesce everything down to one single print() for optimal performance.

The number of variables isn't that bad.  The problem is, rather, that i, j, k, and n are cryptic and headache-inducing.  The headache remedy would be to introduce a helper function and rename the variables to be more meaningful.  Note that in assigning meaning to the variables, you'll want to reverse one of the i loops to be a countdown, so that i serves the same role in both loops.

public class Triangle {
    public static void show(int maxWidth) {
        // Top
        for (int width = maxWidth; width > 1; width--) {
            System.out.println(repeat(" ", maxWidth - width).append(
                               repeat("* ", width)));
        }

        // Middle and bottom
        for (int width = 1; width <= maxWidth; width++) {
            System.out.println(repeat(" ", maxWidth - width).append(
                               repeat("* ", width)));
        }
    }

    private static StringBuilder repeat(CharSequence block, int n) {
        StringBuilder sb = new StringBuilder(block.length() * n);
        while (n --> 0) {
            sb.append(block);
        }
        return sb;
    }

    public static void main(String[] args) {
        show(5);
    }
}

You could try writing one loop to handle both the top and bottom triangles.  Unfortunately, this clever solution is not as elegant as it could be, due to the need to skip 0.

    public static void show(int maxWidth) {
        for (int i = -maxWidth + 1; i < maxWidth; i++) {
            int width = Math.abs(i) + 1;
            System.out.println(repeat(" ", maxWidth - width).append(
                               repeat("* ", width)));
        }
    }


Speeding up an algorithm for finding the number of distinct lines Is there a way to speed up the following python code:

auxdict = {0:0}
divdict = {}

def divisors(n):
    divisors = []
    for i in range(1, int(n**0.5) + 1):
        if (n % i == 0):
            divisors.extend([i, n/i])
    divisors = list(set(divisors))
    divisors.sort()
    return divisors

def aux(k):
    if k in auxdict:
        return auxdict[k]
    else:
        if not k in divdict:
            divdict[k] = divisors(k)
        r = sum(map(aux, divdict[k][:len(divdict[k])-1]))
        auxdict[k] = (k+1)**3 - k**3 - r
        return auxdict[k]

def lines(k):
    result = 0
    for i in range(int(k/2),0,-1):
        result += int(k/i-1) * aux(i)
    return (k+1)**3 - result - 1

for i in [10**6, 10**10]:
    print i, lines(i)

It is a modified version of a Mathematica code I found. The code finds the maximum number of distinct lines in a 3D cube of lattices of size n: http://oeis.org/A090025

The problem is the code is very slow and doesn't work for larger values. I want to evaluate lines(10**10)

You can improve your divisors function:

Version 1: (not includes n)

def divisors(n):
    """Return the divisors of n (including 1)"""
    if(n == 1):
        return (0)

    divs = set([1])
    r = floor(sqrt(n))
    if(r**2 == n):
        divs.add(r)

    f = 2
    step = 1
    if(n & 1 == 1):
        f = 3
        step = 2

    while(f <= r):
        if(n % f == 0):
            divs.add(n/f)
            divs.add(f)
        f = f + step

    return sorted(divs)

Version 2: (includes n) from here

def divisors2(n) :
    """
    Generates all divisors, unordered, from the prime factorization.
    """
    factors = factorI(n)
    ps = sorted(set(factors))
    omega = len(ps)

    def rec_gen(n = 0) :
        if n == omega :
            yield 1
        else :
            pows = [1]
            for j in xrange(factors.count(ps[n])) :
                pows += [pows[-1] * ps[n]]
            for q in rec_gen(n + 1) :
                for p in pows :
                    yield p * q

    for p in rec_gen() :
        yield p

where factorI is:

def factorI(n):
    """Returns the factors of n."""
    lst = []
    num = int(n)
    if(num & 1 == 0):
        lst.append(2)
        num /= 2
        while(num & 1 == 0):
            lst.append(2)
            num /= 2

    factor = 3
    maxFactor = sqrt(num)
    while num>1 and factor <= maxFactor:
        if(num % factor == 0):
            num /= factor
            lst.append(factor)
            while(num % factor == 0):
                lst.append(factor)
                num /= factor
            maxFactor = sqrt(num)
        factor += 2

    if(num > 1):
        lst.append(num)
    return lst

For efficiency reasons you could also make generator functions.


Design a simple card game in JavaScript I wrote a simple card game in JavaScript and I wonder if anyone could give me some advice and make some improvements on my code.

Here is the demo that I build:
https://codesandbox.io/s/cardgame-his95?file=/src/index.js

So it is a 2 person card game. Every card has a number and also has a pattern.
The game goes like this: we have a set of pre-defined rules, and these rules have a ranking. The player that satisfies the highest-ranked rule will win. And there could be tie if they end up with the same ranking. The goal here is not just to make the game work, also I wanted to account for maintainability. For example, we can easily add new rules and swap the ranking and re-define the rule if possible.

There are mainly a couple of classes.
First is the Game class

class Game {
  constructor({ play1, play2, rules, messages }) {
    this.play1 = play1;
    this.play2 = play2;
    this.rules = rules;
    this.messages = messages;
  }
  play() {
    let rankOfP1 = Infinity;
    let rankOfP2 = Infinity;
    for (const rule of this.rules) {
      if (rule.validator(this.play1)) {
        rankOfP1 = rule.rank;
        break;
      }
    }
    for (const rule of this.rules) {
      if (rule.validator(this.play2)) {
        rankOfP2 = rule.rank;
        break;
      }
    }

    return rankOfP1 === rankOfP2
      ? this.messages.tie
      : rankOfP1 < rankOfP2
      ? this.messages.win.player1
      : this.messages.win.player2;
  }
}

Here the rules are an array of rule object where every object looks like this

{
    description: "Six Cards of the same pattern",
    rank: 1,
    validator: cards => {
      return hasSamePattern(cards, 6);
    }

The lower the rank gets, the more important this rule is. So If player1 satisfies a rule with rank 1, and player1 satisfies a rule with rank 2, then we say player1 won.
And validator is the function that takes an array of card object and returns a boolean to determine if the set of cards satisfy the rule.

And lastly we have a Card class which is pretty simple

class Card {
  constructor({ pattern, number }) {
    this.pattern = pattern;
    this.number = number;
  }
}

Please take a look and feel free to make some improvements on it. Also please suggest some better naming for variables if possible, I am not a native English speaker so some of the names of the variables would be a bit weird.

Finally I wrote this game in a OOP style. I know that JavaScript is not the best language out there when it comes to OOP. Also I am not really good at Object Oriented Design. I wonder if anyone knows how to rewrite the game in functional programming style. That would be super cool!

Your constructor has

constructor({ play1, play2, rules, messages }) {
  this.play1 = play1;
  this.play2 = play2;
  this.rules = rules;
  this.messages = messages;
}

You may as well Object.assign the parameter to the instance instead:

constructor(config) {
  Object.assign(this, config);
}

pattern is a slightly odd name for what it represents here - the usual English word for one of the clubs, diamonds, etc, is suit. rule is a bit strange as well - a rule usually refers to the process by which a game is played (eg "Hands consist of 6 cards" or "The player with the best hand wins"). To describe the different possible winning combinations and their ranks, I'd use the word handRanks or something similar. play1 and play2 aren't great descriptors either - these represent the cards held in each player's hand, so maybe use player1Cards or player1Hand.

With regards to the play() method, when you want to find an item in an array which fulfills a condition, it would be more appropriate to use .find, rather than a for loop - find more clearly indicates what the intention of the loop is, and is more concise. You also need to set the rank to Infinity if no handRanks pass - why not integrate this Infinity into the handRanks array itself? You're also writing the looping code twice - you can make it more DRY by putting it into a function, and calling that function twice instead.

new Card({ suit: "spade", number: 1 }),  // <-- Suit

new HandRank({  // <-- HandRank
  description: "Six Cards of the same suit",  // <-- Suit
  rank: 1,
  validator: cards => {
    return hasSameSuit(cards, 6);  // <-- hasSameSuit, not hasSamePattern
  }
}),
new HandRank({  // <-- HandRank
  description: "Nothing special",
  rank: Infinity,                 // <-- add this whole new HandRank
  validator: cards => true,
}),

getRank(cards) {
  return this.handRanks.find(({ validator }) => validator(cards)).rank; // <-- this.handRanks
}
play() {
  const rankOfP1 = this.getRank(this.player1Cards); //  <-- player1Cards
  const rankOfP2 = this.getRank(this.player2Cards); //  <-- player2Cards
  return rankOfP1 === rankOfP2
    ? this.messages.tie
    : rankOfP1 < rankOfP2
      ? this.messages.win.player1
      : this.messages.win.player2;
}

One of the benefits of using arrow functions is that if the function contains only a single expression which is immediately returned, you can omit the { } brackets and the return keyword, if you want to make things concise, eg for the hasSameSuit test above:

validator: cards => hasSameSuit(cards, 6),

If you want to find if any item in an array passes a test, but you don't care about which item passes the test, you should use .some, not .find. (.some returns a boolean indicating whether any passed, .find returns the found item) For the hasSamePattern (or hasSameSuit) method, use:

return Object.values(patterns).some(num => num >= threshold);

Your hasConsecutiveNums method has the bug mentioned in the comments previously - a hand of [1, 2, 2, 3] will not pass a 3-consecutive-number test because the sorted array will contain 2 twice, failing if (prevNum + 1 === num) {. De-duplicate the numbers with a Set first.

const nums = [...new Set(cards.map(card => card.number).sort((a, b) => a - b))];

  I wonder if anyone knows how to rewrite the game in functional programming style.

Javascript isn't entirely suited to completely functional programming either, though it can get most of the way there. To start with, make your functions pure, and avoid side-effects and mutations. For example, assigning to a property of the instance with this.play1 = play1; (or this.player1Cards = player1Cards;) is a mutation. None of your code fundamentally requires anything non-functional (except the console.log at the very end, which is unavoidable), so it should be pretty easy to convert - rather than assigning to properties, just keep the variables in a closure, and return a function for the play method, eg:

const makeGame = ({ player1Cards, player2Cards, handRanks, messages }) => () => {
  // getRank is now a standalone function which takes a handRanks parameter
  const rankOfP1 = getRank(player1Cards, handRanks);
  const rankOfP2 = getRank(player2Cards, handRanks);
  return rankOfP1 === rankOfP2
    ? messages.tie
    : rankOfP1 < rankOfP2
      ? messages.win.player1
      : messages.win.player2;
};

const play = makeGame({ ... });
console.log(play());


SessionHandlerInterface Class The following class is used for handling sessions:

class DBHandler implements SessionHandlerInterface {

    protected $exists;

    public function open($save_path, $name) {
        try {
            DBCxn::get();
            return true;
        } catch(PDOException $e) {
            return false;
        }
    }

    public function close() {
        return true;
    }

    public function destroy($session_id) {
        $sth = DBCxn::get()->prepare("DELETE FROM sessions WHERE session_id = ?");
        $sth->execute(array($session_id));
        return true;
    }

    public function gc($maxlifetime) {
        $sth = DBCxn::get()->prepare("DELETE FROM sessions WHERE last_update < ?");
        $sth->execute(array(time() - $maxlifetime));
        return true;
    }

    public function read($session_id) {
        $sth = DBCxn::get()->prepare("SELECT session_data FROM sessions WHERE session_id = ?");
        $sth->execute(array($session_id));
        $rows = $sth->fetchALL(PDO::FETCH_NUM);
        if (count($rows) == 0) {
            $this->exists = "n";
            return '';
        }
        else {
            $this->exists = "y";
            return $rows[0][0];
        }

    }

    public function write($session_id, $session_data) {

        if ($this->exists == "y") {
            $sth = DBCxn::get()->prepare("UPDATE sessions SET session_data = ?, last_update = NOW() WHERE session_id = ?");
            $sth->execute(array($session_data, $session_id));
        }
        if ($this->exists == "n") {

            $sth = DBCxn::get()->prepare("INSERT INTO sessions (session_id, session_data, last_update) VALUES (?, ?, NOW())");
            $sth->execute(array($session_id, $session_data));
        }

    }
}

session_set_save_handler(new DBHandler);

How could this class be improved?

So there is a consideration that needs to be made when implementing your own session handler that is not really clear in the PHP documentation.  That is that the SessionHandlerInterface does not support the create_sid() method introduced in PHP 5.5.1 & 7.   For this reason, you might strongly consider extending the PHP SessionHandler class as opposed to implementing the interface if you want best compatibility with more current PHP releases.

Again, this is actually contrary to what the PHP documentation states, but it might help give context to this decision if you look through the the feature development conversation from the core PHP developers. In a nutshell, they didn't add create_sid() to the interface in order not to break backward compatibility.  I would particularly point you to the comment from It from Dec 14, 2012 for the most compelling discussion around interface vs. object inheritance.

By having access to create_sid() method call, you could do something like:

function create_sid()
{
    $sessionId = parent::create_sid();
    // create record in database with this new session id
}

The class name seems odd.  It is not just a "Db Handler" right?  It is Db-based session handler.  So perhaps DbSessionHandler, PdoSessionHandler (if PDO is really the dependency here) or similar might be more appropriate.

Why have all the calls to DBCxn::get() throughout the class? Since this is a critical dependency for this class, you should probably pass this dependency (i.e. a valid PDO object) to the constructor.  There should be no reason for this class to keep having to try to instantiate DB connections (even if perhaps you are using a singleton here).  How about usage like:

try {
    $pdo = new DbCxn::get();
    $dbSessionHandler = new DbHandler($pdo);
    session_set_save_handler($dbSessionHandler);
} catch (PdoException $e) {
    /* bail on execution or maybe don't even catch */
}

With constructor like:

public function __construct(Pdo $pdo)
{
    $this->pdo = $pdo;
}

This gives you the benefit of knowing that you have a valid DB connection via PDO before you even try to set the session save handler.  You can bail out in your bootstrapping process rather than having exceptions/error taking place when you are actually trying to interact with session functionality.

Your implementation is a little naive in terms of handling session destruction and garbage collection.  To implement sessions in a secure and resilient way, one should strongly consider thinking of session destruction as nothing more than setting as setting session data as invalid.  That session data might need to remain in order to handle things such as race conditions from asynchronous requests, such that you can maintain good user experience in your application. Most mature implementations require setting of timestamp-based controls around data validity and a separate process for data deletion.

So here your destroy() function might simply update a field on the record that the data is no longer valid and setting a TTL (maybe a few seconds, maybe longer depending on your application needs) on when the data should be eligible for final deletion via garbage collection.

Your gc() method should then look to delete either active sessions where last update is past maximum lifetime or inactive records that have passed the data validity TTL.

I don't understand the exists property at all.  It seems unnecessary.  You can use REPLACE_INTO to design away your two code branches in write() method (or change operational order of update/insert if you wanted to perhaps remain more database agnostic as suggested in another answer).

I also think that perhaps you are thinking of these method calls being made in more of a linear fashion than what might actually be presented.

You may want to take a look at this great article on how session handlers are called relative to session management code in PHP.  By looking at this, you can see that things like session ID change can happen between calls to the other methods.

I especially like this summary comment from the article:

  The simplest way to think about custom session handlers, is to avoid
  thinking of them as objects with instance methods, although that
  happens to be how it's implemented - instead, think of the methods as
  being static; or in other words, avoid state in your implementation,
  with the exception of accidental state (for optimization/caching
  purposes), and bear in mind, the original API for custom session
  handlers was a set of individual functions, essentially the analog to
  a set of static methods, not to an object.

Right now, you are adding state to your class.

How do you handle session forwarding?  When your code calls for session ID to be regenerated, how will you forward any calls from old session to new session?  This could obviously be handled outside the session handler itself, but it certainly ties into the same data expiry consideration I noted before.

Consider namespacing your session library.

    $rows = $sth->fetchALL(PDO::FETCH_NUM);
    if (count($rows) == 0) {
        $this->exists = "n";
        return '';
    }
    else {
        $this->exists = "y";
        return $rows[0][0];
    }

This seems like it could be simplified. You are only fetching a single value after all.

How about just:

return ($data = $sth->fetchColumn(0)) ? $data : '';

Or non-ternary equivalent if you like.

Your DB interactions seem to consider "happy path" only.  What if statement preparation or execution fails?

I am adding a link to a session management library I created, not so much for you to use (though you are free to do so, though it has no built-in database handler and would need to be extended for that), but rather because I think that within the main readme and the individual class documentation, I have tried to assemble some the better known resources (to me) around session security and session management in PHP. Between this and the code itself, I hope it gives you better footing for dealing with sessions in PHP (which PHP certainly does not make easy "out of the box").

https://github.com/mikecbrant/php-ultimate-sessions


Spring password validator library I recently got rejected at a job interview for submitting this:

https://bitbucket.org/gnerr/password-validator

The interviewer asked for a password validation library that was configurable via Spring, with the following default validations:

  Must consist of a mixture of lowercase letters and numerical digits only, with at least one of each.
  Must be between 5 and 12 characters in length.
  Must not contain any sequence of characters immediately followed by the same sequence.

Unfortunately they never told me why my implementation is bad, so would someone be kind enough to tell me what I did wrong here?

Here's the main validation class for the library:

package dm.passwordvalidator;

import java.util.ArrayList;
import java.util.List;

/**
 * @author dm
 *
 */
public class PasswordValidator {

    private List<ValidationRule> rules;

    /**
     * Runs all your validation rules and returns true if it's valid, false otherwise
     * @param inPassword
     * @return boolean true if valid, false if otherwise
     */
    public boolean validate(String inPassword) {
        for (ValidationRule rule : this.getRules()) {
            if (!rule.validate(inPassword)) {
                return false;
            }
        }
        return true;
    }

    /**
     * Runs all your validation rules and returns a {@link ValidationResult} with messages if
     * the password was invalid
     * @param inPassword
     * @return {@link ValidationResult} The validation result
     */
    public ValidationResult validateWithMessages(String inPassword) {
        ValidationResult returnValue = new ValidationResult();
        List<String> messages = new ArrayList<String>();
        for (ValidationRule rule : this.getRules()) {
            if (!rule.validate(inPassword)) {
                messages.add(rule.getMessage());
            }
        }
        if (messages.size() > 0) {
            returnValue.setValid(false);
        }
        returnValue.setMessages(messages);
        return returnValue;
    }

    public List<ValidationRule> getRules() {
        return this.rules == null ? this.getDefaultRules() : this.rules;
    }

    public void setRules(List<ValidationRule> inRules) {
        this.rules = inRules;
    }

    private List<ValidationRule> getDefaultRules() {
        List<ValidationRule> returnValue = new ArrayList<ValidationRule>();
        returnValue.add(new RegexValidationRule("(?!(.+?)\\1).*", DefaultValidatorMessages.NO_CYCLES));
        returnValue.add(new RegexValidationRule("[a-z0-9]+", DefaultValidatorMessages.LOWERCASE_AND_NUMBERS));
        returnValue.add(new RegexValidationRule(".*[a-z].*", DefaultValidatorMessages.LOWERCASE_AND_NUMBERS));
        returnValue.add(new RegexValidationRule(".*[0-9].*", DefaultValidatorMessages.LOWERCASE_AND_NUMBERS));
        returnValue.add(new RegexValidationRule(".{5,15}", DefaultValidatorMessages.FIVE_AND_TWELVE));
        return returnValue;
    }

}

The library is configurable as beans like this:

<bean id="lengthRule" class="dm.passwordvalidator.RegexValidationRule">
    <property name="regex" value=".{5,15}"/>
    <property name="message" value="Your password needs to be between 5 to 15 characters"/>
</bean>

<bean id="passwordValidator" class="dm.passwordvalidator.PasswordValidator">
    <property name="rules">
        <list>
            <ref bean="lengthRule" />
        </list>
    </property>
</bean>

Putting this as an anwer, because it took a while to find it:

I took your code, and put it through some tests..... I made the tests up based on the 'rules'. This is the sort of thing any interviewer will do:

public static void main(String[] args) {
    String[] positive = {"12345abc", "1a2b3c4d", "1234567890ab", "1234a"};
    String[] negative = {"", "1", "1a", "123567890", "abcdefghij", "abcdEfgh1", "abcabc123", "123a", "1234567890abc"};

    PasswordValidator validator = new PasswordValidator();

    for (String pos : positive) {
        ValidationResult result = validator.validateWithMessages(pos);
        if (!result.isValid()) {
            System.out.println("Claims that '" + pos + "' is not valid!: " + result.getMessages());
        }
    }

    for (String neg : negative) {
        if (validator.validate(neg)) {
            System.out.println("Claims that '" + neg + "' is valid (but it is not)!");
        }
    }
}

When I ran that test, it passed the password: "1234567890abc" which is 13 characters long, and the rules say the limit is 12.

Checking your Regex, I see you mis-typed the range-limit on the match: ".{5,15}"

Hate to be the bearer of bad news, but an interviewer could look at this as being an indication that you are not 'detail-oriented', and dismiss the application immediately.

For what it's worth, I look at your code and feel it has a strong structure, a reusable and extensible model. Generally good. Perhaps overkill, but that's OK.

But, bottom line, if your code does not meet the specification, you can expect to be moved down the pile of applicants ......

P.S. It also blew up on a null password, which I removed from my test because I did not think it was 'fair', but regardless, you should handle null input gracefully.


Finding the distance between every indices in an array that contain odd numbers I'm doing this HackerRank problem which basically boils down to what I've written in the title. We have to find out the distance between all the odd values in the array. I've written this code below which passes all the test cases but I think I can do better, namely, I don't have to make a separate array (oddindices) to contain the odd values' indices.

def min_bread(N, breads):
    oddcount = 0
    oddindices = []
    for idx in xrange(N):
        if breads[idx]%2:
            oddcount += 1
            oddindices.append(idx)
    if oddcount%2: # Odd number of odd loaves make it impossible to distribute
        return "NO"
    if oddcount==N or oddcount == 0: # All are odd (return that number) or even (return 0)
        return oddcount
    loaves = 0
    for idx in xrange(0, oddcount, 2):
        loaves += (oddindices[idx+1] - oddindices[idx])*2
    return loaves

if __name__ == '__main__':
    N = int(raw_input())
    breads = map(int, raw_input().split())
    print min_bread(N, breads)

But I can't figure out how to do it without having a separate array. The way I'm thinking is, if I encounter an odd value, I make the loop skip to AFTER the next odd value. I could do this easily with oddindices, but how do I do it without that extra array?

Code organisation

You've separated the input/output from the actual algorithm and that's a very nice thing that is not done as often as it should be in questions tagged with programming-challenge.

The only thing I'd change about the way you've done this is that the min_bread function does not need to be provided the N value as it should correspond to the length of the breads argument.

Loop like a native

I highly recommand Ned Batchelder's excellent talk called "Loop Like A Native". Among other things, it tells you that in general, you should not use a combination of range (or xrange) and len to iterate over something.

At this stage, the code looks like:

def min_bread(breads):
    oddcount = 0
    oddindices = []
    for i, val in enumerate(breads):
        if val % 2:
            oddcount += 1
            oddindices.append(i)
    # Odd number of odd loaves make it impossible to distribute
    if oddcount % 2:
        return "NO"
    # All are odd (return that number) or even (return 0)
    if oddcount==len(breads) or oddcount == 0:
        return oddcount
    loaves = 0
    for idx in xrange(0, oddcount, 2):
        loaves += (oddindices[idx+1] - oddindices[idx])*2
    return loaves

Details

You don't need to compute oddcount as you build oddindices, you can simply do oddcount = len(oddindices) after the loop.

Also, now you have a nice way to initialise oddindices. Indeed, in Python, the pattern l = []; for i in iter: if filter(i): l.append(func(i)) can be rewritten using a structure called "list comprehension" to get : l = [func(i) for i in iter if filter(i)].

In your case, you get:

oddindices = [i for i, val in enumerate(breads) if val % 2]
oddcount = len(oddindices)

Multiply once

You multiply by 2 each value you add to loaves. You could do it only once at the very end.

Sum builtin

You could use the sum builtin to perform the sums efficiently.

You'd get something like:

return 2 * sum(oddindices[idx+1] - oddindices[idx] for idx in xrange(0, oddcount, 2))

Then, the whole code looks like:

def min_bread(breads):
    oddindices = [i for i, val in enumerate(breads) if val % 2]
    oddcount = len(oddindices)
    # Odd number of odd loaves make it impossible to distribute
    if oddcount % 2:
        return "NO"
    # All are odd (return that number) or even (return 0)
    if oddcount==len(breads) or oddcount == 0:
        return oddcount
    return 2 * sum(oddindices[idx+1] - oddindices[idx] for idx in xrange(0, oddcount, 2))

Avoiding the additional array

A different way to write this without an additional array is to see that the different indices of odd values are substracted, then added, then substracted, etc. Also, you have to perform as many additions as substractions. We could use a simple flag to handle both the alternance of sign and the check that we've performed the same number of additions and substractions. For instance (highly not tested):

def min_bread(breads):
    sign = -1
    loaves = 0
    for i, val in enumerate(breads):
        if val % 2:
            loaves += sign * i
            sign *= -1
    return "NO" if sign == 1 else 2 * loaves


Tree-view of Active Directory group memberships using recursion Using Powershell, I've created a script to create a tree-view of a group/s' memberships in Active Directory. I originally had layers of nested loops, but I wanted a more algorithmic solution.

The following code does the following:

Get user input for: input source, depth-level int, output location
Recursively find all the AD Groups from the input and print out in tree-style formatting to a plaintext file.

I'm concerned the following code is too complicated and has reached "spaghetti-code" status.  I've read multiple times code-readability is very important.  What improvements could be made to improve readability and make the code more professional?  Any additional comments and advice to keep me from going down the dark path of sloppy programming are appreciated.

###   -- PRIMARY FUNCTION --   ###
function getGroupMemberships (){
    ### DESCRIPTION: Main Function ###

    # -- SWITCH OPTIONS -- #
    write-host "options:`n1: Type a User/Group`n2: Type filepath of CSV file containing list of users/groups."
    $route = read-host ":"
    switch ($route)
    {
        1 {
            $group = read-host "type group or user"
            $primaryGroups = Get-ADPrincipalGroupMembership $group
        }

        2 {
            $continue = $false
            while ($continue -eq $false) {
                $csv = read-host "CSV Filepath (no quotes!)"
                if ((test-path $csv) -eq $true) {
                    $continue = $true
                }
                elseif ((test-path $csv) -eq $false) {
                    write-host "Location $csv does not exist.  Please try again..."
                }
            }
           $primaryGroups = Import-Csv $csv
        }
    }

    # -- OTHER REQUIRED OPTIONS -- "
    $maxLevelInt = read-host "# depth of levels"
    $ouputFilePath = read-host "TXT Output Filepath: (no quotes!)"
    write-host "------------------------------------"
    $level = 0
    getNextGroupMembers $level $primaryGroups
}

### -- SECONDAY FUNCTIONS -- ###
    function getNextGroupMembers ($currentLevel, $groupIn) {
        # DESCRIPTION: main loop for gathering next groups' memberships and recursively finding

        $level++
        $groupIn = ( $groupIn | sort-object name ).name

        foreach ($group in $groupIn) {
            outputFormat $level $group
            $nextLevelMemberships = Get-ADPrincipalGroupMembership $group | select name
            if ( $level -eq $maxLevelInt ) { break } # if reached maxLevelInt
            getNextGroupMembers $level $nextLevelMemberships
        }

        $level-- # subtracts level counter once the loop completes
    }

        function outputFormat ([int]$level,$groupName) {
            # DESCRIPTION: creates string for formatting tree view; adds bars and spaces to make more readable

            $spaces="    "; $bar="|"
            if ($level -eq 1) { $out = $groupName }
            elseif ($level -eq 2) { $out = ( $spaces + $groupName ) }
            elseif ($level -gt 2) { $out = ( $spaces + $bar )*( $level - 2 ) + $spaces + $groupName }
            write-host $out; writeToFile $out
        }

        function writeToFile ($writeLine) {
            ### DESCRIPTION: writes string to file location ###

            $writeLine | out-file $ouputFilePath -append
        }

    function checkFilePath ($path) {
        $filepathTest = test-path $path
        return $filepathTest
    }

### RUN ###
getGroupMemberships

Separating Presentation From Logic

You're mixing interface code with code that retrieves the group membership information and provides the logic of the script.

Consider separating these. Ideally, you would have a function that accepts as a parameter a user or group object, and returns some kind of object that contains all of that user's memberships (even if it's just a nested [hashtable]).

The object returned from this would be passed to another function to format it for display (creating the tree view).

A separate function or set of functions is responsible for getting the information from the user (prompting, validating, etc.).

This separation allows you to take the object with the actual data, and make other functions that use it (to display it graphically, to write it as structured data to an XML file or to a database, to gather statistics on it, whatever), or to use it in quick one-liners to test some logic.

Similarly, you can change the way the interface operates without worrying about breaking the logic. You can change the way the object is displayed without introducing errors in the other parts.

The way it is now, these are all intertwined in such a way that you can't easily change one thing without changing another.

Actual Code Stuff

Option 2

$continue = $false
while ($continue -eq $false) {
    $csv = read-host "CSV Filepath (no quotes!)"
    if ((test-path $csv) -eq $true) {
        $continue = $true
    }
    elseif ((test-path $csv) -eq $false) {
        write-host "Location $csv does not exist.  Please try again..."
    }
}

There are several things to note here:

if ((test-path $csv) -eq $true) {}

You don't need -eq $true:

if (test-path $csv) {}

Similarly with the elseif:

elseif ((test-path $csv) -eq $false) {}

vs.

elseif (!(test-path $csv)) {}

or:

elseif (-not (test-path $csv)) {}

But actually, you don't need elseif at all here. You're testing for the existence of this file twice. You already checked for it in the if so, just else would do:

if (test-path $csv) {
    break
} else {
    Write-Host "Location $csv does not exist.  Please try again..."
}

By the way, it's good that you're checking user input and figuring out if the file exists. But the user could just press enter (on purpose or by accident) with an empty string. If that happens, Test-Path will throw an exception, so you need to check for this case too:

if ($csv -and (Test-Path $csv)) { break }

It's also a bit strange that you're looping while a variable named $continue is $false but breaking once it's $true (seems backwards). But more than that, you could just use the break keyword instead:

while ($true) {
    $csv = read-host "CSV Filepath (no quotes!)"
    if (test-path $csv) {
        break
    }
}

But going further, you don't need to use an infinite loop and then check a conditional to figure out when to break. It seems that way because you're using a while loop which checks the conditional at the beginning, but you need to execute the loop body at least once before checking the conditional.

That's where do {} while() or do {} until() comes in. Given that, your entire loop can be reduced to:

do {
    $csv = read-host "CSV Filepath (no quotes!)"
} until ($c -and (Test-Path $c))

This is missing the message to the user about the CSV not existing, but frankly I think they'll get it if it keeps prompting. If you really want to do that part, then you can still shorten it:

while ($true) {
    $csv = read-host "CSV Filepath (no quotes!)"
    if ($csv -and (test-path $csv)) {
        break
    } else {
        Write-Host "Location $csv does not exist.  Please try again..."
    }
}

One last thing, if the users are constantly wrapping the file name in quotes, you might consider just stripping them off instead of trying to get them to enter it another way:

$csv = $csv.TrimStart('"').TrimEnd('"')

Or replace all the quotes:

$csv = $csv -replace '"',''

getNextGroupmembers:

There isn't too much to say here other than the big changes that would be made by refactoring everything like I described at the beginning.

But a simple step you could take toward that goal is to remove the call to Get-ADPrincipalGroupMembership from the menu, and make this function do the lookups from the beginning. In other words, you give it the user or group you want the memberships for, not the first set of groups. It's already a recursive function; it can figure out how to get the initial memberships.

Important note about $maxLevelInt: you're reading it one function, then referring to it in another, without passing it along.

Frankly, I don't think that even works. I hesitated to even say this, but you could make that work by explicitly referring to it as global in both places:

$Global:maxLevelInt = read-host "# depth of levels"

and

if ( $level -eq $Global:maxLevelInt ) { break }

Please don't do this.

If you want to use this value, then make it a parameter of this function:

function getNextGroupMembers ($currentLevel, $groupIn, $maxLevel) {
        if ( $level -eq $maxLevel ) { break }
}

You just need to make sure to include it whenever you call the function (including the recursive calls to itself).

I'm assuming this check is to protect against circular nested groups. This will prevent your code from looping infinitely, but you might not get the output you need.

You might consider passing a list of all the groups checked so far so that you can skip a recursive call for those, and never get caught in the infinite loop, while still being able to enumerate the rest of the groups. If that were implemented, you could safely ignore a level limit.

The code for that is a bit outside the scope of a code review, but it's something you could work on.

outputFormat:

Fairly simple function here. I would say you could replace your if () elseif () elseif() with switch () as it seems to be a good use case for it.

Along the lines of separation, ideally this would return the formatted data which the caller can then decide whether to write it to the screen, or to a file, or multiple files, or whatever, but meh.

If you really wanted to go further with this, and you've gotten your functions refactored to a point where you can get a single object that contains all the nested membership data (as objects, not formatted), then you could start looking at using Format-Custom along with a PowerShell formatting file. This is a very advanced option that no one has ever used. Ever. But it would be pretty cool.

Required reading for this:

Format-Custom
Formatting File Overview
And basically all the sub-topics of the above link (concepts, examples, Schema XML reference, etc.).

writeToFile:

This function accepts a line, and then writes to a pre-determined file. You're again using a global variable here (don't do this please), and I don't know how it's working (in my tests I can't get it to work).

You should pass the file name into it and reference it that way.

But going further, I'm not sure why you need this function at all. Why not just use Out-File directly?

checkFilePath:

There's no need to assign the result of Test-Path to a variable just to return it from the function. You could just return Test-Path $path or omit return:

Test-Path $path

But then of course I have to ask (rhetorically), why would you use this function at all?

And then I see that in fact you aren't using it at all. So I recommend deleting it!

Parameterizing the script itself

You can make the script itself take parameters the way a function would. At the very top, put a param() block:

param($UserOrCsv)

Why do this? Because you could give the user the option of skipping the interactive menu altogether and just running the process. Then the script could be called on a schedule, or by some other automated process.

Of course the above param block is rather annoying, because you would need to determine whether the user intended to pass a user name or a CSV file.

A better way is to use advanced function semantics so that you use advanced parameter sets. So at the top you could do something like this:

[CmdletBinding(DefaultParameterSetName='ByUser')]
param(
    [Parameter(
        ParameterSetName='ByCSV',
        Mandatory=$true
    )]
    [String]
    $CSV ,

    [Parameter(
        ParameterSetName='ByUser',
        Mandatory=$true
    )]
    [String[]]
    $User
)

Why do this? Because now they can call:

.\MyScript.ps1 -CSV C:\whatever\stuff.csv
.\MyScript.ps1 -User ThisGuy
.\MyScript.ps1 -User ThisGuy,ThatOne,SomeUser

You can do a lot more than that with advanced function syntax, like validating the existence of the CSV in the parameter declaration, accepting users via the pipeline, etc.

I won't get too much further into it because you may not even want it.


Encryption algorithm for an app I have created an encryption algorithm for this app I am currently in the process of developing. I was wondering if my code looks "sharp", concise, and most importantly, succinct. I was also wondering if it is secure.

I know that 99% of the time, encryption algorithms that are invented are not secure, however I was wondering if this algorithm is in the 1% of the safe ones.

#Key Size: 128 bytes [recommended]

import random
import os
import time
import hashlib

def generate_key(key_size):
    key = os.urandom(key_size)
    return key;

def encrypt(_key, message):
    letter_ascii = []
    letter_ascii_final = []
    ascii_key_value = ''.join(str(ord(x)) for x in str(_key))
    for letter in message:
        letter_ascii.append(ord(letter))
    for _letter in letter_ascii:
        added = int(ascii_key_value) + int(_letter)
        letter_ascii_final.append(str(added))
    return letter_ascii_final;

def decrypt(_key, message):
    message_repieced = []
    ascii_key_value = ''.join(str(ord(x)) for x in str(_key))
    for item in message:
        _item = chr(int(item)-int(ascii_key_value))
        message_repieced.append(str(_item))
    return ''.join(message_repieced);

No. This is just a worse Caeser cypher.

steps to break:

Convert each string to an int in the output
Subtract everything from the min output value - ord('A')
Everything is now between 0 and 26
If the message is readable, you are done (this will be true if the message contains an 'a')
Otherwise, add 1 to the each character
After repeating this between 1 and 24 times, the message will be decrypted.


Counting words from standard input in Swift I know Swift isn't exactly meant to be used to write your classic stdin to stdout scripts, and that Python, Ruby, Perl, bash, awk, and friends are much better in this area, but I'd still like to see how well it can be done.

The problem I chose is: Read from standard input, and write to standard output a space-separated report of each of the words (lowercased) and their counts, sorted by word. For simplicity, words are defined as containing Basic Latin letters (U+0061 through U+007A) and the apostrophe (U+0027) only.

In Ruby, just for illustration, we can do this:

counts = Hash.new(0)
ARGF.each do |line|
  line.downcase.scan /[a-z']+/ {|word| counts[word] += 1}
end
counts.sort.each do |word, count|
  puts "#{word} #{count}"
end

Now, struggling with Swift, all I can come up with is this mess:

import Foundation

let standardInput = NSFileHandle.fileHandleWithStandardInput()
let input = standardInput.availableData
let text = String(data: input, encoding: NSUTF8StringEncoding)!
var counts = [String: Int]()

func isWordChar (c: Character) -> Bool {
    return "abcdefghijklmnopqrstuvwxyz'".rangeOfString("\(c)") != nil
}

for word in (text.lowercaseString.characters.split{!isWordChar($0)}.map(String.init)) {
    if let count = counts[word] {
        counts[word] = count + 1
    } else {
        counts[word] = 1
    }
}

for (word, count) in (counts.sort { $0.0 < $1.0 }) {
    print("\(word) \(count)")
}

There are so many problems with this, particularly:

I'm not sure how to read "line by line" or at least in chunks from stdin. Ruby and Python give you this for free. My Swift code reads up to the theoretical 8EiB limit, and as we know, with big data we want to process as we read rather than slurping in the whole file first.
It appears Swift has no native regex support, so rather than using all the Objective-C regex support, I went with split which works on Swift strings. Is there a simple way to use the regexes instead of splitting?
I think my use of split with the little helper can be improved.
I'm not happy with my use of ! to unwrap the optional string.
Is there a better way to do the counting with the dictionary? Ruby's default value for hash lookups is fabulous here... Can Swift do the same?

Can the Swift code here be professionalized?

Regarding standard input, see Martin's answer below.

As for splitting strings into words and returning them as a sorted dictionary with word counts, that sounds like a useful thing to have around as a String extension. This is simple to implement unless you insist on pure Standard Library solution:

import Foundation

public extension String {

    public var wordCounts: [String:Int] {
        var d: [String:Int] = [:]
        enumerateSubstringsInRange(characters.indices, options: .ByWords) { word, _, _, _ in
            guard let word = word?.lowercaseString else { return }
            d[word] = (d[word] ?? 0) + 1
        }
        return d
    }

    public var sortedWordCounts: [(word: String, count: Int)] {
        return wordCounts.sort{ $0.0 < $1.0 }.map{ (word: $0, count: $1) }
    }
}

Usage as follows (to produce the same output as your code):

if let input = readLine() {
    let report = input.sortedWordCounts.map{"\($0) \($1)"}.joinWithSeparator("\n")
    print(report)
}

For example, the following input ↓

Baa, baa, black sheep,/ Have you any wool?/ Yes, sir, yes, sir,/ Three bags full

...yields the following output ↓

any 1
baa 2
bags 1
black 1
full 1
have 1
sheep 1
sir 2
three 1
wool 1
yes 2
you 1

Edit

As Martin points out in the comment, your Ruby code reads multiple lines, or even multiple files… Here is a version that would do something like that, though you’ll of course need to adapt it to your needs. Note that I’ve added that eof so that we can play with this in the Xcode.

public func += <T> (inout lhs: [T:Int], rhs: [T:Int]) {
    for (k, i) in rhs {
        lhs[k] = (lhs[k] ?? 0) + i
    }
}

print("Type your terminating token or just type return if an empty line works for you:")
let eof = readLine()
print("Enter your lines:")

var wordCounts: [String:Int] = [:]

while let line = readLine() where line != eof {
    wordCounts += line.wordCounts
}

let report = wordCounts
    .sort{ $0.0 < $1.0 }
    .map{"\($0) \($1)"}
    .joinWithSeparator("\n")

print(report)


Is this the right way to do lambda method chaining in python? I come from java background and I am trying to implement lambda chaining in Python. Below is my code
li = ["amol", "vikas", "rahul"]
li2 = list(filter(lambda n: (n == "Amol"), list(map(lambda name: (name.capitalize()), li))))
print(li2)

This Function will

Capitalize the names
Filter the names with "Amol"

Currently the code is running well but it seems to be cluttered and complicated when compared to java

You're better off doing an inline list comprehension.
li2 = [name.capitalize() for name in li if name == "amol"]

For the lambda implementation itself, you can leave the map and filter generators until only when you want to convert back to list:
li2 = filter(lambda n: n == "Amol", map(str.capitalize, li))
print(list(li2))