User registration and login program I've recently stared learning C++ so I'm new to all of it. I've got my fourth "software" under development. Could someone have a look through and give me some constructive criticism on what should I change or keep the same?

#include <iostream>
#include <string>
#include <stdlib.h>
#include <fstream>

using namespace std;

void mainmenu();

int choice;
bool cinfail;
int confirmation;
string username, password, password2;

void writetofile(string username){
    ofstream writefile;
    string file = username+".txt";
    writefile.open(file.c_str());
    writefile << password;
    writefile.close();
    mainmenu(); }

void login(){
    cout << "You are being logged in!";}

void registerpassword(){
    cout << "Please enter the password:" << endl;
    cin >> password;
    cout << "Please renter your password:" << endl;
    cin >> password2;
    if (password == password2){
        cin.clear();
        cin.ignore(10000,'\n');
        writetofile(username);
        exit(1);
    }
    else;{
        cout << "Sorry invalid" << endl;
        registerpassword();
    }}

void registerme(){
    cout << "Please enter your username: " << endl;
    getline(cin, username);
    cout << "\nUsername -  \""<< username << "\"\nConfirm? \n\n[1] Yes\n[2] No" << endl;
    cin >> confirmation;
    if (confirmation == 1){
        registerpassword();
    }

    else; {
        cout << "Sorry invalid input, Please try again" << endl;
        cin.clear();
        cin.ignore(10000,'\n');
        registerme();
    }}

void exit(){
    exit(0);}

void mainmenu(){ cout << "Hello, Would you like to log in or register\n[1] Login\n[2] Register\n[3] Exit" <<endl; cin >> choice; do{
    cinfail = cin.fail();
    cin.clear();
    cin.ignore(10000,'\n');

    }while(cinfail == true);{
        switch(choice){
            case 1:
                login();
                break;

            case 2:
                registerme();
                break;

            case 3:
                exit();}}}

main(){
mainmenu();
}

Use C++ headers instead of C headers. Change #include <stdlib.h> to #include <cstdlib>. You could actually remove this header altogether because you really should not be calling exit() in this program.
Avoid using namespace std;. You are polluting the global namespace and increasing the probability of naming clashes. If you put using namespace std; into a header, you pollute the global namespace of every user that uses your header. Putting it into a CPP file is not as bad as putting it into a header, but the same dangers exist. See this link for more information.
Avoid using global variables when they are not necessary. You want to limit the scope of your variables as much as possible. This program can be rewritten to use no global variable at all.
Use appropriate function names. mainmenu() should be either main_menu() or MainMenu(), writetofile() should be either write_to_file() or WriteToFile(), etc.
Get rid of the unnecessary recursion. Recursion is very nice an clean in some situations. Unfortunately, it's very ugly in this situation. This program can be rewritten to use no recursion at all.
Learn about RAII. It is one of the most important aspects of the C++ language. In your writetofile function, you manually call writefile.close();. You do not have to do this. The std::ofstream will automatically clean up its own resources once it goes out of scope. This is one of the reasons why limiting the scope of your variables is important.
Learn about std::numeric_limits. While cin.ignore(10000,'\n'); should work in most cases, std::cin.ignore(std::numeric_limits <std::streamsize>::max(), '\n'); will work in more cases.

Now with that out of the way, let's see how we can rewrite this program.

Headers

#include <fstream>
#include <iostream>
#include <limits> // for std::numeric_limits
#include <string>

Create a function to get user input

We want to get rid of the ugly recursion that we have going on. One thing we can do is create a helper function that can read user input for various values. This is a function template. It can return ints, std::strings, and other variable types. If bad input is given to this function, then it asks the user to try again.

template <typename T>
T get_input(const std::string &strQuery)
{
    std::cout << strQuery << "\n> ";
    T out = T();

    while (!(std::cin >> out)) {
        std::cin.clear();
        std::cin.ignore(std::numeric_limits <std::streamsize>::max(), '\n');
        std::cout << "Error!" "\n";
        std::cout << strQuery << "\n> ";
    }

    return out;
}

Rewrite the mainmenu() function

First we will rename mainmenu() to main_menu(). Second we will use our new get_input() function. Notice that in C++, two string literals next to each other (such as "[1] Login" "\n") are automatically concatenated into one string literal by the compiler. Even if the string literals are on separate lines, as long as there are no semicolons or anything else between them, then the compiler will concatenate them. Using this strategy make your main_menu() class easier to read. Notice that we only need two switch cases. All other cases result in nothing being done.

void main_menu()
{
    int choice = get_input <int>(
        "Hello, Would you like to log in or register?" "\n"
        "[1] Login" "\n"
        "[2] Register" "\n"
        "[3] Exit");

    switch (choice)
    {
    case 1:
        login();
        break;
    case 2:
        register_user();
        break;
    }
}

Rewrite the login() function

This is a very small change. I add a newline to it.

void login()
{
    std::cout << "You are being logged in!" "\n";
}

Rewrite the registerme() function: Part 1

First, let's give it a better name. Let's rename this function to register_user(). This function does 3 things. It:

Gets a username.
Gets a password.
Saves the password to a file.

To make this function more readable, we will create 3 helper functions to perform each of these tasks.

Get the username

We will create a helper function that will get the username. We will use get_input() to avoid all of the ugly recursion that you have.

std::string get_username()
{
    std::string username = get_input <std::string>("Please enter a username.");
    std::cout << "Username: \"" << username << "\"\n";

    while (get_input <int>("Confirm? [0|1]") != 1) {
        username = get_input <std::string>("Please enter a username.");
        std::cout << "Username: \"" << username << "\"\n";
    }

    return username;
}

Get the password

We just have to make sure the passwords match. Once again, the get_input() function makes this clean.

std::string get_password()
{
    std::string password1 = get_input <std::string> ("Please enter your password.");
    std::string password2 = get_input <std::string> ("Please re-enter your password.");

    while (password1 != password2) {
        std::cout << "Error! Passwords do not match." "\n";
        password1 = get_input <std::string>("Please enter your password.");
        password2 = get_input <std::string>("Please re-enter your password.");
    }

    return password1;
}

Save the username and password

Now we need a function that can save the username and password. This one is pretty similar to your writetofile() function, except it is easier to read. It does require passing both the username and the password though.

void save_user(const std::string &username, const std::string &password)
{
    std::string filename = username + ".txt";
    std::ofstream file(filename);
    file << password << "\n";
}

Rewrite the registerme() function: Part 2

Now with those 3 helper functions implemented, we can write the new registerme() function (which we renamed register_user()). This turns your complicated function into a simple function that is easy to read.

void register_user()
{
    std::string username = get_username();
    std::string password = get_password();
    save_user(username, password);
}

Rewrite the main() function

Your main() function stays pretty much the same. Just call main_menu() instead of mainmenu() since we renamed the function.

All of it together

#include <fstream>
#include <iostream>
#include <limits>
#include <string>

template <typename T>
T get_input(const std::string &strQuery);

std::string get_username();
std::string get_password();
void save_user(const std::string &username, const std::string &password);

void login();
void register_user();
void main_menu();

int main()
{
    main_menu();
}

template <typename T>
T get_input(const std::string &strQuery)
{
    std::cout << strQuery << "\n> ";
    T out = T();

    while (!(std::cin >> out)) {
        std::cin.clear();
        std::cin.ignore(std::numeric_limits <std::streamsize>::max(), '\n');
        std::cout << "Error!" "\n";
        std::cout << strQuery << "\n> ";
    }

    return out;
}

std::string get_password()
{
    std::string password1 = get_input <std::string> ("Please enter your password.");
    std::string password2 = get_input <std::string> ("Please re-enter your password.");

    while (password1 != password2) {
        std::cout << "Error! Passwords do not match." "\n";
        password1 = get_input <std::string>("Please enter your password.");
        password2 = get_input <std::string>("Please re-enter your password.");
    }

    return password1;
}

std::string get_username()
{
    std::string username = get_input <std::string>("Please enter a username.");
    std::cout << "Username: \"" << username << "\"\n";

    while (get_input <int>("Confirm? [0|1]") != 1) {
        username = get_input <std::string>("Please enter a username.");
        std::cout << "Username: \"" << username << "\"\n";
    }

    return username;
}

void login()
{
    std::cout << "You are being logged in!" "\n";
}

void main_menu()
{
    int choice = get_input <int>(
        "Hello, Would you like to log in or register?" "\n"
        "[1] Login" "\n"
        "[2] Register" "\n"
        "[3] Exit");

    switch (choice)
    {
    case 1:
        login();
        break;
    case 2:
        register_user();
        break;
    }
}

void register_user()
{
    std::string username = get_username();
    std::string password = get_password();
    save_user(username, password);
}

void save_user(const std::string &username, const std::string &password)
{
    std::string filename = username + ".txt";
    std::ofstream file(filename);
    file << password << "\n";
}


Filtering values for GridViewItems I have this function in my application that I need to simplify in order to improve the code quality:

void FilterValues()
    {
        List<SecondaryStockRoomDefenition> GridViewItems = new List<SecondaryStockRoomDefenition>();
        GridViewItems = StockRooms.ToList();

        if (cboPrimaryStockRoom.SelectedValue.ToString().Trim() != "0")
        {
            GridViewItems =
           StockRooms.Where((SecondaryStockRoomDefenition, index)
               => GridViewItems[index].DeletedSecondaryStockRoom == false
                  && GridViewItems[index].PrimaryStockroomCode == cboPrimaryStockRoom.SelectedValue.ToString().Trim()).ToList();
        }
        if( txtSecondaryStockRoomCode.Text != "" )
        {
            GridViewItems = GridViewItems.Where((SecondaryStockRoomDefenition,index) =>
                GridViewItems[index].DeletedSecondaryStockRoom == false
                && GridViewItems[index].SecondaryStockroomCode == txtSecondaryStockRoomCode.Text.Trim()).ToList();
        }
        if (txtSecondaryStockRoomName.Text != "")
        {
            GridViewItems = GridViewItems.Where((SecondaryStockRoomDefenition, index) =>
                GridViewItems[index].DeletedSecondaryStockRoom == false
                && GridViewItems[index].SecondaryStockroomDescription.Contains(txtSecondaryStockRoomName.Text.Trim())).ToList();
        }
        if (txtStockRoomLocationCode.Text != "")
        {
            GridViewItems = GridViewItems.Where((SecondaryStockRoomDefenition, index) =>
                GridViewItems[index].DeletedSecondaryStockRoom == false
                && GridViewItems[index].SecondaryStockroomLocationCode == txtStockRoomLocationCode.Text.Trim()).ToList();
        }
        if (cboStockRoomCategory.SelectedValue.ToString().Trim() != "0")
        {
            GridViewItems = GridViewItems.Where((SecondaryStockRoomDefenition, index) =>
                GridViewItems[index].DeletedSecondaryStockRoom == false
                && GridViewItems[index].SecondaryStockroomCategory == cboStockRoomCategory.SelectedValue.ToString().Trim()).ToList();
        }

        if (txtStockRoomType.Text != "")
        {
            GridViewItems = GridViewItems.Where((SecondaryStockRoomDefenition, index) =>
                GridViewItems[index].DeletedSecondaryStockRoom == false
                && GridViewItems[index].SecondaryStockroomType == txtStockRoomType.Text.Trim()).ToList();
        }
        if (cboExpectedUserName.SelectedValue.ToString().Trim() != "0")
        {
            GridViewItems = GridViewItems.Where((SecondaryStockRoomDefenition, index) =>
                GridViewItems[index].DeletedSecondaryStockRoom == false
                && GridViewItems[index].ExpectedUserNameForCommissionEntry == cboExpectedUserName.SelectedValue.ToString().Trim()).ToList();
        }

        dgvCompanyProfileDetails.DataSource = null;
        dgvCompanyProfileDetails.Rows.Clear();
        dgvCompanyProfileDetails.DataSource = GridViewItems;
    }

Just a few things from me:

Check the way you name variables.  Local variables by defacto standard are lower case camel.  See here for more details Microsoft naming conventions.

GridViewItems becomes gridViewItems
I personally try to use implicit local variables where the type is obvious.  This is more a personal preference but Microsoft do recommend as well.  See C# coding conventions for more details.
If gridViewItems is going to be assigned to stockrooms immediately, why not just do straight off the cuff?

var gridViewItems = StockRooms.ToList();

I would convert this method into a bunch of smaller functions.  Perhaps something like (excuse any compile options as I could not fully test this code):

void FilterValues()
{
    dgvCompanyProfileDetails.DataSource = null;
    dgvCompanyProfileDetails.Rows.Clear();
    dgvCompanyProfileDetails.DataSource = Filter(StockRooms).ToList();
}

private IEnumerable<SecondaryStockRoomDefenition> Filter(IEnumerable<SecondaryStockRoomDefenition> stockRooms)
{
    // filter the stock rooms on each function call
    stockRooms = FilterStockRooms(stockRooms, cboPrimaryStockRoom, p => p.PrimaryStockroomCode);
    stockRooms = FilterStockRooms(stockRooms, txtSecondaryStockRoomCode.Text, p => p.SecondaryStockroomCode);
    stockRooms = FilterStockRooms(stockRooms, txtSecondaryStockRoomName.Text, (p, v) => p.SecondaryStockroomDescription.Contains(v));
    stockRooms = FilterStockRooms(stockRooms, txtStockRoomLocationCode.Text, p => p.SecondaryStockroomLocationCode);
    // .. etc

    return stockRooms;
}

private IEnumerable<SecondaryStockRoomDefenition> FilterStockRooms(IEnumerable<SecondaryStockRoomDefenition> rooms, ComboBox combo, Func<SecondaryStockRoomDefenition, string> filter)
{
    var value = combo.SelectedValue.ToString();

    return HasValue(value, "0") ? FilterStockRooms(rooms, v => filter(v).Equals(value, StringComparison.InvariantCultureIgnoreCase)) : rooms;
}

private IEnumerable<SecondaryStockRoomDefenition> FilterStockRooms(IEnumerable<SecondaryStockRoomDefenition> rooms, string text, Func<SecondaryStockRoomDefenition, string> filter)
{
    return HasValue(text) ? FilterStockRooms(rooms, v => filter(v).Equals(text, StringComparison.InvariantCultureIgnoreCase)) : rooms;
}

private IEnumerable<SecondaryStockRoomDefenition> FilterStockRooms(IEnumerable<SecondaryStockRoomDefenition> rooms, string text, Func<SecondaryStockRoomDefenition, string, bool> filter)
{
    return HasValue(text) ? FilterStockRooms(rooms, v => filter(v, text)) : rooms;
}

// The question marks are because I wasn't sure of the StockRooms type
private IEnumerable<SecondaryStockRoomDefenition> FilterStockRooms(IEnumerable<SecondaryStockRoomDefenition> rooms, Func<SecondaryStockRoomDefenition, bool> filter)
{
    return rooms.Where(p => p.DeletedSecondaryStockRoom == false && filter(p));
}

private bool HasValue(string value)
{
    return !string.IsNullOrWhiteSpace(value);
}

private bool HasValue(string value, string compareTo)
{
    return HasValue(value) && value.Trim() != compareTo;
}


Project Euler #12 in C++ - highly divisible triangular number Project Euler Problem 12 asks (paraphrased):

  The nth triangular number Tn = 1 + 2 + … + n.  T7 = 28 has 6 divisors (1, 2, 4, 7, 14, 28).  What is the first Tn to have over 500 divisors?

After asking for some help on Stack Overflow, I've managed to write this code to solve it:

#include "stdafx.h"
#include <iostream>
#include <map>
//#include <algorithm>
#include <numeric>

int divisors (unsigned int num, std::map<int, int>& map)
{
    int orig_num = num;
    for(unsigned int i = 2; i <= num; ++i)
    {
        while (num % i == 0)
        {
            num /= i;
            ++map[i];
            //std::cout << map[i] << "\t";
        }
    }
    std::cout << orig_num << " = ";
    for(auto& iter:map)
        std::cout << iter.first <<  "^" << iter.second << " * ";
    return 0;
}

int overall_factors(unsigned int n)
{
    std::map <int, int> primefactors1, primefactors2;
    int no_of_divisors = 1;
    divisors(n,primefactors1);
    std::cout << std::endl;
    divisors(n+1,primefactors2);
    std::cout << std::endl;
    std::map<int, int> primefactors_triangle = std::accumulate( primefactors1.begin(), primefactors1.end(), std::map<int, int>(),
        []( std::map<int, int> &m, const std::pair<const int, int> &p )
    {
        return ( m[p.first] +=p.second, m );
    } );

    primefactors_triangle = std::accumulate( primefactors2.begin(), primefactors2.end(), primefactors_triangle,
        []( std::map<int, int> &m, const std::pair<const int, int> &p )
    {
        return ( m[p.first] +=p.second, m );
    } );

    for ( const auto &p : primefactors_triangle )
    {
        std::cout << "{ " << p.first << ", " << p.second << " } ";
        if (p.first == 2)
            no_of_divisors *= p.second;
        else
        no_of_divisors *= p.second+1;
    }

    std::cout << std::endl;
    return no_of_divisors;
}

int main()
{
    int i = 2;
    while (overall_factors(i) < 500)
    {
        std::cout << overall_factors(i) << std::endl;
        ++i;
    }
    return 0;
}

There're some pieces left that output debugging info to see that everything is working as intended. But overall, it doesn't work as fast as I want it to - or as fast as I expect a proper solution to a Project Euler problem to work.

How can this code be improved?  Clearly the program, as it is, now calculates primefactors for a given number twice.  What else is wrong and could be improved?

UPD: After I changed the while loop condition, the problem was solved rather quickly. Still, I believe my code is improvable.

This:

int i = 2;
while (overall_factors(i) < 500)
{
    std::cout << overall_factors(i) << std::endl;
    ++i;
}

can just be a for-loop:

for (int i = 2; overall_factors(i) < 500; ++i)
{
    std::cout << overall_factors(i) << std::endl;
}

I'd also recommend using a constant for 500 as it is otherwise a magic number.  This will make it clear as to what this value represents.

If the number isn't significant to the general algorithm, then a comment can be added instead.
There is no need for divisors to just return a 0.  It appears it's just modifying the map and displaying it.  Also, these are two different things, and a function should just have one role.  In this case, it should just modify the map.  You should then have a separate void function for displaying the map in the desired format.

Although passing the map by reference is okay here, passing it by value (in C++11) will also allow the compiler to decide to perform a move, if desired over a copy.


Simple Haskell calculator, using Maybe for error handling I'm reading through Write Yourself a Scheme after finishing going through Learn You a Haskell. I attempted one of the early exercises: writing a program to get an operator and two numbers and do a computation. It works fine.

Things I would like to know:

How should I structure a program, in terms of building larger functions out of smaller functions? Are there redundancies in my code?

What's the most effective way to use the Maybe type to indicate failure when main is of type IO ()? Is my checkSuccess an appropriate way to do this?

module Main where

import System.Environment

-- parses the first arithmetic operator in a string
parseOperator :: String -> Maybe Char
parseOperator [] = Nothing
parseOperator (x:xs)
    | x == '*' = Just '*'
    | x == '/' = Just '/'
    | x == '+' = Just '+'
    | x == '-' = Just '-'
    | otherwise = parseOperator xs

parseNum :: String -> Maybe Double
parseNum x =
    let parsed = reads x :: [(Double,String)]
    in case parsed of
        [(a,"")] -> Just a
        [(_,_)] -> Nothing
        [] -> Nothing

compute :: Maybe Char -> Maybe Double -> Maybe Double -> Maybe Double
compute Nothing _ _ = Nothing
compute _ Nothing _ = Nothing
compute _ _ Nothing = Nothing
compute (Just c) (Just x) (Just y)
    | c == '*' = Just $ x * y
    | c == '/' = Just $ x / y
    | c == '+' = Just $ x + y
    | c == '-' = Just $ x - y

checkSuccess :: Maybe Double -> IO ()
checkSuccess Nothing = putStrLn "Failed. Check correctness of inputs"
checkSuccess (Just r) = putStrLn $ "Result: " ++ (show r)

runSequence :: String -> String -> String -> IO ()
runSequence os xs ys =
    checkSuccess $ compute (parseOperator os) (parseNum xs) (parseNum ys)

main = do
    putStrLn "Enter operator: * / + -"
    operator <- getLine

    putStrLn "Enter first number"
    first <- getLine

    putStrLn "Enter second number"
    second <- getLine

    runSequence operator first second

I'd further suggest to split compute into two parts:

Getting the right operation from a given character:

op :: Char -> Maybe (Double -> Double -> Double)
op '*' = Just (*)
op '/' = Just (/)
op '+' = Just (+)
op '-' = Just (-)
op _   = Nothing

This accepts a character, and either produces a function corresponding to the character, or Nothing.
Applying the operation on values, taking into account all possible Maybes:

compute' :: Maybe Char -> Maybe Double -> Maybe Double -> Maybe Double
compute' c x y = (c >>= op) <*> x <*> y

Here c >>= op results into Maybe (Double -> Double -> Double), where the result is Nothing if the character is Nothing or if it doesn't match any supported operation. And then we use twice <*> from Control.Applicative, which, specialized for Maybe, applies Maybe (a -> b) to Maybe a yelding Maybe b.


Checking that a string matches one regex and doesn't match another I'm using pylint and flake8 to control the code quality. However I am in check-mate position, where styling one does not suit the pylint or flake8:

if (not re.search(regexp, summary) and
    not re.search(regexp2, summary)):
    return False

Output from flake8:

visually indented line with same indent as next logical line [E129]

Moving return is not helping, the previous error still occurs and there is new one thrown by pylint:

if (not re.search(regexp, summary) and
    not re.search(regexp2, summary)):
        return False

Output from pylint:

[bad-indentation] Bad indentation. Found 8 spaces, expected 4

Per PEP-8, you can ("Acceptable options in this situation include, but are not limited to...") further indent the second line of the condition:

# Add some extra indentation on the conditional continuation line.
if (this_is_one_thing
        and that_is_another_thing):
    do_something()

The difference is that indentation in the continuation lines is purely stylistic, whereas indentation of the next code block has semantic meaning. In your case:

if (not re.search(regexp, summary)
        and not re.search(regexp2, summary)):
    return False

Note I have moved the and per the same guidance.


Database Dynamic Table Creation The code below is meant to build an array with certain properties neccesairy to create column for a table in a database.
Would there be any more effcient way of doing this?

Public fieldName As String
Public fieldType As ADOX.DataTypeEnum
Public fieldProperties As String
Public fieldPropertiesValue
Public fieldIndex As Boolean
Public fieldUnique As Boolean
Public fieldKey As Boolean

Public Sub New(ByVal fieldName As String, ByVal fieldType As ADOX.DataTypeEnum, Optional ByVal fieldProperties As String = Nothing,
               Optional ByVal fieldPropertiesValue As Object = Nothing, Optional ByVal fieldIndex As Boolean = False,
               Optional ByVal fieldUnique As Boolean = False, Optional ByVal fieldKey As Boolean = False)

    Me.fieldName = fieldName
    Me.fieldType = fieldType
    Me.fieldProperties = fieldProperties
    Me.fieldPropertiesValue = fieldPropertiesValue
    Me.fieldIndex = fieldIndex
    Me.fieldUnique = fieldUnique
    Me.fieldKey = fieldKey

End Sub

Public Shared IDOwner As classObjectField = New classObjectField("IDOwner", ADOX.DataTypeEnum.adInteger, "AutoIncrement", True, True, True, True)
Public Shared OwnerAccount As classObjectField = New classObjectField("OwnerAccount", ADOX.DataTypeEnum.adVarWChar, fieldIndex:=True)
Public Shared OwnerName As classObjectField = New classObjectField("OwnerName", ADOX.DataTypeEnum.adVarWChar, fieldIndex:=True)
Public Shared OwnerMail As classObjectField = New classObjectField("OwnerMail", ADOX.DataTypeEnum.adVarWChar, fieldIndex:=True, fieldUnique:=True)

Public Shared ArrayTableOwners = New Object(3) {
    IDOwner, OwnerAccount, OwnerName, OwnerMail
}

Public Shared IDUser As classObjectField = New classObjectField("IDUser", ADOX.DataTypeEnum.adInteger, "AutoIncrement", True, True, True, True)
Public Shared UserName As classObjectField = New classObjectField("OwnerAccount", ADOX.DataTypeEnum.adVarWChar, fieldIndex:=True)
Public Shared UserFunctionCode As classObjectField = New classObjectField("OwnerName", ADOX.DataTypeEnum.adVarWChar, fieldIndex:=True)

Public Shared ArrayTableUser = New Object(2) {
    IDUser, UserName, UserFunctionCode
}

This function below creates the table of an existing Access DB

 Public Shared Function createDBTable(Cat As ADOX.Catalog, tableName As String, objField As Object) As Boolean
    Dim confirm As Boolean = True

    Try
        Dim objTable = New ADOX.Table
        Dim objIndex = New ADOX.Index

        objTable.Name = tableName

        For Each valueField In objField

            objTable.Columns.Append(valueField.fieldName, valueField.fieldType)

            If Not valueField.FieldProperties = Nothing Then

                objTable.Columns.Item(valueField.fieldName).ParentCatalog = Cat
                objTable.Columns.Item(valueField.fieldName).Properties(valueField.fieldProperties).Value = valueField.FieldPropertiesValue

            End If

            If valueField.fieldIndex = True Then

                objIndex = New ADOX.Index
                objIndex.Unique = valueField.fieldUnique
                objIndex.Name = valueField.fieldName
                objIndex.PrimaryKey = valueField.fieldKey
                objIndex.Columns.Append(valueField.fieldName)
                objTable.Indexes.Append(objIndex)

            End If

        Next

        Cat.Tables.Append(objTable)

        objIndex = Nothing
        objTable = Nothing
    Catch ex As Exception

        MsgBox(ex.Message)
        confirm = False

    End Try

    Return confirm
End Function

Usage:

Dim objCn = New ADODB.Connection
Dim objCat = New ADOX.Catalog
objCn.Open(DBSourceLocation)
objCat.ActiveConnection = objCn

createDBTable(objCat, "OWNERLIST", ArrayTableOwners)
createDBTable(objCat, "USERLIST", ArrayTableUser)

Encapsulation is taking a beating here:

Public fieldName As String
Public fieldType As ADOX.DataTypeEnum
Public fieldProperties As String
Public fieldPropertiesValue
Public fieldIndex As Boolean
Public fieldUnique As Boolean
Public fieldKey As Boolean

You have a public constructor for assigning these fields, but the instance fields are all Public, which completely defeats the purpose of encapsulation. Fields should be Private, and ideally read-only (I'm a C# guy, not sure how/whether VB.NET implements this but in C# that would be private readonly [type] [name];).

It gets worse:

Public Shared IDOwner As classObjectField = New classObjectField("IDOwner", ADOX.DataTypeEnum.adInteger, "AutoIncrement", True, True, True, True)
Public Shared OwnerAccount As classObjectField = New classObjectField("OwnerAccount", ADOX.DataTypeEnum.adVarWChar, fieldIndex:=True)
Public Shared OwnerName As classObjectField = New classObjectField("OwnerName", ADOX.DataTypeEnum.adVarWChar, fieldIndex:=True)
Public Shared OwnerMail As classObjectField = New classObjectField("OwnerMail", ADOX.DataTypeEnum.adVarWChar, fieldIndex:=True, fieldUnique:=True)

Beyond the constructor [made redundant by public fields], nothing in this class seems to justify having an instance: literally everything is Shared! These public shared object fields are statically initialized but, being fields, any calling code can swap the reference at any time, and because the fields are Shared, every single instance out there suddenly finds itself with a modified IDOwner.

Expose properties, not fields; and only expose Property Get if outside/calling code shouldn't be allowed to tamper with the encapsulated values and references.

OOP is about objects. Shared makes a member belong to a type, not an object instance: Shared is essentially the complete opposite of Object-Oriented.

Seems you mean to use the Public Shared fields as a way to return "canned" instances of this classObjectField class - that's not an awful idea, but then the constructor should be Private.

classObjectField is a bad name for a class. "class" is perfectly redundant and has nothing to do in the name of the type, and class and member names should be PascalCase, so that would be ClassObjectField, and CreateDbTable.

VB.NET isn't just Object-Oriented: it's also *strongly-typed.

Public Shared ArrayTableOwners = New Object(3) {
    IDOwner, OwnerAccount, OwnerName, OwnerMail
}

This is an array of Object references; in .NET everything is an Object - from a simple String to any class instance. Exposing Object on your public API is considered bad practice, because the caller has no idea what they're getting. This would be better (failed encapsulation aside):

Public Shared ArrayTableOwners = New classObjectField(3) {
    IDOwner, OwnerAccount, OwnerName, OwnerMail
}

Now the caller knows they're getting an array of classObjectField objects.

Public Shared Function createDBTable(Cat As ADOX.Catalog, tableName As String, objField As Object) As Boolean

I don't see a reason not to rename Cat to adoxCatalog - it's a catalog, not a cat. And objField is terribly misleading.

The signature says objField As Object, implying ONE object is expected. But then...

    For Each valueField In objField

If you expect an array, make the signature say so! This is what I meant with an object can be literally anything. And for performance reasons you should iterate arrays with a For loop, not For Each.

Actually, keep the For Each. Because you don't need an array here - you need anything that can be iterated. The .NET framework gives you the generic IEnumerable(Of T) interface for that: objField should be plural, to indicate it can "contain" many objects, and then instead of As Object it should be As IEnumerable(Of classObjectField), so that you can't give it a String and still happily compile and run the project.

The function shouldn't return a Boolean. Even your sample usage code doesn't care about the return value:

createDBTable(objCat, "OWNERLIST", ArrayTableOwners)
createDBTable(objCat, "USERLIST", ArrayTableUser)

Truth is, nobody will either, for you've already handled the exception and displayed a message box - and that's not your job!

Instead, make the function a Sub, and let exceptions bubble up the call stack, so that the calling code is responsible for handling exceptions and deciding whether they want to display a message box or log an entry to some text file or whatever. .NET works with exceptions, not return codes.

That said if I had to return a Boolean value for success, I'd default it to False and only switch it to True when I'm absolutely certain that my function did what it had to do. Here you're defaulting to True and switching it to False when an exception is thrown, which only works because you're catching all exceptions - but best practices tell us that we should only be catching the most specific exceptions we can, and let everything else bubble up - and if you did that, your function could return True when it really actually failed. confirm sounds like some flag that would be True when the user confirmed some prompt; success would be a better name for it. But that's moot anyway, as I said it's not that function's job to handle all possible exceptions and display a message box.


Quicksort implementation in Python I have written an implementation of Quicksort in Python. I am new to Python. Any suggestions for improvement or criticism on my use of Python?

def partition(a, lo, hi):
    i, j, v = lo+1, hi, a[lo]
    while(True):
        while(a[i] < v):
            i += 1
            if (i == hi): break
        while(a[j] > v):
            j -= 1
            if (j == lo): break
        if (i >= j): break
        a[i], a[j] = a[j], a[i]
    a[lo], a[j] = a[j], a[lo]
    return j

def sort(a, lo, hi):
    if (hi <= lo):
        return
    q = partition(a, lo, hi)
    sort(a, lo, q-1)
    sort(a, q+1, hi)
    assert isSorted(a, lo, hi)

def quick_sort(a):
    shuffle(a)
    sort(a, 0, len(a)-1)
    assert isSortedArray(a)

def isSorted(a, lo, hi):
    for i in range(lo, hi):
        if a[i+1] < a[i]:
            return False
    return True

def isSortedArray(a):
    for i in range(0, len(a)-1):
        if a[i+1] < a[i]:
            return False
    return True

When describing quicksort partitioning, your v is typically called the "pivot".  The code would be clearer if you named the variable according to that convention.

You always choose a[lo] as the pivot.  However, that produces pathological performance when the input array is already sorted.

I would prefer to see

while(a[i] < v):
      i += 1
      if (i == hi): break

… written as

while i < hi and a[i] < pivot:
    i += 1

Array index bounds usually work better when specified as inclusive-exclusive ranges, such that sort(a, lo, hi) means "sort a where lo ≤ index < hi".  This is a common convention — you can see it in Python's range() and slicings.  Also, Java's Arrays.sort(a, fromIndex, toIndex) works with inclusive-exclusive ranges.

Some nice properties of inclusive-exclusive ranges are:

hi - lo gives you the number of elements in the range.
When creating a range for the entire array a, hi is just len(a).  You save a "-1".
When splitting [lo, hi) into two consecutive ranges, it becomes [lo, mid) and [mid, hi).  You save a "-1".
In Python, you can conveniently write for i in range(lo, hi) for the most common type of iteration. (Admittedly, iterating backwards is uglier, but it's less common.)


Dealing with input and output in Matlab I just wrote a program which performed some elementary set operations as you can see below. It's the first time I have used input and output of text files so I am mainly looking for critique on that, and also how I use cells.

clear all;
clc;

fileID = fopen('inputseto.txt','r');
data = textscan(fileID,'%s',3,'Delimiter','\n');
data = data{1,1};

n = data{1}
n = str2num(n);

A = data{2};
B = data{3};

A = str2num(A(2:end-1));
B = str2num(B(2:end-1));

output = cell(1,6);

output{1} = union(A,B);
output{2} = intersect(A,B);
output{3} = setdiff(A,B);
output{4} = setdiff(B,A);
output{5} = setdiff(1:n,A);
output{6} = setdiff(1:n,B);

fileID = fopen('outputseto.txt','w');

for i = 1:6
    fprintf(fileID,'{');
    fprintf(fileID,'%d, ',output{i}(1:end-1));
    if i ~= 6
        fprintf(fileID,'%d}\n',output{i}(end));
    else
        fprintf(fileID,'%d}',output{i}(end));
    end
end

I got this problem from here. The input is 3 lines, the first lines describes the global set. The first line is an integer n and the global set is the set containing 1,2,...,n. Then the next two lines are two sets A and B. The output consists of various set operations involving A and B.  The solution is correct, but I feel like some of this code doesn't "feel right". I am interested in knowing what you think.

The only thing that I see is that the files are not closed with fclose after the data is read/written.  Especially using the same variable (fileID) as the file handle for the second while the first is still open (effectively losing the first one). Although you can still always do fclose('all') and I think it would close even those files still open that don't have a handle still assigned.


State Design Pattern in Python I'm trying to find the best - read: readable, maintainable, robust, threadsafe, usable - solution for a State Machine in python.
For this I've been looking at the State Design Pattern. However I want a pythonic and lean solution (saying not implementing functions in subclasses that are not needed).

Long story short, here's what I came up with. What do you think?

#!/bin/env python
# -*- utf-8 -*-
"""Boiler Plate code for implementing a state machine according to
state design pattern

"""

class State(object):
    """Base class for a state in a statemachine

    No statemachine class is needed. State transitions have to be
    defined in self.transitions.

    The object is callable and by calling it input is handled and
    the next state is returned.

    Example usage:

    def handle(self, msg):
        self._state = self._state(msg)

    """
    transitions = [(None, None, None)]

    def __init__(self, state_context):
        self._context = state_context

    def __call__(self, inp):
        new_state = self

        state_action_list = [(state, action) for cls, state,
                     action in self.transitions if
                     isinstance(inp, cls)]
        if 1 == len(state_action_list):
            state, action = state_action_list[0]
            if action is not None:
                try:
                    action(inp)
                except TypeError:
                    action(self, inp)
            new_state = state(self._context)

        elif 1 < len(state_action_list):
            raise AmbiguousTransitionError(inp, self)

        print ("[DEBUG] %s -- %s --> %s" %
               (self.__class__.__name__,
            inp.__class__.__name__,
            new_state.__class__.__name__ ))

        return new_state

class AmbiguousTransitionError(Exception):
    """Raised if more than one state transition was found for a
    given input

    """
    pass

################################################## Example

class MsgA(object): pass
class MsgB(object): pass
class MsgC(object): pass

class StateA(State): pass

class StateB(State):
    @staticmethod
    def print_msg(inp):
        print "Inp: ", inp

class StateC(State):
    def print_context(self, unused_inp):
        print "Context: ", self._context

StateA.transitions = [(MsgA, StateB, None)]
StateB.transitions = [(MsgB, StateC, StateB.print_msg)]
StateC.transitions = [(MsgB, StateB, None),
              (MsgC, StateA, StateC.print_context)]

class Context(object):
    def __init__(self):
        self._state = StateA(self)

    def handle(self, msg):
        self._state = self._state(msg)

if __name__ == "__main__":
    CONTEXT = Context()
    CONTEXT.handle(MsgA())
    CONTEXT.handle(MsgB())
    CONTEXT.handle(MsgC())
    CONTEXT.handle(MsgB())

The output looks like this:

python State.py

[DEBUG] StateA -- MsgA --> StateB
Inp:  <__main__.MsgB object at 0x1eb6250>
[DEBUG] StateB -- MsgB --> StateC
Context:  <__main__.Context object at 0x1eb6150>
[DEBUG] StateC -- MsgC --> StateA
[DEBUG] StateA -- MsgB --> StateA

Not a proper code review as per se but I was wondering if things couldn't be done in a more straight-forward way like this :

#!/usr/bin/python

class Machine(object):
    def __init__(self, state, transitions):
        self.transitions = transitions
        self.state = state

    def __call__(self, input):
        old_state = self.state
        self.state = self.transitions.get((self.state, input), self.state)
        print("DEBUG %s -- %s --> %s" % (old_state, input, self.state))

machine = Machine(
    "a",
    {
        ("a","msgA"):"b",
        ("b","msgB"):"c",
        ("c","msgB"):"b",
        ("c","msgC"):"a",
    }
    )

machine("msgA")
machine("msgB")
machine("msgC")
machine("msgB")

Using functions to have actual side effects, this could look like this :

#!/usr/bin/python

# examples of helper functions
def goto(s):
    return lambda state,input: s

def no_move():
    return lambda state,input: state

def verbose_no_move():
    def func(state,input):
        print('Cannot find a transition for %s from %s' % (input, state))
        return state
    return func

def print_input_and_goto(s):
    def func(state,input):
        print(input)
        return s
    return func

def print_old_state_and_goto(s):
    def func(state,input):
        print(state)
        return s
    return func

def raise_exception():
    def func(state,input):
        raise ValueError('Cannot find a transition for %s from %s' % (input, state))

class Machine(object):
    def __init__(self, initial_state, transitions):
        self.transitions = transitions
        self.state = initial_state
        self.default = verbose_no_move() # pick a default behavior

    def __call__(self, input):
        old_state = self.state
        func = self.transitions.get((self.state, input), self.default)
        self.state = func(self.state,input)
        print("DEBUG %s -- %s --> %s" % (old_state, input, self.state))

machine = Machine(
    "a",
    {
        ("a","msgA"):goto("b"),
        ("b","msgB"):print_input_and_goto("c"),
        ("c","msgB"):goto("b"),
        ("c","msgC"):print_old_state_and_goto("a"),
    }
    )

machine("msgA")
machine("msgB")
machine("msgC")
machine("msgB")


JavaScript Tic-Tac-Toe The first function makes 2 players

const Player = (name,mark) => {
return {name, mark}
}
let player1 = Player('Player 1', '
X
');
let player2 = Player('Player 2', '
O
');

The rest of the code which runs the game

const gameBoard = (() => {
  board = [["","",""],["","",""],["","",""]];
  let whichPlayer = getCurrentPlayer();

  const AddEventListeners = (() => {
    $('.square').click(function(event) {
        if(win == false && this.innerHTML == ''){
            console.log(this)
        let a = addToBoard(whichPlayer()).bind(this);
        a(this.id)}
    })
    $('.restart').click(function(e) {
        board = [["","",""],["","",""],["","",""]];
        $('.square').empty()
        btnDiv.style.display = "none";
        $('.winnermsg').hide();
        win = false;
    })
  })();

  function getCurrentPlayer(){
    let currentPlayer = player1
    function getPlayer() {
        if(currentPlayer === player1){
            currentPlayer = player2
            crntPlayer = player1
            return player1
        }else{
            currentPlayer = player1
            crntPlayer = player2
            return player2
        }
    }
    return getPlayer;
  }

  function addToBoard(currentPlayer) {
    return function(square){
        this.innerHTML = currentPlayer.mark
        let coords = square.split('')
        board[coords[0]][coords[1]] = currentPlayer.mark
        checkWin()
    }

  }
  function declareWinner(tie = false){
    win = true;
    $('.winnermsg').show();
    if(tie){
        $('.winnermsg').html(`<p>Tie game, try again</p>`);
    }else{
        $('.winnermsg').html(`<p>Congratulations, ${crntPlayer.name} you have won</p>`);
    }
    if (crntPlayer == player1){whichPlayer();}
    btnDiv.style.display = "grid"
  }

  function checkWin(){
    //check horizontal
    for(let i=0;i<board.length;i++){
        array=[]
        if(board[i].every(x => x == crntPlayer.mark )){
            declareWinner()
        }
        //check vertical
        for(let j=0;j<board[i].length;j++){
            array.push(board[j][i])
            if(array.every(x=>x == crntPlayer.mark && array.length == 3)){
                declareWinner()
            }
        }
    }
    //check diags
    newArray=[]
    newArray.push(board[0][0],board[1][1],board[2][2])
    if(newArray.every(x => x == crntPlayer.mark)){
        declareWinner()
    }
    newArray=[]
    newArray.push(board[0][2],board[1][1],board[2][0])
    if(newArray.every(x => x == crntPlayer.mark)){
        declareWinner()
    }
    //check tie
    newArray=board.reduce((a,b) => a.concat(b))
    if(!newArray.includes('')){declareWinner('tie')}

  }

})();

So I just add event listeners to each div and if it's empty it changes the innerHTML to the player's mark and adds that mark to the board array and then checks if there's a winner.

I feel like it's not very well organized but i'm not sure how it would best be improved.

Just a few pointers, some of which are my personal preference!

Since you use modern concepts like arrow functions, const, and shorthand objects, you might as well replace jQuery $(".square") with document.querySelectorAll(".square")
Passing a div as an argument is not a good idea. I would keep all visualisation of the game in one separate function, for example: function drawBoard(). This function does all the DOM manipulation, and the DOM elements or jQuery functions should not be mentioned anywhere else in your code.
I would use one game state array, and the event handlers should alter that game state array directly. If square 1 is clicked by player one, the state array would become [1,0,0,0,0,0,0,0,0], and after the game state has updated, you call drawBoard() which draws the X and O visuals in the DOM.
You can use this state array in your checkWin function too, instead of using so many temporary arrays! If you use numbers you can even see who won just by adding the numbers.
You use a lot of temporary variables. I've added a suggestion to switch players without using those.
Don't nest so many functions in each other, don't call them implicitly by sometimes adding () at the end, and other times not. It's hard to reason about.
Arrow functions don't have their own scope, but sometimes you DO need a scope, for example in your gameboard class.

Below are just a few sketches to rewrite the code, I realise this is far from complete but it might give you some ideas:

function Player(name, mark, id){
  this.name = name
  this.mark = mark
  this.id = id
}

function GameBoard(){
  let state = [0,0,0,0,0,0,0,0,0]
  let player1 = new Player("Mark","X", 1)
  let player2 = new Player("Joe", "O", 2)
  let currentPlayer = player1

  this.initHandlers = function(){
    let fields = document.querySelectorAll(".field")
    for(let f of fields){
     f.addEventListener("click", (e)=> this.updateState(e))
    }
  }

  this.updateState = function(e){
    console.log("id of the clicked tile is " + e.target.id)
    let id = parseInt(e.target.id)
    state[id] = currentPlayer.id
    this.drawBoard()
    this.checkWin()
    currentPlayer = this.switchPlayers()
  }

  this.switchPlayer = function(){
      return (currentPlayer == player1) ? player2 : player1
  }

  this.drawBoard = function(){
    // all DOM manipulation here
  }

  this.checkWin = function(){
    if(state[0] + state[3] + state[6] == 3) console.log("player one wins!")
  }
}

let board = new GameBoard()
board.initHandlers()


Cancelling an order with a side-effect of logging the operation I have a data access layer method to cancel an order. This operation should have an associated "operation history" entry.

My first (naïve) implementation looks like this:

public void CancelOrder(int orderMastId, string reason)
{
    string order_id = orderMastId.ToString();
    var order = this.db.Commandes.Where(c => c.Order_Mast_Id == order_id);

    if (order == null || order.Count() == 0)
    {
        throw new EntityNotFoundException();
    }

    Parallel.ForEach(order, commande =>
        {
            string previousStatus = commande.Status;
            if (previousStatus != ShippingStatus.Printed)
            {
                // this is what marks the order as cancelled (legacy DB power!)
                commande.Status = ShippingStatus.Printed;

                // this table stores many operations on the order (status change, etc)
                var hist = new DB.Operations_History
                           {
                               Commande_id = commande.Commande_id,
                               Created_Date = DateTime.Now,
                               Feedback = reason,
                               Final_Shipper = string.Empty,
                               Final_Status = ShippingStatus.Printed,
                               Information = "API",
                               Initial_status = previousStatus,
                               Operation_id = 0,
                               Shipping_Info = string.Empty,
                               Shipping_Reference = string.Empty,
                               User_id = 0
                           };

                this.db.Operations_History.Add(hist);
            }
        });

    this.db.SaveChanges();
}

But this is a clear side-effect and in violation of single responsibility principle.

I'm guessing there exist a design pattern for this kind of thing, but I don't know which.

For instance, I could decide that the history entry is a kind of logging, and use (for instance) PostSharp attributes to "log" the history once the methods exits, but somehow that feels wrong, would create a whole logging structure only for operations on a single table, and might prove difficult: here the example history is very simple, but in reality there might be a bunch of rules on what to log and how exactly to log it (depending on what is logged and how, it's used differently).

Cancelling?

  I have a data access layer method to cancel an order.

All I see is code that creates a history-entry for each order you have in your collection. Please forgive and correct me if I'm wrong but if that is the cancellation, I don't see any violation of a single responsibility principle.
Consistency

I see you declaring your variables explicitly and implicitly. Use var everywhere you can or declare them all explicitly. It's just a personal choice, although I'd prefer using var wherever I can.
Naming

Field names use camelCase. The name ´order_id´ is not bad as it describes well what it stands for, but orderId would be more correct as it follows the capitalization styles of Microsoft. Since order is a collection of items, rename it to orders,  order would imply you're only expecting one item.
Code

Checking if an IEnumerable<T> collection has any items, can be done using the Enumerable.Any() method. This is faster than using the Count() method. Let's say the collection has 3 items. Any() will return when it finds the first item whereas Count() will iterate the whole collection. But, if you have a collection that has a Length or Count property, use that for performance.

Since you also do a null-check, I would create an extension method that combines the two. I've written this one for myself which I use often:

public static class Extensions
{
    public static bool AnySafe<T>(this IEnumerable<T> coll)
    {
        return coll.AnySafe(null);
    }

    public static bool AnySafe<T>(this IEnumerable<T> coll, Func<T, bool> predicate)
    {
        if(coll == null)
            return false;

        return (predicate == null) ? coll.Any() : coll.Any(predicate);
    }
}

//Usage:
if (!order.AnySafe())
    throw new EntityNotFoundException();


Meta-program to find square numbers I'm new to template meta-programming and wrote some code to determine if a number is a perfect square.  For example, in my program, SQUARE<1>::value and SQUARE<4>::value evaluate to true while SQUARE<5>::value is false.  I'm open to any suggestions on how I could implement this better, cleaner, or make use of standard library functions.  Also, please point out any meta-programming conventions I may have overlooked.

template<bool condition, bool Then, bool Else>
struct IF {
   static const bool ret = Then;
};

template<bool Then, bool Else>
struct IF<false, Then, Else> {
   static const bool ret = Else;
};

template<size_t v1, size_t v2>
struct GREATER {
   static const bool ret = v1 > v2;
};

template<size_t n, size_t r>
struct IS_SQUARE {
   static const bool value = IF<n == r*r,
                true,
                IF<GREATER<n, r*r>::ret,
                   IS_SQUARE<n, r+1>::value,
                   false>::ret
                >::ret;
};

template<size_t n>
struct IS_SQUARE<n, n> {
   static const bool value = false;
};

template<size_t n>
struct SQUARE {
   static const bool value = IS_SQUARE<n, 1>::value;
};

template<>
struct SQUARE<1> {
   static const bool value = true;
};

// example usage
int main() {
   static_assert(SQUARE<1>::value, "must be square");
   static_assert(SQUARE<4>::value, "must be square");
   static_assert(SQUARE<5>::value, "must be square"); // compile error
}

I'd recommend against using ALL_CAPS for class-names. It's not really in accordance with C++ conventions.

template<size_t n, size_t r>
struct IS_SQUARE {
   static const bool value = IF<n == r*r,
                true,
                IF<GREATER<n, r*r>::ret,
                   IS_SQUARE<n, r+1>::value,
                   false>::ret
                >::ret;
};

You can replace the outer if with || (if foo then true else bar is equivalent to foo || bar) here to get rid of one level of nesting and simplify the condition a bit. One might think that you can also replace the inner if with &&, but that won't work because template-instantiation doesn't short-circuit in the way you'd want it to when using logical operators (i.e. foo && Bar<baz> will try to instantiate Bar<baz> even if foo is false, leading to infinite recursion in this case).

template<size_t n>
struct SQUARE {
   static const bool value = IS_SQUARE<n, 1>::value;
};

You might want to use 0 instead of 1 as the starting value here. Otherwise you'll get incorrect results for n=0 (which is a square of itself and should thus return true).

template<>
struct SQUARE<1> {
   static const bool value = true;
};

This is redundant because IS_SQUARE<1, 1> (or IS_SQUARE<1,0>) is true anyway.

Apart from the nitpicks listed above, your code looks fine (or as fine as TMP code ever looks, anyway). However it can be completely replaced using constexpr, which still gives you compile-time evaluation (including the ability to use the result inside template arguments or static_assert), while being greatly more readable:

The beauty of C++11's constexpr feature is that it allows you to write compile-time expressions using normal C++ syntax, instead of encoding everything using template classes. You're not allowed to use loops or any side-effects, so you still have to use the recursive approach you used (as opposed to the iterative approach you'd probably use at run-time), but it can be greatly simplified by expressing it as a simple recursive function instead of a bunch of templates:

constexpr bool is_square(size_t n, size_t r=0) {
    return (r*r == n)  ||  ( (r*r < n) && is_square(n, r+1));
}

int main() {
    static_assert(is_square(1), "must be square");
    static_assert(is_square(4), "must be square");
    static_assert(is_square(5), "must be square"); // compile error
}

As a learning-exercise for template meta programming this version is completely useless, of course, as it does not actually contain any templates. But it's definitely more readable (and more writable as well) and can be used in any scenario where the template-version can be used. So in any real C++11 meta-programming project, I'd always recommend using constexpr over templates when it's possible. This way you can perform your calculations in functions that look like C++ functions (well, C++ functions that are written in a surprisingly functional style) and use templates to define classes (which depend on the results of those calculations).


Method that reserves a reservable entity and charges the user This method works for what I need it to do, but I just don't think it's extremely readable, and could be abstracted into different methods. I have a feeling I'm going against some best practices. Please let me know your suggestions:

  def reserve
  return render json: { error: "This entity is not available to reserve" }, status: :forbidden if @reservable.status != Reservable::STATUS[:available]
  Reservable.transaction do
    @reservable.status = Reservable::STATUS[:reserved]
    @reservable.current_use = Use.create(
      user_id: @user.id,
      reservable_id: @reservable.id,
      start_location: @reservable.location,
      start_time: DateTime.now,
      status: Use::STATUS[:progress])
    # Check payment
    payment_type = params[:payment_type]
    if payment_type === Transaction::METHODS[:subscription]
      # put subscription logic here
    else
      return render json: { error: @user.errors }, status: :payment_required, location: new_payment_path unless @user.validates_has_payment_and_good_standing
      if payment_type === Transaction::METHODS[:prepay]
        @reservable.current_ride.trans = Transaction.charge_user_for_use(@user, @reservable.current_use, payment_type)
      else # :per_minute
        # put pay_per_minute logic here
      end
    end
    @reservable.save!
  rescue ActiveRecord::RecordInvalid => exception
    render :json => { :error => exception.messages }, status: :unprocessable_entity
    raise ActiveRecord::Rollback #force a rollback
  end
end

Brandon Keepers has a really good tip in his talk Why our code smells.

  Write a top level decription of your class without using the words "and" or "or".

If you can't do that then there is a risk that your class might be doing to much. Since ActiveModel and AR and ActionController gives models and controllers such superpowers it's easy to fall into the trap of making godlike objects.

So when you have a method which has an AND in the description then you have definitely have a code smell.

The first major code smell here is @reservable.status != Reservable::STATUS[:available] this is just a leaky class with the internals hanging out like loose wires. Fortunately it's really easy to fix:

class Reservable
  def available?
    status == Reservable::STATUS[:available]
  end
end

But an even better way would be to crack out the awesomeness of ActiveRecord::Enum, as it will take care of the wiring for you:

class Reservable
  enum status: [:available, :reserved]
end

This gives use reservable.available?, .reserved?, .reserved! etc.

One good pattern to refactor the above would be Service objects. Service objects are dirt simple plain old ruby objects that just do one job - tops. Most service objects just have a single public method - often #call

So lets see if we can start splitting this up into distinct tasks.

# Reserves a reservable
class ReservationService

  def initialize(user:, reservable:)
    @user = user
    @reservable = reservable
  end

  def call
    unless @reservable.available?
      raise Reservable::UnavailableError.new(object: @reservable)
      return false
    end
  end
end

# Charges a user for services rendered
class ReservationChargingService

  attr_reader :transaction

  def initialize(charge, user){
    @charge = charge
    @user = user
  }

  def call(amount, payment_type)
    # @todo charge user for amount
    # @todo return true / false or a status code for the payment
  end
end

This gives us nice detached pieces which can be tested in isolation - which is really nice since testing methods with DB transactions can be problematic.

Then in your controller you can boil things down a bit:

def reserve

  Reservable.transaction do
    begin
    @reserved = ReservationService.new(
                    user: @user,
                    reservable: @reservable
                ).call
    @charged = ReservationChargingService.new(
                    user: @user,
                    reservable: @reservable
               ).call
    raise ActiveRecord::Rollback unless @reserved && @charged
    rescue Reservable::UnavailableError
      raise ActiveRecord::Rollback, 'Reservable not available.'
    end
    rescue ActiveRecord::RecordInvalid
      raise ActiveRecord::Rollback, 'Validation error'
    end # @todo rescue other errors?
  end

  if @reserved && @charged
     render json: @reservable.current_use
  else
     errors = [@charged.transaction, @reservable]
                 .reject(&:valid?).map { |o| o.errors.full_messages }
     render json: { errors: errors }, status :unprocessable_entity
  end
end

As you can see here you want to delegate as much work to services as possible. Also prefer using ActiveModel::Errors instead of creating error hashes in your controller.

Our controller only has to deal with two contingencies here two which is really good: either the complete transaction passes and we return a success object or we return a errors hash.


Which swift screen handler approach is better for instantiating a utility class and then presenting a viewController? 1st approach:

let whatsNewHandler = WhatsNewHandler(buildNumber: buildNumber)
 whatsNewHandler.showWhatNewScreeen(completion: { whatsNewVC in

      self.present(whatsNewVC, animated: true, completion: nil)
 })

2nd approach, which looks a bit functional:

WhatsNewHandler(buildNumber: buildNumber).showWhatNewScreeen(completion: { whatsNewVC in

      self.present(whatsNewVC, animated: true, completion: nil)
})

While both allocate the same amount of memory. I like the functional approach, I just do something and that's really it. But the reason I'm asking is I haven't seen anyone write like this. I see things similar to it when you're using a singleton, but this isn't a singleton.

EDIT:

My WhatNewHandler class:

import Foundation
import UIKit

typealias WhatsNewCompletionHandler = (_ whatsNewViewController: WhatsNewViewController) -> ()

class WhatsNewHandler{

    let buildNumber : String
    lazy var route : Router = {
      return Router.whatsNew(buildNumber: self.buildNumber)
    }()

    init(buildNumber: String){
        self.buildNumber = buildNumber
    }

    func showWhatNewScreeen(completion: @escaping WhatsNewCompletionHandler){

        NetworkHandler.sharedInstance.request(route, success: { json,_  in

            guard let json = json else{
                writelog("what's new json was nil")
                return
            }
            if let releaseBuild = ReleaseBuild(json: json){
                let WhatsNewVC = WhatsNewViewController(releaseBuild: releaseBuild)
                WhatsNewVC.modalPresentationStyle = .overFullScreen

                // TEST ALPHA ON REAL DEVICE
                WhatsNewVC.view.backgroundColor = UIColor.black.withAlphaComponent(0.985)
                completion(WhatsNewVC)
            }
            }, failed: {_,_,_ in

        })
    }
}

As for my WhatsNewViewController, kindly assume it to be a Viewcontroller that shows a list of new features added to the app.

The only difference between the first and the second approach is whether
the handler is assigned to a variable or not. None of them is more
"functional" than the other. Therefore:

If you need a reference to the handler (perhaps to cancel the request)
then use the first approach.
If you don't need that reference, choose whatever you feel more
comfortable with, or what you find better readable.

Some more remarks:

The method name showWhatNewScreeen is (apart from the typo)
misleading. The method creates a view controller but does not
present it. Something like createWhatsNewController might be
more appropriate.
According to the API Design Guidelines, variable names should be lowerCamelCase, that is violated at
let WhatsNewVC = ....
The right-hand side of

whatsNewVC.view.backgroundColor = UIColor.black.withAlphaComponent(0.985)

can be abbreviated to .black.withAlphaComponent(0.985), the
type UIColor is then inferred from the context.


Uploading photos of various sizes In my solution I've implemented transaction-like methods of uploading posts media. For example I'm adding new post with photo. I want to have 700x700, 200x200, 70x70, 10x10 px sizes in my storage and original in device album. And when all images are uploaded, then save new post info to database. But this code is not beautiful at all. How can I improve it?

// MARK: - Christmas tree
// Like transaction :)
// Bad view actually
static func uploadVideoForPost(with videoURL: URL, for postForUpdate: PostItem,
                              screenShot: UIImage,
                              completion: @escaping (_ hasFinished: Bool, _ postWithRefs: PostItem?) -> Void) {
  var post = postForUpdate // we will insert refs to media to this object
  // upload screenshots
  upload(screenShot, for: post, withSize: 700.0)
  { (hasFinishedSuccessfully, url) in

     if hasFinishedSuccessfully {
        post.mediaRef700 = url

        upload(screenShot, for: post, withSize: 200.0)
        { (hasFinishedSuccessfully, url) in

           if hasFinishedSuccessfully {
              post.mediaRef200 = url

              upload(screenShot, for: post, withSize: 70.0)
              { (hasFinishedSuccessfully, url) in

                 if hasFinishedSuccessfully {
                    post.mediaRef70 = url

                    upload(screenShot, for: post, withSize: 10.0)
                    { (hasFinishedSuccessfully, url) in

                       if hasFinishedSuccessfully {
                          post.mediaRef10 = url

                          // upload video
                          upload(with: videoURL, for: post)
                          { (hasFinishedSuccessfully, url) in

                             if hasFinishedSuccessfully {
                                post.videoRef = url

                                let path = videoURL.path

                                if UIVideoAtPathIsCompatibleWithSavedPhotosAlbum(path) {
                                   UISaveVideoAtPathToSavedPhotosAlbum(path, nil, nil, nil)
                                }
                                completion(true, post) // after this func i will save post data to db in other func.
                             } else {
                                completion(false, nil)
                             }
                          }
                       }else {
                          completion(false, nil)
                       }
                    }
                 } else {
                    completion(false, nil)
                 }
              }
           } else {
              completion(false, nil)
           }
        }
     } else {
        completion(false, nil)
     }
   }
}

Upload func is just a func of uploading with completion handler.

If Nesting

You are nesting if's too much which quickly becomes ugly. I don't know your language but if at all possible negate the the conditions and return the result for example:

do:

if not HasCompleted {

return(or equivalent) completion(false, nil)

}

instead of:

if HasCompleted {
...
}
else
{
completion(false, nil)
}


Tic Tac Toe personal project This is a Tic Tac Toe game in Java in preparation for my seconds personal project (chess game). I'm having a little problem though and am thinking that my for loop to create those buttons is a mistake. I believe I should instantiate all 9 JButtons implicitly instead of using a for loop because then I know for sure I will be able to compare each and every button. What are your thoughts?

import javax.swing.*;
import java.awt.*;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;

/**
 * Created by Usman on 5/17/2016.
 */
public class TicTacToeGUI extends JPanel {

    boolean turn = true;

    public TicTacToeGUI() {
        setLayout(new GridLayout(3,3));

        for(int i=0 ; i<9 ; i++) {
            JButton btn = new JButton("");
            btn.setPreferredSize(new Dimension(70,60));
            btn.setFont(new Font("Arial", Font.PLAIN, 40));
            btn.addActionListener(new ActionListener() {
                @Override
                public void actionPerformed(ActionEvent e) {
                    if(btn.isEnabled()){
                        if(turn){
                            btn.setText("X");
                            turn = !turn;
                        }
                        else{
                            btn.setText("O");
                            turn = true;
                        }
                        btn.setEnabled(false);
                    }
                }
            });
            add(btn);
        }
    }

    public static void main(String []args){
        TicTacToeGUI game = new TicTacToeGUI();
        JFrame frame = new JFrame("Tic Tac Toe");
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.getContentPane().add(game);
        frame.pack();
        frame.setVisible(true);
    }
}

Using a loop isn't inherently wrong, the problem you're anticipating though is right -- you won't have a way of comparing and referencing the buttons since they all become anonymous with your current approach.

Instead, use an array of JButton and simply use the iterator of the loop to instantiate at that location which you can later utilize as an index to reference a specific JButton.

JButton[0] board = new JButton[9];
    for (int i = 0; i < board.length; i++) {
        board[i] = new JButton("");
    }

On the topic of anonymity, as of Java 8 rather than using anonymous inner classes for Action Events you can use Lambda Expressions.

So, you can replace this chunk of code:

 btn.addActionListener(new ActionListener() {
                @Override
                public void actionPerformed(ActionEvent e) {

With:

btn.addActionListener(e -> {

Of course, you close the bracket. This also has the benefit of no longer requiring you to import ActionEvent and ActionListener directly.


PDO class for multiple databases I have a PDO class below:

class DB {

        private $dbh;
        private $stmt;

        static $db_type;
        static $connections;

        public function __construct($db, $id="") {

             switch($db) {
                case "db1":
                      try{

                          $this->dbh = new PDO("mysql:host=localhost;dbname=ms".$id, 'root', '', array( PDO::ATTR_PERSISTENT => true ));
                      }  catch(PDOException $e){
                          print "Error!: " . $e->getMessage() . "
";
                          die();
                      }
                break;
                case "db2":
                      try{
                          $this->dbh = new PDO("mysql:host=localhost;dbname=users", 'root', '', array( PDO::ATTR_PERSISTENT => true ));
                      } catch(PDOException $e){
                          print "Error!: " . $e->getMessage() . "
";
                          die();
                      }
                break;
            }
            self::$db_type = $db;
        }

        static function init($db_type = "", $id){

            if(!isset(self::$connections[$db_type])){
                self::$connections[$db_type] = new self($db_type, $id);
            }

            return self::$connections[$db_type];
        }

        public static function query($query) {

            self::$connections[self::$db_type]->stmt = self::$connections[self::$db_type]->dbh->prepare($query);
            return self::$connections[self::$db_type];
        }

        public function bind($pos, $value, $type = null) {

            if( is_null($type) ) {
                switch( true ) {
                    case is_int($value):
                        $type = PDO::PARAM_INT;
                        break;
                    case is_bool($value):
                        $type = PDO::PARAM_BOOL;
                        break;
                    case is_null($value):
                        $type = PDO::PARAM_NULL;
                        break;
                    default:
                        $type = PDO::PARAM_STR;
                }
            }

            self::$connections[self::$db_type]->stmt->bindValue($pos, $value, $type);
            return self::$connections[self::$db_type];
        }

        public function execute() {
            return self::$connections[self::$db_type]->stmt->execute();
        }
    }

Is it ok for multiple DB connections or not?

A quick recap from SO

If you want to implement the Singleton pattern (first read about SOLID, though, and pay special attention to injection), make the constructor private.
Your query method is static, why? it defaults (without the user being able to do anything about this) to the last connection that was established. Are you sure that's what the user wanted? Of course not! But then again, all of the other methods, like execute exhibit the same behaviour, so Everybody will end up working on the same connection, until they run into trouble and revert to using their own instances of PDO.

As ever, everything I have to say about custom wrapper classes around an API like PDO offers, can be read here. What I think of DB wrappers in general can be found here.

If you want to have all current connections available globally, then your code should shift towards a Factory, not a Singleton pattern:

class Factory
{
    private static $connections = array();
    public static function getDB($host, array $params)
    {
        if (!isset(self::connections[$host]))
        {
             self::connections[$host] = new DB($params);
        }
        return self::connections[$host];
    }
}


Check if more engineers are happy than not happy I wish to use this sample to make my code more readable. Could you share with me your ideas on method naming, variable naming, if-statements etc.

It would be great if you have any URLs.

private boolean areEngineersHappy (List<Engineer> engineers) {
  int notHappyEngineers = 0;
  int happyEngineers = 0;
  for (Engineer engineer : engineers) {
    boolean notHappy = !engineer.isHappy();
    if (!notHappy) {
      happyEngineers++;
    }
    if (notHappy) {
      notHappyEngineers++;
    }
  }
  return happyEngineers > notHappyEngineers;
}

Consider

private boolean isMajorityHappy(List<Engineer> engineers) {
    int netHappiness = 0;

    for (Engineer engineer : engineers) {
        if (engineer.isHappy()) {
            netHappiness++;
        } else {
            netHappiness--;
        }
    }

    return netHappiness > 0;
}

Name the method based on what it does.  What it does is check if the majority of the members of the list are happy.  The fact that the list is of engineers is mostly beside the point.

I would expect areEngineersHappy to return false if any engineer is unhappy.

Note that we don't need two variables to tell if the majority is happy.  We can use a single variable and subtract for unhappy members of the list.  If netHappiness is positive, then the majority of the members of the list are happy.

I would try to avoid constructs like if (! notHappy) {.  It would be much simpler to change the logic around so that you can say if (happy) { instead.

You don't need to say if (happy) and if (! happy).  This is exactly the situation for which an else is designed.


Leetcode 1603. Design Parking System Description of the problem:

Design a parking system for a parking lot. The parking lot has three kinds of parking spaces: big, medium, and small, with a fixed number of slots for each size.
Implement the ParkingSystem class:
ParkingSystem(int big, int medium, int small) Initializes object of the ParkingSystem class. The number of slots for each parking space are given as part of the constructor.
bool addCar(int carType) Checks whether there is a parking space of carType for the car that wants to get into the parking lot. carType can be of three kinds: big, medium, or small, which are represented by 1, 2, and 3 respectively. A car can only park in a parking space of its carType. If there is no space available, return false, else park the car in that size space and return true.

Below is my code, I would appreciate any suggestions on how to improve the code.
#include <iostream>

class ParkingSystem {

public:
    ParkingSystem(int big, int medium, int small) :
    m_big_max(big),
    m_medium_max(medium),
    m_small_max(small),
    m_big_curr(0),
    m_medium_curr(0),
    m_small_curr(0)
    {}

    bool addCar(int carType) {
        if (carType == 1) {
            // big car
            if (m_big_curr == m_big_max) {
                return false;
            } else {
                m_big_curr++;
            }
        } else if (carType == 2) {
            // medium car
            if (m_medium_curr == m_medium_max) {
                return false;
            } else {
                m_medium_curr++;
            }
        } else {
            // small car
            if (m_small_curr == m_small_max) {
                return false;
            } else {
                m_small_curr++;
            }
        }

        return true;
    }

private:
    int m_big_max;
    int m_medium_max;
    int m_small_max;

    int m_big_curr;
    int m_medium_curr;
    int m_small_curr;

};

int main() {

    ParkingSystem parking_system(1, 1, 0);

    std::cout << "parking_system.addCar(1) " << parking_system.addCar(1) << std::endl;
    std::cout << "parking_system.addCar(2) " << parking_system.addCar(2) << std::endl;
    std::cout << "parking_system.addCar(3) " << parking_system.addCar(3) << std::endl;
    std::cout << "parking_system.addCar(1) " << parking_system.addCar(1) << std::endl;

    return 0;
}

Observation
You can simplify this using arrays.
You don't need two numbers for each type (max and current). Simply track the number of open spots (per type) and count down.
if you are not using arrays consider a switch statement.
Code Review:
Prefer "\n" over std::endl.
If your main() does not return anything but 0 at the end.
Then leave out the return. It is auto added by the compiler.
Re-Design
template<std::size_t typeCount = 3>
class ParkingSystem
{
    int openSlots[typeCount];

    public:
        template<typename... Args>
        ParkingSystem(Args... args)
            : openSlots{args...}
        {}

        bool addCar(int type)
        {
            if (openSlots[type - 1] == 0) {
                return false;
            }
            --openSlots[type - 1];
            return true;
        }
};

int main()
{

    ParkingSystem parking_system(1, 1, 0);

    std::cout
        << "parking_system.addCar(1) " << parking_system.addCar(1) << "\n"
        << "parking_system.addCar(2) " << parking_system.addCar(2) << "\n"
        << "parking_system.addCar(3) " << parking_system.addCar(3) << "\n"
        << "parking_system.addCar(1) " << parking_system.addCar(1) << "\n";
}


Speeding up repetitive initialisation with structured data? I need to speed up this code: it creates a multilinear function. I considered the below methods but no idea which with the largest benefit.

replacing dummy variables with things such as anonymous functions
having all code in the same file (my instructor's hint)
improving the looping: getting rid of num2str
other?

I need to repeatedly create this kind of initialisations and I am trying to get it as fast as possible. It may be that the reason for the slowness of my code lies somewhere else, not this code, say it if you think so! There may be some elegant functional way to write this fast and more compactly. I already know that the whole code is probably slow because of the mlfnneg but in order to speed it up I need to make sure the bug is not early ie on this code aka during the initialisation of test-cases.

 %Creating a mlf to check the negativity of the multilinear function

 % Example: 0<x_i<i/Density
 lbs_str=containers.Map;  % I need variables here only to initialise mlf at the end
 ubs_str=containers.Map;
 terms=containers.Map;

 for i = 1:100
    i=num2str(i); % possible to make this more elegant? contairers.Map requires string
    lbs_str(i)=0;
    ubs_str(i)=i/100;
    terms(i)=struct('coeff',1,'vars',{{i}});
    % Tested catstruct [1] to append new terms to the mlf_terms but not intended:
    % it does not do {terms('1'), terms('2'),...} aka {terms.values}
 end

 lbs=mlfpoint(lbs_str);
 ubs=mlfpoint(ubs_str);
 mlf_terms=struct('const',0,'terms',{terms.values});
 mlf=mlfcreate(mlf_terms);

 mlfnneg(lbs,ubs,mlf)

You should be able to prevent casting to a string 100 times.

First create all strings in a cell array, then just loop over the strings.

Though it does not show up on the profiler, this looks higly dubious:

i=num2str(i)
ubs_str(i)=i/100;

Now you will get the character code of 'i' and divide that by 100.


Fetch data from database either with a stored procedure or a query I have this method, GetDataAsync(), that grabs data from the database using a stored procedure if sqlToRun Dictionary contains the table name. If not, then there is no stored procedure to run and just grab the table from the db directly (no SP) and convert it to a C# object to use.

Here's the code:

private async Task<Response> GetDataAsync(string table, int? start, int? end)
{
    if (_sqlToRun.ContainsKey(table))
    {
        var sql = _sqlToRun[table];
        var data = new List<Dictionary<string, object>>();
        var resp = new Response();

        using (var conn = new SqlConnection(_ConnectionString))
        {
            var cmd = new SqlCommand(sql, conn) { CommandType = CommandType.StoredProcedure };

            var startParam = new SqlParameter
            {
                ParameterName = "@startRowNum",
                Value = start,
                SqlDbType = SqlDbType.Int
            };

            cmd.Parameters.Add(startParam);

            var endParam = new SqlParameter
            {
                ParameterName = "@endRowNum",
                Value = end,
                SqlDbType = SqlDbType.Int
            };

            cmd.Parameters.Add(endParam);
            var outputParam = cmd.Parameters.Add("@recordCount", SqlDbType.Int);
            outputParam.Direction = ParameterDirection.Output;

            await conn.OpenAsync();
            using (var rdr = await cmd.ExecuteReaderAsync())
            {
                while (await rdr.ReadAsync())
                {
                    var dictionary = new Dictionary<string, object>();
                    foreach (var i in Enumerable.Range(0, rdr.FieldCount))
                    {
                        var dbValue = string.IsNullOrWhiteSpace(rdr.GetValue(i).ToString()) == true
                            ? string.Empty
                            : rdr.GetValue(i);

                        dictionary.Add(rdr.GetName(i), dbValue);
                    }

                    data.Add(dictionary);
                }
            }

            Int32.TryParse(cmd.Parameters["@recordCount"].Value.ToString(), out var recordCount);
            resp.Data = data;
            resp.RecordCount = recordCount;
        }

        return resp;
    }
    else
    {
        var data = new List<Dictionary<string, object>>();
        var resp = new Response();

        using (var conn = new SqlConnection(_ConnectionString))
        {
            string sql = "Select * from " + table;
            SqlCommand cmd = new SqlCommand(sql, conn);

            await conn.OpenAsync();
            using (var rdr = await cmd.ExecuteReaderAsync())
            {
                while (await rdr.ReadAsync())
                {
                    var dictionary = new Dictionary<string, object>();
                    foreach (var i in Enumerable.Range(0, rdr.FieldCount))
                    {
                        var dbValue = string.IsNullOrWhiteSpace(rdr.GetValue(i).ToString()) == true
                            ? string.Empty
                            : rdr.GetValue(i);

                        dictionary.Add(rdr.GetName(i), dbValue);
                    }

                    data.Add(dictionary);
                }
            }

            Int32.TryParse(cmd.Parameters["@recordCount"].Value.ToString(), out var recordCount);
            resp.Data = data;
            resp.RecordCount = recordCount;
        }

        return resp;
    }
}

Where Response is below:

public class Response
{
    public int RecordCount { get; set; }
    public List<Dictionary<string, object>> Data { get; set; }
}

How can I improve this code and refactor it?

My C# is quite rusty, so take everything I'm saying with a grain of salt; I stand by the general principles of what I'm writing, but the syntax/style might be a little off.

The first problem is that the two branches of your function are identical except for the SqlCommand being used. You should factor out how you create the SqlCommand and then do the rest the same way. For example,

private SqlCommand GetSqlCommandFromTableIfExists(string table, int? start, int? end, SqlConnection conn, string default)
{
    SqlCommand cmd;
    if (_sqlToRun.ContainsKey(table))
    {
        string sql = _sqlToRun[table];
        cmd = new SqlCommand(sql, conn) { CommandType = CommandType.StoredProcedure };

        var startParam = new SqlParameter
        {
            ParameterName = "@startRowNum",
            Value = start,
            SqlDbType = SqlDbType.Int
        };

        cmd.Parameters.Add(startParam);

        var endParam = new SqlParameter
        {
            ParameterName = "@endRowNum",
            Value = end,
            SqlDbType = SqlDbType.Int
        };

        cmd.Parameters.Add(endParam);
        var outputParam = cmd.Parameters.Add("@recordCount", SqlDbType.Int);
        outputParam.Direction = ParameterDirection.Output;
    }
    else
    {
        // You could easily not make the default a parameter if you prefer
        cmd = new SqlCommand(default, conn);
    }

    return cmd;
}

Then your function becomes

private async Task<Response> GetDataAsync(string table, int? start, int? end)
{

    var data = new List<Dictionary<string, object>>();
    var resp = new Response();

    using (var conn = new SqlConnection(_ConnectionString))
    {
        var cmd = GetSqlCommandFromTableIfExists(table, start, end, conn, "SELECT * FROM " + table);
        await conn.OpenAsync();
        using (var rdr = await cmd.ExecuteReaderAsync())
        {
            while (await rdr.ReadAsync())
            {
                var dictionary = new Dictionary<string, object>();
                foreach (var i in Enumerable.Range(0, rdr.FieldCount))
                {
                    var dbValue = string.IsNullOrWhiteSpace(rdr.GetValue(i).ToString()) == true
                        ? string.Empty
                        : rdr.GetValue(i);

                    dictionary.Add(rdr.GetName(i), dbValue);
                }

                data.Add(dictionary);
            }
        }

        Int32.TryParse(cmd.Parameters["@recordCount"].Value.ToString(), out var recordCount);
        resp.Data = data;
        resp.RecordCount = recordCount;
    }

    return resp;
}

Type unsafety is bad

Right now you're using a raw object as the value-type in your dictionary and Response - why? I assume its because this is a general function that is independent of the data you're grabbing, but it means you're losing out on what data you're adding and any kind of type safety. I think making the function and your Response type generic, and tweaking your implementation, you can ensure type-safety.

Don't make some random data object; just use resp.Data.


Python class for permutations I am trying to develop a program which will solve any permutation-based puzzle such as 15 puzzle or Rubik's cube, so there will be a follow-up question about class that actually solves puzzle. Here I ask about the class which is essential to this task: Permutation class.

class Permutation(object):
    @staticmethod
    def get_letter_id(num):
        """
        Returns an identity permutation of first :num: number of
        uppercase ASCII letters.
        """
        letters = string.ascii_uppercase[:num]

        return Permutation(tuple(zip(letters, letters)), "Id" + str(num))

    def __init__(self, perm, label=""):
        """
        :param perm: List of tuples of type (A, B), which shows that in given sequence of symbols
                        symbol A will be replaced by symbol B
        :param label: Label for better (or worse) representation.
        """
        top_symbols = [p[0] for p in perm]
        bottom_symbols = [p[1] for p in perm]

        top_symbols = set(top_symbols)
        bottom_symbols = set(bottom_symbols)

        if top_symbols != bottom_symbols:
            raise Exception("Incorrect Permutation")

        self._perm = list(perm)
        self._perm.sort(key=lambda x: x[0])
        self._label = str(label)

    @property
    def inverse(self):
        p = [(p[1], p[0]) for p in self._perm]
        p.sort(key=lambda x: x[0])
        return Permutation(p, "Inverse({0})".format(self._label))

    @property
    def parity(self):
        return self._inversions % 2

    @property
    def symbols(self):
        return sorted([i for i, j in self._perm])

    @property
    def _inversions(self):
        res = 0
        top = [i for i, j in self._perm]
        bottom = [j for i, j in self._perm]
        for i in range(len(top)):
            for j in range(i, len(top)):
                if bottom[i] > bottom[j]:
                    res += 1
        return res

    def print_carrier(self):
        [print("{0} -> {1}".format(mutation[0], mutation[1])) for mutation in self._perm if mutation[0] != mutation[1]]

    def _find_symbol(self, symbol):
        for i in range(len(self._perm)):
            if self._perm[i][0] == symbol:
                return i
        raise Exception("Can't find given symbol in permutation.")

    def __call__(self, sequence):
        """
        Applies this permutation to the given sequence of symbols.
        For performance reasons this permutation assumed applicable to given sequence.
        """
        return tuple([self._perm[self._find_symbol(symbol)][1] for symbol in sequence])

    def __eq__(self, permutation):
        if type(permutation) != Permutation:
            return False
        condition1 = self.symbols == permutation.symbols
        condition2 = self.__call__(self.symbols) == permutation(self.symbols)
        return condition1 and condition2

    def __mul__(self, permutation):
        first = [mutation[0] for mutation in self._perm]
        second = self.__call__(permutation(first))
        return Permutation(tuple(zip(first, second)), str(self) + " * " + str(permutation))

    def __repr__(self):
        return self._label

Performance should not be ignored too.

def __init__(self, perm, label=""):
        """
        :param perm: List of tuples of type (A, B), which shows that in given sequence of symbols
                        symbol A will be replaced by symbol B
        :param label: Label for better (or worse) representation.
        """
        top_symbols = [p[0] for p in perm]
        bottom_symbols = [p[1] for p in perm]

        top_symbols = set(top_symbols)
        bottom_symbols = set(bottom_symbols)

        if top_symbols != bottom_symbols:
            raise Exception("Incorrect Permutation")

That's an important check, but not a sufficient one. If I pass [(1, 1), (2, 1), (2, 2)] then top_symbols and bottom_symbols will be the same set, but it's not a valid permutation.

    @property
    def inverse(self):
        p = [(p[1], p[0]) for p in self._perm]
        p.sort(key=lambda x: x[0])
        return Permutation(p, "Inverse({0})".format(self._label))

Why sort p? The constructor does that anyway.

    @property
    def symbols(self):
        return sorted([i for i, j in self._perm])

Why sorted? The constructor does that already.

Also, why do some methods use [something involving p[i] for p in self._perm] and others [something involving i and/or j for i, j in self._perm])?

    @property
    def _inversions(self):
        res = 0
        top = [i for i, j in self._perm]
        bottom = [j for i, j in self._perm]
        for i in range(len(top)):
            for j in range(i, len(top)):
                if bottom[i] > bottom[j]:
                    res += 1
        return res

This takes \$O(n^2)\$ time. There are \$O(n \lg n)\$ algorithms to do it, so since you say that performance is a concern you probably want to revisit this method.

    def print_carrier(self):
        [print("{0} -> {1}".format(mutation[0], mutation[1])) for mutation in self._perm if mutation[0] != mutation[1]]

The name is somewhat opaque: carrier?!

The implementation is also somewhat hacky. As far as I can tell without executing to double-check, this is using a list comprehension purely to iterate and produce side effects. Ugh.

    def _find_symbol(self, symbol):
        for i in range(len(self._perm)):
            if self._perm[i][0] == symbol:
                return i

Whoa! If this method is used much at all, it's going to be a major performance hit. IMO you need to revisit the way the data is stored: to do this efficiently you want a dict.

Also: why return i? Under what circumstances would you call this method wanting anything other than self._perm[i][1]?

    def __eq__(self, permutation):
        if type(permutation) != Permutation:
            return False
        condition1 = self.symbols == permutation.symbols
        condition2 = self.__call__(self.symbols) == permutation(self.symbols)
        return condition1 and condition2

Why not just do an equality check on _perm and permutation._perm?

    def __mul__(self, permutation):
        first = [mutation[0] for mutation in self._perm]
        second = self.__call__(permutation(first))
        return Permutation(tuple(zip(first, second)), str(self) + " * " + str(permutation))

Firstly, DRY: first is self.symbols. Secondly, IMO a comprehension for second would be more clearly checkable against the definition of permutation composition. Thirdly, why tuple(...) in the call to the constructor? If zip works in get_letter_id it should work here too.


Computing predicates of multiple items avoiding duplication of iteration code I am breaking my head with how to get rid of semantic duplication(Code that is syntactically the same but does different things).

I can't find anywhere a post or something that mentions a bit how to refactor this kind of duplication. All I found was this:
Duplicate Code and Ceremony in Java but it does not go into detail on how to refactor it.

This is the code that is causing me problem:

public class TeamValidator {

      public boolean isThereALeader(List<Member> team) {
           Iterator<Member> iterator = team.iterator();
           while(iterator.hasNext()) {
              Member member = iterator.next();
              String role = member.getRole();
              if(role.equals("Leader"))
                return true;
           }
           return false;
      }

      public boolean areThereAtLeast2NewJoiners(List<Member> team) {
            int amountOfNewJoiners = 0;
            for(Member member:team) {
                if(amountOfNewJoiners == 2)
                    return true;
                DateTime aMonthAgo = DateTime.now().minusMonths(1);
                if(member.startingDate().isAfter(aMonthAgo)) {
                   amountOfNewJoiners++;
                }
            }
            return false;
      }
}

In these two methods there is semantic duplication, because both iterate a list and also check some condition/s. Any idea how could I make this semantic duplication disappear?
I would really appreciate some tip or suggestion on how to refactor this.

Key here is to identify that both functions check whether there are at least n members that match a certain condition.

So we introduce a Matcher interface :

public interface Matcher<T> {

    boolean matches(T candidate);

}

Which we then use to make a method hasAtLeastNMatches(). And both methods can be implemented by calling that.

public class TeamValidator {

    public boolean isThereALeader(List<Member> team) {
        return hasAtLeastNMatches(team, Is.Leader, 1);
    }

    public boolean areThereAtLeast2NewJoiners(List<Member> team) {
        final DateTime aMonthAgo = DateTime.now().minusMonths(1);
        return hasAtLeastNMatches(team, Is.NewJoiner, 1);
    }

    private boolean hasAtLeastNMatches(Iterable<Member> members, Matcher<Member> condition, int minimumNumberOfMatches) {
        int count = 0;
        for (Member member : members) {
            if (condition.matches(member) && ++count >= minimumNumberOfMatches) {
                return true;
            }
        }
        return false;
    }

    private static enum Is implements Matcher<Member> {
        Leader {
            @Override
            public boolean matches(Member candidate) {
                return candidate.getRole().equals("Leader");
            }
        },

        NewJoiner {
            @Override
            public boolean matches(Member candidate) {
                return candidate.startingDate().isAfter(DateTime.now().minusMonths(1));
            }
        }
    }

}

Method hasAtLeastNMatches() can obviously also be reused ouside this class. In fact this kind of reuse is so common that several open source code projects offer these kinds methods and interfaces: e.g. Apache commons collections and Guava . In fact what I called Matcher they both call Predicate.