Finding optimal row sequence for a melody on 5-row Bayan Accordion, v2.0 Another try at this problem in LISP. This time rather than generating every possible row combination for the melody I tried to calculate the best combination directly. Unfortunately there's a problem - the output is a messy list full of vectors and single values. Is there a good way to concatenate vectors together with value between them?

(proclaim '(type (vector (vector symbol 4) 3) *rows*))
(defparameter *rows* #( #(:c :ds :fs :a) #(:cs :e :g :as) #(:d :f :gs :b)))

(defun row-for-note (note)
  (declare (symbol note))
  "Returns the lower row on which a note is played."
  (position-if (lambda (row) (find note row)) *rows*))

(defun equivalent-row (row)
  (declare (fixnum row))
  "Returns the equivalent upper row to a given lower row."
  (case row (1 -2) (2 -1) (0 0)))

(defun rows-for-notelist (notelist)
  (declare ((vector symbol) notelist))
  "Returns the sequence of lower rows on which a sequence of notes are played."
  (map 'vector #'row-for-note notelist))

(defun flip-sequence (rowlist)
  (declare ((vector fixnum) notelist))
  "Returns the sequence of upper rows equivalent to a sequence of lower rows."
  (map 'vector #'equivalent-row rowlist))

(defun vector-last (thevec)
  (declare (vector thevec))
  "Returns the last element of a vector."
  (elt thevec (- (length thevec) 1)))

(defun vector-first (thevec)
  (declare (vector thevec))
  "Returns the first element of a vector."
  (elt thevec 0))

(defun should-flip-block (rowlist)
  (declare ((vector fixnum) rowlist))
  "Determines if a sequence of rows starts and ends on row 2 or not."
  "These sequences are more easily played on upper rows because they will then start and end on row -1."
  (if (= (length rowlist) 0) nil
    (let* ((exithigh (= (vector-last rowlist) 2))
           (enterhigh (= (vector-first rowlist) 2)))
      (and exithigh enterhigh))))

(defun flip-block-if-should (rowlist)
  (declare ((vector number) rowlist))
  (if (should-flip-block rowlist) (flip-sequence rowlist) (eval rowlist)))

(defun optimal-sequence (notelist)
  (declare ((vector symbol) notelist))
  (let* ((baserows (rows-for-notelist notelist))
         (blocks (split-sequence '(0) baserows))
         (flipped (mapcar #'flip-block-if-should blocks)))
    (loop for x in flipped collect x collect 0)))

To concatenate vectors, you can do this:

(apply #'concatenate 'vector
       (mapcar (lambda (x)
                 (typecase x
                   (vector x)
                   (t (vector x))))
               list-of-vectors-or-numbers))

PS. Since this is CR, here are some notes on your code:

Doc string (no more than one, but it can be multi-line!) usually comes before the declarations. Please see Syntactic Interaction of Documentation Strings and Declarations: (defun foo (...) (declare ...) "doc") will define a function foo which has no docstring and return "doc", while (defun foo (...) "doc" (declare ...)) will declare a function with a docstring, returning nil.
(declare ((vector number) rowlist)) should be (declare (type (vector number) rowlist)).
elt should not be used if you know that the object is a vector; use aref instead.
There is no need to bind exithigh and enterhigh in should-flip-block since they are used just once.
declaim is, I think, more "idiomatic" than proclaim in your case.
It is clearer to use 1- in vector-last.


getint that properly handles + or - signs As written, getint treats a + or - not followed by a digit as a valid representation of zero. Fix it to push such a charachter back on the input.

Before the for I check if the charachter after the + or - is a number. If it is, the function will store the representation of that number in *pn. If it is not, it will push back that characther back on the input - such that, everytime the getint is called it will
read that charachter and push it back.

Here is my solution:

int getint(int *pn) {
    int c, sign;
    while(isspace(c = getch()))
        ;
    if(!isdigit(c) && c != EOF && c != '+' && c != '-') {
        ungetch(c);
        return 0;
    }

    sign = (c == '-') ? -1 : 1;
    if(c == '+' || c == '-')
        c = getch();

    if(!isdigit(c)) {
        ungetch(c);
        return 0;
    }

    for(*pn = 0; isdigit(c); (c = getch()))
        *pn = 10 * *pn + (c - '0');
    *pn *= sign;
    if(c != EOF)
        ungetch(c);
    return c;
}

The method modifies the parameter while it is processing input. It should store the processing result locally and then assign it once it is finished.
It is unclear what the return value of the method means just by reading the code. If it is supposed to indicate success (return is not 0) or failure (return value is 0) then I think you have subtle bug in there in the case when the last character read from input is '\0' - in which case you return 0 even though you potentially have parsed a number. You should simply return 1; to indicate the success case.


Part 2: Send HTTP request for each row in Excel table This script was previously reviewed here: Send HTTP request for each row in Excel table (Part 1)
I've made the changes that were suggested in the Code Review answers as well as added a few of my own.
The code is quite a bit different now. Are there any more improvements that should be made?

Description:

A custom function concatenates columns with parameters into the Concatenated Variables column.
Loops through each row in the table where Load? = y
Sends an HTTP request to an external system using the value in the URL column.
Returns a message (created, updated, or problem/error) and stores it in the Message column.

New code:
Option Explicit

Public Const tblName = "tblData"
Public Const colNameLoad = "Load?"
Public Const colNameMessage = "Message"
Public Const colNameURL = "URL"
Public Const colNameTimestamp = "Message Timestamp"

Function CodeName() As Worksheet
Set CodeName = DataSheet
End Function

Public Sub LoadRecords()
    Application.CalculateFull
    Dim message As String, response As String
    Dim n As Long

    'Keep an eye on unecessary calls to the ConcatVars function.
    With CodeName.ListObjects(tblName)
        .ListColumns(colNameMessage).Range.Interior.Color = rgbWhite
        .ListColumns(colNameMessage).Range.Font.Color = rgbLightGrey
        For n = 1 To .ListRows.Count
            If UCase(.ListColumns(colNameLoad).DataBodyRange(n).Value) = "Y" Then
                response = GetHTTP(.ListColumns(colNameURL).DataBodyRange(n).Value)
                .ListColumns(colNameMessage).DataBodyRange(n) = response
                .ListColumns(colNameMessage).DataBodyRange(n).Font.Color = rgbBlack
                .ListColumns(colNameTimestamp).DataBodyRange(n) = Now()

                With .ListColumns(colNameMessage).DataBodyRange(n)
                    message = Left(response, 7)
                    .Interior.Color = Switch(message = "Created", rgbLightGreen, message = "Updated", rgbSkyBlue, message = "Problem", rgbYellow, True, rgbOrangeRed)
                End With

            End If
        Next
    End With
End Sub

Public Function GetHTTP(ByVal url As String) As String

On Error GoTo ConnectionError:

    With CreateObject("MSXML2.XMLHTTP")
        .Open "GET", url, False: .Send
        GetHTTP = VBA.StrConv(.responseBody, vbUnicode)
    End With

On Error GoTo 0
Exit Function
ConnectionError:
GetHTTP = "Problem with URL or server: " & Err.Number & " " & Err.Description

End Function

Function ConcatVars(RowNum As Integer) As String
    Dim Column As ListColumn
    Dim s As String

    For Each Column In CodeName.ListObjects(tblName).ListColumns
        If Column.Name Like "f_*" Then
            s = s & IIf(Len(s) > 0, "&", "") _
                & Mid(Column.Name & "=" & Column.Range.Cells(RowNum).Value, 3)
        End If
    Next
    ConcatVars = s
End Function

I have a follow-up question here: Create or update record via HTTP request  (Python/Jython).

Your code is pretty solid. I so have a couple of minor tweaks.

Function CodeName() As Worksheet
Set CodeName = DataSheet
End Function

This function is just adding a layer obfuscation.   You should just change the worksheets code name.

Ctrl + R will Open the Folder Explorer
Alt + F4

From here you can change the Worksheet's Name property.

Combining lines should be avoided. It detracts from the readability of the code.  I am not opposed to declaring a variable and initiating its value in one line but only because you are allowed to assign values when declaring variables in most other languages.
Dim Target as Range: Set Target = Sheet1.Range("A1")
Here you are combing two actions into a single line.  Not only does it make it harder to read but it may run into problems later on when you are writing similar code.

.Open "GET", url, False: .Send

Consider for instance that weeks down the you are having problems writing a new function because you forgot to .Send your request.  So what do you do?  You reference your code base.  Combing the .Open and .Send request into one line makes it hard to distinguish the ↑code above↑  from the ↓code below↓.

.Open "GET", url, False

Although you never asked about speed I think its worth mentioning.
Adding Application.ScreenUpdating = False to the beginning of your code will make it run considerably faster.
Setting the varAsync parameter of the MSXML2.XMLHTTP.Open() method to True will allow the rest of the code to run while the HTTP XMLHTTP.Request() is processing.  This will allow you to create more connections.  Having 50 or more connections processing at once will greatly sped up the code.

Sub open(bstrMethod As String, bstrUrl As String, [varAsync],
[bstrUser], [bstrPassword])

In my answers to the questions below I create a connection pool.  I initiate the pool with x number of connections.  As the requests are completed the newly freed connection is given a new request.
Shameless plug:

Data scraping from Internet with Excel-VBA
Retrieve data from eBird API and create multi-level hierarchy of locations

In truth, setting up a connection pool, is probably over kill.  But it is nice to know that it can be done in case you every need the extra speed boast.


K&R 1-14 - Histogram This is the K&R Exercise 1-14 of C. The exercise ask you to print a histogram of the frequencies of different characters in input. How can I improve my code? The main doubt is the part when I have to increase letters in the array.

#include <stdio.h>
#include <ctype.h>

int main()
{
    int ch,
        nwhite,
        nother;

    int num[10] = {0};
    int lett[26] = {0};

    nwhite = nother = 0;

    while ((ch = getchar()) != EOF)
    {
        if (isdigit(ch))
            ++num[ch-'0'];
        else if (isalpha(ch))
        {
            if(ch >='A' && ch <= 'Z')
                ch = tolower(ch);

            if(ch >= 'a' && ch <= 'k')
                ++lett[ch-'a'];
            else if(ch >= 'l' && ch <= 'v')
                ++lett[ch-'l'];
            else if(ch >= 'w' && ch <= 'z')
                ++lett[ch-'w'];
        }

        else if (ch == ' ' || ch == '\n' || ch == '\t')
            ++nwhite;
        else
            ++nother;
    }

    putchar('\n');

    printf("Numbers|");
    for(int i = 0; i <= 10; ++i)
    {
        for(int j = 0; j < num[i]; ++j)
            putchar('*');
    }
    putchar('\n');

    printf("Letters|");
    for(int i = 0; i <= 10; ++i)
    {
        for(int j = 0; j < lett[i]; ++j)
            putchar('*');
    }
    putchar('\n');

    printf("White spaces|");
    for(int i = 0; i <= nwhite; ++i)
    {
        if(nwhite - i > 0)
            putchar('*');
    }
    putchar ('\n');

    printf("Others|");
    for(int i = 0; i <= nother; ++i)
    {
        if(nother - i > 0)
            putchar('*');
    }
    putchar ('\n');

    return 0;
}

Bugs.

The loop printing the histogram lines for numbers,

for(int i = 0; i <= 10; ++i)

lets i become 10, which accesses num[10]. It does not exist. Undefined behavior it is.

A c idiom is to use < in a loop termination condition. There are cases which require <=, but they are very rare.

The loop printing the histogram lines for letters,

for(int i = 0; i <= 10; ++i)

only reports counters for a to k inclusive. All other letters are not reported.
The second bug tells why magic numbers are bad. You want to report all counters in the lett array. There are sizeof(lett) / sizeof(lett[0]) of them:

    for(int i = 0; i < sizeof(lett) / sizeof(lett[0]); ++i)

Ditto for num.
It is very unclear why you split handling letters into three cases. A simple

    else if (isalpha(ch)) {
        ch = tolower(ch); // No need to test for `isupper`
        ++lett[ch - `a`];
    }

is enough.
There are more whitespaces than ' ', '\n', '\t'. The ctypes.h has isspace function. Use it.
Testing for nwhite - i > 0 is yet another manifestation of why <= loop condition is wrong.

    for(int i = 0; i < nwhite; ++i) {
        putchar('*');
    }

is all you need.
Once you fix the last two loops, all the loops doing an actual printing become identical. Factor them out into a function

void print_stars(int n)
{
    for (int i = 0; i < n; i++) {
        putchar('*');
    }
    putchar('\n');
}


Replacing DOM elements that have no attributes The institution I work for uses an application that has an awful software bug causing useless span tags to appear next to "misspelled" words in iframes.

For instance, HTML that should look like this:

Hello! My name is Tabitha.

...might look like this instead:

Hello! My name is Tabitha.

My band-aid solution, while we wait for a fix from the application developer, has been to add a script that finds all attribute-less span tags and replaces them with a new textNode. This works because these extra tags are always the direct parent of the textNode containing the misspelled word (or another extra span element). Any formatting or inline styling would occur outside that scope and would thus be unaffected.

But, something tells me there's got to be a better way to do it. Any thoughts?

function fixSpan(iframe) {
    var doc = iframe.contentDocument;
    var spans = doc.getElementsByTagName('span'); //find all  elements
    for (var i = 0; i < spans.length; i++) {
        var span = spans[i];
        var tag = span.outerHTML.slice(0,6);
        if (tag === '') { //if the span has no attributes
            //replace it with a new textNode containing its textContent
            console.log('FOUND: ' + span.textContent + '');
            span.parentNode.replaceChild(document.createTextNode(span.textContent), span);
        }
    }

    if (doc.documentElement.innerHTML.includes('')) {
        //if any attribute-less tags remain, run the method again
        console.log('Fix not complete, re-running...');
        fixSpan(iframe);
    } else {
        console.log('Fix complete.');
    }
}

Some thoughts about your code:

1) You work with the serialized form of the HTML, i.e. with innerHTML and outerHTML. In some (rare) occurrences, it may be acceptable to do so, but most of the time working with the provided API is cleaner. I don't think that the serialized representation is guaranteed to be the same across user agents (attribute ordering, legacy uppercasing of HTML...) - plus what if some inline script declares some '<span>' text... like yours?

2) Re-running the method is a bit dubious, you should be able to remove all spans in a single pass. In your case, the DOM collection is live, so each time you replace a span element, the current index must be shifted back. In itself, manually updating the index of a loop is a bad practice, but Javascript has limited iteration mechanisms, so you have to live with it. It's actually not that unusual in DOM manipulation.

3) Removing the spans without attributes is well and good, but even an empty span may be styled, as part of another selector (e.g. .foo > span). Your fix may bring new problems.

4) Comments should not repeat the code, but bring enlightenment to the reader. Indicating "find all <span> elements" aside a getElementsByTagName is probably unneeded. Indicating why you shift a loop index, if you decide to do it, is necessary.

function fixSpan(doc) {
    var spans = doc.getElementsByTagName("span");
    for (var index = 0; index < spans.length; index++) {
        var span = spans[index];
        if (span.attributes.length == 0) {
            span.parentNode.replaceChild(document.createTextNode(span.textContent), span);
            index--; // Tree is live; removing one span requires shifting the index.
        }
    }
}


Comparing two anagrams in C This is a simple attempt by me to test whether two words are anagrams or not:

#include <stdio.h>
#include <ctype.h>

#define CH_LEN 15
#define N 26

int main(void) {
    char track_letters[N] = {0};
    char first_word[CH_LEN];
    char sec_word[CH_LEN];
    printf("Enter first word: ");
    int i;
    for(i = 0; i < CH_LEN; i++) {
        first_word[i] = getchar();
        if(first_word[i] == '\n')
            break;
        if(!isalpha(first_word[i]))
            --i;

        track_letters[(tolower(first_word[i]) - 'a')]++;

    }
    printf("Enter second word: ");
    for(i = 0; i < CH_LEN; i++) {
        sec_word[i] = getchar();
        if(sec_word[i] == '\n')
            break;
        if(!isalpha(sec_word[i]))
            --i;
        track_letters[(tolower(sec_word[i]) - 'a')]--;

    }
    for(i = 0; i < CH_LEN; i++) {
        if(track_letters[i] != 0) {
            printf("The words are not anagrams");
            return 0;
        }
    }
    printf("The words are anagrams");

    return 0;
}

You can use any method of C (structure, pointers, functions etc.).

You limit yourself to anagrams of 15 letters.

#define CH_LEN 15
char first_word[CH_LEN];
char sec_word[CH_LEN];

But you don't actually use words after they are input.
You can fix the limitation (and a potentially serious bug) by not using the concept of a line. Just read characters.

In the code just read characters:

char nextInput = getchar();

No all alphabets are 26 letters.

#define N 26

I would have done this for clarity to show that you are looking at a-z:

#define N  ('z' - 'a' + 1)

Also N is not that descriptive. Personally I would have preferred a global static variable.

static const AlphabetSize = ('z' - 'a' + 1);

You basically repeat the same piece of code twice. This violates the DRY principle. Remove this code into a function and call the function passing parameters for the differences.

    int i;
    for(i = 0; i < CH_LEN; i++) {
        first_word[i] = getchar();
        if(first_word[i] == '\n')
            break;
        if(!isalpha(first_word[i]))
            --i;

        track_letters[(tolower(first_word[i]) - 'a')]++;

    }
    printf("Enter second word: ");
    for(i = 0; i < CH_LEN; i++) {
        sec_word[i] = getchar();
        if(sec_word[i] == '\n')
            break;
        if(!isalpha(sec_word[i]))
            --i;
        track_letters[(tolower(sec_word[i]) - 'a')]--;

    }

I would write like this:

    ProcessesAnagram(track_letters, +1);  // Reads first Line
    ProcessesAnagram(track_letters, -1);  // Reads second line

Since we are not longer bound by a word size. I would change the loop to check for the end of line (or end of input)).

 void ProcessesAnagram(int* track_letters, int increment)
 {
    int nextLetter = getchar();
    for(;nextLetter != EOF && nextLetter != '\n'; nextLetter = getchar())
    {
        if(!isalpha(nextLetter)
        {    continue;           // If not a letter just start the next iteration
        }

        // Note: increment is +1 first anagram
        //       increment is -1 second anagram (so they will cancel out)
        track_letters[(tolower(nextLetter) - 'a')] += increment;
    }
 }

There is a bug here

You are using the wrong size: CH_LEN is the size of the words. But the track_letters array has a size of N.

    for(i = 0; i < CH_LEN; i++) {
        if(track_letters[i] != 0) {
            printf("The words are not anagrams");
            return 0;
        }
    }


Small Poker program rewrite I had already put up code for review a small poker program but I have rewrote it utilizing a more appropriate data structure.

How would I compare Poker hands accordingly?

Full source repo

enum poker_hands check_hand(vector *hand) /* check what kind of poker hand player has and return a score from 1 to 10 */
{
    sort_hand(hand);
    struct Card_s *temp[MAXHAND];
    for (int t = 0; t < MAXHAND; ++t) {
        temp[t] = (struct Card_s*) vector_get(hand, t);
    }

    /* check for flushes */
    {
        bool flushsuit = true;
        for (int i = 1; i < MAXHAND; ++i) {
            if (temp[0]->nSuit != temp[i]->nSuit) { /* check if card 1 is the same suit as the rest */
                flushsuit = false;
                break;
            }
        }
        if ( flushsuit ) {
            if ( temp[0]->nType == (temp[1]->nType-1) /* check if the cards increment one after the other */
                && temp[1]->nType == (temp[2]->nType-1)
                && temp[2]->nType == (temp[3]->nType-1)
                && temp[3]->nType == (temp[4]->nType-1) )
            {
                if ( temp[4]->nType == ace )
                    return royal_flush; /* found royal flush */
                else
                    return straight_flush; /* found straight flush */
            }
            return normal_flush; /* it was a normal flush */
        }
    }

    int group[MAXHAND+1]    = { 0 } ;
    int rankcount[MAXTYPES] = { 0 } ;

    for(int i = 0; i < MAXHAND; ++i) {
        int rank = temp[i]->nType;  // Get each card's type
        rankcount[rank]++;      // Increment each rank index
        group[rankcount[rank]]++;   // increment each rank's count index
    }

    if ( group[4] )
        return four_kind;

    if ( group[3] && group[2] > 1 ) // Found 3 of a kind and found another pair
        return fullhouse;

    if ( temp[0]->nType == (temp[1]->nType-1)
        && temp[1]->nType == (temp[2]->nType-1)
        && temp[2]->nType == (temp[3]->nType-1)
        && temp[3]->nType == (temp[4]->nType-1) ) {
        return straight; /* found straight */
    }

    if ( group[3] )
        return three_kind;

    if ( group[2] > 1 ) // If the 2nd index of the group[] array is more than 1, two pairs are found
        return two_pair;

    if ( group[2] )
        return pair;

    return highcard ; /* player only has a high card value */
}

Naming problem or design issue.  The first thing check_hand() does is to re-order the data.  I'd expect check_hand() to not alter hand, base on its name - it is a check.  Alternatively  just re-name it sort_check_hand().   Yet later code make a copy in temp[].  Better to copy into temp[] and then sort temp[], leaving hand[] alone.

//enum poker_hands check_hand(vector *hand) {
//  sort_hand(hand);
enum poker_hands sort_check_hand(vector *hand) {
  sort_hand(hand);

Code to detect a straight is redundant in flush strait and normal straight calculate.  Need only one.
Group detection.  Some poke like games find the best hand out of 5 cards (usual) or more (like 7 in 7 card stud) or betting order based on incomplete hands (1 to 9) cards.  A hand assessment that does not depend on 5 cards is more useful.
Rating.  Returning info as what kind of full house (Jacks over fives) is useful info for many games.  This code only returns fullhouse, three_kind etc, yet only needs a little more code to report the hand details.
Minor: Code for understanding.  By decrementing the smaller group count, group[2] only reports the numbers of pairs, not the sum of numbers of pairs, triples and 4-of-a kind.

// original
for(int i = 0; i < MAXHAND; ++i) {
  int rank = temp[i]->nType;  // Get each card's type
  rankcount[rank]++;      // Increment each rank index
  group[rankcount[rank]]++;   // increment each rank's count index
}

if ( group[3] && group[2] > 1 ) // Found 3 of a kind and found another pair
  return fullhouse;

// suggested revision
for(int i = 0; i < MAXHAND; ++i) {
  int rank = temp[i]->nType;
  group[rankcount[rank]]--;   // add
  rankcount[rank]++;
  group[rankcount[rank]]++;
}

if (group[3] > 0 && group[2] > 0) {
  return fullhouse;


Overwriting native console.log function in JavaScript I want to override the console.log function in order to save the data other scripts log. My script executes before any other script, so I have full control over the environment. This is my current function for overwriting console.log:

+function(){
    var a = Function.prototype, b = 'toString', c = a[b], d = c.apply(c).split(c.name), e = [], f = [], g = function(a, b){
        e.push(a);
        f.push(b);
        return a;
    }, _ = [];

    a[b] = function(h){
        return g(function toString(){
            var i = e.indexOf(this);
            return i != -1 ? f[i] || d.join(this.name) : h.apply(this);
        });
    }(c);

    console.log = function(a){
        return g(function log(){
            var b = [], i;
            for(i in arguments) b.push(arguments[i]);
                _.push(b.slice());
            return a.apply(this, arguments);
        });
    }(console.log);
}();

My question is simple: is it possible that some script executed after this can reveal that I have an overwritten console.log function?

I will use it for debugging scripts sent from users to my Node.js server application. I am using vm module, so I need to overwrite sandbox the object's console.log function in order to trace what users log to console in received scripts.

If this code looks obfuscated, here is a better version.

I tried hacking around this in various ways and practically the only way I could detect it was wrapped was by having an old reference to console.log and using ===. Since you state your code will run before any other scripts will run, this shouldn't be a problem.

That said, there's also console.info, warn, trace, so maybe you'd want to wrap those too?

From a code quality perspective, your variable naming needs work. You can haul it through a minifier later; what you have right now is unmaintainable. I don't even know what half of it does thanks to the variable naming.


Write Once Use Everywhere: Multidimensional array traversal I have a structure that is organized much like a Sudoku board, where individual cells reside in an n x n region, and those regions reside in an n x n area. I need to be able to address both the regions and the individual cells within the regions separately.

When operating on every individual cell (in a particular order), I (previously) had a number of huge, unwieldy, 4-deep 'for' loops like so (traversing in a doubly row-major fashion):

for (int i_row = 0; i_row < BOUND; ++i_row) {
    for (int j_row = 0; j_row < BOUND; ++j_row) {
        for (int i_col = 0; i_col < BOUND; ++i_col) {
            for (int j_col = 0; j_col < BOUND; ++j_col) {
                // operate on cell at (i_row, i_col, j_row, j_col)
            }
        }
    }
}

To eliminate these, I created a utility class so I could write it once and use it everywhere:

public class TraversalUtil {
    // private to prevent instantiation
    private TraversalUtil() {}

    public static void traverseDoubleRowMajor(TraversalFunc func) {
        int bound = Context.getInstance().getBound();
        for (int i_row = 0; i_row < bound; ++i_row) {
            for (int j_row = 0; j_row < bound; ++j_row) {
                for (int i_col = 0; i_col < bound; ++i_col) {
                    for (int j_col = 0; j_col < bound; ++j_col) {
                        func.exec(i_row, i_col, j_row, j_col);
                    }
                }
            }
        }
    }

    @FunctionalInterface
    public interface TraversalFunc {
        public void exec(int i_row, int i_col, int j_row, int j_col);
    }
}

So usage becomes:

TraversalUtil.traverseDoubleRowMajor( (i, j, k, m) -> {
    System.out.println(getCellAt(i, j, k, m)); // or whatever
});

I'm wondering if this is bad practice or if anyone has any suggestions/changes/improvements. I'm reluctant to change the '2d array of 2d arrays' structure itself because it is a dream to work with in other contexts and circumstances, so fundamental design changes to that model is a much lower priority for me.

I have run into an issue where I've needed external variables (mainly primitives) to be available to the content inside the lambda. To work around that I've been using wrapped primitives marked as final, which seems to work pretty well for me.

Any thoughts are greatly appreciated!

where individual cells reside in an n x n region, and those regions
  reside in an n x n area

First problem, there are no Area or Region or Cell in the code provided.

  I need to be able to address both the regions and the individual cells
  within the regions separately.

Then you should have written something like

class Area {
    Region getRegion(int row, int col);
}

class Region {
    Cell getCell(int row, int coll);
}

class Cell {

}

  so I could write it once and use it everywhere:

Then you should write something composable. void exec(...) is not very composable; because it returns void it can't be pipelined. Extract Streams (or methods returning a value), and keep Consumers (or methods returning void) simple.

For example if your utility method traverseDoubleRowMajor were something like :

Stream<Cell> cellsOf(Area area)

instead of :

TraversalUtil.traverseDoubleRowMajor( (i, j, k, m) -> {
    System.out.println(getCellAt(i, j, k, m)); // or whatever
});

you could then simply do :

 cellsOf(area).forEach(System.out::println);

So how could you write a cellsOfRegion? It'd be easy if you'd first made areas and regions etc explicit as I've shown above:

private static Stream<Cell> cellsOf(Area area) {
    return gridElements(BOUND, BOUND, area::getRegion)
            .flatMap(region -> gridElements(BOUND, BOUND, region::getCell));
}

private static <R> Stream<R> gridElements(int maxRow, int maxCol, BiFunction<Integer, Integer, R> f) {
    return IntStream.range(0, maxRow).boxed().flatMap(row -> (
            IntStream.range(0, maxCol).boxed().map(col -> f.apply(row, col))));
}

EDIT

  For low iterations, how big is the performance hit for setting up the
  pipeline for a steam?

Short answer, you should try for yourself. Longer prediction: In almost all cases IO dominates anyway. Where IO is not involved memory access should dominate. And a pipeline using streams should be faster than a pipeline that puts the intermediate results in collections.

List<A> inputElements = elementsOf(input);
List<A> result1 = pipelineStep1(inputElements);
List<A> result2 = pipelineStep2(result1);
// etc etc...
B resultN = pipelineStepN(resultNMinusOne);

should be slower than

B result = elementsOf(input)
    .filter(pipelineStep1)
    .flatMap(pipelineStep2)
    // etc etc...
    .collect(pipelineStepN);

  And second, is there a way to achieve different traversal orders (i.e.
  not just row major)?

Sure, one solution would be:

private static <R> Stream<R> colMajor(int maxRow, int maxCol, BiFunction<Integer, Integer, R> f) {
    return IntStream.range(0, maxCol).boxed().flatMap(col -> (
            IntStream.range(0, maxRow).boxed().map(row -> f.apply(row, col))));
}

Or with less repetition just

private static <R> Stream<R> colMajor(int maxRow, int maxCol, BiFunction<Integer, Integer, R> f) {
    return gridElements(maxCol, maxRow, (col, row) -> f.apply(row, col);
}

If you do this you may consider renaming method and parameter names accordingly. e.g. instead of names like row and col in gridElements, which you now would be using for both row-major and col-major traversal; canonical names for integer function parameters/loop counters i, j or m, n etc.


Function to run MySQL SELECT queries with no leaks I am upgrading an old massive website that is a Classic ASP \ ASP.NET hybrid. I still have a long way to MVC.

Right now I want to convert all code that uses ODBC to the mysql connector specific code.

I've made these 2 functions:

1:

DataTable GetData(string query, List<MySqlParameter> sqlParams, string sComment="")
{
    DataTable dt_tmp = new DataTable();

    if (sqlParams==null)
        sqlParams = new List<MySqlParameter>();

    Trace.Write(sComment, GenerateSQLString(query, sqlParams));
    using(MySqlDataReader myReader = MySqlHelper.ExecuteReader(sCon, query, sqlParams.ToArray()))
    {
        dt_tmp.Load(myReader, LoadOption.OverwriteChanges);
    }
    return dt_tmp;
}

2:

DataTable ReadData(string query, List<MySqlParameter> sqlParams, string sComment="")
{
    DataTable dt_tmp = new DataTable();

    if (sqlParams==null)
        sqlParams = new List<MySqlParameter>();

    Trace.Write(sComment, GenerateSQLString(query, sqlParams));

    using(MySqlConnection myConnection = new MySqlConnection(sCon))
    {
        using(MySqlCommand myCommand = myConnection.CreateCommand())
        {
            myCommand.CommandText = query;
            myCommand.Parameters.AddRange(sqlParams.ToArray());
            using (MySqlDataAdapter myAdapter = new MySqlDataAdapter(myCommand))
            {
                myAdapter.Fill(dt_tmp);
            }
        }
    }

    return dt_tmp;
}

Function #1: gets a ConstraintException (MaxLength) on a specific call... but otherwise performs very well.

Which one will perform better? (Ignoring the fact one throws an exception, which I can handle)

My goal is to have the connection open for the minimum amount of time possible so it won't use all connections from the pool.

Hide and seek

The first method is shorter but is that a good thing? Compared to the second one it hides a few thing internally which can be either good or bad and i would say bad. To me this method looks like a core application logic which has to be crystal clear so i think you shold not prefer the shorter version.

But wait

The second one still has some coding issues but nothing catastrophe.
I have changed it in some parts, lets see:

    private DataTable ReadData(string query, string sComment = "")
    {
        return ReadData(query, EmptyMySqlParameters, sComment);
    }

    DataTable ReadData(string query, ICollection<MySqlParameter> sqlParams, string sComment = "")
    {
        var dt = new DataTable();

        Trace.Write(sComment, GenerateSQLString(query, sqlParams));

        using (var myConnection = new MySqlConnection(sCon))
        {
            myConnection.Open();

            using (var myCommand = myConnection.CreateCommand())
            {
                myCommand.CommandText = query;

                foreach (var mySqlParameter in sqlParams)
                {
                    myCommand.Parameters.Add(mySqlParameter);
                }

                using (var myReader = myCommand.ExecuteReader())
                {
                   dt.Load(myReader, LoadOption.OverwriteChanges);
                }
            }
        }

        return dt;
    }

First i have introduced an overloaded method to remove the null check against the List (null object pattern for collections are the empty versions of them).
Changed the type to ICollection which is more flexible and intruduced a new static member which get involved in the process where no parameter is passed (can be static for reading it is thread safe).

I also removed the AddRange call becouse internally it is interating through the parameters with a foreach loop but only works with Array type parameter so after introducing our foreach loop calling Add manually we have removed an extra ToArray() call which would involve an unnecessary memory copy in the background.

Also changed the fill of the DataTable to a more used way and the MySqlHelper.ExecuteReader() does the same thing too.


Python differential analysis of heat loss across a pipe I made a differential equation solver to forecast heat loss through a pipe with heat loss coefficient contours. It does what I want it to do but I had to use global variables to change the outputs of my simulation function - is there a better way to deal with this issue? Another problem I have is that my code is very slow - when I increase dx the answer seems to become less accurate (the graphs seem to approach a single result past 0.0001)

import scipy
# R_w = 0.028# W/mk air heat loss ocefficient
R_w = 0.654# W/mk water heat loss ocefficient
R_f = 0.042 # W/mk pipe heat loss coefficient
r = 0.2 # m pipe radius
T_0 = 50 # C initial temperature
T_a = 20 # C ambiant temperature
dx = 0.0001 # m differential distance
L = 25 # m length of pipe

def heat_balance(T_1,T_2):
   heat_in = (T_1-T_2) * scipy.pi * r**2 * R_w
   heat_out = ( (T_1 + T_2) / 2 - T_a ) * 2 * r * scipy.pi * dx * R_f
   return abs(heat_in-heat_out)

def simulation():
   T = [T_0]
   X = [0]
   print 'r is ',R_f
   import numpy as np
   from scipy.optimize import minimize
   for x in np.arange(dx,L,dx):
     X.append(x)
     fun = lambda s: heat_balance(T[-1],s)
     T.append(float(minimize(fun,T[-1]-dx,method="SLSQP")['x']))
   return([T,X])

from matplotlib import pyplot as plt
typs = ['+','x','|','_','*']
#typs = ['D','s','8','p','^']
cols = ['red','blue','black','purple','brown']

for i, rf in enumerate([7,14,50,100,200]):
    R_f = 1.0/rf
    ans = simulation()
    plt.scatter(ans[1],ans[0],c=cols[i],marker=typs[i],s = 100,label=rf,lw=3)

plt.legend(fontsize=20,title='Pipe Insolation R Value')
plt.xlabel('Distance (m)',fontsize=20)
plt.ylabel('Average Temperature (C)',fontsize=20)
plt.show()

Globals in Python are not necessarily a bad thing, especially for what you're using them for. However, they can be prevented. There are a couple of major flaws I found so I'm glad you came over to get a review. The point you're most worried about will be handled at the end.

Style

Clean Python code adheres to PEP8, the general style guide for Python. Your code violates this guide in a couple of ways. I'll explain the violations I deem most important and fixed it up for you to show the difference. I do recommend you read PEP8 yourself as well.

To verify whether your code adheres to the guide, you can either use the pep8 tool inside Python itself or pep8online. This will validate your code against most of the guide.

Import

Import statements should be on top of your file for maintainability purposes. This is a common practice in many languages, including C, C++, C# and Java. If I now skim your code, I'll see unexpected functions like plt.xlabel() because I didn't notice the following line:

from matplotlib import pyplot as plt

All imports should be on the top.

Globals

Imports should be followed by your global variables. Often those globals are supposed to be constant (note that Python does not actually enforce them being constant). The naming convention for constants is UPPERCASE_WITH_UNDERSCORES. This makes it clear to anyone reading the code the value should not be changed and is probably defined at module level.

This also means that other variables should not use the same casing. All relevant casing styles are in the PEP8.

Variable naming

Now we're handling variables anyway, please use more descriptive variable names. Single-letter variables and functions make for hard-to-read code. Code should be self descriptive. With good names, many of your comments would become redundant. Functions with names are fun can definitely use something more descriptive.

Whitespace

Give your operators and commas some breathing room. foo, bar(woof + 1), fizz is a lot easier on the eyes than foo,bar(woof+1),fizz.

Indentation is done with spaces, not with tabs. Multiples of 4 are preferred, but multiples of 2 are also common.

import scipy
import numpy as np
from scipy.optimize import minimize
from matplotlib import pyplot as plt

R_w = 0.654  # W/mk water heat loss ocefficient
R_f = 0.042  # W/mk pipe heat loss coefficient
PIPE_RADIUS = 0.2  # m pipe radius
PIPE_LENGTH = 25  # m length of pipe
TEMPERATURE_INITIAL = 50  # C initial temperature
TEMPERATURE_AMBIANT = 20  # C ambiant temperature
dx = 0.0001  # m differential distance

def heat_balance(T_1, T_2):
    heat_in = (T_1-T_2) * scipy.pi * PIPE_RADIUS**2 * R_w
    heat_out = ( (T_1 + T_2) / 2 - TEMPERATURE_AMBIANT ) * 2 * PIPE_RADIUS * scipy.pi * dx * R_f
    return abs(heat_in-heat_out)

def simulation():
    T = [TEMPERATURE_INITIAL]
    X = [0]
    print 'r is ', R_f
    for x in np.arange(dx, PIPE_LENGTH, dx):
        X.append(x)
        fun = lambda s: heat_balance(T[-1], s)
        T.append(float(minimize(fun, T[-1] - dx, method="SLSQP")['x']))
    return([T, X])

typs = ['+', 'x', '|', '_', '*']
cols = ['red', 'blue', 'black', 'purple', 'brown']

for i, rf in enumerate([7, 14, 50, 100, 200]):
    R_f = 1.0/rf
    ans = simulation()
    plt.scatter(ans[1], ans[0], c=cols[i], marker=typs[i], s = 100, label=rf, lw=3)

plt.legend(fontsize=20, title='Pipe Insolation R Value')
plt.xlabel('Distance (m)', fontsize=20)
plt.ylabel('Average Temperature (C)', fontsize=20)
plt.show()

Objective Oriented Programming

This is one of the best solutions against global variables: objective oriented programming.

Did you know you can make classes in Python?

class PipeCalculator:
    self.R_w = 0.654  # W/mk water heat loss ocefficient
    self.R_f = 0.042  # W/mk pipe heat loss coefficient
    self.PIPE_RADIUS = 0.2  # m pipe radius
    self.PIPE_LENGTH = 25  # m length of pipe
    self.TEMPERATURE_INITIAL = 50  # C initial temperature
    self.TEMPERATURE_AMBIANT = 20  # C ambiant temperature
    self.dx = 0.0001  # m differential distance

    def heat_balance(self, T_1, T_2):
        heat_in = (T_1-T_2) * scipy.pi * self.PIPE_RADIUS**2 * R_w
        heat_out = ( (T_1 + T_2) / 2 - self.TEMPERATURE_AMBIANT ) * 2 * self.PIPE_RADIUS * scipy.pi * dx * R_f
        return abs(heat_in-heat_out)

    def simulation(self):
        T = [self.TEMPERATURE_INITIAL]
        X = [0]
        print 'r is ', self.R_f
        for x in np.arange(self.dx, self.PIPE_LENGTH, self.dx):
            X.append(x)
            fun = lambda s: self.heat_balance(T[-1], s)
            T.append(float(minimize(fun, T[-1] - self.dx, method="SLSQP")['x']))
        return([T, X])

    def plot_calculations(self):
        typs = ['+', 'x', '|', '_', '*']
        cols = ['red', 'blue', 'black', 'purple', 'brown']

        for i, rf in enumerate([7, 14, 50, 100, 200]):
            self.R_f = 1.0/rf
            ans = simulation()
            plt.scatter(ans[1], ans[0], c=cols[i], marker=typs[i], s = 100, label=rf, lw=3)

        plt.legend(fontsize=20, title='Pipe Insolation R Value')
        plt.xlabel('Distance (m)', fontsize=20)
        plt.ylabel('Average Temperature (C)', fontsize=20)
        plt.show()

calculator = PipeCalculator()
calculator.plot_calculations()

And poof, all the globals are gone.

Of-course, a straight port from what you have to object oriented still looks ugly. Pass some variables around, use the power of arguments and when in doubt don't hesitate to split up to more functions. But at least you now have a start and it will be easier to re-use this code later on in different projects.

As two final notes:

Your lambda could be re-written to a normal function for improved readability. This may help you figure out how to optimize the calculations.
Pipes usually have an inside and outside diameter. You may want to explicitly state which one you're using.


All combinations of two pairs of elements of an array Suppose I have an array of objects. Without loss of generality, it could be an array of indices:
$$a = (0, 1, 2, ..., n)$$
Now suppose I want to choose two pairs of elements of this array. The order is not of the matter inside pairs so for pairs itself.

For example if
$$a = (1,2,3,4)$$
then all those combinations would be
$$(1,2), (3,4)\\(2,3), (1,4)\\...$$
But combinations like
$$(1,2), (3,4)\quad \text{and}\quad (4,3), (1,2)$$
should be considered as equivalent.

I am a little bit confused how to write compact and effective algorithm for my purpose. I feel that there is a way to do it for general case (not just for pairs) but I cannot find it. If you know, please share.

So far, here is my code.

#include <iostream>
#include <algorithm>
#include <vector>

int main()
{
    std::vector< int > targetVector = { 1, 2, 3, 4, 5 };

    std::vector< int > index_pair = { 2, 2, 1, 1, 0 };

    int pair1, pair2;
    do
    {
        //reset pairs to 00000
        pair1 = pair2 = 0;

        for ( int i = 0; i < (int)index_pair.size(); i++ )
        {
            if( index_pair[i] == 1 ) { pair1 += ( 1 << (i+1) ); }
            if( index_pair[i] == 2 ) { pair2 += ( 1 << (i+1) ); }
        }

        if ( pair1 > pair2 )
        {
            for ( auto index : index_pair )
            {
                std::cout << index;
            }
            std::cout << "\n";
        }
    }while( prev_permutation( index_pair.begin(), index_pair.end() ) );

    return 0;
}

Here is the output that seems to be right.

22110
22101
22011
21210
21201
21021
20211
20121
12210
12201
12021
10221
02211
02121
01221

Unused variables
We never use targetVector, so we can omit its definition.
Reduce scope
We can move pair1 and pair2 inside of the do loop.
Don't cast to signed
This looks like a misguided attempt to appease a compiler warning:
    for (int i = 0; i < (int)index_pair.size(); i++) {

Whilst it's good that you have enabled a good set of warnings, the better response would be to change the type of i to match (especially as we intend to use it as an indexer):
    for (std::size_t i = 0u;  i < index_pair.size();  ++i) {

We can also simplify the if chain if we use a small array for pair:
    std::size_t pair[3] = {};

    for (std::size_t i = 0u;  i < index_pair.size();  ++i)
        pair[index_pair[i]] += 1u << (i+1);

    if (pair[1] > pair[2]) {

We could use range-based for instead:
    std::size_t n = 0;
    for (auto i: index_pair)
        pair[i] += 1u << ++n;

It's not obvious whether this is better than the index loop, but you might prefer it.
A typo
You appear to have misspelt std::prev_permutation in the while condition.

Modified code
Without changing the algorithm, I have simplified to just
#include <algorithm>
#include <iostream>
#include <vector>

int main()
{
    std::vector<int> index_pair = { 2, 2, 1, 1, 0 };

    do {
        std::size_t pair[3] = {};

        for (std::size_t i = 0u;  i < index_pair.size();  ++i)
            pair[index_pair[i]] += 1u << (i+1);

        if (pair[1] > pair[2]) {
            for (auto index: index_pair) {
                std::cout << index;
            }
            std::cout << "\n";
        }
    } while (std::prev_permutation(index_pair.begin(), index_pair.end()));
}


An attempt at a simple type safe python enum There have been many posts here about enums in python, but none seem to be both type safe and simple. Here is my attempt at it. Please let me know if you see anything obviously wrong with it.

def typeSafeEnum(enumName, *sequential):
    topLevel = type(enumName, (object,), dict(zip(sequential, [None] * len(sequential))))
    vals = map(lambda x: type(x,(topLevel,),{}), sequential)
    for k, v in zip(sequential, vals):
        setattr(topLevel, k, v())
    return topLevel

Then to test it:

Colors = typeSafeEnum('Colors', 'RED', 'GREEN', 'BLUE')

if __name__ == '__main__':
    x = Colors.RED
    assert isinstance(x, Colors)

def typeSafeEnum(enumName, *sequential):

Python convention is lowercase_with_underscores for functions and parameters. Although since this is a type factory its not totally clear what convention should appply. I also wonder if passing the enum values as a list might be better.

    topLevel = type(enumName, (object,), dict(zip(sequential, [None] * len(sequential))))

There is no point in the dict you are passing since you just setattr all the enums in place already. Here you set all the enum values up with None initially and then fill them in. Why? I can also see why you call this topLevel but its not an immeadiately obvious name

    vals = map(lambda x: type(x,(topLevel,),{}), sequential)

List comprehensions are generally preffered to calling map with a lambda. But also, why? Just do this in the for loop. You don't gain anything by having it done in a map and then zipping over it.

    for k, v in zip(sequential, vals):

I suggest longer less abbreviated names

        setattr(topLevel, k, v())
    return topLevel


Laying out images for a landing page I'm beginning to learn HTML and CSS, and since our lead developer is busy finishing up our project, I was asked to code the landing page.

I read up on some tutorials and beginning books, so I have a general grasp of things.  But I'm curious as to how I use HTML and CSS to layout images efficiently.

When I first made the site, I used relative and absolute positioning to layout all the images. That was not good.

This time I decided to use margins/padding to organize it, but I still feel like there is a much better to lay all this out.

Basically I just want to be able to code the website, where if I add an image or move around images, the original pieces of the website don't end up going all over the place.

This is a rough image of what our landing page is supposed to look like:

HTML

CSS

body{
    background:url(images/whiteBackground.png);
}

.container{
    width:1200px;
    margin:0 auto;
    position:relative;
}

#iphone{
    margin-left: 166px;
    margin-top: 88px;
}
/* images Block */

#imagesBlock{
    position:relative;
    left: 550px;
    bottom:592px;
    width: 515px;
}

#DashLogo{

}

#textQuestion{
    margin-top:35px;
}

#videoThumbnail{
    margin-top:20px;
}

#textQuestion{
}

#textResponse{
    margin-bottom: 50px;
    margin-left: 30px;
}

#appStoreOrange{
    margin-top: 20px;
}

.twitter{
    width:117px;
    height:112px;
    display: block;
    background: url(images/twitter.png);
    margin-left: 280px;
    margin-top:-105px;
}

/*.twitter:hover{
    background-image: url(images/twitterBlue.png);
}*/

.facebook{
    width:117px;
    height:112px;
    display:block;
    background: url(images/facebook.png);
    margin-left:400px;
    margin-top:-112px;
}

/*.facebook:hover{
    background-image:url(images/facebookBlue.png);
}*/

A good tip is to layout in columns and not rows, similar to a newspaper. That is, divide the page into columns and stack the contents as rows, similar to Pinterest. So, in your site, that makes the iPhone 1 column, and the rest in another column. Now this simplifies things.

Stacking the rest as rows controls their bounds and acts as groupings. That way, if one of these rows were to expand like if the slogan has more text, it does not affect the layout of the next row since the next row clears below it cleanly.

As for pseudocode, it looks something like (Sorry, I really hate typing the <> and {}):

HTML:

div id="wrapper"

  div class="left"
    img src="iphone.png"

  div class="right"

    img class="row" src="logo.png"

    p class="row"
      Picking good food fast

    p class="row"
      Ever think...

    div class="row"
      img src="thumb"
      span
        Yeah...

    ul class="row"
      li
        a
          img src="appstore.png"
      li
        a
          img src="twitter.png"
      li
        a
          img src="facebook.png"

CSS:

#wrapper
  overflow:hidden //just a clearfix
  zoom:1

.row
  display:block  //this makes the element a box
  clear:both     //nothing is at the left and right, and must be above and below

.left
  float:left
  width: something

.right
  overflow:hidden //ensures that the right column does not wrap around left

Off-topic:

Substituting text for images is not good for several reasons:

It's a whole lot heavier than text
SEO (if ever it isn't a myth) isn't that good as they say compared to a text site. So if this is a landing page, better pack it with good description text to summarize the site.
Without properly titled images, screen readers suffer
Resizing the browser either squishes the images or if they scale, the text also shrinks making them unreadable.
Like you mentioned, they are hard to move around

However, a little mix of text and images won't hurt. A general tip is to turn into text whatever can be turned into text. That would be:

Your logo text "dash"
The slogan
Your catch phrases

And don't forget to mind the semantics. Use the proper tags for the job.


Project Euler #8 in Ruby Question:

  For a 1000-digit number, find the 13 adjacent digits which gives out the largest product.

My code goes like this:

series = '731...752963450' # the 1000 digit, simplified here.

# Set up variables.
adjacent_count = 13
pointer = 0
candidates = [], products_array = []
pointer_end = series.length - (adjacent_count - 1)

# Get all candidates
begin
    candidates <<  series[pointer..pointer + adjacent_count - 1]
    pointer += 1
end while pointer < pointer_end

# Eliminate All candidates with 0s
candidates = candidates.map {|s| s.include?('0') ? nil : s}.compact

# do the multiplication and compare
candidates.each_with_index do |s, index|
    char_array = s.chars
    product = 1
    char_array.each do |int|
        product = product * int.to_i
    end
    products_array[index] = product
end

answer = products_array.max
equation = candidates[products_array.each_with_index.max[1]].chars * 'x'

puts "Maximum product from this series of digits is #{equation} = #{answer}"

Well I don't know whether I need to eliminate the candidates that contains zeroes in it or just directly go multiply all of those. Not sure if it will save up time using map method first, Any thoughts?

And for the last bit I tried to print out which candidate brings up this largest product so I created another array to store it. It seems it can be done in a more elegant way. Any thoughts?

The easiest way to think of this problem is to use a k-character sized window that slides across the number.

First calculate the product of the first k characters
Check to see if the next digit is 0, if it is, keep going until you hit a non-zero element and restart the process from that digit
If the next digit is not 0, then multiply the existing product by that digit and divide by the first digit of the existing product (the number that is going to be dropped off the window)

Keep track of the best sequence found and its product and you have your answer

The beauty of this approach is you only ever look at/multiply an element exactly once, giving O(n) time (where n is the length of the string). Your approach may multiply the same number k times (where k is the length of the window), resulting in O(kn) time, which is quite a bit slower.


Tag specific Git commits based on commit message contents I am using GNU Bash 4.3, and Git 2.1.4, to tag certain git commits based on their commit messages. Specifically, I want to tag:

as v0.0.1 the commit with message "Commit 5",
as v0.0.2 the commit with message "Commit 10",

given the following commit log:

$ git log --format=oneline --decorate
924d9120dec9f61d0f99a5155c51bad63d24b37f (HEAD, master) Commit 10
c62b2f68ea6b8ff047dd5f1e99010ebf434709ef Commit 9
feec2d76977521018ff33fd6e109530fdb69291d Commit 8
9a25b2ea11d0aa19af33761c7c81bd10d0f17949 Commit 7
c7b02e556738e7cd2a2459e07af731e5677ab090 Commit 6
1bf5b98cf90340e714bd0bd13c7721f94749fa5f Commit 5
8a9c25eda46b3424a870584e4a8a4f90f6e5aa2c Commit 4
0ecf2464ec82a3aa74abb6f6c2127315ef4c2495 Commit 3
e18bdb2ae52f2149421965ff69d96dba46cd7c0a Commit 2
cf10d47067bef60ae787e5421e32b4541bdf52be Commit 1

Here is my initial solution:

$ v=(1 2); c=(5 10); for i in 0 1; do git tag -a v0.0.${v[$i]} -m "Tag v0.0.${v[$i]}" $(git rev-parse :/'Commit '${c[$i]}); done

This clearly works as desired:

$ git log --format=oneline --decorate
924d9120dec9f61d0f99a5155c51bad63d24b37f (HEAD, tag: v0.0.2, master) Commit 10
c62b2f68ea6b8ff047dd5f1e99010ebf434709ef Commit 9
feec2d76977521018ff33fd6e109530fdb69291d Commit 8
9a25b2ea11d0aa19af33761c7c81bd10d0f17949 Commit 7
c7b02e556738e7cd2a2459e07af731e5677ab090 Commit 6
1bf5b98cf90340e714bd0bd13c7721f94749fa5f (tag: v0.0.1) Commit 5
8a9c25eda46b3424a870584e4a8a4f90f6e5aa2c Commit 4
0ecf2464ec82a3aa74abb6f6c2127315ef4c2495 Commit 3
e18bdb2ae52f2149421965ff69d96dba46cd7c0a Commit 2
cf10d47067bef60ae787e5421e32b4541bdf52be Commit 1

But it seems excessively verbose and fragile.

Questions:

Instead of separately defining three arrays (v=(1 2); c=(5 10); for i in 0 1), can I create a single array for the two independent variables (e.g. in pseudocode: vc=((1 5) (2 10))) and then determine the incrementor's value automatically upon each iteration?
Instead of using a for loop, can I use brace expansion or some other more concise and elegant approach?

I would prefer a one-liner, but I do not require a one-liner.

If I had to write a "single-use" quickie command, I would write it like this:

while read version commit; do
    git tag -a v0.0.$version -m "Tag v0.0.$version" $(git rev-parse :/'Commit '$commit)
done

Then, as the script awaits input, type:

1 5
2 10
^D

In contrast to your parallel arrays, the main advantage of this approach is that each line constitutes a commit-version pair.


Python OOP Whac-A-Mole program using turtle I am trying to create an object oriented program in Python for a whac-a-mole program.

I want to keep this simple where there are just a hammer and a mole and when the hammer turtle makes contact with the mole turtle.

I have created 4 classes, one for Game which is the super class, one for sprite which inherits from Turtle and one each for hammer and mole which both inherit from Sprite.

What I am asking is how to instantiate the mole and hammer turtles I have made use of inheritance but want to minimize the amount of methods I am using:

import random
import turtle
wn=turtle.Screen()
wn.addshape("moleSmall.gif")
wn.addshape("Mole.gif")
wn.addshape("Hammer.gif")
turtle.setup(800, 600)

    class Game():
        def __init__(self):
            self.score=0
            self.pointsDisplay=Points()

        def update_score(self,x,y):
            self.score = self.score + 2
            print("Score:", self.score)
            self.callDisplayScore()
        def decrease_score(self,x,y):
            self.score = self.score -1
            print("Score: ", self.score)
            self.callDisplayScore()
        def callDisplayScore(self):
            score = self.score
    class Sprite(turtle.Turtle):
        def __init__(self):
            turtle.Turtle.__init__(self)
            self.x=0
            self.y=0
            crash =False
            self.crash=crash
        def randomMove(self):
            leftbound = -self.wn.window_width()/2
            rightbound = self.wn.window_width()/2
            topbound = self.wn.window_height()/2
            bottombound = -self.wn.window_height()/2
            rand_x = random.randint(leftbound , rightbound)
            rand_y = random.randint(bottombound, topbound)
            self.mole.setpos(rand_x,rand_y)
        def collisionCheck(self,turtle1, turtle2):
            self.crash = False
            turtle1X = turtle1.xcor()
            turtle1Y = turtle1.ycor()
            turtle2X = turtle2.xcor()
            turtle2Y = turtle2.ycor()
            turtle1Pos = (int(turtle1X), int(turtle1Y))
            turtle2Pos = (int(turtle2X), int(turtle2Y))
            if turtle1Pos == turtle2Pos or turtle1X < turtle2X - 50 and \
                turtle1Y > turtle2Y - 50 and  turtle1X < turtle2X - 50 and\
                turtle1Y > turtle2Y + 50:
                self.crash = True
            return self.crash
    class Mole(Sprite):
        def __init__(self):
            Sprite.__init__(self)
            self.mole.pu()
            self.mole.shape("moleSmall.gif")
            self.x= self.mole.xcor()
            self.y = self.mole.ycor()
            self.randomMove()
    class Hammer(Sprite):
        def __init__(self):
            Sprite.__init__(self)
            self.counter = 0
            self.hammer.penup()
            self.hammer.shape("Hammer.gif")
            self.hammer.penup()
            leftbound = -self.wn.window_width()/2
            rightbound = self.wn.window_width()/2
            topbound = self.wn.window_height()/2
            bottombound = -self.wn.window_height()/2
            rand_x = random.randint(leftbound , rightbound)
            rand_y = random.randint(bottombound, topbound)
            self.hammer.setpos(rand_x,rand_y)
        def HammerMove(self):
            self.hammer.pu()
            self.hammer.clear()
            self.wn.onscreenclick(self.hammer.goto)
            while self.collisionCheck(self.mole, self.hammer) == False:
                self.randomMove()
                if self.collisionCheck(self.mole, self.hammer) == True:
                    self.score = self.score + 1
                    print(self.score)

Terminology

  one [class] for Game which is the super class

What you probably mean is that it is the class that controls everything and such is on top of the logical class hierarchy. But when talking about super classes, you usually mean classes from which other classes inherit and thus are on top of (or further up) the inheritance hierarchy. In your code for example, Sprite is a super class of Mole.

White space

To make your code more readible, you should add blank lines appropriately, to separate logically related blocks of code. For example, add a blank line between methods, and two blank lines between classes. Inside of methods you can add single blank lines wherever it seems appropriate, for example around loops.

You should add spaces around operators (like self.pointsDisplay = Points() instead of self.pointsDisplay=Points()) and after commas (e. g. def update_score(self, x, y): instead of def update_score(self,x,y):), however not before commas, as you did here random.randint(leftbound , rightbound).

Refer to PEP 8, which is the Python style guide.

Naming

Your naming scheme is inconsistent. Python uses snake_case, not camelCase, for member names (see also PEP 8). You mixed both schemes, for both methods and member variables. It is always better to be consistent with one naming scheme, and prefered to follow the conventions of the language.

You are using x and y as parameter names. It is better to use speaking names that tell you what they are for or what they contain, e. g. position_x, position_y. In some cases x and y can be sufficient if the method name is explicit enough, like move_to_position(self, x, y), but I wouldn't say this is the case with update_score. Furthermore, you aren't even using the parameters in update_score and decrease_score.

update_score and decrease_score seems a bit confusing. Isn't decreasing also a kind of updating? Shouldn't the opposite be called increase_score? Maybe you can use just one method update_score for both cases, and control the amount how much it is increased or decreased with a parameter.

Logic

Your method callDisplayScore does not do anything, because all that happens when you call it is that a local variable is assigned a value, but it is not used afterwards. After the method had been called, the local variable does not exist anymore. So you could remove that method alltogether, and remove the calls to it from the methods update_score and decrease_score. Then all that remains in those two methods can basically be done without methods, because you can simply change the value of the score and then print it, which is hardly worth a method. Not using a method there will probably also make the code more readible, because it is hard to find a method name that describes well what these two lones of code do.

This is redundant:

crash =False
self.crash=crash

You can just write self.crash = False instead.

This expression is way too long to understand what it means:

if turtle1Pos == turtle2Pos or turtle1X < turtle2X - 50 and \
    turtle1Y > turtle2Y - 50 and  turtle1X < turtle2X - 50 and\
    turtle1Y > turtle2Y + 50:
    self.crash = True

Better save the result in a variable with a speaking name, or define a method that returns the result of the expression, again with a speaking name. Such a name could be is_colliding. Also using parantheses to group parts of the expression (even if they are technically not needed) can improve readability a lot.

This method also contains a lot of redundant logic, and also has not a very good name:

def collisionCheck(self,turtle1, turtle2):
    self.crash = False
    turtle1X = turtle1.xcor()
    turtle1Y = turtle1.ycor()
    turtle2X = turtle2.xcor()
    turtle2Y = turtle2.ycor()
    turtle1Pos = (int(turtle1X), int(turtle1Y))
    turtle2Pos = (int(turtle2X), int(turtle2Y))
    if turtle1Pos == turtle2Pos or turtle1X < turtle2X - 50 and \
        turtle1Y > turtle2Y - 50 and  turtle1X < turtle2X - 50 and\
        turtle1Y > turtle2Y + 50:
        self.crash = True
    return self.crash

Method names should be verbs (in most cases), so check_collision is better than collisionCheck. Even better: is_colliding or is_collision, because check_collision is not explicit about what a result of True means.

All of these variables you assign in the first 7 lines of the method are not needed. Your self.crash is not even used anywhere else, so a local variable (without self.) would be enough even if you needed the variable at all. In some cases it can be good to save values from method calls into variables in order to not have to call the methods multiple times, but if the method is just an accessor to a member variable, like turtle1.xcor(), the overhead from the method call is negligible, and readability is much more important as long as performance is not an issue. And instead of returning a boolean value depending in the result of a logical expression, just return the result of the expression instead.

Checking whether the position is the same (turtle1Pos == turtle2Pos) is obsolete if you then check with a threshold anyway. By the way, you have a bug here: turtle1X < turtle2X - 50 occures twice in the expression, perhaps on of the minuses should be a plus. And since turtle1Y > turtle2Y - 50 and turtle1Y > turtle2Y + 50 are combined with and I assume that you have another bug leading to false collisions in some cases. Here one of the > should be a <. You can simplify your collision detection logic by just checking whether the absolute distance is lower than your threshold. Optionally you could define the threshold somewhere as a variable or inject it as a parameter.

Simplifying the method, I get this:

def is_collision(self, turtle1, turtle2):
    return abs(turtle1.xcor() - turtle2.xcor()) < 50 and \
           abs(turtle1.ycor() - turtle2.ycor()) < 50


Generating a bytearray by choosing an exclusive set of parameters I am new to python so tips to any part of the code are welcome.

My main problem is how to "overload" the function properly. Is this how supposed to be done with parameters that gets passed exculsivly?
It is intended that only Bytes, Words or a DWord will get passed in the corresponding parameter. (If multiple are passed the longest one is prefered. Sidenote: The naming is based on an old 16bit processor thats why word is 16 bit and Dword is 32)

I also wanted to make the range check of the optional arguments in the beginning but as I default them to None,  I had to check for None in the first place before I can check the range and this would make the None check redundant.

I had something like max(Word1,Word2) >= 2**16 in mind, because I hoped in python the max would yield None and the larger would give false, but > is not implemented for None. So to get it to work I used the check for None first. But this seems not elegant.

Afterwards I assign the bigger datachunks to Bytes and raise Errors if the data didn't fit in the range of the type. If nothing is passed the Data is set to 0 to fill up the array in the end.

If no length  was given the length calculated by the checks is used.

In the end all the data gets packed into a bytearray (the Message to be sent later). Because bytearray() checks if the elements are in range, the Bytes are not checked seperatly.

Is there a more pythonic way to do it?

def packMessage(DeviceID, Offset, dataLength=None,
                Byte0=None, Byte1=None, Byte2=None, Byte3=None,
                Word0=None, Word1=None, DWord0=None):

    Bytes = [Byte0, Byte1, Byte2, Byte3]
    length = 0
    # check if the values to be used are in range
    if Offset >= 2**16:
        raise ValueError
    else:
        Offset_low = Offset & 0x00FF
        Offset_high = (Offset & 0xFF00) >> 8

    if dataLength is not None and not 1 <= dataLength <= 4:
        raise ValueError

    # split bigger data to bytes and check the data length
    if DWord0 is not None:
        if 0 <= DWord0 <= 2**32:
            Bytes[0] = DWord0 & 0xFF
            Bytes[1] = (DWord0 & 0xFF00) >> 8
            Bytes[2] = (DWord0 & 0xFF0000) >> 16
            Bytes[3] = (DWord0 & 0xFF000000) >> 24
            length = 4
        else:
            raise ValueError
    elif Word0 is not None:
        if 0 <= Word0 <= 2**16:
            Bytes[0] = Word0 & 0xFF
            Bytes[1] = (Word0 & 0xFF00) >> 8
        else:
            raise ValueError
        if Word1 is not None:
            if 0 <= Word1 <= 2**16:
                Bytes[2] = Word1 & 0xFF
                Bytes[3] = (Word1 & 0xFF00) >> 8
                length = 4
            else:
                raise ValueError
        else:
            Word1 = 0
            Bytes[2] = Bytes[3] = 0
            length = 2
    else:
        for i in range(len(Bytes)):
            if Bytes[i] is not None:
                length = i
            else:
                Bytes[i] = 0

    # if no length given take calculated length
    if dataLength is None:
        dataLength = length

    # return the Message
    return bytearray([DeviceID, dataLength, Offset_low,
                      Offset_high, Bytes[0], Bytes[1], Bytes[2], Bytes[3]])

Side note: I decided that the compare order of the Bytes doesn't matter that much so I ditched it for now and replaced it with a loop for now.

It looked like this before:

if Byte0 is not None:
    if Byte1 is not None:
        if Byte2 is not None:
            if Byte3 is not None:
                length = 4
            else:
                Byte3 = 0
        else:
            length = 3
            Byte2 = 0
            Byte3 = 0
    else:
        length = 2
        Byte1 = 0
        Byte2 = 0
        Byte3 = 0
else:
    length = 1
    Byte0 = 0
    Byte1 = 0
    Byte2 = 0
    Byte3 = 0

I don't see a nice way to wrap this up. So If there is a beautiful solution for this let me know, otherwise feel free to ignore it.

Simplest would be to separate parsing of the bytes, words and dwords to their own respective method, this would simplify the main logic a lot

def pack_message2(DeviceID, Offset, dataLength=None,
                Byte0=None, Byte1=None, Byte2=None, Byte3=None,
                Word0=None, Word1=None, DWord0=None):

    Offset_low, Offset_high = parse_offset(Offset)

    if dataLength is not None and not 1 <= dataLength <= 4:
        raise ValueError('dataLength should be between 1 and 4')

    result = parse_dword(DWord0)
    if result == ():
#         print('no dwords')
        result = tuple(parse_words(Word0, Word1))
        if result == ():
#             print('no words')
            result = parse_bytes(Byte0, Byte1, Byte2, Byte3)

    if dataLength is None:
        dataLength = len(result)

    result = pad_tuple_to(result, 4, 0) # if result should be padded with 0's'
#     print(result, dataLength)
    return bytearray([DeviceID, dataLength, Offset_low, Offset_high, *result])

Even if you wouldn't know what the implementation of each of these methods were, the code would be clear in what happens

offset

parsing the offset is pretty straightforward

def parse_offset(Offset):
    if Offset >= 2**16:
        raise ValueError('Offset should be less than 2**16')
    return (Offset & 0x00FF), ((Offset & 0xFF00) >> 8)

Data

Instead of manually chaining the splitting the words in bytes, you can use that generator expression. This will also allow you to easily parse triple words or even quads without letting the code explode

def parse_dword(dword):
#     print('dword:', dword)
    if dword is None:
        return ()
    if not 0 <= dword <= 2**32:
        raise ValueError('dword should be between 0 and 2**32')
    return tuple((dword & 0xFF * (2**(8*i))) >> (8 * i) for i in range(4))

def parse_words(*words):
#     print('words: ', words)
    for word in words:
        if word is None:
#             yield from (0, 0)
            break
        if not 0 <= word  <= 2**16:
            raise ValueError('word should be between 0 and 2**32')
        yield from ((word & 0xFF * 2 ** (8*i)) >> (8 * i) for i in range(2))

from itertools import takewhile
def parse_bytes(*bytes_):
#     print('bytes: ', bytes_)
    return tuple(takewhile(lambda x: x is not None, bytes_))

These methods can be further generalized, but I'll leave that open as an exercise

padding the result

the simplest (but not purest) way to pad a tuple

def pad_tuple_to(tup, to, default=None):
    return tup + tuple(default for _ in range(to - len(tup)))

generalization

I was intrigued by the generalization, so I tried it myself

def generalized_parse_word(size, words):
#     print('words: ', words)
    if words is None:
        return 'empty'
    for word in words:
#         print('word: ', word)
        if word is None:
#             yield from (0, 0)
            break
        if not 0 <= word  <= 2 ** (8 * size):
            raise ValueError('word with %i should be between 0 and %i' % (size, 2 **(8*size)))
        yield from ((word & 0xFF << (8*i)) >> (8 * i) for i in range(size))

def pack_message3(DeviceID, Offset, dataLength=None,
                Byte0=None, Byte1=None, Byte2=None, Byte3=None,
                Word0=None, Word1=None, DWord0=None):

    Offset_low, Offset_high = parse_offset(Offset)

    if dataLength is not None and not 1 <= dataLength <= 4:
        raise ValueError('dataLength should be between 1 and 4')

    data = (
        (4, (DWord0,)),
        (2, (Word0, Word1)),
        (1, (Byte0, Byte1, Byte2, Byte3)),
    )

    for size, words in data:
        result = tuple(generalized_parse_word(size, words))
        if result != ():
            break

    if dataLength is None:
        dataLength = len(result)

    result = pad_tuple_to(result, 4, 0) # if result should be padded with 0's'
#     print(result, dataLength)
    return bytearray([DeviceID, dataLength, Offset_low, Offset_high, *result])

arguments

Instead of manually naming the different bytes or words, I would accept a collection as argument, something like this:

def pack_message4(DeviceID, Offset, dataLength=None,
                dwords=None, words=None, bytes_=None):
    ...
    data = ((4, dwords), (2, words), (1, bytes))
    ...

testing

inputs = [
    {'Word0': 0x1122, 'Word1': 0x3344},
    {'DWord0': 0x11223344},
    {'Byte0':34, 'Byte1':17, 'Byte2':68, 'Byte3': 51},
]
for i in inputs:
    ans0 = packMessage(DeviceID=3, Offset=220, **i)
    ans1 = pack_message3(DeviceID=3, Offset=220, **i)
    print(ans0 == ans1, ans0, ans1)

True bytearray(b'\x03\x04\xdc\x00"\x11D3') bytearray(b'\x03\x04\xdc\x00"\x11D3')
True bytearray(b'\x03\x04\xdc\x00D3"\x11') bytearray(b'\x03\x04\xdc\x00D3"\x11')
False bytearray(b'\x03\x03\xdc\x00"\x11D3') bytearray(b'\x03\x04\xdc\x00"\x11D3')

Could it be your method to calculate the dataLength in the case of Bytes is off by 1


A Merge Sort implementation for efficiency This is my version of mergesort.
# mergesort

def merge(items, low, mid, high):
    left = [items[i] for i in range(low, mid + 1)]
    right = [items[i] for i in range(mid + 1, high + 1)]
    left.append(4444444444444444444444)
    right.append(4444444444444444444444)

    i = 0
    j = 0
    for k in range(low, high + 1):
        if left[i] < right[j]:
            items[k] = left[i]
            i += 1
        else:
            items[k] = right[j]
            j += 1

def mergesort(items,low, high):
    mid = int(low + (high - low) / 2)
    if low < high:
        mergesort(items, low, mid)
        mergesort(items, mid + 1, high)
        merge(items, low, mid, high)

if __name__ == "__main__":
    a = [6, 5, 4, 3, 2, 1]
    print(mergesort(a, 0, len(a) - 1))

I ran a test with input size of 1000000 in reverse order with a step of 1 i.e range(int(1e6), 1, -1) or range(1000000, 1, -1)  and got the following results
6999991 function calls (4999995 primitive calls) in 9.925 seconds

   Ordered by: internal time
   List reduced from 9 to 8 due to restriction <8>

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
   999998    6.446    0.000    8.190    0.000 test.py:3(merge)
1999997/1    1.735    0.000    9.925    9.925 test.py:19(mergesort)
   999998    0.821    0.000    0.821    0.000 test.py:4(<listcomp>)
   999998    0.740    0.000    0.740    0.000 test.py:5(<listcomp>)
  1999996    0.183    0.000    0.183    0.000 {method 'append' of 'list' objects}
        1    0.000    0.000    9.925    9.925 {built-in method builtins.exec}
        1    0.000    0.000    9.925    9.925 <string>:1(<module>)
        1    0.000    0.000    0.000    0.000 {built-in method builtins.len}

I would love to optimize the code and if possible, eliminate the copies.

Some comments:

Include a convenience function that takes only a list, i.e., where you don't need to provide the bounds.
Don't fight Python's inclusive-start exclusive-stop paradigm and naming.
Write docstrings to explain what the parameters mean, especially if you use unusual stuff.
Don't compute mid when you don't have to anyway.
Compute mid with integer division and from the sum (Python's ints don't overflow).
Don't reimplement slicing.
You can easily do it with copying out just the left part.
For benchmarking, I'd suggest to randomly shuffle the list instead of using a reverse-sorted list. I suspect you did the latter because you thought it's a worst case, but for example for Python's built-in sorting algorithm Timsort, it's a "best case", taking only linear time. And other algorithms might also be unrealistically affected (either faster or slower than for the typical case).
Don't write an unstable merge sort, that's a sin :-P
4444444444444444444444 is unsafe. What if the list has items larger than that?

Here's my version of it, it's about three times as fast as yours both on reverse-sorted and on shuffled lists (tested with 100,000 items).
def merge(items, start, mid, stop):
    """Merge sorted items[start:mid] and items[mid:stop] into items[start:stop]."""
    for left in items[start:mid]:
        while mid < stop and items[mid] < left:
            items[start] = items[mid]
            start += 1
            mid += 1
        items[start] = left
        start += 1

def mergesort_slice(items, start, stop):
    """Sort items[start:stop]."""
    if stop - start > 1:
        mid = (start + stop) // 2
        mergesort_slice(items, start, mid)
        mergesort_slice(items, mid, stop)
        merge(items, start, mid, stop)

def mergesort(items):
    mergesort_slice(items, 0, len(items))


Request dispatcher for PHP JSON web service Below you can find the constructor method and related fields of the main class of my REST API (note that the class contains more code than displayed) written in PHP 7.1. It does various things with the HTTP-request. It starts with getting the arguments and then it gets the method in which the request is made (like POST or GET). The request gets trimmed of tags and that gets saved back into the instance/member variable $this->request.

Now, to my best knowledge, $this->request in PHP always points to the HTTP-request when a HTTP-request has just been made. The code can only get here when a HTTP-request has been made and $this->method has been filled ($this->method = $_SERVER['REQUEST_METHOD'];).

In other words, $this->method should always exist, but I keep getting the 'Field declared dynamically' warning:

Is it okay to just ignore the warning or is there a best practice to deal with it?

Other tips to improve my code are welcome too. I've 'learned' PHP  back in version 4 (I used it for uni once). I struggle with the OO concepts since introduced in a language where so much is allowed.

class BaseAPI {

    protected $method = '';
    protected $endpoint = '';
    protected $verb = '';
    protected $args = Array();

    public function __construct($request)
    {
        header("Access-Control-Allow-Orgin: *");
        header("Access-Control-Allow-Methods: *");
        header("Content-Type: application/json");

        $this->args = explode('/', rtrim($request, '/'));
        $this->endpoint = array_shift($this->args);
        if (array_key_exists(0, $this->args) && !is_numeric($this->args[0])) {
            $this->verb = array_shift($this->args);
        }

        $this->method = $_SERVER['REQUEST_METHOD'];
        if ($this->method == 'POST' && array_key_exists('HTTP_X_HTTP_METHOD', $_SERVER)) {
            if ($_SERVER['HTTP_X_HTTP_METHOD'] == 'DELETE') {
                $this->method = 'DELETE';
            } else if ($_SERVER['HTTP_X_HTTP_METHOD'] == 'PUT') {
                $this->method = 'PUT';
            } else {
                throw new IncorrectHeaderException("Unexpected Header");
            }
        }

        switch ($this->method) {
            case 'DELETE':
            case 'PUT':
                // don't do anything yet
            case 'POST':
                $this->request = $this->_cleanInput($_POST);
                break;
            case 'GET':
                $this->request = $this->_cleanInput($_GET);
                break;
            default:
                throw new IncorrectMethodException('Invalid Method');
                break;
        }
    }

    private function _cleanInput($data)
    {
        $cleaned_input = Array();
        if (is_array($data)) {
            foreach ($data as $k => $v) {
                $cleaned_input[$k] = $this->_cleanInput($v);
            }
        } else {
            $cleaned_input = trim(strip_tags($data));
        }
        return $cleaned_input;
    }
}

Now, to my best knowledge, $this->request in PHP always points to the HTTP-request when a HTTP-request has just been made

My guess is that you are thinking of the super-global $_REQUEST? That is not the same as $this->request (unless somehow that instance variable gets set to something else)...

  Is it okay to just ignore the warning or is there a best practice to deal with it?

Declare the property, just as the other instance properties are declared (i.e. $method, $endpoint, $verb, etc.):

class BaseAPI {

    protected $request;

That way the IDE will know that the property should at least exist for each instance (though it might not be assigned a value until that switch statement).

Other Feedback

Switch statement cases:

switch ($this->method) {
        case 'DELETE':
        case 'PUT':
            // don't do anything yet

Those first cases are missing a break; so the PHP pre-processor will fall-through to the POST case.

header() calls in constructor

The three calls to header() in the constructor feels a little strange but then again it is difficult to know what the rest of your code looks like. Typically those would go in a method to render/send output.


FizzBuzz Optimisation | Modulus vs Decrement Operator Performance This is not the typical debate in FizzBuzz code about how to handle repeated if statement. I believe that it is a matter of personal preference.
I am here to ask about the choice of repeated mathematical operation.
Usually the solution to a FizzBuzz code is to use the modulus operator on every iteration. However, I'm concerned with its performance impact.
From my knowledge, modulus operator is tightly connected with division operator, and has a considerable performance overhead compared to simple increment and decrement operator.
In addition, I don't believe compiler has the ability to analyse and optimise the use of modulus operator in a loop.
Hence, I give the alternative solution that uses only simple decrement operator. The following code are written in C++.
I seek insight on whether my approach will provide performance benefit, or if it is actually a horrible idea that worsen performance.
And if there are other ways I can optimize a FizzBuzz code even further. (I think in a modified FizzBuzz puzzle with lots of cases to compare against, a lookup table might be preferable)

Typical FizzBuzz solution.
for (unsigned int i = 1; i <= 1000000; ++i) {
    if (!(i % 15))
        std::cout << "FizzBuzz\n";
    else if (!(i % 3))
        std::cout << "Fizz\n";
    else if (!(i % 5))
        std::cout << "Buzz\n";
    else
        std::cout << i << '\n';
}

My optimized code.
static const unsigned int fa {3};
static const unsigned int fb {5};

unsigned int a {fa};
unsigned int b {fb};

for (unsigned int i = 1; i <= 1000000; ++i) {
    --a, --b;
    if (!a && !b) {
        a = fa;
        b = fb;
        std::cout << "FizzBuzz\n";
    } else if (!a) {
        a = fa;
        std::cout << "Fizz\n";
    } else if (!b) {
        b = fb;
        std::cout << "Buzz\n";
    } else {
        std::cout << i << '\n';
    }
}
```

Looks good - you're using only simple integer addition and subtraction.
Except for the division hidden here:
    std::cout << i << '\n';

Note that division by a constant isn't necessarily as expensive as you think: it's always possible for an optimising compiler to implement it in terms of multiplication and bitwise operations.  And a test for being an exact multiple is often simpler (think of the well-known algorithms for testing divisibility by 9 or 11 in decimal).
If you really want to drive up performance, consider storing the integer value as a string (any kind), and implement the ++ operation character by character.
You may also be interested in High throughput Fizz Buzz over on the Programming Puzzles site.


Count numbers whose digits are only increasing or decreasing The goal is described here: basically, it is to compute the number of numbers below \$10^n\$ such that its digits are either increasing or decreasing (like 112 or 211, but not like 121).

The original version:

-- f computes the number of numbers with increasing digits.
-- digits >= k and the number of digits is n

f :: (Num t1, Num t, Eq t1, Eq t, Enum t) => t -> t1 -> t
f k 0 = 1
f k 1 = 10 - k
f 9 n = 1
f k n = foldl (\sum i -> sum + f i (n-1)) 0 $ enumFromTo k 9

-- h computes the number of numbers with decreasing digits.
-- digits <= k and the number of digits is n

h :: (Num t1, Num t, Eq t1, Eq t, Enum t) => t -> t1 -> t
h k 0 = 1
h 0 n = 1
h k 1 = k + 1
h k n = foldl (\sum i -> sum + h i (n-1)) 0 $ enumFromTo 0 k

-- The main function is g

g :: (Num t1, Num t, Eq t1, Eq t, Enum t) => t1 -> t
g 0 = 1
g 1 = 10
g n = g (n-1) + foldl (\sum i -> sum + g1 i (n-1)) 0 (enumFromTo 1 9)

-- g1 computes the number of numbers with either increasing or decreasing digits.

g1 :: (Num t1, Num t, Eq t1, Eq t, Enum t) => t -> t1 -> t
g1 0 n = f 0 n -- only increasing
g1 9 n = h 9 n -- only decreasing
g1 k 0 = 1     -- necessary for recursion
g1 k n = g1 k (n-1) + s1 + s2
  where
  s1 = foldl (\sum i -> sum + f i (n-1)) 0 $ enumFromTo (k+1) 9
  s2 = foldl (\sum i -> sum + h i (n-1)) 0 $ enumFromTo 0 (k-1)

The problem is it is too slow, and basically g n needs two times of the time needed by g (n-1).

I think maybe I need to memorize the functions, but don't know how to achieve this. Perhaps a list of lists?

The version using a list of lists follows:

total n  = case n of
  0 -> 1
  _ -> foldl (\sum k -> (sum + incg k n + decg k n) - 1) 0 [1..9] + total (n - 1)

incg k n = incgs !! n !! k
decg k n = decgs !! n !! k

incgs :: [[Integer]]
incgs =  [] : take 10 (repeat 1) : rest
  where
  rest = map (reverse . scanl1 (+) . reverse) $ tail incgs

decgs :: [[Integer]]
decgs =  [] : take 10 (repeat 1) : rest
  where
  rest = map (scanl1 (+)) $ tail decgs

Here's a version of your list of lists version using list comprehension and no !! and no explicit recursion:

total 0 = 1
total n = sum
  [ sum (init decgn) + sum (tail decgn) - 9
  | decgn <- take n $ iterate (scanl1 (+)) $ replicate 10 1
  ] + 1

This can probably be reduced further because either of us introduced some unneeded numerical operation somewhere.


Bills dispenser machine I have just started out learning C++ and I am trying to write out a simple bills' dispenser machine as an exercise.

Upon user input, the system is to check if it can pay out the amount but only in 20 and 50 bills, and it should return with the minimum number of bills.

I need someone to point out and tell me if the following code I have wrote if it is viable (it seems to be working when I tested it myself, but that is too one sided). What about the structure?

There are 2 parts that I did - the first code simply by extending the main function while the latter is done by introducing the functions.

#include <iostream>
using namespace std;

int main()
{
    int inputAmt;

    cout << "This machine only dispense in $20 and $50 bills only" << endl;
    cout << "Enter in the amount to be withdraw : " << endl;
    cin >> inputAmt;

    // Decides the number of $50 bills to be dispense
    int fiftyBill;
    fiftyBill = inputAmt/50;

    int remainderAmt;
    remainderAmt = inputAmt - (fiftyBill*50);

    // Test the Remainder Amount if it is even
    int remE;
    remE = remainderAmt % 20;

    if (remE >= 10 || remE <= 20 )
    {
        // If it is not, reduced $50 by 1 and add it to the remainder amount
        fiftyBill = fiftyBill - 1;
        remainderAmt = remainderAmt + 50;
    }

    // Decides the number of $20 bills to be dispense
    int twentyBill;
    twentyBill = remainderAmt / 20;

    // Output Message
    cout << "Number of $50 bills : " << fiftyBill << endl;
    cout << "Number of $20 bills : " << twentyBill << endl << endl;

    cout << "Number of bills dispensed is " << fiftyBill + twentyBill << endl;

    int leftoverAmt;
    leftoverAmt = inputAmt - (fiftyBill * 50) - (twentyBill * 20);
    if (leftoverAmt != 0)
    {
        cout << "There are a leftover of $" << leftoverAmt << " that cannot be dispensed." << endl;
    }
}

Lastly, I tried to integrate 2 functions bool dispense (int amt) (returns true if the amount indicated can be dispensed in combinations of 20 and 50 bills, false otherwise) and int dispenseFif (int amt) (returns the number of 50 bills to dispense that will results in least number of notes being dispensed).

#include <iostream>
using namespace std;

bool canDispense (unsigned int amount);
int dispense50s (unsigned int amount);

int main()
{
    int inputAmt;
    cout << "This machine only dispense in $20 and $50 bills only" << endl;
    cout << "Enter in the amount to be withdraw : " << endl;
    cin >> inputAmt;

    canDispense(inputAmt);
}

bool canDispense (unsigned int amount)
{
    int fifAmt = dispense50s(amount);
    int remainderAmt;
    remainderAmt = amount - (fifAmt*50);

    // Test the Remainder Amount if it is even
    int remE;
    remE = remainderAmt % 20;
    cout << "remE : " << remE << endl << endl;

    if (remE >= 10 || remE >= 20 )
    {
        fifAmt = fifAmt - 1;
        remainderAmt = remainderAmt + 50;
    }

    // Decides the number of $20 bills to be dispense
    int twentyBill;
    twentyBill = remainderAmt / 20;

    int leftoverAmt;
    leftoverAmt = amount - (fifAmt * 50) - (twentyBill * 20);

    if (leftoverAmt == 0)
        {
            // Output Message
            cout << "(2) Number of $50 bills : " << fifAmt << endl;
            cout << "(2) Number of $20 bills : " << twentyBill << endl;

            cout << "(2) Number of bills dispensed is " << fifAmt + twentyBill << endl;
            return true;
        }
    else
        {
        cout << "The amount is impossible! Please try again... " << endl;
        return false;
        }

}

int dispense50s (unsigned int amount)
{
    // Decides the number of $50 bills to be dispense
    int fiftyBill;
    fiftyBill = amount/50;
    return fiftyBill;
}

This is a tricky problem to solve.  In the general case, the change-making-problem requires dynamic-programming to solve.  It is possible to write a simpler algorithm that works for $20 and $50 only, but yours is buggy:

This machine only dispense in $20 and $50 bills only
Enter in the amount to be withdraw :
30
remE : 10

(2) Number of $50 bills : -1
(2) Number of $20 bills : 4
(2) Number of bills dispensed is 3

Furthermore, if (remE >= 10 || remE >= 20 ) in the second solution looks wrong.

If I had to write a quick-and-dirty non-generalized solution for $50 and $20, I would write it this way:

#include <cassert>
#include <iostream>
#include <iomanip>

/**
 * Composes the given amount as a number of 50-dollar bills and 20-dollar
 * bills (as out-parameters).  Returns the total number of bills needed, or -1
 * if the amount cannot be formed.
 */
int split_50_20_bills(int amount, int *num_50, int *num_20) {
    if (amount < 0 || amount == 10 || amount == 30 || amount % 10) {
        return -1;
    } else {
        *num_50 = (amount % 20) ? ((amount - 50) / 100) * 2 + 1 :
                                  (amount / 100) * 2;
        *num_20 = (amount - 50 * *num_50) / 20;
        assert(amount == 50 * *num_50 + 20 * *num_20);
        return *num_50 + *num_20;
    }
}

int main()
{
    for (int amt = 0; amt <= 1000; amt += 10) {
        int num_50, num_20;
        int total = split_50_20_bills(amt, &num_50, &num_20);
        std::cout << std::setw(4) << amt;
        if (total < 0) {
            std::cout << " cannot be formed\n";
        } else {
            std::cout << " = " << std::setw(2) << num_50 << " x $50 + "
                               << std::setw(2) << num_20 << " x $20 ("
                               << std::setw(2) << total << " bills)\n";
        }
    }
}

Note that one function passes back four pieces of information:

Whether it's possible to form the amount
If so, how many bills it will take altogether
How many $50 bills
How many $20 bills


Toggle Function in JavaScript Here is my current toggle function:

$scope.toggleSelect = event => {
  $scope.selection = event.target.innerHTML;

  if ($scope.selection === 'Select All') {
     $scope.emailList = $scope.users.filter(user => !user.report.emailed);
  } else {
     $scope.emailList = $scope.emailList = [];
  }
};

Here are my thoughts and questions about the best practices:

I make an assignment to $scope.emailList in the if and else statement. Instead of separate assignments, is one assignment better? Refactoring to one assignment will pollute the code with unnecessary variables, but it may make the code more readable?

$scope.toggleSelect = event => {
  $scope.selection = event.target.innerHTML;
  const emailListUpdate;
  if ($scope.selection === 'Select All') {
    emailListUpdate = $scope.users.filter(user => !user.report.emailed);
  } else {
    emailListUpdate = $scope.emailList = [];
  }
   $scope.emailList = emailListUpdate;
};

The main benefit I see here is that anybody reading the code can skim to the last line and know the overall purpose. Overall, I don't see this to be a beneficial refactor since I don't think it adds additional readability and potentially makes it harder to follow. I would appreciate thoughts on this.
Ternary or if/else:

I reviewed a great post about the benefits and use cases of using ternary or if/else. Here is  what the code would look like refactored to a ternary:

$scope.toggleSelect = event => {
  $scope.selection = event.target.innerHTML;
  $scope.emailList = $scope.selection === 'Select All' ? $scope.users.filter(user => !user.report.emailed); : [];

};

Quoting from the article linked above:

  An if/else statement emphasizes the branching first and what's to be done is secondary, while a ternary operator emphasizes what's to be done over the selection of the values to do it with.

I feel the if/else feels more natural, but I'm not sure if that is just related to my lack of experience using ternary.
I have another toggleItem function used when a checkbox is clicked.

$scope.toggleItem = (event, user) => {
  if (event.target.checked) {
    $scope.emailList.push(user);
  } else {
    $scope.emailList = _.reject($scope.emailList, item => item._id === user._id);
  }

I've thought about combining both of my toggle functions but overall I think it is better to keep them separate instead of trying to make a generic toggle function with multiple use cases.

In the toggleItem function above, I think it is cleaner (and more obvious) to use an if/else instead of a ternary since I am only making an assignment in the else statement.

I'm trying to improve on writing cleaner code, If anyone has any input or thoughts on best practices to use here or how to clean up this code it would be appreciated.

Short and sweet is best.
Your statement

"The main benefit I see here is that anybody reading the code can skim to the last line and know the overall purpose"

That makes no sense at all, you say all that is needed to understand the functions is
$scope.emailList = emailListUpdate;

Nobody jumping into someone else code will just skim, the only people that skim code are those that know the code.
You can make a few assumptions.

All that read your code are competent coders.
All that read your code have read the project specs.
Every line of code will be read by a coder new to the code.

Example
The best code is brief as possible without being a code golf entrant.
Notes

Why innerHTML, should it not be textContent????
This function is not a toggle. It is based on selection value.
The ternary expression is too long, break the line so it does need to be scrolled
The ternary has a syntax error. Misplaced ;
the ; on the last line after "}" is redundant.

Code, best option.
$scope.selectEmailUsers = event => {
  $scope.selection = event.target.textContent;
  $scope.emailList = $scope.selection === "Select All" ?
       $scope.users.filter(user => !user.report.emailed); : [];
  //                                                    ^ remove syntax error
}; // << remove the ;