Trying to do Type-Driven Development in F# I've been wanting to try out F# for some real world programming so I decided to try rewriting a program that is being used at work.

It can be reduced to a few steps:

Get report templates.
In this step I query a DB and get some templates for reports
Update data that the reports are working on.
In this step I query some banking services and save our data on our DB
Generate new reports.
In this step I call a stored procedure that generates a new report

I'm trying to do this outside in and have been using Mark Seemann's blog and Scott Wlaschin's guide as a guide and this is what I've got so far:

module Helpers =
let getValuesInListOfOptions list =
    List.choose (fun x ->
                    match x with
                    | Some value -> Some value
                    | _ -> None ) list
module PaymentVelocityReports =
open System;
open Helpers;

type ReportTemplateName = string

type ReportTemplate = {
    Id: int
    GroupId: int
    ReportOwnerSSN: string
    Name: ReportTemplateName
    Description: string
    Enabled: bool
    DayOfMonthToRun: int
    DateFrom: DateTime
    DateTo: DateTime option
    UpdateData: bool}

type Claimant = {
    Id: decimal
    Ssn: string
}

type BankingService = {
    Id: decimal
    ClaimantId: decimal
    Bank: string
    Username: string
    Password: string
    Enabled: bool
}

type ClaimantTemplates ={
    ReportTemplates: ReportTemplate list
    Claimant: Claimant
    BankingServices: BankingService list
}

let updateClaimantData saveNewGiros saveNewPayments (dateFrom: DateTime) (dateTo: DateTime) (bankingServices: BankingService list) =
    List.iter (fun x -> saveNewGiros dateFrom dateTo x) bankingServices
    List.iter (fun x -> saveNewPayments dateFrom dateTo x) bankingServices

let updateData updateClaimantData (claimantTemplates: ClaimantTemplates) : ReportTemplateName list =
    if List.exists (fun x -> x.UpdateData) claimantTemplates.ReportTemplates then
        let minDate = List.map (fun x -> x.DateFrom) claimantTemplates.ReportTemplates |> List.min
        let maxDateWithNone = List.map (fun x -> x.DateTo) claimantTemplates.ReportTemplates |> List.max
        let maxDateWithoutNone = List.map (fun x -> x.DateTo) claimantTemplates.ReportTemplates |> getValuesInListOfOptions |> List.max

        match maxDateWithNone with
            | None -> updateClaimantData minDate DateTime.Today claimantTemplates.BankingServices
            | _ -> updateClaimantData minDate maxDateWithoutNone claimantTemplates.BankingServices

    List.map (fun x -> x.Name) claimantTemplates.ReportTemplates

let getClaimantTemplates data createClaimantTemplate : ClaimantTemplates list =
    data |> List.map createClaimantTemplate

//The root of the program
let generateReports (getClaimantTemplates: unit -> ClaimantTemplates list) (updateData: ClaimantTemplates -> ReportTemplateName list) (createReport: ReportTemplateName -> unit) =
    getClaimantTemplates () |> List.map updateData |> List.concat |> List.map createReport |> ignore

I'm at the point where the updateClaimantData and getClaimantTemplates require data access and the createReport function that generateReports needs is also a data access function.

I was hoping to get some criticism to see if I'm on the right track. I'm both new to the language and functional languages as well.

From a cursory glance, this looks like a good start, but I haven't made a full exegesis of the code.

A couple of observations:

The getValuesInListOfOptions function seems redundant. AFAICT, you can rewrite the function as List.choose id list, in which case you could inline that expression and remove the function.

It seems odd to me that the function with the comment The root of the program is still a higher-order function defined in the same module as all the other functions. I'd expect the root of the program to be something akin to a main function, that only takes command-line string arguments as input, if it takes any arguments at all.


Makefile for CGI program using SQLite library I am developing a CGI program in C on Linux using gcc, GNU make and the SQLite library. How does my makefile look?

# Automatic Variables
# $@ file name of target
# $< name of the first prerequisite
# $? name  of all prerequisites newer than target
# $^ names of all prerequisites
# Pattern rule example - complile object files from prerequisites
# %.o: %.c
#   $(CC) -c $(CFLAGS) $< -o $@
#
# Predefined Variables
# CC compiler, default gcc
# CFLAGS compiler flags
# LDFLAGS linker flags such as -L (extra directories searched for -l)
# LDLIBS libaries such as -lfoo

CC=gcc
CFLAGS=-ansi -pedantic -Wall -Wextra -O2
LDLIBS=-ldl -lpthread
# directory of SQLite library
SQLITEDIR=sqlite

sqlfun.cgi: sqlfuncgi.o sqlite3.o
    $(CC) -o $@ $^ $(CFLAGS) $(LDLIBS)

sqlite3.o: $(SQLITEDIR)/sqlite3.c
    $(CC) -o $@ $^ -c -Os

sqlfuncgi.o: sqlfuncgi.c
    $(CC) -o $@ $^ -c $(CFLAGS)

.PHONY: clean test
clean:
    rm -f sqlfun.cgi *.o

test:
    clear
    cppcheck --enable=all --inconclusive --std=c89 sqlfuncgi.c
    valgrind --leak-check=yes ./sqlfun.cgi test

First impressions
This is clear and easy to read, and follows conventions well.

Use more built-ins
You can use more of Make's built-in rules.  For example, this rule can be omitted entirely:
sqlfuncgi.o: sqlfuncgi.c
    $(CC) -o $@ $^ -c $(CFLAGS)

If you add $(SQLITEDIR) to the VPATH, the sqlite3.o rule becomes simply
sqlite3.o: CFLAGS += -Os

And the link rule can use the built-in LINK.c variable:
sqlfun.cgi: sqlfuncgi.o sqlite3.o
    $(LINK.c) -o $@ $^  $(LDLIBS)

Special targets
Good use of .PHONY.  We probably also want .DELETE_ON_ERROR to ensure that if any command fails, its partially-written outputs don't hang around and appear to be made

Portability improvement
We can use Make's predefined $(RM) in place of rm -f.


Marking a new dataTable row as consumed or cancelled This is working code from a dataTable, which shows the function executed after the row is created:

    "createdRow" : function (nRow, data, iDataIndex) {

            var $row = $(datatable.row(nRow).draw().node());
            var $quantityInput=  $row.find("input.quantity");
            var rowClass;

            if (data.wasConsumed || data.wasCancelled){
                $quantityInput.parent().append("<span>"+ $quantityInput.val() +"</span>");
                $row.find("input, select").remove();

                if(data.wasConsumed){
                   rowClass = ".consumed";
                } else if (data.wasCancelled){
                   rowClass = ".cancelled";
                }

                $row.addClass(rowClass);
            }
      }

I added the if(data.wasConsumed || data.wasCancelled) because repeating the shared code in the other two if-statements seems worse to me.
Here the expected results (Action1 is the shared action):

wasCancelled: Action1, Action2
wasConsumed: Action1; Action3
Both false: noAction

My questions:

Are these if-statements poorly designed? (in general and also in this particular example)
How could these be refactored/improved?
Any other suggestions?

I think you did well to wrap shared code in if (data.wasConsumed || data.wasCancelled) { ... } to avoid code duplication.
However, thanks to that check, you don't need an else if inside,
a simple else will do:

if (data.wasConsumed) {
   rowClass = ".consumed";
} else {
   rowClass = ".cancelled";
}

And for a more compact writing style, a ternary will be handy here:

var rowClass = data.wasConsumed ? ".consumed" : ".cancelled";

On the other hand,
if later you extend your code with one more condition,
for example if (data.wasConsumed || data.wasCancelled || data.wasForwarded) { ... } then you will need to bring back the if-else,
of course.

If at this point you already plan such extension soon, then your current conditions are fine as they are.

The parameter iDataIndex appears to be unused. As such, is better to omit it from the parameter list of the function. It's also a strange name anyway. (The "i" prefix is sometimes (poorly) used as an indicator of an index variable into an array, but then there's the "index" too for the same purpose, and much better.)


Calculating Earnings from attendance details In a C# win forms payroll application I need to calculate employee salary based on their attendance. I've written a stored procedure which calculates earnings of all employees from the data available in attendance table. So when the application calls the SP the data will be shown on a data grid view and then immediately these data will be saved in SalaryTrans table.

Please note in following demo code I've excluded the use of Interfaces and DI.

Since this is an object oriented development employing MVP design pattern, could you please advice me whether the way I've done this acceptable?

I know that I've not used a Model, in this case SalaryModel as the DataService return a DataSet object with all earnings figures of all employees. So Could you please show me a way  of using a Model (in this case SalaryModel) in this scenario if possible?

Form

public partial class frmSalary : Form
{
    public frmSalary()
    {
        InitializeComponent();
    }

    private void btnProcessEarning_Click(object sender, EventArgs e)
    {
        SalaryPresenter presenter= new SalaryPresenter();
        presenter.ShowEarnings();
        presenter.SaveEarnings();
    }
}

Presenter

class SalaryPresenter
{
    private void ShowEarnings()
    {
        _View.dgEmployeeSalary.DataSource = CalculateEarnings(dtpStartDate.Value,dtpEndDate.Value);
    }

    private void SaveEarnings()
    {
        _DataService.InsertEarnings(dgEmployeeSalary);
    }

    private DataTable CalculateEarnings(DateTime startDate, DateTime endDate)
    {
        return _DataService.Calculate(startDate, endDate);
    }
}

Data Service

class DataService
{
    public DataTable Calculate( DateTime fromDate, DateTime toDate)
    {
        using (SqlConnection sqlConnection = new SqlConnection(db.GetConnectionString))
        {
            using (SqlCommand sqlCommand = new SqlCommand("dbo.sp_Earnings", sqlConnection))
            {
                sqlCommand.CommandType = CommandType.StoredProcedure;
                sqlCommand.Parameters.Add("@fromDate", SqlDbType.DateTime).Value = fromDate;
                sqlCommand.Parameters.Add("@toDate", SqlDbType.DateTime).Value = toDate;
                using (SqlDataAdapter sqlAdapter = new SqlDataAdapter(sqlCommand))
                {
                    using (DataSet dataSet = new DataSet())
                    using (DataTable dataTable = new DataTable())
                    {
                        sqlConnection.Open();
                        dataSet.Tables.Add(dataTable);
                        sqlAdapter.Fill(dataTable);
                        return dataTable;
                    }
                }
            }
        }
    }

    public void InsertEarnings(DataGrid dataGrid)
    {
        using (SqlConnection newCon = new SqlConnection(db.GetConnectionString))
        {
            foreach (DataGridViewRow row in dataGrid.Rows)
            {
                if (row.Cells[1].Value != null)
                {
                    string commandString = "IF NOT EXISTS (SELECT * FROM SalaryTrans WHERE emp_id=@emp_id) BEGIN INSERT INTO salaryTrans..."
                    //Database transaction code here
                }
            }
        }
    }
}

General point
Is there a strong reason for going with MVP?  One of the downsides of an MVP approach is that a lot of the nice data-binding facilities within the architecture get bypassed.  A Presentation Model / Model-View-ViewModel (MVVM) approach is pushed for WPF and can be used with Winforms.

There is a nice write-up on MVP to be found at MVC vs. MVP.

Form / Presenter
Whichever flavour of MVx one goes with, a basic idea is that the View doesn't need to know about the Presenter (and vice versa).  You mentioned excluding the interfaces and DI but in the Form code, it still needs to know about the Presenter class - its method signatures et al - and the Presenter knows about each of the controls on the Form.

Another way of structuring this is to create an interface

interface ISalaryFormView {
    event EventHandler<EventArgs> RequestProcessEarnings;
    void SetSalaryDetails(IEnumerable<SalaryDetails> salaryDetailsList);
    DateTime StartDateTime { get; set; }
    DateTime EndDateTime { get; set; }
}

(The SetSalaryDetail() method and StartDateTime, EndDateTime properties are part of the reason why MVVM is being pushed. A lot less plumbing code)

and have the Presenter take an ISalaryFormView as an argument

class SalariesPresenter {

    private readonly ISalaryForm _form;
    private readonly SalariesService _service;

    public SalariesPresenter(ISalaryForm form, SalariesService service) {
        _form = form;
        _service = service;
        _form.ProcessEarningsRequested += HandleProcessEarningsRequested;
    }

    private void HandleProcessEarningsRequested(object sender, EventArgs e) {
        _form.SetSalaryDetails(_service.GetSalaries(_form.StartDateTime, _form.EndDateTime));
    }
}

In the form, we now implement the interface and rout the click to raising the event

public partial class MainWnd : Form, ISalaryForm  {

    public MainWnd() {
       InitializeComponent();
    }

    private void btnProcess_Click(object sender, EventArgs e) {
        RaiseProcessEarningsRequested();
    }

    #region ISalaryForm

    public void SetSalaryDetails(IEnumerable<SalaryDetails> salaryDetailsList) {
        dgvSalaries.DataSource = salaryDetailsList;
        dgvSalaries.Refresh();
    }

    public DateTime StartDateTime {
        get { return dtpStartDate.Value; }
        set { dtpStartDate.Value = value; }
    }

    public DateTime EndDateTime {
        get { return dtpEndDate.Value; }
        set { dtpEndDate.Value = value; }
    }

    #region ProcessEarningsRequested

    public event EventHandler<EventArgs> ProcessEarningsRequested;

    protected void RaiseProcessEarningsRequested() {
        OnRaiseProcessEarningsRequested(EventArgs.Empty);
    }

    protected virtual void OnRaiseProcessEarningsRequested(EventArgs e) {
        var handler = ProcessEarningsRequested;
        if (handler != null) {
            handler(this, e);
        }
    }

    #endregion

    #endregion

}

We do this to help the testing in the Presenter

It doesn't need to know the concrete class of the Form - it is working against an interface.
It doesn't need to know the final screen format - swap out a listview for the datagrid and it doesn't impact.
We can unit test by having a Mock raise and event and verifying that the Setter is called.

DataService
It is a personal preference but I have a dislike of passing around data in DataTables, especially untyped datatables.  A quick web search will give lots of reasons against doing so. Adding data containers and reading into them doesn't add that much work.  That said, it is a personal preference.  The datatable is a lot faster and doesn't impact the MVx shape at all.

Passing the DataGrid in as a parameter though is a bad idea. We have now a linkage to System.Windows.Controls that is totally unnecessary and impacts portability (say we want to use this on the server side of a Web app or migrate to WPF). Even changing the screen layout - changing to a DataGridView instead of a DataGrid is impacted. Passing in a IEnumerable<> of some Data Transfer Object (DTO) (or even a datatable) is a better idea. No layer behind the view should know about or be concerned with the screen controls.

EDIT
Generally I would return an IEnumerable<T> rather than an IList<T> but in the example I am working on this is giving strange results => the DataGridView is not refreshing correctly for IEnumerable<T> but works fine for IList<T>.

For the minimal example I have two columns, Name and Value,

I have a SalaryDetails class

public class SalaryDetails {
    public string Name { get; set; }
    public decimal Value { get; set; }
}

and the SalariesService class

public class SalariesService {

    public IList<SalaryDetails> GetSalaries(DateTime start, DateTime end) {
        return ReadTable(start, end).Rows.OfType<DataRow>().Select(CreateSalaryDetails).ToList();
    }

    private static DataTable ReadTable(DateTime start, DateTime end) {
        var ret = new DataTable();
        ret.Columns.Add("Name", typeof (string));
        ret.Columns.Add("Value", typeof(decimal));

        ret.Rows.Add(new object[] { "January", 1.1m});
        ret.Rows.Add(new object[] { "February", 1.21m });
        ret.Rows.Add(new object[] { "March", 1.331m });
        ret.Rows.Add(new object[] { "April", 1.4641m });
        ret.Rows.Add(new object[] { "May", 1.651051m });

        return ret;
    }

    private static SalaryDetails CreateSalaryDetails(DataRow row) {
        return new SalaryDetails() {Name = (string) row["Name"], Value = (decimal) row["Value"]};
    }

}

The ReadTable() is a placeholder for your current Calculate() call and the Select() call is a quick way to create the SalaryDetails entries from the datarows.

Notes
I hadn't looked over your Calculate code in detail (I felt that wasn't really part of the MVP question) but looking at it now I have a few comments.
1) AFAIK, you shouldn't need a DataSet. You should be able to create and fill the table without one - not worth it to me to set up a DB to try it out but you might.
2) The using block on the datatable looks a bit strange.  On leaving the using block (i.e. when returning), Dispose() is called on the DataTable.  This doesn't appear to do anything (the data is still in the DataTable) so it is probably worth leaving out. (A quick wander down through the DataTable code shows no specific implementation for Dispose, it uses the base MarshalByValueComponent version)

EDIT
There is an IsNull() method on the DataRow.  One can use an extension to check for nulls and return default values

public static class DataRowExtensions {
    public static T GetValue<T>(this DataRow row, string name) {
        return row.IsNull(name)
                ? default(T)
                : row.Field<T>(name);
    }
}


In-place merge two sorted array First post.
Can you please review my code and help me improve my coding. Please also suggest improvements if there is a miss in this post.

Problem Statement
Given two sorted arrays X[] and Y[] of size m and n each, merge elements of X[] with elements of array Y[] by maintaining the sorted order.
i.e. fill X[] with first m smallest elements and fill Y[] with remaining elements.
Input:
X[] = {1, 4, 7, 8, 10}
, Y[] = {2, 3, 9}
Output:
X[] = {1, 2, 3, 4, 7}
, Y[] = {8, 9, 10}

public static void main(String[] args) {
    int[] X = {11, 14, 17, 18, 110};
    int[] Y = {112, 113, 114};

    mergeRunner(X, Y);

    System.out.println("X: " + Arrays.toString(X));
    System.out.println("Y: " + Arrays.toString(Y));

}

private static void mergeRunner(int[] x, int[] y) {

    if (x[x.length - 1] > y[y.length - 1]) {
        merge(x, y);
    } else {
        merge(y, x);
    }

}

private static void merge(int[] x, int[] y) {
    final int m = x.length;
    final int n = y.length;

    int last = x[m - 1];
    int i = m - 2;
    for (int j = n - 1; j >= 0; j--) {

        while (i >= 0 && x[i] > y[j]) {
            i--;
        }

        //Insertion
        insertValue(x, i + 1, y[j]);

        y[j] = last;
        last = x[m - 1];

        //System.out.println("X: " + Arrays.toString(x));
        //System.out.println("Y: " + Arrays.toString(y));
    }

}

private static void insertValue(int[] x, int index, int valueToInsert) {

    int i = x.length - 1;
    while (i > index) {
        x[i] = x[i - 1];
        i--;
    }
    x[i] = valueToInsert;
}

Your code is correct, but the problem you proposed can be solved in a less complicate manner, starting from your example these are some steps of my algorithm making comparisons between elements of the two arrays:
x = [1, 4, 7, 8, 10} , y = [2, 3, 9] step0 : comparing 10, 9
                  ^               ^

x = [1, 4, 7, 8, 9} , y = [2, 3, 10] step1 : 10, 9 swapped
                 ^                ^

x = [1, 4, 7, 8, 9} , y = [2, 3, 10] step2 : comparing 9, 3
                 ^            ^

x = [1, 4, 7, 8, 3} , y = [2, 9, 10] step3 : 3, 9 swapped
                 ^            ^

x = [1, 3, 4, 7, 8} , y = [2, 9, 10] step5 : reordered x with swaps, comparing 8, 9
                 ^            ^

This means that if you start from the right of the two arrays, if the x array element is greater than the array y element, the swap will be done and after you will reorder the x array swapping adiacent elements if one element is smaller than previous element in the array.
With the help of a method to swap elements between the two arrays like this below:
private static void swap(int i, int j, int[] arr1, int[] arr2) {
    int tmp = arr1[i];
    arr1[i] = arr2[j];
    arr2[j] = tmp;
}

Your method  mergeRunner can be rewritten like below:
private static void mergeRunner(int[] x, int[] y) {
    final int maxIndex = x.length - 1;

    for (int j = y.length - 1; j >= 0; --j) {

        if (y[j] < x[maxIndex]) {

            swap(maxIndex, j, x, y);

            for (int i = maxIndex; i > 0 && x[i - 1] > x[i]; --i) {

                swap(i, i - 1, x, x);
            }
        }
    }
}


Translating sentences with an API wrapper I've written a wrapper for this handy Yandex Translate API. I am pretty happy with is as it works just fine and the translations are accurate, even translations of entire sentences. However, I am curious if there is a better way to put the wrapper together, what would you do differently and what could be improved? I'll take any remarks there is.

import requests
from requests.exceptions import ConnectionError

class YandexException(Exception):
    """Common API wrapper exception."""
    pass

class Yandex:
    """YandexTranslate API wrapper."""

    API_VERSION = 'v1.5'
    URL = 'https://translate.yandex.net/api/v1.5/tr.json/'

    def __init__(self, api_key):
        """Use an API key provided by Yandex.
        To obtain a key, visit https://translate.yandex.com/developers
        """
        self.api_key = api_key
        self._session = requests.Session()

    @property
    def supported_languages(self):
        """Return a dictionary of currently supported languages.
        All languages are represented using ISO 639-1 language codes.
        """
        payload = {
            'ui': 'en',
            'key': self.api_key
        }
        try:
            response = self._session.get(self.URL + 'getLangs', params=payload)
            if response.status_code != 200:
                raise YandexException(response.json())
            return response.json().get('langs')
        except ConnectionError:
            raise YandexException('API unavailable')

    def _get_translate_direction(self, source, target):
        """Raise an exception if source and target language are not currently
        supported by the Yandex Translate API.
        """
        if source and target in self.supported_languages:
            return '{}-{}'.format(source, target)
        if source is None and target in self.supported_languages:
            return target
        raise YandexException('language direction not supported')

    def translate(self, text, target='en', source=None):
        """Return translated text.

        Translated text may be inaccurate if source language is not specified.
        This is because the API will try to detect source language, if the
        relevant argument is None.

        Text will be translated to English by default.

        Only use ISO 639-1 language codes with this method.
        """
        translate_direction = self._get_translate_direction(source, target)
        payload = {
            'text': text,
            'format': 'plain',
            'lang': translate_direction,
            'key': self.api_key
        }
        try:
            response = self._session.post(self.URL + 'translate', data=payload)
            if response.status_code != 200:
                raise YandexException(response.json())
            return response.json().get('text')[0]
        except ConnectionError:
            raise YandexException('API unavailable')

    def detect_language(self, text):
        """Return language of passed in text as a ISO 639-1 language code."""
        payload = {
            'text': text,
            'format': 'plain',
            'key': self.api_key
        }
        try:
            response = self._session.post(self.URL + 'detect', data=payload)
            if response.status_code != 200:
                raise YandexException(response.json())
            return response.json().get('lang')
        except ConnectionError:
            raise YandexException('API unavailable')

if __name__ == '__main__':
    t = Yandex(api_key)
    print(t.translate('Piękne jest lepsze niż brzydsze'))
    # => Beautiful is better than ugly

Dobra robota, tak trzymaj.

Your code is rather good code:

Docstrings, yay;
PEP 8 compliant (except a missing second blank line before if __name__ == '__main__');
Custom exception for the module;
Not executing top-level code on imports;
Sensible defaults;
Readable.

However, it's rather WET and you've got a silly rookie mistake.

Bug

Reading the docstring above, I'm pretty sure if source and target in self.supported_languages: is not checking what you intended to check.

This is parsed as if source and (target in self.supported_languages): which checks for the truth value of source rather than its existence in the self.supported_language dictionary.

Instead you could write:

def _get_translate_direction(self, source, target):
    """Raise an exception if source and target language are not currently
    supported by the Yandex Translate API.
    """
    if target not in self.supported_languages:
        raise YandexException('language target \'{}\' not supported'.format(target))
    if source is None:
        return target

    if source not in self.supported_languages:
        raise YandexException('language source \'{}\' not supported'.format(source))
    return '{}-{}'.format(source, target)

DRY it up

Each of the 3 method communicating with the API reuse the same exact structure and mostly share 7 lines in common. You could extract that to simplify things:

def _get_api_response(self, endpoint, payload):
    try:
        response = self._session.get(self.URL + endpoint, params=payload)
        if response.status_code != 200:
            raise YandexException(response.json())
        return response.json()
    except ConnectionError:
        raise YandexException('API unavailable')

@property
def supported_languages(self):
    payload = {
        'ui': 'en',
        'key': self.api_key
    }
    return self._get_api_response('getLangs', payload).get('langs')

def translate(self, text, target='en', source=None):
    translate_direction = self._get_translate_direction(source, target)
    payload = {
        'text': text,
        'format': 'plain',
        'lang': translate_direction,
        'key': self.api_key
    }
    return self._get_api_response('translate', payload).get('text')[0]

def detect_language(self, text):
    payload = {
        'text': text,
        'format': 'plain',
        'key': self.api_key
    }
    return self._get_api_response('detect', payload).get('lang')

Improvements

I’m not sure why you chose to .get elements out of the JSON response rather than accessing them directly with ['lang'] or ['text']: in case the API doesn't respond with the expected JSON, you won't figure it out. Worse, it will crash on other parts of the code with no obvious reasons.

For instance, if the getLangs call does not return a dictionary with a 'langs' key, response.json().get('langs') will return None. This will make the checks target in self.supported_languages raise TypeError because None is not iterable. This is not ideal. You should either:

access the key directly and catch the KeyError to throw your own exception;
or use a default value in your .get calls that is compatible with what is expected later.

For the second option, I'd use:

return self._get_api_response('getLangs', payload).get('langs', {})
return self._get_api_response('translate', payload).get('text', [None])[0]
return self._get_api_response('detect', payload).get('lang')

For the last two, None seems valid as it convey the "I couldn't detect/translate" meaning.

An other improvement I would make is to cache the returned value of supported_languages as there is very few chances that it changes during the lifespan of the application. I’d use functools.lru_cache for that:

from functools import lru_cache

class Yandex:
    ...
    @property
    @lru_cache(maxsize=1)
    def supported_languages(self):
        payload = {
            'ui': 'en',
            'key': self.api_key
        }
        return self._get_api_response('getLangs', payload).get('langs', {})

I chose a maxsize of 1 because there is little to no chance that more than 1 Yandex object get instanciated in a given application.

Lastly, I’m not aware if Yandex provide a sandboxed environment with some kind of public key that anyone can use with restrictions on the amount of requests, for instance. But if they do, it could be a great addition as a default value for the constructor.


Marking projects as done when all tasks' progress are 100% We have project.task model, where each task has many lines and each line has progress field indicating how much work has been done and how much still remaining (from 0 to 100 percent). The task cannot be done until all lines' progress is 100%.

@api.onchange('task_line_ids','task_line_rider_ids')
def progress_on_change(self):
    for record in self:
        if len(record.task_line_ids) != 0 and record.state in ('open', 'done'):
            all_done = True
            for line in record.task_line_ids or record.task_line_rider_ids:
                if line.progress != 100:
                    all_done = False
                    break
                if all_done == True:
                    record.state = 'done'
                else:
                    record.state = 'open'

However, SonarQube reported a code smell:

How can the complexity be reduced?

16 and 15 are pretty artificial values, you should aim for a better code quality rather than a higher/smaller metric in particular.

Let's see what can be improved...

Your boolean comparison if all_done == True: could (and should) be written in a more concise way: if all_done:.
Python has an optional else on for loops which can be understood as "this loop exited the usual way, not via a break". This removes the need for the all_done variable altogether:

if len(record.task_line_ids) != 0 and record.state in ('open', 'done'):
    for line in record.task_line_ids or record.task_line_rider_ids:
        if line.progress != 100:
            record.state = 'open'
            break
    else:
        record.state = 'done'

You could use the all or any builtin to rewrite this without the for... else logic:

if len(record.task_line_ids) != 0 and record.state in ('open', 'done'):
    if all(line.progress == 100 for line in record.task_line_ids or record.task_line_rider_ids):
        record.state = 'done'
    else:
        record.state = 'open'

You could re-introduce the temporary variable and use ternary operator:

if len(record.task_line_ids) != 0 and record.state in ('open', 'done'):
    all_done =  all(line.progress == 100 for line in record.task_line_ids or record.task_line_rider_ids)
    record.state = 'done' if all_done else 'open'


Basic Entity Framework Repository implementation Here is simple repository pattern that I use to access data in my database. I would like some advice on how to improve this code. Commenting on my code quality is also welcome too!

public static class ProdRepository
{
    private static Entities context = null;

    public static Entities Context
    {
        get
        {
            if (context == null)
            {
                return new Entities();
            }
            else
                return context;

        }
    }

    /// <summary>
    /// Getting Outbound list in selected period of dates
    /// </summary>
    /// <param name="dateFrom">begin date</param>
    /// <param name="dateTo">end date</param>
    /// <returns>Outbound List</returns>
    public static List<OutboundModel> GetOutboundsByDate(DateTime dateFrom, DateTime dateTo)
    {
        var q = from o in Context.wms_Outbound
                where o.CreateDT <= dateTo && o.CreateDT >= dateFrom
                select new OutboundModel
                {
                    Id = o.Id,
                    Code = o.Code,
                    CreationDate = o.CreateDT,
                    RootQuantity = o.RootQuantity
                };

        return q.ToList();

    }

}

Very nice and comprehensive. Your names are clear and short, your code is obvious in it's intent. I just see two things that startle me a bit:

if (context == null)
{
   return new Entities();
}
else
   return context;

what you do here is ... amusing. This code doesn't do what you think it does ;)

Both blocks are single line-statements, yet you put the first one in brackets and the second one you don't. This is inconsistent and prone to bugs. Let's say you want to log when someone accessed the context without a new instance created.

The "logical", but wrong code would be:

//if bla bla
else
    Logger.log("Context accessed.", Log.DEBUG);
    return context;

While this would work as planned if your if did what you seem to think it does, the actual flow of code is incorrect.

That whole thing should be rewritten as:

if (context == null)
{
    context = new Entities();
}
return context;

This does what you seem to have had in mind. In your code, you never assign to context, which remains null until all eternity. With each call to the getter you just return a new instance of Entities().

And the next thing that startles me is this:

public static List<OutboundModel> GetOutboundsByDate(DateTime dateFrom, DateTime dateTo)

in combination with the fact that your output is not sorted. Why? Using the Type List , especially in combination with the name, IMO implies a sorting of the data set. In this case your sorting is the Id, but for GetOutboundsByDate, I expect a data set that is ordered by date (and not by Id)

Instead I'd use IEnumerable<T>.


Getting B+ matrix (Warshall algorithm) in Matlab Pseudocode:

// B = nxn binary matrix
// Bm = resulting matrix
for (i=1; i<=n; i++)
{
  for (j=1; j<=n; j++)
  {
    if (B[i,j] == 1)
    {
      for (k=1; k<=n; k++)
      {
        Bm[i,j] = B[i,j] | B[k,j];
      }
    }
  }
}

This is the Warshall algorithm written (in my way):

B = [1 1 0 0 0; 0 0 0 1 0; 0 0 0 0 1; 0 1 0 0 0; 0 0 0 0 0];

n = 5;

Bm = zeros(n);

for i = 1:n
    for j = 1:n
        if B(i,j) == 1
            for k = 1:n
                Bm(i,k) = B(i,k) | B(k,j);
            end
        end
    end
end

It works but, how can I improve the matrix loops?

This post may help you in removing a loop:
http://mathforum.org/kb/message.jspa?messageID=845980

Furthermore, one way to speed up your code is to use a short circuit logical operator:

B(i,k) || B(k,j);

Also it seems to have a small effect if you use a logical matrix as input rather than a double:

B = logical(b);

Last and least, it would be nicer to initialize Bm with the datatype that it will have:

Bm = false(n)


Binary index tree optimization I have written code for a binary index tree. My problem is that there are many functions based on the binary index tree the function stores the sum of element within a given range of binary index tree. My task is to compute the sum of all the functions with provide range.

This example will clarify:

My array elements: A = 1 2 3 4 5

The function \$f(x,y)\$ is the sum of all elements of array between index \$x\$ and \$y\$ (inclusive)

\$f(1,3) = 6\$
\$f(2,5) = 14\$
\$f(4,5) = 9\$
\$f(3,5) = 12\$
\$f(1,2) = 3\$

For a given query of the form sum(a,b), I have to compute the sum of all the function from a to b.

sum(1,4) is the sum of the function from 1 to 4:

\$f(1,3) + f(2,5) + f(4,5) + f(3,5) = 6+14+9+12 = 41\$

The above data structure must also support for updating in the array.

update(a,b) replaces the element stored in index a of the array with b
update(3,7) will cause my new array to be A = 1 2 7 4 5

For any further clarification you can refer to this link.

My code is working for smaller range of input. How can I tune it to work for larger inputs?

def initupdate(x, y, n, lst):
    while x <= n:
        lst[x] += y
        x += (x & -x)

def update(x, y, n, lst):
    a = lst[x]
    initupdate(x, -a, n, lst)
    initupdate(x, y, n, lst)

def sumk(k, lst):
    res = 0
    while k:
        res += lst[k]
        k -= (k & -k)
    return res

def sumfunc(k, nfunc, lst):
        res = 0
        a, b = nfunc[k]
        res += sumk(b, lst)
        res -= sumk(a - 1, lst)
        return res

def sumfuncxy(x,y, nfunc, lst):
    res=0
    for k in range(x,y+1):
        res+=sumfunc(k,nfunc,lst)
    return res

n = int(input())
array = [0] + list(map(int, input().split()))
number = [ 0 for i in range(n + 1)]

for i in range(1, n + 1):
    initupdate(i, array[i], n, number)

nfunction = [[0, 0] for i in range(n + 1)]

for i in range(1, n + 1):
    lst = input().split()
    lst = list(map(int, lst))
    nfunction[i] = lst

q = int(input())

for i in range(q):
    lst = list(map(int, input().split()))
    if lst[0] == 1:
        'update'
        update(lst[1], lst[2], n, number)
    else:
        print(sumfuncxy(lst[1], lst[2], nfunction, number))

I know my naming of the variable is poor.

Is the choice of data structure (binary index tree) good for the problem?
Have I properly modularized my code?
Are there any vulnerability in my code?
Are there any further suggestions for cleaning the code?

Binary Indexed Tree is a suitable data structure for this question. However, you have to be careful of how you want to use it. For me, I would use it for storing values of "Functions".
Based on the code you posted here, it is difficult to tell if you modularized it properly. However, since you said it works for small input, then I believe that you have coded correctly. Try to add comments to each helper function, so others can easily follow the logic.
Add cases where input values exceed the maximum value or are below the minimum value specified in the original question under restriction section. Checking extreme cases is really important.
As you said yourself, the naming of the codes is poor. Try to use more meaningful name, i.e. in this problem, I would name "Function" as F, or "BIT_XXX" for each helper function that involves BIT implementation. Also, try to separate the main operations with helper functions (and add lots of comments).

Good luck!!


Computing the powerset of a list I've written the following code for computing the powerset of a list (we consider the list has all distinct elements, although it doesn't really matter).

Can this function be further optimized?

def powerset(A):
    if A == []:
        return [[]]
    a = A[0]
    incomplete_pset = powerset(A[1:])
    rest = []
    for set in incomplete_pset:
        rest.append([a] + set)
    return rest + incomplete_pset

This is reinventing-the-wheel, because the official documentation gives a recipe:

def powerset(iterable):
    "powerset([1,2,3]) --> () (1,) (2,) (3,) (1,2) (1,3) (2,3) (1,2,3)"
    s = list(iterable)
    return chain.from_iterable(combinations(s, r) for r in range(len(s)+1))

By using iterables it tackles the memory-optimization issue effectively.

However, it is possibly not as efficient as it could be because of the way it splits up the work.

To make your code more memory efficient, you could also introduce iterables. I'm assuming that you don't care about the order:

def powerset(A):
    if A == []:
        yield []
    else:
        a = A[0]
        for tail in powerset(A[1:]):
            yield tail
            yield [a] + tail

I've chosen tail to avoid aliasing the built-in set function.

The fastest memory-efficient approach is probably going to be an iterable using a Gray code to create a non-recursive solution which adds or removes a single element to the set between each yield.

Also, FWIW, your code could be more Pythonic.

    rest = []
    for set in incomplete_pset:
        rest.append([a] + set)

could be

rest = [[a] + tail for tail in incomplete_pset]

It's possible that my iterable rewrite of it could also be more Pythonic using a comprehension - I'm open to comments on this point.


Parsing and creating JSON packets For several of my pet projects, I have had to interpret and/or send JSON WebSocket or HTTP responses in Java. I did some research and picked the library GSON to use. In each project, I have implemented a custom deserializer in order to correctly interpret different packet types.

For example:

{"type": "set-name", "data":{"name":"daniel"}}
{"type": "set-timeperiod", "data":{"start":"2017-03-16", "end":"2017-03-17"}}

In this case I would use two classes, SetName and SetTimePeriod:

public class SetName implements Data {
    private String name;

    @Override
    public void handle(Foo foo) throws InvalidPacketException {
        if(name == null) throw new InvalidPacketException();
        foo.doThingWithName(name);
    }

    @Override
    public String getType() { return "set-name"; }
}

Foo represents any arbitrary resources, the packet might need to perform its function. The data interface is fairly straightforward:

public interface Data {
    void handle(Foo foo);
    String getType();
}

The Data objects are wrapped in a Packet representing the whole thing:

public void Packet {
    private String type;
    private Data data;

    public Packet(Data data) {
        this.data = data;
        type = data.getType();
    }

    public void handle(Foo foo) throws InvalidPacketException { data.handle(foo); }

    public Packet initData(Data data) {
        if(this.data != null) throw new IllegalStateException("Data already initialised.");
        this.data = data;
        return this;
    }
}

And finally, the packet would be converted from raw JSON to an instance of Packet using a custom Deserializer.

public class PacketDeserializer implements JsonDeserializer<Packet> {

    @Override
    public Packet deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context)
        throws JsonParseException {
        JsonObject jsonObject = json.getAsJsonObject();
        // Remove data object, GSON won't know what to turn it into.
        JsonObject dataObject = jsonObject.remove("data").getAsJsonObject();
        // Determine what type of data object we need.
        Class dataClass = typeToClass(jsonObject.get("type").getAsString());
        // Construct and return a Packet of the correct type.
        Data data = context.deserialize(dataObject, dataClass);
        return new Gson().fromJson(jsonObject, Packet.class).initData(data);
    }

    private Class<? extends Data> typeToClass(String type) throws JsonParseException {
        switch(type) {
            case("set-name"): return SetName.class;
            case("set-timeperiod"): return SetTimePeriod.class;
            default: throw new JsonParseException("Type " + type + " is invalid.");
        }
    }
}

This means GSON will now deserialize the object into a Packet and then call the handle() to do whatever needs to be done with that packet:

Packet packet = new GsonBuilder()
    .registerTypeAdapter(Packet.class, new PacketDeserializer())
    .create()
    .fromJson(message, Packet.class);
packet.handle(foo);

In addition, a Packet instance can be converted to JSON without the need for any custom serializer. Once this structure is built, it is very easy to add new packet types or to send/parse packets.

This seems like an elegant solution to me, but I have some concerns:

Is it actually efficient? Would it be much faster to parse the JSON directly?
I can end up with a lot of subclasses of Data which makes me feel like there could be a better way of doing it
If this is a good solution, would it be a good thing to use wherever I'm parsing JSON with some object with varying contents like data?

I came up with this solution years ago, when I was fairly new to programming and haven't changed it much at all, so I feel like I must be doing something wrong.

A quick braindump:

PacketDeserializer#deserialize  is overcommented. Most comments restate what the code says. Try commenting as little as possible, but as much as necessary.
PacketDeserializer could benefit from not hardcoding the mapping between the type strings and classes. Consider passing a Map<String, Class<? extends Data>> as constructor parameter and replacing typeToClass with a map.get
Lastly you should also avoid rawtypes. Class without generics specification is a rawtype.

Continuing that thought, you really want a different name than Foo for the "PacketHandler" you pass in to Packet#handle. You should consider going so far as to extract the responsibility of handling the package into a separate class. A Packet should not know how to handle itself. It's originally just a data-holder, nothing intelligent :)


Between dates : from highest -> current -> lowest For example I have 2 dates (year and month) like :

2013-03

2013-01 <-- Jan. current month.

2012-10

So my code is :

// Get current month and year
$m = date('m');
$y = date('Y');

// So I do something to get highest date so result is..
$highest = "2013-03";

// Current month and year
$current = $y."-".$m;

// So I do something to get lowest date so result is..
$lowest = "2012-10";

// My idea is get lower month from variables

function do_lower($date){
    $date = explode("-",$date);
    $y = $date[0];
    $m = $date[1];

    if($m != "01") {
        $m = $m-1;
        $m = sprintf('%02d',$m);
        return $y."-".$m;
    } else {
       return ($y-1)."-12";
    }

}

// This is my simple function to get result between high and low

function different($h,$l) {
    global $data;
    $h = do_lower($h);
    if($h!=$l){
        $data[] = $h;
        different($h,$l);
    }
}

Then .. to do all functions like.. (JSON)

$data[] = $highest;
different($highest,$current);
$data[] = $current;
different($current,$lowest);
$data[] = $lowest;

So my result is correct

["2013-03","2013-02","2013-01","2012-12","2012-11","2012-10"]

Anyone can review my code ?

Thanks so much

There's not really much to review here since it's such a short snippet, but a few things:

I tend to avoid manipulating dates as strings.  With just a year-month format it's actually fine since there's no odd rules. It's generally a lot safer to use DateTime though (or even strtotime/date). The problem with dates and times are that they are not simple units. There are complex, ever-changing 'business rules' (of sorts) that surround dates and times and cause huge headaches.

As said though, you're fine with year-date format since there's no gotcha's.

Just for consistency sake though, I would work with a standardized date format and use a date-time specific API.

For example:

$start = new DateTime(date('Y-m')); //First of this month
$end = new DateTime('2013-03'); //March 1st
$oneDay = new DateInterval('P1D');
$dates = array();
for (; $end >= $start; $end->sub($oneDay)) {
    $dates[] = $end->format('Y-m');
}

$dates is then an array in the format you want.  This code is quite ugly, but unfortunately I've never been able to figure out a way to manipulate dates in PHP that isn't ugly.

Here's a strtotime/date based version:

$start = strtotime(date('Y-m'));
$end = strtotime('2013-03');
$dates = array();
while ($end >= $start) {
    $dates[] = date('Y-m', $end);
    $end = strtotime(date('Y-m-d', $end) . ' +1 day');
}

Rather than the strtotime call, you could just use 864000 seconds. I'd have to think for a few minutes to make sure that won't have any DST mistakes though. Once again, a situation where I like to keep date operations as abstracted away as possible. 864000 is a lie since not all days have 24 hours (at least not in places that observe DST).

If I were to go with a string approach, your do_lower function is basically what I'd use.  It's named rather poorly though since it has no mention of what it's 'lowering' or what 'lowering' even means. Also, I would probably use a strict comparison against 01.

The problem with a purely string based approach is that it's not very reusable. Your function has a very specific input format and a very specific output format. Anything else and it breaks.

If you pass around DateTime's instead, dates are no longer represented as a string. They're represented as a type that is much more appropriate for such a complicated system. They're represented as a type that is aware that they are a date.


Sleeping barber problem in Erlang I'm learning Erlang. It's my first time with a non-imperative programming language.

I've written a code and I want some thoughts about it:

Is my logic easy to understand?
Is my code idiomatic?
Is my code well organized?
Is my code using Erlang features correctly?

My goal here is improve readability and functional-thinking. I'm not concerned about performance.

-module(sb).
-export([start/0, barberShop/3, clientGenerator/2, barber/2]).
-define(MAX_BARBERS, 1).
-define(MAX_SITS, 5).

start() ->
  BarberShopPID = spawn(sb, barberShop, [[], 0, yes]),
  spawn(sb, clientGenerator, [BarberShopPID, 10]).

clientGenerator(BarberShopPID, ClientsToGenerate) ->
  case ClientsToGenerate of
    0 ->
      BarberShopPID ! clientGeneratorIsDone;
    _ ->
      timer:sleep(random(1000, 6000)),
      Hair = random(100, 600),
      BarberShopPID ! {client, Hair},
      clientGenerator(BarberShopPID, ClientsToGenerate-1)
  end.

barberShop(Clients, Barbers, IsCGRunning) ->
  receive
    {client, Hair} ->
      case length(Clients) of
        ?MAX_SITS ->
          write("A client entered, but there are no available sits."),
          barberShop(Clients, Barbers, IsCGRunning);
        _ ->
          write("A client entered and found a available sit."),
          {NewC, NewB} = tryAttend([Hair | Clients], Barbers, self()),
          barberShop(NewC, NewB, IsCGRunning)
      end;
    barberIsDone ->
      {NewC, NewB} = tryAttend(Clients, Barbers-1, self()),
      case shouldContinue(NewC, NewB, IsCGRunning) of
        yes ->
          barberShop(NewC, NewB, IsCGRunning);
        no ->
          write("Bye from the glorious barber shop.")
      end;
    clientGeneratorIsDone ->
      case shouldContinue(Clients, Barbers, no) of
        yes ->
          {NewC, NewB} = tryAttend(Clients, Barbers, self()),
            barberShop(NewC, NewB, no);
        no ->
          write("Bye from the glorious barber shop.")
      end
  end.

barber(Hair, BarberShopPID) ->
  write("Barber started his job."),
  timer:sleep(Hair * 10),
  write("Barber has done his job."),
  BarberShopPID ! barberIsDone.

tryAttend(Clients, ?MAX_BARBERS, _) ->
  {Clients, ?MAX_BARBERS};
tryAttend([], Barbers, _) ->
  {[], Barbers};
tryAttend(Clients, Barbers, BarberShopPID) ->
  [First | Rest] = Clients,
  spawn(sb, barber, [First, BarberShopPID]),
  {Rest, Barbers+1}.

shouldContinue([], 0, no) ->
  no;
shouldContinue(_, _, _) ->
  yes.

random(Min, Max) ->
  random:uniform(Max-Min+1)+Min.

write(Message) ->
  io:format(Message ++ "~n", []).

Overall it looks good, I would just have some minor comments. In any case please have a look at the official Ericsson guidance.

Use macro ?MODULE instead of repeating sb.
In Erlang function names are usually_with_underscores rather than camelCasing. There are some exceptions, even in OTP applications, but in general underscores help to distinguish functions from variable names, which are usually CamelCased. The same holds for atom names.
I would usually put a code like this: [First | Rest] = Clients directly to the function argument to make it shorter, e.g.: tryAttend([First | Rest] = Clients, Barbers, BarberShopPID) ->
NewC and NewB aren't very descriptive. I would rather use NClients if NewClients are too long for you.
When accessing the head and tail of a list the names of variables are usually H and T, unless you want a specific name for the head, in which case the tail is just T or Tail, e.g. [H|T] = Clients or [First|T] = Clients. It's a convention which you will notice when reading someone else's code.
When spawning process with a function that doesn't need to be exported for its own sake try to use fun, e.g.: spawn(fun() -> barber(First, BarberShopPID) end). It's better to not expose functions that are not meant to be called from outside. And if you do have to export a function like that, then it's better to put it into a separate export with an additional comment, e.g. %% Internal API.
Didn't you want to write MAX_SEATS rather than MAX_SITS? :)


Critique Request: Reversing a singly linked list I find the tail in the first while loop and store it in the tailEnd node.
I update the tail to the previous node and change the tail.next to point to the previous node.
Once head.next.next == head, the reversing is finished.

package LinkedLists;

class Reverse {

    public static LinkedList reverse(LinkedList Node) {
        LinkedList head = Node;

        while (Node.next != null) {
            Node = Node.next;
        }

        LinkedList tail = Node;
        LinkedList tailEnd = Node;

        Node = head;

        while (Node.next != null) {
            if (head.next.next == head) {
                head.next = null;
                return tailEnd;
            }

            while (Node.next != tail) {
                Node = Node.next;
            }
            tail.next = Node;
            tail = Node;
            Node = head;
        }

        return tailEnd;
    }

    public static void main(String[] args) {
        LinkedList HeadNode = new LinkedList(1);
        LinkedList Node1 = new LinkedList(2);
        LinkedList Node2 = new LinkedList(3);
        LinkedList Node3 = new LinkedList(4);
        LinkedList TailNode = new LinkedList(5);
        HeadNode.next = Node1;
        Node1.next = Node2;
        Node2.next = Node3;
        Node3.next = TailNode;
        TailNode.next = null;
        LinkedList revHead = Reverse.reverse(HeadNode);
        while (revHead != null) {
        System.out.println(revHead.data);
        revHead = revHead.next;
        }
    }
}

This could be done simpler and more efficiently. This only requires one iteration through the list:

public static LinkedList reverse(LinkedList Node) {
    LinkedList previous = null;
    while (Node != null) {
        LinkedList next = Node.next;
        Node.next = previous;
        previous = Node;
        Node = next;
    }
    return previous;
}

On another note, it is more common for package names to be all lowercase and for local variable and parameter names to be camel case. For example:

package linkedlists;

public static LinkedList reverse(LinkedList node) {

LinkedList headNode = new LinkedList(1);


Advent of code: day 1 I am learning Haskell, and what is better than advent of code to do so? The day 1 problem is about adding together the digits that are followed by the same digit from a "circular" string (the next of the last is the first). My solution is the following:

import Data.List
import Data.Char

result :: [Char] -> Int
result s = sum . map (digitToInt . fst)
               . filter (uncurry (==))
               . zip s
               $ (drop 1 . cycle $ s)

My question is not only about the algorithm (I think that it is ok, and the problem is not that difficult) but also about the "haskellish" style.

Because I am used to Rust/Ocaml/Elm etc., at first I wrote:

import Data.List
import Data.Char
import Data.Function

result :: [Char] -> Int
result str = str & zip (drop 1 . cycle $ str)
                 & filter (uncurry (==))
                 & map (digitToInt . fst)
                 & sum

then I read that Haskell people prefer to compose functions and then pass the parameter, not doing the forward thing. What do you think about that?

Also, I searched over the Internet, and I found some divided code, more like that:

import Data.List
import Data.Char

transformCharPairsToInts :: [(Char, Char)] -> [Int]
transformCharPairsToInts = map $ digitToInt . fst

keepPairsWithSameChars :: [(Char, Char)] -> [(Char, Char)]
keepPairsWithSameChars = filter $ uncurry (==)

result :: [Char] -> Int
result str = sum . transformCharPairsToInts . keepPairsWithSameChars $ zip str shiftedStr
  where
    shiftedStr = drop 1 . cycle $ str

What is the most idiomatic thing? The style that Haskell people prefer?

The main:

-- exemple cases
main = mapM_ (print . result) $ ["1122", "1111", "1234", "91212129"]

Thank you for your remarks.

Personally I'd prefer to write this as an explicit fold. IMO that's somewhat cleaner and significantly easier to grasp when skimming

result :: [Char] -> Int
result cs@(c:_) = go $ cs ++ [c]
  where
  go :: [Char] -> Int
  go (c1:chars@(c2:_)) = (if c1 == c2 then digitToInt(c1) else 0) + (go chars)
  go _ = 0
result = 0

This removes a lot of text that I consider pretty confusing to read.


Calculate Ranked Probability Score I have a csv-file that consists of all match outcome probabilities for soccer matches. Each math can be result in a win, draw or loss. I also included the actual outcome. In order to test how accurate my predictions are I want to use the Ranked Probability Score (RPS). Basically, the RPS compares the cumulative probability distributions of the predictions and the outcome:

  \$ RPS = \frac{1}{r-1} \sum\limits_{i=1}^{r}\left(\sum\limits_{j=1}^i p_j - \sum\limits_{j=1}^i e_j \right)^2, \$

  where \$r\$ is the number of potential outcomes, and \$p_j\$ and
  \$e_j\$ are the forecasts and observed outcomes at position \$j\$.

For additional information, see the following link.

import numpy as np
import pandas as pd

def RPS(predictions, observed):
   ncat = 3
   npred = len(predictions)
   RPS = np.zeros(npred)

   for x in range(0, npred):
      obsvec = np.zeros(ncat)
      obsvec[observed.iloc[x]-1] = 1
      cumulative = 0
      for i in range(1, ncat):
          cumulative = cumulative + (sum(predictions.iloc[x, 1:i]) - sum(obsvec[1:i])) ** 2
          RPS[x] = (1/(ncat-1)) * cumulative

    return RPS

df = pd.read_csv('test.csv', header=0)
predictions = df[['H', 'D', 'L']]
observed = df[['Outcome']]
RPS = RPS(predictions, observed)

The first argument (predictions) is a matrix with the predictions and the corresponding probabilities.  Each row is one prediction, laid out in the proper order (H, D, L), where each element is a probability and each row sum to 1. The second argument (observed) is a numeric vector that indicates which outcome that was actually observed (1, 2, 3)

Feel free to give any feedback!
Thank you

Edit:

For some reason I am not able to reproduce the results of Table 3 of the link. I use Table 1 as input for predictions and observed. Any help is much appreciated!

Edit #2:

Hereby the small sample of the paper:

predictions = {'H': [1, 0.9, 0.8, 0.5, 0.35, 0.6, 0.6, 0.6, 0.5, 0.55],
               'D': [0, 0.1, 0.1, 0.25, 0.3, 0.3, 0.3, 0.1, 0.45, 0.1],
               'L': [0, 0, 0.1, 0.25, 0.35, 0.1, 0.1, 0.3, 0.05, 0.35]}

observed = {'Outcome': [1, 1, 1, 1, 2, 2, 1, 1, 1, 1]}

Tidy up your math

cumulative = cumulative + (sum(predictions.iloc[x, 1:i]) - sum(obsvec[1:i])) ** 2

can be

cumulative += (sum(predictions.iloc[x, 1:i]) - sum(obsvec[1:i])) ** 2

and

RPS[x] = (1/(ncat-1)) * cumulative

should be

RPS[x] = cumulative / (ncat-1)

Make a main method

This is a very small script, but still benefits from pulling your global code into a main.

PEP8

By convention, method names (i.e. RPS) should be lowercase.

That's all I see for now.


Creating objects to be added to a list When creating objects to be added to a list in Python, we can avoid the if statement by changing our create_object function to return a list.

Is this a good idea or not? Why?

(assume create_object is called in multiple places and is always added to a list)

def _create_object(self, data):
  if data:
    return Object(data)

if _create_object(data):
  mylist.append(_create_object(data))

or

def _create_object(self, data):
  if data:
    return [Object(data)]
  return []

# Now we no longer need the if
mylist += _create_object(data)

or (late addition) should we be using exceptions:

def _create_object(self, data):
  if data:
    return Object(data)
  raise BadDataException()

try:
  mylist.append(_create_object(data))
except BadDataException:
  pass

Avoiding an if statement is not a useful goal. Occasionally it is the correct means towards a goal, such as making code easier to write or to understand. Thus I think your question is too narrowly focused, or oversimplified. The problem here is that the wrapper function _create_object is too simple to merit being its own function, and the complexity outside of it matches that inside.

Let's consider some small transformations of this. First we'll take the if data outside of _create_object. I do this because I find the current implementation is close to do-what-I-mean. This may have been less true if the conditions were more intricate.

def _create_object(data):
    return Object(data)

if data:
    mylist.append(_create_object(data))

From there it's pretty obvious that _create_object is another spelling of Object, and the final transformation is to just remove the wrapper:

if data:
    mylist.append(Object(data))

Let's take a step further back and assume you're pulling subsequent values of data from a list that I'll call data_source. You can avoid the kind of if statement you had before by writing either a list comprehension (with embedded if), or using filter:

mylist += [Object(data) for data in data_source if data]
# or
mylist += [Object(data) for data in filter(None, data_source)]
# or, if _create_object's conditions are too complex for this transformation
mylist += [_create_object(data) for data in data_source if _create_object(data)]
# or, two-layers
mylist += filter(None, [_create_object(data) for data in data_source])

Note that I don't particularly like the third as it calls _create_object twice per item, but the fourth one requires your second wrapper - the one that returns None.

As for whether you should be using exceptions, the answer is two-fold. If you're dealing with a large number of false values in data, and this is in an inner loop, performance could be a problem. In cases like that, it can make sense to implement the look-before-you-leap semantics of your other examples. However, if Object cannot reasonably construct itself from a false data parameter, it should itself raise an exception. That way if a given caller doesn't check first, it will result in a clear fixable error.


Atm program xUnit unit test I have these 3 methods for ATM mock program.
    public decimal CheckBalance(BankAccount account)
    {
        return account.AccountBalance;
    }

    public void Deposit(BankAccount account, BankTransaction bankTransaction)
    {
        account.AccountBalance += bankTransaction.TransactionAmount;
    }

    public void Withdraw(BankAccount account, BankTransaction bankTransaction)
    {
        if(bankTransaction.TransactionAmount > account.AccountBalance)
        {
            throw new Exception("Withdraw failed. Transaction amount is more than account balance.");
        }

        account.AccountBalance -= bankTransaction.TransactionAmount;
    }

And here are my xUnit unit test methods. The test data is in-memory for this version.
    [Theory, MemberData(nameof(CheckBalanceShouldReturnValidBalanceAmount_Data))]
    public void CheckBalanceShouldReturnValidBalanceAmount(BankAccount account, decimal accountBalance)
    {
        // Act
        var balance = _atm.CheckBalance(account);

        // Assert
        Assert.Equal(accountBalance, balance);
    }

    [Theory, MemberData(nameof(DepositShouldPass_Data))]
    public void DepositShouldPass(BankAccount account, BankTransaction bankTransaction, BankAccount accountExpected)
    {
        // Act
        _atm.Deposit(account, bankTransaction);
        var balance = _atm.CheckBalance(account);

        // Assert
        Assert.Equal(accountExpected.AccountBalance, balance);
    }

    [Theory, MemberData(nameof(WithdrawShouldPass_Data))]
    public void WithdrawShouldPass(BankAccount account, BankTransaction bankTransaction, BankAccount accountExpected)
    {
        // Act
        _atm.Withdraw(account, bankTransaction);
        var balance = _atm.CheckBalance(account);

        // Assert
        Assert.Equal(accountExpected.AccountBalance, balance);
    }

    [Theory, MemberData(nameof(WithdrawAmountMoreThanBalanceShouldFail_Data))]
    public void WithdrawAmountMoreThanBalanceShouldFail(BankAccount account, BankTransaction bankTransaction)
    {
        // Assert and Act
        var exception = Assert.Throws<Exception>(() => _atm.Withdraw(account, bankTransaction));
        Assert.Equal("Withdraw failed. Transaction amount is more than account balance.",
            exception.Message);
    }

All tests passed successfully. Any comments on coding style of unit test?
Edited After Answer with extra class:
namespace XUnitSample.ClassLib.Models
{
    public class BankAccount
    {
        public int Id { get; set; }
        public int BankAccountNo { get; set; }
        public decimal AccountBalance { get; set; }
    }
}

namespace XUnitSample.ClassLib.Models
{
    public class BankTransaction
    {
        public int Id { get; set; }
        public decimal TransactionAmount { get; set; }
        public TransactionTypeEnum TransactionType { get; set; }
    }

    public enum TransactionTypeEnum
    {
        Deposit, Withdraw, ThirdPartyTransfer
    }
}

Is your mock ATM class literally named _atm?!  Or is it a non-static class that you have assigned to an instance named _atm?  Either way, I don't like it and find the _atm class to be useless in your limited example.
The BankAccount class, which you do not share, should have a Deposit and Withdraw method, both accepting a BankTransaction as a input argument.  You should include validation of a particular transaction within each method, i.e. do not accept a deposit of negative amount.  You also do not share BankTransaction with us.
CheckBalance is not needed, as you would just as easily show the BankAccount.AccountBalance property.  Also the property name could be shortened to Balance since it is the balance of the bank account.


Refine a math calculation function I'm developing a small website which is responsive, and I'm planning on displaying a number of items ( Demo ) on the page. As you can see in the link the items are displayed just fine, but what I'm trying to do is calculate the exact number of items that would fit inside the window ( more specific the actual space between the footer and the header ) so none of them will be shown only by half or overflown by the footer.

I got this small function working :

Cluster.prototype.initiate_items_per_page_calculation = function() {
    return Math.floor(Math.floor(($(window).width() - ($(window).width() * 4 / 100)) / $('body').find(this.cluster_public_classes.item_wrapper).outerWidth()) * Math.floor(Math.floor($(window).height() - ($('body').find('header').outerHeight() + $('body').find('footer').outerHeight()) - ($(window).height() * 4 / 100)) / $('body').find(this.cluster_public_classes.item_wrapper).outerHeight()));
};

Just that the calculations are not that exact. For instance I have the item's width of 260px and it's height 160px ( adding a few pixels because there two more layers of background behind the visible layer that are rotated 3deg, so there are about 5 pixels more on each side ) plus the white space between them which is 15px on each side, and there are the footer and the header heights which cut from the space, so taking all of this into consideration my calculations are not that accurate anymore. So I was wondering if I could get a little bit of help with making this work perfectly no matter what the screen size is (:

The best and easiest way to do this would be to place the items in a box container with a pre-computed height and width. Then in order to find the max amount of items that can be placed in the container, you just divide the area of the box container by the area of an item.

I used this method with your previous function and got something like this.

Cluster.prototype.initiate_items_per_page_calculation = function () {
    var o = this.get_init_calculation_object(),
        areaOfHeaderAndFooter = ( o.header.w * o.header.h ) + ( o.footer.w * o.footer.h ),
        areaOfExtraPaddingToContainer = 150 * 250 * 2,
        areaOfBoxContainer = (o.screen.w * o.screen.h ) - areaOfHeaderAndFooter - areaOfExtraPaddingToContainer,
        areaOfBox = (o.item.w * o.item.h),
        containerBoxLimit = Math.floor( areaOfBoxContainer / areaOfBox );

    return containerBoxLimit;
};
Cluster.prototype.get_init_calculation_object = function () {
    var $body = $('body'),
        $header = $body.find('header'),
        $footer = $body.find('footer'),
        $item = $body.find(this.cluster_public_classes.item_wrapper);

    return {
        footer:{
            h: $footer.outerHeight(true),
            w: $footer.outerWidth(true)
        },
        header:{
            h: $header.outerHeight(true),
            w: $header.outerWidth(true)
        },
        item:{
            h: $item.outerHeight(true),
            w: $item.outerWidth(true)
        },
        screen:{
            h: $body.outerHeight(true),
            w: $body.outerWidth(true)
        }
    };
};


TabPage version of SelectedIndexChanged I order to speed up opening of a file (which contents are displayed in a series of tabs), I made it so that tab content is generated only when the user first clicks on the tab. If user never opens the tab, code never executes.

The problem is I could not any find any TabPage events that would trigger when that tab is selected. I had to use SelectedIndexChanged event from the TabControl.

In order to make it work, I created a new interface (ICreateTab) that every TabPage implemented. It only has CreateTab() method which would contain the code that would generate data for the TabPage.

This is the snippet of the code:

    TabControl tabs;
    bool[] tabCreated;

    public ConstructorForClassThatHasTabControl()
    {
        tabs = new TabControl();
        tabs.Controls.Add(new TabPageWithImplementedICreateTabInterface1());
        .
        .
        .
        tabs.Controls.Add(new TabPageWithImplementedICreateTabInterfaceN());

        tabs.SelectedIndexChanged += OnSelectedIndexChanged;

        tabCreated = new bool[tabs.TabCount];

        // Create first tab, since the event won't be initially triggered
        ((ICreateTab)tabs.Controls[0]).CreateTab();
        tabCreated[0] = true;

        Controls.Add(tabs);
    }

    private void OnSelectedIndexChanged(object sender, EventArgs e)
    {
        if (!tabCreated[tabs.SelectedIndex])
        {
            tabCreated[tabs.SelectedIndex] = true;
            ((ICreateTab)tabs.SelectedTab).CreateTab();
        }
    }

This looks very ugly and messy to me. I'd rather each TabPage have an event that would create it's contents and remove itself so it's not triggered again. I need to avoid dragging TabControl into this matter.

Does TabPage have an event that is triggered when the TabPage is selected? Even if it doesn't, is there a proper way of doing this? I find it hard to believe that TabPage does not know when it's selected.

I don't think there is any other way to solve it. By using the  SelectedIndexChanged event and initializing a TabPage you do it just the way it should/can be done.

You may however consider renaming the interface to IInitializable and the method to Initialize which more clearly describes what you are doing. You don't create any tabs, you initialize them so name the interface and the method accordingly.

Internally you can use a boolean field _isInitialized so that the Initialize method checks it and doesn't initialize your tab-page multiple times. Then you just do:

private void OnSelectedIndexChanged(object sender, EventArgs e)
{
    var initializableTabPage = (sender as TabControl).SelectedTab as IInitializable;

    // In case you have other tab-pages.
    if (initializableTabPage != null)
    {
        initializableTabPage.Initialize();
    }
}

and the tap-page with its interface:

interface IInitializable
{
    void Initialize();
}

class MyTabPage : TabPage, IInitializable
{
    private bool _isInitialized;

    public void Initialize()
    {
        if (_isInitialized) { return ; }

        ...initialize

        _isInitialized = true;
    }
}

  I find it hard to believe that TabPage does not know when it's selected.
  It looks like it really doesn't:
  TabPage Class Events

You could experimet with GotFocus but it can be tricky. I wouldn't


Continuously execute runnables on the UI thread I need to run a sequence of runnables on the main (ui) thread with a given delay between them indefinitely until signalled to stop. Code:

public class UiLoop {

    private Handler h;
    private AtomicBoolean breakLoop;

    @UiThread
    public UiLoop() {

        if (Looper.myLooper() != Looper.getMainLooper()) {
            throw new RuntimeException("Not called on ui thread!");
        }

        h = new Handler();
        breakLoop = new AtomicBoolean(false);
    }

    /**
     * Break out of the loop on the next iteration.
     */
    public void breakLoop() {
        this.breakLoop.set(true);
    }

    /**
     * @param workers    A Deque of runnables to execute sequentially.
     * @param delay The delay between executing the runnables. This is excluding the time it
     *              takes for the runnable to complete.
     */
    @UiThread
    public void loop(Deque<Runnable> workers, int delay) {

        if (Looper.myLooper() != Looper.getMainLooper()) {
            throw new RuntimeException("Not called on ui thread!");
        }

        if (workers.isEmpty()) return;

        if (breakLoop.get()) return;

        final Runnable first = workers.removeFirst();
        h.post(first);
        workers.addLast(first);
        h.postDelayed(() -> loop(workers, delay), delay);

    }
}

Is the above code OK?

PS: I am using retrolambda to convert Anonymous Inner Classes to lambda functions.

Your variable names could be more clear.  h is a poor choice, for example.  loop is probably not a great name for a method either - maybe startLoop?
You have inconsistent code style.  Some one-line if blocks are in curlies while others are not.
Why final Runnable first?  It's fine if you want to use final wherever possible, but that doesn't appear to be the case with the rest of the code, and there's no explicit reason for it be final, so what is the strategy here?
I'd save the Runnable you pass to postDelayed as an instance member instead of creating a new one each time.
Why do you use an explicit this in this.breakLoop.set, but nowhere else?  I'd choose one or the other.
If you've got those tests to ensure everything is happening on the main thread, then why h.post(first)?  Wouldn't first.run() to the same thing?


Decreasing complexity of loop & Find- VBA I have written the following code; both Sheet 1 and Sheet 2 are rather big sheet with lots of data. Run this Macro is extremely time comsuming ( It has a high complexity I guess). I'm quite new at VBA and therefore having trouble make the code more effective.

Sub Find()

    Dim rgFound As Range
    Dim Index As Long: Index = 6
    Dim Row as Long

    Do While Worksheets("Sheet2").Cells(Index, "D").Value > 0

         Sheets("Sheet1").Select
         Set rgFound = Range("A1:A20000").Find(Worksheets("Sheet2").Cells(Index, "D").Value)

         If Not rgFound Is Nothing Then

            Row = rgFound.Row

            Worksheets("Sheet1").Range("E" & Row).Value = Worksheets("Sheet2").Range("AA" & Index).Value
            Worksheets("Sheet1").Range("F" & Row).Value = Worksheets("Sheet2").Range("AB" & Index).Value
            Worksheets("Sheet1").Range("G" & Row).Value = Worksheets("Sheet2").Range("AC" & Index).Value
            Worksheets("Sheet1").Range("H" & Row).Value = Worksheets("Sheet2").Range("Z" & Index).Value
            Worksheets("Sheet1").Range("J" & Row).Value = Worksheets("Sheet2").Range("AG" & Index).Value
            Worksheets("Sheet1").Range("I" & Row).Value = Worksheets("Sheet2").Range("AD" & Index).Value

        Else
            ' Function // Not done yet
        End If

    Index = Index + 1
    Loop
End Sub

Is the built in Find function effective? The loop loops trough roughly 250-400 values.

Basically, Sheet 2 refers to a specific month. Sheet one contians 12 rows for each employee ( one for each month ). Each Sheet (Sheet2-Sheet13) Have exactly the same format. The values copied from these sheets to Sheet1 are salary, days worked and so forth.

I'll start with the main performance suggestions, but there are a few other issues as well

By far, the most important improvement to be made is to work in memory (arrays, dictionaries)
If moving data to arrays is not possible:

turn off the display, events, calculations, page breaks, etc
use With statements to cache the objects
loops: minimum amount of work necessary, and exit them as soon as possible
one of the few reasons to interact with the ranges is when changing formats for all cells

To illustrate the points I'll provide 2 versions and compare timings

v1 - your code
v2 - screen updating off
v3 - arrays

3 tests each, with 10,000 rows on both sheets:

  v1: 64.164 sec, 40.539 sec, 88.797 sec
  v2: 11.969 sec, 12.055 sec, 11.156 sec
  v3:  0.031 sec,  0.281 sec,  0.033 sec Arrays

Here is Version 2:

Public Sub UpdateData2()
    Dim ws1 As Worksheet, ws2 As Worksheet, r As Long, fr As Long, found As Range, t As Long

    t = Timer
    r = 6
    Set ws1 = ThisWorkbook.Worksheets("Sheet1")
    Set ws2 = ThisWorkbook.Worksheets("Sheet2")
    With Application
        .ScreenUpdating = False
        .EnableEvents = False
    End With
    With ws2
        Do While .Cells(r, "D").Value > 0
            Set found = ws1.Range("A1:A20000").Find(.Cells(r, "D").Value)
            If Not found Is Nothing Then
                fr = found.Row
                ws1.Range("E" & fr).Value2 = .Range("AA" & r).Value2
                ws1.Range("F" & fr).Value2 = .Range("AB" & r).Value2
                ws1.Range("G" & fr).Value2 = .Range("AC" & r).Value2
                ws1.Range("H" & fr).Value2 = .Range("Z" & r).Value2
                ws1.Range("J" & fr).Value2 = .Range("AG" & r).Value2
                ws1.Range("I" & fr).Value2 = .Range("AD" & r).Value2
            Else
                ' Function // Not done yet
            End If
            r = r + 1
        Loop
    End With
    With Application
        .ScreenUpdating = True
        .EnableEvents = True
    End With
    Debug.Print "Time: " & Format(Timer - t, "0.000") & " sec"
End Sub

Version 3 (Arrays and Dictionary):

  For dictionaries late binding is slow: CreateObject("Scripting.Dictionary")

  Use Early Binding: VBA Editor -> Tools -> References -> Add Microsoft Scripting Runtime

Public Sub UpdateData3()
    Const FIELDS = 6, MIN_WS2 = 6
    Dim ws1 As Worksheet, ws2 As Worksheet, search1 As Variant, search2 As Variant
    Dim max1 As Long, max2 As Long, ur1 As Variant, ur2 As Variant, uniques As Dictionary
    Dim maps(1 To FIELDS, 1 To 2) As Byte, r As Long, itm As Variant, fld As Long, t As Long

    t = Timer:          Set uniques = New Dictionary
    maps(1, 1) = 5:     maps(1, 2) = 27     'E to AA
    maps(2, 1) = 6:     maps(2, 2) = 28     'F to AB
    maps(3, 1) = 7:     maps(3, 2) = 29     'G to AC
    maps(4, 1) = 8:     maps(4, 2) = 26     'H to Z
    maps(5, 1) = 9:     maps(5, 2) = 30     'I to AD
    maps(6, 1) = 10:    maps(6, 2) = 33     'J to AG
    Set ws1 = ThisWorkbook.Worksheets("Sheet1"):    max1 = ws1.UsedRange.Rows.Count
    Set ws2 = ThisWorkbook.Worksheets("Sheet2"):    max2 = ws2.UsedRange.Rows.Count
    ur1 = ws1.Range(ws1.Cells(1, "A"), ws1.Cells(max1, "J"))
    ur2 = ws2.Range(ws2.Cells(1, "A"), ws2.Cells(max2, "AG"))
    search1 = ws1.Range(ws1.Cells(1, "A"), ws1.Cells(max1, "A"))
    search2 = ws2.Range(ws2.Cells(1, "D"), ws2.Cells(max2, "D"))
    For r = MIN_WS2 To max2
        uniques(Trim$(search2(r, 1))) = r
    Next
    For r = 1 To max1
        itm = Trim$(search1(r, 1))
        If uniques.Exists(itm) Then
            For fld = 1 To FIELDS
                ur1(r, maps(fld, 1)) = ur2(uniques(itm), maps(fld, 2))
            Next
            uniques.Remove itm
        End If
        If uniques.Count = 0 Then Exit For
    Next
    ws1.Range(ws1.Cells(1, "A"), ws1.Cells(max1, "J")) = ur1
    Debug.Print "Rows: " & max1 & "; Time: " & Format(Timer - t, "0.000") & " sec"
End Sub

Other notes related to your code

It's good to qualify references to sheets and ranges, but it's even better to use With statements for performance and maintenance
Select and Activate are almost never needed, and impact performance

Sheets("Sheet1").Select
  Set rgFound = Range("A1:A20000").Find...

should be replaced with Sheets("Sheet1").Range("A1:A20000").Find...

Variable names and sub names like Find, Index, and Row overwrite Excel built-in functions and properties like Sheet1.Row(1), WorkSheet/WorkBook indexes, etc
Make all ranges dynamic, as opposed to Range("A1:A20000") (determine last row and column at run-time)
The Do While loop makes the assumption that a value in column D will never be deleted or empty in the middle of the data
Cells().Value2 is a bit faster than Cells().Value if Currency and Date are not a factor


Hurst Exponent calculator I am aiming to compute the Hurst Exponent of a 1-D signal time series in Python.  For now, I have one existing function hurst(sig) which returns the Hurst exponent of sig as a float.  Unfortunately, the code runs very slowly even for signals with only ~7500 data points.  My main concerns are:

Calculate the computational complexity of my algorithm given the size of sig.
Reduce computational complexity of the algorithm.
Reduce run-time (~1s).
Improve readability and address style convention as applies.

My code, awaiting your feedback:

import numpy as np

def hurst(sig: np.ndarray) -> float:
    """Compute the Hurst exponent of sig.

    arguments
        sig -- 1D signal
    returns
        hurst_exponent -- float
    """

    n = sig.size # num timesteps
    t = np.arange(1, n + 1)
    y = sig.cumsum() # marginally more efficient than: np.cumsum(sig)
    mean_t = y / t # running mean

    s_t = np.zeros(n)
    r_t = np.zeros(n)

    for i in range(n):
        s_t[i] = np.std(sig[:i + 1])
        x_t = y - t * mean_t[i]
        r_t[i] = np.ptp(x_t[:i + 1])

    r_s = r_t / s_t
    r_s = np.log(r_s)[1:]
    n = np.log(t)[1:]
    a = np.column_stack((n, np.ones(n.size)))
    [hurst_exponent, c] = np.linalg.lstsq(a, r_s)[0]
    return hurst_exponent

A few observations:

s_t = np.zeros(n) and r_t = np.zeros(n) are more than you need. Since you don't actually use the array values but solely overwrite them, you can use np.empty here.

You're doing quite a bit of redundant work in the for loop.

When calculating s_t[i], numpy basically has to repeat all the computations it has already done for s_t[i-1]. If you have a look at the definition of np.std, you can see that the mean plays a role here. Since you have already come up with a nice solution for the mean value, you could reuse it here:

s_t = np.empty(n)

for i in range(n):
    s_t[i] = np.mean((sig[:i+1] - mean_t[i])**2)
    ...
s_t = np.sqrt(s_t)

I validated this against the original code with np.allclose.

r_t also has some extra work that you can get rid of. At the moment your code does the following:

x_t = y - t * mean_t[i]
r_t[i] = np.ptp(x_t[:i + 1])

All the values of x_t at [i+1:n] are computed, but never used, so you can do

r_t[i] = np.ptp(y[:i+1] - t[:i+1] * mean_t[i])

instead, without changing the outcome. You could also write this as two list comprehensions:

s_t = np.sqrt(
    np.array([np.mean((sig[:i+1] - mean_t[i])**2) for i in range(n)])
)
r_t = np.array([np.ptp(y[:i+1] - t[:i+1] * mean_t[i]) for i in range(n)])

Or even a single one if you'd like to get a little bit creative:

rs_t = np.array([
    [
        np.mean((sig[:i + 1] - mean_t[i])**2),
        np.ptp(y[:i + 1] - t[:i + 1] * mean_t[i])
    ] for i in range(n)
])
rs_t[:, 0] = np.sqrt(rs_t[:, 0])

A fully vectorized computation of x_t I have come up with, starting from

x_t = y.reshape(1, -1) - mean_t.reshape(-1, 1) @ t.reshape(1, -1)

is considerably slower than the looped version.

I'm actually not quite sure about the rest of the code from this point on at the moment. I will have to take a closer look to give a meaningful review performancewise. As a first non-functional feedback I would like to invite you to think about more meaningful variable names in the future. Especially there at the end of the code around the least-squares optimization, they are quite hard to understand.

I did some timing of different versions of this algorithm. The full benchmark code can be found in this gist. This is what I got so far (for n=15360):

base:              3.748915s
better_loop:       2.605374s
double_lc:         2.430255s
single_lc:         2.445943s
-----------------------
base_numba:        0.866583s
better_loop_numba: 0.813630s
double_lc_numba:   0.776361s
single_lc_numba*:  N/A

The lower group of results uses numba, a just-in-time compiler for Python code, that can sometimes help to speed up loops. Unfortunately, numba does not support all types of array operations in its fast nonpython mode, e.g. I could not get the single list comprehension version to properly work with numba.

The results are actually a little bit worse than I had originally expected, especially without numba, but there is still a good amount profiling work to be done here.


Templated division by a power of 2 I've implemented a div_by_power_of_2() function, which lets me force the compiler to use left-shifting rather than proper division, in cases when the developer know the divisor will be a power of 2, but does not know which power of 2; and the compiler cannot prove to itself that this is the case.

template <typename P> constexpr P log2_of_power_of_2(P non_negative_power_of_2) noexcept
{
    static_assert(std::is_integral<P>::value, "Only integral types are supported");
    static_assert(sizeof(P) <= sizeof(unsigned long long), "Unexpectedly large type");
    using cast_target_type = typename
        std::conditional<sizeof(P) <= sizeof(unsigned),
            unsigned,
            typename std::conditional<sizeof(P) <= sizeof(unsigned long),
                unsigned long,
                unsigned long long
            >::type
        >::type;
    return log2_of_power_of_2<cast_target_type>(
        static_cast<cast_target_type>(non_negative_power_of_2));
}
template <>
constexpr unsigned
log2_of_power_of_2<unsigned>(unsigned non_negative_power_of_2) noexcept
{ return __builtin_ctz  (non_negative_power_of_2); }

template <>
constexpr unsigned long
log2_of_power_of_2<unsigned long>(unsigned long non_negative_power_of_2)  noexcept
{ return __builtin_ctzl (non_negative_power_of_2); }

template <>
constexpr unsigned long long
log2_of_power_of_2<unsigned long long>(unsigned long long non_negative_power_of_2) noexcept
{ return __builtin_ctzll(non_negative_power_of_2); }

template <typename I, typename P>
constexpr I div_by_power_of_2(I dividend, P power_of_2_divisor) noexcept
{ return dividend >> log2_of_power_of_2(power_of_2_divisor); }

Questions:

Is my approach to covering the possible types of power_of_2 appropriate? Can it perhaps be made less verbose but with the same effect?
Am I reinventing the wheel with this code?
Currently, this depends on certain compiler intrinsics available in GCC and clang but not necessarily elsewhere. I could generalize it a bit using this method to also support MSVC. Is there a better approach to generalizing the code?
Should I change the return type of log2_of_power_of_2 functions to be uniform rather than I? e.g. an unsigned?
Any other comments/suggestions are welcome.

Notes:

This is intended to be C++11; obviously with C++17 I could simplify it further
The constexpr qualifier is not very meaningful, since in a constexpr context we could just do plain division, but I've tacked it on nonetheless. To make the use of these utility function(s) more uniform.

The code is missing #include <type_traits>; it would have been nice to have had the test cases too.
It would be nice to be able to check the argument restriction using assert.  Unfortunately, this doesn't fit nicely with C++11 constexpr functions, which may contain only a single return statement.  If you might also compile against a newer standard, we could perhaps conditionally test it:
#include <cassert>

template <typename I, typename P>
constexpr I div_by_power_of_2(I dividend, P power_of_2_divisor) noexcept
{
#if __cplusplus >= 201402L
    assert((power_of_2_divisor & power_of_2_divisor - 1) == 0);
#endif
    return dividend >> log2_of_power_of_2(power_of_2_divisor);
}

I don't see why we have a template and specializations, rather than simple overloading of log2_of_power_of_2, particularly as we don't widen signed types to preserve sign.
Contrary to your comment, the constexpr is valuable, as it allows this function to be called from within another constexpr function, whether or not that one is being called with constant arguments.

Modified code
static constexpr unsigned
log2_of_power_of_2(unsigned non_negative_power_of_2) noexcept
{ return __builtin_ctz  (non_negative_power_of_2); }

static constexpr unsigned long
log2_of_power_of_2(unsigned long non_negative_power_of_2)  noexcept
{ return __builtin_ctzl (non_negative_power_of_2); }

static constexpr unsigned long long
log2_of_power_of_2(unsigned long long non_negative_power_of_2) noexcept
{ return __builtin_ctzll(non_negative_power_of_2); }

static constexpr int
log2_of_power_of_2(int non_negative_power_of_2) noexcept
{ return __builtin_ctz  (non_negative_power_of_2); }

static constexpr long
log2_of_power_of_2(long non_negative_power_of_2)  noexcept
{ return __builtin_ctzl (non_negative_power_of_2); }

static constexpr long long
log2_of_power_of_2(long long non_negative_power_of_2) noexcept
{ return __builtin_ctzll(non_negative_power_of_2); }

#include <cassert>

template <typename I, typename P>
constexpr I div_by_power_of_2(I dividend, P power_of_2_divisor) noexcept
{
#if __cplusplus >= 201402L
    assert((power_of_2_divisor & power_of_2_divisor - 1) == 0);
#endif
    return dividend >> log2_of_power_of_2(power_of_2_divisor);
}

#include <iostream>
int main()
{
    std::cout << div_by_power_of_2(15ul, 4) << '\n';
}


Recursively patching the properties of an ORM model and related models The following excerpt is part of a method for recursively patching the properties of an ORM model and related models using a dictionary:

try:
    anhaenge = d['anhaenge']
except KeyError:
    self.logger.debug('anhaenge unchanged')
else:
    try:
        anhang = anhaenge['anhang']
    except KeyError:
        self.logger.debug('anhaenge.anhang unchanged')
    except TypeError:  # anhaenge is probably None
        for record in self.anhang:
            yield (record, PatchMode.DELETE)
    else:
        for record in self.anhang:
            yield (record, PatchMode.DELETE)

        if anhang:
            for anhang_ in anhang:
                with suppress(EmptyDict):
                    for record in Anhang.from_dict(
                            anhang_, immobilie=self):
                        yield (record, PatchMode.CREATE)

I currently violate the don't repeat yourself (DRY) principle, since I do the very same operation within the block handling the TypeError and at the beginning of the inner else block.
How can I resolve this to have the code

for record in self.anhang:
    yield (record, PatchMode.DELETE)

only once?

Clarifying the input:

The inputd is a dictionary derived by json.loads() from a JSON string through a web API.
The method, the above excerpt is extracted from uses this dictionary to modify database records represented by the ORM model this methods belongs to.
Examples of d:

# Delete all subsequent anhang records
d = {'anhaenge': {
    'anhang': None}}

# Change all related anhang records to those represented by the dicts in
# the list, meaning:
# 1) Delete all old records
# 2) Create new ones from the dicts
d = {'anhaenge': {
    'anhang': [
        {'anhangtitel': 'Attachment1'},
        {'anhangtitel': 'Attachment2'}]}}

# All subsequent anhang records are left as they are.
# Subsequent foo records may be altered accordingly
d = {'anhaenge': {
    'foo': 'bar'}}

If we assume that either for loop can come before or after the other for loop.
Then you should check if anhaenge is not None.
As your except TypeError is to guard against that.
You can then return from the function after you log the error, remove the except TypeError, remove the duplicate for loop,
and finally, put the duplicate for loop after the if, as you wish to perform the loop here on either branch. EG:

try:
    anhaenge = d['anhaenge']
except KeyError:
    self.logger.debug('anhaenge unchanged')
else:
    if anhaenge is not None:
        try:
            anhang = anhaenge['anhang']:
        except KeyError:
            self.logger.debug('anhaenge.anhang unchanged')
            return
        else:
            for anhang_ in anhang:
                with suppress(EmptyDict):
                    for record in Anhang.from_dict(anhang_, immobilie=self):
                        yield record, PatchMode.CREATE
    for record in self.anhang:
        yield (record, PatchMode.DELETE)

But this is not possible if the duplicated for loop has to occur before the other for loop.

Either way, I honestly don't think you're input is right.
Your input is strange, None is normally used to say it's not there, and so dict.get('a', None) is a common occurrence.
However your code performs differently whether it's None or not there.
And so I'd say you should restructure the input so you can use say:

anhang = d.get('anhaenge', {}).get('anhang', None)
if anhang:
    for record in self.anhang:
        yield (record, PatchMode.DELETE)

    for anhang_ in anhang:
        with suppress(EmptyDict):
            for record in Anhang.from_dict(
                    anhang_, immobilie=self):
                yield (record, PatchMode.CREATE)
else:
    self.logger.debug('anhaenge or anhaenge.anhang is unchanged')

If you can't adapt either of the above, then I'd say your code is good for your situation.


Listing every date between two dates in input fields Using the following HTML:

<input type="date" value="2015-07-26" name="startdate">
<input type="date" value="2015-07-29" name="enddate">

First, the JS calculates the number of days between two dates:

var oneDay = 24*60*60*1000; // hours*minutes*seconds*milliseconds
var start = document.querySelector('[name="startdate"]').value;
var end = document.querySelector('[name="enddate"]').value;
var firstDate = new Date(start);
var secondDate = new Date(end);
var diffDays = Math.round(Math.abs((firstDate.getTime() - secondDate.getTime())/(oneDay)));

↑ startdate is the name of the first input field and enddate is the name of the second field.

Date.prototype.addDays = function(diffDays) {
    var dat = new Date(this.valueOf());
    dat.setDate(dat.getDate() + diffDays);
    return dat;
};

function getDates(startDate, stopDate) {
    var dateArray = [];
    var currentDate = startDate;
    while (currentDate <= stopDate) {
        dateArray.push(currentDate);
        currentDate = currentDate.addDays(1);
    }
    return dateArray;
}

var dateArray = getDates(new Date(start), (new Date(start)).addDays(diffDays));

↑ That is the part of the script I'm not really contented with, because I can imagine that the code could have been written simpler.

for (i = 0; i < dateArray.length; i++) {
    var input = document.createElement('input');
    var br = document.createElement("br");
    input.setAttribute("value", dateArray[i]);
    input.setAttribute("type", "date");
    document.body.appendChild(input);
    document.body.appendChild(br);
}

↑ Finally, it'll create for each day (between two dates) an input field, plus an linebreak after each input.

By the way, it would be nice to use the date format "DD.MM.YYYY", instead of "YYYY-MM-DD".

// Calculate number of days between two dates
var oneDay = 24*60*60*1000; // hours*minutes*seconds*milliseconds
var start = document.querySelector('[name="startdate"]').value;
var end = document.querySelector('[name="enddate"]').value;
var firstDate = new Date(start);
var secondDate = new Date(end);
var diffDays = Math.round(Math.abs((firstDate.getTime() - secondDate.getTime())/(oneDay)));

Date.prototype.addDays = function(diffDays) {
   var dat = new Date(this.valueOf());
   dat.setDate(dat.getDate() + diffDays);
   return dat;
};

function getDates(startDate, stopDate) {
    var dateArray = [];
    var currentDate = startDate;
    while (currentDate <= stopDate) {
        dateArray.push(currentDate);
        currentDate = currentDate.addDays(1);
    }
    return dateArray;
}

var dateArray = getDates(new Date(start), (new Date(start)).addDays(diffDays));

// create input fields (for each date one field)
for (i = 0; i < dateArray.length; i++) {
    var input = document.createElement('input');
    var br = document.createElement("br");
    input.setAttribute("value", dateArray[i]);
    input.setAttribute("type", "date");
    document.body.appendChild(input);
    document.body.appendChild(br); // linebreak after each input field
}
<input type="text" value="2015-07-26" name="startdate">
<input type="text" value="2015-07-29" name="enddate">

I wouldn't bother with calculating the difference - especially as it's not actually used for anything.

Instead I'd just do this:

var start = new Date(document.querySelector('[name="startdate"]').value),
    end = new Date(document.querySelector('[name="enddate"]').value),
    year = start.getFullYear(),
    month = start.getMonth()
    day = start.getDate(),
    dates = [start];

while(dates[dates.length-1] < end) {
  dates.push(new Date(year, month, ++day));
}

That'll give you a dates array beginning with the start date and continuing up to and including the end date.

From there, it's pretty much the same as now: Create and append input elements. I'm not sure I like the <br>, though. I imagine it's mostly for layout purposes, in which case I'm sure you could handle it better in CSS.

As for the date formatting, I don't know of a way to change that. Browser support for the date-type input is very poor so just be glad if you can use it at all. But since it's not widely supported, you'll probably want to use a cross-browser widget anyway, and in that case you'll likely have options for formatting the date.