Stop Watch application I've done the difficult dailyprogrammer challenge #2:

  Your mission is to create a stopwatch program. this program should have start, stop, and lap options, and it should write out to a file to be viewed later.

I wanted to see from you guys if what I did was correct. I'm looking for shortcuts or better coding practices I could have missed. This is a Windows Form Application.

namespace _Hard0002_StopWatch {
public partial class StopWatchForm : Form {
    StateEnum state;
    Stopwatch stopWatch;
    TimeSpan timeSpan;
    string elapsedTime;
    List<string> times = new List<string>();

    public StopWatchForm() {
        InitializeComponent();
        state = StateEnum.Stoped;
    }

    private void StartAndLapButton_Click(object sender, EventArgs e) {
        if (state == StateEnum.Stoped) {
            stopWatch = Stopwatch.StartNew();
            state = StateEnum.Started;
            ChangeButtonText(StartAndLapButton, "Lap");
            ChangeFormTittle("Started");
            timer1.Enabled = true;
        } else if (state == StateEnum.Started) {
            times.Add(elapsedTime);
        }
    }

    private void StopButton_Click(object sender, EventArgs e) {
        if (state == StateEnum.Started) {
            ChangeButtonText(StartAndLapButton, "Start");
            ChangeFormTittle("Stop Watch");
            stopWatch.Stop();
            times.Add(elapsedTime);
            state = StateEnum.Stoped;
            timer1.Enabled = false;
        }
    }

    private void FileButton_Click(object sender, EventArgs e) {
        saveFileDialog1 = new SaveFileDialog();
        saveFileDialog1.Filter = "txt files (*.txt)|*.txt|All files (*.*)|*.*";
        saveFileDialog1.FilterIndex = 2;
        saveFileDialog1.RestoreDirectory = true;

        if (times.Count > -1) {
            if (saveFileDialog1.ShowDialog() == DialogResult.OK) {
                string path = saveFileDialog1.FileName;
                System.IO.File.WriteAllLines(path + ".txt", times);
                times.Clear();
            }
        }
    }

    void ChangeButtonText(Button button, string text) {
        button.Text = text;
    }

    private void ChangeFormTittle(string text) {
        this.Text = text;
    }

    private void timer1_Tick(object sender, EventArgs e) {
        timeSpan = stopWatch.Elapsed;
        elapsedTime = String.Format("{0:00}:{1:00}:{2:00}", timeSpan.Hours, timeSpan.Minutes, timeSpan.Seconds);
        TimeKeeperText.Text = elapsedTime;
    }

    ///Your mission is to create a stopwatch program.
    ///this program should have start, stop, and lap options,
    ///and it should write out to a file to be viewed later.
    ///

    ///Nouns:
    ///Start
    ///lap
    ///stop
    ///file (txt)
    ///
    ///Verbs:
    ///Keeps time
    ///Writes file
    ///
}
}

There is a lot to review here, so I'll just cover this handler:

private void FileButton_Click(object sender, EventArgs e) {
    saveFileDialog1 = new SaveFileDialog();
    saveFileDialog1.Filter = "txt files (*.txt)|*.txt|All files (*.*)|*.*";
    saveFileDialog1.FilterIndex = 2;
    saveFileDialog1.RestoreDirectory = true;

    if (times.Count > -1) {
        if (saveFileDialog1.ShowDialog() == DialogResult.OK) {
            string path = saveFileDialog1.FileName;
            System.IO.File.WriteAllLines(path + ".txt", times);
            times.Clear();
        }
    }
}

It's very easy to cram all of a functionality's logic inside an event handler in WinForms; it's a little bit harder to handle a Click event and not have the Single Responsibility Principle take a beating. This handler:

Configures and displays a SaveFileDialog.
Writes to the specified file.
Clears the times list.

These are 3 distinct operations, and yet they all happen in the FileButton's Click handler.

One of my favorite refactorings, is the Extract Method functionality - in Visual Studio, there's a convenient keyboard shortcut for it: just select the code you want to extract, and then Ctrl+R, M.

I would select the first part, extract it into its own function, and get this:

private string PromptSaveAsFileName()
{
    var dialog = new SaveFileDialog
        {
            Filter = "txt files (*.txt)|*.txt|All files (*.*)|*.*",
            FilterIndex = 2,
            RestoreDirectory = true
        };

    if (dialog.ShowDialog() == DialogResult.OK)
    {
        return dialog.FileName;
    }

    return null;
}

This would leave the handler, with a few little more changes, like this:

private void FileButton_Click(object sender, EventArgs e) {

    if (times.Count == -1)
    {
        return;
    }

    var fileName = PromptSaveAsFileName();
    if (fileName == null)
    {
        return;
    }

    System.IO.File.WriteAllLines(fileName + ".txt", times);
    times.Clear();
}

It's still more than I'd like to see, but it's already much better. A few question marks before going any further:

Under what circumstances would times.Count be -1?

   Never. The Count property of an empty list will return 0.

Since we have a function whose role is to return our SaveAsFileName, who's responsibility is it to ensure the file has a .txt extension?

   The function should return the full file name, ready to be used, including its extension.

So. I'd do whatever I have to do, to make the handler look like this:

private void FileButton_Click(object sender, EventArgs e)
{
    if (!times.Any())
    {
        return;
    }

    var fileName = PromptSaveAsFileName();
    if (fileName == null)
    {
        return;
    }

    WriteResultsToFile(fileName);
    times.Clear();
}

Notice a couple more points:

{ scope opening brace is placed on the next line. This is C# convention; opening the scope on the same line is Java convention. I like that your brace style is consistent though, so that's just a minor point.
Return early, and don't work for nothing: the above code will not set up a new SaveFileDialog and then return if there's nothing to save.
Avoid extraneous nesting: it's often possible to reduce nesting by simply reversing a condition.


ConstantQ transformation method I've got this transform method with a triple-nested loop. The Generate methods do their own caching so they are fast. K will have a worst-case value of 150. N will have a worst-case value of about 20000. It's that 200002 that's killing me. It looks to me like the innermost loop could be moved out, but would you store every possible sum?

    public static void Transform(IList<float> x, int offset, int sampleRate, int binsPerOctave,
        double minFrequency, double maxFrequency, out double[] magnitudes, out double maxMagnitude)
    {
        var Q = 1.0 / (Math.Pow(2.0, 1.0 / binsPerOctave) - 1.0);
        var K = (int)Math.Ceiling(Math.Log(maxFrequency / minFrequency, 2.0) * binsPerOctave);

        magnitudes = new double[K];
        var maxN = (int)Math.Round(Q * sampleRate / minFrequency);
        var halfN = -maxN / 2;
        double[] wcs = GenerateWindowConstants(maxN);
        maxMagnitude = double.NegativeInfinity;

        for(int k = 0; k < K; k++)
        {
            var N = (int)Math.Round(Q * sampleRate / (minFrequency * Math.Pow(2.0, (double)k / binsPerOctave)));
            Complex[] piqs = GeneratePiQConstants(N, Q);

            var maxMag = double.NegativeInfinity;
            for(int i = 0; i < N; i++)
            {
                Complex sum = new Complex();
                for(int n = 0; n < maxN; n++)
                {
                    var index = n + offset + halfN;
                    index = Math.Min(x.Count - 1, index);
                    index = Math.Max(0, index);
                    sum += (wcs[n] * x[index]) * piqs[(n + i) % N];
                }
                sum /= maxN;
                maxMag = Math.Max(maxMag, sum.Magnitude);
            }
            magnitudes[k] = maxMag;
            maxMagnitude = Math.Max(maxMagnitude, maxMag);
        }
    }

It looks like based on the 2 inner loops where you are calculating maxMag you could refactor the code to utilize Parallel.ForEach() so you can start to use multiple cores for the math.  Just put all of the individual maxMags into a concurrent dictionary.  When your done, run your aggregation methods.  You'll have to decide whether it is better as an array that gets copied, or whether to refactor them into a concurrent collection type.  I hope this helps.

Edit/Update:

Per your comment relating to using all cores "However, the best improvement this would bring on typical hardware would be 8x. 8x isn't quite going to be enough for my application." (found under cgilmeanu's answer)

If utilizing all of the computer resources and taking an 8-fold gain won't get anywhere close, then ...

Answer = you need to horizontally scale across multiple computers until you reach the point where you are satisfied with the "timed" outcome.

Going to c, c++, or .net-under-the-hood should help make gains for you.  However, commonly, the programming involved results in something complicated, hard to understand, and more extensive documentation.  This can make it tough to be nimble and make changes.  Also, if you can assume that the computational load/iterations may increase in the future, your extensive efforts in extremely fine tuning this item may be all for nothing since it was internally just vertical scaling by optimization.


Longest running string I came across this problem in a career cup. Below I have the questions and the Java solution. I was wondering if anyone could scrutinize my code and tell me if there are any improvements, or better coding practices that I should know, as I am working on being able to produce proficient code.

Problem statement:

  Write a program to accept a nonempty string of alphanumeric characters. Define a run as a consecutive sequence of a single character. For example, aaaa is a run of length \$4\$. The program will print the longest run in the given string. If there is no single longest run, then you may print any of those runs whose length is at least as long as all other runs in the string.

Example input: a

Example output: a

Example input: aab

Example output: aa

Example input: abbbbbcc

Example output: bbbbb

Example input: aabbccdd

Example output: aa

Solution:

public class StringRunCount {

    public static void main(String[] args) {
        // TODO Auto-generated method stub
        System.out.println(maxRun("aaabbbbbbbbcccccc"));
    }

    public static int maxRun(String testString) {
        int len = testString.length();
        String substr = "";
        for (int i = 0; i < len;) {
            char current = testString.charAt(i);
            String tempSubstr = "" + current;
            while ((i + 1) < len && (current == testString.charAt(i + 1))) {
                tempSubstr = tempSubstr + testString.charAt(++i);
            }

            if (tempSubstr.length() >= substr.length()) {
                substr = tempSubstr;

            }
            i += 1;
        }
        System.out.println(substr);
        return substr.length();
    }

}

You should keep 4 variables:

The start index of the longest run
The end index (or length) of the longest run
The start index of the current run
The iterating index

For performance, you'd likely also store the char of the current run in a variable.

When done iterating, return substring of longest run.


Web scraping using proxy in vba I've written a script in vba to parse movie names and year from a torrent site. The script is doing just awesome. Although the scraper is leaving no room for complaint, I'm still dubious about how the way I've set the proxy is accurate. Moreover, I've set two proxies in my scraper. Btw, I went through few codes where the proxy has been set using .setProxy 2,"xxxxxxx" whereas I used .setProxy 1 in my below code because the earlier one was throwing errors. Once again, the code is working flawlessly. Thanks in advance.

Here is what I've written:

Sub Torrent_Scraper()
    Dim http As New ServerXMLHTTP60, html As New HTMLDocument
    Dim post As HTMLHtmlElement

    With http
        .Open "GET", "https://yts.ag/browse-movies", False
        .setRequestHeader "Content-Type", "text/html; charset=utf-8"
        .setRequestHeader "User-Agent", "Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.113 Safari/537.36"
        .setProxy 1, "61.233.25.166:80", "46.101.27.218:8118"
        .send
        html.body.innerHTML = .responseText
    End With

    For Each post In html.getElementsByClassName("browse-movie-bottom")
        With post.getElementsByClassName("browse-movie-title")
            x = x + 1: Cells(x, 1) = .item(0).innerText
        End With
        With post.getElementsByClassName("browse-movie-year")
            If .Length Then Cells(x, 2) = .item(0).innerText
        End With
    Next post
End Sub

Here are my suggestions:

Use Application.ScreenUpdating = False to speed up the execution of the code. See here for more details.

Declare elements where you need them. While your code is short (which is great, btw), it's a good habit to get into.

Use Option Explicit and declare that x variable appropriately. x right now is of type Variant but you're only using it like a Long, so you should declare it as Long. Also, change x to a more meaningful name like row.

Don't put two executable statements in the same line.

Having x = x + 1: Cells... is clever, but it may confuse other people later on (or even yourself if you don't touch this code for months).

Set a reference to the active sheet if that's the sheet you intend to use. When you use Cells(...) you are referring to the active sheet. However, while you are looping through your items in the For loop, if you were to click on another sheet, then your data will get messed up when you could avoid this issue pretty easily. In the code below, Cells(...) becomes activeWs.Cells(...).

Personally, I would only use With ... End With if I had many statements I needed it for (like the first use you have of it). In the second and third uses of With, I would just remove it. I would also use If ... Then ... End If so the If statement isn't so long.

I can tell that you wanted to make the code as short as possible, but sacrificing a few lines for clarity will be more beneficial in the long run, in my opinion.

Lastly, about If post.getElementsByClassName("browse-movie-year").Length Then, I would explicitly state what you really mean, which is that if the length if greater than 0. While this statement returns an array and I get that you're checking for an array length and it will always be >=0, it's best to be explicit for clarity.

EDIT: Thinking about this further, this code could be considered to do too much. If you were to follow the Single Responsibility Principle, then we could split out the work to other, private functions.

Here is my version of the refactored code (untested):

Option Explicit

Sub Torrent_Scraper()
    Application.ScreenUpdating = False

    Dim url as String
    Dim proxyServer As Variant
    Dim bypassList as Variant

    url = "https://yts.ag/browse-movies"
    proxyServer = "61.233.25.166:80"
    bypassList = "46.101.27.218:8118"

    Dim html As New HTMLDocument
    html.body.innerHTML = GetHttpResponseTextByProxy(url, proxyServer, bypassList)

    Call SetMovieData(html, ActiveSheet)

    Application.ScreenUpdating = True
End Sub

Private Function GetHttpResponseTextByProxy( _
    ByRef url as String, _
    ByRef proxyServer as Variant, _
    ByRef bypassList as Variant) As String

    Dim http As New ServerXMLHTTP60

    With http
        .Open "GET", url, False
        .setRequestHeader "Content-Type", "text/html; charset=utf-8"
        .setRequestHeader "User-Agent", "Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.113 Safari/537.36"
        .setProxy 1, proxyServer, bypassList
        .send
    End With

    GetHttpResponseTextByProxy = http.responseText
End Function

Private Sub SetMovieData(ByRef html as HTMLDocument, ByRef ws As Worksheet)
    Dim post As HTMLHtmlElement
    Dim row As Long

    For Each post In html.getElementsByClassName("browse-movie-bottom")
        row = row + 1
        ws.Cells(row, 1) = post.getElementsByClassName("browse-movie-title").item(0).innerText

        If post.getElementsByClassName("browse-movie-year").Length > 0 Then
            ws.Cells(row, 2) = post.getElementsByClassName("browse-movie-year").item(0).innerText
        End If
    Next post
End Sub


How to make my code faster and easier? This is my .cs part code where I am calling storeprocedure in LINQ to SQL:

var rr_j_cat = db.allcategories().ToList();//its store procedure calling thousands of rows

if (rr_j_cat.Count() != 0)
 {
   DataTable dt = new DataTable();//making dynamic datatable
   dt.Columns.Add("sub_id");  //dynamic columns
   dt.Columns.Add("sibheadername");  //dynamic columns
   foreach (var it in rr_j_cat) // will rotate thousands time which makes process talking more time
   {
      DataRow dr = dt.NewRow(); // dyanamic rows
      var rr_sel_cat = db.selcategories(Convert.ToInt32(it.sub_id)).ToList(); //another storeprocedure to check a count of data present in another data or table
      int count = Convert.ToInt32(rr_sel_cat.First().Column1);
      if (count != 0)
       {
         dr["sub_id"] = it.sub_id;
         dr["sibheadername"] = it.sibheadername + " (" + count + ")"; // adding count besides subheader name which you can see in below image on dance
         count =0;
        }
        else
        {
           dr["sub_id"] = it.sub_id;
            dr["sibheadername"] = it.sibheadername;
        }
                dt.Rows.Add(dr);
     }
     dd_j_area.DataSource = dt; // binding with dropdownlist
     dd_j_area.DataTextField = "sibheadername";
     dd_j_area.DataValueField = "sub_id";
     dd_j_area.DataBind();
     dd_j_area.Items.Insert(0, "");
}

It's working fine, but taking more time.

It will show as

The principal performance issues are on the database side (or maybe you could do some kind of caching or pre-loading), but your code could be made much clearer and shorter (there should be also some minor performance improvements):

string[] columnNames = new[] { "sub_id", "sibheadername" };

var categories = db.allcategories().ToList();

if (categories.Count == 0)
{
    return;
}

var dt = new DataTable();
dt.Columns.Add(columnNames[0]);
dt.Columns.Add(columnNames[1]);

foreach (var category in categories)
{
    var selectedCategories = db.selcategories(Convert.ToInt32(category.sub_id)).ToList();
    int count = Convert.ToInt32(selectedCategories.First().Column1);

    DataRow dr = dt.NewRow();
    dr[0] = category.sub_id;
    dr[1] = count != 0
                ? category.sibheadername + " (" + count + ")" // btw. this is FASTER (in this case) than a StringBuilder!
                : category.sibheadername;

    dt.Rows.Add(dr);
}

areaDropdown.DataSource = dt;
areaDropdown.DataTextField = columnNames[1];
areaDropdown.DataValueField = columnNames[0];
areaDropdown.DataBind();
areaDropdown.Items.Insert(0, "");


Network Send and Reply I have developed a TCP listener to receive messages from a client machine, take that string and put it in a db, then send back a response with some data relating to that string. First attempt at a tcp network stream send and reply, just need to see if this will result in any easily avoidable "hiccups" or incorrect data being returned.

private void ReceivePortMessages()
{
    try
    {
        _TcpListener.Start();
        Debug.Print(" >> Server Started");
        _TcpClient = _TcpListener.AcceptTcpClient();
        Debug.Print(" >> Accept connection from client");
        Boolean isProcessing = true;
        using (NetworkStream networkStream = _TcpClient.GetStream())
        {
            while (isProcessing)
            {
                try
                {
                    int receivingBufferSize = (int)_TcpClient.ReceiveBufferSize;
                    byte[] bytesFrom = new byte[receivingBufferSize];
                    networkStream.Read(bytesFrom, 0, receivingBufferSize);
                    Stopwatch sw = new Stopwatch();
                    sw.Start();
                    string dataFromClient = System.Text.Encoding.ASCII.GetString(bytesFrom);
                    dataFromClient = dataFromClient.Substring(0, dataFromClient.IndexOf("\0"));
                    if (dataFromClient != string.Empty)
                    {
                        XmlDocument xm = new XmlDocument();
                        xm.LoadXml(string.Format("<root>{0}</root>", dataFromClient));
                        XmlElement root = xm.DocumentElement;
                        string rootName = root.FirstChild.Name;
                        // Data is sent to be processed and sent back.
                        RouteInboundXML(rootName, dataFromClient, sw);
                    }
                }
                catch (ArgumentOutOfRangeException ex)
                {
                    Debug.Print("ReceivePortMessages: Remote client disconnected. " + ex.ToString());
                    _TcpClient.Close();
                    _TcpListener.Stop();
                    ErrorLog.Write("MotionIndustriesXmlProcessing", ex.ToString(), "ReceivePortMessages()");
                    return;
                }
                catch (Exception ex)
                {
                    Debug.Print("ReceivePortMessages: " + ex.ToString());
                    _TcpClient.Close();
                    _TcpListener.Stop();
                    ErrorLog.Write("MotionIndustriesXmlProcessing", ex.ToString(), "ReceivePortMessages()");
                    return;
                }
            }
            Debug.Print(" >> exit");
            isProcessing = false;
        }
    }
    catch (Exception ex)
    {
        Debug.Print("ReceivePortMessages: " + ex.ToString());
        ErrorLog.Write("MotionIndustriesXmlProcessing", ex.ToString(), "ReceivePortMessages()");
    }
    ReceivePortMessages();
}

private void RouteInboundXML(string requestType, string requestXML, Stopwatch sw)
{
    //
    // A bunch of other operations happen and then they call the SendReply() method.
    //
    SendReply();
}

private void SendReply(string reply)
{
    try
    {
        using (NetworkStream networkStream = _TcpClient.GetStream())
        {
            string serverResponse = reply;
            Byte[] sendBytes = Encoding.ASCII.GetBytes(serverResponse);
            networkStream.Write(sendBytes, 0, sendBytes.Length);
            networkStream.Flush();
            Debug.Print(" >> " + serverResponse);
        }
    }
    catch (ArgumentOutOfRangeException ex)
    {
        Debug.Print("SendReply: Remote client disconnected. " + ex.ToString());
        _TcpClient.Close();
        _TcpListener.Stop();
        return;
    }
}

I see you aren't using any sort of multithreading or asynchrony. That means that your server is an iterative one - i.e. it can only process one client request at a time. If a request takes long, subsequent requests may have to wait to be processed. Depending on what you're doing, this may be acceptable or not.

What I don't quite like with your implementation is the way you're dealing with incoming data. You're doing low-level buffer read operations, and you are then looking for the \0 to see where it ends, and then transforming it into a string.

The reason you're doing this is that if your buffer is 10 bytes long and the incoming data is 5 bytes long, then the buffer will look like this:

Hello\0\0\0\0\0

But that can lead to a number of problems. What if the incoming data is larger than the buffer? What if there's a \0 within the actual data itself?

There is no single way to do this, as this is a protocol design thing. Let's take a look at a few ways you could have done this.

1. Newline-delimited requests

If you know your data will never contain newlines, then you can end each request with a newline, and then easily use StreamReader.ReadLine() to process your data without all the low-level buffer details. Example:

        var server = new TcpListener(6969);
        server.Start();
        var client = server.AcceptTcpClient();
        var stream = client.GetStream();

        using (var reader = new StreamReader(stream))
        {
            string data = reader.ReadLine();

            // do something with the data...
        }

2. Counting bytes read

#1 above isn't always an option, e.g. if your data contains newlines.

An important observation here is that your data can arrive at the application in chunks - not necessarily as one long string as you might expect. So it is a good idea to likewise read it in chunks, similar to the following:

        var server = new TcpListener(6969);
        server.Start();
        var client = server.AcceptTcpClient();
        var stream = client.GetStream();

        using (var reader = new StreamReader(stream))
        {
            var sb = new StringBuilder();
            var buffer = new char[8192];
            int bytesRead = reader.Read(buffer, 0, buffer.Length);

            if (bytesRead > 0)
            {
                var buffer2 = new char[bytesRead];
                Array.Copy(buffer, buffer2, bytesRead);
                sb.Append(buffer2);

                // if sb meets some criteria, process the data...
            }
            else
                Console.WriteLine("Client disconnected!");
        }

Over here we're reading data into a buffer - not much different from what you were doing. But there are a few things we're doing differently:

We are reading in chunks. We aren't assuming that we get all the data at once; we're appending the partial data to a StringBuilder, and then doing something with it once any criteria are met (e.g. there is a terminator character, or a predefined number of bytes have been read).
We're copying into a secondary buffer. If we keep to the example I mentioned earlier where we had a 10-byte buffer but 5 incoming bytes, then we copy just those 5 bytes into a second, 5-byte buffer, to get rid of the extra padding (\0s).
We know how much data was read with each call to StreamReader.Read(), because it returns the number of bytes read. This allows us to both do #2 above and also to detect a disconnect.

3. Length-prefixed requests

In #2 above, I mentioned that you must check the data itself to see that you received a whole, integral request and that you can now start processing it. Some protocols do this using a terminator character (e.g. \n, which may or may not be a candidate for #1 above, or possibly other characters).

Another variant places the number of bytes at the beginning of the request, so that the receiving end knows how many to expect. For example, the agreed protocol would be that the first four characters are a number representing the data length, allowing a maximum of 9999 bytes. The actual data would look something like this:

38  Indiana Jones and the Fate of Atlantis

The code would be quite similar to #2 above, but you'd have to extract the length as soon as you have those four bytes available in your buffer, and then call Read() repeatedly until you have all the bytes you were expecting. At that point, you can process the data.

Encoding

I also noticed that you're using an ASCII encoding to convert your data between bytes and strings. This may be okay for some scenarios, but you'll have trouble if you need to deal with multilingual data. UTF8 may be more appropriate in such cases.


Showing and hiding element, making my code more useful I have just started learning about JavaScript and events. I am interested in learning the JavaScript and not jQuery until I am better at JavaScript.

I wrote this code that will show and hide my div because I basically want to be able to use this code in future web development projects but at the moment it will only apply to one element on page but what if I want to have a few pop up boxes. I understand I will have to have the element positioned absolute.

Can I also ask anyone that is good at JavaScript events to check my code to see if it is well written or if it could be improved? I am just a learning.

To make my questions clearer:

How would I make this code more usable for more than one element?
Is my code well written or does it look like a noob wrote it?

Click Me

SUPRISE

Item 1: I'd suggest getting rid of all global variables except tog.  They don't look like they are needed and just open you up for a potential conflict with other global variables.

Item 2: This line of code:

if (tog = 1 && clicked != menu) {

should probably be:

if (tog == 1 && clicked != menu) {

Comparisons are done with == or ===, not with = which is an assignment.

Item 3:  addEventListener() does not exist in versions of IE before IE9, so you will need to use attachEvent() if addEventListener() is not present.

Item 4: Here's a version I've done that gets rid of all globals except tog (which I renamed menuVisble and made it a boolean.  It also uses anonymous functions (less global namespace pollution) for the handlers and makes some other cleanups.  You can see it work here: http://jsfiddle.net/jfriend00/MEaPs/.

var menuVisible = false; // set up a toggle so you can display block and then none.

// listener for any clicks on the document.
document.addEventListener("click", function(e) {
    var clicked = e.target;
    var menu = document.getElementById("menu");
    //check to see if user clicks outside of the
    if (menuVisible && clicked != menu) {
            menu.style.display = "none";
            document.getElementById("button").innerHTML = "Click Me";
            menuVisible = false;
        }

}, false);

document.getElementById("button").addEventListener("click", function(e) {
    var menu = document.getElementById("menu");
    e.preventDefault();
    e.stopPropagation();
    if (menuVisible){
        menu.style.display = "none";
        this.innerHTML = "Click Me";
        menuVisible = false;
    } else {
        menu.style.display = "block";
        this.innerHTML = "Opened!";
        menuVisible = true;
    }
}, false);

Item 5:  And here's a version with no globals at all.  It uses the class of the menu to control and discern visibility.  You can see it work here: http://jsfiddle.net/jfriend00/96NfD/.

// listener for any clicks on the document.
document.addEventListener("click", function(e) {
    var menu = document.getElementById("menu");
    var menuVisible = menu.className.indexOf("hidden") == -1;
    //check to see if user clicks outside of the
    if (menuVisible && e.target != menu) {
            document.getElementById("button").innerHTML = "Click Me";
            menu.className = "hidden";
        }

}, false);

document.getElementById("button").addEventListener("click", function(e) {
    e.preventDefault();
    e.stopPropagation();
    var menu = document.getElementById("menu");
    var menuVisible = menu.className.indexOf("hidden") == -1;
    if (menuVisible) {
        this.innerHTML = "Click Me";
        menu.className = "hidden";
    } else {
        this.innerHTML = "Opened!";
        menu.className = "";
    }
}, false);


Unit testing for Python function that checks whether a value is in a range This is the first time I've ever tried to write unit tests before, so I wanted to post here to see if I'm doing it "right". I read a couple of articles on the subject, but I work alone so I want to ensure that I fully understand the standard ideas before continuing. Could you please take a look at this and let me know what you think?

Here's an example method:

def is_in_range(value: float,
                allowed_range: (float, float),
                inclusive: bool = True) -> bool:
    """Checks if the given value falls within the allowed range.

    Raises:
        ValueError: If value or range are non numeric.
    """
    if float(allowed_range[0]) != allowed_range[0] or float(
            allowed_range[1]) != allowed_range[1] or float(value) != value:
        raise ValueError("Inputs must be numeric.")

    if inclusive:
        return min(allowed_range) <= value <= max(allowed_range)
    else:
        return min(allowed_range) < value < max(allowed_range)

And here are the unit tests I wrote:

class Test_IsInRange(object):
    def test_in_range(self):
        value = 4
        surrounding_range = (value-1, value+1)
        assert utilities.is_in_range(value, surrounding_range)

    def test_out_range(self):
        value = 76
        not_surrounding_range = (value+1, value+2)
        assert not utilities.is_in_range(value, not_surrounding_range)

    def test_exclusive_range(self):
        value = 23
        inclusive_range = (value, value+10)
        assert not utilities.is_in_range(value, inclusive_range, inclusive=False)

    def test_inclusive_range(self):
        value = 1003
        inclusive_range = (value-500, value)
        assert utilities.is_in_range(value, inclusive_range, inclusive=True)

    def test_string(self):
        string_input = "tt"
        with pytest.raises(ValueError):
            utilities.is_in_range(string_input, ("a","zzz"))

Am I testing properly? Am I testing too many different cases? Too few? Should I combine or separate any tests?

Regarding your tests.  You definitely don't have too many.  You actually could have too few.  Consider:

In one dimension, you have 5 cases you can check:

val < min
val == min
min < val < max
val == max
val > max

In another dimension, you have inclusive=True and inclusive=False.  That makes at least 10 combinations you should test.  You also have a "inclusive uses the default" which could add 5 more.

Then, you have allowed_range which expects a tuple with limits in either order.  So you should test allowed_range=(1.5, 5.1) separate from allowed_range=(5.1, 1.5).  That would further double the number of tests.

What about tests where min == max, using both inclusive and non-inclusive limits?

You don't need to write separate functions for each test; each function can easily perform a suite of several tests:

def test_unequal_limits(self):
    #          val, min, max, incl,  excl
    tests = [ (1.5, 2.5, 5.5, False, False),
              (2.5, 2.5, 5.5, True,  False),
              (3.5, 2.5, 5.5, True,  True),
              (5.5, 2.5, 5.5, True,  False),
              (6.5, 2.5, 5.5, False, False) ]
    for val, lower, upper, incl, excl in tests:
          assert is_in_range(val, (lower, upper)) == incl
          assert is_in_range(val, (lower, upper), False) == excl
          assert is_in_range(val, (upper, lower)) == incl
          assert is_in_range(val, (upper, lower), False) == excl

def test_equal_limits(self):
    #          val, limit, incl
    tests = [ (1.5, 3.5,   False),
              (3.5, 3.5,   True),
              (5.5, 3.5,   False) ]
    for val, limit, incl in tests:
          assert is_in_range(val, (limit, limit)) == incl
          assert is_in_range(val, (limit, limit), False) == False

You test exactly one case where all inputs are non-numeric strings.  You probably need to test the minimum, maximum, and value separately.

Regarding your function is_in_range() function.

Why raise a ValueError when the input is considered to be "non-numeric"?  Without the non-numeric check, is_in_range("tt", ("a","zzz")) would return True, which would be quite acceptable.

The non-numeric check causes more problems:

>>> val = 1000000000000000000
>>> is_in_range(val, (val-1, val+1))
Traceback (most recent call last):
  File "<pyshell#46>", line 1, in <module>
    is_in_range(val, (val-1, val+1))
  File "<pyshell#8>", line 11, in is_in_range
    raise ValueError("Inputs must be numeric.")
ValueError: Inputs must be numeric.

Without the non-numeric check, the function would return True, as expected.

The parameter allowed_range: (float, float) has a type-hint of "tuple of two floats", but there is nothing in Python that will enforce that.  You can provide a more than 2 values.  50 is not in the range of 1 to 10, but it is in the range of (1, 10, 100) ... whatever that means!

>>> is_in_range(50, (1, 10, 100))
True

And you are only testing the first two values for numeric-ishness.  0 is not in the range of 1 to 10, but it is in the range of [1, 10, False]!

>>> is_in_range(0, [1, 10, False])
True


Simple async task scheduler I've written a .NET Core console application to monitor the operation of a sensor network and I'd like a review of the task scheduling performed by the main program loop. There are three tasks that are all declared as public static async Task that perform the following operations:

CheckGateways.CheckAll connects to an external API at one minute intervals and is the most likely to fail or take a long time. Normally it'll take a few seconds but I'd like other tasks to continue meanwhile.

CheckNodes.CheckAll is an internal database check that may take a few seconds but is not time critical and runs once per minute.

CheckAlerts.CheckAll checks for alert conditions on the sensors and is the most time critical so I'm checking that once per second.

While the code seems to be working it hasn't been well stress-tested and a few things I'd like reviewed are:

Are there any potential race conditions with the way I'm checking the status of the first two tasks?

I'm repeating the code to start / check the first two tasks so perhaps there's a cleaner way to do that without introduction too much extra code as there's unlikely to be any more tasks added.

Also any general comments on coding / naming standards would be appreciated.
static async Task Main()
{
    IConfiguration configuration = new ConfigurationBuilder()
        .AddJsonFile("appsettings.json", optional: false, reloadOnChange: true)
        .Build();
    Log.Logger = new LoggerConfiguration()
        .ReadFrom.Configuration(configuration)
        .Enrich.FromLogContext()
        .MinimumLevel.Override("Microsoft", LogEventLevel.Warning)
        .CreateLogger();
    Task checkGateways = null;
    DateTime lastGatewayCheck = DateTime.UtcNow;
    Task checkNodes = null;
    DateTime lastNodeCheck = DateTime.UtcNow;
    while (true)
    {
        try
        {
            // Check gateway connectivity loss at one minute intervals
            if (DateTime.UtcNow.Subtract(lastGatewayCheck).TotalMinutes >= 1
                    && (checkGateways == null || checkGateways.IsCompleted))
            {
                lastGatewayCheck = DateTime.UtcNow;
                checkGateways = CheckGateways.CheckAll(configuration);
            }
            if (checkGateways?.Status == TaskStatus.Faulted)
            {
                throw checkGateways.Exception;
            }

            // Check node connectivity loss at one minute intervals
            if (DateTime.UtcNow.Subtract(lastNodeCheck).TotalMinutes >= 1
                    && (checkNodes == null || checkNodes.IsCompleted))
            {
                lastNodeCheck = DateTime.UtcNow;
                checkNodes = CheckNodes.CheckAll(configuration);
            }
            if (checkNodes?.Status == TaskStatus.Faulted)
            {
                throw checkNodes.Exception;
            }

            // Check for pending alerts to send at one second interval
            await CheckAlerts.CheckAll(configuration);
        }
        catch (AggregateException ae)
        {
            foreach (var ex in ae.InnerExceptions)
            {
                LogException(ex);
            }
        }
        catch (Exception ex)
        {
            LogException(ex);
        }
        await Task.Delay(1000);
    }
}

public static void LogException(Exception ex)
{
    ConsoleMessage(ex.ToString());
    Serilog.Log.Error(ex, "Error occured in TelemetryService");
}

public static void ConsoleMessage(string msg)
{
    Console.WriteLine($"{DateTime.Now:yyyy-MM-dd HH:mm:ss}: {msg}");
}

No way to stop the cycle.
The standard with TPL is to use the CancellationToken.  Even if it is not need now, which I don't know why, it would be easier in the future if it supported it.  For example if it gets turned into a service can just cancel the Token Source or if stays a console app could trap Esc key and cancel token source.
Numbers for Task Delay.
It's easier to read/maintain
Task.Delay(TimeSpan.FromSeconds(1))

then
Task.Delay(1000)

For each Task instead of checking their time you can combine them with Task.Delay()
For example
Task.WhenAll(lastNodeCheck, Task.Delay(TimeSpan.FromMinutes(1)));

Now we have one task that will only complete when either a minute has passed and the main task has completed.  We can make a helper method for this
    public static Task DelayedTask(Task task, TimeSpan delay, CancellationToken token)
    {
        return Task.WhenAll(task, Task.Delay(delay, token));
    }

The downside to this is even if the task failed right away it will still wait the delay time before it gets logged that it failed. I don't think that's a big deal breaker but only you know that for sure.
Optional but you can make this into a queue
We can create a dictionary that waits for task to be complete then re-adds them if needed.   This will make the code a bit more complex but easier to add new task later on.
Something like
    private static async Task TaskQueue(CancellationToken token, params Func<Task>[] tasks)
    {
        if (tasks.Length == 0)
        {
            return;
        }

        var queue = new ConcurrentDictionary<Task, Func<Task>>();
        foreach (var task in tasks)
        {
            queue.TryAdd(task(), task);
        }

        while (!token.IsCancellationRequested)
        {
            await Task.WhenAny(queue.Keys).ContinueWith(completedTask =>
            {
                Func<Task> factory;
                var mainTask = completedTask.Unwrap();
                queue.TryRemove(mainTask, out factory);
                if (!mainTask.IsCanceled)
                {
                    queue.GetOrAdd(factory(), factory);
                }

                if (mainTask.IsFaulted)
                {
                    foreach (var ex in mainTask.Exception.InnerExceptions)
                    {
                        LogException(ex);
                    }
                }
            });
        }
    }

I haven't tested this with all options but with basic test seems to work.   We create a concurrent dictionary and load it with tasks that are in flight and factories to make the task.  As task complete we remove from the dictionary and re-add them.  Continue until we get told to cancel.
Now in the main method can look something similar to this
        var cancellation = new CancellationTokenSource();
        var cancelToken = cancellation.Token;
        Func<Task> checkGateWayFactory = () => DelayedTask(CheckGateways.CheckAll(configuration), TimeSpan.FromMinutes(1), cancelToken);
        Func<Task> checkNodeFactory = () => DelayedTask(CheckNodes.CheckAll(configuration), TimeSpan.FromMinutes(1), cancelToken);
        Func<Task> checkAlertFactory = () => DelayedTask(CheckAlerts.CheckAll(configuration), TimeSpan.FromSeconds(1), cancelToken);
        await TaskQueue(cancelToken,
            checkAlertFactory,
            checkGateWayFactory,
            checkNodeFactory);


Class template for beginners I want to allow anyone that has never coded to make classes depending on my template with little to no knowledge of Python.

These classes need to do 2 things:

Inherit from other classes similar to them.
Contain variables, updating values if they have been changed or keeping them if not.

For the sake of simplicity I don't want to have super() on these new classes, as I'm trying to not frighten newcomers with things they can't understand yet. Here's how I did it:

class Template(object):

    def __init__(self):
        self.value = "hello"
        self.other_value = "bonjour"
        self.constant_value = 42
        current_class = self.__class__
        inits = []
        while (current_class.__name__ != "Template"):
            inits.append(current_class.init)
            current_class = current_class.__bases__[0]
        for i in reversed(inits):
            i(self)

    def init(self):
        pass

    def info(self):
        print self.value
        print self.other_value
        print self.constant_value
        print ""

class Deep(Template):
    def init(self):
        self.value = "howdy"
        self.other_value = "salut"

class Deeep(Deep):
    def init(self):
        self.value = "hi"

class Deeeep(Deeep):
    def init(self):
        self.value = "'sup"

very_deep = Deeeep()
not_so_deep = Deep()
very_deep.info()
not_so_deep.info()

This outputs :

sup
salut
42

howdy
salut
42

Which is exactly what I want, but it looks really weird, obfuscated and non-Pythonic. Any ideas for improvements?

This:

...
while (current_class.__name__ != "Template"):
...

is weird. Do this:

...
while current_class != Template:
...

it does the same, except comparing classes instead of strings.

This is strange as well:

for i in reversed(inits):
     i(self)

i is used for iteration so it becomes obsucre, are you calling an int? This:

for init in reversed(inits):
    init(self)

is more explanatory.

I don't really know about this next part, but you could define a small lambda

extract_first = lambda x: x[0]

with an explanatory name, leaving that part of the code as this:

extract_first = lambda x: x[0]

while current_class != Template:
    inits.append(current_class.init)
    current_class = extract_first(current_class.__bases__)
for init in reversed(inits):
    init(self)

It leaves it with more words and text but less programming lingo. Might be good.

Now what I would do if this was for someone new to programming is relay on something else then abstraction of the init and abstract in something like biology:

class Exists(object):
    perceptible = True

class Alive(Exists):
    can_use_sustenance = True

class Vertebrate(Alive):
    can_move = True

class Mammal(Vertebrate):
    heat_generation = True

class Dog(Mammal):
    pass

class Cat(Mammal):
    pass

Tested on my significant other :-)


SkipLast of an IEnumerable - Linq Extension As my answer to this question, I came up with this solution:

static public IEnumerable<T> SkipLast<T>(this IEnumerable<T> data, int count)
{
  if (data == null || count < 0) yield break;

  Queue<T> queue = new Queue<T>(data.Take(count));

  foreach (T item in data.Skip(count))
  {
    queue.Enqueue(item);
    yield return queue.Dequeue();
  }
}

It returns all items in the set but the count last, but without knowing anything about the size of the data set. I think it's funny and that it's working, but a comment claims that is doesn't. Am I overlooking something?

A version with a circular queue could be:

static public IEnumerable<T> SkipLast<T>(this IEnumerable<T> data, int count)
{
  if (data == null || count < 0) yield break;
  if (count == 0)
  {
    foreach (T item in data)
      yield return item;
  }
  else
  {
    T[] queue = data.Take(count).ToArray();
    int index = 0;

    foreach (T item in data.Skip(count))
    {
      index %= count;
      yield return queue[index];
      queue[index] = item;
      index++;
    }
  }
}

Performance wise they seems to be even.

Compared to other solutions like the most obvious:

data.Reverse().Skip(count).Reverse()

It seems to be at least as fast and for very large set about twice as fast.

Test case:

  int count = 20;
  var data = Enumerable.Range(1, count);

  for (int i = 0; i < count + 5; i++)
  {
    Console.WriteLine($"Skip: {i} => {(string.Join(", ", data.SkipLast1(i)))}");
  }

Any comments are useful.

There are 2 cases for which both your and VisualMelon's implementation can be improved:

If count is less than or equal to 0, then you can return source directly. This speeds up iteration by removing an unnecessary intermediate step. For this to work, the yielding part of the method has to be moved into another method, but that's easy with local functions.
If source is ICollection<T> collection, then you can return source.Take(collection.Count - count), which is faster and uses less memory because it doesn't need to buffer anything. The higher count is, the more of a difference this makes.

The first of these is probably not a very useful edge-case, so it might not be worth the extra code, but I would definitely include the second optimization.


Simple Pool Allocator This is a simple pool allocator, based (to at least some degree) on a previous answer. The basic idea is pretty simple: allow a user to allocate objects of some type quickly (in relatively large contiguous chunks) and only free objects when the allocator itself is freed, then destroy all the objects and free the memory they occupy.

#include <iostream>
#include <vector>

template <class T, size_t T_per_page = 200>
class Allocator {
    const size_t pool_size = T_per_page * sizeof(T);
    std::vector<T *> pools;
    size_t next_pos;

    void alloc_pool() {
        next_pos = 0;
        void *temp = operator new(pool_size);
        pools.push_back(static_cast<T *>(temp));
    }

public:
    Allocator() {
        alloc_pool();
    }

    T* operator()(T const &x) {
        if (next_pos == T_per_page)
            alloc_pool();

        T *ret = new(pools.back() + next_pos) T(x);
        ++next_pos;
        return ret;
    }

    ~Allocator() {
        while (!pools.empty()) {
            T *p = pools.back();
            for (size_t pos = T_per_page; pos > 0; --pos)
                p[pos - 1].~T();
            operator delete(static_cast<void *>(p));
            pools.pop_back();
        }
    }
};

#ifdef TEST

struct list_node {
    int val;
    list_node *next;

    list_node(int x) : val(x), next(nullptr) {}

    friend std::ostream &operator<<(std::ostream &os, list_node const &n) { return os << n.val; }
};

Allocator<list_node> alloc;

int main() {
    for (int i = 0; i < 20; i++) {
        list_node *j = alloc(i + 1000);
    }
}

#endif

I'd welcome any and all suggestions (including adding specialization to avoid executing destructors on trivially destructible types). I'm also wondering (sort of thinking out loud) whether it would make sense to use a geometric allocation strategy, similar to std::vector's, to (perhaps) fit allocation time to size more effectively.

Allocation of raw memory
Rather than using operator new you could use std::get_temporary_buffer<T>(n). This allocates contiguous "correctly aligned" storage for enough memory to store approx n objects of type T (its non binding contract read documents).
Constructor
It may be more efficient to use normal constructor than the copy constructor. You should also provide the move constructor.
T* operator()(T&& x);
T* operator()(T const &x);
template<typename... Args>
T* operator()(Args... args);

Destructor
Yes you can optimize the destructor if Ts destructor is guranteed to do nothing.
template<typename T>
struct SimpleDestructableTrivialy
{
    static constexpr bool value = std::is_trivially_destructible<T>::value;
};
template<typename T, bool = SimpleDestructableTrivialy<T>::value>
class SimpleDestroy
{
    public:
        void destroyElements(T* p, std::size_t T_per_page);
};
template<typename T>
class SimpleDestroy<T, true>
{
    public:
        void destroyElements(T* p, std::size_t T_per_page) {}
};
template<typename T>
class SimpleDestroy<T, false>
{
    public:
        void destroyElements(T* p, std::size_t T_per_page)
        {
            for (size_t pos = T_per_page; pos > 0; --pos)
                p[pos - 1].~T();
        }
};


Pixel manipulation performance I have created a function that adds "shadow" to the content of a canvas. The algorithm for adding shadow follows these rules:

If the either x or y is 0, ignore.
If the pixel on the current pixel's top left corner is not completely transparent, set current pixel's alpha value to 0.1.

This algorithm runs from top to bottom, left to right. Here's the relevant part, where rendered is a canvas that already contain rendered content.

    function getIndex(x, y){
        return (y * width + x) * 4;
    }

    function isClear(x, y){
        var index = getIndex(x, y);
        return data[index + 3] === 0;
    }

    function isInShadow(x, y){
        if(x < 1 || y < 1)                          return false;
        if(isClear(x, y) && !isClear(x - 1, y - 1)) return true;

        return false;
    }

    var width = rendered.width, height = rendered.height,
        imgData = renderedCtx.getImageData(0, 0, width, height),
        data = imgData.data;

    for(var x = 0; x < width; x++){
        for(var y = 0; y < height; y++){
            if(isInShadow(x, y)){
                data[getIndex(x, y) + 3] = 256 * 0.1;
            }
        }
    }

    renderedCtx.putImageData(imgData, 0, 0);

As you can see, this function performs per pixel manipulation and is quite slow in performance. What is a good way to improve performance, probably without checking each pixel independently?

Use diagonals. The idea is to have only one pixel check, compared to two you have now:

find the first occupied pixel on a diagonal
skip to an unoccupied one
fill with 0.1 while unoccupied
repeat 1-3 until out of bounds

Considering everything should be inlined, the code would be like this:

var x, y, index, index0,
    len = width * height * 4,
    stride = width * 4;
    strideDia = stride + 4; // distance to the sibling pixel towards bottom-right

// walk diagonals from the bottom-left corner to the top-left corner
for (x = 0, y = height, index0 = (y * width + x)*4 + 3; --y >= 0; ) {
    index = index0 -= stride;

    while (index < len && x < width) {
        // find an occupied pixel
        while (index < len && x < width && !data[index])
            index += strideDia, x++;
        // skip to an unoccupied
        while (index < len && x < width && data[index])
            index += strideDia, x++;
        // fill with 0.1 all unoccupied
        while (index < len && x < width && !data[index]) {
            data[index] = 25; // 0.1 * 256
            index += strideDia;
            x++;
        }
    }

    x = 0;
}

// walk diagonals from the top-left corner to the top-right corner
// (0,0) is skipped as it was processed in the previous walk block
for (x = 0, y = 0, index0 = (y * width + x)*4 + 3; ++x < width; ) {
    index = index0 += 4;
    var x0 = x;

    while (index < len && x < width) {
        // find an occupied pixel
        while (index < len && x < width && !data[index])
            index += strideDia, x++;
        // skip to an unoccupied
        while (index < len && x < width && data[index])
            index += strideDia, x++;
        // fill with 0.1 all unoccupied
        while (index < len && x < width && !data[index]) {
            data[index] = 25; // 0.1 * 256
            index += strideDia;
            x++;
        }
    }

    x = x0;
}

Another potentially promising idea would be to use asm.js.


Simple terminal style message logger I have a short function written in JavaScript using jQuery which takes a message and appends it to a div. It does this one character at a time with a small delay between characters, and then returns the time that it took it will take for the function to finish running.
const log = $('#log')

function logMessage(message, type) {
    log.append('

')
    log.children().last().addClass(type);
    for (let charIndex = 0; charIndex < message.length; charIndex++)
        setTimeout(() => {
            log.children().last().append(message[charIndex])
        }, charIndex * 10)
    return message.length * 10
}

This code feels a little bit unoptimized but I'm not sure what would be a better of going about doing this.

You're using the right approach with the log - you're saving it in a variable so you don't have to select the element again. Do the same thing with the newly appended
.

const log = $('#log')

function logMessage(message, type) {
    const newDiv = $('

')
      .addClass(type)
      .appendTo(log);
    for (let charIndex = 0; charIndex < message.length; charIndex++) {
        setTimeout(() => {
            newDiv.append(message[charIndex])
        }, charIndex * 10)
    }
}

logMessage('something with lots and lots and lots and lots of characters', 'sometype');

A tiny issue with iterating over the indicies of the string manually is that characters composed of surrogate pairs (like ) will take twice as long to print as others, and if you were to slow down the animation, you'd get a broken character before the second part gets fully rendered:

const log = $('#log')

function logMessage(message, type) {
    const newDiv = $('

')
      .addClass(type)
      .appendTo(log);
    for (let charIndex = 0; charIndex < message.length; charIndex++) {
        setTimeout(() => {
            newDiv.append(message[charIndex])
        }, charIndex * 500)
    }
}

logMessage('something with lots and lots and lots and lots of characters', 'sometype');

It also might be useful to return a Promise indicating that the message has been fully logged, instead of a number indicating how long to wait before the message is finished.
To tweak this, and to avoid having to touch the string indicies at all (since you only really care about the characters, not the indicies of characters) would be to invoke the string's iterator and await a Promise inside the loop:

const log = $('#log');
const delay = ms => new Promise(resolve => setTimeout(resolve, ms));

async function logMessage(message, type) {
    const newDiv = $('

')
      .addClass(type)
      .appendTo(log);
    for (const char of message) {
        newDiv.append(char);
        await delay(500);
    }
}

logMessage('something', 'sometype')
  .then(() => {
      console.log('message fully printed');
  });

I'm a bit worried about the log ID - you have both a global identifier log which refers to the HTMLElement and exists on the window. Then the code shadows the global identifier and creates a log which refers to the jQuery collection. It's an unlikely source of bugs, but I'd use a class instead of an ID.
I notice you're not using semicolons. If this is a deliberate style choice and you consider yourself an expert who can avoid the pitfalls of Automatic Semicolon Insertion, that's just fine. Otherwise, you may occasionally run into very confusing bugs due to a statement unexpectedly crossing a linebreak, and I'd recommend using them.


JavaScript: Simulating the process of removing multiple videos at once from a YouTube playlist I've written the code below with what little knowledge I have of JavaScript at the moment.

The code simulates the process of removing multiple videos at once from a playlist like the one you see on YouTube. As it stands now, the code removes a total of four videos (each positioned at the index 0, 2, 5, and 7), using the combination of the for loop and splice methods. But here are a couple of things I can't figure out on my own:

1. Practically speaking, videos to be removed, and by extension, their corresponding indexes in an array should not be predetermined, but rather be selected by the user. In other words, the playlistIndex array should be empty at first, and index numbers should be added to it one by one as the user ticks checkboxes.

2. Even after I have ironed out the first problem mentioned above, the current code will remove a selected video immediately after one index number is pushed to the playlistIndex array. So I have to make it so that the code does not start to remove videos until the user has selected every video they want to have removed.

3. Lastly, before setting the removal process in motion, the code needs to have the user confirm their decision. Ideally, the user should be asked to press "yes" or "no" in a pop-up that goes something like "You are about to remove 7 videos. Are you sure you want to proceed?"

var playlist = ["video1", "video2", "video3", "video4", "video5", "video6", "video7", "video8", "video9", "video10"];

var playlistIndex = [0, 2, 5, 7];

var bin = [];

for (var i = playlistIndex.length - 1; i >= 0; i--) {
    bin.unshift(playlist.splice(playlistIndex[i], 1));
}

for (var j = bin.length -1; j >= 0; j--) {
    if (bin[j].length === 0) {
        bin.splice(j, 1);
    }
}

if (playlist.length === 0) {
    alert("No videos in the playlist.");
}
else {
    alert(playlist.length+" video(s) remain in the playlist: "+playlist.join(", ")+".");
}

bin = [].concat.apply([], bin);

alert(bin.length+" video(s) have been removed: "+bin.join(", ")+".");

for (var i = playlistIndex.length - 1; i >= 0; i--)

Instead of having it negatively looping, which in this instance is unnecessary, do it positively:

for (var i = 0, length = playlistIndex.length; i < length; i++){

Instead of the negatively recurring for loop, consider using a length subtraction operation instead like in the example below:

for (var j = bin.length -1; j >= 0; j--) {
    if (bin[j].length === 0) {
        bin.splice(j, 1);
    }
}

into:

for (var i = 0, length = bin.length; i <= length; i++){
    bin.splice(length - 1 - i, 1);
}

For this example, a negative iteration is the way to go, because otherwise as @Tibos pointed out in the comments below, the array indexes can get messed up and end up leaving them in.

For example, if b were ['a', 'b', 'c'], a normal for loop here with bin.splice(i, 1) would leave 'b'.

bin.unshift with the a negative for loop is the same as bin.push with a positive for loop.

Positive for loops are always preferred when possible.

for (var i = playlistIndex.length - 1; i >= 0; i--) {
    bin.unshift(playlist.splice(playlistIndex[i], 1));
}

into:

for (var i = 0, length = playlistIndex.length; i < length; i++){
    bin.push(playlist.splice(playlistIndex[i], 1));
}

You should have whitespace between your operators:

playlist.length+" video(s) remain in the playlist: "+playlist.join(", ")+"."

into:

playlist.length + " video(s) remain in the playlist: " + playlist.join(", ") + "."

The following block can be converted to a ternary statement:

if (playlist.length === 0) {
    alert("No videos in the playlist.");
}
else {
    alert(playlist.length+" video(s) remain in the playlist: "+playlist.join(", ")+".");
}

into:

alert(playlist.length === 0
    ? "No videos in the playlist"
    : playlist.length + " video(s) remain in the playlist: " + playlist.join(", ") + "."
);

bin = [].concat.apply([], bin): this seems unnecessary; I wouldn't bother doing it.

I thought this was unnecessary, but I was wrong! as you receive arrays from splice, it was necessary to transform it into an singular array, instead of a 2d array.

Instead of that, attach [0] to the end of your bin.push(playlist.splice(playlistIndex[i], 1)), so it would look like:

bin.push(playlist.splice(playlistIndex[i], 1)[0]);

This works because you only splice a single value from the array, but if you were to splice more, you would need to use that concat statement!

Again with the whitespace:

alert(bin.length+" video(s) have been removed: "+bin.join(", ")+".");

into:

alert(bin.length + " video(s) have been removed: " + bin.join(", ") + ".");

  Practically speaking, videos to be removed, and by extension, their corresponding indexes in an array should not be predetermined, but rather be selected by the user. In other words, the playlistIndex array should be empty at first, and index numbers should be added to it one by one as the user ticks checkboxes.

Look at this page(It's w3schools, I couldn't find a MDN post on it, sorry) on HTML checkboxes, this may be exactly what you're looking for.

Then, you can iterate over the array of checkboxes and add the indexes of the ones  that are checked.

  Even after I have ironed out the first problem mentioned above, the current code will remove a selected video immediately after one index number is pushed to the playlistIndex array. So I have to make it so that the code does not start to remove videos until the user has selected every video they want to have removed.

Simply move this to a function! Then attach the function to a submit button on your <form>. It will only get called after the form has been submitted, rather than on addition.

  Lastly, before setting the removal process in motion, the code needs to have the user confirm their decision. Ideally, the user should be asked to press "yes" or "no" in a pop-up that goes something like "You are about to remove 7 videos. Are you sure you want to proceed?"

Use confirm(), it lets you ask a user a question and return a boolean response.

For example:

if (!confirm('Are you sure you want to delete these videos?')){
    return;
}
// do deleting here.

Note the reverse condition allows you to get away from an extra layer of conditional over your entire post.


Password checker containing many conditional statements Can this be shortened/improved? I'm trying to make a password checker in Python.

Could the ifs be put into a for loop?  If so, how?

pw = input("Enter password to test: ")

caps = sum(1 for c in pw if c.isupper())
lower = sum(1 for c in pw if c.islower())
nums = sum(1 for c in pw if c.isnumeric())

scr = ['weak', 'medium', 'strong']
r = [caps, lower, nums]

if len(pw) < 6:
    print("too short")
elif len(pw) > 12:
    print("too long")

if caps >= 1:
    if lower >= 1:
        if nums >= 1:
            print(scr[2])
        elif nums < 1:
            print("your password is " + scr[1])
    elif lower < 1:
        print("your password strength is " + scr[0])
elif caps < 1:
    print("your password strength is " + scr[1])

Enter password to test: premaintenance disdainful hayloft seer
too long
your password strength is medium

Enter password to test: NXJCWGGDVQZO
your password strength is weak

Enter password to test: Password1
strong

Your knowledge of password strength is: weak.

Explanation

Password strength is normally measured in "bits of entropy" — the idea being that if a password has been picked randomly from a pool of similar passwords of size N, then its entropy is log2N bits.

The first password I tried above was picked using a method suggested by Randall Munroe, like this:

>>> words = list(open('/usr/share/dict/words'))
>>> import random
>>> random.SystemRandom().shuffle(words)
>>> print(' '.join(w.strip() for w in words[:4]))
premaintenance disdainful hayloft seer

Its entropy can be calculated like this:

>>> from math import log
>>> l = len(words)
>>> log(l * (l - 1) * (l - 2) * (l - 3), 2)
71.39088438576361

This is a strong password—a cracker that tried a billion such passwords a second would take on average about 50,000 years to find it.

The second password is also strong, but not as good as the first. I generated it like this:

$ </dev/random base64 | tr -cd A-Z | head -c 12
NXJCWGGDVQZO

Its entropy is 12 × log226 = 56.4 bits.

The third password is, of course, the weakest. password1 is about the 600th most common password (according to Mark Burnett, here) and the initial capital letter is a common substitution that password cracking programs know all about.


Extracting numbers from a string into an ArrayList This first stores a string as input, loops through it to extract all of the numbers into an ArrayList, then prints all of these numbers along with their sum and product.

How would I decompose this Java program into multiple methods?

public class Assignment1 {
    public static void main(String[] args){

        // Creates scanner for storing input string
        Scanner numScanner = new Scanner(System.in);
        String input;
        System.out.println("Enter a string of numbers to compute their sum and product:");
        System.out.println("(Enter '.' to terminate program)");
        input = numScanner.nextLine();

        // Terminates the program if there is no input
        if (input.length() == 0){
            System.out.println("Invalid input: Not enough characters");
            System.exit(-1);
        }

        // Terminates the program if the first character is '.'
        if (input.charAt(0) == '.'){
            System.out.println("Thank you for using numberScanner!");
            System.exit(-1);
        }

        // Defines all of the variables used in the loops
        int index = 0;
        int sum = 0;
        int product = 1;
        Integer start = null;
        int end = 0;
        ArrayList<Integer> numbers = new ArrayList<>();

        // Loop that extracts all numbers from the string and computes their sum and product
        while (index < input.length()){
            if (input.charAt(index) >= 'A' && input.charAt(index) <= 'Z' && start == null){
                index++;
            }else if (input.charAt(index) >= '1' && input.charAt(index) <= '9' && start == null){
                start = index;
                index++;
            }else if (Character.isDigit(input.charAt(index))){
                index++;
            }else{
                end = index;
                numbers.add(Integer.parseInt(input.substring(start,end)));
                sum += Integer.parseInt(input.substring(start,end));
                product *= Integer.parseInt(input.substring(start,end));
                index++;
                start = null;
            }
        }

        // For the last number, the end is greater than the length of the string
        // This prints the last number without using the end
        if (index == input.length()){
            numbers.add(Integer.parseInt(input.substring(start)));
            sum += Integer.parseInt(input.substring(start));
            product *= Integer.parseInt(input.substring(start));
            index++;
        }

        // Prints the Numbers, Sum and Product beside each other
        System.out.print("Numbers: ");
        for (Object a : numbers) {
            System.out.print(a.toString() + " ");
            }
        System.out.println("Sum: " + sum + " Product: " + product);
    }
}

Start by separating your code into input handling, computation, and output.  Each of those could have its own method(s).

For the input, you're working too hard.  The java.util.Scanner has methods to give you ints.

Think about what the inputs and outputs of each function should be.  For example, your input handling function should take an InputStream and return a List of Integers or an array of ints.


Retrieve and load playlists from YouTube I have this code I am using in a private project.  The following is a snippet of code I use to retrieve and load playlists from YouTube:

for (feedEntry in a.feed.entry) {
  if (a.feed.entry.hasOwnProperty(feedEntry)) {
  feedTitle = a.feed.entry[feedEntry].title.$t.replace('] - ', ']').replace(' : ', '').replace(/\(.*?\)/g, "").replace(/\[.*?\]/g, "");
  feedURL = a.feed.entry[feedEntry].link[1].href;
  fragments = feedURL.split("/");
  videoID = fragments[fragments.length - 2];
  url = videoURL + videoID;
  var c = feedTitle.split(' - ');
  tracklist.push({
    'title': c[1],
    'artist': c[0],
    'url': 'http://youtubeinmp3.com/fetch/?video=http://www.youtube.com/watch?v=' + videoID + '',
    'album': 'YouTube'
  })
}

This code, understandably, causes the browser to freeze a few seconds when loading larger playlists.

What ways could I go about optimizing this?

The freezing comes primarily from the fact that JavaScript is single-threaded so when your loop is busy-looping everything else will wait until the scope has finished before the browser can continue to poll on the event queue.

To solve this you can convert your code to asynchronous executing using setTimeout() to allow the browser to do other tasks in-between.

Another thing is that for in loops are very slow so this can be replaced by using Object keys instead.

Your parsing can be optimized by caching the current object as well as re-factor the parsing itself to a separate function method:

var keys = Object.keys(a.feed.entry),  // retrieve all keys from the object
    count = keys.length,               // number of keys to parse
    segment = 10,                      // number to parse each segment
    current;                           // counter for segment

// start loop
(function doSegment() {

    current = segment;                 // reset segment counter for each iteration

    while(current-- && count)
        parseFeedEntry(a.feed.entry[keys[--count]]);

    if (count)
        setTimeout(doSegment, 11);     // delay 11ms to allow browser to breath
})();

function parseFeedEntry(entry) {
    // parse the entry here
    var feedTitle = entry.title.$t.replace('] - ', ']'). ...8X...
    // etc., notice entry is now cached
}

Online demo

Hope this helps!


Implementing Abstract class I used Pythons source code/documentation/textbook to come up with an abstract class and following subclasses.

My requirements are:

Domestic students don't require documentation
International student do require documentation (passports, etc..)

from abc import ABCMeta

class Student(metaclass=ABCMeta):

    def __init__(self, id, firstname, lastname):
        self.__id = id
        self.__firstname = firstname
        self.__lastname = lastname

    @property
    def iden(self):
        return self.__id

    @property
    def first_name(self):
        return self.__firstname

    @property
    def last_name(self):
        return self.__lastname

International Subclass:

from Student import Student
from copy import deepcopy

class International(Student):
    def __init__(self, iden, firstname, lastname, docuemnts):
        super().__init__(iden, firstname, lastname)

        self.__documents = deepcopy(docuemnts)
        self.__type_of_student = "International"

    @property
    def international_documents(self):
        return deepcopy(self.__documents)

Domestic Subclass:

from Student import Student

class Domestic(Student):

    def __init__(self, iden, firstname, lastname):
        super().__init__(iden, firstname, lastname)

        self.__type_of_student = "Domestic"

    @property
    def student_type(self):
        return self.__type_of_student

Have I constructed and implementation the abstract class correctly?

How do you tell what the student type is, without erroring on both International and Domestic?

With your current code, you'd have to do something like:

if hasattr(student, 'student_type'):
    student_type = student.student_type()
else:
    student_type = None

What makes this worse, is you assume None to be International.
Why can't Domestic students have international_documents?

In your requirements you state:

  Domestic students don't require documentation
  International student do require documentation (passports, etc..)

Why should these be separate classes?
Your name mangled variables make your code longer, and generally more horrible to read.
Instead you could make them just normal properties.

However, since they are unsetable, you may want to use a namedtuple instead.

And so I'd use something like:

from collections import namedtuple
from Enum import enum

class StudentType(Enum):
    INTERNATIONAL = 'international'
    DOMESTIC = 'domestic'

Student = namedtuple('Student', 'id first_name last_name type documents')

s1 = Student(0, 'John', 'Doe', StudentType.INTERNATIONAL, [...])
s2 = Student(1, 'Eva', 'Smith', StudentType.DOMESTIC, [])


Getting the business day of the month I'm working on figuring out what the current 'business' day of the month it is. For instance, if it's the 20th, but there are 3 weekends, it would be the 14th business day of the month.

Is there a better way of doing this?

var first = DateTime.Today.FirstDayOfMonth();
var businessDayOfMonth = 0;
for (var i = first; i <= DateTime.Today; i = i.AddDays(1))
{
    if (i.DayOfWeek != DayOfWeek.Saturday && i.DayOfWeek != DayOfWeek.Sunday)
        businessDayOfMonth++;
}

As explained in comments (in which I pointed out a few possible edge cases), even though it's not optimized in terms of performance, this is about as clear as it gets.

A more clever algorithm - calculating the number of weekdays without iterating through all of them - is probably possible, but I bet it would be much less readable. If performance isn't a concern at this point, I'd leave it.

You could maybe replace the loop with a LINQ-based solution.

int WhichWorkingDay(DateTime day)
{
    // edge cases
    switch (day.DayOfWeek)
    {
        case DayOfWeek.Saturday:
            return day.Day > 1
                // Friday the day before
                ? WhichWorkingDay(day.AddDays(-1))
                // or next Monday if previous Friday was on last month
                : WhichWorkingDay(day.AddDays(2));
        // analogically here
        case DayOfWeek.Sunday:
            return day.Day > 2
                ? WhichWorkingDay(day.AddDays(-2))
                : WhichWorkingDay(day.AddDays(1));
    }

    // or you could inline it, obviously
    Func<DateTime, bool> isWorkingDay = d => d.DayOfWeek != DayOfWeek.Saturday
        && d.DayOfWeek != DayOfWeek.Sunday;

    return Enumerable
        .Range(1, day.Day)
        .Select(d => new DateTime(day.Year, day.Month, d))
        .Count(isWorkingDay);
}


Mail merge (Parsec string formatting) example My intent is to perform "mail merge" where I can write strings like "hi <<name>>" and format them according to a HashMap.

Specifically, the string contains keys formatted as <<key>>, and the map contains the corresponding values.

I have a main concern that I'd like help with. I think it would be best to perform the parsing in multiple stages:

first find the Keys
second find the remaining Chunks
and for more complicated parsing tasks, perhaps more stages

I couldn't figure that out and instead use the more expensive lookahead function notFollowedBy and one pass. That obviously wouldn't work well if I had a slightly more complicated need.

import Data.Functor.Identity (Identity)
import Data.HashMap.Lazy as HM
import Text.Parsec
import Text.Parsec.String

-- Parsing ----------

data Merge a = Chunk a | Key a deriving (Show)

key :: Parser (Merge String)
key = Key <$> between (string "<<") (string ">>") (many1 letter)

chunk :: Parser (Merge String)
chunk = Chunk <$> many1 (notFollowedBy key >> anyChar)

prose :: ParsecT String () Identity [Merge String]
prose = many1 $ key <|> chunk

-- Formatting ----------

format :: HM.HashMap String String -> [Merge String] -> String
format _ []             = ""
format hmap (Chunk x : xs) = x ++ format hmap xs
format hmap (Key k : xs) =
  case HM.lookup k hmap of
    -- I could obviate the `error` by working within a failure monad
    Nothing -> error $ "missing key: " ++ k
    Just v -> v ++ format hmap xs

-- Testing ----------

testString = "Hi <<name>>! Do you like <<thing>>?"
testMap = HM.fromList [("name", "Adam"), ("thing", "Apples")]
main = print $ format testMap <$> parse prose "" testString

Instead of notFollowedBy, you can use noneOf in chunk:

chunk :: Parser (Merge String)
chunk = Chunk <$> many1 (noneOf "<") <|> try (sequence [char '<', noneOf "<"])

That prevents a lookahead but it is no longer that nice to look at.

format can be rewritten without explicit recursion if we use concatMap:

format :: HM.HashMap String String -> [Merge String] -> String
format hmap xs = concatMap go xs
  where
    go (Chunk c) = c
    go (Key   k) = maybe (error $ "missing key: " ++ k) id (HM.lookup k hmap)

However, this would make our function partial, which we try to prevent. So let's use Either instead:

format :: HM.HashMap String String -> [Merge String] -> Either String String
format hmap xs = concat <$> mapM go xs
  where
    go (Chunk c) = Right c
    go (Key   k) = maybe (Left $ "missing key: " ++ k) Right (HM.lookup k hmap)


Convert infix regular expression notation to postfix This is a small part of a larger program for implementing a limited syntax regular expression constructor using Ken Thompson's construction algorithm. Converting to postfix before the regular expression is processed makes the processing vastly simpler because everything can be smoothly read and processed left to right. The following algorithm for performing the conversion works in a shunting-yard like manner where an operator stack is used to determine when operators should be sent to the output string.
Conversion Function:
typedef struct _conv_ret {
    char *re;
    int err;
} conv_ret;

conv_ret conv(char *re) {
    /* converts limited regex infix notation with explicit
     * catenation denoted by '.' to postfix in a shunting-yard manner */

    conv_ret ret = {NULL, REGEX_TOOLARGE};

    if(strlen(re) > MAX_LINE)
        return ret;

    static char buf[MAX_LINE];
    char *bufp = buf;

    ret.re = buf;
    ret.err = 0;

    /* operator stack */
    int bp[strlen(re)];
    int *sp = bp;

    #define OP_NUM 6

    /* placeholder for id 0 */
    char id_map[OP_NUM+1] = {' ', '(', '|', '.', '?', '+', '*'};
    int prec_map[OP_NUM+1] = {0, 1, 2, 3, 4, 4, 4};

    #define push(id) *++sp = id
    #define pop()    *bufp = id_map[*sp--]; bufp++

    for(; *re; re++) {
        /* loop skips open paren (id 1) because it is only there
         * as a placeholder until the closing paren is pushed */
        for(int id = 2; id < OP_NUM+1; id++) {
            /* pop until incoming op is
             * highest precedence on stack */
            if(id_map[id] == *re) {
                if(sp > bp) {
                    while(prec_map[id] <= prec_map[*sp]) {
                        pop();
                    }
                }
                push(id);
                goto RELOOP;
            }
        }
        switch(*re) {
        case '(':
            push(1);
            goto RELOOP;
        case ')':
            while(*sp != 1) {
                /* couldn't find matching paren. send error */
                if(sp == bp) {
                    ret.re = NULL;
                    ret.err = PAREN_MISMATCH;
                    return ret;
                }
                pop();
            }
            /* pop without sending paren to buf */
            --sp;
            goto RELOOP;
        default:
            /* send non op to buf */
            *bufp = *re;
            bufp++;
        }
        RELOOP: ;
    }
    /* pop all leftover values in stack to buf */
    while(sp > bp) {
        /* error if unmatched open paren */
        if(*sp == 1) {
            ret.re = NULL;
            ret.err = PAREN_MISMATCH;
            return ret;
        }
        pop();
    }

    /* null terminate */
    *bufp = 0;

    return ret;
}

Header:
#include <string.h>

#define MAX_LINE 10000

/* error codes */
#define REGEX_TOOLARGE 1
#define PAREN_MISMATCH 2

Note: Further errors are caught in later stages of parsing within the program, but this post is just about the postfix conversion and the conversion itself is not meant to do a whole lot of syntactic and semantic parsing.
Examples:
a+a -> aa+
a+a* -> aa+*
a.(a+b)*.b -> aab+*.b.
a.(a+b)*.b() -> aab+*.b.
a.(a+b)*.b) -> PAREN_MISMATCH
a.(a+b)*.b( -> PAREN_MISMATCH
Any criticisms aimed towards improving the efficiency and readability of this code would be greatly appreciated.

General Observations
It is difficult to accurately define any bottle necks when only one function is presented. The brief moment when main() and match() were visible was very helpful, although it would have been nice if the body of match() was included as well.
It might be better to use a power of 2 (1024, 2048, ...) for MAX_LINE rather than a round number like 10000.
The code is overly complex and should be broken into multiple functions, this is actually proved by the multiple goto RELOOP; statements. These goto statements can be replaced by break; and continue and in one case by the return of a function. Try to avoid writing Spaghetti code.
Implement Stacks using Structs
It is much easier to maintain code when the stack pointer and the stack container (array) can be found in one place. Rather than write push and pop as macros, implement them as functions that take a stack struct, and in the case of push the parameter of what is being pushed on the stack.
Magic Numbers
While there are symbolic constants used rather than number constants in some parts of the code, this could be improved, it is also possible to use enums rather than #define to define symbolic constants in C and I would recommend using enums to represent the error ids because it is expandable.
typedef enum Error_Code
{
    REGEX_TOOLARGE = 1,
    PAREN_MISMATCH = 2
} Error_Code;

Just a quick though here, if the error codes start at 0 rather than 1 than any error messages could be stored as an array of strings.
The place where there are still magic numbers is in this code:
    int prec_map[OP_NUM] = { 1, 2, 3, 4, 4, 4 };

It isn't clear what any of those numbers mean.
It isn't clear that OP_NUM is necessary because the count can be determined by either one of the following:
    char id_map[] = { '(', '|', '.', '?', '+', '*' };
    const size_t OP_NUM = sizeof(id_map)/sizeof(*id_map);

or
    int prec_map[] = { 1, 2, 3, 4, 4, 4 };
    const size_t OP_NUM = sizeof(prec_map)/sizeof(*prec_map);

Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them.
Posible Optimization
Use strlen() only once and store the value in a variable.


Is there more efficient way of processing my XML file in PHP? The page is assigning $gid to the url paramater gid. It then reads an XML file that looks like this:

<games>
 <game>
  <name>name 1</name>
  <appID>1234</appID>
 </game>
 <game>
  <name>name 2</name>
  <appID>5678</appID>
 </game>
</games>

The code I am using works correctly but takes a second to load because there are so many <game> elements on the list. Is there are more efficient way to go about foreach? Here is the code:

$gid = (int) $_GET['gid'];
$gamespage = simplexml_load_file("http://gamepage.com/games?xml=1");
$games = $gamespage->games;

foreach ($games->game as $game) {
  if ($gid == $game->appID) {
    $appid = $game->appID;
    $gamename = $game->name;
  }
}

echo $gid, "
";
echo $appid, "
";
echo $gamename;

Add a break to that if statement to stop looping once you've found the correct node.


Prime factorization of numbers up to 1000 I was reading Mathematics: A Very Short Introduction and it mentioned prime factorization. Being a curious person I had to write my own implementation in Perl.

This program outputs the prime factorization of the numbers between 0 and 1001.

I don't like listing all of my subroutines before everything else, but I'm not sure what would be a better alternative.

Also, I remember seeing somewhere a non-bruteforce way of discovering whether a number is a prime. Does anyone have any ideas? I'm a beginner/intermediate Perl programmer and would appreciate suggestions on improving my technique.

#!/usr/bin/perl
use strict;
use warnings;
use 5.010;

#This array will contian the results of the factorization
our @result;

sub factorize {
  my($num,$factorsRef) = @_;

  # If the only factor is 1, it is a prime number
  # return the number itself since primes don't
  # factorize in the known universe
  if(${$factorsRef}[0] == 1) {
    push @result, $num;
    return;
  }

  if($num % ${$factorsRef}[0] == 0) {
    push @result, ${$factorsRef}[0];
    my $divResult = $num/${$factorsRef}[0];

    # If the result of the division is a prime
    # we have reached the end of the process
    if(isPrime($divResult)) {
      push @result, ($divResult);

    # If it is not a prime, go down to the
    # next level
    } else {
      factorize($divResult,$factorsRef);
    }

  # If the number is no longer divisible by the
  # current factor, take the factor out so that
  # the function can use te next factor
  } else {
    shift @{$factorsRef};
    factorize($num,$factorsRef);
  }
}

sub getPrimeFactors {
  my $num = shift;
  my $counter = 1;
  my @primeFactors;

  if(isPrime($num)) {
    push @primeFactors, 1;
    return \@primeFactors;
  }

  while($counter++ <= ($num / 2)) {
    next unless $num % $counter == 0;
    push @primeFactors, $counter if(isPrime($counter));
  }
  return \@primeFactors;
}

sub isPrime {
  my $num = shift;
  my $limit = $num/2;
  for(my $i=2; $i<=$limit ;$i++) {
    if ($num%$i == 0) { return 0;}
  }
  return 1;
}

sub printResults {
  my $num = shift;
  print $num . ' = ' . shift @result;
  print " x $_" for @result;
  print "\n";
}

# Where everything happens
for(1..1000) {
  my $num = $_;
  factorize($num,getPrimeFactors($num));
  printResults($num);

  @result = ();
}

Sample output:

983 = 983
984 = 2 x 2 x 2 x 3 x 41
985 = 5 x 197
986 = 2 x 17 x 29
987 = 3 x 7 x 47
988 = 2 x 2 x 13 x 19
989 = 23 x 43
990 = 2 x 3 x 3 x 5 x 11
991 = 991
992 = 2 x 2 x 2 x 2 x 2 x 31
993 = 3 x 331
994 = 2 x 7 x 71
995 = 5 x 199
996 = 2 x 2 x 3 x 83
997 = 997
998 = 2 x 499
999 = 3 x 3 x 3 x 37
1000 = 2 x 2 x 2 x 5 x 5 x 5

There are a few things which can improve this logic:

In order to find out whether a number n is prime, you only need to go up to sqrt(n), not n/2; that will speed up the prime validation considerably for large numbers.
Also, if you're verifying whether a sequence of numbers are primes, you should store the previous results instead of starting from scratch all the time. The function below calculates the primes from 2-1000, which is a good "pre-processing" step for your function (factorizing is now a matter of traversing only that prime numbers list)

Calculate prime numbers from 1..1000:

use strict;

my @primes;
my $i;

for my $i (2..1000) {
    my $isPrime = 1;
    my $sqrt = int(sqrt($i));
    for my $j (@primes) {
        if (($i % $j) == 0) {
            $isPrime = 0;
            last;
        }
        last if $j > $sqrt;
    }
    if ($isPrime) {
        push @primes, $i;
    }
}

print join(", ", @primes);


Add or remove a class on two elements when two different events fire I want to add/remove a class on the html and body tags in the dom when my nav menu is opened or closed. Is this the most optimal way of writing this logic?

$( '#navbarNav' ).on( 'show.bs.collapse', function() {
    $( 'html, body' ).addClass( 'noscroll' );
});

$( '#navbarNav' ).on( 'hide.bs.collapse', function() {
    $( 'html, body' ).removeClass( 'noscroll' );
});

Don't repeat the queries. Store any queries that are called more than once in a variable, and utilize jQuery's chaining:

const navbarNav = $( '#navbarNav' );
const htmlBody = $( 'html, body' );

navbarNav.on( 'show.bs.collapse', function() {
    htmlBody.addClass( 'noscroll' );
}).on( 'hide.bs.collapse', function() {
    htmlBody.removeClass( 'noscroll' );
});

Why are you setting the class on both the html and body elements? Either should be sufficient.

Name the class semantically. Just like you call a class for important text "important" and not "red", just because it happens to be red, you don't call a class that indicates whether the main navigation is open "noscroll" but "main-nav-open" (for example).


Mapping array to class properties I am wondering what is the best way to map array values to properties in a class. Consider the following sample array describing information for an airport:

        [0] "6523"  string
        [1] "00A"   string
        [2] "heliport"  string
        [3] "Total Rf Heliport" string
        [4] "40.07080078125"    string
        [5] "-74.9336013793945" string
        [6] "11"    string
        [7] "NA"    string
        [8] "US"    string
        [9] "US-PA" string
        [10]    "Bensalem"  string
        [11]    "no"    string
        [12]    "00A"   string
        [13]    ""  string
        [14]    "00A"   string
        [15]    ""  string
        [16]    ""  string
        [17]    ""  string

I have the following class:

    public class Airport
    {
        public Airport(string[] data)
        {
            Action<string>[] PropertyMappings =
            {
                x=>this.Id=x,
                x=>this.Ident=x,
                x=>this.Type=x,
                x=>this.Name=x,
                x=>this.Latitude=x,
                x=>this.Longtitude=x,
                x=>this.Elevation=x,
                x=>this.Continent=x,
                x=>this.CountryIso=x,
                x=>this.RegionIso=x,
                x=>this.Municipality=x,
                x=>this.ScheduledService=x,
                x=>this.GPSCode=x,
                x=>this.DataCode=x,
                x=>this.LocalCode=x,
                x=>this.HomeLink=x,
                x=>this.WikipediaLink=x,
                x=>this.Keywords=x
            };
            for(int i=0;i<data.Count();i++)
            {
                PropertyMappings[i](data[i]);
            }
        }
        public string Id { get; set; }
        public string Ident { get; set; }
        public string Type { get; set; }
        public string Name { get; set; }
        public string Latitude { get; set; }
        public string Longtitude { get; set; }
        public string Elevation { get; set; }
        public string Continent { get; set; }
        public string CountryIso { get; set; }
        public string RegionIso { get; set; }
        public string Municipality { get; set; }
        public string ScheduledService { get; set; }
        public string GPSCode { get; set; }
        public string DataCode { get; set; }
        public string LocalCode { get; set; }
        public string HomeLink { get; set; }
        public string WikipediaLink { get; set; }
        public string Keywords { get; set; }
    }
}

And I call it like this:

Airport airport = new Airport(data);

Do you think this is a good way to do the mapping from the elements of the array to the properties of the class or is there a better way. I couldn't really find anything online.
Obviously I haven't done all of the safety checks etc. This is just a small experiment.

As you said that you will always have the right number of elements in the array, I would suggest the following:

public class Airport
{
    public string Id { get; set; }
    public string Ident { get; set; }
    public string Type { get; set; }
    public string Name { get; set; }
    public string Latitude { get; set; }
    public string Longtitude { get; set; }
    public string Elevation { get; set; }
    public string Continent { get; set; }
    public string CountryIso { get; set; }
    public string RegionIso { get; set; }
    public string Municipality { get; set; }
    public string ScheduledService { get; set; }
    public string GPSCode { get; set; }
    public string DataCode { get; set; }
    public string LocalCode { get; set; }
    public string HomeLink { get; set; }
    public string WikipediaLink { get; set; }
    public string Keywords { get; set; }

    public static Airport CreateFromData(string[] data)
    {
        if (data.Length != 18) {
            throw new ArgumentException("...");
        }
        return new Airport
                   {
                       Id = data[0],
                       Ident = data[1],
                       Type = data[2],
                       Name = data[3],
                       Latitude = data[4],
                       Longtitude = data[5],
                       Elevation = data[6],
                       Continent = data[7],
                       CountryIso = data[8],
                       RegionIso = data[9],
                       Municipality = data[10],
                       ScheduledService = data[11],
                       GPSCode = data[12],
                       DataCode = data[13],
                       LocalCode = data[14],
                       HomeLink = data[15],
                       WikipediaLink = data[16],
                       Keywords = data[17]
                   };
    }
}

Depending on usage I would suggest that you alter the accessibility of the setters on the properties and consider making the default constructor protected (if you only want to create via a data array).

Why am I making this suggestion?

Keeps it simple
It's about 30 times faster than your method
You can view the code as a schema for the data array. I.e. Type is the third element

Edit

As RufusL notes in the comments, it would be a good idea to add a null check for the data array:

public static Airport CreateFromData(string[] data)
{
    if (data == null) {
        throw new ArgumentNullException("data");
    }
    if (data.Length != 18)
    {
        ...

Further to my comment about my proposed solution being faster - it's not because your code is slow in any way. If you want a dictionary of mappings of property -> index in data array your code will be slower but that probably wont matter.

You could just move your array of property mappings to a field. Just change the signature to Action<Airport,string>[] you'd then populate it like:

Action<Airport, string>[] PropertyMappings =
{
     (a, s) => a.Id = s,
     // ...
}

public static Airport CreateFromData(string[] data)
{
    // Guards omitted.
    var result = new Airport();
    for (var i = 0; i < data.Length; i++)
    {
        PropertyMappings(result, data[i]);
    }
    return result;
}

(all of the code in the edit has just been typed directly in the browser, if it doesn't compile, hopefully it should be close enough to correct.)