Select the next TextBox in order Could somebody help me out with trying to clean up this big list of if / else if statements please? I've found that switch statements won't work because it won't handle a TextBox as the parameter. I've seen many answers involving the Dictionary, but those won't work either in my case.

    private void SelectNextTextBox()
    {
        TextBox newTextBox = null;

        if (_currentTextBox == keyText1)
        {
            newTextBox = keyText2;
        }
        else if (this._currentTextBox == keyText2)
        {
            newTextBox = keyText3;
        }
        else if (this._currentTextBox == keyText3)
        {
            newTextBox = keyText4;
        }
        else if (this._currentTextBox == keyText4)
        {
            newTextBox = keyText5;
        }
        else if (this._currentTextBox == this.keyText5)
        {
            newTextBox = regText1;
        }
        else if (this._currentTextBox == regText1)
        {
            newTextBox = regText2;
        }
        else if (this._currentTextBox == regText2)
        {
            newTextBox = regText3;
        }
        else if (this._currentTextBox == regText3)
        {
            newTextBox = regText4;
        }
        else if (this._currentTextBox == regText4)
        {
            newTextBox = regText5;
        }
        else
        {
            return;
        }
        newTextBox.SelectAll();
        newTextBox.Focus();
    }

There's 10 total text boxes and when one is full it changes to the next. This is the method that tells it which text box is next in the list.

You absolutely can use a Dictionary solution.  Here is what it would look like.

In some initialization code, maybe the form's constructor, write the following code:

// global mapping
private Dictionary<TextBox, TextBox> nextTextboxMap = new Dictionary<TextBox, TextBox>();

private void InitNextTextBoxMap()
{
    this.nextTextboxMap[keyText1] = keyText2;
    this.nextTextboxMap[keyText2] = keyText3;
    this.nextTextboxMap[keyText3] = keyText4;
    // add the other mappings here
    // where key = current textbox, and value = next textbox.
}

And then, your SelectNextTextBox() method can be simplified to this:

private void SelectNextTextBox()
{
    TextBox newTextBox = null;
    if (this.nextTextboxMap.TryGetValue(this._currentTextBox, out newTextBox))
    {
        newTextBox.SelectAll();
        newTextBox.Focus();

        // Maybe you also want to reset this._currentTextBox at this point?
        // this._currentTextBox = newTextBox;
    }
}


Producing all allocations by n items from a list I have strong feeling that the code below is ugly at least as there are 2 same "cons".
I would appreciate if you advise me ways to improve it.

The code produces all allocation by n items from list lst.

(defun allocations (lst n)
  (if (= n 1)
      (loop for i in lst
       collect (cons i nil))
      (loop for i in lst
   append (mapcar #'(lambda (l) (cons i l))
                      (allocations (remove i lst) (- n 1))))))

(defun allocations (source length)
  (if (= 1 length)
      (list (list (car source)))
      (loop for processed = nil then (cons (car i) processed)
         for i on source
         for todo = (cdr i)
         appending
           (loop for intermediate
              in (allocations
                  (append (reverse processed) todo)
                  (1- length))
              appending
                (loop for prefix = nil then (cons (car suffix) prefix)
                   for suffix on intermediate
                   collect (append prefix (list (car i)) suffix))))))

This should be a more general case of the problem (i.e. it makes no assumption of the nature of the elements in the list), but it doesn't verify that there is enough of the elements in the source list to build the required number of permutations.


Finding answers where happy customers commented "thanks" To find answers where users expressed their gratitude (by mentioning "thank" in comments),
I put together this SEDE query (latest):

DECLARE @username AS NVARCHAR(60) = ##DisplayName:string? ##;
DECLARE @userId AS INT = ##UserId:int?-1##;

-- SpecialThanks: Special thanks only "Enter 1 for YES, 0 for NO"
DECLARE @SpecialThanks AS INT = ##SpecialThanks:int?0##;

WITH Answers AS (
  SELECT * FROM Posts WHERE PostTypeId = 2
)

SELECT TOP ##Limit:int?100##
  a.Id AS [Post Link],
  c.Id AS [Comment Link],
  c.Text,
  c.CreationDate
FROM Answers a
  INNER JOIN Users u ON a.OwnerUserId = u.Id
  INNER JOIN Comments c ON a.Id = c.PostId AND c.UserId != u.Id
WHERE
  (@username = '' OR u.DisplayName = @username)
  AND (@userId = -1 OR u.Id = @userId)
  AND LOWER(c.Text) LIKE '%thank%'
  AND (
    @SpecialThanks <= 0 OR (
      LOWER(c.Text) LIKE '%excellent%'
      OR LOWER(c.Text) LIKE '%awesome%'
      OR LOWER(c.Text) LIKE '%awesum%'
      OR LOWER(c.Text) LIKE '%awsum%'
      OR LOWER(c.Text) LIKE '%wonderful%'
      OR LOWER(c.Text) LIKE '%fantastic%'
      OR LOWER(c.Text) LIKE '%amazing%'
      OR LOWER(c.Text) LIKE '%great%'
      OR LOWER(c.Text) LIKE '%best%'
      OR LOWER(c.Text) LIKE '%impressive%'
      OR LOWER(c.Text) LIKE '%master%'
      OR LOWER(c.Text) LIKE '%guru%'
      OR LOWER(c.Text) LIKE '%wizard%'
      OR LOWER(c.Text) LIKE '%ninja%'
      OR LOWER(c.Text) LIKE '%grateful%'
      OR LOWER(c.Text) LIKE '%appreciat%'
      OR LOWER(c.Text) LIKE '%by far%'
      OR LOWER(c.Text) LIKE '%!!!%'
    )
  )
ORDER BY c.CreationDate DESC

What do you think? Any improvement ideas?

Mainly I'm wondering if there's a way to eliminate all those duplicated LOWER(c.Text)

Answers

Your Answers CTE is convenient, but it should be doing a lot more of the heavy lifting for you, and it is also over-generous with the selection.

Your intention is to report on answers for the specified user (UserID or Display Name). You should be filtering those answers out immediately. Also, the inner join to the Users table is just for the display name is a problem too. Also, there's a little trick with CommunityWiki or a couple of other posts types, where the DisplayName is actually populated on the Post table.

So, your Answers CTE is:

Answers AS (
  SELECT * FROM Posts WHERE PostTypeId = 2
)

I would make it do all the heavy lifting, as:

Answers AS (
  SELECT Id,
         OwnerUserId
  FROM Posts
  WHERE PostTypeId = 2
    AND (@userId < 0 OR OwnerUserId = @userId)
    AND (@username = '' OR OwnerUserId in (
        SELECT Id
        FROM Users
        WHERE DisplayName = @username))
)

With that CTE, the only answers returned are ones that are valid for the input conditions on who to report for.

Note that it only selects the two columns needed for the remainder of the query.

Selection

Your column selection returns the Comment text twice, once as a link, and again for the text. In some conditions this is useful (like getting the data as a CSV report) but I don't believe this is the intention here. Additionally, you do not report the actual User who created the answer either. I would find that useful.

SEDE allows you to return the Id for the user, but display the name (as a link). Your regular join to the Users table is not necessary for that.

So, with the previous Answers CTE, I would have the much simpler final selection as:

SELECT TOP ##Limit:int?100##
  a.Id AS [Post Link],
  c.Id AS [Comment Link],
  a.OwnerUserId as [User Link],
  c.CreationDate as [Comment Date]
FROM Answers a
  INNER JOIN Comments c ON a.Id = c.PostId AND c.UserId != a.OwnerUserId
WHERE LOWER(c.Text) LIKE '%thank%'
  AND (
    @SpecialThanks <= 0 OR (
      LOWER(c.Text) LIKE '%excellent%'
      OR LOWER(c.Text) LIKE '%awesome%'
      OR LOWER(c.Text) LIKE '%awesum%'
      OR LOWER(c.Text) LIKE '%awsum%'
      OR LOWER(c.Text) LIKE '%wonderful%'
      OR LOWER(c.Text) LIKE '%fantastic%'
      OR LOWER(c.Text) LIKE '%amazing%'
      OR LOWER(c.Text) LIKE '%great%'
      OR LOWER(c.Text) LIKE '%best%'
      OR LOWER(c.Text) LIKE '%impressive%'
      OR LOWER(c.Text) LIKE '%master%'
      OR LOWER(c.Text) LIKE '%guru%'
      OR LOWER(c.Text) LIKE '%wizard%'
      OR LOWER(c.Text) LIKE '%ninja%'
      OR LOWER(c.Text) LIKE '%grateful%'
      OR LOWER(c.Text) LIKE '%appreciat%'
      OR LOWER(c.Text) LIKE '%by far%'
      OR LOWER(c.Text) LIKE '%!!!%'
    )
  )
ORDER BY c.CreationDate DESC

Observations

Note that your query (and my suggestion above) will return the answer multiple times if there are multiple 'thank you' comments.

I would consider adding a join back to posts to only consider comments made by the owner of the question that was answered too. At the moment, anyone can add a 'thanks' comment to any post, and make that answer appear.

Joining back to the questions would also allow you to remove closed questions as well.

In general, the code style is consistent, and that's always good.

I considered recommending an in-memory table containing the values to search for in the comments, something like:

create table @words (
    word NVarchar(50)
)

insert into @words values
    ('awesome'),
    ('amazing'),
    .....

and then using that in a join like:

AND INDEXOF(LOWER(text), word) >= 0

but that will produce even more duplicates if the comment is something like:

  Amazing answer, Thanks!!! That was awesome.

It could be cured with a 'distinct', but I am not sure it's worth it.


Temperature converter using "switch" statement I wrote a program to convert temperature into different units, where the users have to put a space between temperature and unit:

  23 f, 34 c, 45 k

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace PracticeSet1
{

    class Program
    {
        public static float temp;
        public static char tempUnit;

        static void Main(string[] args)
        {
            //Getting user input
            Console.WriteLine("Enter Temprature in to convert it into other i.e  30 k, 45 f, 50 c *Put space between value and unit* ");
            string[] tempInput = Console.ReadLine().Split();

            //parse element 0
            temp = float.Parse(tempInput[0]);
            //assinging tempUnit
            tempUnit = char.Parse(tempInput[1]);

            switch (tempUnit)
            {
                //Converting temp to F and K if tempUnit == c
                case 'c':
                    Console.WriteLine("Celsius To Farhenheit and Kelvin");
                    convertCelsiusToFarhenheit();
                    convertCelsiusToKelvin();
                    break;
                //Converting temp to C and F if tempUnit == K

                case 'k':
                    Console.WriteLine("Kelvin");
                    convertKelvinToCelsius();
                    convertKelvinToFarhenheit();
                    break;
                //Converting temp to C and K if tempUnit == F

                case 'f':
                    Console.WriteLine("Farhenheit to Celsius and kelvin");
                    convertFarhenheitToCelsius();
                    convertFarhenheitToKelvin();
                    break;

            }

        }
        static void convertFarhenheitToCelsius()
        {
            Console.WriteLine((temp - 32) * 0.5556 + "°C");
        }
        static void convertFarhenheitToKelvin()
        {
            Console.WriteLine((temp + 459.67) * 5 / 9 + "°K");
        }
        static void convertCelsiusToFarhenheit()
        {
            Console.WriteLine((temp * 1.8) + 32 + "°F");
        }
        static void convertCelsiusToKelvin()
        {
            Console.WriteLine(temp + 273.15 + "°K");

        }
        static void convertKelvinToCelsius()
        {
            Console.WriteLine(temp - 273.15 + "°C");
        }
        static void convertKelvinToFarhenheit()
        {
            Console.WriteLine(temp - 459.67 + "°F");
        }
    }
}

public static float temp;
public static char tempUnit;

There's no reason to make these into global variables. It's better to use local variables:

static void Main(string[] args)
{
    float temp;
    char tempUnit;
}

If you want a first step into OOP, try wrapping the temp and tempUnit into a class (or struct) of their own.

While Console.ReadLine().Split() is essentially the same as Console.ReadLine().Split(' '), I would suggest putting the character in there.

It's a matter of readability. Initially, I assumed it would split on newlines, not spaces, until I remembered. Or, alternatively, add a comments that mentions you're splitting on spaces.

You're not handling garbage values. What happens if the user enters 123f or batman or k 123? Try to handle invalid data and show the user an error message that describes the problem.

if(!float.TryParse(tempInput[0], out temp)
{
     Console.WriteLine($"Cannot parse the value \"{tempInput[0]}\"");

     //Wait until user presses enter and close the application
     Console.ReadLine();
     return;
}

The subsequent code is the same, as TryParse will still put a usable value in the temp variable.

Console.WriteLine(temp + 273.15 + "°K");

While this currently works, I highly suggest not doing this.

It's very hard to spot that you're first doing a mathematical operation (number + number), and then a string concatenation (number + string, which is handled as string + string).

Suppose you're in a culture where the unit comes before the value (e.g. °K 123). You can't just simply invert the code ("°K" + temp + 273.15); because if temp equals 123, your result will be °K123273.15 because it is now concatenating three string values.
While you could work around this issue with parentheses, it's still not a good fix.

There's no reason to inline all statement. Especially when you're a beginner, but this applies to experts as well. Handling the same operations on multiple lines does not affect application performance, but it does dramatically lower readability.

var convertedValue = temp + 273.15;
var convertedValueString = convertedValue + "°K";

Notice that I didn't use Console.WriteLine() yet. You've had to copy paste that in every method, which is not a reusable pattern.

It would be better to have the method return a string:

static string convertCelsiusToKelvin()
{
    var convertedValue = temp + 273.15;
    var convertedValueString = convertedValue + "°K";
    return convertedValueString;
}

And wrap the method call in the Main() method:

switch (tempUnit)
{
    //Converting temp to F and K if tempUnit == c
    case 'c':
        Console.WriteLine("Celsius To Farhenheit and Kelvin");
        Console.WriteLine(convertCelsiusToFarhenheit());
        Console.WriteLine(convertCelsiusToKelvin());

Note: This can be optimized further, but this is already a good first step. It teaches you to separate the calculation from the presentation of the calculated value.

Try to separate the logic into methods more. Your Main() method currently is doing many things that don't really have anything to do with each other:

Read the values
Decide which conversion calculations to do
(As per my suggestion) Printing the converted values.

A minor redesign to show you what I mean. First, let's define a struct for our values:

public struct Temperature
{
    float Value;
    char Unit;
}

You could redesign your main method to be much more abstracted:

static void Main(string[] args)
{
     Temperature input = GetInputFromUser();

     Temperature valueKelvin = CalculateKelvin(input);
     Print(valueKelvin);

     Temperature valueFahrenheit = CalculateFahrenheit(input);
     Print(valueFahrenheit);

     Temperature valueCelsius = CalculateCelsius(input);
     Print(valueCelsius);

    Console.ReadLine();
}

Some explanatory examples:

public static Temperature CalculateKelvin(Temperature inputValue)
{
     switch(inputValue.Unit)
     {
         case 'K':
             //Same unit, so no conversion needed
             return value;
         case 'F':
             // F => K
             return convertFahrenheitToKelvin(inputValue);
         case 'C':
             // C => K
             return convertCelsiusToKelvin(inputValue);

     }
}

public static void Print(Temperature value)
{
    Console.WriteLine($"{value.Value} °{value.Unit}");
}


Storing Enum values as Strings in DB I have a Data First Migration and have been struggling a little with storing an enum value as a string.

(Im aware that enums should be stored as ints but, personally I have never liked this, yes enums are hard and fast values but I have always percived them to vary a little bit as the code evolves, i am therefore worried about the integrity of using an int value as new values are added.)

However in my example I need to store the value as a string anyway so that other systems will be able to understand it.

So the database has the following generated property

public string billing_location { get; set; }

The View Model

public string billing_location { get; set; }

public MobiusBillingLocation BillingLocation
{
    get
    {
        MobiusBillingLocation mbl;
        if (Enum.TryParse(billing_location, true, out mbl))
        {
            return (MobiusBillingLocation)      Enum.Parse(typeof(MobiusBillingLocation), billing_location, true);
        }

        return MobiusBillingLocation.None;
    }
    //This also needs setting in the controller code
    set { billing_location = value == MobiusBillingLocation.None ? null : value.ToString(); }
}

public enum MobiusBillingLocation { None, UK, US, SGSingapore }

The code has been designed so that it will be happy with null values and translates None as null and back again.

My questions are, does this approach seem reasonable?
Any ideas / suggestions for improvments?
Would you use this approach for enums on a code first model for example?

UPDATE

I will add that, in this database, due to it being legacy and unnormalized (thats another topic) and has systems on it that i can't touch I cant be adding extra lookup tables at this time, the data in it however needs to have meaning to a user, ie integer values won't work as they don't pass meaning.

I am however also interested in some side discussion about use of enums.

There are clear guidelines for Enum Design where one of them says:

  X DO NOT use an enum for open sets (such as the operating system version, names of your friends, etc.).

and this

public enum MobiusBillingLocation { None, UK, US, SGSingapore }

looks like an open set. const values might be a better solution in this case. You won't have to convert anything and you can use any names for the values.

public static class MobiusBillingLocation
{
    public const string None = null;
    public const string GreatBritan = "UK";
    public const string UnitedStates = "US";
    public const string SomethingSingapure = "SGSingapore";
}


Thread safe lock-free stack using win32 threading primitives class CSList
{
public:
    CSList()
    {
        ::InitializeSListHead(&m_head);
    }

    USHORT Depth()
    {
        return ::QueryDepthSList(&m_head);
    }

    PSLIST_ENTRY Flush()
    {
        return ::InterlockedFlushSList(&m_head);
    }

    PSLIST_ENTRY First()
    {
        return ::RtlFirstEntrySList(&m_head);
    }

    PSLIST_ENTRY Push(PSLIST_ENTRY ListEntry)
    {
        return ::InterlockedPushEntrySList(&m_head, ListEntry);
    }

    PSLIST_ENTRY Pop()
    {
        return ::InterlockedPopEntrySList(&m_head);
    }

private:
    SLIST_HEADER m_head;
};

template <typename T>
class TSimpleStack
{
public:
    typedef struct DATA_ENTRY : SINGLE_LIST_ENTRY
    {
        T Data;
        DATA_ENTRY(const T& val) : Data(val)
        {
            SINGLE_LIST_ENTRY::Next = NULL;
        }
    } *LPDATA_ENTRY;

    ~TSimpleStack()
    {
        while (m_list.Depth() > 0)
        {
            LPDATA_ENTRY lpEntry = (LPDATA_ENTRY) m_list.Pop();
            if (lpEntry != NULL)
            {
                lpEntry->~DATA_ENTRY();
            }
            _aligned_free(lpEntry);
        }
    }

    USHORT Count()
    {
        return m_list.Depth();
    }

    LPDATA_ENTRY Push(const T& data)
    {
        LPVOID lpMemBuff = _aligned_malloc(sizeof(DATA_ENTRY), MEMORY_ALLOCATION_ALIGNMENT);
        LPDATA_ENTRY lpEntry = new (lpMemBuff) DATA_ENTRY(data);
        return (LPDATA_ENTRY)m_list.Push(lpEntry);
    }

    BOOL Pop(T& outVal)
    {
        BOOL bOut = FALSE;
        if (m_list.Depth() > 0)
        {
            LPDATA_ENTRY lpEntry = static_cast<LPDATA_ENTRY>(m_list.Pop());
            if (lpEntry != NULL)
            {
                outVal = lpEntry->Data;
                lpEntry->~DATA_ENTRY();
                _aligned_free(lpEntry);
                bOut = TRUE;
            }
        }
        return bOut;
    }

private:
    CSList m_list;
};

In TSimpleStack::Pop you don't need to check the depth as m_list.Pop() will return null if the stack is empty.

The repeated allocations and frees kind of rub me the wrong way. Instead you can add a CSList m_freeList; member and allocate from that:

DATA_ENTRY* AllocEntry(){
    DATA_ENTRY* result = m_freeList.Pop();
    if(!result)
        result = _aligned_malloc(sizeof(DATA_ENTRY), MEMORY_ALLOCATION_ALIGNMENT);
    return result;
}

void FreeEntry(DATA_ENTRY* entry){
    m_freeList.Push(entry);
}

A destructor to CSList with at least an assert that m_head is empty.

CSList should probably disallow copying and moving. But TSimpleStack should definatley have a copy constructor and assignment operator (plus the move variants) to follow the rule of 3 (5) or disallow them which is saner because it is hard to ensure thread safety when copying/moving.


Implementation of a simple stack with min method The problem statement:

  Implement a stack which has push and pop method and also min method whereas min method has \$O(1)\$ complexity.

From an interview standpoint, what mistakes would you point out in terms of algorithmic design and other things?

#include <iostream>
#include <string>

template<typename T>
class stack
{
  private:
    struct stackNode
    {
       stackNode(T val) : val(val), next(nullptr) {}
       stackNode() {next=nullptr;}
       T val;
       stackNode *next;
    };

  public:
    stack() { first = min = nullptr; sz = 0;}
    ~stack()
    {
      while(first) {
        stackNode *tmp = first->next;
        delete first;
        first = tmp;
      }
    }

    void print()
    {
      stackNode *tmp = first;
      std::cout << "Stack => ";
      while(tmp) {
        std::cout << tmp->val << ", ";
        tmp = tmp->next;
      }
      std::cout << std::endl;
    }

    stackNode* push(T val, stackNode *head)
    {
      stackNode *tmp = new stackNode(val);
      tmp->next = head;
      return head;
    }

    void push(T val)
    {
      first = push(val, first);

      T tVal = val;
      if(min && (min->val < val)) {
        tVal = min->val;
      }
      min = push(tVal, min);
      sz++;
    }

    size_t size()
    {
      return sz;
    }

    void pop()
    {
      if(first) {
        stackNode *tmp = first->next;
        delete first;
        first = tmp;

        //pop min temporary
        tmp = min->next;
        delete min;
        min = tmp;
        sz--;
      }
    }

    T minimum()
    {
      if(min) {
        return min->val;
      }
      return T();
    }

  private:
    stackNode *first, *min;
    size_t sz;
};

using myStack = stack<int>;

int main()
{
  myStack s;

  s.push(10);
  s.push(100);
  s.push(33);
  s.push(55);
  s.push(17);
  s.push(9);
  s.push(201);

  s.print();
  std::cout << "Size of stack = " << s.size() << std::endl;
  std::cout << "Minimum = " << s.minimum() << std::endl;

  s.pop(); s.pop();
  s.print();
  std::cout << "Minimum = " << s.minimum() << std::endl;

  s.pop(); s.pop();
  s.print();
  std::cout << "Minimum = " << s.minimum() << std::endl;

  return 0;
}

It all depends how much time you had to do this, overall it looks ok. In a couple of places you are initializing in the body of the constructor rather than using the initializer list, that is if nothing somewhat inconsistent but you should be using the initializer list in all of these cases. Also the way you use it, the stackNode constructor might as well take the next pointer, this way you don't forget to set it. I might try and get you to figure that out via questions about the constructor.

There is no way to get to value of the top of the stack, pop doesn't return it, neither is there a top() function that would return the top value.

I'd probably also would like to see an error condition rather than the default constructed value being returned for minimum() and pop(). It's worse for minimum because you are returning a valid value e.g. int() for an empty stack, i.e. calling minimum() on an empty stack<int> will return 0. That is just plain wrong therefore a bug.

I'd probably ask you why you consider min a temporary, this is from the comment in pop() just to clarify language.

stackNode* push(T val, stackNode *head) should be private

Algorithm wise it looks like a reasonable solution, depending on how the inteview goes i might ask you follow up questions, e.g. could you come up with a solution that doesn't need two separate stacks, these days I'd also ask you how to make this datastructure thread safe. I might ask what requirements there are to type T and what might happen if they aren't fulfilled. Maybe again depending how the interview goes, how to extend the stack to allow classes that don't fulfill the requirements to still used for the stack.


I need code review on using singleton to access local database As I get to some performance issues in my app and find that I use database access in a bad way.
So I decided to move to singleton pattern.
I need someone to review this code and confirm me that I made a good database access via singleton pattern or I am doing something wrong:
This is the class:

@interface DataAccessController : NSObject{
    sqlite3 *databaseHandle;
}
+ (id)sharedManager;
-(void)initDatabase;
...
+ (id)sharedManager {
    static DataAccessController *sharedMyManager = nil;
    static dispatch_once_t onceToken;
    dispatch_once(&onceToken, ^{
        sharedMyManager = [[self alloc] init];
    });
    return sharedMyManager;
}

- (id)init {
    if (self = [super init]) {
        [self initDatabase]; // open database connection
    }
    return self;
}
...

AppDelegate.m

DataAccessController *d = [DataAccessController sharedManager];

Usage through the application every time I need data I use:

DataAccessController *d = [DataAccessController sharedManager];
NSMutableArray* data = [d getAllRecordedUnits];

Based on your provided code, this is the correct way to create a singleton object.

In such, here's a reference link from Matt Gallow's site showing the same singleton setup as you're doing.

However, there is not enough information provided to determine the cause of your performance issues (whether or not caused by accessing the database). In general, writing efficient database code is hard, which is why Core Data exists.

You might consider looking into Core Data to see if this will work better for your app or perhaps creating a question on StackOverflow describing the issue you're seeing in more detail and asking for help on how to implement what you're trying to do.


Reposition vertices at the origin of a coordinate system This method repositions vertices of an object in such a way that the center of the object is at the origin of the coordinate system. The task is the commented part. While my code does the job I feel that it is very basic. I am looking for advice on how to make it faster and more elegant, concise and efficient. Any comments are welcome.

import java.nio.FloatBuffer;

public class ObjModifier {
    public static void centerVertices(FloatBuffer buf) {
        // We often get 3D models which are not positioned at the origin of the coordinate system. Placing such models
        // on a 3D map is harder for users. One solution is to reposition such a model so that its center is at the
        // origin of the coordinate system. Your task is to implement the centering.
        //
        // FloatBuffer stores the vertices of a mesh in a continuous array of floats (see below)
        // [x0, y0, z0, x1, y1, z1, ..., xn, yn, zn]
        // This kind of layout is common in low-level 3D graphics APIs.
        // TODO: Implement your solution here

        float[] a = new float[buf.capacity()];
        buf.get(a);

        float[] sums = new float[3];
        float[] avgs = new float[3];

        for (int i = 0 ; i < a.length; i+=3) {
            sums[0] += a[i];
            sums[1] += a[i+1];
            sums[2] += a[i+2];

        }

        for (int i = 0; i < avgs.length; i++) {
            avgs[i] = sums[i]/(a.length/3);
        }

        for (int i = 0 ; i < a.length; i+=3) {
            a[i] -= avgs[0];
            a[i+1] -= avgs[1];
            a[i+2] -= avgs[2];

        }

        for (int i = 0; i < a.length; i++) {
            buf.put(i, a[i]);
        }
        //

    }
}

I would create a separate method for calculating averages and use variables for them, so that the code is more legible.

I don't personally like the use of arrays such as sum[] and avg[] because looping over them makes it hard to tell what you are trying to access. If you do want to use them, I would save their indices (or offsets) in static final fields so that the accesses looks like this:

    sum[i + X_OFFSET] = ...;
    sum[i + Y_OFFSET] = ...;
    sum[i + Z_OFFSET] = ...;

I used the stream API for convenience - a loop would be fine too.

private static final int X_OFFSET = 0;
private static final int Y_OFFSET = 1;
private static final int Z_OFFSET = 2;

public static void centerVertices(FloatBuffer buf) {

    float[] objVertices = new float[buf.capacity()];
    buf.get(objVertices);

    float xAvg = (float) getAverage(objVertices, X_OFFSET);
    float yAvg = (float) getAverage(objVertices, Y_OFFSET);
    float zAvg = (float) getAverage(objVertices, Z_OFFSET);

    for (int i = 0; i < objVertices.length; i += 3) {
        objVertices[i + X_OFFSET] -= xAvg;
        objVertices[i + Y_OFFSET] -= yAvg;
        objVertices[i + Z_OFFSET] -= zAvg;
    }

    for (int i = 0; i < objVertices.length; i++) {
        buf.put(i, objVertices[i]);
    }
}

/**
 * Get the average of every third element in the array (with offset)
 */
private static double getAverage(float[] arr, int offset) {
    return IntStream //
            .range(0, arr.length) //
            .filter(i -> i % 3 == offset) //
            .mapToDouble(i -> arr[i]).average() //
            .getAsDouble();
}
```


Porting Haskell list comprehensions to Standard ML Out of curiosity, I'm porting the Haskell exercises of Programming in Haskell by Graham Hutton to Standard ML. At the beginning everything seemed pretty similar until I reached list comprehensions:

A triple (x,y,z) of positive integers is called pythagorean if x2 + y2 = z2. Using a list comprehension, define a function pyths ∷ Int → [(Int,Int,Int)] that maps an integer n to all such triples with components in [1..n].

The Haskell version is pretty simple:
pyths ∷ Int → [(Int, Int, Int)]
pyths n = [(x, y, z) |
  x ← [1..n],
  y ← [1..n],
  z ← [1..n],
  x^2 + y^2 == z^2]

My Standard ML attempt, not so much:
(* No list comprehensions in SML so we need a helper function *)
val generateIntsUpTo = fn n =>
  let val rec helper = fn current => fn xs =>
    if current <= n
    (* then current :: helper (current + 1) xs *)
    then helper (current + 1) (current :: xs)
    else xs
  in
    rev (helper 1 [])
  end

(* Now we can find the pyths *)
val pyths = fn n =>
  let
    val numbers = generateIntsUpTo n
  in
    foldr (fn (x, acc) =>
      foldr (fn (y, acc) =>
        foldr (fn (z, acc) =>
          if x * x + y * y = z * z
          then (x, y, z) :: acc
          else acc
        ) acc numbers
      ) acc numbers
    ) [] numbers
  end

The first thing that I don't like is the call to rev at the end of generateIntsUpTo but I did it to accomplish a tail recursive function. Commented out is the line that lets me avoid the reversion of the list but then it's not tail recursive.
Besides that, there's the three nested foldr calls. I have been thinking of writing my own recursive functions, but I wanted to do this with the minimum amount of custom code.

Feedback:

The Haskell pyths is not very efficient, and it contains duplicate answers.

fun instead of val/val rec is syntactic sugar for function declarations.

Instead of generateIntsUpTo n, use List.tabulate (n, fn i => i+1).

Using rev to achieve a tail-recursive function can be a good choice. Neither SML/NJ's or Moscow ML's List.tabulate is tail-recursive, though. If you're worrying about performance here, consider the fact that you don't actually need to store these numbers in lists -- this is purely a convenience so that you can use list comprehensions to iterate their combinations.

Pythagorean triples in Standard ML

I wanted to do it using the minimum amount of custom code.

To achieve the same efficiency as the Haskell pyths and use the minimum amount of custom code, here is one version that uses List.filter, List.concat and List.tabulate:
fun isPythTriple (x, y, z) = x*x + y*y = z*z
fun tab1 n f = List.tabulate (n, fn i => f (i+1))
fun pyths n =
    List.filter isPythTriple (
      List.concat (tab1 n (fn x =>
      List.concat (tab1 n (fn y =>
                   tab1 n (fn z => (x,y,z)))))))

This takes several seconds for pyths 10; there really is not reason to generate O(n³) list elements when the solution subset is so sparse.

I have been thinking on write my own recursive functions

Writing your own helper functions is really not something that should be avoided. Generally, using library functions is good, but Standard ML's library is somewhat limited. For example, List.tabulate can't iterate a range of numbers without generating the list in memory. And as you're hinting at, the multiple nested foldrs does not make the code particularly readable.
You could for example combine List.tabulate and List.filter to reduce memory consumption:
fun tabfilter (from, to, f) =
    if from > to then [] else
      case f from of
           SOME value => value :: tabfilter (from+1, to, f)
         | NONE       =>          tabfilter (from+1, to, f)

fun isPythTriple (x, y, z) = x*x + y*y = z*z

fun pyths n =
    List.concat (tabfilter (1, n, fn x =>
      SOME (List.concat (tabfilter (1, n, fn y =>
        SOME (tabfilter (1, n, fn z => Option.filter isPythTriple (x, y, z))))))))

This runs orders of magnitude faster. Still, it is a little convoluted.
A plain recursive version:
fun isPythTriple (x, y, z) = x*x + y*y = z*z
fun pyths n =
    let fun loop (0, _, _) = []
          | loop (x, 0, _) = loop (x-1, n, n)
          | loop (x, y, 0) = loop (x, y-1, n)
          | loop (t as (x, y, z)) =
            if isPythTriple t then t :: loop (x,y,z-1) else loop (x,y,z-1)
    in rev (loop (n, n, n)) end

And a tail-recursive variant of the same:
fun isPythTriple x y z = x*x + y*y = z*z
fun pyths n =
    let fun loop 0 _ _ res = res
          | loop x 0 _ res = loop (x-1) n n res
          | loop x y 0 res = loop x (y-1) n res
          | loop x y z res =
            let val res' = if isPythTriple x y z
                           then (x,y,z) :: res
                           else res
            in loop x y (z-1) res'
            end
    in loop n n n [] end


Golang async but sequential enqueue with buffered channel The objective of this program is to call enqueue sequentially with multiple URLs, and never block, fetching the URLs asynchronously, but on the order they were entered.

In other words: keep calling enqueue to add urls to a buffered channel (queue) (possibly with a specific callback), and consume one by one in the order they where entered in the queue. I used WaitGroup to prevent the go program from immediately exiting, but I feel I'm not taking advantage of the buffered channel at all.

package main

import (
    "fmt"
    "net/http"
    "sync"
)

type callback func(url string, res *http.Response, err error)

type callbackRequest struct {
    cb  callback
    url string
}

var queue = make(chan callbackRequest, 1)
var wg sync.WaitGroup

func worker() {
    for {
        req := <-queue
        fmt.Println("processing", req.url)
        res, err := http.Get(req.url)
        req.cb(req.url, res, err)
    }

}

func enqueue(url string) {
    fmt.Println("enqueue", url)
    cb := func(url string, res *http.Response, err error) {
        if err != nil {
            fmt.Println("error")
        }
        fmt.Println(url, res.Status)
        wg.Done()
    }
    cbReq := callbackRequest{url: url, cb: cb}
    wg.Add(1)
    go func(cbr callbackRequest) {
        queue <- cbReq
    }(cbReq)
}

func main() {
    go worker()
    enqueue("http://google.com")
    enqueue("http://reddit.com")
    enqueue("http://yahoo.com")
    enqueue("http://bing.com")
    enqueue("http://google.com")
    enqueue("http://yahoo.com")
    enqueue("http://reddit.com")
    enqueue("http://bing.com")
    enqueue("http://google.com")

    wg.Wait()
}

Output:

$ go run enqueue/main.go
enqueue http://google.com
enqueue http://reddit.com
enqueue http://yahoo.com
enqueue http://bing.com
enqueue http://google.com
enqueue http://yahoo.com
enqueue http://reddit.com
enqueue http://bing.com
enqueue http://google.com
processing http://google.com
http://google.com 200 OK
processing http://reddit.com
http://reddit.com 200 OK
processing http://yahoo.com
http://yahoo.com 200 OK
processing http://bing.com
http://bing.com 200 OK
processing http://google.com
http://google.com 200 OK
processing http://yahoo.com
http://yahoo.com 504 Gateway Timeout
processing http://reddit.com
http://reddit.com 200 OK
processing http://bing.com
http://bing.com 200 OK
processing http://google.com
http://google.com 200 OK

I believe that you are correct. The channel buffer really doesn't matter here because you are manually building an un-bounded buffer here:

go func(cbr callbackRequest) {
    queue <- cbReq
}(cbReq)

The queue <- cbReq line will block until queue has room, but since it is running in its own goroutine, it won't block progress for the entire program. Once there is room in queue, the goroutine will progress and immediately terminate, having deposited cbReq in the queue.

You can think about it as a bunch of people waiting in line to drop off packages at a post office for shipping. As the postal employee gets time, he removes the packages from the counter in the order they were brought in (maybe they are on a conveyor belt) and sends them on their way. If the counter (or conveyor belt) is large enough, several packages can be placed on it at one time. In this case, once a person has deposited her package on the counter, she may leave. If the counter was smaller, the people would have to wait in line longer, but their packages would still ship in the same order.

That is kind of a complicated analogy, but maybe it will help. The idea is that the queue either happens in the channel buffer, or in the blocked goroutines.

The one wrinkle here is that I am not entirely sure about the semantics of a blocked channel insertion operation. It appears from your output that the first goroutine to attempt to insert to a channel will be the first goroutine to be allowed to insert to that channel, but I couldn't find a reference to back that up.


Is this code thread-safe? EDIT: The full source code in question can be found here.

Is this code reasonably safe against dead/livelocks, and if not, how can I fix it?

I know processes are usually recommended over threads for operations like this, but processing's version of queue doesn't do what I need.
The choose function will be called on 100 objects, but in sequence. I think each one should be finished before the next, but I'm not certain.
The appraise function that choose calls only gets values from dicts, and does math on them.

---thread_queue.py---

class ThreadsnQueues(threading.Thread):
    def __init__(self, queue, out_queue=None, func=None, args=None, semaphore=None):
        super(ThreadsnQueues, self).__init__()
        self.queue = queue
        self.out_queue = out_queue
        self.semaphore = semaphore
        if func is None:
            try:
                func = self.queue.get(block=False)
            except Empty:
                self.func = None
                return
        if func is not None:
            if type(func) is list:
                self.func = func[0]
                self.args = func[1:]
            else:
                self.func = func
        if args != [] and args != None: self.args = args

class ThreadScore(ThreadsnQueues):
    def __init__(self, queue, out_queue, func, semaphore):
        super(ThreadScore, self).__init__(queue, out_queue, func, None, semaphore)
    def run(self):
        try:
            s = self.queue.get() #should be a stock object
        except Empty:
            print "Empty queue?!"
            return
        except:
            return
        while True:
            try:
                self.semaphore.acquire()
                ret = self.func(s) #should return a score (int)
                self.out_queue.put((s, ret)) #tuple with Stock, int
                self.queue.task_done()
                self.semaphore.release()
                return
            except KeyboardInterrupt:
                raise
            except ArithmeticError:
                a, b, c = exc_info()
                print a, b
                pdb.post_mortem(c)
            except StandardError:
                pass

---genome.py---

def choose(self, stocks):
    """
    Scores the whole list of stocks, and puts the top twelve on a list to be
    purchased.
    """
    tmp = {}
    queue = Queue.Queue()
    out_queue = Queue.Queue()
    procs = []
    thread_limit = 10
    self.prog = ProgressBar(widgets=[Percentage(), ' ', Bar(marker=RotatingMarker()), ' ', ETA()],
        maxval=len(stocks)).start()
    sym = threading.BoundedSemaphore(thread_limit)
    for stock in stocks:
        queue.put(stock)
    while queue.empty() != True:
        try:
            for j in range(thread_limit):
                t = ThreadScore(queue=queue, out_queue=out_queue, func=self.appraise, semaphore=sym)
                t.daemon = True
                t.start()
                procs.append(t)
        except ArithmeticError:
            raise
        except threrror:
            print "Encounted a thread error, blocking until threads complete..."
            [ p.join(5.0) for p in procs ]
            procs = []
        except KeyboardInterrupt:
            raise
        except StandardError as e:
            print e
            [ p.join(5.0) for p in procs ]
            procs = []
    [ p.join(timeout=5.0) for p in procs ]
    print "Finished scoring stocks, sorting..."
    self.prog.finish()
    while out_queue.qsize() > 0:
        l, r = out_queue.get() #unpack tuples of Stock, int
        tmp[l] = r #tmp[<Stock>] = score
    pmt = sorted(tmp, key=lambda x: tmp.get(x), reverse=True)
    self.prefers = { key: value for (key, value) in zip(pmt, sorted(tmp.values(), reverse=True))}
    self.scores = self.prefers.copy()
    return self.prefers

"reasonably safe against deadlocks" is a strange expression :)
In ThreadsnQueues the if func is not None: can be turned into an else.
Your code in ThreadScore indicates that some exceptions might be expected because you handle them and continue. The problem is if that happens while you have the semaphore acquired then it will not release it. You should move the release into finally block.
In choose you join on all threads in multiple places and this code [ p.join(5.0) for p in procs ] gets repeated several times. Should be refactored into a method.
I'm not sure what the point of the semaphore is. The way I read you code is:

You initialize the semaphore with N
Then you spawn N threads each calling acquire/release in it's run method.

This means that all threads can simultaneously enter the block protected by the semaphore. So what's the point of having it?
In ThreadScore.run() the main part is the while True loop. However it never seems to take anything of the queue (it only seems to do it once at the beginning of the run method) so how does it work?


Simplify finding indices of all instances of certain characters in a string I have a string util method for finding potential "wrapping points" in a string.

For example, if " " and "-" are considered wrapping chars, then for input string
 "Antoni Gil-Bao" the method would return the sorted set [6, 10].

private static final String WRAPPING_CHARS = " -";
private static final CharMatcher WRAP_CHAR_MATCHER = CharMatcher.anyOf(WRAPPING_CHARS);

public static SortedSet<Integer> findWrappingIndices(String s) {
    SortedSet<Integer> indices = Sets.newTreeSet();
    while (WRAP_CHAR_MATCHER.indexIn(s) != -1) {
        int index = WRAP_CHAR_MATCHER.indexIn(s);
        int indexInOriginalString = indices.isEmpty() ? index :  index + indices.last() + 1;
        indices.add(indexInOriginalString);
        s = s.substring(index + 1);
    }
    return indices;
}

Question is, is there a simpler way to implement this method using Guava? (Besides trivialities like inlining indexInOriginalString.) Based on my unit tests, the above does the job, but it doesn't feel that elegant. Obviously pure Java solutions are welcome too, if they are simpler than my Guava version. In any case, the method should take a String and return SortedSet<Integer>.

(Background: the reason I just want the indices, instead of doing any string wrapping here directly, is that eventually I'll be operating inside onDraw(), on a char array, and I want to measure pieces of text in the actual font before making wrapping decisions.)

Simplify is a relative term. If you want simple, then I would have:

public static int[] findWrappingIndices(final String s) {
    int pos = 0;
    int[] ret = new int[s.length()];
    for (int i = 0; i < s.length(); i++) {
        if (WRAPPING_CHARS.indexOf(s.charAt(i)) >= 0) {
            ret[pos++] = i;
        }
    }
    return Arrays.copyOf(ret, pos);
}

But, you want to complexify it by using a SortedSet (because int[] is not easy to use?), so you can then convert the int[] to the SortedSet.

I don't see why the SortedSet is a good idea, but, it is easy enough to convert to using it (even though it is bigger and slower than an int[] array).

I don't think you will find a simpler (or faster) alternative.

I would guess that you could quavify it with (note the data is being inserted in sorted order anyway):

public static SortedSet<Integer> findWrappingIndices(final String s) {
    SortedSet<Integer> indices = Sets.newTreeSet();
    for (int i = 0; i < s.length(); i++) {
        if (WRAPPING_CHARS.indexOf(s.charAt(i)) >= 0) {
            indices.add(i);
        }
    }
    return indices;
}


C++ custom exception handling using std::error_category and std::system_error I want to make my own custom exception handling and I am curious if I am going the right path, maybe some of you could suggest to me how I could improve it? Or maybe I have went the wrong direction? (Also it would be amazing to know how I could make this piece of code more of a modern C++.)
Here is a simple namespace with classes, which inherits from std::error_category and std::system_error.
#include <string>
#include <system_error>
#include <iostream>

namespace CustomException {

    class ErrorC : public std::error_category {
        using Base = std::error_category;
    public:
        char const *name() const noexcept
        override { return "App error"; }

        std::error_condition default_error_condition(int const code) const noexcept
        override {
            (void) code;
            return {};
        }

        bool equivalent(int const code, std::error_condition const &condition) const noexcept
        override {
            (void) code;
            (void) condition;
            return false;
        }

        bool equivalent(std::error_code const &code, int const condition) const noexcept
        override { return Base::equivalent(code, condition); }

        std::string message(int const condition) const
        override { return "An application error occurred, code = " + std::to_string(condition); }

        constexpr ErrorC() : Base{} {}
    };

    auto app_error_category()
    -> ErrorC const & {
        static ErrorC the_instance;
        return the_instance;
    }

    enum class BreakErrorCode {
        FAILED_INIT_BREAKS = 1,
        FAILED_TO_LINK_BREAKS = 2,
    };

    class BreakError : public std::system_error {
        using Base = std::system_error;
    public:
        BreakErrorCode appErrorCode() const {
            return static_cast<BreakErrorCode>(code().value());
        }

        explicit BreakError(const BreakErrorCode code)
                : Base{(int) code, app_error_category()} {
        }

        BreakError(const BreakErrorCode code, const std::string &description)
                : Base{(int) code, app_error_category(), description} {
        }
    };

    enum class EngineErrorCode {
        FAILED_INIT_ENGINE = 101,
        MOTHER_BOARD_FAILED_LINKING = 102,
    };

    class EngineError : public std::system_error {
        using Base = std::system_error;
    public:
        EngineErrorCode appErrorCode() const {
            return static_cast<EngineErrorCode>(code().value());
        }

        EngineError(const EngineErrorCode code)
                : Base{(int) code, app_error_category()} {
        }

        EngineError(const EngineErrorCode code, const std::string &description)
                : Base{(int) code, app_error_category(), description} {
        }
    };

    enum class ControlErrorCode {
        CONTROL_GIMBAL_FAILED = 301,
        OBTAIN_CONTROL_AUTHORITY_FAILED = 302,
    };

}

Main.cpp, which invokes methods throwing exceptions from previously declared namespace.
using namespace CustomException;

void initEngine() {
    throw EngineError(EngineErrorCode::FAILED_INIT_ENGINE, "Failed init engine");
}

void initBreaks() {
    throw BreakError(BreakErrorCode::FAILED_INIT_BREAKS, "Failed init breaks");
}

int main() {
    try {
        initEngine();
        initBreaks();
    }
    catch (const BreakError &error) {
        std::cout << error.what() << std::endl;
    }
    catch (const EngineError &error) {
        std::cout << error.what() << std::endl;
    }
    catch (...) {
        std::cout << "Caught something unexpected error" << std::endl;
    }
    return 0;
}

I would appreciate very much any kind of comments / help.

Here are some things that may help you improve your code.
Separate interface from implementation
The interface goes into a header file and the implementation (that is, everything that actually emits bytes including all functions and data) should be in a separate .cpp file.  The reason is that you might have multiple source files including the .h file but only one instance of the corresponding .cpp file.  In other words, split your existing CustomException namespace into into a .h file and a .cpp file.
Correctly override functions
The code currently contains this as part of class ErrorC:
bool equivalent(int const code, std::error_condition const &condition) const noexcept
override {
    (void) code;
    (void) condition;
    return false;
}

This is not right for several reasons.  First, the standard says that this should return true if the error is equivalent, but this does not do that.  Instead, I'd write:
return default_error_condition(code) == condition;

Better still, omit it, since this is not a pure virtual method in std::error_category.
Don't create pointless classes
The CustomException class is doing very little here and I would strongly recommend omitting it and just deriving directly from std::error_category.  See C.120 for further guidance.
Don't use all caps for enum name
ALL CAPS have been traditionally used for macros.  To avoid misleading the reader, don't use them if it's not a macro.  See ES.9.
Fix the spelling
If the machine has "breaks" it is broken.  If it has "brakes" it has the ability to slow down.  These homonyms are easily confused, but it's important to make sure you don't have spelling errors, especially in interface code, so that you don't confuse users of your code.
Consider combining error codes
It appears that you are intending to have non-overlapping and unique error codes for each type of error, but that is not assured because the errors are in three different enum classes.  I'd suggest combining them.
Reconsider your classes
If the error codes were combined, as suggested above, then the differentiation among them would be by the error code.  Having different classes doing exactly the same thing makes little sense to me.  I'd suggest a single error class derived from std::error_category would be sufficient.


Is this code efficient for file parsing? Is the below code efficient for parsing the file, or do I face performance issues?  If the latter, please offer suggestions.

#include <iostream>
#include <fstream>
#include <map>

using namespace std;
ofstream outfile;
void writedata(string data, string file)
{
    outfile.open(file.c_str(), ios::app);
    outfile << data;
    outfile.close();
}

int main()
{
    ifstream input("data.txt", ios::in);

    ofstream out;
    string line;
    string tmp;
    string data;
    string filename;
    string name;
    size_t found;
    map<string, string> MyFileMap;
    map<string, string>::iterator it;

    while (input >> line)
    {
        found = line.find_last_of("pmf");
        if (found != string::npos)
        {
            name = line.substr(found + 1, line.length());
            filename = line.substr(found + 1, line.length());
            cout << "filename without txt " << filename << endl;
            for (it = MyFileMap.begin(); it != MyFileMap.end(); ++it)
            {
                if (name.compare(it->first) == 0)
                {
                    while (input >> tmp && tmp.compare("pme"))
                    {
                        data += tmp;
                    }
                    cout << "Data push " << data << " to " << it->second << endl;
                    writedata(data, it->second);
                    data.clear();
                    break;
                }
            }
            if (it == MyFileMap.end() || MyFileMap.empty())
            {
                cout << "No match in map" << endl;
                filename = filename + ".txt";
                cout << "Filename " << filename << endl;
                while (input >> tmp && tmp.compare("pme"))
                {
                    data += tmp;
                }
                cout << "Data push " << data << " to " << it->second << endl;
                out.open(filename.c_str());
                MyFileMap[name] = filename;
                for (it = MyFileMap.begin(); it != MyFileMap.end(); ++it)
                {
                    if (name.compare(it->first) == 0)
                    {
                        writedata(data, it->second);
                        data.clear();
                        break;
                    }
                }
            }
        }
    }
    system("pause");
}

sample file : data.txt

pmfwork1
data1
data1
pme
pmfwork2
data2
data2
pme
pmfwork3
data3
data3
pme
pmfwork4
data4
data4
pme

In the above code, I am trying to parse the sample file with pmf and pme as begin and end flags, work as the file name, and data as file contents.  Below is the sample file.

I would like to reduce the use of open call for multiple times.  It would be of great help if you provide any suggestions.

Above code is generating four different text files: work1, work2 and so on.

ofstream outfile;

Why is this declared global? The variable is only used in writedata. Declare it in this scope.
ofstream out;

What is the purpose of this stream? It is opened (multiple times!) in the code but never written to.
string tmp;
string data;
string filename;
string name;
size_t found;

You should declare objects in the narrowest scope that they need to exist, instead of the function head.
while (input >> line)

The operator >> will read one word at a time, not a whole line. If you want to read whole lines, you should use while(std::getline(input, line)).
found = line.find_last_of("pmf");

I think this doesn't do what you expect it to do. It finds the position of the last character in line which is equal to either of the single characters in "pmf". You probably want to use line.rfind("pmf") instead, which returns the position of the last occurrence of the full string "pmf".

Looking at your example file however I believe you actually want to check whether the beginning of the line is "pmf" (consider the work string might include "pmf"), so you would use line.find("pmf") and check it against 0 instead of string::npos.
name = line.substr(found + 1, line.length());
filename = line.substr(found + 1, line.length());

These two strings are the same? Why? In the following code they are used interchangeable, so stick to one and remove the other. Even if they were used differently you could still use filename = name.

Also you are trying to get a substring of the same length as the whole line (the second argument to substr is the length of the substring to be extracted), which obviously doesn't work. However substr truncates in this case such that a substring ending at the end of the input string is returned.

The supposed filename you are extracting should be the part after pmf in pmfwork1. However you start the substring one position after the position of the last occurrence of pmf, which would be result in "mf" being part of the string. However with the particular example file you provided and in combination with the semantic error I laid out in 5., you actually get the expected result. Here two errors cancel each other. For different input however you would have very unexpected results.
for (it = MyFileMap.begin(); it != MyFileMap.end(); ++it)
    if (name.compare(it->first) == 0)

The whole purpose of a map is to be able to access the values via a key instead of iterating over them. You should use this functionality:

auto it = MyFileMap.find(name);
if(it != MyFileMap.end())
    // "name" found it is at "it"
else
    // "name" is not in "MyFileMap"!

while (input >> tmp && tmp.compare("pme"))

Again the input is one word each instead of lines. Data which includes the word "pme" would trigger the end.
if (it == MyFileMap.end() || MyFileMap.empty())

The second condition is redundant. If MyFileMap.empty() is true, then also it == MyFileMap.begin() == MyFileMap.end() at this point.
cout << "Data push " << data << " to " << it->second << endl;

Here you try to dereference it, which you previously checked to be MyFileMap.end(). Dereferencing the end() iterator invokes undefined behaviour. Since I guess these lines are debug output only anyway, just remove that part.
MyFileMap[name] = filename;
for (it = MyFileMap.begin(); it != MyFileMap.end(); ++it)
{
    if (name.compare(it->first) == 0)
    {
        writedata(data, it->second);
        data.clear();
        break;
    }
}

What is the purpose of this loop? You just inserted the key name into the map and now you search it again. Why not simply access it directly:

MyFileMap[name] = filename;
writedata(data, MyFileMap[name]);
data.clear();

Note however that this is the same as, but in general slower than:

MyFileMap[name] = filename;
writedata(data, filename);
data.clear();

system("pause");

In order to use std::system you need to #include<cstdlib>.
What is the purpose of the MyFileMap at all? It stores the names as keys and filenames as values. The only difference between the two is that filename has an ".txt" added. Why do you need to save this? You can append the ".txt" each time you have a new name. This would be much easier to read and much faster.
If you are trying to minimize the number of open calls you might want to store all the opened ofstreams in the map and access them as you need them, closing all at the end of the function. Notice however that ofstream is not copyable, so you need to store references to them in the map, instead of the objects themself.

If the input files are not that big, you might also consider saving all the parsed data in memory first and then writing it to the output files in one go. While parsing you can use a string-string map to save the data for each file.
You are lacking any check of syntax errors in the input file. You probably should add code that reports back if the syntax is not as expected instead.

In summary:

Your code will likely not do what you expect it to do in many cases. You should generate more diverse test cases. (try pmfworkm instead of pmfwork1 in your example file and report what happens).

Several parts of the code are redundant (all the map-iterating loops) and at the same time they might be very time costly for large input files (since the map might grow rapidly).


AI to avoid obstacle The AI I'm making is really simple, however it might be a bit too inefficient for what it is doing. The chart below shows speed differences between various arithmetic and math operations. sin, cos and especially atan are really inefficient. It's tested in C++ but should still hold true to JavaScript.

Is it possible to achieve the same result with more efficient math?

Chart Source

const canvas = document.getElementById('canvas');
const ctx = canvas.getContext('2d');

let targetX = 50;
let targetY = 50;

let obstacleX = 150;
let obstacleY = 50;

let aiX = 250;
let aiY = 51;

function loop() {
    // Distance between the vector points
    let disTargetX = targetX - aiX;
    let disTargetY = targetY - aiY;

    let disObstacleX = obstacleX - aiX;
    let disObstacleY = obstacleY - aiY;

    // Moves to target by default
    const angleTarget = Math.atan2(disTargetY, disTargetX);
    let moveAngle = angleTarget;

    // If near obstacle, adjust course and try to avoid it
    if (Math.sqrt(disObstacleX * disObstacleX + disObstacleY * disObstacleY) < 60) {
        const angleObstacle = Math.atan2(disObstacleY, disObstacleX);
        moveAngle += angleTarget - angleObstacle;
    }

    // Move the vector to desired location
    aiX += Math.cos(moveAngle);
    aiY += Math.sin(moveAngle);

    //Drawing
    ctx.clearRect(0, 0, 600, 200);

    ctx.beginPath();
    ctx.fillStyle = "teal";
    ctx.arc(aiX, aiY, 10, 0, Math.PI * 2, true);
    ctx.fill();

    ctx.beginPath();
    ctx.fillStyle = "purple";
    ctx.arc(obstacleX, obstacleY, 10, 0, Math.PI * 2, true);
    ctx.fill();

    ctx.rect(targetX - 20, targetY - 20,40,40);
    ctx.stroke();

    requestAnimationFrame(loop);
}

requestAnimationFrame(loop);
<canvas id="canvas" width="600" height="200"></canvas>

You are probably aware of this but representing floating point numbers in JavaScript is difficult to do without rounding errors
Are you sure you the math is correct in the calculations? Specifically, when subtracting angleObstacle from moveAngle, should angleTarget really be added? I attempted to change it such that angleTarget does not get added when that happens. It appears to allow the green circle to avoid colliding with the obstacle circle. I am not sure why the speed changes... perhaps because of the exponential change with the arc tan or other sin functions.

const canvas = document.getElementById('canvas');
const ctx = canvas.getContext('2d');

const targetX = 50;
const targetY = 50;

const obstacleX = 150;
const obstacleY = 50;

let aiX = 250;
let aiY = 51;

function loop() {
    // Distance between the vector points
    const disTargetX = targetX - aiX;
    const disTargetY = targetY - aiY;

    const disObstacleX = obstacleX - aiX;
    const disObstacleY = obstacleY - aiY;

    // Moves to target by default
    const angleTarget = Math.atan2(disTargetY, disTargetX);
    let moveAngle = angleTarget;

    // If near obstacle, adjust course and try to avoid it
    if (Math.sqrt(disObstacleX * disObstacleX + disObstacleY * disObstacleY) < 60) {
        const angleObstacle = Math.atan2(disObstacleY, disObstacleX);
        moveAngle += /*angleTarget -*/angleObstacle;
    }

    // Move the vector to desired location
    aiX += Math.cos(moveAngle);
    aiY += Math.sin(moveAngle);

    //Drawing
    ctx.clearRect(0, 0, 600, 200);

    ctx.beginPath();
    ctx.fillStyle = "teal";
    ctx.arc(aiX, aiY, 10, 0, Math.PI * 2, true);
    ctx.fill();

    ctx.beginPath();
    ctx.fillStyle = "purple";
    ctx.arc(obstacleX, obstacleY, 10, 0, Math.PI * 2, true);
    ctx.fill();

    ctx.rect(targetX - 20, targetY - 20,40,40);
    ctx.stroke();

    if (aiX > 50) {
        requestAnimationFrame(loop);
    }
}

requestAnimationFrame(loop);
<canvas id="canvas" width="600" height="200"></canvas>

Other review points
In the code snippet above, you will notice that let has been replaced with const for many of the declarations of variables that are not re-assigned within the function - e.g. targetX, targetY, obstacleX, obstacleY, disTargetX, disTargetY . This avoids any unintentional re-assignment.
Also, the original code loops forever. I added a condition in the snippet above - i.e. aiX > 50 which needs to be true in order for the function to be called again (via requestAnimationFrame). This avoids unnecessary processing.


Applescript to change file extensions I'm new at scripting, but I thought it would be handy to have a script that would change the extensions of selected files in Finder.  I run this script from an automator service so that batch changing extensions in Finder is always just a right click away.

So far, it has worked as I expect, but since this is one of my first scripts, my question is: is there a cleaner way to obtain this functionality?  Can anyone point out a case I haven't thought of in which this would fail?

What this script does:

Asks the user for a new extension.
Adds the new extension to any files that don't already have one.
Changes the extension in other files even if it's a hidden file or has multiple points.

A test case that does not function correctly:

The selected file's name has multiple points, but no extension. Here, my script replaces the text after the last point with the new extension.

Here's the code:

# Ask user for new extension to be used
set newExt to the text returned of (display dialog "Enter extension:" default answer "Do not include first point")

tell application "Finder"

    # Make a list of the selected files in Finder
    set selFiles to selection as list

    set TID to AppleScript's text item delimiters

    repeat with eachFile in selFiles

        # Make a text item list of the file name delimited by points
        set filePath to eachFile as text
        set AppleScript's text item delimiters to {":"}
        set fileName to last text item of filePath
        set AppleScript's text item delimiters to {"."}
        set textItems to text items of fileName

        # Handle case where there is currently no extension, but one should be added.
        if number of textItems is 1 then
            set name of eachFile to fileName & "." & newExt

        # If an extension does already exist...
        else

            set newName to ""
            set numItems to number of items in textItems
            set n to 1

            repeat numItems times

                # If the current text item is not the extension, add it & "." to the new file name.  No need here to consider the last text item since it's the old extension and we want to get rid of it.
                if n is not numItems then
                    set newName to newName & item n in textItems & "."
                    set n to n + 1
                end if

            end repeat

            set name of eachFile to newName & newExt

        end if

    end repeat

    # Is this line necessary?  I just saw it at the end of someone else's script that had earlier set TID to AppleScript's text item delimiters.
    set AppleScript's text item delimiters to TID

end tell

Feel free to use this if it would be useful for you.

Breaking file paths into file names and file extensions is unneeded when properties exist to access these elements directly, in this case the name extension property.

set newExt to the text returned of (display dialog "Enter extension:" default answer "Do not include first point")

tell application "Finder"
    set selFiles to selection
    repeat with eachFile in selFiles
        set name extension of eachFile to newExt
    end repeat
end tell


Calculating premium from a rate chart I have to calculate the premium from a rate chart based on a person's age and the amount of insurance they request. I started out using if/switch statements, but the code is getting too long and seems pretty redundant.

var premium;
var beneAmt = $('#PrimaryApplicantTTLBenefitAmountInput').val();
var age = $('#PrimaryApplicantAge').val();

if (50000 <= beneAmt < 75000) {

    switch (true) {

        case (age < 30):

            premium = 2.00;
            break;

        case (30 <= age < 40):

            premium = 2.50;
            break;

        case (40 <= age < 50):

            premium = 5.00;
            break;

        case (50 <= age < 60):

            premium = 15.00;
            break;

        case (60 <= age < 65):

            premium = 21.50;
            break;

        case (65 <= age < 70):

            premium = 20.80;
            break;

        case (70 <= age < 75):

            premium = 23.25;
            break;

        case (age >= 75):

            premium = 35.00;
            break;

    }

} else if (75000 <= beneAmt < 100000) {

    switch (true) {

        case (age < 30):

            premium = 3.00;
            break;

        case (30 <= age < 40):

            premium = 3.75;
            break;

        case (40 <= age < 50):

            premium = 7.50;
            break;

        case (50 <= age < 60):

            premium = 22.50;
            break;

        case (60 <= age < 65):

            premium = 32.25;
            break;

        case (65 <= age < 70):

            premium = 31.20;
            break;

        case (70 <= age < 75):

            premium = 34.88;
            break;

        case (age >= 75):

            premium = 52.50;
            break;

    }

} else if (100000 <= beneAmt < 125000) {

    switch (true) {

        case (age < 30):

            premium = 4.00;
            break;

        case (30 <= age < 40):

            premium = 5.00;
            break;

        case (40 <= age < 50):

            premium = 10.00;
            break;

        case (50 <= age < 60):

            premium = 30.00;
            break;

        case (60 <= age < 65):

            premium = 43.00;
            break;

        case (65 <= age < 70):

            premium = 41.60;
            break;

        case (70 <= age < 75):

            premium = 46.50;
            break;

        case (age >= 75):

            premium = 70.00;
            break;

    }

}

How else could I go about this?

I think the main improvement to this code would be to separate the definition of the premium lookup table from the logic that finds the premium for a given benefit and age, for example, using an Object-Oriented design.  Doing so will simplify changes to the table and make the code easier to test and debug.

For example, consider the usage below that makes the table itself an object mapping benefit and age ranges to premium values:

var premiumTable = new PremiumTable({
  '[50000, 75000)': {  // 50k <= benefit < 75k
    '     30)':  2.00, //          age <  30
    '[30, 40)':  2.50, //    30 <= age <= 40
    '[40, 50)':  5.00, //    40 <= age <= 50
    // ...
    '[75     ': 35.00, //    75 >= age
  },
  '[75000, 100000)': {
    // ...
  },
  // ...
});

var premium = premiumTable.valueForBenefitAndAge(someBenefit, someAge);

Now you can implement the valueForBenefitAndAge method in a way that can be unit tested and changes to the premium table are localized to the initializer object rather than being related directly to the code itself.

Moreover, the PremiumTable constructor function can validate the premium table input object based on rules that will be easier to encode than using your example code.  Additionally, the table could be loaded as a separate resource to further isolate the code that applies the logic from the premium table values themselves.


Program to provide CRUD operations for a Boxers(Fighter) table in a database I have been through a few SQLite tutorials and wrote this code on my own to reinforce the principles. The tutorials I went through varied widely in a few areas so this is what I came up with as a combination of everything.

Other than the two classes I have listed below, all I have is one activity that adds, remove, inserts, update and display data from the database.

I have a few specific questions:

When should I close the helper class or does garbage collection deal with it automatically?
Should I create a Boxer POJO (Plain Old Java Object) to pass boxer data to and from the DAO?
Is the DAO implementation efficient?
Does the code deviate from Java and Android best practices in any way?

Helper Class

public class BoxScoresHelper extends SQLiteOpenHelper {

    private static final String DB_NAME = "boxing_scores.db";
    private static final int VERSION = 1;
    private static BoxScoresHelper instance = null;

    public static BoxScoresHelper getInstance(Context context){

        if(instance == null){

            instance = new BoxScoresHelper(context);

        }

        return instance;
    }

    private BoxScoresHelper(Context context) {
        super(context, DB_NAME, null, VERSION);

    }

    @Override
    public void onCreate(SQLiteDatabase db) {

        db.execSQL(createBoxerSQLString());

    }

    @Override
    public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) {

        db.execSQL("Drop Table If Exists " + BoxerDAO.TABLE_NAME);
        onCreate(db);

    }

    private String createBoxerSQLString(){

        String boxerCreateString = "create table " + BoxerDAO.TABLE_NAME +
                "(" + BoxerDAO._ID + " Integer Primary Key AutoIncrement, " +
                BoxerDAO.BOXER_NAME +  " Text Not Null, " +
                BoxerDAO.WEIGHT_CLASS + " Text Not Null, " +
                BoxerDAO.WINS + " Integer Not Null, " +
                BoxerDAO.LOSSES + " Integer Not Null);";

        return boxerCreateString;
    }

}

DAO Class

public class BoxerDAO {

    public static final String TABLE_NAME = "Boxer";
    public static final String _ID  = "_id";
    public static final String BOXER_NAME = "boxer_name";
    public static final String WEIGHT_CLASS = "weight_class";
    public static final String WINS = "wins";
    public static final String LOSSES ="losses";

    private final BoxScoresHelper myScoresHelper;
    private SQLiteDatabase myBoxerDB;

    public BoxerDAO(Context context){

        myScoresHelper = BoxScoresHelper.getInstance(context);

    }

    public Cursor query(String[] projection,String selection,String[] selectionArgs, String orderBy){

        Cursor cursor;
        myBoxerDB = myScoresHelper.getReadableDatabase();

        cursor = myBoxerDB.query(TABLE_NAME, projection, selection, selectionArgs, null, null, orderBy);

        //myBoxerDB.close();

        return cursor;

    }

    public Cursor queryAll(){

        Cursor cursor;
        myBoxerDB = myScoresHelper.getReadableDatabase();

        cursor = myBoxerDB.rawQuery("Select * From " + TABLE_NAME, null);

        //myBoxerDB.close();
        return cursor;

    }

    public int delete(int id){

        int rowsDel;
        myBoxerDB = myScoresHelper.getWritableDatabase();

        rowsDel = myBoxerDB.delete(TABLE_NAME, _ID + " = " + id , null);

        //myBoxerDB.close();
        return rowsDel;

    }

    public long insert(ContentValues values){

        long insertId = -1;

        myBoxerDB = myScoresHelper.getWritableDatabase();

        insertId = myBoxerDB.insert(TABLE_NAME, null, values);

        //myBoxerDB.close();

        return insertId;

    }

    public int update(ContentValues values,String selection, String[] selectionArgs){

        int updatedRows;
        myBoxerDB = myScoresHelper.getWritableDatabase();

        updatedRows = myBoxerDB.update(TABLE_NAME, values, _ID + " = " + selection, selectionArgs);
        //myBoxerDB.close();

        return updatedRows;

    }

}

Convert member field to local variable when you can

In BoxerDAO, the myBoxerDB member can be converted to a local variable.
Each utility method gets an SQLiteDatabase instance from the helper.
The class needs only an SQLiteOpenHelper as a member field.

Refer to types by interfaces

In BoxerDAO,
myScoresHelper can be declared as an instance of SQLiteOpenHelper.
It doesn't use any features of BoxScoresHelper.

Class design

BoxerDAO is only aware of Context to get an instance of BoxScoresHelper.
It would be better if it wasn't aware of Context at all.
You could pass to the constructor an SQLiteOpenHelper instead.

Naming

Prefixing member fields with "my" is not a common convention.
You could simply drop that.

Coding style

There are too many blank lines in the code.
This is unnecessary, it would be good to trim the code a little.


Reading a file in chunks I currently have the following class that takes a custom file object as an argument and makes a copy of the file locally.

class FileCopier
 def initialize(file_info_object)
  @file_info_object = file_info_object
 end

 def copy
  File.open(local_file_path, 'wb') do |f|
   read_file_in_chunks do |chunk|
     f.write chunk
   end
  end
 end

private

 def local_file_path
  File.join(Rails.root,'tmp/blah.txt')
 end

 def read_file_in_chunks(chunksize=10343)
  index = 0
  size = @file_info_object.size
  path = @file_info_object.path

  while index < filesize
   yield Client.read(path, offset: index, length: chunksize)
   index += chunksize
  end

 end

end

The copy method seems to be fairly straight forward but I was wondering if there is a way to simplify it. It seems difficult to test to me at the moment.

Some notes:

Use 2-space indentation.
f.write chunk. You use parens in other places, be consistent.

Other than these minor issues, I think your code is pretty good. I'd just refactor read_file_in_chunks to use a enumerator instead (more versatile) and a range.

def read_file_in_chunks(chunksize=10343)
  Enumerator.new do |yielder|
    (0...@file_info_object.size).step(chunksize).each do |offset|
      data = Client.read(@file_info_object.path, offset: offset, length: chunksize)
      yielder.yield(data)
    end
  end
end

Using lazy collections (Ruby >= 2.0) it can be simplified even more:

def read_file_in_chunks(chunksize=10343)
  (0...@file_info_object.size).step(chunksize).lazy.map do |offset|
    Client.read(@file_info_object.path, offset: offset, length: chunksize)
  end
end


NLTK sentence / word tokenize I have a method that takes in a String parameter, and uses NLTK to break the String down to sentences, then into words. Afterwards, it converts each word into lowercase, and finally creates a dictionary of the frequency of each word.

import nltk
from collections import Counter

def freq(string):
    f = Counter()
    sentence_list = nltk.tokenize.sent_tokenize(string)
    for sentence in sentence_list:
        words = nltk.word_tokenize(sentence)
        words = [word.lower() for word in words]
        for word in words:
            f[word] += 1
    return f

I'm supposed to optimize the above code further to result in faster preprocessing time, and am unsure how to do so. The return value should obviously be exactly the same as the above, so I'm expected to use nltk though not explicitly required to do so. I also have access to scipy, numpy and pandas though I'm not sure how they would help in this case.

Is there any way to speed up the code? Or, if not, any way to make the code at least more elegant (i.e. pythonic)?

1. Quick review:

The collections.Counter class has an update method that adds counts for items in an iterable. So instead of:

words = nltk.word_tokenize(sentence)
words = [word.lower() for word in words]
for word in words:
    f[word] += 1

you could write:

f.update(word.lower() for word in nltk.word_tokenize(sentence))

There's no need to call sent_tokenize if you are then going to call word_tokenize on the results — if you look at the implementation of word_tokenize you'll see that it calls sent_tokenize, so by calling it yourself you're doubling the amount of work here.

Revised code:

def freq2(string):
    return Counter(word.lower() for word in nltk.word_tokenize(string))

This is about 35% faster than the original code, mostly due to avoiding the duplicate sentence processing. But can we make further progress?

2. Profiling

When you have to improve the speed of some piece of code, there is a standard approach that works like this:

Prepare a repeatable and representative test case whose execution time you can measure.
Profile the execution of the code on the test case you prepared in step 1.
The profiling results will usually show that a small fraction of the code is responsible for a large fraction of the runtime. (If this is not the case, you've reached the limits of what can be achieved by this approach, so stop here.)
Investigate the code you identified in step 3 and make it faster.
Go to step 2.

So let's try this. Here's the test case:

TEST_FILE = 'nltk_data/corpora/gutenberg/melville-moby_dick.txt'
TEST_TEXT = open(TEST_FILE).read()
TEST_CASE = lambda:freq2(TEST_TEXT)

We can time the execution using the timeit module:

>>> from timeit import timeit
>>> timeit(TEST_CASE, number=1)
1.699578917992767

And profile the execution using the cProfile module:

>>> cProfile.run('TEST_CASE()', sort='time')
         2201133 function calls (2158188 primitive calls) in 2.127 seconds

   Ordered by: internal time

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
   281219    1.100    0.000    1.303    0.000 {method 'sub' of '_sre.SRE_Pattern' objects}
     9853    0.165    0.000    0.573    0.000 punkt.py:1287(_slices_from_text)
   216744    0.082    0.000    0.129    0.000 re.py:323(_subx)
     9852    0.064    0.000    1.366    0.000 treebank.py:96(tokenize)
   254990    0.060    0.000    0.084    0.000 cr153873.py:16(<genexpr>)
        1    0.058    0.058    0.142    0.142 {built-in method _collections._count_elements}
    25066    0.054    0.000    0.120    0.000 punkt.py:388(__init__)
    35055    0.052    0.000    0.225    0.000 punkt.py:533(_tokenize_words)
   216744    0.047    0.000    0.047    0.000 re.py:306(_compile_repl)
    37938    0.042    0.000    0.056    0.000 sre_parse.py:931(expand_template)
    12491    0.033    0.000    0.033    0.000 {method 'findall' of '_sre.SRE_Pattern' objects}
    25066    0.031    0.000    0.056    0.000 punkt.py:581(_first_pass_annotation)
   294926    0.029    0.000    0.029    0.000 {method 'lower' of 'str' objects}
[... and so on ...]

It takes a bit of practice to read this. Each row of the table gives statistics for a single function. The "tottime" column gives the total time spent running code in that function (but only in that function, not in any functions called by it). The "cumtime" column gives the cumulative time spent in that function (including time spent in functions called by it).

The profiling results satisfy the condition in step (3) — that is, a large fraction of the runtime (more than half) is spent in a single function, the sub method on regular expression objects. Why is so much time being spent here? In order to find out who is calling this method, we can use the print_callers method from the pstats module, like this:

>>> cProfile.run('TEST_CASE()', 'cr153873.profile')
>>> import pstats
>>> pstats.Stats('cr153873.profile').print_callers("method 'sub'")
   Random listing order was used
   List reduced from 83 to 1 due to restriction <"method 'sub'">

Function        was called by...
                    ncalls  tottime  cumtime
{method 'sub'}  <-       1    0.000    0.000  re.py:175(sub)
                     25066    0.022    0.022  nltk/tokenize/punkt.py:411(_get_type)
                    256152    1.078    1.282  nltk/tokenize/treebank.py:96(tokenize)

So the caller that's responsible is the tokenize method of the TreebankWordTokenizer class, which is called by nltk.word_tokenize. Looking at the implementation, you can see that it works by making many regular expression substitutions to the text being tokenized. Each substitution requires a pass over the text and then making a copy of the text. This is wasteful: it would be more efficient to pass once over the text being tokenized, and yield the tokens as they are found.

3. A faster word tokenizer

So it looks as if one way to significantly improve the performance of the code would be to write your own word tokenizer with the same (or similar enough) behaviour as TreebankWordTokenizer, but improved performance, and use that instead.

Here's a very rough sketch of how to do this:

import re
from nltk.tokenize.api import TokenizerI

class FasterTreebankishWordTokenizer(TokenizerI):
    """Word tokenizer that behaves similarly to TreebankWordTokenizer, but
    runs faster.

    """

    # Words that are tokenized as compounds.
    COMPOUND_WORDS = """
        can/not d/'ye gim/me gon/na got/ta lem/me mor/'n wan/na
    """

    # Dictionary mapping a compound word to a tuple of its tokens.
    COMPOUND = {
        a + b: (a, b) for a, b in (w.split('/') for w in COMPOUND_WORDS.split())
    }

    # Regular expression matching a token or a compound word.
    TOKEN_RE = re.compile(
        r'''
        '(?:[msd]|(?:ll|re|ve|tis|twas))\b
        |\b(?:[a-z]+n't|{})\b
        |--
        |\.\.\.
        |[^\s.\[\](){{}}<>;@#$%&:,?!"']+
        |\S
        '''.format('|'.join(COMPOUND)),
        re.VERBOSE | re.IGNORECASE)

    def tokenize(self, text):
        _compound = self.COMPOUND
        for token in self.TOKEN_RE.findall(text):
            if token.endswith("n't"):
                yield token[:-3]
                yield "n't"
            elif token in _compound:
                yield from _compound[token]
            else:
                yield token

Note that this is only a rough sketch to help illustrate the approach: it's untested, and I know that it doesn't have the same behaviour as TreebankWordTokenizer in lots of cases. If you end up going down this road, then there will lots of work ahead getting the details right.

But just for illustration, here's the new implementation of freq using the faster tokenizer:

def freq3(string):
    word_tokenizer = FasterTreebankishWordTokenizer().tokenize
    result = Counter()
    for sentence in nltk.tokenize.sent_tokenize(string):
        result.update(word_tokenizer(sentence.lower()))
    return result

and this is about twice as fast as freq2 (three times as fast as the original freq):

>>> timeit(lambda:freq3(TEST_TEXT), number=1)
0.8160900929942727


Return the best factors of a whole number I've just finished writing my code to return the prime factors of a number and I feel the GetBestFactors function could be improved.
The GetFactors function was hacked together from the code found here:https://support.microsoft.com/en-us/kb/202782.  This returns all the factors of a number in a collection.

The GetBestFactors then loops through each number finding the best set to use.
So my question is - could the GetBestFactors function use less For...Next loops?

Sub Test()

    Dim tmp As Collection
    Set tmp = New Collection

    Dim tmp1 As Collection
    Set tmp1 = New Collection

    Set tmp = GetFactors(15) 'Change number - will return all factors.
    Set tmp1 = GetBestFactors(15) 'Change number - will return best factors.

    Debug.Assert False

End Sub

'Returns the factors of a whole number.
Public Function GetFactors(NumToFactor As Single) As Collection

    Dim Count As Integer
    Dim Factor As Single
    Dim y As Single
    Dim tmpCollection As Collection

    Set tmpCollection = New Collection

    Count = 0
    For y = 1 To NumToFactor
        Factor = NumToFactor Mod y
        If Factor = 0 Then
            tmpCollection.Add y
        End If
    Next y

    Set GetFactors = tmpCollection

End Function

'Returns the highest factors of a number.
Public Function GetBestFactors(NumToFactor As Single) As Collection

    Dim tmpFactors As Collection
    Dim FactorNums As Collection
    Dim x As Single, y As Single, z As Single
    Dim FirstFactor As Single

    Set tmpFactors = New Collection
    Set FactorNums = New Collection

    'Get all factors for the number.
    Set FactorNums = GetFactors(NumToFactor)

    'If the collection has 1 item then the NumToFactor is 1.
    'If there's 2 items then it's a prime number (1 and NumToFactor)
    If FactorNums.Count = 1 Or FactorNums.Count = 2 Then
        tmpFactors.Add FactorNums(FactorNums.Count)
    Else
        For x = FactorNums.Count - 1 To 1 Step -1
            If FactorNums(x) ^ 2 = NumToFactor Then
                tmpFactors.Add FactorNums(x)
                tmpFactors.Add FactorNums(x)
                Exit For
            Else
                For y = x To 1 Step -1
                    FirstFactor = FactorNums(y)
                    For z = y - 1 To 1 Step -1
                        If FirstFactor * FactorNums(z) = NumToFactor Then
                            tmpFactors.Add FirstFactor
                            tmpFactors.Add FactorNums(z)
                            Exit For
                        End If
                    Next z
                    If tmpFactors.Count = 2 Then Exit For
                Next y
            End If
            If tmpFactors.Count = 2 Then Exit For
        Next x
    End If

    Set GetBestFactors = tmpFactors

End Function

I don't understand the logic specifically

allFactors(15) = 1,3,5,15
bestFactors(15) = 3,5

allFactors(7) = 1,7
bestFactors(7) = 7

allFactors(1) = 1
bestFactors(1) = 1

allFactors(0) = empty
bestFactors(0) = empty

allFactors(-15) = empty
bestFactors(-15) = empty

All of your Single variables can be Long - single is hardly used in VBA.

Your naming could use some improvement:

Sub TestForFactors()

    Dim allFactors As Collection
    Set allFactors = New Collection

    Dim bestFactors As Collection
    Set bestFactors = New Collection

    Set allFactors = GetFactors(15) 'Change number - will return all factors.
    Set bestFactors = GetBestFactors(15) 'Change number - will return best factors.

    Debug.Assert False

End Sub

That's easier to understand.

Standard VBA naming conventions have camelCase for local variables and PascalCase for other variables and names. Variable names - give your variables meaningful names. Instead of

NumToFactor = numberToFactor
Count = count
Factor = factor
tmpCollection = getFactorsCollection
tmpFactors = bestFactorsCollection
FactorNums = factorNumbers
FirstFactor = firstFactor

x,y,z = ??

All of your procedures are Public - they should be Private unless needed to be Public.

All of your function arguments are being passed ByRef - they should be passed ByVal

In terms of your logic, you're looping through every number to find factors. There are certain rules you know you can abide by
 - things that end with 0 or 5
 - no primes end in an even number
 - If 5 is a factor, no need to test any multiples of 5 etc

You're using a lot of Collections - https://stackoverflow.com/questions/10579457/why-use-arrays-in-vba-when-there-are-collections you might want to try arrays or dictionaries.

In GetFactors what is Count doing? It's set to 0 then.. never used.

This is one heck of an arrow

    If FactorNums.Count = 1 Or FactorNums.Count = 2 Then
        tmpFactors.Add FactorNums(FactorNums.Count)
    Else
        For x = FactorNums.Count - 1 To 1 Step -1
            If FactorNums(x) ^ 2 = NumToFactor Then
                tmpFactors.Add FactorNums(x)
                tmpFactors.Add FactorNums(x)
                Exit For
            Else
                For y = x To 1 Step -1
                    FirstFactor = FactorNums(y)
                    For z = y - 1 To 1 Step -1
                        If FirstFactor * FactorNums(z) = NumToFactor Then
                            tmpFactors.Add FirstFactor
                            tmpFactors.Add FactorNums(z)
                            Exit For
                        End If
                    Next z
                    If tmpFactors.Count = 2 Then Exit For
                Next y
            End If
            If tmpFactors.Count = 2 Then Exit For
        Next x
    End If

And a lot of Exit For - there's four of those against 3 For Next loops - hard to follow which exits what loop.


Subquery performance too slow I'm having performance issues with this query. If I remove the status column, it runs very fast; but adding the subquery in the column section delays the query way too much  (1.02 min execution). How can I modify this query so it runs faster while still getting the desired data?

The reason I put that subquery there is because I needed the status for the latest activity; some activities have null status so I have to ignore them.

Establishments: 6.5k rows -
EstablishmentActivities: 70k rows -
Status: 2 (Active, Inactive)

SELECT DISTINCT
    est.id,
    est.trackingNumber,
    est.NAME AS 'establishment',
    actTypes.NAME AS 'activity',
    (
        SELECT stat3.NAME
        FROM SACPAN_EstablishmentActivities eact3
        INNER JOIN SACPAN_ActivityTypes at3
            ON eact3.activityType_FK = at3.code
        INNER JOIN SACPAN_Status stat3
            ON stat3.id = at3.status_FK
        WHERE eact3.establishment_FK = est.id
          AND eact3.rowCreatedDT = (
            SELECT MAX(est4.rowCreatedDT)
            FROM SACPAN_EstablishmentActivities est4
            INNER JOIN SACPAN_ActivityTypes at4
                ON est4.establishment_fk = est.id
                AND est4.activityType_FK = at4.code
            WHERE est4.establishment_fk = est.id
              AND at4.status_FK IS NOT NULL
          )
          AND at3.status_FK IS NOT NULL
    ) AS 'status',
    est.authorizationNumber,
    reg.NAME AS 'region',
    mun.NAME AS 'municipality',
    ISNULL(usr.NAME,  '') + ISNULL(+ ' ' + usr.lastName,  '')
        AS 'created',
    ISNULL(usr2.NAME, '') + ISNULL(+ ' ' + usr2.lastName, '')
        AS 'updated',
    est.rowCreatedDT,
    est.rowUpdatedDT,
    CASE WHEN est.rowUpdatedDT >= est.rowCreatedDT
         THEN est.rowUpdatedDT
         ELSE est.rowCreatedDT
    END AS 'LatestCreatedOrModified'
FROM SACPAN_Establishments est
INNER JOIN SACPAN_EstablishmentActivities eact
    ON est.id = eact.establishment_FK
INNER JOIN SACPAN_ActivityTypes actTypes
    ON eact.activityType_FK = actTypes.code
INNER JOIN SACPAN_Regions reg
    ON est.region_FK = reg.code --
INNER JOIN SACPAN_Municipalities mun
    ON est.municipality_FK = mun.code
INNER JOIN SACPAN_ContactEstablishments ce
    ON ce.establishment_FK = est.id
INNER JOIN SACPAN_Contacts con
    ON ce.contact_FK = con.id
--JOIN SACPAN_Status stat ON stat.id = actTypes.status_FK
INNER JOIN SACPAN_Users usr
    ON usr.id = est.rowCreatedBy_FK
LEFT JOIN SACPAN_Users usr2
    ON usr2.id = est.rowUpdatedBy_FK
WHERE (con.ssn = @ssn OR @ssn = '*')
  AND eact.rowCreatedDT = (
    SELECT MAX(eact2.rowCreatedDT)
    FROM SACPAN_EstablishmentActivities eact2
    WHERE eact2.establishment_FK = est.id
)
--AND est.id = 6266
ORDER BY 'LatestCreatedOrModified' DESC

I can't upload the execution plan because of rep points, but there's a 21% on one of the establishment activity tables.

Nitpicking
I find your table aliases to be difficult to follow. Take this with a grain of salt coming from an outsider to your organization. I find eact, eact3, eact4, etc. to be a bit confusing, for example. Granted, if there is an internal, consistent standard, disregard this part.
SELECT DISTINCT
This is a pretty expensive operation, and usually better to use GROUP BY instead. That way you run your uniqueness check on the result set instead of the source data set (which could be a LOT larger) and only on the columns you think/know may have duplicate values. For example:
GROUP BY est.id, est.trackingNumber, actTypes.NAME

Create a stored statement
Given the look of this whole script, this doesn't smell like an ad-hoc query to me. Assuming that's accurate, you would benefit from creating a stored statement, in this case likely a stored procedure. This commented line at the bottom kind of gave me a hint:
-- AND est.id = 6266

For example:
CREATE PROCEDURE FooBar
    @Establishment_ID INT
AS
BEGIN
/* your query here */
AND est.id = @Establishment_ID    -- your commented line
ORDER BY 'LatestCreatedOrModified' DESC
END;
GO

Then, whenever needed:
EXEC FooBar 6266;

This way, the execution plan will be stored after it is called for the first time, speeding up (sometimes dramatically) execution after it is ran once.
Compliments
I have to note that your formatting, capitalization and aliasing are consistent. Kudos for that.
The elephant in the room
To me, the slowness of this query could be largely due to multiple levels of nested subqueries, each with multiple join operations. It appears that you join the same tables over and over, and this could be mitigated to some extent by having your nested where clauses in your outer query, whenever possible.
This:
    FROM SACPAN_EstablishmentActivities eact3 --(1)
    INNER JOIN SACPAN_ActivityTypes at3 -- (2)
        ON eact3.activityType_FK = at3.code
    INNER JOIN SACPAN_Status stat3 -- (3)

That:
        FROM SACPAN_EstablishmentActivities est4 -- (1)
        INNER JOIN SACPAN_ActivityTypes at4 -- (2)

And the other:
INNER JOIN SACPAN_EstablishmentActivities eact -- (1)
    ON est.id = eact.establishment_FK
INNER JOIN SACPAN_ActivityTypes actTypes -- (2)
    ON eact.activityType_FK = actTypes.code
INNER JOIN SACPAN_Regions reg
    ON est.region_FK = reg.code --
INNER JOIN SACPAN_Municipalities mun
    ON est.municipality_FK = mun.code
INNER JOIN SACPAN_ContactEstablishments ce
    ON ce.establishment_FK = est.id
INNER JOIN SACPAN_Contacts con
    ON ce.contact_FK = con.id
--JOIN SACPAN_Status stat -- (3)

Notice I've commented out where you are joining all the same tables. You could more than likely work most these conditions into your where clause and avoid unneeded overhead. It's impossible for me to test it out, since I don't have access to your DB, but try to eliminate subqueries/CTEs as much as you can help it.