Finding even or odd numbers I'm looking for critiques to see what I could have done better or different ways I could approach writing a script for finding even or odd numbers. I am new to programming with JavaScript, and programming in general. This is one of the first challenges I wrote for finding even or odd numbers.
    var numList = [];
    while(numList.length < 5){
    numList.push(window.prompt());
    }

    var evenNumbers = [];

    numList.forEach(function(element){
        if (element % 2 === 0){
            evenNumbers.push(element);
        }
    })
    document.write(evenNumbers);

You can use Array.prototype.filter.
You can read like this: return an array that only has the elements which satisfies the return function expression.
This way, you can avoid instantiating an array and only after this iteration to make the push.

var numList = [];
while(numList.length < 5){
  numList.push(window.prompt());
}

var evenNumbers = numList.filter(function (element) {
  return element % 2 === 0
});
// that's the same that
// var evenNumbers = numList.filter(element => element % 2 === 0);

document.write(evenNumbers);

Although, you can use arrow functions, that has a cleaner syntax.


ARC 067 - read ints and find the best choices I just started learning Rust, and this is my solution to a problem from Atcoder's Regular Contest #067, D - Walk and Teleport.

The problem can be summarized as follows:

There are N towns in a line, and you are about to visit all of them.
Town i is located at the point with coordinate xi.
You can travel either by walking or by teleporting. (You can combine them)

Walking costs you a * |Δx|, where a is a constant given and Δx is the distance you are about to travel.
Teleporing to any location costs you b, regardless of the distance you travel.

Given a, b and a sorted list of coordinates (xi), find the minimum cost to travel all towns.

Input is given in the form of

N a b
x1 x2 ... xN

where N is the number of cities and a, b, xi is what described above. Again, xi is sorted. All a, b, xi are integers in the range of [1, 109].

My speculation is that you can just travel from left to right, and choose the cheaper way to travel to the next town (a*(xj+1-xj) versus b).

This is my code.

use std::io;
use std::cmp::min;

fn main (){
    let input_one = read_ints();
    let a = input_one[1]; let b = input_one[2];
    let xs = read_ints();
    println!("{}", solve(&a,&b,&xs));
}

fn distances(xs:&[u64]) -> Vec<u64>{
    // Find the distances between adjacent towns
    xs.iter().zip(xs.iter().skip(1)).map(|(a,b)| b-a).collect()
}

fn solve(a:&u64, b:&u64, xs:&[u64]) -> u64 {
    let dists = distances(xs);
    dists.iter().fold(0, |acc,dist| {
        acc + min(a*dist, *b)
    })
}
fn read_ints() -> Vec<u64> {
    get_line().trim().split_whitespace().map(|n| n.parse().unwrap()).collect()
}

fn get_line() -> String {
    let mut buf = String::new();
    io::stdin().read_line(&mut buf).ok();
    buf
}

As I'm new to Rust, my main concern is the idiomaticness of the code. I've been writing Haskell and Python and there are many "it works but I'm not sure if I'm doing it right" moments. (Like, &[u64]'s were &Vec<u64> just until I was about to post this question.)

Any comments, including styles, naming, "loop is preferred", or anything are very much appreciated. Thank you.

Learn to love rustfmt.

No space between a method name and the parens, space after the parens.

-fn main (){
+fn main() {

Multiple statements belong on different lines.

-    let a = input_one[1]; let b = input_one[2];
+    let a = input_one[1];
+    let b = input_one[2];

Spaces go between comma-separated items.

-    println!("{}", solve(&a,&b,&xs));
+    println!("{}", solve(&a, &b, &xs));

Spaces go after the : in declarations.

-fn distances(xs:&[u64]) -> Vec<u64>{
+fn distances(xs: &[u64]) -> Vec<u64> {

Using Result::ok is not appropriate error handling. If there had been a failure while reading from standard input, your program would continue with an empty string. That's unlikely to be what you want.
unwrapping a Result is better than losing an error, expecting is better than that as you get a little bit of a clue as to where there was a problem, and returning a Result is even better.
If you are going to document a function, use doc comments, not programmer comments.
Zipping the slice with itself is fine, but using slice::windows is a bit shorter, and may be easier to understand when reading the code.
folding over the distances is fine, but a combination of map and sum is probably easier to understand and is as efficient.
There's no real benefit to taking values like u64 by reference. They are small and implement Copy. I'd expect that passing by value would be the same speed if not faster.
a, b, 'xs' are bad variable names. Make them something useful.

use std::io;
use std::cmp::min;

fn main() {
    let input_one = read_ints();
    let walk_cost = input_one[1];
    let teleport_cost = input_one[2];
    let locations = read_ints();
    println!("{}", solve(walk_cost, teleport_cost, &locations));
}

/// Find the distances between adjacent towns
fn distances(locations: &[u64]) -> Vec<u64> {
    locations.windows(2).map(|w| w[1] - w[0]).collect()
}

fn solve(walk_cost: u64, teleport_cost: u64, locations: &[u64]) -> u64 {
    let dists = distances(locations);
    dists.iter().map(|dist| min(walk_cost * dist, teleport_cost)).sum()
}

fn read_ints() -> Vec<u64> {
    get_line()
        .trim()
        .split_whitespace()
        .map(|n| n.parse().expect("Unable to parse"))
        .collect()
}

fn get_line() -> String {
    let mut buf = String::new();
    io::stdin().read_line(&mut buf).expect("Unable to read");
    buf
}

Overall, your code was easy to read and understand.

However, it feels like there's more allocation than there needs to be. Specifically, taking an iterator and making a Vec to make it into an iterator to make a Vec to just iterate through it again seems unfortunate.

One silly alternative would be something like this:

extern crate itertools;

use std::io::{self, BufRead};
use std::cmp::min;

use itertools::Itertools;

fn main() {
    let input = io::stdin();
    let input = input.lock();
    let mut lines = input.lines();

    let line = lines.next().expect("no line").expect("Unable to read line");
    let mut config_line = line.split_whitespace().map(|n| n.parse::<u64>().expect("not an integer")).skip(1);
    let walk_cost = config_line.next().expect("no walk cost provided");
    let teleport_cost = config_line.next().expect("no teleport cost provided");

    let line = lines.next().expect("no line").expect("Unable to read line");
    let locations = line.split_whitespace().map(|n| n.parse::<u64>().expect("not an integer"));
    let distances = locations.tuple_windows().map(|(a, b)| b - a);
    let costs = distances.map(|dist| min(walk_cost * dist, teleport_cost));
    let total_cost: u64 = costs.sum();

    println!("{}", total_cost);
}

This is not what I'd consider idiomatic Rust code, however. I always find iterators that return Results to be a bit awkward to deal with while still passing up errors to the calling function.


Positive integer to Roman Numeral conversion function For the past two hours I was searching for a built-in function for converting an integer to an equivalent Roman numeral, but it failed to achieve  the target. Hence I wrote my own routine for performing this conversion. I think it needs some more clarifications.

Public Class Form1
    Dim ind() As Integer = {1, 2, 3, 4, 5, 10, 50, 100, 500, 1000}
    Dim rom() As String = {"I", "II", "III", "IV", "V", "X", "L", "C", "D", "M"}
    Dim limit As Integer = 9
    Dim output As String = ""
    Private Sub Button1_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button1.Click
        Dim num As Integer
        output = ""
        num = CInt(txt1.Text)
        While num > 0
            num = find(num)
        End While
        txt2.Text = output
    End Sub
    Public Function find(ByVal Num As Integer) As Integer
        Dim i As Integer = 0
        While ind(i) <= Num
            i += 1
        End While
        If i <> 0 Then
            limit = i - 1
        Else
            limit = 0
        End If
        output = output & rom(limit)
        Num = Num - ind(limit)
        Return Num
    End Function
End Class

Note:  This function can only be used for range of 1-999

This code is bound far too tight to your form. The first thing we need to do is separate the logic for converting an integer to a roman numeral from the event procedures of the form. This will allow us to reuse this code anywhere.

Module IntegerExtensions

    Dim ind() As Integer = {1, 2, 3, 4, 5, 10, 50, 100, 500, 1000}
    Dim rom() As String = {"I", "II", "III", "IV", "V", "X", "L", "C", "D", "M"}
    Dim limit As Integer = 9
    Dim output As String = ""

    Private Function ConvertToRomanNumeral(ByVal input As Integer) As String
        output = ""
        While input > 0
            input = find(input)
        End While
        Return output
    End Function

    Private Function Find(ByVal Num As Integer) As Integer
        Dim i As Integer = 0
        While ind(i) <= Num
            i += 1
        End While
        If i <> 0 Then
            limit = i - 1
        Else
            limit = 0
        End If
        output = output & rom(limit)
        Num = Num - ind(limit)
        Return Num
    End Function

End Module

Public Class Form1
    Private Sub Button1_Click(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles Button1.Click

        txt2.Text = IntegerExtensions.ConvertToRomanNumeral(999)

    End Sub
End Class

Next we need to make sure that the argument passed to ConvertToRomanNumeral is within the accepted range of 1-999. Receiving a Index out of bounds message is not nearly as useful as knowing that ConvertToRomanNumeral doesn't accept the value you passed into it.

Private Function ConvertToRomanNumeral(ByVal input As Integer) As String

    If (input < 1 Or input > 999) Then
        Throw New ArgumentOutOfRangeException("input","Input must be in the range 1-999")
    End If

    output = ""
    While input > 0
        input = find(input)
    End While
    Return output
End Function

ind and rom are not very good variable names. I get that rom stands for Roman Numerals, but I have no idea what ind means. Indices maybe? Also, I like plural names for arrays. Let's replace those. ind >> indices and rom >> romanNumerals.
There are way too many class level variables. Limit should be strictly declared inside of Find. I also don't find it a good idea to set the value of output from the Find function. I would consider this to be a side effect, and side effects are confusing and hard to debug. The only way for me to properly scope output was to merge Find into ConvertToRomanNumeral.

Module IntegerExtensions

    Private Function ConvertToRomanNumeral(ByVal input As Integer) As String

        If (input < 1 Or input > 999) Then
            Throw New ArgumentOutOfRangeException("input","Input must be in the range 1-999")
        End If

        Dim indices() As Integer = {1, 2, 3, 4, 5, 10, 50, 100, 500, 1000}
        Dim romanNumerals() As String = {"I", "II", "III", "IV", "V", "X", "L", "C", "D", "M"}
        Dim limit As Integer = 9
        Dim output As String = ""

        While input > 0
            Dim number as Integer = input
            Dim i As Integer = 0
            While indices(i) <= number
                i += 1
            End While
            If i <> 0 Then
                limit = i - 1
            Else
                limit = 0
            End If
            output = output & romanNumerals(limit)
            number = number - indices(limit)
            ' this was previously our return statement
            input = number
        End While
        Return output
    End Function

End Module

You may have noticed earlier that I named the module IntegerExtensions. That's because I've been planning on making this exactly that the entire time. Extension methods will allow us to call the function directly on any integer, instead of passing an integer into it. It looks a little cleaner when in use. I changed the function name to ToRomanNumberal and adjusted the ArgumentOutOfRangeException message accordingly.

Module IntegerExtensions

    <Extension()>
    Public Function ToRomanNumeral(ByVal input As Integer) As String

        If (input < 1 Or input > 999) Then
            Throw New ArgumentOutOfRangeException("input","Cannot convert integers outside of the range 1-999")
        End If

        Dim indices() As Integer = {1, 2, 3, 4, 5, 10, 50, 100, 500, 1000}
        Dim romanNumerals() As String = {"I", "II", "III", "IV", "V", "X", "L", "C", "D", "M"}
        Dim limit As Integer = 9
        Dim output As String = ""

        While input > 0
            Dim number as Integer = input
            Dim i As Integer = 0
            While indices(i) <= number
                i += 1
            End While
            If i <> 0 Then
                limit = i - 1
            Else
                limit = 0
            End If
            output = output & romanNumerals(limit)
            number = number - indices(limit)
            ' this was previously our return statement
            input = number
        End While
        Return output
    End Function

End Module

Excercises for you:

How could this be done using a dictionary instead.
How can this be changed so that numbers outside of 1-999 can be converted?


Best practice to upload files on server using Rest Service I am using building REST services for file upload on the server. Here is some code of that

[OprationContract]
[WebInvoke(Method = "POST", UriTemplate = "UploadFile/{fileName}")]
public string UploadFile(string fileName, Stream fileStream)
{
  string filePath = serverDirectory + "\\" + fileName;
  using (var output = File.Open(filePath, FileMode.Create))
  {
     fileStream.CopyTo(output);
  }
  return "success";
}

However this method works fine when file size is small. but when the file size is large say 30-40 Mb then the performance become low.

So I want to know what are the best practices to upload the files on the server with speed.

The lines of code inside your UploadFile method look optimal to me.

Perhaps the problem is that your Stream contents are being fully buffered before they're being delivered.

I've no experience with this myself, but
How to: Enable Streaming suggests you may need to change your configured TransferMode, and change your API so that it has a method whose only parameter is Stream (perhaps pass the filename in one method and the Stream in the next method).


Simple bank-account application using composition I'm trying to use composition to design an application that works with different types of bank-accounts. Instead of deriving from a Account Class I want to have separate account classes that are created with appropriate behaviours with a factory. For the example below can I please get some comments on:

Overall design decision
Testability
As i'm adding more behaviours/features does it look sustainable to keep passing objects in constructor?

#include <iostream>
#include <memory>
#include <string>

using namespace std;

class Balance {
public:
  Balance() {}
  Balance(double amount, std::string currency)
      : d_amount(amount), d_currency(currency) {}
  double amount() const { return d_amount; }
  std::string currency() const { return d_currency; }

private:
  double d_amount;
  std::string d_currency;
};

std::ostream &operator<<(std::ostream &os, const Balance &balance) {
  os << "Balance->" << balance.amount() << ":Currency->" << balance.currency();
  return os;
}

class InterestRate {
private:
  double d_interest_rate;

public:
  virtual double interest_rate() const = 0;
};

class CurrentAccountInterestRate : public InterestRate {
public:
  double interest_rate() const override { return 0.5; }
};

class SavingsAccountInterestRate : public InterestRate {
public:
  double interest_rate() const override { return 2.0; }
};

std::ostream &operator<<(std::ostream &os, const InterestRate &interest_rate) {
  os << "Interest-Rate->" << interest_rate.interest_rate() << std::endl;
  return os;
}

class CurrentAccount {
public:
  CurrentAccount(const Balance &balance,
                 std::unique_ptr<InterestRate> interest_rate)
      : d_balance(balance), d_interest_rate(std::move(interest_rate)) {}

  void print_balance() { std::cout << d_balance << ":" << *d_interest_rate; }

private:
  Balance d_balance;
  std::unique_ptr<InterestRate> d_interest_rate;
};

class SavingsAccount {
public:
  SavingsAccount(const Balance &balance,
                 std::unique_ptr<InterestRate> interest_rate)
      : d_balance(balance), d_interest_rate(std::move(interest_rate)) {}

  void print_balance() { std::cout << d_balance << ":" << *d_interest_rate; }

private:
  Balance d_balance;
  std::unique_ptr<InterestRate> d_interest_rate;
};

class AccountFactory {
public:
  static CurrentAccount create_current_account(double amount,
                                               const std::string &currency) {
    std::unique_ptr<InterestRate> currentAccountInterestRate =
        std::make_unique<CurrentAccountInterestRate>();
    CurrentAccount ca{Balance{amount, currency},
                      std::move(currentAccountInterestRate)};
    return ca;
  }

  static SavingsAccount create_savings_account(double amount,
                                               const std::string &currency) {
    std::unique_ptr<InterestRate> savingsAccountInterestRate =
        std::make_unique<SavingsAccountInterestRate>();
    SavingsAccount sa{Balance{amount, currency},
                      std::move(savingsAccountInterestRate)};
    return sa;
  }
};

int main() {
  CurrentAccount ca = AccountFactory::create_current_account(20.5, "USD");
  ca.print_balance();
  SavingsAccount sa = AccountFactory::create_savings_account(40.5, "GBP");
  sa.print_balance();
  return 0;
}

Don’t write using namespace std;.

You can, however, in a CPP file (not H file) or inside a function put individual using std::string; etc.  (See SF.7.)

Balance(double amount, std::string currency)
    : d_amount(amount), d_currency(currency) {}

std::string d_currency;

You are taking the currency parameter by value, and then copying it to the member, then you don’t need the first copy anymore.  I think you were trying to use the “sink” parameter idiom?

 : d_amount{amount}, d_currency{std::move(currency)} {}

You forgot the move!  Also, you were using old syntax for the initializers: use uniform initialization style now (since C++11).  (Later) I see you did move in another class, so that was your intent here I think.

I see you are using initializers, which is good.  Likewise, using exactly one of virtual/override/final on a virtual function, and using unique_ptr.

void print_balance() { std::cout << d_balance << ":" << *d_interest_rate; }

That’s exactly the same for CurrentAccount and SavingsAccount.  I thought at first you were overriding a virtual function with no need, but looking closer I see that these classes are not related, and yet they are identical!  I don’t want to do a diff to find out if there is a few chars different somewhere, so that is a concern for understandibility.

Likewise, AccountFactory is duplicating the same code for both creation functions.  What is the deal here?  If you need “strong types”, just add a template parameter.  Don’t duplicate the entire code for the class manually!

Why is InterestRate a polymorphic interface?  Is what’s shown here just as example, or is there really nothing more to it than different values of a field?  Because in that case, you should just use a value.  This is especially odd since you always use the same Interest Rate class with a corresponding Account class — there is no need to make it polymorphic at all, even if they were really different.


all the ways to fit some pegs into given holes Suppose you have a list of the diameters of some circular holes, and a list of the diameters of some cylindrical pegs. Now the problem is finding all the ways you can fit the pegs in the holes, where the order in which you put them in doesn't matter, and it's OK to put a peg in a hole that's too large.

I've written the code below to solve the problem, and it works as intended. The innermost tuples of the answer matches a peg to a hole, i.e. (4, 7) means put d4 peg in d7 hole, and these tuples make up a set that represents one way of putting all the pegs in.

I'm not completely happy with the solution. It's not... snappy enough, and it feels overly complicated. Is there a smarter way to go about it?

def fit(pegs, holes):
    for h in holes:
        for p in pegs:
            if p <= h:
                pegs_left = [x for x in pegs if x != p]
                if pegs_left:
                    free_holes = [x for x in holes if x != h]
                    for rest in fit(pegs_left, free_holes):
                        yield frozenset(((p, h),)) | rest
                else:
                    yield frozenset(((p, h),))

def fitted(pegs, holes):
    return sorted(sorted(x) for x in set(fit(pegs, holes)))

pegs = [2, 4, 7]
holes = [1, 3, 4, 6, 9]

print fitted(pegs, holes)
# [[(2, 3), (4, 4), (7, 9)],
#  [(2, 3), (4, 6), (7, 9)],
#  [(2, 4), (4, 6), (7, 9)],
#  [(2, 6), (4, 4), (7, 9)]]

I recommend against using single letter variable names, in particular your use of p and h.
I think the code is clearer if you use peg and hole.
Your code is going to break if you have multiple holes or pegs of the same size.
You'd be better off using tuples instead of frozensets. You don't appear to be using the set features.

In order to have better code/algorithm you need to look at the problem differently. Right now you are trying to find peg/hole combinations. Instead, you should try to figure out where each peg fits.

You have the pegs: 2, 4, 7

Your goal is to select three of the holes: 1, 3, 4, 6, 9 which fit the pegs.

itertools.permutations(holes, 3)

will produce an iterator over all possible choices of 3 holes from the list. Something like:

1 3 4
1 3 6
1 3 9
1 4 3
...

We interpret each value as an assignment of holes to pegs. So 1,3,4 means (2, 1), (4, 3), (7,4). Of course not all such assignments are valid. So we filter out the invalid ones (i.e. where the pegs don't fit).

As an added bonus, itertools.permutations generates its values in sorted order. Because we only filter the results, the resulting function will also produce its values in sorted order. This removes the neccesity of doing the sorting yourself.

Here is my implementation:

import itertools

def check_fits(pegs, holes):
    for peg, hole in zip(pegs, holes):
        if peg > hole:
            return False
    return True

def fit(pegs, holes):
    assert len(pegs) <= len(holes)
    for selected_holes in itertools.permutations(holes, len(pegs)):
        if check_fits(pegs, selected_holes):
            yield zip(pegs, selected_holes)

pegs = [2, 4, 7]
holes = [1, 3, 4, 6, 9]

for item in fit(pegs, holes):
    print item

itertools.permutations will generate many invalid assignment of pegs to holes. For example, in the example above, it will generate a significant number of assignment which attempt to fit a peg of size 2 into a hole of size 1. A simple recursive function could avoid spending time on so many bad assignments. However, itertools.permutation is written in C, and so probably still has a speed advantage over such a recursive algorithm.


How to check list of environment variable in python? I have requirement to check list of environment variable in Linux. Based on environment variable, exit code will be different. how to optimize the below code?

def check_mandatory_envs():
    if "ENVCONTEST_1" not in os.environ:
        exit(125)
    if "ENVCONTEST_2" not in os.environ:
        exit(126)
    if "ENVVIRTEST_3" not in os.environ
        exit(127)
    if "ENVVIRTEST_4" not in os.environ
        exit(128)
    if "ENVPATHTEST_5" not in os.environ
        exit(129)
    if "ENVPATHTEST_6" not in os.environ
        exit(130)

You can group the messages and corresponding exit codes into a dict, and then iterate over that one:

def check_mandatory_envs():
    exit_codes = {
        "ENVCONTEST_1" :125,
        "ENVCONTEST_2" :126,
        "ENVVIRTEST_3" :127,
        "ENVVIRTEST_4" :128,
        "ENVPATHTEST_5":129,
        "ENVPATHTEST_6":130,
    }
    for variable, code in exit_codes.items():
        if variable not in os.environ:
            exit(code)

If the order of the iteration is important and you are on python before 3.6, you canuse a collections.OrderedDict or a list of tuples, without the .items() call


"Parking" challenge solution This is the problem statement. I have submitted successfully a \$O(n^2)\$ solution and I would like to know if there is a way to improve the running time to, maybe, \$O(n \log n)\$.

  Having dropped out of school because of chemistry, Luka got a job
  driving trucks. One evening he parked his three trucks in a rest area
  which charges for parking in an unusual way – they give a discount on
  quantity.

  When only one truck is parked, the driver pays AA kuna per minute.
  When two trucks are parked, the drivers each pay BB kuna per minute.
  When three trucks are parked, the drivers each pay CC kuna per minute.

  Given the numbers AA, BB and CC, as well as the intervals in which
  Luka’s three trucks are parked, determines how much Luka needs to pay
  the owner of the rest area.

  Input

  The first line contains three integers AA, BB and CC
  (\$1 \le C \le B \le A \le 100\$), the prices of parking as defined above. Each of the following three lines contains two integers each. These are the
  arrival and departure times (in minutes) of one of Luka’s trucks. The
  arrival time will always be earlier than the departure time. All time
  indexes will be between 1 and 100.

  Output

  Output the overall cost of Luka’s parking his three trucks.

Current solution (please note that I use custom I/O which I haven't included):

public class Parking {

    public static void main(String[] args) {
        InputReader in = new InputReader(System.in);
        OutputWriter out = new OutputWriter(System.out);

        int A = in.readInt();
        int B = in.readInt();
        int C = in.readInt();

        int minArrival = Integer.MAX_VALUE;
        int maxDeparture = Integer.MIN_VALUE;

        Times[] times = new Times[3];

        for (int i = 0; i < 3; ++i) {
            int arrival = in.readInt();
            int departure = in.readInt();

            minArrival = Math.min(minArrival, arrival);
            maxDeparture = Math.max(maxDeparture, departure);
            times[i] = new Times(arrival, departure);
        }

        int result = 0;
        for (int i = minArrival; i <= maxDeparture; ++i) {
            int count = 0;
            for (Times t : times) {
                if (t.arrival <= i && t.departure > i) {
                    count++;
                }
            }

            switch (count) {
                case 1:
                    result += A;
                    break;
                case 2:
                    result += B * 2;
                    break;
                case 3:
                    result += C * 3;
                    break;
                default:
                    break;
            }

        }

        out.print(result);
        out.close();
    }

    private static class Times {
        final int arrival;
        final int departure;

        Times(int arrival, int departure) {
            this.arrival = arrival;
            this.departure = departure;
        }
    }
}

If you turn the arrival and departure times into objects that give +1 or -1 to a truckCounter variable, then sort this array (\$O(n log(n))\$), then you can iterate over the list of events.

As a result, you'll have 6 iterations after sorting, and the algorithm will bound on not the length of the durations but the amount of trucks.

So read inputs, sort the array, and then...

//assuming there is an array "sortedTruckEvents" of type TruckEvent containing an object which has
//getModifier as -1 for truck leaving and +1 for truck arriving
//and getTime for retrieving when this happens
//and assuming there is an array "prices" containing 0 at index 0, priceA * 1 at index 1, priceB * 2 at index 2, priceC * 3 at index 3
int result = 0;
int truckCounter = 0;
int currentTime = 0;
for(int i = 0; i < sortedTruckEvents.length; i++) {
    TruckEvent event = sortedTruckEvents[i];
    result += prices[truckCounter] * (event.getTime() - currentTime);
    truckCounter += event.getModifier();
    currentTime = event.getTime();
}
//now result contains the total price

It's as easy as that.

The trick here is to realize that the points of interest in this case is where things change. And the problem variables don't change per timestep; they change per truck arrival and departure.

The problem has even made it easy for you by guaranteeing all the trucks will arrive before they leave and that the trucks will leave at all.


Sorting an array of positive integers including 0 much faster than Radix Sort I was working on an Limit Order Book structure in JS and came up with this algorithm. I am pretty sure this must have already been implemented but couldn't even find a clue over the web.
The thing is, it's very fast especially when you have an array of many duplicate items. However the real beauty is, after inserting k new items into an array of already sorted n items the pseudo sort (explained below) takes only O(k) and sort takes only O(n+k). To achieve this i keep a pseudo sorted array of m items in a sparse array where m is the number of unique items.
Take for instance we need to sort [42,1,31,17,1453,5,17,0,5] where n is 9 and then we just use values as keys and construct a sparse array (Pseudo Sorted) like;
Value: 1 1 2  2  1  1    1
Index: 0 1 5 17 31 42 1453

Where Value keeps the repeat count. I think now you start to see where I am getting at. JS have a fantastic ability. In JS accessing the sparse array keys can be very fast by jumping over the non existent ones. To achieve this you either use a for in loop of Object.keys().
So you can keep your sparse (pseudo sorted) array to insert new items and they will always be kept in pseudo sorted state having all insertions and deletions done in O(1). Whenever you need a real sorted array just construct it in O(n). Now this is very important in a Limit Order Book implementation because say you have to respond to your clients with the top 100 bids and bottom 100 asks in every 500ms over a web socket connection, now you no longer need to sort the whole order book list again even if it gets updated with many new bids and asks continuously.
So here is sparseSort code which could possibly be trimmed up by employing for loops instead of .reduces etc. Still beats Radix and Array.prototype.sort().
function sparseSort(arr){
    var tmp = arr.reduce((t,r) => t[r] ? (t[r]++,t) : (t[r]=1,t),[]);
    return Object.keys(tmp)
                 .reduce((a,k) => {
                           for (var i = 0; i < tmp[k]; i++) a.push(+k);
                           return a;
                         },[]);
}

Here you can see a bench against Array.prototype.sort() and radixSort. I just would like to know if this is reasonable and what might be the handicaps involved.

I think you re-invented counting sort, but with some small differences. Also you probably need to sort your sparse array keys before iterating over them so that's O(k log k) at least unless your possible range of values is small (let's call it N) so you can just try all N values and it will be O( N ).
Performance wise, it's hard to beat if you have a small range of value N.


findViewById - in separate class I'm new to Android and I wanted to create a class that would load an ad inside the current layout.  All the layouts have a RelativeLayout id=adLayout

From my main, I have:

AdLoader al = new AdLoader();
al.GrabAd(this,"123");

Then on my AdLoader.java class I have:

public class AdLoader {

    private DfpAdView adView;

    public void GrabAdFor(Activity act, String adId) {
        adView = new DfpAdView(act, AdSize.BANNER, adId);
        RelativeLayout rl = (RelativeLayout) act.findViewById(R.id.adLayout);
        rl.addView(adView);
        adView.loadAd(new AdRequest());
    }
}

Is it incorrect to pass the Activity?  Am I even passing the activity or the View/Context?

You could also make a baseactivity for all the other activity that are going to use the adView

    public class BaseActivity extends Activity {

    private DfpAdView adView;
    RelativeLayout rl;
    //some other stuff

    @Override
    public void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.baseactivity_layout)
        rl = (RelativeLayout) findViewById(R.id.adLayout);
        adView = new DfpAdView(this, AdSize.BANNER, adId);
        rl.addView(adView);
        adView.loadAd(new AdRequest());
    }
}

And and make your MainActivity (or all the other activities that are using the adview)

public class MainActivity extends BaseActivity {
    @Override
    public void onCreate(Bundle savedInstanceState){

    }
}


Find the shortest whole repetitive substring I'm working on a problem to find wholly repeated shortest substring of a given string, and if no match, return length of the string.

My major idea is using a Trie tree to build substrings from length 1 to half length of the whole string, then traverse the Trie tree to find if there is a wholly repetitive match or not (since when I build Trie tree, I record the depth of leaf node and also how many times the leaf node has been reached).

I think my algorithm is still \$O(n^2)\$ and I'm looking for any code review comments for my current code and better ideas to improve time complexity.

Input and output example

catcatcat => 3
aaaaaa=>1
aaaaaba = > 7

My code

from __future__ import print_function
from collections import defaultdict
import sys
class TrieNode:
    def __init__(self):
        self.isEnd = False
        self.children = defaultdict(TrieNode)
        self.count = 0 # reached how many times
        self.depth = 0 # reached by how long sub-string
    def addNode(self, word):
        if not word:
            return
        node = self
        depth = 0
        for ch in word:
            node = node.children[ch]
        node.isEnd = True
        node.count += 1
        node.depth = len(word)
    # return minimal length of whole repetitive match
    # in a recursive way
    def traverseNode(self, totalLen):
        if self.isEnd == True:
            if (self.count * self.depth == totalLen):
                return self.depth
            else:
                return totalLen
        result = sys.maxint # set to a very big value
        for node in self.children.values():
            result = min(node.traverseNode(totalLen), result)

        return result
if __name__ == "__main__":
    results = []
    word = 'catcatcatcat' # output 3
    #word = 'aaaaaa' # output 1
    #word = 'aaaaaab' # output 7
    for step in range(1, len(word)//2 + 1):
        # whole repetive string must be start from zero
        i = 0
        root = TrieNode()
        while i+step <= len(word):
            root.addNode(word[i:i+step])
            i += step
        results.append(root.traverseNode(len(word)))
        print (step, results[-1])

    print (min(results))

Why not use a regular expression?

>>> import re
>>> len(re.match(r'(.+?)\1*$', 'catcatcat').group(1))
3

This has runtime \$Θ(n^2)\$, same as the code in the post, but it's a lot shorter.


Filter out elements from std::vector I have ended up with the following code in order to filter out some bad elements from a std::vector:

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

typedef struct mystruct {
    int id;
    std::string name;
};

int main()
{
    std::vector<mystruct> all_items = {{151, "test1"}, {154, "test4"}, {152, "test2"}, {151, "test1"}, {151, "test1"}, {153, "test3"}};
    std::vector<int> bad_ids = {151, 152};
    std::vector<mystruct> filter_items;

    for (const auto& item : all_items) {
        if ( std::find(bad_ids.begin(), bad_ids.end(), item.id) != bad_ids.end() ) {
            std::cout << "id: " << item.id << " is bad" << std::endl;
        } else {
            std::cout << "id: " << item.id << " is good item" << std::endl;
            filter_items.emplace_back(item);
        }
    }

    for (auto f : filter_items) {
        std::cout << "Good item: " << f.id << std::endl;
    }
}

Is there any more efficient way? Can the std::remove_copy_if or Boost be used here and how?

You're essentially copying copy_if with some additional debug messages. One can rewrite your code with

std::copy_if(all_items.begin(), all_items.end(), std::back_inserter(filter_items),
             [&bad_ids](const mystruct& item)
{
     return std::find(bad_ids.begin(), bad_ids.end(), item.id) == bad_ids.end();
});

But that's not necessarily more efficient, because copy_if and remove_copy_if are usually implemented as

for(; first != last; ++first) {
    if(predicate(*first)){     // copy if; remove_copy_if would negate
        *result = *first;
        result++;
    }
}

A more efficient way would be a faster predicate, e.g. instead of find(bad_ids…) you could use

for(const auto & item : all_items) {
    if(item.id < 151 || 152 < item.id) {
        filtered_items.push_back(item);
    }
}

But that's not as flexible anymore. That being said, we're talking about a small code snippet. As always, you should measure the time your code needs. And in this case, the output by cout will most likely take the most time.

For now, use the variant that you understand best. Your code is fine, there's not much one can improve at that point without resorting to personal preference because it's too short.

By the way, I would call filter_items filtered_items instead, since it doesn't actually filter items. It is a std::vector of items that have been filtered.


Count number of ways to paint a fence with N posts using K colors A friend sent me this question, and I'd love somebody to review this and give his opinion.

  Write an algorithm that counts the number of ways you can paint a
  fence with N posts using K colors such that no more than 2 adjacent
  fence posts are painted with the same color"

Is this algorithm accurate? Could it be implemented in a better way?  Is there anything really un-pythonic that I did there?  I hope I haven't made any embarrassing mistakes.

def find_combinations(n, colours):

combos_list = []

    for i in range(1, n+1):

        #Check if the combos_list is empty (happens when post number == 1)
        if not combos_list:
            #Add all variations for when there's only one post
            for c in colours:
                combos_list.append([c])
        else:
            new_combo_list = []
            for combo in combos_list:
                for c in colours:
                    #Check if last 2 posts are from the same colour, meaning we can't add another one
                    if not combo[len(combo) - 2:] == [c, c]:
                        combo_to_add = combo[:]
                        combo_to_add.append(c)
                        new_combo_list.append(combo_to_add)
            combos_list = new_combo_list[:]

    return combos_list, len(combos_list)

colours = ["black", "white"]
n = 4

print find_combinations(n, colours)

Example of input:

  colours = ["a", "b"]
  n = 3

Output:

  a, a, b
  a, b, a
  a, b, b
  b, a, a
  b, a, b
  b, b, a

Number of combos - 6

1. Code review

The question says, "Write an algorithm that counts the number of ways" but the program does not only do this: it also builds a list of the combinations. This means that the program is limited to values of \$N\$ and \$K\$ that are small enough that the list of combinations fits in the storage space of the computer. Whereas a program that did not have to produce this list could work for much larger values of \$N\$ and \$K\$, as we'll see in §3 below.
It's good practice to write a docstring for each function, describing what the function does, the arguments that it takes, and what it returns.
If a function returns a list, it does not also need to return the length of the list. If the caller needs that information, it can call len just as easily. So the function could just return combos_list.
In the line:

combos_list = new_combo_list[:]

it's unnecessary (and wasteful) to take a copy of new_combo_list. The new list can just be assigned to the old name, like this:

combos_list = new_combo_list

When a loop index (here i) is not used, it's conventional to name it _.
The code that's guarded by the line:

if not combos_list:

only runs once (on the first iteration of the loop), so it would be simpler to move it out of the loop, have one fewer iteration of the loop, and so avoid the if ... else .... Like this:

combos_list = []
for c in colours:
    combos_list.append([c])
for _ in range(1, n):
    new_combo_list = []
    # etc.

Instead of:

combos_list = []
for c in colours:
    combos_list.append([c])

use a list comprehension and write:

combos_list = [[c] for c in colours]

These lines:

combo_to_add = combo[:]
combo_to_add.append(c)
new_combo_list.append(combo_to_add)

could be simplified to:

new_combo_list.append(combo + [c])

This condition:

if not combo[len(combo) - 2:] == [c, c]:

could be simplified to:

if combo[-2:] != [c, c]:

(since Python allows you index a list backwards from the end using negative numbers).

2. Revised code

def find_combinations(n, colours):
    """Return a list of combinations of n items from the sequence colours,
    with the constraint that no colour appears three times in a row.

    >>> [''.join(combo) for combo in find_combinations(3, 'ab')]
    ['aab', 'aba', 'abb', 'baa', 'bab', 'bba']

    """
    combos = [[c] for c in colours]

    for _ in range(1, n):
        new_combos = []
        for combo in combos:
            for c in colours:
                # Check that last two colours aren't both c.
                if combo[-2:] != [c, c]:
                    new_combos.append(combo + [c])
        combos = new_combos

    return combos

Note the example code in the docstring. This can be run as a test case using the doctest module.

3. Let me count the ways

The question asked for "an algorithm that counts the number of ways", so here's how I'd go about that.

I'll start with some notation. Assume that \$k\$ is fixed. Then:

\$T(n)\$ is the total number of ways to paint \$n\$ fenceposts with \$k\$ colours, subject to the constraint that no three adjacent posts are the same colour.
\$D(n)\$ is the number of ways to paint \$n\$ fenceposts with \$k\$ colours such that the last two fenceposts are different colours (or \$n < 2\$).
\$S(n)\$ is the number of ways to paint \$n\$ fenceposts with \$k\$ colours such that the last two fenceposts are the same colour.

And then the following recurrence relations are clear:

\$D(n) = (k−1)(D(n−1) + S(n−1))\$ for \$n ≥ 2\$
\$S(n) = D(n−1)\$ for \$n ≥ 2\$
\$T(n) = D(n) + S(n)\$

We can use (2) to eliminate \$S\$ (being careful about the bound on \$n\$ as we do so), getting the recurrences:

\$D(n) = (k−1)(D(n−1) + D(n−2))\$ for \$n ≥ 3\$
\$T(n) = D(n) + D(n−1)\$ for \$n ≥ 2\$

And then substitute (5) into (4), getting:

\$D(n) = (k−1)(T(n−1))\$ for \$n ≥ 3\$

and so:

\$T(n) = (k−1)(T(n−1) + T(n−2))\$ for \$n ≥ 3\$

Since the recurrence depends only on the last two values of the sequence, it's easy to program:

def triple_free_combinations(n, k):
    """Return the number of ways to choose n items (with k choices for
    each item), subject to the constraint that no colour appears three
    times in a row.

    """
    if n == 0:
        return 1
    a, b = k, k * k
    for _ in range(n - 1):
        a, b = b, (k - 1) * (a + b)
    return a

(You should recognize this as a very simple example of the tabular method, aka "dynamic programming".)

The effort that went into find_combinations is not wasted, because now we can use that function as source of test cases:

>>> from itertools import product
>>> all(triple_free_combinations(n, k)
...     == len(find_combinations(n, list(range(k))))
...     for n, k in product(range(1, 8), repeat=2))
True

Another useful source of test cases is the On-Line Encyclopedia of Integer Sequences. For example, when \$k=2\$ we have sequence A128588:

>>> [triple_free_combinations(i, 2) for i in range(16)]
[1, 2, 4, 6, 10, 16, 26, 42, 68, 110, 178, 288, 466, 754, 1220, 1974]

and similarly when \$k=3,4,5,\ldots\$ we have A121907, A123620, A123871, ....

Finally, note that triple_free_combinations is capable of computing values that are far beyond the reach of the original function:

>>> triple_free_combinations(40, 40)
472189463994618801762969606024644975976973566598588071193029590400


Bash on Synology - Delete recycle bin entries over X days old Synology NASs have a Task Scheduler that allows you to schedule deletion of files over X days old from recycle bins. This feature isn't working for me–running the task results in no files being deleted. The task works if I schedule a full deletion of the recycle bin, but I'd prefer to only delete a file if it sits in the bin for more than X days. As such, I've created this user script to do so. I want to make sure that it deletes exactly as I want.

Only entries within the recycle bin of the specific shared folder /volume1/share1/#recycle/
Only files that are over 60 days old
Not the recycle bin folder itself
Delete empty folders

Script:

deletepath="/volume1/share1/#recycle/"
logpath="/volume1/share2/SynoScripts/logs/deleteOlderThanXDays.txt"
errorpath="/volume1/share2/SynoScripts/errors/deleteOlderThanXDays.txt"
now=`date "+%Y-%m-%d %H:%M:%S"`

echo "" >> $logpath
echo $now >> $logpath

echo "" >> $errorpath
echo $now >> $errorpath

# Delete files
/usr/bin/find $deletepath -type f -mtime +60 -exec rm -v {} \; >>$logpath 2>>$errorpath

# Delete empty folders
/usr/bin/find $deletepath -mindepth 1 -type d -empty -exec rmdir {} \; >>$logpath 2>>$errorpath

Does this script appear to satisfy my requirements?

If this is intended to be run as a command, I recommend you add a suitable shebang line.  Although the question is tagged bash, there's nothing here that isn't portable POSIX shell, so I recommend

#!/bin/sh

Is it intentional that the paths all share a common initial prefix /volume1 and that the log and error paths share a longer common prefix?  If so, encode that for easier re-use:

volume=/volume1
scriptdir=$volume/share2/SynoScripts
logpath=$scriptdir/logs/deleteOlderThanXDays.txt
errorpath=$scriptdir/errors/deleteOlderThanXDays.txt

Personally, I'd call those last two logfile and errorfile for clarity.

There's no need to quote the values in these assignments, but the values should be quoted when used later, so that we don't break the script when they change to include spaces or other significant characters.

Instead of multiple echo commands, consider using a single date with tee:

date '+%n%Y-%m-%d %T' | tee -a "$logpath" >>"$errorpath"

After that, we can simply redirect all output and errors:

exec >>"$logpath" 2>>"$errorpath"

When using find, prefer to group many arguments into a few commands, using + instead of \;:

find "$deletepath" \! -type d -mtime +60 -exec rm -v '{}' +
find "$deletepath" -mindepth 1 -type d -empty -exec rmdir -v '{}' +

I assume you meant to use -v consistently for both commands here.

Modified version

#!/bin/sh

volume=/volume1
scriptdir="$volume/share2/SynoScripts"

deletepath="$volume/share1/#recycle"
logpath="$scriptdir/logs/deleteOlderThanXDays.txt"
errorpath="$scriptdir/errors/deleteOlderThanXDays.txt"

# redirect all output and errors
exec >>"$logpath" 2>>"$errorpath"

# log the start time to both logs
date '+%n%Y-%m-%d %T' | tee -a "$errorpath"

# delete old non-directory files (including devices, sockets, etc)
find "$deletepath" \! -type d -mtime +60 -exec rm -v '{}' +

# delete empty directories, regardless of age
find "$deletepath" -mindepth 1 -type d -empty -exec rmdir -v '{}' +


FizzBuzz in F# with functional programming Learning F#, created a somewhat elaborate fizzbuzz-builder implementation to see if I'm starting to "get" F# and thinking functionally:

type FBCase = FBCase of (int * string)

let makeFBCase num = FBCase (num, "")

let unpackFBCase case =
    match case with
    | FBCase (num, str) when str = "" -> num |> string
    | FBCase (_, str) -> str

let fbBuilder (num) (str:string) input =
    match input with
    | FBCase (x, y) when x % num = 0 -> FBCase (x, y+str)
    | _ -> input

let createFBFunc input = fbBuilder (fst input) (snd input)

let createFBPipeline inputList =
    inputList
    |> List.map createFBFunc
    |> List.reduce (>>)

let createFBPattern inputList =
    makeFBCase
    >> (inputList |> createFBPipeline)
    >> unpackFBCase

let fizzBuzz = createFBPattern [(3, "Fizz"); (5, "Buzz")]

let fizzBuzzBaz = createFBPattern [(3, "Fizz"); (5, "Buzz"); (7, "Baz")]

let fizzBuzzBazQuux = createFBPattern [(3, "Fizz"); (5, "Buzz"); (7, "Baz"); (11, "Quux")]

[1..100]
    |> List.iter (fizzBuzz >> printfn "%A")
[1..100]
    |> List.iter (fizzBuzzBaz >> printfn "%A")
[1..100]
    |> List.iter (fizzBuzzBazQuux >> printfn "%A")

Where is it obvious that I'm not thinking functionally?
What could I change to make a more idiomatic F#-style program?
(Note that I don't really "get" monads or workflows yet, although I'm aware they exist).

I think it all seems quite functional. So nothing to argue about there. I like your use of List.reduce (>>).

Instead of the use of the union type FBCase, I would use the tuples directly in a way like this (some of the functions then may need a renaming):

let makeFBCase num =
    (num, "")

let unpackFBCase (num, txt) =
    match txt with
    | "" -> num |> string
    | _ -> txt

// Instead of input I use the tuple (num, text) to make it possible to call the elements directly
let fbBuilder (denom, str) (num, txt) =
    match num % denom with
    | 0 -> (num, txt + str)
    | _ -> (num, txt)

let createFBPipeline inputList =
    inputList |> List.map fbBuilder |> List.reduce (>>)

let createFBPattern inputList =
    makeFBCase >> (inputList |> createFBPipeline) >> unpackFBCase

let fizzBuzz = createFBPattern [(3, "Fizz"); (5, "Buzz")]

let fizzBuzzBaz = createFBPattern [(3, "Fizz"); (5, "Buzz"); (7, "Baz")]

let fizzBuzzBazQuux = createFBPattern [(3, "Fizz"); (5, "Buzz"); (7, "Baz"); (11, "Quux")]

In this way I don't think you lose any clarity.

Although your efforts show a good understanding of details of functional programming, I think you make it all too complicated. The overall task is to map a list of integers to either the integer itself or a text in respect to one or more predicates. That can be done like this:

let fizzBuzz fizzBuzzes data =
    let checkFizzBuzzes value =
        let result = fizzBuzzes |> List.fold (fun acc (num, txt) -> if value % num = 0 then sprintf "%s%s" acc txt else acc) ""
        match result with
        | "" -> value |> string
        | _ -> result

    data |> List.map checkFizzBuzzes

let data = [1..30]
data |> fizzBuzz [ (3, "Fizz"); (5, "Buzz"); (7, "Baz"); (11, "Quux") ] |> List.iter (printf "%s ")


Mars Rover problem in J I've written an implementation of the "famous" Mars Rover simulation, (another question on this) whose problem statement is this:

The first line of input is the upper-right coordinates of the plateau, the lower-left coordinates are assumed to be 0,0.
The rest of the input is information pertaining to the rovers that have been deployed. Each rover has two lines of input. The first line gives the rover's position, and the second line is a series of instructions telling the rover how to explore the plateau.
The position is made up of two integers and a letter separated by spaces, corresponding to the x and y co-ordinates and the rover's orientation.
Each rover will be finished sequentially, which means that the second rover won't start to move until the first one has finished moving.
Output:
The output for each rover should be its final co-ordinates and heading.
Test Input:
5 5
1 2 N
LMLMLMLMM
3 3 E
MMRMMRMRRM

Expected Output:
1 3 N
5 1 E

This is my working solution:
dirs =: 'NESW'

input =. _4]\ 2 }. '' cut (' ',LF) charsub fread '~/Desktop/marsrover.txt'

NB. State transitions are the directions rotated forward and backwards
st =. 0,."1~ (] ,. 1 |. ] ,.  _2 |. ]) i. # dirs

NB. R = 1, L = 2, anything else = 0
a =. ('R'=a.) + (2 * 'L'=a.)

NB. Run the state machine against the input, initial facing on left
facing =: dirs {~ 4 {"1 ] ;:~ 5 ; st; a; 0 _1 , _1 ,~ dirs i. [

NB. Detect motion in each direction
moves =: ] #~ 1 - 'M' i. [

simulate =: monad define "1
  'x0 y0 dir commands' =. y
  pos =. ". x0, ' ', y0
  f =. dir facing commands
  dest =. pos + (+/ (dirs i. commands moves f) { 4 2 $ 0 1   1 0   0 _1   _1 0)
  (": dest), ' ', {: f
)

echo simulate input
exit ''

My plan for this program was basically this:

Run a state machine with the current facing against the input to figure out what the facing is at the moment of each command. This is what facing calculates.
Get the facing corresponding to each M command
Take the sum of "move" vectors to get the total change in coordinates. N = 0 1, so three moves with facing N will be the sum of 0 1 + 0 1 + 0 1 = 0 3.
The solution is the sum of the total change in coordinates with the initial coordinate, plus the final facing state.

What I'd like review for, is to find out if this code is idiomatic J, if there are more direct ways to do what I'm trying to do here, and especially in the simulate verb, if there are better ways to handle taking apart the input and passing it through the other verbs. I am happy that I figured out how to use sequential machine, but overall this solution feels a bit gassy to me. But I haven't found another implementation to compare to.

I applaud any use sequential machine.  A primitive that simulates a state machine... fruit only the tree of J could produce... why resist?

So while in that way ;: is quintessential J, state machines are, on the other hand, procedural in mindset.  Idiomatic J, by contrast, tends to frame the problem as a whole -- to solve in a single leap, rather than step by step.

How can we do that in this case?

The big trick is to encode the directions as complex numbers.  An L just multiplies the current direction by i, or 0j1 in J.  And an R multiplies by 0j_1.  Then your direction at any point in time is just the scan product */\ of your starting position and the sequence of encoded L and R.  For this step, M is encoded as 1, since it doesn't alter the direction.

Making the above concrete, let's see how the first example looks.  The encoded commands (top) and their scan product (bottom):

┌───┬───┬──┬────┬────┬───┬─┬───┬───┬───┐
│0j1│0j1│1 │0j1 │1   │0j1│1│0j1│1  │1  │
├───┼───┼──┼────┼────┼───┼─┼───┼───┼───┤
│0j1│_1 │_1│0j_1│0j_1│1  │1│0j1│0j1│0j1│
└───┴───┴──┴────┴────┴───┴─┴───┴───┴───┘

We end facing N, as desired.

Similarly, thinking of adding complex numbers as vector addition, the final position is just the sum of the initial position and every direction vector corresponding to an M.  The latter sum is just the sum of the element-wise product of the bottom row above, and the boolean mask of Ms:

┌───┬──┬──┬────┬────┬─┬─┬───┬───┬───┐
│0j1│_1│_1│0j_1│0j_1│1│1│0j1│0j1│0j1│
├───┼──┼──┼────┼────┼─┼─┼───┼───┼───┤
│0  │0 │1 │0   │1   │0│1│0  │1  │1  │
└───┴──┴──┴────┴────┴─┴─┴───┴───┴───┘

Using your code for argument parsing, and putting this all together, we get:

NB. convert direction to complex number

j =: 0j1 ^ 'ENWS' i. ]

NB. full solution

final =. 3 : 0
  'a b dir cmds' =. y
  dirs=. */\ (j dir) , 0j1 ^ 'ML.R' i. cmds       NB. all directions
  pos =. (a j.&". b) + +/ dirs * 0 , 'M' = cmds   NB. all positions
  (+. pos) ; j inv {: dirs
)

and running it:

f=. 0 : 0
5 5
1 2 N
LMLMLMLMM
3 3 E
MMRMMRMRRM
)

input =.  _4]\ 2 }. '' cut (' ',LF) charsub f

echo input
echo final"1 input

produces:

┌─┬─┬─┬──────────┐
│1│2│N│LMLMLMLMM │
├─┼─┼─┼──────────┤
│3│3│E│MMRMMRMRRM│
└─┴─┴─┴──────────┘
┌───┬─┐
│1 3│N│
├───┼─┤
│5 1│E│
└───┴─┘

Try it online!


C-Style unsigned char parsing and manipulation Note that I'm using a C++ compiler ( hence, the cast on the calloc function calls) to do this, but the code is essentially C.

Basically, I have a typedef to an unsigned char known as viByte, which I'm using to create a string buffer to parse a file from binary (a TGA file, to be exact - but, that's irrelevant).

I'm writing basic functions for it right now; append, prepend, new, etc.

The problem is that, on the first iteration of the first loop in viByteBuf_Prepend, I get a segmentation fault. I need to know why, exactly, as this is something which could keep me up all night without some pointers (pun intended).

I also would like to know if my algorithms are correct in terms of how the buffer is pre-pending the viByte string. For example, I have a feeling that using memset too much might be a bad idea, and whether or not my printf format for the unsigned char is correct (I have a feeling it isn't, as nothing is getting output to my console).

Compiling on GCC, Linux.

#ifdef VI_BYTEBUF_DEBUG
void viByteBuf_TestPrepend( void )
{
    viByteBuf* buf = viByteBuf_New( 4 );

    buf->str = ( viByte* ) 0x1;

    printf(" Before viByteBuf_Prepend => %uc ", buf->str);

    viByteBuf_Prepend( buf, 3, ( viByte* ) 0x2 );

    printf(" After viByteBuf_Prepend => %uc ", buf->str);
}
#endif

viByteBuf* viByteBuf_New( unsigned int len )
{
    viByteBuf* buf = ( viByteBuf* ) calloc( sizeof( viByteBuf ), 1 );

    const int buflen = len + 1;

    buf->str = ( viByte* ) calloc( sizeof( viByte ), buflen );
    buf->len = buflen;

    buf->str[ buflen ] = '\0';

    return buf;
}

void viByteBuf_Prepend( viByteBuf* buf, unsigned int len, viByte* str )
{
    unsigned int pos, i;
    const unsigned int totallen = buf->len + len;
    viByteBuf* tmp = viByteBuf_New( totallen );
    viByte* strpos = buf->str;

    memset( tmp->str, 0, tmp->len );

    int index;

    for( i = 0; i < buf->len; ++i )
    {

       index = ( buf->len - i ) - 1;

       *strpos = buf->str[ 0 ];
       ++strpos;
    }

    memset( buf->str, 0, buf->len );

    printf( "%uc\n", buf->str );

    i = totallen;

    for ( pos = 0; pos < len; ++pos )
    {
        tmp->str[ pos ] = str[ pos ];
        tmp->str[ i ]   = buf->str[ i ];

        --i;
    }

    memset( buf->str, 0, buf->len );

    buf->len = tmp->len;

    memcpy( buf->str, tmp->str, tmp->len );

    viByteBuf_Free( tmp );

    //memset(  )
    //realloc( ( viByteBuf* ) buf, sizeof( viByteBuf ) * tmp->len );
}

Your problem is here:
buf->str[ buflen ] = '\0';

You allocate a buffer (str) of buflen bytes.
This means you can index it (str) from 0 -> (buflen-1).
Thus the accesses above is writing one past the end of the buffer and corrupting memory.
You probably meant:
buf->str[ len ] = '\0';

Since you are using C++ compiler. You should use std::vector<viByte> or std::basic_string<viByte>.
This kind of bug is easy to introduce and unless you have good unit test hard to find. So why not use a class that already is unit test and has had 10 years of millions of people lookign at it to make sure it works correctly.
Other comments:
Broken and illegal.
    buf->str = ( viByte* ) 0x1;

Wrong. buf->str is a pointer and it was expecting a unsigned integer (they are not the same). Use %p to print a pointer value.
    printf(" Before viByteBuf_Prepend => %uc ", buf->str);

Broken and illegal (0x2 is not yours)
    viByteBuf_Prepend( buf, 3, ( viByte* ) 0x2 );

No check for NULL. (calloc can return NULL when it fails).
    viByteBuf* buf = ( viByteBuf* ) calloc( sizeof( viByteBuf ), 1 );

Again no check for NULL.
    buf->str = ( viByte* ) calloc( sizeof( viByte ), buflen );

Is this really what you want? Buflen includes the portion with the '\0' character. You need to be consistent and decide if len does or does not include the null.
    buf->len = buflen;

As pointed out above (write past the end of the buffer).
    buf->str[ buflen ] = '\0';

How do you handle failure?
As pointed out above calloc can fail. How do you propagate that failure to the caller.
    viByteBuf* tmp = viByteBuf_New( totallen );

Why are you nulling the whole array. You are just about to write over it.
Seems like a complete waste of time.
    memset( tmp->str, 0, tmp->len );

Looks like this loop is trying to reverse the string in place.
Which will fail as by the time you overwrite the first half the second half is a copy of the back half. Luckily it completely fails because you don't actually use the index variable.
    for( i = 0; i < buf->len; ++i )
    {

       index = ( buf->len - i ) - 1;

       *strpos = buf->str[ 0 ];
       ++strpos;
    }

Now that you have failed to reverse it. You then overwrite it again with 0 (presumably to cover up the failure to reverse it).
    memset( buf->str, 0, buf->len );

Illegal parameter to printf() again.
    printf( "%uc\n", buf->str );

More copying for unknow reason.
    memcpy( buf->str, tmp->str, tmp->len );

This code is so full of bugs. It has no write to work.


Created a serializable object that can handle primitive types I don't need a lot and I don't need for my code to handle everything.  Even now, I'm just looking at handling serialization.  I'm pretty much building on it as needed.  I am trying to support vectors right now and am looking for an idea on how to extend what I have a little to support this.  Basically I've created a property class that has a callback function when the property changes (keeping it simple with equals operator for now).  This updates a dictionary object that can be serialized.  Below is my code.  It's my first time posting.  If I didn't follow some rules by accident, I'd love some guidance.  Working sample here: https://onlinegdb.com/H10iJVC3f

#include <iostream>
#include <functional>
#include <sstream>
#include <map>

using namespace std;

template <class T>
class Prop
{
public:
   Prop(const std::string &key, const std::function<void(std::string, std::string)> &changed);

private:
   std::string Key;
   T Value;
   std::function<void(std::string, std::string)> changed;

public:
   void operator=(const T &value) {
      this->Value = value;
      this->changed(this->Key, static_cast<ostringstream&>(ostringstream() << value).str());
   }
};

template <class T>
Prop<T>::Prop(const std::string &key, const std::function<void(std::string, std::string)> &changed)
{
   this->Key = key;
   this->changed = changed;
}

class TheList
{
private:
   std::map<std::string, std::string> theList;

protected:
   void Update(const std::string &key, const std::string &value);

public:
   std::string ToJson();
};

void TheList::Update(const std::string &key, const std::string &value)
{
   theList[key] = value;
}

// TODO: body of this method will change and will use a processing library.
std::string TheList::ToJson()
{
   std::string json = "{\n";

   for (auto it : theList)
   {
      json += "\t\"" + it.first + "\": " + "\"" + it.second + "\",\n";
   }

   json.pop_back();
   json.pop_back();

   json += "\n}";

   return json;
}

// A Serializable object.
class User : public TheList
{
public:
   User();

public:
   Prop<std::string> Name;
   Prop<short> Age;
   Prop<std::string> Address;
};

User::User() : Name("Name", std::bind(&User::Update, this, std::placeholders::_1, std::placeholders::_2)),
Age("Age", std::bind(&User::Update, this, std::placeholders::_1, std::placeholders::_2)),
Address("Address", std::bind(&User::Update, this, std::placeholders::_1, std::placeholders::_2))
{
}

int main()
{
    User user;

    user.Name = "Pamela";
    user.Age = 25;
    user.Address = "USA";

    std::string json = user.ToJson();

    cout << json << endl;

    return 0;
}

Don’t say using namespace std;.

And you were qualifying names in std anyway!  What gives?

std::function<void(std::string, std::string)> &changed)

Why must you pass the strings by value to the callback?

You have this std::function signature spelled out twice — you might want to make an alias for that.

using cbfunc = std::function<void(std::string, std::string)> changed)
  ⋮
⋯Prop(const std::string& key, const cbfunc& changed)

In C++, the style is to put the & modifier with the type, not the declared name.  This has been true since Bjarne’s original book, where it is pointed out explicitly.

this->Key = key;
   this->changed = changed;

No…
Write your constructor initializers, not assignment on top of default-initialized members.  Learn the difference between initialization and assignment.

: Key{key}, changed{changed}

and note that having the parameter with the same name as the data member is not a problem and is in fact idiomatic.  If you ever need to write this-> a lot, you are doing something wrong.

In this particular case, take advantage of move semantics.  In full:

Prop<T>::Prop(std::string key, cbfunc changed)
: Key{std::move(key)}, changed{std::move(changed)}
{ }

static_cast<ostringstream&>(ostringstream() << value).str()

What’s with the static_cast?  Don’t do that.

Since you are modelling a value that does things behind the scenes when written to, using operator= for the name is probably OK in this case.  But it should not be void.  Either return *this or value.

Again, don’t use this-> to access members.

Prop& operator= (const T& value)
    {
    set(value);
    return *this;
    }

void set (const T& value)
    {
    Value= value;
    std::ostringstream& fmt;
    fmt << value;
    changed (Key, fmt.str());
    }

But, there is a handy package that already does the formatting exactly like this.  So just write

    Value= value;
    changed (Key, boost::lexical_cast<std::string>(value));

Note that I made a normal-named function set and then defined operator= to call it.  That will be much better for people who read your code later, since operator= carries certain expectations.

You did not provide the matching get function!  Your values are write-only!

In keeping the callbacks as invisible as possible, the design would be to have a function get and then an operator T that calls it.

 int x;
 Prop<int> px {"px", some_function};
 px = 5;  // invisible in
 x = px;  // and invisible out.

std::bind is deprecated since C++11.  Sometimes it is indeed the simplest way to write something.  But with placeholders, not so much.

Since all your props use the exact same function, you should just set it up once.

// member of class User
const auto binderCB = [this](std::string left, std::string right)
    { Update (left, right); }

Then the constructor※ is

User::User()
: Name{cb("Name")}, Age{cb("Age")}, ⋯

(just be sure cb is the first member so it is initialized first!)

(almost always) use auto.
You don’t have any uses at all, so I’ll point it out with an example in your main:

auto json = user.ToJson();

The main might be a throw-away example, but I’d rather initialize the user object in one go, not assign to each prop individually.

User user { "Pamela", 25, "USA" };

for a plain struct, this would just work as an aggregate initializer.  You need a constructor that takes three arguments (with defaults) to provide the same functionality.

Notes

※ I’d normally write all those as default initializers in-line and not give a constructor at all.  But for this specific example, I worry about referring to other members.  I’d need to look that up to see if it’s OK.

Rather than updating a map every time you change a value, you can use the same effort of setting up each member with an identifier and callback but provide a general way to explore the current values and types when you are ready to do something with them.  That is, pull rather than push.

There are libraries that do this, most notably Boost::Fusion.  But there are more modern ways — I think Boost::Hanna has that, but I don’t recall for sure.  An any case, the thing to do is to supply a non-member get template so the object can be used just like a tuple`.  This will let it interoperate with other code that follows this approach, including the built-in structured bindings.

Since your callback turns everything into a string representation, your Json rendering will not know what things were originally strings and what are numbers.  Your code will output

"Age" : "25"

instead of

"Age" : 25

In your design (where everything is handed over as a string) you can include the quoting as part of the update callback.  Use one for strings that add quotes and also escapes out needed characters, and another for numbers that does not.  Then, don’t add the quotes in your outputing function.


Check if multiple lists contain an element from a large external list I have a list of lists (many tokenised sentences). For anyone who doesn't know what tokenised sentences are, my list is like so:

list1 = [['hello', 'my', 'name'], ['this', 'is', 'stack', 'exchange'], ... ]

I have a list of key words as well, key_words.

For every sentence in list, I want to check if it is in key_words. Moreover, I want a single method to be applied to each sentence. Below is my working (but inefficient) code:

list1 = [['hello', 'my', 'name'], ['this', 'is', 'stack', 'exchange']]
key_words = ['hello', 'name', 'stack']

def get_features(sentence, key_words):
    return [word for word in sentence if word in key_words]

f = []
for sent in list1:
    f.append(get_features(sent, key_words))

This is fine, but my dimensions are like so:

len(list1) = 45,000
len(key_words) = 35,000

This is of course inefficient, and I would like to find a faster way of doing this. Could dictionaries be utilised in some way? I was thinking of changing key_words from a list to a dictionary of key:value = word:1. Then I could do something like

return [word for word in sentence if key_words[word] does not give error]

but I'm unsure how if does not give error would be implemented. Doing this would allow O(1) access to words in key_words if they are actually in there, rather than having to search the whole list until it is found, with O(n).

In the question, and as suggested by Mathias Ettinger, the reasoning behind locating an \$O(1)\$ search time-complexity as opposed to the current \$O(n)\$ complexity is correct.

However, the best approach is to use the set data structure instead of the list structure. Sets have \$O(1)\$ search time complexity since they are implemented using hash tables (https://wiki.python.org/moin/TimeComplexity) and they are similar to a list conceptually, and thus make much more sense than complicating things by using a dictionary.

The code (with the large dimensions mentioned in the question) runs in under 10 seconds like so:

list1 = [['hello', 'my', 'name'], ['this', 'is', 'stack', 'exchange']]
key_words = ['hello', 'name', 'stack']

def get_features(sentence, key_words):
    return [word for word in sentence if word in key_words]

f = []
key_words = set(key_words)
for sent in list1:
    f.append(get_features(sent, key_words))


SkipList implementation in Java The code works fine, but could likely be better. How can I improve it?

From Wikipedia:

A skip list is a data structure that allows fast search within an ordered sequence of elements. Fast search is made possible by maintaining a linked hierarchy of subsequences, with each successive subsequence skipping over fewer elements than the previous one. Searching starts in the sparsest subsequence until two consecutive elements have been found, one smaller and one larger than or equal to the element searched for. Via the linked hierarchy, these two elements link to elements of the next sparsest subsequence, where searching is continued until finally we are searching in the full sequence

public class SkipList<T extends Comparable<T>>{

int maxLevels;
Random rand = new Random();
Node first;
int size;

public SkipList(int listLevels){
    this.maxLevels = listLevels;

    first = new Node(null);
    Node d = new Node(null);
    first.down = d;

    for(int j = listLevels - 2; j >= 0; j--){

        d.down = new Node(null);
        d = d.down;
    }
}

/*
 *Makes a new Node containing data and links the node to the node
 *previous and after on all levels from the nodes height and below.
 */

public void insert(T data){

    int levels = setHeight();
    Node newNode = new Node(data);
    Node node = first;

    for(int i = maxLevels; i > levels; i--){
        node = node.down;
    }

    for(int i = levels; i >= 1; i--){

        Node previous = findPreviousNodeOnLevel(data, node);
        newNode.next = previous.next;
        previous.next = newNode;

        if(i > 1){
            newNode.down = new Node(data);
            newNode = newNode.down;
            node = previous.down;
            }
    }
    size++;
}

/*
 * Gives a random number between 1 and maxLevels
 */
private int setHeight(){
    int n = 0;
    int level = 0;
    while( n != 1 && level < maxLevels){
        n = rand.nextInt(2) + 1;
        level ++;
    }
    return level;
}

/*
 * Finds @param data in the list
 */
public T get(T data){
    Node node = getPreviousNode(data);
    if(node == null || node.next == null || node.next.data == null){
        return null;
    }
    return getPreviousNode(data).next.data;
}

/*
 * Removes @param data from the list
 */

public boolean remove(T data){
    Node previous = getPreviousNode(data);
    if(previous.next != null){
        Node toRemove = previous.next;
        previous.next = toRemove.next;

        while(toRemove.down != null){
            toRemove = toRemove.down;
            previous = findPreviousNodeOnLevel(data, previous.down);
            previous.next = toRemove.next;
        }

        return true;
    }

    return false;
}

/*
 * Returns the node that is before @param data in the list.
 */
private Node getPreviousNode(T data){
    Node previous = findPreviousNodeOnLevel(data, first);

    while(true){
        if(previous.next != null && previous.next.data.equals(data)){
            return previous;
        }

        if(previous.down != null){
        previous = previous.down;
        previous = findPreviousNodeOnLevel(data, previous);
        }
        else{
            return null;
        }
    }
}

/*
 * Returns the node before @param data at the same height/level as @param current
 */
private Node findPreviousNodeOnLevel(T data, Node current){

    while(current.next != null && current.next.data.compareTo(data) < 0){
        current = current.next;
    }
    return current;
}

public int getSize(){
    return size;
}

private class Node {

    T data;
    Node next;
    Node down;

    Node(T data){
        this.data = data;
    }
}

}

The goal of a skip list is that you can do a find by doing a loop like:

Node find(T value){
    Node start = root;

    while(start!=null && comp(start.value, value) != 0){

        if(comp(start.next.value, value) < 0){
            start = start.next;
        }else{
            start = start.down;
        }

    }
    return start;
}

Note the comp helper function. I added that so you can easily make it use a custom Comparable<T> passed in during construction.

This can be easily expanded to also keep a list of nodes to update should you want to insert/delete a value:

List<Node> findAllPrevs(T value){
    List<Node> prevs = new ArrayList<>();
    Node start = root;

    while(start!=null){

        if(comp(start.next.value, value) < 0){
            start = start.next;
        }else{
            prevs.add(start);
            start = start.down;
        }

    }
    while(prevs.get(prevs.size()-1).down!=null){
        prevs.add(prevs.get(prevs.size()-1).down);
    }
    return prevs;
}

Then you only need to update the last depth nodes in prevs when you add or remove a value.


Async method with multiple tasks I'm trying to make my ASP.net Web API methods asynchronous.  This method is in the Business Logic layer of my N-tier. I'm waiting for the result from my 1st task and use it on the 2nd task, but I think I'm doing it in a wrong way.  Can you please give me advice or feedback regarding my code?

public async Task<DTOUser> CreateUser(DTORegister source)
{
    var result = new DTOUser();

    try
    {
        var user = new DTOUser()
        {
            Active = (int)defaultNumber.One,
            Deleted = (int)defaultNumber.Zero,
            DateCreated = DateTime.Now,
        };

        result = await Task.Run(() =>_RepositoryUser.Add<DTOUser, DTOUser>(user));

        //This part is where I dont feel confident
        var login = new DTOLogin()
        {
            Email = source.Email,
            Password = source.Password,
            OwnerID = result.UserID, //using the result of my 1st task here.
            DateCreated = DateTime.Now

        };
        //use the login object to my 2nd task
        var task2 = await Task.Run(() =>_RepositoryLogIn.Add<DTOLogin, DTOLogin>(login));

        result.IsSuccessful = true;

    }
    catch (Exception ex)
    {
        result.ErrorMsg = "BL Error - " + ex.Message;
        result.IsSuccessful = false;
    }
    return result;
}

Both _RepositoryUser.Add and _RepositoryLogIn.Add are calling the same Generic Repository method from DAL:

public TOutPut Add<TOutPut, TInput>(TInput input)
        {
            var entityObj = MappingConfiguration.MapObjects<TInput, TType>(input); //Calling my automapper method to map my Entity object and DTO Object
            _entity.AddObject(this.GetEntitySetName(typeof(TType).Name), entityObj);
            _entity.SaveChanges();
            return MappingConfiguration.MapObjects<TType, TOutPut>(entityObj);

        }

To me it seems like you are trying to make a async task for the sake of making it async to fit in with.

  I'm trying to make my ASP.net Web API methods asynchronous

I don't see any reason for these pieces of code to be async.  They aren't doing long running tasks and aren't even hitting the db (unless Add doesn't something we don't know about of course like saves as well which is a different point of review right there).

result = await Task.Run(() =>_RepositoryUser.Add<DTOUser, DTOUser>(user));

//use the login object to my 2nd task
var task2 = await Task.Run(() =>_RepositoryLogIn.Add<DTOLogin, DTOLogin>(login));

Once you have removed these, the method becomes a truly synchronous method.  As it's a pretty trivial method anyway this shouldn't be a problem and making it async only confuses the issue for no benefit.


Mapping one JSON message format to another in Java This code takes a JSONObject, passes it to another class which is responsible for mapping the data in the JSONObject to a different format (Still a JSONObject though).
I'd just like confirmation if the code below is bad practice or not, I feel it is:

public class DeliveredCommunication{

    private String deviceId;
    private int versionMajor;
    private int versionMinor;
    private int versionPatch;
    private JSONArray messages;
    private DataMapper dataMapper = null;
    private Logger log = (Logger) LogManager.getLogger();

public DeliveredCommunication(String deviceId, int versionMajor, int versionMinor, int versionPatch,
            JSONArray messages) {
        this.deviceId = deviceId;
        this.versionMajor = versionMajor;
        this.versionMinor = versionMinor;
        this.versionPatch = versionPatch;
        this.messages = messages;

    }

    public String getDeviceId() {
        return deviceId;
    }

    public int getVersionMajor() {
        return versionMajor;
    }

    public int getVersionMinor() {
        return versionMinor;
    }

    public int getVersionPatch() {
        return versionPatch;
    }

    public JSONArray getMessages() {
        return messages;
    }

    public void mapData(){
        dataMapper = new DataMapper();
        JSONObject msg = null;
        for(int i = 0; i < messages.length(); i++){
            try {

                msg = messages.getJSONObject(i);
                dataMapper.map(msg);

            } catch (JSONException e) {

                log.warn(deviceId + ": A JSONException occurred trying to map Data for database insertion: " + msg, e);
            }
        }
    }

    public DataMapper getDataMapper(){

            return dataMapper;

    }
}

I'm not 100% sure though, because the mapData() method in the DataMapper only maps one object at a time.

Would it be a better design to pass the whole list into the DataMapper first, then return the DataMapper object without the data being mapped and let the class that called getDataMapper() call dataMapper.mapData() to map all the data in one go?

Does that make sense?

Or Instead of having getDataMapper() I could just have a method called getMappedData() and have that method do the work in mapData() and just get and return a list of mapped objects from the DataMapper.

It's always a bad sign if you do have different levels of presentations inside a class.

It would be much better to separate the concerns and implement based on the "single responsibility principle".

That would mean that you have the class DeliveredCommunication which holds a set of Strings/Objects for the messages. This is also known as a DTO (Data Transfer Object).

Everyone who wants to use it, can now decide how the presentation should look like.

Transforming data to a specific exchange format should be done at the level of your application where the exchange happens.

Hope it is clear enough and that it helps

P.S.: I would change DeliveredCommunicaton to Communication with a property called state which could be realized with an enum of which one value would be 'delivered'.


Function to spell numbers below 1,000,000,000 I wrote simple code to spell the number below 1,000,000,000. It works perfectly as per my intention. However, I do think my code is a little bit messy since I only able to imagine of using decision statements in my function/method.

I'm still learning in writing Python function/method code. I will be glad if anyone here can review my code and comment on how to improve it.

def main():
    value = int(input("\nPlease enter a positive integer < 1000000000 : "))
    if value < 100:
        print(processDigit(value))
    elif value > 100:
        print(intName(value))

# turns a number into its English name
def intName(number):
    part = number
    nameMillion = ""
    nameThousand = ""
    nameHundred = ""

    if part >= 1000000:
        divide = part // 1000000
        part = part % 1000000
        divide1 = divide // 100
        part1 = divide % 100
        nameMillion = digitName(divide1) + " hundred" + processDigit(part1) + " million "

    if part >= 1000:
        divide = part // 1000
        part = part % 1000
        divide1 = divide // 100
        part1 = divide % 100
        nameThousand = digitName(divide1) + " hundred" + processDigit(part1) + " thousand "

    if part >= 100:
        divide = part // 100
        part = part % 100
        nameHundred = digitName(divide) + " hundred" + processDigit(part)

        return nameMillion + nameThousand + nameHundred

# process the three digit number
def processDigit(number):
    part = number
    name = ""

    if part >= 20:
        name = name + " " + tensName(part)
        part = part % 10

    elif part >= 10:
        name = name + " " + teenName(part)
        part = 0

    if part > 0:
        name = name + " " + digitName(part)

    return name

# turns a digit into its English name
def digitName(digit):
    if digit == 1: return "one"
    if digit == 2: return "two"
    if digit == 3: return "three"
    if digit == 4: return "four"
    if digit == 5: return "five"
    if digit == 6: return "six"
    if digit == 7: return "seven"
    if digit == 8: return "eight"
    if digit == 9: return "nine"
    return ""

# turns a number between 10 and 19 into its English name
def teenName(number):
    if number == 10: return "ten"
    if number == 11: return "eleven"
    if number == 12: return "twelve"
    if number == 13: return "thirteen"
    if number == 14: return "fourteen"
    if number == 15: return "fifteen"
    if number == 16: return "sixteen"
    if number == 17: return "seventeen"
    if number == 18: return "eighteen"
    if number == 19: return "nineteen"
    return ""

# gives the name of tens part of a number between 20 and 99
def tensName(number):
    if number >= 90: return "ninety"
    if number >= 80: return "eighty"
    if number >= 70: return "seventy"
    if number >= 60: return "sixty"
    if number >= 50: return "fifty"
    if number >= 40: return "fourty"
    if number >= 30: return "thirty"
    if number >= 20: return "twenty"
    return ""

# start the program
main()

def main():
    value = int(input("\nPlease enter a positive integer < 1000000000 : "))
    if value < 100:
        print(processDigit(value))
    elif value > 100:
        print(intName(value))

You extracted logic which should have been encapsulated inside your function into the main function. That should never happen. If intName can't handle values between 1 and 99 itself, it's not living up to its name.

def intName(number):
    part = number
    nameMillion = ""
    nameThousand = ""
    nameHundred = ""

    if part >= 1000000:
        divide = part // 1000000
        part = part % 1000000
        divide1 = divide // 100
        part1 = divide % 100
        nameMillion = digitName(divide1) + " hundred" + processDigit(part1) + " million "

    if part >= 1000:
        divide = part // 1000
        part = part % 1000
        divide1 = divide // 100
        part1 = divide % 100
        nameThousand = digitName(divide1) + " hundred" + processDigit(part1) + " thousand "

    if part >= 100:
        divide = part // 100
        part = part % 100
        nameHundred = digitName(divide) + " hundred" + processDigit(part)

        return nameMillion + nameThousand + nameHundred

This function is simply wrong, for multiple reasons. Let's step through them one by one:

The return statement is sometimes unreachable

Check your indentation. Currently the return statement is only reachable if number % 1000 >= 100.

Numbers >=1,000 and <100,000 do exist

Your forgot to handle the case when the number doesn't contain the term "hundred". The same issue exists with numbers >= 1,000,000 and <100,000,000.

Writing the same code 3x in a row, with the same mistakes

You are using different constants, but the logic is mostly identical. The copy & paste is pretty obvious. And it had exactly the effect you would expect: You copied the mistakes as well.

This would have been easily solved by extracting the common parts of the logic properly.

0 is a number

While you don't say "zero hundred" or "zero thousand", "zero" alone is a valid number and should be supported properly.

Structuring the code properly

nameMillion = digitName(divide1) + " hundred" + processDigit(part1) + " million "

Why did you bother writing this explicitly for every potence of 1,000?

Have another look at how the numbers in the english language are constructed, and try to find ALL patterns.

If you that, you realize that for every number, every potence of 1,000 is formed identically:

[([digit "hundred" ]decimal) "suffix"]

The only exception to that is the part below 1,000 which has no suffix. However, a missing suffix can simply be approximated by an empty suffix instead.

So the logic approach is to write yourself a helper function which correctly formats every number from 0 to 999, because that is the range you need commonly.

Your next realization should be that it is easier to start constructing a number from the LSB, than from the MSB, since you then only ever need one set of constant numbers to extract the relevant part of the number.

Once you have that function, intName() can be written much clearer:

def intName(number):
    output = "";
    suffixes = ["", "thousand", "million", "billion"]

    if number == 0
        return "zero"

    while number > 0:
        number, part = divmod(number, 1000);
        if part > 0
            output = intThousandName(part) + " " + suffixes[0] + " " + output
        suffixes.pop(0)

    return output.strip()

The strip() at the end removes the trailing white space, resulting from the empty suffix, respectively the concatenation of the first valid suffix with the empty string.


Traversing through single large array to convert single array into array of arrays as per condition I have a huge fileList array where many elements of array start from sub-** pattern. I want to group these files belonging to same sub-**(for e.g.: files which start with sub-01) into one subject specific array. Thus after creating all these sub specific array they can be pushed to a large array nifti_fileList.
I have written following code in JavaScript with very crude logic:

        subFilelist = [];
    var previous_sub = null;
    var capture = null;
    var file_no = 0

    async.eachOfLimit(fileList, 200, function (file, key, cb) {
        testfile = file.relativePath;
        sub_capture_re = /sub-[a-zA-Z0-9]*/gi;
        var current_sub = testfile.match(sub_capture_re)
        if (current_sub) {
            if((file_no === 0)){
                subFilelist.push(testfile)
                file_no = file_no + 1;
                previous_sub = current_sub[0];
            }

            if ((current_sub[0] === previous_sub) && (file_no > 0)){
                previous_sub = current_sub[0];
                subFilelist.push(file.relativePath)
            } else {
                nifti_fileList.push(subFilelist)
                subFilelist = [];
                file_no =0
            }
        }
        process.nextTick(cb);
    });
    console.log(nifti_fileList);

Sample input:

fileList = [/sub-09/func/sub-09_task-genInstrAv_run-05_physio.tsv.gz,
/sub-10/anat/sub-10_T1w.json,
/sub-10/anat/sub-10_T1w.nii.gz,
/sub-10/fmap/sub-10_magnitude1.nii.gz,
/sub-10/fmap/sub-10_magnitude2.nii.gz,
/sub-10/fmap/sub-10_phasediff.json,
/sub-10/fmap/sub-10_phasediff.nii.gz,
/sub-11/anat/sub-11_T1w.nii.gz]

nifti_fileList = [[/sub-09/func/sub-09_task-genInstrAv_run-05_physio.tsv.gz],
    [/sub-10/anat/sub-10_T1w.json,
    /sub-10/anat/sub-10_T1w.nii.gz,
    /sub-10/fmap/sub-10_magnitude1.nii.gz,
    /sub-10/fmap/sub-10_magnitude2.nii.gz,
    /sub-10/fmap/sub-10_phasediff.json,
    /sub-10/fmap/sub-10_phasediff.nii.gz],
    [/sub-11/anat/sub-11_T1w.nii.gz]]

Can this be made more efficient?

Observations and Remarks

Your code is pretty complex and hard to read, as it involves a
callback with a complex outside state encoded within the global
revious_sub, capture and file_no variables. Each time your
callback is executed, it modifies some of those state variables =
there are many side-effects, which makes things confusing.
The variable capture is unused.
Not all variables are declared and thus become global.
Your sample input seems to be a list of strings, but within your
callback you access the path via testfile = file.relativePath;
indicating that you deal with objects.
It doesn't seem necessary to me to perform a basic array operation
without IO delays asynchronously. So I recommend a simpler
synchronous function.
Your resulting grouped file list is not very useful, as its groups
are unnamed. If we want to know the 'sub-*' prefix of a group, we
would have to manually inspect a random group element. An explicit
mapping from prefix or name to file would allow us to query e.g.
groups.get('sub-03') in constant time.
Your current implementation requires a sorted input file list. By
using a Map, you can guarantee the same runtime complexity without
having to rely on sorted input.

Suggested Implementation
Write a general purpose groupBy helper function or use a library which already includes one. Such a function would group array elements by their label or name. The user supplies a callback which returns the label or name for each array element:
function groupBy(array, cp) {
    return array.reduce((groups, next) => {
        const name = cp(next);
        if (name !== undefined) {
            const group = groups.get(name);
            if (group) group.push(next);
            else groups.set(name, [next]);
        }
        return groups;
    }, new Map());
}

For a given list of file paths, you could then simply write:
const files = [
    '/sub-09/func/sub-09_task-genInstrAv_run-05_physio.tsv.gz',
    '/sub-10/anat/sub-10_T1w.json',
    '/sub-10/anat/sub-10_T1w.nii.gz',
    '/sub-10/fmap/sub-10_magnitude1.nii.gz',
    '/sub-10/fmap/sub-10_magnitude2.nii.gz',
    '/sub-10/fmap/sub-10_phasediff.json',
    '/sub-10/fmap/sub-10_phasediff.nii.gz',
    '/sub-11/anat/sub-11_T1w.nii.gz'
];

const groups = groupBy(files, path => {
    const match = path.match(/^\/(sub-\w+)\//);
    if (match) return match[1];
});

If your file list is made up of objects instead of strings, you could write:
const groups = groupBy(files, file => {
    const match = file.relativePath.match(/^\/(sub-\w+)\//);
    if (match) return match[1];
});

The result is a Map { "sub-09" → […], "sub-10" → […], "sub-11" → […] } which we can query via e.g. groups.get('sub-09') or transform into your array of arrays via e.g. [...groups.values()].


Interop between C++ and C# via C++/CLI with callbacks I want to use an existing C++ library in a C# app. The C++ library will have async callbacks for things like OnConnect and user updates so I want to reflect this in the eventual C# code. I would appreciate thoughts on the following implementation particularly in the area of memory leaks in the CLI part.

To begin, I have a simple C++ class:

#include <string>

class CppClass
{
public:

    typedef void(*Callback)(int);

    CppClass(){};
    ~CppClass(){};

    void Connect(std::string conn_info)
    {
        OnConnect();
    };

    void OnConnect()
    {
        m_callback(42);
    };

    void SetOnCallback(Callback callback)
    {
        m_callback = callback;
    };

private:
    Callback m_callback;
};

To communicate with this, I have a C++/CLI wrapper:

using namespace System;
using namespace System::Runtime::InteropServices;

namespace CliCpp {

    public delegate void OnConnectDelegate(int);

    public ref class WrapperClass
    {
    public:

        // C# callback delegates
        CliCpp::OnConnectDelegate^ OnConnectHandler;

        // Ctor / dtor
        WrapperClass()
        {
            m_native = new CppClass();

            // Set OnConnect callback
            CliCpp::OnConnectDelegate^ managed_on_connect = gcnew CliCpp::OnConnectDelegate(this, &WrapperClass::OnConnect);
            IntPtr stub_ptr = Marshal::GetFunctionPointerForDelegate(managed_on_connect);
            CppClass::Callback fptr = static_cast<CppClass::Callback>(stub_ptr.ToPointer());
            m_native->SetOnCallback(fptr);
            GC::KeepAlive(managed_on_connect);
        };

        ~WrapperClass()
        {
            this->!WrapperClass();
        };

        !WrapperClass()
        {
            if (m_native != nullptr)
                delete m_native;
        };

        // Methods
        void Connect(String^ conn_info)
        {
            m_native->Connect(msclr::interop::marshal_as<std::string>(conn_info));
        };

        // Event handler from C++ code
        void OnConnect(int i)
        {
            // Call C# code:
            OnConnectHandler(i);
        };

    private:
        CppClass* m_native;
    };
}

And finally, I have some C# calling code:

using CliCpp;

namespace Caller
{
    class Program
    {
        static void Main(string[] args)
        {
            using (WrapperClass cls = new WrapperClass())
            {
                // Set up the C# callback
                cls.on_connect_handler = OnConnect;
                cls.Connect("Hello");
                Console.ReadLine();
            }
        }

        // Callback Function
        static void OnConnect(int i)
        {
            Console.WriteLine("C# handler called!");
        }

    }
}

So, what I would like an opinion on is:

Is this implementation of callbacks good practice?
In the C++/CLI constructor where I set up the callback, am I using gcnew and 'GC::KeepAlive(x)' correctly so as not to leak (do I need a corresponding delete etc.)?

Any input appreciated!

I'm not sure about the callbacks but you are using the destructor/finalizer in a way that might cause problems.

   ~WrapperClass()
    {
        this->!WrapperClass();
    };

    !WrapperClass()
    {
        if (m_native != nullptr)
            delete m_native;
    };

If you are doing this then the descructor should call GC.SuppressFinalize(this); because the finalizer has already been called.

Then in your finalizer you should set the m_native = nullptr; because if the GC calls it again later it will crash as the object has been deleted but it's still pointing to it.

    !WrapperClass()
    {
        if (m_native != nullptr)
        {
            delete m_native;
            m_native = nullptr;
        }
    }

As to the m_native I'd call it _nativePtr.


Centering a line of text I need a procedure that "centers" a string inside a string that is consecutive characters of a fixed length. For instance,

======================== Instructions ============================

or

**************************** hehe ********************************

might be printed by calling

PrintBetweenChars("Instructions",'=');

or

PrintBetweenChars("hehe",'*');

respectively. Per my rules, the output will always have atleast one leading character and space and one trailing character and space, and the input will be shrunk if need be. So if input is abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ and the maximum line length is only 26 and the character is '-' then the ouput is

- abcdefghijklmnopqrstuv -

Furthermore, to handle the case of input having an odd length like 5, there will be one more character on the left than right. So the output would be like

-- abcdefghijklmnopqrstu -

if the input was abcdefghijklmnopqrstu and the maximum line length was 26. I tried implementing this and came up with the messiest-looking procedure ever:

public static void PrintBetweenChars ( string input, char c )
{

    // Require input be at most (_outputMaxLength - 4) chars to accomodate 1 beginning and trailing char and space
    // Shrink input if necessary
    int shrunkInputLen = OutputFormatter._outputMaxLength - 4;
    if ( input.Length > (shrunkInputLen) )
    {
        input.Remove(shrunkInputLen);
    }
    string charline = new String(c, (OutputFormatter._outputMaxLength - input.Length - 2) / 2);
    Console.WriteLine("{0} {1} {2}", (input.Length & 1) == 1 ? charline + c.ToString() : charline, input, charline);
}

Is there a more compact, elegant, efficient, clever and readable way of doing this using the .NET library?

In testing your code I noticed a crash with text longer than the max.  Not sure if you consider this simpler but it does handle the tails case.

public void PrintBetweenChars(string input, char c)
{
    // Require input be at most (_outputMaxLength - 4) chars to accomodate 1 beginning and trailing char and space
    // Shrink input if necessary
    string UserText = " " + input.Substring(0, Math.Min(OutputFormatter._outputMaxLength - 4, input.Length)) + " ";
    string DisplayText = UserText.PadLeft((OutputFormatter._outputMaxLength / 2) + (UserText.Length / 2) + (UserText.Length % 2), c);
    Console.WriteLine(DisplayText.PadRight(OutputFormatter._outputMaxLength, c));
}