Comic Image Web Scraper I've written my first web scraper, which (surprisingly) does the job. I'm scraping a popular comic website for the images (there are over 950 of them) but the problem is the scraper is way too slow.

For example, if I download a sample of 10 comics it takes an average of 4 to 5 secs per image (a total of > 40 secs for the sample), which is a bit too slow if you ask me because each image is approx. the size of 80KB to 800KB.

I've read I could switch to LXML to do the scraping asynchronously but the package is not compatible with Python 3.6.

I'm submitting the scraper for both the code and performance review. Any insight on both topic will be highly appreciated.

import time
import os
import sys
import re
import requests
import itertools
from requests import get
from bs4 import BeautifulSoup as bs

HOME_DIR = os.getcwd()
DEFAULT_DIR_NAME = 'poorly_created_folder'

def show_logo():
  print("""
a Python comic(al) scraper for poorlydwarnlines.com
                         __
.-----.-----.-----.----.|  |.--.--.
|  _  |  _  |  _  |   _||  ||  |  |
|   __|_____|_____|__|  |__||___  |
|__|                        |_____|
                __ __   __
.--.--.--.----.|__|  |_|  |_.-----.-----.
|  |  |  |   _||  |   _|   _|  -__|     |
|________|__|  |__|____|____|_____|__|__|

.-----.----.----.---.-.-----.-----.----.
|__ --|  __|   _|  _  |  _  |  -__|   _|
|_____|____|__| |___._|   __|_____|__|
                      |__|
version: 0.2 | author: baduker | https://github.com/baduker
  """)

def handle_menu():
  print("\nThe scraper has found {} comics.".format(len(found_comics)))
  print("How many comics do you want to download?")
  print("Type 0 to exit.")

  while True:
    try:
      global n_of_comics
      n_of_comics = int(input(">> ").strip())
    except ValueError:
      print("Error: incorrect value. Try again.")
      continue
    if n_of_comics > len(found_comics) or n_of_comics < 0:
      print("Error: incorrect number of comics to download. Try again.")
      continue
    elif n_of_comics == 0:
      sys.exit()
    else:
      break
  return n_of_comics

def move_to_dir(title):
  if os.getcwd() != HOME_DIR:
    os.chdir(HOME_DIR)
  try:
    os.mkdir(title)
    os.chdir(title)
  except FileExistsError:
    os.chdir(title)
  except:
    print("Couldn't create directory!")

def generate_comic_link(array, num):
  for link in itertools.islice(array, 0, num):
    yield link

def grab_image_src_url(link):
  req = requests.get(link)
  comic = req.text
  soup = bs(comic, 'html.parser')
  for i in soup.find_all('p'):
    for img in i.find_all('img', src=True):
      return img['src']

def download_image(link):
  file_name = url.split('/')[-1]
  with open(file_name, "wb") as file:
    response = get(url)
    file.write(response.content)

def fetch_comic_archive():
  url = 'http://www.poorlydrawnlines.com/archive/'
  req = requests.get(url)
  page = req.text
  soup = bs(page, 'html.parser')
  all_links = []
  for link in soup.find_all('a'):
    all_links.append(link.get('href'))
  return all_links

def filter_comic_archive(archive):
  pattern = re.compile(r'http://www.poorlydrawnlines.com/comic/.+')
  filtered_links = [i for i in archive if pattern.match(i)]
  return filtered_links

show_logo()

all_comics = fetch_comic_archive()
found_comics = filter_comic_archive(all_comics)

handle_menu()

start = time.time()
for link in generate_comic_link(found_comics, n_of_comics):
  print("Downloading: {}".format(link))
  move_to_dir(DEFAULT_DIR_NAME)
  url = grab_image_src_url(link)
  download_image(url)
end = time.time()

print("Successfully downloaded {} comics in {:.2f} seconds.".format(n_of_comics, end - start))

This seems like a cool project, and a nice first introduction to web scraping! I'll cover some general advice first, and then address your main concern: speed.

General

You use two spaces for indentation. PEP-8 recommends 4 spaces (most code editors have the option to automatically convert tabs to spaces, which I highly recommend).
While on the subject, PEP-8 also recommends two blank lines between top-level functions.
In general, the use of globals is discouraged. There are some exceptions:

Global constants are okay;
Under certain circumstances, avoiding global state would result in unacceptably complicated code.

However, neither of these apply here. Using global, non-const variables makes your code harder to debug, and is a burden for anyone reading / updating it in the future. You can avoid global state here easily by passing variables as parameters.
In my opinion, show_logo() is a bit overkill. I'd make it a constant instead (LOGO).
The while loop in handle_menu() can be improved:

while True:
  try:
    global n_of_comics
    n_of_comics = int(input(">> ").strip())

There's no need to call str.strip() before passing the return value to int.

  except ValueError:
    print("Error: incorrect value. Try again.")
    continue

The error message is a bit vague. Why is the value incorrect? Maybe something like 'Error: expected a number. Try again.' would fit better.

  if n_of_comics > len(found_comics) or n_of_comics < 0:
    print("Error: incorrect number of comics to download. Try again.")
    continue

This error message is quite clear :)

  elif n_of_comics == 0:
    sys.exit()
  else:
    break

You don't need the else clause here, since the elif clause stops the program if executed. In fact, you can simply return n_of_comics there. Putting this all together:

while True:
  try:
    global n_of_comics
    n_of_comics = int(input(">> "))
  except ValueError:
    print("Error: expected a number. Try again.")
    continue
  if n_of_comics > len(found_comics) or n_of_comics < 0:
    print("Error: incorrect number of comics to download. Try again.")
    continue
  elif n_of_comics == 0:
    sys.exit()
  return n_of_comics

Your 'main routine' isn't encapsulated in a function, which means it's hard to test / explore functions individually from a Python interactive session (or from a separate file). I suggest putting all this top level code into a main() function:

def main():
    show_logo()

    all_comics = fetch_comic_archive()
    found_comics = filter_comic_archive(all_comics)

    handle_menu()

    start = time.time()
    for link in generate_comic_link(found_comics, n_of_comics):
        print("Downloading: {}".format(link))
        move_to_dir(DEFAULT_DIR_NAME)
        url = grab_image_src_url(link)
        download_image(url)
    end = time.time()

    print("Successfully downloaded {} comics in {:.2f} seconds.".format(n_of_comics, end - start))

You can then check if __name__ == "__main__", to run main if the script is called as the main program (see 'What does if __name__ == "__main__" do?'):

if __name__ == "__main__":
    main()

I don't get the point of move_to_dir(). It handles the case where the current working directory has been changed, and it's called for each comic to download. That seems pretty pointless to me. Instead, I would only create the directory once, in main():

DEFAULT_DIR_NAME = "poorly_created_folder"
COMICS_DIRECTORY = os.path.join(os.getcwd(), DEFAULT_DIR_NAME)

...

def download_image(link):
    ...
    with open(os.path.join(COMICS_DIRECTORY, file_name), "wb") as file:
        ...

...

def main():
    ...
    try:
        os.mkdir(COMICS_DIRECTORY)
    except OSError as exc:
        sys.exit("Failed to create directory (errno {})".format(exc.errno))
        # `sys.exit` will write the message to stderr and return with status code 1

generate_comic_link() is superfluous. The following functions all do the same:

def generate_comic_link(array, num):
    # Using itertools.islice()
    for link in itertools.islice(array, 0, num):
        yield link

def generate_comic_link2(array, num):
    # Using slicing
    for link in array[:num]:
        yield link

def generate_comic_link3(array, num):
    # Using slicing with yield from
    yield from array[:num]

itertools.islice() is overkill (and a little harder to read). Since generate_comic_link3() is a one-liner, you can probably get rid of the function altogether, iterating over the URLs directly using slicing.
A bit of a nitpick, but req = <requests>.get(<url>) is wrong. requests.get doesn't return the request, it returns the response. Thus, response = <requests>.get(<url>) makes more sense.
Some of your variables could (should) be constants:

url = 'http://www.poorlydrawnlines.com/archive/'
# Might be
ARCHIVE_URL = "http://www.poorlydrawnlines.com/archive/"

pattern = re.compile(r'http://www.poorlydrawnlines.com/comic/.+')
# Might be
COMIC_PATTERN = re.compile(r"http://www.poorlydrawnlines.com/comic/.+")

(I prefer double quotes, as you might be able to tell)
In fetch_comic_archive():

all_links = []
for link in soup.find_all('a'):
    all_links.append(link.get('href'))
return all_links

... might be a one-liner:

return [link.get("href") for link in soup.find_all("a")]

In filter_comic_archive():

filtered_links = [i for i in archive if pattern.match(i)]
return filtered_links

There's no need for the intermediate variable.

Using threading.Thread

Now for the actual challenge: improving performance.
You don't need asynchronous lxml, threads will do just fine here!
By wrapping the relevant code into a function, we can spawn any amount of threads to do the work:

import threading

def download_comic(link):
    print("Downloading: {}".format(link))
    move_to_dir(DEFAULT_DIR_NAME)
    url = grab_image_src_url(link)
    download_image(url)

...

def main():
    ...
    for link in generate_comic_link(found_comics, n_of_comics):
        thread = threading.Thread(target=download_comic, args=(link,))
        thread.start()
    thread.join()
    # Join the last thread to make sure all comics have been
    # downloaded before printing the time difference
    ...

Rewrite

Putting everything together (I've changed names, reordered some functionality and added comments explaining certain changes):

import time
import os
import sys
import re
import threading

# PEP-8 recommends a blank line in between
# stdlib imports and third-party imports.

import requests
# Importing `requests` *and* `get` from `requests` is confusing
from bs4 import BeautifulSoup as bs

DEFAULT_DIR_NAME = "poorly_created_folder"
COMICS_DIRECTORY = os.path.join(os.getcwd(), DEFAULT_DIR_NAME)
LOGO = """
a Python comic(al) scraper for poorlydwarnlines.com
                         __
.-----.-----.-----.----.|  |.--.--.
|  _  |  _  |  _  |   _||  ||  |  |
|   __|_____|_____|__|  |__||___  |
|__|                        |_____|
                __ __   __
.--.--.--.----.|__|  |_|  |_.-----.-----.
|  |  |  |   _||  |   _|   _|  -__|     |
|________|__|  |__|____|____|_____|__|__|

.-----.----.----.---.-.-----.-----.----.
|__ --|  __|   _|  _  |  _  |  -__|   _|
|_____|____|__| |___._|   __|_____|__|
                      |__|
version: 0.2 | author: baduker | https://github.com/baduker
"""
ARCHIVE_URL = "http://www.poorlydrawnlines.com/archive/"
COMIC_PATTERN = re.compile(r"http://www.poorlydrawnlines.com/comic/.+")

def download_comics_menu(comics_found):
    print("\nThe scraper has found {} comics.".format(len(comics_found)))
    print("How many comics do you want to download?")
    print("Type 0 to exit.")

    while True:
        try:
            comics_to_download = int(input(">> "))
        except ValueError:
            print("Error: expected a number. Try again.")
            continue
        if comics_to_download > len(comics_found) or comics_to_download < 0:
            print("Error: incorrect number of comics to download. Try again.")
            continue
        elif comics_to_download == 0:
            sys.exit()
        return comics_to_download

def grab_image_src_url(url):
    response = requests.get(url)
    soup = bs(response.text, "html.parser")
    for i in soup.find_all("p"):
        for img in i.find_all("img", src=True):
            return img["src"]

def download_and_write_image(url):
    # `download_and_write_image` is a bit more accurate, since
    # it also writes the image to the disk
    file_name = url.split("/")[-1]
    with open(os.path.join(COMICS_DIRECTORY, file_name), "wb") as file:
        response = requests.get(url)
        # Replced `get` with `requests.get`
        file.write(response.content)

def fetch_comics_from_archive():
    # Merged `fetch_comic_archive` and `filter_comic_archive`
    # into a single function
    response = requests.get(ARCHIVE_URL)
    soup = bs(response.text, "html.parser")
    comics = [url.get("href") for url in soup.find_all("a")]
    return [url for url in comics if COMIC_PATTERN.match(url)]

def download_comic(url):
    print("Downloading: {}".format(url))
    url = grab_image_src_url(url)
    download_and_write_image(url)

def main():
    print(LOGO)

    comics = fetch_comics_from_archive()
    comics_to_download = download_comics_menu(comics)

    try:
        os.mkdir(DEFAULT_DIR_NAME)
    except OSError as exc:
        sys.exit("Failed to create directory (errno {})".format(exc.errno))

    start = time.time()
    for url in comics[:comics_to_download]:
        thread = threading.Thread(target=download_comic, args=(url,))
        thread.start()
    thread.join()

    end = time.time()
    print("Successfully downloaded {} comics in {:.2f} seconds.".format(
        comics_to_download, end - start)
    )

if __name__ == "__main__":
    main()

Results

Non-threaded:

The scraper has found 957 comics.
How many comics do you want to download?
Type 0 to exit.
>> 6
Downloading: http://www.poorlydrawnlines.com/comic/new-phone/
Downloading: http://www.poorlydrawnlines.com/comic/new-things/
Downloading: http://www.poorlydrawnlines.com/comic/return-to-nature/
Downloading: http://www.poorlydrawnlines.com/comic/phone/
Downloading: http://www.poorlydrawnlines.com/comic/stars/
Downloading: http://www.poorlydrawnlines.com/comic/big-dreams/
Successfully downloaded 6 comics in 37.13 seconds.

Threaded:

The scraper has found 957 comics.
How many comics do you want to download?
Type 0 to exit.
>> 6
Downloading: http://www.poorlydrawnlines.com/comic/new-phone/
Downloading: http://www.poorlydrawnlines.com/comic/new-things/
Downloading: http://www.poorlydrawnlines.com/comic/return-to-nature/
Downloading: http://www.poorlydrawnlines.com/comic/phone/
Downloading: http://www.poorlydrawnlines.com/comic/stars/
Downloading: http://www.poorlydrawnlines.com/comic/big-dreams/
Successfully downloaded 6 comics in 7.07 seconds.

Alternatively: using concurrent.futures.ThreadPoolExecutor

If you run the rewritten code, you may notice the program takes a couple of seconds to shut down after (reportedly) having downloaded all images. This is because the last thread started doesn't necessarily end last (that's the whole point of threads, after all!). To avoid this, and to get rid of some of the boilerplate code, we can use ThreadPoolExecutor.map() and ThreadPoolExecutor.shutdown().

I've created a GitHub gist here which uses ThreadPoolExecutor together with requests.Session, which reuses the underlying TCP connection, potentially resulting in even better performance.


Steganographic message hiding This code hides a message in an image by altering the blue values in the top left part. This is just for fun as I think it would be easy for a cryptographer to find and decrypt this.

Original

With message We will meet at London bridge at dawn

As you can (cannot) see, as long as the message is short enough, the naked eye has difficulty noticing the difference. Zooming in the top left corner will reveal the trick though.

from PIL import Image
from itertools import zip_longest
import doctest

def read_and_write_after_tranformation(infile, outfile, transformation):
    # Based on a StackOverflow answer.
    im = Image.open(infile)
    list_of_pixels = list(im.getdata())
    im2 = Image.new(im.mode, im.size)
    im2.putdata(transformation(list_of_pixels))
    im2.save(outfile)

def read_rgb(infile):
    return list(Image.open(infile).getdata())

def steganographic_encrypt(message, rgb_tuples):
    """
    Encodes a message in the blue values of the pixels on the top left corner of the image.

    >>> steganographic_encrypt("Hello, World!", [(a, b, c, luminosity) for a, b, c, luminosity in zip(range(12, 30), range(20, 48), range(100, 128), range(100, 128))])
    [(12, 20, 72, 100), (13, 21, 101, 101), (14, 22, 108, 102), (15, 23, 108, 103), (16, 24, 111, 104), (17, 25, 44, 105), (18, 26, 32, 106), (19, 27, 87, 107), (20, 28, 111, 108), (21, 29, 114, 109), (22, 30, 108, 110), (23, 31, 100, 111), (24, 32, 33, 112), (25, 33, 113, 113), (26, 34, 114, 114), (27, 35, 115, 115), (28, 36, 116, 116), (29, 37, 117, 117)]
    """
    return [(red, green, blue if ascii_val is None else ascii_val, luminosity)
              for (red, green, blue, luminosity), ascii_val in zip_longest(rgb_tuples, map(ord, message))]

def steganographic_decrypt(rgb_tuples):
    """
    Decodes a message from the blue values of the pixels on the top left corner of the image.

    >>> steganographic_decrypt([(12, 20, 72, 100), (13, 21, 101, 101), (14, 22, 108, 102), (15, 23, 108, 103), (16, 24, 111, 104), (17, 25, 44, 105), (18, 26, 32, 106), (19, 27, 87, 107), (20, 28, 111, 108), (21, 29, 114, 109), (22, 30, 108, 110), (23, 31, 100, 111), (24, 32, 33, 112), (25, 33, 113, 113), (26, 34, 114, 114), (27, 35, 115, 115), (28, 36, 116, 116), (29, 37, 117, 117)])
    'Hello, World!qrstu'
    """
    return ''.join([chr(blue) for (red, green, blue, luminosity) in rgb_tuples])

if __name__ == "__main__":
    doctest.testmod()
    read_and_write_after_tranformation(
        "cc.png",
        "cc2.png",
        lambda image: steganographic_encrypt("We will meet at London bridge at dawn", image)
    )

    print(steganographic_decrypt(read_rgb("cc2.png"))[0:100])

Working with resources

In read_and_write_after_tranformation you didn't bother to close any of the image resources:

# Based on a StackOverflow answer.
im = Image.open(infile)
list_of_pixels = list(im.getdata())
im2 = Image.new(im.mode, im.size)
im2.putdata(transformation(list_of_pixels))
im2.save(outfile)

Don't believe everything you read on Stack Overflow =)

You can wrap those in a with ... as. And while at it,
im and im2 are not great names.

with Image.open(infile) as in_img:
    list_of_pixels = list(in_img.getdata())
    with Image.new(in_img.mode, in_img.size) as out_img:
        out_img.putdata(transformation(list_of_pixels))
        out_img.save(outfile)

Usability

This is a nice script.
It would be great to make it more usable,
with command line parameters:

hide sub-command: message to hide, input file, output file
show sub-command: file

You probably already know this, but argparse would be a good tool for this job.


Outlook VBA-Macro for to check mails concerning recipients and attachments Every week I've got to send a report to a clerk. I also have to add his deputy in case the clerk isn't available.
For not to forget the second recipient and the attachment I've wrote myself this VBA-Macro.
Perhaps one could enhance it in a way that it works with more then two recipients?
Any comments and suggestions welcome!

' Main Sub which defines the mail-addresses to check. Then checks if one
'  of the addresses is used. In case of used it checks the remaining
'  constraints.

Private Sub Application_ItemSend(ByVal Item As Object, Cancel As Boolean)
    Dim firstRecipient As String

    ' Define the addresses to check for.
    Const FIRST_ADDRESS = "main.clerk@smth.com" ' Main recipient clerk
    Const SECOND_ADDRESS = "deputy.clerk@smth.com" ' Deputy

    On Error GoTo stopSending

    ' Getting the first mail address from the outlook GUI.
    firstRecipient = Application.ActiveInspector.CurrentItem.Recipients.Item(1).Address

    ' If the first recipient is the specified clerk then check for the deputy.
    If (InStr(1, firstRecipient, FIRST_ADDRESS, vbTextCompare) <> 0) Then

      If checkStateOfMail(SECOND_ADDRESS, True) = False Then
        Cancel = True
        Exit Sub
      End If

    ' If the first recipient is the deputy then check for the specified clerk.
    ElseIf (InStr(1, firstRecipient, SECOND_ADDRESS, vbTextCompare) <> 0) Then

      If checkStateOfMail(FIRST_ADDRESS, True) = False Then
        Cancel = True
        Exit Sub
      End If

    End If

    ' ... if none of both is specified as recipient then just go on.

    Exit Sub

stopSending:
    MsgBox "Runtime error occured.", vbCritical, "Runtime error!"
    Cancel = True

End Sub

' If a doubtful condition has been detected then the user is asked if to cancel
'  cancel or not.

' Parameter
' 1. String - Text for the message box.
' 2. String - Title for the message box.

' Return
' Boolean - Answer if sending shall be cancelled or not.

Function askToCancel(boxMessage As String, boxTitle As String) As Boolean
  Dim answer As String

  answer = MsgBox(boxMessage & vbLf & "Send mail anyway?", vbYesNoCancel, boxTitle)

  If answer = vbNo Then
    askToCancel = True
  ElseIf answer = vbCancel Then
    askToCancel = True
  Else
    askToCancel = False
  End If

End Function

' Optional: Checks if an mail-attachment exists.
' Checks if two recipients exist. If case of two recipient it checks the
' correctness of the second address.

' Parameter
' 1. String - Expected second address.
' 2. Boolean - Checking for attachment or not.

' Return
' Boolean - Mail in correct state or not.

Function checkStateOfMail(expectedSecond As String, checkAttachment As Boolean) As Boolean
  Dim olObj As Object
  Dim attachmentCount As Variant
  Dim secondRecipient As String

  On Error GoTo errorOccured

  Set olObj = Application.ActiveInspector.CurrentItem

  If checkAttachment = True Then
    ' Get the count of mail attachments.
    attachmentCount = olObj.Attachments.count

    If attachmentCount = 0 Then

      If askToCancel("E-Mail Attachment doesn't exist.", "Caution - Attachment") = True Then
        checkStateOfMail = False
        Exit Function
      End If

    End If
  End If
  ' Two recipients are expected.
  If olObj.Recipients.count < 2 Then

    If askToCancel("Second recipient not available.", "Forgot 2nd recipient?") = True Then
      checkStateOfMail = False
      Exit Function
    End If

  Else
    ' Two recipients are available. But the second address could be wrong.
    secondRecipient = olObj.Recipients.Item(2).Address

    If InStr(1, secondRecipient, expectedSecond, vbTextCompare) = 0 Then

      If askToCancel("Second recipient isn't correct.", "Caution!") = True Then
        checkStateOfMail = False
        Exit Function
      End If

    End If

  End If

  checkStateOfMail = True
  Exit Function

errorOccured:
    MsgBox "Runtime error occured in sub procedure.", vbCritical, "Runtime error!"
    Exit Function

End Function

For multiple address, why not have a collection of emails?

Public Function GetSendToList () As Collection

    Dim emailList As Collection
    Set emailList = New Collection

    emailList.Add "main.clerk@smth.com"
    emailList.Add "deputy.clerk@smth.com"
    emailList.Add etc.

    Set GetSendToList = emailList

End Function

And then:

Dim emailList as Collection
Set emailList = GetSendToList

...
...
...

For i = 1 to emailList.Count
    emailToCheck = emailList(i)
    '/ code to check the email against your message
Next i

This means that you can:

Keep your hard-coded list away from your business logic, making both
cleaner and easier to find
Have just one place you go to to add/remove emails from your list, rather than wading through a sub to find it.
Have just one (validate email) loop, so you don't have to repeat your code for each one.

In my projects, I often have a separate module for "Manually-Set Values" but that's just personal preference.


Optimize Recursive Fetching of information? I wrote a small class that demonstrates my problem:

class Root
{
    private Leaf RootLeaf;
    private Dictionary<object, Leaf> AllItems = new Dictionary<object, Leaf>();  //dictionary contains a reference to each item, and the leaf it is assigned to (its parent)
    public Root()
    {
        RootLeaf = new Leaf();
    }

    public List<object> GetAllChildren
    {
        get { return RootLeaf.getAllChildren; }
    }

    public void Add(object o)
    {
        Leaf AddedTo = RootLeaf.Add(o);
        AllItems.Add(o, AddedTo); //Add object reference to AllItems dictionary
    }

    private IEnumerable<object> GetNeighbors(object obj)
    {
        foreach (object o in this.AllItems[obj].getAllChildren)
        {
            if (obj != o)
                yield return o;
        }
    }

    public void Update()
    {
        foreach (KeyValuePair<object, Leaf> i in AllItems)
        {
            foreach (object b in this.GetNeighbors(i.Key))
            {
                //Would do collision checks here
            }
        }
    }
}

class Leaf
{
    private const int MaxChildCount = 1;

    private List<object> Children = new List<object>();

    private Leaf[] ChildLeaves = null;
    private bool _LeavesGenerated = false; //Have the leaves been created? (Low Level, do not touch)
    private bool HasLeaves = false; //Should we use the leaves?

    protected void CreateLeaves()
    {
        if (!_LeavesGenerated)
        {
            //create each of the four leaves
            for (int i = 0; i < 4; i++)
                ChildLeaves[i] = new Leaf();
            _LeavesGenerated = true;
        }
        HasLeaves = true;
    }
    protected void RemoveLeaves()
    {
        HasLeaves = false;
    }

    /// <summary>
    /// Returns children of this leaf, and all of its subleaves
    /// </summary>
    public List<object> getAllChildren
    {
        get
        {
            List<object> outp = Children.ToList();
            if (HasLeaves)
            {
                foreach (Leaf l in ChildLeaves)
                    outp.AddRange(l.getAllChildren);
            }
            return outp;
        }
    }
    /// <summary>
    /// Get count of all children in this leaf, and its subleaves
    /// </summary>
    public int getChildCount
    {
        get
        {
            int outp = Children.Count;
            if (HasLeaves)
            {
                foreach (Leaf l in ChildLeaves)
                    outp += l.getChildCount;
            }
            return outp;
        }
    }

    static Random rand = new Random();
    /// <summary>
    ///
    /// </summary>
    /// <param name="o">The object to be added</param>
    /// <returns>The leaf the object was added to</returns>
    public Leaf Add(object o)
    {
        if (Children.Count >= MaxChildCount)
        {
            //Pick random subleaf, I know this isn't correct for a quadtree, but it will simplify this explanation code
            if (!HasLeaves)
                CreateLeaves();
            return ChildLeaves[rand.Next(0, 3)].Add(o);
        }
        else
        {
            Children.Add(o);
            return this;
        }
    }
}

I ran this through the ANTS profiler, and it not surprisingly pulled the Leaf.getAllChildren and Leaf.getChildCount as the two most expensive operations.  I'm ok with how everything thing else is laid out.  My question is: how could I optimize the two aforementioned properties?

The properties are called when the Root.Update() function is run.

All right, I've done a bit of work here (a good most of it stylistic, to be sure).  Do note I replaced object with a generic implementation to help avoid boxing of value types.  It does also employ LINQ in a couple instances to remove some complexity.  When I run it and generate 1,000,000 integer leaves, I find that the bulk of the time is taken in the class constructors and the methods themselves are close enough to 0 time to be considered 0.  Hope this helps:

internal sealed class Root<T>
{
    private readonly Leaf<T> rootLeaf = new Leaf<T>();

    // dictionary contains a reference to each item, and the leaf it is assigned to (its parent)
    private readonly IDictionary<T, Leaf<T>> allItems = new Dictionary<T, Leaf<T>>();

    /// <summary>
    /// Gets GetAllChildren.
    /// </summary>
    public IList<T> GetAllChildren
    {
        get
        {
            return this.rootLeaf.GetAllChildren;
        }
    }

    public void Add(T o)
    {
        var addedTo = this.rootLeaf.Add(o);

        this.allItems.Add(o, addedTo); // Add object reference to AllItems dictionary
    }

    public void Update()
    {
        foreach (var i in from i in this.allItems from b in this.GetNeighbors(i.Key) select i)
        {
            // Would do collision checks here
        }
    }

    private IEnumerable<T> GetNeighbors(T obj)
    {
        return this.allItems[obj].GetAllChildren.Where(o => ReferenceEquals(obj, o));
    }
}

internal class Leaf<T>
{
    private const int MaxChildCount = 1;

    private static readonly Random rand = new Random();

    private readonly IList<T> children = new List<T>();

    private List<Leaf<T>> childLeaves;

    private bool hasLeaves; // Should we use the leaves?

    private bool leavesGenerated; // Have the leaves been created? (Low Level, do not touch)

    /// <summary>
    /// Returns children of this leaf, and all of its subleaves
    /// </summary>
    public IList<T> GetAllChildren
    {
        get
        {
            var allChildren = this.children.ToList();

            if (this.hasLeaves)
            {
                this.childLeaves.ToList().ForEach(l => allChildren.AddRange(l.GetAllChildren));
            }

            return allChildren;
        }
    }

    /// <summary>
    /// Get count of all children in this leaf, and its subleaves
    /// </summary>
    public int GetChildCount
    {
        get
        {
            var allChildrenCount = this.children.Count;

            if (this.hasLeaves)
            {
                allChildrenCount += this.childLeaves.Sum(l => l.GetChildCount);
            }

            return allChildrenCount;
        }
    }

    /// <summary>
    ///
    /// </summary>
    /// <param name="o">The object to be added</param>
    /// <returns>The leaf the object was added to</returns>
    public Leaf<T> Add(T o)
    {
        if (this.children.Count < MaxChildCount)
        {
            this.children.Add(o);
            return this;
        }

        // Pick random subleaf, I know this isn't correct for a quadtree, but it will simplify this explanation code
        if (!this.hasLeaves)
        {
            this.CreateLeaves();
        }

        return this.childLeaves[rand.Next(0, 3)].Add(o);
    }

    protected void CreateLeaves()
    {
        if (!this.leavesGenerated)
        {
            // create each of the four leaves
            this.childLeaves = new List<Leaf<T>> { new Leaf<T>(), new Leaf<T>(), new Leaf<T>(), new Leaf<T>() };

            this.leavesGenerated = true;
        }

        this.hasLeaves = true;
    }

    protected void RemoveLeaves()
    {
        this.hasLeaves = false;
    }
}


Simple SVM in MATLAB I'm studying SVMs and wrote a demo in MATLAB (because I couldn't get a quadratic programming package to work correctly in Python). Right now it's simple and can only do linearly-separable cases (nonlinear kernels will be implemented later). This is the first MATLAB program I've written outside of a MATLAB seminar I'm taking, so any thoughts would be appreciated.

% simple SVM demo
clc;clear all;

% input data; y's are labels for data
y = [ -1; -1; -1; -1; -1; 1; 1; 1; 1; 1; 1 ];
data = [
    0 3;
    1 2;
    2 1;
    3 3;
    5 1;
    5 5;
    6 5;
    6 7;
    7 3;
    8 6;
    8 1;
];

% separate positive and negative datapoints
posDps = data(find(y == 1), :);
negDps = data(find(y == -1), :);

% space is number of dimensions (n-space)
% right now, b/c being plotted in 2D, can only be 2-space
space = size(data, 2);
% n is number of input variables
n = length(data);
[ y1, y2 ] = meshgrid(y, y);
[ i, j ] = meshgrid(1:n, 1:n);

% generate matrices for minimization
P(i, j) = y1(i, j) .* y2(i, j) .* (data(i,:) * data(j,:)');
q = -1 * ones(n, 1);

% generate matrices for inequality constraint (alpha >= 0)
% also can use LB parameter
A = -1 * eye(n);
b = zeros(n, 1);

% generate matrices for equality constraint
Aeq = y';
beq = [ 0 ];

% use quadprog package to generate alphas
alpha = quadprog(P, q, A, b, Aeq, beq, [], [], [], optimoptions('quadprog', 'Display', 'off'));
% calculate w from alpha
w = (data' .* repmat(y', [space 1])) * alpha;

% finding b parameter
% (y_n)(x_n * w + b) = 1 for support vector, so b = y_n - w * x_n
threshold = 1e-5;
svIndices = find(alpha > threshold);
b = y(svIndices(1)) - data(svIndices(1),:) * w;

% display points
figure;
hold on;
scatter(posDps(:,1), posDps(:,2));
scatter(negDps(:,1), negDps(:,2));

% draw line (only 2D for now)
margin = 1;
domain = (min(data(:,1)) - margin):(max(data(:,1)) + margin);
plot(domain, (w(1) .* domain + b)/(-w(2)));

% plotting gutters
plot(domain, (-1 + w(1) .* domain + b)/(-w(2)), 'g:');
plot(domain, (1 + w(1) .* domain + b)/(-w(2)), 'g:');

I'd like any feedback, but here are some specific questions I had in mind:

For storing one-dimensional data (arrays), is there usually a preference between column and row matrices?
Is there an easy way to deal with nested matrices? I tried to combine the labels and attribute vectors like so:

data = [
    -1 [ 0 3 ];
    -1 [ 1 2 ];
    % ...
]

and use more indices (e.g., data(1,2,2)) but that didn't work.
There are a ton of different ways to create matrices, and I'm not sure if the ones I am using are the most appropriate (e.g., via literals, find(), meshgrid(), ones() and zeros(), repmat(), etc.).
What is there to do about arbitrary imprecision? For example, I had to put in an arbitrary value threshold to find non-zero values because using find(A > 0) wasn't working. (Since the values were on the magnitude of about 1e-12, this was larger than the value of eps() so that function didn't seem very helpful.)
Naming conventions -- is there one for MATLAB? I don't believe I've seen a consistent style.

First to answer your specific questions:

  For storing one-dimensional data (arrays), is there usually a preference between column and row matrices?

It doesn't matter, use what is most convenient. Often horizontal vectors are used because they are easy to create: 1:10.

  Is there an easy way to deal with nested matrices?

The best way to collect matrices is using a cell array. A cell array's elements are arbitrary values (matrices, other cell arrays, objects, function handles, whatever you can assign to a variable).

  There are a ton of different ways to create matrices, and I'm not sure if the ones I am using are the most appropriate (e.g., via literals, find(), meshgrid(), ones() and zeros(), repmat(), etc.).

Yes, MATLAB has a huge library and it is impossible to know if you are using the best possible function in each case. Experience will help.

  What is there to do about arbitrary imprecision? For example, I had to put in an arbitrary value threshold to find non-zero values because using find(A > 0) wasn't working. (Since the values were on the magnitude of about 1e-12, this was larger than the value of eps() so that function didn't seem very helpful.)

Yes, you need to pick some threshold there. It depends on the source of the data what an appropriate value is. eps is the distance between 1.0 and the next representable floating-point value. That is, there are no possible values in between 1.0 and 1.0+eps. eps is not necessarily an appropriate threshold to find "approximately zero" values.

  Naming conventions -- is there one for MATLAB? I don't believe I've seen a consistent style.

Not that I know of. Early versions of MATLAB were not case sensitive, so all code was typically written in lower case. Currently some built-in functionality uses an upper case for the first letter, and some use camelCase. There is no PEP8 for MATLAB. :)

Now I'll go through the bits of code that can be improved:

clc;clear all;

Please use the MATLAB Editor, and heeds its warnings. clear all is not useful here, it not only clears variables, it also clears functions from memory. These will need to be read in and parsed again, slowing down your program. clear suffices to remove variables from your workspace.

posDps = data(find(y == 1), :);
negDps = data(find(y == -1), :);

Here you can remove find. The MATLAB Editor also warns about this.

posDps = data(y == 1, :);
negDps = data(y == -1, :);

find is usually not necessary when indexing, only when you need to store an index to an array element.

space = size(data, 2);
n = length(data);

It is always better to specify directly what size you need. length(data) is defined as max(size(data)), I consider this dangerous. Instead:

[n,space] = size(data);

or

space = size(data, 2);
n = size(data, 1);

You should also know numel, which returns the number of elements in a matrix. This is much more efficient than prod(size(data)).

[ y1, y2 ] = meshgrid(y, y);
[ i, j ] = meshgrid(1:n, 1:n);

These are not necessary at all. Since MATLAB R2016b we have implicit singleton expansion, but before that you could use bsxfun to operate on two arrays of different sizes. So none of the following indexing operations are really necessary.

P(i, j) = y1(i, j) .* y2(i, j) .* (data(i,:) * data(j,:)');

can be rewritten as

P = (y .* y.') .* (data * data.');

Note that I use .' here, where you had '. ' is the complex conjugate transpose. Unless you want to compute a dot product, you should avoid it. .' transposes the matrix without changing any of its values. For real-valued data they're the same, but it's good to get used to using the right form, it will prevent difficult to find errors down the road.

Aeq = y';

Same here: y.'.

beq = [ 0 ];

Again, the MATLAB Editor warns here about the useless [] brackets. Heed the warnings!

There is actually no difference in MATLAB between a matrix and a scalar. A scalar is a matrix with a single value. The [...] operator is for concatenation. It concatenates two or more matrices into a single matrix. Every time you have a single matrix inside [], the MATLAB Editor will warn you about it.

Similarly, I often see things like [1:10]. 1:10 is a matrix, the [] don't do anything there.

w = (data' .* repmat(y', [space 1])) * alpha;

Same here: data.', y.'.

svIndices = find(alpha > threshold);
b = y(svIndices(1)) - data(svIndices(1),:) * w;

Here you can use the second argument to find to tell it to give you only one index:

svIndex = find(alpha > threshold,1);
b = y(svIndex) - data(svIndex,:) * w;

domain = (min(data(:,1)) - margin):(max(data(:,1)) + margin);

You don't need this many parentheses here, but they don't hurt either. I actually think they help readability. But the : has a much lower precedence than + and -. This should also work:

domain = min(data(:,1))-margin : max(data(:,1))+margin;


Optimise JavaScript loop I have this code:

Tabzilla.fillZGContacts = function(){
   if (!Tabzilla.panel.id) return;
   $.ajax({
       url: 'zgcontacts.json',
      success: function(d){   // "Type","Name","Link","Contact","Location","Icon"
        Tabzilla.zgContacts = d;
        var countries = [];
        d.rows.forEach(function(row){
          if (row[0] == 'Country') countries.push(
            {link:row[2], contact:row[3], country: row[4]}
          );
        });

        //alphabetically
        countries.sort(sortByKey('country'));

        //adding link
        countryTemplate = function (country){
          s = ''
      +'
'
        +''+country.country+'
'
        return s;
        }

        var byletter = {};

        //count countries starting from each letter
        countries.forEach(function(c){
          var firstletter = c.country.toLowerCase().charAt(0);
          if (byletter[firstletter]) byletter[firstletter]++;
          else byletter[firstletter]=1;
        });
        console.log(byletter);
        //prepare containers
        var panel = $("#"+Tabzilla.panel.id);
        var $cols = [];

        $cols.push(panel.find(".c_COL4"));
        $cols.push(panel.find(".c_COL3"));
        $cols.push(panel.find(".c_COL2"));
        $cols.push(panel.find(".c_COL1"));
        var columns = $cols.length;
        var targetlen = countries.length/columns;

        var FirstLetter = countries[0].country.toLowerCase().charAt(0);
        var cc = [];

        //fill containers. this loop is buggy. should be reviewed.
        countries.forEach(function(c){
          var newFirstLetter = c.country.toLowerCase().charAt(0);
          if (FirstLetter != newFirstLetter)
          {

             var l1 = cc.length;
             var l2 = l1 + byletter[newFirstLetter];
             //condition maybe shd be changed..

             if (Math.abs(l2-targetlen) >= Math.abs(l1-targetlen)){
               var $col;
               if ($cols.length>0) $col = $cols.pop();
               cc.forEach(function(c){
                 $col.append(countryTemplate(c));
               });
               cc=[];

               //does not work :(
               //could generate another template with first letter raised
               $col.find('span').first().addClass("first-letter");
             }
             cc.push(c);
          }
          else cc.push(c);
          FirstLetter = newFirstLetter;
        });

      },
   });
}

The zgcontacts.json file can be found here.

Any pointers in optimizing this is much appreciated.  For example, this loop:

countries.forEach(function(c)

Use braces { in all your if blocks. It makes it easier to read.

if (byletter[firstletter]){
    byletter[firstletter]++;
}else{
    byletter[firstletter]=1;
}

You have for loops like : countries.forEach(function(c){ which are also hard to read. I'd only use this if you have a function you want to apply.

for(var countryindex = 0; countryindex < countries.length; countryindex++)
{
    var c = countries[countryindex];
    // ...
}

Use descriptive variable names.

if (FirstLetter != newFirstLetter)
{
    var l1 = cc.length;
    var l2 = l1 + byletter[newFirstLetter];

What is l1 & l2 ? What is c or cc?


Adding users for login I have some questions about best practices to apply in JavaScript. I would like to ask about my code and how I could improve.

Starting with an example, I have 2 functions with different ways of handling promises

Here I have a route to add a user:

 addUsers(req,res,next) {
    try {
        const {name,email,login} = req.body;
        User.existLogin(req.body.login)
        .then(async result => {
            if(!result){
                const password = await bcrypt.hashSync(req.body.password, 10);
                User.create({
                    name, email, login, password }).then(result => {
                    res.status(201).json({Results: result.dataValues})
                })
            }else{
                return res.status(409).json({message: 'Login already exists'});
            }
        })
    } catch (error) {
        res.status(500).json({error: error})
    }
}

And here I have another function to login:

async login(req,res){
    const  user  = await User.existLogin(req.body.login);
    if (!user) { return res.status(400).json({result: 'Login is wrong '});}
    const isPassword = await User.isPassword(user.dataValues.password, req.body.password);
    if (!isPassword) { return res.status(400).json({result: 'Password is wrong '}); }
    const jwt = auth.signjwt(auth.payload(user));
    console.log(jwt);
    res.status(200);
    res.json(jwt);
}

Looking at this the login function seems to me to be cleaner, but I have doubts if really yours I could improve in one of two (I follow different logics to do both).

I have an auth folder, where I export functions, such as my payload, my sign, my middleware to validate my jwt, but I don't know if this is a correct decision.

const jwt = require('jsonwebtoken');
const User = require('../models/User')
const config= require('../config/dbconfig');
const moment = require('moment');

module.exports = {
    signjwt (payload) {
        return jwt.sign(payload,
            config.secretToken
                   )
    },
    payload (usuario) {
        return {
            sub: usuario.id,
            name: usuario.nome,
            email: usuario.email,
            login: usuario.username,
            admin: true,
            iat: Math.floor(moment.now()/1000), // Timestamp de hoje
            exp: moment().add(10, 'minutes').unix() // Validade de 2 dias
        }
    },
    async auth(req,res,next){
        const token = req.header('Authorization');
        console.log(token);
        if(!token) return res.status(401).json('Unauthorized');
        try{
            const decoded = jwt.verify(token,config.secretToken);
            const user = await User.findByPk(decoded.sub);
            console.log(user);
            if(!user){
                return res.status(404).json('User not Found');
            }
            res.json(user);
            next();
        }catch(error){
            console.error(error);
            res.status(400).json('Invalid Token');
        }
    }
}

Follow some standards, use code linting (for example ESLint) and code formatting (for example Prettier), try to prevent code duplication as much as possible,
use design patterns and don't complex things. Those things will make your code more readable for others.

There is no right or wrong while the code works and it's easy to read.

Martin Fowler.

  Any fool can write code that a computer can understand. Good
  programmers write code that humans can understand.


Configuring DI when using four-layer architecture In my presentation layer I use an MVC pattern with DI to achieve thin controllers like so:

public class HomeController : Controller
{
    private IHomeService _service;

    public HomeController(IHomeService service)
        : base()
    {
        _service = service;
    }

    public override ActionResult Index()
    {
        HomeIndexViewModel viewModel = new HomeIndexViewModel();
        viewModel.NewsArticles = _service.GetLatestNewsArticles();
        return View(viewModel);
    }
}

I'm using Ninject thus I've configured Ninject within the presentation layer like so:

public class NinjectConfig
{
    public static IKernel CreateKernel()
    {
        StandardKernel kernel = new StandardKernel();

        kernel.Bind<IHomeService>().To<HomeService>();
        kernel.Bind<ISearchService>().To<SearchService>();

        return kernel;
    }
}

I'm a bit confused as to whether I should be configuring Ninject in the presentation layer, or whether this should be done in the application layer.

I'm also wondering whether it is acceptable to have multiple Ninject configurations in multiple layers, such as a configuration in the application layer and another in the business layer.

Actually, in this case you are not configuring ninject in presentation layer, as the presentation layer in case of MVC is cshtml files.

DI engines (like ninject) should be generally configured in the start-up part of application, not in any specific layer. DI has nothing to do with logical decomposition of your application into layers. DI engines only role is to automatically 'distribute' dependencies to objects, usually by interfaces.
In case your application is modular and modules shouldn't see each other, than yes - in this case you probably need per-module DI set-up.

Coming back to ASP.NET MVC, in this case the initial object that are created are controllers and you need to provide implementations for it's dependencies (like you did in the code piece). Now if these dependencies - for exampl HomeService, have their own dependencies, these would need to be registered in the same ninject DI contaier right? So as you see, DI spawns across whole MVC application layers.


How can I make this userform timer faster? I have a userform that displays a goal time for workers to shoot for when completing a task. It also has a stopwatch on it that is controlled by a start, stop, and reset button on the userform. If the stopwatch time reaches the goal time and goes over, then there is a box called "extra time" that starts counting up. It just displays how much extra time the employees are taking to complete a task. The code runs insanely slow, and I think it's just because it is continuously runs a do until statement.

Is there anything I could do to make Excel not freeze up as much when I run this?

Dim StopTimer As Boolean
Dim Etime As Single
Dim Etime0 As Single
Dim LastEtime As Single
Dim goal As Single
Dim Etime1 As Single
Dim Etime2 As Single
Dim LastEtime2 As Single

Public Sub btnReset_Click()
StopTimer = True
Etime = 0
Etime0 = 0
LastEtime = 0
Etime1 = 0
Etime2 = 0
LastEtime2 = 0
lblTime.Caption = "00:00:00"
lblExtra.Caption = "00:00:00"
End Sub

Public Sub btnStart_Click()
goal = 86400 * (Sheets("Input").Range("C2"))    'goal time in seconds
StopTimer = False
Etime0 = Timer()
Etime1 = Timer() + goal
Do Until StopTimer

If Etime < goal Then
    Etime = Int((Timer() - Etime0) * 100) / 100
    If Etime > LastEtime Then
        LastEtime = Etime
        lblTime.Caption = Format(Etime / 86400, "hh:mm:ss")
        DoEvents
    End If
Else
    Etime2 = Int((Timer() - Etime1) * 100) / 100
    If Etime2 > LastEtime2 Then
        LastEtime2 = Etime2
        lblExtra.Caption = Format(Etime2 / 86400, "hh:mm:ss")
        DoEvents
    End If
End If

Loop
End Sub

Public Sub btnStop_Click()
StopTimer = True
End Sub

Public Sub ComboBox1_Change()
Dim cotime As Single
'Dim lookup As String
'lookup = Application.WorksheetFunction.c
'cotime = application.WorksheetFunction.IF(ISERROR(VLOOKUP(Combobox1&Combobox2,AvgFor12to14,2,FALSE)),"N/A",VLOOKUP(Combobox1&Combobox2,AvgFor12to14,2,FALSE)/1440))

Sheets("Input").Range("A2") = Me.ComboBox1.Value
'Me.AvgTime.Value = Format(cotime, "hh:mm:ss")
Me.AvgTime.Caption = Format(Sheets("Input").Range("C2"), "hh:mm:ss")

End Sub

Public Sub ComboBox2_Change()

Sheets("Input").Range("B2") = Me.ComboBox2.Value
Me.AvgTime.Caption = Format(Sheets("Input").Range("C2"), "hh:mm:ss")

End Sub

Private Sub UserForm_DblClick(ByVal Cancel As MSForms.ReturnBoolean)
Application.Visible = True
End Sub

Private Sub UserForm_Initialize()
ComboBox2 = ""
ComboBox1 = ""
End Sub

Just formatting first:

Your code is missing indentation and has some extra newlines in it.

Your Do Until loop should be double indented because it is inside of a sub.
You should also get rid of the extraneous commented code. If you got it working, get rid of the junk.

Here is what the code should look like:

Dim StopTimer As Boolean
Dim Etime As Single
Dim Etime0 As Single
Dim LastEtime As Single
Dim goal As Single
Dim Etime1 As Single
Dim Etime2 As Single
Dim LastEtime2 As Single

Public Sub btnReset_Click()
    StopTimer = True
    Etime = 0
    Etime0 = 0
    LastEtime = 0
    Etime1 = 0
    Etime2 = 0
    LastEtime2 = 0
    lblTime.Caption = "00:00:00"
    lblExtra.Caption = "00:00:00"
End Sub

Public Sub btnStart_Click()
    goal = 86400 * (Sheets("Input").Range("C2"))    'goal time in seconds
    StopTimer = False
    Etime0 = Timer()
    Etime1 = Timer() + goal
    Do Until StopTimer
        If Etime < goal Then
            Etime = Int((Timer() - Etime0) * 100) / 100
            If Etime > LastEtime Then
                LastEtime = Etime
                lblTime.Caption = Format(Etime / 86400, "hh:mm:ss")
                DoEvents
            End If
        Else
            Etime2 = Int((Timer() - Etime1) * 100) / 100
            If Etime2 > LastEtime2 Then
                LastEtime2 = Etime2
                lblExtra.Caption = Format(Etime2 / 86400, "hh:mm:ss")
                DoEvents
            End If
        End If
    Loop
End Sub

Public Sub btnStop_Click()
    StopTimer = True
End Sub

Public Sub ComboBox1_Change()
    Dim cotime As Single
    Sheets("Input").Range("A2") = Me.ComboBox1.Value
    Me.AvgTime.Caption = Format(Sheets("Input").Range("C2"), "hh:mm:ss")
End Sub

Public Sub ComboBox2_Change()
    Sheets("Input").Range("B2") = Me.ComboBox2.Value
    Me.AvgTime.Caption = Format(Sheets("Input").Range("C2"), "hh:mm:ss")
End Sub

Private Sub UserForm_DblClick(ByVal Cancel As MSForms.ReturnBoolean)
    Application.Visible = True
End Sub

Private Sub UserForm_Initialize()
    ComboBox2 = ""
    ComboBox1 = ""
End Sub

It is a lot easier to read this way as well.

Your naming should be "pascalCase" for everything.

Lose the Hungarian notation on the labels; name the labels appropriately, like timeOutput or something like that.

StopTimer -> stopTimer
Etime -> eTime
LastEtime -> lastETime


How random is this password-generating algorithm? This code's sole purpose is to create a very random password, using letters and numbers.

I think that it fulfills its purpose, but I wonder, could it be done better? I'm not talking about efficiency, but security.

In your eyes, how secure is this method for generating passwords, and if it is barely secure at all, what better practices are there for this sort of thing?

import random  as r
import sys
import os
from subprocess import call
alpha_lower = "abcdefghijklmnopqrstuvwxyz"
alpha_upper = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
call('color a', shell=True)
class MutateSeed(object):
    def __init__(self, seed, alpha_lower, alpha_upper):
        self.seed = seed
        self.mutated_seed = []
        self.alphabet_lower = list(alpha_lower)
        self.alphabet_upper = list(alpha_upper)
    def mutate_seed(self):
        for x in range(self.seed):
            a = r.randint(0, 9)
            b = r.randint(0, 9)
            c = r.randint(0, 9)
            d = r.randint(0, 25)
            e = r.randint(0, 25)
            f = r.randint(0, 25)

            con = r.randint(1, 6)
            if con == 1:
                mut = a
            elif con == 2:
                mut = b
            elif con == 3:
                mut = c
            elif con == 4:
                lower_or_higher = r.randint(0, 1)
                if lower_or_higher == 0:
                    mut = self.alphabet_lower[d]
                elif lower_or_higher == 1:
                    mut = self.alphabet_upper[d]
            elif con == 5:
                lower_or_higher = r.randint(0, 1)
                if lower_or_higher == 0:
                    mut = self.alphabet_lower[e]
                elif lower_or_higher == 1:
                    mut = self.alphabet_upper[e]
            elif con == 6:
                lower_or_higher = r.randint(0, 1)
                if lower_or_higher == 0:
                    mut = self.alphabet_lower[f]
                elif lower_or_higher == 1:
                    mut = self.alphabet_upper[f]

            self.mutated_seed.append(mut)

def Display():
    os.system("cls")
    user_input = int(raw_input("Length of password: "))
    mutate_seed = MutateSeed(user_input, alpha_lower, alpha_upper)
    mutate_seed.mutate_seed()
    display_pass = "".join(str(x) for x in mutate_seed.mutated_seed)
    f = open('PasswordFile.txt', 'a+')
    f.write(display_pass)
    f.write("\n")
    print "\n" "Your Password: "+display_pass+ "\n"
    raw_input("Press Enter to continue: ")
    pass
while True:
    Display()

Copy&pasting code multiple times does not make it more secure. If we clean up your mutate_seed method, we'd end up with this:

def mutate_seed(self):
    for x in range(self.seed):
        number_or_letter = r.randint(0, 1)
        if number_or_letter:
            self.mutated_seed.append(r.randint(0, 9))
        else:
            lower_or_higher = r.randint(0, 1)
            if lower_or_higher:
                self.mutated_seed.append(r.choice(self.alphabet_lower))
            else:
                self.mutated_seed.append(r.choice(self.alphabet_upper))

Quick Notes on the changes:

I don't request random values which I don't end up using. Throwing away values from the random number generator does not make those numbers that you use more secure.
I don't use single-letter variable names or abbreviations like con or mut that obscure the meaning.
random.choice is so simple and nice – consider using it.
This solution features far less duplicate code. Did you calculate r.ranint(1, 6) as an analogy for six-sided dice?

Now the code is cleaned up so that we can do some more substantial reviewing.

There is no use in separating between numbers, lowercase letters, and uppercase letters. They all should be in the same character pool so that each character has the same probability of occuring at each position in the password. With your current methods, a digit is much more likely to occur than any given letter, simply because there are less digits than letters.

So we should have something like

lower = [chr(x) for x in range(ord('a'), ord('z') + 1)] # I can't be bothered to type
upper = [chr(x) for x in range(ord('A'), ord('Z') + 1)] #   out all letters manually
digits = [str(x) for x in range(0, 9)]
character_pool = lower + upper + digits
...
password = [random.choice(character_pool) for _ in range(0, length)]

The interface of your class is horrible. We have to create an instance, then call the mutate_seed method, then access an instance variable, then must join that array to a string until we can finally display it. Consider packing all of this functionality into a single, convenient function instead:

def make_password(length, *collections_of_characters):
    # join all character collections into a single set
    characters = set()
    for collection in collections_of_characters:
        characters.update(str(c) for c in collection)
    characters = list(characters)

    password = [random.choice(characters) for _ in range(0, length)]
    return "".join(password)

Then using this is as simple as

password = make_password(10,
                         "0123456789",
                         "abcdefghijklmnopqrstuvwxyz",
                         "ABCDEFGHIJKLMNOPQRSTUVWXYZ")
print(password)

If you need a certain set of characters more often, put those into a list:

alphanumeric_chars = ["0123456789",
                      "abcdefghijklmnopqrstuvwxyz",
                      "ABCDEFGHIJKLMNOPQRSTUVWXYZ"]
password = make_password(10, *alphanumeric_chars)
print(password)

or create a wrapper function:

def make_alphanumeric_password(length):
    return make_password(length,
                         "0123456789",
                         "abcdefghijklmnopqrstuvwxyz",
                         "ABCDEFGHIJKLMNOPQRSTUVWXYZ")
print(make_alphanumeric_password(10))

Not all abstractions map cleanly to classes.

Your code uses the word “seed” quite often. A seed is the initial state of a random number generator. When set to the same seed each time, the random number generator will produce the same sequence of numbers. In your class, “seed” seems to mean “length” instead. Use clear, obvious names for your variables rather than using technical lingo if you aren't sure what it means – although a quick trip to Wikipedia would have cleared that up.

You issue a few Windows-only system commands such as cls. Unless this is central to the working of your application, don't manipulate the console, such as setting the color or clearing the screen. Users can do this themselves if they want to. Removing these commands also makes your code usable on other operating systems like OS X or Linux.

You are using the random library as a random number generator. The numbers produced by this library aren't really random, they are just pseudo-random. This is usually not an issue as long as a really random seed is chosen, but because the number sequence is derived algorithmically, it should not be used for really really important stuff. Most passwords are only important and not really really important, so this should be fine ;-) but it's good to remember that real randomness is something different.

That is one aspect where the security of your program is not optimal, but good enough. Writing the password to a file is however absolutely irresponsible. When the password is only displayed on the command line, we can assume that it will only be immediately visible until the screen is cleared or other content has caused it to scroll out of sight. It will be held in memory for a while longer, but at least a rebooting of the system will remove any trace of this.
A plain text file however is much more persistent (it stays there after a reboot). It is also much easier for other users to access that password. Don't store secrets at all, or if you have to store them, secure them properly.

You should consider writing your Python code so that it is usable both as a module, and a script. For this, we first write all definitions that should be usable as a module, and at the end add an if __name__ == '__main__' section, which will only be executed if the file was invoked as a script. Now we can both do:

# in some other program:
import make_password as mp
random_hex = mp.make_password(10, "0123456789abcdef")

or

python make_password.py

to use the functionality of your code. Designing for code reuse is a generally good idea.


Avoid while(true) in task scheduling code I am writing a task scheduler (that will one day grow up to be a windows service when I look into them) that should pretty much loop infinitely until I tell the program to stop. But having while(true) seems incorrect. What is the correct way to implement the loop? Use a dummy boolean variable that the code doesn't actually set to false ever (or should it set to false in a catch whose try encompasses the whole inside of the loop?)

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace DataPump
{
    class TaskManager
    {
        // Tasks added externally on an instance of TaskManager
        public Dictionary<DatapumpTask, Task> Tasks = new Dictionary<DatapumpTask, Task>();

        public void Go()
        {
            Func<Task, bool> isBusy = task => task.Status == TaskStatus.Running ||
                                              task.Status == TaskStatus.WaitingToRun ||
                                              task.Status == TaskStatus.WaitingForActivation;

            //Run Loop  --- To Do move off the main thread
            while (true)
            {
                for (int t = 0; t < Tasks.Count; t++)
                {
                    KeyValuePair<DatapumpTask, Task> task = Tasks.ElementAt(t);

                    if (task.Key.NextRun < DateTime.Now && (task.Value == null || !isBusy(task.Value)))
                    {
                        Tasks[task.Key] = Task.Factory.StartNew(() => task.Key.Run());
                    }

                    if (task.Value != null && task.Value.IsFaulted)
                    {
                        task.Key.Log("Task faulted - something is wrong!" , LogType.Error);
                        Tasks[task.Key] = null;
                    }
                }
            }
        }

    }

Note DatapumpTask has a public void Run() method, a public void Log() method that logs to a text file and Run() will set its DateTime NextRun property correctly...

The question is a bit diffuse but I can give some hopefully useful feedback to help you move forward.

First of all I have to say that there are many libraries available for scheduling tasks. So be aware that you are reinventing the wheel, not to mention a very non-trivial wheel!

Threading is difficult, I recommend you have a look at http://www.albahari.com/threading/

Using while(true) is fine. But you should pause execution within the loop to avoid spinning cpu. One way would be to figure out when the next task is scheduled to run and Thread.Sleep until then. You also need to deal with the situation where a new task is added that is due before the call to Sleep returns.

Consider using a callback to deal with completed tasks and update the Log/Status.

The Tasks field is public so TaskManager is not thread safe. Consider making it private and adding synchronized methods to manipulate it. Also, the DataPumpTask objects used as keys are mutable. What happens if the client code reads/writes these objects while they are being manipulated by the TaskManager thread?

I wouldn't use a for loop to iterate a dictionary unless I needed the loop index variable for some reason. Try this instead:

foreach(KeyValuePair<DataPumpTask, Task> pair in Tasks)
{
    var dataPumpTask = pair.Key;
    var task = pair.Value;
    //do stuff
}


C++ makefile for CPLEX with environment loading CPLEX is a solver for mathematical problems made by IBM, and it offers callable C libraries.

My usual project configuration is a main.cpp file will all the important code and a simple cpxmacro.h file with some macros. Sometimes there are also .dat files with problem data. When the solver does its job (i.e. the main is run), it outputs .lp and .sol files.

I used to let my IDE come up with proper make files, but this time I was provided with a hand-made make file. What a good opportunity to look into it.

CXX = g++
CXXFLAGS = -g -Wall -O
LDADD =

CPX_INCDIR  = /opt/ibm/ILOG/CPLEX_Studio126/cplex/include
CPX_LDPATH  = /opt/ibm/ILOG/CPLEX_Studio126/cplex/lib/x86-64_linux/static_pic
CPX_LDADD = -lcplex -lm -pthread

OBJ = main.o

%.o: %.cpp
    $(CXX) $(CXXFLAGS) -I$(CPX_INCDIR) -c $^ -o $@

main: $(OBJ)
    $(CXX) $(CXXFLAGS) $(OBJ) -o main -L$(CPX_LDPATH) $(CPX_LDADD)

clean:
    rm -rf $(OBJ) main

.PHONY: clean

As long as I only change the main file, it works. I just have to manually load a small file first, by typing . cplex_env. This is what the file contains:

#!/bin/sh

export PATH=$PATH:/opt/ibm/ILOG/CPLEX_Studio126/opl/bin/x86-64_linux:/opt/ibm/ILOG/CPLEX_Studio126/cplex/bin/x86-64_linux
export LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:/opt/ibm/ILOG/CPLEX_Studio126/opl/bin/x86-64_linux:/opt/ibm/ILOG/CPLEX_Studio126/cplex/bin/x86-64_linux:/opt/ibm/ILOG/CPLEX_Studio126/cpoptimizer/bin/x86-64_linux/

I never tried to change the .h macro file and see what happens, it doesn't look to me like it's being accounted for anywhere in the makefile.

I'd like to:

include the path exports in the make file
make the makefile a little more generic

Instead of providing your own %.o: %.cpp rule, you can let Make use its built-in one.  You need to pass your extra include directory, and the standard way to do that is to add to CXXFLAGS:
CXXFLAGS += -I$(CPX_INCDIR)

Or perhaps
CPLEX_DIR = /opt/ibm/ILOG/CPLEX_Studio126/cplex
CXXFLAGS += -I$(CPLEX_DIR)/include

Similarly, take advantage of the built-in link command; this is especially easy, as there's a built-in rule that will create main from main.o.  You need to tell Make to use the C++ linker rather than the plain (C) linker:
LINK.o = $(LINK.cc)
LDFLAGS += -L$(CPLEX_DIR)/lib/x86-64_linux/static_pic
LDLIBS += -lcplex -lm -pthread

To declare the dependency on the header file, note that main.o needs to be rebuilt whenever the header changes:
main.o: cpxmacro.h

That's all!
Header file dependencies can be hard to manage by hand in larger projects; for these, search for "automatic dependency generation", or read Paul D. Smith's excellent article.

Modified makfile
With my changes:
CPLEX_DIR = /opt/ibm/ILOG/CPLEX_Studio126/cplex

CXX = g++
CXXFLAGS = -g -Wall -O
CXXFLAGS += -I$(CPLEX_DIR)/include

LINK.o = $(LINK.cc)
LDFLAGS += -L$(CPLEX_DIR)/lib/x86-64_linux/static_pic
LDLIBS += -lcplex -lm -pthread

all: main

main.o: cpxmacro.h

clean:
    $(RM) *~ *.o main

.PHONY: all clean


Palindrome checker in JavaScript I've built a Palindrome checker in JavaScript. It's fairly simple.

I'm still learning JS and am looking to learn.

So I would love hear ideas on how to improve the JavaScript in this. Like more efficient ways to resolve the task of checking for Palindrome.

Source code is below. Link to CodePen is her: http://codepen.io/MarkBuskbjerg/pen/JWMWwN?editors=1010

HTML:

Palindrome checker

A nut for a jar of tuna

                <textarea class="col-md-12" id="inputPalindrome" rows="5">Borrow or rob?</textarea>

            <button id="checkPalindrome" class="btn btn-default btn-lg btn-block col-sm-6 col-sm-offset-4" type="submit">Check palindrome</button>

Palindrome has not been checked yet

CSS:

.cover {
  height: 100vh;
}

.row-margin {
  margin: 4vh auto;
}

body {
  background: #FF4E50;
  background: -webkit-linear-gradient(135deg, #FF4E50, #F9D423);
  background: linear-gradient(135deg, #FF4E50, #F9D423);
}

.header {
  color: #fff;
}

h1 {
  font-family: "Pacifico";
}

.btn-default {
  background-color: black;
  color: #fff;
  font-weight: bold;
  text-transform: uppercase;
 }

JavaScript:

var checkButton = document.getElementById("checkPalindrome");

function isPalindrome(str) {
    str = str.toLowerCase().replace(/[^a-z0123456789]+/g,"");
    var reversedStr = str.split("").reverse().join("");
    if (str == reversedStr) {
        return true
    }
    return false
}

checkButton.addEventListener("click", function() {
    var palindromeInput = document.getElementById("inputPalindrome").value;
    var palindromeReturn = isPalindrome(palindromeInput);
    if(palindromeReturn === true) {
        document.getElementById("notification").innerHTML = "Yay! You've got yourself a palindrome";
        document.getElementById("notification").className = "alert alert-success";
    } else {
        document.getElementById("notification").innerHTML = "Nay! Ain't no palindrome";
        document.getElementById("notification").className = "alert alert-danger";
    }
});

Link to CodePen with the Palindrome Checker
http://codepen.io/MarkBuskbjerg/pen/JWMWwN

RegEx

In character class, 0123456789 can be written as range 0-9. Although, this has no effect on working of RegEx, it saves some keystrokes and looks consistent with alphabets range a-z.

toLowerCase after replace

First remove the special characters and then convert the string to lower case. This, in my opinion will run fast than other way round as the number of characters to work on are reduced.(Not Tested)

When doing this, don't forget to add i flag on the RegEx.

str.replace(/[^a-z0-9]+/gi, "").toLowerCase();

return boolean;

if (str == reversedStr) {
    return true
}
return false

can be written as return str == reversedStr. It is also recommended to use strict equality operator.

return str === reversedStr;

See Which equals operator (== vs ===) should be used in JavaScript comparisons?

Caching DOM element reference

In the click handler of button, #notification is referenced four times. All times, it is read again from DOM. This can be slower. The element can be cached and used whenever required.

var notification = document.getElementById('notification');
...
...
notification.innerHTML = 'Hello World!';

Complete Code

Updated after suggestions from Ismael Miguel

// After DOM is completely loaded
document.addEventListener("DOMContentLoaded", function() {
    "use strict";

    // Cache
    var palindromeInput = document.getElementById("inputPalindrome");
    var notification = document.getElementById("notification");

    function isPalindrome(str) {
        str = str.replace(/[^a-z0-9]+/gi, "").toLowerCase();
        return str.split("").reverse().join("") === str;
    }

    document.getElementById("checkPalindrome")
        .addEventListener("click", function() {

            if (isPalindrome(palindromeInput.value)) {
                notification.innerHTML = "Yay! You've got yourself a palindrome";
                notification.className = "alert alert-success";
            } else {
                notification.innerHTML = "Nay! Ain't no palindrome";
                notification.className = "alert alert-danger";
            }
        });
});


Digits should know their place Challenge
Swap the numbers surrounding the words in sentences.
Specifications

The first argument is a path to a file.
The file contains multiple lines.
Each line is a test case represented by a sentence.
Each sentence begins and ends with a number.
Print out the sentence obtained by swapping the numbers surrounding each word.

Constraints

The suffix and the prefix of each word may be equal.
Sentences are from 1 to 17 words long.
The number of test cases is 40.
All characters are ASCII.
The numbers are single digit positive intgers 0-9.

Sample Input

4Always0 5look8 4on9 7the2 4bright8 9side7 3of8 5life5
5Nobody5 7expects3 5the4 6Spanish4 9inquisition0

Sample Output

0Always4 8look5 9on4 2the7 8bright4 7side9 8of3 5life5
5Nobody5 3expects7 4the5 4Spanish6 0inquisition9

Source
My solution
#include <stdio.h>
#include <ctype.h>

#define LINE_BUFFER 256

int fileExists(char *filename) {
    FILE *file = fopen(filename, "r");
    if (file != NULL) {
        fclose(file);
    }
    return file != NULL;
}

void swapNumbers(char line[LINE_BUFFER]) {
    int temp = 0;
    int position = 0;
    int swapped = 0;

    for (int i = 0; i < LINE_BUFFER; i++) {
        if (isdigit(line[i])) {
            if (!swapped) {
                position = i;
                temp = line[i];
                swapped = 1;
            } else {
                line[position] = line[i];
                line[i] = temp;
                swapped = 0;
            }
        }
    }
}

int main(int argc, char *args[]) {
    if (argc < 2) {
        puts("File path not provided.");
        return 1;
    }

    if (argc > 2) {
        puts("Excessive arguments, only the first will be considered.");
    }

    if (!fileExists(args[1])) {
        puts("Could not access file / file not found.");
        return 1;
    }

    FILE *file = fopen(args[1], "r");
    char line[LINE_BUFFER];

    while (fgets(line, LINE_BUFFER, file)) {
        swapNumbers(line);
        printf("%s", line);
    }

    fclose(file);
}

if (!fileExists(args[1])) {
        puts("Could not access file / file not found.");
        return 1;
    }

    FILE *file = fopen(args[1], "r");

This seems more complicated than necessary.

    FILE *file = fopen(args[1], "r");
    if (NULL == file) {
        puts("Could not access file / file not found.");
        return 1;
    }

This has the exact same effect as fileExists but it leaves you with an open file handle.  The other method tested that it could open a file and then assumed that the second open worked.  So it's actually slightly less safe, as well as more code.

You might argue that it would be extremely rare for the first open to succeed and the second one to fail.  I agree with you.  But rare is not never.  Thus, "slightly" less safe.

    int temp = 0;
    int position = 0;
    int swapped = 0;

    for (int i = 0; i < LINE_BUFFER; i++) {
        if (isdigit(line[i])) {
            if (!swapped) {
                position = i;
                temp = line[i];
                swapped = 1;
            } else {
                line[position] = line[i];
                line[i] = temp;
                swapped = 0;
            }
        }
    }

I don't like using negative test conditions.  I'd normally fix this by switching the if and else blocks.  In this case though, the current order makes more sense.

My second thought was that the name was wrong.  So I was going to change the name to needs_swapped or similar.  But then I realized that if I changed the name, the current form would be correct.  The name swapped is actually backwards.  When swapped is true, then you haven't swapped but need to do so.  So my third thought was to simply swap the meaning.

    int temp = 0;
    int position = 0;
    int swapped = 1;

    for (int i = 0; i < LINE_BUFFER; i++) {
        if (isdigit(line[i])) {
            if (swapped) {
                position = i;
                temp = line[i];
                swapped = 0;
            } else {
                line[position] = line[i];
                line[i] = temp;
                swapped = 1;
            }
        }
    }

Now I have the else as the negative of the condition, which I like.  And we've also made it so that when swapped is 1 (true in C parlance), we've just swapped.  When swapped is 0 (false in C), we're ready to swap.

But I actually think that there is a more idiomatic way to do this in C.  You are saving an index (position), a value (temp), and a Boolean (swapped).  But you actually only need to save the location.  Consider

    char *location = NULL;
    for (int i = 0; i < LINE_BUFFER; i++) {
        if (isdigit(line[i])) {
            if (NULL == location) {
                location = &line[i];
            } else {
                int temp = *location;
                *location = line[i];
                line[i] = temp;
                location = NULL;
            }
        }
    }

or less explicitly (and slightly out of order)

    char *location = NULL;
    for (int i = 0; i < LINE_BUFFER; i++) {
        if (isdigit(line[i])) {
            if (location) {
                int temp = *location;
                *location = line[i];
                line[i] = temp;
                location = NULL;
            } else {
                location = line + i;
            }
        }
    }

Either version reduces three variables at function scope down to one at function scope and one at the most local block scope.

It's up to you which version you find more readable.  I also switched between line + i and &line[i].  They do the same thing, so it's pretty much a readability difference.

And I like this better, as rather than using a Boolean to track whether or not we need to swap, we track based on whether we know where to swap.  Less parallel logic increases robustness.

Twice now I've used the notation NULL == variable (where variable is any declared variable like location or file).  The reason for this is that in C, if (variable = NULL) { is valid and will compile and run.  It creates unreachable code, as NULL will always be false.  So modern compilers often warn on this.  But it's valid grammatically.  If you instead always write if (VALUE == variable) { (where VALUE is any constant like NULL or 3), then a typo leads to if (VALUE = variable) { which is not valid and won't compile.

Like I said, most modern compilers will warn on this.  So it's not as important as it once was.  But if you start editing C code, you are likely to see this idiom used.  This is why.


Longest subarray with a zero sum I'm working through problems on this site.

The problem I'm currently doing is:

  Given an array having both positive and negative integers, your task
  is to complete the function maxLen which returns the length of maximum
  subarray with 0 sum. The function takes two arguments an array A and n
  where n is the size of the array A.

However the code is too slow to pass so I can't check if the solution is working correctly or not - (it works fine for the single test case that's provided)

Can anyone suggest how to improve the performance of this code?

from itertools import combinations

def maxLen(n, arr):
    for i in range(n, 0, -1):
        for combo in combinations(arr, i):
            if sum(combo) == 0:
                return len(combo)
    else:
        return 0

(EDIT: Go look at @RE60K's solution before mine. His is faster and easier to understand.)

Unfortunately, I don't think this code will work, efficiency aside.

for combo in combinations(arr, i):

Here, you're looping through permutations, but the challenge asks for subarrays.

Instead, you should do something like:

def maxLen(n, arr):
    for subarray_length in range(n, 0, -1):
        for subarray_start in range(len(arr) - subarray_length + 1):
            subarray = arr[subarray_start : subarray_start + subarray_length]
            if sum(subarray) == 0: return len(subarray)
    return 0

Which works, but isn't terribly quick. If sum is O(n), then this is O(n^3).

I was able to come up with a quicker algorithm (O(n^2)) which seems to be consistently faster on my timeit tests (which may not be perfect).

To show the idea of how this works, I'll use an example.

Consider if the input array were [1, -4, 4, -3, -1, 8, -3].

We'd start by trying the whole array, which sums to -4. Then we'd try all arrays one size down, then two, etc.

Let's skip to trying subarray size 3, since that's where the answer lies. First we try [1, -4, 4] which sums to 1. Normally, we'd then try the next subarray, [-4, 4, -3], but we can take a shortcut here. These two subarrays only differ by two items: the first item of the first, and the last item of the second. As such, we can instead keep a running sum, and only worry ourselves with those two key items.

So, instead, we go like this: first length 3 subarray is [1, -4, 4], which sums to 1. The next item after this subarray is -3. We add this to the running sum, and remove 1, (+= -3, -= 1), and then consider our array to have moved to the right one. So, the next item we consider is the one after -3, namely, -1. And so we continue and find that our running sum does hit 0, and so return 3.

def max_len_2(arr_len, arr):
    for sub_len in range(arr_len, 0, -1):  # For all subarray lengths
        arr_sum = sum(arr[0:sub_len])  # Get the initial sum
        if arr_sum == 0: return sub_len  # Return if it works
        for gain_index in range(sub_len, arr_len):  # Otherwise, loop through all the indices after the array
            arr_sum -= arr[gain_index - sub_len]  # Drop first item in array
            arr_sum += arr[gain_index]  # Gain first item after array
            if arr_sum == 0: return sub_len  # If sum works, return
    return 0

This is more efficient, awesome! But we can take it one step further. What we just did is we used information we already know about a row in order to have to make less calculations. Essentially, we're not wasting any information.

And we can do this between rows, as well, in order to be even more efficient (although still O(n^2), but whatever...).

I'll explain this algorithm with the same example, [1, -4, 4, -3, -1, 8, -3].

We start by noting that the sum of this is -4. We'll call this sum 0 0. If it were 0, we'd be done, but it isn't.

Now, we start with the largest subarrays, length 6. We note that the first subarray is the same as the whole array, except for the last element; so, the sum is sum 0 0 minus the last element, which is -3. The sum is -4 - -3 = -1. This is sum 1 0. The second length-6 subarray is the same as the whole array, except for the first element, so we know the sum is sum 0 0 - first element = -4 - 1 = -5.

Now, onto length 5. The sum of the first length 5 subarray (sum 2 0) is the same as the first length 6 subarray (sum 1 0), except for the first element...

See the pattern? With each new subarray length n, sum n k = sum (n-1) k - last element of array (n-1) k. The only exception here is that there is 1 length-7 sub, 2 length-6, 3 length-5, etc.; each time we gain a subarray, and this new sub isn't accounted for with this algorithm. Instead, we know that the sum of the last sub sum n k is the same as sum (n-1) k - first element of array (n-1) k Note the difference: it's the first element we're taking away, not the last.

def max_len_dynamic_programming(arr_len, arr):
    dp = []  # This is our data storage

    # We initialize it with the sum of the array
    s = sum(arr)
    if s == 0: return arr_len
    dp.append([s])

    for sub_len in range(arr_len - 1, 0, -1):  # Iterate over sub lengths, decreasing...
        row = []  # We'll construct the new row
        prev_row = dp[-1]  # With info from the old row
        prev_row_len = arr_len - sub_len

        for i in range(prev_row_len):
            # This is where the magic happens.
            # cell is `sum n k`, and
            # prev_row[i] is `sum (n-1) k`,
            # and arr[i + sub_len] is
            # the last item of `array (n-1) k`
            cell = prev_row[i] - arr[i + sub_len]
            if cell == 0: return sub_len
            row.append(cell)

        # Deal with the last element seperately
        last = prev_row[-1] - arr[~sub_len]  # -sub_len - 1
        if last == 0: return sub_len
        row.append(last)

        dp.append(row)

    return 0

If you want a faster*, harder-to-understand version, make these swaps in the code:

Swap

row = []
prev_row = dp[-1]
prev_row_len = arr_len - sub_len

for

prev_row = dp[-1]
prev_row_len = arr_len - sub_len
row_len = prev_row_len + 1
row = [0] * row_len

& swap row.append(cell) for row[i] = cell

& swap row.append(last) for row[-1] = last

*Most of the time, in my tests, at least. The swaps change this from using all dynamic arrays to preinitialized arrays for each row, and dynamic for the grid. Going full-preinitialized-arrays will be even faster some of the time. Not sure how to tell when it'll be faster and when it'll be slower.


VBA performance with Early Bind vs. Late Bind on called Sub-procedure I was using early bind to reference the dictionary and realised that some users might not be willing to manually add the reference to use it and just consider the macro to be 'broken'.

I looked at programmatically enabling the reference but that would still require the user to go through several steps to change their macro security level, which again would seem 'broken' to an end user.

So my solution is to offer early bind and late bind options to the user, early bind for those comfortable with the VBE, late bind if not.

My question is, would the potential efficiency/performance gain in early binding still be present in going with this method, or is it wiped out simply by having to call to another sub?

If the gain is lost, I can simply use late bind within the main sub-procedure.

Option Explicit

Public Sub MatchArraysEarlyBind()

    Dim arrayMatchDictionary As Dictionary
    Set arrayMatchDictionary = New Dictionary

    Call ContinueMatchArray(arrayMatchDictionary)

End Sub

Public Sub MatchArraysLateBind()

    Dim arrayMatchDictionary As Object
    Set arrayMatchDictionary = CreateObject("Scripting.Dictionary")

    Call ContinueMatchArray(arrayMatchDictionary)

End Sub

Kudos for Option Explicit and explicitly making the procedures Public - I suppose the ContinueMatchArray procedure looks something like this?

Private Sub ContinueMatchArray(ByVal arrayMatchDictionary As Object)
    'do something
End Sub

This arrayMatchDictionary As Object parameter is nice - you have a dependency and you're putting its creation in its own method, and the dependency can be a Dictionary obtained with New Dictionary or CreateObject, it makes no difference... because as far as VBA is concerned, this arrayMatchDictionary is late-bound, regardless of whether it was MatchArrayMatchEarlyBind that created the object.

Calling members of an Object type means the runtime is going to have to query the IDispatch interface of the object to discover the handle to the member being called.

By declaring the parameter As Object, you're actually downcasting to the simplest COM interface that is known to VBA - as long as the object is in scope,  to the face of the world that procedure its underlying type is unknown.

The VBA type casting mechanism involves Setting a reference to another pointer type:

Dim foo As Object
Set foo = New Scripting.Dictionary

Here foo is a Dictionary, but even if the reference assignment was early bound, every single member call to foo will be late-bound - you're asking the underlying COM engine to query the object's interface, and because we're looking at an Object, that interface is IDispatch; the query locates a function pointer in the object's vtable, or blows up if it can't find it.

The performance penalty with late binding isn't only with the original reference assignment: every single member call incurs a hit.

An early-bound member call is easy: the runtime doesn't need to query IDispatch, it has the actual object's interface handy, so the whole "lookup the function pointer" part is skipped, because the function pointer's location is already known.

ContinueMatchArray works late-bound in both cases.

Compilation

Your code runs, but doesn't compile: it relies on the interpreted nature of the language - if no scope in the execution path encounters an early-bound Dictionary, the code can run. But if you go Debug > Compile VBAProject, your project fails to compile:

Code that runs but doesn't compile isn't... ideal.

Explicit Call Syntax

Visual Basic for Applications (VBA) didn't just appear; it evolved from older languages: this is a fizzbuzz program written in Commodore-64 BASIC 2.0, which didn't even have a Mod operator:

10 GOSUB 100
20 GOSUB 1000
99 END
100 REM CLEAR SCREEN
110 PRINT CHR$(147)
120 RETURN
200 REM MODULO
210 LET MOD% = V%-INT(V%/FB%)*FB%
220 RETURN
1000 REM INIT VARIABLES
1010 LET FIZZ$ = "FIZZ"
1011 LET BUZZ$ = "BUZZ"
1020 LET FIZZ% = 3
1021 LET BUZZ% = 5
1030 LET MIN% = 1
1031 LET MAX% = 15
1100 PRINT FIZZ$ + ":" + STR$(FIZZ%)
1101 PRINT BUZZ$ + ":" + STR(BUZZ%)
1102 PRINT FIZZ$ + BUZZ$ + ":" + STR$(FIZZ%*BUZZ%)
1105 PRINT
2000 REM ACTUAL FIZZBUZZ LOOP
2010 FOR X = MIN% TO MAX%
2015 LET RESULT$ = STR$(X)
2020 LET FB% = FIZZ%*BUZZ%
2021 LET V% = X
2024 GOSUB 200
2025 IF MOD%=0 THEN LET RESULT$=FIZZ$+BUZZ$ : GOTO 2050
2030 LET FB% = FIZZ%
2031 GOSUB 200
2035 IF MOD%=0 THEN LET RESULT$=FIZZ$ : GOTO 2050
2040 LET FB% = BUZZ%
2041 GOSUB 200
2045 IF MOD%=0 THEN LET RESULT$=BUZZ$ : GOTO 2050
2050 PRINT RESULT$
2090 NEXT X
2099 RETURN

With pretty slight modifications, the BASIC 2.0 code above can be executed by the VBA runtime:

Sub Main()
10     GoSub 100
20     GoSub 1000
99     End
100     Rem CLEAR SCREEN
110     Debug.Print Chr$(147)
120     Return
200     Rem MODULO
210     Let Modulo% = V% - Int(V% / FB%) * FB%
220     Return
1000     Rem INIT VARIABLES
1010     Let FIZZ$ = "FIZZ"
1011     Let BUZZ$ = "BUZZ"
1020     Let FZZ% = 3
1021     Let BZZ% = 5
1030     Let Min% = 1
1031     Let Max% = 15
1100     Debug.Print FIZZ$ + ":" + Str$(FZZ%)
1101     Debug.Print BUZZ$ + ":" + Str(BZZ%)
1102     Debug.Print FIZZ$ + BUZZ$ + ":" + Str$(FZZ% * BZZ%)
1105     Debug.Print
2000     Rem ACTUAL FIZZBUZZ LOOP
2010     For X = Min% To Max%
2015     Let RESULT$ = Str$(X)
2020     Let FB% = FZZ% * BZZ%
2021     Let V% = X
2024     GoSub 200
2025     If Modulo% = 0 Then Let RESULT$ = FIZZ$ + BUZZ$: GoTo 2050
2030     Let FB% = FZZ%
2031     GoSub 200
2035     If Modulo% = 0 Then Let RESULT$ = FIZZ$: GoTo 2050
2040     Let FB% = BZZ%
2041     GoSub 200
2045     If Modulo% = 0 Then Let RESULT$ = BUZZ$: GoTo 2050
2050     Debug.Print RESULT$
2090     Next X
2099     Return
End Sub

So BASIC went from  to structured programming, where "structure" started meaning procedures instead of line number ranges, and eventually modules too - and then the Call keyword showed up. BASIC eventually became Visual; comments were denoted by a single quote instead of a REM statement, and the explicit call syntax was abandoned, along with the explicit value assignment syntax: the Call and the Let keywords were deprecated. Oh you can still use them and they still work... just like the above fizzbuzz program still works. It's just that, the modern language has more elegant ways to do things.

Instead of this:

Call ContinueMatchArray(arrayMatchDictionary)

You can simply have this:

ContinueMatchArray arrayMatchDictionary

Don't write dino-basic, use the implicit call syntax.

Early-bind?

If you're going to be working against an Object, don't bother with early-binding: remove the reference and delete the early-bound method - at least your code will be compilable and so a compile error becomes meaningful.

On the other hand, why late-bind the Microsoft Scripting Runtime library? It's on every Windows machine, and hasn't changed this century, and is somewhat more likely to not ship at all with a new version of Windows than to be issued a new version anytime soon: if you add the project reference, it will work on every Windows user that uses it.

  some users might not be willing to manually add the reference to use it and just consider the macro to be 'broken'.

That makes no sense - if you save the VBA project with a reference to the scripting runtime, the reference isn't gone when another user opens your macro-enabled workbook: if a user needs to manually add a reference, it's because they broke it themselves.

If the referenced library is missing or unregistered on the other computer, late-binding isn't going to make it work any better: there's nothing to bind to. When you call CreateObject like this:

Set foo = CreateObject("Scripting.Dictionary")

You're telling the runtime to fetch the CLSID from this registry key:

That CLSID is then used to locate the library:

And then the ProgId "Scripting.Dictionary" is used to locate the type and create the object, which is returned to VBA as an IDispatch - an Object.

If you don't need late-binding, don't late-bind - keep it simple:

Private Sub ContinueMatchArray(ByVal arrayMatchDictionary As Scripting.Dictionary)
    'do something
End Sub


Java - Downloading Wikipedia Articles This is my first experiment that uses a network API and connects to the internet.
I tried to write a program that gives you an article of wikipedia directly by typing the name.

You can get an article like this:

java ArticleSupplier computer science
java ArticleSupplier captain underpants

If you have tips for me how to improve the performance or how to make a better structure, I would be thankful!

import java.util.Scanner;
import java.io.InputStream;
import java.io.FileWriter;
import java.io.BufferedWriter;
import java.io.IOException;
import java.net.URL;
import java.net.MalformedURLException;

public class ArticleSupplier {
    private String nameOfArticle; // will be the name of the created document
    private String urlName;
    private URL url;
    private String article;

    public ArticleSupplier(String[] urlComponents) {
        // initialize article
        article = "";

        // initialize urlName and nameOfArticle
        urlName = "https://en.wikipedia.org/wiki/";
        nameOfArticle = "";
        for(int i = 0; i < urlComponents.length; i++) {
            urlName += urlComponents[i];
            urlName += "_";

            nameOfArticle += urlComponents[i];
            nameOfArticle += " ";
        }
        urlName = urlName.substring(0, urlName.length() - 1);
        nameOfArticle = nameOfArticle.substring(0, nameOfArticle.length() - 1);

        // initialize url
        try {
            url = new URL(urlName);
        } catch(MalformedURLException e) {
            e.printStackTrace();
        }
    }

    public void getArticle() {
        try(InputStream inputStream = url.openStream(); Scanner scanner = new Scanner(inputStream)) {
            while(scanner.hasNext()) {
                article += scanner.next() + "\n";
            }
        } catch(IOException e) {
            System.out.println(e);
        }
    }

    public void saveAsHTML() {
        String filename = nameOfArticle + ".html";
        try(FileWriter fileWriter = new FileWriter(filename);
            BufferedWriter bufferedWriter = new BufferedWriter(fileWriter)) {
            bufferedWriter.write(article);
        } catch(IOException e) {
            e.printStackTrace();
        }
    }

    public static void main(String[] args) {
        ArticleSupplier as = new ArticleSupplier(args);
        as.getArticle();
        as.saveAsHTML();
    }
}

Advice 1

urlName = "https://en.wikipedia.org/wiki/";

I suggest you make that magic string a static constant:

private static final String WIKIPEDIA_MAIN_URL =
        "https://en.wikipedia.org/wiki/";

Advice 2

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

According to (common) Java coding conventions there must be a single space between for and the opening brace:

for (int i ...
   ^

Advice 3

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

may be written as

for (String urlComponent : urlComponents) {

Advice 4

for(int i = 0; i < urlComponents.length; i++) {
    urlName += urlComponents[i];
    urlName += "_";

    nameOfArticle += urlComponents[i];
    nameOfArticle += " ";
}
urlName = urlName.substring(0, urlName.length() - 1);
nameOfArticle = nameOfArticle.substring(0, nameOfArticle.length() - 1);

You are essentially repeating yourself; I would do this instead:

StringBuilder stringBuilder = new StringBuilder();
String separator = "";

for (String urlComponent : urlComponents) {
    stringBuilder.append(separator).append(urlComponent);
    separator = "_";
}

this.articleName = stringBuilder.toString();
String urlName = WIKIPEDIA_MAIN_URL + stringBuilder.toString();

Advice 5

public void getArticle() {
    try(InputStream inputStream = url.openStream(); Scanner scanner = new Scanner(inputStream)) {
        while(scanner.hasNext()) {
            article += scanner.next() + "\n";
        }
    } catch(IOException e) {
        System.out.println(e);
    }
}

Why read one "word" at a time when you can read an entire line. Also, whenever building such strings, StringBuilder will be much more efficient:

public void getArticle() {
    StringBuilder stringBuilder = new StringBuilder();

    try (Scanner scanner = new Scanner(url.openStream())) {
        while (scanner.hasNextLine()) {
            stringBuilder.append(scanner.nextLine()); // <-- note the builder
        }
    } catch (IOException ex) {
        ex.printStackTrace();
    }

    this.article = stringBuilder.toString();
}

Alternative implementation

Gathering all the advices, I had this in mind:

import java.util.Scanner;
import java.io.FileWriter;
import java.io.BufferedWriter;
import java.io.IOException;
import java.net.URL;
import java.net.MalformedURLException;

public class ArticleSupplier {

    private static final String WIKIPEDIA_MAIN_URL =
            "https://en.wikipedia.org/wiki/";

    private String articleName;
    private URL url;
    private String article;

    public ArticleSupplier(String[] urlComponents) {
        StringBuilder stringBuilder = new StringBuilder();
        String separator = "";

        for (String urlComponent : urlComponents) {
            stringBuilder.append(separator).append(urlComponent);
            separator = "_";
        }

        this.articleName = stringBuilder.toString();
        String urlName = WIKIPEDIA_MAIN_URL + stringBuilder.toString();

        try {
            this.url = new URL(urlName);
        } catch(MalformedURLException ex) {
            ex.printStackTrace();
        }
    }

    public void getArticle() {
        StringBuilder stringBuilder = new StringBuilder();

        try (Scanner scanner = new Scanner(url.openStream())) {
            while (scanner.hasNextLine()) {
                stringBuilder.append(scanner.nextLine());
            }
        } catch (IOException ex) {
            ex.printStackTrace();
        }

        this.article = stringBuilder.toString();
    }

    public void saveAsHTML() {
        String filename = articleName + ".html";

        try (BufferedWriter writer =
                new BufferedWriter(new FileWriter(filename))) {
            writer.write(article);
        } catch(IOException ex) {
            ex.printStackTrace();
        }
    }

    public static void main(String[] args) {
        ArticleSupplier as = new ArticleSupplier(args);
        as.getArticle();
        as.saveAsHTML();
    }
}