Function to generate a UUID I am trying to prevent CSRF for my login page (linked from here), and for this I am going to use a random token generator to create a UUID for the session.

The function that I have so far is as such:

function UUID()
{
    mt_srand((double)microtime()*10000);
    $charid = strtoupper(md5(uniqid(rand(), true)));
    $hyphen = chr(45);
    $uuid =  substr($charid, 0, 8).$hyphen
            .substr($charid, 8, 4).$hyphen
            .substr($charid,12, 4).$hyphen
            .substr($charid,16, 4).$hyphen
            .substr($charid,20,12);
    return $uuid;
}

This function is working perfect and on a loop of 30 runs it gives me this result:

  E1BDBF20-A481-AD53-648F-C89E5242B934
  E2329CE9-B79D-999A-C61F-65C3C3976EBE
  B2FE5771-60DE-9E9B-ADF2-1A78B6BD8BC4
  77384406-B472-8192-E301-50CCA1E0CC10
  7854EF9F-991F-90D9-ACA5-07C01C25F355
  EB4D6BC0-6AB1-9020-0C94-BCB81C540EA9
  0AAABBCC-B53F-AF23-2D18-92B971AFD76C
  298FAE1A-F25A-7BF9-ECA9-3C5E80CCBC82
  0533A8D8-AC1F-3A94-FD51-8A95B69DB9C7
  CC650B31-14B6-8709-9E6C-84AA307E77DA
  52AEE9D4-FAC4-7AEC-DB31-882CA61A6AE6
  EED5154B-7425-ED7C-95A1-2239A79F0FAC
  42094233-09D7-BB08-8404-908889B5AB6C
  1A3C9D28-4FF3-5A4B-1994-AFE189D02C01
  4F5D3BC5-C94A-CFD5-87C1-4E2903811DF0
  6BC0612C-93EE-DE33-299B-44888BCCC670
  D7029641-ED26-C136-C5BB-6A6C932A1354
  AB3CF87D-8FA1-C38F-13AA-BFEEA016122C
  687DC395-CF5C-2797-F825-9122D179D6B4
  7970708B-F266-B308-6DA5-49DB190C9326
  ABF5494D-C8B3-976D-42D6-FE108BFF593E
  F0A3A1C0-3A57-EED3-54FB-60C644C8F329
  7A687548-B0B3-E266-E473-74E9BE6D45BE
  A445E134-A9FD-E554-D6E7-E6D1DE9D1046
  F8FCE8F8-7B2C-92E4-00B3-9F834BCE1E9A
  982E093A-0D93-AEAC-DEC3-EFEFA9401EEE
  353E75FC-80B5-6177-F987-4EAF77BC4F97
  8262F39E-F4C0-B60B-9007-A533BC043600
  3810166A-38AC-1275-2593-922CD25E42C6
  59E6A361-8FEA-FE81-B75E-927FC439159E

But it seems like there is a better way of creating this, although I have tried to use com_create_guid, this did not give quite the result I wanted.

I think mt_srand seeds the rng for mt_rand, not for rand. What you probably want is srand. Although seeding it with the time is not that good of an idea, as time is a value an attacker might guess.

But really, there is no reason to use rand in the first place (the documentation also warns against using it when secure values are needed). Just use openssl_random_pseudo_bytes:

base64_encode(openssl_random_pseudo_bytes(32));

I don't really see a reason to place hyphens in-between the characters. It doesn't really add anything. It just makes the token longer, without adding randomness. I also don't see a reason for strtoupper, it even reduces randomness.

Also note that uniqid is not supposed to be used for any security purposes. If we assume that it is completely broken (which it is probably not, but I haven't looked at the code, and it does have a big warning in the documentation), that would only leave you with 10 random numerical characters (from a not-so-great rng), which doesn't seem that great.

I don't think that there is really a security issue with what you have, but it is definitely not the correct approach. Just use openssl_random_pseudo_bytes instead. It's definitely more secure, and the code is considerably shorter, and can easily be adapted (just change 32 to 64 if you want more security; with your code, you would have to change a lot more).


Retrieving mail attachments given criteria with VBA Scenario: I (together with great help from SO) finished this piece of code which downloads attachments from an e-mail account (outlook) with VBA.

Issue: Given the internal functioning of the code, and the sheer amount of e-mails to check (more than 800k) the code cannot function in available time. Last time I tried to run it, it went on for 8 days and then just stopped.

Question: Does anyone have any ideas on how to improve the efficiency of this code?

Obs: The code goes through a secondary e-mail account specified (first criteria), loops through the emails if they are from a certain sender (second criteria), opens file ad check if there are worksheets names "alpha" and "beta" (very smart idea given to me in SO, to find the third criteria), and if it does, saves the file in a final location.

Code:

Sub Get_Mail_Attachments()

Application.ScreenUpdating = False

Dim olApp As New Outlook.Application
Dim olNameSpace As Object
Dim olMailItem As Outlook.MailItem
Dim olFolder As Object
Dim olFolderName As String
Dim olAtt As Outlook.Attachments
Dim strName As String
Dim sPath As String
Dim i As Long
Dim j As Integer
Dim olSubject As String
Dim olSender As String
Dim sh As Worksheet
Dim LastRow As Integer

Dim TempFolder As String: TempFolder = VBA.Environ$("TEMP")
Dim wB As Excel.Workbook

'delete content except from row 1
ThisWorkbook.Worksheets("FileNames").Rows(2 & ":" & ThisWorkbook.Worksheets("FileNames").Rows.count).Delete

'set foldername and subject
olFolderName = ThisWorkbook.Worksheets("Control").Range("D10")
olSender = ThisWorkbook.Worksheets("Control").Range("D16")

sPath = Application.FileDialog(msoFileDialogFolderPicker).Show
sPathstr = Application.FileDialog(msoFileDialogFolderPicker).SelectedItems(1)

Set olNameSpace = olApp.GetNamespace("MAPI")

'check if folder is subfolder or not and choose olFolder accordingly
Set olFolder = olNameSpace.Folders("email@email.com").Folders("Inbox")

If (olFolder = "") Then
    Set olFolder = olNameSpace.Folders("email@email.com").Folders("Inbox")
End If

'loop through mails
h = 2
For i = 1 To olFolder.Items.count

'check to see if it is an e-mail
If olFolder.Items(i).Class <> olMail Then
Else
Set olMailItem = olFolder.Items(i)

'check if the search name is in the email subject
    If (InStr(1, olMailItem.Sender, olSender, vbTextCompare) <> 0) Then

        With olMailItem

        For j = 1 To .Attachments.count

            strName = .Attachments.Item(j).DisplayName

'check if file already exists
            If Not Dir(sPathstr & "\" & strName) = vbNullString Then
                strName = "(1)" & strName
            Else
            End If

'Save in temp
            .Attachments(j).SaveAsFile TempFolder & "\" & strName
            ThisWorkbook.Worksheets("FileNames").Range("A" & h) = strName

'Open file as read only
            Set wB = Workbooks.Open(TempFolder & "\" & strName, True)
            DoEvents

'Start error handling
            On Error Resume Next
            Set sh = wB.Sheets("alpha")
            Set sh = wB.Sheets("beta")
            If Err.Number <> 0 Then

'Error = At least one sheet is not detected
            Else

'No error = both sheets found
                .Attachments(j).SaveAsFile sPathstr & "\" & strName
            End If
            Err.Clear
            Set sh = Nothing
            wB.Close
            On Error GoTo 0

            h = h + 1

        Next j

    End With

    End If

End If

Next i

Application.ScreenUpdating = True
MsgBox "Download complete!", vbInformation + vbOKOnly, "Done"

End Sub

Some general review suggestions:

Ditch the Hungarian Notation. Most people implement it incorrectly through no fault of their own.
Add Option Explicit to the top of your code module. There are a couple of undefined variables in there.
Use clear and meaningful variables names. What is h? After spending some time pondering the code, it appears to be the row number where you're writing information in your logging workbook, so call it something like logRowNum. That will make it pretty explicit to the next person (or you in 60 8 days.)
You make reference to ThisWorkbook - it appears that you're expecting it to be your logging workbook, so Dim logWorkbook as Workbook and set it before opening any other workbooks. It appears that you don't have any issues with it for now but if you have an 8-day process running, someone could click on another workbook when it's open and ThisWorkbook now points at something else breaking your code.
Indent your code neatly. Rubberduck (I'm a big fan and hang out in their chat room, but haven't yet started contributing) will help you with that. (It'll also help with renaming variables and a ton of other things.) Also, eliminating a lot of those extra blank lines will make it a bit more readable.
Modify your If statements to test for what you're after instead of what you're not after. Having a blank True "statement" is very awkward to follow.

    'change:
        If olFolder.Items(i).Class <> olMail Then
        Else
          'some code here
        End If
    'to:
        If olFolder.Items(i).Class = olMail Then
          'some code here
        End If

Eliminate empty Else clauses - they make me think you're going to write more code but haven't yet.

Specific to improving speed:

Work with arrays instead of the spreadsheet itself.
Turn off other screen updating things in Excel.
Instead of downloading all attachments, make sure you're only downloading Excel workbooks, no need to download a PDF if you're not going to do anything with it:

    strName = .Attachments.Item(j).DisplayName
    If Instr(0, strName, "xls") > 0 then
      'rest of the code here
    End If
    'loops to next email message

Short-cut your worksheet check, as well as ensuring they both exist:

    Dim AlphaFound as Boolean
    Dim BetaFound as Boolean
    On Error Resume Next
    Set sh = wB.Sheets("alpha")
    If Err.Number = 0 Then    'alpha exists
      AlphaFound = True
    End If
    If not AlphaFound Then
      'only look for "is alpha" if "alpha" isn't there
      Set sh = wB.Sheets("is alpha")
      If Err.Number = 0 Then    'alpha exists
        AlphaFound = True
      End If
    End If
    If AlphaFound Then
      Set sh = wB.Sheets("beta")
      If Err.Number = 0 Then  'beta exists
        BetaFound = True
      End If
    End If
    'turn off error ignoring as soon as possible vvv
    On error goto 0
    If AlphaFound and BetaFound then
      .Attachments(j).SaveAsFile sPathstr & "\" & strName
    End If

Ditch the Set wb = nothing. You're doing it 800,000 times and each time you do, Excel has to free the memory then reallocate it when you hit the next Set wb = something statement. Leave the object in memory until you're done processing. Actually, VBA will Set all objects to Nothing automatically at the end of the Sub for you, so there's no reason to do it for any of them. (Note that wb is the only object you're manually freeing.)
Put the trailing \ on all paths as early as possible to minimize concatenation later. (Don't know how much execution time it will save, but it will save typing time.)

    sPath = Application.FileDialog(msoFileDialogFolderPicker).Show
    If Right(sPath, 1) <> "\" then
      sPath = sPath & "\"
    End If

You have a DoEvents in there, which is being a good Windows citizen, but you may have it a bit too frequently. Maybe move it to outside your attachment count loop so you only do it once per email. I hate to suggest another counter in there, but maybe one that counts processed emails - every time you process 10 emails with attachments, DoEvents and reset the counter. That way other things can still run on the machine, but you're not pausing your process too frequently. If this is running on a dedicated machine (for 8 days!) maybe make it every 100 messages... If the machine is dedicated to this process, you might get away with not having it in there at all.


Design for container that holds multiple instances of same base class I have four different classes. First being a base class for the second and third. The fourth being a container class that holds instances of second and third (stored as first).

My base class for PC and Companion

public class Controllable
{
    protected String name;
    protected int x;

    public Controllable (String name) {
        this.name = name;
    }

    public void move () {
        x++;
    }
}

public class PC extends Controllable
{
    private int someUniqueVariable;

    public PC (String name, int someUniqueVariable) {
        super (name);
        this.someUniqueVariable = someUniqueVariable;
    }
}

public class Companion extends Controllable
{
    public Companion (String name) {
        super (name);
    }
}

And finally the wrapper/container class.

public class Party
{
    private PC pc;
    private List<Controllable> members;

    public Party () {
        PC = new PC ("PC", 1); // edit #1: should be pc, bad copy-paste
        members = new ArrayList<Controllable>();
        members.add (PC);
        members.add (new Companion ("Companion1");
        members.add (new Companion ("Companion2");
    }

    public moveAll () {
        for (Controllable m : members) {
            m.move();
        }
    }

    public getPC () {
        return pc;
    }
}

Now if I want to move all Controllable I will iterate over members. If there are pc  specifics I will simply call the PC and use its move function. PC will be more separated from Companion later on.

I would mostly like feedback on the design of my code.

Edit #1:

The most critical area were I want feedback is this. Most things are shared between Companion and PC, however there are some unique features of PC. In 99% I want to be able to iterate over all of the party members In more rare cases I want to only access the Companions and sometimes I only want to access the PC.

public Party () {
    pc = new PC ("PC", 1);
    members = new ArrayList<>();
    members.add (PC);
    members.add (new Companion ("Companion1");
    members.add (new Companion ("Companion2");
}

Minor edit to show an error in the code.

Controllable class

can be abstract, it doesn't seem you will instantiate it
int x field shall be private, you don't use it anywhere

Party class

you declared variable of type PC with lower letters so object assignment shall be too pc = new PC ("PC", 1);
members = new ArrayList<Controllable>(); with Java 8 it is not necessary to declare list's type if you do this with variable declaration, so members = new ArrayList<>(); is enough

It is a good practice to make method arguments final, as well class fields if they are provided through constructor. It means their declared type cannot be changed.


A bag of numbers in C++ for constant time statistics queries (See the next iteration.)

I have this simple "data structure" for storing numbers and querying the statistics on them (average, variance and standard deviation):

numberbag.hpp

#ifndef CODERODDE_STAT_NUMBER_BAG
#define CODERODDE_STAT_NUMBER_BAG

#include <cmath>
#include <cstdlib>
#include <iostream>

namespace coderodde {

    namespace stat {

template<typename Num = long double>
class number_bag {

    size_t m_size;
    Num    m_sum;
    Num    m_square_sum;

public:
    number_bag() : m_size{0}, m_sum{0}, m_square_sum{0} {}

    void add(Num num) {
        m_size++;
        m_sum += num;
        m_square_sum += num * num;
    }

    void remove(Num num) {
        m_size--;
        m_sum -= num;
        m_square_sum -= num * num;
    }

    void clear() {
        m_size = 0;
        m_sum = 0;
        m_square_sum = 0;
    }

    size_t size() const {
        return m_size;
    }

    Num ave() const {
        return m_sum / m_size;
    }

    Num var() const {
        Num step1 = m_square_sum - m_sum * m_sum / m_size;
        return step1 / (m_size - 1);
    }

    Num std() const {
        return sqrt(var());
    }
};

template<typename Num>
std::ostream& operator<<(std::ostream& out, number_bag<Num> const &bag)
{
    out << "[size=" << bag.size() << ", ave=" << bag.ave() << ", var="
        << bag.var() << ", std=" << bag.std() << "]";
    return out;
}

    } // end of namespace coderodde::stat

} // end of namespace coderodde

#endif // CODERODDE_STAT_NUMBER_BAG

main.cpp

#include "numberbag.hpp"
#include <iostream>

int main(int argc, const char * argv[]) {
    using coderodde::stat::number_bag;
    using namespace std;

    number_bag<> bag;
    bag.add(1.0);
    bag.add(1.0);
    bag.add(3.0);

    cout << bag << "\n";

    bag.remove(1.0);

    cout << bag << "\n";

    bag.clear();

    cout << bag << "\n";
    return 0;
}

Demo output

[size=3, ave=1.66667, var=1.33333, std=1.1547]
[size=2, ave=2, var=2, std=1.41421]
[size=0, ave=nan, var=nan, std=nan]

Critique request

Please tell me anything that comes to mind. Especially, I am not sure that putting the output override (operator<<) in the namespace is OK.

Since the code is not doing a rocket science, I'll only mention subtle things that slipped through.

Non-associative math:

Floating point math is very dangerous to deal with. This is especially true when optimizations are all high (I actually had to observe it myself). So, the expression:

m_sum * m_sum / m_size;

has chances to produce different results on different optimization levels. It is better to put parentheses:

(m_sum * m_sum) / m_size;

Calling non explicit constructor:

    m_size = 0;
    m_sum = 0;
    m_square_sum = 0;

Those statements might not compile if the constructor of Num is explicit. I think sometimes it is reasonable to make it explicit. Instead, directly call the constructor with the 0 in it:

    m_size = Num(0);
    //ditto

Non-portable code:

return sqrt(var());

Unqualified call assumes that sqrt() is present in the current namespace. It is not always so, since cmath guarantees only functions being available in std namespace.

Unnecessary include:

<cstdlib> looks useless here. <cstddef> should be used instead for std::size_t.

cosmetics:

Num is too generic. FloatingPoint would do better, though the class can use fixed point as well. If I would know word for combination of both, I would put it instead of Num. It will tell more about the type.

Indentations inside of namespaces is weird for me.

Design:

May be it would be possible to write something like in a python:

bag - num(12);

so it would remove the number from bag. The current design seems slightly verbose.

ADL:

As mentioned in the comment, it is good to put the operator inside of the namespace in which the class appears, and in namespaces that inlined into namespace that declares the class. It will trigger argument dependent lookup, the reason why using std::swap; swap(a, b); works.


Identifying an object pointer by generating and using a unique ID I have an image class and a table class. To each image a single table can be "attached". Each <image, table> pair should be identified with an ID, which can later be used to get the pointer of image or table associated with that ID. Images in this "map" should be unique. Below is my solution, please let me know if it can be improved. Thanks.

browserInfo.h

#include <vector>
#include <tuple>

class BrowserInfo
{
public:
    // Returns a unique ID for the <image, table> pair
    /*
        @param  img The image file pointer
        @param  tbl     The table view pointer
        @return         A unique ID for the input pair
    */
    unsigned getId(image *img, table *tbl) const;

    /// Returns a image pointer for the given ID
    /*
        @param  uId The ID of <image, table> pair
        @return     A pointer to image file if ID exists, otherwise nullptr
    */
    image *getImage(unsigned uId) const;

    /// Returns a table pointer for the given ID
    /*
        @param  uId The ID of <image, table> pair
        @return     A pointer to table if ID exists, otherwise nullptr
    */
    table *getTable(unsigned uId) const;

private:
    // alias for the type
    using imageTableToId = std::tuple<image *, table *, unsigned>;

    // This vector keeps track of all unique <image, table> pair IDs
    mutable std::vector<imageTableToId> m_vecImageTableIds;

    // The current ID
    mutable unsigned m_iCurrentId = 0;

}; // class BrowserInfo

browserInfo.cpp

unsigned BrowserInfo::getId(image *img, table *tbl) const
{
    // first, try to see if we have worked with the provided image before
    for (auto &tuple : m_vecImageTableIds)
    {
        if (std::get<0>(tuple) == img)
        {
            // we support a single table view for each image.
            // therefore, if we find that the image is already stored
            // in our vector, we just need to update the corresponding
            // table pointer and return a new unique ID for this pair
            std::get<1>(tuple) = tbl;
            std::get<2>(tuple) = ++m_iCurrentId;

            return m_iCurrentId;
        }
    }

    // if we got here it means the image pointer wasn't stored before
    // so we can just insert a new tuple into the vector
    m_vecImageTableIds.push_back(std::make_tuple(img, tbl, ++m_iCurrentId));
    return m_iCurrentId;
}

image *BrowserInfo::getImage(const unsigned uId) const
{
    for (const auto &tuple : m_vecImageTableIds)
    {
        if (std::get<2>(tuple) == uId)
            return std::get<0>(tuple);
    }

    return nullptr;
}

table *BrowserInfo::getTable(const unsigned uId) const
{
    for (const auto &tuple : m_vecImageTableIds)
    {
        if (std::get<2>(tuple) == uId)
            return std::get<1>(tuple);
    }

    return nullptr;
}

While your solution works, it can be made even more understandable.

Instead of using a vector of tuples, consider using an std::map:

std::map<unsigned, std::tuple<image *, table *>> imageTablePairs;

This is the most natural expression of the problem statement: mapping an ID to an image-table pair. It will also simplify the logic of your getId function. Here's an algorithm for what you need to do:

  Cycle through the map and check if the image exists. If it does, delete its record from the map (using std::map::erase).

Then, simply do:

imageTablePairs[++m_iCurrentId] = std::make_tuple(img, tbl);

This covers both the case when the image exists (in which case its old record gets deleted per the algorithm above) and the case when the image does not exist (in which case we simply make a new record).

For the other two getter functions, we obviously can't assume that the ID being passed in exists in the map, so we can create a private helper function that takes an ID and returns true if it exists and false otherwise. Then, the logic becomes:

getImage: if the ID exists, return std::get<0>(imageTablePairs[uId]).
getTable: if the ID exists, return std::get<1>(imageTablePairs[uId]).

If I misunderstood the problem statement and this solution is not possible, please let me know.

Edit: Here's the code I'd use. Tested in Visual Studio 2017 and confirmed that it compiles and runs as expected (I used empty image and table structs for testing).

browserInfo.h

#pragma once
#include <map>
#include <tuple>

class BrowserInfo
{
public:
    // Returns a unique ID for the <image, table> pair
    /*
        @param  img The image file pointer
        @param  tbl     The table view pointer
        @return         A unique ID for the input pair
    */
    unsigned getId(image *img, table *tbl) const;

    /// Returns a image pointer for the given ID
    /*
        @param  uId The ID of <image, table> pair
        @return     A pointer to image file if ID exists, otherwise nullptr
    */
    image *getImage(unsigned uId) const;

    /// Returns a table pointer for the given ID
    /*
        @param  uId The ID of <image, table> pair
        @return     A pointer to table if ID exists, otherwise nullptr
    */
    table *getTable(unsigned uId) const;

private:

    bool idExists(unsigned uId) const;

    mutable std::map<unsigned, std::tuple<image *, table *>> imageTablePairs;

    // The current ID
    mutable unsigned m_iCurrentId = 0;

}; // class BrowserInfo

browserInfo.cpp

#include "browserInfo.h"

unsigned BrowserInfo::getId(image * img, table * tbl) const
{
    for (auto &record : imageTablePairs)
    {
        if (std::get<0>(record.second) == img)
        {
            imageTablePairs.erase(record.first);
            break;
        }
    }

    imageTablePairs[++m_iCurrentId] = std::make_tuple(img, tbl);
    return m_iCurrentId;
}

image * BrowserInfo::getImage(unsigned uId) const
{
    if (idExists(uId))
    {
        return std::get<0>(imageTablePairs[uId]);
    }

    return nullptr;
}

table * BrowserInfo::getTable(unsigned uId) const
{
    if (idExists(uId))
    {
        return std::get<1>(imageTablePairs[uId]);
    }

    return nullptr;
}

bool BrowserInfo::idExists(unsigned uId) const
{
    std::map<unsigned, std::tuple<image*, table*>>::iterator it = imageTablePairs.find(uId);
    return it != imageTablePairs.end();
}


Degrees of Wikipedia Here is my attempt on implementing "Degrees of Wikipedia" (much faster version can be seen at degreesofwikipedia.com.It's basically an algorithm which tries to connect two unrelated Wikipedia pages by traversing the internal wikilinks they contain. This version in order to produce faster result doesn't look for the exact match, but just for the matching substring in the url, but that can be changed by editing one line, so it's not really an issue.

import re
from collections import deque

import requests
from bs4 import BeautifulSoup

class UrlNode:
    def __init__(self, url, prev):
        self.url = url
        self.prev = prev
        self.transform_url()

    def transform_url(self):
        if self.url.startswith("/wiki"):
            self.url = "https://en.wikipedia.org" + self.url

    def get_subject(self):
        return self.url.rsplit("/", 1)[1].replace("_", " ")

def is_wiki_url(url):
    url_pattern = r"^/wiki/[a-zA-Z0-9_\,.'-]+$"
    return re.match(url_pattern, url, re.I)

def print_path(result):
    path = []
    distance = 0
    while result is not None:
        path.append(result.get_subject())
        distance += 1
        result = result.prev
    path.reverse()

    print("DISTANCE:", distance)
    print(" -> ".join(path))

def crawl(start, query):
    print(" ==== WIKI CRAWLER === ")
    print("Searching for connnection between '", start, "' and '", query, "'", sep="")
    query = query.replace(" ", "_")
    start = "/wiki/" + start.replace(" ", "_")
    session = requests.session()
    visited_urls = []
    start_url = UrlNode(start, None)
    url_queue = deque()
    url_queue.append(start_url)
    while url_queue:
        url_node = url_queue.popleft()
        if url_node.url in visited_urls:
            continue
        visited_urls.append(url_node.url)
        print(url_node.url)
        req = session.get(url_node.url)
        bs = BeautifulSoup(req.text, 'html.parser')
        for link in bs.find_all('a'):
            if not link.has_attr("href"):
                continue
            if is_wiki_url(link["href"]) and link["href"] not in visited_urls:
                # print("\tFound: ", link["href"])
                new_node = UrlNode(link["href"], url_node)
                if query in new_node.url:
                    print("Total pages visited:", str(len(visited_urls)))
                    return new_node
                url_queue.append(new_node)
    return None

start = "Stack Overflow"
query = "Banana"
result = crawl(start, query)
print_path(result)

This generates result:

Total pages visited: 1054
DISTANCE: 4
Stack Overflow -> Wiki -> University of California -> UC Santa Cruz Banana Slugs

My question is: how can I make this work faster? I welcome both algorithmic suggestions and various "hacks", e.g. requesting the mobile version of the page, which should load faster etc. Here I used breadth-first algorithm, which is very basic and I'm pretty sure better options exist.

I think you can potentially remove the impact of the network altogether by using offline Wikipedia dumps. Wikipedia itself asks to not crawl them:

  Suppose you are building a piece of software that at certain points
  displays information that came from Wikipedia. If you want your
  program to display the information in a different way than can be seen
  in the live version, you'll probably need the wikicode that is used to
  enter it, instead of the finished HTML.

  Also, if you want to get all the data, you'll probably want to
  transfer it in the most efficient way that's possible. The
  wikipedia.org servers need to do quite a bit of work to convert the
  wikicode into HTML. That's time consuming both for you and for the
  wikipedia.org servers, so simply spidering all pages is not the way to
  go.

  Please do not use a web crawler to download large numbers of articles. Aggressive crawling of the server can cause a dramatic slow-down of Wikipedia.

as far as improving HTML parsing, you can replace html.parser with a faster lxml:

bs = BeautifulSoup(req.text, 'lxml')

Note: this requires lxml package to be installed.
make visited_urls a set - this would dramatically improve the lookups
instead of locating all links and then filtering them, you can ask BeautifulSoup to locate only "wiki" links by using a a[href^="/wiki/"] CSS selector - this would also lead to removing the "href" attribute presence check:

for link in bs.select('a[href^="/wiki/"]'):
    if link["href"] not in visited_urls:

there is also this SoupStrainer class that would allow you to parse only a part of the document - see if you can apply it to skip some irrelevant parts of the pages
I am not sure if you actually need a UrlNode class, but if you think you do, __slots__ should improve performance and memory usage:

class UrlNode:
    __slots__ = ['url', 'prev']


Volatility updating with Heston-Nandi model I'm trying to program a volatility updating rule using iteration. I start with the well-known Heston-Nandi model where the returns dynamics are:

$$
\left\{
  \begin{array}{rcl}
    R_{t+1} &=& r + \gamma h_{t+1} + \sqrt{h_{t+1}} z_{t+1}\\
    h_{t+1} &=& \beta_0 + \beta_1 h_t + \beta_2\left(z_t - \beta_3 \sqrt{h_t}\right)^2
  \end{array}
\right.
$$

where

\$z_t\$ is an IID standard normal random variable
\$h_t\$ is time-varying squared volatility
\$\beta_0 > 0,\ \beta_1 \ge 0,\ \beta_2 \ge 0\$, and \$\gamma > -\frac{1}{2}\$.

I want to do is to write the code associate to the volatility updating rule, explained in this algorithm:

Define \$h_0 = c\$ equals the given unconditional variance  which is constant,
Iteration for \$t \in \{1, 2, \cdots, n\}\$:

$$
\left\{
  \begin{array}{rcl}
    h_{t+1} &=& \beta_0 + \beta_1 h_t + \beta_2\left(z_t - \beta_3 \sqrt{h_t}\right)^2 \\
    z_t     &=& \frac{[R_t - r - \gamma h_t]}{\sqrt{h_t}}
  \end{array}
\right.
   $$

to obtain the returns based proxy for spot variances \$(h_t^R)_t\$. Which yields an updating function that exclusively involves observation:

$$ h_{t+1} = \beta_0 + \beta_1 h_t + \frac{\beta_2}{h_t} \left( R_t - r - (\beta_3 + \gamma)h_t \right)^2$$

My program is the following:

library(fGarch)

T=3000
    # For the example I simulate a GARCH
    #process parameters
    eta = 0.2 #eta = 0 is equivalent to Geometric Brownian Motion
    mu = 100 #the mean of the process

    #GARCH volatility model
    specs = garchSpec(model = list(omega = 0.000001, alpha = 0.5, beta = 0.4))
    sigma = garchSim(spec = specs, n = T)

    P_0 = mu #starting price, known
    P = rep(P_0,T)

    for(i in 2:T){
      P[i] = P[i-1] + eta * (mu - P[i-1]) + sigma[i] * P[i-1]
    }

    # Set the parameters :
para<-c(0.1,0.2,0.3,0.4,0.5,0.7) # (beta_0,beta_1, beta_2, beta_3, r, gamma)
    # Iteration to obtain the volatility associate to the model :

vol = c()
vol[1]=sd(P)
for (i in 2:length(P)){
      para_vol <- para[1:6]
      vol[i]=para_vol[1]+ (para_vol[2]*vol[i-1])+ (para_vol[3]/vol[i-1])*(P[i-1]-para_vol[5]-(para_vol[4]+para_vol[6])*vol[i-1])
      }
vol

This is an example where I simulate a GARCH (as data set). I am trying to extract the volatility associated with the Heston-Nandi model.

I know I'm using a lot of bad things for R, but I could not figure out a better solution. Are there any corrections or suggestions needed to improve this process?

First, in R we use <- for assignment. It has its advantages.
It is prefered that you don't use T and F as variable names as these are
aliases to TRUE and FALSE.
Instead of this P <-  rep(P_0, T) you could have P <- vector(mode = "numeric", length = T).
This is just a suggestion: But there is no reason to not name your parameters:
para <- c(0.1, 0.2, 0.3, 0.4, 0.5, 0.7) # (beta_0,beta_1, beta_2, beta_3, r, gamma)
para <- c(beta_0 = 0.1,  beta_1 = 0.2, beta_2 = 0.3, beta_4 = 0.4, r = 0.5, gamma = 0.7) # (beta_0,beta_1, beta_2, beta_3, r, gamma)

Then, you may access them like para["gamma"], but not para$gamma.
Also, there is no need for para_vol<-para[1:6] in the loop, but I wager this is remnant
from other attempts.
In terms of performance, the pre-allocation I used did a 70 ms to 50 ms. Although,
I'm not confident in those numbers, as I did it fairly unscientifically.
Also, the referenced post shows a more direct way of calculating this.
Collapsed version of the code I ended up with:

library(fGarch)

T <-  3000
# For the example I simulate a GARCH
#process parameters
eta <-  0.2 #eta = 0 is equivalent to Geometric Brownian Motion
mu <-  100 #the mean of the process

#GARCH volatility model
specs <-  garchSpec(model = list(
  omega = 0.000001,
  alpha = 0.5,
  beta = 0.4
))
sigma <-  garchSim(spec = specs, n = T)

# library(ggfortify)
# autoplot(sigma)

P_0 <- mu #starting price, known
# P <-  rep(P_0, T)
P <- vector(mode = "numeric", length = T)
P[1] <- P_0

for (i in 2:T) {
  P[i] <-  P[i - 1] + eta * (mu - P[i - 1]) + sigma[i] * P[i - 1]
}
# DOESN'T WORK
# P[-T] <-  P[-T] + eta * (mu - P[-T]) + sigma[-1] * P[-T]

# Set the parameters :
para <-
  c(0.1, 0.2, 0.3, 0.4, 0.5, 0.7) # (beta_0,beta_1, beta_2, beta_3, r, gamma)
# Iteration to obtain the volatility associate to the model :

vol <- vector("numeric", length = length(P))
vol[1] <-  sd(P)
for (i in 2:length(P)) {
  # para_vol <- para[1:6]
  vol[i] <-
    para_vol[1] + (para_vol[2] * vol[i - 1]) + (para_vol[3] /
                                                  vol[i - 1]) * (P[i - 1] - para_vol[5] - (para_vol[4] + para_vol[6]) * vol[i - 1])
}
vol

# autoplot(vol)


Continually process results of multi-thread worker Let's say we've got some SQL queries and want to display the results. Processing takes some time, so I do it multi-threaded.
The following works quite well for me. I reduced the code to the important parts and would be glad to get some comments on it:

Queries and results are stored in thread safe collections.

ConcurrentLinkedQueue<String> sqlQueries;
ConcurrentLinkedQueue<ResultSet> sqlResults;

In the GUI thread I start the worker threads which continuously get new queries from the sqlQueries, execute them, and put the results into sqlResults.

ExecutorService executorService = Executors.newFixedThreadPool(4);
for(int i=0; i<4; i++){
    executorService.execute(new QueryThread(sqlResults, sqlQueries));
}
executorService.shutdown(); //threads terminate after they finish

//Result processing
while ((!executorService.isTerminated() || !sqlResults.isEmpty())) {
    Thread.Sleep(100); //sleep for some time
    while(!sqlResults.isEmpty()) {
        //process available results
    }
}

Within the threads I do sth like:

do{
    currentSqlQuery = sqlQueries.poll();
    if(currentSqlQuery != null) {
        sqlResults.add(executeQuery(currentSqlQuery));
    }
} while (currentSqlQuery != null)

I think you are misunderstanding the concept of Executor (and ExecutorService). It already acts as a queue, so you don't need to maintain a queue of your own. Your code actually creates one job per thread, and every job processes multiple questions from the queue. Instead, you should rather have one task per question:

Collection<String> questions = ...;
for (String question : questions) {
    executor.execute(new QueryTask(question));
}

Also note that you're calling your class QuestionThread. But it's not a thread, it's a task! Please always differ between task and thread: A task is a piece of work that should be done, while threads are a mechanism to execute tasks in parallel.

Your QueryTask should only execute one single query and return it as a result:

class QueryTask implements Callable<String> {
    private final String question;

    public QueryTask(String question) { this.question = question; }

    public String call() {
        // maybe long running
        // get and return answer!
    }
}

The processing part is flawed too, because you're doing a busy-wait there! Instead, you can solve your problem very elegant using a CompletionService. The full algorithm will then be as follows:

public void query(ExecutorService executor, Collection<String> questions, Consumer<String> answerConsumer) throws InterruptedException, ExecutionException {
    CompletionService ecs = new ExecutorCompletionService(executor);
    // execute all tasks
    for (String question : questions) {
        ecs.execute(new QueryTask(question));
    }

    // wait for tasks to complete and hand them over to consumer
    final int size = questions.size();
    for (int i = 0; i < size; i++) {
        answerConsumer.accept(ecs.take().get());
    }
}

(The interface Consumer exists since Java 8; it can easily be replaced by something similar).

Appendix: Calling query from UI thread

Note that the calling thread is blocked until all questions and answers are processed. In case this is the UI thread, it will not be able to run UI events in the meantime. As a result, your UI will "freeze" (it will not react to any user input nor doing repaints of dirty UI regions).

To solve this problem, the call to query must be asnychronous and the consumer must put the result into a UI updating task and hand it over to the UI thread (how this exactly works depends on the UI framework you are using. SWT: Display.(a)syncExec(...), Swing: SwingUtilites.invokeLater(...), JavaFX: Platform.runLater(...), ...). The following example will assume you're using JavaFX (it also uses Java 8 lambda expressions):

public void queryAsynchWithUIUpdate(Collection<String> questions) {
    Executor e = Executors.newSingleThreadExecutor();
    e.execute(() -> {
        ExecutorService executor = Executors.newWorkStealingPool();
        query(executor, questions, answer -> {
            Platform.runLater(() -> {
                // update UI using answer, e.g.:
                answerTextField.setText(answer);
            });
        });
        executor.shutdown();
    });
    e.shutdown();
}


Arduino project to indicate soil moisture using LEDs I've recently started going through the Head First C book. I'd like to get up to speed in terms of writing good, idiomatic, modern C. I have read the first few chapters of the Modern C book too but found Head First to be a better introduction to the language.

The code below is for the first, bigger, arduino assignment. It's a pretty straight forward assignment, an LED should be used to indicate the soil moisture of a plant. I've gone beyond the base assignment by using a 3-color LED  where pin 11 should be high for green, 12 high for red and both high for orange.

This creates a bit of a challenge in creating a nice, consistent api which abstracts those details. I wanted to use pointers and pointer arithmentic in the solution just to get some exposure to them. I could probably re-write using arrays and avoid the need for 0 last elements although I would still need to deal with pointer decay.

Any pointers :-) on what could be changed to turn this into a good, idiomatic, modern solution? Regardless of pointer use or not.

int const RED[] = { 12, 0 };
int const GREEN[] = { 11, 0 };
int const ORANGE[] = { 11, 12, 0 };
int const DEFAULT_DURATION = 1000;
int const DEFAULT_COUNT = 1;

void blink(int color[]);
void blink(int color[], int duration, int count);
void blinkSequence(int* color[]);
void blinkSequence(int* color[], int duration, int count);
void on(int color[]);
void off(int color[]);

void setup() {
  // initialize LED Pins as output
  Serial.begin(9600);
  pinMode(11, OUTPUT);
  pinMode(12, OUTPUT);
  pinMode(13, OUTPUT);
  digitalWrite(11, LOW);
  digitalWrite(12, LOW);
  digitalWrite(13, LOW);
}

void loop() {
  int m = analogRead(0);
  int interval = 2000;
  Serial.print("Moisture is: ");
  Serial.println(m);
  if (m <= 580) {
    //Really Dry!: 580-
    int* seq[] = { RED, ORANGE, 0 };
    blinkSequence(seq, 150, 2);
    interval = 0;
  } else if (m <= 700) {
    //Needs water: 700 - 581
    blink(RED, 500);
    interval = 500;
  } else if (m <= 740) {
    //Could be wetter: 740 - 701
    blink(ORANGE, 4000);
    interval = 50;
  } else if (m <= 870) {
    //Happy range: 870 - 741
    blink(GREEN, 5000);
    interval = 50;
  } else {
    //Overwatered: 870+
    int* seq[] = { GREEN, ORANGE, 0 };
    blinkSequence(seq, 200, 5);
    interval = 3000;
  }

  delay(interval);
}

void blinkSequence(int* colors[], int duration, int count) {
  for (int i = count; i > 0; i--) {
    int** inner = colors;
    for(;*inner;inner++) {
      blink(*inner, duration);
    }
  }
}

void blink(int color[], int duration) {
  on(color);
  delay(duration);
  off(color);
}

void blinkSequence(int* colors[]) {
  for(;*colors;colors++) {
    blink(*colors);
  }
}

void blink(int color[]) {
  on(color);
  delay(DEFAULT_DURATION);
  off(color);
}

void on(int color[]) {
  for(;*color;color++) {
    digitalWrite(*color, HIGH);
    //Serial.print("HIGH:");
    //Serial.println(*color);
  }
}

void off(int color[]) {
  for(;*color;color++) {
    digitalWrite(*color, LOW);
    //Serial.print("LOW:");
    //Serial.println(*color);
  }
}

You wrote several for loops without any spaces.
Please don't do that, as it makes the code harder to read.

Instead of

  for(;*colors;colors++) {
    blink(*colors);
  }

you might prefer

  while (*colors) {
    blink(*colors++);
  }

Moving loop() to the bottom of the file would save you
some forward declarations.

Your Serial.print() statements might benefit from the addition
of a debug flag, so there's no need to comment them.

Things like counts and durations won't go negative.
I can't see the signatures for delay() and digitalWrite(),
but it seems like some of your ints might want to be uint.


MVC Async Action Invoking Workflow I've just started working with Workflow (WF4) and have been playing with an idea of using it in MVC3.0 controller actions to see if it improves the maintainability of complex actions; also potentially giving the ability to make multiple DB calls in parallel to populate the output model.   Along with this I've been looking at Async controller actions but I'm not sure I've really got my head around those.  I know you can load workflow into a WorkflowApplication and call BeginRun(...) to get that to run asynchronously but I'm thinking that's the wrong approach; from looking online I've come up with the following implementation which does run as expected but I'm wondering if there's anything wrong with the code below or it will cause other issues that I've not thought about.

    [HttpPost]
    public void ConfigureAsync(ConfigureExportInputModel inputModel)
    {
        if (inputModel == null)
        {
            throw new ArgumentNullException("inputModel");
        }

        var arguments = new Dictionary<string, object> { { "ExportType", inputModel.ExportType } };
        var wf = new ErrorHandledExportWorkflow();

        AsyncManager.OutstandingOperations.Increment();
        ThreadPool.QueueUserWorkItem(o =>
                {
                    AsyncManager.Parameters["result"] = WorkflowInvoker.Invoke(wf, arguments)["MvcOutput"] as IDefineMvcOutput;
                    AsyncManager.OutstandingOperations.Decrement();
                });
    }

    public ActionResult ConfigureCompleted(IDefineMvcOutput result)
    {
        return this.ActionResultFactory.Create(this, result);
    }

I cannot believe this went unanswered for 5 and a half years (I guess I can, it is a difficult question to answer) - I'm going to try to answer it from the respect of early 2012, and the respect of today (mid 2017).

2012

The biggest concern I see comes from this line:

AsyncManager.Parameters["result"] = WorkflowInvoker.Invoke(wf, arguments)["MvcOutput"] as IDefineMvcOutput;

I don't know how WorkflowInvoker works, but I assume that it has the capability of throwing an exception, which means that your AsyncManager.OutstandingOperations.Decrement(); line would not be reached, and I wonder what other issues that might cause for your application. (Looking for operations that aren't there, for example.) I would consider a try/finally block, or decrement before your operation gets invoked. (You can probably consider "In Progress" as a non-outstanding operation.)

The other issue I see with your Lambda method is regarding the idea of "closures", and captured variables. In .NET the wf and arguments variables are still part of the local method and are simply referring to the local copy, which means if you modify either variable after queuing up the worker, you could end up with a different result. This may not be an issue with your infrastructure, but I feel it's worth pointing out none-the-less.

2017

Let's fast-forward five years and six months (which is an agonizingly long time) and examine what language features might make this a different process. I'm just going to post the form that takes advantage of the new language features, then we'll discuss where async/await might help you.

[HttpPost]
public void ConfigureAsync(ConfigureExportInputModel inputModel)
{
    if (inputModel == null)
    {
        throw new ArgumentNullException("inputModel");
    }

    var arguments = new Dictionary<string, object> { [nameof(inputModel.ExportType)] = inputModel.ExportType };
    var wf = new ErrorHandledExportWorkflow();

    AsyncManager.OutstandingOperations.Increment();
    ThreadPool.QueueUserWorkItem(o =>
            {
                AsyncManager.Parameters["result"] = WorkflowInvoker.Invoke(wf, arguments)["MvcOutput"] as IDefineMvcOutput;
                AsyncManager.OutstandingOperations.Decrement();
            });
}

public ActionResult ConfigureCompleted(IDefineMvcOutput result) =>
    this.ActionResultFactory.Create(this, result);

So it doesn't look much different, but you can see that it's just a bit shorter. The expression-bodied method syntax, the dictionary initializer, and the nameof operator. But what really makes a difference is the async/await of the TPL. This starts making our code come alive.

With the proper implementation, you can leverage async/await to really bring a good flow to the asynchronous nature of your code. If properly implemented on the WorkflowInvoker.Invoke method, you might get away with something like:

[HttpPost]
public Task<ActionResult> RunAsync(ConfigureExportInputModel inputModel)
{
    if (inputModel == null)
    {
        throw new ArgumentNullException("inputModel");
    }

    var arguments = new Dictionary<string, object> { [nameof(inputModel.ExportType)] = inputModel.ExportType };
    var wf = new ErrorHandledExportWorkflow();

    var result = await WorkflowInvoker.InvokeAsync(wf, arguments);
    return this.ActionResultFactory.Create(this, result["MvcOutput"] as IDefineMvcOutput);
}

This would allow you to await the whole method, and return the thread to the pool until done. By leveraging what .NET built-in with C#5.0, you can write succinct asynchronous code that doesn't have to worry about the overhead of performing the async operations. The framework and language really take that burden away from you.

Of course, this expects an InvokeAsync method on the WorkflowInvoker, which may or may not exist, and as such this is pseudo-/hypothetical code, but the idea should remain the same. You can read the MSDN article for more explanation of what is possible with this design.


Determine total number of ways of reaching a destination Question:

  One of Scotland Yard’s most wanted criminal (Mister X) is on the run
  and needs to reach a certain destination safely. There are three modes
  of transport available for him - By air, by train or by cab.  With air
  travel, he can go from station i to station i+3. Using a train would
  make him reach station i+2 when starting from i and by taxi he could
  go to the next station (i+1).

  Now, clearly there can be multiple ways of travelling between two
  stations. Thus, you need to help him find out the total number of ways
  of reaching station N from station 0. Since the number of ways can be
  very large output the answer mod 109 +7.

  Input Specification:

  The first line contains the number of test cases, T. This is followed
  by T lines each containing an integer N, the station ID.

  Output Specification:

  Print T lines, each line having the corresponding test case's output.

  Sample Input:

  2 3 4

  Sample Output:

  4 7

  Constraints:

  1 <= T <= 100000

  2 <= N <= 107

My code to be optimized:

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
public class Solution {
    public static void main(String[] args) throws IOException
    {
        BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
        int testCases=Integer.parseInt(br.readLine());
        short ways[]=new short[testCases],i=0;
        int stationId;
        while(testCases!=0)
        {
            stationId = Integer.parseInt(br.readLine());
            ways[i++] = (short)(getWays(stationId)%(10^9 +7));
            testCases--;
        }
        for(int k:ways)
            System.out.println(k);
    }
    static int getWays(int n)
    {
        int sum = 1,i=1;
        while(i<=n)
        {
            sum+=(i-1);
            i++;
        }
        return sum;
    }
}

Although this is not related to optimization, it looks like you could improve some of the ways you use loops.  If you're using a counter, chances are you should use a for loop:

for (int i = number; i > 0; i--) { /*...*/ }

This works best if the loop counter initialization is pretty short.  But if it's too long and should remain on its own line, then instead have a while loop that updates the counter within the statement:

int i; // some long initialization here instead...

while (i-- > 0) { /*...*/ }

First, I want to mention that you should use whitespace between operators and keywords for readability.  You use it in some places, but you should use it everywhere for consistency.

Second, non-empty code blocks should have the opening curly brace at the end of the line, not on the following line.  This is referenced in Google Java Style.

Instead of decrementing testCases within the loop:

while(testCases!=0)
{
    stationId = Integer.parseInt(br.readLine());
    ways[i++] = (short)(getWays(stationId)%(10^9 +7));
    testCases--;
}

have it done in the while loop statement:

while (testCases-- > 0) {
    stationId = Integer.parseInt(br.readLine());
    ways[i++] = (short)(getWays(stationId)%(10^9 +7));
}

Instead of incrementing i within the loop:

while(i<=n)
{
    sum+=(i-1);
    i++;
}

make it a for loop:

// move the initialization of i into the loop

for (int i = 1; i <= n; i++) {
    sum += (i-1);
}

On another note, this:

short ways[]=new short[testCases],i=0;

should be on separate lines:

short ways[] = new short[testCases];
short i = 0;

Although they're of the same type, i is hard to see since it's crammed right next to ways[].


Payment function with too many parameters I have a method with too many parameters.

How would you refactor it?

Things I considered so far:

pass an array to the object and check for required keys (no code hint, phpdoc, programmer doesn't know which are required)
pass an object (silly method with getters and setters, programmer still doesn't know which are required)

Here's the method:

public function doDirectPayment(
    $amount,
    $credit_card_type,
    $credit_card_number,
    $expiration_month,
    $expiration_year,
    $cvv2,
    $first_name,
    $last_name,
    $address1,
    $address2,
    $city,
    $state,
    $zip,
    $country,
    $currency_code,
    $ip_address,
    $payment_action = 'Sale'
)
{
    $client = $this->getClient();

    $client->setParameterGet('METHOD', 'DoDirectPayment');

    $month = str_pad(ltrim((string)$expiration_month, 0), 2, '0', STR_PAD_LEFT);
    $expiration_date = $month . $expiration_year;

    $client->setParameterGet('PAYMENTACTION', urlencode($payment_action));
    $client->setParameterGet('AMT', urlencode($amount));
    $client->setParameterGet('CREDITCARDTYPE', urlencode($credit_card_type));
    $client->setParameterGet('ACCT', urlencode($credit_card_number));
    $client->setParameterGet('EXPDATE', urlencode($expiration_date));
    $client->setParameterGet('CVV2', urlencode($cvv2));
    $client->setParameterGet('FIRSTNAME', urlencode($first_name));
    $client->setParameterGet('LASTNAME', urlencode($last_name));
    $client->setParameterGet('STREET', urlencode($address1));

    if (!empty($address2)) {
        $client->setParameterGet('STREET2', urlencode($address2));
    }

    $client->setParameterGet('CITY', urlencode($city));
    $client->setParameterGet('STATE', urlencode($state));
    $client->setParameterGet('ZIP', urlencode($zip));
    $client->setParameterGet('COUNTRYCODE', urlencode($country));
    $client->setParameterGet('CURRENCYCODE', urlencode($currency_code));
    $client->setParameterGet('IPADDRESS', urlencode($ip_address));

    $response = $client->request(Zend_Http_Client::GET);

    return $response;
}

I'd group the parameters:

doDirectPayment(CreditCard $card, User $user, array $options = array())

But this doesn't solve all problems.

To mark something required you'd use interface, but the interface can only force the existence of the methods, not the properties.

So the best option seems to be forcing the params in the constructor:

class CreditCard {
     public function __construct($required1, $required2 /*, etc*/);
}

But this way you can't instantiate the object without providing options (no fluent interface).

You may mark the params optional and provide some validation method. The problem is, how to automatically call this method. Maybe some SPL interface may help here.

From Fowler's Refactoring:

  Long Parameter Lists

  Too many parameters. Functionality is wrong, or not enough use of
  fields

  Hinders: comprehension, use, adding parameters

  Main Refactoring: Replace Parameter with Method, Preserve Whole Object, Introduce Parameter Object

Looks like most of those params will come from the user filled form, so you could simply do:

doDirectPayment(PaymentForm $form, array $otherOptions = array()) {
    // ...
    foreach ($form->getValues() as $name => $value) {
        $client->setParameterGet(strtoupper($name), urlencode($value));
    }
    // ...
}


Program to calculate a student's test average I made a very simple program where you input a students grades (edit the code with the set method?) and the program will output the average. Each test has a different weight associated with it.

The problems I think I have right now are:

I am not properly utilizing my methods. The getters and setters are probably done wrong.
Should I make testScore1 its own class since a test can have a test score, test worth % (weight)?
Is it good coding practice to use code separators to separate code for easier viewing?

Main question

Should the declaring of a students name and grades be inside the student class or was I correct in setting them in the main method? (I think that a student's info (name and grades) should be kept inside the Student class and that the main method should be using get methods to access them rather than setting them. Is my thinking correct?)

The basis of what I am trying to achieve is the first comment in my code.

Pastebin link for application class
Pastebin link for Student class

application class

/*
 * --- Student Grade Book Application ---
 * Keep track of students (with a student class that has their name,     average, and scores) in a class and their grades.
 * Assign their scores on tests and assignments to the students and figure     out their average and grade for the class.
 * For added complexity put the students on a bell curve.
 */

// I think this will require multidimensional arrays
public class application {

public static void main(String[] args) {

    Student student1 = new Student();

    student1.setName("Bob"); // Should the name of the student be declared here with a setName?
    student1.setTestScore1(80);
    student1.setTestScore2(60);
    student1.setFinalExamScore(90);

    student1.calculateEntireAverage();
    System.out.println(student1.calculateEntireAverage() + "%");

}
}

Student class

public class Student{
private String name;
private double average;
private double testScore1; // worth 25% (should I make testScore1 it's own     class since a test can have a test score, test worth %)
private double testScore2; // worth 25%
private double finalExamScore; // worth 50%

// Getter for name
public String getName() {
    return name;
}

// Setter for name
public void setName(String name) {
    this.name = name;
}

// ----------------------------------------------------
// Getter for testScore1
public double getTestScore1() {
    return testScore1;
}

// Setter for testScore1
public void setTestScore1(double testScore1) {
    this.testScore1 = testScore1;
}

// Getter for testScore2
public double getTestScore2() {
    return testScore2;
}

// Setter for testScore2
public void setTestScore2(double testScore2) {
    this.testScore2 = testScore2;
}

// ----------------------------------------------------
// Getter for finalExamScore
public double getFinalExamScore() {
    return finalExamScore;
}

// Setter for finalExamScore
public void setFinalExamScore(double finalExamScore) {
    this.finalExamScore = finalExamScore;
}

// ----------------------------------------------------
// Getter for average
public double getAverage() {
    return average;
}

// I think this is essentially a setter for average
public double calculateEntireAverage() {
    average = (testScore1*0.25 + testScore2*0.25 + finalExamScore*0.5);
    // am I just supposed to use a getter / setter for the average instead of making a method like this?
    return average;
}
}

Here are some comments regarding how I would approach this:

Coding Style

Avoid using comments to document things that are evident: For example, "Getter for average" is not needed when the method is a very standard getAverage method, which most people will know what is and what to expect from it. You will find many different positions on this topic, but I try to add comments just for situations where the purpose of the code is not evident, or something is used/done in a not very standard way.
Code separators: I wouldn't recommend using separators like the one you ask about for separating portions of code. IT is not very standard, and the code separation should come from good class and method definitions, and not from artificial elements.

Main Programing Considerations

Requirements and Objective: always start with clear objectives and requirements. In your case, I would say (from your comments) that it is to calculate the average score for your students. This is important because it allows you to focus on what's required, and avoid what's not needed.
Constructor: as you ask in your comments, probably the best way to create a Student is not doing it with a "blank" object, which is what you do when you don't use a constructor to set its initial state. Passing the student name to the constructor would be a better way to initialize this type of object, because there's no point (in this particular case) to create an instance of Student that has no name and belongs to nobody.
Extensibility: the way things are now, adding a new test would require more code than I would like. Adding 10 tests would require a lot of code. I would approach this using a collection of Tests, and would decide which methods I really need to supply the required functionality.
Decomposition: as your comments suggest, each Test can have it's own score and weight. I would create a Test class to keep those attributes (I would also consider adding a Name property to identify each test easily (in case I decide to print or produce the full Test summary for the Student).
Getters and setters: it is ok to have getter and setters for your class properties, but just because you have properties doesn't mean you need to provide setters and/or getters for them. I would choose for which properties I really need to have getters and setters, according to your objectives and requirements. Some properties can be modified and consumed internally (only from within the class), and that's fine. By choosing wisely the public methods each class provides (the class API), you simplify the class API, which IMHO is always desirable.

With this comments in mind, this is the code I produced to solve your problem:

Application.java

public class TestCases {
    public static void main(String[] args) {
        Student student1 = new Student("Bob");
        student1.addTestScore(new Test("test1", 80, 0.25));
        student1.addTestScore(new Test("test2", 60, 0.25));
        student1.addTestScore(new Test("final", 90, 0.5));

        student1.printStudentTestSummary();
        student1.printStudentTestAverage();
    }
}

Student.java

public class Student{

    private String name;
    private ArrayList<Test> tests = new ArrayList<>();

    public Student(String name) {
        this.name = name;
    }

    public void addTestScore(Test test) {
        tests.add(test);
    }

    public void printStudentTestAverage() {
        float average = 0;
        for (Test test: tests) {
            average += test.getWeightedScore();
        }
        System.out.println("Test average for student: " + average);
    }

    public void printStudentTestSummary() {
        System.out.println("Test scores for student: " + name);
        for (Test test: tests) {
            System.out.println(test.toString());
        }
    }
}

Test.java

public class Test {

    private String name;
    private int score;
    private double weight;

    public Test(String name, int score, double weight) {
        this.name = name;
        this.score = score < 0 ? 0 : score; // basic check for valid score
        this.weight = weight < 0 ? 0 : weight; // basic check for valid weight
    }

    public double getWeightedScore() {
        return score * weight;
    }

    public String toString() {
        return "Test: " + name + ". Score: " + score + " (w " + weight * 100 + "%).";
    }
}

Finally, see that you can now add more complex validations to scores and weights, if needed. You can easily extend this code now to support additional use cases also.

Hope this helps!


split items of batch into multiple categories I have a batch job that contains a lot of items. I need to split these items into 4 categories - 1,2 category should be moved to a separate jobs as is; 3,4 categories job be additionally checked on validness and split into separate jobs - invalid items should be moved to a separate jobs.

Roughly diagram of the expected flow looks like this

For now I come up the prototype that looks like this

import java.util.*;
import java.util.stream.*;

public class MyClass {
    public static void main(String args[]) {
        Job job = new Job();
        job.items = Arrays.asList(
            new Item(ItemCategory.CAT_1, "1"),
            new Item(ItemCategory.CAT_2, "2"),
            new Item(ItemCategory.CAT_3, "3"),
            new Item(ItemCategory.CAT_1, "4"),
            new Item(ItemCategory.CAT_2, "5"),
            new Item(ItemCategory.CAT_3, "6"),
            new Item(ItemCategory.CAT_4, "7"),
            new Item(ItemCategory.CAT_4, "8")
        );

        /*
            [1:CAT_1, 4:CAT_1]
            [2:CAT_2, 5:CAT_2]
            [6:CAT_3]
            [8:CAT_4]
            [3:CAT_3, 7:CAT_4]
        */
        List<List<Item>> categorizedItems = job.items.parallelStream().collect(
            () -> Arrays.asList(new ArrayList<>(), new ArrayList<>(), new ArrayList<>(), new ArrayList<>(), new ArrayList<>()), // last item is error aggregator
            (list, item) -> {
                switch (item.category) {
                    case CAT_1: list.get(0).add(item); break;
                    case CAT_2: list.get(1).add(item); break;
                    case CAT_3: {

                        if (isValid(item.id)) {
                            list.get(2).add(item);
                        } else {
                            list.get(4).add(item);
                        }

                        break;
                    }
                    case CAT_4: {

                        if (isValid(item.id)) {
                            list.get(3).add(item);
                        } else {
                            list.get(4).add(item);
                        }

                        break;
                    }
                }
            },
            (list1, list2) -> {
                list1.get(0).addAll(list2.get(0));
                list1.get(1).addAll(list2.get(1));
                list1.get(2).addAll(list2.get(2));
                list1.get(3).addAll(list2.get(3));
                list1.get(4).addAll(list2.get(4));
            }
        );
    }

    public static boolean isValid(String id) {
        return Integer.valueOf(id) % 2 == 0;
    }

    public static class Item {
        ItemCategory category;
        String id;

        Item(ItemCategory category, String id) {
            this.id = id;
            this.category = category;
        }

        @Override
        public String toString() {
            return id + ":" + category;
        }
    }

    public enum ItemCategory {
        CAT_1, CAT_2, CAT_3, CAT_4;
    }

    public static class Job {
        List<Item> items;
    }
}

Is it a good a idea to do everything in a single stream? Or for example it is better split into 4 categories first and then process 3,4 categories separately?

Your Collector is reinventing the wheel.  There already exists a collector which partitions the collected items into groups: Collectors.groupingBy, which you could use something like:

.collect(
    Collectors.groupingBy(
        item -> (item.category == CAT_3 || item.category == CAT_4) && !isValid(item.id)
                     ? CAT_5 : item.category)
    )
)

Of course, this will return a Map<ItemCategory, List<Item>>.  You can transform this back to a list of lists, if needed.

And of course, you would need to add the CAT_5 to the ItemCategory enum.  Alternately, you could use null at the category 5 key, if you don't mind null as a key value, but be warned that it will make some people's skin crawl.

If the order of the items in the all of the groups (as opposed to just the Category 5 group) does not matter, then groupingByConcurrent will give better parallel stream performance.


Async distributed locking with StackExchange.Redis I have been trying to write an analogy to this approach to distributed locking with two differences:

making it asynchronous
making it work with StackExchange.Redis rather than AppFabric Cache.

My biggest concern is going out of my way to use IDisposable to make it compatible with using() but thus calling an async method from within Dispose (in a sort of 'fire and forget' way). The basic tests I have run so far seem to pass ok but it doesn't seem right.

public static class RedisExtensions
{
    public static Task<IDisposable> AcquireLockAsync(this IDatabaseAsync db, string key, TimeSpan? expiry = null, TimeSpan? retryTimeout = null)
    {
        if (db == null)
        {
            throw new ArgumentNullException("db");
        }
        if (key == null)
        {
            throw new ArgumentNullException("key");
        }
        return DataCacheLock.AcquireAsync(db, key, expiry, retryTimeout);

    }

    private class DataCacheLock : IDisposable
    {
        private static StackExchange.Redis.IDatabaseAsync _db;
        public readonly RedisKey Key;
        public readonly RedisValue Value;
        public readonly TimeSpan? Expiry;
        private DataCacheLock(IDatabaseAsync db, string key, TimeSpan? expiry)
        {
            _db = db;
            Key = "ninlock:" + key;
            Value = Guid.NewGuid().ToString();
            Expiry = expiry;
        }
        public static async Task<IDisposable> AcquireAsync(IDatabaseAsync db, string key, TimeSpan? expiry, TimeSpan? retryTimeout)
        {
            DataCacheLock dataCacheLock = new DataCacheLock(db, key, expiry);
            Debug.WriteLine(dataCacheLock.Key.ToString() + ":" + dataCacheLock.Value.ToString());
            Func<Task<bool>> task = async () =>
            {
                try
                {
                    return await _db.LockTakeAsync(dataCacheLock.Key, dataCacheLock.Value, dataCacheLock.Expiry ?? TimeSpan.MaxValue);
                }
                catch
                {
                    return false;
                }
            };

            await RetryUntilTrueAsync(task, retryTimeout);
            return dataCacheLock;
        }
        public void Dispose()
        {
            Debug.WriteLine("release the lock:" + Value);
            _db.LockReleaseAsync(Key, Value);
        }
    }
    private static readonly Random _random = new Random();
    private static async Task<bool> RetryUntilTrueAsync(Func<Task<bool>> task, TimeSpan? retryTimeout)
    {
        int i = 0;
        DateTime utcNow = DateTime.UtcNow;
        while (!retryTimeout.HasValue || DateTime.UtcNow - utcNow < retryTimeout.Value)
        {
            i++;
            if (await task())
            {
                return true;
            }
            var waitFor = _random.Next((int)Math.Pow(i, 2), (int)Math.Pow(i + 1, 2) + 1);
            Debug.WriteLine(waitFor);
            await Task.Delay(waitFor);
        }
        throw new TimeoutException(string.Format("Exceeded timeout of {0}", retryTimeout.Value));
    }
}

As mentioned in my comment I think in general Dispose is expected to be synchronous hence it should block until finished which is easy enough to achieve:

_db.LockReleaseAsync(Key, Value).Wait();

One other gripe I have with the code is the static IDatabaseAsync member of DataCacheLock. Maybe I'm missing something but I think this can get you into trouble. Assume you have two databases you want to talk to and you fire off to calls to AcquireLockAsync - now the member is set to whatever instance where the calls happened to be executed last which will probably cause funny results as LockTakeAsync might now wait on the wrong database instance for one of the calls and/or in Dispose the lock will be released against the wrong instance.

While this might be a rare use case I don't see the benefit of making this a static member in the first place - it's only accessed locally in either the task  trying to acquire the lock or in the Dispose method. For the Dispose case a non-static member would work fine. For the locking task the instance can be captured locally in the closure as it's passed in. So it should be easy enough to remove the problematic hidden gotcha.


Iterative Fibonacci sequence I implemented a function that prints all the numbers from the Fibonacci sequence until max_num. The minimum value allowed is 0 (so, fib(0) prints 1). It works until 92, and I want to know how to improve the code, in general.

void fib(unsigned int max_num)
{
    unsigned long fib_num = 1;
    unsigned long fib_temp = 0;
    size_t count = 0;

    if (max_num < 0)
    {
        fprintf(stderr, "Please, enter a non-negative number\n");
        return;
    }

    for (; count <= max_num; count++)
    {
        printf("%lu\n", fib_num);
        fib_num += fib_temp;
        fib_temp = fib_num - fib_temp;
    }
}

Consider documenting this function with something like doxygen. At a quick glance, your function can be interpreted two different ways: Print out fib(0), fib(1), ..., or print out all Fibonacci numbers less than or equal to max_num. Documentation will clarify that.

max_num will never be negative. It is defined as an unsigned int, which means it can never hold a negative value. So this:

if (max_num < 0)
{
    fprintf(stderr, "Please, enter a non-negative number\n");
    return;
}

will never happen. (Try calling fib(-2), see what happens)

(Optional) Put size_t count = 0 inside the loop if you can? (You may have to add -std=c99 or something like that to make it work)

for (size_t count = 0; count <= max_num; count++)
{
    printf("%lu\n", fib_num);
    fib_num += fib_temp;
    fib_temp = fib_num - fib_temp;
}


x86 assembly method that mimics strcat I'm trying to learn x86 assembly and hopefully move on to projects of the scope that more than one person would be involved.

This is a (relatively) simple subroutine written in Intel x86 assembly using MASM that copies one string onto the end of another. I named it strglue to avoid conflict with the C library function strcat (since the driver for this method is a C program) and the MASM macro CatStr.

I'm wondering if I can have some insight on the quality of:

The code itself (optimality of the instructions and register usage)
The documentation
The indentation of the loops and labels

.386
.MODEL FLAT, C
.DATA
.CODE

    ; Glue together (concatenate) two strings
    ; PARAMS:
    ;  [ESP+4] - Pointer to first character in null-terminated destination string
    ;  [ESP+8] - Pointer to first character in null-terminated source string
    ; USES:
    ;  EAX: Holds the destination string during execution
    ;  ECX: Holds the source string during execution:
    ;  EDX: Lower 8 bits (DL) holds characters being copied from source to destination
    ; OUTPUT:
    ;  [ESP+4] - Pointer to first character in null-terminated concatenation of destination string + source string (excluding the null-terminator from the destination string)
    strglue PROC

        ; Step 1: Get a pointer to the null-terminator character in the destination string
        MOV EAX, DWORD PTR [ESP+4]
        ENDOFDEST:
            INC EAX
            CMP BYTE PTR [EAX], 0
        JNZ ENDOFDEST

        ; Step 2: Copy the source string into the pointer to the end of dest, overwriting the null-terminator
        MOV ECX, DWORD PTR [ESP+8]
        STRCATLOOP:
            MOV DL, BYTE PTR [ECX]
            LEA EAX, [EAX+1]
            MOV BYTE PTR [EAX-1], DL
            LEA ECX, [ECX+1]

            ; If ZF=0  the null terminator in the source string has been reached; stop looping
            TEST DL, DL
        JNZ STRCATLOOP
        RET
    strglue ENDP
END

This strikes me as looking a little like code I'd expect to see from a compiler--code that works, but completely ignores both traditional x86 register usage, and the special instructions the x86 provides for the job.

Traditionally, you'd use edi to point to your destination string, and esi to point to your source string. You'd use a repnz scasb to find the end of the first string, and lodsb/stosb to copy bytes from the source to the destination (there is actually a movsb to copy bytes as well, but it doesn't allow you to check for the end of the string as you copy).

Note that this depends on the direction flag being 0, but every compiler of which I'm aware leaves it that way (and depend on your leaving it that way as well).

    xor eax, eax     ; set al to value we're looking for, 0 in this case.
    lea ecx, [eax-1] ; set ecx to 0ffffffffh

    mov edi, [esp+4] ; find end of dest string
    repnz scasb

    mov esi, [esp+8] ; we've found the end of the destination, now do the copy
    dec edi

copy_loop:
    lodsb
    stosb
    test al, al
    jnz  copy_loop
    ret

This is somewhat shorter. Under normal circumstances, we can expect either one to be memory bound, so speed probably isn't particularly relevant (but if we did care, it would depend on the exact processor in use--this would tend to work better on older processors, but would be about the same to possibly even a little slower on newer processors).

As far are readability goes, either one is fairly easy to read, but I think my code fits more closely with how somebody with experience writing code for an x86 would expect to see the registers used. As far as indentation goes, there doesn't seem to be much agreement about whether to indent assembly language. Your indentation certainly doesn't hurt anything, but it isn't like higher level languages where it's expected to the point that unindented code is clearly a problem. The way I've written the code above (instructions indented one stop, label(s) at the left margin) is almost certainly more common.

I should probably add that I'm a bit hesitant to post on this subject--while I think I wrote assembly language quite well at one time, it's been years since I wrote enough to notice. I do look forward to @Edward posting a review on this (though I'm not sure whether he writes much assembly code any more either).


View for logging telephone calls As part of a web-based claims system I have created, there is a view for logging telephone calls. If a call is incoming, the claims handler asks the caller a series of data protection questions to validate who they are. The questions are generated from a database and each question has one or more valid answers. The valid answers are also generated from the database.

Example view:

Things of note for my question:

In addition to the valid answers, there are also 'Unanswered' and 'Incorrect Response' answers. These are added via client code and are not part of the original valid answers generated by the database.
Each valid answer has a 'check' icon
The invalid answer has a 'cross' icon
The unanswered answer has no icon

Here's my client code for each type of answer:

class Answer {
    constructor(answer, state) {
        this.el = document.createElement('option');
        this.el.value = answer;
        this.el.textContent = answer;

        state.apply(this);
    }
}

class AnswerState {
    constructor(validationIcon, validationColor) {
        this.validationIcon = validationIcon;
        this.validationColor = validationColor;
    }

    apply(answer) {
        answer.el.setAttribute('data-validation-icon', this.validationIcon);
        answer.el.setAttribute('data-validation-color', this.validationColor);
    }
}

class UnansweredState extends AnswerState {
    apply(answer) {
        super.apply(answer);
        answer.el.setAttribute('data-unanswered', '');
    }
}

class ValidAnswerState extends AnswerState {
    apply(answer) {
        super.apply(answer);
        answer.el.setAttribute('data-valid', '');
        answer.el.setAttribute('data-icon', 'icons/round-done-24px.svg');
    }
}

class InvalidAnswerState extends AnswerState {
    apply(answer) {
        super.apply(answer);
        answer.el.setAttribute('data-invalid', '');
        answer.el.setAttribute('data-icon', 'icons/round-clear-24px.svg');
    }
}

export { Answer, UnansweredState, ValidAnswerState, InvalidAnswerState };

And this is how I create the answer select element for each question:

function createAnswerSelect(validAnswers) {
    const select = document.createElement('select');
    const unansweredState = new UnansweredState('remove', 'grey');
    const invalidAnswerState = new InvalidAnswerState('clear', 'red');
    const validAnswerState = new ValidAnswerState('done', 'green');

    select.add(new Answer('Unanswered', unansweredState).el);

    validAnswers.forEach(answer => {
        select.add(new Answer(answer.answer, validAnswerState).el);
    });

    select.add(new Answer('Incorrect response', invalidAnswerState).el);

    return select;
}

Is this a reasonable way to encapsulate the variability of the different type of answers (unanswered, valid, invalid)? It feels like 'unanswered' should be the state of a question, not an answer.

As per usual, I am questioning myself. The code works, but I have a nagging feeling it is a 'bad' design somehow.

Object literals
By the looks this is client side UI only and the actual results are stored in the database.
Your concern is not that of the data structure, but rather one of representation.
The task is defined by what you have in createAnswerSelect(validAnswers) {
Your code is attempting to imitate a higher level abstraction unrelated to the task you need to complete. This has forced you to presented 4 additional interfaces to the scope you are working in  Answer, UnansweredState, ValidAnswerState, InvalidAnswerState that you are unsure of.
Javascript lets you define objects as literals, you can side step the noise related to defining named Objects, Prototypes, functions, calls to super etc, yet still keep the same underlying relationship of shared and unique state.
Example
The best code is that which does it quickly and as simply as possible. Creating an inherited structure just to hold some shared and unique states is too complex.
This example is an alternative (based on the information you have provided)
It defines the 3 states as named constants that access state information via a Map of named states, rather than the 3 objects you defined UnansweredState, ValidAnswerState, InvalidAnswerState
const UNANSWERED = 1;
const INVALID = 2;
const VALID = 3;

const createElement = (type, opts = {}) => Object.assign(document.createElement(type), opts);
const common = (icon, color) => ({icon, color});
const states = new Map([
  [UNANSWERED, {...common("remove", "grey"), data: {unanswered: ""}}],
  [INVALID, {...common("clear", "red"), data: {valid: "", icon: "icons/round-done-24px.svg"}}],
  [VALID, {...common("done", "green"), data: {invalid: "", icon: "icons/round-clear-24px.svg"}}],
]);
const option = (answer, stateId) => {
  const state = states.get(stateId);
  const option = createElement("option", {value: answer, textContent: answer });
  Object.assign(option.dataset, state.data);
  option.dataset.validationIcon = state.icon;
  option.dataset.validationColor = state.color;
  return option;
}

function createAnswerSelect(answers) {
  const select = createElement('select');
  select.add(option("Unanswered", UNANSWERED));
  answers.forEach(answer => select.add(option(answer.answer, VALID)) );
  select.add(option("Incorrect response", INVALID));
  return select;
}
export { createAnswerSelect };


Splitting each word using longest substring that matches in a vocab/dictionary This function takes in a string and a fixed list of vocab:

splits a string into words by spaces
for each word, check in the dictionary/vocab and find the longest matching substrings, if none matching use the [UNK] word
if a word is longer than a predefined length, it'll also become [UNK]

Code:

def tokenize(text, vocab, max_input_chars_per_word=10, unk_token="[UNK]"):
    output_tokens = []
    for token in text.split():
        chars = list(token)
        if len(chars) > max_input_chars_per_word:
            output_tokens.append(unk_token)
            continue

        is_bad = False
        start = 0
        sub_tokens = []

        while start < len(chars):
            end = len(chars)
            cur_substr = None
            while start < end:
                substr = "".join(chars[start:end])
                if start > 0:
                    substr = "##" + substr
                if substr in vocab:
                    cur_substr = substr
                    break
                end -= 1
            if cur_substr is None:
                is_bad = True
                break
            sub_tokens.append(cur_substr)
            start = end

        if is_bad:
            output_tokens.append(unk_token)
        else:
            output_tokens.extend(sub_tokens)
    return output_tokens

And example input and expected output:

vocab = ["the", "go", "##es", "to", "eat", "pum", "##pkins", "of", "##gos", "##stein"
         "#400", "1", "boredom", "##folk", "man", "##go", "out", "folks", "##0",
        "un", "##aff", "##able"]

s = "the unaffable folks goes to eat 1400 folkspkinsgosgo pumpkins and 10 mangos out of boredom"

tokenize(s, vocab)

[out]:

['the',
 'un',
 '##aff',
 '##able',
 'folks',
 'go',
 '##es',
 'to',
 'eat',
 '[UNK]',
 '[UNK]',
 'pum',
 '##pkins',
 '[UNK]',
 '1',
 '##0',
 'man',
 '##gos',
 'out',
 'of',
 'boredom']

The nested while loop looks a complicated and checking each token isn't parallelized since it looks like it's independent of other tokens.  How can this function be improved?

Simpler loops, or checking in parallel or maybe an easier to find longest matching substring from a list when iterating through the tokens? Or regexes?

The following is a basic syntax and usage pass without looking into your algorithm in too much depth. I'll first show the suggested code and then highlight significant differences where it can offer improvements on the original.

#!/usr/bin/env python3

from typing import Iterable, Set

def tokenize(
    text: str,
    vocab: Set[str],
    max_input_chars_per_word=10,
    unk_token="[UNK]"
) -> Iterable[str]:
    for token in text.split():
        n = len(token)
        if n > max_input_chars_per_word:
            yield unk_token
            continue

        start = 0
        sub_tokens = []

        while start < n:
            end = n
            cur_substr = None
            while start < end:
                substr = token[start:end]
                if start > 0:
                    substr = "##" + substr
                if substr in vocab:
                    cur_substr = substr
                    break
                end -= 1
            if cur_substr is None:
                yield unk_token
                break
            sub_tokens.append(cur_substr)
            start = end
        else:
            yield from sub_tokens

def test():
    vocab = {"the", "go", "##es", "to", "eat", "pum", "##pkins", "of", "##gos", "##stein"
             "#400", "1", "boredom", "##folk", "man",
             "##go", "out", "folks", "##0",
             "un", "##aff", "##able"}

    s = "the unaffable folks goes to eat 1400 folkspkinsgosgo pumpkins and 10 mangos out of boredom"

    result = tokenize(s, vocab)
    assert list(result) == [
        'the',
        'un',
        '##aff',
        '##able',
        'folks',
        'go',
        '##es',
        'to',
        'eat',
        '[UNK]',
        '[UNK]',
        'pum',
        '##pkins',
        '[UNK]',
        '1',
        '##0',
        'man',
        '##gos',
        'out',
        'of',
        'boredom']

test()

Rationale

Add PEP484 type hints such as Iterable for the return of your function.
Require that you accept a set for vocab. All you do on it is membership testing, i.e. if substr in vocab:, so a set will be much faster.
Make this function a generator and yield instead of building up an output_tokens list.
Store the token length in a variable such as n, considering you need it so often.
Do not split out token into chars, and do not call join. All you need is the token string.
Do not track an is_bad flag. Use a for/else to detect whether a break occurred.
Use your test data in actual assert tests.


Serial-data read buffer handling This is a follow-up to yesterday's codereview-question about reading serial data and parsing it.

The code below is run into a seperate thread looped endlessly.
Currently I track the position in my inputBufer manually and copy all bytes which haven't been processed into a new byte-array and assigning that to the old inputBuffer.

Can the read buffer handling be improved?

bytesRead = SerialPort.Read(inputBuffer, bufferPos, SerialPort.BytesToRead);
bufferPos += bytesRead;
bytesProcessed = 0;

// process data-packets
for (int i = 0; i < this.bufferPos; i++) {

  // Flex sensor data
  //
  if (this.bufferPos-i >= 14 && inputBuffer[i] == 'F'){
    i++;
    short[] sensorData = new short[6];
    sensorData[0] = (short)((short)inputBuffer[i++] << 8 | inputBuffer[i++]); // thumb
    sensorData[1] = (short)((short)inputBuffer[i++] << 8 | inputBuffer[i++]); // index finger (finger knuckle)
    sensorData[2] = (short)((short)inputBuffer[i++] << 8 | inputBuffer[i++]); // middle finger
    sensorData[3] = (short)((short)inputBuffer[i++] << 8 | inputBuffer[i++]); // ring finger
    sensorData[4] = (short)((short)inputBuffer[i++] << 8 | inputBuffer[i++]); // not used
    sensorData[5] = (short)((short)inputBuffer[i++] << 8 | inputBuffer[i++]); // index finger (hand knucke)
    // checksum
    byte checksum = inputBuffer[i++];
    ParseSerialData(PacketType.Flex, sensorData, checksum);
    bytesProcessed = i;
    //Debug.Log (sensorData[0] + " then " + bytesProcessed);
  }

  // Raw sensor data
  //
  if (this.bufferPos-i >= 20 && inputBuffer[i] == 'S'){
    i++;
    short[] sensorData = new short[9];
    sensorData[0] = (short)((short)inputBuffer[i++] << 8 | inputBuffer[i++]); // gyroscope X
    sensorData[1] = (short)((short)inputBuffer[i++] << 8 | inputBuffer[i++]); // gyroscope Y
    sensorData[2] = (short)((short)inputBuffer[i++] << 8 | inputBuffer[i++]); // gyroscope Z
    sensorData[3] = (short)((short)inputBuffer[i++] << 8 | inputBuffer[i++]); // accelerometer X
    sensorData[4] = (short)((short)inputBuffer[i++] << 8 | inputBuffer[i++]); // accelerometer Y
    sensorData[5] = (short)((short)inputBuffer[i++] << 8 | inputBuffer[i++]); // accelerometer Z
    sensorData[6] = (short)((short)inputBuffer[i++] << 8 | inputBuffer[i++]); // magnetometer X
    sensorData[7] = (short)((short)inputBuffer[i++] << 8 | inputBuffer[i++]); // magnetometer Y
    sensorData[8] = (short)((short)inputBuffer[i++] << 8 | inputBuffer[i++]); // magnetometer Z
    // checksum
    byte checksum = inputBuffer[i++];
    ParseSerialData(PacketType.Raw, sensorData, checksum);
    bytesProcessed = i;
  }

  // Quaternion of device's rotation
  //
  if (this.bufferPos-i >= 20 &&  inputBuffer[i] == 'Q'){
    i++;
    short[] sensorData = new short[4];
    sensorData[0] = (short)((short)inputBuffer[i++] << 8 | inputBuffer[i++]); // W
    sensorData[1] = (short)((short)inputBuffer[i++] << 8 | inputBuffer[i++]); // X
    sensorData[2] = (short)((short)inputBuffer[i++] << 8 | inputBuffer[i++]); // Y
    sensorData[3] = (short)((short)inputBuffer[i++] << 8 | inputBuffer[i++]); // Z
    // checksum
    byte checksum = inputBuffer[i++];
    ParseSerialData(PacketType.Quaternion, sensorData, checksum);
    bytesProcessed = i;
  }
}

// Copy unprocessed rest of stream
int rest = bufferPos - bytesProcessed;
  if (rest > 0){
  byte[] tempBuffer = new byte[4096];
  Array.Copy (inputBuffer, bytesProcessed, tempBuffer, 0, rest);
  inputBuffer = tempBuffer;
  bufferPos = rest;
}
else {
  inputBuffer = new byte[4096];
  bufferPos = 0;
}

Another suggestion, to go on top of Heslacher's:

Make the (short)((short)inputBuffer[... a method.

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static short BytesToShort(byte a, byte b)
{
    return (short)(a << 8 | b);
}

The MethodImplOptions.AggressiveInlining enumeration value tells the C# compiler to try to inline the method as much as possible. That is, if you come form a C/C++ environment, it is very similar to macro's.

The C++ macro:

#define LERP(a,b,c) (((b) - (a)) * (c) + (a))

In C# could be written as:

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static double Lerp(double a, double b, double c)
{
    return (b - a) * c + a;
}

And effectively be the same thing. The compiler would try to inline the method body instead of the method whenever possible.

This should help performance, while allowing you to move some of the more mundane code away from the main area you would be working.

Note: this does not guarantee that this is the choice the compiler would make, nor does it preclude you from doing other optimization strategies. However, for a method this small and simple, it would likely make that choice in the end.

https://msdn.microsoft.com/en-us/library/system.runtime.compilerservices.methodimploptions(v=vs.110).aspx

This means, that you could essentially do:

for (int i = 0; i < sensorData.Length; i++)
{
    sensorData[i] = BytesToShort(inputBuffer[startposition++], inputBuffer[startPosition++]);
}

With no ill-effects.


Java blocking queue download process scenario I have the following scenario .
I have incoming file download requests and each download is happening in different thread until pool size is exceeded. And after  a download completed, a processor processes the downloaded item. So I created the following. I wonder if uses of thread and executors make sense
DownloadTaskProcessor
public class DownloadTaskEnqueuer {
    private static final BlockingQueue<Task> downloadQueue = new LinkedBlockingQueue<>();
    private static final BlockingQueue<Task> processQueue = new LinkedBlockingQueue<>();
    private static final ExecutorService executor = Executors.newCachedThreadPool();

    public void offer(Task task) {
        return downloadQueue.offer(task);
    }

    public void createPool(int size) {
        for (int i = 0; i < size; i++) {
            executor.execute(new DownloadTask(downloadQueue, processQueue);
            executor.execute(new ProcessTask(processQueue));
        }
    }
}

Download task
public class DownloadTask implements Runnable {
    private BlockingQueue<Task> downloadQueue;
    private BlockingQueue<Task> processQueue;

    // constructor for initing two queue

    public void offer(Task task) {
        return processQueue.offer(task);
    }

    @Override
    public void run() {
        while (true) {
           Task task = downloadQueue.poll();
           if (task != null) {
               task.getDownloadTask().download();
               offer(task);
           } else {
               // sleep 250 ms
           }
        }
    }
}

Process task
public class ProcessTask implements Runnable {
    private BlockingQueue<Task> processQueue;

    // constructor for initing queue

    @Override
    public void run() {
        while (true) {
           Task task = processQueue.poll();
           if (task != null) {
                task.getProcessTask().process();
           } else {
               // sleep 250 ms
           }
        }
    }
}

Use case (pseudo)
createPool(10);

listener.listen((task) -> {
    downloadTaskEnqueuer.offer(task);
}

In your DownloadTaskEnqueuer it might be a little bit weird to mix instance methods with static fields (unless there is more code which you omitted). It might be better to either use instance fields (i.e. remove static from downloadQueue, processQueue and executor) or make the methods static.
You DownloadTaskEnqueuer.offer(...) method has void as return type but contains a return statement returning a value.
Executors.newCachedThreadPool() is not that well suited for your use case. You are creating a fixed number of work tasks which run indefinitely so an executor which creates a new thread per task would work equally well. Normally you also don't submit an indefinitely running work task which itself performs tasks to an executor, but instead directly submit the tasks to the executor.
This leads to the next question: How are you shutting down workers? It appears with your current code the DownloadTask and ProcessTask run forever. Or is there no need to shut down workers in your use case?
Using BlockingQueue.poll() and then waiting seems a little bit inefficient. Since you are already dealing with BlockingQueue you could use the blocking method BlockingQueue.take().
If I may make a suggestion, it appears CompletableFuture would be well suited for your use case:
public class DownloadProcessor {
    private final ExecutorService downloadExecutor;
    private final ExecutorService processingExecutor;

    public DownloadProcessor(int poolsSize) {
        downloadExecutor = Executors.newFixedThreadPool(poolsSize);
        processingExecutor = Executors.newFixedThreadPool(poolsSize);
    }

    public CompletableFuture<Void> downloadAndProcess(DownloadTask task) {
        return CompletableFuture.supplyAsync(() -> task.download(), downloadExecutor)
            // Assumes that DownloadTask.download() returns ProcessingTask which
            // has method `process()`
            .thenAcceptAsync(task -> task.process(), processingExecutor);
    }

    public void shutdown() {
        downloadExecutor.shutdown();
        processExecutor.shutdown();
        // Maybe also call awaitTermination
    }
}


Chain LINQ expressions for filtering Example of my method where I am chaining linq expressions based on model values provided:

public async Task<List<HistoryWorkoutExerciseModel>> GetHistoryWorkoutExerciseByFilterAsync(ChartFilterModel model)
{
    Expression<Func<HistoryWorkoutExercise, bool>> baseFilter = x => true && x.CreatedBy == model.CreatedBy;
    Expression<Func<HistoryWorkoutExercise, bool>> exerciseFilter = x => x.ExerciseId == model.ExerciseFilter;
    Expression<Func<HistoryWorkoutExercise, bool>> timeSpanFilter = x => x.HistoryWorkout.TimeTakenDate >= model.StartDate && x.HistoryWorkout.TimeTakenDate <= model.EndDate;

    if (model.ExerciseFilter > 0 && model.DateFilter == 0)
    {
        baseFilter = baseFilter.AndAlso(exerciseFilter);
    }

    if (model.ExerciseFilter == 0 && model.DateFilter > 0)
    {
        baseFilter = baseFilter.AndAlso(timeSpanFilter);
    }

    if(model.ExerciseFilter > 0 && model.DateFilter > 0)
    {
        baseFilter = baseFilter.AndAlso(timeSpanFilter).AndAlso(exerciseFilter);
    }

    return _mapper.Map<List<HistoryWorkoutExerciseModel>>(await _repository.GetAsync(baseFilter, x => x.OrderByDescending(y => y.HistoryWorkout.TimeTakenDate), "Exercise,HistoryWorkout"));
}

What's the best way to refactor this, as I am not the best fan of if conditional statements? Is there any other specific approach for this?

You have such properties as ExerciseFilter and DateFilter that I guess return respecitively the ExerciseId and (EndDate - StartDate).TotalMinutes or something similar. Thus, I find it would be much easier to understand your filter class if you renamed the ExerciseFilter to ExerciseId and replaced both properties with CanApplyExerciseFilter and CanApplyDateFilter. This would result in a much cleaner code:

if (model.CanApplyExerciseFilter && !model.CanApplyDateFilter)
{
    baseFilter = baseFilter.AndAlso(exerciseFilter);
}

if (!model.CanApplyExerciseFilter && model.CanApplyDateFilter)
{
    baseFilter = baseFilter.AndAlso(timeSpanFilter);
}

if (model.CanApplyExerciseFilter && model.CanApplyDateFilter)
{
    baseFilter = baseFilter.AndAlso(timeSpanFilter).AndAlso(exerciseFilter);
}

which would finally lead us to just two lines of code here:

baseFilter =
    baseFilter
        .AndAlso(model.CanApplyExerciseFilter ? exerciseFilter : x => true)
        .AndAlso(model.CanApplyDateFilter ? timeSpanFilter : x => true);