Javascript/Typescript anagram String search I've created a random word generator which allows me to create random words/anagrams.

Then I've created Anagram search app to find the angarams when user start typing in inbox.

Could you review this with SOLID principles, Performance, Productions level ready scenarios in mind? Please be critical

// Generate random words to be used
class RandomWordGenerator {

  private wordList = [];

  constructor() {
    this.generate().createAnagrams();
  }
  generate() {
    for (let i = 0; i < 10000; i++) {
      this.wordList.push(Math.random().toString(32).substr(2, Math.floor(Math.random() * 10 + 1)));
    }

    return this;

  }

  createAnagrams() {
    let anagrams = this.wordList.slice(0, this.wordList.length/2).map((str) => {
      return str.substr(2, str.length / 2).split('').reverse().join('')
    })
    this.wordList = [...this.wordList, ...anagrams];

    return this;
  }

  getWordList() {
    return this.wordList;
  }

}

// Create Search index
class SearchIndex {

  private searchResults: Array<String>;
  private indexMap = {};
  private indexMapCharArray = [];

  constructor(private wordService: RandomWordGenerator) {
    this.searchResults = this.wordService.getWordList();
    this.sanitizeList().createIndex();
    console.log(this.searchResults);
  }

  sanitizeList() {
    this.searchResults = this.searchResults.filter((str) => !!str).sort();
    this.searchResults = Array.from(new Set(this.searchResults));

    return this;
  }

  createIndex() {
    this.indexMap = {};
    this.indexMapCharArray = [];

    for (let i = 0; i < this.searchResults.length; i++) {
      let sortedStrArr = this.searchResults[i].split('').sort();
      let sortedStr = sortedStrArr.join('')

      if (!!this.indexMap[sortedStr]) {
        this.indexMap[sortedStr].push(this.searchResults[i]);
      } else {
        this.indexMap[sortedStr] = [this.searchResults[i]];

      }
    }

    console.log(this.indexMap);
  }

  getSearchResults(str) {
    let result = this.indexMap[str];
    console.log('SEARCHING');
    return result;;
  }

}

// App
class App {

  constructor(private resultIndex: SearchIndex) {
  }

  start() {
    this.bindEvents();
  }

  bindEvents() {
    let element = <HTMLInputElement>document.querySelector('.search-field');

    element.addEventListener('keyup', (e) => {
      let target = <HTMLInputElement>e.target;
      console.log(target.value, e);
      console.log(this.resultIndex.getSearchResults(target.value))
    })
  }
}

window.onload = () => {
  var el = document.getElementById('content');
  var generator = new RandomWordGenerator();

  let searchIndex = new SearchIndex(generator);
  let app = new App(searchIndex);

  app.start();

};

This is quite a nice idea for a learning project.

I created a jsfiddle to test your code. Initially I was not able to run your program, as there were a few minor bugs, most relevant:

When you use element.addEventListener, the this context of your App instance is not captured, even though you are using an arrow function. This leads to this.searchIndex being undefined when the keyup event handler is executed. I fixed this, by introducing a local variable before the handler:

const index = this.resultIndex;
element.addEventListener('keyup', (e) => {
  let target = <HTMLInputElement>e.target;
  console.log(target.value, e);
  console.log(index.getSearchResults(target.value))
})

You can also bind the this context to your handler explicitly, if you prefer.

element.addEventListener('keyup', ((e) => {
  let target = <HTMLInputElement>e.target;
  console.log(target.value, e);
  console.log(this.resultIndex.getSearchResults(target.value))
}).bind(this))

I wasn't able to transpile the arrow function with direct return of the double-negated str and changed it to

(str) => { return !!str; }

even though

str => str

does the same (with respect to the filter function.)

Coding Style

I have some remarks regarding your coding style.

You use semicolons inconsistently. I would prefer to always use semicolons at the end of a statement.
const vs. let: You should use const, whenever you don't intend to reassign the given variable, which is almost always the case - except string concatenations and declaration before assignment.
implicit return types (e.g. in RandomWordGenerator::getWordList()) are not very helpful. If you make the return type explicit, i.e. getWordList() : string[], the compiler will enforce all code paths to return a value, which helps to reduce programming errors.
implicit parameter types: the SearchIndex::getSearchResults(str) method does not specify the parameter type explicitly, but rather implicitly by naming the parameter str. Use getSearchResults(searchTerm : string) instead and two things are resolved at the same time: First your parameter type is specified, and second, your parameter name is way more relevant in the context of the usage of this method. This is a problem that occurs more often in your code, also with variables. sortedStrArr could be defined as sortedCharacters : string[] and sortedStr could be sorted : string. You could also make the member variable types explicit. indexMap should be index : {[key : string] : string[]}
using private methods for methods that don't need to be accessible from the outside. In your code all of SearchIndex's methods, except getSearchResults could be private.
unused code: You call document.getElementById('content'); without ever using the returned element. The member variable indexMapCharArray is reset to an empty array, but never used.

I am not a big fan of the method chaining in RandomWordGenerator either. The chaining as you implement it does not prevent the user of your code to call createAnagrams before calling createWords.

SOLID Principles

A scenario for extending your application could be to replace the random word generation with e.g. an English dictionary. I assume that in the real world use case one would also only want to find the anagrams already present in the dictionary.

Currently this is not possible without touching the SearchIndex class, which is unfavorable coupling. Also I'm not very satisfied with the fact, that SearchIndex does need to call the getWordList() method of RandomWordGenerator.

I would recommend the following signature of SearchIndex's constructor:

constructor(private wordList : string[])

This way you don't depend on the way the word list is generated to provide it to the SearchIndex anymore.

If you want to allow the user to provide multiple sources of word lists, you could have an interface for this:

interface IWordListSource {
    getWordList() : string[];
}

and make RandomWordGenerator, DictionaryWordSource or other implementations implement that interface. It would be okay to inject that interface into the SearchIndex, like constructor(private wordListSource : IWordListSource) instead of directly providing the word list, if you prefer.

Your App class is coupled to a specific document structure, i.e. the existence of an input element with the class search-field is required. I would recommend to inject this element into the App instance either when calling start() or when constructing the instance.

One should also think about, whether logging to the browser's console is the best way of outputting information to the user of your program.


Code to check for memory leaks I want to write a header file for c++. When included in a program, the header file overloads the new and delete operators so that it can count the number of allocations and deallocations. The header file will also print out the number of allocations and deallocations when the program exits. Is this the proper way to go about doing that? Will this code cause any problems or leave any gaps? The header file is only meant to be included when debugging.

#pragma once
#include <cstdint>
#include <cstdlib>

size_t __allocations = 0;
void* operator new(std::size_t sz) {
    ++__allocations;
    return std::malloc(sz);
}

size_t __array_allocations = 0;
void* operator new[](std::size_t sz) {
    ++__array_allocations;
    return std::malloc(sz);
}
size_t __deallocations = 0;
void operator delete(void* ptr) noexcept
{
    ++__deallocations;
    std::free(ptr);
}
size_t __array_deallocations = 0;
void operator delete[](void* ptr) noexcept
{
    ++__array_deallocations;
    std::free(ptr);
}

#include <iostream>
struct EnsureOutput
{
    ~EnsureOutput()
    {
        using namespace std;
        cout << "Allocations:   " << __allocations << endl;
        cout << "Deallocations: " << __deallocations << endl;
        cout << "Array Allocations:   " << __array_allocations << endl;
        cout << "Array Deallocations: " << __array_deallocations << endl;
        if(__allocations == __deallocations && __array_deallocations == __array_deallocations)
        {
            cout << "No leaks discovered" << endl;
        }
        else
        {
            cout << "You have a memory leak." << endl;
        }
    }
};
EnsureOutput o = {};

Identifiers containing two consecutive underscores are reserved for use by the C++ implementation for any purpose.  That means they can even be macros!  Don't use __ in your identifiers; if you want to keep them separate from user code, you can use a suitable namespace (ideally an anonymous namespace).

Another observation is that this code only keeps count of operations - a double delete could easily cancel a leak and lead your code to report "No leaks discovered".

Although you've kept using namespace std; within a sensible, small scope, I think I'd still prefer using std::cout; using std::endl or just fully-qualified names there.  Consider using std::cerr instead for this kind of information that's not part of normal program output - that may be essential when used in a pipeline.


JSON conversion to single quotes The JSON standard uses double quotes for key names and string values, such as the following:

{"one":1,"two":"two"}

However, the current project I am working on requires JSON-encoded objects to use single quotes instead of double quotes. (In this case {'one':1,'two':'two'}). Right now, there is no reasonable way to get rid of this requirement. I am using the following code to convert a JSON-encoded string from double quotes to single quotes:

var encoded = JSON.stringify(...);
encoded = encoded.replace(/\\"/g, '"')
    .replace(/([\{|:|,])(?:[\s]*)(")/g, "$1'")
    .replace(/(?:[\s]*)(?:")([\}|,|:])/g, "'$1")
    .replace(/([^\{|:|,])(?:')([^\}|,|:])/g, "$1\\'$2");

This code replaces double quotes with single quotes on key names and string values, and handles possible quote escaping issues (such as {"\"key\"": "'value'"}, which is replaced with {'"key"', '\'value\''}).

Is this code enough to guarantee that, for any given object, a valid JSON-like string with single quotes will be generated? In other words, is there any way to break this code such that the result is not valid?

P.S: if you are interested, the reason that JSON strings must have single quotes is that I am working with Cassandra's map type, which doesn't accept double quotes.

No, that code is not enough to handle any valid JSON.

Here are two examples where the result is not only different from the expected, but also unparsable:

The string:

{"hello":"wor\":ld"}

will be converted to:

{'hello':'wor':ld'}

The string:

{"hello":"w''orld"}

will be converted to:

{'hello':'w\''orld'}


Seemingly redundant conditionals...or not? I'm working on some code that has the following pattern of conditionals inside it. It doesn't look great, but it doesn't look terrible.

Please, shed some light on the underlying reason this is not passing my smell test. I'd love to see some alternatives. I'm guessing it's because it's completely ignoring a certain permutation.

Note: I'm asking this question with the intent of bettering the code, not code golf.

public class RoleManager
{
    public string Role { get; private set; }

    internal void SetRole(bool isManager, bool hasBackendAccess)
    {
        if (isManager && hasBackendAccess)
        {
            Role = "FullAccess";
        }
        else if (isManager)
        {
            Role = "Manager";
        }
        else if (hasBackendAccess)
        {
            Role = "LimitedBackend";
        }
    }
}

You have 3 different role values and 1 for not assigning anything, so you'll somehow need 4 discrete values to differentiate. But you could rewrite as:

if (isManager)
    Role = hasBackendAccess ? "FullAccess" : "Manager";
else if (hasBackendAccess)
    Role = "LimitedBackend";


More efficient way to retrieve data from JSON I am using the jsmn JSON parser (source code) to get extract data from a JSON string.  jsmn stores the data in tokens that just point to the token boundaries in the JSON string instead of copying the data. For example, jsmn will create tokens like:

Object [0..31]
String [3..7], String [12..16], String [20..23]
Number [27..29]

This method is used to retrieve the actual characters between those values (for String objects):

char* getTextFromJSON(const char *json)
{
    json_parser p;
    jsontok_t tokens[15];
    jsontok_t key;

    if (!json) return NULL;

    initJsonParser(&p);
    parseJson(&p, json, tokens, sizeof(tokens) / sizeof(tokens[0]));
    for(unsigned int i = 0; i < sizeof(tokens) / sizeof(tokens[0]); ++i)
    {
        key = tokens[i];
        unsigned int length = key.end - key.start;
        char keyString[length + 1];
        memcpy(keyString, &json[key.start], length);
        keyString[length] = '\0';
        if(!strcmp(keyString, "utterance"))
        {
            key = tokens[i + 1];
            length = key.end - key.start;
            char* keyString = (char*) malloc(length + 1);
            memcpy(keyString, &json[key.start], length);
            keyString[length] = '\0';
            return keyString;
        }
    }
    return NULL;
}

Here are some JSON examples that would be thrown into the parser:

{"status":0,"id":"432eac38858968c108899cc6c3a4bade-1","hypotheses" [{"utterance":"test","confidence":0.84134156}]}

{"status":5,"id":"695118aaa3d01dc2ac4aa8054d1e5bb0-1","hypotheses":[]}

There are a few issues with getTextFromJSON involving use of pointers, code
duplication and use of an inappropriate function.

Pointers first.  In this code:

jsontok_t tokens[15];
jsontok_t key;
...
key = tokens[i];

you are copying the token when you could be using a pointer:

jsontok_t *key = &tokens[i];

Copying is obviously more costly than pointing.  Also note that it is often
best to define the variable at its first point of use.

There are two points of duplication, one determining the size of the token
array and one concerned with copying the string.  Instead of this:

jsontok_t tokens[15];
...
parseJson(&p, json, tokens, sizeof(tokens) / sizeof(tokens[0]));
for(unsigned int i = 0; i < sizeof(tokens) / sizeof(tokens[0]); ++i)

I would do either:

#define N_TOKENS 15
jsontok_t tokens[N_TOKENS];
...
parseJson(&p, json, tokens, N_TOKENS);
for(unsigned int i = 0; i < N_TOKENS; ++i)

or

jsontok_t tokens[15];
#define N_TOKENS (sizeof tokens / sizeof tokens[0])  // or const int N_TOKENS inseatd of #define
...etc

The string copying code-duplication is actually unnecessary if you use a
better standard library function...

Instead of using strcmp to compare the string (which involves constructing
the string because it lacks a \0):

    if(!strcmp(keyString, "utterance"))

use memcmp.  And you can use strndup to create a copy of the key.  If you
don't have strndup which is non-standard, you can easily write it (in a
separate function).

These changes wont speed it up much, but are certainly cleaner:

char* getTextFromJSON(const char *json)
{
    if (!json) return NULL;

    json_parser p;
    #define N_TOKENS 15  // this normally would be at the start of the file
    jsontok_t tokens[N_TOKENS];

    initJsonParser(&p);
    int n = parseJson(&p, json, tokens, N_TOKENS);
    for (int i = 0; i < n; ++i) {
        jsontok_t *key = &tokens[i];
        if (!memcmp("utterance", &json[key->start], (size_t) (key->end - key->start))) {
            ++key;
            return strndup(&json[key->start], (size_t)(key->end - key->start));
        }
    }
    return NULL;
}


Assigning values to a particular table column I have a set of Excel data which I read and assign to corresponding columns of an Excel table.  In our Excel we have provided the user the option to rearrange the Excel column fields at runtime.

int rowIndex =int.Parse(ExcelSettingManager.ItemLine);

int lastItem = numberOfFilledRows();

numberOfFilledRows();

int maxIndex = rowIndex + lastItem;

MSExcel.Range cell = null;
while (rowIndex < maxIndex)
{
    DataRow dataRow = ExcelDataManager.Instance.DocumentItemTable.NewRow();
    cell = this.worksheet.Range[ExcelSettingManager.LineItemNo + rowIndex.ToString()];
    if (cell.Value2 == null)
    {
        return;
    }

    dataRow["LineItemNo"] = cell.Value;
    ExcelUtilities.ReleaseResources(cell);

    if (ExcelSettingManager.CostCenter != string.Empty)
    {
        cell = this.worksheet.Range[ExcelSettingManager.CostCenter + rowIndex.ToString()];
        dataRow["CostCenter"] = cell.Value;
        ExcelUtilities.ReleaseResources(cell);
    }

    if (ExcelSettingManager.GLAccount != string.Empty)
    {
        cell = this.worksheet.Range[ExcelSettingManager.GLAccount + rowIndex.ToString()];
        dataRow["GeneralLedgerAccount"] = cell.Value;
        ExcelUtilities.ReleaseResources(cell);
    }

    if (ExcelSettingManager.Description != string.Empty)
    {
        cell = this.worksheet.Range[ExcelSettingManager.Description + rowIndex.ToString()];
        dataRow["ItemText"] = cell.Value;
        ExcelUtilities.ReleaseResources(cell);
    }
}

internal int numberOfFilledRows()
{
    int noft = 0;
    string sNextCell = ExcelSettingManager.LineItemNo ExcelSettingManager.ItemLine;
    MSExcel.Range cell = this.worksheet.Range[sNextCell];

    while(!string.IsNullOrEmpty(cell.Value))
    {
        ExcelUtilities.ReleaseResources(cell);
        noft++;
        sNextCell = ExcelSettingManager.LineItemNo + (int.Parse(ExcelSettingManager.ItemLine) + noft).ToString();
        cell = this.worksheet.Range[sNextCell];
    }
    return noft;
}

If my Excel has 10 rows according to the current logic, it will read cell by cell each row and will assign to the corresponding Excel table column.

This is having a lot of performance impact. When there are more rows, the performance is worse.

How can I increase performance of this code?

One big problem is that your code is not complete so I have to guess. In the future please post complete working code.

There are some obvious inefficiencies in your code, though I'm not sure if the compiler will fix them for you or not, but it's still good programming practice to:

What's the point of the second numberOfFilledRows(); ?
You have the same code repeated three times, with the only difference being the two strings, one used as the index to dataRow[] and the other being put in the string concatenation. In cases like this you should factor out the common code to a separate function.
You repeatedly call rowIndex.ToString(). The compiler may be smart enough to put this in a temporary variable, but this is a clue that you're doing something the wrong way. You don't say what type worksheet is, but isn't there a method that takes two ints for the row and column numbers?
your numberOfFilledRows() is reinventing the wheel. Excel already has functions to find the end of a range (probably much faster too).

EDIT Also, the fastest way to check if a string is empty is to compare the .Length member to 0. See http://www.dotnetperls.com/empty-string

You may also want to look into StyleCop and/or FXCop.


Extracting certain records The purpose of this SQL query would be to extract the record 10192 only, because the end_time is both in the past and its foreign counterpart in the Node table still has the status 1. I don't want to select the record 10111 because it hasn't expired.

I've just finished writing this SQL query, it works! Although I'm sure it can be written better and I'm interested to here what you have to say.

SELECT MAX(l.id) AS highestID, l.nid, n.status
                        FROM mbl AS l
                        INNER JOIN node AS n
                        ON l.nid = n.nid
                        WHERE n.status = 1
                        AND (   SELECT end_time
                                FROM mbl
                                WHERE id = (    SELECT MAX(id)
                                                FROM mbl
                                                WHERE nid = l.nid
                                                GROUP BY nid
                                            )
                            ) <= NOW()
                        GROUP BY l.nid

Mlb table data:

id  | nid | start_time | end_time

105 | 5 | 20/07/2014 01:00:00 | 24/07/2014 01:00:00

589 | 52 | 20/07/2014 01:00:00 | 31/07/2015 01:32:00

10108 | 874 | 20/07/2014 01:00:00 | 24/07/2014 01:00:00

10111 | 874 | 20/07/2014 01:00:00 | 24/07/2015 01:00:00

10156 | 967 | 20/07/2012 01:00:00 | 24/07/2013 01:00:00

10161 | 967 | 20/07/2013 01:00:00 | 20/07/2014 01:00:00

10192 | 967 | 20/07/2014 01:00:00 | 24/07/2014 01:00:00

Node table data:

nid  | status

5 | 0

52 | 1

874 | 1

967 | 1

First, let's go over some problems in your original query:

SELECT MAX(l.id) AS highestID, l.nid, n.status
..............
GROUP BY l.nid

... if there's more than one n.status for a given l.nid, you get a "random" row, decided by the optimizer (and not controllable by you).  I'm presuming that you only have one row here (we'll exploit this in a bit).

(SELECT end_time
 FROM mbl
 WHERE id = (SELECT MAX(id)
             FROM mbl
             WHERE nid = l.nid
             GROUP BY nid)) <= NOW()

I'm presuming that id is still some sort of autogenerated value.  The problem is, outside of some very specific contexts, the only thing a db-generated id should be used for is its uniqueness.  Especially in situations where rows may be updated, max/min/ordering by ids rapidly becomes irrelevant.  You seem to be attempting to make sure the most-recently-added row has an end_time value in the past - if you have some sort of inserted_at or similar timestamp, that would be a better column to use.  It would also be helpful to know why you're looking for the greatest id in this case.  (However, as I don't know the other columns in you table, I'm stuck with it for the moment).

It turns out that your statement can be reduced to this:

SELECT Most_Recent.highestId, Most_Recent.nid, Node.status
FROM (SELECT nid, MAX(id) AS highestId
      FROM Mbl
      GROUP BY nid) Most_Recent
JOIN Node
  ON Node.nid = Most_Recent.nid
JOIN Mbl
  ON Mbl.nid = Most_Recent.nid
     AND Mbl.id = Most_Recent.highestId
     AND Mbl.end_time <= NOW()

.... which not only eliminates a table reference, but your original version had issues with potential RBAR (row-by-agonizing-row)/Cartesian product plans.  Note that we're still required to join to Mbl twice, with the second time being solely to make sure that the most-recent row also has an end_time value in the past (this can't be done in the subquery, because that would collect the most-recent-id of rows with end_time in the past)


Python - Faster random business date generation I wrote a program that generates some synthetic data. Everything runs fine, but right now the program is so slow that I am only generating 1000 rows of data (and it still takes 3 or so minutes). Ideally I would like to generate about 100,000 (which at the moment takes upwards of 10 minutes, I killed the program before it finished running).

I've narrowed down the problem to the way I am generating random dates. After those three lines, the rest of the program executes in a few seconds.

import numpy.random as rnd
import datetime
import pandas as pd

random_days = []

for num in range(0,n):
    random_days.append(pd.to_datetime(rnd.choice(pd.bdate_range(start_date, end_date))))

What I need is, given some number n, to generate that many dates at random from a sequence of business days (the business days part is also important). I need to convert the value to datetime because otherwise it returns a numpy timedelta64 object, which causes problems in other parts of the program.

Is there any way to improve my code to have it generate dates faster? Or do I need to settle for a small sample size?

EDIT:

Just to add a little more context:

I use this loop to generate the rest of my data:

for day in random_days:
    new_trans = one_trans(day)
    frame.append(new_trans)

frame = pd.concat(frame)

The function one_trans is the following:

def one_trans(date):

    trans = pd.Series([date.year, date.month, date.date(), fake.company(),
                       fake.company(), fake.ssn(),
                       (rnd.normal(5000000,10000)),
                       random.sample(["USD", "EUR", "JPY", "BTC"], 1)[0]],
                       index=["YEAR", "MONTH","DATE","SENDER","RECEIVER",
                         "TRANSACID","AMOUNT","CURRENCY"])

    return trans.to_frame().transpose()

EDIT 2:
This is how I implemented Vogel612's suggestion:

def rng_dates(n, start_date, end_date):
    for _ in range(n):
        yield pd.to_datetime(rnd.choice(pd.bdate_range(start_date, end_date)))

random_days = rng_dates(n, start_date, end_date)

for day in random_days:
    new_trans = one_trans(day)
    frame.append(new_trans)

frame = pd.concat(frame)

one of the really easy ways to probably make this faster is by foregoing the idea of "I generate n records and then do things with them".

Instead think something like "I generate a record and do things with it. n times".

Python has the really handy concept of iterators. Consider the following:

def rng_dates():
   while true:
       yield pd.to_datetime(rnd.choice(pd.bdate_range(start_date, end_date)))

to make this a little less infinity-ish, you could pass a number of records to it:

def rng_dates(n):
    # hat tip to Peilonrayz
    for _ in range(n):
        yield pd.to_datetime(rnd.choice(pd.bdate_range(start_date, end_date)))

this should allow some optimizations wrt. memory management, cache-misses and list-appending, which should translate into a pretty hefty speedup in larger sample-sizes


Processing XYZ data from a large file I'm new to programming (1.5 years experience at university), and I'm working on my first computer vision related project! My goal is to take a file which might have CSV (comma separated) or txt format with XYZ coordinates and process the data. I want to round the X and Y coordinates into integers and leave the Z coordinate as a floating point.

Each file ranges from 200000 to 1400000 lines of XYZ data is is formatted as so:

# information about .txt file
253.9999929 58.0428367 -21.3930063253
.9999929 59.0435773 -21.2499391255
etc.....

Do note that the file might have commas, like a CSV file.

The function that is my main bottleneck is:

Loadfile(string filename);

I'm pretty sure my bottleneck is from the ifstream for reading the file one line at a time, but I'm having trouble finding ways to improve it. One idea I had was to convert the file into an array of bytes and read split it up into different domains and give it to multiple threads to read concurrently, but I'm not sure if that's a good practice.

Here is the my code so far:

class XYZCoordinate{
public:
    int x;
    int y;
    float z;

    XYZCoordinate(int X, int Y, float Z){
    x = X;
    y = Y;
    z = Z;
    }
};

class Loadfileoutput
{
public:
    int xmin;
    int xmax;
    int ymin;
    int ymax;
    vector<XYZCoordinate> values;

    Loadfileoutput(int xmn, int xmx, int ymn, int ymx, vector<XYZCoordinate> val){
    xmin = xmn;
    xmax = xmx;
    ymin = ymn;
    ymax = ymx;
    values = val;
    }
};

Loadfileoutput Loadfile(string filename)
{
    std::ifstream in(filename);
    if (!in) {
        cout << "Cannot open input file.\n";
        exit(EXIT_FAILURE);
    }

    std::string str;
    bool isfirst = true;
    int xmin, xmax, ymin, ymax;
    vector<XYZCoordinate> file = vector<XYZCoordinate>();
    while (std::getline(in, str)) //potential cause of bottleneck
    {
        if (str[0] == '#') //.skipping the .txt file header
            continue;
        vector<string> v = vector<string>();

        boost::split(v, str, boost::is_any_of(" ,"));

        string xstr = v[0];
        string ystr = v[1];
        string zstr = v[2];
        int xint, yint;
        float x,y,z;

        stringstream(v[0]) >> x;
        xint = (int)round(x);

        stringstream(v[1]) >> y;
        yint = (int)round(y);

        stringstream(v[2]) >> z;

        XYZCoordinate temp = XYZCoordinate(xint, yint, z);
        file.push_back(temp);

        //get bounds
        if (isfirst)
        {
            xmin = xint;
            xmax = xint;
            ymin = yint;
            ymax = yint;
            isfirst = false;
        }
        else
        {
            //set xmin/max for scaling
            if (xint > xmax)
                xmax = xint;
            else if (xint < xmin)
                xmin = xint;
            //set ymin/max for scaling
            if (yint > ymax)
                ymax = yint;
            else if (yint < ymin)
                ymin = yint;
        }
    }
    Loadfileoutput output = Loadfileoutput(xmin,xmax,ymin,ymax,file);

    return output;
}

There are a number of things that may help you improve your program.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Use the required #includes

The code uses a number of #includes that are not listed.  It was not difficult to infer, but it helps reviewers if the code is complete.  After a bit of looking, it seems that this is what's needed:

#include <vector>
#include <string>
#include <iostream>
#include <fstream>
#include <sstream>
#include <cmath>
#include <boost/algorithm/string.hpp>

Prefer modern initializers for constructors

The constructor use the more modern initializer style rather than the old style you're currently using.  Instead of this:

XYZCoordinate(int X, int Y, float Z){
x = X;
y = Y;
z = Z;
}

I would write this:

XYZCoordinate(int X, int Y, float Z) : x{X}, y{Y}, z{Z} {}

However, in this case, for such a simple class, in real code I wouldn't write a constructor at all and would simply delete the line.  The class can still be initialized like this: XYZCoordinate xyz{3, 5, -23.22}; via aggregate initialization.

Use better variable names

The variable names xmax, xmin, etc. are good, but the name file is not.  The first names explain something about what the variable means within the context of the code, but the latter is only confusing. A better name might be coordinates.  Similarly, Loadfileoutput is another poor name -- I'd suggest Coordinates for the name of that class.

Use C++ idiom

The code currently contains this:

vector<XYZCoordinate> file = vector<XYZCoordinate>();

That's really not necessary.  Just write this instead:

vector<XYZCoordinate> coordinates{};

Prefer a struct to a class for objects with no invariants

C++ has two primary means to encapsulate some behavior and data: struct and class.  The only difference is that a struct has its members public by default, while the class members are private by default.  If there is no invariant, (a thing that must always be true for the class to be coherent), then it often makes sense to use a struct instead.  In this case the XYZCoordinate class has no invariants and is probably better suited as a struct.

Write a custom extractor

Here's the code I'd prefer to write to read in and create the desired object:

std::ifstream in(filename);
Coordinates c;
in >> c;

This can be done by writing an extractor for the data.  In this case, I'd write two extractors.  One for the Coordinates class and one for the XYZCoordinate class.  Here's one way to write one for the XYZCoordinate class.  Here's the complete definition for that class, which I have converted to a struct:

struct XYZCoordinate{
    int x;
    int y;
    float z;
    friend std::istream& operator>>(std::istream& in, XYZCoordinate &xyz) {
        float a, b;
        in >> a >> b >> xyz.z;
        xyz.x = std::rint(a);
        xyz.y = std::rint(b);
        return in;
    }
};

Now we can write an extractor for the Coordinates class.  Here's that complete class:

class Coordinates
{
    int xmin;
    int xmax;
    int ymin;
    int ymax;
    std::vector<XYZCoordinate> values;

public:
    friend std::ostream& operator<<(std::ostream& out, const Coordinates &c) {
        return out << "[ " << c.xmin << ", " << c.xmax << "], [ "
            << c.ymin << ", " << c.ymax << "], with " << c.values.size() << " points\n";
    }
    friend std::istream& operator>>(std::istream& in, Coordinates &c) {
        // skip comment lines
        while (in.peek() == '#') {
            in.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        }
        XYZCoordinate xyz;
        while (in >> xyz) {
            c.values.push_back(xyz);
            c.xmin = std::min<int>(c.xmin, xyz.x);
            c.xmax = std::max<int>(c.xmax, xyz.x);
            c.ymin = std::min<int>(c.xmin, xyz.y);
            c.ymax = std::max<int>(c.xmax, xyz.y);
        }
        return in;
    }
};

Note that the values of xmin, xmax, etc. are invariants for this class, so we leave it as a class with only private data members.  If you need to access the items in the vector, you can add custom functions to handle that.

Provide a test driver

This is more about getting good reviews than the code itself, but it's often very useful if you provide some simple main that exercises the code.  This helps reviewers (and yourself!) understand how it's to be used in context.  In this case, I wrote this:

#include <vector>
#include <iostream>
#include <fstream>
#include <locale>
#include <limits>
#include <algorithm>

// all of the class code above goes here

int main(int argc, char *argv[]) {
    if (argc != 2) {
        std::cerr << "Usage: xyz infile.txt\n";
        return 1;
    }
    std::ifstream in(argv[1]);
    in.imbue(std::locale(std::locale(), new csv_reader()));
    Coordinates c;
    in >> c;
    std::cout << c;
}

The call to imbue uses a locale that treats commas as whitespace.  That code is in this excellent answer.  Now you don't have to explicitly check for commas or spaces and don't need boost.  Everything here is standard C++.

Results

I wrote a program to create a set of 20 millon sets of random coordinates.  Using that input, your original program took 41.9 seconds on my machine, while the one presented above took 14.8 seconds.


Area and volume calculator I'm a beginner coder, doing it solely for fun, having started coding about two months ago with Python. I have a working piece of code for calculating area and volume of different geometric shapes, using user input of "radius".

As I have said, the code works but I would like to know how I can improve it to be shorter, faster and more efficient. Essentially, I think it would be instructive to compare my rank amateur code to the code that an experienced software developer would produce to accomplish the same result.

from tkinter import *
from tkinter import ttk
import sys

root = Tk()

pi = 3.141

# Option Box

var = StringVar(root)
var.set("Functions")

lbl_title = Label(root, text = "GEOMETRIC SHAPES AND VOLUMES")
lbl_title.grid(row = 0, column = 0, columnspan = 2, padx =5, pady = 10)

lbl_choose = Label(root, text = "Choose a function and hit OK:")
lbl_choose.grid(row = 1, column = 0, padx = 5, pady = 0, sticky = W)

box = ttk.OptionMenu(root, var, "Circle Circumference", "Circle Area", "Sphere Volume",
               "Rectangle Area", "Rectangle Prism Volume", "Cone Volume")
box.grid(row = 2, column = 0, padx=5, pady=5, sticky= W)

# Separator

line1 = ttk.Separator(root, orient = HORIZONTAL)
line1.grid(row = 4, column = 0, columnspan = 3, sticky = EW) # Note separator sticky EW!

#txt_data_in = Text(root, height = 1, width = 30, bg = "Light Grey")
#txt_data_in.grid(row = 5, column = 0, padx = 5, sticky = W)

# Functions

def circ_circumf(r):
    print("\nThe circumference of the circle is: " + str(2*pi*r) + " units.")

def circ_area(r):
    print("\nThe area of the circle is: " + str(pi*r**2) + " units.")

def sphere_vol(r):
    print("\nThe volume of the sphere is: " +str(4/3 * pi * r**3) + " units.")

def rect_area(l,w):
    pass

def rect_prism_vol(l, w, h):
    pass

def cone_vol(r, h):
    pass

def exit():
    sys.exit()

# Main Function

def main():
    if var.get() == "Circle Circumference":
        r = int(input("Enter the radius of the circle in the units of your choice: "))
        circ_circumf(r)

    elif var.get() == "Circle Area":
        r = int(input("Enter the radius of the circle in the units of your choice: "))
        circ_area(r)

    elif var.get() == "Sphere Volume":
        r = int(input("Enter the radius of the sphere in the units of your choice: "))
        sphere_vol(r)

# Function Button

butt1 = ttk.Button(root, text = "OK", command = main)
butt1.grid(row = 2, column = 1, padx = 5, sticky = W)

# Exit Button

butt2 = ttk.Button(root, text = "Exit", command = exit)
butt2.grid(row = 6, column = 0, padx = 5, pady = 5,  sticky = W)

root.mainloop()

I think it would be instructive to compare my rank amateur code to the code that an experienced software developer would produce to accomplish the same result.

To me, the most obvious differences between experienced and novice programmers are:

consideration of edge cases
organization of data in a manner that lets the data drive the program's flow
broadness of their working definition of the word data

In that light, here are things I'd do differently from what you have done.

Make the most of the data you've got

You're silently truncating the user's input with int().  This is bad!  Mangling user data is necessary sometimes and to be avoided otherwise.  Especially here, where the change will affect the output in surprising and subtle ways.

You're displaying results with maximum "precision" but basing them on a value of pi that has only four significant figures.  This is misleading.  Define pi with more digits, and shorten the result so that it shows the actual precision.

Separate content from presentation

A common pattern arises when you create something that's useful to someone (yourself, usually), later it becomes useful to other people, and those people need the output in a different format.  Not only do you have to change the code, but it's been months since you looked at it! You won't remember how everything works.  Make it easy to affect how output is displayed.  Do this by keeping results in a structure that preserves the semantics of the data, and defer formatting to the very end.

Think about the task of using your code to make a web page: it's going to be hairy because your math functions have print statements in them.  It's better if those functions return numbers to be printed later by formatter function.

Separate program logic from UI elements

This is the same point as the previous one, just applied to input instead of output.  Use data structures that group input characteristics together, keeping display values independant from logic-affecting values.

As written, the button labels dictate the program flow (if var.get() == "Circle Circumference": etc.).  If you translate your program to another language, it won't work anymore.

Functions can be data

Each kind of user-selected computation has a formula to go with it.  It is natural to group those formulae alongside the other traits of the calculation.

Example

Here is one approach to your problem.  Observe how the actual code has almost nothing to do with what you're calculating, or how, or what the output will look like.  Instead, the data structure dictates how many variables to ask for, how to transform them into an answer, and how to format the answer for display.

From here, you could generalize this in all kinds of ways with few changes to the code.  For example, float(input(prompt)) is "hardcoded" in the logic.  This could be another lambda in the data, perhaps attached to each prompt, so that you can configure it to ask for integers or strings instead of just floats.

PI = 3.141592653589793
calculations = [
    dict(
        label="Circle Circumference",
        prompt=[
            "Enter the radius of the circle in the units of your choice: "
        ],
        result_format="\nThe volume of the sphere is: {0:.4f} units.",
        formula=lambda r: r*2*PI
    ),
    dict(
        label="Sphere Volume",
        prompt=[
            "Enter the radius of the sphere in the units of your choice: "
        ],
        result_format="\nThe volume of the sphere is: {0:.4f} units.",
        formula=lambda r: r**3 * 4/3 * PI
    ),
    dict(
        label="Cone Volume",
        prompt=[
            "Enter the radius of the cone in the units of your choice: ",
            "Enter the height of the cone in those same units: "
        ],
        result_format="\nThe volume of the cone is: {0:.4f} units.",
        formula=lambda r,h: r**2 * h * PI / 3
    ),
    # ... etc.
]

# selected_menu = var.get()
selected_menu="Cone Volume"  # hardcoded for demonstration purposes

action = next( filter(lambda x: x['label'] == selected_menu, calculations) )

inputs = list( map( lambda prompt: float(input(prompt)), action['prompt'] ))

answer = action['formula'](*inputs)

result_text = action['result_format'].format(answer)

print(result_text)


Updating user log in time while he logs in to the web page My intention is to store the user's last login date and time when he logs in to the web page. I pass his username and password to the stored procedure to check whether is it correct or not! I believe it'll be fine to store his last login date and time if any record matches.

USE [DBNAME]
GO
/****** Object:  StoredProcedure [dbo].[WP_LogInUser]    Script Date: 27-Apr-15 4:09:38 PM ******/
SET ANSI_NULLS ON
GO
SET QUOTED_IDENTIFIER ON
GO
ALTER Procedure [dbo].[WP_LogInUser]
    @Username   Nvarchar(100),
    @Password   Nvarchar(100)

as
Begin
    if exists(Select Username from TableName where Username=@Username and UserPassword=@Password)
        Begin
            Update TableName Set UserLastLogin = GETUTCDATE() where Username=@Username and UserPassword=@Password and IsActive = 1
            Select UserID, Username, UserEmail, UserFullName, UserType from TableName where Username=@Username and UserPassword=@Password and IsActive = 1
        End
End

What do you think about it? Is it okay? Or can I make this even better?

The first point I notice looking at your code is that it looks as though you are storing passwords in plain text. You should not do this. It is highly irresponsible. They should be salted and hashed in almost all cases.

Secondly there is no point looking up the same information three times.

Once to check if it exists, secondly to update it, and thirdly to select it.

You can do all this in one operation.

ALTER PROCEDURE [dbo].[WP_LogInUser] @Username NVARCHAR(100),
                                     @Password NVARCHAR(100)
AS
  BEGIN
      UPDATE TableName
      SET    UserLastLogin = GETUTCDATE()
      OUTPUT INSERTED.UserID,
             INSERTED.Username,
             INSERTED.UserEmail,
             INSERTED.UserFullName,
             INSERTED.UserType
      FROM   TableName
      WHERE  Username = @Username
             AND UserPassword = @Password
             AND IsActive = 1
  END


Partitioning a sequence into sublists This function divides a sequence into partitions, where a partition is a list of consecutive matching elements.

Example
Input: (A, A, B, B, B, A, C, C)
Output: ((A, A), (B, B, B), (A), (C, C))

I've tried to make this code "obviously correct", but it still doesn't look that way to me.
public static IEnumerable<List<T>> PartitionBy<T, PK>(this IEnumerable<T> sequence, Func<T, PK> partitionKey)
{
    return sequence.PartitionBy(partitionKey, EqualityComparer<PK>.Default);
}

public static IEnumerable<List<T>> PartitionBy<T>(this IEnumerable<T> sequence, IEqualityComparer<T> comparer)
{
    return sequence.PartitionBy(item => item, comparer);
}

public static IEnumerable<List<T>> PartitionBy<T, X>(this IEnumerable<T> sequence, Func<T, X> partitionKey, IEqualityComparer<X> comparer)
{
    var itr = sequence.GetEnumerator();
    if (!itr.MoveNext())
    {
        // empty sequence was passed in, so return empty sequence
        yield break;
    }

    // Start the first partition.
    var currentList = new List<T>(new[] { itr.Current });

    while (itr.MoveNext())
    {
        var key1 = partitionKey(currentList[0]);
        var key2 = partitionKey(itr.Current);

        if (comparer.Equals(key1, key2))
        {
            // continue current partition
            currentList.Add(itr.Current);
        }
        else
        {
            // yield current partition and start a new one
            yield return currentList;
            currentList = new List<T>(new[] { itr.Current });
        }
    }

    // We know it has at least 1 element here.
    yield return currentList;
}

Just a small point to start:

new List<T>(new[] { itr.Current });

You don't need the array too, you can just do:

new List<T> { itr.Current };

I'd suggest that you aim for consistency with your generic type names too. Why X vs PK? I'd suggest TKey for both.

You could do it just with a foreach:

public static IEnumerable<IEnumerable<T>> Partition<T, TKey>(
    this IEnumerable<T> items,
    Func<T, TKey> keySelector,
    IEqualityComparer<TKey> comparer)
{
    List<T> currentPartition = null;
    foreach (var item in items)
    {
        if (currentPartition != null
            && comparer.Equals(keySelector(item), keySelector(currentPartition[0])))
        {
            currentPartition.Add(item);
        }
        else
        {
            if (currentPartition != null)
            {
                yield return currentPartition;
            }
            currentPartition = new List<T> { item };
        }
    }
    if (currentPartition != null)
    {
        yield return currentPartition;
    }
}

Is it clearer than your code? I'm not convinced it is.


Finding the first triangle number with over five hundred divisors Project Euler Problem #12:

  The sequence of triangle numbers is generated by adding the natural
  numbers. So the 7th triangle number would be 1 + 2 + 3 + 4 + 5 + 6 + 7
  = 28. The first ten terms would be:

  1, 3, 6, 10, 15, 21, 28, 36, 45, 55, ...

  Let us list the factors of the first seven triangle numbers:

  1: 1
    3: 1,3
    6: 1,2,3,6
   10: 1,2,5,10
   15: 1,3,5,15
   21: 1,3,7,21
   28: 1,2,4,7,14,28

  We can see that 28 is the first triangle number to have over five
  divisors.

The code runs correctly, but I want to know if I can make any improvements to the code.

#include <iostream>
#include <limits>
#include <cmath>
int highlyDivisibleTriangularNum();

int main() {
std::cout <<highlyDivisibleTriangularNum() << std::endl;
return 0;
}

int highlyDivisibleTriangularNum()
{
//The number to be added to all the previous numbers
int i = 1;
//The number that adds all the previous numbers
int overallAdd = 0;
//Max number
int max = std::numeric_limits<int>::max();
for(int counter = 0; counter < max; counter++)
{
    int total = 0;
    int sum = overallAdd + i;
    i++;
    overallAdd = sum;

    int sqrtSum = (int)sqrt(sum);
    for(int c = 1; c <=sqrtSum;c++)
    {
        if(sum%c == 0)
        {
            total += 2;
        }
        if(total > 500)
        {
            return sum;
        }
    }

}
return 0;
}

Code

total += 2 is a bug. In case sum is a perfect square, and c happens to be its root, total shall be incremented by 1.
Using floating point (sqrt) in a number theoretical problem is dubious at best.
You don't need overallAdd. sum += i; suffices.
Naming looks arbitrary. The sum is a triangular number, total is a sum of divisors, so call them appropriately. Single-letter identifiers, like c shall be avoided.
Add your operators some breathing space.
Inner loop computes the number of divisors. Better factor it into a separate function.

Algorithm is brute force, which is never good, especially for Project Euler problems. This problem was discussed here many times. See this shameless self-plug for example.


Merge Sorting in Python This my implementation of the Merge Sorting Algorithm:

def merge(series1, series2):
    output = []
    while series1 != [] or series2 != []:
        if series1 == []:
            output.append(series2[0])
            series2.pop(0)
            continue
        if series2 == []:
            output.append(series1[0])
            series1.pop(0)
            continue
        last1 = series1[0]
        last2 = series2[0]
        if last1 <= last2:
            series1.pop(0)
            output.append(last1)
            continue
        if last2 < last1:
            series2.pop(0)
            output.append(last2)
            continue
    return output

def is_even(x):
    if x%2 == 0:
        return True
    return False

def merge_sort(series):
    iseries = []
    for i in series:
        iseries.append([i])
    while len(iseries) > 1:
        output = []
        length = len(iseries)
        if not is_even(length):
            length -= 1
            for i in range(0, (length-1), 2):
                a = iseries[i]
                b = iseries[i+1]
                output.append(merge(a, b))
            output.append(iseries[-1])
        else:
            for i in range(0, (length-1), 2):
                a = iseries[i]
                b = iseries[i+1]
                output.append(merge(a, b))
        iseries = []
        iseries += output
    return iseries[0]

Basically it splits the items in the initial series into lists of one item (series to iseries). Then I merge the items two-by-two appending the new items to another list (output), and set the main series equals to output. I do this until it's 1 item only and return the 1st item (return iseries[0]).

In merge you are doing list comparison (series1 != [] or series2 != []). This is expensive. You can avoid this by storing the length of two list beforehand and using that value to detect when to come out of the loop.

Also by using a small trick you can avoid using pop() function.

Avoid using continue. You can easily remove it by putting if else condition properly.

Your variable names are not indicating their actual purpose. It is recommended to use meaning names for variables.

def merge(series1, series2):

output = []
len_series1 = len(series1)
len_series2 = len(series2)
pointer_series1 = 0
pointer_series2 = 0

while pointer_series1 < len_series1 and pointer_series2 < len_series2:
    last1 = series1[pointer_series1] #the variable name is not indicating the actual task of this variable
    last2 = series2[pointer_series2]
    if last1 <= last2:
        output.append(last1)
        pointer_series1 += 1
    elif last2 < last1:
        output.append(last2)
        pointer_series2 += 1

output += series1[pointer_series1:]
output += series2[pointer_series2:]
#the above two statements removes the need to check the emptiness of each list
return output

In is_even function

if x%2== 0

can be rewritten as

if not x%2

The latter one is more 'pythonic'

If I were you I would define a function is_odd instead of is_even, because in the later part of the code you are checking whether a number is odd or not.

merge_sort can also be made more pythonic. The following code actually shows the power of Python to write complicated code in fewer lines:

def merge_sort(series):

iseries = [[i] for i in series]

while len(iseries) > 1:
    iseries = [merge(a,b) if b else a for a,b in map(None,*[iter(iseries)]*2) ]
return iseries[0]

# working explained
#iseries = [[1],[2],[3],[4]]
#iter(iseries) returns an iterable object lets say (o1)
# [iter(iseries)]*2 creates a list [o1,o1]
# map(None,[o1,o1]) calls Identity function; the arguments are o1.next() and o1.next()
# so for the first case Identity() is called on [1] (o1.next()) and [2] (o1.next())


Putting an Array into a table split In PHP, I'm getting multiple series and dumping various bits of information into an array. I will be left with 180 items (if I put in 30 series details that have 6 events per series)

$eventsArray = explode(',', $events); // This gets passed in via a param
\load_curl();
$array = Array();
$curl_array = array();

$ch = curl_multi_init();
foreach($eventsArray as $count => $id) {
    $curl_array[$count] = curl_init("https://www.eventbriteapi.com/v3/series/".$id."/events/?expand=ticket_classes&token=XXX");
    curl_setopt($curl_array[$count], CURLOPT_RETURNTRANSFER, 1);
    curl_setopt($curl_array[$count], CURLOPT_VERBOSE, true);
    curl_setopt($curl_array[$count], CURLOPT_SSL_VERIFYPEER, false);
    curl_multi_add_handle($ch, $curl_array[$count]);
}

do {
    curl_multi_exec($ch, $exec);
} while($exec > 0);

foreach($eventsArray as $count => $id) {
    $returned = curl_multi_getcontent($curl_array[$count]);
    $json = \json_decode($returned,true);
    foreach ( $json["events"] as $event) {
        $remaining = $event["ticket_classes"][0]["quantity_total"] - $event["ticket_classes"][0]["quantity_sold"];
        $array[] = Array('id'=>$event["id"],
                         'url'=>$event["url"],
                         'status'=>$event["status"],
                         'remaining'=>$remaining,
                         'start'=>date('l, jS F Y', strtotime($event["start"]["local"]))
                   );
    }
}

foreach($eventsArray as $count => $id) {
    curl_multi_remove_handle($ch, $curl_array[$count]);
}

curl_multi_close($ch);

foreach($eventsArray as $count => $id) {
    curl_close($curl_array[$count]);
}

$array = array_chunk($array, 30);
$array = json_encode($array);
return $array;

I then use EJS to display the information like below. The first item in each chunk is displayed in a row, then second item of each chunk in the next row etc.

<% for(var i=0; i<30; i++) { %>

        <%= this[0][i]['start'] %>
    <% for (var x=0; x<6; x++) {

    if (this[x][i]['status'] == 'live') {
        if (this[x][i]['remaining']<=0) {
            status='FULL';
            style = 'class="danger"';
        }
        else {
            status ='Book now
'+this[x][i]['remaining']+' AVAILABLE';
            style = 'class="success"';
        }
    }
    else {
            status='COMPLETE';
            style = 'class="warning"';
    }

    %>
        <td <%=style%>> <%=status%>
    <% } %>

<% } %>

It works but takes about 5 seconds to run. What could I do to make it faster? Is there much improvement that could be made?

Use a local database to store information

You could start by testing what takes the longest, and concentrate your efforts there. I cannot do this test. However, getting multiple bits of information, from an external API, can never be quick. This is probably what is slowing you down.

The API is: https://www.eventbrite.com/developer/v3

I had a very short look at the API you're using, and quickly stopped when I noticed it was quite complex. :-)

Do you retrieve information from the API for every page you render? Does this mean that the same information is requested from the API several times?

In that case it would be better to store the retrieved information in a local database, and use the information from your local database to build the web pages.

The API has webhooks. These can be useful to update the information in your database. Use them.

I know this isn't really an answer that reviews your code.


Help with refactoring my Tic Tac Toe game I'm very new to coding and have been diving into the skill with Python as my first language. One of the first programs I wrote for a class was this Tic Tac Toe game. I re-wrote this game using classes and have just recently refactored it looking for different smells. The person who's been teaching the class suggested that I post it here for additional help optimizing it. Any help or suggestions for improvement is much appreciated! Is there anything that smells funky to you?

#Written in Python 3.3.0, adapted for Python 2.7.3
#Tic Tac Toe Part 2

class Board(object):
    """Represents tic tac toe board"""
    def __init__(self):
        self.move_names = '012345678'
        self.Free_move = ' '
        self.Player_X = 'x'
        self.Player_O = 'o'
        self.moves = [self.Free_move]*9

    def winner(self):
        """Wining combinations. Returns Player or None"""
        winning_rows = [[0,1,2],[3,4,5],[6,7,8], # up and down
                        [0,3,6],[1,4,7],[2,5,8], # across
                        [0,4,8],[2,4,6]]         # diagonal
        for row in winning_rows:
            if self.moves[row[0]] != self.Free_move and self.allEqual([self.moves[i] for i in row]):
                return self.moves[row[0]]

    def allEqual(self, list):
        """returns True if all the elements in a list are equal, or if the list is empty."""
        return not list or list == [list[0]] * len(list)

    def getValidMoves(self):
        """Returns list of valid moves"""
        return [pos for pos in range(9) if self.moves[pos] == self.Free_move]

    def gameOver(self):
        return bool(self.winner()) or not self.getValidMoves()
    def startover(self):
        """Clears board"""
        self.moves = [self.Free_move]*9

class Player(object):

    """ Represents player and his/her moves"""
    def __init__(self, moves):
        self.moves = moves
        self.whoseturn = 1

    def makeMove(self, move, player):
        """Plays a move. Note: this doesn't check if the move is legal!"""
        self.moves[move] = player

class PlayerX(Player):
    """ Player X's moves"""
    def __init__(self, moves):
    self.moves = moves

    def xmove(self):
        print("Move Player X")
        move = int(input())
        if -1 < move < 9:
            for i in self.moves[move]:
                if i == ' ':
                    super(PlayerX, self).makeMove(move, "X")
                    print(self.moves)
                    return 1
            else:
                print("That's not a valid move")
                return 0
        else:
            print("That's not a valid move")

class PlayerO(Player):
    """ Player O's Moves"""
    def __init__(self, moves):
        self.moves = moves

    def ymove(self):
        print("Move Player O")
        move = int(input())
        if -1 < move < 9:
            for i in self.moves[move]:
                if i == ' ':
                    super(PlayerO, self).makeMove(move, "O")
                    print(self.moves)
                    return 1
                else:
                    print("That's not a valid move")
                    return 0
        else:
            print("That's not a valid move")

def tictac():
    print(welcome_mes)
    b = Board()
    x = PlayerX(b.moves)
    y = PlayerO(b.moves)
    p = Player(b.moves)
    while b.gameOver() == False:
        if p.whoseturn % 2 == 0:
            print("Turn #" + str(p.whoseturn))
            if x.xmove() == 1:
                p.whoseturn += 1
        else:
            print("Turn #" + str(p.whoseturn))
            if y.ymove() == 1:
                p.whoseturn += 1

    print(b.winner() + " has won the game")
    print("would you like to play again?")
    Yes, yes, YES, yeah, Yeah = "yes","yes", "yes", "yes", "yes"
    answer = input()
    if str(answer) == "yes":
        b.startover()
        tictac()
    else:
        quit()

welcome_mes = """Welcome to Tic Tac Toe!

The Game board is as follows:

    _0_|_1_|_2_
    _3_|_4_|_5_
     6 | 7 | 8   ""

For someone new to coding, there's a lot here that is good. Naming of classes, functions and variables are generally good. Comments follow convention, with simple, concise Docstrings for everything. Methods are generally simple and do only one thing, which is nice.

Let's go through a few things that I think can be improved:

In the winner function, every time this is called, winning_rows is recreated. However, this will be the same for any Board, and won't change. Hence, we can make this a class attribute:

class Board(object):

    winning_rows = [[0,1,2],[3,4,5],[6,7,8], # up and down
                    [0,3,6],[1,4,7],[2,5,8], # across
                    [0,4,8],[2,4,6]]         # diagonal

In winner, we simply call it as follows:

def winner(self):
    """Wining combinations. Returns Player or None"""
    for row in Board.winning_rows:
        #Code as before

In fact, the user of Board probably shouldn't change this. Other object-oriented language have the notion of private variables: variables that aren't visible and can't be accessed outside their own class. Python doesn't have this restriction, but it does have a convention that anything prefixed by an underscore should probably be treated as such, hence it should be _winning_rows.

Naming is generally good, but just watch out for the few inconsistencies that are lurking around: self.move_names or self.Free_move? It's nice to keep things uniform, either start everything with lower case, or start everything with upper case. Starting everything with lowercase would be "more pythonic" in my eyes, but consistency is key. The same thing applies to method names, e.g. gameOver vs startover.

Using Player as a base class and PlayerX, PlayerY as subclasses isn't really the way to go here. Note that xmove and ymove are almost exactly the same. This sort of code duplication is a definite sign that you should reconsider your design. Instead of subclassing for each type of Player, it would be far better to simply create difference instances of Player, each one with perhaps an attribute like self.piece. Hence, it would look something like:

class Player(object):

    """ Represents player and his/her moves"""
    def __init__(self, piece, moves):
        self.moves = moves
        self.piece = piece

    def getMove(self):
        print("Move Player %s" % self.piece)
        #...

    def makeMove(self, move):
        #...

    def __str__(self):
        return "Player " + self.piece

I've modified the names and shuffled things around a bit here, but hopefully you can see what's going on. This cuts down on the repeated code. Note we've also directly created a __str__ method that will be called whenever we call str() on a Player.

There are a few bits of leaky abstraction. self.whoseturn = 1 shouldn't live in Player - this doesn't really encapsulate anything about the player themselves, it's instead information about the state of the game - hence it's in the wrong spot. It likely shouldn't be a simple int either. In fact, I think the part of the code that needs the most work is the tictac() function. Instead of simply having a function, there should be another class that encapsulates the idea of a game. This will hold a Board, both the Players, and will co-ordinate whose turn it is, whether the game is finished, and perform cleanup and reset functions for a new game:

class Game(object):

    _welcome_mes = """Welcome to Tic Tac Toe!

The Game board is as follows:

    _0_|_1_|_2_
    _3_|_4_|_5_
     6 | 7 | 8   """

    def __init__(self):
        self.board = Board()
        self.playerX = Player('x', self.board.moves)
        self.playerO = Player('o', self.board.moves)
        self._currentTurn = self.playerX

    def _swapCurrentTurn(self):
        if self._currentTurn == self.playerX:
            self._currentTurn = self.playerO
        else: self._currentTurn = self.playerX

    def start(self):
        print(Game._welcome_mes)
        while not self.isGameOver():
            print("Turn: " + str(self._currentTurn))
            if self._currentTurn.move():
                self._swapCurrentTurn()
        self._atGameEnd()

    def _atGameEnd(self):
        print(self.board.winner() + " has won the game")
        print("Would you like to play again?")
        answer = input()
        if str(answer).lower() in ["yes", "y", "yeah"]:
            self._restart()

    def _restart(self):
        self.board.clear()
        self.playerX = Player('x', self.board.moves)
        self.playerO = Player('o', self.board.moves)
        self._currentTurn = self.playerX

    def isGameOver(self):
        return self.board.gameOver()

To play the game, all you need to do is make a Game object and call start() on it. This cleans up the logic a bit (and was done a bit hastily, so can probably be further improved. However, it should give you a general idea at least). I've left out any docstrings, which you should add if you decide to go this route.

I've also cleaned up the string checking to see if the user wants to play again: firstly, we convert it to lowercase, so we don't need to worry about dealing with the difference between "yes" and "YES", and then we simply check if it is in a few different options with in.

There's another minor thing I would think about fixing/adding: having the board print out in rows of 3, instead of a single flat list. This should be fairly simple.


Check if two checkboxes are checked I have a page where a user needs to click two checkboxes before continuing (agree to two terms of service). My jQuery is fairly straightforward, you have to have both check boxes checked to continue:

$("#agree1, #agree2").click(function () {
    if ($("#agree1").is(':checked') == true && $("#agree2").is(':checked') == true) {
        $("#nextPage").removeAttr("disabled");
    } else {
        $("#nextPage").attr("disabled", "disabled");
    }
});

I'm wondering if there is a simpler way of writing this. I expected a line like this to check for both boxes to be checked, but it doesn't:

if ($("#agree1 #agree2").is(':checked') == true)

Is there a way to simplify this?

Example code: jsFiddle

You can look for only the checked checkboxes, and then check that you found two:

if ($("#agree1:checked,#agree2:checked").length == 2) { ... }


Monte Carlo pi calculation In order to learn the basics of Monte Carlo I calculated pi with it.
I also wrote an explanation of the reasoning behind the code.

Down here you can see the circle with random points that I simulated in my code.

"""
        This programme calculates pi with Monte Carlo

Given a square and a circle inside it.
We have

Area_of_the_square = LENGTH ** 2
Area_of_the_circle = radius ** 2 * pi => (LENGTH ** 2) / 4 * pi

The circle is obviously smaller than the square.
We have the equation:

Area_of_the_square * number_less_than_one == Area_of_the_circle

This programme is going to put a big number of points inside the square
(I suggest TIMES_TO_REPEAT = 10**5).

It will then count how many of them are inside the circle,

number_less_than_one = points_inside_the_circle / total_points

After doing some simple math with this formula:

Area_of_the_square * number_less_than_one == Area_of_the_circle

we get that

pi = number_less_than_one * 4

NOTE: This method is deadly slow and quite complicated,
I made this programme just in order to learn.

"""
import random

TIMES_TO_REPEAT = 10**5
LENGTH = 10**5
CENTER = [LENGTH/2,LENGTH/2]

def in_circle(point):
    x = point[0]
    y = point[1]
    center_x = CENTER[0]
    center_y = CENTER[1]
    radius = LENGTH/2
    return (x - center_x)**2 + (y - center_y)**2 < radius**2

count = inside_count = 0
for i in range(TIMES_TO_REPEAT):
    point = random.randint(1,LENGTH),random.randint(1,LENGTH)
    if in_circle(point):
        inside_count += 1
    count += 1

pi = (inside_count / count) * 4

print(pi)

Your code seems to be working. Here are a few things to make it better.

Tuple unpacking

x = point[0]
y = point[1]

can be written :

x, y = point

Remove what is not required

You maintain a count variable but you already know the count : it is TIMES_TO_REPEAT.

Variable name

TIMES_TO_REPEAT does not seem to be such a good name. NB_POINTS would probably be better.

Removing the counting logic

Using iterators and sum, you can remove the logic to count points :

inside_count = sum(1 for i in range(NB_POINTS) if in_circle((random.randint(1,LENGTH),random.randint(1,LENGTH))))

Underscore for throwaway values

In your iteration, you don't really care about the value of i. It is quite a common thing to call it _ (more about this).

Move things to a function

It makes things easier to test

def compute_pi(nb_it):
    inside_count = sum(1 for _ in range(nb_it) if in_circle((random.randint(1,LENGTH),random.randint(1,LENGTH))))
    return (inside_count / nb_it) * 4

Add an if main guard

It is quite common to define functions/values/classes/whatever from your python file but to use them only behind an if main guard to make things easier to reuse via module imports for instance.

Oops

Currently, changing the value of LENGTH seems to be changing the result. This is probably wrong.

At this point, my code looks like :

import random

NB_POINTS = 10**5
LENGTH = 10**5
CENTER = [LENGTH/2,LENGTH/2]

def in_circle(point):
    x,y = point
    center_x, center_y = CENTER
    radius = LENGTH/2
    return (x - center_x)**2 + (y - center_y)**2 < radius**2

def compute_pi(nb_it):
    inside_count = sum(1 for _ in range(nb_it) if in_circle((random.randint(1,LENGTH),random.randint(1,LENGTH))))
    return (inside_count / nb_it) * 4

if __name__ == "__main__":
    print(compute_pi(NB_POINTS))

Using the right type and the right values

In order to solve the problem above and make things easier, you could change a bit the referential you are using.

Instead of using a circle of radius R (equal to length/2) and with center (R, R) and picking integer values in square [1, R] * [1, R] maybe you could consider a circle of radius R and with center 0 and pick floating values in [-R, +R]. Even better, you could focus on the corner of the circle by considering points in [0, R]*[0, R]. Finally, R is not really important here, you could pick 1.

Then the code becomes :

import random

NB_POINTS = 10**5

def in_circle(x, y):
    return x**2 + y**2 < 1

def compute_pi(nb_it):
    inside_count = sum(1 for _ in range(nb_it) if in_circle(random.random(),random.random()))
    return (inside_count / nb_it) * 4

if __name__ == "__main__":
    print(compute_pi(NB_POINTS))

And then, this can be simplified.

import random

NB_POINTS = 10**5

def compute_pi(nb_it):
    return 4 * sum(1 for _ in range(nb_it) if random.random()**2 + random.random()**2 < 1) / nb_it

if __name__ == "__main__":
    print(compute_pi(NB_POINTS))


Vectorizing equations for fsolve I have a system of 49 equations and 49 unknowns (\$x_{1},...,x_{49}\$). All equations have the following form.

\$x_{s}\sum_{r=1}^{R}\sum_{t=1}^{T}a_{s,t}b_{rs,t}=\sum_{r=1}^{R}\sum_{t=1}^{T}a_{r,t}b_{sr,t}x_{r}\$

where \$s = 1,...,49\$ with \$S = R = 50\$ and \$T = 30\$. The \$a\$'s, the \$b\$'s and \$x_{50}\$ are known parameters. Since all equations have the same form, I believe that vectorizing the problem is possible. I appreciate any suggestions of how to do this.

The code below solves a smaller system with \$S=R=T=3\$. The code runs. I describe how the parameters are arranged beneath the code.

    %% Define parameters
        T = 3; % Number of types (T)
        S = 3; % Number of senders (S)
        R = S; % Number of receivers (R), equals the number of senders
        A = [.1 .1 .1   % Columns are senders (S), rows are types (T)
             .3 .3 .6
             .6 .6 .3];
        B1 = [.9 .0 .1
              .5 .1 .2
              .8 .2 .1];
        B2 = [.0 .7 .2
              .2 .9 .0
              .1 .8 .1];
        B3 = [.1 .3 .7
              .3 .0 .8
              .1 .0 .8];
        B = zeros(T,R,S); % Initialize
        B(:,1,:) = B1;    % Pages of B are receivers
        B(:,2,:) = B2;    % Rows of B are types
        B(:,3,:) = B3;    % Columns of B are senders

        % Starting values
        x0 = [36 26];
        % Call functions
        F = fsolve(@(x) myfun1(x,A,B),x0);

    %% Define Functions
        function F = myfun1(x,A,B)
        F(1) =   x(1)*(sum(A(:,1).*B(:,2,1))  +  sum(A(:,1).*B(:,3,1))) ...
              -  x(2)*(sum(A(:,2).*B(:,1,2))) - (sum(A(:,3).*B(:,1,3)));
        F(2) =   x(2)*(sum(A(:,2).*B(:,1,2))  +  sum(A(:,2).*B(:,3,2))) ...
              -  x(1)*(sum(A(:,1).*B(:,2,1))) - (sum(A(:,3).*B(:,2,3)));
        end

Is it possible to avoid the "sum"? Maybe by rearranging the
parameters?
Is it possible to vectorize the function itself to avoid writing F(1), F(2), ... , F(49)? I know it is possible, but for this particular problem I have difficulties understanding how to do it.

A visualization of the B array:
In the example, the parameters are arranged as shown below.

\$A:\$
\begin{pmatrix} a_{1,1} & a_{2,1} & a_{3,1}\\ a_{1,2} & a_{2,2} & a_{3,2}\\ a_{1,3} & a_{2,3} & a_{3,3} \end{pmatrix}

\$B1:\$
\begin{pmatrix} b_{11,1} & b_{12,1} & b_{13,1}\\ b_{21,1} & b_{22,1} & b_{23,1}\\ b_{31,1} & b_{32,1} & b_{33,1} \end{pmatrix}

\$B2:\$
\begin{pmatrix} b_{11,2} & b_{12,2} & b_{13,2}\\ b_{21,2} & b_{22,2} & b_{23,2}\\ b_{31,2} & b_{32,2} & b_{33,2} \end{pmatrix}

\$B3:\$
\begin{pmatrix} b_{11,3} & b_{12,3} & b_{13,3}\\ b_{21,3} & b_{22,3} & b_{23,3}\\ b_{31,3} & b_{32,3} & b_{33,3} \end{pmatrix}

Yes. You can simplify this code. I made two main changes.

First, I forget that \$x_{50}\$ is known and write an equation for it just like all the other variables, then I replace that equation by the equation \$x_{50}=1\$.

Second, this is a system of linear equations. Notice that on both sides of the equation we have \$a_{ij}b_{kij}\$ with \$\{i,j,k\}=\{s,t,r\}\$ on the left and \$\{i,j,k\}=\{r,t,s\}\$ on the right, and both are summed over \$t\$ which corresponds to \$j\$. So first I find the matrix \$C_{ik}=\sum_ja_{ij}b_{kij}\$, which requires quite some tweaking due to the messed up dimensions/slices. Now the equation reads

\$\displaystyle x_{s}\sum_{r=1}^R C_{sr} = \sum_{r=1}^R C_{rs}x_r \$

for each \$s\in1,2,\ldots,S\$. The sum of the left is just sum(C,2) in Matlab, and the sum on the right is the matrix multiplication of \$C^t\$ with the vector of unknowns \$\vec x\$. We can write the equation in matrix form as

\$\displaystyle \vec c\vec x-C\vec x=0 \$

where \$[\vec c]_s=\sum_r C_{sr}\$. Written another way, we have

\$\displaystyle \left(\mathrm{diag}(\vec c)-C\right)\vec x=0. \$

Now since we know \$x_{50}=1\$ just replace the last row of the matrix with \$[0,0,\ldots,0,1]\$. Then if \$M=\mathrm{diag}(\vec c)-C\$ our equation is \$M\vec x=\vec v\$ where \$\vec v=[0,0,\ldots,0,1]^t\$. We can solve this system directly using \.

%% Define parameters
T = 3; % Number of types (T)
S = 3; % Number of senders (S)
R = S; % Number of receivers (R), equals the number of senders
A = [.1 .1 .1   % Columns are senders (S), rows are types (T)
     .3 .3 .6
     .6 .6 .3];
B1 = [.9 .0 .1
      .5 .1 .2
      .8 .2 .1];
B2 = [.0 .7 .2
      .2 .9 .0
      .1 .8 .1];
B3 = [.1 .3 .7
      .3 .0 .8
      .1 .0 .8];

C = squeeze(sum(A.*cat(3,B1,B2,B3),1)).';

M = diag(sum(C,1))-C; % matrix equations
M(end,:) = [zeros(1,S-1) 1]; % replace the last equation

v = [zeros(S-1,1);1]; % modify final RHS entry

F = M\v;

An additional benefit is that solving linear systems is much faster than solving nonlinear systems, so this code is much faster. I couldn't test with different R, S, or T so something might have to change, I'm not sure if it will just work, but I think it should.

With the new method for generating the equations, we can make a few more simplifications to avoid having to concatenate the Bi's explicitly, using a cell array:

T = 3; % Number of types (T)
S = 3; % Number of senders (S)
R = S; % Number of receivers (R), equals the number of senders
A = [.1 .1 .1   % Columns are senders (S), rows are types (T)
     .3 .3 .6
     .6 .6 .3];
B{1} = [.9 .0 .1
      .5 .1 .2
      .8 .2 .1];
B{2} = [.0 .7 .2
      .2 .9 .0
      .1 .8 .1];
B{3} = [.1 .3 .7
      .3 .0 .8
      .1 .0 .8];

C = squeeze(sum(A.*cat(3,B{:}),1)).';

M = diag(sum(C,1))-C;
M(end,:) = [zeros(1,S-1) 1];

v = [zeros(S-1,1);1];

F = M\v;


Convert total minutes into "Hours:minute:seconds" for marathon data The point of the script below is to read in the 10th column which is the total number of minutes a runner took to finish the marathon and convert it into form of "hours:minutes:seconds".  Is there a better way?

require 'csv'

File.open('bostonmarathon-2012-results.txt', 'w') {  |f|
  skip = true
  CSV.foreach("bostonmarathon-2012-results.csv") do |row|
    if skip
      skip = false
    else
      total_seconds = row[10].to_f * 60
      seconds = total_seconds % 60
      minutes = (total_seconds / 60) % 60
      hours = total_seconds / (60 * 60)

      f.puts(format("%02d:%02d:%02d", hours, minutes, seconds))
    end
  end
}

Time starts as in total minutes in decimal format: 142.93

Example Data:

division,name,city,gender,age,official,bib,overall,state,genderdiv,net,country
4        / 26,"Soejima, Masazumi",Fukuoka City,M,41,83.45,W1,4        / 35,,4        / 26,83.45,JPN
14       / 4335,"Gebremariam, Gebregziabher",Tigray,M,27,142.93,2,14       / 21616,,14       / 12621,142.93,ETH
6        / 26,"Van Dyk, Ernst F.",Paarl,M,39,84.38,W2,6        / 35,,6        / 26,84.38,RSA
2        / 26,"Fearnley, Kurt H.",Hamilton,M,31,81.65,W3,2        / 35,,2        / 26,81.65,AUS
3        / 26,"Hokinoue, Kota",Iizuka,M,38,83.43,W4,3        / 35,,3        / 26,83.43,JPN

Use do...end for multiline blocks. You're already doing it for the innermost block, but not the File.open block for some reason
CSV.foreach accepts a hash of options. One of them is :headers which you can set to true or :first_row. This will

skip the header row when iterating (so you can skip skip and the if..else-branching), and
let you refer to a column by its header text instead of an opaque numerical index.

For the time-formatting you can use #divmod to get both quotient and remainder of a division in one go. Add some array destructuring, and you get this:

hours, remainder = total_seconds.divmod(60**2)
minutes, seconds = remainder.divmod(60)

Pretty clean, I think.

Alternatively, here's a hacky(!) idea. You could just treat the seconds as a regular unix timestamp, and do something like

Time.at(total_seconds).strftime("%k:%M:%S")

The assumption here is of course that no one took more than 24 hours to complete the marathon. If they did, the hour would of course roll over. It seems like an ok assumption in this particular case, but it's obviously not something that'd work for any context. It's kinda clever, but it's definitely a hack.


What is the potential risk with this code? I am trying to implement a distinct counter to detect if all tasks that is added to the ThreadPool has completed and want to use it as a barrier. So, when a function that is added to the ThreadPool is completed, workDone will be called, and count will be decreased. The main thread will be locked by calling waitForALL() method until the count is cleared to 0. Is this implementation reasonable and what risk does it have from the concurrency perspective?

I am still using .net 3.5, code has been updated as below.

public class BarrierCount
{
    private static volatile BarrierCount INSTANCE = new BarrierCount();
    private int count = 0;
    private static volatile Object o = new Object();

    public static BarrierCount getInstance()
    {
        return INSTANCE;
    }

    private BarrierCount() { }
    public void increaseCount()
    {
        lock (o)
        {
            count = count + 1;
        }
    }

    public void workDone()
    {
        lock (o)
        {
            count = count - 1;
        }
    }

    public void WaitForAll()
    {
        while(true)
        {
            lock (o)
            {
                if (count == 0)
                {
                    break;
                }
            }

            Thread.SpinWait(100);
        }
    }
}

Update

OP has since updated question so this answer makes no sense any more WRT question asked!! Those edits have also been lost in migration from stack overflow it seems.

Purely from a concurrency perspective and not commenting on if the design is fit for purpose:

Newly added Volatile

You do not understand this, I am not going to go into why, just read up on stack overflow/MSDN or don't use it. As I remember you had a readonly on the instance. That was correct.

In general don't lock(this), because (although not in this case... yet) this can also be locked from external code (lock(BarrierCount.getInstance())), and that allows deadlocks to occur. Although no deadlock is possible here as the methods are guaranteed to return.

Instead add a specific lock object:

private readonly object _lock = new object();

Note

readonly not volatile. readonly makes it clear to readers/modifiers that the reference is safe to use from multiple threads. It also stops modifiers from changing the reference (unless they remove the readonly, but they should make them think twice at least).

In WaitForAll you read count without a lock. Suggest you add a private GetCount() which locks.

I'm not sure how this is used, the setCount is presumably called once before the work commences. If the tasks have started and there is any risk in them calling workDone before setCount then it won't ever reach zero.

I would loose setCount in favour of an IncrementCount() method called before starting each task.

But

You should look at Task.WaitAll before you do more on this.


Android audio player I want to implement basic audio functions like play, stop and pause. I have stuffed all the code inside onCreate method. Is this best practise to follow?

public class MainActivity extends Activity {
        Button start,pause,stop;
        @Override
        protected void onCreate(Bundle savedInstanceState) {
            super.onCreate(savedInstanceState);
            setContentView(R.layout.activity_main);

            start=(Button)findViewById(R.id.button1);
            pause=(Button)findViewById(R.id.button2);
            stop=(Button)findViewById(R.id.button3);
            //creating media player
            final MediaPlayer mp=new MediaPlayer();
            try{
                    //you can change the path, here path is external directory(e.g. sdcard) /Music/maine.mp3
            mp.setDataSource(Environment.getExternalStorageDirectory().getPath()+"/Music/maine.mp3");

            mp.prepare();
            }catch(Exception e){e.printStackTrace();}

            start.setOnClickListener(new OnClickListener() {
                @Override
                public void onClick(View v) {
                    mp.start();
                }
            });
            pause.setOnClickListener(new OnClickListener() {
                @Override
                public void onClick(View v) {
                    mp.pause();
                }
            });
            stop.setOnClickListener(new OnClickListener() {
                @Override
                public void onClick(View v) {
                    mp.stop();
                }
            });
        }
    }

You could let your class implement OnClickListener

public class MainActivity extends Activity implements OnClickListener{
    ...
}

and then use the onClickfunction like this:

public void onClick(View v)
{
    switch (v.getId())
    {
        case R.id.button1:
            mp.start();
            break;
        case R.id.button2:
            mp.pause();
            break;
        case R.id.button3:
            mp.stop();
            break;
        default:
            break;
    }
}

That way you have all actions that happen on a press of a button in the same place in your program.

You should also change the ID to something more meaningful, eg. play, pause, stop.
This makes your code far more maintainable and easier to understand which Button does what just by skimming the code of the MainActivity(which you also might want to rename to something like MediaPlayerActivity incase you decide to add additional Activity classes, for example to select a new song.

Additionally I would advise you to change the visibility of the Buttons to private, rather than default.


MySQL query in nested loop to get scores and averages for students I am trying to get the scores and average for all assignments for records in a MYSQL database. The code below works but seems to take quite a bit of time to run because of the nested while loop. Any suggestions on how to combine the queries into one?

$query4 ="SELECT studentsisid,coursename,learningoutcomeid,learningoutcomename,
count(RecordID) as assessmentcount,
count(if(outcomescore >= 3,outcomescore, NULL)) as over3,
avg(outcomescore) as scoreavg
from studentscores
WHERE studentsisid LIKE '$studentid' AND coursename='$coursename'
Group by studentsisid,coursename, learningoutcomeid
ORDER BY studentname,   learningoutcomename";

$result4=mysql_query($query4);

if (mysql_numrows($result4) == 0){
die("No result.");
}

while ($row4 = mysql_fetch_assoc($result4)) {
  $learningoutcomeid = (int)$row4['learningoutcomeid'];
  $coursename = $row4['coursename'];
  $studentid = $row4['studentsisid'];
  $learningoutcomename = $row4['learningoutcomename'];
  $learningoutcomeid = (int)$row4['learningoutcomeid'];
  $assessmentcount = (int)$row4['assessmentcount'];
  $scoreavg = number_format($row4['scoreavg'], 1);
  $over3 = $row4['over3'];

$query5 ="SELECT count(if(outcomescore >= 3,outcomescore, NULL)) as recentover3,
    avg(outcomescore) as recentscoreavg
    FROM
    (
    SELECT outcomescore FROM studentscores
    WHERE
    studentsisid='$studentid' AND coursename='$coursename'
    AND learningoutcomeid='$learningoutcomeid'
    ORDER BY RecordID DESC LIMIT 3) as r";

$result5=mysql_query($query5);

while ($row5 = mysql_fetch_assoc($result5)) {
$recentscoreavg = number_format($row5['recentscoreavg'], 1);
$recentover3 = $row5['recentover3'];

I am trying to get the data below and the code above does accomplish the goal:

studentsisid;coursename;loid;loname;assessmentcount;over3;scoreavg;recentover3;recentscoretavg
1234";"Course1";"7982";"LearningOutcome1";"1";"1";"3.2";"1";"3.2"
1234;"Course1";"7995";"LearningOutcome2";"4";"4";"2.5";"2";"2.5"
1234;"Course1";"7991";"LearningOutcome3";"6";"6";"3.8";"3";"3.2"
1234;"Course1";"7889";"LearningOutcome4";"1";"1";"3.4";"1";"3.2"
1234;"Course1";"7839";"LearningOutcome5";"2";"2";"2,6";"1";"3.2"

You can replace:

  $learningoutcomeid = (int)$row4['learningoutcomeid'];
  $coursename = $row4['coursename'];
  $studentid = $row4['studentsisid'];
  $learningoutcomename = $row4['learningoutcomename'];
  $learningoutcomeid = (int)$row4['learningoutcomeid'];
  $assessmentcount = (int)$row4['assessmentcount'];
  $scoreavg = number_format($row4['scoreavg'], 1);
  $over3 = $row4['over3'];

by:

extract($row4);
$scoreavg  = number_format($scoreavg,1);

Note that there are several weird things in your code: 2x 'learningoutcomeid' and 'studentsisid'. Sloppy coding?

Your code here is also incomplete and not working. Sorry, that means no answer to your question.

What is also a serious problem is that the code is undocumented. There are NO comments to explain what's being done. Suppose you hand over this project to someone else, how is this next person to make sense of it? I looked at it for a while, and I cannot make sense of it. There seems to be just one table you use: 'studentscores', but it's structure eludes me.


HackerRank Sherlock and Squares: count perfect squares in an interval I've exceeded the time limit on Sherlock and Squares on hackerrank.com.
Problem
Given the number of test cases and a range (inclusive), find the number of numbers within that range that are perfect squares.
Sample Input

2   (number of test cases)
3 9 (first test case)
17 24 (second test case)

Sample Output

2 (in range [3,9], there are two perfect square numbers)
0 (in range [17, 24], there are no perfect square numbers)

My code in C++
#include <iostream>
#include <cmath>
#include <vector>
using namespace std;

int main()
{
  unsigned test_cases, low_range, high_range, count = 0;
  vector<unsigned> sherlock_squares;
  cin >> test_cases;
  while(test_cases != 0)
  {
    cin >> low_range >> high_range;
    for(unsigned i = low_range; i <= high_range; ++i)
    {
      if(sqrt(i) == floor(sqrt(i)))
        count += 1;
    }
    sherlock_squares.push_back(count);
    count = 0;
    --test_cases;
  }
  for(unsigned i = 0; i < sherlock_squares.size() - 1; ++i)
    cout << sherlock_squares.at(i) << endl;
  cout << sherlock_squares.at(sherlock_squares.size() - 1);
  return 0;
}

I'd appreciate it if an explanation is provided in your response, since I'm new to programming and Stack Exchange.

using namespace std;

Why is “using namespace std;” considered bad practice?

Personally, I find code more readable if it says what namespace is being used.  E.g. std::sqrt.

  unsigned test_cases, low_range, high_range, count = 0;

Three of these variables are only used inside the loop, so you can simplify by just saying

  unsigned test_cases;

As a general rule, you should declare variables at the smallest scope where they are needed.

  while(test_cases != 0)
  {
    cin >> low_range >> high_range;

Some people prefer for loops for this kind of situation.

  for (; test_cases > 0; --test_cases)
  {
     uint32_t count = 0;
     uint32_t low_range, high_range;
     std::cin >> low_range >> high_range;

Also added declarations inside the loop for variables that are only used inside the loop.  Note how this also takes care of resetting the count on each iteration.  This saves clearing the count after it's no longer used, which your original code does.

Note that unsigned is not guaranteed to be a 32-bit integer, although it almost always will be.  Using uint32_t guarantees 32 bits and is found in #include <stdint.h>.

    for(unsigned i = low_range; i <= high_range; ++i)
    {
      if(sqrt(i) == floor(sqrt(i)))
        count += 1;
    }

First, why not say

      {
        count++
      }

That's simpler than count += 1;.

  I am unsure of how I can simplify my code for this HackerRank activity:

But you are experiencing a timeout.  You don't need to make it simpler.  You need to make it faster.

Rather than calculating the square root on every iteration, calculate them only once.

    // +1 because both ends are inclusive
    // floor to keep within the high end (inclusive)
    // ceil to keep within the low end (inclusive)
    int32_t count = (unsigned)std::floor(std::sqrt(high_range)) - (unsigned)std::ceil(std::sqrt(low_range)) + 1;
    if (count < 0)
    {
      count = 0;
    }
    sherlock_squares.push((uint32_t)count);

I was originally going to do a for loop where I only calculated the squares, but I realized that I didn't have to do so.  This will get the right answer consistently.  It counts the number of integers from the square root of A to the square root of B.  That will correspond to the number of squares between A and B, since we're only counting the squares of integers.

  for(unsigned i = 0; i < sherlock_squares.size() - 1; ++i)
    cout << sherlock_squares.at(i) << endl;
  cout << sherlock_squares.at(sherlock_squares.size() - 1);

Rather than subtracting 1 from the end, add 1 to the beginning.  That saves math operations.

  std::cout << sherlock_squares.at(0);
  for (unsigned i = 1; i < sherlock_squares.size(); ++i)
  {
    std::cout << '\n' << sherlock_squares.at(i);
  }

Switching from std::endl to '\n' is faster, as std::endl triggers a flush, which you don't need here.  Probably not the performance bottleneck in this case, but it can be in some cases.

Always using the block form of control structures is more reliable and simplifies source control if you add statements in the future.

But I actually think that what the challenge wanted was

  for (auto sherlock_square : sherlock_squares)
  {
    std::cout << sherlock_square << '\n';
  }

with new lines after every line rather than only between them.

It may even have wanted you to interleave input and output.  But that's uncertain.


PHP RESTful API for a student repository All of the codes are based on the PSR-2 standard. Also Dependency-Injection and Repository patterns are followed. I would love to hear some advices or comments.

StudentRepository Class

namespace REST;

use Slim\PDO\Database;

interface IStudentRepository
{
    //Functions to be defined
    public function __construct();
    public function getStudentNames($request, $response, array $args);
    public function getAll($request, $response, array $args);
    public function addStudent($name, $id);
    public function updateName($id, $new_name);
}

class StudentRepository implements IStudentRepository
{
    //Queries and MYSQL connection
    protected $pdo;

public function __construct()
{
    //Database connection
    $dsn = 'mysql:host=localhost;dbname=College_DB;charset=utf8';
    $usr = 'root';
    $pwd = '';

    $this->pdo = new \Slim\PDO\Database($dsn, $usr, $pwd);
}

public function getStudentNames($request, $response, array $args)
{
    $array_val = array();
    $sql = 'SELECT name FROM Students';
    $cntrl=0;
    foreach ($this->pdo->query($sql) as $row) {
        $array_val[$cntrl]=$row["name"];
        $cntrl++;
    }
    $n_response = $response->withJson($array_val, 200);
    return $n_response;
}

public function getAll($request, $response, array $args)
{
    $array_val = array();
    $sql = 'SELECT * FROM Students';

    foreach ($this->pdo->query($sql) as $row) {
        $array_val[$row["id"]]=$row["name"];
    }

    $n_response = $response->withJson($array_val, 200);
    return $n_response;
}

public function addStudent($id, $name)
{

    $stmt = $this->pdo->prepare('INSERT into Students(id,name) values(:id,:namee)');
    $stmt->bindParam(":id", $id, \Slim\PDO\Database::PARAM_STR);
    $stmt->bindParam(":namee", $name, \Slim\PDO\Database::PARAM_STR);

    $stmt->execute();
}

public function updateName($id, $new_name)
{
    $stmt = $this->pdo->prepare('UPDATE Students set name=:namee where id=:id');
    $stmt->bindParam(":id", $id, \Slim\PDO\Database::PARAM_STR);
    $stmt->bindParam(":namee", $new_name, \Slim\PDO\Database::PARAM_STR);

    $stmt->execute();
}
}

StudentController Class

<?php

namespace REST;

class StudentController
{
    protected $Student_DB;

public function __construct(IStudentRepository $Student_DB)
{
    $this->Student_DB=$Student_DB;
}

public function getStudents($request, $response, array $args)
{
    return $this->Student_DB->getAll($request, $response, $args);
}

public function getStudentNames($request, $response, array $args)
{
    return $this->Student_DB->getStudentNames($request, $response, $args);
}

public function addStudent($request, $response, array $args)
{
    if (array_key_exists("id", $args) && array_key_exists("name", $args)) {
        $id = $args["id"];
        $name = $args["name"];
        if (!is_numeric($id) || !ctype_alpha($name)) {
            $n_response = $response->withStatus(400);
            return $n_response;
        }
    }
    $this->Student_DB->addStudent($id, $name);
    $n_response = $response->withStatus(200);
    return $n_response;
}
}

Edit : I have edited the code in order to return JSON data with spesific status codes.

The StudentRepository is all about database, and shouldn't know about $request and $response objects, that is the job of the controller.

What if you were fetching data to put in an excel file, then you can't use the StudentRepository as it was to fetch that data.

<?php

namespace REST;

use Slim\PDO\Database;

interface IStudentRepository
{
    //Functions to be defined
    public function __construct(array $credentials);
    public function getStudentNames(array $args);
    public function getAll(array $args);
    public function addStudent($name, $id);
    public function updateName($id, $new_name);
}

class StudentRepository implements IStudentRepository
{
    //Queries and MYSQL connection
    protected $pdo;

    public function __construct(array $credentials)
    {
        // TODO configuration should be stored outside the class, in a config object or file
//        //Database connection
//        $dsn = 'mysql:host=localhost;dbname=College_DB;charset=utf8';
//        $usr = 'root';
//        $pwd = '';

        $this->pdo = new \Slim\PDO\Database($credentials['dsn'], $credentials['usr'], $credentials['pwd']);
    }

    public function getStudentNames(array $args)
    {
        // TODO recommend you use ORDER BY to get a consistent order from request, to request
        $sql = 'SELECT name FROM Students';

        $array_val = array();

        foreach ($this->pdo->query($sql) as $row) {
            $array_val[]=$row["name"];
        }

        return $array_val;
    }

    public function getAll(array $args)
    {
        // TODO recommend you use ORDER BY to get a consistent order from request, to request
        $sql = 'SELECT * FROM Students';

        $array_val = array();

        foreach ($this->pdo->query($sql) as $row) {
            $array_val[$row["id"]]=$row["name"];
        }

        return $array_val;
    }

    public function addStudent($id, $name)
    {
        $stmt = $this->pdo->prepare('INSERT into Students(id,name) values(:id,:namee)');
        $stmt->bindParam(":id", $id, \Slim\PDO\Database::PARAM_STR);
        $stmt->bindParam(":namee", $name, \Slim\PDO\Database::PARAM_STR);

        $stmt->execute();
    }

    public function updateName($id, $new_name)
    {
        $stmt = $this->pdo->prepare('UPDATE Students set name=:namee where id=:id');
        $stmt->bindParam(":id", $id, \Slim\PDO\Database::PARAM_STR);
        $stmt->bindParam(":namee", $new_name, \Slim\PDO\Database::PARAM_STR);

        $stmt->execute();
    }
}
?>

<?php

namespace REST;

class StudentController
{
    protected $Student_DB;

    public function __construct(IStudentRepository $Student_DB)
    {
        $this->Student_DB=$Student_DB;
    }

    public function getStudents($request, $response, array $args)
    {
        $all = $this->Student_DB->getAll();
        return $response->withJson($all, 200);
    }

    public function getStudentNames($request, $response, array $args)
    {
        $names = $this->Student_DB->getStudentNames();
        return $response->withJson($names, 200);
    }

    public function addStudent($request, $response, array $args)
    {

        // improve tests, as it was possible for $id and $name to be undefined with previous tests
        if (!array_key_exists("id", $args) || !array_key_exists("name", $args)) {
            return $response->withStatus(400);
        }

        $id = $args["id"];
        $name = $args["name"];

        if (!is_numeric($id) || !ctype_alpha($name)) {
            return $response->withStatus(400);
        }

        $this->Student_DB->addStudent($id, $name);
        return $response->withStatus(200);
    }
}