Mapping DNA nucleotides into two-dimensional coordinates I am the author of this package that turns DNA sequences into two dimensional visualizations. DNA, for the unaware, consists of four letters (A, T, G, and C). To convert the sequence, I am using this code:

import numpy as np

def transform(sequence):
    running_value = 0
    x, y = np.linspace(0, len(sequence), 2 * len(sequence) + 1), [0]
    for character in sequence:
        if character == "A":
            y.extend([running_value + 0.5, running_value])
        elif character == "C":
            y.extend([running_value - 0.5, running_value])
        elif character == "T":
            y.extend([running_value - 0.5, running_value - 1])
            running_value -= 1
        elif character == "G":
            y.extend([running_value + 0.5, running_value + 1])
            running_value += 1
        else:
            y.extend([running_value] * 2)
    return list(x), y

Note that seq may be a very long sequence, and this transformation process may be done (hundreds of) thousands of time.

What are some things I could do to improve performance?

Edit:

The else clause is the because some DNA sequences have ambiguity (i.e. non ATGC characters) which must be accounted for as horizontal lines.
sequence may safely assumed to be a string. For example, "GATTACA" is a valid DNA sequence that should return ([0.0, 0.5, 1.0, 1.5, 2.0, 2.5, 3.0, 3.5, 4.0, 4.5, 5.0, 5.5, 6.0, 6.5, 7.0],
[0, 0.5, 1, 1.5, 1, 0.5, 0, -0.5, -1, -0.5, -1, -1.5, -1, -0.5, -1]).
By "long", I mean anywhere from thousands to billions. That being said, the most common use case likely will involve fewer than ten thousand characters, although this process may be done to thousands of such sequences. For context, I am currently running this function over 400,000 sequences with a median length of 800 characters.

First of all I'd like to say that your code is fast. From studying it, the major bottlenecks that I've found come from the input being a string, and from converting to numpy arrays and back.

Since what you're doing is a kind of cumulative sum with some alterations, I'd go with numpy.cumsum. It's a bit tricky to get everything in place, but the calculations of the actual sequence is actually not the major bottleneck. From what I've seen, the solution I came up with is about twice as fast, and 75% of the time is used to convert the string input to something useful, which in this case is a numpy array representing the character index in the string "ATCG" for each character in the input string.

def transform_fast(seq, d):
    l = len(seq)
    x = np.linspace(0, l, 2 * l + 1, dtype = np.float32)
    y = np.zeros(2* l + 1, dtype = np.float32)

    atcg = np.array([d[x] for x in seq], dtype = np.int8)

    a = (atcg == 1).astype(np.int8)
    t = (atcg == 2).astype(np.int8)
    c = (atcg == 4).astype(np.int8)
    g = (atcg == 8).astype(np.int8)
    ac = a - c
    tg = -t + g

    cum_sum = np.concatenate((
        np.array([0]),
        np.cumsum(tg[:-1])
    ))

    y[1::2] = cum_sum + 0.5*(ac + tg)
    y[2::2] = cum_sum + tg

    return x, y

In the function above, the argument d is a dictionary which looks like:

d = {
    "A": 1, "T": 2, "C": 4, "G": 8,
    "U": 0, "W": 0, "S": 0, "M": 0,
    "K": 0, "R": 0, "Y": 0, "B": 0,
    "D": 0, "H": 0, "V": 0, "N": 0, "Z": 0
}

The only difference compared to your solution is that this returns numpy arrays rather than lists. If this doesn't suit your use case, you can convert them to lists. However, this will slow down the overall performance significantly.

The code above also does not handle the non-ATCG characters in the input string. However, I think that you'll be able to follow the code and implement that in a similar fashion.

Benchmark with random input string of length 10^3
transform(seq): 0.45ms
transform_fast(seq): 0.24ms
Speedup factor: 1.82

Benchmark with random input string of length 10^6
transform(seq): 388.42ms
transform_fast(seq): 133.40ms
Speedup factor: 2.91

EDIT: I found that the np.fromiter(map(...)) that was previously used was unnecessary, and replaced it. This increased performance, benchmarks have been updated. I also found that -t + g was used thrice, and saved the result to a variable. I also switched x and y to be np.float32 arrays, as that improved performance slightly.

It should be noted that the approach above uses more memory than your original approach, which could be a problem for large enough sequences. On my machine (16GB RAM) I couldn't get the result for a sequence of length \$10^8\$, as I ran out of memory.

For the sake of curiosity, here are the benchmarks for when the input string is parsed into a numpy array before transform_fast, which could be used if the input sequences are reused often (these benchmarks haven't been updated, but should remain roughly the same):

Benchmark with random input string of length 10^3
transform(seq): 0.42ms
transform_fast(seq): 0.13ms
Speedup factor: 3.15

Benchmark with random input string of length 10^6
transform(seq): 380.23ms
transform_fast(seq): 37.23ms
Speedup factor: 10.21


Elegant way to do the dataframe operations twice I am trying to do certain operations in dataframe as shown in my code below. Though this works fine.However I am repeating the same statements twice or following a dirty approach to perform the operations twice. Can you help me to make it efficient and elegant?

I have a dataframe df in which I have to apply certain rules (see at the bottom PEEP_con and Fio2_con). PEEP_con has to be applied only to records with item_name == PEEP (df_P) and FiO2_con to item_name == Fio2 (df_F)

Before I apply the final conditions, I do certain preprocessing tasks which is 100 pc same for both the conditions but has to be done once for df_P and df_F . Hope this helps

Please find my code below (credit to SO users for help)

df = pd.read_csv('sample_data.csv')
df['charttime'] = pd.to_datetime(df['charttime'])
df = df.loc[df.value.mod(1).eq(0)]  # removes records with values like 5.4,6.1 etc

# I am creating two dataframes here. Is it possible to do this on the fly without creating two dataframes
df_P = df[df['item_name'] == 'PEEP']
df_P['value'] = np.where(df_P['value'] < 5, 5, df_P['value'])
df_F = df[df['item_name'] == 'Fi02/100']

# for the first time, I manually keep it to df_P and later I change it to df_F manually
dfa = df_F

# below operations is same for both the dataframes
m = dfa.assign(date=dfa.charttime.dt.date).groupby(['date',dfa.value.ne(dfa.value.shift()).cumsum()]).first().reset_index(drop=True)
c = pd.to_timedelta(24,unit='h')-(m.charttime-m.charttime.dt.normalize())
m['total_hr'] = m.groupby(['subject_id'])['charttime'].diff().shift(-1).fillna(c).dt.total_seconds()/3600
final_df = m.assign(date = m.charttime.dt.date)
gone_hr = final_df[final_df['total_hr'] > 1]
lone_hr = final_df[final_df['total_hr'] < 1]
min_gone_hr_data = gone_hr.groupby(['subject_id','date'])['value'].min().reset_index()
min_lone_hr_data = lone_hr.groupby(['subject_id','date'])['value'].min().reset_index()
op1 = gone_hr.merge(min_gone_hr_data, on = ['subject_id','date','value'],how = 'inner')
op2 = lone_hr.merge(min_lone_hr_data, on = ['subject_id','date','value'],how = 'inner')
op = (op1.set_index(['subject_id','date']).combine_first(op2.set_index(['subject_id','date'])).reset_index())

# This condition is only for `df_P`
PEEP_con = lambda x: (x.shift(2).ge(x.shift(1))) & (x.ge(x.shift(2).add(3))) & (x.shift(-1).ge(x.shift(2).add(3)))

# This condition is only for `df_F`
Fi02_con = lambda x: (x.shift(2).ge(x.shift(1))) & (x.ge(x.shift(2).add(20))) & (x.shift(-1).ge(x.shift(2).add(20)))

# this field should only be applied for df_P
op['fake_VAC'] = op.groupby('subject_id')['value'].transform(PEEP_con).map({True:'fake VAC',False:''})

# this field should only be applied for df_F
op['Fi02_flag'] = op.groupby('subject_id')['value'].transform(Fi02_con).map({True:'VAC',False:''})

I request your inputs in making this more elegant and efficient. Though suggestions w.r.t to logic are welcome, I am mostly looking for ways to optimize the code structure

Currently I generate two csv file output (one for PEEP and other for Fi02). Instead I would like to have both the columns Fi02_flag and fake_VAC in the same csv file (one final dataframe)

You're indeed doing a lot of stuff twice. The first step is recognizing that you have a problem :D

The following collection of functions could be used to decrease the repetition:

def load_df(item_name):
   return df[df['item_name'] == item_name]

def get_hr_op(predicate):
   """ Predicate is a lambda """
   hr = final_df[predicate(final_df['total_hr'])]
   min_hr = hr.groupby(['subject_id','date'])['value'].min().reset_index()
   op = hr.merge(min_hr, on = ['subject_id','date','value'],how = 'inner')
   return op.set_index(['subject_id','date'])

def make_con(add_qty):
   def con(x):
      return (
         x.shift(2).ge(x.shift(1))
      ) & (
         x.ge(x.shift(2).add(3))
      ) & (
         x.shift(-1).ge(
            x.shift(2).add(add_qty)
         )
      )
   return con

def apply_field(name, title, add_qty):
   con = make_con(add_qty)
   op[name] = op.groupby('subject_id')['value'].transform(con).map({True:title,False:''})

You get the idea. Basically, functions are your friend. There may be some fancier way of cleaning this up using Pandas-specific stuff, but this is the Python way.


Hex to Double converter I stumbled on this SO question which was asking about a way to convert larger hex values to a positive numeric value:

?Val("&H8000")
 -32768
Val("&HFFFF")
 -1

My answer involved iterating the string digits one by one, and computing their respective value into the result:

Function ConvertHex(ByVal value As String) As Double

    If Left(value, 2) = "&H" Then
        value = Right(value, Len(value) - 2)
    End If

    Dim result As Double

    Dim i As Integer, j As Integer
    For i = Len(value) To 1 Step -1

        Dim digit As String
        digit = Mid$(value, i, 1)

        result = result + (16 ^ j) * Val("&H" & digit)
        j = j + 1

    Next

    ConvertHex = result

End Function

It works, but I can't help thinking I've done something stupidly over-complicated for something that should be pretty simple.

There's a better way, isn't there?

From what I can tell a string starting with &H is a hex literal.

There exists a number of conversion functions that can convert an expression to the desired type.

So it should simply be, depending on desired type:

Function ConvertHex(ByVal value As String) As Currency
    Dim result As Currency
    result = CCur(value)

    If result < 0 Then
        'Add two times Int32.MaxValue and another 2 for the overflow
        'Because the hex value is apparently parsed as a signed Int64/Int32
        result = result + &H7FFFFFFF + &H7FFFFFFF + 2
    End If

    ConvertHex = result
End Function

Currency vs Double

Maximum accurately representable positive integer value :

&H0020000000000000 (9,007,199,254,740,992) for Double (IEEE 754 binary64)
&H000346DC5D638865 (922,337,203,685,477) for Currency

So why use Currency over Double when the latter works for a larger range of integers?

Currency is always accurate. If we overflow a Currency value we get an error. If we overflow the maximum representable integer value of a double we get an approximate integer value:

Dim doubleMax As Double
Dim doubleAfter As Double
doubleMax = CDbl("&H0020000000000000")
doubleAfter = doubleMax + 1

MsgBox "Double before: " & Format(doubleMax, "#") & vbNewLine & "after: " & Format(doubleAfter, "#")

Dim currencyMax As Currency
Dim currencyAfter As Currency
currencyMax = CCur("&H000346DC5D638865")
currencyAfter = currencyMax + 1

MsgBox "Currency before: " & Format(currencyMax, "#") & vbNewLine & "after: " & Format(currencyAfter, "#")

The output of this example is:

Double before: 9007199254740990
after: 9007199254740990

And then a run-time error '6': Overflow which is great if you want to avoid rounding errors. Now MSDN claims Double is

  stored as IEEE 64-bit (8-byte) floating-point number

but if you've read anything about the IEEE 754 binary64 you should be a bit surprised about the output from the example. The actual maximum is &H00038D7EA4C68000 (1,000,000,000,000,000).


Passing data to HTML template How can I pass data from database to my HTML template without putting PHP code in my HTML file.

Thanks.

This is my controller

public function index($aRequest)
{
    $oModel = new todoModel();
    $aTodoList = $oModel->getTodoList();
    return array('type' => 'view', 'view' => 'view/index.tpl', 'data' =>
    array('aTodoList' => $aTodoList));
}

my Model

public function getTodoList()
{
    $aTodoList = [];
    $this->oConnection->setAttribute(PDO::ATTR_ERRMODE,
    PDO::ERRMODE_EXCEPTION);

    $sQuery = 'SELECT * FROM t_todolist';
    foreach ($this->oConnection->query($sQuery) as $aTodo) {
        array_push($aTodoList, array(
            'sequence'    => $aTodo['sequence'],
            'todo'     => $aTodo['todo']
        ));
    }
    print_r($aTodoList);
    return $aTodoList;
}

my tpl -(this is working but it has php code inside)

<?php
foreach ($aTodoList as $aTodo) {
?>

<?php echo $aTodo['todo'];?>  update delete

<?php
}
?>

<button id="todo_create">create</button>

First, let me review some of of the code you have

public function getTodoList()
{
    return $this->oConnection->query('SELECT * FROM t_todolist')->fetchAll();
}

This is actually all the code you need for this task. Just one line. Thanks PDO being not an ordinary database driver like mysqli. You can check other PDO wonders in my article

There is also one important issue that should be fixed in the controller

public function index($aRequest)
{
    $connection = new Database; // whatever you have to supply a database connection
    $oModel = new todoModel($connection);
    $aTodoList = $oModel->getTodoList();
    return ['type' => 'view', 'view' => 'view/index.tpl', 'data' =>
    ['aTodoList' => $aTodoList]];
}

You may check the above article in order to learn why it is very important to create a database connection only once instead of making each model to create its own.

Now, to the template.

First off, there is absolutely nothing wrong with having some PHP code in a template. PHP is a template language after all, and a good one. All you need is to keep an eye on your template and make sure that all PHP code does in it is actually related to the output only. You can only make it a bit prettier and safer

<?php foreach ($aTodoList as $aTodo): ?>

       <?= htmlspecialchars($aTodo['todo']) ?>
       update
         delete

<?php endforeach ?>

However, to answer your question literally, you will need a dedicated template engine, like Twig. Twig has many good features compared to raw PHP. One of them is autoescaping. There is one critical flaw in your current template, namely

<?php echo $aTodo['todo'];?>

The moment a malicious user will put some bad Javascript in the todo description, your site will be hacked. So you have to escape HTML when echoing a variable.

Unlike PHP, Twig escapes your values automatically, so a code

{% for aTodo in aTodoList %}

        {{ aTodo.todo }}
        update
          delete

{% endfor %}

will be automatically protected from XSS


Console application menu Sometimes I need to write a simple Java console applications and menu look:

do {
    System.out.println("please select menu item");
    System.out.println("1-create, 2-remove, 3 - rename");
    int select;
    Scanner scanner = new Scanner(System.in);
    select = scanner.nextInt();

    switch (select) {
        case 1:
            System.out.println("please select menu item");
            System.out.println("1-image, 2-txt");
            select = scanner.nextInt();
            if (select == 1) fileHolder.create("image");
            else if (select == 2) fileHolder.create("image");
            else System.out.println("invalid menu item");
            break;
        case 2:
            System.out.println("please select menu item");
            System.out.println("1-file from pc1, 2-file from pc2, 3-file from pc3");
            select = scanner.nextInt();
            if (select == 1) fileHolder.removeFile("pc1");
            else if (select == 2) fileHolder.removeFile("pc2");
            else if (select == 3) fileHolder.removeFile("pc3");
            else System.out.println("invalid menu item");
            break;
        case 3:
            System.out.println("please type new name");
            String newName = scanner.next();
            fileHolder.rename(newName);
            break;
    }

} while (select != 4);

But I believe that it is not the best choice. I believe that I should use any pattern to improve my code. By description - the state pattern is applicable for my situation but I have no idea how I can use this in this situation. The menu should show the respective menu according to the internal state.

First of all, there are at least three buggy things:

select variable is declared inside the do... while loop, so the condition in while is checked against another one (probably, a field in the class?). So, the loop won't work as expected!
copy-paste is evil. The calls fileHolder.create("image"); are repeated in two different conditionals. The "txt" option is not used. Please pay more attention to the things you write and do not copy-paste such elementary things.
the Scanner is not closed after its usage, which is desirable, as for any other items that use I/O.

For the rest, the code seems quite straightforward, rigid and repetitive, hence difficult to maintain. It contains many if - else if clauses that smell bad.

Of course, the whole story may be refactored with patterns usage. But I don't think that the state pattern suits well here. It'd be better refactored with the command pattern.

All the menu actions (create, remove, rename and their dependent ones) may be wrapped into separate objects, which associate the expected keys (1,2,3..), the text and other necessary stuff and sugar.

For example, a (very short) version of Menu class could be like this:

class Menu {

  private final String action;
  private final Consumer<String> consumer;

  Menu(String action, Consumer<String> consumer) {
    this.action = action;
    this.consumer = consumer;
  }

  void execute() {
    this.consumer.accept(action);
  }
}

And it would be initialized and used:

Menu create = new Menu("create", (a) -> fileHolder.create(a));
// to trigger the execution:
create.execute();

But for this very concrete and short example of code I would consider decomposing it into patterns as a sort of overkill.

There is some job to do about the original straightforward approach, so let's just try to improve it.

The input numbers are read several times using the Scanner object. They should also be validated, so a dedicated method would be very useful:

// int minValue may also be defined if necessary
private static int askUserForNumberInput(Scanner scanner, String prompt, int maxValue) {
  System.out.println("please select menu item");
  System.out.println(prompt);
  int value = scanner.nextInt();
  while (value < 1 || value > maxValue) {
    System.out.println("invalid menu item, please try again");
    // java.util.InputMismatchException should also be caught
    // to intercept non-numeric input
    value = scanner.nextInt();
  }
  return value;
}

The frame for the main logic would look like this:

try (Scanner scanner = new Scanner(System.in)) {
  final int mainMenuSelection = askUserForNumberInput(scanner, "1-create, 2-remove, 3 - rename", 3);
  switch (mainMenuSelection) {
    // cases
  }
}

And there is not a single if - if else in the cases any more:

case 1: {
  String[] createActions = { "image", "txt" }; // to be extracted to constants
  int createOption = askUserForNumberInput(scanner, "1-image, 2-txt", 2);
  fileHolder.create(createActions[createOption - 1]);
  break;
}
case 2: {
  int removeIndex = askUserForNumberInput(scanner, "1-file from pc1, 2-file from pc2, 3-file from pc3", 3);
  fileHolder.removeFile("pc" + removeIndex);
  break;
}
case 3: {
  System.out.println("please type new name");
  String newName = scanner.next();
  fileHolder.rename(newName);
  break;
}


Restructuring loop logic and its effect on allocations I came across in a code base # TODO this seems very clumsy to duplicate the loop code like this? so I figured I'd give it a shot as a learning exercise. I'd like some feedback on what I came up with, particularly:

Is this proper idiomatic Julia, or close? Or am I just writing Python in Julia?
What explains the difference in allocations between the two approaches?
(Arguments used: [1,1], octaves=10)

Old: 0.000013 seconds (63 allocations: 1.062 KiB)
New: 0.000008 seconds (41 allocations: 1.500 KiB)

Thanks!

Original function:

# coords is [x] or [x, y] or [x, y, z] or [x, y, z, w]
function _octaves(coords::Array{T, 1} where T <: Real;
        octaves::Int = 1,
        persistence=1.0)
    total     = 0.0
    frequency = 1.0
    amplitude = 1.0
    maxval    = 0.0
    # TODO this seems very clumsy to duplicate the loop code like this?
    l = length(coords)
    if l == 1
        for i in 1:octaves
            total += simplexnoise(coords[1] * frequency) * amplitude
            maxval += amplitude
            amplitude *= persistence
            frequency *= 2
        end
    elseif l == 2
        for i in 1:octaves
            total += simplexnoise(coords[1] * frequency, coords[2] * frequency) * amplitude
            maxval += amplitude
            amplitude *= persistence
            frequency *= 2
        end
    elseif l == 3
        for i in 1:octaves
            total += simplexnoise(coords[1] * frequency, coords[2] * frequency, coords[3] * frequency) * amplitude
            maxval += amplitude
            amplitude *= persistence
            frequency *= 2
        end
    elseif l == 4
        for i in 1:octaves
            total += simplexnoise(coords[1] * frequency, coords[2] * frequency, coords[3] * frequency, coords[4] * frequency) * amplitude
            maxval += amplitude
            amplitude *= persistence
            frequency *= 2
        end
    end
    return total / maxval
end

And my refactor:

function _octaves(coords::Array{T, 1} where T <: Real;
        octaves::Int = 1,
        persistence=1.0)
    total     = 0.0
    frequency = 1.0
    amplitude = 1.0
    maxval    = 0.0

    for _ in 1:octaves
        inputs = coords .* frequency
        total += simplexnoise(inputs...) * amplitude

        maxval += amplitude
        amplitude *= persistence
        frequency *= 2
    end

    return total / maxval
end

This is very idomatic, in principle.  The loop refactoring is exactly what I would have done.

Here's some other suggestions:

const Coords{T} = NTuple{N, T} where N

function _octaves(coords::Coords{T}; octaves::Int=1, persistence::T=one(T)) where {T<:Real}
    if octaves < 1
        # prevent zero division
        throw(DomainError(octaves, "`octaves` must be at least 1!"))
    end

    total = zero(T)
    maxval = zero(T)

    for octave in 1:octaves
        p = octave - 1
        frequency = 2 ^ p
        amplitude = persistence ^ p

        maxval += amplitude

        scaled_coords = coords .* frequency
        total += simplexnoise(scaled_coords...) * amplitude
    end

    return total / maxval
end

Using one and zero to ensure type stability (a very important concept, which you should read about if you don't know it!)
Replacing some intermediate updates by explicit powers.  The calculation of frequency is trivial to the compiler, since it is an integer power of 2.  amplitude might do some unnecessary work; I think it is clearer that way, and not worth the microoptimisation, but you'd have to test that for your use case.

The choice of type for coords is really determined by the rest of the code.  If this is only ever going to be used for coordinates of bounded dimension, I'd very much prefer a tuple as I wrote above.  If you use vectors all the way down (this looks a bit like signal processing?), then staying with Coords{T} = Vector{T} is easier.

While not part of the question, it might be nicer to have simplenoise work on an iterable, not a varargs argument.  Possibly even as simplenoise(frequency, coords), then the allocation of scaled_coords is removed.

If there's any chance that the summation will overflow, take a look at Base.widen.  Also, you might want to ensure that octaves < 8 * sizeof(Int) or something like that, and add this to the check at the beginning.

If this is really the hot part of some code to be optimized, with octaves being constant over many calls, there's the possibility to write a @generated function to unroll the loop completely, but unless it is critical, it's not worth it.

As for the difference in benchmarking: first, you should do that with BenchmarkTools.jl, not @time.  Then, I haven't measured myself, but it's quite surely due to the intermediate inputs array you construct.


Rotating and printing an image I carefully used smart pointer to avoid memory leaks and dangling pointers, but someone says my code has memory leaks. Is it true? If so, how can I fix it?

#include <iostream>
#include <initializer_list>
#include <memory>

using namespace std;
using Ptr2D = unique_ptr<unique_ptr<int[]>[]>;
using IniList = initializer_list<int>;

void printImage(const Ptr2D& image, const IniList& il)
{
    for (auto& i : il)
    {
        for (auto& j : il)
        {
            cout << image[i][j] << ',';
        }

        cout << '\n';
    }
}

template<size_t SIZE>
void rotateImage(Ptr2D& image, const IniList& il)
{
    Ptr2D temp(new unique_ptr<int[]>[SIZE]);

    for (auto& i : il)
    {
        unique_ptr<int[]> subTemp(new int[SIZE]);

        for (auto& j : il)
        {
            subTemp[j] = 0;
        }

        temp[i] = move(subTemp);
    }

    for (auto& i : il)
    {
        for (auto& j : il)
        {
            temp[i][j] = image[SIZE - j - 1][i];
        }
    }

    for (auto& i : il)
    {
        for (auto& j : il)
        {
            image[i][j] = move(temp[i][j]);
        }
    }
}

int main()
{
    const size_t SIZE = 5;

    Ptr2D image(new unique_ptr<int[]>[SIZE]);

    IniList il = { 0, 1, 2, 3, 4 };

    for (auto& i : il)
    {
        unique_ptr<int[]> temp(new int[SIZE]);

        for (auto& j : il)
        {
            temp[j] = j + il.size() * i;
        }

        image[i] = move(temp);
    }

    cout << "******* original image ******\n";

    printImage(image, il);

    rotateImage<SIZE>(image, il);

    cout << "******* rotated image ******\n";

    printImage(image, il);
}

At a quick glance there doesn't appear to be any memory leaks:

valgrind ./mem_leak_test
==3914== Memcheck, a memory error detector
==3914== Copyright (C) 2002-2011, and GNU GPL'd, by Julian Seward et al.
==3914== Using Valgrind-3.7.0 and LibVEX; rerun with -h for copyright info
==3914== Command: ./mem_leak
==3914==
******* original image ******
0,1,2,3,4,
5,6,7,8,9,
10,11,12,13,14,
15,16,17,18,19,
20,21,22,23,24,
******* rotated image ******
20,15,10,5,0,
21,16,11,6,1,
22,17,12,7,2,
23,18,13,8,3,
24,19,14,9,4,
==3914==
==3914== HEAP SUMMARY:
==3914==     in use at exit: 0 bytes in 0 blocks
==3914==   total heap usage: 12 allocs, 12 frees, 296 bytes allocated
==3914==
==3914== All heap blocks were freed -- no leaks are possible
==3914==
==3914== For counts of detected and suppressed errors, rerun with: -v
==3914== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 4 from 4)

Valgrind appears to agree about the lack of memory leaks.

I would just use std::array instead of code such as unique_ptr<int[]> temp(new int[SIZE]);. You get the benefits of automatic memory cleanup at the end of the variables scope from std::array without as much complexity. The main reason I would go for std::array is because you appear to be mostly concerned with memory management and not ownership. However if your intent here is to clearly have single ownership then unique_ptr is a good choice.

std::array will reduce complexity because it contains information about its dimensions. This would completely remove the need for the initializer list for initializing the array. Unless you are going to do something more fancy with it, the initializer list is an overly complex way to iterate over all the indexes of an array.

Assuming we changed this to use std::array the other thing I would do differently is to return the rotated image from rotateImage as opposed to modifying a reference parameter. Fewer side effects tends to keep your code more conceptually simple.


Counting standard deviation from parsed XML file I wrote a method that calculates the average and standard deviation of sell currency exchange-course (ASK).  But I think that it isn't the easiest way to define such behavior.

In my opinion there is too much creating of BigDecimal values.
I have also doubts about using an ArrayList to get values in order to count standard deviation, maybe there is a better data structure to achieve that goal.

The method contains also counting average of buy currency exchange-course (BID in parsed file) but don't focus on it please.

package pl.parser.nbp;

import java.math.BigDecimal;
import java.math.RoundingMode;
import java.util.ArrayList;
import java.util.List;

import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;

import org.w3c.dom.Document;
import org.w3c.dom.Element;
import org.w3c.dom.Node;
import org.w3c.dom.NodeList;

public class Counting {
public void countAverageAndStandartDeviaton(String address){
    try
    {
        DocumentBuilderFactory df = DocumentBuilderFactory.newInstance();
        DocumentBuilder db = df.newDocumentBuilder();
        Document doc = db.parse(address);
        doc.getDocumentElement().normalize();

        NodeList nList = doc.getElementsByTagName("Rate");

        List<BigDecimal> listForStandartDeviation = new ArrayList<>();
        BigDecimal averageOfBid = new BigDecimal("0");
        BigDecimal averageOfAsk = new BigDecimal("0");
        BigDecimal divisor = new BigDecimal(nList.getLength());

        for (int temp = 0; temp < nList.getLength(); temp++) {

            Node nNode = nList.item(temp);
            if (nNode.getNodeType() == Node.ELEMENT_NODE) {

                Element eElement = (Element) nNode;

                BigDecimal valueForBidAverage = new BigDecimal(eElement.getElementsByTagName("Bid").item(0).getTextContent());
                BigDecimal valueForStandartDeviation = new BigDecimal(eElement.getElementsByTagName("Ask").item(0).getTextContent());
                averageOfBid = averageOfBid.add(valueForBidAverage);
                averageOfAsk = averageOfAsk.add(valueForStandartDeviation);
                listForStandartDeviation.add(valueForStandartDeviation);

            }
        }
        averageOfBid = new BigDecimal(averageOfBid.divide(divisor).toString()).setScale(4, RoundingMode.HALF_UP);
        averageOfAsk = new BigDecimal(averageOfAsk.divide(divisor).toString());
        System.out.println(averageOfBid + " - BID Average");

        BigDecimal sumStandartDeviation = new BigDecimal("0");
        for(int i = 0 ; i<listForStandartDeviation.size(); i++){
            BigDecimal valueFromList = new BigDecimal(listForStandartDeviation.get(i).toString());
            sumStandartDeviation = sumStandartDeviation.add((valueFromList.subtract(averageOfAsk)).pow(2));
        }
        sumStandartDeviation = sumStandartDeviation.divide(divisor);
        sumStandartDeviation = new BigDecimal(Math.sqrt(sumStandartDeviation.doubleValue()))
                .setScale(4, RoundingMode.HALF_UP);
        System.out.println(sumStandartDeviation +  " - ASK Standart Deviation");

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

Be careful with instation of same object multiple times for same use- as it may change in use. If and when the first instantiation will be disposed, however, depends. The Java runtime environment traces references, so as long as there are any references to an object in use, the object won't be disposed. Note that the dispose itself does not happen immediately, but sometimes later, the next time when the garbage collector is run. But that is independent from the semantics of the code discussed, thats only a question of memory and performance.
I would use a HashMap for the Data Structure to count deviation.
Such as this example:

  HashMap hm = new HashMap();
  // Put elements to the map
  hm.put("Zara", new Double(3434.34));
  hm.put("Mahnaz", new Double(123.22));
  hm.put("Ayan", new Double(1378.00));
  hm.put("Daisy", new Double(99.22));
  hm.put("Qadir", new Double(-19.08));


Creating and testing a Fraction class The exercise is as follows:

  Create a class called Fraction that can be used to represent the ratio of two integers.
  Include appropriate constructors, properties, and methods. If the denominator becomes
  zero, throw and handle an exception. Create an application class to test the Fraction class.

The program runs.  Will someone look over my code for this exercise?

Fraction.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace Part2
{
    public class Fraction
    {
        private int numerator;
        private int denominator;

        /// <summary>
        /// Initialize the fraction properties
        /// </summary>
        /// <param name="numerator">Upper number</param>
        /// <param name="denominator">Lower number</param>
        public Fraction(int numerator, int denominator)
        {
            this.Numerator = numerator;
            this.Denominator = denominator;
        }

        /// <summary>
        /// Access tot he numerator property
        /// </summary>
        public int Numerator
        {
            get { return this.numerator; }
            set { this.numerator = value; }
        }

        /// <summary>
        /// Access to the denominator property
        /// </summary>
        public int Denominator
        {
            get { return this.denominator; }
            set
            {
                if (value == 0)
                {
                    throw new Exception("0 denominator");
                }

                this.denominator = value;
            }
        }

        /// <summary>
        /// Return a string representation of a fraction
        /// </summary>
        /// <returns>Displayable attribute</returns>
        public override string ToString()
        {
            StringBuilder sb = new StringBuilder();
            sb.Append(this.Numerator + "/" + this.numerator);
            sb.Append(" or ");
            sb.Append(this.Numerator / this.Denominator);

            return sb.ToString();
        }
    }
}

Application.cs

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace Part2
{
    public class Application
    {
        /// <summary>
        /// Entry point of the program
        /// </summary>
        public static void Main(string[] args)
        {
            // Create a fraction
            Fraction f1 = new Fraction(1, 2);
            Console.WriteLine(f1);

            try
            {
                // Create another fraction
                Fraction f2 = new Fraction(2, 0);
                Console.WriteLine(f2);
            }
            catch(Exception e)
            {
                Console.WriteLine(e.Message);
            }

            Console.ReadKey();
        }
    }
}

I think your class is pretty much useless. If you have a class that represents a fraction, you should be able to do operations with it that you can do with a fraction, like addition, subtraction, multiplication and division (possibly using operator overloading). Another possible operation is fraction simplification.

Also, you shouldn't throw Exception, you should instead a custom type that inherits from Exception (called something like ZeroDenominatorException) and throw that.

And your Main() isn't very realistic. In real code, you wouldn't write code that you know will always throw an exception. So I think that your try should be around both of your new Fraction() statements.

Also, if you're going to have all the operations I have mentioned above, a better application to show that would be a very simple fraction calculator.

Something like:

Enter numerator:
1
Enter denominator:
4
Enter operation:
+
Enter numerator:
3
Enter denominator:
4
Result: 4/4
Simplifies result: 1/1


A simple register VM written in Rust I'm teaching myself Rust, and to do this I've written a toy register based virtual machine. I hope the code is easy to follow - I just want to know if there are any mistakes I am making with the language philosophy or if there are syntax tricks I could be using. For example, the program counter pc and the register fields reg_field are mutable. I feel like this is the right thing to do in this case as it better reflects processor architecture. But  of course, I might be wrong...

const NUMBER_OF_REGISTERS: usize = 3;

enum Instruction {
    Halt,
    Load { reg: usize, value: u16 },
    Swap { reg1: usize, reg2: usize, reg3: usize },
    Add { reg1: usize, reg2: usize, reg3: usize },
    Branch { offset: usize }
}

fn main() {
    let encoded_instructions = &[0x1110, 0x2100, 0x3010, 0x0];

    cpu(encoded_instructions);
}

fn cpu(encoded_instructions: &[u16]) {
    let mut pc = 0;
    let mut reg_field = [0; NUMBER_OF_REGISTERS];

    loop {
        let encoded_instruction = fetch(pc, encoded_instructions);
        let decoded_instruction = decode(encoded_instruction);

        match decoded_instruction {
            Some(Instruction::Load { reg, value })
                => load(reg, value, &mut reg_field, &mut pc),
            Some(Instruction::Swap { reg1, reg2, reg3 })
                => swap(reg1, reg2, reg3, &mut reg_field, &mut pc),
            Some(Instruction::Add { reg1, reg2, reg3 })
                => add(reg1, reg2, reg3, &mut reg_field, &mut pc),
            Some(Instruction::Branch { offset })
                => branch(offset, &mut pc),
            Some(Instruction::Halt)
                => { halt(&reg_field); break }
            None => break,
        }
    }
}

fn fetch(pc: usize, instructions: &[u16]) -> u16 {
    instructions[pc]
}

fn halt(register_field: &[u16]) {
    println!("{:?}", register_field[0]);
}

fn load(register: usize, value: u16, register_field: &mut [u16], pc: &mut usize) {
    register_field[register] = value;
    *pc += 1;
}

fn swap(reg1: usize, reg2: usize, reg3: usize, register_field: &mut [u16], pc: &mut usize) {
    register_field[reg3] = register_field[reg1];
    register_field[reg1] = register_field[reg2];
    register_field[reg2] = register_field[reg3];
    *pc += 1;
}

fn add(reg1: usize, reg2: usize, reg3: usize, register_field: &mut [u16], pc: &mut usize) {
    register_field[reg3] = register_field[reg1] + register_field[reg2];
    *pc += 1;
}

fn branch(offset: usize, pc: &mut usize) {
    *pc -= offset;
}

fn decode(encoded_instruction: u16) -> Option<Instruction> {
    let operator = encoded_instruction >> 12;
    let reg1 = ((encoded_instruction >> 8) & 0xF) as usize;
    let reg2 = ((encoded_instruction >> 4) & 0xF) as usize;
    let reg3 = (encoded_instruction & 0xF) as usize;
    let offset = (encoded_instruction & 0xFFF) as usize;
    let value = encoded_instruction & 0xFF;

    match operator {
        0 => Some(Instruction::Halt),
        1 => Some(Instruction::Load { reg: reg1, value: value }),
        2 => Some(Instruction::Swap { reg1: reg1, reg2: reg2, reg3: reg3 }),
        3 => Some(Instruction::Add { reg1: reg1, reg2: reg2, reg3: reg3 }),
        4 => Some(Instruction::Branch { offset: offset }),
        _ => None,
    }
}

Your code was very readable and easy to follow. Most of my suggestions center around trying to showcase some more Rust idioms and features.

A collection of registers in normally called a register file.
Since the constant for the number of registers is only used to declare the register file, create a type alias and use that. This sets you up nicely to make it a standalone type in the future.
I'm glad to see names for registers in Instruction, but maybe give them more meaningful names? Add is a great example - which registers are the inputs and which is the output?
You almost always want to derive Debug for a type. Copy and Clone are also very common. Implementing PartialEq will allow easier test writing.
There's a distinct lack of types and methods, overall the code feels pretty C-like. I'd associate functions with types, making them methods. For example, decode is the most obvious change to me.
Returning an Option from decode is iffy. Failing to decode an instruction doesn't seem like the absence of a value, it feels like a failure, which is normally reserved for Result.
When splitting match arms onto a different line from the pattern, use braces.
Consider inlining some of the instruction implementations into execute as many are only one line long anyway.
In a processor, it's very typical to automatically increment the program counter every instruction. Branch offsets take that automatic increment into account. This is nice here because it removes pc from all the non-branching methods. I just subtracted 1 when applying the jump, but it's probably better to fixup the offsets.
Create a type for sequences of instructions, I used Program. This will give you somewhere to hang fetch.

type RegisterFile = [u16; 3];

#[derive(Debug, Copy, Clone)]
enum Instruction {
    Halt,
    Load { reg: usize, value: u16 },
    Swap { reg1: usize, reg2: usize, reg3: usize },
    Add { reg1: usize, reg2: usize, reg3: usize },
    Branch { offset: usize }
}

impl Instruction {
    fn decode(encoded_instruction: u16) -> Option<Self> {
        let operator = encoded_instruction >> 12;
        let reg1 = ((encoded_instruction >> 8) & 0xF) as usize;
        let reg2 = ((encoded_instruction >> 4) & 0xF) as usize;
        let reg3 = (encoded_instruction & 0xF) as usize;
        let offset = (encoded_instruction & 0xFFF) as usize;
        let value = encoded_instruction & 0xFF;

        match operator {
            0 => Some(Instruction::Halt),
            1 => Some(Instruction::Load { reg: reg1, value: value }),
            2 => Some(Instruction::Swap { reg1: reg1, reg2: reg2, reg3: reg3 }),
            3 => Some(Instruction::Add { reg1: reg1, reg2: reg2, reg3: reg3 }),
            4 => Some(Instruction::Branch { offset: offset }),
            _ => None,
        }
    }

    fn execute(&self, registers: &mut [u16], pc: &mut usize) -> bool {
        match *self {
            Instruction::Load { reg, value } => {
                load(reg, value, registers);
            },
            Instruction::Swap { reg1, reg2, reg3 } => {
                swap(reg1, reg2, reg3, registers);
            },
            Instruction::Add { reg1, reg2, reg3 } => {
                add(reg1, reg2, reg3, registers);
            },
            Instruction::Branch { offset } => {
                branch(offset, pc);
            },
            Instruction::Halt => {
                halt(registers);
                return false;
            },
        }

        true
    }
}

fn halt(register_file: &[u16]) {
    println!("{:?}", register_file[0]);
}

fn load(register: usize, value: u16, register_file: &mut [u16]) {
    register_file[register] = value;
}

fn swap(reg1: usize, reg2: usize, reg3: usize, register_file: &mut [u16]) {
    register_file[reg3] = register_file[reg1];
    register_file[reg1] = register_file[reg2];
    register_file[reg2] = register_file[reg3];
}

fn add(reg1: usize, reg2: usize, reg3: usize, register_file: &mut [u16]) {
    register_file[reg3] = register_file[reg1] + register_file[reg2];
}

fn branch(offset: usize, pc: &mut usize) {
    *pc -= offset - 1;
}

struct Program<'a> {
    instructions: &'a [u16],
}

impl<'a> Program<'a> {
    fn fetch(&self, pc: usize) -> u16 {
        self.instructions[pc]
    }
}

fn cpu(program: Program) {
    let mut pc = 0;
    let mut registers = RegisterFile::default();

    loop {
        let encoded_instruction = program.fetch(pc);
        let decoded_instruction = Instruction::decode(encoded_instruction);

        match decoded_instruction {
            Some(instr) => {
                if !instr.execute(&mut registers, &mut pc) { break }
            }
            None => break,
        }

        pc += 1;
    }
}

fn main() {
    let encoded_instructions = Program { instructions: &[0x1110, 0x2100, 0x3010, 0x0] };

    cpu(encoded_instructions);
}


Absolute Values and Fractions $$
\frac{|x|-|y|}{1+|xy|}
$$

How can I reduce the amount of code without losing quality? What are your tips? Your comments on the structure, logic, in general, everything. How to make the type of data entered by the user determined automatically?

 using System;
 class taskTwo {

static void Main() {

    int x, y; // Declare variables
    double z, w;
    Console.WriteLine("Please, enter two the numbers");
   x = int.Parse(Console.ReadLine()); //The user enters
    y = int.Parse(Console.ReadLine());

    if (x == 0 && y == 0)//If two zeros are entered, then the numerator is
                                                            zero.
    {
        Console.WriteLine("The numerator and denominator can not be zero");
    }

    else {
        z = Math.Abs(x) - Math.Abs(y);//Know what the numerator is equal to
        if (z == 0)//If the numerator is zero, then ...
              {
                    Console.WriteLine("Numerator is zero, division is impossible");
                }
        else {
            w = z / (1 + Math.Abs(x) * Math.Abs(y)); //Final calculation
            Console.WriteLine("W = " + w); //Display the result on the screen
        }
    }

}}

There is no integer value for which this undefined. Having a zero in the numerator is a legal math operation that results as zero. having a zero in the denominator would result in an undefined result, but that concern is mitigated by the fact that the absolute value of the product of two integers will be greater than or equal to zero and then you add one to that product before doing the division. In other words, the smallest denominator that the equation could have is one which is legal. So you could remove the if statements

using System;

class TaskTwo {

    static void Main() {
        // Declare variables
        int x, y;
        double w;

        Console.WriteLine("Please, enter two the numbers");
        x = int.Parse(Console.ReadLine());
        y = int.Parse(Console.ReadLine());

        w=(Math.Abs(x) - Math.Abs(y))/ (1 + Math.Abs(x*Y));

        // Display the result on the screen
        Console.WriteLine("W = " + w);
       }
    }

This should suffice.


Importing of big files:right approach? I want to import in python some ascii file ( from tecplot, software for cfd post processing).
Rules for those files are (at least, for those that I need to import):
-The file is divided in several section
-Each section has two lines as header like:

VARIABLES = "x" "y" "z" "ro" "rovx" "rovy" "rovz" "roE" "M" "p" "Pi" "tsta" "tgen"
ZONE T="Window(s) : E_W_Block0002_ALL",  I=29,  J=17,  K=25, F=BLOCK

Each section has a set of variable given by the first line. When a section ends, a new section starts with two similar lines.
For each variable there are I*J*K values.
Each variable is a continous block of values.
There are a fixed number of values per row (6).
When a variable ends,the next one start in a new line.
Variables are "IJK ordered data".The I-index varies the fastest; the J-index the next fastest; the K-index the slowest. The I-index should be the inner loop, the K-index shoould be the outer loop, and the J-index the loop in between.

Here an example of data:

VARIABLES = "x" "y" "z" "ro" "rovx" "rovy" "rovz" "roE" "M" "p" "Pi" "tsta" "tgen"
ZONE T="Window(s) : E_W_Block0002_ALL",  I=29,  J=17,  K=25, F=BLOCK
-3.9999999E+00 -3.3327306E+00 -2.7760824E+00 -2.3117116E+00 -1.9243209E+00 -1.6011492E+00
[...]
0.0000000E+00 #fin first variable
-4.3532482E-02 -4.3584235E-02 -4.3627592E-02 -4.3663762E-02 -4.3693815E-02 -4.3718831E-02 #second variable, 'y'
[...]
1.0738781E-01 #end of second variable
[...]
[...]
VARIABLES = "x" "y" "z" "ro" "rovx" "rovy" "rovz" "roE" "M" "p" "Pi" "tsta" "tgen" #next zone
ZONE T="Window(s) : E_W_Block0003_ALL",  I=17,  J=17,  K=25, F=BLOCK

I am quite new at python and I have written a code to import the data to a dictionary, writing the variables as 3d numpy.array . Those files could be very big, (up to Gb).  How can I make this code faster?(or more generally, how can I import such files as fast as possible)?.

import numpy as *
from numpy.linalg import norm
import re

def vectorr(I, J, K):
    arr = empty((I*J*K, 3), int)
    arr[:,0] = tile(arange(I), J*K)
    arr[:,1] = tile(repeat(arange(J), I), K)
    arr[:,2] = repeat(arange(K), I*J)
    return arr

def all_data(pathfilename,NumberCol = 6):
    with open(pathfilename) as a:
        filelist = a.readlines()
        Count = 0
        data = dict()
        leng = len(filelist)
        Countzone = 0
        while Count < leng:
            strVARIABLES = re.findall('VARIABLES', filelist[Count])
            variables = re.findall(r'"(.*?)"',  filelist[Count])
            Countzone = Countzone+1
            data[Countzone] = {key:[] for key in variables}
            Count = Count+1
            strI = re.findall('I=....', filelist[Count])
            strI = re.findall(r'\d+', strI[0])
            I = int(strI[0])
            strJ = re.findall('J=....', filelist[Count])
            strJ = re.findall(r'\d+', strJ[0])
            J = int(strJ[0])
            strK = re.findall('K=....', filelist[Count])
            strK = re.findall(r'\d+', strK[0])
            K = int(strK[0])
            data[Countzone]['indmax'] = array([I, J, K])
            pr = prod(data[Countzone]['indmax'])
            lin = pr // NumberCol
            if pr % NumberCol != 0 :
                lin = lin+1
            vect = vectorr(I, J, K)
            for key in variables:
                init = zeros((I, J, K))
                for ii in range(0, lin):
                    Count = Count+1
                    temp = [ float(x.replace('D', 'E')) for  x in filelist[Count].split() ]
                    for iii in range(0, len(temp)):
                        init.itemset(tuple(vect[ii*NumberCol+iii]), temp[iii])
                        data[Countzone][key] = init
            Count = Count+1
    return data

There are two algorithmic aspects to your code which concern me. The most significant problem is that you read the entire file in to memory before you start processing the data:

filelist = a.readlines()

This is an inefficient way to do things with large input files.

You should instead be reading the data 1 line at a time and processing each line as you get it. This allows you to have a much smaller memory 'footprint', and it allows you to discard data that you do not need.... instead of your while loop while count < leng consider the approach:

for line in a:
    ....

This should reduce your memory significantly.

(Asker updated question....)

The second issue you should consider changing is using numpy arrays for the vectorr method as well. This array is about the same size as the final array and you are incrementally allocating memory for it. Doing it with a single initializer (perhaps vect = empty((I*J*K, 3)) ) would be worth considering.


React continuous polling I have a Notifications component in my React app, which continuously polls the server for new notifications. The app uses Redux for storing notifications. When a new notification is received, it is shown on screen. Here's what the code looks like:

import * as React from 'react'
import { connect } from 'react-redux'

interface ReduxProps {
    notifications: INotification[]
    fetchNotifications: () => any
}

interface IProps extends ReduxProps {}

class NotificationsWidget extends React.Component<IProps> {
    timer: number

    refreshNotifications = () => {
        this.props.fetchNotifications()
    }

    componentDidMount() {
        this.timer = window.setInterval(() => this.refreshNotifications(), 3000)
    }

    render() {
        // ... Show `this.props.notifications`.
    }
}

const mapStateToProps = (state: any) => ({
    notifications: state.notifications.items
})

export default connect(mapStateToProps, {fetchNotifications})(NotificationsWidget)

Is this the correct way to go about polling with a React/Redux app? Is there a better way to do polling, or is there an alternative way to push notifications to the browser without polling?

There are many ways you could implement polling but this one is valid enough for a lot of cases.  You could hide your timer off in a redux action somewhere if you were so inclined.

It's worth highlighting that you should clear out your timer when the Component unmounts.

componentWillUnmount() {
    window.clearInterval(this.timer)
}


Python program to find the topic with the most meeting time dedicated to it I must create a program for information regarding schedules and topics of meetings at work.
The data relating to the meetings are stored in an "M" matrix with 5 columns, in which the generic row [g, m, in, fin, arg] denotes the fact that a meeting has been scheduled for day "g" of month "m", from hours "in" to "fin" hours, on the topic "arg".
Write a function "common_argument(M)" that returns the topic to which more time is dedicated.
I wrote the function and correctly returns the element but is it ok or is it too long?

def common_argument(M):
    element = M[0][4]
    for i in range(len(M)):
        if M[i][4] != element:
            if period(M, element) < period(M, M[i][4]):
                element = M[i][4]
    return element

 def period(M,c):
        s = 0
        for i in range(len(M)):
            if c in M[i]:
                if M[i][2] != M[i][3]:
                     if M[i][2] > M[i][3]:
                         s += M[i][2] - M[i][3]
                     if M[i][3] > M[i][2]:
                         s += M[i][3] - M[i][2]
         return s

common_argument([[10, 1, 15, 17, 'Vendite'],
    [10, 1, 17, 18, 'Progetti'],
    [16, 2, 10, 12, 'Vendite'],
    [16, 2, 13, 15, 'Progetti'],
    [5,3,10, 13, 'Progetti']])

This can be radically simplified (and made much more legible) in a variety of ways.

First off, the you use 5 lines of code to compute the duration of a meeting, including checking in advance if that duration will be 0 (if M[i][2] != M[i][3]). You don't care if the duration is 0 (s += 0 leaves s unchanged), and the rest of that logic just ensures that you get a positive difference even if the order of those values is backwards. Technically, the definition of a "meeting" that you give doesn't allow those values to be backwards, so it should be safe to simply subtract the start from the end time; however, even if you want to ensure you end up with a positive value, you can just use abs (absolute value):

def period(M,c):
    s = 0
    for i in range(len(M)):
        if c in M[i]:
            s += abs(M[i][3] - M[i][2])
    return s

Next, don't iterate over the indices of a list when you can just iterate over the list. Why do you care about i? You don't, you only want the value M[i], which is much more cheaply and clearly obtained by simply looping on M:

def period(M,c):
    s = 0
    for meeting in M:
        if c in meeting:
            s += abs(meeting[3] - meeting[2])
    return s

Also, you check if c in m, but that's not really what you mean: you actually want to know if the topic c is the topic that meeting m is about. While we're at it, let's also unpack exactly what a meeting is expected to be, rather than use arbitrary integer indices into a list:

def period(M,c):
    s = 0
    for _, _, start, fin, topic in M:
        if c == topic:
            s += abs(fin - start)
    return s

That's become much more clear now that we've given every value a name (rather than just a number) and been explicit about what we're doing.

Alright, let's tackle the main function now. You iterate over every meeting, and compare it to every other meeting, but this isn't strictly necessary. It also risks doing repeated work (e.g., if you have one REALLY long meeting about 'A' and a thousand tiny ones about 'B', you'll compute the total meeting durations for 'B' a thousand times, and actually do the same with 'A'). Let's first figure out all unique topics, and that way we'll be sure to not re-do any work:

def common_argument(M):
    all_topics = {topic for _, _, _, _, topic in M}
    element = all_topics.pop()
    for other_topic in all_topics:
        if period(M, element) < period(M, other_topic):
            element = other_topic
    return element

Well, that still doesn't prevent us from not duplicating work... let's make sure that we only compute the period of each topic once:

def common_argument(M):
    topic_duration = {topic: period(M, topic) for _, _, _, _, topic in M}
    element, element_duration = ...  # What to put here?
    for other_topic, other_duration in topic_duration.items():
        if element_duration < other_duration:
            element = other_topic
    return element

Alright, we're now only compute each duration once. But the rest of that function is just asking "What is the max topic, sorted by duration?" And, in Python, there's a much easier way to ask that:

def common_argument(M):
    topic_duration = {topic: period(M, topic) for _, _, _, _, topic in M}
    return max(topic_duration, key=topic_duration.get)

The function topic_duration.get takes a topic and returns its duration, and iterating over topic_duration yields each unique topic once, so this will give us exactly what we want.

The one thing we could still improve is that we're iterating over M once for each topic, yielding a runtime of O(N*T) where N = len(M) and T is the number of unique topics. There's no reason for that, we could easily loop over the array just once. That would look more like the following:

from collections import defaultdict

def common_argument(M):
    topic_duration = defaultdict(int)
    for _, _, start, fin, topic in M:
        topic_duration[topic] += abs(fin - start)

    return max(topic_duration, key=topic_duration.get)

And voila, we've collapsed all of the same logic into effectively 4 lines of code, and reduced computational complexity significantly.


Nested Google Maps Listener This nests Google Maps Event Listener for click or drag. How should I refactor the last part where it reuses the function getAddressComponents()? If there are other parts to be refactored, please do offer suggestions.

  function getAddressComponents() {
    geocoder.geocode({'latLng': marker.getPosition()}, function(results, status) {
      if (status == google.maps.GeocoderStatus.OK) {
        clearValue();
        if (results[0]) {
          // Get address_components
          for (var i = 0; i < results[0].address_components.length; i++)
          {
            var addr = results[0].address_components[i];
            if (addr.types[0] == 'street_address')
              $('#spot_street_address').val(addr.long_name);
          }
        } else {
          alert('No results found. Please revise the address or adjust the map marker.');
        }
      }
    });
  }

  // Add drag listener to marker for reverse geocoding
  google.maps.event.addListener(marker, 'drag', function() {
    getAddressComponents();
  });

  // Add click listener to marker for reverse geocoding
  google.maps.event.addListener(map, 'click', function(event) {
    marker.setMap(null);
    addMarker(event.latLng);
    getAddressComponents();
    google.maps.event.addListener(marker, 'drag', function() {
      getAddressComponents();
    });
  });

The map will have existing marker, which is draggable and geocodable. But once I click on the map to create a marker (thus, removing the existing marker), that new marker as I drag it, can't be geocodable. That's why I have to nest the listener to get it working. What have I done wrong?

google.maps.event.addListener(marker, 'drag', fgetAddressComponents);

And you don't need to add it twice. No need to add it in the click handler again.

Otherwise it's mostly ok, except for the == (instead of ===), I just have styling nitpicks to say about this code.


Takes function and throws exception if passed function doesn't throw an exception The code below is part of a toy test assertion library I've written.

I'm looking for feedback specifically on lines 4 - 9.  I'd like to extract them from the throws function.  I can think of two options.

Create a function with a name like maybeReportExceptionError().  This seems not great because a) it feels like the function has two responsibilities and b) the maybe in the name seems hard to understand for someone not familiar with the inner workings of the function.
Replace the if/else if with a polymorphism.  I'm not sure how to go about this.

Any thoughts greatly appreciated!

function throws(testFunction, expectedMessage) {
  var thrownMessage = getFunctionExceptionMessage(testFunction);

  if (thrownMessage === undefined) {
    throw new Error("Expected an exception");
  } else if (thrownMessage !== expectedMessage) {
    throw new Error("Threw - " + thrownMessage + " - not - " + expectedMessage);
  }
};

function getFunctionExceptionMessage(fn) {
  try {
    fn();
  } catch (exception) {
    return exception.message;
  }
};

// USAGE:

// Throws "Error: Expected an exception"
throws(function() {

}, "Expected message");

// Throws "Error: Threw - Thrown message - not - Expected message"
throws(function() {
  throw new Error("Thrown message");
}, "Expected message");

It kind of boils down to coding style, but this is how I'd refactor your code.

I like my assertions to clearly state they are; rename throws() to assertThrows().
I like guard clauses rather than "unneccessary" else statements.
If a function ever throws an exception with an undefined message, your assertion would fail.
I like a specific format of assert failures: expected xxx but got yyy.

Code:

function assertThrows(testFunction, expectedMessage) {

    var actual = catchExceptionFrom(testFunction);

    if (actual === undefined)
        throw new Error("Function did not throw an exception.");

    if (actual.message !== expectedMessage)
        throw new Error("Expected exception '" + expectedMessage + "' but got '" + actual.message + "'");
};

function catchExceptionFrom(fn) {
    try {
        fn();
    }
    catch (exception) {
        return exception;
    }
};


Best OOP method for a layout library I had a code for a layout library which looked like :

$this->layout->setLayout($layout)->setTitle($title)->render()
etc.

Today I started writing a new code, simpler and smaller :

$this->layout->data(array(
    'view_name'     => 'layout_admin',
    'title'         => 'Administration',
    'js'            => 'admin/matrix.chat'
))->view('admin/index');

The previous setters/add methods were like :

public function setDoctype($doctype) {
    $this->doctype = $doctype;
    return $this;
}

public function addJs($name) {
    if(!in_array($name, $this->js)) {
        $this->js[] = $name;
    }
    return $this;
}

The new data method :

public function data($data = array()) {
    if(isset($data['view_name'])) $this->ci->config->load($data['view_name']);
    else $this->ci->config->load($this->data['view_name']);

    // Mark as forbid because are stored in arrays and
    // I can add values to those
    $forbid = array('css', 'js', 'rss');

    foreach($this->ci->config->config['layout'] as $key => $value) {
        if(isset($data[$key]) && !in_array($key, $forbid))
            $this->data[$key] = $data[$key];
        else
            $this->data[$key] = $value;

        if(isset($data[$key]) && in_array($key, $forbid)) {
            if(is_array($data[$key])) {
                if(count($array) != count($array, COUNT_RECURSIVE))
                    foreach($data[$key] as $key2 => $value2)
                        $this->data[$key][$key2] = $value2;
                else
                    foreach($data[$key] as $value2)
                        $this->data[$key][] = $value2;
            } else
                $this->data[$key][] = $data[$key];
        }
    }

    return $this;
}

I changed to this because of the many things that I can update for every page. And use ->setThing would be annoying. But I don't find that my code is so OOP.

Note that I'm using CodeIgniter.

What's the best oop code ? If you want more indications, please tell me I'm new and don't know how to  say more ;) .

The core of your question is whether or not you should use setters ($tpl->setSomeParam()) or a simple array to bind template parameters. While using an array my not seem very OO, it's actually a pretty effective way of decoupling your data from your templates. As you've discovered, having to add a new setter to your layout manager every time you add a new parameter to one of your templates means the layout manager is tightly coupled to all of your templates – not good.

This is pretty common practice these days. Twig, Underscore.js, and many others do the same thing.


Simple program to split arguments up I am writing this small tester program to be used as part of a larger project, and I am quite unsure about the strtok function, which I (think) I need to use to split some character arrays up on a certain character (in my case this is a colon). For each character array (which is passed in as an argument), it should split the array into two. Apologies if it's hard to follow, I'm bad at explaining things.

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

int main(int argc, char **argv)
{
    char *pt;

    for (int i = 1; i < argc; i++)
    {
        pt = strtok(argv[i], ":");

        int j = 0;

        while (pt != NULL || j < 2)
        {
            j++;

            if (j == 1)
            {
                printf("item: ");
            } else if (j == 2)
            {
                printf("quantity: ");
            }

            printf("%s\n", pt);
            pt = strtok(NULL, ":");
        }

        j = 0;
    }

    return 0;
}

Yes, I know the program will crash if not enough arguments are supplied, but I just quickly threw this together to try and learn more about strtok. Now although this code works, I feel it is not the most efficient. I have not been programming C for very long, so any help for how I could improve this loop would be greatly appreciated.

I see some things that may help you improve your program.

Break up the code into smaller functions

This is a very short program, so it's not critical here, but generally, rather than having everything in the main() function, it would be easier to read and maintain if each discrete step were its own function.  For instance, I'd separate out the token splitting function so that main looks like this:

int main(int argc, char **argv) {
    for (int i = 1; i < argc; i++) {
        split(argv[i]);
    }
}

Eliminate work that does not need to be done

The final j = 0 has no practical effect and should be eliminated.  Think carefully about what your program is doing so that you don't have the computer do more work than is necessary.

Eliminate "magic numbers"

The code is mostly free of "magic numbers' (that is, hard-coded values for which there is no obvious meaning), but I'd suggest that it would make sense to have the delimter string be an assigned variable.

Use a for loop instead of while

Because both pt and the j variables are only used within the while loop, consider again reducing scope and turning that into a for loop with one of those variables.

Use a single call to printf where practical

Instead of calling printf twice for each parsed value, it would simplify the program to simply call printf once instead of twice.

Sanitize user input better

The code doesn't quite work as posted, as you apparently already know.  It doesn't really take much more time or code to make the program not crash with malformed input, so I would strongly suggest doing so to make your program robust.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments:  "I didn't know that." or "That's bad advice!"  My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard.  For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

  [...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

  If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea.  We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function.  Reasons against omitting seem to boil down to "it looks weird".  If, like me, you're curious about the rationale for the change to the C standard read this question.  Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!)  In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.

Results

Here's a reworked version of your program incorporating all of these suggestions:

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

void split(char *arg) {
    static const char* label[] = { "item", "quantity", NULL };
    static const char *delim = ":";
    const char **lbl = label;
    for (char *pt = strtok(arg, delim); pt != NULL && *lbl != NULL; ++lbl) {
        printf("\t%s: %s\n", *lbl, pt);
        pt = strtok(NULL, delim);
    }
}

int main(int argc, char **argv) {
    for (int i = 1; i < argc; i++) {
        printf("\nInput: %s\nOutput:\n", argv[i]);
        split(argv[i]);
    }
}

Some notes

The revised code includes two static const variables.  This means that they are initialized at compile time and do not change during the course of the program.  Further, every invocation of the split function has those variables set to the exact same values.  The advantage is that the compiler can turn this into very efficient code requiring little or no runtime execution and minimal code size.

The lbl variable simply points to the first label "item" first, and then steps through the labels in label until it encounters NULL at which point the routine will end whether or not there are more delimiters in the input string.

Finally, I'd recommend creating test code that runs a number of test cases and compares the actual output to the desired output.  By creating such a test program, you can be assured that if you change the program later (e.g. for more efficiency or to add additional features), you will be able to assure that the function still operates as intended and that no new unintended behavior (bugs!) have been introduced.

Sample run

  ./splitter foo:bar toes:10 Edward: nocolon two:colons:here :: : smoo

Input: foo:bar
Output:
        item: foo
        quantity: bar

Input: toes:10
Output:
        item: toes
        quantity: 10

Input: Edward:
Output:
        item: Edward

Input: nocolon
Output:
        item: nocolon

Input: two:colons:here
Output:
        item: two
        quantity: colons

Input: ::
Output:

Input: :
Output:

Input: smoo
Output:
        item: smoo


How can this autoloader be improved? I don't understand the concept of autoloaders, other than the bare minimum, which is that to load a class file when the class is being called. Or as the PHP manual cites it, to avoid having to require multiple time.

I get that, but what I don't understand is the complexity behind almost all the autoloaders which I sought to learn some things from. It's almost impossible to understand what goes on behind the scenes.

Just to understand it a bit more with your help, I made a simple autoloader as you can see below:

class Autoloader
{
    private $pathToClass;

    //register the path
    function __construct($pathToClass)
    {
        $this->pathToClass = $pathToClass;
    }

    //load the file
    public function load($class)
    {

        // expload the namespaces ex: foo\bar\tar array(foo, bar, tar)
        $explode = explode('\\', $class);

        //get the last exploaded string and append .php so it becomes tar.php
        $class = $explode[count($explode) - 1].'.php';

        // required tar.php in the path it is found ex:
        // require '/foo/bar/tar.php' in lowecase to avoid windows/unix conflict
        if(file_exists(strtolower($this->pathToClass.$class))){
             require strtolower($this->pathToClass.$class);

            return true;
        }

        return false;
    }

    // autoload
    public function register()
    {
        spl_autoload_register([$this, 'load']);
    }
}

The class explains itself. It does work, and I just would like to know what I am missing?  As you can see, it is the most basic autoloader.  What would be the next most important thing you would add to it?

Here is how it is instantiated:

$myLibLoader = new Autoloader(__DIR__.'/foo/bar/');
$myLibLoader->register();

$foo =  new foo();

Comments on your code

The access modifier for your constructor is missing.

function __construct...
public function __construct

Although no specified access modifier on methods means that PHP chooses public automatically (see PHP docs), I would add it for consistency.
Your autoloader does not cope with namespaces. You might want to read PSR-0, a specification stating how autoloaders should cope with namespaces, folders and filenames.
Do not add random newlines.

public function load($class)
{

    // expload the namespaces ex: foo\bar\tar array(foo, bar, tar)

public function load($class)
{
    // expload the namespaces ex: foo\bar\tar array(foo, bar, tar)

Do not add random spaces in comments.

// expload the namespaces ex: foo\bar\tar array(foo, bar, tar)
//get the last exploaded string and append .php so it becomes tar.php

Be consistent! I suggest using a space after //.
Be specific in variable names. $pathToClass lets me think that the string points to a specific class file. It does however point to a directory containing class files. I suggest using $classDir. $class is also an edge case since it does contain more than a single class name.
You don't need explode if you only wish to extract the class name.
Use strrpos():

$classFilename = substr($class, strrpos($class, '\\') + 1) . '.php';

Don't apply strtolower two times. Just save the result in a temporary variable. It makes your code DRY.
Since you declare pathToClass private anyway, consider marking the class final. Some arguments are provided in an article Final should be Default for Classes in Java (the same principles not only apply to Java, but also to many other languages including PHP).
Mark your load method private. It's an implementation detail, which shouldn't be accessible from outside the class.
Your load method is not required to return a boolean value. Returning true or false is not bad per se, maybe you will want to encapsulate the autoloading mechanism further and throw exceptions depending on the return value.

Elaborating on the PSR-0 specification

The spec defines mandatory requirements your autoloader must fulfill.

They provide example inputs your autoloader must accept:

\Doctrine\Common\IsolatedClassLoader => /path/to/project/lib/vendor/Doctrine/Common/IsolatedClassLoader.php
\Symfony\Core\Request => /path/to/project/lib/vendor/Symfony/Core/Request.php
\Zend\Acl => /path/to/project/lib/vendor/Zend/Acl.php
\Zend\Mail\Message => /path/to/project/lib/vendor/Zend/Mail/Message.php

\namespace\package\Class_Name => /path/to/project/lib/vendor/namespace/package/Class/Name.php
\namespace\package_name\Class_Name => /path/to/project/lib/vendor/namespace/package_name/Class/Name.php

Roughly speaking, your code is missing the following aspects:

Taking the namespace into account and using it to build a directory hierarchy.
Transforming underscores in class names to a directory hierarchy as well.


Multithreaded Python implementation of a Sudoku validator I've just come across these day with the multithreading subject in Python. I remembered @200_success's answer and I thought of making that multithreaded as it follows:

one thread to check that each column contains the digits 1-9 only once
one thread to check that each row contains the digits 1-9 only once
another nine threads to check each 3x3 sub-grid for the digits 1-9

I know multithreading won't bring any speed improvement in this case but I wanted to start learning about this subject and this looked like a good starting point.

from itertools import product
from threading import Thread
from timeit import default_timer as timer
import queue

DIGITS = set(range(1, 10))

def check_grid_size(grid):
    """Check that the grid is 9x9."""
    well_formed = len(grid) == 9 and all(len(row) == 9 for row in grid)
    return well_formed or None

def check_rows(grid, q):
    """Check that each number appears exactly once per row."""
    q.put(all(set(row) == DIGITS for row in grid))

def check_columns(grid, q):
    """Check that each number appears exactly once per column."""
    columns = [[row[c] for row in grid] for c in range(9)]
    q.put(all(set(col) == DIGITS for col in columns))

def check_3x3_grid(grid, q):
    """Check that each number appears exactly once per 3x3 grid."""
    threes = [(0, 1, 2), (3, 4, 5), (6, 7, 8)]
    for row_block, col_block in product(threes, threes):
        block = [grid[r][c] for r, c in product(row_block, col_block)]
        if set(block) != DIGITS:
            q.put(False)
            return

    q.put(True)

def check_sudoku(grid):
    """
    Validate a sudoku solution.

    Given a grid as a list of lists, return None if it is ill-formed,
    False if it is invalid, or True if it is a valid solution.
    """
    assert isinstance(grid, list)

    q = queue.Queue()

    if not check_grid_size(grid):
        return None

    row_thread = Thread(target=check_rows, args=(grid, q))
    row_thread.start()

    columns_thread = Thread(target=check_columns, args=(grid, q))
    columns_thread.start()

    grid_threads = []
    for _ in range(9):
        t = Thread(target=check_3x3_grid, args=(grid, q))
        t.start()
        grid_threads.append(t)

    row_thread.join()
    columns_thread.join()

    [t.join() for t in grid_threads]

    results = []
    while not q.empty():
        results.append(q.get())

    return all(results)

def main():
    ill_formed = [[5, 3, 4, 6, 7, 8, 9, 1, 2],
                  [6, 7, 2, 1, 9, 5, 3, 4, 8],
                  [1, 9, 8, 3, 4, 2, 5, 6, 7],
                  [8, 5, 9, 7, 6, 1, 4, 2, 3],
                  [4, 2, 6, 8, 5, 3, 7, 9],  # <---
                  [7, 1, 3, 9, 2, 4, 8, 5, 6],
                  [9, 6, 1, 5, 3, 7, 2, 8, 4],
                  [2, 8, 7, 4, 1, 9, 6, 3, 5],
                  [3, 4, 5, 2, 8, 6, 1, 7, 9]]

    # check_sudoku should return True
    valid = [[5, 3, 4, 6, 7, 8, 9, 1, 2],
             [6, 7, 2, 1, 9, 5, 3, 4, 8],
             [1, 9, 8, 3, 4, 2, 5, 6, 7],
             [8, 5, 9, 7, 6, 1, 4, 2, 3],
             [4, 2, 6, 8, 5, 3, 7, 9, 1],
             [7, 1, 3, 9, 2, 4, 8, 5, 6],
             [9, 6, 1, 5, 3, 7, 2, 8, 4],
             [2, 8, 7, 4, 1, 9, 6, 3, 5],
             [3, 4, 5, 2, 8, 6, 1, 7, 9]]

    # check_sudoku should return False
    invalid = [[5, 3, 4, 6, 7, 8, 9, 1, 2],
               [6, 7, 2, 1, 9, 5, 3, 4, 8],
               [1, 9, 8, 3, 8, 2, 5, 6, 7],
               [8, 5, 9, 7, 6, 1, 4, 2, 3],
               [4, 2, 6, 8, 5, 3, 7, 9, 1],
               [7, 1, 3, 9, 2, 4, 8, 5, 6],
               [9, 6, 1, 5, 3, 7, 2, 8, 4],
               [2, 8, 7, 4, 1, 9, 6, 3, 5],
               [3, 4, 5, 2, 8, 6, 1, 7, 9]]

    print(check_sudoku(valid))
    print(check_sudoku(invalid))
    print(check_sudoku(ill_formed))

if __name__ == '__main__':
    main()

This is working nice when I'm profiling to see how fast it works, but I need to know if I implemented the threads correctly, especially when talking about the 9 threads from the 3x3 sub-grids.

The way you're starting the block threads looks wrong to me.  You're starting each thread with the same parameters (the grid and a queue):

t = Thread(target=check_3x3_grid, args=(grid, q))

I can't see anywhere that the thread is told which block it is supposed to check.  So, instead of starting 9 threads, one to check each block, it looks like you're starting 9 threads and having each of them check all nine of the blocks.

You could partition the blocks and then start a thread to validate each block, but this seems like overkill.  Simply starting a single thread to check the nine blocks is the smallest change and probably the right one to make, since checking nine blocks is almost the same amount of work as checking the nine horizontals/verticals.  With that in mind, you can simply change the code to the following (an achieve the same result with 8 less threads):

#...

grid_thread = Thread(target=check_3x3_grid, args=(grid, q))
grid_thread.start()

#...

grid_thread.join()

Or a slightly more concise:

validation_threads = [Thread(target=check_rows, args=(grid, q))
                     ,Thread(target=check_columns, args=(grid, q))
                     ,Thread(target=check_3x3_grid, args=(grid, q))]

[t.start() for t in validation_threads]
[t.join() for t in validation_threads]


Tic-Tactics implementation I've tried to take the shortest, simplest path to a C# solution to the Ultimate Tic-Tac-Toe challenge.  This implementation plays in the console.  I admit the game logic could be hived off into a separate method, but I'm not sure that would be an improvement.  I also admit that the fields should be tighter than public, but that's neither here nor there.

I would appreciate feedback on:

How readable people find this solution;
Any suggestions to make the code more succinct;
Any other comments people would care to make.

void Main()
{
    var mb = new MetaBoard();
    var board = -1;
    var player = Who.X;
    while (WinFor(mb) == Who.Neither && mb.Boards.Any(x => WinFor(x) == Who.Neither)) {
        mb.Write();
        Console.WriteLine(player + " to play.");
        // Choose a board if we've just started or the current board is finished.
        while (board == -1 || mb[board] != Who.Neither) board = InputNum("an unfinished board");
        var cell = -1;
        // Choose an empty cell on the board.
        while (cell == -1 || mb.Boards[board][cell] != Who.Neither) cell = InputNum("an empty cell on board " + (board + 1));
        Console.WriteLine();
        mb.Play(board, cell, player);
        // The next board is decided by the cell just played.  Swap turns between the players.
        board = cell;
        player = (player == Who.X ? Who.O : Who.X);
    }
    mb.Write();
    Console.WriteLine(WinFor(mb) + " wins!");
}

static int InputNum(string what) {
    var i = -1;
    do {
        Console.Write("Choose " + what + " [1-9]: ");
    } while (!int.TryParse(Console.ReadLine().Trim(), out i) || i < 1 || 9 < i);
    return i - 1;
}

enum Who { Neither, X, O }

static int[][] Lines = new int[][] {
    new int[] {0,1,2}, new int[] {3,4,5}, new int[] {6,7,8},
    new int[] {0,3,6}, new int[] {1,4,7}, new int[] {2,5,8},
    new int[] {0,4,8}, new int[] {2,4,6}
};

interface IBoard { Who this[int i] { get; } } // Indicate who, if any, has won position i.

static Who Min(Who p, Who q) { return p == q ? p : Who.Neither; }

static Who WinFor(IBoard board) { // X if a line of three Xs, O if a line of three Os.
    return Lines.Select(line => Min(board[line[0]], Min(board[line[1]], board[line[2]])))
                .FirstOrDefault(who => who != Who.Neither);
}

class Board: IBoard { // A 3x3 grid of cells.
    public Who[] Cells = new Who[9];
    public Who this[int i] { get { return Cells[i]; } }
    public void Play(int i, Who who) { Cells[i] = who; }
}

class MetaBoard: IBoard { // A 3x3 grid of boards.
    public Board[] Boards = Enumerable.Range(0, 9).Select(i => new Board()).ToArray();
    public Who this[int i] { get { return WinFor(Boards[i]); } }
    public void Play(int i, int j, Who who) { Boards[i].Play(j, who); }

    public void Write() {
        Action<Who> writeCell = w => { Console.Write(w == Who.X ? 'X' : w == Who.O ? 'O' : '.'); };
        var rg = Enumerable.Range(0, 3);
        foreach (var rr in rg) foreach (var r in rg) foreach (var cc in rg) foreach (var c in rg) {
            writeCell(Boards[cc + 3 * rr].Cells[c + 3 * r]);
            if (c == 2) Console.Write("  ");
            if (c == 2 && cc == 2) Console.WriteLine();
            if (c == 2 && cc == 2 && r == 2) Console.WriteLine();
        }
    }
}

This doesn't strike me as particularly readable, although it is definitely succinct. Here are a few of the things I noticed that I think could be improved:

The name Min() for a method that determines if someone has filled a row is very confusing.

WinFor() should be a method on IBoard rather than a static method that takes an IBoard.

The variable names in MetaBoard.Write()are also confusing and something like outerRow and innerColumn would be more informative that rr and c.

The quadruple foreach loop in the MetaBoard.Write() scares me. I realize what it is doing, but that many nested loops is a code smell. You might want to create a method on IBoard that prints out board contents line-by-line so that you can append them to each other and control when you insert padding and newlines.

If you're going to do that, you may consider extracting out a BoardPrinter class or something similar to encapsulate that behavior.

The size of the board is hard-coded and although it isn't a requirement, redesigning this to support different-sized boards will make a more flexible design. (I got this idea in a review for my Ultimate Tic-Tac-Toe entry and am happy I re-worked it).
This will require removing Lines and calculating them manually. As a side note, indices for diagonals can be calculated by consecutively adding the (board width + 1) for downward-right and the (board width - 1) for downward-left diagonals. It took me a while to figure that out.

The use of Board[] as the type for the MetaBoard.Boards property indicates to me that the IBoard interface is not as useful as it should be (since MetaBoard is tightly coupled to Board). If you pull out the printing so that it can be done line-by-line as I suggested earlier, you could go back to using IBoard. If done carefully, you could nest MetaBoards within MetaBoards (for the Ultimate in Ultimate Tic-Tac-Toe).

I don't see where you restrict players from selecting an already completed board.

I know I'm basically ruining the brevity if you implement my comments, but I am impressed with how you managed to keep it so short. It was fun to review, thanks for sharing!


Scraper to deal with some complicated site with ads I've written a script in Python in combination with Selenium to scrape data from a slightly complicated web page. When the web page is made to open, it comes up with an ad sitting on the data making the browser break unless it is kicked out or the scraper waits a certain time for the ad to disappear.

I have made the scraper remove it successfully and reach the data as i wanted to. Another problem was to click on the next page button which I've been able to do effectively as well. Now, it is working perfectly along with writing the scraped data in a CSV file. There are around 100 pages the website has traversed. Although it's a very small script to get reviewed, I suppose any clue to make it better might be useful for future reference.

from selenium import webdriver
from selenium.webdriver.common.by import By
from selenium.webdriver.support.ui import WebDriverWait
from selenium.common.exceptions import TimeoutException
from selenium.webdriver.support import expected_conditions as EC
import csv

driver = webdriver.Chrome()
wait = WebDriverWait(driver,10)

driver.get("https://www.inc.com/inc5000/list/2017")

try:
    wait.until(EC.element_to_be_clickable((By.XPATH, "//span[.='SKIP']"))).click()
except TimeoutException: pass

outfile = open("inc.csv","w",newline='')
writer = csv.writer(outfile)
writer.writerow(["Rank","Company","Growth","Revenue","Industry"])

while True:
    for item in driver.find_elements_by_css_selector("#data-container .row"):
        rank = item.find_elements_by_css_selector(".rank")[0].text
        company = item.find_elements_by_css_selector(".company a")[0].text
        growth =item.find_elements_by_css_selector(".growth")[0].text
        revenue = item.find_elements_by_css_selector(".revenue")[0].text
        industry = item.find_elements_by_css_selector(".industry")[0].text
        writer.writerow([rank,company,growth,revenue,industry])
        print(rank,company,growth,revenue,industry)
    try:
        wait.until(EC.element_to_be_clickable((By.CSS_SELECTOR, "#arrow-r .fa-caret-right"))).click()
    except:break

driver.quit()

There is a room for improvement - like there always is:

avoid bare exception handling. Instead of having an except clause without specifying exception classes to handle, you can handle TimeoutException (which you even already have imported)
use with context manager when dealing with files
instead of using the find_elements_by_css_selector() method and then getting the first result - just use find_element_by_css_selector() directly
use try/finally to make sure the browser is closed in case of an unexpected exception during the web-scraping
respect PEP8 rules - specifically: spaces between arguments, order of imports
adding couple clarifying comments might help to improve readability
I think you can switch to "by id" locator for the "next" button

Improved version:

import csv

from selenium import webdriver
from selenium.webdriver.common.by import By
from selenium.webdriver.support.ui import WebDriverWait
from selenium.common.exceptions import TimeoutException
from selenium.webdriver.support import expected_conditions as EC

driver = webdriver.Chrome()
wait = WebDriverWait(driver, 10)

driver.get("https://www.inc.com/inc5000/list/2017")

# skip ad popup if present
try:
    wait.until(EC.element_to_be_clickable((By.XPATH, "//span[.='SKIP']"))).click()
except TimeoutException:
    pass

try:
    with open("inc.csv", "w", newline='') as outfile:
        writer = csv.writer(outfile)
        writer.writerow(["Rank", "Company", "Growth", "Revenue", "Industry"])

        while True:
            for row in driver.find_elements_by_css_selector("#data-container .row"):
                rank = row.find_element_by_css_selector(".rank").text
                company = row.find_element_by_css_selector(".company a").text
                growth = row.find_element_by_css_selector(".growth").text
                revenue = row.find_element_by_css_selector(".revenue").text
                industry = row.find_element_by_css_selector(".industry").text

                writer.writerow([rank, company, growth, revenue, industry])
                print(rank, company, growth, revenue, industry)

            # next page
            try:
                wait.until(EC.element_to_be_clickable((By.ID, "arrow-r"))).click()
            except TimeoutException:
                break  # reached the end of the page
finally:
    driver.quit()


Help to Refactor PHP Flip Image Code to smallest size as possible I have written this PHP function which will flip an image (horizontally, like looking in a mirror). It reads the file, flips it, and writes it to a new filed called filename"_flipped".ext. I would like to refactor this to the smallest possible size, but maintaining human readability.

Here is the function:

function flip_image($filepath)
{
    if(file_exists($filepath)):

        //Array ( [dirname] => images
        //        [basename] => pig.png
        //        [extension] => png
        //        [filename] => pig
        //        [filesize] => 72 )
        $file = $this->get_file_info($filepath); // Returns above array

        // Is it allowed in our extensions array
        if(in_array($file['extension'], $this->allowed_exts)):

            // Reference our Image for the correct extension
            switch ($file['extension']) {
                case 'png':
                    $img = imagecreatefrompng($filepath);
                    break;
                case 'gif':
                    $img = imagecreatefromgif($filepath);
                    break;
                default: // Has to be JPG, or JPEG
                    $img = imagecreatefromjpeg($filepath);
                    break;
            }

            // Now we have our image...
            // Image Width & Height
            $size_x = imagesx($img);
            $size_y = imagesy($img);

            // Create Temp Image
            $temp = imagecreatetruecolor($size_x, $size_y);

            // Define a colour as transparant, allocate the colour, resample it
            imagecolortransparent($temp, imagecolorallocate($temp, 0, 0, 0));
            imagealphablending($temp, false);
            imagesavealpha($temp, true);

            $x = imagecopyresampled($temp, $img, 0, 0, ($size_x-1), 0, $size_x, $size_y, 0-$size_x, $size_y);
            if ($x) {
                $img = $temp;
            }
            else {
                die('Unable to flip image');
            }

            $writepath = $file['dirname'] . "/" . $file['filename'] . "_flipped" . ".";
            // Write our file
            switch ($file['extension']) {
                case 'png':
                    imagepng($img, $writepath.$file['extension']);
                    break;
                case 'gif':
                    imagegif($img, $writepath.$file['extension']);
                    break;
                default: // Has to be PNG
                    imagejpeg($img, $writepath.$file['extension']);
                    break;
            }
            $flipped_path = $writepath.$file['extension'];
            imagedestroy($img);
            return $flipped_path;
        else:
            echo 'Incorrect File Type, Not an Image, .png .gif, jpeg. jpeg Only.';
            return false;
        endif;
    else:
        echo 'No Such file found, Check the file path';
        return false;
    endif;

}

You could use call_user_func to check the extension only once. Something like this (I haven't got the time even to read the code now)

function flip_image($filepath)
{
if(file_exists($filepath)):

    //Array ( [dirname] => images
    //        [basename] => pig.png
    //        [extension] => png
    //        [filename] => pig
    //        [filesize] => 72 )
    $file = $this->get_file_info($filepath); // Returns above array

    // Is it allowed in our extensions array
    if(in_array($file['extension'], $this->allowed_exts)):
         // Reference our Image for the correct extension
        $outfile = $file['dirname'] . "/" . $file['filename'] . "_flipped" . "." . $file['extension'];
        switch ($file['extension']) {
            case 'png':
                return flip_image_in($filepath, $outfile, 'imagecreatefrompng', 'imagepng');
            case 'gif':
                return flip_image_in($filepath, $outfile, 'imagecreatefromgif', 'imagegif');
            default: // Has to be JPG, or JPEG
                return flip_image_in($filepath, $outfile, 'imagecreatefromjpeg', 'imagejpeg');
        }
    else:
        echo 'Incorrect File Type, Not an Image, .png .gif, jpeg. jpeg Only.';
        return false;
    endif;
else:
    echo 'No Such file found, Check the file path';
    return false;
endif;

}

function flip_image_in($inpath, $outfile, $create_function, $write_function)
{
        $img = call_user_func($create_function, $inpath);

        // Now we have our image...
        // Image Width & Height
        $size_x = imagesx($img);
        $size_y = imagesy($img);

        // Create Temp Image
        $temp = imagecreatetruecolor($size_x, $size_y);

        // Define a colour as transparant, allocate the colour, resample it
        imagecolortransparent($temp, imagecolorallocate($temp, 0, 0, 0));
        imagealphablending($temp, false);
        imagesavealpha($temp, true);

        $x = imagecopyresampled($temp, $img, 0, 0, ($size_x-1), 0, $size_x, $size_y, 0-$size_x, $size_y);
        if ($x) {
            $img = $temp;
        }
        else {
            die('Unable to flip image');
        }

        call_user_func($write_function, $img, $outfile);
        imagedestroy($img);
        return $outfile;
}