Parsing a shop receipt I've been working on a parser for shop receipts which extracts data about the payment. Here is the text that I'm parsing:

  * Vic107Payment
    Text: PINNEN
    TicketData: POI: 12345678
KLANTTICKET
--------------------------------
Terminal:                 ABC123
Merchant:                 123456
Periode:                    1234
Transactie:             12345678

MAESTRO
(A0000000012345)
MAESTRO
Kaart:       xxxxxxxxxxxxxxx1234
Kaartserienummer:              0

BETALING
Datum:          01/01/2020 04:15
Autorisatiecode:          123ABC

Totaal:                 1,00 EUR

Leesmethode: NFC Chip

    CardTypeId: 1234
    CardTypeText: MAESTRO
    ReceiptNumber:
    DrawerAmount: 1,00
    Number: 1
    DrawerId: drawers/default
    DrawerNumber: 1
    Amount: 1,00
    IsCancelable: True
===

Here is the code that parses this fragment from the receipt. Now my concern is that people will find it difficult to read and/or difficult to maintain, so I was wondering whether it could be improved in any way?

def parse_card_payment(product):
    # cp stands for card payment
    cp_poi = None
    cp_terminal = None
    cp_merchant = None
    cp_period = None
    cp_transaction = None
    cp_card = None
    cp_card_serial_number = None
    cp_date = None
    cp_authorisation_code = None
    cp_total = None
    cp_card_type_id = None
    cp_card_type_text = None
    cp_drawer_id = None
    cp_drawer_amount = None
    cp_cancelable = None
    cp_card_type = None

    for line in product.strip().split('\n'):
        if 'POI' in line:
            cp_poi = line.split(':')[1].strip()
        elif 'Terminal' in line:
            cp_terminal = line.split(':')[1].strip()
        elif 'Merchant' in line:
            cp_merchant = line.split(':')[1].strip()
        elif 'Periode' in line:
            cp_period = line.split(':')[1].strip()
        elif 'Transactie' in line:
            cp_transaction = line.split(':')[1].strip()
        elif 'Kaart:' in line:
            cp_card = line.split(':')[1].strip()
        elif 'Kaartserienummer' in line:
            cp_card_serial_number = line.split(':')[1].strip()
        elif 'Datum' in line:
            cp_date = line.split(':')[1].strip()
        elif 'Autorisatiecode' in line:
            cp_authorisation_code = line.split(':')[1].strip()
        elif 'Totaal' in line:
            cp_total = line.split(':')[1].strip()
        elif 'CardTypeId' in line:
            cp_card_type_id = line.split(':')[1].strip()
        elif 'CardTypeText' in line:
            cp_card_type_text = line.split(':')[1].strip()
        elif 'DrawerAmount' in line:
            cp_drawer_amount = line.split(':')[1].strip()
        elif 'DrawerId' in line:
            cp_drawer_id = line.split(':')[1].strip()
        elif 'Cancelable' in line:
            cp_cancelable = line.split(':')[1].strip()
        elif 'Leesmethode' in line:
            cp_card_type = line.split(':')[1].strip()

    cp = {'cp_poi': cp_poi,
          'cp_terminal': cp_terminal,
          'cp_merchant': cp_merchant,
          'cp_period': cp_period,
          'cp_transaction': cp_transaction,
          'cp_card': cp_card,
          'cp_card_serial_number': cp_card_serial_number,
          'cp_date': cp_date,
          'cp_authorisation_code': cp_authorisation_code,
          'cp_total': cp_total,
          'cp_card_type_id': cp_card_type_id,
          'cp_card_type_text': cp_card_type_text,
          'cp_drawer_id': cp_drawer_id,
          'cp_drawer_amount': cp_drawer_amount,
          'cp_cancelable': cp_cancelable,
          'cp_card_type': cp_card_type}
    return cp

In programming there is the general principle Don't Repeat Yourself (DRY). Your code is a lot of repetition of exactly the same pattern, with only the string changing.

So, just put those strings into a dictionary, with the final variable name as keys:

RECEIPT_ITEMS = {"cp_poi": "POI", "cp_terminal": "Terminal",
                 "cp_merchant": "Merchant", "cp_period": "Periode",
                 "cp_transaction": "Transactie", "cp_card": "Kaart",
                 "cp_card_serial_number": "Kaartserienummer", "cp_date": "Datum",
                 "cp_authorisation_code": "Autorisatiecode",
                 "cp_total": "Totaal", "cp_card_type_id": "CardTypeId",
                 "cp_card_type_text": "CardTypeText",
                 "cp_drawer_amount": "DrawerAmount", "cp_drawer_id": "DrawerId",
                 "cp_cancelable": "Cancelable", "cp_card_type": "Leesmethode"}

def parse_card_payment(product):
    cp = dict.fromkeys(RECEIPT_ITEMS.keys())
    for line in product.splitlines():
        for key, value in RECEIPT_ITEMS.items():
            if value in line:
                cp[key] = line.split(":")[1].strip()
                break
    return cp

This has the advantage that if you ever have receipts in another language than Dutch (but with the same structure), you only need to localize the values of this dictionary and not change your whole code.

Note that I used str.splitlines, which automatically ignores trailing newlines.

A different approach might be to use a multi-line RegEx to perform the search directly, but that will probably be more complicated.


Sliding window over a 2D vector, implementation of Marching Squares algorithm I have not used C++ in quite a while and looking for advice/feedback, the code below implements the Marching Squares Algorithm which is detailed here.
While the implementation appears correct and gives the same result as the example on the Wikipedia page, I think that some of the code could be improved and I'm looking for general feedback on how.
In particular,

how I handle vectors
the 2x2 sliding matrix that moves across the test data
where I OR in a value

#include <iostream>
#include <stdio.h>
#include <tchar.h>
#include <vector>
using std::vector;

void ApplyThreshold(vector<vector<int>> &Matrix2D, int threshold);
vector<vector<int>> CreateContourMatrix(vector<vector<int>> &pMatrix);

int main()
{

    const int cutOff = 2;//The value on which to base the creation of contours
    vector<vector<int>> matrix;
    //Setup test data
    matrix.push_back(vector<int> { 1, 1, 1, 1, 1 });
    matrix.push_back(vector<int> {1, 2, 3, 2, 1, });
    matrix.push_back(vector<int> {1, 3, 3, 3, 1, });
    matrix.push_back(vector<int> {1, 2, 3, 2, 1, });
    matrix.push_back(vector<int> {1, 1, 1, 1, 1, });

    vector<vector<int>> pMatrix = matrix;

    ApplyThreshold(pMatrix, cutOff);//Establishthe contours
    auto contourMatrix = CreateContourMatrix(pMatrix);//Build contour shapes
    //Print the result
    for (auto contour_row : contourMatrix)
    {
        for (int element : contour_row)
        {
            std::cout << element << '\t';
        }
        std::cout << std::endl;
    }
    return 0;
}
/*
 * Move a 2x2 window across a 2d matrix, calculating contour shapes
 */
vector<vector<int>> CreateContourMatrix(vector<vector<int>> &pMatrix)
{
    vector<vector<int>> contourMatrix;
    //vector<int> rows[5];
    for (int i =0;i<pMatrix.size() - 1;i++)
    {
        vector<int> row;//Should this allocation be moved out of the for loop?

        for (int j=0;j<pMatrix[0].size() - 1;j++)//Assume that all rows of the image are the same size
        {
            int shape = 0;
            if (pMatrix[i][j] == 1)
                shape = shape | 8;
            if (pMatrix[i][j + 1] == 1)
                shape = shape | 4;
            if (pMatrix[i+1][j] == 1)
                shape = shape | 1;
            if (pMatrix[i+1][j+1] == 1)
                shape = shape | 2;
            row.push_back(shape);
        }
        contourMatrix.push_back(row);
    }
    return contourMatrix;
}

void ApplyThreshold(vector<vector<int>> &Matrix2D, int threshhold)
{
    for (auto rowVector : Matrix2D)
    {
        for (auto vectorValue : rowVector)
        {
            if (vectorValue >= threshhold)
                vectorValue = 1;
            else
                vectorValue = 0;
        }
    }
}

This isn't supposed to be a complete implementation, i.e. I don't deal with ambiguous cases or actually create an image of the contours. Any advice appreciated.

A couple miscellaneous things.

I don't think you need the vector<int> for the initialization. eg:

matrix.push_back({ 1, 1, 1, 1, 1, });

You can use references in the for loops, just for good measure. IMO using the type instead of auto makes it easier to read when it's a simple type too.

for (auto& contour_row : contourMatrix)
{
    for (int& element : contour_row)
    {
        std::cout << element << '\t';
    }
    std::cout << std::endl;
}

In CreateContourMatrix() the row vector can come out of the for loop. Use .clear() or .fill() on it inside the loop, then you aren't reallocating.


Checking where/if another creature lies in a creature's field of view Note: this is a more specific form of this

I am working on a simulation of some virtual creatures. They all have vision that lets them see other creatures. To do this, I calculate the difference between the creature's current heading and the angles to other creatures. Then, I check if the distance between them is close enough so that the creature can see the other.

It works, but it is quite slow. This is because it has complexity O(n2). It has to loop through the array and apply the angle function to every combination of creatures. Then, it needs to check distance between some of them.

My current code:

import math
import random
def getAngle(pos1,pos2):
    dx=pos2[0]-pos1[0]
    dy=pos2[1]-pos1[1]
    rads=math.atan2(dy,dx)
    rads%=2*math.pi
    degs=math.degrees(rads)
    return degs

def getDist(pos1, pos2):
    return math.hypot(pos1[0] - pos2[0], pos1[1] - pos2[1])
def angleDiff(source,target):
    a = target - source
    a = (a + 180) % 360 - 180
    return a
class Creature(object):
    """A virtual creature"""
    def __init__(self):
        self.pos=[random.randint(0,500) for _ in range(2)] #random x,y
        self.heading=random.randint(0,360)
        self.vision=[0,0] #left and right relative to creature's perspective

creatures=[Creature() for _ in range(100)]
creatureFOV=60 #can look 60 degrees left or right
creatureViewDist=100
for creature in creatures:
    for otherC in creatures:
        if otherC==creature:continue #don't check own angle
        ang=angleDiff(creature.heading,getAngle(creature.pos,otherC.pos))
        if abs(ang) < creatureFOV:
            if(getDist(creature.pos,otherC.pos)<creatureViewDist):
                if ang < 0:
                    creature.vision[0]=1 #left side
                else:
                    creature.vision[1]=1 #right side
        if sum(creature.vision)==2:
            break #if there is already something on both sides, stop checking

I feel like it could be greatly sped up by using numPy or some other method. How can this code be optimized for more speed?

In this case, you should divide your world into a 2D grid, where each grid cell is at least as large as your viewing distance. Lets say that the grid has \$k\$ cells. Then, in each step of the iteration you place each creature into its appropriate gridcell \$(O(n))\$, then for each cell you iterate over the creatures in it, and check it against every creature in the up to 9 closest cells \$(O(\frac{9n^2}{k}))\$.

This is still not perfect, but it could provide a significant speedup if you have a lot of creatures, and if your viewing distance is small compared to the size of your world. This would imply that you can make \$k\$ large, and thus you could make your algorithm 10-1000 times faster depending on your situation.

Another thing: since calculating the difference in heading takes longer time than checking pairwise distance, you should check the distance first, and only compare the heading if the creatures are close to each other.

If you want to get even more efficient, you could save half the pairwise distance checks by not comparing B to A if you have already compared A to B. To do this, you could sort the creatures by some ID once, and in your iteration you only compare distances if the first creature has lower ID than the second. However, this will most likely not impact you runtime significantly.

EDIT: I tried implementing a grid structure, but since the creatures are spaced on a 500x500 area, and their view distance is 100, the maximum speedup is only about \$\frac{25}{9}\$. However, I managed to get everything running about 3 times faster by switching to checking distance first, checking distance squared instead of distance, and using a grid. It's not very pretty, but it's something that can be improved:

import math
import random
import time

def getAngle(c1, c2):
    dx=c2.x-c1.x
    dy=c2.y-c1.y
    rads=math.atan2(dy,dx)
    return rads

def getDist(c1, c2):
    return (c1.x-c2.x)**2 + (c1.y-c2.y)**2

def angleDiff(source,target):
    a = target - source
    a = (a + math.pi) % (2*math.pi) - math.pi
    return a

class Creature(object):
    """A virtual creature"""
    def __init__(self):
        self.x = 500*random.random()
        self.y = 500*random.random()
        self.heading=random.random()*2*math.pi
        self.vision_right = False
        self.vision_left = False
        self.FOV = 60/180*math.pi
        self.viewDistanceSq = 100**2

def check_visibility(creature, other_creature):
    if getDist(creature, other_creature) < creature.viewDistanceSq:
        ang = angleDiff(creature.heading,getAngle(creature,other_creature))
        if abs(ang) < creature.FOV:
            if ang < 0:
                creature.vision_left = True #vision_left side
                if creature.vision_right:
                    return True
            else:
                creature.vision_right = True #vision_right side
                if creature.vision_left:
                    return True
    return False

def check_neighbors(creature, grid, i, j):
    for di in range(-1, 2):
        if not 0 <= i+di < 5:
            continue
        for dj in range(-1, 2):
            if not 0 <= j+dj < 5:
                continue
            for other_creature in grid[i+di][j+dj]:
                if creature == other_creature:
                    continue
                checked = check_visibility(creature, other_creature)
                if checked:
                    return

def run_simulation(creatures, grid):
    for creature in creatures:
        grid[int(creature.x/100)][int(creature.y/100)].append(creature)

    for i, row in enumerate(grid):
        for j, cell in enumerate(row):
            for creature in cell:
                check_neighbors(creature, grid, i, j)

creatures=[Creature() for _ in range(2000)]
t0 = time.clock()
for _ in range(1):
    grid = [[[] for i in range(5)] for j in range(6)]
    run_simulation(creatures, grid)
t1 = time.clock()
print(t1-t0)


Naive password checker I'm looking for advice on how I could improve this. I'm using VS2015 to compile so whatever language features are supported in there is fair game!

Problem: Create a program that reads a username and a password from
the user and grants access if that username and password exists in
code. Bonus points for storing the possible usernames in a file like
so:
user1,pass
user2,pass
...

Encryption is not really a concern; I'm just getting the hang of things. Points I'd like to improve:

I'm using a placeholder vector for the user input, initialized with empty strings. It feels like there would be a more elegant way to get user input without using a holding variable?

I imagine there's a more elegant way to go through each line of the file too, again, lots of placeholder variables for data I could process in place I think.

#include <iostream>
#include <fstream>
#include <vector>
#include <string>

using namespace std;

// Extract the contents of a user file for processing.
vector<string> get_user_list(ifstream& userfile)
{
    string line;
    vector<string> userlist;

    if (userfile.is_open())
    {
        while (getline(userfile, line))
            userlist.push_back(line);

        userfile.close();
    }
    else
        cout << "Couldn't read from file.";

    return userlist;
}

// Go through each line of the user list and extract a comma
// seperated username and pass; match against the supplied
// username and pass.
bool check_pass(vector<string> tomatch, vector<string> userlist)
{
    string fuser, fpass;
    bool match = false;

    for (string uline : userlist)
    {
        for (size_t i = 0; i < uline.size(); i++)
        {
            if (uline[i] == ',')
            {
                fuser = uline.substr(0, i);
                fpass = uline.substr(i + 1, string::npos);
                break;
            }
        }

        if (tomatch[0] == fuser && tomatch[1] == fpass)
        {
            match = true;
            break;
        }
    }

    return match;
}

int main()
{
    ifstream userfile("data/users.txt");
    vector<string> userlist = get_user_list(userfile);
    vector<string> input = { "", "" };

    cout << "Enter your username followed by your pass: ";
    cin >> input[0] >> input[1];

    if (check_pass(input, userlist))
        cout << "ACCESS GRANTED" << endl;
    else
        cout << "ACCESS DENIED" << endl;

    return 0;
}

Whenever you are passing arguments in C++, you must be a bit more careful. Since C++ is a default by-value language, passing in a std::vector the way you are doing it will create a full copy of the vector each time you call the function. This is expensive, and not needed. Instead, you should be passing by const &:

bool check_pass(const vector<string>& tomatch, const vector<string>& userlist)

The same thing applies when using a range-based for loop:

for (const string& uline : userlist)

Or you could simply let C++ deduce the type for you, using auto:

for (const auto& uline : userlist)

string has some methods that can help make your code a bit shorter and clearer; such as find:

auto place = uline.find(',');

As @janos has already mentioned, using a map (or unordered_map) would be a better fit here. This could look something like:

std::unordered_map<std::string, std::string> user_to_pwd;
while (getline(userfile, line)) {
    const auto index = line.find(',');
    user_to_pwd.insert(
        std::make_pair(line.substr(0, index), line.substr(index + 1));
}

Your check_pass function could then simply take three parameters: the map, and the username and password. This then becomes a (fast) lookup:

bool check_pass(
    const std::unordered_map<std::string, std::string>& user_pwds,
    const std::string& user,
    const std::string& password)
{
    auto it = user_pwds.find(user);
    if(it == user_pwds.end()) {
        return false;
    }
    return it->second == password;
}

Note also that you should not use using namespace std; up the top. This pulls everything from the std namespace into the global namespace, which can cause all sorts of issues. For small programs it probably won't, but it's best not to get into the habit of using it anyway.


Binary search guess game in C++ I'm trying to write a program where computer attempts to guess a number entered by user. Computer should keep asking until guessed number will be equal to user number.

If you know what can be improved to make this code more readable, if implementation of the binary search algorithm is efficient, let me know!

#include <cstdlib>
#include <iostream>

int main() {
    std::srand(time(nullptr));
    constexpr int MIN = 1;
    constexpr int MAX = 100;
    std::cout << "Choose a number from " << MIN << " to " << MAX << "\n";
    int number;
    std::cin >> number;
    int high = MAX;
    int low = MIN;
    int guess = (high + low) / 2;
    char lessThan = std::rand() % 2 == 0 ? 'y' : 'n';
    char greaterThan = std::rand() % 2 == 0 ? 'y' : 'n';
    while (greaterThan == lessThan)
        greaterThan = std::rand() % 2 == 0 ? 'y' : 'n';
    while (guess != number) {
        if (lessThan == 'y' && greaterThan == 'n') {
            std::cout << "Is your number less than " << guess << "? [y|n]\n";
            std::cin >> lessThan;
            if (lessThan == 'y') {
                high = guess - 1;
                greaterThan = 'n';
            } else if (lessThan == 'n') {
                low = guess + 1;
                greaterThan = 'y';
            }
        } else if (greaterThan == 'y' && lessThan == 'n') {
            std::cout << "Is your number greater than " << guess << "? [y|n]\n";
            std::cin >> greaterThan;
            if (greaterThan == 'y') {
                low = guess + 1;
                lessThan = 'n';
            } else if (greaterThan == 'n') {
                high = guess - 1;
                lessThan = 'y';
            }
        }
        guess = (high + low) / 2;
    }
    std::cout << "I guessed!\n";
    return 0;
}

I don't know, if you can understand all my code, but I would implement the task this way:

#include <iostream>
#include <random>
#include <string>

enum class CompareAnswer { LESS, LESS_OR_EQUAL, GREATER, GREATER_OR_EQUAL };
enum class TruthAnswer { YES, NO };

CompareAnswer ask_user_compare(int compare_with) {
  static auto randfn = []() -> bool {
    static auto dist = std::uniform_int_distribution<>(0, 1);
    static auto gen = std::default_random_engine();
    return static_cast<bool>( dist(gen) );
  };

  char user_input;
  std::cout << "Is your number ";

  if (randfn()) {
    std::cout << "less than " << compare_with << "? [y|n]\n";
    std::cin >> user_input;
    if (user_input == 'y')
      return CompareAnswer::LESS;
    if (user_input == 'n')
      return CompareAnswer::GREATER_OR_EQUAL;
    throw std::runtime_error("invalid answer");
  };

  std::cout << "greater than " << compare_with << "? [y|n]\n";
  std::cin >> user_input;
  if (user_input == 'y')
    return CompareAnswer::GREATER;
  if (user_input == 'n')
    return CompareAnswer::LESS_OR_EQUAL;
  throw std::runtime_error("invalid answer");
};

TruthAnswer ask_user_is(int number) {
  std::cout << "Is your number " << number << "? [y|n]\n";
  char user_input;
  std::cin >> user_input;

  if (user_input == 'y')
    return TruthAnswer::YES;
  if (user_input == 'n')
    return TruthAnswer::NO;
  throw std::runtime_error("invalid answer");
}

int guess_number(const int min, const int max) {
  // std::cout << "\ndebug: min=" << min << " max=" << max << "\n";

  const int median = (min + max) / 2;  // (77+78)/2  =77

  if (median == min) {
    return ask_user_is(min) == TruthAnswer::YES ? min : max;
  };

  switch (ask_user_compare(median)) {
    case CompareAnswer::GREATER:
      return guess_number(median + 1, max);
    case CompareAnswer::GREATER_OR_EQUAL:
      return guess_number(median, max);
    case CompareAnswer::LESS:
      return guess_number(min, median - 1);
    case CompareAnswer::LESS_OR_EQUAL:
      return guess_number(min, median);
    default:
      throw std::logic_error("need case for some of 'enum Answer' ");
  };
};

int main() {
  constexpr int MIN = 0;
  constexpr int MAX = 10000;

  std::cout << "Choose a number from " << MIN << " to " << MAX << "\n";
  int number;
  std::cin >> number;
  if(number < MIN || number > MAX) throw std::runtime_error("Invalid number");

  int guessed = guess_number(MIN, MAX);
  std::cout << "Your number is: " << guessed << "\n";
}

Even though guess_number is recursive function, probability of stack overflow extremely low (cause log(N) complexity). Asking user again instead of throwing exception may be good idea too.

Also:
Generally forget about srand, use <random> instead (srand is only for homework)


SheetChange event VBA I would like to ask for a code review as I feel like it's not the most efficient way of doing it. For context I have a add-ins with all the code in module and then the SheetChangeevent is in ThisWorkbook. Also in the add-ins I have 2 sheets that will have data on it so that the ActiveWorkbook will be able to read this info when running code form module the events will fire and do a vlookup against the sheet in the add-ins.

Private Sub ExcelApp_SheetChange(ByVal Sh As Object, ByVal Target As Range)
    Dim KeyCells As Range
    Dim wsMyList As Worksheet
    Set wsMyList = ThisWorkbook.Sheets(2)

    Set KeyCells = [B3,B5] 'I only need this 2 cells to fire events
    If Sh.Name <> "Response" Then
         If Not Intersect(Target, Sh.Range("B3:B5")) Is Nothing Then 'not too sure how to do it here so I put 3 cells instead of 2
            If Target.Row = 3 Then
                If Range("B3").Value = vbNullString Then Exit Sub
                Application.EnableEvents = False
                If Sh.Range("B3").Value <> vbNullString Then
                    Sh.Range("B4").Value = Application.WorksheetFunction.VLookup(Sh.Range("B3").Value, wsMyList.Range("A:B"), 2, False)
                    Sh.Range("B6").Value = "Type or Select a transaction"
                Else
                    Sh.Range("B4").Value = "Type or Select a program"
                End If
                Columns.ClearColumns
                Transactions.FetchTransactions
                Application.EnableEvents = True
            ElseIf Target.Row = 5 Then
                If Range("B5").Value = vbNullString Then Exit Sub
                Application.EnableEvents = False
                If Range("B5").Value <> vbNullString Then
                    Sh.Range("B6").Value = Application.WorksheetFunction.VLookup(Sh.Range("B5").Value, wsMyList.Range("D:E"), 2, False)
                    Columns.ClearColumns
                    Columns.PopulateFields
                End If
                Application.EnableEvents = True
            End If
        End If
    End If
End Sub

When writing any kind of event method, I always try to minimize the code that executes because you don't want the user to be aware of processing that is happening between the keystrokes. In your case, your code is firing each time ANY cell on ANY sheet is changed. So in the spirit of keeping things streamlined, don't create, initialize, or perform any logic that you don't really need (until you need it). Using this philosophy, the beginning of my example method would look like this:

If Sh.Name = "Response" Then Exit Sub

Dim checkCells As Range
Set checkCells = Union(Sh.Range("B3"), Sh.Range("B5"))
If Intersect(Target, checkCells) Is Nothing Then Exit Sub
If Target.Address = vbNullString Then Exit Sub

Notice that if the changed sheet is "Response", then initializing any other variable is meaningless. Once we get past that, a checkCells range is established using the Union function. It may be a bit overkill sometimes, but clearly illustrates the idea that you're looking at multiple, non-contiguous cells/ranges.

Also, from your OP code, if either cell is empty, you are immediately exiting. So you can check the target address after the other checks right away.

But then I get into an issue where you check if the cell value is null, but right away you're checking that it has a value:

If Range("B3").Value = vbNullString Then Exit Sub
Application.EnableEvents = False
If Sh.Range("B3").Value <> vbNullString Then ...

This is redundant. And -- by the way -- you'll never get to your Else statement because you've already exited if B3 is a null string.

The statement in both parts of your If statement is confusing:

Columns.ClearColumns

Columns is a property of a Range or Worksheet, and ClearColumns is not a method of Range at all that I know of. So I'm assuming it's a part of the add-in. But if Columns is the name of one of your code modules, then change it. Using Columns is not a good name to use because it is the same as an existing property and is confusing. If you are clearing columns on a worksheet, then specify which worksheet always. I'm also assuming the Transactions is a code module in your VBA project as well.

Here is all of the code:

Option Explicit

Private Sub Workbook_SheetChange(ByVal Sh As Object, ByVal Target As Range)
    If Sh.Name = "Response" Then Exit Sub

    Dim checkCells As Range
    Set checkCells = Union(Sh.Range("B3"), Sh.Range("B5"))
    If Intersect(Target, checkCells) Is Nothing Then Exit Sub
    If Target.Address = vbNullString Then Exit Sub

    Dim wsMyList As Worksheet
    Set wsMyList = ThisWorkbook.Sheets(2)

    Application.EnableEvents = False
    With Sh
        Dim lookupArea As Range
        If Target.Row = 3 Then
            Set lookupArea = wsMyList.Range("A:B")
            .Range("B4").Value = Application.WorksheetFunction.VLookup(Target.Value, _
                                                                       lookupArea, _
                                                                       2, False)
            .Range("B6").Value = "Type or Select a transaction"
            Columns.ClearColumns
            Transactions.FetchTransactions
        ElseIf Target.Row = 5 Then
            Set lookupArea = wsMyList.Range("D:E")
            .Range("B6").Value = Application.WorksheetFunction.VLookup(Target.Value, _
                                                                       lookupArea, _
                                                                       2, False)
            Columns.ClearColumns
            Columns.PopulateFields
        End If
    End With
    Application.EnableEvents = True
End Sub


PHP MySQL make insert and update large amount of users faster I'am using php on server side to manage data with MySQL. I have to request a API that gives me an list of users. I need to check for each user if he is in the database. If yes, I update his information. If not, I insert him in the data base.

The issue is that there is more than 2000+ users each times and my code in PHP is really slow (sometimes I get 504 Gateway Time-out). We will have even more users very soon.

How can I make my code faster ? Is Php ok if there is more users?

    $userList = getFromAPI();

      foreach ($userList as $userId){

        $db = dbConnect();

  $query = $db->prepare(
      "SELECT openid FROM USERS WHERE openid = :openid"
    );

  $query->execute([
    "openid"=>$openId,
    ]);

    $res = $query->fetch(PDO::FETCH_ASSOC);

  $tagList = implode(",", $user["tagid_list"]);

        if (!empty($res)) {

          $query = $db->prepare(
      "INSERT INTO USERS(id, name, group) VALUES(:id, :name, :group)
       ON DUPLICATE KEY UPDATE name=:name, group=:group"
          );

          $query->execute([
        "id"=>$id,
        "name"=>$name,
        "group"=>$group
          ]);
        }

     }

Well, the key point would be to wrap all your inserts in a transaction to make them dramatically faster.

Other points of less significance but rather a matter of sanity are:

connect once
prepare once and also make sure you are using a non-emulated prepared statement
get rid of the useless SELECT

So it should be like

$db = dbConnect();
$query = $db->prepare(
  "INSERT INTO USERS(id, name, `group`) VALUES(:id, :name, :group)
   ON DUPLICATE KEY UPDATE name=values(name), group=values(`group`)"
);

$userList = getFromAPI();
$db->beginTransaction();
foreach ($userList as $userId){
    // assuming you have a code to define these variables
    $query->execute([
    "id"=>$id,
    "name"=>$name,
    "group"=>$group
      ]);
}
$db->commit();


Read file into char* Because I use the task so often, I wrote a C header for reading a file. I never formally learned C, so I'd really appreciate a code review on the code.

As a matter of personal taste, I do not put a space between the final parenthesis and an opening brace (){).

readfile.h

#ifndef _READ_FILE_INCL
#define _READ_FILE_INCL
#include <stdio.h>     /* for FILE, fopen, getc, fclose, EOF */
#include <stdlib.h>    /* for malloc, realloc */

// the amount of bytes the file is initially guessed to be
#ifndef _READFILE_GUESS
    #define _READFILE_GUESS 256
#endif

/*
 * function `read_file`
 * arguments: a file name (char*)
 * returns: the contents of the file (char*) or NULL if:
 *   1. the file does not exist
 *   2. memory allocation failed
 * return value must be freed
 */
char* read_file(char* name){
    FILE* file;
    file = fopen(name, "r");

    // 1. the file does not exist: return null
    if(!file)
        return NULL;

    // allocate memory according to the initial guess
    // (+ 1) for trailing null byte
    char* result = malloc(sizeof(char) * _READFILE_GUESS + 1);

    // 2. memory allocation failed
    if(result == NULL)
        return NULL;

    // the position in the string to write the character to
    size_t pos = 0;

    // the amount of memory allocated for the string
    size_t capacity = _READFILE_GUESS;

    // character to hold the currently-read character from the file
    char ch;

    // until the character is the EOF character
    while((ch = getc(file)) != EOF){
        // update the character at position `pos` to `ch`
        result[pos++] = ch;

        // if the next position would exceed bounds
        if(pos >= capacity){
            // add the guess to the capacity
            capacity += _READFILE_GUESS;

            // allocate memory accordingly
            // (+ 1) for trailing null byte
            result = realloc(result, sizeof(char) * capacity + 1);

            // 2. memory allocation failed
            if(result == NULL)
                return NULL;
        }
    }

    // close the file
    fclose(file);

    // remove extra memory
    result = realloc(result, sizeof(char) * pos);

    // 2. memory allocation failed
    if(result == NULL)
        return NULL;

    // `pos` now points to the index after the last character read
    result[pos] = '\0';

    return result;
}

#undef _READFILE_GUESS

/*
 * MIT License
 *
 * Copyright (c) 2017 Conor O'Brien
 *
 * Permission is hereby granted, free of charge, to any person obtaining a copy
 * of this software and associated documentation files (the "Software"), to deal
 * in the Software without restriction, including without limitation the rights
 * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
 * copies of the Software, and to permit persons to whom the Software is
 * furnished to do so, subject to the following conditions:
 *
 * The above copyright notice and this permission notice shall be included in all
 * copies or substantial portions of the Software.
 *
 * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
 * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
 * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
 * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
 * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
 * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 * SOFTWARE.
 */

#endif

Getting Size of File

You can get the size of file (without asking the filesystem) by using fseek() and ftell().

if (fseeko(file, 0, SEEK_END) != 0) {
    // Error State do something
}
off_t sizeOfFile = ftello(file);
if (fseeko(file, 0, SEEK_SET) != 0) {
    // Error State do something
}

// Your file is now ready to be read and the size of the file is
// in `sizeOfFile`

Identifier Names

The identifier _READFILE_GUESS is reserved for the implementation. Don't use _ (underscore) as the first character in an identifier name.

Realloc

This is the wrong way to use realloc:

result = realloc(result, sizeof(char) * capacity + 1);

As you have noticed, when realloc fails it returns NULL. But it does not free the result value in this case. This means you have overwritten the value of result with null and thus leaked the memory it was pointing at.

char* newArea = realloc(result, sizeof(char) * capacity + 1);
if (newArea == NULL) {
    // Error condition handle it
    // You should probably release `result` by calling `free()`
    // before returning result
}
result = newArea;

Reading

Why are you only reading one byte at a time?

while((ch = getc(file)) != EOF)

Why not (attempt to) read capacity bytes at a time?

size_t blocksRead= 0;
size_t totalRead = 0;

while(1) {
    size_t read = fread(result + (blocksRead * capacity) + totalRead, 1, capacity - totalRead, file);
    if (ferror(file) != 0) {
        // Error State Deal with it.
    }
    totalRead += read;
    if (feof(file)) {
        break;
    }
    if (totalRead == capacity) {
        ++blocksRead;
        totalRead = 0;
        // Handle resize of block if you need too.
    }
}

License

Also note that by pasting the code into any Stack Exchange site you are already dual licensing your code under the CC license (see the bottom of the page for details).

So though your MIT License is valid it is not the only license this code is now available under.


Fine-grained synchronization to serve a file I'm writing a proxy server that serves cached resources. If resource is not cached, it's retrieved and put into filesystem. Server is multi-threaded and I want to avoid any kind of global locking. If there are several requests for the same resource that is not cached at the same time, one request should retrieve resource, put it into filesystem and other requests should be blocked and then serve cached resource.

My first approach was to synchronize on interned string:

void serveFile(Path file) {
    if (!Files.exists(file)) {
        synchronized (file.toString().intern()) {
            if (!Files.exists(file)) {
                retrieveResource(file);
            }
        }
    }
    serveResource(file);
}

But there are many discussions about why this approach is not the best, so I rewrote is as follows:

private final ConcurrentMap<Path, Object> downloadLocks = new ConcurrentHashMap<>();

void serveFile(Path file) {
    if (!Files.exists(file)) {
        Object lock = downloadLocks.get(file);
        if (lock == null) {
            Object newLock = new Object();
            Object existingLock = downloadLocks.putIfAbsent(file, newLock);
            lock = existingLock == null ? newLock : existingLock;
        }
        try {
            synchronized (lock) {
                if (!Files.exists(file)) {
                    retrieveResource(file);
                }
            }
        } finally {
            downloadLocks.remove(file, lock);
        }
    }
    serveResource(file);
}

retrieveResource method retrieves resource into a temporary file and then performs atomical move into the final destination, so I assume that it's safe to use.

My question is if it's a correct multithreaded code and if there are better suited primitives for this fine-grained synchronization. I'm using Java 7.

Java's low level thread synchronization mechanism (synchronized blocks, volatile variables, wait(), notify(), etc) were proven to be tricky for the human single threaded mindset. Threading hazards like deadlock, thread starvation, and race conditions, which result from incorrect use of low level thread synchronization, are also hard to detect and debug.

To that end, Java 7 introduced several new features/classes in the java.util.concurrency package that allow developers to approach the thread synchronization domain from a high(er) level.  java.util.concurrent.locks.ReentrantLock is more flexible and gives more fine grain control over the scope of locking.  it also performs better.  The advice that I see around the web (including on Stack Overflow) is that ReentrantLock should replace synchronized block in all new code.


Method for Access to a Resource with 'Retry' Logic I am trying to implement so me 'retry' logic in a Data Access layer where the DB occassionaly times out (and the method is called several thousand times a day). This is the best I can come up with, but I don't like throwing the exception if the code falls out of my while loop; it doesn't feel like a succinct solution. Can it be improved?

NB: No need for multi-threaded access.

private static currentDbAttempt

private static Int32? GetDbValue()
        {
            currentDbAttempt = 1;
            while(currentDbAttempt <= MAX_DB_ATTEMPTS)
            {
                try
                {
                    Int32? nextId = [some DB work]
                    return nextId.Value;
                }
                catch (Exception ex)
                {
                    if (currentDbAttempt == MAX_DB_ATTEMPTS)
                        throw new Exception(string.Format("Error in [method name]. Tried {0} times",
                            MAX_DB_ATTEMPTS), ex);

                    _log.WarnFormat("Error in [method name]: {0}. Try count {1}",
                        ex.Message, currentDbAttempt++);
                }
            }

            // Code smell...
            throw new Exception("[method name] - shouldn't reach here.");

        }

I agree that having to throw that exception to appease the compiler is ugly. There's also another bit of unpleasantness which isn't quite so egregious, but helps point to what's actually wrong here:

Your while loop condition is

while(currentDbAttempt <= MAX_DB_ATTEMPTS)

But inside the same loop, you do

if (currentDbAttempt == MAX_DB_ATTEMPTS)

So what's happening, causing both these problems, is that you're trying to do something inside the loop which is actually meant to happen when the loop is finished. That should happen outside the loop. Fortunately in this case it's a simple fix- just move your failure exception outside the loop:

static Int32? GetDbValue()
    {
        currentDbAttempt = 1;
        Exception lastException;
        while(currentDbAttempt <= MAX_DB_ATTEMPTS)
        {
            try
            {
                Int32? nextId = [some DB work]
                return nextId.Value;
            }
            catch (Exception ex)
            {
                lastException = ex;
                _log.WarnFormat("Error in [method name]: {0}. Try count {1}",
                    ex.Message, currentDbAttempt++);
            }
        }

        throw new Exception(string.Format("Error in [method name]. Tried {0} times",
                        MAX_DB_ATTEMPTS), lastException);

    }

Doing the above uncovered something else: you swallow the exceptions most of the time, but somewhat arbitrarily expose the last one as an inner exception if you fail. This does make sense on the assumption that the exceptions will always occur for the same reason (e.g. timeout), but you might consider using an AggregateException or similar to expose all of them.

Either the method should have an int return value, or you should be returning nextId instead of nextId.Value. Either you have a potential InvalidOperationException waiting to happen if nextId can truly not have a value, or you're needlessly giving your caller extra work having to do its own HasValue check.

You're doing a worrying amount with statics. Not only are you holding state in currentDbAttempt, you're also accessing what I can only assume is some dependency in _log. While you say multi-threading isn't an issue, over-reliance on statics also sacrifices your ability to take full advantage of OO features like polymorphism.

The currentDbAttempt variable is fixible easily enough by just moving it inside the method, where it should be anyway. But I think more generally you should make sure you're not making things static over-eagerly.

The way you increment currentDbAttempt is a good example of being "too clever". It's much less clear than if you just had an extra currentDbAttempt++ statement outside the WarnFormat line, and that would also be far less prone to accidentally introducing bugs if you later come back to maintain this method.

Even better, why not pick an idiomatic option and use a for loop?


Web-crawler for yellowpage designed using python I've written some code to scrape name, address, and phone number from yellowpage using python. This scraper has got input parameter. If the input is properly filled in and the filled in url exists in yellowpage then the scraper will definitely parse the three categories I mentioned earlier. I tried to make it maintaining the guidelines of OOP. It is working specklessly at this moment. Hope there is something to do betterment of this crawler:

import requests
from lxml import html

class YellowPage:
    @classmethod
    def Crawl(self, name, state):
        page = requests.get("https://www.yellowpages.com/search?search_terms=" + name + "&geo_location_terms=" + state).text
        tree = html.fromstring(page)
        for items in tree.xpath("//div[@class='info']"):
            name = items.findtext(".//span[@itemprop='name']")
            address = items.findtext(".//span[@class='street-address']")
            phone = items.findtext(".//div[@itemprop='telephone']")
            print(name, address, phone)

YellowPage.Crawl("pizza","florida")

If you've got a single method, there is probably not much sense in having a class and just have a separate function instead. More info at Stop Writing Classes.
If you do need a classmethod, the first argument to such a method is agreed to be named cls and not self.
Crawl should be named crawl according to Python PEP8 naming practices.
Use string formatting instead of string concatenation to make the final URL.
Or, you can use params to pass GET parameters.
Use if __name__ == '__main__':.
I would return from the crawl() method instead of printing.

Modified code:

import requests
from lxml import html

class YellowPage:
    URL_TEMPLATE = "https://www.yellowpages.com/search?search_terms={name}&geo_location_terms={state}"

    @classmethod
    def crawl(cls, name, state):
        page = requests.get(cls.URL_TEMPLATE.format(name=name, state=state)).text
        tree = html.fromstring(page)
        for items in tree.xpath("//div[@class='info']"):
            name = items.findtext(".//span[@itemprop='name']")
            address = items.findtext(".//span[@class='street-address']")
            phone = items.findtext(".//div[@itemprop='telephone']")
            yield (name, address, phone)

if __name__ == '__main__':
    for result in YellowPage.crawl("pizza", "florida"):
        print(result)

Another version (with params):

import requests
from lxml import html

class YellowPage:
    SEARCH_URL = "https://www.yellowpages.com/search"

    @classmethod
    def crawl(cls, name, state):
        page = requests.get(cls.SEARCH_URL, params={'search_terms': name, 'geo_location_terms': state}).text
        tree = html.fromstring(page)
        for items in tree.xpath("//div[@class='info']"):
            name = items.findtext(".//span[@itemprop='name']")
            address = items.findtext(".//span[@class='street-address']")
            phone = items.findtext(".//div[@itemprop='telephone']")
            yield (name, address, phone)

if __name__ == '__main__':
    for result in YellowPage.crawl("pizza", "florida"):
        print(result)

Version without a class:

import requests
from lxml import html

SEARCH_URL = "https://www.yellowpages.com/search"

def crawl(name, state):
    page = requests.get(SEARCH_URL, params={'search_terms': name, 'geo_location_terms': state}).text
    tree = html.fromstring(page)
    for items in tree.xpath("//div[@class='info']"):
        name = items.findtext(".//span[@itemprop='name']")
        address = items.findtext(".//span[@class='street-address']")
        phone = items.findtext(".//div[@itemprop='telephone']")
        yield (name, address, phone)

if __name__ == '__main__':
    for result in crawl("pizza", "florida"):
        print(result)


Extracting a sentence containing a specific word from a longer text My goal is to return a specific sentence in a story that contains a given word. Returns null if the word is not in the story. I'm ok with returning the first occurrence if there are multiples. Is there a more efficient or cleaner way to do this?

public static String getSentence(String text, String word) {
    String sentence = "";
    if (text.toLowerCase().contains(word)) {
        if (text.contains(".")) {  //Are there sentences terminating in a period?
            int loc = text.toLowerCase().indexOf(word);
            int a = loc;
            while (a >= 0) {
                if (text.charAt(a) == '.' || a == 0) {
                    sentence = text.substring(a,loc);
                    a = 0;
                }
                a--;
            }
            a = loc + word.length();
            while (a <= text.length()) {
                if (text.charAt(a) == '.' || a == text.length()) {
                    sentence += text.substring(loc,a+1);
                    a = text.length()+1;
                }
                a++;
            }
            return sentence;
        } else {
            return text;      //If no period, return full text
        }
    } else {
        return null;
    }
}

FYI - I'm implementing this in Android, so I don't believe I have access to Java 8.

There are a few things of interest with your solution. Firstly, it is a very literal implementation of the problem to solve, and I worry that it is too literal. For example, are you sure that sentences end with just a period .? Is it not a period and some whitespace? Is a URL like example.com two sentences?

The second issue I have is the blind trust you have in the inputs. You happily convert the input text to lower-case (too often, actually), but you do not convert the word to lower-case. If someone gives an upper-case word, you'll never find it.

I would prefer a more zen approach, using regular expressions... actually, just a split, and some Java 8 niceness.

private static final Pattern END_OF_SENTENCE = Pattern.compile("\\.\\s+");

public static String getSentence(String text, String word) {
    final String lcword = word.toLowerCase();
    return END_OF_SENTENCE.splitAsStream(text)
            .filter(s -> s.toLowerCase().contains(lcword))
            .findAny()
            .orElse(null);
}

Why is that better? Well, it streams the text in the form of sentences, and then finds the first match in a sentence. If there are no sentences, it matches the whole thing.

Note that the same principles can be used with a non-streaming approach. Split by sentences, then find the first match.

In an android environment, you could do:

private static final Pattern END_OF_SENTENCE = Pattern.compile("\\.\\s+");

public static String getSentence(String text, String word) {
    final String lcword = word.toLowerCase();
    for (String sentence : END_OF_SENTENCE.split(text)) {
        if (sentence.toLowerCase().contains(lcword)) {
            return sentence;
        }
    }
    return null;
}

Note that the results from the above code may, or may not include the terminating period. If the match is in the last sentence of a text, and that text ends with a period, then the period may be returned as part of the result. If there is a match in the middle of the text, then the period will not be included.


ConditionalActions through std::function Lambdas So I just experimented with some code to make a huge if-else construct more human readable, extendable and configurable (switch positions of conditions).

The idea was to have a vector that contains all conditions and their actions and then just iterate over the vector, executing their ConditionalAction.

I solved it with lambdas with capture so I could use membervariables of the class where the ConditionalActions() method is implemented as well as additional parameters without having to define parameters in the std::function declaration.

Additionally I outsourced the CConditionalAction definitions to another header that I included in the method itself. This is the main part that is causing a hassle with colleagues.

So

void ElseIfConstruct()
{
    int SomeVariable = 0;

    if(FirstCondition)
    {
        FirstAction(); // uses membervariables
    }
    else if (SecondCondition)
    {
        SecondAction(SomeVariable); // void SecondAction(&int refParameter)
    }
    //else if ... and so on
    else
    {
        LastAction();
    }
}

becomes

MyConditionalActions.h

#ifdef USE_CONDITIONAL_ACTIONS
// ifdef so the compiler does not try to compile.
// alternatively exclude it from build.

CConditionalAction FirstConditionalAction([&]()
{
    if (false)
    {
        return true;
    }
    return false;
});

CConditionalAction SecondConditionalAction([&]()
{
    if (false)
    {
        return true;
    }
    return false;
});

CConditionalAction LastConditionalAction([&]()
{
    if (true)
    {
        return true;
    }
    return false;
});

#endif

ConditionalAction.h

#include <functional>

class CConditionalAction
{
public:
    CConditionalAction(std::function<bool()> ConditionalAction)
    : m_ConditionalAction(ConditionalAction) {}

    bool operator()() const { return m_ConditionalAction(); }
    std::function<bool()> m_ConditionalAction = nullptr;
};

MyClass.cpp

#include "ConditionalAction.h"
#include <vector>

void ConditionedActions()
{
    #define USE_CONDITIONAL_ACTIONS
    #include "MyConditionalActions.h"
    #undef USE_CONDITIONAL_ACTIONS

    // the vector will later be created just once but could also be used to sort out
    // ConditionalActions that don't have to be tested at all in runtime.

    std::vector<CConditionalAction> vecConditionalActions;
    vecConditionalActions.push_back(FirstConditionalAction);
    vecConditionalActions.push_back(SecondConditionalAction);
    //vecConditionalActions.push_back(MoreConditionalActions);
    vecConditionalActions.push_back(LastConditionalAction);

    for (const auto& ConditionalAction : vecConditionalActions)
    {
        if (ConditionalAction())
        {
            return;
        }
    }
}

Restructuring then becomes a copy-paste in MyClass.cpp
Extending becomes a push_back into the vector in MyClass.cpp and a copy-paste with adjustments in the MyConditionalActions.h

This is long winded:
std::vector<CConditionalAction> vecConditionalActions;
vecConditionalActions.push_back(FirstConditionalAction);
vecConditionalActions.push_back(SecondConditionalAction);
//vecConditionalActions.push_back(MoreConditionalActions);
vecConditionalActions.push_back(LastConditionalAction);

Easier to use the constructor to put everything in place.
std::vector<CConditionalAction> vecConditionalActions
    {
        FirstConditionalAction,
        SecondConditionalAction,
        // MoreConditionalActions,
        LastConditionalAction
    };

// Note: You should probably make that a static variable so it is
//       only built once the first time it is created.

I don't see the point in writing a functor to hold a functor (a lambda) then wrap that in a functor (std::function) to hold in a vector. The whole CConditionalAction class is not necessary.
std::vector<std::function<bool()>> vecConditionalActions {
        [&](){if (false){return true;}return false;},
        ....
    };

I am surprised this compiles:
std::function<bool()> m_ConditionalAction = nullptr;
                                      ///   ^^^^^^^^

But it does. But what does it mean? Not sure. Not sure I want to find out either as there is no pointer semantics involved here. But since you have a constructor this will never be activated anyway. Writing code that has no clear meaning is a bad idea.
I am assuming your code in the lambdas is just example code as it does nothing.
Final Thought.
The point of this exercise was:

to make a huge if-else construct more human readable, extendable and configurable (switch positions of conditions)

I don't think you have a succeeded. I think this code just makes the code more obscure by hiding the actions in a separate file.


Printing a diamond of numbers If you input the number 3 from the keyboard, the program will show this:

      0
    0 1 0
  0 1 2 1 0
0 1 2 3 2 1 0
  0 1 2 1 0
    0 1 0
      0

Here is my code:

#include <iostream>

using namespace std;

int main()
{
    unsigned i,k=0,n;
    cout<<"n= "; cin>>n;

    while(k<=n)
        {

            for (i=0;i<=k;i++)
                cout<<i;
            for (i=k;i>0;i--)
                cout<<i-1;
            cout<<endl;
            k++;
        }

    k=n;

    while (k)
        {
            k--;
            for (i=0;i<=k;i++)
                cout<<i;
            for (i=k;i>0;i--)
                cout<<i-1;
            cout<<endl;

        }
}

I have two questions:

Is there a better/easier/clever way to do this?
How to deal with spaces? How to show them? (because my code is working but it doesn't show spaces to make this diamond look)

using namespace std is not preferred, although not that bad for small programs.
Each variable should be declared/initialized on separate lines.  This improves readability and also makes it easier to add any necessary comments.

unsigned i;
unsigned k = 0;
unsigned n;

Same with this:

cout << "n= ";
cin >> n;

Prefer "\n" to std::endl here (the latter flushes the buffer, which takes longer).  It's still okay to use the latter where both flushing and newlining are needed.
Always use descriptive names for variables.  Single-characters are best for loop counters (such as i).  This will vastly improve readability as you won't need comments to describe them.
It looks like you could use recursion instead of all these loops.  It may take longer (if you input a large number), but it should at least simplify the logic.


Storing passwords in a database I just wrote this function and am wondering if it's secure enough for storing passwords in a database:

public static function hash($salt, $str) {
    $salt = md5($salt);
    $salt = '$2y$12$HfuPgoDY94nHJssqQeVLmH' . $salt . '$';

    return crypt($str, $salt);
}

Yes
Blowfish is a great algorithm for storing passwords, it makes generating a hash slow such that brute force attacks are unviable. It's also php's recommended means of storing passwords.
But
The salt for blowfish is supposed to be:

22 digits from the alphabet "./0-9A-Za-z".

This applies to the salt string which in the question is:
'HfuPgoDY94nHJssqQeVLmH' . $salt . '$'

That's 22 chars + 32 chars + '$', which is 65 characters long and contains an illegal '$'. The consequence of that is that the salt actually used is a fixed string, the first 22 characters and is the same for all generated hashes.
The salt should be random, and for example you could use something as simple as this:
$salt = '$2y$12$' . substr(md5(time()), 0, 22);
return crypt($str, $salt);

Note however that this code is simple for brevity and can be significantly improved upon since the salt will only be made up of hexadecimal (0-9a-f) characters. Here's an example implementation that could be used to generate a better salt.
Make cost a parameter
A cost of 12 is fine for passwords, but you may wish to make it a parameter so that e.g. unit tests are not needlessly slow whenever they generate a password.


Cycle through filenames in vim So I have a list of files which names are something like this

long_boring_filename_del_1.tex
long_boring_filename_del_2.tex
long_boring_filename_del_3.sty
long_boring_filename_del_4.tex

I want to cycle through these filenames using some mapping defined in vim. Meaning if I have long_boring_filename_del_2.tex open and hit the mapping it should take me to the next file long_boring_filename_del_3.sty. It should also loop, meaning long_boring_filename_del_4.tex should take me back to long_boring_filename_del_4.tex.

In addition I want to be able to cycle thorugh the files in both directions.

After obtaining a bit of help over at vi and vim I was able to come up with the following solution (which works).

function! s:next_del()

    " do nothing if current filename doesn't match "
    if @% !~# 'del-\v[1234]\.'
        return
    endif

    " open next file "
    let fnameA = substitute(@%, '\v[1234]\ze\.', '\=submatch(0)%4+1', '')
    if filereadable(fnameA)
        exec 'echo'
        exec 'e ' . fnameA
    else
        " if the .tex file does not exist check if the .sty versions does "
        let fnameB = substitute(fnameA, '\.tex', '\.sty', '')
        if filereadable(fnameB)
            exec 'e ' . fnameB
        else
            " if the above fails, check if changing sty to tex works "
            let fnameB = substitute(fnameA, '\.sty', '\.tex', '')
            if filereadable(fnameB)
                exec 'e ' . fnameB
            endif
        endif
    endif
endfunction

" Same as s:next_del() except this cycles in the opposite direction "
function! s:prev_del()

    " do nothing if current filename doesn't match "
    if @% !~# 'del-\v[1234]\.'
        return
    endif

    " open previous file "
    let fnameA = substitute(@%, '\v[1234]\ze\.', '\=submatch(0)-1', '')
    let fnameA = substitute(fnameA, '\v[0]\ze\.', '\=submatch(0)+4', '')
    if filereadable(fnameA)
        exec 'e ' . fnameA
    else
        " if the .tex file does not exist check if the .sty versions does "
        let fnameB = substitute(fnameA, '\.tex', '\.sty', '')
        if filereadable(fnameB)
            exec 'e ' . fnameB
        else
            " if the above fails, check if changing sty to tex works "
            let fnameB = substitute(fnameA, '\.sty', '\.tex', '')
            if filereadable(fnameB)
                exec 'e ' . fnameB
            endif
        endif
    endif
endfunction

nnoremap <silent> <localleader>w :call <sid>next_del()<CR>
nnoremap <silent> <localleader>q :call <sid>prev_del()<CR>

While it work I would love to get some pointers on how to improve the code as I am very much a beginner when it come to vimscript. Here are the major points in the code that needs improvement:

Can the two functions s:next_del() and s:prev_del() be combined into one?
My method of cycling backwards is very barbaric.

let fnameA = substitute(@%, '\v[1234]\ze\.', '\=submatch(0)-1', '')
let fnameA = substitute(fnameA, '\v[0]\ze\.', '\=submatch(0)+4', '')

I did it in this fashion because apparently you  can't separate the
leading \ and = , as otherwise it's a pure string. I would like to get some pointers on how to improve it.
Lastly my method of "checking" whether I have a sty file or tex file is strange at best. I do not really check, just change sty to tex and see if it works. I tried to get some regex going by using (tex|sty) but for some reason vimscript did not accept it. I would like to learn a better method of alternating between the sty and tex files.

Your code can be improved in many aspects, so it is easier to write my own solution rather to rewrite your.

Differences from your:

I suggest to use glob() function to find all files, which are matching to a pattern.
Singular Move(move_direction) function is enough, a duplication is not needed.
Not only new files are opened, but switching between already opened buffers are supported.

function! s:MyCompare(i1, i2)
    let l:f = str2nr(matchstr(a:i1, '[0-9]\+'))
    let l:s = str2nr(matchstr(a:i2, '[0-9]\+'))
    return l:f == l:s ? 0 : l:f > l:s ? 1 : -1
endfunc

function! s:Move(step)
    let l:current_filename = bufname('%')
    let l:common_part = matchstr(l:current_filename, '[^0-9]*')

    "" sort() is needed for numerical order
    let l:matched_file_list = sort(glob(l:common_part . '*', 0, 1), 's:MyCompare')

    let l:list_size = len(l:matched_file_list)
    let l:cur_idx = index(l:matched_file_list, l:current_filename)

    let l:next_idx = (l:cur_idx + a:step) % l:list_size
    let l:next_buf_name = l:matched_file_list[l:next_idx]

    if !bufloaded(l:next_buf_name)
        execute 'e' l:next_buf_name
    endif

    execute 'buffer' l:next_buf_name
endfunction

command! Fwrd call s:Move(1)
command! Bwrd call s:Move(-1)

noremap <Leader>c :Fwrd<CR>
noremap <Leader>d :Bwrd<CR>

Directory content

ls -1

long_boring_filename_del_101.tex
long_boring_filename_del_1.tex
long_boring_filename_del_202.tex
long_boring_filename_del_2.tex
long_boring_filename_del_3.sty
long_boring_filename_del_4.tex
long_boring_filename_del_5.tex

Demonstration

, is a Leader key.


Sending many unique emails at same time I have an asp.net core app that sends emails with a link. When they click the link they can start taking an online test without logging in. The link contains a number that is validated by the controller who then returns their test.

I log into my app and create a project. Here I enter a list of emails in a textbox separated by commas. In the backend I put this textbox string into a list like so:

string emailList;
List<string> list = emailList.Split(',').ToList();
//removes whitespace in list:
for (int i = 0; i < list.Count(); i++)
{
 list[i] = string.Join("", list[i].Split(default(string[]),
                       StringSplitOptions.RemoveEmptyEntries));
}

The only way i can see on how to send many unique emails would be to loop over this list and then use the normal SendEmail method in mimekit:

string message = "Press this link to take the test";
int number = 0;

foreach(var email in emailList)
{
  //gets a unique number stored in a table for each person:
  number = GetNumber(email);
  message = $"http://www.mywebsite.com/Test/{number}";
  await _emailSender.SendEmailAsync(email, "Subject", message);
}

Now the emailSender uses the normal mimekit method to send one email for each in the list.

This takes a long time for a big list. Are there any other ways to do it, or can I write this more effectively? What could be the bottlenecks of my solution? Any feedback would be great, from my bad async code to security . I suspect that I use too much async when I should return only a task, but i'm not sure how to do it. I also wonder how I can even test this on a larger scale as I don't have a big emaillist to use.

Here is the mimekit code to send the email:

public async Task SendEmailAsync(string email, string subject, string message)
{
  var melding = new MimeMessage();
  melding.From.Add(new MailboxAddress("Frank", _options.Option1));
  melding.To.Add(new MailboxAddress(email));
  melding.Subject = subject;

  melding.Body = new TextPart("plain")
  {
     Text = message
  };
  Try
  {
     await SendMessage(melding);
  }
  catch(Exception ex)
  {
     //log error
     throw new ApplicationException($"something went wrong:'{ex.Message}'.");
  }
}
private async Task SendMessage(MimeMessage message)
{
   using (var client = new SmtpClient())
   {
      await client.ConnectAsync("smtp.WebSiteLive.net", 587, SecureSocketOptions.None);
      client.AuthenticationMechanisms.Remove("XOAUTH2"); // Must be removed for Gmail SMTP
      await client.AuthenticateAsync(_options.Option1, _options.Option2);
      await client.SendAsync(message);
      await client.DisconnectAsync(true);
    }
}

any help much apprechiated!!

using (var client = new SmtpClient())
   {
      await client.ConnectAsync("smtp.WebSiteLive.net", 587, SecureSocketOptions.None);
      client.AuthenticationMechanisms.Remove("XOAUTH2"); // Must be removed for Gmail SMTP
      await client.AuthenticateAsync(_options.Option1, _options.Option2);
      ...
      await client.DisconnectAsync(true);
    }

Look at how much work you're doing per message. If you reuse the client, you save setup and teardown time on every message after the first one.

If you reuse the client then you'll need to handle errors and be prepared to close it, open a new one, and continue where you left off; so there is a speed-complexity trade-off.


Implementation of a Resizing Array Queue Below is a problem from Sedgewick's Algorithms and my solution. Any thoughts or suggestions for improvement would be much appreciated.

  Develop a class that implements the queue abstraction with
  a fixed-sized array, and then extend your implementation to use array
  resizing.

package chapter_1_3_bagsQueuesStacks;

import java.util.Arrays;
import java.util.Iterator;

// Exercise 1.3.14 | pg. 163
public class ArrayQueue<E> implements Iterable<E> {
    private E[] a = (E[]) new Object[1];
    private int head;
    private int tail;
    private int N;

    public boolean isEmpty() {
        return N == 0;
    }

    private boolean isFull() {
        return N == a.length;
    }

    public int size() {
        return N;
    }

    private void resize(int cap) {
        E[] temp = (E[]) new Object[cap];
        int curr = head;
        for (int i = 0; i < N; i++) {
            temp[i] = a[curr];
            if (curr == a.length-1) {
                curr = 0;
            } else {
                curr++;
            }
        }
        a = temp;
    }

    public void enqueue(E element) {
        if (isFull()) {
            resize(a.length*2);
            head = 0;
            tail = N-1;
        }

        if (isEmpty()) {
            head = tail = 0;
        } else if (tail == a.length-1) {
            tail = 0;
        } else {
            tail++;
        }

        a[tail] = element;
        N++;

    }

    public E dequeue() {
        if (isEmpty())
            throw new RuntimeException();

        E element = a[head];
        a[head] = null;
        N--;

        if (head == a.length-1) {
            head = 0;
        } else {
            head++;
        }

        if (N == a.length/4) {
            resize(a.length/2);
            head = 0;
            tail = N-1;
        }

        return element;
    }

    @Override
    public Iterator<E> iterator() {
        return new ArrayIterator();
    }

    private class ArrayIterator implements Iterator<E> {

        int curr = head;

        @Override
        public boolean hasNext() {
            return a[curr] != null;
        }

        @Override
        public E next() {
            E element = a[curr++];
            return element;
        }

    }

    @Override
    public String toString() {
        String formatStr = "HEAD: %s - TAIL: %s - %s";
        return String.format(formatStr, this.head, this.tail, Arrays.toString(this.a));
    }

}

The code does not compile cleanly.  Use @SuppressWarnings("unchecked") if necessary.

$ javac -Xlint:unchecked ArrayQueue.java
ArrayQueue.java:8: warning: [unchecked] unchecked cast
    private E[] a = (E[]) new Object[1];
                          ^
  required: E[]
  found:    Object[]
  where E is a type-variable:
    E extends Object declared in class ArrayQueue
ArrayQueue.java:26: warning: [unchecked] unchecked cast
        E[] temp = (E[]) new Object[cap];
                         ^
  required: E[]
  found:    Object[]
  where E is a type-variable:
    E extends Object declared in class ArrayQueue
2 warnings

This test would crash:

public class ArrayQueueTest {
    public static void main(String[] args) {
        ArrayQueue<Integer> q = new ArrayQueue<>();
        q.enqueue(1);
        for (Integer n : q) { System.out.println(n); }
    }
}

1
Exception in thread "main" java.lang.ArrayIndexOutOfBoundsException: 1
  at ArrayQueue$ArrayIterator.hasNext(ArrayQueue.java:93)
  at ArrayQueueTest.main(ArrayQueueTest.java:5)


What might be another way to test if int is 32 bits wide at compile time? Below is code that will compile when INT_MAX is equal to 232. It will generate a compiler error when INT_MAX is not equal to 232.

#include <iostream>
#include <climits>

namespace NBitCheck
{
    template<bool> struct CTAssert;
    template<> struct CTAssert<true> {};

    constexpr bool intbitwidthcheck=INT_MAX-(2^32)+1;
    CTAssert<intbitwidthcheck> a;
}

int main(void) {

  return 0;
}

In C++, sizeof returns the size of a number relative to the size of char.

So, sizeof(int) should give you how many chars each int takes.

Next, we need to know how many bits each char has. CHAR_BIT has that information.

So try this:

constexpr size_t bits_per_int = sizeof(int) * CHAR_BIT;
constexpr bool intbitwidthcheck = (bits_per_int == 32);

Frankly, I believe this way is more readable.

Not to mention, can one reasonably expect a 32-bits signed int to contain values up to 2^32 - 1? Shouldn't that be the UINT_MAX? INT_MAX should be up to 2^31 - 1.


Ticker Tackor Soldier Spy This is a Tic Tac Toe game with a "new game" button. I'm after a general review of the code with some tips on how to make it simpler and tidier.

GitHub

HoboChess.java

package sample;

import javafx.event.ActionEvent;
import javafx.event.EventHandler;
import javafx.fxml.FXML;
import javafx.fxml.Initializable;
import javafx.scene.control.Button;
import javafx.scene.control.Label;
import javafx.scene.layout.GridPane;
import java.net.URL;
import java.util.ResourceBundle;

public class HoboChess implements Initializable {

    @FXML Label systemOut;
    @FXML GridPane gridPane;
    final static int ROWS = 3;
    final static int COLUMNS = 3;
    int[][] grid;
    boolean turn;

    @Override
    public void initialize(URL location, ResourceBundle resources) {
        grid = new int[COLUMNS][ROWS];
        newVisualField(ROWS, COLUMNS);
    }

    public void newGame(ActionEvent action) {
        grid = new int[COLUMNS][ROWS];
        newVisualField(ROWS, COLUMNS);
    }

    public void WhoWon(int x, int[] zone){
        if (zone[x] == 3){
            systemOut.setText("Os won!");
        }else if (zone[x] == 30){
            systemOut.setText("Xs won!");
        }
    }

    public void winCheck(){
        int[] zone = new int[COLUMNS * ROWS];
        int zoneNumber = 0;

        for (int y = 0; y < 3; y++) {
            for (int x = 0; x < 3; x++) {
                zone[zoneNumber] = grid[y][x];
                zoneNumber++;
            }
        }

        for (int x = 0; x < 3; x++) {
            if (zone[x * 3] == zone[x * 3 + 1] && zone[x * 3] == zone[x * 3 + 2]){
                WhoWon(x * 3, zone);
            }
        }
        for (int x = 0; x < 3; x++) {
            if (zone[x] == zone[x + 3] && zone[x] == zone[x + 6]){
                WhoWon(x, zone);
            }
        }
        if (zone[0] == zone[4] && zone[0] == zone[8]){
            WhoWon(0, zone);
        }else if (zone[2] == zone[4] && zone[2] == zone[6]){
            WhoWon(2, zone);
        }
    }

    public void newVisualField(int rows, int columns){
        gridPane.getChildren().clear();
        for (int y = 0; y < rows; y++) {
            for (int x = 0; x < columns; x++) {
                ButtonCell button = new ButtonCell(grid[x][y], x, y, this);
                gridPane.add(button.aButton, x, y);
            }
        }
    }
}

class ButtonCell {

    public Button aButton;
    boolean buttonPushed;

    ButtonCell(final int button, final int x, final int y, final HoboChess hoboChess) {
        aButton = new Button(String.valueOf(button));
        aButton.setOnAction(new EventHandler<ActionEvent>() {
            @Override
            public void handle(ActionEvent e) {
                if (buttonPushed == false) {
                    if (hoboChess.turn) {
                        hoboChess.grid[y][x] = 3;
                        aButton.setText("O");
                    } else {
                        hoboChess.grid[y][x] = 30;
                        aButton.setText("X");
                    }
                    hoboChess.turn = !hoboChess.turn;
                    buttonPushed = !buttonPushed;
                    hoboChess.winCheck();
                }
            }
        });
    }
}

All your fields can be declared private. There is really no need to expose these more than necessary.

@FXML private Label systemOut;
@FXML private GridPane gridPane;
private final static int ROWS = 3;
private final static int COLUMNS = 3;
private int[][] grid;
private boolean turn;

Your initialize and newGame methods looks very much the same to me, you could change your initialize to:

public void initialize(URL location, ResourceBundle resources) {
    newGame(null);
}

public void WhoWon(int x, int[] zone){
    if (zone[x] == 3){
        systemOut.setText("Os won!");
    }else if (zone[x] == 30){
        systemOut.setText("Xs won!");
    }
}

@barq is correct that the method name can be improved, and that passing a single int value to this method should be enough, which can make it:

public void declareWinner(int value) {
    if (value == 3) {
        systemOut.setText("Os won!");
    } else if (value == 30) {
        systemOut.setText("Xs won!");
    }
}

However, these magic numbers 3 and 30 really needs to be fixed. I'd recommend an enum:

public enum Player {
    X, O;
}

And then use a Player[][] instead of int[][]. and a Player turn instead of boolean.

Your win-check approach is a bit... more complicated than necessary. You begin with:

for (int y = 0; y < 3; y++) {
    for (int x = 0; x < 3; x++) {
        zone[zoneNumber] = grid[y][x];
        zoneNumber++;
    }
}

But in reality, there is no need to transform all the positions to a number, instead you can access the 2d array directly, like this:

for (int y = 0; y < 3; y++) {
    if (zone[0][y] == zone[1][y] && zone[0][y] == zone[2][y]) {
        declareWinner(zone[0][y]);
    }
}
for (int x = 0; x < 3; x++) {
    if (zone[x][0] == zone[x][1] && zone[x][0] == zone[x][2]) {
        declareWinner(zone[x][0]);
    }
}
if (zone[0][0] == zone[1][1] && zone[0][0] == zone[2][2]) {
    declareWinner(zone[0][0]);
} else if (zone[2][0] == zone[1][1] && zone[2][0] == zone[0][2]) {
    declareWinner(zone[2][0]);
}

In ButtonCell you are doing:

hoboChess.grid[y][x] = 3;

And violà, you have flipped x and y! In other places you are doing grid[x][y] but here we have grid[y][x]. Thanks to how Tic Tac Toe works, you won't see a difference, but it is still quite important to be consistent.

Also, when changing to using an enum this can become

hoboChess.grid[x][y] = Player.X;

or

hoboChess.grid[x][y] = Player.O;

Overall it does look like you have improved over time. A little additional improvement I would recommend is to separate the game board and logic more from your Java FX controller class.

I would use one file for TicTacToeGame, containing the Player[][] array and who's turn it is, then I would use a method Player makeMove(int x, int y) that would place a mark at the specified position and return which player made the move (for the button to know what mark to set). This game logic class can be made entirely independent of your JavaFX controller, so that it can be easily used in a Java console application, a Swing application, or another framework. This is an important step and skill in programming, to be able to create re-usable classes.


Playing audio from two data sources I want to play some audio of two byte arrays. I was wondering whether it would be better to play them as two separate arrays both calling the method, or to put them into one array and play it from there.

Note: The starting from position 4 because the first 4 bytes are saved for the sequence number in the array.

All in one array:

   byte[] das = ByteBuffer.allocate((temp.length-4)*2)
                        .put(Arrays.copyOfRange(temp, 4, temp.length - 4))
                        .put(Arrays.copyOfRange(udpPacketBytes, 4, udpPacketBytes.length - 4))
                        .array();
                player.playBlock(das);

or split like this:

  byte[] filteredByteArray = Arrays.copyOfRange(temp, 4, temp.length - 4);

                player.playBlock(filteredByteArray);

                // play the current one
                byte[] fba = Arrays.copyOfRange(udpPacketBytes, 4, udpPacketBytes.length - 4);

                player.playBlock(fba);

I was also wondering whether there'd be a way to calculate the complexity of the two.

Implementation
Better is a nebulous concept. The first one assumes that both arrays are the same length and fails to play all audio otherwise, so I'll say that's worse. I would suggest pulling the subarray computation into a method.
private void playAudio(final byte[] audio) {
    this.player.playBlock(Arrays.copyOfRange(audio, 4, audio.length - 4));
}


Simple Python + Gtk.Window I'm learning Python and Gtk so I decided to write a simple application that will take some image let user annotate with a pen and send results through email or save to a file.

So far I have the skeleton of the interface:

from gi.repository import Gtk
import os, sys

class GUI:

image = 0;

def __init__(self):

    self.window = Gtk.Window()
    self.window.set_title ("Imagine")
    self.window.connect_after('destroy', self.destroy)
    self.window.set_border_width(10)
    self.window.set_default_size(400, 200)

    self.window.vbox = Gtk.Box()
    self.window.vbox.set_spacing (5)
    self.window.vbox.set_orientation (Gtk.Orientation.VERTICAL)
    self.window.add (self.window.vbox)

    self.window.hbox = Gtk.ButtonBox()
    self.window.vbox.add(self.window.hbox)

    self.window.new_button = Gtk.Button(label="New")
    self.window.new_button.connect("clicked", self.on_new_clicked)
    self.window.hbox.pack_start(self.window.new_button, False, False, 0)

    self.window.save_button = Gtk.Button(label="Save")
    self.window.save_button.connect("clicked", self.on_save_clicked)
    self.window.hbox.pack_start(self.window.save_button, False, False, 0)

    self.window.email_button = Gtk.Button(label="eMail")
    self.window.email_button.connect("clicked", self.on_email_clicked)
    self.window.hbox.pack_start(self.window.email_button, False, False, 0)

    self.window.pen_button = Gtk.Button(label="Pen")
    self.window.pen_button.connect("clicked", self.on_pen_clicked)
    self.window.hbox.pack_start(self.window.pen_button, False, False, 0)

    self.image = Gtk.Image()
    self.window.vbox.pack_start (self.image, False, False, 0)

    self.window.show_all()

def on_save_clicked(self, widget):
    print("fixme: Save Clicked")

def on_email_clicked(self, widget):
    print("fixme: eMail Clicked")

def on_pen_clicked(self, widget):
    print("fixme: Pen Clicked")

def on_new_clicked (self, button):
    print("fixme: New Clicked")#code below is just to test the image panel works
    dialog = Gtk.FileChooserDialog ("Open Image", button.get_toplevel(), Gtk.FileChooserAction.OPEN);
    dialog.add_button ("Cancel", 0)
    dialog.add_button ("Ok", 1)
    dialog.set_default_response(1)

    filefilter = Gtk.FileFilter ()
    filefilter.add_pixbuf_formats ()
    dialog.set_filter(filefilter)

    if dialog.run() == 1:
        self.image.set_from_file(dialog.get_filename())

    dialog.destroy()

def destroy(self, window):
    Gtk.main_quit()

class Controls:

    def send_email (self):
        print("fixme: Send Email")

    def pen_controls (self):
        print("fixme: Pen Controls")

    def on_save_clicked(self, widget):
        print("fixme: Save Clicked")

def main () :
    win = GUI()
    Gtk.main()

if __name__ == "__main__":
    sys.exit(main())

Can you please review my code and let me know what could have been done better? Any documentation along will be highly appreciated.

Also, my main doubts are:

Is my code compliant with the object oriented Python?
Will vbox and hbox exist in two places in my window because I created them in self.window and then added to the window again with self.window.add?
Should I have created vbox and hbox outside of window and then used the window.add command?
The idea is to use Gtk3 PyGObject and Python 3

I don't think your considerations are a big problem.

However what I think is a bigger problem is the amount of verbosity and repetition in your code. While the code is readable as is, adding more elements in the future will make your __init__() method inflate and become unreadable.

This is because you are adding each Element individually. In large applications this is largely unfeasible when you have more than a dozen widgets, for reasons I am sure you can understand.

There are a couple of solutions to this:

Generate Layout using code

instead of doing this:

self.window.new_button = Gtk.Button(label="New")
self.window.new_button.connect("clicked", self.on_new_clicked)
self.window.hbox.pack_start(self.window.new_button, False, False, 0)

# ... repeat for all buttons

you can do this

buttons = [("New", self.on_new_clicked),
           ("Save", self.on_save_clicked),
           ("eMail", self.on_email_clicked),
           ("Pen", self.on_pen_clicked),
          ]

for name, callback in buttons:
    b = Gtk.Button(label=button)
    b.connect("clicked", callback)
    self.window.hbox.pack_start(b, False, False, 0)

Subclass standard widgets

Often you can also clean up your application by subclassing other widgets:

class myButtonBox(Gtk.ButtonBox):
    def __init__(self, *args, **kwargs):

        # add all buttons to self here

        super().__init__(*args, **kwargs)
        # note: super() without args works only in new versions of Python

Seperate Interface and logic by using GtkBuilder xml

I am mentioning this last since this is the best option, but not very frequently worth the effort.

The Gtk 3 Tutorial has a very nice page on GtkBuilder

  The Gtk.Builder class offers you the opportunity to design user
  interfaces without writing a single line of code. This is possible
  through describing the interface by a XML file and then loading the
  XML description at runtime and create the objects automatically, which
  the Builder class does for you. For the purpose of not needing to
  write the XML manually the Glade application lets you create the user
  interface in a WYSIWYG (what you see is what you get) manner

  This method has several advantages:

  Less code needs to be written.
  UI changes can be seen more quickly, so UIs are able to improve.
  Designers without programming skills can create and edit UIs.
  The description of the user interface is independent from the programming language being used.

  There is still code required for handling interface changes triggered by the user,
  but Gtk.Builder allows you to focus on implementing that
  functionality.


Optimizing a script that switches function arguments in code files I made a quick script, that inputs file paths(made for C files), function names and 2 indexes. The script goes over all of the files and switches the indexes of the 2 functions. e.g.:
Running the following code:
python switch_args.py temp_funct 0 1 tmp.c

If tmp.c contains this code:
int my_func(a, b, c, d)

after the execution of the script it would have this code:
int my_func(b, a, c, d)

Code:
https://github.com/martin-varbanov96/toolboxz/blob/master/switch_function_args/switch_args.py:
import argparse
import re

parser = argparse.ArgumentParser()
parser.add_argument("function_name", type=str,
                    help="function name whose arguments will be switched")
parser.add_argument("index", type=int,
                    help="first argument to be switched")
parser.add_argument("destination", type=int,
                    help="second argument to be switched")
parser.add_argument('files', metavar='files', type=str, nargs='+',
                    help='files in which function arguments will be switched')

args = parser.parse_args()
def switch_function_args(func_name, args, index=0, destination=1, ):
    function_call_pattern = "{}[^\)]+".format(func_name)
    for file in args:
        has_function = False
        file_content = ""
        with open(file, "r") as f:
            file_content = f.read()
            if(func_name in file_content):
                has_function = True
        if(has_function):
            for current_function_structure in re.findall(function_call_pattern, file_content):
                current_arguments = re.split("\(", current_function_structure)[1]
                current_arguments = re.split(",", current_arguments)

                # switch arguments
                current_arguments[index], current_arguments[destination] = current_arguments[destination], current_arguments[index]
                fixed_arguments = ",".join(current_arguments)
                fixed_function_structure = "{}({}".format(func_name, fixed_arguments)
                old_struct_pattern = current_function_structure.replace("(", "\(")
                file_content = re.sub(old_struct_pattern, fixed_function_structure, file_content)
            with open(file, "w") as f:
                f.write(file_content)

switch_function_args(args.function_name, args.files, index=args.index, destination=args.destination)

Question:
How can this code be optimized for performance?
EDIT:
what is the consistency of the following code and is it a good idea to ignore it?
with open(file, "r") as f_read:
    # code
    with open(file, "w") as f_write:
        # code

General
C function arguments are only passed by position and not by name. As such, any attempt to make this modify real code is likely to break callers, and it's possible for that breakage to be silent during compile-time if the exchanged parameters have the same type. That makes this kind of refactoring deeply ill-advised. You're better off using the built-in signature refactoring feature of (pick any grown-up IDE).
Early continuation
Consider rewriting
        if(func_name in file_content):
            has_function = True

as
if func_name not in file_content:
    continue

Note that the parens have been dropped; you don't need a flag variable; and you can de-indent the rest of the loop.
Fragility
Before asking

How can this code be optimized for performance?

you need to make it correct, and it definitely isn't right now for all cases. Functions in C are easily (and fairly often) aliased by pointer, and these aliases will not be modified if the pointer doesn't have the same name as the original function. Following such aliases would only reasonably be done by calling into an existing C parser like Clang/LLVM, which you should probably be doing anyway for robustness.


Find the highest frequency of messages inside a time slot of 24 hours I have a very large number (millions) of messages, each labeled with the unixtime it was sent in a SQLite database. Each message has its own userid, for the user that have sent it. I want to know what's the max number of messages that is sent within an 24hr timeslot for each user. The 24hr timeslot is defined as the time from one message to another. So if there are five messages, where the 5th one is sent 24 hours after the first one, 5 is the number I want.

I have code that gives me this frequency, but the problem is that the running time of this is just too large - and I guess thats partly because of my not-optimal code, and too high complexity. How can I optimize this?

    con = lite.connect(databasepath)
    userID = []
    messages = []
    messageFrequency = []
    with con:
        cur = con.cursor()
        #Get all UserID
        cur.execute('SELECT DISTINCT userid FROM MessageType1')
        userID = cur.fetchall()
        userID = [x[0] for x in userID]
        #For each UserID
        for user in userID:
            messageFrequency.append(0)
            #Get all MSG with UserID = UserID sorted by UNIXTIME
            cur.execute('SELECT unixtime FROM MessageType1 WHERE userID ='+str(user)+' ORDER BY unixtime asc')
            Messages = cur.fetchall()
            Messages = [x[0] for x in Messages]
            length = len(Messages)
            #Loop through every MSG
            for message in Messages:
                index = Messages.index(message)
                nextmessage = Messages[index+1]
                frequency = 0
                #Loop through every message that is within 24 hours
                while nextmessage < message+(24*60*60) and index<length-1:
                    #Count the number of occurences
                    frequency += 1
                    index += 1
                    nextmessage = Messages[index]
                #Add best benchmark for every message to a list
                if messageFrequency[-1]<frequency:
                    messageFrequency[-1] = frequency

First of all: python has not native array.

In other words Message is a list and every time you type Message[n] you will pay O(n) computational cost to execute it.

The worst case analysis of your code computational complexity is O(n^3) where n is total number of messages.

Avoid the use of list index in python for cycle and use iterators or dictionary instead. In your case you scan the list item per item so my first try is to use iterator that should give to you an O(n) computational complexity.

Now after took in account that use lists as array in python is the best way to increase computational complexity, we can take a look to the alternatives:

Use list as list by and rewrite the code by use Iterators and itertools
Use dictionary instead lists
Use numpy.array instead lists

numpy.array maybe is little bit overkill approach; in this case iterators are always the best because in your algorithm you scan the message as list, but use dictionary can be implemented without change your algorithm.

The Dictionary way

First of all replace Messages list by a dictionary where keys are the list index. To do it just replace

Messages = [x[0] for x in Messages]

by

Messages = {index:x[0] for index,x in enumerate(Messages)}

Messages now is a dictionary where access to Messages[k] have an amortized constant cost. So the same syntax that you used to get elements from your list now can be used on dictionary but its cost doesn't depend from the number of messages at all.

The only thing that we must fix now is the cycle that should follow index order (key array are not ordered for standard dictionary). To do it replace the list iteration for message in Messages by a cycle for each index from 0 to len(Messages) and message variable become Messages[index]. In other words instead of for message in Messages: write

for index in range(len(Messages)):
    message = Messages[index]

By that simple trick you algorithm become O(n)... Is it magic? No. Dictionary are amortized O(1) for access, inserting and delete operation and in python are very well optimized because almost everything in python is based on dictionary.

What do you pay by use dictionary is memory and constants. So your next step could be rewrite it by use iterator but just dictionary should decrease execution time drastically.

Use iterator means change your algorithm deeply and maybe you don't really need it even I think it could be a very good exercise.


Implementing Todo in Haskell Per Learn You a Haskell, I implemented a simple command-line program, Todo, that accepts 1 of 3 arguments: view or add or delete.

Please review my code for Haskell idioms & overall quality.

-- view, add and delete tasks
-- borrows delete from previous LYAH exercise
import System.Environment
import System.Directory
import System.IO
import Data.List

data Action = Add | View | Delete

main = do
    args <- getArgs
    progName <- getProgName
    let maybeAction = validArgs args
    executeTodo maybeAction

executeTodo :: (Maybe Action) -> IO ()
executeTodo (Just Add)    = addTodo
executeTodo (Just View)   = viewTodo
executeTodo (Just Delete) = deleteTodo
executeTodo Nothing       = print "invalid user input"

viewTodo :: IO ()
viewTodo = do
    withFile "todo_file.txt" ReadMode (\handle -> do
        contents <- hGetContents handle
        putStr contents)
    return ()

addTodo :: IO ()
addTodo = do
   todoItem <- getLine
   appendFile "todo_file.txt" (todoItem ++ "\n")

deleteTodo :: IO ()
deleteTodo = do
    handle <- openFile "todo_file.txt" ReadMode
    (tempName, tempHandle) <- openTempFile "." "temp"
    contents <- hGetContents handle
    let todoTasks = lines contents
        numberedTasks = zipWith (\n line -> show n ++ " - " ++ line) [0..] todoTasks
    putStrLn "These are your TO-DO items:"
    putStr $ unlines numberedTasks
    putStrLn "Which one do you want to delete?"
    numberString <- getLine
    let number = read numberString
        newTodoItems = delete (todoTasks !! number) todoTasks
    hPutStr tempHandle $ unlines newTodoItems
    hClose handle
    hClose tempHandle
    removeFile "todo_file.txt"
    renameFile tempName "todo_file.txt"

validArgs :: [String] -> Maybe Action
validArgs (x:[]) = isValidAction x
validArgs _      = Nothing

isValidAction :: String -> Maybe Action
isValidAction x
  | x == "view"   = Just View
  | x == "add"    = Just Add
  | x == "delete" = Just Delete
  | otherwise     = Nothing

It looks good overall.

I see two possible problems.

Race condition in deleteTodo

Running the program twice at the same time you might lose one change because reading the old file, writing the new file and renaming is not atomic.

You can avoid that problem when using a "tododir" with one single file per item. It will also scale better with the number of todo items. Note, that that's very similar to why maildir is superior to mbox. See the rationale at djb's site.

Error handling is missing

You don't catch exceptions when doing file operations and use the partial read. For that simple interactive program I would not catch file operation errors but would surely use readMaybe.