Updating a chart from Yahoo Finance CSV files I want to load some CSV columns or contents in my Ajax code. Please help me or improve my code. This will be live API code for me.

Data will be downloaded from Yahoo Finance in CSV format on an interval, and updated into this chart code. The values will be updated and chart will continue to be updated.

<?php $row = 1; if(($handle = fopen("table.csv", "r")) !== FALSE)  {
while (($data = fgetcsv($handle, 1000, ",")) !== FALSE){
$row++;
for ($c=0; $c<1; $c++)
{
echo $data[$c];
}
for ($c1=5; $c1<6; $c1++)
{
$data[$c];
}
for ($c2=4; $c2<5; $c2++)
{
$data[$c];
}
fclose($handle);
}
?>

var chart;
var chartData = [

{   year: '(here is the data from csv "col 1 row 1")',  income: '(here is the data from csv "col 5 row 1")',     expenses: '(here is the data from csv "col 4 row 1")'},{ year: '(here is the data from csv "col 1 row 2")',    income: '(here is the data from csv "col 5 row 2")',     expenses: '(here is the data from csv "col 4 row 2")'},{year: '(here is the data from csv "col 1 row 3")',    income: '(here is the data from csv "col 5 row 3")',expenses: '(here is the data from csv "col 3 row 3")'}];

AmCharts.ready(function () {
        // SERIAL CHART
        chart = new AmCharts.AmSerialChart();
        chart.pathToImages = "../amcharts/images/";
        chart.dataProvider = chartData;
        chart.categoryField = "year";
        chart.startDuration = 1;

        // AXES
        // category
        var categoryAxis = chart.categoryAxis;
        categoryAxis.gridPosition = "start";

        // value
        // in case you don't want to change default settings of value axis,
        // you don't need to create it, as one value axis is created automatically.

        // GRAPHS
        // column graph
        var graph1 = new AmCharts.AmGraph();
        graph1.type = "column";
        graph1.title = "Income";
        graph1.valueField = "income";
        graph1.lineAlpha = 0;
        graph1.fillAlphas = 1;
        chart.addGraph(graph1);

        // line
        var graph2 = new AmCharts.AmGraph();
        graph2.type = "line";
        graph2.title = "Expenses";
        graph2.valueField = "expenses";
        graph2.lineThickness = 2;

        chart.addGraph(graph2);

        // LEGEND

        // WRITE
        chart.write("chartdiv");
    });
</script>

Did you read or even try this code before posting it? Sorry if this seems rude, but it just seems obvious that if this was run you would immediately notice something was wrong. I'm mostly looking at the PHP code here, as it is full of issues. The JS appears to be fine, except I would consider moving those graphs into a separate functions. More on that below.

PHP

Wow, so many things on just the first line.

Only do one thing per line, this helps with legibility. Also, try to avoid putting statements on the same line as the opening or closing php tags, unless opening and closing them on the same line (templates usually).

Don't assign variables in statements (if, while, etc...) as it can lead to mistakes. PHP allows this, but IMO they should deprecate it. Though they probably won't because it might cause issues with the way for loops are declared. It is very easy to forget a second equals sign and mistakenly assign a new value to a variable that you were actually trying to compare, and your IDE wont catch it because its valid. If you get in the habit of not doing this then you know that if you ever see it while troubleshooting you know you've found your issue. And if your IDE is smart enough it will warn you about accidental assignments.

Double quotes are used to tell PHP that the following string has a character or variable that needs to be escaped. As such PHP requires a little bit of extra processing power to read that string than if it were just single quoted. The speed is really negligible and is not really a "good" reason for making this switch, but I like to point it out anyways. This is a very controversial issue, so take it or leave it.

The first line rewritten:

<?php
$row = 1;
$handle = fopen( 'table.csv', 'r' );
if( $handle !== FALSE )  {

Correct me if I'm wrong, but doesn't your for loop just print once on each while iteration? In this case, why loop at all? The same thing can be accomplished with array_shift() or by specifying the first index.

echo array_shift( $data );
//or to avoid manipulating array
echo $data[ 0 ];

Why are all of your for loops only looping once? What is their purpose? Let me read you your code and you can follow along. Starting inside the while loop and doing the following on each iteration.

Increment unused row variable
Loop once

Echo first element of new CSV array
End of loop, increment a counter that won't really be used

Loop once with a specific number that's not used

Use first counter to point at the second value of array, Do nothing with it
End of loop, increment a counter that isn't used

Loop once with another specific number that's not used

Use first counter to point at the second value of array, Do nothing with it
End of loop, increment a counter that isn't used

Close file early so that only first entry was retrieved and while loop ends early

I'm not sure what those other loops are being used for, but If you want to do something every 5th iteration just add an if statement with a mod of 5.

if( $c % 5 === 0 ) {

What is the purpose of $row, $c1, and $c2? These variables are unused. And What is the purpose of just declaring $data[ $c ] in each of these loops, it doesn't do anything. At least the first loop echoed it.

Why are you closing your handle while you are still using the file? Close it after the loop unless you meant to only fetch the first entry.

JS

You can reduce the amount of work you are doing by following the "Don't Repeat Yourself" (DRY) Principle. You should also look at the Single Responsibility Principle to break up that ready function a little more. Now, my JS is a little rusty, but here's how I'd apply the DRY principle. I have not tested it, but hopefully it gets the point across.

function graph( properties ) {
    graph = new AmCharts.AmGraph();
    for( property in properties ) {
        graph[ property ] = properties[ property ];
    }

    return graph;
}

graph1Properties = {
    type : "column",
    title : "Income",
    valueField : "income",
    lineAlpha : 0,
    fillAlphas : 1
};
graph1 = graph( graph1Properties );
chart.addGraph( graph1 );

graph2Properties = {
    type : "line",
    title : "Expenses",
    valueField : "expenses",
    lineThickness : 2
};
graph2 = graph( graph2Properties );
chart.addGraph( graph2 );


Scraping table contents from a webpage using vba with selenium My script is able to harvest full contents of a table from a webpage with javascript encrypted using vba in combination with selenium. The table has got a drop-down option from where the full contents can be selected by hitting "all". The table has got 300 rows of data which spread 7 column across. There are around seven steps to traverse to reach the destination page. It takes a while to parse them all. Just run it, sit back and relax until the browser is closed. It works perfectly. I could not manage to create the script with explicit wait cause I doubt there is any option in vba. Here is the working code.

Sub Table_Data()

Dim driver As New WebDriver
Dim posts As Object, post As Object, t_data As Object

With driver
    .Start "chrome", "http://apps.tga.gov.au/Prod/devices"
    .get "/daen-entry.aspx"
    .FindElementById("disclaimer-accept").Click
    .Wait 3000
    .FindElementById("medicine-name").SendKeys ("pump")
    .Wait 10000
    .FindElementByClass("medicines-check-all").Click
    .Wait 3000
    .FindElementById("submit-button").Click
    .Wait 5000
    .FindElementById("ctl00_body_MedicineSummaryControl_cmbPageSelection").Click
    .Wait 5000
    .FindElementByXPath("//option[@value='all']").Click
    .Wait 5000
End With

For Each posts In driver.FindElementsByXPath("//table[contains(@class,'daen-report')]")
    For Each post In posts.FindElementsByXPath(".//tr")
        For Each t_data In post.FindElementsByXPath(".//td[@class='row-odd']|.//td")
        y = y + 1
        Cells(x, y) = t_data.Text
        Next t_data
        x = x + 1
        y = 0
    Next post
Next posts
End Sub

Good job once again!

I would eliminate one loop depth level - there is only a single table that you are interested it - iterate over its rows directly. And, I would switch to more concise CSS selectors to locate table rows and cells.

Also, I would rename post to a row and t_data to cell to correspond to what these variables represent:

For Each row In driver.FindElementsByCss("table#ctl00_body_MedicineSummaryControl_grdSummary tr")
    For Each cell In row.FindElementsByCss("td")
        y = y + 1
        Cells(x, y) = cell.Text
    Next cell

    x = x + 1
    y = 0
Next row


Simple GCF calculator app I am trying to learn some basic Python and am trying to find tricks to write speedy code:

import sys
if len(sys.argv) < 3:
    print("You left out the arguments")
    sys.exit()
gcf = 0

numbers = []
collections = []
unique = False

for i in range(1,len(sys.argv)):
    numbers.append(int(sys.argv[i]))
for x in range(0,len(numbers)):
    collections.append([])
    for s in range(1,numbers[x] + 1):
        if numbers[x] % s == 0:
            collections[x].append(s)
    print str(numbers[x]) + ": ",
    print ','.join(map(str,collections[x]))
for i in collections[0]:
        for x in collections:
            if i in x:
                unique = True
            else:
                unique = False
        if unique:
            gcf = i
print "gcf is: "+str(gcf)

You can use sets to whittle down the possible GCFs.

for i, argument in enumerate(sys.argv[1:]):
    factors = set(n for n in range(2, int(argument) + 1) if int(argument) & n is 0)
    print argument, factors
    if i is 0:
        possible_gcfs = factors
    else:
        possible_gcfs &= factors
print "gcf is:", max(possible_gcfs | {1})

Better, use reduce:

def get_factors(x):
    factors = set(n for n in range(2, int(x) + 1) if int(x) % n is 0)
    print x, ':', factors
    return factors

possible_gcfs = reduce(set.intersection, map(get_factors, sys.argv[1:]))
print "gcf is:", max(possible_gcfs | {1})

Or if you don't need it to print out the factors of each number you can make it a bit more efficient by noting that the GCF must be no larger than the lowest of the arguments entered.

arguments = map(int, sys.argv[1:])
possible_gcfs = range(2, min(arguments) + 1)
for argument in arguments:
    possible_gcfs = [n for n in possible_gcfs if argument % n is 0]
print "gcf is: ", max(possible_gcfs + [1])

... but this can again be simplified using reduce and noting that the gcf is associative

def gcf(a, b):
    a, b = int(a), int(b)
    return max(n for n in range(1, min(a, b) + 1) if a % n is 0 and b % n is 0)

print "gcf is:", reduce(gcf, sys.argv[1:])

(And as a comment pointed out there are plenty of other solutions posted to earlier questions and you could use fractions.gcd.)


Creating vector of primes using Sieve of Eratosthenes Please review my Sieve of Eratosthenes implementation.

std::vector<int> generate(const int& n) {
    std::vector<bool> input(n+1, true);
    for (int i = 2; i * i<= n; ++i) {
        for (int j = i; i*j <= n    ; ++j) {
            input[i*j] = false;
        }
    }
    std::vector<int> primes{};
    for (auto i = 1; i <= n; ++i) {
        if (input[i]) {
            primes.push_back(i);
        }
    }
    return primes;
}

How can I improve and make this faster?

From Wikipedia, the Sieve of Eratosthenes algorithm is:

  To find all the prime numbers less than or equal to a given integer n
  by Eratosthenes' method:

  Create a list of consecutive integers from 2 through n: (2, 3, 4,
  ..., n).
  Initially, let p equal 2, the first prime number.
  Starting from p, enumerate its multiples by counting to n in increments of p, and mark them in the list (these will be 2p, 3p,
  4p, etc.; the p itself should not be marked).
  Find the first number greater than p in the list that is not marked. If there was no such number, stop. Otherwise, let p now
  equal this new number (which is the next prime), and repeat from
  step 3.

Your algorithm does not seem right really, notably it skips the step 4 which is essential for fast operation; and there are some non-C++ idioms too

Do not return the std::vector; instead pass it as a reference to the function.
Do not pass integers as const references.
If n < 2, return empty vector.
The Eratosthenes sieve goes like: "for all values from 2 to sqrt(n) - if the number is a prime then mark all its multiples not prime. However all multiples less than i * i have been marked already composite by previous iterations.
Do not multiply the loop variables when a simple addition suffices.
As a simple optimization, the 2 is always in the primes as we skipped the "no values" already; thus we add it. Then we iterate only odd integers starting from 3.

Thus we get

std::vector<int> generate(int n) {
    std::vector<int> result = std::vector<int>();
    if (n < 2) {
        return result;
    }

    // initialize the vector
    std::vector<bool> input(n + 1, true);

    // calculate the upper limit as the square root of
    // of N. all composite numbers <= N must have a
    // factor <= sqrt(N)
    int sqrtN = (int)sqrt(n);

    // iterate from 2 up to the square root of N
    for (int i = 2; i <= sqrtN; i ++) {
        if (! input[i]) {
            // i is a proven composite number,
            // all its multiples have been
            // marked not prime by all its prime factors by now.
            continue;
        }

        // as an optimization, all multiples *less* than
        // the square of i are already marked, (they have
        // another prime factor less than i), so we can start
        // from the square which is the smallest composite
        // not yet marked
        for (int j = i * i; j <= n; j += i) {
            input[j] = false;
        }
    }

    // as n >= 2, then add 2 here
    result.push_back(2);

    // and check only odd numbers here,
    // no other even number can be set
    for (int i = 3; i <= n; i += 2) {
        if (input[i]) {
            result.push_back(i);
        }
    }

    return result;
}

Of course you do not need the second loop to just fill in the result vector; if the input[i] is true (the continue was not executed), the number is a prime and you can push_back it to the result vector there:

if (! input[i]) {
    // i is an already proven composite number,
    // all its multiples have been
    // marked not prime by all its prime factors by now.
    continue;
}

// i is a prime
result.push_back(i);


IntModulus long addition and subtraction One of the remarks in this answer to my question concerning IntModulus says

  What's more problematic is that you don't have matching argument symmetry for add and subtract. You should have long versions of those.

and the reason for this part missing is that I hoped to get something faster than the trivial

public int add(long x, long y) {
    return mod(mod(x) + mod(y));
}

The optimized solution has only one modulus operation (sure, only benchmarking will tell if it was worth it).

Please have a short look at the original question before reviewing. I mean the comments, not the code.

public final class IntModulus {
    ///////// Irrelevant code removed, see the original question if you miss anything.

    ///////// New code below.

    /** Return a non-negative value less than modulus and congruent to the exact sum. */
    public int add(long x, long y) {
        final long sum = x + y;
        // The addition overflowed iff both operands have the same sign are the result's sign differs.
        final boolean overflow = (x^y) >= 0 & (x^sum) < 0;
        return overflow ? overflowedMod(sum) : mod(sum);
    }

    /** Return a non-negative value less than modulus and congruent to the exact sum. */
    public int sub(long x, long y) {
        final long diff = x - y;
        // The subtraction overflowed iff the operands have opposing signs and the result's sign differs from the minuend.
        final boolean overflow = (x^y) < 0 & (x^diff) < 0;
        return overflow ? overflowedMod(diff) : mod(diff);
    }

    /**
     * Compute a modulus of the argument under the assumption that it overflowed by exactly {@code 2**64},
     * i.e., it's a result of an overflowing {@code long} addition or subtraction.
     *
     * <p>This means that the sign is surely wrong and that the number would fit into a 65-bit signed integer.
     */
    private int overflowedMod(long x) {
        // The proper value of the argument divided by two and floored (i.e., rounded towards negative infinity).
        // The sign bit got restored by simply flipping it as we know it was wrong.
        final long half = (x >> 1) ^ Long.MIN_VALUE;
        final long lsb = x & 1; // the least significant bit
        long result = 2L * mod(half) + lsb;
        // Now the result lies in the range 0 to 2*modulus+1 (both included), so conditionally reduce it twice.
        if (result>=modulus) result -= modulus;
        if (result>=modulus) result -= modulus;
        return (int) result;
    }

    ///////// Old but relevant code below.

    /** Return a non-negative value less than modulus and congruent to the operand. */
    public int mod(long x) {
        // As modulus is an int, the cast is safe.
        return fixModulus((int) (x % modulus));
    }

    /**
     * Fix the modulus, i.e., make it non-negative.
     * The argument must be between {@code -modulus+1} and  {@code modulus-1}.
     */
    private int fixModulus(int x) {
        return x<0 ? x+modulus : x;
    }

    @Getter private final int modulus;
}

The benchmark results shows that it's much faster than using BigInteger (suffix "bi"). It's assuming that the operands are already available in the needed form (i.e., they don't have to be converted to/from BigInteger).

Subtraction is not shown, as it's obviously as fast as addition. The code for multiplication is given in the old question.

Small improvements

Overall, it seems that what you wrote is correct and is an improvement over the mod(mod(x)+mod(y)) solution, especially in the case where there is no overflow.  I have three small improvements:

Add instead of multiply by 2

You can replace this line:

   long result = 2L * mod(half) + lsb;

with:

    long halfmod = mod(half);
    long result = halfmod + halfmod + lsb;

The modified version showed a 1% improvement in the overflow case.  (See benchmarks below)

Remove one unnecessary check

Following the code above, you have this code:

    // Now the result lies in the range 0 to 2*modulus+1 (both included),
    // so conditionally reduce it twice.
    if (result>=modulus) result -= modulus;
    if (result>=modulus) result -= modulus;

But actually, the value of mod(half) is in the range 0..modulus-1.  So if you multiply that by 2 and add 1, result will be in the range 0..2*modulus-1, not 0..2*modulus+1 as you stated in the comment.  Therefore, after the first result -= modulus, the range of result will be 0..modulus-1 and there is no need for the second check.

    // Now the result lies in the range 0 to 2*modulus-1 (both included),
    // so conditionally reduce it once.
    if (result>=modulus) result -= modulus;

By removing the second check, I found a 1% speed improvement in the overflow case.

Checking for overflow

I replaced this overflow check for add:

   final boolean overflow = (x^y) >= 0 & (x^sum) < 0;

with this very similar one:

    final boolean overflow = ((x^y^Long.MIN_VALUE) & (x^sum)) < 0;

Edit: Later I found that this inverted version is faster:

    final boolean overflow = ((x^y) | ~(x^sum)) >= 0;

It tests the same thing but uses one extra xor instead of a comparison.  For the overflow case I found a 2% speed improvement, and it was about 4% faster in the non-overflow case.

For subtraction I did a similar thing:

   final boolean overflow = (x^y) < 0 & (x^diff) < 0;

became:

    final boolean overflow = ((x^y) & (x^diff)) < 0;

Here I didn't even have to add an extra xor.

Benchmarks

I took the suggestion of using JMH to run benchmarks.  Here are the results:

Overflow add test
-----------------
Methodology: Add 0x4000000000000000L with itself modulo 0x12345678
             in a loop of 100000 iterations.

Method                   Operations per second     Speed
------                   ---------------------     -----
Original code              238.754 +- 0.778        100.0%
Multiply change            241.527 +- 0.884        101.1%
Remove extra check         241.180 +- 1.007        101.0%
Change overflow check      243.335 +- 1.662        101.9%
All changes                256.452 +- 1.325        107.4%

Non-overflow add test
---------------------
Methodology: Add 0x4000000000000000L with 1 modulo 0x12345678
             in a loop of 100000 iterations.

Method                   Operations per second     Speed
------                   ---------------------     -----
Original code              330.298 +- 1.635        100.0%
Change overflow check      342.496 +- 2.037        103.7%


Memory searcher function test This is part of the project I started just now. It is a function that returns the offset address of a first occurrence of a string match into a given range of memory blocks.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

typedef struct
{
    unsigned long offset;
    unsigned long terminal;
} Range;

unsigned long memgr_locate (char string[], Range iRange)
{
    unsigned char * ptr = (char*)iRange.offset;
    unsigned long i;
    unsigned long c = 0;

    for(i = iRange.offset; i <= iRange.terminal; i += 1)
    {
        ptr = (char*)i;

        (*ptr == string[c]) ? (c++) : (c=0);
        if(string[c + strlen(string) - 1] == '\0')
            break;
    }
    return i;
}

int main(void)
{
    char * test = malloc(100);
    strcpy(test, "Looking for Sample string directly in memory..");

    Range iRange = {test, test + 50};

    printf("%s", (char*)memgr_locate("Sample", iRange));

    return 0;
}

Test result:

  Sample string directly in memory..

Note that the string searcher function is constructed like so to return the closest result.

Example:

If we look for the string "s" it will return the offset byte of the first word from the memory range that starts with "s". If we look for "str" it will return the first word starting with "str". This behavior IS intended.

The compiler gives me several warnings:

st.c: In function 'main':
st.c:34:5: warning: initialization makes integer from pointer without a cast [enabled by default]
     Range iRange = {test, test + 50};
     ^
st.c:34:5: warning: (near initialization for 'iRange.offset') [enabled by default]
st.c:34:5: warning: initialization makes integer from pointer without a cast [enabled by default]
st.c:34:5: warning: (near initialization for 'iRange.terminal') [enabled by default]

Indeed, your program has many casts from unsigned long to char *.
You shouldn't ignore warnings, and strive to eliminate them all.

There seem to be several problems with this piece:

    if(string[c + strlen(string) - 1] == '\0')
        break;

First of all, calling strlen in every iteration of the loop is wasteful,
it would be better to call it once before and cache the length in a variable.

Secondly, I doubt this is intended behavior.
This statement will cause the loop to exit when c = 1.
So if your test string is "Looking for S Sample string" and the string parameter is "Sample", then the output will be "S Sample string" instead of "Sample string".
So this looks like a bug.
Perhaps you're looking for something like if (c == len) { i -= c - 1; break; } (where len is strlen(string), cached before the loop).

The variable names don't seem great.
Why not simply range instead of iRange?
How about target instead of string?
Why use Range at all instead of two variables?


Searching text for a string This is my current code:

def search(word,source):
    places = []
    for x in range(0,len(source)):
        if source[x] == word[0]:
            if source[x:x+len(word)] == word:
                places.append(x)
    return places

How would I optimize this without using regular expressions?

Using timeit, we can make changes and see how it affects the speed. Baseline:

>>> def search1(word,source):
    places = []
    for x in range(0,len(source)):
        if source[x] == word[0]:
            if source[x:x+len(word)] == word:
                places.append(x)
    return places

>>> timeit.timeit("search1(word, source)",
                  setup="from __main__ import search1, search2, search3; import re; word, source = 'foo', 'foobarbaz'*1000",
                  number=10000)
15.60436644058882

Note the following:

You calculate word[0] and len(word) on every loop, these can be factored out;
A list comprehension is generally faster than for looping and appending; and
enumerate allows you to get the index and character from source simultaneously.

With those in mind:

>>> def search2(word, source):
    """Find every index in source at which word starts."""
    firstchar = word[0]
    wordlen = len(word)
    return [index for index, char in enumerate(source)
            if firstchar == char and word == source[index:index+wordlen]]

>>> timeit.timeit("search2(word, source)",
                  setup="from __main__ import search1, search2, search3; import re; word, source = 'foo', 'foobarbaz'*1000",
                  number=10000)
9.99040215947457

I have also added a docstring to explain what the function does.

However, if you really want efficiency (and readability, for that matter), ignoring regular expressions is a bad move:

>>> def search3(word, source):
    """Find every index in source at which word starts."""
    return [m.start() for m in re.finditer(word, source)]

>>> timeit.timeit("search3(word, source)",
                  setup="from __main__ import search1, search2, search3; import re; word, source = 'foo', 'foobarbaz'*1000",
                  number=10000)
2.1652778798459167


Extract indices of visible nodes in a tree where only some nodes are expanded A program that I work on constantly updates and modifies program state containing an arbitrarily deep and wide tree. The data looks something like this.

(defonce state
         (r/atom {:some  "program"
                  :state "here"
                  :tree  [{:topic    "Books"
                           :expanded true
                           :children [{:topic "Titles"}
                                      {:topic    "Authors"
                                       :expanded true
                                       :children [{:topic "Alice"}
                                                  {:topic "Bob"}
                                                  {:topic "Carol"}]}
                                      {:topic "Genres"}]}
                          {:topic    "CDs"
                           :children [{:topic "Genres"}
                                      {:topic "Albums"}
                                      {:topic "Artists"}]}
                          {:topic    "To Do"
                           :expanded true
                           :children [{:topic    "Spouse Birthday"
                                       :expanded nil
                                       :due-date "07/31/2025"
                                       :children [{:topic "Buy Card"}
                                                  {:topic "Buy Jewelry"}
                                                  {:topic "Buy Cake"}]}]}]}))

As the program runs, I need to extract the series of numeric vector indices to reach the visible topics (those that have :children where the :expanded flag is present and "truthy").

Here is a method that works.

;; This flattening function is from an idea presented in response to
;; this question:
;; https://stackoverflow.com/questions/5232350/clojure-semi-flattening-a-nested-sequence
(defn flatten-to-vectors
  [s]
  (mapcat #(if (every? coll? %) (flatten-to-vectors %) (list %)) s))

(defn visible-nodes
  [tree so-far]
  (flatten-to-vectors
    (map-indexed
      (fn [idx ele]
        (let [new-id (conj so-far idx)]
          (if (not (and (:children ele) (:expanded ele)))
            new-id
            (cons new-id (visible-nodes (:children ele) new-id)))))
      tree)))

;; The function can be called like this on the data above.
(println (visible-nodes (:tree @state) []))
;; => ([0] [0 0] [0 1] [0 1 0] [0 1 1] [0 1 2] [0 2] [1] [2] [2 0])

Although this works, it seems like I should be able to generate the correct result without creating nested sequences that require flattening. But I just can't come up with anything.

Any suggestions would be appreciated.

You are right to want to avoid creating the nested structure to begin with. I imagine you read my answer in the question you linked (the second one). As there, here the solution is to use mapcat instead of map. And as I also say in the comments there, while mapcat-indexed does not exist, you can just pass an extra (range) argument to get numbering.

(defn visible-nodes [tree]
  (mapcat (fn [idx ele]
            (map #(cons idx %)
                 (cons []
                       (when (:expanded ele)
                         (visible-nodes (:children ele))))))
          (range), tree))


Document iteration: what problems should I account for? (function( d, w ){

    function showClimb( e ){
        var t = e.target;
        console.log( t.id );
        while( t.id !== 'document-body' ){
            t = t.parentNode;
            console.log( t.id );
        }
    }

    d.body.addEventListener( 'click', showClimb, false );

}( document, window ))

I just wrote this snippet to test if an element or it's child has been selected. What risks does this approach have? What failsaves should I include to make sure this loop doesn't get stuck somehow? I am writing for IE9 and higher.

Well, overall, I'd worry first about your variable names :D d, w and t especially. A dev coming in seeing this would say "what sorcery is this?".

Now t.id isn't a guarantee. Not all elements may have an id attribute. You might encounter blanks while climbing up. Try filling it in with element.tagName.

Next would be your hard-coded document-body. Seems like document-body is something specific to your code. Losing it would mean an infinite climb. Why not compare against document for a more guaranteed way to hit the top?

Here's my take on it. Not optimal, but does the same thing.

function climb(element) {
  var elementName = element.id || element.tagName;
  return element === document ? [elementName] : climb(element.parentNode).concat(elementName);
}

document.body.addEventListener('click', function(e) {
  console.log(climb(e.target).join(' <- '));
}, false);
test


Repeat an action for a set of elements until all succeed, or progression halts Context

I'm trying to write a function that drops a list of tables from a database (e.g., tables A, B and C).
This function has the following type, it returns a list of booleans indicating whether each table was successfully dropped:

dropTables :: (IConnection conn) => conn -> [String] -> IO [Bool]

Now, imagine table C depends on table B, i.e., table B cannot be dropped while table C exists. The function will be able to drop tables A and C, but not B, and so it will return [True, False, True].

So, I wanted to write a function that takes a list of tables, and calls dropTables. If any of the operations fail, it calls dropTables again but this time only with the tables that could not be dropped in the first iteration. And so on, until either all tables have been dropped, or progression halts (i.e. all operations in a given iteration fail, due to e.g. a connection being closed)

This was my first stab at it:

dropTablesRec :: IConnection conn => conn -> [String] -> IO ()
dropTablesRec conn tables = do
  results <- dropTables conn tables
  let failures = map fst $ filter (not . snd) $ zip tables results

  unless (null failures) $
    if length failures == length tables
      then print $ "Failed to drop the following tables: " ++ intercalate ", " failures
    else dropTablesRec conn failures

I then tried to generalize the core algorithm, and remove all references to tables and database connections:

-- Repeats a given action for a set of elements, until it succeeds for all elements, or fails for all elements.
-- Elements for which the operation succeeds are not passed onto the next iteration.
-- Returns the elements for which the action failed.
repeatUntilAll :: Monad m => [a] -> ([a] -> m [Bool]) -> m [a]
repeatUntilAll xs f =
  let
    go [] = return []
    go failures
      | length failures == length xs  = return failures   -- halt
      | otherwise                     = repeatUntilAll failures f
  in
    do
      results <- f xs
      let failures = map fst $ filter (not . snd) $ zip xs results
      go failures

dropTablesRec can now be reduced to:

dropTablesRec' :: IConnection conn => conn -> [String] -> IO ()
dropTablesRec' conn tables = do
  failures <- repeatUntilAll tables (dropTables conn)
  unless (null failures) $ print $ "Failed to drop the following tables: " ++ intercalate ", " failures

I was wondering if there's a simpler way of implementing repeatUntilAll.

I had a look around Control.Monad.Loops to see if there was anything I could use, but no such luck. Being a Haskell novice doesn't help either, I'm only familiar with a very small part of the core libraries.

Other improvements (naming, conventions, greater generalization) are also welcome.

I quite like your aim for simplifying the function and factoring out the generic part. One solution (untested, just compiled) could be done using iterateUntilM from Control.Monad.Loops:

repeatUntilAll :: (Monad m) => ([a] -> m [Bool]) -> [a] -> m [a]
repeatUntilAll f = fmap snd . iterateUntilM fst step . ((,) True)
  where
    step (_, xs) = do
        results <- f xs
        let ys = map fst $ filter (not . snd) $ zip xs results
        return (length xs > length ys, ys)

We pass an additional boolean flag that tells if a particular step was an improvement or not. Note that we don't need to check for the final condition - if everything succeeds, there will be one final step with an empty list and then we'll stop.

However, I like a bit more yet another solution. Notice this: Let's say the first removal succeeds, the second fails and because of this, everything else. Then we iterate through the whole list, and then once more, before we finish. But in fact we know that we're bound to fail when we fail to remove all the other elements. In this second solution, we loop over all elements without distinguishing a full pass. Instead we "reset" the state every time we succeed and try all the failed elements again. I also like its simplicity, compared to the previous one:

-- | Tries to repeatedly act on each element until the action succeeds on each.
-- Alternatively stops when the action fails for all unfinished elements in one full round.
tryOnAll :: (Monad m) => (a -> m Bool) -> [a] -> m [a]
tryOnAll f = loop []
  where
    loop fs [] = return fs
    loop fs (w : ws) = do
        r <- f w
        if r then loop [] (fs ++ ws)
             else loop (w : fs) ws

(Again compiled but untested.) Note that in fs ++ ws we change the order of elements. This shouldn't be a problem, as we need to try out all before we fail anyway, but if you don't like it, you can use Seq instead of [] and maintain sequential order. Or you could also replace it with ws ++ reverse fs, but only if the total number of elements is moderate, otherwise you could start having problems with its O(n) complexity.

A tiny nit, unrelated to the main problem: Haddock comments allow you to generate nice documentation.


Easy Deep Copy of Array Javascript I recently learned about the difference between a copy and a deep copy. Some people where writing some rather lengthy functions to in order to create a deep copy.

Then I was just playing around and wondering if there is anything wrong with the following method:

var newDeepCopy = originalArray.concat();

Right, so... just concat the original array with... nothing? Am I missing anything?

What you're doing there is copying the array, not its contents. I.e. it's a shallow copy.

As mherzig mentions in a comment, what you end up with is an extra array containing references to the same elements as the first one. As long as those elements are primitive/immutable values, you're fine. But not so with other objects.

Besides, the conventional way of copying an array is to use slice() or slice(0) - not concat() (although the result is the same).

Anyway, as an example:

var obj = { foo: "bar" };
var array1 = [1, 2, obj];

// shallow copy
var array2 = array1.concat();

// changing a primitive works as you might expect
array2[0] += 10;
console.log(array1); // => [1, 2, { foo: "bar" }]
console.log(array2); // => [11, 2, { foo: "bar" }]

// but the object hasn't been copied, merely referenced in two places,
// so changing it via one array means it's changed in the other as well
array2[2].foo = "Hello, world!";
console.log(array1); // => [1, 2, { foo: "Hello, world!" }]
console.log(array2); // => [11, 2, { foo: "Hello, world!" }]

Hence why people go to great lengths to make deep copies, where the entire contents is copied, so there are no shared references to the same things.

Basically, you want shallow copies if you're just going to be manipulating the array itself (e.g. calling push, pop, splice, sort etc.), and don't want side-effects. E.g. a function that takes an array as its argument is usually best served making a shallow copy, if it intends to mess around with that array.

You want deep copies if you truly want to copy everything, because you're going to be changing the contents, or because you want to avoid other code altering something you hold a reference to.


Generic array flattening function With help from the codereview community, I've written a function to flatten a generic nested array. I have it written several ways but want to focus on the following implementation:

func flatten (input: [Any]) -> [Any] {

    var outputArray = [Any] ()

    for i in 0..<input.count {
        let data = input[i];
        (data is [Any]) ? outputArray += flatten(input: data as! [Any]) : outputArray.append(data)

    }

    return outputArray

}

Is there a more "swifty" way to write this without using higher order functions?

Additionally, I've been reading about tail call optimization and wonder how I can make this tail call optimized?

A caveat & context: I'm writing this answer because I think it's an interesting question, however I'm drawing from CS lectures from 15 years ago and some hasty Googling, so rather than aiming for a correct answer, I'm hoping this will prompt some responses from smarter people.

flatten1:

First of all, an iteration of your original function. The only thing that stood out was that the ternary operation was a bit hard to read on a single line.

I don't like using white-space for formatting, but in this case I'd probably put each expression on it's own line:

(data is [Any])
  ? outputArray += flatten(input: data as! [Any])
  : outputArray.append(data)

But in this case I prefer to use avoid the short hand format, and use the longer conditional assignment, which I prefer because then you can avoid the forced cast to [Any]:

func flatten1(input: [Any]) -> [Any] {

    var flattened = [Any] ()

    for index in 0..<input.count {
        let element = input[index]

        if let array = element as? [Any] {
            flattened.append(contentsOf: flatten1(input: array))
        }
        else {
            flattened.append(element)
        }
    }

    return flattened
}

flatten2:

The second iteration was that I moved the loop into the recursion. As far as I know (and it is likely that I misunderstand this), compliers can't perform a tail call optimization unless you pass through an accumulator that collects up the value as the recursive calls are made (otherwise the runtime needs to increase memory to keep track of the incremental values, then perform some operation - in this case concatinating arrays, as it works itself back to the first recursive call).

func flatten2(input: [Any]) -> [Any] {

    guard let head = input.first else { return [] }

    if let headArray = head as? [Any] {
        return flatten2(input: headArray) + flatten2(input: Array(input.dropFirst()))
    }
    else {
        return [head] + flatten2(input: Array(input.dropFirst()))
    }
}

flatten3:

With that in mind, here's a third iteration using an accumulator:

func flatten3(input: [Any], accumulated: [Any] = []) -> [Any] {

    guard let head = input.first else { return accumulated }

    if let headArray = head as? [Any] {
        return
            flatten3(input: Array(input.dropFirst()),
                     accumulated: flatten3(input: headArray, accumulated: accumulated))
    }
    else {
        var _accumulated = accumulated
        _accumulated.append(head)

        return flatten3(input: Array(input.dropFirst()),
                        accumulated: _accumulated)
    }
}

Comparisons

I made an attempt at profiling these to compare performance with each other. I can into a bit of a stumbling block here when I tried to manually write in a deeply nested array where Xcode gave an Expression was too complex to be solved in reasonable time error. I ended up creating the nested array for testing like this:

var input: [Any] = []

override func setUp() {

    let test: [Any] = [1, [2], [3, [4]], [5, [6]], 7, [8], [[[[9, [10]]], [11, [12], 13]], 14, [15, [16]], [17, [18]], 19], 20, [21, 22], [23, 24, 25]]

    let level1 = [test, test, test, test, test]
    let level2 = [level1, level1, level1, level1]
    let level3 = [level2, level2, level2, level2]
    let level4 = [level3, level3, level3, level3]
    input = [level4, level4, level4, level4]
}

So now for testing (MBP late 2016, using -O3 flag for optimization):

flatten1    0.089s  6% STDEV
flatten2    0.599s  4% STDEV
flatten3    3.297s  3% STDEV
flatMap     0.000s  175% STDEV (actual time ranged 0.000016s to 0.000251s)

So flatten3 is actually a lot slower than the original loop approach, but with the benefit (if tail-call optimization is performed) of using a constant amount of memory.

But the standout is how fast flatMap performs. The post asks for ways to write the function more swifty, but unless you're planning the fucntion to have some sort of additional side effects as the intermediary steps are performed, then the simple input.flatmap{ $0 } is the best case on all fronts!

Edit: As @MartinR points out, flatMap only flattens the first level. I feel a bit silly for not actually checking the output. I've also realized that I don't understand flatMap as shown by this snippet...

print([[1, 2], [3, 4]].flatMap{ $0 })
// -> [1, 2, 3, 4] (expected output)

print([1, [2, 3, 4]].flatMap{ $0 })
// -> [1, [2, 3, 4]] (I didn't expect this!)

Here's a gist if anyone wants to use this as a starting point of their own -- like I started the post, it would be great to see someone more experienced chime in with thoughts or corrections :)


Given N*N matrix, rotate it by 90 degree to left and right without extra memory I'm looking for a code review, clever optimizations, and good coding practices. Unit testing has been a visual inspection and concluded expectations match. Also, is the space complexity of the extra variable used to swap \$O(1)\$? Typically, is \$O(1)\$ an acceptable space to use when code requires no extra memory?

public final class RotateNinetyInPlace {

    private RotateNinetyInPlace() {}

    private static void transpose(int[][] m) {

        for (int i = 0; i < m.length; i++) {
            for (int j = i; j < m[0].length; j++) {
                int x = m[i][j];
                m[i][j] = m[j][i];
                m[j][i] = x;
            }
        }
    }

    public static void rotateByNinetyToLeft(int[][] m) {
        // transpose
        transpose(m);

        //  swap rows
        for (int  i = 0; i < m.length/2; i++) {
            for (int j = 0; j < m[0].length; j++) {
                int x = m[i][j];
                m[i][j] = m[m.length -1 -i][j];
                m[m.length -1 -i][j] = x;
            }
        }
    }

    public static void rotateByNinetyToRight(int[][] m) {
        // transpose
        transpose(m);

        // swap columns
        for (int  j = 0; j < m[0].length/2; j++) {
            for (int i = 0; i < m.length; i++) {
                int x = m[i][j];
                m[i][j] = m[i][m[0].length -1 -j];
                m[i][m[0].length -1 -j] = x;
            }
        }
    }

    public static void main(String[] args) {
        int[][] mEven = {{1, 3},
                        {2, 4}};

        rotateByNinetyToLeft(mEven);

        for (int i = 0; i < mEven.length; i++) {
            for (int j = 0; j < mEven[0].length; j++) {
                System.out.print(mEven[i][j] + " ");
            }
            System.out.println();
        }

        System.out.println("---------------------------------");

        rotateByNinetyToRight(mEven);

        for (int i = 0; i < mEven.length; i++) {
            for (int j = 0; j < mEven[0].length; j++) {
                System.out.print(mEven[i][j] + " ");
            }
            System.out.println();
        }

        System.out.println("---------------------------------");

        int[][] mOdd = {{1, 2, 3},
                        {4, 5, 6},
                        {7, 8, 9}};

        rotateByNinetyToLeft(mOdd);

        for (int i = 0; i < mOdd.length; i++) {
            for (int j = 0; j < mOdd[0].length; j++) {
                System.out.print(mOdd[i][j] + " ");
            }
            System.out.println();
        }

        System.out.println("---------------------------------");

        rotateByNinetyToRight(mOdd);

        for (int i = 0; i < mOdd.length; i++) {
            for (int j = 0; j < mOdd[0].length; j++) {
                System.out.print(mOdd[i][j] + " ");
            }
            System.out.println();
        }

        System.out.println("---------------------------------");

    }
}

You ask many questions on CodeReview, which in itself is good, but you have to start helping the reviewers actually review your code. You have this habit of dumping code and expecting a review. It does not work that way (very well). For a start, let's review the ideal process for a 'real' review:

you present neat and working code for review.
you include a description of the context of the code - what it does, why it is needed, and what constraints it has.
you provide some specifications or documentation for any complicated or significant algorithms
the reviewer reviews the code, not only for neatness, and style, but to:

check the code works the way it is supposed to (by visual inspection)
suggest alternative approaches that may achieve the same results faster or in a better way
suggest alternative approaches that may produce different results but may be better for other reasons

signs off that the code is:

good (or great and gives you a high five)
good enough but has some future anticipated improvements
not good enough and needs to be fixed then resubmitted

The details, descriptions, references and specifications you provide with the request is essential if the reviewer is going to give a decent review.

On Code review, the process continues with:

upvoting answers that are helpful (and down-voting those that are not).
accepting answers that (among other answers) best address your concerns.

Your question above is lacking any description of what the code is supposed to do, and how it does it. All you do say is:

I want hints and reviews
it works
what is the complexity

As a result, the only specification available and description of what the code should be doing, is what the code actually does. Or the title of the post....

Bottom line is that you have only given a fraction of what is needed for a good review, so, I will take a stab at doing a review with similar feedback....

A good alternative approach for your code is:

public static void rotateByNinetyToLeft(int[][] m) {

    int e = m.length - 1;
    int c = e / 2;
    int b = e % 2;
    for (int r = c; r >= 0; r--) {
        for (int d = c - r; d < c + r + b; d++) {
            int t   = m[c - r][d];
            m[c - r][d] = m[d][e - c + r];
            m[d][e - c + r] = m[e - c + r][e - d];
            m[e - c + r][e - d] = m[e - d][c - r];
            m[e - d][c - r] = t;
        }
    }
}

This approach is O(n) time complexity (n is the number of pixels in the matrix), and O(1) space complexity

I have tested this and it works.

You can use the right-handed approach for rotateByNinetyToRight


Streaming variable length integers from a file I wrote this script as a proof of concept of reading a file from disk as variable-length quantities (ints encoded using 7 bits from as many bytes as required). I'm mostly making use of the file system ReadStream, for reading byte buffers from files and the varint package for decoding variable length values.

This is is my first attempt at working with nodejs. When I tested it on a 1GB file read from SSD, it took about 7 minutes during which it used about 50mb of ram and saturated one CPU core.

So my question is, is there anything I'm doing here which could be optimised for CPU load?

var fs, vi, sys, rs, ints, total, remainder;
fs = require('fs');
vi = require('varint');
sys = require('sys');

rs = fs.createReadStream('/path/to/datafile');
total = 0;

// a buffer for carrying over incomplete values between chunks
remainder = new Buffer(0);

rs.on('data', function(chunk) {
  var buffer, value, byteLen;
  buffer = Buffer.concat([remainder, chunk]);

  while(buffer.length){
    // decode the value and check how many bytes it required
    value = vi.decode(buffer);
    byteLen = vi.decode.bytesRead;

    if (byteLen <= buffer.length){
      // keep the result and update position in the buffer
      total += value;
    } else {
      // result is incomplete, store remainder to be prepended to the next chunk
      remainder = new Buffer(buffer.length);
      buffer.copy(remainder);
    }

    // shift decoded bytes off the buffer
    buffer = buffer.slice(byteLen);
  }
});

// display grand total after the whole file has been read
rs.on('end', function() {
  sys.puts((total));
});

It turns out it was possible to increase the speed about 12 times by using the offset argument of varint.decode to traverse the buffer rather than slicing read bytes off of the buffer for each value.

rs.on('data', function(buffer) {
  var buffer, value, byteLen, offset;

  if (remainder) {
    buffer = Buffer.concat([remainder, buffer]);
    remainder = null;
  }

  offset = 0;

  while(offset < buffer.length){
    // decode the value and check how many bytes it required
    value = vi.decode(buffer, offset);
    offset += vi.decode.bytesRead;

    if (offset <= buffer.length){
      // keep the result and update position in the buffer
      total += value;
    } else {
      // result is incomplete, store remainder to be prepended to the next chunk
      remainder = new Buffer(buffer.length - offset);
      buffer.copy(remainder, 0, offset);
    }
  }
});

Much better.

UPDATED to incorporate suggestions.


XMLHttpRequest with cancel, timeout and retry option I've made the following code-snippet, trying to achieve a replacement for Fetch Which have the abortion, timeout and retry options.

It worked for me, but i'm not sure if it is a good idea to do so or not. Any better ideas?

const customXmlHttpRequest = (url, opts={}, cancel={}, timeoutMS = 30000) => {

  return new Promise((resolve, reject)=>{

    var xhr = new XMLHttpRequest();
    xhr.open(opts.method || 'get', url);
    xhr.timeout = timeoutMS;
    for (var k in opts.headers || {}){
      xhr.setRequestHeader(k, opts.headers[k]);
    }
    xhr.onload = e => resolve(e.target);
    cancel.abort = () => {
      xhr.abort();
      reject("Cancelled")
    };
    xhr.onerror = reject;
    xhr.ontimeout = () => {
      reject("Timeout")
    }
    xhr.send(opts.body);

  });

}

const FetchWithRetry = (url, opts={}, retry=3, retryDelay=1000, cancel={}, timeoutMS = 30000) => {
  return new Promise((resolve, reject)=>{
    var counter = 0;

    const FetchWithTimeout = ()=>{
      if(counter < retry){
        counter++
        customXmlHttpRequest(url, opts, cancel={}, timeoutMS = 30000)
        .then((res)=>{
          resolve(res)
        })
        .catch((e)=>{
          if(e === "Cancelled"){
            reject(e)
          } else if(e === "Timeout"){
            ///retry
            setTimeout(()=>{
              FetchWithTimeout()
            }, retryDelay)
          } else if(String(e).includes("Network request failed")){
            /// retry
            setTimeout(()=>{
              FetchWithTimeout()
            }, retryDelay)
          } else {
            reject(e)
          }
        })
      }
    }
    FetchWithTimeout()
  })
}

First of all, there's already a fetch function that's already available in most modern browsers. If you care about IE, there's also a polyfill for it. With that out of the way, we can ignore the first part of your code and focus on the second part of the code.

While it's perfectly fine JS, having very long argument lists is horrible. As a developer, you have to evaluate which arguments take priority and come first. As a consumer, you have to remember which ones come first. I suggest turning your options into an object. You can use Object.assign to merge a defaults object.

With regards to your control flow, you can simplify the retry routine by simply calling fetchWithRetry recursively, passing it the retries remaining. Since this operates on just promises and promises cannot be cancelled, this version's abort bails out when abort is called during the delay or after the request, not immediately.

const fetchWithRetry = (userOptions) => {
  let abort = false;
  const options = {
    url: '',
    options: {},
    cancel: {},
    retries: 3,
    retryDelay: 1000,
    ...userOptions
  };

  // Add an abort to the cancel object.
  cancel.abort = () => { abort = true };

  // Abort or proceed?
  return abort ? Promise.reject('aborted') : fetch(options.url).then(response => {

      // Reject because of abort
    return abort ? Promise.reject('aborted')

      // Response is good
      : response.ok ? Promise.resolve(response.text())

      // Retries exceeded
      : !options.retries ? Promise.reject('retries exceeded')

      // Retry with one less retry
      : new Promise((resolve, reject){
          setTimeout(() => {

            // We use the returned promise's resolve and reject as
            // callback so that the nested call propagates backwards.
            fetchWithRetry({ ...options, retries: retries - 1 }).then(resolve, reject);
          }, options.retryDelay);

        });
  });
}


"Remove Friends" problem time efficiency too slow I'm doing this problem:

  After getting her PhD, Christie has become a celebrity at her
  university, and her Facebook profile is full of friend requests. Being
  the nice girl she is, Christie has accepted all the requests.

  Now Kuldeep is jealous of all the attention she is getting from other
  guys, so he asks her to delete some of the guys from her friend list.

  To avoid a 'scene', Christie decides to remove some friends from her
  friend list, since she knows the popularity of each of the friend she
  has, she uses the following algorithm to delete a friend.

Algorithm Delete(Friend):
    DeleteFriend=false
    for i = 1 to Friend.length-1
         if (Friend[i].popularity < Friend[i+1].popularity)
            delete i th friend
            DeleteFriend=true
            break
    if(DeleteFriend == false)
        delete the last friend

  Input: First line contains T number of test cases. First line of each
  test case contains N, the number of friends Christie currently has and
  K ,the number of friends Christie decides to delete. Next lines
  contains popularity of her friends separated by space.

  Output: For each test case print N-K numbers which represent
  popularity of Christie friend's after deleting K friends.

  Constraints:

1 <= T <= 1000
1 <= N <= 100000
0 <= K < N
0 <= popularity_of_friend <= 100

  NOTE: Order of friends after deleting exactly K friends should be
  maintained as given in input.

  Sample Input

3
3 1
3 100 1
5 2
19 12 3 4 17
5 3
23 45 11 77 18

  Sample Output

100 1
19 12 17
77 18

  Time Limit 1 sec(s) (Time limit is for each input file.)

  Memory Limit 256 MB

  Source Limit 1024 KB

Initially I would read and add the numbers to the list, then follow that algorithm.  I figured doing what that algorithm does except as I add it in would be much faster (which it was):

import java.util.*;

class TestClass {
    public static void main(String args[] ) throws Exception {
        Scanner scanner = null;
        try{
            scanner = new Scanner(System.in);
        }catch(Exception e){
            return;
        }
        int T = Integer.parseInt(scanner.next());

        //remove as i add to list
        for(int i = 0; i < T; i++){
            int friendsStart = Integer.parseInt(scanner.next());
            int friendsDelete = Integer.parseInt(scanner.next());
            ArrayList<Integer> inputList = new ArrayList<Integer>();
            int currentPos = 0;
            for(int j = 0; j < friendsStart ; j++){
                p.add(Integer.parseInt(scanner.next()));
                if(fD > 0 && currentPos > 0 && inputList.get(currentPos) > inputList.get(currentPos-1)){
                    inputList.remove(currentPos-1);
                    friendsDelete--;
                    currentPos--;
                }else{
                    currentPos++;
                }
            }

            //remove remainder required to remove
            while(friendsDelete > 0 && c < inputList.size()){
                if(currentPos > 0 && inputList.get(currentPos) > inputList.get(currentPos -1)){
                    inputList.remove(currentPos-1);
                    friendsDelete--;
                    currentPos--;
                }else{
                    currentPos++;
                }
            }

            //print
            for(int j = 0; j < inputList.size(); j++){
                System.out.print(inputList.get(j) + " ");
            }
            System.out.println();
        }
    }
}

However, when I submit, it does some bigger test cases (not sure what the cases are), but it ends up being too slow.  I wanted some hints as to how I can make this faster than it already is.  I don't want a direct answer.

Hint: The problem statement asks you to remove friends from the end of the list as necessary. Think carefully about this.

   You don't actually have to remove the remaining friends to delete. Instead, just don't print them.

 Change your while and for to the following code.

Spoiler tags and code blocks don't interact nicely, so the solution code is base 64 encoded.

Zm9yIChpbnQgaiA9IDAsIGptYXggPSBpbnB1dExpc3Quc2l6ZSgpIC0gZnJpZW5kc0RlbGV0ZTsgaiA8IGptYXg7IGorKykgew0KICAgICBTeXN0ZW0ub3V0LnByaW50KGlucHV0TGlzdC5nZXQoaikgKyAiICIpOw0KfQ==

I believe @EmanuelePaolini is correct in that you're likely getting TLE because of I/O. You're using Scanner, which is way too slow. Try a different input method.

   You can try switching to BufferedReader. String.split() is still slow, so you can combine it with StringTokenizer. Here's a sample on ideone.

Encoded:

c3RhdGljIEJ1ZmZlcmVkUmVhZGVyIGJyID0gbmV3IEJ1ZmZlcmVkUmVhZGVyKG5ldyBJbnB1dFN0cmVhbVJlYWRlcihTeXN0ZW0uaW4pKTsNCnN0YXRpYyBTdHJpbmdUb2tlbml6ZXIgc3QgPSBuZXcgU3RyaW5nVG9rZW5pemVyKCIiKTsNCnN0YXRpYyBpbnQgbmV4dEludCgpIHRocm93cyBJT0V4Y2VwdGlvbiB7DQoJd2hpbGUgKCFzdC5oYXNNb3JlVG9rZW5zKCkpDQoJCXN0ID0gbmV3IFN0cmluZ1Rva2VuaXplcihici5yZWFkTGluZSgpKTsNCglyZXR1cm4gSW50ZWdlci5wYXJzZUludChzdC5uZXh0VG9rZW4oKSk7DQp9


Optimizing puzzle game involving sequence numbers I am currently developing a puzzle game that has sequence numbers. The player has to fill the grid with sequence numbers in ascending order. Starting from 1 the player can move horizontally or across and can skip a square in between if it is empty or blocked.

To generate the puzzle automatically, my code is doing the following process:

Make some of the squares blocked.
Select a random position for 1 and calculate the possible moves for 2.
Select a move from possible moves of 2 and store other possible moves in an array so that they can be used for while backtracking if I don't get the solution.
Proceed and backtrack if I don't get a solution.

The grid of puzzle can be anywhere between 4x4 and 9x9.

$possibleMoves = getPossibleMoves($rowForOne,$columnForOne,$solution,$value);
$puzzleSolvedFlag = false;

while($value <= $maximumNumberToBeEntered) {
    $randomMove = array_rand($possibleMoves);
    $position = explode(",",$possibleMoves[$randomMove]);
    unset($possibleMoves[$randomMove]);
    array_push($moves,$possibleMoves);
    $solution[$position[0]][$position[1]] = $value;
    $value++;
    $possibleMoves = getPossibleMoves($position[0],$position[1],$solution,$value);
//  echo $value;
    //print_r($possibleMoves);
    while(count($possibleMoves)==0 && $value<=$maximumNumberToBeEntered) {
        $possibleMoves = array_pop($moves);
        //print_r($possibleMoves);
        $value--;

        foreach ($solution as &$v1) {
            foreach ($v1 as &$v2) {
               if($v2 >= $value) {
                    $v2 = -1;
                }
            }
        }
        if(count($possibleMoves) > 0) {
            break;
        }
    }
    //print_r($possibleMoves);
}

The solution is the array grid. Before the above code I just selected a random move for 1 and the $value will be 2. getPossibleMoves() returns possible moves for 2.

$maximumNumberToBeEntered is the maximum value in the grid (e.g. for a 4x4 grid, if 3 squares are blocked, then the maximum value will be 16-3 = 13).

Can someone help me to optimize the above code? It involves a lot of backtracking.

I've dug into the concept a little bit and as a start here are some bits of code and discussion to help out. Obviously, the real meat here is doing the backtracking efficiently but I think starting with simplifying the problem a bit would help.

For starters, it seems that turning the grid into a simple list would make it easier to deal with the relevant code.

// Initialize game parameters
// --------------------------
$game = array();
$game['xlen']    = 4;
$game['ylen']    = 4;
$game['blocked'] = 1;
$game['size']    = $game['xlen'] * $game['ylen'];
$game['playing'] = 0;

// Initialize empty grid
// ---------------------
$grid = array();
for ( $i = 0; $i < $game['size']; $i++ )
  $grid[] = 0;

// Let's take a look
// -----------------
echo "Initialized game grid...

\n";
dump_grid($game,$grid,3);

So, here's a grid dumping function that will allow very detailed insight into the behavior of the running program in case it isn't doing exactly what one might expect.

// It's always important to be able to "see" what is happening to get insight
// into what is working and what isn't.
// --------------------------------------------------------------------------
function dump_grid($game,$grid,$indents = 0)
{
  // Build up indent value
  // ---------------------
  $indent = '';
  if ( $indents )
  {
    $indent = str_repeat("&nbsp;",$indents);
    echo $indent;
  }

  // Dump out grid contents
  // ----------------------
  $x = 0;
  foreach ($grid as $value)
  {
    echo "| " . sprintf("%02d",$value) . " ";

    $x++;
    if ( $x >= $game['xlen'] )
    {
      echo "|
\n$indent";
      $x=0;
    }
  }
  echo "
\n";
}

Using the above I can watch events unfold in the following manner... as I like to iterate code/test efforts by reloading the code in a browser page.

Initialized game grid...

   | 00 | 00 | 00 | 00 |
   | 00 | 00 | 00 | 00 |
   | 00 | 00 | 00 | 00 |
   | 00 | 00 | 00 | 00 |

Sample blocked square...

   | 00 | 00 | 00 | 00 |
   | -1 | 00 | 00 | 00 |
   | 00 | 00 | 00 | 00 |
   | 00 | 00 | 00 | 00 |

Starting position selected...

   | 00 | 00 | 00 | 00 |
   | -1 | 00 | 00 | 00 |
   | 00 | 00 | 00 | 00 |
   | 00 | 00 | 00 | 01 |

Of course, the hard part is left. What I would consider doing at this point is using the "grid" to store game state with respect to blocked squares and moves and maintaining a separate move tree to ensure that random selection doesn't end up trying failed branches over and over.

Using your game objects, or mine, a method of maintaining the known failing moves from a given state will be needed. That could be integrated into a move selection function. I have a rudimentary move selection function used to choose the blocked location and starting point in the above.

// Pick a random grid position. Optional parameter to force game placement
// rules to be applied when picking a position.
// --------------------------------------------------------------------------
function pick_open($game,$grid,$playing = 1)
{
  $open = array();
  for ($i=0; $i < $game['size']; $i++)
    if ( $grid[$i]==0 )
      $open[$i] = $i;

  if ( !count($open) )
    die("Attempting to pick an open spot when none are available!");

  if ( !$playing )
    return array_rand($open,1);

  die("Rules based open spot select not yet implemented!");
}

As I'm trying to help and not create the whole solution I'll have to leave that to you. However, I would strongly suggest making the effects of your code as visible as possible and looking for ways to simplify the problem.

Hint: Perhaps use the game state (the grid being output) as a key to determine if any moves from that state have already proven to be failures.

Note: If you weren't using random move selection you could just iterate through all possible moves and accept the first valid solution. However, a non-random iteration might make a game too predictable.


Tail recursive FizzBuzz in Common Lisp I solved FizzBuzz using tail recursion. Is it efficient enough?

(defun mod-3 (n)
   (zerop (mod n 3)))

(defun mod-5 (n)
  (zerop (mod n 5)))

(defun analyzer (n)
  (if (mod-3 n)
  (if (mod-5 n)
      'fizzbuzz
      'fizz)
  (if (mod-5 n)
      'buzz
      n)))

(defun fizzbuzz (limit)
  (fizzbuzz-helper limit nil))

(defun fizzbuzz-helper (limit result)
  (cond ((zerop limit) result)
    (t (fizzbuzz-helper (- limit 1)
            (cons (analyzer limit) result)))))

When a function does too little...
I personally find that your mod-3 and mod-5 functions do something so very specific that you may as well at least combine them into a generic function.
I would call them something different, because what your functions are really doing is not a modulus (there's already a mod function for that), it's verifying if a number is evenly divisible by another number. I would write something like this:
(defun evenly-divisible-by (n x)
  "Given divisor n and dividend x, returns true if x ÷ n has no remainder."
  (zerop (mod x n)))

analyzer
This function's name really doesn't say much to the next person reading the code. What does it analyze? Why? What goal is it trying to achieve? I think a name like calc-fizzbuzz-for-n would honestly be much more clear about (1) the action "calc" and (2) the expected result from the action "fizzbuzz".
In this function, the indentation you applied is a bit misleading. I added some comments below to illustrate:

(defun calc-fizzbuzz (n)
  (if (evenly-divisible-by 3 n)
  ;then...
  (if (evenly-divisible-by 5 n)
      ;then...
      'fizzbuzz
      ;else...
      'fizz)
  ;else...
  (if (evenly-divisible-by 5 n)
    ;then...
    'buzz
    ;else...
      n)))

This seems minor for just a small indentation (and Lisp can make it seem more trivial since you have all those parentheses anyways), but if you wrote something like this in a more "typical" language, I would certainly expect it would be pointed out during a code review...

if (condA) {
if (condB) {
    foo.actionB();
} else {
    foo.actionA();
}
if (condC) {
    foo.ActionC();
} else {
    foo.defaultAction();
}
}

Applying above changes as well, I think this would be better:
(defun calc-fizzbuzz-for-n (n)
  "Given a number n, return whether it is evenly divisible by 3 (fizz), 5 (buzz),
  or both (fizzbuzz). Returns n when neither condition is true."
  (if (evenly-divisible-by 3 n)
      (if (evenly-divisible-by 5 n)
          'fizzbuzz
          'fizz)
      (if (evenly-divisible-by 5 n)
          'buzz
          n)))

Optional & default values
As far as I can tell, the only purpose of the fizzbuzz function is to take a limit argument, then call fizzbuzz-helper with the limit and added nil for its result argument. This can be simplified by making the result argument optional, like so:
(defun fizzbuzz (limit &optional result)

Note: I changed its name to fizzbuzz since it is no longer a "helper" to your original fizzbuzz function.
According to a related article on nullprogram.com:

If second and third arguments are not provided, b and c will be bound to nil. To provide a default argument, put that parameter inside a list. Below, when a third argument is not provided, c will be bound to "bar".
(defun foo (a &optional b (c "bar"))

So in your case, result will be nil unless it is provided a different argument. Alternatively, and as it appears to be the intention, you can force result to be nil by default using the let special operator:
(defun fizzbuzz (limit)
  (let (result) ;; defaults to nil if not specified
                ;; you can also do this if you find it more clear: (let ((result nil))
    (cond ;...

Alternative solution using lambda expression
This type of approach is often more idiomatic to Lisp/functional programming. You can use an anonymous function (a.k.a. lambda expression) as an argument to a map function. Your last line would look something like this:
(t (map 'list
        #'fizzbuzz
        (loop for i from 1 to limit by 1 collect i)))

or simpler:
(t (loop for i from 1 to limit collect (fizzbuzz i)))

More modern Lisps have a built-in range function to make things simpler, which Common Lisp doesn't; thankfully though, you can just write your own (code below from Stack Overflow answer Python's range() analog in Common Lisp):

(defun range (max &key (min 0) (step 1))
   (loop for n from min below max by step
      collect n))

And your last line can simply become:
(t (map 'integer #'(lambda (N) (fizzbuzz N)) (range limit 1)))
Thanks to @Renzo for the correction,  your last function could be written as:
(defun fizzbuzz (limit)
  (mapcar #'calc-fizzbuzz-for-n (range (1+ limit) :min 1)))

or using LOOP:
(defun fizzbuzz (limit)
  (loop for n from 1 upto limit
        collect (calc-fizzbuzz-for-n n)))


Unit testing code that verifies order of method execution Say I have a piece of code

public class OrderValidator : IValidator<OrderCommand>
{
    private readonly IValidationService validationService;

    public OrderValidator(IValidationService validationService)
    {
        this.validationService = validationService;
    }

    public ValidationResult Validate(OrderCommand command)
    {
         var stepBuilder = this.validationService.CreateValidationStepBuilder(command);

         stepBuilder
             .AddCommandStep<OrderedItemIsValid>()
             .AddBreakIfNotValidStep()
             .AddCommandStep<CustomerIsValid>()
             .AddCommandStep<CustomerAddressIsValid>()
             .AddBreakIfNotValidStep()
             .AddCommandStep<OrderPaymentIsValid>()
             .AddCommandStep<OrderPaymentSuccessful>();

         return stepBuilder.Validate();
    }
}

The purpose of the validator is to setup the various validation steps (the Add... bit). The actual task of validation is delegated to a service, which creates a step builder, which exposes a validate method. So the validation step builder has abstracted away the job of actually storing and running the steps in order away from the validator.

In my unit test for this class, I have to make sure that the validation steps have been applied in the correct order. Since that is the only responsibility of the validator class.

[Fact]
public void AssertThatStepsAreAddedInOrder()
{
    var validationStepBuilderMock = this.CreateStepBuilderMock();

    // assume step builder mock has been hooked up correctly to the validator under test at this point
    this.Validator.Validate(new OrderCommand());

    // Using NSubstitute, verify that the order of steps is correct
    Received.InOrder(() =>
    {
        validationStepBuilderMock
             .AddCommandStep<OrderedItemIsValid>()
             .AddBreakIfNotValidStep()
             .AddCommandStep<CustomerIsValid>()
             .AddCommandStep<CustomerAddressIsValid>()
             .AddBreakIfNotValidStep()
             .AddCommandStep<OrderPaymentIsValid>()
             .AddCommandStep<OrderPaymentSuccessful>();
    });
}

Is this a good unit test? My colleagues have argued no since all I have done is copy and paste the steps into the test (unit test exhibits tautology). If the steps are changed, or a new one is added, then all that needs to be done is to update the test method order.

The code does look extremely similar to the validator under test due to the fluent nature of the step builder (Add... method chaining). But how else would you test for order of execution? Surely the point of a test that tests the order of code execution is to break when that order is violated?

I think your colleagues are correct in that your test looks like it is testing code written as part of your test.  Meaning code that looks similar to the production code, but isn't.  But there are things of value here to test.

You mentioned that your builder has tests around it.  I would think that these "do things execute in order" questions are best answered by testing your builder in different configurations.  the question is simply, "can the builder build things that execute in order".

Secondly, the validation service is a concrete implementation with and interface (abstraction).  You'll likely want to test the concrete implementation because it will probably (unless configured dynamically) have a very fixed set of validation rules that are set for your business needs.  Those tests become, "is my validation service catching all of the currently considered scenarios".


Select first value if exists, otherwise select default value Given an instance of SQL Server, imagine there's a table named Configuration, which has three columns: ID, Name, and Data.  There should be no duplicate rows for Name.

Now imagine that you want to select the data stored for a particular configuration, but if that configuration doesn't have a row in the table, then you just want to select a default value instead.

This, then, is what I have been using:

SELECT CASE
         WHEN EXISTS(SELECT 1
                     FROM   Configuration
                     WHERE  Name = 'NameOfConfiguration')
           THEN (SELECT Data
                 FROM   Configuration
                 WHERE  Name = 'NameOfConfiguration')
         ELSE 'Default Value'
       END

But that looks bad, so I was trying to think of a way to shorten the EXISTS() call, make the code a little clearer, get rid of duplicate code, and possibly even speed it up, and this is what I've been able to find so far:

SELECT CASE
         WHEN COUNT(1) > 0
           THEN MIN(Data)
         ELSE 'Default Value'
       END
FROM   Configuration
WHERE  Name = 'NameOfConfiguration'

Is this latter code snippet the right way to go about this sort of thing in general? Unfortunately, SQL Server requires an aggregate function call or GROUP BY clause to be applied to the latter, and FIRST() evidently doesn't exist in SQL Server.  I am not restricting this call to MIN() to any sort of datatype whatsoever.  Is there a better way to write this?

The shorter query I can think of is

SELECT COALESCE(MAX(Data), 'Default Value')
FROM   Configuration
WHERE  Name = 'NameOfConfiguration';

If the configuration is present it'll take it.
If the configuration is not present the MAX will generate a NULL value (the MAX of nothing is NULL) that will be coalesced to the default value

There is a more generic approach that takes care of what said by Martin Smith in his comment, but is more verbose (using the hypothesis that the configuration name are unique)

WITH Param AS (
  SELECT ID, Data
  FROM   Configuration
  WHERE  Name = 'NameOfConfiguration'
  UNION ALL
  SELECT NULL, 'Default Value'
)
SELECT TOP 1
       Data
FROM   Param
ORDER BY ID DESC

The ID of the Default Value is set to NULL to avoid to use a magic number.
From the BOL: NULL values are treated as the lowest possible values.

IMO a general solution should be used only where a specific one is not possible, and that doesn't seems the case. Also in a generic case the Default Value will need to be a valid value for the datatype of the configuration values otherwise the UNION will return a conversion error.


Wedding Photography Cost Calculator This is my first code. I would really appreciate a critique for it, if anyone can spare the time.

def wedcost(hours):

    print " "

    try:
        hours = int(hours)
    except ValueError:
        print "Invalid input"
        new_hours = raw_input("Please use only numeric characters. ")
        hours = int(new_hours)
        print " "

    tcost = hours * 75 # $75/hr for two shooters
    if hours < 4:
        tcost = 250 # price starts at a fixed $250 from 1 to 3 hours
    if hours >= 6:
        tcost = hours * 100 # I don't like long shifts

    print "The total cost of our services will be $" + str(tcost) + "."

    print " "

    def recalc(answer): # this second function is where I had the most trouble

        outcome = 0 # only method I could get to work properly
        if answer.startswith('y' or 'Y'):
            print " "
            outcome += 1
            wedcost(raw_input("How long will we be on location? "))
        if answer.startswith('n' or 'N'):
            print " "
            print "Have a nice day :)"
            outcome += 1
        if outcome == 0:
            print "Invalid input"
            recalc(raw_input("Would you like to recalculate? ('Yes' or 'No') "))

        # I had previously tried checking 'answer' to items in lists

    recalc(raw_input("Would you like to recalculate? ('Yes' or 'No') "))

wedcost(raw_input("How long will we be on location? "))

I'm sure that there's a better way to go about this task. Any suggestions or hints?

Well, this code has a few problems:

Typing non-numbers twice in a row causes a ValueError.
It is not quite PEP-8 compliant (the official python style guide)
No docstrings
Final bit of code should be wrapped in a if __name__ == "__main__": block to add reusability.
It is not very reusable

Now let's fix all that:

Wrap your input section in it's own functions like this:

def get_num(prompt, errorMsg = "I'm sorry, that's not a valid number"):
    """Returns a number from the user"""
    while True:
        try:
            input = int(raw_input(prompt))
        except ValueError:
            print errorMsg
        else:
            return input

def get_yes_no(prompt, errorMsg = "I'm sorry, I didn't catch that. Could you try again?"):
    """Returns True for yes, False for no"""
    while True:
        input = raw_input(prompt)
        if input.startswith('y' or 'Y'):
            return True
        elif input.startswith('n' or 'N'):
            return False
        print errorMsg

This allows you to have more robust and more readable code

Naming: function names should be lowercase and separated by _. So wedcost(hours) becomes wed_cost(hours).

Also your function doesn't do what is advertised, since it keeps on recalculating.

Let's wrap that into a main function like this:

def main():
    wed_cost(get_num("How long will we be on location? "))
    while get_yes_no("Would you like to recalculate? ('Yes' or 'No') "):
        wed_cost(get_num("How long will we be on location? "))
    print "Have a nice day :)"

Then we can add this little bit of code to the bottom:

if __name__ == "__main__":
    main()

Which will run your code if it is directly called, but also allows you to reuse this code elsewhere with an import without causing any code to be run.

Finally let's put all your magic numbers (such as 75, 100 etc) somewhere more recyclable by defining some constants at the top of the code:

MIN_FEE = 250
HOURLY_FEE = 75
OVERTIME_HOURS = 6
OVERTIME_FEE = 100

Then the wed_cost function becomes:

def wed_cost(hours):
    """Prints a message with the cost of a wedding shoot given a number of hours"""
    totalCost = 0
    if hours >= OVERTIME_HOURS:
        totalCost = hours * OVERTIME_FEE
    totalCost = max(hours * HOURLY_FEE, MIN_FEE)
    print "The total cost of our services will be ${}.".format(totalCost)

This makes your code more understandable, and more debuggable :)


Observer pattern in Java I'm going to have a lot of integer values, and at any value change I might need to update any number of UI elements, other values which are computed from the first ones, etc.  Sounds like time for the Visitor pattern, but I'm new to Java (more comfortable in C++).  I have:

import java.lang.ref.WeakReference;
import java.util.ArrayList;
import java.util.Iterator;

public abstract class Value {
    static public interface Listener {
        public abstract void valueChanged(Value val);
    };

    public Value(int init_val) {
        mVal = init_val;
        mListeners = new ArrayList<WeakReference<Listener>>();
    }

    public int getValue() { return mVal; }

    public void setValue(int val) {
        mVal = val;
        Iterator<WeakReference<Listener>> iter = mListeners.iterator();
        while (iter.hasNext()) {
            Listener listen = iter.next().get();
            if (listen == null)
                iter.remove();
            else
                listen.valueChanged(this);
        }
    }

    public void addListener(Listener listen) {
        cleanListeners();
        mListeners.add(new WeakReference<Listener>(listen));
    }

    private void cleanListeners() {
        Iterator<WeakReference<Listener>> iter = mListeners.iterator();
        while (iter.hasNext()) {
            if (iter.next().get() == null)
                iter.remove();
        }
    }

    private int mVal;
    private ArrayList<WeakReference<Listener>> mListeners;
};

I'm wondering if there are technical, style, or common practice issues, specifically relating to:

Is this a good and correct way to use WeakReferences, or is there a simpler way?  I don't want the Value to prevent its listeners from getting cleaned up, or to manually break links when a listener is no longer needed by the UI/calculations/whatever.
Use the interface List<...> or the actual implementation type ArrayList<...> when declaring the member mListeners?
Initialize mListeners with an empty list at the member declaration, or in the constructor?

Other comments and suggestions are welcome too.

Addressing your specific questions

  Is this a good and correct way to use WeakReferences, or is there a simpler way? I don't want the Value to prevent its listeners from getting cleaned up, or to manually break links when a listener is no longer needed by the UI/calculations/whatever.

See "Pros and Cons of Listeners as WeakReferences" on StackOverflow.  BegemoT's answer explains how using weak references changes the semantics. To prevent anyone from being surprised when using an anonymous class listener (which are common in UI frameworks like SWT), I recommend clearly documenting this semantic on the addListener method. Also, in my experience, weak references are not as well known by many developers which further reiterates the need for documentation.

There's other fascinating corners to explore in weak references including WeakHashMap, which uses a ReferenceQueue to cleanup, as discussed another SO question with some facinating links.  If you dive into WeakHashMap (perhaps wrapped as a Set using Collections.newSetFromMap) at some point, as noted, Guava (Google's common java library) makes a case for using their MapMaker instead.

Because of their relative obscurity and behavior depending on when the GC will run, I personally shy away from weak references and use them sparingly.

  Use the interface List<...> or the actual implementation type ArrayList<...> when declaring the member mListeners?

See "Type List vs type ArrayList in Java" on StackOverflow.  Declaring to the interface (List<...>) is to be preferred.

  Initialize mListeners with an empty list at the member declaration, or in the constructor?

See "Should I initialize variable within constructor or outside constructor" on StackOverflow.  Where you can, I recommend initializing at declaration for clarity.

Additional Thoughts

Java already has an implementation for the Observer pattern in java.util.Observable.  This has found to be wanting in that you have to extend Observable.  As suggested in "Alternative to Java's Observable class?", Guava's EventBus is a good alternative.  I've used it before and recommend it for its ease of use and aide in decoupling code.
Declare fields as final where you can. Use final fields where you can to help prevent potential issues in mutability.  Qualify mListeners as final.
I was surprised to see the fields at the bottom of the class.  For customary readability, fields are typically declared prior to the constructors.


Using  element instead of  to get clickable rows I want to create clickable rows in an html table. Each row should be a link leading to a different page. I can of course add onclick handler, but that will create kind of a crappy UI - poor accessibility, no keyboard navigation, no right/middle click...
I found a better way to do it. Instead of using normal table element, force divs to act as different table tags through css display rules. And for rows, use a-s, with appropriate href attribute.
Here's what I mean:

  .table {
    display: table;
    border-collapse: collapse;
    font-family: sans-serif;
    width: 100%;
  }
  .thead {
    display: table-header-group;
  }
  .tbody {
    display: table-row-group;
  }
  .td, .th {
    display: table-cell;
    padding: 5px 10px;
    text-align: center;
  }
  .tr {
    display: table-row;
  }
  .thead > .tr {
    background: #ccc;
    color: #555;
  }
  a.tr {
    text-decoration: none;
    color: black;
  }
  a.tr:hover {
    background: #eee;
    cursor: pointer;
  }

ID

Name

524

John Smith

331

Miles Corner

Note how rows are clickable, you can tab through them and there is a nice tooltip showing link you will go to. Much better than onclick.
If I use a framework like React, I don't even have to replace all elements with div-s. I can just add  tags instead of
s and it will work.
My question is, what's the catch? What am I missing? Is there some accessibility, browser compatibility, or other reason not to do this?

If it's a table, make it a <table>. Write a traditional <tbody> containing <tr>, in turn containing <td>, in turn containing individual <a>. Then the text will be links. This is simpler, doesn't require any fancy CSS, and better communicates your intent to browsers.
All of the usual keyboard and mouse functionality will be preserved.


Setting variables from a text file I'm new to PHP, having coded in Perl and ColdFusion years ago.  In the process of trying to familiarize myself with my new job, I have found some code that would neither seem efficient nor logical, although it does execute properly.

I would like to get feedback about whether one particular piece of code is poorly written, or whether there is in fact a reason it was done this way that is not apparent to someone new to PHP.

The code reads in the contents of a text file, and then sets variables from that file. The input file contains customer records, with fields/columns separated by commas and rows separated by line breaks. After the variables are set, they are sliced and diced in various ways, and at the end of each foreach block, a row is appended to another text file.

The data in the input text file might look something like this:

123456789,MOSName,Bob,Jones,2012 Parakeet Lane
999999999,OtherMOSName,Samantha,Smith,1212 Whatever Street
236751235,YetAnotherMOSName,Tom,Baker,555 Blahblah Boulevard

… and so on.

The below code only includes what is essential to understand the question.

$openit = file('thedirectory/thefilename.txt');

foreach($openit as $values) {

  //acct[0]
  $acct = explode(',', $values, 2);
  //mos[1]
  $mos = explode(',', $values, 3);
  //f_name[2]
  $f_name = explode(',', $values, 4);
  //l_name[3]
  $l_name = explode(',', $values, 5);
  //addr[4]
  $addr = explode(',', $values, 6);

...and so on, for 26 variables.

My inclination would be to simply explode the string once for each row, and place each value in a variable, like:

list($acct, $mos, $f_name, $l_name, $addr) = explode(",", $values);

But instead each variable is being set as an array of N items, where N is the limit attribute of explode, and where the final item includes the entire rest of the string.  Then the actual piece of data is being referenced in the rest of the file using the element of that array where the relevant info is actually stored.  So, acct would be referenced later on in the code as $acct[0], mos would be referenced as $mos[1], etc.

Is this as inefficient and illogical as it seems?  Or what am I missing here? If it is indeed an odd way of doing things, what would have been the motivation to do it this way?

Is this as inefficient and illogical as it seems?

Yes- as others have pointed out in comments, it seems to be somewhat poorly written. Not only is it difficult to read (as evidenced by your question here), but it isn't as efficient as it could be- each variable (e.g. $acct, $mos) is an array -and each sequential array grows in size! This is quite wasteful - see this playground example for an illustration.

If there are 26 fields per row, that means 376 elements* across 26 arrays. Storing each value as a string (or cast numbers to integer/float values) would dramatically decrease the amount of memory used. While the memory would likely be reclaimed between iterations, it would still make quite a difference.

  If it is indeed an odd way of doing things, what would have been the motivation to do it this way?

That would be difficult for us to answer without knowing the history of the team, code, etc. Perhaps the original implementor(s) didn't know any other technique for abstracting those values.

  My inclination would be to simply explode the string once for each row, and place each value in a variable, {and use list()}

That sounds like a better choice, though if there truly are 26+ values in each row, that line of code would get very lengthy. Perhaps a good choice would be to define a mapping of indexes like below, or just assert (/use error handling) to ensure the expected data exists in each line. Or, as Claudio mentioned in a comment str_getcsv() could also be utilized to simplify things here.

const FIELD_MAPPING = array(
    'acct',
    'mos',
    'f_name',
    'l_name',
    ...
);

It is unclear how the variables are used but presumably they are passed as parameters to a function/method call. If that is the case, then call_user_func_array() could be used, or if PHP 5.6+ is used, then call the function directly and pass the parameters using the spread/splat operator.

*Triangle series sum of 26 (351) + 26 - 1 (since each array except the last one has an extra element (the remaining fields separated by commas)


Determine whether a magazine contains the words needed for a ransom note This is a website Question on Hackrrank called Hash Tables: Ransom Note:
Given the words in the magazine and the words in the ransom note, print "Yes" if we can replicate the ransom note exactly using whole words from the magazine; otherwise, print "No".
Here is an example input:
6 4
give me one grand today night
give one grand today

Output: Yes
And another:
6 5
two times three is not four
two times two is four

Output: No

def checkMagazine(magazine, note):

    #Creating 2 Empty Dictionaries for the "Magazine" and the "Note" then filling them up
    UniqueWordsMag = set(magazine)
    UniqueCountMag = [0]*len(UniqueWordsMag)
    UniqueWordDictMag = dict(zip(UniqueWordsMag, UniqueCountMag))

    UniqueWordsNote= set(note)
    UniqueCountNote = [0]*len(UniqueWordsNote)
    UniqueWordDictNote = dict(zip(UniqueWordsNote, UniqueCountNote))

    for i in magazine:
        if i in list(UniqueWordDictMag.keys()):
            UniqueWordDictMag[i] += 1

    for i in note:
        if i in list(UniqueWordDictNote.keys()):
            UniqueWordDictNote[i] += 1

    #Checking for existance in the magazine then checking for the correct count, print no if it does not fulfil conditions
    Success = False
    DesiredCount = len(note)
    Count = 0

    for index,i in enumerate(UniqueWordsNote):
        if i in list(UniqueWordDictMag.keys()):
            if UniqueWordDictNote[i] <= UniqueWordDictMag[i]:
                Count += UniqueWordDictNote[i]
            else:
                break
        else:
            break

    if Count == DesiredCount:
        Success = True
        print("Yes")
    else:
        print("No")

It's called from this main program, that's provided by the challenge:

def main():
    mn = input().split()

    m = int(mn[0])

    n = int(mn[1])

    magazine = input().rstrip().split()

    note = input().rstrip().split()

    checkMagazine(magazine, note)

if __name__ == "__main__":
    main()

My code is currently taking too long on some lists (e.g. lists of size 30,000 or more).  Are there any optimisations I can make to make this a bit more legible and faster?

You're making it enormously complicated. This is a job for Counter:
from collections import Counter

def checkMagazine(magazine, note):
    print('No' if Counter(note) - Counter(magazine) else 'Yes')