Background execution in iOS The following class BackgroundExecutable is to be used to take a function f and return a function f' where f' asks iOS for background execution. f takes a completion closure which it uses to end the background execution.

class BackgroundExecutable {
  var identifier: UIBackgroundTaskIdentifier
  let f: (() -> Void) -> Void

  init(f: (completion: () -> Void) -> Void) {
    self.f = f
    self.identifier = UIBackgroundTaskInvalid
  }

  func execute() {
    let application = UIApplication.sharedApplication()
    identifier = application.beginBackgroundTaskWithExpirationHandler {
      application.endBackgroundTask(self.identifier)
    }
    f(endBackgroundTask)
  }

  func endBackgroundTask() {
    UIApplication.sharedApplication().endBackgroundTask(identifier)
  }
}

func backgroundExecutable(f: (() -> Void) -> Void) -> (() -> Void) {
  let b = BackgroundExecutable(f: f)
  return b.execute
}

Usage:

func F(completion: () -> Void) {
  println("F Called")
  completion()
}

let newF = backgroundExecutable(F)

func backgroundExecutable(f: (() -> Void) -> Void) -> (() -> Void)

This is madness and it takes a number of mental gymnastics to figure out what is going on here.

This method takes, as its only (and poorly named) argument, a closure which returns void and takes, as its single argument, a closure which returns void from taking no arguments.  Then this method returns a closure which takes no arguments and returns void.

Even writing it out in plain-English, it's hard to understand what's actually going on.  And I'm left scratching my head trying to figure out why we even need a closure which takes a closure as its argument.

The major problem here is that we're using this closure's closure argument to pass the call to endBackgroundTask(), a method which must be called, or we risk the OS killing our app.

Your class is attempting to provide a convenient wrapper to handling the background means for executing code while your application is in the background.  But your class hasn't made things any more convenient then what they already are.  It only makes them more nested and complicated.

So let's look at execute for a second, shall we?

func execute() {
    let application = UIApplication.sharedApplication()
    identifier = application.beginBackgroundTaskWithExpirationHandler {
        application.endBackgroundTask(self.identifier)
    }
    f(endBackgroundTask)
}

The crux of this is that endBackgroundTask, another method of this class (which should perhaps be private) is passed in as the closure the the user's background executing closure.  If they don't call the closure that's passed in (and call it only as the last thing their closure does), there will be unintended problems going on here.

But why do that?  We can instead do this:

f()
endBackgroundTask()

That's a step in the right direction.  We're no longer relying on the caller to end the background task.

But there's another problem here.  What if we call execute multiple times and on multiple threads?  The identifier will be different each time we call execute, and each time it overwrites the previous identifier.  So all the previous background tasks can't be stopped, and so the OS will eventually kill our app for running indefinitely in the background.

Instead, we need our endBackgroundTask method to take a string argument... the identifier of the task we're stopping.  And we need to not rely on a string variable for our class.

So, it needs to look more like this:

f()
endBackgroundTask(identifier)

At this point, I'm not entirely certain the point of the class at all when a single method with just a handful of lines can manage it:

func backgroundExecute(worker: () -> Void) -> Void {
    let application = UIApplication.sharedApplication()
    identifier = application.beginBackgroundTaskWithExpirationHandler {
        // you can't call endBackgroundTask here, and you don't need to
        // here you should put code to handle the task running out of time before it is complete
    }
    worker()
    application.endBackgroundTask(identifier)
}


String combinations of 0 and 1 I was asked this question in an interview:

  Given a string s consisting of 0, 1 and ?. The question mark can
  be either 0 or 1. Find all possible combinations for the string.

I came up with below code but I wanted to know whether it is the best way to solve the problem?

  public static void main(String[] args) {
    List<String> output = new ArrayList<>();
    addCombinations("0?1?", 0, output);
    System.out.println(output);
  }

  private static void addCombinations(String input, int index, List<String> output) {
    for (int i = index; i < input.length(); ++i) {
      if (input.charAt(i) == '?') {
        addCombinations(input.substring(0, i) + "0" + input.substring(i + 1), i + 1, output);
        addCombinations(input.substring(0, i) + "1" + input.substring(i + 1), i + 1, output);
        return;
      }
    }
    output.add(input);
  }

Usability

Your addCombinations(String input, int index, List<String> output) is harder to use than necessary.  The user must:

create the storage for the result
pass in a mysterious 0 value

It would be better to add a "helper" function, to do these tasks for the user:

  public static void main(String[] args) {
    List<String> output = combinations("0?1?");
    System.out.println(output);
  }

  public static List<String> combinations(String input) {
      List<String> output = new ArrayList<>();
      addCombinations(input, 0, output);
      return output;
  }

Flexibility

Will '?' always be the replacement target?  Will the replacements only be the characters '0' and '1'?  It wasn't asked by the interviewer, but you could make the function more flexible.  Or at least mention the possibility to the interviewer.

Code Style

This screams "hacky":

    for (int i = index; i < input.length(); ++i) {
      if (input.charAt(i) == '?') {
        // Do work
        return;
      }
    }
    output.add(input);

You are looping ... but only execute the "real" body of the loop exactly once -- and return if you do -- and if you get to the end without doing work, you do some different work.  As an interviewer, unless you've impressed me elsewhere, I'd be moving on to the next candidate.

What you are doing - what you intend to do - is find the first '?', replace that with '0' and '1', and recursing to find other occurrences for replacement.  Make that clearer.

    int position = input.indexOf('?', index);
    if (position >= 0) {
        addCombinations( ... );
        addCombinations( ... );
    } else {
        output.add(input);
    }

You've got two cases: found and not found.  Recursive and leaf.  No embedded return.  Pretty clear.

Premature Optimization?

You've got an optimization which may be unnecessary.  You pass in index to indicate where to start searching for the next '?'.  But you've replaced the '?', so if you omitted this, and simply started searching from the start of the string, it would still work.  Yes, you will be doing some unnecessary searching of the beginning of the string over and over again, but you've also removed passing the extra argument on the stack, which complicates the addCombinations(...) call.

To be fair, I've used String.indexOf() to do the searching, which is very fast, where as your code did the search itself, with a loop, a String.charAt() and a comparison, which would be significantly slower, so the optimization definitely makes sense in the original code.

The Better Way

As metioned by @greybeard, StringBuffer.setCharAt() would be much simpler to the string manipulation currently being done.  But it is still the wrong choice.  StringBuilder.setCharAt() is what you want.  (StringBuilder is StringBuffer, but without the synchronization overhead).

Instead of creating and throwing away multiple StringBuilder objects, it would be possible to reuse one StringBuilder object for all recursive calls.  Create the StringBuilder in the helper method I introduced, and pass it (instead of input) into the addCombinations() method.  When a ? is found, replace it with a 0 and recurse, then a 1 and recurse, and then change it back to a ? before returning to ensure subsequent processing still works.

public static List<String> combinations(String input) {
    StringBuilder workspace = new StringBuilder(input);
    List<String> output = new ArrayList<>();
    addCombinations(workspace, output);
    return output;
}

private static void addCombination(StringBuilder workspace, List<String> output) {
    int position = workspace.indexOf('?');
    if (position >= 0) {
        workspace.setCharAt(position, '0');
        addCombination(workspace, output);
        workspace.setCharAt(position, '1');
        addCombination(workspace, output);
        workspace.setCharAt(position, '?');
    } else {
        output.add(workspace.toString());
    }
}

There is still a lot of repeated work going on.  With 4 question marks, the first question mark is found once, the second is found twice, the third is found 4 times, and the fourth is found 8 times.  You could count the number of question marks, create an int positions[] array, and locate all the question marks only once.  You could also create an int state[] array, and use that to turn the recursion into a loop.  Finally, once each execution of the loop generates one new combination, you could extract the body of the loop into its own Supplier<String>, and use Stream::generate(Supplier<? extends T>) to create a stream of combinations on demand, instead of filling in a list, but we've ran way beyond reasonable expectations for an interview question.


Write directory new entries to file periodically, keeping the ones that existed in the past Some backup Bash script on my machine copies a specific directory entries filenames into a text file. Entries that no longer exist have to be kept (entries added during a previous run).

The script makes sure that entries which still exist will not be inserted twice in the backup file using sort and uniq.

local file=~/bar-entries.txt
touch "$file" "${file}_"
find ~/foo/bar -maxdepth 1 -type d \! -name bar | cat <"$file" - | sort | uniq > "${file}_"
mv "${file}_" "$file"

Can this code be optimized? Like can this scenario be achieved without relying on a temporary file? Any other improvement is welcome.

Use sort's -u and -o switches:

find … | sort -u - $file -o $file


Object keys to lowercase I wrote a small snippet to convert object keys to lowercase. I would like to listen about the ways to improve the following code:

function toLowerCaseKeys(obj) {
  return Object.keys(obj).reduce(function(accum, key) {
    accum[key.toLowerCase()] = obj[key];
    return accum;
  }, {});
}

console.clear();
console.log(toLowerCaseKeys({'Foo': true}).foo);
console.log(toLowerCaseKeys({'FoO': true}).foo);

If you can go ES6, you can get a more concise version:

Object.keys(o).reduce((c, k) => (c[k.toLowerCase()] = o[k], c), {});

Now, if one or more strings lowercase into the same string, expect only one of them to remain.


More concise way to sum elements of columns in R Is there a more efficient/concise way to add the elements of these columns? Basically the D-# codes are error code for documents, and the Q# codes are classes of error.

##Summary columns##
errorarray$Q1 <- errorarray$"D-03" + errorarray$"D-06" + errorarray$"D-07" + errorarray$"D-14" + errorarray$"D-15" + errorarray$"D-16"
errorarray$Q2 <- errorarray$"D-01" + errorarray$"D-02" + errorarray$"D-04" + errorarray$"D-05"
errorarray$Q3 <- errorarray$"D-08" + errorarray$"D-09" + errorarray$"D-10" + errorarray$"D-11" + errorarray$"D-12" + errorarray$"D-13" + errorarray$"D-17" + errorarray$"D-18" +  errorarray$"D-19"

Thank you

Assuming your data are numeric and your columns are ordered by D-#, the following approaches should work:

Using apply:

errorarray$Q1 <- apply(errorarray[, c(3, 6, 7, 14:16)], 1, sum) # using col indices
errorarray$Q1 <- apply(errorarray[c("D-03", "D-06", "D-07", "D-15", "D-16")], 1, sum) # using column names

Using rowSums:

errorarray$Q1 <- rowSums(errorarray[, c(3, 6, 7, 14:16)]) # using indices
errorarray$Q1 <- rowSums(errorarray[c("D-03", "D-06", "D-07", "D-15", "D-16")]) # using column names

Note you can use ":" to denote the start and end of a number of consecutive columns.


URL and source page scraper The code does seem a bit repetitive in places such as the parenturlscraper module and the childurlscraper module.

Does anyone have any recommendations for improving my code and condensing it a little?

In essence, the code scrapes this site and populates a table with details about each crash, geocoding the location data extracted from the site using Google.

__version__ = '0.1'
__author__ = 'antmancoder'

# Importing of modules required for the script to run successfully
import scraperwiki
import lxml.html
import urlparse
import urllib2
import dateutil.parser
from geopy import geocoders

# Introduction of various global variables required throughout the running of the code
urlstem = "http://planecrashinfo.com"
urlyeardb = "database.htm"
yearsource = urlparse.urljoin(urlstem, urlyeardb)
yearlist = []
sourcepageurl = []

def parenturlscraper():
    """Function scrapes all of the parent URLs from 'planecrashinfo.com/database'"""
    html = scraperwiki.scrape(yearsource)
    root = lxml.html.fromstring(html)

    hrefs = root.cssselect('td a')

    for href in hrefs:
        link = href.attrib['href']
        url = urlparse.urljoin(urlstem, link)
        yearlist.append(url)

def childurlscraper():
    """Function scrapes all of the child URLs from those scraped in the parenturlscraper module"""
    for url in yearlist:
        html = scraperwiki.scrape(url)
        root = lxml.html.fromstring(html)
        hrefs = root.cssselect('td a')
        url = url[0:34]
        for href in hrefs:
            linkurl = href.attrib['href']
            url = urlparse.urljoin(url, linkurl)
            sourcepageurl.append(url)

def sourcepagescraper():
    """Function scrapes respective data for each accident and placed it into DB"""
    for url in sourcepageurl:
        try:
            html = scraperwiki.scrape(url)
            root = lxml.html.fromstring(html)
            for tr in root.cssselect("body"):
                tds = tr.cssselect("td")
                location = coorlookup(tds[7].text_content())
                for td in tds:
                    crashinfo = {}
                    crashinfo['url'] = url
                    crashinfo['date'] = dateutil.parser.parse(tds[3].text_content()).date()
                    crashinfo['time'] = tds[5].text_content()
                    crashinfo['location'] = tds[7].text_content()
                    crashinfo['latitude'] = location[1][0]
                    crashinfo['longitude'] = location[1][1]
                    crashinfo['operator'] = tds[9].text_content()
                    crashinfo['flight no'] = tds[11].text_content()
                    crashinfo['route'] = tds[13].text_content()
                    crashinfo['aircraft type'] = tds[15].text_content()
                    crashinfo['registration'] = tds[17].text_content()
                    crashinfo['cn ln'] = tds[19].text_content()
                    crashinfo['aboard'] = tds[21].text_content()
                    crashinfo['fatalities'] = tds[23].text_content()
                    crashinfo['ground'] = tds[25].text_content()
                    crashinfo['summary'] = tds[27].text_content()

                scraperwiki.sqlite.save(unique_keys=['url'], data=crashinfo)
        except urllib2.HTTPError, err:
            if err.code == 404:
                continue

def coorlookup(location):
    """Function is called from the 'sourcepagescraper' function to geolocate locations listed on website for each accident"""
    g = geocoders.Google()
    try:
        loc = g.geocode(location, exactly_one=True)
        return loc
    except:
        return ("",("",""))

parenturlscraper()
childurlscraper()
sourcepagescraper()

__version__ = '0.1'
__author__ = 'antmancoder'

# Importing of modules required for the script to run successfully

Comments should be used for the non-obvious parts of your code. Explaining that we have imports is obvious. It doesn't need a comment.

import scraperwiki
import lxml.html
import urlparse
import urllib2
import dateutil.parser
from geopy import geocoders

# Introduction of various global variables required throughout the running of the code
urlstem = "http://planecrashinfo.com"
urlyeardb = "database.htm"

Global constants should be in ALL_CAPS, by convention

yearsource = urlparse.urljoin(urlstem, urlyeardb)
yearlist = []
sourcepageurl = []

You shouldn't have global variables. It is better to return and pass parameters.

def parenturlscraper():
    """Function scrapes all of the parent URLs from 'planecrashinfo.com/database'"""
    html = scraperwiki.scrape(yearsource)
    root = lxml.html.fromstring(html)

    hrefs = root.cssselect('td a')

    for href in hrefs:
        link = href.attrib['href']
        url = urlparse.urljoin(urlstem, link)
        yearlist.append(url)

Instead of appending to a global variable, you should append them into a local list and return it.

def childurlscraper():
    """Function scrapes all of the child URLs from those scraped in the parenturlscraper module"""
    for url in yearlist:
        html = scraperwiki.scrape(url)
        root = lxml.html.fromstring(html)

Just did this, you should write a function that that combines the above two lines

        hrefs = root.cssselect('td a')
        url = url[0:34]

Why? This is a good place to have a comment explaining why you just did that.

        for href in hrefs:
            linkurl = href.attrib['href']
            url = urlparse.urljoin(url, linkurl)

You should store in  child_url. As it is you are reusing the variable from before. Your code will actually reeatedly join the elements onto each other. So I'm pretty sure that's not what you wanted.

            sourcepageurl.append(url)

Again, append onto a local list, and then return that.

def sourcepagescraper():
    """Function scrapes respective data for each accident and placed it into DB"""
    for url in sourcepageurl:
        try:
            html = scraperwiki.scrape(url)
            root = lxml.html.fromstring(html)
            for tr in root.cssselect("body"):

Okay, you call it tr but its actually the body. Also, isn't there only ever one body? Do you really need a lloop?

                tds = tr.cssselect("td")
                location = coorlookup(tds[7].text_content())
                for td in tds:

This doesn't seem right. You wnat to iterate over each cell? But then you look at all the cells to fetch the data.

                    crashinfo = {}
                    crashinfo['url'] = url
                    crashinfo['date'] = dateutil.parser.parse(tds[3].text_content()).date()
                    crashinfo['time'] = tds[5].text_content()
                    crashinfo['location'] = tds[7].text_content()
                    crashinfo['latitude'] = location[1][0]
                    crashinfo['longitude'] = location[1][1]
                    crashinfo['operator'] = tds[9].text_content()
                    crashinfo['flight no'] = tds[11].text_content()
                    crashinfo['route'] = tds[13].text_content()
                    crashinfo['aircraft type'] = tds[15].text_content()
                    crashinfo['registration'] = tds[17].text_content()
                    crashinfo['cn ln'] = tds[19].text_content()
                    crashinfo['aboard'] = tds[21].text_content()
                    crashinfo['fatalities'] = tds[23].text_content()
                    crashinfo['ground'] = tds[25].text_content()
                    crashinfo['summary'] = tds[27].text_content()

You can use something like:

crashinfo = {
        'url' : url,
        'date' : dateutil.parser.parse(tds[3].text_content())).date()
        ...
}

which will probably be a bit better. Also, I'd avoid reference the cells by the index. That'll break if new cells are added. Are there classes on the cells you can use? I'd also probably make a big list like:

DATA = [
    ('time', 5),
    ('operator', 9)
]

And then loop over the list to fill in the dict.

                scraperwiki.sqlite.save(unique_keys=['url'], data=crashinfo)
        except urllib2.HTTPError, err:
            if err.code == 404:
                continue

Firstly, you want to put as little as possible in the try block. This is helped by the fact that python has an else block. Everything after .scrape should be in the else block. But the error handling also is a bit off. I think you want to ignore scrapings when the page cannot be found. But you should really reraise the exception when its not that. As it stands, you ignore it. Also, continue is exactly the same as pass here.

def coorlookup(location):
    """Function is called from the 'sourcepagescraper' function to geolocate locations listed on website for each accident"""
    g = geocoders.Google()

Avoid single letter variables names, they are harder to follow

    try:
        loc = g.geocode(location, exactly_one=True)
        return loc

Combine those two lines

    except:

Don't ever (well almost ever) use bare excepts. Catch the specif error you want to handle.
            return ("",("",""))

parenturlscraper()
childurlscraper()
sourcepagescraper()


Checking password strength while honoring functional requirements A program needs a password strength estimator but has several implementation restrictions:

Nothing outside of Python's standard library may be used.
A database of common passwords may not be consulted.
The code must be simple, self-contained, and easy to verify.

As for the interface of the password strength estimator, several requirements are provided:

Empty passwords must have a strength of zero.
Passwords may only use ASCII whitespace, alphanumeric, and punctuation characters.
Passwords with other characters must have a strength less than zero.
Otherwise, strength must be estimated as the bit-space needed to store the password.

In keeping with the caveats listed above, the following function appears to do as required:

import math
import string

def estimate_quality(password):
    if not password:
        return 0
    types = ([False, frozenset(string.whitespace)],
             [False, frozenset(string.ascii_lowercase)],
             [False, frozenset(string.ascii_uppercase)],
             [False, frozenset(string.digits)],
             [False, frozenset(string.punctuation)])
    for character in password:
        for index, (flag, group) in enumerate(types):
            if character in group:
                types[index][0] = True
                break
        else:
            return -1
    space = sum(len(group) for flag, group in types if flag)
    return math.ceil(math.log2(space) * len(password))

Is it possible to refactor the code to improve upon the third implementation restriction?

Your flags shouldn't be in the types definition.
types should be a constant, move it out of the function.
It'd be simpler if you used set.__isub__ and set.__and__, and looped through the types array.

It leaves all the unwanted values in the password too. Allowing for possible improvements on point 3.
You can join the sum and the base for loop together, removing the need for a types list.

TYPES = (
    frozenset(string.whitespace),
    frozenset(string.ascii_lowercase),
    frozenset(string.ascii_uppercase),
    frozenset(string.digits),
    frozenset(string.punctuation)
)

def estimate_quality(password):
    if not password:
        return 0

    password_set = set(password)
    space = 0
    for group in TYPES:
        if password_set & group:
            space += len(group)
        password_set -= group

    if password_set:
        return -1

    return math.ceil(math.log2(space) * len(password))


ExcelVBA loop exports data from spreadsheet to table within Excel I want to nest a for loop to cycle through the Range.Value but ("B4") throws me off and this can't be avoided.  I was considering using an array to do this but can't seem to get the format correct.  "Q3" is a spreadsheet and "Bac Form" is the attached table imported from a PDF that must be kept.  This code works wonderfully - but it is clunky!  I have 3 other unique PDFs with which to complete a similar task and rather than just forcing this thing to work would like to use this opportunity to improve my VBA for-looping skills.

Private Sub CommandButton1_Click()

Dim lr As Long
Dim FolderPath As String

FolderPath = "C:\Users\Joe.Dimaggio\Desktop\PDFs"

lr = Cells(Rows.Count, 1).End(xlUp).Row
lc = Cells(1, Columns.Count).End(xlToLeft).Column

For x = 2 To lr
    Sheets("Bac Form").Range("A2").Value = _
    Sheets("Q3").Cells(1, 1).Value & Sheets("Q3").Cells(x, 1).Value
    Sheets("Bac Form").Range("A3").Value = _
    Sheets("Q3").Cells(1, 2).Value & Sheets("Q3").Cells(x, 2).Value & _
    " (Third Bacterial Quarter)"
    Sheets("Bac Form").Range("A4").Value = _
    Sheets("Q3").Cells(1, 3).Value & Sheets("Q3").Cells(x, 3).Value
    Sheets("Bac Form").Range("B4").Value = _
    Sheets("Q3").Cells(1, 4).Value & Sheets("Q3").Cells(x, 4).Value
    Sheets("Bac Form").Range("A5").Value = _
    Sheets("Q3").Cells(1, 5).Value & Sheets("Q3").Cells(x, 5).Value
    Sheets("Bac Form").Range("A6").Value = _
    Sheets("Q3").Cells(1, 6).Value & Sheets("Q3").Cells(x, 6).Value
    Sheets("Bac Form").Range("A7").Value = _
    Sheets("Q3").Cells(1, 7).Value & Sheets("Q3").Cells(x, 7).Value
    Sheets("Bac Form").Range("A8").Value = _
    Sheets("Q3").Cells(1, 8).Value & Sheets("Q3").Cells(x, 8).Value
    Sheets("Bac Form").Range("A9").Value = _
    Sheets("Q3").Cells(1, 9).Value & Sheets("Q3").Cells(x, 9).Value
    Sheets("Bac Form").Range("A10").Value = _
    Sheets("Q3").Cells(1, 10).Value & Sheets("Q3").Cells(x, 10).Value
    Sheets("Bac Form").Range("A11").Value = _
    Sheets("Q3").Cells(1, 11).Value & Sheets("Q3").Cells(x, 11).Value
    Sheets("Bac Form").Range("A13").Value = _
    Sheets("Q3").Cells(1, 12).Value & Sheets("Q3").Cells(x, 12).Value
    Sheets("Bac Form").Range("A14").Value = _
    Sheets("Q3").Cells(1, 13).Value & Sheets("Q3").Cells(x, 13).Value
    Sheets("Bac Form").Range("A15").Value = _
    Sheets("Q3").Cells(1, 14).Value & Sheets("Q3").Cells(x, 14).Value & _
    " colony/100 ml"
    Sheets("Bac Form").Range("A16").Value = _
    Sheets("Q3").Cells(1, 15).Value & Sheets("Q3").Cells(x, 15).Value
    Sheets("Bac Form").Range("A17").Value = _
    Sheets("Q3").Cells(1, 16).Value & Sheets("Q3").Cells(x, 16).Value & _
    " MPN/100 ml"
    Sheets("Bac Form").Range("A18").Value = _
    Sheets("Q3").Cells(1, 17).Value & Sheets("Q3").Cells(x, 17).Value

        Worksheets("Bac Form").ExportAsFixedFormat Type:=xlTypePDF, Filename:=FolderPath & "\" & _
        Worksheets("Bac Form").Name & "(Q3)" & (x - 1), openafterpublish:=False

Next x

End Sub

Here is the PDF form after importing it into Excel:

I'm having difficulty in visualising your data here, a screenshot would be really useful.

In the meantime:

Option Explicit

This should be at the top of every code module you ever create in VBA. Go to Tools --> Options --> Require Variable Declaration and it will automatically insert it for you from now on.

This is important because without it, VBA will interpret any new variable names (including mis-spellings) as entirely new variables, instead of what you intended.

It also forces you to declare your variables. So you must explicitly give them a type (Long, String, Variant etc.) and a scope (Procedure Dim var As, Module Private Var As, Project Public Var As).

This will then automatically catch all sorts of unintended situations (such as accidentally setting a number equal to an object) which would not be caught if VBA has to assume that all your variables are Variant because you never explicitly declared them.

Magic Variables

And not the good kind of magic either. A Magic Variable is any value which is hardcoded. Especially if it's hardcoded in multiple places.

  lr = Cells(Rows.Count, 1).End(xlUp).Row

Why 1? How was that number determined?

  "Bac Form", "A1", "Q3" etc.

Why those values in particular? If you can't look at a variable and know what it represents, then it has appeared in your code as if by magic.

If you have to hardcode values, then they should be hardcoded precisely once, into a descriptively named variable.

This:

  FolderPath = "C:\Users\Joe.Dimaggio\Desktop\PDFs"

is the right way to hardcode a value. It's written once, and I can see FolderPath later on in the code and know exactly what it is.

This:

For x = 2 To lr
    Sheets("Bac Form").Range("A2").Value = _
    Sheets("Q3").Cells(1, 1).Value & Sheets("Q3").Cells(x, 1).Value
    Sheets("Bac Form").Range("A3").Value = _
    Sheets("Q3").Cells(1, 2).Value & Sheets("Q3").Cells(x, 2).Value & _
    " (Third Bacterial Quarter)"
    Sheets("Bac Form").Range("A4").Value = _
    Sheets("Q3").Cells(1, 3).Value & Sheets("Q3").Cells(x, 3).Value
    Sheets("Bac Form").Range("B4").Value = _
    Sheets("Q3").Cells(1, 4).Value & Sheets("Q3").Cells(x, 4).Value
    Sheets("Bac Form").Range("A5").Value = _
    Sheets("Q3").Cells(1, 5).Value & Sheets("Q3").Cells(x, 5).Value
    Sheets("Bac Form").Range("A6").Value = _
    Sheets("Q3").Cells(1, 6).Value & Sheets("Q3").Cells(x, 6).Value
    Sheets("Bac Form").Range("A7").Value = _
    Sheets("Q3").Cells(1, 7).Value & Sheets("Q3").Cells(x, 7).Value
    Sheets("Bac Form").Range("A8").Value = _
    Sheets("Q3").Cells(1, 8).Value & Sheets("Q3").Cells(x, 8).Value
    Sheets("Bac Form").Range("A9").Value = _
    Sheets("Q3").Cells(1, 9).Value & Sheets("Q3").Cells(x, 9).Value
    Sheets("Bac Form").Range("A10").Value = _
    Sheets("Q3").Cells(1, 10).Value & Sheets("Q3").Cells(x, 10).Value

is not. What happens if the name of your sheet changes? Or your data table moves because someone inserted/deleted a column/row? You'll have to go and re-code every single value.

Naming

Variable names should be Concise, Descriptive and, above all, Unambiguous.

  FolderPath

is a good name because it's clear exactly what the variable contains/represents.

  lr

is not. If I encounter that line halfway through your code, I'm going to go all the way back through to remind myself what it's meant to be representing. Just call it lastRow and you avoid all that trouble.

VBA Naming Conventions:

Typical VBA Naming conventions go as follows:

camelCase for local variables.

PascalCase for Module/Global Variables.

SHOUTY_SNAKE_CASE for constants.

Get in the habit of following them.

Bringing it all together

Step 1, put your worksheets in variables:

For x = 2 To lr
    wsBacForm.Range("A2").Value = wsQ3.Cells(1, 1).Value & wsQ3.Cells(x, 1).Value
    wsBacForm.Range("A3").Value = wsQ3.Cells(1, 2).Value & wsQ3.Cells(x, 2).Value & " (Third Bacterial Quarter)"
    wsBacForm.Range("A4").Value = wsQ3.Cells(1, 3).Value & wsQ3.Cells(x, 3).Value
    wsBacForm.Range("B4").Value = wsQ3.Cells(1, 4).Value & wsQ3.Cells(x, 4).Value
    wsBacForm.Range("A5").Value = wsQ3.Cells(1, 5).Value & wsQ3.Cells(x, 5).Value
    wsBacForm.Range("A6").Value = wsQ3.Cells(1, 6).Value & wsQ3.Cells(x, 6).Value
    wsBacForm.Range("A7").Value = wsQ3.Cells(1, 7).Value & wsQ3.Cells(x, 7).Value
    wsBacForm.Range("A8").Value = wsQ3.Cells(1, 8).Value & wsQ3.Cells(x, 8).Value
    wsBacForm.Range("A9").Value = wsQ3.Cells(1, 9).Value & wsQ3.Cells(x, 9).Value
    wsBacForm.Range("A10").Value = wsQ3.Cells(1, 10).Value & wsQ3.Cells(x, 10).Value

And suddenly the structure becomes clear.

Then:

Add options to change the row/column position of your tables.
Loop through rows/columns.
Add separate variables for each sheet to deal with (potentially) separate positions.
Add an index counter to track where to insert extra text.
Put the filename creation on a separate line.
Add proper Range objects.

And now, if you need to change something, or your data tables get bigger, or you need to modify a specific aspect of the execution, you can just change one value and the rest is done for you.

Private Sub CommandButton1_Click()

    Dim fileName As String

    Const FOLDER_PATH As String = "C:\Users\Joe.Dimaggio\Desktop\PDFs"

    Dim wsBacForm As Worksheet, wsQ3 As Worksheet
    Set wsBacForm = Sheets("Bac Form")
    Set wsQ3 = Sheets("Q3")

    Dim q3BaseRow As Long, q3BaseCol As Long '/ Location of the Q3 data table
    q3BaseRow = 1
    q3BaseCol = 1

    Dim bacBaseRow As Long, bacBaseCol As Long '/ Location of the bac output table
    bacBaseRow = 1
    bacBaseCol = 1

    Dim lastRow As Long, lastCol As Long
    lastRow = wsQ3.Cells(Rows.Count, q3BaseCol).End(xlUp).row
    lastCol = wsQ3.Cells(q3BaseRow, Columns.Count).End(xlToLeft).Column

    Dim bacRow As Long, bacCol As Long, q3Row As Long, q3Col As Long
    Dim row As Long, col As Long, counter As Long

    Dim bacOutputCell As Range, q3HeaderCell As Range, q3DataCell As Range
    Dim outputString As String

    For row = q3BaseRow To lastRow
        bacRow = row - q3BaseRow + bacBaseRow
        q3Row = row

        counter = 0
        For col = q3BaseCol To lastCol
            counter = counter + 1
            q3Col = col

            Set bacOutputCell = wsBacForm.Cells(bacRow, bacBaseCol)
            Set q3HeaderCell = wsQ3.Cells(q3BaseRow, q3Col)
            Set q3DataCell = wsQ3.Cells(q3Row, q3Col)

            outputString = q3HeaderCell.Value & q3DataCell.Value

            Select Case counter '/ used a counter so that it is (absolute position of column) agnostic

                Case Is = 2
                outputString = outputString & " (Third Bacterial Quarter)"

                Case Is = 14
                outputString = outputString & " colony/100 ml"

                Case Is = 16
                outputString = outputString & " MPN/100 ml"

            End Select

            bacOutputCell.Value = outputString
        Next col

        fileName = FOLDER_PATH & "\" & wsBacForm.Name & "(" & wsQ3.Name & ")" & (row - 1)
        wsBacForm.ExportAsFixedFormat Type:=xlTypePDF, fileName:=fileName, openafterpublish:=False
    Next row

End Sub


Script for showing/hiding on click - how can I write this code cleaner? I have this simple code, but it's messy - how do I do it cleaner?
It works like this: If you click on the menu-btn1 the div shows up, and hide others, but if you click on menu-btn1 again it will hide as well.
let coll = document.getElementsByClassName('collection');
let dis = document.getElementsByClassName('display');
let drop = document.getElementsByClassName('drop');

document.getElementById("menu-btn1").addEventListener("click", function() {
  if(coll[0].style.display == 'none') {
    coll[0].style.display = 'block';
    dis[0].style.display = 'none';
    drop[0].style.display = 'none';
  } else {
    coll[0].style.display = 'none';
  }
});

    collection

There is nothing important in css really, only the grid positioning.

First I think that using document.querySelector seems much user friendly than using document.getElementsByClassName. Instead of let coll = document.getElementsByClassName('collection'); coll[0].xxx, we can just use document.querySeletor('collection').xxx.
So code can refactor a bit to
    let coll = document.querySelector('.collection');
    let dis = document.querySelector('.display');
    let drop = document.querySelector('.drop');

    document.getElementById("menu-btn1").addEventListener("click", function() {
        if(coll.style.display == 'none') {
            coll.style.display = 'block';
            dis.style.display = 'none';
            drop.style.display = 'none';
        } else {
            coll.style.display = 'none';
        }
    });

Next, I will place the DOM retrieval code into the event handler, to prevent unnecessary namespace pollution. Though maybe in your working code, you may use the coll for other handling, then it is okay to place there. In addition, I don't think one will change the variable of a DOM to other value, so I would use a constant instead.
    document.getElementById("menu-btn1").addEventListener("click", function() {
        const coll = document.querySelector('.collection');
        const dis = document.querySelector('.display');
        const drop = document.querySelector('.drop');
        if(coll.style.display == 'none') {
            coll.style.display = 'block';
            dis.style.display = 'none';
            drop.style.display = 'none';
        } else {
            coll.style.display = 'none';
        }
    });

However these changes does not resolve the fact that display: none is actually scattered in both HTML and Javascript. I think a better idea is just to place the code in one place. That is remove the styling in HTML.

To do so, I think there are possibly 2 approaches, one is the use of RxJS, another way is to use of data driven approach, e.g. React, Angular, Vue, ...
Data Driven Approach: Use of VueJS
Let's first have a look at How we can implement it in Data Driven Approach.
I will use Vue for demonstration, as it is the simplest to setup among the three.

new Vue({
            el: "#app",
            template: `

                    collection

                    Collection Inside

            `,
            data: function() {
                return {
                    show: false
                }
            },
            computed: {
                styles: function() {
                    return {
                        "display": this.show ? "block": "none"
                    };
                },
            },
        })

As you can see, the HTML code is now placed in Javascript as a template, and we make use of :style and @click in the template, instead of normal HTML. These are Vue specific
syntax and is used in Vue for easy interaction between the Vue component Javascript and the template.
Event Driven Approach: RxJS
Another approach would be RxJS, that just work without a framework, so makes it more easy to adopt in a legacy web application.

rxjs.merge(
    rxjs.of(1), // so that it immediately triggers
    rxjs.fromEvent(document.querySelector("#menu-btn1"), 'click'),
)
    .pipe(
        rxjs.operators.scan((accum, value, index) => {
            return index % 2 == 1;
        }, false)
    ).subscribe(show => {
        const coll = document.querySelector(".collection");
        const value = show ? "block" : "none";
        coll.style.display = value;
    }

    )

    collection

    Collection Inside

Here is all the solution I think of, and it is left to you to decide which way is the cleanest code. To me, I just like the way RxJS is constructed, and do not need to maintain an extra data for storage.


Hangman Game with Python I'm super excited to share my hangman game with you. (haven't added the graphics yet...)
I will be using this program as my final project (for school), but I would like to have this polished before handing it in. I have given my program the limit of 7 attempts, but if the user continues to guess the wrong letters and then suddenly guesses the right letter, the attempt count stays where it's at.
Here's what I mean:
Let's say the word the computer has chosen is 'cougar' (I'm going to pretend I don't actually know this).
BTW this is on my console:
Guess the word: ______
7 chances left

a # <-- my input (1st actual input)
Yes! a is in the word!

Guess the word: ____a_
7 chances left

b # <-- my input (2nd actual input)
Sorry, b is not in the word. Try again.

Guess the word: ____a_
6 chances left

b # <-- my input (this was just to test out if my program would let me know that I have repeated a letter)
You already guessed b. Please try another one!

Guess the word: ____a_
6 chances left

t # <-- my input (3rd actual input)
Sorry, t is not in the word. Try again.

Guess the word: ____a_
5 chances left

q # <-- my input (4th actual input)
Sorry, q is not in the word. Try again.

Guess the word: ____a_
4 chances left

r # <-- my input (5th actual input)
Yes! r is in the word!

Guess the word: ____ar
4 chances left

h # <-- my input (6th actual input)
Sorry, h is not in the word. Try again.

Guess the word: ____ar
3 chances left

c # <-- my input (7th actual input)

this is where the program should've initially said
'Sorry, you ran out of attempts. The word was cougar. Maybe next time ☹.
instead, I get this:
Yes! c is in the word!

Guess the word: c___ar
3 chances left # <-- this is WRONG!!

g # <-- my input (my 1st extra attempt)
Yes! g is in the word!

Guess the word: c__gar
3 chances left

i # <-- my input (my 2nd extra attempt)
Sorry, i is not in the word. Try again.

Guess the word: c__gar
2 chances left

o # <-- my input (my 3rd extra attempt)
Yes! o is in the word!

Guess the word: co_gar
2 chances left

u # <-- my input (my 4th extra attempt)
Yes! u is in the word!

Awesome! You guessed cougar!

So my program is supposed to allow only 7 attempts (with the exception of a repeated letter). As you can clearly see I got 12 attempts! Out of which only 8 were acceptable (7 actual attempts and 1 for repeating 'b'), meaning I got 4 EXTRA attempts. (this is what I need to fix).
Here's my code:
from random import choice

words = choice(['ant', 'alpaca', 'baboon', 'badger', 'bat', 'bear', 'beaver', 'camel', 'cat', 'clam', 'cobra', 'cougar',
                'coyote', 'crow', 'deer', 'dog', 'donkey', 'duck', 'eagle', 'ferret', 'fox', 'frog', 'goat', 'goose',
                'hawk', 'lion', 'lizard', 'llama', 'mole', 'monkey', 'moose', 'mouse', 'mule', 'newt', 'otter', 'owl',
                'panda', 'parrot', 'pigeon', 'python', 'rabbit', 'ram', 'rat', 'raven', 'rhino', 'salmon', 'seal',
                'shark', 'sheep', 'skunk', 'sloth', 'snake', 'spider', 'stork', 'swan', 'tiger', 'toad', 'trout',
                'turkey', 'turtle', 'weasel', 'whale', 'wolf', 'wombat', 'zebra'])

guessed = []
wrong = []
attempts = 7

while attempts > 0:

    out = ''

    for letter in words:
        if letter in guessed:
            out = out + letter
        else:
            out = out + '_'

    if out == words:
        break

    print('Guess the word:',out)
    print(attempts,'chances left\n')

    guess = input()

    if guess in guessed or guess in wrong:
        print('You already guessed',guess+ '.','Please try another one!')
    elif guess in words:
        print('Yes!',guess,'is in the word!')
        guessed.append(guess)
    else:
        print('Sorry,',guess,'is not in the word. Try again.')
        attempts = attempts - 1
        wrong.append(guess)

    print()

if attempts:
    print('Awesome! You guessed',words + '!')
else:
    print('Sorry you ran out of attempts. The word was',words + '.', 'Maybe next time ☹')

You are improving, that's nice.
Since the project is quite minimal, I would just highlight few points for consideration.
Misleading variable names
words and out are misleading, what does out mean, your variables should explain themselves clearly and be as unambiguous as possible, words gives the intent of holding multiple words but actually, it holds just a word.
You might not need it
wrong list is totally unnecessary here, you might have implemented it for future purposes but right now, it isn't really used, define only what you are currently using. So it means guess in wrong is an irrelevant check
guessed happens to append only correct guesses, that's misleading, you might want to make it append both correct and wrong guess and it makes the variable wrong even more irrelevant.


Extracting Country Names from Cell Values This fairly clunky looking VBA script successfully pulls names of countries from an Array, but could it be made shorter?

Mass of text from Column C onward in each row may contain the names of countries, which I want to pull into the corresponding cell in row A. What I have so far is not pretty...

Sub PullCountries()
Dim Rng As Range
Dim Block As Range
Dim i As Long
i = 1

Dim LastRow As Long
LastRow = Range("B1").End(xlDown).Row

While i <= LastRow
Set Rng = Range("B" & i)
Set Block = Rows(i)

Range("I1").Value = i

If InStr(1, Range("C" & i), "Canada") Or InStr(1, Range("D" & i), "Canada") Or InStr(1, Range("E" & i), "Canada") Then
Rng.Offset(0, -1).Value = "Canada"
i = i + 1
ElseIf InStr(1, Range("C" & i), "United States") Or InStr(1, Range("D" & i), "United States") Or InStr(1, Range("E" & i), "United States") Then
Rng.Offset(0, -1).Value = "United States"
i = i + 1
ElseIf InStr(1, Range("C" & i), "Britian") Or InStr(1, Range("D" & i), "Britian") Or InStr(1, Range("E" & i), "Britian") Then
Rng.Offset(0, -1).Value = "UK"
i = i + 1
ElseIf InStr(1, Range("C" & i), "UK") Or InStr(1, Range("D" & i), "UK") Or InStr(1, Range("E" & i), "UK") Then
Rng.Offset(0, -1).Value = "UK"
i = i + 1
ElseIf InStr(1, Range("C" & i), "Spain") Or InStr(1, Range("D" & i), "Spain") Or InStr(1, Range("E" & i), "Spain") Then
Rng.Offset(0, -1).Value = "Spain"
i = i + 1
ElseIf InStr(1, Range("C" & i), "Portugal") Or InStr(1, Range("D" & i), "Portugal") Or InStr(1, Range("E" & i), "Portugal") Then
Rng.Offset(0, -1).Value = "Portugal"
i = i + 1
ElseIf InStr(1, Range("C" & i), "Ireland") Or InStr(1, Range("D" & i), "Ireland") Or InStr(1, Range("E" & i), "Ireland") Then
Rng.Offset(0, -1).Value = "Ireland"
i = i + 1
ElseIf InStr(1, Range("C" & i), "Japan") Or InStr(1, Range("D" & i), "Japan") Or InStr(1, Range("E" & i), "Japan") Then
Rng.Offset(0, -1).Value = "Japan"
i = i + 1
ElseIf InStr(1, Range("C" & i), "Greece") Or InStr(1, Range("D" & i), "Greece") Or InStr(1, Range("E" & i), "Greece") Then
Rng.Offset(0, -1).Value = "Greece"
i = i + 1
ElseIf InStr(1, Range("C" & i), "Italy") Or InStr(1, Range("D" & i), "Italy") Or InStr(1, Range("E" & i), "Italy") Then
Rng.Offset(0, -1).Value = "Italy"
i = i + 1

ElseIf Application.WorksheetFunction.CountA(Rng.Offset(0, 1)) = 0 Then
i = i + 1

Else: i = i + 1
End If
Wend
End Sub

Obviously, this isn't pretty. And given that the task is very repetitive, I reckon it could be made shorter. If this is possible, how could I start.

Furthermore, would it be possible to pull from an array of countries pre-prepared in another worksheet, and refer to that instead of inserting the actual names into the code, as is done here? And finally, if a range contained more than one country, how could I expand the code to pull that information too?

The small(er) things before we tackle the elephant in the room:

Naming

Your names aren't terrible, but could be a lot better. However, whatever you name things, you should comply with standard naming conventions. To wit:

  Local Variables: Written in camelCase.

  Dim localVariable As String
  includes method arguments.

  Module / Global Variables: Written in PascalCase.

  Private ModuleVariable As String
  Global PublicVariable As Long

  Method Names: Verbs. Written in PascalCase

  Private Function ReturnThisValue() As Long
  Public Sub DoThisThing()

  Constants: Written in SHOUTY_SNAKE_CASE

  Public Const CONSTANT_VALUE As String = "This Value Never Changes"

Also, Block is declared and set but never used. It should be removed.

Don't use :

Just don't. Keep your instructions on separate lines. They're too easy to miss and violate too many conventions.

Put things in Variables

Range("C" & i)

You see this? This is telling Excel to go and find that range. Every time you write it. What if you want to check a different column? Right now, you'll have to rewrite the declaration on 20 different lines.

Instead, put it in a variable then just reference the Variable. Now, if the variables need to change, you only have to change them in 1 place, and the rest takes care of itself.

Dim cCell As Range, dCell As Range, eCell As Range
Set cCell = Range("C" & i)
Set dCell = Range("D" & i)
Set eCell = Range("E" & i)

Dim countryName As String

countryName = "Canada"
If Instr(1, cCell, countryName) Or Instr(1, dCell, countryName) Or Instr(1, eCell, countryName) Then
...
...

Don't Repeat Yourself

Also known as DRY. Take your i = i + 1 statement. That is always going to happen. So why write it 20 times when you can just put it at the start or end of your loop?

While i <= lastRow

    Code
    Code
    Code

    ...

    i = i + 1

Wend

Boom. 12 lines of code gone

And now the big stuff:

Refactoring

Refactoring is the process of splitting one big thing into many little things.
Any time you find yourself copy-pasting code, you should be thinking "Hmm, this can probably be turned into a method of some kind".

1st Refactoring

This check:

If Instr(1, cCell, countryName) Or Instr(1, dCell, countryName) Or Instr(1, eCell, countryName) Then

Can be a Separate Method Like so:

Public Function NameIsInRange(ByVal searchName As String, ByRef range1 As Range, range2 As Range, range3 As Range) As Boolean

    Dim result As Boolean
    result = InStr(1, range1, searchName) Or InStr(1, range2, searchName) Or InStr(1, range3, searchName)

    NameIsInRange = result

End Function

And now we're down to:

Sub PullCountries()

    Dim i As Long
    i = 1

    Dim LastRow As Long
    LastRow = Range("B1").End(xlDown).Row

    While i <= LastRow

        Dim resultRange As Range
        Set resultRange = Range("A" & i)

        Dim cCell As Range, dCell As Range, eCell As Range
        Set cCell = Range("C" & i)
        Set dCell = Range("D" & i)
        Set eCell = Range("E" & i)

        If NameIsInRange("Canada", cCell, dCell, eCell) Then
        resultRange = "Canada"

        ElseIf NameIsInRange("United States", cCell, dCell, eCell) Then
        resultRange = "United States"

        ElseIf NameIsInRange("Britian", cCell, dCell, eCell) Then
        resultRange = "UK"

        ElseIf NameIsInRange("UK", cCell, dCell, eCell) Then
        resultRange = "UK"

        ElseIf NameIsInRange("Spain", cCell, dCell, eCell) Then
        resultRange = "Spain"

        ElseIf NameIsInRange("Portugal", cCell, dCell, eCell) Then
        resultRange = "Portugal"

        ElseIf NameIsInRange("Ireland", cCell, dCell, eCell) Then
        resultRange = "Ireland"

        ElseIf NameIsInRange("Japan", cCell, dCell, eCell) Then
        resultRange = "Japan"

        ElseIf NameIsInRange("Greece", cCell, dCell, eCell) Then
        resultRange = "Greece"

        ElseIf NameIsInRange("Italy", cCell, dCell, eCell) Then
        resultRange = "Italy"

        End If

        i = i + 1

    Wend

End Sub

Public Function NameIsInRange(ByVal searchName As String, ByRef range1 As Range, range2 As Range, range3 As Range) As Boolean

    Dim result As Boolean
    result = InStr(1, range1, searchName) Or InStr(1, range2, searchName) Or InStr(1, range3, searchName)

    NameIsInRange = result

End Function

2nd Refactoring

The only things that actually change in the loop are the name to check and the name to output. So, why don't we make those a list? For an iterable list with more than 1 element per line, I'd use an Array.

Let's make a new sheet and give it the codename wsCountryNames. Then a function to get the table and pass it to an Array:

Public Function GetCountryNamesTable() As Variant

    With wsCountryNames

        Dim topLeftCell As Range
        Set topLeftCell = .Cells(1, 1) '/ "A1"

        Dim finalRow As Long
        finalRow = .Cells(.Rows.Count, topLeftCell.Column).End(xlUp).Row

        Dim tableWidth As Long
        tableWidth = 2

        Dim tableRange As Range
        Set tableRange = .Range(topLeftCell, Cells(finalRow, topLeftCell.Column + tableWidth - 1))

    End With

    GetCountryNamesTable = tableRange

End Function

The value in Cell A1 is now in GetCountryNamesTable(1, 1), A2, (2, 1), B1, (1, 2) etc.

Now, we can just iterate through your list:

Sub PullCountries()

    Dim i As Long, j As Long
    i = 1

    Dim LastRow As Long
    LastRow = Range("B1").End(xlDown).Row

    Dim namesList As Variant
    namesList = GetCountryNamesTable

    Dim searchName As String, displayName As String

    While i <= LastRow

        Dim resultRange As Range
        Set resultRange = Range("A" & i)

        Dim cCell As Range, dCell As Range, eCell As Range
        Set cCell = Range("C" & i)
        Set dCell = Range("D" & i)
        Set eCell = Range("E" & i)

        For j = LBound(namesList, 1) + 1 To UBound(namesList, 1) '/ +1 for header row
            searchName = namesList(j, 1)
            displayName = namesList(j, 2)

            If NameIsInRange(searchName, cCell, dCell, eCell) Then
                resultRange = displayName
                Exit For '/ We found our result so we can terminate the loop early
            End If

        Next j

        i = i + 1

    Wend

End Sub

Public Function NameIsInRange(ByVal searchName As String, ByRef range1 As Range, range2 As Range, range3 As Range) As Boolean

    Dim result As Boolean
    result = InStr(1, range1, searchName) Or InStr(1, range2, searchName) Or InStr(1, range3, searchName)

    NameIsInRange = result

End Function

Public Function GetCountryNamesTable() As Variant

    With wsCountryNames

        Dim topLeftCell As Range
        Set topLeftCell = .Cells(1, 1) '/ "A1"

        Dim finalRow As Long
        finalRow = .Cells(.Rows.Count, topLeftCell.Column).End(xlUp).Row

        Dim tableWidth As Long
        tableWidth = 2

        Dim tableRange As Range
        Set tableRange = .Range(topLeftCell, Cells(finalRow, topLeftCell.Column + tableWidth - 1))

    End With

    GetCountryNamesTable = tableRange

End Function

3rd Refactoring

I'm going to put your search range in an Array:

Public Function GetSearchRange() As Variant

    With (codename of your sheet here, call it wsSearchSheet for now)

        Dim topLeftCell As Range
        Set topLeftCell = .Cells(1, 1) '/ "A1"

        Dim finalRow As Long
        finalRow = .Cells(.Rows.Count, 2).End(xlUp).Row '/ "2" for "B" column

        Dim finalCol As Long
        finalCol = 5 '/ "E" column

        Dim tableRange As Range
        Set tableRange = .Range(topLeftCell, Cells(finalRow, finalCol))

    End With

    GetCountryNamesTable = tableRange

End Function

And Re-Jig the IsInRange function to deal with an array value instead:

Public Function ValueContainsString(ByVal valueToSearch As Variant, ByVal searchString As String) As Boolean

    ValueContainsString = InStr(1, CStr(valueToSearch), searchString)

End Function

And implement these changes to the main sub:

Option Explicit

Sub PullCountries()

    Dim i As Long, j As Long, k As Long

    Dim namesList As Variant
    namesList = GetCountryNamesTable

    Dim searchNameCol As Long, displayNameCol As Long
    searchNameCol = 1
    displayNameCol = 2

    Dim searchArray As Variant
    searchArray = GetSearchValues

    Dim searchStartCol As Long, searchEndCol As Long
    searchStartCol = 3
    searchEndCol = 5

    Dim outputCol As Long
    outputCol = 1

    Dim foundMatch As Boolean

    Dim valueToSearch As Variant
    Dim searchName As String, displayName As String

    For i = LBound(searchArray, 1) To UBound(searchArray, 1)
        foundMatch = False

        For j = searchStartCol To searchEndCol
            valueToSearch = searchArray(i, j)

                For k = LBound(namesList, 1) + 1 To UBound(namesList, 1) '/ +1 for header row
                    searchName = namesList(k, searchNameCol)

                    If ValueContainsString(valueToSearch, searchName) Then
                        displayName = namesList(k, displayNameCol)
                        searchArray(i, outputCol) = displayName
                        foundMatch = True
                        Exit For '/ We found our result so we can terminate the loop early
                    End If
                Next k

            If foundMatch Then Exit For
        Next j
    Next i

    '/ Read output back to sheet.
    For i = LBound(searchArray, 1) To UBound(searchArray, 1)
        Range("A" & i) = searchArray(i, outputCol)
    Next i

End Sub

Public Function ValueContainsString(ByVal valueToSearch As Variant, ByVal searchString As String) As Boolean

    ValueContainsString = InStr(1, CStr(valueToSearch), searchString)

End Function

Public Function GetCountryNamesTable() As Variant

    With wsCountryNames

        Dim topLeftCell As Range
        Set topLeftCell = .Cells(1, 1) '/ "A1"

        Dim finalRow As Long
        finalRow = .Cells(.Rows.Count, topLeftCell.Column).End(xlUp).Row

        Dim tableWidth As Long
        tableWidth = 2

        Dim tableRange As Range
        Set tableRange = .Range(topLeftCell, Cells(finalRow, topLeftCell.Column + tableWidth - 1))

    End With

    GetCountryNamesTable = tableRange

End Function

Public Function GetSearchValues() As Variant

    With (codename of your sheet here, call it wsSearchSheet for now)

        Dim topLeftCell As Range
        Set topLeftCell = .Cells(1, 1) '/ "A1"

        Dim finalRow As Long
        finalRow = .Cells(.Rows.Count, 2).End(xlUp).Row '/ "2" for "B" column

        Dim finalCol As Long
        finalCol = 5 '/ "E" column

        Dim tableRange As Range
        Set tableRange = .Range(topLeftCell, Cells(finalRow, finalCol))

    End With

    GetSearchValues = tableRange

End Function

Want to change your country names? Just change the values in your table. Your data moves about? Just change the GetSearchValues targets. You can extend either as far as you like.

Total number of values you now have to change in your code: Maybe 10. And then only if your sheet data moves around positions.

Codenames Addendum

Codenames are big and clever. Every worksheet and workbook has a "name" that the user can see and change.

MyCurrentWB.Worksheets("Country Names")

is referencing a sheet name.

A Codename on the other hand is a secret name that can only be set/changed in the IDE.

the name in brackets is the "name". The name not in brackets is the "codename". It is set in the properties window.

If you give a sheet a codename then the user can change the name as much as they like, all you have to do is use

wsCountryNames.Cells()

in your code and it will keep running.


Compressing large jpeg images I'm working with thousands of large image files in a regularly updated library. The following script does the job (on average reduces my file size ~95%) but costs me around 25 seconds to compress one image. Obviously, I can just let the script run overnight, but it would be cool if I can shave some time off this process. I'm mostly looking for any unnecessary redundancies or overhead in the script that can be trimmed out to speed up the process. I'm still new to Python, so go easy on me.

from PIL import Image
from pathlib import Path
import os, sys
import glob

root_dir = "/.../"

basewidth = 3500

for filename in glob.iglob(root_dir + '*.jpg', recursive=True):
    p = Path(filename)
    img = p.relative_to(root_dir)
    new_name = (root_dir + 'compressed/' + str(img))
    print(new_name)
    im = Image.open(filename)
    wpercent = (basewidth/float(im.size[0]))
    hsize = int((float(im.size[1])*float(wpercent)))
    im = im.resize((basewidth,hsize), Image.ANTIALIAS)
    im.save(new_name, 'JPEG', quality=40)

I think what greybeard is getting at in the comments is that you could get away with squishing these images much more than you presently are. It sounds like you're basically using the reduced versions as thumbnails, but these "thumbnails" are almost twice the width (over three times the area) of a standard HD monitor.

Dropping basewidth to 1920 (or possibly much lower still) seems like a good idea. Contrary to greybeard, I think your JPEG "quality" setting is fine, but you could play around with different variations of smaller-vs-larger and crisp-vs-compressed.

A minute of googling suggests that PIL is in fact the normal choice for image handling. Maybe there's something better, but I'll suppose not. Given that the originals are large (50MB? more?), it may simply be that there's a lot of work to be done. That said, there are some things you can try.

General hardware: Given that this is a heavy task, you may have some luck just running the same code on a fancier computer. This is an expensive option; even if you feel like throwing money at the problem still do plenty of research first.
Storage hardware: One possible bottleneck is the hard drive you're reading from and writing to. If you can easily try the task against a different (faster or slower) harddrive while keeping everything else the same, that may be informative.
Memory: Does you computer have a lot of RAM? Is that memory available for this python script to use? If you have a way of making half the currently available memory unavaliable, check if that makes the process twice as slow.
Memory leaks: I see the line im = Image.open(filename), and looking at the docs suggests that you should probably have a call to load or more likely close someplace.
Parallelism: Your computer probably has multiple processor cores. It's hard to say if or how well this script is using all of those cores. If you try running your script twice at the same time (against non-overlapping target sets), how much slower is each concurrent process?
It's not clear if ANTIALIAS is a valid option to the resize method in the 3.x version. Try NEAREST.
Oh hey there's a thumbnail function that affects the way the file is read from disk!. Playing around with the underlying draft function may also be helpful.

And because this is code review:

Move your format and quality constants up to the same place as basewidth.


Abstracting getter and setter method references for testing Is the following code easy to understand? If not how should it be changed to be more understandable. The purpose of this is reducing similar code in unit tests. This avoids repeating lines for calling getters, setters, and checking for equality.

/**
 * Example string for testing.
 */
private static final String STRING_EXAMPLE="stringExample";

/**
 * Test that a getter and setter are consistent.
 */
@Test
public void testGetterSettersForA() {
    A a = new A();
    assertGetterSetterConsistent(STRING_EXAMPLE,a::setC,a::getC);
}

/**
 * Assert that a getter and setter are consistent.
 * @param input the object to be set.
 * @param setter the setter.
 * @param getter the getter.
 */
private static <T> void assertGetterSetterConsistent(T input, Consumer<T> setter, Supplier<T> getter) {
    boolean consistent =isSetConsistentToGet(input,setter,getter);
    assertTrue("Getter and Setter should be consistent.",consistent);
}

/**
 * Check if the object being set is the same as the object being gotten.
 * @param input the object to be set.
 * @param setter the setter.
 * @param getter the getter.
 * @return whether the object being set is the same as the object being gotten.
 */
private static <T> boolean isSetConsistentToGet(T input, Consumer<T> setter, Supplier<T> getter) {
    setter.accept(input);
    T getValue = getter.get();
    return areEqual(input,getValue);
}

/**
 * Null safe check if two objects are equal.
 * @param first the first object.
 * @param second the second object.
 * @return whether the two objects are equal.
 */
private static <T> boolean areEqual(Object first, Object second) {
    if(first==null) {
        return second==null;
    }
    return first.equals(second);
}

/**
 * Class for testing a getter and setter.
 */
private static final class A{
    /**
     * Variable named c.
     */
    private String c;

    /**
     * Get c.
     * @return c.
     */
    public String getC() {
        return c;
    }

    /**
     * Set c.
     * @param c the value to be set.
     */
    public void setC(String c) {
        this.c = c;
    }
}

I had no issues, but I'm not sure if "easy to understand" is actually answerable.

There's still room for improvement, though: Your areEqual method can be replaced by Objects.equals.

private static <T> boolean isSetConsistentToGet(T input, Consumer<T> setter, Supplier<T> getter) {
    setter.accept(input);
    T getValue = getter.get();
    return Objects.equals(input, getValue);
}

Let me also throw in two frameworks that might make your life easier:

Project Lombok. Lombok can generate getters and setters for your fields, (IMHO) removing the necessity for writing any explicit tests completely.
OpenPojo. I haven't used that one yet, but it claims to do getter/setter validation automagically and has been suggested on Stack Overflow.


Converting image size from Maybe (Int, Int) to (Float, Float) I have a function that returns size of an image IO (Maybe (Int, Int))
It would be nice to convert it to float tuple beautifully.

I am sure there must be a better way than what I currently have:

data Vec2D = Vec2D Float Float

getImageSize :: FilePath -> IO (Maybe (Int, Int))
getImageSize path = do
    img <- readImage path
    case img of
        Left _ -> return Nothing
        Right img' -> return (go img')
    where
        go :: DynamicImage -> Maybe (Int, Int)
        go (ImageRGB8 (Image w h _)) = Just (w, h)
        go (ImageRGBA8 (Image w h _)) = Just (w, h)
        go _ = Nothing

getImageSizeVec :: FilePath -> IO Vec2D
getImageSizeVec path = do
    res <- getImageSize path
    let dims = fromMaybe (0, 0) res
    return $ Vec2D (fromIntegral (fst dims)) (fromIntegral (snd dims))

I do not understand what benefit you get from the conversion to float
I would advise you to not mix - IO with the mostly pure calculation you have

so let us look at your code (minus the IO)

getImageSize :: DynamicImage -> Maybe (Int, Int)
getImageSize (ImageRGB8  (Image w h _)) = Just (w, h)
getImageSize (ImageRGBA8 (Image w h _)) = Just (w, h)
getImageSize _ = Nothing

but readImage already has a more useful return type than Maybe it has an Either String which is giving you error messages instead of Nothing so we can adjust.

getImageSize :: DynamicImage -> Either String (Int, Int)
getImageSize (ImageRGB8  (Image w h _)) = Right (w, h)
getImageSize (ImageRGBA8 (Image w h _)) = Right (w, h)
getImageSize _ = Left "Image type neither ImageRGB8 or ImageRGBA8"

this section could be improved a bit - also working on pure data

sizeToVec :: (Int,Int) -> Vec2D
sizeToVec (x,y) = Vec2D (fromIntegral x) (fromIntegral y)

you can then use those with the many mechanisms haskell provides you:

 readImage path :: IO (Either String DynamicImage)
 getImageSize :: DynamicImage -> Either String (Int, Int)
 sizeToVec :: (Int,Int) -> Vec2D

the last two you can combine à la

fmap sizeToVec (getImageSize img) -- or fmap sizeToVec . getImageSize

and the first two

doStuff = do eitherImg <- readImg mypathtoimg
             return $ do size <- getImageSize
                         sizeToVec size

or

doStuff = do eitherImg <- readImg mypathtoimg
             return (getImageSize eitherImg >>= sizeToVec)

or even shorter

doStuff = readImg mypathtoimg >>= (getImageSize >>= sizeToVec)

doStuff = readImg path >>= fmap sizeToVec . getImageSize

if I am not mistaken (I have no haskell env on this machine, therefore this code is untested - sorry)


Creating a basic calculator for integers Create a simple function that given 3 inputs - number 1, number 2 and operator -  calculates the result. The function supports DMAS operations, and accepts only integers

Is there a better way of solving this problem?

#calculator
def calculate(x , y , op):
    if op == '+' :
        z = x  + y
    elif op == '-' :
        z = x - y
    elif op == '*' :
        z = x * y
    elif op == '//' :
        if y!=0:
            z = x // y
        else:
            z = "undefined"
    return z

print calculate(9 , 9 , '//')
print calculate(11 , 0 , '//')

Make it robust

Your solution could be a lot more robust. For example, you don't currently validate that x or y are integers - you should check, and if they aren't you should reject them. Additionally, you might want to do some string manipulation of the operator (for example, strip spaces).

It could also be useful to not take strings, but instead create an enumeration (Python 3.4+ Python 3.4-). I'd also advise that you look at the linked stack overflow post from the comments (here for posterity). That makes this into a trivial function, where the only work is in your lookup table.

Use exceptions

Your prompt doesn't define what should happen if you get 0 as the denominator; I'd throw an exception (or if you're using operator let it handle it for you) instead of returning "undefined", as it is much more surprising to get this:

>>> calculate(9, 0, "//") + 2
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: must be str, not int

Than it is to get this:

>>> calculate(9, 0, "//") + 2
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 1, in calculate
ZeroDivisionError: division by zero

Additionally, by throwing the expected error other applications that may know how to handle a divide-by-zero event will "just work"TM with your module.

Format your code

Instead of the comment #calculator right above the function definition, which tells us literally nothing, add a docstring that defines inputs, outputs, and purpose of the function.

Additionally, your spacing is a little weird; look up PEP8 and see what it recommends.


Print input one word per line (K&R 1-12 exercise) attempt I'm currently trying to learn C from the K&R book, reading through and attempting the exercises. I came across exercise 1-12 yesterday and was a little stumped but managed to complete it. I was wondering if someone on here would help me improve it, and also guide me on how to approach testing something like this too.

The exercise is to write a program which "prints input one word per line".

#include <stdio.h>

#define TRUE 1
#define FALSE 0

int main(){

    int c, previous_space;

    previous_space = FALSE;
    while ( (c = getchar()) != EOF ){
        if ( c == ' ' || c == '\n' || c == '\t' ){
            if ( previous_space == FALSE ){
                putchar('\n');
            }
            previous_space = TRUE;
        }
        else{
            putchar(c);
            previous_space = FALSE;
        }
    }
    return 0;
}

A few points to consider.

C99 added an actual Boolean type, named _Bool, with constants name _True and _False. By including <stdboolh.h>, you get aliases for those spelled bool, true and false. So unless you need to continue using C89, you might consider using these instead of defining your own names and such.
When you have a value that's conceptually Boolean in nature (e.g., your previous_space) I'd treat it as a Boolean, so to test if it's false, I'd prefer to use if (!previous_space).
Rather than explicitly checking for every possible white-space character in-line, I'd prefer to move that check into a separately little function of its own. Only this is such a common requirement that the standard library already provides isspace, so (at least in real code) it's generally preferred to #include <ctype.h>, and us that (but the book probably hasn't introduced this yet, so I wouldn't worry about it a whole lot just yet).


Linked list operations for a competition After checking some incoming coding competitions, I feel I may want to develop a little bit more on my coding skills and style using some basic algorithms/data structures.

The following two pieces of Java code are about some LinkedList operation. Can you take a quick look and give me some feedback about the accuracy, efficiency, better solution, or code style?

The Java code includes some simple comments, test cases, and it is tested at Java 6 and reviewed by myself.

/*
* purpose of program - for a singly linkedList, switch the elements at odd number positions with
* the ones at even number positions
*
input sample : 1>2>3>4>5; expecting output: 2>1>4>3>5;
input sample : 1>2>3>4; expecting output: 2>1>4>3;

*/

ListTwister.java

package com.algo;

/*
 * purpose of program - for a singly linkedList, switch the elements at odd number positions with
 * the ones at even number positions
 *
input sample : 1>2>3>4>5; expecting output: 2>1>4>3>5;
input sample : 1>2>3>4;   expecting output: 2>1>4>3;

 */

public class ListTwister {

    /* for odd sequence element (array index = 0, 2, etc), store a "branch head", store prev node, and move to next;
    for even sequence element (array index = 1, 3, etc), store the "nextNode", twist current and prev node, link the "branch head" to the currentNode, and link the "prev node" to cached "nextNode"
     */
    public static Node twist(Node head) {
        Node current = head, newHead = null, branchHead = null, tempNext = null, prev = null ;
        int index = 0;

        if (head!=null & head.next!=null ) {
            newHead = head.next;
        }
        else {
            newHead = head;
        }

        while (current != null) {
            //odd sequence
            if (index % 2 == 0) {
                branchHead  = prev;
                prev = current;
                current = current.next;
            }
            //even sequence
            else {
                tempNext = current.next;
                current.next = prev;
                if (branchHead != null ) {
                    branchHead.next = current;
                }
                prev.next = tempNext;
                current = tempNext;
            }

            index ++;
        } // end of while

        return newHead;

    } // end of twist()

    public static void main (String args[]) {
        Node head_list1 = new Node(1, null);
        Node node2_list1 = new Node(2, null);
        Node node3_list1 = new Node(3, null);
        Node node4_list1 = new Node(4, null);
        Node node5_list1 = new Node(5, null);

        head_list1.add(node2_list1 );
        node2_list1.add(node3_list1 );
        node3_list1.add(node4_list1 );
        node4_list1.add(node5_list1 );

        Node head_list2 = new Node(1, null);
        Node node2_list2 = new Node(2, null);
        Node node3_list2 = new Node(3, null);
        Node node4_list2 = new Node(4, null);

        head_list2.add(node2_list2 );
        node2_list2.add(node3_list2 );
        node3_list2.add(node4_list2 );

        head_list1.print();
        //expect to output : 2->1->4->3->5->END.
        twist(head_list1).print();

        head_list2.print();
        //expect to output : 2->1->4->3->END.
        twist(head_list2).print();

    } // end of main()

} // end of class

Node.java

package com.algo;

/*
Node for singly LinkedList;
 */

public class Node {
    public int value;
    public Node next;

    public Node (int value, Node next) {
        this.value = value;
        this.next = next;
    }

    public void add(Node node) {
        this.next = node;
    }

    public void print() {
        Node current = this;
        while (current!=null ) {
            System.out.print(current.value + "->");
            current = current.next;
        }
        if (current==null) {
            System.out.println("END.");
        }
    }
} // end of class

Rename Node.add to setNext, since its a setter method:

public void setNext(final Node node) {
    this.next = node;
}

Create a constructor with only the value parameter:

public Node(final int value) {
    this.value = value;
}

and call this in your main method instead of passing nulls:

final Node head_list1 = new Node(1);

Do not use public fields:

public int value;
public Node next;

Make them private and provide getters and setters.

The value field could be final:

public class Node {
    private final int value;
    ...
}

It should not change, so protect it with final.

Instead of the main method write self-checking unit tests. The posted main should be separated to two test.

The Node class shouldn't be responsible for printing. It should return a String instead of System.out.print() calls. Maybe another client wants to write it to a file or a socket, but System.out.print() made it impossible.

Declare only one variable per line.

    while (current != null) {
        System.out.print(current.value + "->");
        current = current.next;
    }
    if (current == null) {
        System.out.println("END.");
    }

The current == null condition seems always true, so it's unnecessary:

    while (current != null) {
        System.out.print(current.value + "->");
        current = current.next;
    }
    System.out.println("END.");

Check input:

public static Node twist(final Node head) {
    if (head == null) {
        throw new NullPointerException("head cannot be null");
    }
    ...
}

It's unnecessary to set default value for all variables to null:

Node newHead = null;

Furthermore, it could be final since it get its value before it will be used:

final Node newHead;

Read Effective Java, Second Edition, Item 45: Minimize the scope of local variables. (Google for "minimize the scope of local variables", it's on Google Books too.)

For example, tempNext should be declared inside the else branch:

else {
    final Node tempNext = current.next;
    current.next = prev;
    if (branchHead != null) {
        branchHead.next = current;
    }
    prev.next = tempNext;
    current = tempNext;
}

Use camelCase variable names: head_list1 -> headList1. (Effective Java, Second Edition, Item 56: Adhere to generally accepted naming conventions)


Producing a pyramid of capital letters Now I'm reading C Premier Plus by Stephen Prata and there's an exercise 4 in chapter 6 which has no solution in the book, so I can't check if I did it properly. My code works like it should, but I want to know maybe there are some better or cleaner solutions.

Here's the description:

  Have a program request the user to enter an uppercase letter. Use
  nested loops to produce a pyramid pattern like this:

    A
   ABA
  ABCBA
 ABCDCBA
ABCDEDCBA

  The pattern should extend to the character entered. For example, the
  preceding pattern would result from an input value of E .

  Hint: Use an outer loop to handle the rows. Use three inner loops in a row,
  one to handle the spaces, one for printing letters in ascending order, and
  one for printing letters in descending order.

#include <stdio.h>

int main(void) {
    char ch;
    char row;
    char spaces;
    char let;
    char back;

    printf("Please, insert an uppercase letter: ");
        scanf("%c", &ch);

    for(row = 'A'; row <= ch; row++) {
        for(spaces = ch; spaces > row; spaces--)
            printf(" ");

        for(let = 'A'; let < row; let++)
            printf("%c", let);

        for(back = let; back >= 'A'; back--)
            printf("%c", back);
        printf("\n");
    }
    return 0;
}

It's been a while since I coded in C but here are couple observations.

Input validation. You should check if the scanned character is really an upper case letter or exit with an error otherwise.
Choice of functions. Since you're scanning and printing out a character at a time, you could have used putchar instead of printf, and getchar instead of scanf.
Variable naming. let is not the best variable name since it is often used as a keyword in other languages. You also could have used the same variable in place of let and back.
Code formatting. There are a lot of different opinions about how to format source code, but most agree that it is better to put parenthesis around loop and condition bodies.

Here is how I would have formatted your code.

#include <stdio.h>

int main(void) {
    char ch;
    char row;
    char spaces;
    char letter;

    printf("Please, insert an uppercase letter: ");
    scanf("%c", &ch);

    for (row = 'A'; row <= ch; row++) {
        for (spaces = ch; spaces > row; spaces--) {
            printf(" ");
        }
        for (letter = 'A'; letter < row; letter++) {
            printf("%c", letter);
        }
        for (; letter >= 'A'; letter--) {
            printf("%c", letter);
        }
        printf("\n");
    }
    return 0;
}


Bytes to binary conversion function I created a quick function to convert n bytes into binary for my student.
She is not on that level yet, but I think it would be best for her if my demonstrative functions aren't trash.
void binary (void* p, size_t nBytes)
{
    int iBit    = 0;
    int iByte   = 0;

    putchar('[');

    for(iByte = 0; iByte < nBytes; iByte++)
    {
        for(iBit = 0; iBit < CHAR_BIT; iBit++)
        {
            int bit = ((unsigned char* )p)[iByte] >> iBit;

            putchar((bit & 1) ? '1':'0');
        }

        if(iByte < nBytes-1)
        {
            printf("]\t[");
        }
    }

    printf("]\n");
}

it goes with a macro for easier use: #define BINARY(x) binary(&x, sizeof x)
and the obvious disadvantage that it expects a variable and not cannot work with literals.

And yes, endianness is explained to her.

It's not terrible as it stands, but I think there are some ways it might be improved.
Include all needed files
The function needs several #include files that are not listed.  Specifically, it needs these:
#include <stddef.h>
#include <stdio.h>
#include <limits.h>

It's important to list those, especially for a beginner.
Be careful with signed and unsigned
In the binary function, the compares an int iByte to a size_t nBytes, but size_t is unsigned and int is signed.  Instead, declare both variables as size_t types, or better, see the next suggestion.
Count down instead of up
If we count down instead of up, not only do many compilers generate more efficient code, but we also avoid the signed/unsigned problem mentioned above.
Declare variables in as small a scope as practical
Putting all definitions at the top of the function is an antique style of C.  In modern C (C99 and later) we declare variables as late as practical, ideally as they are defined, to avoid problems of uninitialized variables.
Use a bitmask directly instead of calculating
I would write the inner loop like this:
for(unsigned char mask = 1U << (CHAR_BIT-1); mask; mask >>= 1)

That makes it very clear that we're using a bitmask and how it is calculated and handled.
Use a convenience cast
Rather than casting p every time, I'd be inclined to declare an internal unsigned char * variable and use that instead.
Avoid encoding types in names
C is a statically typed language, so it is neither necessary nor desirable to encode the type within the name.  See NL.5 for more.  (Those are C++ guidelines, but this is equally applicable to C.)
Use pointers
Sooner or later, even beginners are going to need to learn how to use pointers.  They have the considerable advantage here in simplifying the code.
Use const where practical
The function does not and should not modify the pointed-to value, so it's better to make that explicit and declare it const.
Consider separating the function into two
One way to think of this code is that it's printing one or more bytes with additional separators and formatting.  That suggests an alternative which is to separate out the part that just prints the ones and zeroes from the loop.
Results
Here's the slightly refactored code using all of the suggestions above:
#include <stddef.h>
#include <stdio.h>
#include <limits.h>

void toBinary(const unsigned char *ptr) {
    for(unsigned char mask = 1U << (CHAR_BIT-1); mask; mask >>= 1) {
        putchar(*ptr & mask ? '1' : '0');
    }
}

void binary(const void* p, size_t nBytes) {
    for(const unsigned char* ptr = p; nBytes; --nBytes) {
        putchar('[');
        toBinary(ptr++);
        putchar(']');
        if(nBytes > 1) {
            putchar('\t');
        }
    }
    putchar('\n');
}

#define BINARY(x) binary(&x, sizeof x)

int main(void) {
    char a = 85;
    int b = 0x12345678;
    BINARY(a);
    BINARY(b);
}


Better container for tracking number of string elements? I'm looking for the most simple and efficient way to track the number of string elements in a container.

I'm currently using std::vector but I wonder if there's a better container which is more suitable for my purpose.

My Code :

#include <iostream>
#include <string>
#include <vector>

class Table
{
public:
    void insert(const std::string &name)
    {
        data.push_back(name);
    }
    void remove(const std::string &name)
    {
        auto it = std::find(data.begin(), data.end(), name);
        if (it != data.end())
            data.erase(it);
    }
    /* get the number of occurrences */
    int count(const std::string &name)
    {
        auto num = std::count(data.begin(), data.end(), name);
        return static_cast<int>(num);
    }
private:
    std::vector<std::string> data;
};

int main(int argc, const char * argv[])
{
    Table table;
    table.insert("Apple");
    table.insert("Apple");
    table.insert("Orange");
    table.insert("Orange");
    table.remove("Orange");
    std::cout << "Number of Apples : " << table.count("Apple") << std::endl;
    std::cout << "Number of Oranges : " << table.count("Orange") << std::endl;
    std::cout << "Number of Lemons : " << table.count("Lemon") << std::endl;
    return 0;
}

The Result :

Number of Apples : 2
Number of Oranges : 1
Number of Lemons : 0
Program ended with exit code: 0

Is there a way to improve the class Table so my program can be more efficient?

And would you recommend a different container than std::vector for the class Table?

P.S: Usually, from 50 to 500 strings will be stored to a container in my program.

@Deduplicator already did a worthy review of the code itself, so I'm not going to repeat their points. I'll just add a few things:

Don't use std::endl, ever. It's almost always wrong, and in the few cases it's not, it's better to use an explicit flush anyway. Just use '\n'.
Add const and noexcept qualifiers to member functions whenever possible. The const helps with correctness, the noexcept can help considerably with performance. In your case count() can be both const and noexcept, and remove can be noexcept.

Now, considering what your code currently does (which I assume accurately reflects your intentions), you can't get any more efficient than a vector. vector just can't be beat. The creator C++ himself didn't believe this, so he tested it himself (he talks about it in this presentation starting at around 40-45 minutes, but his slides were broken, so check this blog post to see them). He tested up to 100,000 elements, so your ~500 element use case is covered.

Okay, so you're already using the best data structure. Can it still be better?

Yes.

The first trick, which @Deduplicator mentioned, is sorting. When a list of elements is sorted, you can do much faster binary searches on it, rather than the linear searches that find() does. So let's start by using a sorted vector:

class Table
{
public:
    void insert(std::string);
    void remove(std::string_view name) noexcept;

    int count(std::string_view name) const noexcept;

private:
    std::vector<std::string> data;
};

As you can see, the class structure itself doesn't change.

Now the simplest thing to do with insert() is simply to push_back() and then sort(). But... that's a little inefficient given that you know the vector is already sorted except for that one last element.

So what would be better would be to find the place where the new value should be inserted, and then insert it there. For that, there's std::upper_bound(), which does a binary search and returns the first value greater than the given value. So if the list is {"apple", "orange", "orange", "pear"}, searching with "orange" would return an iterator to "pear"; if the list is {"apple", "pear"}, it would still return an iterator to "pear". This is ideal because it works whether the value is already in the list or not, and if it is, it gives us the end of the repetitions, so that an insertion will trigger the smallest amount of shuffling around.

So insert() might look like this:

void Table::insert(std::string name)
{
    auto const p = std::upper_bound(data.begin(), data.end(), name);
    data.insert(p, std::move(name));
}

As for remove() it works just fine as is! (It could be a little better. First you could use a binary search rather than std::find(). Then, rather than removing the first value you find, you could remove the last. This will trigger less moving around of stuff in the vector. For that you could use reverse iterators and std::lower_bound().)

For count() it also works just fine as it is! (Once again, it could be better. You could use std::equal_range() to find the full range of matches quickly, then just subtract the iterators you get as result for the count.)

So this is all cool, and should be a bit faster than what you've got. But can we do even better?

Yes.

Consider: Is there any reason to store multiple copies of a string in the data? If someone adds "apple" 10 times, is it really necessary to add 10 "apple"s? Why not just keep "apple" and a count of 10?

To do this, you need to modify your data structure a little bit. Rather than data being a vector<string>, it should be a vector<tuple<string, int>>. That tuple stores the name and the count.

So the data structure now looks like this:

class Table
{
public:
    void insert(std::string);
    void remove(std::string_view name) noexcept;

    int count(std::string_view name) const noexcept;

private:
    std::vector<std::tuple<std::string, int>> data;
};

Note the only change is to the type of data. The class's interface is unchanged.

So what would insert() look like. Well, the first thing you'd have to do is a lower_bound() search to find the value. Then:

If the result of lower_bound() is not data.end(), and it points to a value equal to name, just increment the count.
Otherwise, add a new element to the vector with a count of 1.

That might look like this:

void Table::insert(std::string name)
{
    // A lambda comparator is needed to compare a string to a
    // tuple<string, int>. All it needs to do is get the first
    // element (the string), and do a normal comparison.
    auto const p = std::lower_bound(data.begin(), data.end(), name,
        [](auto&& v, auto&& e) { return v < std::get<0>(e); });

    // If the result is not end, and the string matches...
    if (p != data.end() && std::get<0>(*p) == name))
    {
        // ... update the count.
        ++std::get<1>(*p);
    }
    else
    {
        // ... otherwise, add a new item with a count of 1. Because
        // we used lower_bound, p either points to end or the first
        // element greater than name, so it's the right place to
        // insert before.
        data.emplace(p, std::move(name), 1);
    }
}

remove() also has a special case to consider. First you find the item you want - you can use std::binary_search() for that. Then (assuming you find an item), you decrement its count. Now the special case is if the count is zero, you have to remove the item from the table.

That might look like this:

void Table::remove(std::string_view name) noexcept
{
    auto const p = std::binary_search(data.begin(), data.end(), name,
        [](auto&& v, auto&& e) { return v < std::get<0>(e); });

    if (p != data.end())
    {
        --std::get<1>(*p);
        if (std::get<1>(*p) == 0)
            data.erase(p);
    }
}

And from that, count() should be obvious:

void Table::count(std::string_view name) const noexcept
{
    auto const p = std::binary_search(data.begin(), data.end(), name,
        [](auto&& v, auto&& e) { return v < std::get<0>(e); });

    if (p != data.end())
    {
        return std::get<1>(*p);
    }
    else
    {
        return 0;
    }
}

That will give you the most compact, and almost certainly the fastest results.

The two key factors here are:

Compact data representation. vector is as compact as it gets, but using a single tuple with a name and a count is also much more compact that storing a name multiple times.
Sorting. Sorted data is predictable, which means you can optimize accessing it with tools like binary_search() and lower_bound() and the other binary search algorithms.

Put together, that will match the observable behaviour of your code, but should be many times faster.


Getting matching items in lists based on nullable properties I'm iterating through two lists to find matching objects. The objects have two string properties, Id1 and Id2 below. If Id1 is not null, empty or whitespace, I want to use it to find a matching object in the other list. Otherwise I want to make use of Id2.

If both Id1 and Id2 are set to null by the extension method on either p or p2, I want to skip the iteration, since I know that I won't find a match.

public static string NullIfEmptyOrWhiteSpace(this string value)
{
    return string.IsNullOrEmpty(value) ? null : value;
}

foreach (Product p2 in productList1)
{
    Product match = productList2.FirstOrDefault(p =>
    {
        string pId = p.Id1.NullIfEmptyOrWhiteSpace() ??
                     p.Id2.NullIfEmptyOrWhiteSpace();

        string p2Id = p2.Id1.NullIfEmptyOrWhiteSpace() ??
                      p2.Id2.NullIfEmptyOrWhiteSpace();

        if (pId == null || p2Id == null)
            return false;

        return pId.Equals(p2Id);
    });

    // do stuff with match
}

This seems to work, but it feels like I'm re-inventing the wheel here. Are there any built in methods that I can make use of to simplify the code? Perhaps a class that implements IEqualityComparer?

Example use case:

List<Product> list1 = new List<Product>()
{
    new Product() { Id1 = "123", Id2 = null },
    new Product() { Id1 = "435", Id2 = null },
    new Product() { Id1 = null, Id2 = "123" }
}

List<Product> list12 = new List<Product>()
{
    new Product() { Id1 = "123", Id2 = null },
    new Product() { Id1 = "456", Id2 = null },
    new Product() { Id1 = null, Id2 = "123" }
}

// do something with each matching item in the first list
// which would be item #1 and item #3 above

Let's start refactoring!

foreach (Product p2 in productList1)
{
    Product match = productList2.FirstOrDefault(p =>
    {
        string pId = p.Id1.NullIfEmptyOrWhiteSpace() ??
                     p.Id2.NullIfEmptyOrWhiteSpace();

        string p2Id = p2.Id1.NullIfEmptyOrWhiteSpace() ??
                      p2.Id2.NullIfEmptyOrWhiteSpace();

        if (pId == null || p2Id == null)
            return false;

        return pId.Equals(p2Id);
    });

    // do stuff with match
}

p2Id does not depend on p, so let's move it out of the lambda.

foreach (Product p2 in productList1)
{
    string p2Id = p2.Id1.NullIfEmptyOrWhiteSpace() ??
                  p2.Id2.NullIfEmptyOrWhiteSpace();

    Product match = productList2.FirstOrDefault(p =>
    {
        string pId = p.Id1.NullIfEmptyOrWhiteSpace() ??
                     p.Id2.NullIfEmptyOrWhiteSpace();

        if (pId == null || p2Id == null)
            return false;

        return pId.Equals(p2Id);
    });

    // do stuff with match
}

Now let's define a method on Product to remove the repetition. I called it GetKey, but there's probably a better name.

public string GetKey()
{
    return this.Id1.NullIfEmptyOrWhiteSpace() ?? this.Id2.NullIfEmptyOrWhiteSpace();
}

foreach (Product p2 in productList1)
{
    string p2Id = p2.GetKey();

    Product match = productList2.FirstOrDefault(p =>
    {
        string pId = p.GetKey();

        if (pId == null || p2Id == null)
            return false;

        return pId.Equals(p2Id);
    });

    // do stuff with match
}

It looks like you actually want to call IsNullOrWhiteSpace in NullIfEmptyOrWhiteSpace, so let's fix that.

public static string NullIfEmptyOrWhiteSpace(this string value)
{
    return string.IsNullOrWhiteSpace(value) ? null : value;
}

Actually, we don't need to go through productList2 if p2Id is null.

foreach (Product p2 in productList1)
{
    string p2Id = p2.GetKey();
    if (p2Id == null)
    {
        continue;
    }

    Product match = productList2.FirstOrDefault(p =>
    {
        string pId = p.GetKey();

        if (pId == null)
            return false;

        return pId.Equals(p2Id);
    });

    // do stuff with match
}

Given that we only use p2 for its key, we can rewrite this as

foreach (var key in productList1.Select(product => product.GetKey()).Where(key => key != null))
{
    Product match = productList2.FirstOrDefault(p =>
    {
        string pId = p.GetKey();

        if (pId == null)
            return false;

        return pId.Equals(key);
    });

    // do stuff with match
}

Now let's use a dictionary to replace the call to productList2.FirstOrDefault.

var firstProductWithKey = new Dictionary<string, Product>();
foreach (var product in productList2)
{
    var key = product.GetKey();
    if (key != null && !firstProductWithKey.ContainsKey(key))
    {
        firstProductWithKey.Add(key, product);
    }
}

if (!firstProductWithKey.Any())
{
    return;
}

foreach (var key in productList1.Select(product => product.GetKey()).Where(key => key != null))
{
    Product match;
    if (firstProductWithKey.TryGetValue(key, out match))
    {
        // do stuff with match
    }
}

So what have we gained? Hopefully the intention of the code is a bit clearer, but there's also a performance benefit.

Let's say that the lists have \$n\$ and \$m\$ elements respectively, and that there are no keys common to the two lists.

In the original code, we're evaluating each key in the first list \$m\$ times, and each key in the second list \$n\$ times. So \$2nm\$ keys in total.

In the refactored version, we evaluate each key in the first list once, and each key in the second list once, for \$n + m\$ keys. We're performing much less work, and generating much less garbage for the GC.