Largest Prime Factor (PE3) using OOP I want to learn some C# syntax/paradigms (I'm more used to Java), and have been wanting to get a bit better at math as well, so I solved ProjectEuler3: Largest prime factor with the following small program in LINQPad. It gives the correct answer and completes in 0.05s-0.08s.

For reference, here is the problem statement:

  The prime factors of 13195 are 5, 7, 13 and 29.

  What is the largest prime factor of the number 600851475143 ?

All feedback welcomed. In particular, are there some C# mistakes I am making or features I am missing on due to being very new at it?

I'm also not that great at math at the moment, if you know of edge cases that I may have missed with the calculations below, please let me know so I can improve my math knowledge a bit as well!

P.S.: Note the BigNumbersUtils.Sqrt extension method comes from this Stack Overflow answer. I am excluding it since it is not my code. it gives the same result as Math.Sqrt but for BigInteger type.

void Main()
{
    Console.WriteLine("ProjectEuler3: Largest prime factor");
    BigInteger testCase = 600851475143;
    ProjectEuler3 PE3 = new ProjectEuler3(testCase);
    Console.WriteLine("Prime factor of {0} is: {1}", testCase, PE3.GetAnswer());
}

class ProjectEuler3
{
    private BigInteger number;

    public ProjectEuler3(BigInteger number)
    {
        this.number = number;
    }

    public BigInteger GetAnswer()
    {
        // largest possible prime factor of a number is its square root [citation needed]
        BigInteger maxPrimeFactor = BigNumbersUtils.Sqrt(number);
        // make sure number we start from is odd, as even numbers are never going to be prime
        if (maxPrimeFactor % 2 == 0) { maxPrimeFactor += 1; }
        // iterating by 2s to skip even numbers
        for (BigInteger i = maxPrimeFactor; i >= 1; i = i - 2)
        {
            if (IsFactor(i, number) && IsPrime(i))
            {
                return i;
            }
        }
        return 1;
    }

    private bool IsFactor(BigInteger n, BigInteger factorOf)
    {
        return (factorOf % n == 0) ? true : false;
    }

    private bool IsPrime(BigInteger n)
    // Based on Wikipedia page for "Primality test"
    // https://en.wikipedia.org/wiki/Primality_test#Simple_methods
    {
        // short-circuit very common numbers
        if (n <= 1)
        {
            return false;
        }
        else if (n <= 3)
        {
            return true;
        }
        else if (n % 2 == 0 || n % 3 ==0)
        {
            return false;
        }
        else if (  (n != 5 && n % 5 == 0))
        {
            return false;
        }
        // iterate with trial division
        BigInteger i = 5;
        while (i * i <= n)
        {
            if (n % i == 0 || n % (i + 2) == 0)
            {
                return false;
            }
            i++;
        }
        return true;
    }

}

Project Euler questions are primarily about math.  This isn't really the place to practice object-oriented programming.  In fact, trying to model the problem literally will lead you to a suboptimal solution.

The "right" algorithm is very simple, and involves no primality testing.

I wouldn't bother with an IsFactor() method at all — if you just write it as a modulo test, there is no confusion about the order the parameters.

class ProjectEuler3
{
    private long number;

    public ProjectEuler3(long number)
    {
        this.number = number;
    }

    public long GetAnswer()
    {
        long n = this.number;
        while (n % 2 == 0)
        {
            n /= 2;
        }
        for (long factor = 3; factor < n; factor += 2)
        {
            while (n % factor == 0)
            {
                n /= factor;
            }
        }
        return n;
    }

}


Using a final class for string constants I want to use something like Setting.RES_WIDTH in my code without the need for a getter function (i.e.: custom getter, enum's .name(), .toString() or .valueOf()).

Someone suggested using a final class, so I wrote this:

public final class Setting {

    public final static String RES_WIDTH        = "resolution_width";
    public final static String RES_HEIGHT       = "resolution_height";
    public final static String FULLSCREEN       = "fullscreen_enabled";
    public final static String VSYNC            = "vsync_enabled";

    //Prevent someone from creating an instance of this class
    private Setting() {}
}

This code works, but I am wondering if there are any kind of issues (performance, GC, etc) that might arise from using this.

There won't be any GC overhead since there won't be any instantiated instances to collect.

I think there's a small overhead by accessing a class' static field rather than an in-scope variable ; however, I would consider it largely worth the improved readability/maintainability.

I guess you could also argue you'll use up a little bit more of your permGen / Meta space, but I've never heard it be a consequent problem.


Surface Density Profile I am trying to compute the surface density profile, given the spherical density profile in 3D for different parameters in order to interpolate and have it as a function of them to use later on for some gravitational lensing modelling. However, looping through all parameters and evaluating the integral over the 2D Radius array R is extremely time consuming (many hours for complicated profiles).

The code that follows describe the procedure I am following in order to do that. Does anyone have a better suggestion to improve the performance of the following code in Python?

def density_profile(r, par_a, par_b, par_c):
    ...
    return ...

def surface_denstiy_profile(R, density_profile, *args):

    surface_density_array = []

    for R_i in R:

        surface_density_array.append( quad(lambda r : 2.0 * density_profile(r , *args, ) * r / np.sqrt(r**2. - R_i**2.), R_i, np.inf, epsabs=1.49e-03, epsrel=1.49e-03)[0] ) )

    return surface_density_array

surface_density = np.zeros((len(par_a_array), len(par_b_array), len(par_b_array)), dtype = object)

for par_a in range(0, len(par_a_array)) :

    for par_b in range(0, len(par_b_array)):

        for par_c in range(0, len(par_c_array)):

            surface_density[par_a, par_b, par_c] = surface_denstiy_profile(np.logspace(-2., 2., 100), density_profile, par_a_array[par_a], par_b_array[par_b], par_c_array[par_c])

Firstly try and do as few computation as possible - e.g. the
np.logspace call is essentially constant, no reason to call it over
and over for every single element.

Also the dtype of object for surface_density makes using NumPy
somewhat useless for that array, try instead one dimension more and the
right datatype / dimensions of the array to actually save some space /
make things faster.

With NumPy you really want to avoid these loops and try to find some
operations that work on the whole array instead of per-element.  If
there's no way to do that easily, consider some of the apply functions
to at least make it look better and then look into processing parts of
the array in multiple threads / processes to speed things up as it looks
like to be a highly parallel computation.  Also Cython, but it's not clear to me if it would help that much here, even though trying it is not very complicated, so you could just do that and see.

Also avoid slower language features like the variable arguments in often called functions, e.g. here pass a fixed number of arguments instead of doing the *args thing.

Unrelated to NumPy and won't help here, but related to general Python, just in case for your next scripts:

There's also a typo in surface_denstiy_profile, it should be
density.
Don't use the range(..., len(...)) pattern, better would be to have
enumerate since you get both the index and value delivered
directly.


Evaluate the best common type to fit both of types Here is an algorithm to evaluate best matching System.Type for two types in hierarhy. This is one of answers for this StackOverflow question by Ken Kin:

  Evaluate the maximum possible type to fit both of types

Algorithm

Searching for common base class (either concrete or abstract)

FindBaseClassWith method

If there is no common base class, search for common implemented interface

It's possible for one class to implement multiple interfaces, in this case return first common based interface
FindInterfaceWith method

Questions

GetInterfaceHierarchy implementaion

I'm not sure that this is correct implementation in case of complex class hierarhy with many implemented interfaces
Now all tests are passing for selected collection from BCL and my own testing hierarhy

Are there any other flaws that I do not pay attention to?
I had wrote this sample in LinqPad, that's why all unittests are located at Main method. Nevertheless, any improvments to Test.Assert are ok!

Full source code with unittests is available here

// provide common base class or implemented interface
public static Type FindEqualTypeWith(this Type typeLeft, Type typeRight)
{
    if(typeLeft == null || typeRight == null) return null;

    var commonBaseClass = typeLeft.FindBaseClassWith(typeRight) ?? typeof(object);

    return commonBaseClass.Equals(typeof(object))
            ? typeLeft.FindInterfaceWith(typeRight)
            : commonBaseClass;
}

// searching for common base class (either concrete or abstract)
public static Type FindBaseClassWith(this Type typeLeft, Type typeRight)
{
    if(typeLeft == null || typeRight == null) return null;

    return typeLeft
            .GetClassHierarchy()
            .Intersect(typeRight.GetClassHierarchy())
            .FirstOrDefault(type => !type.IsInterface);
}

// searching for common implemented interface
// it's possible for one class to implement multiple interfaces,
// in this case return first common based interface
public static Type FindInterfaceWith(this Type typeLeft, Type typeRight)
{
    if(typeLeft == null || typeRight == null) return null;

    return typeLeft
            .GetInterfaceHierarchy()
            .Intersect(typeRight.GetInterfaceHierarchy())
            .FirstOrDefault();
}

// iterate on interface hierarhy
public static IEnumerable<Type> GetInterfaceHierarchy(this Type type)
{
    if(type.IsInterface) return new [] { type }.AsEnumerable();

    return type
            .GetInterfaces()
            .OrderByDescending(current => current.GetInterfaces().Count())
            .AsEnumerable();
}

// interate on class hierarhy
public static IEnumerable<Type> GetClassHierarchy(this Type type)
{
    if(type == null) yield break;

    Type typeInHierarchy = type;

    do
    {
        yield return typeInHierarchy;
        typeInHierarchy = typeInHierarchy.BaseType;
    }
    while(typeInHierarchy != null && !typeInHierarchy.IsInterface);
}

Your code does not take in account interface covariance and contravariance issues.

Here is the sample code that demonstrates it:

class ObservableOfString : IObservable<string> { /*...*/ }
class ObservableOfObject : IObservable<object> { /*...*/ }
class ObserverOfObject : IObserver<object> { /*...*/ }
class ObserverOfString : IObserver<string> { /*...*/ }
class Program
{

    static void Main(string[] args)
    {
        IObservable<object> observableIsCovariant = new ObservableOfString();
        IObserver<string> observerIsContravariant = new ObserverOfObject();
        Console.WriteLine(typeof(ObservableOfString).FindEqualTypeWith(typeof(ObservableOfObject)) ?? (object)"<NULL>"); // Would it not be more useful to return IObservable<object>?
        Console.WriteLine(typeof(ObserverOfString).FindEqualTypeWith(typeof(ObserverOfObject)) ?? (object)"<NULL>"); // Would it not be more useful to return IObserver<string>?
        Console.ReadLine();
    }
}

The fundamental issue is, as soon as you want to find the "best common interface", you are going to run in an issue, as "best" and "common" are at best dependent on the context. You should clearly identify which use cases you are going to support, in the context of why that method was needed, and write a specific implementation that solves your need.


Finding all possible pairs of square numbers in an array I am writing a program that allows me to find all possible pairs of square numbers including duplicates. We can also assume the array elements to be of positive integers only.
e.g an array of {5,25,3,25,4,2,25} will return [5,25],[5,25],[2,4],[5,25]
since 25 is square of 5.

Currently, I am using a nested for loop to find the squares. I'm just wondering if there is a better way to do this?

import java.lang.Math.*;

public static void main(String args[])
{
    int arr[] = {5,25,3,25,4,2,25};
    String s = "";

    for(int i =0; i < arr.length;i++)
    {

        for(int j = 0;j < arr.length;j++)
        {
            if(Math.sqrt(arr[i]) == arr[j])
            {
                s += arr[j] + "," + arr[i] + " ";
            }
        }

    }

    System.out.println(s);

}

Avoid string addition

String addition is not good for building up strings from many pieces inside of loops.  You should use StringBuilder instead.

StringBuilder sb = new StringBuilder();

// ... omitted ...

        sb.append(arr[j]).append(',').append(arr[j]).append(' ');

// ... omitted ...

String s = sb.toString();
System.out.println(s);

Avoid square-roots

Checking \$\sqrt x == y\$ is ... dangerous.

The results of the square-root are a float-point number, and may not exactly equal your integer value.
If you have a negative number in your list, Math.sqrt() will raise an exception, yet { -5, 25 } is a valid pair.

Testing x == y*y is safer, as long as there is no danger of y*y overflowing.

Avoid repeated calculations

for(int i =0; i < arr.length;i++) {
    for(int j = 0;j < arr.length;j++) {
        if(Math.sqrt(arr[i]) == arr[j]) {
            ...

In the inner-loop, i is constant.  Yet you are computing Math.sqrt(arr[i]) every time through the loop.  The value should not be changing, so you could compute it once, outside of the inner loop.

for(int i =0; i < arr.length;i++) {
    double sqrt_arr_i = Math.sqrt(arr[i]);
    for(int j = 0;j < arr.length;j++) {
        if(sqrt_arr_i == arr[j]) {
            ...

Pairs need distinct indices

If your input contains a single 0 or 1, it will mistakenly report that it has found a pair, since 0 == 0*0 and 1 == 1*1.  You can protect against this by adding i != j && to your test.

If the input contains two 0's (or two 1's), your algorithm will emit 4 pairs: [first,first], [first,second], [second,first], and [second,second].  Adding the i != j guard will eliminate the first and last of those pairs, but it will still declare two pairs: [first,second], [second,first] since first² = second and first = second² would both be true.  You'd have to weight in on whether this would be two distinct pairs or not.

Formatting

Consistent and liberal use of white space is always recommended.  Add white space after every semicolon inside of for(...), on both sides of operators (i = 0 not i =0), and after every comma in {5,25,3,25,4,2,25}.

With the above recommendations, your function body would become:

int arr[] = { 5, 25, 3, 25, 4, 2, 25 };
StringBuilder sb = new StringBuilder();

for(int i = 0; i < arr.length; i++)
{
    for(int j = 0; j < arr.length; j++)
    {
        if(arr[i] == arr[j] * arr[j])
        {
            sb.append(arr[j]).append(',').append(arr[i]).append(" ");
        }
    }
}

String s = sb.toString();
System.out.println(s);

Additional considerations

You have a trailing space in your resulting string.  There are several tricks you can use to remove it.  However, an interesting alternative is to use StringJoiner:

StringJoiner sj = new StringJoiner(" ");

// ... omitted ...

        sj.add(arr[j] + "," + arr[j]);

// ... omitted ...

String s = sj.toString();
System.out.println(s);

When StringJoiner adds the second and successive strings, it automatically adds the delimiter specified in the constructor.


Parsing a url into host and path I have a program which splits a url into host and path:

int main(int argc, char** argv){
  std::vector<std::string> url = parseUrl(argv[1]);

  memset(&hints, 0, sizeof hints);
  hints.ai_family = AF_UNSPEC;
  hints.ai_socktype = SOCK_STREAM;
  int MAXDATASIZE = 1024;
  std::string request = "GET " +url.at(1) + " HTTP/1.1\r\n\r\n";
  const char* charRequest = request.c_str();
  char buf[MAXDATASIZE];
  if((status = getaddrinfo(url.at(0).c_str(), "80", &hints, &servinfo)) != 0){
    std::cout << "getaddrinfo: " << gai_strerror(status) << "\n";
    return 2;
  } else {
    std::cout << "Server struct created successfully\n";
  }
  if((sock = socket(servinfo->ai_family, servinfo->ai_socktype, servinfo->ai_protocol)) == -1){
    std::cout << "Error creating socket\n";
    return 2;
  } else {
    std::cout << "Successfully created socket\n";
  }
  if(connect(sock, servinfo->ai_addr, servinfo->ai_addrlen) == -1){
    std::cout << "Error connecting to host " << url.at(0) << " at port " << "80" << "\n";
    return 2;
  } else {
      std::cout << "Connected successfully to " << url.at(0) << "at port " << "80" << "\n";
  }
  if((send(sock, charRequest, strlen(charRequest), 0)) == -1){
     std::cout << "Error communicating with website\n";
     return 2;
  } else {
     std::cout << "Sent request successfully to " << url.at(0) << " at port " << "80" << "\n";
  }
  if(recv(sock, buf, MAXDATASIZE, 0) == -1){
     std::cout << "Error recciving data from " << url.at(0) << " at port " << "80" << "\n";
  } else {
    std::cout << "Recieved data successfully from " << url.at(0) << " at port " << "80" << "\n";
  }
  std::ofstream file;
  file.open(argv[2]);
  file << buf;
  file.close();
  close(sock);

  freeaddrinfo(servinfo);
}

 // Returns vector with host as first argument and path as second
  std::vector<std::string> parseUrl(std::string url){
    std::vector<std::string> ret;
    size_t dotPos = url.find('.');
    size_t slashPos = url.find('/', dotPos);
    std::string host = url.substr(0, slashPos);
    std::string path = url.substr(slashPos, url.length());
    ret.push_back(host);
    ret.push_back(path);
    return ret;
  }

Does anyone have suggestions for improvement. Especially about the clunky vector return?

Rather than using a std::vector and using the indices to specify the host and path, we can create a little struct and refer to the members by name:
struct URL
{
  std::string host;
  std::string path;
};

So we can refer to url.host instead of url.at(0).

As pointed out in the comments, the parsing method could be more accurate and comprehensive. It's fine to only support a small subset of the url syntax, but we should at least check that we got what we expected in the string.
In this case, I think we should:

Throw an error if we don't find a dot.
Either throw an error if we don't find a slash, OR set the path part of the url to "/" to ensure that our GET request is valid.

int MAXDATASIZE = 1024;
...
char buf[MAXDATASIZE];

This doesn't compile for me. MAXDATASIZE must be a constant.
We can remove the need for a separate constant by using std::array:
std::array<char, 1024> buf;
// now we can use buf.size() :)

Even for something small like this, it's probably worth writing our own socket class. This provides some cleanup safety (RAII), as well as making the code in main() a lot simpler and easier to read by removing some of details of the network code. e.g. something like the following:
struct socket
{
public:

    socket();
    explicit socket(struct addrinfo const& address);

    socket(socket const&) = delete;
    socket& operator=(socket const&) = delete;

    socket(socket&& other);
    socket& operator=(socket&& other);

    ~socket();

    std::size_t send(char const* data, std::size_t data_size);
    std::size_t receive(char* buffer, std::size_t buffer_size);

    bool is_open() const;
    void close();

private:

    SOCKET m_handle;
};

We could also move the address lookup out of main() into a separate function, and keep the addrinfo in a unique pointer with a custom deleter that calls freeaddrinfo(). Something like:
std::unique_ptr<addrinfo, std::function<void(addrinfo*)>> lookup_address(std::string const& node, std::string const& service, int ai_family, int ai_socktype, int ai_protocol, int ai_flags = 0)
{
  auto ai_hints = addrinfo();
  // ... set up hints!

  auto output = (addrinfo*) nullptr;
  auto result = getaddrinfo(node.c_str(), service.c_str(), &ai_hints, &output);
  // ... check errors!

  return { output, [] (addrinfo* ai) { freeaddrinfo(ai); } };
}

So we might end up a main like:
int main(int argc, char** argv)
{
  if (argc != 3)
    throw std::runtime_error("Unexpected argument count!");

  auto url = parse_url(argv[1]);
  auto address = lookup_address(url.host, "80", AF_UNSPEC, SOCK_STREAM, 0, 0);

  if (!address)
    throw std::runtime_error("Failed address lookup!");

  auto s = socket(*address);

  if (!s.is_open())
    throw std::runtime_error("Failed to connect!");

  auto request = "GET " + url.path + " HTTP/1.1\r\n\r\n";
  auto sent = s.send(request.c_str(), request.size() + 1);

  if (sent != request.size() + 1)
    throw std::runtime_error("Failed to send request!");

  auto buffer = std::array<char, 1024>();
  auto received = s.receive(buffer.data(), buffer.size());

  if (received == 0)
    throw std::runtime_error("No data received!");

  std::ofstream(argv[2]).write(buffer.data(), received);
}

(code not compiled or tested).


Getting contact information How can I improve this code?

public class Address {
   public string house_no { get; set; }
   public string street { get; set; }
   public string zip { get; set; }
}
public class Contact {
   public string email { get; set; }
   public string ph_no { get; set; }
   public Address address { get; set; }
}

//Test program
class Program {
    static void GetContacts(string input)   {
        JavaScriptSerializer serializer = new JavaScriptSerializer();
        Dictionary<string, Contact> ContactList = serializer.Deserialize<Dictionary<string, Contact>>(input);
    }

    static void Main(string[] args)     {
        //jsonText is a string that I retrieve from a web service.
        string jsonText = ws.GetContactList();
        GetContacts(jsonText);
    }
}

It is already short and to the point. One minor formatting change; I would make your class properties upper camel case, which is typical for C#. I also dropped the abbreviations.

public class Address {
   public string HouseNumber { get; set; }
   public string Street { get; set; }
   public string Zip { get; set; }
}

public class Contact {
   public string Email { get; set; }
   public string PhoneNumber { get; set; }
   public Address Address { get; set; }
}


Obtain values from SQL I have a simple app which is checking occurrences of values. If there is already occurrences which I am looking for, it display that. If not, it adds.

create table Strings
(
    StringID INT IDENTITY(1,1) NOT NULL PRIMARY KEY
    , StringValue VARCHAR(100) NOT NULL
)

create table Links
(
    LinkID INT IDENTITY(1,1) NOT NULL PRIMARY KEY
    ,Link VARCHAR(255) NOT NULL
)

CREATE TABLE Occurences
(
    OccurenceID INT IDENTITY(1,1) NOT NULL PRIMARY KEY
    ,StringID INT NOT NULL
    ,LinkID INT NOT NULL
)

Class:

private SqlConnection conn;

public void Open()
{
    this.conn = new SqlConnection();
    this.conn.ConnectionString = @"Server=PC\SQLEXPRESS;Database=example;User=sa;Pwd=example;";
    this.conn.Open();
}

public void Close()
{
    this.conn.Close();
}

public string Obtain(string code)
{
    SqlConnection sqlConnection = new SqlConnection();
    sqlConnection.ConnectionString = @"Server=PC\SQLEXPRESS;Database=example;User=sa;Pwd=example;";
    SqlCommand sqlCommand = new SqlCommand();
    sqlCommand.Connection = sqlConnection;
    sqlCommand.CommandText = code;
    sqlConnection.Open();
    SqlDataReader sqlDataReader = sqlCommand.ExecuteReader();
    sqlDataReader.Read();
    string result = sqlDataReader.GetValue(0).ToString();
    sqlConnection.Close();
    return result;
}

public void Insert(string StringID, string LinkID)
{
    SqlConnection sqlConnection = new SqlConnection();
    sqlConnection.ConnectionString = @"Server=PC\SQLEXPRESS;Database=example;User=sa;Pwd=example;";
    SqlCommand sqlCommand = new SqlCommand();
    sqlCommand.Connection = sqlConnection;
    sqlCommand.CommandText = string.Concat(new string[]
    {
        "INSERT INTO Occurences VALUES ('",
        StringID,
        "', '",
        LinkID,
        "')"
    });
    sqlConnection.Open();
    sqlCommand.ExecuteNonQuery();
    sqlConnection.Close();
}

Button click:

private void button1_Click(object sender, EventArgs e)
{

    try
    {
        sql.Open();
        StringID = sql.Obtain("SELECT StringID from dbo.Strings where StringValue='" + textBox1.Text + "'");
        LinkID = sql.Obtain("SELECT LinkID from dbo.Links where Link='" + textBox2.Text + "'");
        Occurence = sql.Obtain("SELECT OccurenceID from dbo.Occurences where StringID='" + StringID + "' and LinkID = '" + LinkID + "'");
        if (Convert.ToInt32(Occurence) > 0)
        {
            MessageBox.Show(Occurence);
        }
    }

    catch
    {
        sql.Insert(StringID, LinkID);
    }

    finally
    {
        sql.Close();

    }
}

Is this coded well? Should I change my catch to more specific? Should I do something like these?

If there is no StringValue in dbo.Strings then insert StringValue
If there is no Link in dbo.Links then insert Link

Usually when we do something similar we use a repository (pattern). It allows us to encapsulate the entire interaction with the database so that we don't have to think about the queries again and again.

For example:

class MyDatabase
{
    public MyDatabase(string connectionString = null)
    {
        // use the specified or the default connection string
        ConnectionString = connectionString ?? @"Server=PC\SQLEXPRESS;Database=example;User=sa;Pwd=example;";
    }

    public string ConnectionString { get; private set; }

    public int GetStringID(string stringValue)
    {
        // here comes the select and connection manintenance
    }
}

When working with database objects you should always use the using statement to free the resources. Always check the documentaiton if something implements the IDisposable interface.

Your actual code should look like this:

using(var sqlConnection = new SqlConnection(ConnectionString))
{
    // work with the connection where
}

There are few more types that are disposable too (i.e. the SqlCommand or the SqlDataReader).

With the repository your button1_Click could look like this:

private void button1_Click(object sender, EventArgs e)
{
    var db = new MyDatabase();
    StringID = db.GetStringID(textBox1.Text);
}

Naming

Even when working with the simplest projects you should always use meaningful names and button1 or textBox1 are really bad. I don't know what the button1 does but the text-box could be named stringValueTextBox or the textBox2 should be called linkTextBox etc.


Batch convert txt files to xls In this SO question  I provided two answers for the batch conversion of more than 100K txt to xlsx files

I suspect any automation of Excel is too slow, but would appreciate if there is a superior (i.e. faster) approach using powershell.

$files = Get-ChildItem C:\Temp\*.txt
Write "Loading Files..."

$Excel = New-Object -ComObject Excel.Application
$Excel.visible = $false
$Excel.DisplayAlerts = $false

ForEach ($file in $files)
{

 $WorkBook = $Excel.Workbooks.Open($file.Fullname)
 $NewFilepath = $file.Fullname -replace ".{4}$"
 $NewFilepath =  $NewFilepath + ".xls"
 $Workbook.SaveAs($NewFilepath,56)

}
  Stop-Process -processname EXCEL
  $Excel.Quit()

As far as performance goes there are two general approaches to address this
Parallel processing
Running the com object is dragging in itself. Processing 10's of 1000's of files with one Excel instance will be draining. PowerShell does support multiple avenues of mitigating this

https://stackoverflow.com/questions/4016451/can-powershell-run-commands-in-parallel

https://stackoverflow.com/questions/8781666/run-n-parallel-jobs-in-powershell

about_foreach_parallel

I am going to show an approach using Jobs. What this will do is group up identified files. Each group will be past to its own Excel job for processing. Some comments in the code below explain it more.
# Root directory containing your files.
$path = "E:\temp\csv"

# Get Current EXCEL Process ID's so they are not affected but the scripts cleanup
# SilentlyContinue in case there are no active Excels
$currentExcelProcessIDs = (Get-Process excel -ErrorAction SilentlyContinue).Id

# Collect the files
$files = Get-ChildItem -Path $path -Filter "*.txt" -File

# Split the files up into processing groups. For Each group and excel process will be started.
$numberOfGroups = 5
$maxGroupMemberSize = [math]::Ceiling($files.Count / $numberOfGroups)

# Create as many file groups
$fileGroups = 0..($numberOfGroups - 1) | Foreach-object{
    $groupIndexStart = $maxGroupMemberSize * $_
    # Use the unary comma operator to be sure an array is returned and not unrolled
    ,$files[$groupIndexStart..($groupIndexStart + $maxGroupMemberSize - 1)]
}

# Create a job for each file group.
for($jobCount = 0; $jobCount -lt $fileGroups.Count; $jobCount++){
    # Start a unique Excel instance for this group of files.
    Start-Job -Name "Excel$jobCount" -ScriptBlock {
        param($files)
        $excelFileFormat = 56 #xlExcel8 format

        # Create a new Excel Instance
        $excel = New-Object -ComObject Excel.Application
        $excel.Visible = $false
        $excel.DisplayAlerts = $false

        ForEach ($file in $files){
            $workbook = $Excel.Workbooks.Open($file.Fullname)
            $newFilepath = $file.Fullname -replace "\..*$",".xls"
            $workbook.SaveAs($newFilepath, $excelFileFormat)
        }

        # Quit this instance and return its memory
        $excel.Quit()
        while([System.Runtime.Interopservices.Marshal]::ReleaseComObject($workbook)){}
        while([System.Runtime.Interopservices.Marshal]::ReleaseComObject($excel)){}
        Remove-Variable "workbook","excel"

    } -ArgumentList (,($fileGroups[$jobCount])) | Out-Null
}

# Wait for the jobs to be completed and remove them from inventory since they won't have output we need
Get-Job -Name "Excel*" | Wait-Job | Receive-Job

# Remove any stale Excel processes created by this scripts execution
Get-Process excel -ErrorAction SilentlyContinue | Where-Object{$currentExcelProcessIDs -notcontains $_.id} | Stop-Process

Excel automation is full of pitfalls but the general approach seems to be working. For one com objects can prevent from being closed. Best thing to do is just kill any remaining processes. There is logic that can help to be sure any Excel opened before the script was run are not affected. Jobs are done in the way as well to not affect processing of other scripts.
This all would be better as a script using proper parameters but showing the logic of jobs is the point here.
Automate without Excel
There are a number of libraries out there that can create Excel documents without the need for COM objects or Excel itself. These would perform faster as well. EPPlus comes to mind and even [ImportExcel module] (which uses EPPlus) would be solid choices as well. I don't know what your text files look like so you will need to experiment.
Get-Content E:\temp\csv\data.txt | Export-Excel -Path "e:\temp\csv\file.xlsx"

Export-Excel works better with objects so, depending on your data structure, you would do this instead.
Import-Csv E:\temp\csv\data.txt | Export-Excel -Path "e:\temp\csv\file.xlsx"

Import-CSV is generally a slow process so you would want to try using streamreader and Convert-FromCSV if performance suffers.
Magic Numbers
Your 56 would be an example of that. You have to look it up to know what it represents. So 56 is for Excel Version 8. At a minimum that should be its own variable. My code above uses
$excelFileFormat = 56 #xlExcel8 format

If you wanted to go a little crazier you could actually import the assembly to access the enum
[reflection.assembly]::LoadWithPartialName("Microsoft.Office.InterOp.Excel") | Out-Null
[Microsoft.Office.Interop.Excel.XlFileFormat]::xlExcel8

BaseName

 $NewFilepath = $file.Fullname -replace ".{4}$"
 $NewFilepath =  $NewFilepath + ".xls"

This is what you are doing to replace the extension of the file. File objects have a basename property that is the file name without the extension. I know that you are dealing with txt files, so you already know how many characters on the end to remove, but using .basename would remove that complexity in other areas as well.
$NewFilepath = "$($file.BaseName).xls"


Multi threading with CachedThreadPool I have server-client communication architecture where there is one server and 150 clients.

The server-client communication happens via Java NIO where all the clients send some or the other data every 10 seconds.
Previously we used to queue all the process messages and process all those in a single thread, as the number of clients are more so as the messages, server is not able to process all the messages instantly and there is a delay in processing in turn data loss.
So I have thought of implementing CachecThreadPool to process the tasks simultaneously as soon as they come, picked CachedThreadPool over FixedThreadPool because the tasks are short lived and many in number, below is the code for that.
The thread which receives messages from client calls ProcessorClass.processData(message) as soon as it receives the message.

public class ProcessorClass{

    private static final Logger LOGGER = Logger.getLogger(ProcessorClass.class);
    static ThreadPoolExecutor executor = (ThreadPoolExecutor) Executors.newCachedThreadPool();

    public static void processData(StringBuffer message) {
        Runnable task = new Runnable() {
            @Override
            public void run() {
                try {
                    LOGGER.info("Queue size:"+executor.getQueue().size());
                    if (message != null){
                        processMessage(message);
                    }
                }
                catch(Exception e) {
                    LOGGER.error("Error happened in run() method" + e.getMessage());
                }

            }
        };
        executor.execute(task);
    }
    public static void processMessage(StringBuffer message){
        // all the processing of message such as DB operations goes here.
    }
}

Doubts:

How CachedThreadPool stores the message in the queue because i haven't defined any explicitly.
Should i chose FixedThreadPool over this?
Should i make my processMessage() method synchronized?

All the suggestions and review comments are welcome.

The thread pool is not storing messages on a queue; it is storing Runnable tasks.  However, each Runnable task has access to the effectively final StringBuffer message variable from the calling context.
See this StackOverflow question.  Each is a ThreadPoolExecutor, just with different parameters to control how many threads are created and the type of queue.  I would lean in favour of the FixedThreadPool with 1 thread for each CPU core, but many other factors can and will impact that; profile to be sure.
No.  Making processMessage() synchronized will mean you are single threaded ... worse, because the overhead of multiple threads being scheduled to process the task queue in a single threaded fashion.   At 150 messages/10 seconds, with one thread the function would need to complete in 66ms.  If you have 8 cores, with 1 thread per core, each thread could take a leisurely 500ms to process the message, as long as they can run independently.  You may have parts of processMessage that you may need to make synchronized, but you’ll want to make those regions as small and fast as possible.

Other comments:

A StringBuffer is a mutable object;  since message is being passed to another thread, for safety, I’d want the message to be transformed into a immutable String so the caller doesn’t clear the message buffer while the executor thread is processing it.

processMessage() does not need to be public; private would be more appropriate.

It looks like executor should be private and final as well.


Filter for duplicate elements I wrote code to filter for duplicate elements in an array. I'd like to know your opinion, whether it's efficient at all, or if there's too many objects created, use of both array and list. That's in preparation for an interview. I'm not interested in lambdas (distinct) or helper methods, I'd like to see where I can improve and understand what's efficient or not in what I wrote, also without using Set, HashSet.
public class Duplicate {
    public static void main (String [] args){
        // filter duplicate elements
        int [] arr = {5,4,3,5,4,6,7,8,6};
        Arrays.sort(arr);
        int length = arr.length;
        List<Integer> list = new ArrayList<>();
        list.add(arr[0]);
        for(int i =0; i <length-1;i++){
            if(arr[i] != arr[i+1]){
                list.add(arr[i+1]);
            }
        }
        System.out.println(Arrays.toString(list.toArray()));
    }
}

You can add the items to a Set instead of a List to avoid duplicates. That way you delegate to Set for uniqueness:
int [] arr = {5, 4, 3, 5, 4, 6, 7, 8, 6};

Set<Integer> set = new HashSet<>();

int length = arr.length;

for(int i = 0; i < length; i++){
    set.add(arr[i]);
}

System.out.println(Arrays.toString(set.toArray()));


Find the maximum price change over any 1 to 5 day rolling window The code below solves the following task

Find the maximum price change over any 1 to 5 day rolling window, over a 1000 day period.

To be clear "any 1 to 5 day rolling window" means t[i:i + j] where j can range from 1 to 5. Rather than t[i:i + 5] if it were just "any 5 day rolling window".
I used NumPy native functions to do this. But I've used a for-loop for the inner iteration.
import numpy as np
import numpy.random as npr

prices = npr.random([1000,1])*1000

max_array = np.zeros([(prices.size-5),1])
for index, elem in np.ndenumerate(prices[:-5,:]):
    local_max = 0.0
    for i in range(1,6,1):
        price_return = prices[(index[0] + i),0] / elem
        local_max = max(local_max, price_return)
    max_array[index[0]] = local_max
global_max = np.amax(max_array)

Can I somehow eliminate the inner for loop and use NumPy vectorization instead?
I also don't particularly like using "index[0]" to extract the actual index of the current loop from the tuple object that is returned into the variable "index" via the call:

for index, elem in np.ndenumerate(prices[:-5,:]):

A first pass will see (as Vishesh suggests) collapsing one dimension, and properly unpacking the index tuple:
import numpy as np

# Window size
W = 5

prices = np.arange(1, 10)  # npr.random([1000,1])*1000

max_array = np.zeros(prices.size-W)
for (index,), elem in np.ndenumerate(prices[:-W]):
    local_max = 0
    for i in range(1, W+1):
        price_return = prices[index + i] / elem
        local_max = max(local_max, price_return)
    max_array[index] = local_max
global_max = np.amax(max_array)

I've also replaced your random array with a simple one so that this is repeatable and verifiable. You should make your window size parametric or at least configurable via constant.
Unfortunately, none of the native Numpy options are great. Your best bet is to use Pandas:
import numpy as np
import pandas as pd

# Window size
W = 5

rolled = (
    pd.Series(prices)
    .rolling(window=W)
    .min()
    .to_numpy()
)
max_array = prices[1-W:] / rolled[W-1:-1]

I have tested this to be equivalent. Note that you need to start with a rolling minimum in the denominator to get a maximum numerator.


Checking for a series of uniform non-zero digits def standvastig(reeks):
    """
    Checks if the given input (list or number) is a series of one number (eg. 444444, 2222, etc.)
    Remark: raise an error if the input is a series of 0s or non-integers

    >>> standvastig(44444444)
    True
    >>> standvastig('44544444')
    False
    """
    if str(reeks).isdigit() == False or int(str(reeks)) == 0:
        raise AssertionError('ongeldig serienummer')# gives an error when the input is not a list of integers or 0s

    if any([str(reeks) == '1'*len(str(reeks)), str(reeks) == '2'*len(str(reeks)), str(reeks) == '3'*len(str(reeks)), str(reeks) == '4'*len(str(reeks)), str(reeks) == '5'*len(str(reeks)), str(reeks) == '6'*len(str(reeks)), str(reeks) == '7'*len(str(reeks)), str(reeks) == '8'*len(str(reeks)), str(reeks) == '9'*len(str(reeks))]):
        return(True)
    else: return(False)

if __name__ == "__main__":
    import doctest
    doctest.testmod()

Can this code be written more pythonic? Mainly the if condition.

Here are a couple of suggestions for you to implement:

You convert reeks into a string far more frequently than is necessary; just do it once, at the top of the function.
The correct exception to raise if a value is not acceptable is ValueError.
Rather than thing == False, you can use the more readable not thing.
Your approach to the solution seems strange to me; rather than "are all of the digits one, or are all of the digits two, or are all of the digits...", why not "are all of the digits the same as the first digit"?
Python has a style guide, which discourages e.g. redundant parentheses and multi-statement lines:

else:
    return False


Stripping Quotations from a List of Lists I have written a short function as part of my program that strips the quotation marks (") from a list of lists. An example of the input data is:
[['"foo"', '"Spam"', '123', '"Angry Badger"'],
 ['"dead"', '"Parrot"', '321', '987']]

The target result would be:
[['foo', 'Spam', '123', 'Angry Badger'],
 ['dead', 'Parrot', '321', '987']]

This code I am using to parse this is:
def quote_strip(input_file):

    for k1, v1 in enumerate(input_file):
        for k2, v2 in enumerate(v1):
            input_file[k1][k2] = v2.lstrip('"').rstrip('"')

    return input_file

This works fine, but I am pretty sure there is a way of making this a single line. I just always seem to mess up comprehensions when they have the = sign in them like this.
Any feedback would be greatly appreciated.

I just always seem to mess up comprehensions when they have the = sign in them like this.

I found the best way to think about comprehensions is instead think of comprehensions as .appends.
Lets change your inner loop to use .append instead.
We don't need to use enumerate as we'll be building a new list.
def quote_strip(input_file):
    for k1, v1 in enumerate(input_file):
        inner_list = []
        for v2 in v1:
            inner_list.append(v2.lstrip('"').rstrip('"'))
        input_file[k1] = inner_list
    return input_file

We can now change the code to a comprehension.
I'll show you a pattern which we can just copy:
my_list = []
for item in other_list:
    my_list.append(item + 1)

becomes:
my_list = [
    item + 1
    for item in other_list
]

We just move whatever is in the append(...) to the top of the comprehension.
And move the for loop into the bottom of the list.
def quote_strip(input_file):
    for k1, v1 in enumerate(input_file):
        inner_list = [
            v2.lstrip('"').rstrip('"')
            for v2 in v1
        ]
        input_file[k1] = inner_list
    return input_file

You should notice we can remove the need for assigning to the name inner_list.
def quote_strip(input_file):
    for k1, v1 in enumerate(input_file):
        input_file[k1] = [
            v2.lstrip('"').rstrip('"')
            for v2 in v1
        ]
    return input_file

Now we can change the loop to use .append again.
def quote_strip(input_file):
    output_list = []
    for v1 in input_file:
        output_list.append([
            v2.lstrip('"').rstrip('"')
            for v2 in v1
        ])
    return output_list

And again we can change the code to a list comprehension.
def quote_strip(input_file):
    return [
        [
            v2.lstrip('"').rstrip('"')
            for v2 in v1
        ]
        for v1 in input_file
    ]

Extra

I am pretty sure there is a way of making this a single line

We can put the comprehension on one line but please do not do so.
How many lines of code you write is not important, how easy to read your code is.
Putting the comprehension on one line will make your code harder to read.


Using parser_combinators to parse a string I am playing with Rust, and am trying to write a simple parser.

I need to parse the string:

"0123,456789"

into a stucture:

Passport { label : i8 = 0, body : i32 = 123456789 }

I am using parser_combinators and my code is working, but very ugly.

How can I rewrite this code?

extern crate parser_combinators as pc;
 use pc::*;
 use pc::primitives::{State, Stream};

 fn main() {
     match parser(parse_passport).parse("a123,456") {
         Ok((r,l)) => println!("{} {} {}", r.label, r.body, l),
         Err(e) => println!("{}", e)
     }
 }

 struct Passport {
     label : i8,
     body  : i32,
 }

 fn parse_passport<I>(input: State<I>) -> ParseResult<Passport, I> where I: Stream<Item=char> {
     let mut label = digit().map(|i : char| (i as i8) - ('0' as i8));
     let mut fst = many1(digit()).map(|string : String| string.parse::<i32>().unwrap());
     let (i,input) = match label.parse_state(input) {
         Ok((x,n)) => (x,n.into_inner()),
         Err(e) => return Err(e)
     };
     let (f,input) = match fst.parse_state(input) {
         Ok((x,n)) => (x,n.into_inner()),
         Err(e) => return Err(e)
     };
     let (_,input) = match satisfy(|c| c == ',').parse_state(input) {
         Ok((x,n)) => (x,n.into_inner()),
         Err(e) => return Err(e)
     };
     let (s,input) = match fst.parse_state(input) {
         Ok((x,n)) => (x,n),
         Err(e) => return Err(e)
     };
     let p = Passport { label : i, body : f * 1000000 + s };
     Ok((p, input))
 }

Some small style nits to fix before we dive into the meat. When specifying a variable and a type, there should be space after the :, but not before:

// label : i8,
label: i8,

I break where clauses onto the next line, with each type restriction on its own line as well:

// fn parse_passport<I>(input: State<I>) -> ParseResult<Passport, I> where I: Stream<Item=char> {
fn parse_passport<I>(input: State<I>) -> ParseResult<Passport, I>
    where I: Stream<Item=char>
{

Let the compiler use type inference as much as possible. You don't always need to specify the type of a closure variable, for instance:

// let mut label = digit().map(|i : char| (i as i8) - ('0' as i8));
let mut label = digit().map(|i| (i as i8) - ('0' as i8));

Using try! is going to be your biggest win for clarity:

// let (i,input) = match label.parse_state(input) {
//     Ok((x,n)) => (x,n.into_inner()),
//     Err(e) => return Err(e)
// };
let (i, input) = try!(label.parse_state(input));
let input = input.into_inner();

That's as far as I got with generic Rust knowledge. After reading the docs a bit, I came up with this though (note: this is my first time using this library, so I make no claims to use it well!):

fn parse_passport<I>(input: State<I>) -> ParseResult<Passport, I>
    where I: Stream<Item=char>
{
    let label = digit().map(|i| (i as i8) - ('0' as i8));
    fn to_number(string: String) -> i32 { string.parse().unwrap() }
    let fst = many1(digit());
    let comma = satisfy(|c| c == ',');

    let mut parser = label.and(fst.clone().map(to_number)).and(comma).and(fst.map(to_number));

    let ((((i, f), _), s), input) = try!(parser.parse_state(input));

    let p = Passport { label: i, body: f * 1000000 + s };

    Ok((p, input))
}

I'm certain there's a better way though. The nested tuples are a bit off-putting. Also, I feel like there should be a nice way to map over the result of the parsing so we didn't have to unpack and repack the Result, including the input. There's the straight forward but awkward:

parser.parse_state(input).map(|((((i, f), _), s), input)| {
    (Passport { label: i, body: f * 1000000 + s }, input)
})

This is the final thing I ended up at:

extern crate parser_combinators as pc;
use pc::*;
use pc::primitives::{State, Stream};

fn main() {
    match parser(parse_passport).parse("0123,456789") {
        Ok((r, l)) => println!("{} {} {}", r.label, r.body, l),
        Err(e)     => println!("{}", e)
    }
}

struct Passport {
    label: i8,
    body: i32,
}

fn parse_passport<I>(input: State<I>) -> ParseResult<Passport, I>
    where I: Stream<Item=char>
{
    let label = digit().map(|i| (i as i8) - ('0' as i8));
    fn to_number(string: String) -> i32 { string.parse().unwrap() }
    let fst = many1(digit());
    let comma = satisfy(|c| c == ',');

    let mut parser = label.and(fst.clone().map(to_number)).and(comma).and(fst.map(to_number));

    parser.parse_state(input).map(|((((i, f), _), s), input)| {
        (Passport { label: i, body: f * 1000000 + s }, input)
    })
}


Exotic bitmap renderer for core drawing functions I am very interested in inventing the best way possible for rendering bitmaps on-screen using the base drawing functions (putpixel(x, y), lineto(x, y), moveto(x, y), setpen(r, g, b, transp, size) )

You don't need to use graphic library to have them. You can call BIOS interrupt or initialize registers manually to set the video mode to 13h (for example) for 320x200 canvas field and 256-color mode and write from offset segment A000h

I know there are ways to make the rendering faster.

If loop iterations are reduced
If loop body computations are reduced
If the function is smart enough to figure out where to use lineto or larger pen

  C89, no std libs, implementation of putpixel/setpen functions (from
  graphics.h, sdl, cairo etc)

#define OFFSET_OF_ID        (0x0)
#define OFFSET_OF_SIZE      (0x2)
#define OFFSET_OF_PIXELS    (0xA)
#define OFFSET_OF_NDIB      (0xE)
#define OFFSET_OF_WIDTH     (0x12)
#define OFFSET_OF_HEIGHT    (0x16)
#define OFFSET_OF_BPP       (0x1C)
#define OFFSET_OF_NRAW      (0x22)

#define loop(x, y, z) int x; for(; y; z)

typedef const volatile unsigned long data, *pdata;
typedef unsigned char pixel [0x3], (*byte_buffer);

void bmp_render (byte_buffer image)
{
    data image_width            = *(pdata)&image[OFFSET_OF_WIDTH];
    data image_height           = *(pdata)&image[OFFSET_OF_HEIGHT];
    data image_offset_pixels    = *(pdata)&image[OFFSET_OF_PIXELS];
    data image_raw_size         = *(pdata)&image[OFFSET_OF_NRAW];
    data image_row              = image_width * sizeof(pixel);

    unsigned h = 0, w = 0, pad = 0, padding = image_width % 4;

    loop(i = image_offset_pixels, i < image_raw_size, i += sizeof(pixel))
    {
        if(i == image_offset_pixels + (image_row * ((h + 1)) + (pad * h)))
        {
            pad = padding;
            i += pad;
            h += 1;
            w = !w;
        }

        setpen(image[i + 2], image[i + 1], image[i], 0, 1); // setpen(r, g, b, transp, size)
        putpixel(w++, image_height - h); // putpixel(x, y) of the canvas
    }
}

If loop iterations are reduced

Loop iterations can be only lowered in occasions in which you use lineto or/and larger than 1 pixel pen size.

  If loop body computations are reduced

The padding computation can be omitted from each iteration. Videlicet the modulo % operator slows down tremendously. Nevertheless with your current approach of raster scan I believe you are combining minimum resources during the loop.

  If the function is smart enough to figure out where to use lineto or
  larger pen

Relates to the first idea for speculative optimization. Idea, that can be achieved along the second idea for optimization. And that is to give RLE-compressed (figuratively) image instead of raw image, which compression determines where to use line, where to use larger pen size or a combination of these two.

I've been through similar cases where optimizing is precious and with that consideration and in this particular circumstance I am only capable of confirming these "theories" as well as share your opinion about how useful that could be. Quite frequently people's first game projects are based on drawing from scratch.. and they have a hard time rendering images with the desired quality.


Flag words that would be difficult for an early reader This is part of a project I made a couple of years ago and was looking at again. Its purpose is to check text for words that an early reader (about a late kindergarten or first grade level) would likely be unable to read. It does this by checking for CVC (consonant-vowel-consonant) and CVCE (consonant-vowel-consonant-E) words as well as slight variations on the CVC that begin or end with common two-letter combinations. (For example, an early reader should have little trouble with a word like "clap."). It also checks for  words on the pre-primer Dolch lists. (These are common "sight words" that kids should immediately recognize.)

Is there any way to make this more efficient? In particular, as I look at it, there are an awful lot of "if" statements. Is there some way to cut down on those, or does it even matter?

Also, do you think this is readable as is, or is there anything I should do to format it better?

Here is the "core" of the code:

    # Check if a word could be CVC or CVCE

def check_pattern(text):
    while len(text) >= 3:
        l= [text[0], text[1], text[2]]
        if l[0] in vowels:
            return False
        elif l[1] not in vowels:
            return False
        elif l[2] in vowels:
            return False
        else:
            return True

# Check if a word is CVC.
def check_CVC(text):
    pattern = check_pattern(text)
    if pattern == False:
        return False
    elif len(text) != 3:
        return False
    else:
        return True

# Check if a word is CVCE.
def check_CVCE(text):
    pattern = check_pattern(text)
    if pattern == False:
        return False
    elif len(text) != 4:
        return False
    elif text[3] != "e":
        return False
    else:
        return True

#Check if a word begins with a consonant blend or digraph. If so, read as if one letter
def check_bb (text):
    if len(text) >= 3:
        st= text[0]+text[1]
        if st in IC:
            p = list(text)
            del p[1]
            text = "".join(p)
            return text
        else:
            return text
    else:
            return text

#Check if a word ends with a consonant blend, digraph, or double letter. If so, read as if one letter
def check_eb (text):
    if len(text) >= 3:
        end = text[-2]+text[-1]
        if end  in FC:
            p = list(text)
            del p[-1]
            text = "".join(p)
            return text
        else:
            return text
    else:
            return text
# Strip off any punctuation at the end of a word.
def no_punct(text):
    punct = ["!", ".", "?", ",", ";", ":"]
    if text[-1] in punct:
        p = list(text)
        del p[-1]
        text = "".join(p)
        return text
    else:
        return text
#Check if a word is preprimer, primer, or easily decodable
def check_readable(text):
    word = no_punct(text)
    lc = word.lower()
    merge_start = check_bb(lc)
    merge_end = check_eb(merge_start)
    CVC = check_CVC(merge_end)
    CVCE = check_CVCE(merge_end)

    if CVC == True:
        return True
    elif CVCE == True:
        return True
    elif lc.lower() in preprimer:
        return True
    elif lc.lower() in primer:
        return True
    else:
        return False
#Flag words in a text that are not easily decodable, preprimer, or primer
def flag(text):
    words = text.split()
    i = 0
    while i < len(words):
        r = check_readable(words[i])
        if r == False:
            words[i] = "*" + words[i] + "*"
            i += 1
        else:
            i += 1
    text = " ".join(words)
    return text

For reference, here is the link to the entire code, including the letter/blend/word lists and a Tkinter GUI: https://github.com/casinclair/Reader

The code you presented is not readable and definitely needs refactoring.

You can simplify several code constructions and apply the "Extract Variable" refactoring method to improve readability of the program:

  You have a complicated expression.
  Put the result of the expression, or parts of the expression, in a
  temporary variable with a name that explains the purpose.

For instance, check_pattern() may use the variable unpacking and condition merging, resulting into an explicit and understandable code. I've also renamed it to is_cvc:

def is_cvc(text):
    """Checks if a word is CVC: consonant-vowel-consonant."""
    has_3_characters = len(text) == 3
    if not has_3_characters:
        return False

    first, second, third = text[:3]
    first_is_consonant = first not in vowels
    second_is_vowel = second in vowels
    third_is_consonant = third not in vowels

    return first_is_consonant and second_is_vowel and third_is_consonant

Then, we can reuse this for the cvce check as well:

def is_cvce(text):
    """Checks if a word is CVCE: consonant-vowel-consonant-E."""
    has_4_characters = len(text) == 4
    if not has_4_characters:
        return False

    ends_with_e = text[-1].lower() == 'e'
    is_also_cvc = is_cvc(text[:3])

    return ends_with_e and is_also_cvc

Also note that I've moved the function comments into proper docstrings.

Note that I've also applied .lower() to the last character to handle both e and E.

  In particular, as I look at it, there are an awful lot of "if"
  statements. Is there some way to cut down on those, or does it even
  matter?

Yes, it matters a lot.

The code is much more often read than written. The more if statements and branches you have, the more complicated, less readable and more difficult to maintain it is. There are some code complexity measurement metrics like "Cyclomatic code complexity" or "Maintainability Index" that may give a rough idea of how readable and maintainable the code is. It is also important to note that a more complex code tend to have more bugs in it:

  Some studies find a positive correlation between cyclomatic
  complexity and defects: functions and methods that have the highest
  complexity tend to also contain the most defects.

Also, since you are dealing with natural language here and the input comes in a form of a text, see if switching to nltk for word tokenization would be easier and more robust (especially, in terms of handling punctuation).


Avoid existSync function on Node.js - Express API controller I have many json files that I want to serve through an API. The json files look something like this:

...
170102.json
170103.json
170104.json
170108.json
170109.json
170120.json
...

The name of each file is the date in the format 'yymmdd.json'. There are a lot of files missing, so if a user ask for a file that is not present, the data from the most recent previous file must be served. For example, with the above files, if someone ask for the data from date 2017-01-07, he should get the data from file 170104.json. But if the gap between files is greater than five, then no data should be provided. With the previous files if someone request for the data from date 2017-01-20, then a 404 error should be return.

I have this controller which does the job, but I'm concern that it is using the existSync function and therefore blocking the node process (the end point is localhost:3000/api/:year/:month/:day):

import fs from 'fs';
import path from 'path';

const getData = (req, res) => {
  const {
    year,
    month,
    day,
  } = req.params;
  const date = new Date(`${year}/${month}/${day}`);
  let fileDate = date.toISOString().substring(2, 10).replace(/-/g, '');
  let jsonFile = path.join(__dirname, 'data', `${fileDate}.json`);

  for (let i = 1; i <= 5; i += 1) {
    if (fs.existsSync(jsonFile)) {
      break;
    } else {
      date.setDate(date.getDate() - 1);
      fileDate = date.toISOString().substring(2, 10).replace(/-/g, '');
      jsonFile = path.join(__dirname, 'data', `${fileDate}.json`);
    }
  }

  fs.readFile(jsonFile, 'UTF-8', (err, data) => {
    if (err) {
      res.status(404).send('Not found');
    } else {
      res.json(JSON.parse(data));
    }
  });
};

export default getData;

// this is routes.js

import getData from '../controllers/apiController';

const routes = (app) => {
  app.route('/api/:year/:month/:day')
    .get(getData);
};

export default routes;

How can I improve this to be non blocking, or improve it's quality or performance overall? I'm learning nodejs and express and trying to grasp all of its best features, but having a hard time getting my head around the async functions, so any advice to improve this in that direction is very welcome.

I would consider asynchronously checking for all 5 potential
files at once, meaning that checks can be paralellized.  Once all are complete, you can evaluate the compiled results and return the most recent file.

Depending on your expected call pattern ( for example, are more recent files typically the ones being requested?), you may consider keeping a cached copy of all or part of the file listings in memory to avoid disk read altogether.

Another thing you may really want to think about is your API interface. I would find it extremely hard to understand that a request for a file that may not actually exist would return a 200 result. This would be very non-standard from a REST API standpoint. Perhaps the client calling this service should own the logic of checking for 5 days’ worth of results.

Why all the weird formatting steps from Date object? Just output directly to your desired format instead of chaining together a bunch of string manipulation.

If file is already JSON format, why parse it and then reserialize when delivering response? Had you consider serving these files statically?  You could GREATLY simplify this code (down to literally two or three lines by serving these files statically and letting caller own the logic of looking for days in the past. Not to mention. You can get all the serve static behaviors around cache-control, etags, etc. this way if cacheability matters to your client.


Shorthand JS library, take 2 Based on the feedback and suggestions I received in my previous post (Lightweight shorthand JS library), I've updated and optimized Mozart JS.

Again, the goal is to have some jQuery-like functionality in a minimal size. Thoughts on this updated version?

Full library on GitHub.

Mozart JS

Element Retrieval

Shorthand for document.querySelectorAll or document.querySelector. My decision to include both is just personal preference.

function $get(input) {
    return document.querySelectorAll(input);
}
function $grab(input) {
    return document.querySelector(input);
}

Example:

var foo = $grab('#bar'); //returns element with the Id of "bar"
var zed = $get('.cop'); //returns collection with the classname "cop"

Element Creation

Shorthand for document.createElement and Element.setAttribute. Though text is not an attribute, I chose to allow users to set textContent as well, similar to jQuery.

function $make(tag, params) {
    var obj = document.createElement(tag);
    for (var key in params) {
        if (key === 'text') {
            obj.textContent = params[key];
        } else {
            obj.setAttribute(key, params[key]);
        }
    }
    return obj;
}

Example:

var g = $make('button', {
    style:'float: left',
    class:'myClass',
    text:'Some text here',
    value:'myValue'
});

//Equivalent to:

var g = document.createElement('button');
g.setAttribute('style','float: left');
g.setAttribute('class','myClass');
g.textContent = 'Some text here';
g.setAttribute('value','myValue');

Element Processing

This function is not meant to be called outside the library. It ensures that data is in an iterable format that can be processed by the insertion and removal methods.

//turn something into a set
function $set(input) {
    var set;
    if (typeof input === 'string' || input instanceof String) {
        set = $get(input);
    } else if (input.nodeType === 1 || input.nodeType === 3) {
        set = [input];
    } else if (input[Symbol.iterator] !== undefined) {
        set = input;
    } else {
        throw new TypeError('Cannot convert ' + typeof input + ' to iterable');
    }
    return set;
}

Element Sibling Insertion

Designed to mimic most of jQuery's insertBefore() method. I chose to allow only one element inserted at a time, but that element can be inserted multiple times. I also chose to clone the element for all but the first insertion so that event listeners would be preserved for the first one. There may be a better way to do this, though, so I'd appreciate suggestions.

//equivalent function $addAfter() not shown here
function $addBefore(input, target) {
    var newElm = $set(input)[0];
    var oldElms = $set(target);
    if (oldElms[0]) {
        var firstTarget = oldElms[0];
        firstTarget.parentNode.insertBefore(newElm, firstTarget);
        for (var i = 1; i < oldElms.length; i++) {
            var oldElm = oldElms[i];
            var clone = newElm.cloneNode(true);
            oldElm.parentNode.insertBefore(clone, oldElm);
        }
    }
}

Example:

var foo = $make('div', {'text': 'This is a test'});
$addBefore(foo, '.myClass'); //adds an instance of foo before each myClass

Element Removal

Removes an element or collection of elements.

//element removal
function $remove(input) {
    var rem = $set(input);
    for (var i = 0; i < rem.length; i++) {
        var elm = rem[i];
        elm.parentElement.removeChild(elm);
    }
}

Example:

$remove('#myId');
$remove('.myClass');

//Equivalent to:

var rem = document.querySelectorAll('#myId');
rem.parentNode.remove(rem);

var myClassColl = document.querySelectorAll('.myClass');
for (var i = 0; i < myClassColl.length; i++) {
    var elm = myClassColl[i];
    elm.parentNode.remove(elm);
}

function $get(input) {
    return document.querySelectorAll(input);
}
function $grab(input) {
    return document.querySelector(input);
}

If I would like to get DOM elements from the page, its overhead to think about having 2 ways to get them and having 2 possible values to deal with. Best if you just deal with sets. If you just need one, it's as simple as the grabbing the set, and get one. No extra code needed.

var justOneDiv = $get('div')[0];

Speaking of sets and array-like objects, querySelectorAll returns a NodeList. It's like an array, but without the array methods. What you could do is to convert the NodeList into a real array. That way, you have instant access to array methods. This could be done with array.slice.

function toArray(arrayLike){
  return Array.prototype.slice.call(arrayLike);
}

function $get(selector){
  return toArray(document.querySelectorAll(selector));
}

// We have instant access to forEach
$get('div').forEach(function(div){
  div.style.color = 'red';
})

// We can use filter
$get('div').filter(function(div){
  return ~toArray(div.classList).indexOf('bold');
}).forEach(function(div){
  div.style.fontWeight= 'bold';
});
.red{
  color: red;
}

Red

Red

Red

Now we're almost like jQuery but jQuery takes it a bit further. It has the concept of a "jQuery object" which is a custom object that holds the set we're interested in, and has a prototype containing all the methods that operate on that set.

function $(selector){
  if(!(this instanceof $)) return new $(selector);
  this.set = this.find(selector);
}

$.prototype.find = function(selector){/* code that finds elements */};
$.prototype.insert = function(something){/* code that inserts something */};
$.prototype.each = function(callback){ this.set.forEach(callback} };

var div = $('div').each(function(div){/* do something */});

The above is a contrived example of how jQuery works. Adding methods is as easy as adding to the prototype (jQuery exposes it to the world as $.fn). This is how jQuery is so extensible in addition to just exposing one object to the global namespace.

var g = $make('button', {
    style:'float: left',
    class:'myClass',
    text:'Some text here',
    value:'myValue'
});

Still, text is the odd one here. Text are separate nodes, totally different from the element itself. They're essentially "children" of the element and not part of the element. I'd suggest you do something similar to jQuery's text() method. value is different because it is a legit property of some inputs.

Also, all your functions reside in the global scope. You risk clobbering other methods of the same name, or risk yours getting overridden. I suggest you take up just one name in the global scope and nest your methods under it. Mozart would be a good name.

var Mozart = {
  $get: function(){...},
  $remove: function(){...},
  ...
};

var divs = Mozart.get('div');


Modifying a string by passing a pointer to a void function Please explain the relationship between pointers and arrays.  In this tutorial, they change int c by changing *r in the function.  How is that possible?

Also, please review my code for reversing a string.  The string variable belongs to the main() function, and I would like to pass a pointer to let the strreverse() function write to the string.  Does my code work?

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

void strreverse(char *string)
{
 int i, len;
 char c;
 len=strlen(string);
 char string2[len+1];

 for(i=0; i<len; i++)
 {
  c=string[i];
  string2[len-i]=c;

 }
 string2[len+1]='\0';
 string=string2;
 //printf("%s\n", string); <-- this would work,but should be in main

}

int main(int argc, char *argv[])
{
  char str[256];
  printf("Type a String to reverse it.[max. 255 chars]\n");
  fgets(str, 255, stdin);
  strreverse(&str[0]);
  printf("%s", str);
  return EXIT_SUCCESS;
}

No, I've compiled your code with C90 and it doesn't reverse the given string.

int main(int argc, char *argv[]) If you're not compiling from the command line with arguments to be used by the program, then int main() suffices.
Passing argument by pointer is used when you want the value of the variable changed. Say I have a variable int var and a function change(.), I want change to alter the value of var. If I declare change as void change(int n) and I call change(var), function change will take a copy of var named var but it's only a copy, its address is different from the original var. Any change you make on copy var will not affect the original var. If i declare change as void change(int* n), then change will only accept int pointers, I have to pass the address, &var into change as change(&var). Now working on this address is exactly like working on the initial var.

To understand pointer-array relationship read wikipedia.

int array[5];      // Declares 5 contiguous integers
int *ptr = array;  // Arrays can be used as pointers
ptr[0] = 1;        // Pointers can be indexed with array syntax
*(array + 1) = 2;  // Arrays can be dereferenced with pointer syntax
*(1 + array) = 3;  // Pointer addition is commutative
2[array] = 4;      // Subscript operator is commutative

Array names are not really pointers but can be used as pointers. Instead of  strreverse(&str[0]), you can do strreverse(str). Same result.

You've passed argument as pointer but your code still fails, why?

One thing to know about fgets is that unless there is an [EOF][2] which you can only get from an input file or Ctrl+Z if you run from the command line, fgets exits when the length argument is reached or it encounters a newline. In summary, fgets reads the newline as a character when you press enter key, increasing your desired length by 1. So if I had entered "my string" + Enter, your variable str becomes "my string\n".
So you got the length of the string into len. The array string is zero-based, calling string[len] return the char after the desired last one. The last char is string[len - 1].

You should have done this;

char string2[len];

for(i=0; i<len; i++){
  c=string[i];      //variable c is unimportant
  string2[len-i-1] = string[i];  //observe the -1, when i = 0, string2[len - 1] = string[0]
}
string2[len] ='\0';

Now that you're done reversing, you need to understand the implication of your next move.

string = string2;

string is a pointer, but that doesn't make it any less of a variable, it's a pointer variable, it also has an address. And if I declare a pointer variable to its address, that pointer will also have an address. Going back to what I said earlier, when you call change(&var), a copy of the address of var is passed into the function, so when you change the value of this pointer, it no longer holds the address of var. You may think about dereferencing, like this

*string = *string2;

but this will only alter the first value of the array since *string is same as string[0]. The solution is to copy element by element.

for(i=0; i<=len; i++)
  string[i] = string2[i];

Now your string is reversed.

Read this wikipedia article to understand how an array can be reversed faster.


Run dir scan as fast and efficient as possible / clean code I'm not a very experience python programmer. But I want to make my code as fast and efficient as possible as well as write it clean and therefore got the advice to share it here. So I'm having the following code, which works well but isn't very fast as I have dirs bigger than 4TB and I'm executing that code in my network. So I'm looking for advice, to read out all data in one path instead of doing redundant second scanning of dirs and files. Any advice would be appreciated!
I'm also thinking of how to implement multiprocessing but I think that wouldn't help much as a lot is IO.

def get_size_and_fcount_pathlib(scan_path):
    """Gets the total size of given dir and counts how many folders and files are in the given
    path directory and return a file count, folder count and all types as a sum"""
    root_directory = Path(scan_path)
    total_size = 0
    all_types_count = 0
    file_count = 0
    folder_count = 0
    for f in root_directory.glob('**/*'):
        if f.is_file():
            file_count += 1
            total_size += f.stat().st_size
            if not str(f.name).startswith("."):
                all_types_count += 1
        if f.is_dir():
            folder_count += 1
    size_gb = ", ".join(map(str, (round(total_size/1000/1000/1000, 2), 'GB'))).replace(', ', '')

    print('Amount of all types searched: {}'.format(all_types_count))
    print('Amount of files searched:     {}'.format(file_count))
    print('Amount of folders searched:   {}'.format(folder_count))
    print('Directory size in GB:         {}'.format(size_gb))

    file_count_collection = [size_gb, all_types_count, file_count, folder_count]
    return file_count_collection

Naming things

Be consistent in the way you name things. This applies to the words chosen, whether you abbreviate things or not, whether you separate multiple words or not. There are some names you are using that are not consistent. For example:

def get_size_and_fcount_pathlib(scan_path):

You start with fully spelled out words separated by underscores, and then suddenly there is fcount, two words without separators and with one word being abbreviated. I would start by writing this out consistently as:

def get_size_and_file_count_pathlib(scan_path):

If you want to reduce the size of the name, I would rather omit redundant parts of it. Is pathlib really adding anything to this? If get_size_and_file_count() gives you enough context to deduce what the function does, I would go for that.

Another issue is with this variable:

file_count_collection = [size_gb, all_types_count, file_count, folder_count]

It says it is a file count collection, but it also contains the total size. I would change this to size_and_file_count_collection, to make it clear it also contains the size, and this also better matches the name of the function. Again, if you want to make it shorter, I would then drop _collection from the name, since size_and_file_count already implies it is more than one bit of information.

Arguably, size_and_file_count is also not completely descriptive since it also includes directory count and a count of all types of filesystem entities.
There might be an even better name for this, perhaps directory_statistics?

Use a class to represent structured data

The file_count_collection is just a list. Unless you read the code of this function, it is not obvious in which order you stored the size and counts in this list. It is better to create a class that represents this collection of data, where each piece will get its own name, like so:

class size_and_file_count:
   def __init__(self, size_gb, all_types_count, file_count, folder_count):
       self.size_gb = size_gb
       self.all_types_count = all_types_count
       self.file_count = file_count
       self.folder_count = folder_count

Then, in get_size_and_file_count(), you can replace the last two lines with:

return size_and_file_count(size_gb, all_types_count, file_count, folder_count)

And in the code that calls that function, instead of writing:

result = get_size_and_file_count("...")
file_count = result[2]

You can now write:

result = get_size_and_file_count("...")
file_count = result.file_count

Don't convert values too early

You are calculating the total size, and then converting it to a value in gigabytes (with only 2 decimals), and only storing that final result. What if I want to get the size of a directory that contains less than 10 megabytes of data? Your function will tell me it is 0.00 gigabytes big, which is not helpful. It is better to store the size in bytes, and only convert to gigabytes when printing the result, like so:

print('Directory size in GB:         {:.2f} GB'.format(total_size / 1.0e9))

And of course also store the total size in bytes in the returned collection.

Separate obtaining results from printing results

Your function does two things at once: it calculates the results and it prints them. It is best to separate the two issues, and have get_size_and_file_count() only calculate the results and return them, and then have a seperate function that takes a size_and_file_count variables as a parameter and prints the values to the screen.

If you have made a class size_and_file_count, you could make the function that prints the results a member function of that class.

What is all_types_count?

There are many different types of filesystem entries besides regular files and directories; there are fifos, sockets, symlinks, character devices and so on.
I would expect a variable named all_types_count to count all of them. However, in your code you actually disregard all things that are neither regular file nor directory, and instead all_types_count just counts those regular files whose name does not start with .. So actually your all_types_count is just a non_hidden_files_count. Either this was your intention, in which case you should rename the variable, or you actually wanted to count all filesystem items, in which case your count is wrong.

Optimizing the code

If you want to make the code as fast as possible, there are several ways to do that. First, I would start by replacing the call to glob() with os.walk(). Since you want all files and directories anyway, the glob function is likely incurring an unnecessary overhead of comparing each item it finds against the glob pattern you gave. Also, os.walk() already splits the results for each directory in a list of file names and list of directory names, making your life a bit easier. It would look like:

for root, dirs, files in os.walk(root_directory):
    folder_count += len(dirs)
    for f in files:
        if Path(root, f).is_file():
            ...

If you want more speed, then there are two other ways to consider, that can even be combined. First, write the function in C, and make a Python wrapper function around the C code. Second, you could try to paralellize the code, for example by scanning multiple directories in parallel. However, the latter is a lot of work, and there is a good chance it will not be faster at all, since you might actually not be CPU bound but I/O bound, and even if you are CPU bound, then the overhead of spawning multiple threads or tasks and synchronizing them might be more than the performance gain from the actual parallel execution of the code.

When you are interested in optimizing the code, start with benchmarking it, and then see if the suggestions actually help with performance. Also, before trying to parallelize the code, actually check if your Python code is actually using 100% CPU when scanning directories. If not, it's not worth going that route.


Simple compression reloaded++ As a follow up to my previous question I've improved the code + algorithm. The compression now works the following way:

Each character is followed by a length byte. The top 3 bits of that byte denote the number of additional length bytes which encode the count. The count is stored in "little endian order" (least significant byte first) with the first 5 bits being encoded in the initial length byte.

Some examples:

Count == 1  yields in length byte  00000001
Count == 31 yields in length byte  00011111
Count == 32 yields in length bytes 00100000 followed by 0000001
Count == 33 yields in length bytes 00100001 followed by 0000001

The program supports a command line option for printing the output in hex for easier reading and debugging. I tried to use a const int for the NUM_LENGTH_BITS as well but then the compiler claimed that the initializer element for MAX_COUNT isn't constant. Not sure if that can be worked around.

I've limited the max count to 61 bits to leave 3 bits for the length specifier and have a maximum of 8 bytes for the length output.

All types of feedback appreciated.

#include <stdio.h>
#include <stdbool.h>
#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#include <assert.h>

#define NUM_LENGTH_BITS 3
static const uint64_t MAX_COUNT = (~0ULL) >> NUM_LENGTH_BITS;

static bool print_hex = false;

static int most_significant_bit(uint64_t value)
{
    int i = 0;
    while (value)
    {
        ++i;
        value >>= 1;
    }
    return i;
}

static int num_bytes_required(uint64_t value)
{
    int msb = most_significant_bit(value);
    int bytes = msb / 8;
    int remainder = msb % 8;
    if (remainder)
    {
        bytes += (remainder + NUM_LENGTH_BITS) > 8 ? 2 : 1;
    }
    return bytes;
}

void write_char(int c)
{
    if (print_hex)
    {
        if (printf("0x%x ", c) < 0)
        {
            perror("error printing to stdout");
            exit(EXIT_FAILURE);
        }
    }
    else
    {
        if (EOF == putchar(c) && ferror(stdout))
        {
            perror("error writing to stdout");
            exit(EXIT_FAILURE);
        }
    }
}

void write_count(uint64_t count)
{
    int additional_length_bytes = num_bytes_required(count) - 1;
    assert(additional_length_bytes >= 0 && additional_length_bytes < 8);

    int first = ((additional_length_bytes << 5) | (count & 0x1F)) & 0xFF;
    write_char(first);

    count >>= 5;

    while (count)
    {
        write_char(count & 0xFF);
        count >>= 8;
    }
}

static void parse_args(int argc, char **argv)
{
    if (argc == 2)
    {
        print_hex = (stricmp(argv[1], "-h") == 0 || stricmp(argv[1], "--hex") == 0);
    }
    else
    {
        printf("Invalid number of arguments passed: %d\n", argc - 1);
        exit(EXIT_FAILURE);
    }
}

int main(int argc, char** argv)
{
    int current_char = 0;
    int previous_char = 0;
    uint64_t current_char_count = 0;

    parse_args(argc, argv);

    while (true)
    {
        current_char = getchar();
        if (current_char_count == 0 || current_char_count == MAX_COUNT || previous_char != current_char)
        {
            if (current_char_count > 0)
            {
                write_count(current_char_count);
            }
            if (EOF != current_char)
            {
                write_char(current_char);
                current_char_count = 1;
                previous_char = current_char;
            }
            else
            {
                break;
            }
        }
        else
        {
            current_char_count += 1;
        }
    }
}

A few minor notes (looks pretty good for the most part!):

Your most_significant_bit() function loop should be condensed into a for loop.  The main difference between the for's and the while's is a matter of pragmatics: we usually use for when there is a known number of iterations (which might not seem like the case here, but it's true), and use while constructs when the number of iterations in not known in advance.
I'm not a fan of you using -h as the command line argument equivalent to --hex.  Whenever I think of -h, I think of it as the shorthand of --help.  A better shorthand would be -x in my opinion.
Why do you do current_char_count += 1?  Why not current_char_count++/++current_char_count (both do the same thing in the same amount of time in this case)?


Getting the average score for hotel scores in different countries using pandas I am diving into Data Analysis with pandas, and I have just written this Python script to calculate the average of hotel review scores of each country. The dataset contains an individual average score for each customer review, like: 8.86 or 7.95. My goal was to average all these individual scores for a particular country.

For example, if the hotels in United Kingdom got the following hotel review scores: 8.65, 7.89, 4.35, and 6.98, I would average these four scores and create a dataframe where the first column is "Country" and the second column is the "Overall Average Score" for that country.

I tried to write a concise code as much as I could. Would you mind giving your opinions and recommendations about it? I'll be adding this to my portfolio. What should be kept and/or avoided in a professional and real-world setting?

Script:

# Average all scores that belong to a particular country.

import pandas as pd

# Reading original hotel reviews dataset.
df = pd.read_csv(DATASET_PATH)

# Getting a dataframe with two columns: 'Hotel_Address' and 'Average_Score'.
df = df.loc[:, ["Hotel_Address", "Average_Score"]]

# List of tuples.
countries_w_avg_list = []
for _, row in df.iterrows():
    address = row[0].split()
    country_name = address[len(address) - 1]
    countries_w_avg_list.append( (country_name, row[1]) )

# Getting the sum of all 'Average_Score' values for each country.
d = {} # Empty dictionary. It will be a dictionary with list values, like: {"Netherlands": [sum, counter]}
counter = 0
for country, individual_average in countries_w_avg_list:
    if country not in d:
        d[country] = [0, 0]

    d[country][0] += individual_average
    d[country][1] += 1

# Getting the average of all 'Average_Score' values for each country.
for key, value in d.items():
    d[key] = round((d[key][0] / d[key][1]), 2)
# print(d)

# Now, I believe there are two ways to transform this dictionary in the df I want.
# 1 - Transform d in a df, and then transpose it. Then rename the columns.
# 2 - Create a dataframe with the column names "Country" and "Overall Average Score"
# and their values as d's keys as the value for the first column and d's values as the
# values for the second column.
df = pd.DataFrame({"Country": list(d.keys()), "Overall Average Score": list(d.values())})
print(df)

You should probably use pandas.DataFrame.groupby.

The string manipulations to extract the countries can also be simplified using pandas.DataFrame.str methods.

import pandas as pd

# Reading original hotel reviews dataset.
# Reduce dataframe to two columns: 'Hotel_Address' and 'Average_Score'.
df = pd.read_csv(DATASET_PATH).loc[:, ["Hotel_Address", "Average_Score"]]

# Extract country from address
df["Country"] = df.Hotel_Address.str.split().str[-1]
df.drop(columns=["Hotel_Address"], inplace=True)

# Get average average score per country, rounded to two decimal places
print(df.groupby("Country").mean().round(2))


Implementation of an Stack without Linked Lists in C I'm a beginning C programmer. As an exercise, I have attempted to implement a stack of strings in C. This is my first real exercise working with pointers. I looked at some other similar posts on here, and read through the comments. It seems that most individuals have chosen to do this by using linked lists, whereas I've just created an ArrayStack.

Maybe it would have been better to go the linked list route. Nonetheless, this is how I did it. I'm hoping there's not any glaring errors, but I'm sure there will be.

Also, question: I have a variable for the number of slots available in the array, which is used to determine when more memory needs to be allocated. But it would seem to me that I can use sizeof on the array, and sizeof on a single pointer in the array, and use division to get the number of slots, thus eliminating a variable. Would this be a better approach?

stack.h

#include <stdlib.h>

typedef struct stringstack {
    int count;
    int slots;
    char** stack;
} stringstack;

void * makestack(void);
void push(struct stringstack* ptr, char stringtext[]);
char * pop(struct stringstack* ptr);
char * peek(struct stringstack* ptr);
int isempty(struct stringstack* ptr);
void destroy(struct stringstack* ptr);

stack.c

#include "stack.h"

/* Function creates a new stringstack object and returns a pointer to it */
void * makestack(void) {
    stringstack *ptr = (stringstack *) malloc(sizeof(stringstack));
    ptr->count = 0;
    ptr->slots = 1;
    ptr->stack = (char **) malloc(sizeof(char*));
    return ptr;
}

/* Pushes a string onto the top of a stack */
void push(struct stringstack* ptr, char stringtext[]) {
    ptr->count++;
    if (ptr->count == ptr->slots) /* Allocate more space if necessary */
        ptr->stack = (char **) realloc(ptr->stack, sizeof(char *)*((ptr->slots*=2)+1));
    char *stringptr = (char *) malloc(sizeof(stringtext));
    stringptr = stringtext;
    *(ptr->stack + ptr->count) = stringptr;
}

/* Removes the top item from the stack, returning a pointer to it */
char * pop(struct stringstack* ptr) {
    if (ptr->count>0)
        return *(ptr->stack + ptr->count--);
    return NULL;
}

/* Returns a pointer to top item from stack but does not pop it */
char * peek(struct stringstack* ptr) {
    if (ptr->count>0)
        return *(ptr->stack + ptr->count);
    return NULL;
}

/* Returns whether a stringstack contains at least one string */
int isempty(struct stringstack* ptr) {
    return (ptr->count == 0);
}

/* Frees a stringtack from memory */
void destroy(struct stringstack* ptr) {
    size_t n;
    for (n = 0; n <= ptr->count; n++ )
        free(ptr->stack+n);
    free(ptr->stack);
    free(ptr);
}

makestack should return a struct stringstack * instead of void *.
malloc and realloc may fail. Check their return values.
push doesn't copy the string (it only makes a redundant copy of the pointer). It is OK as long as you can guarantee that the string parameter has a lifetime long enough. Consider a scenario:

void foo(struct stringstack * stack)
{
    char bar[] = "aaa";
    push(stack, bar);
}

Upon the return, bar disappears, and the stack entry points to the pretty random place in the application stack. Do not expect to have "aaa" there.

To actually copy the string, do

 *(ptr->stack + ptr->count) = strdup(stringtext);

which may introduce other problems in interacting with client (who owns the string after peek?). Unfortunately, C doesn't offer a clean solution.
You may want to shrink the stack when ptr->count hits the low watermark of ptr->slots.


Vector-based flood-fill algorithm queue class I want to implement the flood-fill algorithm for https://stackoverflow.com/questions/12995378/is-there-a-proper-algorithm-for-detecting-the-background-color-of-a-figure. I did it recursively, but I had a stack-overflow error. No matter - Wikipedia has a spot for a non-recursive, iterative solution.

It requires a queue. For school, we're not supposed to use Lists nor the Java Queue class (I don't even know if it does what I think it does, but it doesn't matter - I can't use it).

public class QueueOfPixeles
{
    public Pixel[] elements;
    public int numberOfElements;

    public QueueOfPixeles()
    {
        numberOfElements = 0;
        elements = new Pixel[1];
    }

    // Next pixel is supposed to return the last pixel element in the queue.
    // When a pixel element is returned, it will become null in the system.
    public Pixel nextPixel() {
        Pixel result = null;
        for (int i = elements.length - 1; i >= 0 && result == null; --i) {
            result = elements[i];
            if (result != null) {
                elements[i] = null;
            }
        }
        return result;
    }

    // Adds a new pixel to the queue.
    // Then, it checks if the capacity of the vector has been reached.
    public void add(Pixel pixel) {
        elements[numberOfElements] = pixel;
        ++numberOfElements;
        fixVector();
    }

    // If the vector capacity has been reached, create a new vector with all old elements
    // but with more capacity.
    private void fixVector() {
        if (numberOfElements >= elements.length) {
            Pixel[] newVector = new Pixel[(elements.length * 2) + 1];
            for (int i = 0; i < numberOfElements; ++i) {
                newVector[i] = elements[i];
            }
            elements = newVector;
        }
    }
}

WHAT IS IT SUPPOSED TO DO?

You should be able to add pixels to the end of the queue, and get the next pixel by using nextPixel(). When you retrieve the next pixel, it should be removed from the queue automatically.

DOES IT WORK?

Yes, it seems to be working. I've done a few tests and it has indeed returned all I expected.

WHAT'S THE PROBLEM?

I'd like to get some feedback about this. I can only use vectors for this project, and I feel there is something rather not-efficient with the way I manage the pixels in the queue (converting the elements to null and just that etc, or asking for more capacity size instead of using the null-spaces I made).

What do you think?

A note on terminology: What you have implemented is usually called a stack rather than a queue, but it can also be called a LIFO (last in, first out) queue, so it's not wrong to call it a queue, only uncommon.

The nextPixel() method is rather inefficient. You keep track of the number of elements in the queue, and you never do anything that allows any array element with an index >= numberOfElements to be anything but null. So instead of starting the search at elements.length - 1, you should start at numberOfElements - 1.

But that still leaves an inconsistency/inefficiency. The add() method doesn't guard against adding nulls to the queue, but you never return a null until the queue is empty.

You should either return nulls if you accept them to be added to the queue, or, better, I think, guard against nulls in add(). So I'd recommend

public void add(Pixel pixel) {
    if (pixel == null) return; // nothing to do
    // pixel isn't null, add it
    elements[numberOfElements] = pixel;
    ++numberOfElements;
    fixVector();
}

and then - also if you decide to return nulls if they're added - nextPixel() can simply be

public Pixel nextPixel() {
    // queue empty
    if (numberOfElements == 0) return null;
    // decrement numberOfElements and
    // return the last added Pixel
    return elements[--numberOfElements];
}

In fixVector(), you can probably be a bit more efficient if you use the generic Arrays.copyOf method, but maybe you are not allowed to use that either.


Analyzing the U.S. Births dataset in Python I'm a Data Science newbie and currently looking to improve my code. I was trying to calculate the total number of the following:

The total number of births on each month
The total number of births on each day of the week

Sample dataset from CSV:

year, month, date_of_month, day_of_week, births
1994, 1,     1,             6,           8096
1994, 1,     2,             7,           7772
1994, 1,     3,             1,           10142
1994, 1,     4,             2,           11248
1994, 1,     5,             3,           11053
...

It led me into this implementation:

def weekly_births(lst):
    mon = birth_counter(lst, 3, 1, 4)
    tue = birth_counter(lst, 3, 2, 4)
    wed = birth_counter(lst, 3, 3, 4)
    thu = birth_counter(lst, 3, 4, 4)
    fri = birth_counter(lst, 3, 5, 4)
    sat = birth_counter(lst, 3, 6, 4)
    sun = birth_counter(lst, 3, 7, 4)

    births_per_week = {
        1: mon,
        2: tue,
        3: wed,
        4: thu,
        5: fri,
        6: sat,
        7: sun
    }

    return births_per_week

def monthly_births(lst):

    jan_births = birth_counter(lst, 1, 1, 4)
    feb_births = birth_counter(lst, 1, 2, 4)
    mar_births = birth_counter(lst, 1, 3, 4)
    apr_births = birth_counter(lst, 1, 4, 4)
    may_births = birth_counter(lst, 1, 5, 4)
    jun_births = birth_counter(lst, 1, 6, 4)
    jul_births = birth_counter(lst, 1, 7, 4)
    aug_births = birth_counter(lst, 1, 8, 4)
    sep_births = birth_counter(lst, 1, 9, 4)
    oct_births = birth_counter(lst, 1, 10, 4)
    nov_births = birth_counter(lst, 1, 11, 4)
    dec_births = birth_counter(lst, 1, 12, 4)

    births_per_month = {
        1: jan_births,
        2: feb_births,
        3: mar_births,
        4: apr_births,
        5: may_births,
        6: jun_births,
        7: jul_births,
        8: aug_births,
        9: sep_births,
        10: oct_births,
        11: nov_births,
        12: dec_births
    }

    return births_per_month

The birth_counter function:

def birth_counter(lst, index, head, tail):
    sum = 0
    for each in lst:
        if each[index] == head:
            sum = sum + each[tail]
    return sum

The parameters:

lst - The list of dataset
index - The lst's index
head - Will be compared from lst's index
tail - The target data that needs to be computed

Example usage:

[lst]  [0]  [1] [2] [3]  [4]
lst = [1994, 1,  1,  6, 8096]...
sample_births = birth_counter(lst, 1, 1, 4)

    if sample_births[1] == 1 then
        extract index[4] #8096

Questions regarding weekly_births and monthly_births:

If you notice, I manually entered the number of weeks and months then calculated the total births on each. Is there a way to iterate over weeks and months to avoid a lengthy piece of code?

If you want to do data-analysis in Python, you should learn about numpy and pandas. The former implements efficient numeric calculations (on whole arrays). The latter uses numpy and introduces a DataFrame, which is a bit like a table that can be manipulated in many ways. You can sort it by some column(s), you can transform them and you can even group them by some column(s) and perform operations on the groups (which is what you want to do here).

Your current code boils down to very few lines with pandas:

import pandas as pd

df = pd.read_csv("us_birth_statistics.csv", skipinitialspace=True)
birth_per_month = df.groupby("month").births.sum()
birth_per_weekday = df.groupby("day_of_week").births.sum()

print(birth_per_month)
print()
print(birth_per_weekday)

#month
#1    48311
#Name: births, dtype: int64

#day_of_week
#1    10142
#2    11248
#3    11053
#6     8096
#7     7772
#Name: births, dtype: int64