Receiving and sending notifications from database I'm not much familiar with Task Api programming in C#. Can anyone review my following code?

The following is a ElapsedEventHandler of a System.Timers.Timer. In this event, I'm getting pending notifications from database and sending them. After I send the notification, I update that notification in database by setting HasSent = true. We require our notifications to be sent as soon as possible. This is the reason, I'm updating HAsSent in a separate thread by using Task Api. I would like to know if this is a bad practise? Should I use this? If not then why?

protected void ProcessNotifications(object source, ElapsedEventArgs e)
{
    UtilityMethods.StopTimer(_notificationTimer);

    try
    {
        var notificationService = new NotificationService();
        var pendingNotifications = notificationService.GetPendingNotifications();

        if (pendingNotifications.Count > 0 && LoadConfigurations())
        {
            foreach (var notification in pendingNotifications)
            {
                try
                {
                    var notificationType = (NotificationType)notification.NotificationTypeId;

                    notification.NotificationTemplate = new NotificationTemplateService().GetById(notification.NotificationTemplateId);
                    notification.NotificationAttachments = notificationType == NotificationType.Email ?
                        new NotificationAttachmentService().GetByNotificationId(notification.NotificationId) : null;

                    using (var sender = NotificationSenderFactory.Create(notificationType))
                    {
                        if (sender.Send(notification))
                        {
                            Task.Factory.StartNew(() =>
                            {
                                notificationService.MarkNotificationsAsSent(notification.NotificationId);// A database Hit
                            });
                        }
                    }
                }
                catch (Exception ex)
                {
                    AppLogger.LogException(ex);
                }
            }
        }
    }
    catch (Exception ex)
    {
        AppLogger.LogException(ex);
    }
    finally
    {
        UtilityMethods.StartTimer(_notificationTimer);
    }
}

Should I use this?

NO

If not then why?

Reference TaskFactory.StartNew Method

Remarks
Starting with the .NET Framework 4.5, the Task.Run method is the recommended way to launch a compute-bound task. Use the StartNew method only when you require fine-grained control for a long-running, compute-bound task. .......

Which would end up as
//...omitted for brevity

Task.Run(() => {
    notificationService.MarkNotificationsAsSent(notification.NotificationId);// A database Hit
});

//...omitted for brevity


database connection, fetching rows and handling error in csharp Fetching database records and displaying as JSON. Asking for revision to ensure if everything's ok, i.e. connections are properly handled and closed in case of error.

public class Department
{
    public Department(int id, String name)
    {
        this.Id = id;
        this.Name = name;
    }

    public int Id { get; set; }

    public String Name { get; set; }
}

public List<Department> FindAllDepartment()
{
    List<Department> rows = new List<Department>();
    using (SqlConnection sqlConnection = new SqlConnection("Data Source=;Initial Catalog=;Persist Security Info=True;User ID=sa;Password=P@ssw0rd;pooling=true"))
    {
        SqlCommand command = new SqlCommand("SELECT * FROM Employees.dbo.Department", sqlConnection);
        try
        {
            sqlConnection.Open();
            using (SqlDataReader sqlDataReader = command.ExecuteReader())
            {
                while (sqlDataReader.Read())
                    rows.Add(new Department(sqlDataReader.GetInt32(0), sqlDataReader.GetString(1)));
            }
            return rows;
        }
        finally
        {
            sqlConnection.Close();
        }
    }
}

using (SqlConnection sqlConnection = new SqlConnection("Data Source=;Initial Catalog=;Persist Security Info=True;User ID=sa;Password=P@ssw0rd;pooling=true"))
  {
      ...
      }
      finally
      {
          sqlConnection.Close();
      }
  }

You're correctly using a using statement for the connection, which will both close and dispose the connection when its scope finishes. So no need to explicit call Close().

Almost (if not) every database related object implements IDisposable - including SqlCommand, so you should encapsulate that in a using as well.

All in all, your method should look like something like this:

public List<Department> FindAllDepartment()
{
  using (SqlConnection sqlConnection = new SqlConnection("Data Source=;Initial Catalog=;Persist Security Info=True;User ID=sa;Password=P@ssw0rd;pooling=true"))
  {
    sqlConnection.Open();

    using (SqlCommand command = new SqlCommand("SELECT * FROM Employees.dbo.Department", sqlConnection))
    using (SqlDataReader sqlDataReader = command.ExecuteReader())
    {
      List<Department> rows = new List<Department>();
      while (sqlDataReader.Read())
      {
        rows.Add(new Department(sqlDataReader.GetInt32(0), sqlDataReader.GetString(1)));
      }

      return rows;
    }
  }
}

where the using statements handle the clean up - even if an exception is thrown.


Convert international datestring to ISO-format The function below takes a datestring in the form d-mmm-yyyy and converts it to ISO date format (yyyy-mm-dd). Delimiters may be hyphen, space or /,  and Dutch or English month abbreviations may be used.

Now, I know there is dateutil, but it returns unknown string format if you try to parse something with a non-English month in it. I haven't digested all its documentation, but I think dateutil is mainly intended for date calculation. I'm not doing that, I'm just cleaning up user input.

So I wrote my own.

import re
.
.

def ISOdate(date):
    '''
        converts the following date string format to ISO (yyyy-mm-dd):
        28-okt-1924 (dutch month abbreviations)
        28 oct 1924 (english..)
         9/nov/2012 (single digit)
    '''

    shortmonths = [
        'jan', 'feb', 'mrt', 'apr', 'mei', 'jun',
        'jul', 'aug', 'sep', 'okt', 'nov', 'dec',
        'jan', 'feb', 'mar', 'apr', 'may', 'jun',
        'jul', 'aug', 'sep', 'oct', 'nov', 'dec'
        ]

    # Month abbrevs are only different march, may and october.

    pat = r'(\d{1,2})\s?[-\/]?\s?(\w{3})\s?[-\/]?\s?(\d{4})'

    q = re.match(pat, date)
    if q:
        year = q.group(3)
        day = int(q.group(1))
        month = shortmonths.index(q.group(2).lower()) % 12 + 1
        return u'{}-{:02d}-{:02d}'.format(year, month, day)
    else:
        # just return input, date fields may be empty
        return date

The regex match parses date month and year may not be pretty, but it works and it's easy to expand for other patterns. Likewise for the month number lookup with index, which is more concise than a chain of if, elif to match month strings to numbers.

Instead of the code between if q: and else:, I also had this, which uses datetime:

year = int(q.group(3))
day = int(q.group(1))
month = shortmonths.index(q.group(2).lower()) % 12 + 1
d = datetime.datetime(year, month, day)
return u'{:%YY-%m-%d}'.format(d)

This works too, but I think the first version is more to-the-point.

I'm always striving for optimal Pythonicness, but I'm by no means a seasoned coder.

Please comment.

Neat stuff.

Suggestions:

Changing shortmonths to a dictionary.  This will allow for a pair between numerical months and alphabetical months.  No need to repeat 'jan' for example, as you have it now.

Pythonic: unpack month, year, day in a one liner.

Use datetime's strftime to format dates...makes life easier in case you want to change the format down the road.

import re
import datetime

def ISOdate(date):

    month_d = {'01': 'jan',
               '02': 'feb',
               '03': ['mar', 'mrt'],
               '04': 'apr',
               '05': ['may', 'mei'],
               '06': 'jun',
               '07': 'jul',
               '08': 'aug',
               '09': 'sep',
               '10': ['oct', 'okt'],
               '11': 'nov',
               '12': 'dec'
               }

     pat = r'(\d{1,2})\s?[-\/]?\s?(\w{3})\s?[-\/]?\s?(\d{4})'
     q = re.match(pat, date)

     if q:
         day, month, year = [q.group(idx+1) for idx in range(3)]
         if month.isalpha(): # change from letters to numbers
             month = [k for k, v in month_d.items() if month in v][0]
         out_date = datetime.date(int(year), int(month), int(day))
         return datetime.datetime.strftime(out_date, '%Y-%m-%d')

     else:
         return date


Performance Optimization of n:1-mapping I am filtering a List (childList) based on the contents of another List (parentList).

The parentList can contain 0-n entities where items of the childList are referenced by an ID (btw. ID is in this case a string, you could also call it "Name" or whatever you like). I want to filter the childList in a way that the result is all elements that have at least one entry in the parentList.

The obvious solution is:

List<childItem> resultList = new List<childItem>();

foreach(var childItem in childList){
    foreach(var parentItem in parentList){
        if(childItem.ID.Equals(parentItem.ChildID,StringComparison.InvariantCultureIgnoreCase){
            resultList.Add(childItem); //found entity
            break; // search for next childItem
        }
}

What I have done by now is:

// Group by childID and create List with all relevant IDs
var groupedParentList = parentList.AsParallel().GroupBy(p => p.ChildID).Select(group => group.First()).ToList();

// Filter childList on groupedParentList
var result = childList.AsParallel().Where(child => groupedParentList.AsParallel().Count(parent => parent.childID.Equals(child.ID, StringComparison.InvariantCultureIgnoreCase)) > 0).ToList();

But it's lasting rather long. The parentList contains about 75k entries and the childList about 4.5k entries. I just can't believe it actually takes time - imo this should be done in almost no time?

Maybe I am on the wrong track or am I missing some obvious performance-stopper?

Btw.: I have chosen the names parent- and childList just as an example. It's actaully no parent-child-relation.

Additional Question: Could it be faster to direcly check if the parentList contains the childList-Item? Something a lá:

var test = childList.RemoveWhere(child => !parentList.Contains(item => item.childID.Equals(child.ID));

So I would just skip the grouping? Any suggestions/ideas?

First red flag; you're using Count(predicate) > 0. The Count() overload that takes a predicate must iterate over all elements of the parent enumerable to determine the correct count, and you're telling it to do so for each element of the child list. You simply want to know if at least one of those elements matches; for that, the Any() overload which accepts the same parameter will be faster, because it will quit as soon as it finds the first element that matches (unfortunately the worst-case of a match not existing will still require a full scan of the enumerable).

You're also parallelizing parallelization. While parallel is good, threads waiting on threads waiting on threads can easily result in an algorithm that is bound by the ThreadPool's ability to spin up new threads (the default is a 250ms wait for each new thread after a predetermined number of "readily-available" threads have been scheduled). So, after creating, let's say 10 threads for this algorithm, most of which will be waiting on sub-threads, the ThreadPool will only create four new threads per second. Let's say the TPL begind the PLinq library thinks that one thread per 100 elements of both collections is necessary. On a collection of 4500 elements, for each of which a comparison of 4500 more elements is necessary, the ThreadPool could schedule two thousand worker threads to execute. After 10 of them are created near-instantaneously, only four more per second will be created as long as at least 10 are running, meaning this algorithm will take up to 500 seconds just to schedule all the necessary threads. The more "instantaneous" threads you tell the ThreadPool to spin up before queueing them, the more cache-thrashing you'll do forcing the CPU to juggle all these threads.

First step to improving this: Instead of grouping, select a list of distinct child IDs from the parent list. This process will produce strings instead of groups of larger ParentItems, which should hopefully reduce the amount of "heap-thrashing" required to generate the groups only to reduce it down to a much smaller collection. You only ever need the child IDs from the parent list anyway.

// Group by childID and create List with all relevant IDs
var childrenOfParents = parentList.AsParallel()
   .Select(p => p.childId)
   .Distinct()
   .OrderBy(x=>x)
   .ToList();

Then, filter the child list based on the ID being in the derived list of child IDs:

// Filter childList on groupedParentList
var result = childList.AsParallel()
   .Where(child => childrenOfParents
      .Any(cid=>cid.Equals(child.ID, StringComparison.InvariantCultureIgnoreCase)))
   .ToList();

I do not know about the culture-specific comparison behavior, but if it's at all possible to compare the strings verbatim (even ToLower()ed), it would likely be preferable performance-wise.

Other things that may help include building custom extension methods that make certain assumptions:

Naive grouping or deduping (Distinct()) of an enumerable is an N2-complexity operation (compare each element to a list of already-processed elements and remove if it exists). This is the least efficient part of the first query. Deduping of a list sorted by the same projection that must be de-duped is linear (compare each element to the previous one and remove if equal). You will be able to dedupe the list of child IDs more efficiently than PLinq's default implementation if you know the list is sorted by the same projection (in this case the identity projection) that you must dedupe.

public IEnumerable<TObj> SortAndDedupe<TObj, TProj>(this IEnumerable<TObj> input, Func<TObj, TProj> projection)
{
    sortedInput = input.OrderBy(projection);

    TProj last;
    var init = false; //default(T) might be valid
    foreach(var element in sortedInput)
    {
       if(!init || !projection(element).Equals(last))
       {
          init = true;
          last = projection(element);
          yield return element;
       }
    }
}

...

// Your first query is now an NlogN operation
var childrenOfParents = parentList.AsParallel()
   .Select(p => p.childId)
   .SortAndDedupe(s=>s)
   .ToList();

Naive searching for elements that match a predicate is linear, which makes the second query N2-complexity. In your case, with a sorted list of childID strings being searched for a value that is equal to a passed projection, you can use a logarithmic binary search:

public bool IsInSortedList(this T toFind, List<T> collection)
    where T: IComparable<T>
{
    var max = collection.Count - 1;
    var min = 0;
    var idx = (max-min)/2;

    while(max >= min)
    {
       if(collection[idx].CompareTo(toFind) == 0) return true;
       else if(collection[idx].CompareTo(toFind) < 0) //search right
          min = idx+1;
       else if(collection[idx].CompareTo(toFind) > 0) //search left
          max = idx-1;

       idx = (max-min)/2 + min;
    }

    return false;
}

...

//This query is now an NlogN as well
var result = childList.AsParallel()
  .Where(child => child.IsInSortedList(childrenOfParents)
  .ToList();


Sort and Search Algorithms I'm new to programming, but want to build up a good habit of code review so that I can develop best practice. It's been couple of weeks I'm coding in C and my knowledge, so far, is data types, conditions, arrays, loop and, very minimal, pointers.

I'm creating multiple search and sorting algorithms - not only to understand them better, but also solidifying my understanding of concepts.

#include <stdio.h>
#include <cs50.h>

void swap(int *x, int *);
int midval(int x, int y);

int main(void)
{
    // taking count for characters
    printf("How many characters are you entering? ");
    int count = get_int();

    // creating the array
    int array[count];

    // taking string as an input and converting them to integers
    for (int i=0; i<count; i++)
    {
        printf("Please enter the character number %i: ", (i+1));
        array[i] = atoi(get_string());
    }

    //sorting - bubble sort
    int j=count-1;
    do
    {
        for(int i=0; i<j; i++)
        {
            if (array[i] > array[i+1])
            {
                swap(&array[i], &array[i+1]);
            }
        }
        j--;
    }
    while (j!=0);

    //binary search
    printf("Please enter the character you want to search: ");
    int search = atoi(get_string());
    int i=0;

    do
    {
        if (search == array[midval(i, count-1)])
        {
            printf("The character exists in the list.\n");
            return 0;
        }
        else if (search > array[midval(i, count-1)])
        {
            i = midval(i, count-1) + 1;
        }
        else if (search < array[midval(i,count-1)])
        {
            count = midval(i, count-1);
        }
    }
    while (count != i);

    printf("The character does not exist in the list.\n");

}

void swap(int *x, int *y)
{
    int temp[1];
    temp[0] = *x;
    *x = *y;
    *y = temp[0];
}

int midval(int x, int y)
{
    int z = (x+y)/2;
    return z;
}

Great work overall implementing these algorithms! I really liked that you extracted some of the common pieces into methods, and even a header file! Also it's great that you're attempting these algorithms on your own, before looking at standard implementations.

For the Bubble Sort portion

The do-while is generally less readable than using a for-loop, and is preferably only used when it's very necessary - for example, if you need to ensure at least one execution of the loop.

For the Binary Search portion

A more standard-used naming convention for the variables used in binary search are:

i: low (indicating the start of the search segment)
count: high (the end of the search segment)
midval(): mid (the middle value you compute once, and compare to in a given iteration)
search: target (the value you are searching for)

You are repeating the call to the midval function several times in the code. Instead, you can compute this value once at the start of the loop, and use the computed value later on.

General comments

As you did with some parts of your code, you should also extract out separate functions for Bubble Sort and Binary Search. This not only improves reusability, but also makes your code more readable.
It will be very helpful as you learn more to look for standard implementations of different algorithms to solidify your understanding and style (after you try to implement them yourself of course!). Generally you want to write code that's readable to other people, which is easier to do when you use standard practices.


PHP TCP socket server I created a PHP script which will be running 24/7 as a TCP socket server and will send data from the DB back to clients. The code is working, and so far, I see no problems.

However, I can only test it with one client. The info I need is, what will happen when there will be 100+ clients? Can any of you see any weaknesses in the code? Can I test it somehow?

include('db_con.php');
include('db_functions.php');
error_reporting(E_ALL);

// Allow the script to hang around waiting for connections
set_time_limit(0);

// Turn on implicit output flushing so we see what we're getting as it comes in
ob_implicit_flush();

$address = '10.4.1.6';
$port = 4950;

$failed = 0;

try {
    // Create a socket (AF_INET = IPV4, SOCK_STREAM=TCP stream)
    $failed = 1;
    $sock = socket_create(AF_INET, SOCK_STREAM, SOL_TCP);

    // Bind connection to IP/port
    $failed = 2;
    socket_bind($sock, $address, $port);

    $failed = 3;
    socket_listen($sock, SOMAXCONN);

    while (true) {
        // Accept connection
        $failed = 4;
        $client = socket_accept($sock);

        $failed = 6;
        $rec = socket_read($client, 1024);
        $id_d = substr($rec, 0, 1);
        $id = intval(substr($rec, 1, 1));

        // Connect to database and get results
        $failed = 7;

        $failed = 8;
        $data = "ok\r\n";

        if (strcmp($id_d, 'o') == 0) {
            $in_O = returnResultsForTCP_O($db, $id);
            foreach ($in_O as $r) {
                $data .= 'o'
                        . $r['ring_time'] . ' '
                        . ((int) $r['duration'] < 100 ? '0' : '')
                        . $r['duration'] . ' '
                        . $r['bell_mode'] . '\r\n';
            }
        } else {
            $in_D = returnResultsForTCP_D($db, $id);
            $in_W = returnResultsForTCP_W($db, $id);
            foreach ($in_D as $r) {
                $data .= 'd'
                        . $r['ring_time'] . ' '
                        . ((int) $r['duration'] < 100 ? '0' : '')
                        . $r['duration'] . ' '
                        . $r['bell_mode'] . '\r\n';
            }

            foreach ($in_W as $r) {
                $data .= 'w'
                        . $r['ring_time'] . ' '
                        . ((int) $r['duration'] < 100 ? '0' : '')
                        . $r['duration'] . ' '
                        . $r['day'] . ' '
                        . $r['bell_mode'] . '\r\n';
            }
        }

        $failed = 5;
        socket_write($client, $data);
        socket_close($client);
    }
} catch (Exception $e) {
    if ($failed < 6) {
        $causes = array('create', 'bind', 'listen', 'accept', 'write');
        echo 'socket_' . $causes[$failed - 1] . '() failed. Reason: '
                . socket_strerror(socket_last_error($sock)) . '\n';
    } else {
        echo 'Failed at ' . $failed . '\n' . $e . '\n';
    }

    socket_close($sock);
}

The $failed part is just for testing; I'll remake it later (change it so the logs will be written to file - logging).

Here are a few things:

The loop only handles one connection at a time, which could cause some issues for other clients connecting in the future. If returnResultsForTCP_O is slow and blocking, that could even prevent other clients from working.

Two possible solutions include:

Use http://reactphp.org/ to handle the connections

Or if you would still like to handle the connections manually, you can accept many connections at the same time by adding them all to an array and using socket_select to get arrays of all the sockets ready to read from or write to:

http://php.net/manual/en/function.socket-select.php

You would still need to make the database calls non-blocking. Again, reactphp has some database connection libraries available for it. Otherwise you will have to find another way to access the database in a non-blocking way. Although it isn't nonblocking, you can make a secondary php process you open with proc_open and pass the request into. Then just set stream_set_blocking on the proc_open pipe file handles, and read from it in your main loop.


Fill missing dates into array of np.datetime Here is the problem:

Given an array of a non-continuous sequence of dates. Make it a
continuous sequence of dates, by filling in the missing dates.

# Input

dates = np.arange(np.datetime64('2018-02-01'), np.datetime64('2018-02-25'), 2)

print(dates)
#> ['2018-02-01' '2018-02-03' '2018-02-05' '2018-02-07' '2018-02-09'
#>  '2018-02-11' '2018-02-13' '2018-02-15' '2018-02-17' '2018-02-19'
#>  '2018-02-21' '2018-02-23']

And here is my solution:
import numpy as n
dates = np.arange(np.datetime64('2018-02-01'), np.datetime64('2018-02-25'), 2)
stride = (dates[1] - dates[0])
result = np.arange(np.datetime64(dates[0]), np.datetime64(dates[-1] + stride))
print(dates)
print(result)

Is there a better way to do this task?

As you will be able to see from your provided sample input, your code does not produce the intended result. Here is a minimal example:
dates = np.arange(np.datetime64('2018-02-01'), np.datetime64('2018-02-05'), 2)
stride = (dates[1] - dates[0])
result = np.arange(np.datetime64(dates[0]), np.datetime64(dates[-1] + stride))

print(dates)   # > ['2018-02-01' '2018-02-03']
print(result)  # > ['2018-02-01' '2018-02-02' '2018-02-03' '2018-02-04']

result includes 2018-02-04, which is outside of the range of dates. This is only the simplest case for which this code fails, it will also fail for input arrays that are not sorted in ascending order. The only case your code would cover correctly is an input array of continuous dates (sorted in ascending order). There are two central errors in your code:

We do not need stride
We want the output range to include everything up until and including the maximum value of our input array. Since the stop argument to numpy.arange is exclusive, we need the following:
stop:
maximum of input array + 1

last element of output array:
(maximum of input array + 1) - 1
= maximum of input array

Your current implementation does the following:
stop:
maximum of input array + stride

last element of output array:
(maximum of input array + stride) - 1

The larger the difference between dates[0] and dates[1] (= stride), the more undesired values will be contained at the end of our output array.

The maximum is not always the last element
So we need to access the input array's maximum value to get the correct stop argument for numpy.arange. This will only be the last element if the input array is sorted in ascending order or if we get rather lucky, both cases are not specified by the problem statement.

Correcting these two errors will give us this simple implementation:
def make_continuous_sorted(np_array):
    return np.arange(np.min(np_array), np.max(np_array) + 1)


Auto-click the sequence The following pascal script needs a bit of context for it's purpose to be understood. For those who don't give about purpose, feel free to skip ahead.

Background

A game I'm terribly fond of has a couple of levels which have nothing to do with the rest of the game. One of those is a puzzle: Alpha Sector::virgilw::Brain

When you open the game, it looks like this:

Turns out you need to connect all the 8 red dots to the emitter like this:

All dots must be clicked in a specific sequence. If a dot is clicked and allowed to switch, it will light up if not already lit up. If it was already lit up it will darken again. The sequence to light up the 8th dot looks like this:

1 2 1 3 1 2 1 4
1 2 1 3 1 2 1 5
1 2 1 3 1 2 1 4
1 2 1 3 1 2 1 6
1 2 1 3 1 2 1 4
1 2 1 3 1 2 1 5
1 2 1 3 1 2 1 4
1 2 1 3 1 2 1 7
1 2 1 3 1 2 1 4
1 2 1 3 1 2 1 5
1 2 1 3 1 2 1 4
1 2 1 3 1 2 1 6
1 2 1 3 1 2 1 4
1 2 1 3 1 2 1 5
1 2 1 3 1 2 1 4
1 2 1 3 1 2 1 8

The mathematically inclined can probably tell us what such a sequence is called, but it should be obvious to the rest of us that the first 7 characters are always the same and only the 8th varies.

Now, there's an achievement for lighting up all 8 dots within 60 seconds. Since it's already 128 clicks for lighting up the 8th one alone, I decided to automate the process using SCAR Divi (manual), an environment running Pascal scripts which allows you to automate keyboard presses and mouse movements.

Code

The resulting code is ugly, but I was mainly looking for fast code. This means it may not be up to par with Pascal-style and I took a short-cut wherever I thought it would influence the execution speed.

I'm looking for improvements on readability wherever this does not hurt the performance in any way. I've written a fair bit of Pascal but I've always been a little fuzzy on what did and didn't impact performance. General tips about this are welcome.

The most used procedure is MnC(x, y), short for Move'n'Click. It moves the mouse to whatever coordinate you give it, clicks once with the left mouse-button and halts in between to give the rest of the computer time to catch up. Or whatever it does in that time, but it breaks terribly if it doesn't wait long enough.

All values are hardcoded. I'm aware I can put them at the front of the program as global variables, but I wasn't sure whether the additional look-up would penalize performance. Current script manages to win in 21.86 seconds :-)

virgilwBrain.scar

Procedure MnC(x,y : integer);
begin
  MoveMouse(x,y);
  Wait(30);
  ClickMouse(x,y,false);
  Wait(29);
end;

Procedure hit1();
begin
  MnC(421,298);
end;

Procedure hit2();
begin
  MnC(470,311);
end;

Procedure hit3();
begin
  MnC(500,318);
end;

Procedure hit4();
begin
  MnC(539,325);
end;

Procedure hit5();
begin
  MnC(577,325);
end;

Procedure hit6();
begin
  MnC(611,318);
end;

Procedure hit7();
begin
  MnC(655,311);
end;

Procedure hit8();
begin
  MnC(694,298);
end;

Procedure line();
begin
  hit1;
  hit2;
  hit1;
  hit3;
  hit1;
  hit2;
  hit1;
end;

begin
  line;
  hit4;
  line;
  hit5;
  line;
  hit4;
  line;
  hit6;
  line;
  hit4;
  line;
  hit5;
  line;
  hit4;
  line;
  hit7;
  line;
  hit4;
  line;
  hit5;
  line;
  hit4;
  line;
  hit6;
  line;
  hit4;
  line;
  hit5;
  line;
  hit4;
  line;
  hit8;

  line;
  hit4;
  line;
  hit5;
  line;
  hit4;
  line;
  hit6;
  line;
  hit4;
  hit1;
  hit2;
end.

I've done similar things in the past, although never with a scripting language and I haven't used Pascal in a really long time, so this is more of an approach review than a code review.

The action happens in MnC:

Procedure MnC(x,y : integer);
begin
  MoveMouse(x,y);
  Wait(30);
  ClickMouse(x,y,false);
  Wait(29);
end;

Moving the Mouse

The first thing you do is MouseMove.  The question that immediately springs to mind is do you need to move the mouse?  Unless the program your are interacting with actually tracks mouse movements in an attempt to detect bots, you probably don't need to move it.  Most of the times in the programs I have written I have been able to just send mouse click events at the relevant coordinates.  The library you're using supports a SetMousePos method that will simply set the coordinates of the mouse.  However, as ClickMouse calls this under the hood, this might not work for you.  I'd try running the script with just two clicks and a longer wait between them to detect if the program really requires the movements or if it's only concerned by the clicks.

ClickMouse(421,298,false);
Wait(100);
ClickMouse(470,311,false);
Wait(100);

If it is only concerned by the clicks, then great, you only need to worry about clicking the mouse.  If the program does care about the movement then you might want to think about using the MoveMouseEx function, which supports an additional parameter MouseSpeed.  The default speed is 20, with larger numbers being faster so increasing the number should either make the function return faster, or allow you to wait for less time before clicking.  Depending on whether the movement is async, you may also find that you need to wait less time for shorter movements (i.e. the mouse will get from 8 to 7 faster than it will get from 8 to 1).

Clicking the mouse

After you click the mouse, you wait before doing the next thing.  You may be able to make it so that you don't need to wait as long by using playing with MouseClickEx and passing in different values for Interval which is how long the mouse button is held down for.  Alternately, depending on how your application works you may be able to detect that the mouse click has been recognised (because some pixels have changes from red to green for example).  The library appears to have a few functions that might support this FindColors or FindBitmap would appear to be good starting points.  You could then loop until the click has been recognised, rather than waiting.


Configuring an editor for various languages using Factory Method Pattern I need to know if my code is a valid implementation of the factory method pattern. Also, if the Configuration and Editor were interfaces would this still be a valid implementation of the Factory Pattern?

public abstract class Configuration {

    protected List<String> keyWords;

    abstract void getTheme();
    abstract String getKeyWords();
}

public abstract class Editor {

    public final void useEditor() {
        Configuration configuration = createConfiguration();
        configuration.getTheme();
        parse(configuration.getKeyWords());
        save();
    }

    //The Factory Method
    public abstract Configuration createConfiguration();

    public abstract void parse(String sourceCode);

    public void save() {
        System.out.println("The file was saved..");
    }
}

public class JavaConfiguration extends Configuration {
    public JavaConfiguration() {
        keyWords = new ArrayList<>();
        keyWords.add("class");
        keyWords.add("protected");
    }

    @Override
    void getTheme() {
        System.out.println("Applied Java Theme...");
    }

    @Override
    String getKeyWords() {
        return keyWords.toString();
    }
}

public class PythonConfiguration extends Configuration {

    public PythonConfiguration() {
        keyWords = new ArrayList<>();
        keyWords.add("def");
        keyWords.add("print");
    }

    @Override
    void getTheme() {
        System.out.println("Applied Python Theme...");
    }

    @Override
    String getKeyWords() {
        return keyWords.toString();
    }
}

public class JavaEditor extends Editor {
    @Override
    public Configuration createConfiguration() {
        return new JavaConfiguration();
    }

    @Override
    public void parse(String sourceCode) {
        System.out.println("Parsing Java code...");
    }
}

public class PythonEditor extends Editor {

    @Override
    public Configuration createConfiguration() {
        return new PythonConfiguration();
    }

    @Override
    public void parse(String sourceCode) {
        System.out.println("Parsing python code...");
    }
}

public class App {

    public static void main(String[] args) {
        Scanner sc = new Scanner(System.in);
        String language = sc.next();
        Editor editor = createEditor(language);
        editor.useEditor();
    }

    private static Editor createEditor(String language) {
        switch(language) {
            case "Java":
                return new JavaEditor();
            case "Python":
                return new PythonEditor();
            default:
                return null;
        }
    }
}

Edit: It seems that the Factory Method Pattern is not the appropriate design pattern to use in this case, as suggested by @200_success and @Timothy Truckle. A simple Editor class which receives a Configuration as a parameter should be enough.

I need to know if my code is a valid implementation of the factory method pattern.

No.

The reason is that the factory cannot be part of the objects to be created. I think you would be surprised to find a factory in the the hedge of your car, wouldn't you?

Generally spoken your approach fails the Separation of Concerns principle.

You have two responsibilities in your code that belong to a factory:

identifying the requested type.
creating the actual object.

Both should be in a separate factory class.

Also: your "factory" is producing the wrong type of objects because of your misconception of inheritance.

We employ inheritance when the child classes differ in behavior. But your Editor extensions only differ in configuration (as far as your code example is showing...).

So what your (yet to build) factory should do is creating the "language depended"  key word list and pass that into a new instance of the one and only Editor class.


PHP HTTP request class I would like to get you opinion on this class, which is used to send HTTP requests (at that moment only POST method).

It is working ok, but I would like to get response on maintainability, reusability, security, code formatting, etc...

Class HttpRequest{

    private $POST = 'POST';
    private $PUT = 'PUT';
    private $GET = 'GET';
    private $DELETE = 'DELETE';
    private $PATCH = 'PATCH';

    private $body;
    private $options;
    private $handle;
    private $httpCode;
    private $response;

    public function __construct(){}

    /**
     * send post request
     * @param url
     * @param header
     * @param options
     * @param body
     * @return json object
     */
    public function post($url, $header, $options, $body){
        if(!$this->handle || get_resource_type($this->handle) == "Unknown"){
            $this->handle = curl_init();
        }
        curl_setopt($this->handle, CURLOPT_URL, $url);
        curl_setopt($this->handle, CURLOPT_HTTPHEADER, $header);
        curl_setopt_array($this->handle, $options);
        // curl_setopt($this->handle, CURLOPT_SSL_VERIFYPEER, true);
        // curl_setopt($this->handle, CURLOPT_SSL_VERIFYHOST, 0);
        curl_setopt($this->handle, CURLOPT_RETURNTRANSFER, true);
        curl_setopt($this->handle, CURLOPT_CUSTOMREQUEST, $this->POST);
        curl_setopt($this->handle, CURLOPT_POSTFIELDS, $body);
        $this->response = curl_exec($this->handle);
        $this->httpCode = curl_getinfo($this->handle, CURLINFO_HTTP_CODE);
        curl_close($this->handle);
        return $this->response;
    }

    public function getResponse(){
        return $this->response;
    }

    public function getHttpCode(){
        return $this->httpCode;
    }

    /**
     * send get request
     * @param url
     * @param header
     * @param options
     */
    public function get($url,$header=array(), $options=array()){
        /**
         * @todo
         * implemets this method
         */
    }

    /**
     * send patch request
     */
    public function patch(){
        /**
         * @todo
         * implemets this method
         */
    }
    /**
     * send delete request
     */
    public function delete(){
        /**
         * @todo
         * implemets this method
         */
    }
    /**
     * send put request
     */
    public function put(){
        /**
         * @todo
         * implemets this method
         */
    }
}

?>

Use class constants to store values that will remain the same and are unchangeable

Your HTTP method verbs are better off set as class constants than class properties since they will remain the same and are unchangeable. (i.e. constant)

private $POST = 'POST';
private $PUT = 'PUT';
private $GET = 'GET';
private $DELETE = 'DELETE';
private $PATCH = 'PATCH';

becomes

const POST = 'POST';
const PUT = 'PUT';
const GET = 'GET';
const DELETE = 'DELETE';
const PATCH = 'PATCH';

You can then refer to them using self

self::POST

No need for an empty constructor

If your contractor doesn't have any code in it you could, and should omit it. It's just noise otherwise. You can always add it if the constructor definition changes.

Be sure to use docblock comments for your class properties

Not just class methods should have well written docblock comments, so should your class properties.

/**
 * @var string The body of the HTTP request
 */
private $body;

Follow PSR coding standards

The PSR coding standards exist to ensure a high level of technical interoperability between shared PHP code. They also ensure conformity for projects with multiple developers.

PSR-2  says that:

  Opening braces for classes MUST go on the next line, and closing braces MUST go on the next line after the body.
  and
  Opening braces for methods MUST go on the next line, and closing braces MUST go on the next line after the body.

Class HttpRequest{

becomes

Class HttpRequest
{

and

public function post($url, $header, $options, $body){

becomes

public function post($url, $header, $options, $body)
{

Use type hinting to variable types in class methods

From the manual:

  Type declarations allow functions to require that parameters are of a certain type at call time. If the given value is of the incorrect type, then an error is generated

To use your code as an example, you can declare that $url must be a string and $header and $options must be arrays. This will be enforced by PHP at run time and prevent obvious and non obvious errors from occurring.

public function get($url,$header=array(), $options=array()){

becomes

public function get(string $url, array $header=[], array $options=[])
{

(I used shortened array syntax [] versus the more verbose array() for brevity.)

Use return type declarations

Just like you can enforce that class method parameters are a certain data type, you can also enforce the value returned by that method is a certain data type. Since all of your methods are stubs I'll pretend your HttpRequestget() method returns a Boolean to use as an example.

public function get(string $url, array $header=[], array $options=[]): bool
{

Now my calling code will know that a Boolean, and only a Boolean value, will be returned by that method.

When doing comparisons use === whenever possible

Unlike == which compares values only, === compares both values and type. This strict comparison helps to avoid error, and attacks, that occur when PHP encounters a comparison of two variables of different types it will coerce one of the variables into the type of the other variable in order to do the comparison.

For example

1 == '1'  // true
1 === '1' // false

How much does this matter? It depends. If you get into a situation where you are getting numbers as strings but you are trying to use them as numbers, for something like sorting, you can get unexpected results if your check only checks value instead of type. And those of us who remember phpBB remember when it was subject to a slew of high profile vulnerabilities many of which were resolved simply by using a stricter comparison. So, yes, it matters.


A simple template engine to display value For a specific usage I made a very simple template engine in Python, to display value. The code:

import re

class Template(object):
    """A simple template engine support only the substitute of variable"""
    def __init__(
            self, delimiter_start='[', delimiter_end=']',
            identifier=r'^[^\d\W]\w*$'):
        """To initialize the template class.

        @param delimiter_start: To specify the delimiter start (default [)
        @type param: str

        @param delimiter_end: To specify the delimiter stop (default ])
        @type param: str

        @param identifier: Identifier for variable name (default ^[^\d\W]\w*$)
        @type param: str
        """
        self.delimiter_start = delimiter_start
        self.delimiter_end = delimiter_end
        self.identifier = re.compile(identifier, re.UNICODE)

    def substitute(self, string, template={}):
        """To substitute the string.

        @param string: The string to substitute
        @type string: str

        @param template: The template
        @type string: dict

        @return:  The string substitute
        @rtype :  str

        @raise ValueError:  Identifier is not valid
        """
        words = string.split(" ")

        for index, word in enumerate(words):
            if word.startswith(self.delimiter_start) \
                    and word.endswith(self.delimiter_end):
                        word_without_delimiter = word[1:-1]
                        result = re.match(
                            self.identifier, word_without_delimiter)

                        if result is None:
                            raise ValueError(
                                'The identifier is not a valid identifier')

                        if word_without_delimiter not in template:
                            continue

                        words[index] = '{value}'.format(
                            value=template[word_without_delimiter])

        return ' '.join(words)

And the test:

import unittest

from passport.passport import Template

class TemplateTestCase(unittest.TestCase):
    """To test the template"""
    def setUp(self):
        self.template = Template()

    def test_simple(self):
        string = "To try only [var] it's display correctly"
        result = self.template.substitute(string, {'var': 'if'})

        self.assertEqual("To try only if it's display correctly", result)

    def test_multiple_variable(self):
        string = "To try only [var] it's [other_var] correctly"
        result = self.template.substitute(string, {
            'var': 'if', 'other_var': 'display'})

        self.assertEqual("To try only if it's display correctly", result)

    def test_one_word(self):
        string = '[var]'
        result = self.template.substitute(string, {'var': 'Try'})

        self.assertEqual('Try', result)

    def test_without_template(self):
        string = 'Try without a template in string no substitute'
        result = self.template.substitute(string, {'var': 'Try'})

        self.assertEqual(string, result)

    def test_with_other_delimiter(self):
        template = Template(delimiter_start='{', delimiter_end='}')
        string = "To try only {var} it's display correctly"
        result = template.substitute(string, {'var': 'if'})

        self.assertEqual("To try only if it's display correctly", result)

    def test_key_is_not_present(self):
        string = "To try only [var] it's display correctly"
        result = self.template.substitute(string)

        self.assertEqual("To try only [var] it's display correctly", result)

    def test_with_invalid_identifier(self):
        string = '[42var]'

        with self.assertRaises(ValueError):
            self.template.substitute(string, {'42var': 'Try'})

New methods will be added soon, for example add some take a file and not a string.

1. Introduction

This code looks mostly fine to me. I make a bunch of points below, but it's a case of tightening things up rather than fixing mistakes.

2. Review

The Template class is misleadlingly named. Normally the term template refers to the string into which the parameters are substituted. But an instance of the Template class actually represents the settings controlling the substitution. A better name for the class would be TemplateProcessor.
Since the only thing you can do with a Template object is to call its substitute method, it would simplify things to use __call__ instead.
The parameter name template to the substitute function is misleadingly named: this isn't the template, but actually the parameters to be substituted into the template. A better name would be params.
It would be simpler to take the substitution parameters as keyword arguments rather than a dictionary. This would slightly simplify the callers because instead of writing:

processor(string, {'var': 'Try'})

you could write:

processor(string, var='Try')

The value of result is a match object and so match or m would be a better name.
The code takes four steps to match a word against the syntax rules:

if word.startswith(self.delimiter_start) \
        and word.endswith(self.delimiter_end):
            word_without_delimiter = word[1:-1]
            result = re.match(
                self.identifier, word_without_delimiter)

But this could all be done in a single step using a regular expression that matches the initial delimiter, the identifer or nonidentifier, and the final delimiter: that is,

\[(?:([^\d\W]\w*)|(.*))\]$

See the revised code below for how this regular expression can be built and used.
The error message ought to include the incorrect identifier, for assistance with debugging.
Instead of:

'{value}'.format(value=template[word_without_delimiter])

use the format function and write:

format(template[word_without_delimiter])

3. Revised code

(Docstrings omitted—they were fine in the original.)

class TemplateProcessor(object):
    def __init__(self, delimiter_start='[', delimiter_end=']',
                 identifier=r'[^\d\W]\w*'):
        self.identifier_re = re.compile(
            '{}(?:({})|(.*)){}$'.format(re.escape(delimiter_start),
                                        identifier,
                                        re.escape(delimiter_end)),
            re.UNICODE)

    def __call__(self, string, **params):
        words = string.split(' ')
        for i, word in enumerate(words):
            m = self.identifier_re.match(word)
            if not m:
                continue
            identifier, nonidentifier = m.groups()
            if identifier is None:
                raise ValueError("{!r} is not a valid identifier"
                                 .format(nonidentifier))
            elif identifier in params:
                words[i] = format(params[identifier])
        return ' '.join(words)


Work carousel efficiency This seems like an awful lot of code and I'm guessing can be condensed and made more efficient somehow. Can anyone suggest this to me at all?

// Add the active class to the first element
$('.indexWorkDetailsInner:first').addClass('active');

// Next Button Click
$('.indexWorkRight').click(function() {

    // Move all except the active element to the left
    $('.indexWorkDetailsInner').each(function() {
        if($(this).hasClass('active')) { }
            else {
                $(this).css({'left':'-450px'});
            }
    });

    // Animate the active element off the screen
    $('.indexWorkDetailsInner.active').stop().animate({'left' : '450px'}, 1000, function () {

        $(this).removeClass('active').css({'left':'-450px'}); // Remove the active class, and move element back to the left
        $(this).next('.indexWorkDetailsInner').animate({'left' : '0px'}, 1000).addClass('active'); // Animate the next element in to view
        $('.indexWorkDetailsInner:last').after($('.indexWorkDetailsInner:first')); // Move the last element to after the first

    });

});

// Previous Button Click
$('.indexWorkLeft').click(function() {

    // Move all except the active element to the right
    $('.indexWorkDetailsInner').each(function() {
        if($(this).hasClass('active')) { }
            else {
                $(this).css({'left':'450px'});
            }
    });

    // Animate the active element off the screen
    $('.indexWorkDetailsInner.active').stop().animate({'left' : '-450px'}, 1000, function () {

    $(this).removeClass('active').css({'left':'450px'}); // Remove the active class, and move element back to the right
    $('.indexWorkDetailsInner:first').before($('.indexWorkDetailsInner:last')); // Move the first element before the last item
    $(this).prev('.indexWorkDetailsInner').animate({'left' : '0px'}, 1000).addClass('active'); // Animate the previous element in to view

    });

});

// Add the active class to the first element
$('.indexMonitorWork img:first').addClass('active');

// Next Button Click
$('.indexWorkRight').click(function() {

    // Move all except the active element to the left
    $('.indexMonitorWork img').each(function() {
        if($(this).hasClass('active')) { }
            else {
                $(this).css({'left':'-370px'});
            }
    });

    // Animate the active element off the screen
    $('.indexMonitorWork img.active').stop().animate({'left' : '370px'}, 1000, function () {

        $(this).removeClass('active').css({'left':'-370px'}); // Remove the active class, and move element back to the left
        $(this).next('.indexMonitorWork img').animate({'left' : '0px'}, 1000).addClass('active'); // Animate the next element in to view
        $('.indexMonitorWork img:last').after($('.indexMonitorWork img:first')); // Move the last element to after the first

    });

});

// Previous Button Click
$('.indexWorkLeft').click(function() {

    // Move all except the active element to the right
    $('.indexMonitorWork img').each(function() {
        if($(this).hasClass('active')) { }
            else {
                $(this).css({'left':'370px'});
            }
    });

    // Animate the active element off the screen
    $('.indexMonitorWork img.active').stop().animate({'left' : '-370px'}, 1000, function () {

    $(this).removeClass('active').css({'left':'370px'}); // Remove the active class, and move element back to the right
    $('.indexMonitorWork img:first').before($('.indexMonitorWork img:last')); // Move the first element before the last item
    $(this).prev('.indexMonitorWork img').animate({'left' : '0px'}, 1000).addClass('active'); // Animate the previous element in to view

    });

});

It is code for a work carousel. The first two functions move the text left and right. The bottom function moves the image screenshot.

With help from @Ingo I got it down to the following, which I think is better.

var innerWork = $('div.indexWorkDetailsInner');
var innerImg = $('div.indexMonitorWork img');

// Add the active class to the first element
innerWork.first().addClass('active');
innerImg.first().addClass('active');

function workAnimate(direction){

    var signOne, signTwo;

    if(direction === "right" ) { signOne = '-'; signTwo = ''; }
    if(direction === "left" ) { signOne = ''; signTwo = '-'; }

    innerWork.not('.active').css({'left': signOne + '450px'}); // Move the not active text to the left
    innerImg.not('.active').css({'left': signOne + '370px'}); // Move the not active images to the left

    // Animate the active element off the screen
    innerWork.filter('.active').stop().animate({'left' : signTwo + '450px'}, 1000, function () {

        $(this).removeClass('active').css({'left': signOne + '450px'}); // Remove the active class, and move element back to the left
        if(direction === "right" ) {
            $(this).next(innerWork).animate({'left' : '0px'}, 1000).addClass('active'); // Animate the next element in to view
            innerWork.filter(':last').after(innerWork.filter(':first')); // Move the last element to after the first
        } else {
            innerWork.filter(':first').before(innerWork.filter(':last')); // Move the first element before the last item
            $(this).prev(innerWork).animate({'left' : '0px'}, 1000).addClass('active'); // Animate the next element in to view
        }
    });

    // Animate the active element off the screen
    innerImg.filter('.active').stop().animate({'left' : signTwo + '370px'}, 1000, function () {

        $(this).removeClass('active').css({'left': signOne + '370px'}); // Remove the active class, and move element back to the left
        if(direction === "right" ) {
            $(this).next(innerImg).animate({'left' : '0px'}, 1000).addClass('active'); // Animate the next element in to view
            innerImg.filter(':last').after(innerImg.filter(':first')); // Move the last element to after the first
        } else {
            innerImg.filter(':first').before(innerImg.filter(':last')); // Move the first element before the last item
            $(this).prev(innerImg).animate({'left' : '0px'}, 1000).addClass('active'); // Animate the previous element in to view
        }
    });
}

// Next Button Click
$('div#indexWorkRight').on('click', function() {
    workAnimate('right');
});

// Previous Button Click
$('div#indexWorkLeft').on('click', function() {
    workAnimate('left');
});


Russian doll envelops Interview question from the interwebz

  You have a set of envelopes of different widths and heights.  One
  envelope can fit into another if and only if both the width and height
  of one envelope is greater than the width and height of the other
  envelope. What is the maximum number of envelopes can you russian
  doll?

My implementation:

# assuming no dups
def max_russian_doll(enve):
    if not enve: return 0
    enve.sort()
    max_global = 1
    for j in xrange(len(enve) - 1):
        max_local = 1
        for i in xrange(j, len(enve) - 1):
            if enve[i][1] < enve[i + 1][1] and enve[i][0] != enve[i + 1][0]: # @comment
                max_local += 1
        max_global = max(max_global, max_local)
    return max_global

envelopes = [(4,5), (6,7), (2,3)]
max_russian_doll(envelopes)

obviously this is \$O(n^2)\$. Right now I'm trying to figure out faster solution. Any tips?

Using LIS from Wikipedia.

def max_russian_doll(seq):
    if not seq: return 0
    seq.sort()
    M = [0] * (len(seq) + 1)
    L = 1
    for i in xrange(len(seq)):
        lo = 1
        hi = L
        while lo <= hi:
            mid = (hi - lo)/2 + lo
            if seq[M[mid]][1] < seq[i][1] and seq[M[mid]][0] < seq[i][0]:
                lo = mid + 1
            else:
                hi = mid - 1
        newL = lo
        if newL > L:
            M[newL] = i
            L = newL
    return L

print max_russian_doll([]) == 0
print max_russian_doll([(1,1)]) == 1
print max_russian_doll([(1,1), (1,1), (1,1)]) == 1
print max_russian_doll([(4,5), (4,6), (6,7), (2,3), (1,1)]) == 4 # e.g (1,1) -> (2,3) -> (4,5) -> (6,7)
print max_russian_doll([(4,5), (4,6), (6,7), (2,3), (1,1), (1,1)]) == 4
print max_russian_doll([(5,4), (6,4), (6,7), (2,3)]) == 3


Sieve of Sundaram for Project Euler 7: Python implementation slower than C++ and R A friend of mine recently started learning R, and she complained to me that R is very slow. She was working on Project Euler Problem 7, which asks for the value of the 10001st prime number. For comparison, I decided to write the exact same program in C++ (which I use most frequently), R (which I do not use regularly, but used for comparison), and Python (because I wanted to advise her to switch her learning from R to Python).

The program follows these steps:

Find the upper bound on the Nth prime.
Fill a prime number sieve (Sieve of Sundaram) large enough to capture the upper bound.
Count the primes in the prime number sieve until the Nth prime is found.

To my surprise, the Python solution was significantly slower than the R version. Here are the results from time for the three implementations:

C++: real 0.002s, user 0.001s, sys 0.001s
R: real 2.239s, user 2.065s, sys 0.033s
Python: real 13.588s, user 13.566s, sys 0.025s

I tried using numpy arrays instead of lists, but to little effect.

I would appreciate an explanation of why the Python implementation is so slow relative to the other two. I would also appreciate tips on how to use lists in R and Python more efficiently. Is there a better way to do this?

C++:

#include <cmath>
#include <iostream>
#include <vector>
using namespace std;

const int N = 10001;

int main()
{
  int max = floor(N*(log(N) + log(log(N))));
  vector<bool> marked(max/2, false);
  for (size_t i = 1; i < marked.size(); i++) {
    for (size_t j = 1; j <= i; j++) {
      size_t m = i + j + 2*i*j;
      if (m < marked.size()) {
        marked[m] = true;
      } else {
        break;
      }
    }
  }
  int count = 1;
  for (size_t m = 1; m < marked.size(); m++) {
    if (not marked[m]) count++;
    if (count == N) {
      cout << 2*m + 1 << endl;
      break;
    }
  }
  return 0;
}

R:

n <- 10001
max <- n*(log(n) + log(log(n)))
marked <- vector(mode="logical", length=max/2)
for (i in 1:length(marked)) {
  for (j in 1:i) {
    m <- i + j + 2*i*j
    if (m > length(marked)) break
    marked[m] <- T
  }
}
count <- 1
for (i in 1:length(marked)) {
  if (!marked[i]) count <- count + 1
  if (count == n) {
    print(2*i + 1)
    break
  }
}

Python:

from math import log

n = 10001
max = n*(log(n) + log(log(n)))
marked = [False] * int(max/2)
for i in range(1, len(marked)):
    for j in range(1, i + 1):
        m = i + j + 2*i*j
        if m > len(marked):
            break
        marked[m] = True
count = 0
for i in range(0, len(marked)):
  if not marked[i]:
      count += 1
  if count == n:
      print 2*i + 1
      break

TL;DR: Just use PyPy; it gets you to about 10x the time of C++. If you really want to use CPython, a lot of clever optimizations (not algorithm changes) gets you as fast as PyPy and then using Numpy gets you close to C++ (2x the time).

The first thing of note is that your Python code is broken:

    if m > len(marked):
        break

Remember that Python is 0-indexed. What about when m == len(marked)?

So that's the first thing to fix. Going from the top, I'd do the
translation so:

from math import log

n = 10001                              n <- 10001
maximum = n * (log(n) + log(log(n)))   max <- n*(log(n) + log(log(n)))
marked = [False] * int(maximum // 2)   marked <- vector(mode="logical", length=max/2)
for i in range(1, len(marked)+1):      for (i in 1:length(marked)) {
    for j in range(1, i+1):              for (j in 1:i) {
        m = i + j + 2*i*j                  m <- i + j + 2*i*j
        if m > len(marked): break          if (m > length(marked)) break
        marked[m-1] = True                 marked[m] <- T
                                         }
                                       }
count = 1                              count <- 1
for i in range(1, len(marked)+1):      for (i in 1:length(marked)) {
    if not marked[i-1]: count += 1       if (!marked[i]) count <- count + 1
    if count == n:                       if (count == n) {
        print(2*i + 1)                     print(2*i + 1)
        break                              break
                                         }
                                       }

This is as direct a mapping as possible; instead of changing the comparison
I just shifted the index when indexing. This isn't idiomatic, but it's direct.
It's largely the same as your code, but it's correct. This matters when we go to
larger N, where your code fails. It's also Python 3 compatible simply by using
print with brackets.

Let's time them:

$ time Rscript r.r
[1] 104743
Rscript r.r  1.59s user 0.78s system 99% cpu 2.375 total

$ time python2 p.py
104743
python2 p.py  12.88s user 0.00s system 100% cpu 12.873 total

$ time python3 p.py
104743
python3 p.py  0.16s user 0.00s system 98% cpu 0.163 total

$ # A faster, very compatible Python interpreter
$ time pypy p.py
104743
pypy p.py  0.04s user 0.01s system 98% cpu 0.051 total

$ time pypy3 p.py
104743
pypy3 p.py  0.05s user 0.01s system 99% cpu 0.054 total

So there we have it. Python is over an order of magnitude faster on 75% of
interpreters, and under an order of magnitude slower in the worst case...

But why is it so slow with python2? line_profiler is a good utility:

import line_profiler
profiler = line_profiler.LineProfiler()

def main():
    # ... the code ...

profiler.enable()
profiler.add_function(main)
main()
profiler.print_stats()

Giving:

Line #      Hits         Time  Per Hit   % Time  Line Contents
==============================================================
     4                                           def main():
     5         1          271    271.0      0.0      from math import log
     6
     7         1            1      1.0      0.0      n = 10001
     8         1           22     22.0      0.0      maximum = n * (log(n) + log(log(n)))
     9         1          373    373.0      0.0      marked = [False] * int(maximum // 2)
    10     57160        37122      0.6      0.3      for i in range(1, len(marked)+1):
    11    188523      8444200     44.8     62.9          for j in range(1, i+1):
    12    188355       118260      0.6      0.9              m = i + j + 2*i*j
    13    188355      4672922     24.8     34.8              if m > len(marked): break
    14    131364        75962      0.6      0.6              marked[m-1] = True
    15
    16
    17         1            1      1.0      0.0      count = 1
    18     52371        25618      0.5      0.2      for i in range(1, len(marked)+1):
    19     52371        28393      0.5      0.2          if not marked[i-1]: count += 1
    20     52371        26003      0.5      0.2          if count == n:
    21         1           60     60.0      0.0              print(2*i + 1)
    22         1          329    329.0      0.0              break

So our most likely offending line is:

11    188523      8444200     44.8     62.9          for j in range(1, i+1):

On Python 2 there is both range and xrange. Using

for i in range(...):

will generate a list of numbers and then loop over them, whereas

for i in xrange(...):

will just increment i as wanted each time. This is often much faster. So let's add:

# Set range to xrange on Python 2
try:
    range = xrange
except NameError:
    pass

to the top of the script and retime:

$ time python2 p.py
104743
python2 p.py  0.09s user 0.00s system 96% cpu 0.090 total

$ time python3 p.py
104743
python3 p.py  0.16s user 0.00s system 99% cpu 0.161 total

$ time pypy p.py
104743
pypy p.py  0.04s user 0.00s system 93% cpu 0.050 total

$ time pypy3 p.py
104743
pypy3 p.py  0.04s user 0.01s system 99% cpu 0.054 total

Yup. CPython is much improved.

So how would we make the code good? First you can transform the i+j+2ij
calculation to directly use a stepping range:

step = 2*i + 1
for m in range(i+step-1, i+i*step, step):
    if m >= len(marked):
      break

    marked[m] = True

Then you can just do a slice assignment:

for i in range(1, len(marked)+1):
    step = 2*i + 1
    marked[i+step-1::step] = [True] * len(marked[i+step-1::step])

and you can immediately see that you can stop when...

    if i+step-1 >= len(marked):
        break

This leads to calculating the bound in the for loop itself.
step = 2*i + 1, so i+step-1 = i+(2*i+1)-1 = 3*i, so we should
stop before len(marked)//3:

for i in range(1, len(marked)//3):
    step = 2*i + 1
    marked[i+step-1::step] = [True] * len(marked[i+step-1::step])

The count part can use enumerate:

count = 1
for i, mark in enumerate(marked, 1):
    if not mark:
        count += 1

    if count == n:
        print(2*i + 1)
        break

I suggest inverting the True/False in the array to make this simpler,
and then you can just do count += mark. Why? Well, it lets us use
itertools.accumulate on Python 3:

for i, count in enumerate(accumulate(marked), 1):
    if count+1 == n:
        print(2*i + 1)
        break

You might also think len(marked[i+step-1::step]) is wasteful, since
it's generating a new array each time: it is!
It's still really fast, but Jaime's solution is better... except it
involves risky math steps. How could we get the best of both?

Well, remember how in Python 3 range is a fancy "pseudo-list" and
doesn't build a real list? Python 3's range can be sliced, too.
We just need to build a range the same length as the list and slice
that instead. This is actually slower on CPython (the one invoked
as python3) for normal-sized lists because CPython's list slices
are really fast, but on PyPy the overheads of the range version
are optimized out:

from itertools import accumulate
from math import log

n = 1000001
maximum = n * (log(n) + log(log(n)))
marked = [True] * int(maximum // 2)

slice_proxy = range(len(marked))
for i in range(1, len(marked)//3):
    step = 2*i + 1
    marked[i+step-1::step] = [False] * len(slice_proxy[i+step-1::step])

for i, count in enumerate(accumulate(marked), 1):
    if count+1 == n:
        print(2*i + 1)
        break

(Note the much larger n)

Times without slice_proxy:

$ time python3 p.py
15485867
python3 p.py  5.57s user 0.01s system 100% cpu 5.576 total

$ time pypy3 p.py
15485867
pypy3 p.py  3.58s user 0.08s system 100% cpu 3.654 total

Times with slice_proxy:

$ time python3 p.py
15485867
python3 p.py  5.94s user 0.02s system 100% cpu 5.949 total

$ time pypy3 p.py
15485867
pypy3 p.py  2.52s user 0.06s system 100% cpu 2.585 total

Then just wrap the whole thing in a main function, because local
variables are faster than global ones:

from itertools import accumulate
from math import log

def main():
    # ...

main()

Times:

$ time python3 p.py
15485867
python3 p.py  4.74s user 0.02s system 100% cpu 4.754 total

$ time pypy3 p.py
15485867
pypy3 p.py  2.40s user 0.06s system 100% cpu 2.456 total

But wait! We dropped the end point!

for i in range(1, len(marked)//3):
    step  = 2*i + 1
    start = i+step-1
    stop  = 2*i*(i+1)
    marked[start:stop:step] = [False] * len(slice_proxy[start:stop:step])

This is important on PyPy; on CPython it's about the same speed:

$ time python3 p.py
15485867
python3 p.py  4.88s user 0.01s system 100% cpu 4.890 total

$ time pypy3 p.py
15485867
pypy3 p.py  1.44s user 0.02s system 99% cpu 1.461 total

It's not really faster on CPython because the number of things you do tends to be more important
than what you do, simply from interpreter overhead.

We know it's way faster on CPython, but how does this compare to the original version on PyPy
(once stuck inside a main function)?

from math import log

def main():
    n = 1000001
    maximum = n * (log(n) + log(log(n)))
    marked = [False] * int(maximum // 2)

    for i in range(1, len(marked)+1):
        for j in range(1, i+1):
            m = i + j + 2*i*j

            if m > len(marked):
              break

            marked[m-1] = True

    count = 1
    for i in range(1, len(marked)+1):
        if not marked[i-1]:
            count += 1

        if count == n:
            print(2*i + 1)
            break

main()

Times:

$ time pypy p.py
15485867
pypy p.py  0.83s user 0.02s system 99% cpu 0.851 total

$ time pypy3 p.py
15485867
pypy3 p.py  0.78s user 0.01s system 99% cpu 0.790 total

Interestingly, the original is significantly faster on PyPy. This implies
that unless you want a code base optimized for both CPython and PyPy, some
of these optimizations (like slice_proxy) just aren't needed.

But, eh, what if this isn't fast enough?

Eh?

Well, let's quickly draw the things we're setting. T are the parts we set. . is the part we would set
if not for the :end: optimization.

index:  1  2  3  4  5  6  7  8  9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33
  i=1         T        .        .        .        .        .        .        .        .        .
  i=2                  T              T              .              .              .              .
  i=3                           T                    T                    T                    .
  i=4                                    T                          T                          T
  i=5                                             T                                T
  i=6                                                      T                                      T
  i=7                                                               T
  i=8                                                                        T
  i=9                                                                                T
  i=10                                                                                T
  i=11                                                                                T

We're doing i=1 then i=2 then i=3, etc. From i² ≈ len(marked) onwards, though, there aren't many
things set each run. We end up moving back-and-forth in the array a lot, spending more time "seeking" in
memory than actually setting items. Consider, though, this decomposition:

index:  1  2  3  4  5  6  7  8  9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33
  i=1         1        2        3        4        5        6        7        8        9       10
  i=2                  1              2              3              4              5              6
  i=3                           1                    2                    3                    4
  i=4                                    1                          2                          3
  i=5                                             1                                2
  i=6                                                      1                                      2
  i=7                                                               1
  i=8                                                                        1
  i=9                                                                                1
  i=10                                                                                1
  i=11                                                                                1

For low numbers, 1, 2, 3, ... this hits a lot of those later values. We can then split the work into
an upper part:

  i=1         T        .        .        .        .        .        .        .        .        .
  i=2                  T              T              .              .              .              .
  i=3                           T                    T                    T                    .
  i=4                                    T                          T                          T
  i=5                                             T                                T

and a lower part:

  i=6                                                      1                                      2
  i=7                                                               1
  i=8                                                                        1
  i=9                                                                                1
  i=10                                                                                1
  i=11                                                                                1

This sets the same values, but does so in a different order.

Here's the code:

from itertools import accumulate
from math import log

def main():
    n = 1000001
    maximum = n * (log(n) + log(log(n)))
    marked = [True] * int(maximum // 2)

    slice_proxy = range(len(marked))
    for i in range(1, int(len(marked)**0.5)):
        step  = 2*i + 1
        start = 3*i
        stop  = 2*i*(i+1)
        marked[start:stop:step] = [False] * len(slice_proxy[start:stop:step])

    i += 1
    start_step  = 2*i + 1
    start_start = 3*i
    step = 3
    for start in range(start_start, len(marked), start_step):
        marked[start::step] = [False] * len(slice_proxy[start::step])
        step += 2

    for i, count in enumerate(accumulate(marked), 1):
        if count+1 == n:
            print(2*i + 1)
            break

main()

How fast is it?

$ time python3 p.py
15485867
python3 p.py  1.53s user 0.01s system 99% cpu 1.538 total

$ time pypy3 p.py
15485867
pypy3 p.py  1.26s user 0.06s system 100% cpu 1.319 total

CPython improves a lot with this, although PyPy doesn't really improve. This makes sense: you do a lot
fewer operations but each operation is more complicated. Since CPython has a really fast implementation
under the hood but a really slow interpreter, fewer operations is important. PyPy, however, is bound
by the size of the operations themselves.

Of course, for PyPy you'd want to modify the "original" code:

from math import log

def main():
    n = 1000001
    maximum = n * (log(n) + log(log(n)))
    marked = [True] * int(maximum // 2)

    for i in range(1, int(len(marked)**0.5)):
        step  = 2*i + 1
        start = 3*i
        stop  = 2*i*(i+1)

        if stop > len(marked):
            stop = len(marked)

        for m in range(start, stop, step):
            marked[m] = False

    i += 1
    start_step  = 2*i + 1
    start_start = 3*i
    step = 3
    for start in range(start_start, len(marked), start_step):
        for m in range(start, len(marked), step):
            marked[m] = False

        step += 2

    count = 1
    for i in range(1, len(marked)+1):
        if marked[i-1]:
            count += 1

        if count == n:
            print(2*i + 1)
            break

main()

Which gives

$ time pypy p.py
15485867
pypy p.py  0.65s user 0.02s system 99% cpu 0.672 total

$ time pypy3 p.py
15485867
pypy3 p.py  0.61s user 0.01s system 99% cpu 0.625 total

The difference is obviously much less, but it exists. This is probably
due to improvements in memory locality. It actually turns out that just
terminating the range with len(marked)//3 gets you almost to this
point, so the optimization, IMHO, isn't worth it.

Sadly, CPython still trails the first version on PyPy. Janne Karila gave a good suggestion in the comments for speeding up CPython. The key part is to use itertools (or filter) to apply a filter and islice to do the counting. The large advantage comes from the iteration and counting being done inside C routines.

I went with compress from Python 3.1 and Janne Karila's use of islice to get the wanted element:

primes = compress(range(3, int(maximum), 2), marked)
print(next(islice(primes, n-2, None)))

Timings are really good:

$ time python3 p.py
15485867
python3 p.py  0.58s user 0.01s system 99% cpu 0.598 total

This is (negligibly) faster than PyPy!

But, eh, but... but...

What if this isn't fast enough?

Eh?

Well, that's what Numpy is for. Numpy is Python's fast array library.

from math import log
import numpy

def main():

    n = 1000001
    maximum = n * (log(n) + log(log(n)))
    marked = numpy.ones(int(maximum // 2), dtype=bool)

    for i in range(1, int(len(marked)**0.5)):
        step  = 2*i + 1
        start = 3*i
        stop  = 2*i*(i+1)
        marked[start:stop:step] = False

    i += 1
    start_step  = 2*i + 1
    start_start = 3*i
    step = 3
    for start in range(start_start, len(marked), start_step):
        marked[start::step] = False
        step += 2

    accumulated = numpy.add.accumulate(marked)
    [where] = numpy.where(accumulated+1 == n)
    print(2 * where[0] + 3)

main()

This also works on Python 2. Unfortunately PyPy doesn't (yet)
have an easy Numpy port. Even so, the times:

$ time python3 p.py
15485867
python3 p.py  0.23s user 0.02s system 99% cpu 0.252 total

$ time python2 p.py
15485867
python2 p.py  0.17s user 0.02s system 98% cpu 0.196 total

C++ with this N takes:

$ g++ -O3 c.cpp -o c; time ./c
15485867
./c  0.06s user 0.00s system 96% cpu 0.065 total

Neat, but we're still not done. One slow part is

accumulated = numpy.add.accumulate(marked)
[where] = numpy.where(accumulated+1 == n)
print(2 * where[0] + 3)

It turns out that Numpy's count_nonzero is really fast, so let's try capitalizing on that with what amounts to a binary search. There are similarities to Quickselect:

# I can't think of a good name...
def count_nonzero_sorted(array, count):
    # We always assume the count is achievable
    if array.shape[0] == 1:
        return 0

    # Split the array in two
    midpoint = len(array) // 2
    first_half = array[:midpoint]
    second_half = array[midpoint:]

    first_counts = numpy.count_nonzero(first_half)

    # If the counts in the first half is greater
    # than the wanted index, check that half
    if first_counts >= count:
        return count_nonzero_sorted(first_half, count)

    # else check the other half and offset the result
    return midpoint + count_nonzero_sorted(second_half, count-first_counts)

This gives

# compress sieve
where = count_nonzero_sorted(marked, n-1)
print(2 * where + 3)

and timings of just

$ time python2 p.py
15485867
python2 p.py  0.10s user 0.01s system 97% cpu 0.116 total

$ time python3 p.py
15485867
python3 p.py  0.16s user 0.01s system 99% cpu 0.167 total

On Python 2 that's half the speed of C++! Vectorization is a wonderful thing.

It would be interesting how many of these optimizations can apply to R.
A preliminary guess puts the time required for the original R code to run as about 7 hours.

What should you take away from this?

PyPy is really, really fast. The original code (after the
bug is fixed) takes only 0.7-0.9 seconds, or a tenth the speed of C++.

PyPy thus ends up, by my guess, about 10000 times as fast
as the implementation of R that I have available (for this task).
Optimizing PyPy is possible but it's hard; the optimized PyPy was
only a little faster than the original PyPy code at 0.6-0.7s.
This is because the JIT gives you most optimizations for free.
CPython has sharp corners (range vs xrange) but Python 3 takes
some of these problems away by default. Once you handle this, CPython
is an order of magnitude faster than R on this code.
CPython can be forced to go fast if you know what you are doing.
When forced it became a touch faster than PyPy.
CPython is ridiculously fast when Numpy is used; getting to half the speed
of C++.

I thought to check how a traditional sieve compares against this:

import sys
from itertools import compress, islice
from math import log

def main():
    n = 1000001
    maximum = int(n * (log(n) + log(log(n))))
    maxidx = maximum//2
    sieve = [True] * maxidx # 2, 3, 5, 7... might be prime

    j = 0
    for i in range(1, int(maxidx**0.5)+1):
        j += 4*i

        if sieve[i]:
            step = 2*i + 1
            sieve[j::step] = [False] * -(-(maxidx-j) // step)

    # compress sieve
    print(next(islice(compress(range(1, maximum, 2), sieve), n-1, None)))

main()

and the PyPy version:

import sys
from itertools import compress, islice
from math import log

def main():
    n = 1000001
    maximum = int(n * (log(n) + log(log(n))))
    maxidx = maximum//2
    sieve = [True] * maxidx # 2, 3, 5, 7... might be prime

    j = 0
    for i in range(1, int(maxidx**0.5)+1):
        j += 4*i

        if sieve[i]:
            for k in range(j, maxidx, 2*i + 1):
                sieve[k] = False

    # compress sieve
    count = 1
    for i in range(1, maxidx):
        count += sieve[i]

        if count == n:
            print(2*i + 1)
            break

main()

and the Numpy version:

import numpy
from math import log

def count_nonzero_sorted(array, count):
    # We always assume the count is achievable
    if array.shape[0] == 1:
        return 0

    midpoint = len(array) // 2
    first_half = array[:midpoint]
    second_half = array[midpoint:]

    first_counts = numpy.count_nonzero(first_half)

    if first_counts >= count:
        return count_nonzero_sorted(first_half, count)

    return midpoint + count_nonzero_sorted(second_half, count-first_counts)

def main():
    n = 1000001
    maximum = int(n * (log(n) + log(log(n))))
    maxidx = maximum//2
    sieve = numpy.ones(maxidx, dtype=bool) # 2, 3, 5, 7... might be prime

    j = 0
    for i in range(1, int(maxidx**0.5)+1):
        j += 4*i

        if sieve[i]:
            sieve[j::2*i + 1] = False

    # compress sieve
    where = count_nonzero_sorted(sieve, n)
    print(2 * where + 1)

main()

Timings:

$ time python3 p.py
15485867
python3 p.py  0.40s user 0.01s system 99% cpu 0.415 total

$ time pypy3 p.py
15485867
pypy3 p.py  0.36s user 0.01s system 99% cpu 0.368 total

$ # Numpy versions
$ time python2 p.py
15485867
python2 p.py  0.07s user 0.01s system 97% cpu 0.075 total

$ time python3 p.py
15485867
python3 p.py  0.12s user 0.00s system 99% cpu 0.124 total

Sorry, Sieve of Sundaram, you're not that great after all. :P

And, heck, that Python 2 version is way too fast.

Note: I made a mistake previously in comparing vector<bool> to vector<int>; I should have compared it to vector<uint8_t> instead. It turns out that the difference is actually relatively minor (30-40%) and although significant it isn't nearly the factor-of-4 difference from before.


Finding the most common section from the visible cells in a collection view My goal is to determine, of all of the currently visible cells in a collection view, which section has the most visible cells.

Start by getting the index paths for the visible cells:

let visible = collectionView.indexPathsForVisibleItems // [IndexPath]

Originally I used an NSCountedSet followed by mapping and sorting.

let counted = NSCountedSet(array: visible.map { $0.section })
let section = counted.allObjects.map { ($0, counted.count(for: $0)) }.sorted { $0.1 > $1.1 }.first?.0 as? Int

This works but I don't like NSCountedSet because it deals with Any.

I then replaced the use of NSCountedSet with reduce by reducing the visible array into a dictionary and then using a similar map and sort.

let counted = visible.reduce([Int: Int]()) { (result, path) -> [Int: Int] in
    var updated = result
    updated[path.section, default: 0] += 1
    return updated
}
let section = counted.keys.map { ($0, counted[$0]!) }.sorted { $0.1 > $1.1 }.first?.0

This works as well but I'm hoping there are ways to improve this.

Two main questions:

Can the reduce closure be improved? Is there a better way to return the updated dictionary where the keys are section and the values are the count?
Once I have the dictionary of sections and counts, is there a better way to find the section with the highest count? Is there something better than mapping to a tuple, sorting those tuples, and finally grabbing the first one?

BTW - If there is a tie for highest count, I don't care which of those sections is returned.

Re 1: You can take advantage of reduce(into:_:) which was introduced in Swift 4 precisely for this purpose:

This method is preferred over reduce(_:_:) for efficiency when the result is a copy-on-write type, for example an Array or a Dictionary.

See also SE-0171 Reduce with inout:

Motivation
The current version of reduce needs to make copies of the result type for each element in the sequence. The proposed version can eliminate the need to make copies (when the inout is optimized away).

In your case:
 let counted = visible.reduce(into: [Int: Int]()) { (result, path) in
    result[path.section, default: 0] += 1
}

The compiler can also infer the type of the initial accumulator automatically if the closure consists only of a single statement:
let counted = visible.reduce(into: [:]) { (result, path) in
    result[path.section, default: 0] += 1
}

Re 2: A dictionary is a collection of key/value pairs, therefore it can be sorted directly, without mapping each key to a tuple first:
let section = counted.sorted(by: { $0.value > $1.value }).first?.key

Even better, use max(by:) to find the dictionary entry with the maximal value:
let section = counted.max(by: { $0.value < $1.value })?.key

This is shorter and eliminates the intermediate arrays and the dictionary lookup. It is more efficient than sorting an array because only a single traversal of the collection is done. And even if the forced unwrapping is safe in your case, it is nice not to have it.

One could also make this a generic method for sequences
extension Sequence {
    func mostFrequent<T: Hashable>(by map: (Element) -> T) -> T? {
        let counted = reduce(into: [:]) { (result, elem) in
            result[map(elem), default: 0] += 1
        }
        return counted.max(by: { $0.value < $1.value })?.key
    }
}

which is then used as
let section = collectionView.indexPathsForVisibleItems.mostFrequent(by: { $0.section })


Extract contiguous elements from an array in JavaScript I'm trying to extract contiguous elements from an outcome array and print their equivalent elements in a reference array. This is my test case:

it('should split into fragments', function() {
  let ranges = getRanges({
    reference: ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h'],
    outcome: [true, true, false, false, true, false, false, true]
  })
  expect(ranges).to.eql([
    ['a b', true],
    ['c d', false],
    ['e', true],
    ['f g', false],
    ['h', true]
  ])
})

This works, but I'm wondering if there is a more elegant way of doing this:

function getRanges({reference, outcome}) {
  function withinBoundsAndSameOutcome(i, currentOutcome) {
    return i < outcome.length && outcome[i] == currentOutcome
  }
  let ranges = []
  for (let i = 0; i < outcome.length; i++) {
    let range = [reference[i]]
    let currentOutcome = outcome[i]
    while (withinBoundsAndSameOutcome(i + 1, currentOutcome)) {
      i += 1
      range.push(reference[i])
    }
    ranges.push([range.join(' '), currentOutcome])
  }
  return ranges
}

Remarks on the provided implementation:

The code is already well structured and easy to follow along, even though it is not immediately clear what you are actually trying to accomplish.

The chosen function, argument and variable names could be more concise. Example:

ranges.push([range.join(' '), currentOutcome]);

Here, you push a 'range' and an 'outcome' into an array of 'ranges'. So 'range' actually represents two different things which should probably have their own names:

A subsequence of reference
A pair consisting of a joined 'range' as per 1. and an 'outcome'.

Moving the while loop condition to its own function with somewhat self-documenting name withinBoundsAndSameOutcome is an interesting idea. However, it disrupts the visual parsing of your code as the reader has to jump back to the definition in order to gain a complete understanding of that loop condition. As the condition itself is pretty standard, I'd prefer inlining it.
Terminating statements with semicolons instead of relying on JavaScript's semicolon auto-insertion is the norm as it avoids some ambiguities.

Alternative implementation:

  I'm wondering if there is a more elegant way of doing this:

  I'm trying to extract contiguous elements from an outcome array and
  print their equivalent elements in a reference array.

So you already identified two tasks:

Partition an array into intervals of equal values
Map those partitions to reference values

I recommend to follow this 'natural' structure and solve these two tasks independently.

Generator functions are a great tool for untangling your seemingly interwoven code and pushing individual functionality into their own reusable, testable functions. They are especially useful when you are not interested in storing intermediate results but rather want to directly process (e.g. print) them:

// Partition array into intervals of equal values:
function* partitions(array, start = 0, next = start + 1) {
  while (next < array.length && array[start] === array[next]) next++;
  if (next <= array.length) {
    yield [start, next];
    yield* partitions(array, next);
  }
}

// Return reference strings for partitions of outcome:
function* ranges({reference, outcome}) {
  for (let [start, end] of partitions(outcome)) {
    yield [reference.slice(start, end).join(' '), outcome[start]];
  }
}

// Example:
let data = {
  reference: ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h'],
  outcome: [true, true, false, false, true, false, false, true]
};

for (let range of ranges(data)) {
  console.log(range);
}

But even without generators, your implementation could benefit from the suggested split.


Loading the stylesheets and scripts on my website I have made a simple website and I have tried to reduce load times as much as possible. All the content that is loaded is necessary.

Is there anything thing I can do to my code, such as structurally, to make it load even faster?

Side Note

I do not want to minify the HTML, as I want it to remain legible and I am not concerned about the content in the body, I am only concerned about the content in the  and the scripts at the bottom of the .

The website is hosted at http://discourse.rf.gd.

<!DOCTYPE html>

        <meta charset="UTF-8">
        <meta name="viewport" content="width=device-width, height=device-height, initial-scale=1, minimum-scale=1">
        <link rel="manifest" href="./manifest.webmanifest">
        <link rel="stylesheet" href="./src/stylesheet.css">
        <link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Muli:400,400i,700,700i,900,900i&display=swap">
        <title>Discourse</title>

        <fieldset>
            <legend>Login</legend>
            Username:
            <input type="text" name="username" id="username">

            Password:
            <input type="password" name="password" id="password">

            <input type="button" value="login" onclick="login()">

        </fieldset>

While your code is technically review-able, any review won't help you with your request, because the limited example page doesn't provide a realistic use case for your site, mostly because it doesn't have a style sheet and doesn't actually use any of the fonts, so they won't be downloaded.

Why do want to have the HTML "remain legible"? (Minifying HTML usually doesn't help much with performance anyway, but it's a strange reason for not wanting to do it).

There are automatized performance tests out there, which should be your first stop. For example, Google Chrome's Lighthouse: https://googlechrome.github.io/lighthouse/viewer/?psiurl=http%3A%2F%2Fdiscourse.rf.gd%2F%3Fi%3D1&strategy=mobile&category=performance&category=accessibility&category=best-practices&category=seo&category=pwa&utm_source=lh-chrome-ext

One big resource hog is jQuery. Unless you are using every single feature it provides multiple times, you shouldn't be using it, if performance is important to you.

Make sure you are actually using the fonts and corresponding style sheets.

Some remarks to the HTML:

Don't use on* attributes to add JavaScript event listeners. You generally want to keep HTML, CSS and JavaScript separate. Event handlers are part of the JavaScript and don't belong in the HTML, just like you don't use style attributes to assign CSS in the HTML. See https://developer.mozilla.org/en-US/docs/Learn/JavaScript/Building_blocks/Events#Inline_event_handlers_%E2%80%94_dont_use_these
Don't (only) use JavaScript to handle the login. Use a normal HTTP POST from the form.
Don't use <br>s for line breaks or spacing. They are for logical line breaks such as lines in a postal address or in the stanzas of poems. To separate the input fields of a form wrap each label/field  pair in a block element, such as a div, or put the input inside the label and give the label display: block.