Python program to calculate GCD def gcd(a,b):
    if (a>b):
        r1=a
        r2=b
    else:
        r1=b
        r2=a
    if(r1%r2==0):
        print (r2)
    else:
        gcd(r2, r1%r2)
a= int(input("Enter a number"))
b= int(input("Enter a number"))
gcd(a,b)

This code is about finding the greatest common divisor of two numbers.
Are there any better methods?
How can I improve this code?

Bug:

When I input -6 and -3 I expect the result to be 3 but  I get a RecursionError exception instead:

RecursionError: maximum recursion depth exceeded in comparison

For the sake of UX:

Leave some space between the text you display and the data to input:

Enter a number6
Enter a number5

This would be the minimum better:

Enter a number: 6
Enter a number: 5

You know what your program does, but not the user, so your messages must be clear and precise. Ask the user not to input a number but an integer number. Otherwise this can mislead the user to input a real number, or input a character by mistake (or even try to break into your software):

Traceback (most recent call last):
  File "cr.py", line 12, in <module>
    a= int(input("Enter a number"))
ValueError: invalid literal for int() with base 10: 'g'

Your program does not give a choice for the user to exit. The minimum you can do for this purpose is to handle the KeyboardInterrupt exception corresponding to when the user presses Ctrl + c
The messages are intended to communicate with the user, so let him know what the programs does:

print('Computing the GCD of two numbers.\n')

Divide and conquer:

This is a consequence of the aformentioned: it is time to implement a function whose only purpose is care about the user's input:

def read_an_integer_user_input():
    a = None
    while type(a) != int:
        try:
            a = int(input('Enter an integer: '))
        except ValueError:
            print('I expect an integer value. Try again.')
        except KeyboardInterrupt:
            print('\nGoodbye!')
            break
    return a

Do not print, return:

We expect the function that computes something to return the result not to print it. Not only this is the function definition by essence, but this way you give more chance for your code to be reused and properly. So instead of coding print(r2) I would write return r2; by the way, do not leave space between  print and (.
Iterative version:

Given the recursion bug previsouly mentioned, I would opt for an iterative implementation of your code:

def compute_gcd(a,b):
    a = abs(a)
    b = abs(b)
    while b:
        a, b = b, a%b
    return a

Full program:

Let us gather the elements above to build a small coherent program:

def compute_gcd(a,b):
    a = abs(a)
    b = abs(b)
    while b:
        a, b = b, a%b
    return a

def read_an_integer_user_input():
   a = None
   while type(a) != int:
       try:
           a = int(input('Enter an integer: '))
       except ValueError:
           print('I expect an integer value. Try again.')
       except KeyboardInterrupt:
           print('\nGoodbye!')
           break
   return a

if __name__ == '__main__':
    print('Computing the GCD of two numbers.')
    a = read_an_integer_user_input()
    b = read_an_integer_user_input()
    if a and b:
       print('The GCD of {} and {} is: {}'.format(a, b, compute_gcd(a, b)))

#--input
a = int(input("Enter a number: "))
print() # new line
b = int(input("Enter a number: "))
print() # new line

#--check/make a > b only once
if (b > a):
    a,b = b,a

#--one simple loop for GCD
while(a%b != 0):
    x,a = a,b
    b = x%b

#--If one or both values are negative
if(b < 0):
    b *= -1

print ("GCD = ", b)

Try this. It has one small loop for GCD. And -ve value correction. I've added comments to describe the code.


Extract a list of users from a text file fo=open("haproxyfile.txt","r")
users=[]
for lines in fo.readlines():
    if 'user' and 'password' in lines:
        items=lines.split() #converts to list"""
        users.append(items[1]) #append the second item that is the username"""

print users

The content of haproxyfile.txt is below

ssl-default-bind-ciphers EECDH+EDSA+ACM:CDH+aRSA+AEdfGCM:EECDH+ECDSA+SHA384:EEC+
ESA+SHA:EDH+aRSA+SHA384:EECDH+aRSA+SHA256:EDH:AES128-GCM-SHA256:AES128-SHA256:AES128-S
HA:EDH+SA:AES256-GCM-SHA384:AES-SHA256:AES256-SHA:!RC4:!aNULL:!eNULL:!LOW:!3D:!MD5:!EXP:
!PSK:!SRP:!DSS

        # Users
        userlist IntacctAdmins
          group admins users aasghar,achouchoulas,bwright,calla,gspiliotis,haris,pdam
          user aasghar password $1$CFdTKLSRSbQME/
          user achouchoulas password $1$uLy//uPCaVSNhfFG/
          user bwright password $1$q3Fk$kmqM.J531
          user calla password $1$f%g23a44$Bl.1fPj/
          user gspiliotis password $1$d/2OI.TQOiV1
          user haris password $1$mwQ$uG/1mUjE0
          user pdam password $1t24Z$K4FUH/

What I need to parse from the content

The line after having the words groups are the lines where my user names are located. They are the single words after user keyword.I have fetched it using the method above.Is there any better way to fetch it?

There are quite a few possible improvements.

You should open files with the with keyword, this way they get closed no matter what (you don't even close your file).

if 'user' and 'password' in lines: does not mean what you think it does. All non-empty strings are truthy in Python, so if "user" is always True. So you only really check that the line contains "password". Fortunately you don't have any lines containing "password", but not "users", so this bug has not yet become obvious.

The easiest fix for this would be using this:

if "user" in lines and "password" in lines:
    # Stuff

In summary, I would re-write your code like this:

with open("haproxyfile.txt") as f:
    users = [line.split()[1] for line in f
             if line.strip().startswith("user") and "password" in line]

print users

Note that I omitted the "r" parameter to open, because read-only is the default mode. I also used str.startswith to check if it starts with a string. The strip is probably needed, to get rid of the whitespace at the beginning of your lines. If that is a copy&paste error as well, omit it.

I also wrote it as a list-comprehension and used the fact that you can directly iterate over a file.

Shouldn't passwords be coded?  If passwords are not coded-could it put user privacy and/or administrator settings at risk?


Idiomatic Ruby FizzBuzz Can anyone offer any criticism on the following at a simple FizzBuzz in terms of Ruby idioms or otherwise?

class FizzBuzz

  def initialize(fizz = 'Fizz', buzz = 'Buzz')
    @fizz = fizz
    @buzz = buzz
  end

  def is_number_divisible_by?(number, divisor)
    number % divisor == 0
  end

  def get_text(number)
    fizz_buzz_text = ''
    if is_number_divisible_by?(number, 3)
      fizz_buzz_text += @fizz
    end
    if is_number_divisible_by?(number, 5)
      fizz_buzz_text += @buzz
    end
    return fizz_buzz_text
  end

end

fb = FizzBuzz.new
(1...101).each do |i|
  fb_text = fb.get_text(i)
  puts fb_text.empty? ? i : fb_text
end

First off: If you want a range that goes from 1 to x, inclusive, write (1..x), not (1...x+1).

Second, if it's the last line executed in a method, it's automatically returned -- so in get_text, you don't need return in the last line, just the actual code.

Also, get_text should return the text to be printed, not what it might be. Move the empty check (fb_text.empty ? i : fb_text) to get_text instead of having it at the puts call. See the code at the end for clarification.

if statements with just one line should be expressions at the end. Again, see the modified code for what I mean.

Methods defined in a class are generally talking about the object they're run on. I'd pull is_number_divisible_by? out of the class and dropping it at the beginning of your code instead, and renaming it to something like divisible_by? instead (since it's shorter and carries the same information).

Aside from that (and an odd extra newline at the beginning of your class definition), it looks good! Here's your code with these suggestions:

def divisible_by?(number, divisor)
  number % divisor == 0
end

class FizzBuzz
  def initialize(fizz = 'Fizz', buzz = 'Buzz')
    @fizz = fizz
    @buzz = buzz
  end

  def get_text(number)
    fizz_buzz_text = ''
    fizz_buzz_text += @fizz if divisible_by? number, 3
    fizz_buzz_text += @buzz if divisible_by? number, 5
    fizz_buzz_text.empty? ? number : fizz_buzz_text
  end

end

fb = FizzBuzz.new
(1..100).each do |i|
  puts fb.get_text i
end

Note: Personally, I wouldn't do this in an OOP way. Ruby is much more oriented towards functional programming, though of course OOP is perfectly valid; besides, functional is a bit simpler. My preferred way to do it would be something like this:

(1..100).each do |num|
  output = ''
  output += 'Fizz' if num % 3 == 0
  output += 'Buzz' if num % 5 == 0
  puts output.empty? ? num : output
end

QPaysTaxes has a lot of great points, but one thing that stands out is that you are asking for the "Idiomatic" way of doing things in ruby. For fizzbuzz, that would involve a hash mapping numbers to words so you could easily and arbitrarily extend it.  I would rewrite QPaysTaxes get_text function as such to get more idiomatic ruby:

# in class FizzBuzz
TEXT = {
  3 => "Fizz",
  5 => "Buzz",
  # 7 => "Jazz",
  # 11 => "Guzz",
} # or set this dynamically during initialization, whatever

def get_text(number)
  fizz_buzz_text = TEXT.inject("") do |str, rule|
    number % rule[0] == 0 ? str + rule[1] : str
  end

  fizz_buzz_text.empty? ? number : fizz_buzz_text
end

Now you can uncomment those extra rules or throw your own in with ease. If the method for determining replacement or not changes from being num divisible by rule to ((num * 6) + 4) is divisible by rule, you only need to change a single location instead of once per rule. (This practice is called DRY, or "Don't Repeat Yourself".)

I can explain the code further if you want, just leave a comment below.


Consolidating a dictionary from nested dictionary Currently learning from "LPTHW - Zed Shaw" and "Automating the boring stuff".

There is a nested dictionary of the type {'string1': {inner dict 1}, 'string2':{inner dict 2},...}.

I want a non nested consolidated dictionary of all the keys and values from all of the {inner dicts}. I have written the code below, but I am not convinced it is the most efficient way. I am looking for suggestions on how the code can be simplified by utilising logic within a single loop or by making use of some other modules and methods.

Is that new method efficient from a processing point of view?

allGuests = {'Alice': {'apples': 5, 'pretzels': 12},
           'Bob': {'ham sandwiches': 3, 'apples': 2},
           'Carol': {'cups': 3, 'apple pies': 1}}

item_list = []
count_list = []
consolidated = {}
all_items_list = list(allGuests.values()) #This is a list of sub dictionaries

#Loop for the number of guests who brought items
for guest in range(0, len(allGuests)):

#Creating 2 separate lists for 'items' and 'their count'

    item_list = item_list + list(all_items_list[guest].keys())
    count_list = count_list + list(all_items_list[guest].values())

#Creating a consolidated dictionary
for index in range(0, len(item_list)):

    if not consolidated.get(item_list[index]):
        consolidated.setdefault(item_list[index], count_list[index])

    else:
        consolidated[item_list[index]] = consolidated[item_list[index]] + count_list[index]

    pprint.pprint(consolidated)

Expected output:

{'apple pies': 1, 'apples': 7, 'cups': 3, 'ham sandwiches': 3, 'pretzels': 12}

If your dictionary always has the structure {key: {key: int}}, i.e. the values of the inner dictionaries are all integers, this can be easily achieved using collections.Counter.update:

from collections import Counter

def consolidate(d):
    consolidated = Counter()
    for sub_dict in d.values():
        consolidated.update(sub_dict)
    return dict(consolidated)

if __name__ == "__main__":
    all_guests = {'Alice': {'apples': 5, 'pretzels': 12},
                  'Bob': {'ham sandwiches': 3, 'apples': 2},
                  'Carol': {'cups': 3, 'apple pies': 1}}
    print(consolidate(all_guests))
    # {'apple pies': 1, 'apples': 7, 'cups': 3, 'ham sandwiches': 3, 'pretzels': 12}

Note that Python has an official style-guide, PEP8, that recommends using lower_case both for functions and for variables. I also put this code into a function, so it is reusable, and only called it with the test case under a if __name__ == "__main__": guard to allow importing from this module without executing the test case.

Below code worked as per one suggestion on stackoverflow

  consolidated = {}
  for guest in allGuests.values():
      for key, value in guest.items():
          if not consolidated.get(key):
              consolidated.setdefault(key, value)

          else:
              consolidated[key] = consolidated[key] + value

          pprint.pprint(consolidated)


Turning numbers into a string of words - challenge I just completed this challenge at CodeEval.

  You are given a positive integer number. This represents the sales made that day in your department store. The payables department however, needs this printed out in english. NOTE: The correct spelling of 40 is Forty. (NOT Fourty)

My solution works, and it works pretty well. However I think there has to be an easier way to do this:

import sys

def nums_to_words(number):
    units = ['', 'one', 'two', 'three', 'four', 'five', 'six', 'seven', 'eight', 'nine']
    teens = ['', 'eleven', 'twelve', 'thirteen', 'fourteen', 'fifteen', 'sixteen',
             'seventeen', 'eighteen', 'nineteen']
    tens = ['', 'ten', 'twenty', 'thirty', 'forty', 'fifty', 'sixty', 'seventy',
            'eighty', 'ninety']
    thousands = ['', 'thousand', 'million', 'billion', 'trillion', 'quadrillion',
                 'quintillion', 'sextillion', 'septillion', 'octillion',
                 'nonillion', 'decillion', 'undecillion', 'duodecillion',
                 'tredecillion', 'quattuordecillion', 'sexdecillion',
                 'septendecillion', 'octodecillion', 'novemdecillion',
                 'vigintillion']
    words = []

    if number == 0:
        words.append("zero")
    else:
        number_string = "%d" % number
        length_of_number_string = len(number_string)
        number_groups = (length_of_number_string + 2) / 3
        new_number_string = number_string.zfill(number_groups * 3)
        for i in range(0, number_groups * 3, 3):
            x, y, z = int(new_number_string[i]), int(new_number_string[i + 1]), int(new_number_string[i + 2])
            w = number_groups - (i / 3 + 1)
            if x >= 1:
                words.append(units[x])
                words.append("hundred")
            if y > 1:
                words.append(tens[y])
                if z >= 1:
                    words.append(units[z])
            elif y == 1:
                if z >= 1:
                    words.append(teens[z])
                else:
                    words.append(tens[y])
            else:
                if z >= 1:
                    words.append(units[z])
            if (w >= 1) and ((x + y + z) > 0):
                words.append(thousands[w])

    name = []

    for word in words:
        name.append(word.title())

    return ''.join(name) + "Dollars"

if __name__ == '__main__':
    with open(sys.argv[1], "r") as numbers:
        for num in numbers.readlines():
            print(nums_to_words(int(num)))

Is there anything that is obviously noticeable that I can do better?

I think the code in the post is fine: the English language rules are quite complicated, and it's going to take a certain amount of computing effort however you implement it.
The variables units, teens, tens, and so on are always the same, so they could be global constants, rather than being constructed again every time the function is called.
It's conventional in Python for constants to have names written in all capital letters (see PEP8), so UNITS instead of units.
Instead of writing:

UNITS = ['', 'one', 'two', 'three', 'four', 'five', 'six', 'seven', 'eight', 'nine']

I think this is easier to read:

UNITS = 'zero one two three four five six seven eight nine'.split()

It's convenient to combine UNITS and TEENS into a single list giving all the names for numbers less than 20.
The code reads a string from standard input, converts it to an integer (by calling int(num), then converts the number back to a string (by calling "%d" % number), then splits the string into pieces, and converts each piece back to a number (by calling int(new_number_string[i])). This back-and-forth conversion is unnecessary, because you can split a number into 3-digit groups by repeatedly calling divmod until you get down to zero:

>>> divmod(123456789, 1000)
(123456, 789)
>>> divmod(123456, 1000)
(123, 456)
>>> divmod(123, 1000)
(0, 123)

The function nums_to_words does two things: (i) it converts a number to English words; and (ii) it capitalizes the words, joins them and appends "Dollars". The principle of separation of concerns suggests that these should be two functions. And that would be useful, because then the function implementing part (i) could call itself recursively.

It's also slightly more convenient to generate the results (by calling yield or yield from) instead of constructing a list. Then you could write this for concern (i):

UNITS = """
    zero one two three four five six seven eight nine ten eleven
    twelve thirteen fourteen fifteen sixteen seventeen eighteen
    nineteen
""".split()
TENS = "zero ten twenty thirty forty fifty sixty seventy eighty ninety".split()
THOUSANDS = """
    zero thousand million billion trillion quadrillion quintillion
    sextillion septillion octillion nonillion decillion undecillion
    duodecillion tredecillion quattuordecillion sexdecillion
    septendecillion octodecillion novemdecillion vigintillion
""".split()

def english_words(n):
    """Generate words (excluding "and") naming the non-negative number n
    in English.

    """
    if n < 20:
        yield UNITS[n]
    elif n < 100:
        tens, units = divmod(n, 10)
        yield TENS[tens]
        if units:
            yield UNITS[units]
    elif n < 1000:
        hundreds, remainder = divmod(n, 100)
        yield UNITS[hundreds]
        yield "hundred"
        if remainder:
            yield from english_words(remainder)
    else:
        groups = []             # Groups of three digits from n.
        while n:
            n, group = divmod(n, 1000)
            groups.append(group)
        for i, group in reversed(list(enumerate(groups))):
            if group:
                yield from english_words(group)
                if i:
                    yield THOUSANDS[i]

and this for concern (ii):

def num_to_words(n):
    return ''.join(w.title() for w in english_words(n)) + "Dollars"

This is another way that you could solve it. Using the patterns inherent in the 'tricky' numbers (teens, twenties, etc...) the majority required strings can be constructed on the fly, rather than precomputed. For instance, for the number 30, the tens string is 'thirty', which is the first two characters of the string representation of 3-th- plus the tens modifier for 3-ir-plus the end ty. This general pattern holds true for number from 20 to 99, and a similar one is true for 13,15,16,17,18, and 19 (not 14 because of the for/four spelling issue) with the teen ending. The rest of the digits of the number (100s etc...) are starightforward to solve.

def toString(number):
    numbers={1:'one',2:'two',3:'three',4:'four',5:'five',6:'six',7:'seven',8:'eight',9:'nine'}
    teens={0:'ten ',1:'eleven ',2:'twelve ',4:'fourteen '}
    modifiers_tens={2:'en',3:'ir',4:'r',5:'f',6:'x',7:'ven',8:'gh',9:'ne'}
    positions={2:'hundred and ', 3:'thousand ', 4:'million ', 5:'billion ', 6:'trillion ', 7:'quadrillion ',
                 8:'quintillion ', 9:'sextillion ', 10:'septillion ', 11:'octillion ',
                 12:'nonillion ', 13:'decillion ', 14:'undecillion ', 15:'duodecillion ',
                 16:'tredecillion ', 17:'quattuordecillion ', 18:'sexdecillion ',
                 19:'septendecillion ', 20:'octodecillion ', 21:'novemdecillion ',
                 22:'vigintillion '}
    number_str=str(number)
    number_list=list(number_str)[::-1]#Reverse input order to parse number in ascending powers of 10, rather than descending
    answer=[]
    if len(number_list)==1:#Handle the trivial case
        answer.append(numbers[int(number_list[0])])
        return "".join(answer[::-1])
    try:#Handle special 10s digit cases
        if int(number_list[1])==1:#If the number is a teen
            answer.append(teens[int(number_list[0])])#Try to use string from precomputed teen numbers that don't follow a pattern.
        else:#If the 10s digit is not 1
            answer.append(numbers[int(number_list[0])])#Append the ones digit, since it has no effect on the 10s digit
            answer.append(numbers[int(number_list[1])][:2]+modifiers_tens[int(number_list[1])]+'ty ')#Construct the 10s digit using the pattern
    except KeyError:#If the teen value had not been precomputed, construct it using the pattern.
        answer.append(numbers[int(number_list[0])][:2]+modifiers_tens[int(number_list[0])]+'teen ')
    for index,i in enumerate(number_list):#For all other digits of 'number', add the string representation.
        if index>1:
            try:
                answer.append(numbers[int(i)]+' '+positions[index])
            except KeyError:
                pass
    return "".join(answer[::-1])#Reverse to show output in the usual descending powers of 10


Eliminate copy/paste and Factor logic out of foreach loop I have two conditions that need to traverse the same data but inside the traversal the logic changes slightly for each respective condition and in different places. If it was the same place that would be easy to factor out, but what's the best approach if it's not all in the same place for every condition? I seem to run into this situation often when parsing data.

switch(batch.DocumentType.ImportedFileDisposition)
{
    case ImportedFileDispositionActions.Delete:
        foreach (string documentMetadataFile in documentMetadataPaths)
        {
            string documentPath = Path.GetDirectoryName(documentMetadataFile);
            XDocument metadata = XDocument.Load(documentMetadataFile);
            foreach (var pageElement in metadata.Root.Element("Pages").Elements("Page"))
            {
                string pageName = null;
                XAttribute attribute = pageElement.Attribute("FileName");
                if (attribute != null)
                    pageName = attribute.Value;

                string pagePath = null;
                string originalPagePath = null;
                if (!string.IsNullOrWhiteSpace(pageName))
                {
                    pagePath = IOUtility.PathCombine(documentPath, pageName);
                    File.Delete(pagePath);
                }
            }
            File.Delete(documentMetadataFile);
            Directory.Delete(documentPath, true);
        }
        break;
    case ImportedFileDispositionActions.Archive:
        foreach (string documentMetadataFile in documentMetadataPaths)
        {
            string documentPath = Path.GetDirectoryName(documentMetadataFile);
            string baseImportDirectory = documentPath;
            string archiveDirectory = GenerateArchiveDirectory(batch, documentPath);
            XDocument metadata = XDocument.Load(documentMetadataFile);
            foreach (var pageElement in metadata.Root.Element("Pages").Elements("Page"))
            {
                string pageName = null;
                XAttribute attribute = pageElement.Attribute("FileName");
                if (attribute != null)
                    pageName = attribute.Value;

                string pagePath = null;
                string originalPagePath = null;
                if (!string.IsNullOrWhiteSpace(pageName))
                {
                    pagePath = IOUtility.PathCombine(documentPath, pageName);
                    string newArchivePath = IOUtility.PathCombine(archiveDirectory, pageName);
                    try
                    {
                        IOUtility.MoveFile(pagePath, newArchivePath);
                    }
                    catch
                    {
                        try
                        {
                            IOUtility.CopyFile(pagePath,newArchivePath, true);
                        }
                        catch (Exception ex)
                        {
                            throw new IOException("Failed to move or copy file: " + pagePath + " to archive: " + newArchivePath, ex);
                        }
                    }
                    File.Delete(pagePath);
                }
            }
            File.Delete(documentMetadataFile);
            Directory.Delete(documentPath, true);
        }
        break;
}

I'd start by writing another method, containing what's common in both blocks; then I'd extract another method for processing a single file, and in that method I'd decide if I want to archive or just delete the file... and then I'd get annoyed with having a method that does two things and I'd extract some MoveOrCopyToArchive method, and try to delete the file in all cases.

But that's just easy moving stuff around and reducing redundancies.

This is more worrying:

try
{
    IOUtility.MoveFile(pagePath, newArchivePath);
}
catch
{
    try
    {
        IOUtility.CopyFile(pagePath,newArchivePath, true);
    }
    catch (Exception ex)
    {
        throw new IOException("Failed to move or copy file: " + pagePath + " to archive: " + newArchivePath, ex);
    }
}
File.Delete(pagePath);

Nested try/catch blocks have a funky smell, but the worrying line is the last one: what makes you think pagePath can be safely deleted? If something went wrong moving or copying it, my bet is on File.Delete throwing an exception as well - and that one will bubble up your stack, meaning Directory.Delete would never get called... and the entire application might just die a horrible death.

Now back to that nested try/catch - first you attempt to move the file, and if that fails, you try to copy it. There are several reasons for failing to move a file, several of which will also fail to copy it. I think that's a good case for cherry-picking which exceptions will warrant trying to copy, and which ones aren't worth it:

ArgumentNullException is almost certainly ruled out, you're checking if pageName is null already. But I don't know what IOUtility.PathCombine does, and I have no garantee that GenerateArchiveDirectory returns a valid path either. Better safe than sorry; catching this exception means attempting to copy the same file/path is 100% certain to fail as well.
ArgumentException or NotSupportedException would be thrown for a path containing characters invalid in a path, or for a path in an invalid format. No need to try copying that file, it's garanteed to fail anyway.
DirectoryNotFoundException and PathTooLongException would also fail copying.
IOException would be thrown if the destination file already exists, or if the source filename wouldn't be found. Clearly not worth trying to copy.
UnauthorizedAccessException - ok, moving can't work, but maybe the file can be copied.

https://msdn.microsoft.com/library/system.io.file.move(v=vs.110).aspx

The catch block that doesn't specify any exception not only enters on any exception, without a handle on the exception object you have lost the original exception that tells you how the first operation - moving the file - failed.

Catching System.Exception would be over-the-top here; as shown above, you'd only really want to try a copy when an UnauthorizedAccessException is thrown - every other exception you can possibly catch there, would be caused by something that would also make the copy fail.

But then if any of these exceptions is caught, my bet is on File.Delete throwing the same exception.

I'll add that throwing your own IOException is confusing here, because the System.IO methods you're using can throw that same exception type, with a different meaning. If you really want to wrap the exception with one of your own, then you have a case for a custom exception type here - a FileArchiveException or something similar.

Why is there no try/catch block in the ImportedFileDispositionActions.Delete case?

I'd do it something like this:

if (fileDisposition == ImportedFileDispositionActions.Archive)
{
    SendToArchive(pagePath, documentMetadataFile, batch);
}

if (File.Exists(pagePath))
{
    File.Delete(pagePath);
}

//Assert.IsFalse(File.Exists(pagePath));

Understanding at a glance what's going on is easier if each idea reads like a sentence.

The nested try/catch can be avoided by extracting a method out of the try block:

private readonly ILogger _logger; // do something about exceptions!

private void SendToArchive(string path, string metadata, FooBar batch)
{
    try
    {
        var archivePath = GenerateArchiveDirectory(path, metadata, batch);
        MoveOrCopyToArchive(archivePath);
    }
    catch(IOException exception)
    {
        _logger.Info(exception);
    }
    catch(Exception exception)
    {
        _logger.Debug(exception);
    }
}

private void MoveOrCopyToArchive(string path)
{
    try
    {
        File.Move(path, ArchivePath);
    }
    catch(UnauthorizedAccessException exception)
    {
        _logger.Warn(exception);
        File.Copy(path, ArchivePath, true);
    }
}

The stack trace will contain enough information to determine if the exception was thrown moving, copy, or deleting the file; there's not enough information in your post to recommend whether exceptions should be handled, rethrown or simply bubbled up - I've made a few assumptions here.

I refactored the code as followed. I completely removed the need for both traversals.

        bool importedSuccessfully = Import(settings, batch, importPath);
        if(importedSuccessfully)
        {
            ImportedFileDispositionActions importedFileDispositionAction = batch.DocumentType.ImportedFileDisposition;

            if(importedFileDispositionAction == ImportedFileDispositionActions.None)
                return true;

            string[] documentMetadataPaths;
            if(!LoadDocumentMetadataPaths(importPath, out documentMetadataPaths))
                return false;

            foreach(string documentMetadataFile in documentMetadataPaths)
            {
                string documentPath = Path.GetDirectoryName(documentMetadataFile);
                XDocument metadata = XDocument.Load(documentMetadataFile);
                foreach(var pageElement in metadata.Root.Element("Pages").Elements("Page"))
                {
                    string pageName = null;
                    XAttribute attribute = pageElement.Attribute("FileName");
                    if(attribute != null)
                        pageName = attribute.Value;

                    string pagePath = null;
                    string originalPagePath = null;
                    if(!string.IsNullOrWhiteSpace(pageName))
                    {
                        pagePath = IOUtility.PathCombine(documentPath, pageName);
                        if(importedFileDispositionAction == ImportedFileDispositionActions.Archive)
                        {
                            SendToArchive(pagePath, documentMetadataFile, batch);
                        }
                        if(File.Exists(pagePath))
                        {
                            File.Delete(pagePath);
                        }
                    }
                }
                File.Delete(documentMetadataFile);
                Directory.Delete(documentPath, true);
            }
        }
        return importedSuccessfully;
    }
    private void SendToArchive(string path,string metadata, Batch batch)
    {
        try
        {
            string pageName = Path.GetFileName(path);
            string documentPath = Path.GetDirectoryName(metadata);
            var archivePath = GenerateArchiveDirectory(batch, documentPath);
            string archivedFullPathFileName = Path.Combine(archivePath, pageName);
            if(!Directory.Exists(archivePath))
            {
                Directory.CreateDirectory(archivePath);
            }
            MoveOrCopyToArchive(path, archivedFullPathFileName);
        }
        catch(IOException exception)
        {
            App.CurrentApplication.Log.Log(LogLevel.Error, exception);
        }
        catch(Exception exception)
        {
            App.CurrentApplication.Log.Log(LogLevel.Error, exception);
        }
    }

    private void MoveOrCopyToArchive(string source,string target)
    {
        try
        {
            File.Move(source, target);
        }
        catch(UnauthorizedAccessException exception)
        {
            App.CurrentApplication.Log.Log(LogLevel.Warning, exception);
            File.Copy(source, target, true);
        }
    }


Python Prime Testing This code seems surprisingly fast at checking if a number is prime.

def is_prime(number: int) -> bool:
    """Checks if a number is prime or not"""
    # number must be integer
    if type(number) != int:
        raise TypeError("Non-integers cannot be tested for primality.")
    # negatives aren't prime, 0 and 1 aren't prime either
    if number <= 1:
        return False
    # 2 and 3 are prime
    elif number <= 3:
        return True
    # multiples of 2 and 3 aren't prime
    elif number % 2 == 0 or number % 3 == 0:
        return False
    # only need to check if divisible by potential prime numbers
    # all prime numbers are of form 6k +/- 1
    # only need to check up to square root of number
    for i in range(5, int(number**0.5) + 1, 6):
        if number % i == 0 or number % (i + 2) == 0:
            return False

    return True

However, is there any way to speed this prime testing up?

Congratulations, you just came up with a prime sieve! And indeed, it's faster than always considering all the multiples of 2 of 3. But why do you stop at multiples of 2 and 3? What about 5? And 7? But why stop somewhere?

Let's be more ambitious. What about skipping all the multiples of all the primes encountered so far? This is called the sieve of Eratosthenes. It uses more memory (if you double the maximum number you're interested in, you need twice the memory), but it will be faster and will tell you if any number below your maximum number is prime.

If you want more improvements, note that other prime sieves exist: both the sieve of Sundaram and the sieve of Atkin improve over the sieve of Eratosthenes. And of course, if you're interested in raw speed, use numpy or even a language like C.

When testing your solution compared to a regex, the result looks like the regex is slightly faster in this test.

import re
import timeit
start_time = timeit.default_timer()
re.match(r'^1?$|^(11+?)\1+$', '1'*12739)
elapsed = timeit.default_timer() - start_time

start_time = timeit.default_timer()
re.match(r'^1?$|^(11+?)\1+$', '1'*14)
elapsed = timeit.default_timer() - start_time

print(elapsed)

def is_prime(number):
    """Checks if a number is prime or not"""
    # number must be integer
    if type(number) != int:
        raise TypeError("Non-integers cannot be tested for primality.")
    # negatives aren't prime
    if number <= 1:
        return False
    # 2 and 3 are prime
    elif number <= 3:
        return True
    # multiples of 2 and 3 aren't prime
    elif number % 2 == 0 or number % 3 == 0:
        return False
    # only need to check if divisible by potential prime numbers
    # all prime numbers are of form 6k +/- 1
    # only need to check up to square root of number
    for i in range(5, int(number**0.5) + 1, 6):
        if number % i == 0 or number % (i + 2) == 0:
            return False

    return True

start_time = timeit.default_timer()
is_prime(12739)
elapsed = timeit.default_timer() - start_time

print(elapsed)

You can try the above code online.


How can I make my program that calculates π faster? For my first Raku program, I thought it might be a fun challange to port over a program I'd written in C# about two years ago, and I was right: it was.
There's just one problem: while the C# program runs in 5 seconds, the Raku program takes nearly 5 minutes. I think the Raku program is doing more or less the same as the C# code, and so it should be equally fast (or equally slow, depending on how you look at it).
Here's my code:
use v6;

class Point {
    has Num $.x;
    has Num $.y;

    method is-inside-unit-circle(--> Bool) {
        $!x²+$!y² <= 1;
    }
}

sub random-points(Int $count --> Seq) {
    gather for 0..^$count {
        take Point.new(x => 1.rand.Num, y => 1.rand.Num);
    }
}

sub compute-pi(Int $batch --> Seq) {
    my Num $total = 0e0;
    my Num $count = 0e0;

    gather for 0..^∞ {
        my Seq $pointsInside = random-points($batch).grep: *.is-inside-unit-circle;

        $total += $batch;
        $count += $pointsInside.elems;
        my Num $ratio = $count / $total;

        take $ratio * 4;
    }
}

{ say "π ≈ $_" } for compute-pi(100_000).head(500);

And here's part of the output:
% time raku estimation.raku
π ≈ 3.12912
π ≈ 3.12874
π ≈ 3.1326533333333333
…
π ≈ 3.1419357429718877
π ≈ 3.1419323446893785
π ≈ 3.14193696
raku estimation.raku  272,98s user 1,78s system 100% cpu 4:34,76 total

My two questions are:

How can I make it faster?
Is my code idiomatic, and how could I make it more idiomatic?

For completeness' sake, here's the C# code:
using System;
using System.Collections.Generic;
using System.Linq;

internal class Program
{
    internal readonly struct Point
    {
        private readonly double _x;
        private readonly double _y;

        internal Point(double x, double y)
        {
            _x = x;
            _y = y;
        }

        internal bool IsInsideUnitCircle()
        {
            return Math.Pow(_x, 2) + Math.Pow(_y, 2) <= 1;
        }
    }

    internal class PointGenerator
    {
        private readonly Random _random;

        public PointGenerator() => _random = new Random();

        public IEnumerable<Point> GeneratePoints(int count)
        {
            for (var i = 0; i < count; i++)
            {
                yield return new Point(_random.NextDouble(), _random.NextDouble());
            }
        }
    }

    private static IEnumerable<double> ComputePi(int batch)
    {
        var pointGenerator = new PointGenerator();

        var total = 0.0;
        var count = 0.0;

        while (true)
        {
            var pointsInside = pointGenerator.GeneratePoints(batch)
                .Where(point => point.IsInsideUnitCircle());

            total += batch;
            count += pointsInside.Count();
            var ratio = count / total;

            yield return ratio * 4;
        }
    }

    private static void Main()
    {
        foreach (var estimate in ComputePi(100_000).Take(500))
        {
            Console.WriteLine($"π ≈ {estimate}");
        }
    }
}

And part of its output:
% time dotnet run -c Release
π ≈ 3,13948
π ≈ 3,13904
π ≈ 3,138786666666667
…
π ≈ 3,1415373493975904
π ≈ 3,14154501002004
π ≈ 3,14155416
dotnet run -c Release  5,43s user 0,30s system 103% cpu 5,555 total

You random-points sub has a few things which can be removed or simplified.

1.rand returns a Num, so there is no reason to call the method .Num on its result.
Rather than gather for take you can use the xx operator.
You only want a positive number for the count, so declare that with UInt

sub random-points(UInt $count --> Seq) {
    Point.new(x => 1.rand, y => 1.rand) xx $count
}

xx seems to be on the order of 10× faster than gather for take.
The reason is mostly that take works by throwing a CONTROL and gather captures that message.
On my computer that change alone takes it from nearly 9 minutes (8m49.881s) to around 2 and a half minutes (2m27.582s). (The other changes have no meaningful effect on the run time.)
The sweet spot for the gather take feature is where there isn't already a built-in for what you are doing, and the speed doesn't matter as much as the ease of writing it.
You could go to the work of creating your own Iterator class which could speed it up. That is why xx is so much faster, there is already an Iterator class that was created specifically for it.
(Note that I did actually test to see if writing a new Iterator class would help. It didn't help, in fact it was slower.)

There are similar issues in compute-pi.
To start off, there are improvements to be made to gather for 0..^∞.

Don't mix Int and Num endpoints for Range objects if you can help it. 0e0..∞
Don't use for for infinite loops, just use loop

gather loop {

    ...

    take $ratio * 4;
}

I would not store the filtered random-points Seq in a variable, because the only thing you need from it is the count.
my Int $points-inside = +random-points($batch).grep: *.is-inside-unit-circle;

The prefix + operator just coerces to a Numeric value. Obviously the only number that makes sense for a Seq is the same as .elems, the count of elements.
.elems could potentially return a Num, specifically Inf. Considering that would break the rest of the code anyway, I think it's fine to store it in an Int scalar.

Ok now that we are done with the more obvious cleanups we can try to make it faster.
Thankfully changing from gather for take in random-points to xx helped a lot. (No other change had any meaningful change on the amount of time taken.)
The first thing I did at this point was to run the profiler
raku --profile test.raku

There were no obvious slow parts of our code. In fact a good ⅔ of the run time was in the Raku runtime, not our code.
So let's add parallelism to see if we can reduce the time by using more CPU cores.
We could just make the sequence returned from random-points a race sequence to see if it helps.
my Int $pointsInside = +random-points($batch).race.grep(*.is-inside-unit-circle);

After that change it now takes 4 minutes, almost twice the time it took from before that change.
I did mess around with the batch and degree arguments to race, but no combination seemed to improve it meaningfully. At best I made it match the original timing.
So now we know that was a dead-end. random-points.grep was already about as fast as possible.

So let's add the asynchrony to compute-pi instead.
One obvious thing we could do is have a bunch of threads creating the sequences of random-points.grep.
Of course we don't know how many we need to create. So let's give that as an argument.
sub compute-pi(UInt $batch-size, UInt $batch-count --> Seq) {

Now we want to have an existing sequence that we can call race on.
my \batches = ^$batch-count;

Add race to it.
(We don't care about the order of random-points.grep values we get.)
my \batches = (^$batch-count).race(batch => 1);

The reason for the batch argument of 1 is that we know each random-points.grep takes a long time, so we don't want it to batch them together. We also don't want to wait for 64 of them to be calculated before we get any results.
Now to turn that into the sequence of values that we want by using map.
my \batches = (^$batch-count).race(batch => 1).map: {
    +random-points($batch-size).grep(*.is-inside-unit-circle)
}

Rather than have an infinite loop, we will loop over those batches
gather for batches -> Int $pointsInside {

So the last part of the file now looks like this:
sub compute-pi(UInt $batch-size, UInt $batch-count --> Seq) {

    my \batches = (^$batch-count).race(batch => 1).map: {
        +random-points($batch-size).grep(*.is-inside-unit-circle)
    }

    my Num $total = 0e0;
    my Num $count = 0e0;

    gather for batches -> Int $pointsInside {

        $total += $batch;
        $count += $pointsInside;
        my Num $ratio = $count / $total;

        take $ratio * 4;
    }
}

{ say "π ≈ $_" } for compute-pi(100_000, 500);

It now takes just over a minute (1m13.905s) with a cpu time of over 5 minutes (5m19.237s).
Which means we have more than 400% utilization. Which is fairly good on a 4 core CPU, with hyperthreading.

We can gain a bit more if we use native num variables instead of boxed Num variables.
You have to be careful with this as you can actually make it take more time if the value ends up needing to be boxed for an operation.
class Point {
    has num $.x;
    has num $.y;
    ...
}

...

sub compute-pi(UInt $batch-size, UInt $batch-count --> Seq) {

    my \batches = (^$batch-count).race(batch => 1).map: {
        random-points($batch-size).grep(*.is-inside-unit-circle).Num
    }

    my num $total = 0e0;
    my num $count = 0e0;

    gather for batches -> num $pointsInside {

        $total += $batch;
        $count += $pointsInside;
        my num $ratio = $count / $total;

        take $ratio * 4;
    }
}

That final change brings it down to about 42 seconds (0m42.771s) with 4 minutes of CPU time (4m0.929s).
Which also seems to increase the CPU utilization up over 500%.

The biggest improvement was going from gather for take to just xx. Which I'd argue was warranted just for how much it improved the clarity of the code. The other changes were of minimal benefit.
I think to get any significant improvement would require improving the compiler, runtime, or the VM.
Really what we ended up with is fairly fast considering how much of higher level language Raku is than C#, and how few people are working on it.

You can speed it up as follows
use v6;

class Point {
    has Num $.x;
    has Num $.y;

    method is-inside-unit-circle(--> Bool) {
        $.x²+$.y² <= 1;
    }
}

sub random-points(Int $count --> RaceSeq) {
    [^$count].race.map({ Point.new(x => 1.rand, y => 1.rand) });
}

sub compute-pi(Int $batch --> Seq) {
    my Num $total = 0e0;
    my Num $count = 0e0;

    gather while True {
        my RaceSeq $pointsInside = random-points($batch).race.grep: *.is-inside-unit-circle;

        $total += $batch;
        $count += $pointsInside.elems;
        my Num $ratio = $count / $total;

        take $ratio * 4;
    }
}

say "π ≈ $_" for compute-pi(100_000).head(500);

Output:
real    2m53,695s
user    9m16,476s
sys     0m6,780s


Fizzbuzz in COBOL, Verbose dinosaur style I was long a professional in COBOL, but noone ever dared comment my style. As the FizzBuzz seems fashion here, I made one. Verbose, of course, we're speaking about COBOL. I know I could have made in 15 lines, but decided instead to provide the full structure for a production program. Result makes 87 lines.

I'll be happy to read your thoughts, comments, critics, or executions. Made on OpenCOBOLIDE.

  ******************************************************************
  * Author:Gazzz0X2Z                                              *
  * Date:2015, Dec the 12th                                      *
  * Purpose:FizzBuzz, what else?                                 *
  * Tectonics: cobc                                               *
  ******************************************************************
   IDENTIFICATION DIVISION.
  *-*-*-*-*-*-*-*-*-*-*-*-*-
   PROGRAM-ID. FIZZBUZZ.
  *-*-*-*-*-*-*-*-*-*-*--
   ENVIRONMENT DIVISION.
  *-*-*-*-*-*-*-*-*-*-*--
   CONFIGURATION SECTION.
  *----------------------
   INPUT-OUTPUT SECTION.
  *----------------------
   DATA DIVISION.
  *-*-*-*-*-*-*-*-
   FILE SECTION.
  *-------------------------
   WORKING-STORAGE SECTION.
  *-------------------------
   01  WS-DATA.
       05  WS-COUNTS.
           10  WS-FIRST        PIC 9(4)    VALUE ZERO.
           10  WS-LAST         PIC 9(4)    VALUE 20.
           10  WS-POSN         PIC 9(4).
       05  WS-RESULTS.
           10  WS-RS00         PIC 9(4).
           10  WS-RM05         PIC 9(4).
               88  WS-ML05                 VALUE ZERO.
           10  WS-RM03         PIC 9(4).
               88  WS-ML03                 VALUE ZERO.
       05  WS-DISPLAY-STRING   PIC X(10).
       05  WS-DISPLAY-NUM REDEFINES WS-DISPLAY-STRING.
           10  WS-FIZZBUZZ-PRI PIC X(3).
           10  WS-FIZZBUZZ-INT PIC Z(3)9.
           10  WS-FIZZBUZZ-SUI PIC X(3).
       05  WS-DISPLAY-ALPHA REDEFINES WS-DISPLAY-STRING.
           10  WS-FIZZBUZZ-STR PIC X(10).
               88  WS-IS-OTHER             VALUE "*  0000  *".
               88  WS-IS-FIZZ              VALUE "*  FIZZ  *".
               88  WS-IS-BUZZ              VALUE "*  BUZZ  *".
               88  WS-IS-FIZZBUZZ          VALUE "*FIZZBUZZ*".

  *-*-*-*-*-*-*-*-*-*-*
   PROCEDURE DIVISION.
  *-*-*-*-*-*-*-*-*-*-*
   MAIN-PROCEDURE.
        PERFORM    10000-BEGIN
        PERFORM    20000-MAIN
           VARYING WS-POSN FROM WS-FIRST BY 1
             UNTIL WS-POSN > WS-LAST
        PERFORM    30000-END
        .
        STOP RUN.
  *
   10000-BEGIN.
       DISPLAY "**********"
       .
  *
   20000-MAIN.
       PERFORM 40000-COMPUTE-FIZZBUZZ
       DISPLAY WS-DISPLAY-STRING
       .
  *
   30000-END.
       DISPLAY "**********"
       .
  *
   40000-COMPUTE-FIZZBUZZ.
       DIVIDE WS-POSN BY 5 GIVING WS-RS00 REMAINDER WS-RM05
       DIVIDE WS-POSN BY 3 GIVING WS-RS00 REMAINDER WS-RM03
       EVALUATE TRUE
           WHEN (WS-ML05 AND WS-ML03)
               SET WS-IS-FIZZBUZZ  TO TRUE
           WHEN WS-ML05
               SET WS-IS-BUZZ      TO TRUE
           WHEN WS-ML03
               SET WS-IS-FIZZ      TO TRUE
           WHEN OTHER
               SET WS-IS-OTHER     TO TRUE
               MOVE WS-POSN        TO WS-FIZZBUZZ-INT
       END-EVALUATE
       .
  *
   END PROGRAM FIZZBUZZ.

Result :

**********
*FIZZBUZZ*
*     1  *
*     2  *
*  FIZZ  *
*     4  *
*  BUZZ  *
*  FIZZ  *
*     7  *
*     8  *
*  FIZZ  *
*  BUZZ  *
*    11  *
*  FIZZ  *
*    13  *
*    14  *
*FIZZBUZZ*
*    16  *
*    17  *
*  FIZZ  *
*    19  *
*  BUZZ  *
**********

As I'm sure you are aware, COBOL tends to be written one way at one site, another way at another.

Often things like name prefixes, and paragraph/SECTION prefixes cannot be other than dictated. Same with case of identifiers.

I dislike WSnn- (drives people nuts trying to insert numbers so that new data can be located rationally rather than in the order of arrival in specification) and WS- (similar for the LINKAGE SECTION) but am happy with W-/L-/LS- (or nothing). Absolutely hate single-word identifiers. Even these days with forewarning of what may appear later, it is plain dumb to have them there waiting for some compiler-change to make them invalid.

Also, since COBOL has no concept of "main", it is something to avoid pretending exists.

For paragraphs, or SECTIONS, I go with two numerics. Then with each subsequent level, append an alpha, A, D, G, J, M etc (allowing for insertions), and keep all related paragraphs/SECTIONS together, physically. Your program only has one level, so it doesn't really matter what you do in that one.

Nicely laid-out. I tend to be more extreme, but the effort you've invested impresses the reader (if they are a programmer, rather than a 9-5-er).

Hate the comments.

I'd remove all the redundant code (you don't need an ENVIRONMENT DIVISION, for instance).

Ah, for these last two, they have been generated for you, so I'd suggest copying an existing model program rather than using the OpenCOBOLIDE bolierplate.

I only use VARYING with things that, well, vary. So I'd use TIMES. Describe the situation better. You have to do more typing, but in addition to the better description of the data, your code will be faster than using VARYING.

Forget ideas like using REM. COBOL has done remainders since day one, a considerable time ago, so why (I don't know the answer to this) is there a function for it? In Enterprise COBOL (IBM Mainframe) REM would return a floating-point item, then converted to what you define in your program. Wow, do we need that?

I would work on the data-names a bit, but they are better than VAR1, VAR2 (or WS13-VAR1 and WS34-VAR2).

Here's an example of how I like to format a program:

http://ibmmainframes.com/viewtopic.php?p=278927#278927

And since that is a link, here's the code:

   IDENTIFICATION DIVISION.
   PROGRAM-ID. BBHEXD.
   ENVIRONMENT DIVISION.
   DATA DIVISION.
   WORKING-STORAGE SECTION.
   01  W-WHEN-COMPILED                      PIC X(8)BX(8).

   01  W-DISPLAY-HEX-MAX-UIND      USAGE IS INDEX.
   01  W-FIRST-TIME-FLAG                    PIC X VALUE "Y".
       88  W-FIRST-TIME-IN-PROGRAM          VALUE "Y".
       88  W-NOT-FIRST-TIME-FLAG            VALUE "N".

   01  W-WORK-BYTE-FOR-CALC         COMP    PIC S9(4) VALUE ZERO.
   01  FILLER REDEFINES W-WORK-BYTE-FOR-CALC.
       05  FILLER                           PIC X.
       05  W-WORK-BYTE                      PIC X.

   01  W-LEFT-HALF-BYTE-NUM         COMP    PIC S9(4).
   01  W-RIGHT-HALF-BYTE-NUM        COMP    PIC S9(4).
   01  W-BYTES-TO-OUTPUT            COMP    PIC S9(4).
   01  W-BYTES-MOVED                COMP    PIC S9(4).

   01  W-HEX-ALPHA-SUB              COMP    PIC S9(4).
   01  W-HEX-ALPHA-TABLE VALUE "ABCDEF".
       05  FILLER OCCURS 6 TIMES
            INDEXED BY W-HEX-ALPHA-IDX.
           10  W-HEX-ALPHA                  PIC X.

   01  W-DISPLAY-HEX.
       05  W-LEFT-HEX                       PIC X.
       05  W-RIGHT-HEX                      PIC X.

   01  W-HEX-TO-DISPLAY-LEFT-TABLE VALUE SPACE.
       05  FILLER OCCURS 1 TO 132 TIMES
           DEPENDING ON W-BYTES-TO-OUTPUT
           INDEXED BY W-DISPLAY-HEX-LEFT-IND.
           10  W-HEX-TO-DISPLAY-LEFT        PIC X.
               88  W-HEX-TO-DISPLAY-LEFT-IS-EMPTY
                                            VALUE SPACE.
   01  W-HEX-TO-DISPLAY-RIGHT-TABLE VALUE SPACE.
       05  FILLER OCCURS 1 TO 132 TIMES
           DEPENDING ON W-BYTES-TO-OUTPUT
           INDEXED BY W-DISPLAY-HEX-RIGHT-IND.
           10  W-HEX-TO-DISPLAY-RIGHT       PIC X.

   01  W-BYTE-TO-DISPLAY-TABLE VALUE SPACE.
       05  FILLER OCCURS 1 TO 132 TIMES
           DEPENDING ON W-BYTES-TO-OUTPUT
           INDEXED BY W-DISPLAY-BYTE-IND.
           10  W-BYTE-TO-DISPLAY            PIC X.
   01  W-CONVERT-TO-HEX-FROM-DIGIT   COMP   PIC S9(4).

   01  W-CONVERTED-HEX-DIGIT                PIC X.
   01  W-CONVERTED-HEX-DIGIT-NUM
         REDEFINES W-CONVERTED-HEX-DIGIT    PIC 9.

   01  W-CONVERTED-HEX-DIGIT-NUM4           PIC 9(4).
   01  FILLER REDEFINES W-CONVERTED-HEX-DIGIT-NUM4.
       05  FILLER                           PIC XXX.
       05  W-CONVERTED-HEX-DIGIT-NUM4L      PIC 9.

   01  W-COLS-LINE.
       05  FILLER OCCURS 1 TO 132 TIMES
           DEPENDING ON W-BYTES-TO-OUTPUT.
           10  FILLER                       PIC X.

   01  W-COLS-TABLE.
       05  FILLER                           PIC X(10) VALUE
           "----+----1".
       05  FILLER                           PIC X(10) VALUE
           "----+----2".
       05  FILLER                           PIC X(10) VALUE
           "----+----3".
       05  FILLER                           PIC X(10) VALUE
           "----+----4".
       05  FILLER                           PIC X(10) VALUE
           "----+----5".
       05  FILLER                           PIC X(10) VALUE
           "----+----6".
       05  FILLER                           PIC X(10) VALUE
           "----+----7".
       05  FILLER                           PIC X(10) VALUE
           "----+----8".
       05  FILLER                           PIC X(10) VALUE
           "----+----9".
       05  FILLER                           PIC X(10) VALUE
           "----+----0".
       05  FILLER                           PIC X(10) VALUE
           "----+----1".
       05  FILLER                           PIC X(10) VALUE
           "----+----2".
       05  FILLER                           PIC X(10) VALUE
           "----+----3".
       05  FILLER                           PIC X(02) VALUE
           "--".

   LINKAGE SECTION.

   01  L-DATA-LENGTH COMP PIC S9(8).

   01  L-BBHEXD-CONTROL-BLOCK.
       05  L-BBHEXD-CB-MAX-LINE         COMP PIC S9(4) VALUE +80.
           88  L-BBHEXD-CB-MAX-LINE-VALID    VALUE +80 +132.
       05  L-BBHEXD-CB-CHUNK-LENGTH     COMP PIC S9(4) VALUE ZERO.
           88  L-BBHEXD-CB-CHUNK-LENGTH-ZERO VALUE ZERO.
       05  FILLER                            PIC X(96) VALUE SPACE.

   01  L-DATA-PICX1-FOR-USING PIC X.

   01  L-DATA-TO-DISPLAY REDEFINES L-DATA-PICX1-FOR-USING.
       05  FILLER OCCURS 99999999 TIMES
             INDEXED BY L-DTD-BYTE-FOR-C-AND-C-IDX.
           10  L-DTD-BYTE-FOR-CONV-AND-COPY PIC X.

   PROCEDURE DIVISION USING
                                    L-DATA-PICX1-FOR-USING
                                    L-DATA-LENGTH
                                    L-BBHEXD-CONTROL-BLOCK
                                    .
   00-CONTROL SECTION.

       PERFORM                      10-FIRST-TIME
       PERFORM                      30-CNV-LK-DATA-TO-HEX-AND-DISP
       GOBACK
       .
   10-FIRST-TIME SECTION.

       IF W-FIRST-TIME-IN-PROGRAM
           PERFORM                  10A-SAY-WHO-WE-ARE
           SET W-NOT-FIRST-TIME-FLAG
                                    TO TRUE
       END-IF

       PERFORM                      10D-CHECK-OUTPUT-LENGTH
       PERFORM                      10G-SET-OUTPUT-LNGTH-AND-CHUNK
       .
   10A-SAY-WHO-WE-ARE SECTION.

       MOVE WHEN-COMPILED           TO W-WHEN-COMPILED
       DISPLAY "BBHEXD This program compiled on "
                                    W-WHEN-COMPILED
       .
   10D-CHECK-OUTPUT-LENGTH SECTION.

       IF ( NOT L-BBHEXD-CB-MAX-LINE-VALID )
           DISPLAY "BBHEXD maximum line length should be 80 or 132"
           DISPLAY "BBHEXD found>" L-BBHEXD-CB-MAX-LINE "<"
           CALL "FOODUMP"
       END-IF
       .
   10G-SET-OUTPUT-LNGTH-AND-CHUNK SECTION.

       IF L-BBHEXD-CB-CHUNK-LENGTH-ZERO
           MOVE L-BBHEXD-CB-MAX-LINE
                                    TO W-BYTES-TO-OUTPUT
       ELSE
           IF ( L-BBHEXD-CB-CHUNK-LENGTH
               GREATER THAN L-BBHEXD-CB-MAX-LINE )
               DISPLAY "BBHEXD Sorry, chunks ignored, too big"
               DISPLAY "BBHEXD Chunks>" L-BBHEXD-CB-CHUNK-LENGTH "<"
               DISPLAY "BBHEXD Maxlen>" L-BBHEXD-CB-MAX-LINE "<"
               MOVE L-BBHEXD-CB-MAX-LINE
                                    TO W-BYTES-TO-OUTPUT
           ELSE
               MOVE L-BBHEXD-CB-CHUNK-LENGTH
                                    TO W-BYTES-TO-OUTPUT
           END-IF
       END-IF

       SET W-DISPLAY-BYTE-IND       TO W-BYTES-TO-OUTPUT
       SET W-DISPLAY-HEX-MAX-UIND   TO W-DISPLAY-BYTE-IND

       IF L-DATA-LENGTH EQUAL TO ZERO
           DISPLAY "BBHEXD You want to display zero bytes?"
           DISPLAY "BBHEXD Do not expect to see them"
       END-IF
       .
   30-CNV-LK-DATA-TO-HEX-AND-DISP SECTION.

       SET W-DISPLAY-HEX-LEFT-IND
           W-DISPLAY-HEX-RIGHT-IND
           W-DISPLAY-BYTE-IND       TO +1
       MOVE ZERO                    TO W-BYTES-MOVED

       PERFORM                      30A-BYTE-BY-BYTE-CONVERT
         VARYING                    L-DTD-BYTE-FOR-C-AND-C-IDX
         FROM                       1
         BY                         1
         UNTIL                      L-DTD-BYTE-FOR-C-AND-C-IDX
           GREATER THAN             L-DATA-LENGTH

       IF ( W-BYTES-MOVED NOT EQUAL TO ZERO )
           MOVE W-BYTES-MOVED       TO W-BYTES-TO-OUTPUT
           PERFORM                  99A-DISPLAY-PART-OF-OUTPUT
       END-IF

       .
   30A-BYTE-BY-BYTE-CONVERT SECTION.

       MOVE L-DTD-BYTE-FOR-CONV-AND-COPY
             ( L-DTD-BYTE-FOR-C-AND-C-IDX )
                                    TO W-WORK-BYTE

       DIVIDE W-WORK-BYTE-FOR-CALC  BY 16
         GIVING                     W-LEFT-HALF-BYTE-NUM
         REMAINDER                  W-RIGHT-HALF-BYTE-NUM

       MOVE W-LEFT-HALF-BYTE-NUM    TO W-CONVERT-TO-HEX-FROM-DIGIT
       PERFORM                      30AA-CONVERT-HALF-BYTE
       MOVE W-CONVERTED-HEX-DIGIT   TO W-LEFT-HEX
       MOVE W-RIGHT-HALF-BYTE-NUM   TO W-CONVERT-TO-HEX-FROM-DIGIT
       PERFORM                      30AA-CONVERT-HALF-BYTE
       MOVE W-CONVERTED-HEX-DIGIT   TO W-RIGHT-HEX

       IF ( W-DISPLAY-HEX-LEFT-IND
           GREATER THAN W-DISPLAY-HEX-MAX-UIND )
           PERFORM                  99A-DISPLAY-PART-OF-OUTPUT
           SET W-DISPLAY-HEX-LEFT-IND
               W-DISPLAY-HEX-RIGHT-IND
               W-DISPLAY-BYTE-IND
                                    TO +1
       END-IF

       MOVE W-LEFT-HEX              TO W-HEX-TO-DISPLAY-LEFT
                                        ( W-DISPLAY-HEX-LEFT-IND )
       MOVE W-RIGHT-HEX             TO W-HEX-TO-DISPLAY-RIGHT
                                        ( W-DISPLAY-HEX-RIGHT-IND )
       MOVE W-WORK-BYTE             TO W-BYTE-TO-DISPLAY
                                        ( W-DISPLAY-BYTE-IND )
       ADD +1                       TO W-BYTES-MOVED
       SET W-DISPLAY-HEX-LEFT-IND
           W-DISPLAY-HEX-RIGHT-IND
           W-DISPLAY-BYTE-IND       UP BY +1
       .
   30AA-CONVERT-HALF-BYTE SECTION.

       IF ( W-CONVERT-TO-HEX-FROM-DIGIT GREATER THAN 9 )
           SUBTRACT 9               FROM W-CONVERT-TO-HEX-FROM-DIGIT
             GIVING                 W-HEX-ALPHA-SUB
           SET W-HEX-ALPHA-IDX      TO W-HEX-ALPHA-SUB
           MOVE W-HEX-ALPHA ( W-HEX-ALPHA-IDX )
                                    TO W-CONVERTED-HEX-DIGIT
       ELSE
           MOVE W-CONVERT-TO-HEX-FROM-DIGIT
                                    TO W-CONVERTED-HEX-DIGIT-NUM4
           MOVE W-CONVERTED-HEX-DIGIT-NUM4L
                                    TO W-CONVERTED-HEX-DIGIT-NUM
       END-IF

       .
   99A-DISPLAY-PART-OF-OUTPUT SECTION.

       MOVE W-COLS-TABLE            TO W-COLS-LINE
       DISPLAY                      W-COLS-LINE

       DISPLAY                      W-BYTE-TO-DISPLAY-TABLE
       DISPLAY                      W-HEX-TO-DISPLAY-LEFT-TABLE
       DISPLAY                      W-HEX-TO-DISPLAY-RIGHT-TABLE
       MOVE SPACE                   TO W-BYTE-TO-DISPLAY-TABLE
                                       W-HEX-TO-DISPLAY-LEFT-TABLE
                                       W-HEX-TO-DISPLAY-RIGHT-TABLE
       MOVE ZERO                    TO W-BYTES-MOVED

       .

To demonstrate the usefulness of meaningful names, here's a version with the names rubbished:

   IDENTIFICATION DIVISION.
   PROGRAM-ID. UGLY.
   ENVIRONMENT DIVISION.
   DATA DIVISION.
   WORKING-STORAGE SECTION.
   01  W-A                      PIC X(8)BX(8).

   01  W-B            USAGE IS INDEX.
   01  W-C          USAGE IS INDEX.

   01  W-D              PIC S9(9).
   01  FILLER REDEFINES W-D.
       05  FILLER                           PIC X(5).
       05  W-E         PIC 9(4).
   01  W-F PIC X VALUE "Y".
       88  W-F-Y VALUE "Y".
       88  W-F-N VALUE "N".

   01  W-G         COMP    PIC S9(4) VALUE ZERO.
   01  FILLER REDEFINES W-G.
       05  FILLER                           PIC X.
       05  W-H                      PIC X.

   01  W-I         COMP    PIC S9(4).
   01  W-J        COMP    PIC S9(4).
   01  W-K            COMP    PIC S9(4).

   01  W-L              COMP    PIC S9(4).
   01  W-M VALUE "ABCDEF".
       05  FILLER OCCURS 6 TIMES
            INDEXED BY I.
           10  W-N                  PIC X.

   01  W-O.
       05  W-P                       PIC X.
       05  W-Q                      PIC X.

   01  W-R.
       05  FILLER OCCURS 1 TO 132 TIMES
           DEPENDING ON W-K
           INDEXED BY J.
           10  W-S        PIC X.
               88  W-S-Y
                                            VALUE SPACE.
   01  W-T.
       05  FILLER OCCURS 1 TO 132 TIMES
           DEPENDING ON W-K
           INDEXED BY K.
           10  W-U       PIC X.

   01  W-V.
       05  FILLER OCCURS 1 TO 132 TIMES
           DEPENDING ON W-K
           INDEXED BY L.
           10  W-W            PIC X.
   01  W-X   COMP   PIC S9(4).

   01  W-Y                PIC X.
   01  W-Z
         REDEFINES W-Y    PIC 9.

   01  W-AA           PIC 9(4).
   01  FILLER REDEFINES W-AA.
       05  FILLER                           PIC XXX.
       05  W-AB      PIC 9.

   01  W-AC.
       05  FILLER OCCURS 1 TO 132 TIMES
           DEPENDING ON W-K.
           10  FILLER                       PIC X.

   01  W-AD.
       05  FILLER                           PIC X(10) VALUE
           "----+----1".
       05  FILLER                           PIC X(10) VALUE
           "----+----2".
       05  FILLER                           PIC X(10) VALUE
           "----+----3".
       05  FILLER                           PIC X(10) VALUE
           "----+----4".
       05  FILLER                           PIC X(10) VALUE
           "----+----5".
       05  FILLER                           PIC X(10) VALUE
           "----+----6".
       05  FILLER                           PIC X(10) VALUE
           "----+----7".
       05  FILLER                           PIC X(10) VALUE
           "----+----8".
       05  FILLER                           PIC X(10) VALUE
           "----+----9".
       05  FILLER                           PIC X(10) VALUE
           "----+----0".
       05  FILLER                           PIC X(10) VALUE
           "----+----1".
       05  FILLER                           PIC X(10) VALUE
           "----+----2".
       05  FILLER                           PIC X(10) VALUE
           "----+----3".
       05  FILLER                           PIC X(02) VALUE
           "--".

   LINKAGE SECTION.

   01  L-A PIC X.

   01  L-B REDEFINES L-A.
       05  FILLER OCCURS 99999999 TIMES
             INDEXED BY M.
           10  L-C PIC X.

   01  L-D COMP PIC S9(8).

   01  L-E.
       05  L-F         COMP PIC S9(4) VALUE +80.
           88  L-F-VALID    VALUE +80 +132.
       05  L-G     COMP PIC S9(4) VALUE ZERO.
           88  L-G-Y VALUE ZERO.
       05  FILLER                            PIC X(96) VALUE SPACE.

   PROCEDURE DIVISION USING         L-A
                                    L-D
                                    L-E
                                    .
   00-A SECTION.
       IF W-F-Y
           MOVE WHEN-COMPILED       TO W-A
           DISPLAY                  "UGLY program compiled on "
                                    W-A
           SET W-F-N                TO TRUE
       END-IF

       IF ( NOT L-F-VALID )
           DISPLAY "UGLY maximum line length should be 80 or 132"
           DISPLAY "UGLY found>" L-F "<"
           CALL "FOODUMP"
       END-IF
       IF L-G-Y
           MOVE L-F                 TO W-K
       ELSE
           IF ( L-G
               GREATER THAN L-F )
               DISPLAY "UGLY Sorry, chunks ignored, too big"
               DISPLAY "UGLY Chunks>" L-G "<"
               DISPLAY "UGLY Maxlen>" L-F "<"
               MOVE L-F             TO W-K
           ELSE
               MOVE L-G             TO W-K
           END-IF
       END-IF
       SET L                        TO W-K
       SET W-B                      TO L
       SET J
           K
           L                       TO ZERO

       PERFORM                      00-B
         VARYING                    M
         FROM                       1
         BY                         1
         UNTIL                      M
           GREATER THAN             L-D

       IF ( NOT W-S-Y )
           SET W-C                  TO J
           MOVE W-C                 TO W-D
           MOVE W-E                 TO W-K
           PERFORM                  00-D
       END-IF

       GOBACK
       .
   00-B SECTION.

       MOVE L-C
             ( M )
                                    TO W-H

       DIVIDE W-G                   BY 16
         GIVING                     W-I
         REMAINDER                  W-J

       MOVE W-I                     TO W-X
       PERFORM                      00-C
       MOVE W-Y                     TO W-P
       MOVE W-J                     TO W-X
       PERFORM                      00-C
       MOVE W-Y                     TO W-Q

       SET J
           K
           L                        UP BY +1

       IF ( J GREATER THAN W-B )
           PERFORM                  00-D
           MOVE SPACE               TO W-R
                                       W-U
           SET J
               K
               L
                                    TO +1
       END-IF

       MOVE W-P                     TO W-S
                                        ( J )
       MOVE W-Q                     TO W-U
                                        ( K )
       MOVE W-H                     TO W-W
                                        ( L )
       .
   00-C SECTION.

       IF ( W-X GREATER THAN 9 )
           SUBTRACT 9               FROM W-X
             GIVING                 W-L
           SET I                    TO W-L
           MOVE W-N ( I )           TO W-Y
       ELSE
           MOVE W-X                 TO W-AA
           MOVE W-AB                TO W-Z
       END-IF

       .
   00-D SECTION.

       MOVE W-AD                    TO W-AC
       DISPLAY                      W-AC

       DISPLAY                      W-V
       DISPLAY                      W-R
       DISPLAY                      W-T
       .

Now, there are a couple of bugs in this version. Enjoy finding them, as you first have to work out, with no aid, what the program does.

Since you have GnuCOBOL, track down the discussion area at SourceForge.Net, the current home of GnuCOBOL.

I think this is better:

IDENTIFICATION DIVISION.
PROGRAM-ID. FIZZ-BUZZ.
DATA DIVISION.
   WORKING-STORAGE SECTION.
   01 WS-STR PIC X(30).
   01 WS-CNT PIC 9(03).
   01 WS-DEVIDENT PIC 9(03).
   01 WS-REMAINDER PIC 9(03).
PROCEDURE DIVISION.
    FIRST-PARA.
    PERFORM VARYING WS-CNT FROM 1 BY 1 UNTIL WS-CNT >= 20
        INITIALIZE WS-STR
        DIVIDE WS-CNT BY 3 GIVING WS-DEVIDENT REMAINDER WS-REMAINDER
        IF WS-REMAINDER = 0
            STRING
                WS-STR DELIMITED BY SPACE
                "FIZZ" DELIMITED BY SIZE
                INTO WS-STR
            END-STRING
        END-IF
        DIVIDE WS-CNT BY 5 GIVING WS-DEVIDENT REMAINDER WS-REMAINDER
        IF WS-REMAINDER = 0
            STRING
                WS-STR DELIMITED BY SPACE
                "BUZZ" DELIMITED BY SIZE
                INTO WS-STR
            END-STRING
        END-IF
        IF WS-STR = SPACES
            MOVE WS-CNT TO WS-STR
        END-IF
        DISPLAY WS-STR
    END-PERFORM
STOP RUN.


Extracting keywords from 3 billion CSV records I am processing around 3 billion records using this piece of code. It's pretty slow; it would be really helpful if you could suggest better ways to do this.

'''
Created on 27-Mar-2015
@author: siddarth
'''

import rake
import multiprocessing as mp
import time
import os
import csv

filename = 'Large_Input_File.csv'
outName = 'Large_Output_File.csv'
BYTES_PER_MB = 1048576

start = time.time()
def elapsed():
    return time.time() - start

''' Worker function used to call rake
    This method basically calls the rake class which returns a
    list of keywords, also each process
    writes to a different output file'''

def fileparser_worker(filename, start, end, c):
    outFileName = outName + str(c)
    outFile = open(outFileName,'w')
    with open(filename) as inFile:
        inFile.seek(start)
        lines = inFile.readlines(end-start)
        for line in lines:
            title = line.split('\t')
            try:
                keywords = rake.execute_rake(title[2])
            except:
                print('Index out of bound error due to NULL Values')
                continue
            for keyword in range(0,len(keywords)):
                outFile.write(keywords[keyword][0] + '\n')
    outFile.close()

if __name__ == '__main__':
    start = time.time()
    chunk_start = 0
    chunk_size =  512 * BYTES_PER_MB
    chunk_end = 512 * BYTES_PER_MB

    filesize = os.path.getsize(filename)

    print '\n%.3fs: file has %s rows' % (elapsed(), filesize)

    c = 0
    pause = 0
    iterations = (filesize / chunk_size) + 1
    ''' Chunk the file '''
    print(iterations)
    with open(filename) as inFile:
        while c < iterations:
            if chunk_start + chunk_size > filesize:
                chunk_end = filesize
            else:
                chunk_end = chunk_start + chunk_size

            inFile.seek(chunk_end)
            line = inFile.readline()

            if line == '':
                continue
            else:
                chunk_end = inFile.tell()

            print("Start chunk",chunk_start)
            ''' Initializing Processes '''
            proc = mp.Process(target=fileparser_worker, args = (filename,chunk_start,chunk_end, c) )
            proc.start()
            pause += 1
            print("End chunk",chunk_end)
            chunk_start = chunk_end
            c += 1

            ''' Making sure that there are atmost 4 processes
                running at a single point of time,
                cause I have a 4 core machine'''

            if pause == 4:
                proc.join() # Waits for processes to close
                pause = 0

    proc.join() # Close off any running processes
    print("TOTAL TIME TAKEN",elapsed())

Below are some metrics which may help you to understand what's going on under the hood.

File Size = 18GB

Usage Metrics:
CPU Usage = 99% ( Using 4 Processes )
RAM Usage = 30% ( May be I can use more RAM to improve performance ? )

I am not familiar with multiprocessing. I've learned about it today and wrote some sample code based on what I understood.

def fileparser_worker(filename, start, end, c):
    with open(filename) as inFile, open(outName + str(c),'w') as outFile:
            inFile.seek(start)
            #lines = inFile.readlines(end-start)

because readlines calls readline multiple times it can be replaced with
read.

If the text should be split by newline and each split line should be splitted by a tab character, there are two ways to do it depending on the input text.

    for title in (line.split('\t') for line inFile.read(end-start).splitlines()):

Here title is list of strings.

Another one is to call str.split() or str.split(None) in which case the string is split by white spaces (space, tab or newline), this is best if it's ok to split by space character or the text has no space character.

    for title in inFile.read(end-start).split():

Here the title is string.

If the read().split() becomes a memory hog, I think a generator will give
better result.

    #def get_title():
    #   yield next((line.split('\t') for line inFile.read(end-start).splitlines()))

    for title in (line.split('\t') for line inFile.read(end-start).splitlines()):
        try:
            keywords = rake.execute_rake(title[2])
        except:
            print('Index out of bound error due to NULL Values')
            continue

If we could build a sequence of strings out of keywords we can call
writelines straight.

        else:
            outFile.writelines((keyword[0] + '\n' for keyword in keywords))

~

Motherboards with multiple processors, processors with multiple cores, and the processor's microarchitecture will greatly affect the benefits of parallel processing, and multithreading.

Without knowing your motherboard and which processors are used, I am very limited as to what techniques will help or penalize your approach.

It appears you have not implemented any thread synchronization, communications or division of tasks for optimized allocation of resources.

There is not much chance you can take advantage of multithreading your app. Unless your workstation uses an Intel micro architecture such as Haswell where every core has its own L1 cache, the motherboard has dual memory access, and the PCIe bus is not shared with the memory bus.

In a nutshell, you have to split up the tasks. Running the same code on multiple threads is not going to help much. Your task list is bringing in data from the hard drive (SSD would help, or RAM disk), and store it in memory, search the data, store the results. Given your system architecture if any of these tasks can run simultaneously you can allocate those tasks to separate threads.

Basically you do not want multiple threads accessing the same cache memory. Set up your memory to best utilize the size of your on chip cache.  You would have to coordinate the resources between the threads with cooperative multitasking. Set up a queue between the processes that coordinates the threads to prevent accessing the same resources at the same time.

I doubt you can get a big boost from multitasking. Your best chance for performance gains is in optimizing your search.

Just because your CPU is at 99% does not mean each core is crunching your code. Most likely they are sitting in wait loops waiting for access to resources.

With 4 cores you should likely be running 3 or fewer threads. Your code be penalized for your uncooperative method.  You likely have a lot of cache eviction going on.

If you are going to use multiple threads, put in pauses to allow the other threads to get out of their wait loop. Keep loop iterations small.

You biggest problem is an 18GB flat file with no indexing.

You want to have the data stored in a way where as much of the processing is done in the data structure and organization.

Get rid of the CSV.  The only way to increase performance would be to break up the job to run on separate workstations.

Look into how search engines store the key words.  There are some open source projects that work fairly well.  Consider using Google's search services if you don't care they are going to analyze your data to find a way to capitalize on it. If you were to use 3rd party resources READ THE TERMS OF SERVICE! It is absolutely amazing how little people know what Google is doing.  More and more are learning from Google and doing the same appalling stuff.

A project like Heritrix, Solr, or even Sphider will do much better than an un-indexed CSV flat file.

Break the file into pieces and store them in pieces.  But only if you have a legitimate reason for keeping the CSV files. Why use your code to do that task when it can be done off line.

Your best bet is to index the data.

In PHP I use a text file technique that beats CSV.  I organize the data into huge arrays serialize them and save them as text files. Convert your CSV into arrays, serialize then, gzip them and store.

Size the arrays to fit in L1 or L2 cache. Pad them if necessary to organize then on memory page boundaries.

If you have not read Intel's 64 and IA-32 Architectures Optimization Reference Manual, read it.  All programmers should be familiar with it.

You don't have to read all 642 pages.  At a minimum read Chapter 3 and read the User/Source Coding Rules, search the document using this search phrase: "User/Source Coding Rule". Lots of tips for multithreading.

The most important: Source coding rules pertaining to branching (if else), loops (while), and variable declaration and organization.

My number one programming rule for fast code: Use arrays to eliminate branches in your code (see Intel's Optimization Manual).

This is assuming that retrieval of comments and generation of the HTML takes about 50-100 micro-seconds per comment is an improvement over Disqus.  The number of comments in the database has no affect on performance. The query is a single simple, query for all the records (including threading) associated with the topic.


Linked list loop detection in Java A schoolmate is going through the interview process, and was given the question of detecting if a linked list data structure has an infinite loop in it. I wrote this example, and wanted to see what experience programmers thought of it.

/*
* detecting loops in a linked list, java
*
* This linked list doesn't have a payload, but that is irrelevant to
* this problem, although might be useful for debugging.
*
*/

import java.util.*;

class Node
{
  Node n = null;
  Node()
  {
  }
  Node(Node n)
  {
    setNext(n);
  }
  void setNext(Node n)
  {
    this.n = n;
  }
  boolean hasNext()
  {
    return n!=null;
  }
  Node next()
  {
    return n;
  }
  boolean isLoopy(Set<Node> visited)
  {
    if(visited.contains(this))
    {
      return true;
    }else
    {
      if(hasNext())
      {
        visited.add(this);
        return next().isLoopy(visited);
      }else
      {
        return false;
      }
    }
  }
  boolean isLoopy()
  {
    return isLoopy(new HashSet<Node>());
  }
  // test harness
  public static void main(String[] args)
  {
    Node n1 = new Node();
    Node n2 = new Node(n1);
    n1.setNext(n2);

    System.out.println("loopy list is loopy:");
    System.out.println(n1.isLoopy());

    Node n4 = new Node();
    Node n3 = new Node(n4);

    System.out.println("non loopy list is loopy:");
    System.out.println(n3.isLoopy());
  }
}

This is a classic interview question.  At this point, I think it measures more whether you've been to many interviews than whether you can think creatively algorithmically while on a job interview.  Anyway,

public class Node {
  public Node next = null;
  public Node() {}
  public Node(Node n) { next = n; }
  public boolean hasNext() { return next != null; }
  public boolean hasLoop() {
    Node n = this, n2 = this;
    while (n.hasNext()) {
      n = n.next;
      if (n == n2) return true;
      if (!n.hasNext()) return false;
      n = n.next;
      if (n == n2) return true;
      n2 = n2.next;
    }
    return false;
  }
}

(Not compiled, but you get the idea.  I think getters/setters for next is overkill, given that the whole point is to manipulate the field at a low level, given this data structure.)

This uses constant space; since n travels twice as fast as n2, if there is a loop, both n and n2 will get caught at it and n will overtake n2 from behind (after a maximum number of steps equal to twice (lead-in plus loop length)).  Otherwise, n will reach the end and we're done.

Node n = null;

In Java, objects by default initialized to null, so

Node n;


String Zigzag Conversion Overview

The string 0123456789 is written in a zigzag pattern (following the path going all the way down, then diagonally moving towards the top right) using a given number of rows. Like this example in the case of 3 rows.

0   4   8
1 3 5 7 9
2   6

This string is then read line by line and converted into a new string. In this case this new string from the zigzag will be 0481357926.

I'd like to understand any optimizations I can make using C++17 features, edge cases I may gave missed, possible overflows errors, etc.

#include <cmath>
#include <iostream>
#include <string>

class Solution {
    public:
        std::string convert(const std::string& s, const int numRows) {
            if (numRows == 1) { return s; }
            const int size = s.size();
            std::string converted = "";
            const int num  = numRows + (numRows - 2);
            int row = 0;
            int i   = 0;
            int sub = 0;
            bool useDiff = true;
            while (row < numRows) {
                converted += s[row];
                if (row == 0 || row == numRows - 1) {
                    i = row + num;
                    while (i < size) {
                        std::string str(1, s[i]);
                        converted.append(str);
                        i += num;
                    }
                } else {
                    int diff = std::abs(num - sub);
                    i = diff;
                    while (i < size && row + i < size) {
                        if (useDiff) {
                            std::string str(1, s[row + i]);
                            converted.append(str);
                            useDiff = false;
                            i += sub;
                        } else {
                            std::string str(1, s[row + i]);
                            converted.append(str);
                            useDiff = true;
                            i += diff;
                        }
                    }
                }
                useDiff = true;
                sub += 2;
                row++;
            }

            return converted;
        }
};

int main()
{
    Solution solution;
    std::string s = "0123456789";
    int numRows = 3;
    std::cout << s << '\n';
    s = solution.convert(s, numRows);
    std::cout << s << '\n';
}

Don't use an alibi-class. It doesn't make the code any more OO, even in the cases it would be desirable, it just adds to the clutter.
Solution and convert are such generic names. Naming is hard, but good names really help.
You know an int need not be positive? An unsigned type may be more appropriate for a count, but even then you should handle zero.
Only if you need a null-terminated string should you consider a parameter of type std::string const&. In all other cases, using std::string_view is more efficient.
While creating a temporary string of length 1 probably doesn't cause dynamic allocation due to small-string-optimisation (or allocation ellision), doing so just to add a character to the result-string is cumbersome and potentially quite inefficient (in theory, the compiler could optimise it all out). Just append it directly.
If you assign directly, diff is gone.
useDiff doesn't make a difference, so remove it.
Consider adding additional test-cases. For that, you should rewrite main().

See the modified code live on coliru:

#include <string>
#include <string_view>
#include <stdexcept>

auto zigzag(std::string_view s, std::size_t cRows) {
    if (!cRows)
        throw std::domain_error("Must output at least one row.");
    if (!cRows || cRows >= s.size())
        return std::string(s);
    std::string r;
    r.reserve(s.size());
    const auto period = 2 * cRows - 2;
    for (auto row = 0 * cRows; row < cRows; ++row) {
        auto delta = 2 * row;
        auto delta2 = period - delta;
        if (!delta) delta = period;
        if (!delta2) delta2 = period;
        for (auto i = row; i < s.size(); i += delta) {
            r += s[i];
            std::swap(delta, delta2);
        }
    }
    return r;
}

#include <iostream>

struct test {
    std::string_view in;
    std::size_t n;
    std::string_view out;
};

int main() {
    constexpr test cases[]{
        "0123456789", 3, "0481357926",
        "", 1, "",
        "0123456789", 10, "0123456789",
        "0123456789", 2, "0246813579",
    };
    for (auto&& x : cases) {
        std::cout << "zigzag(\"" << x.in << "\", " << x.n << ") == \"" << x.out;
        auto r = zigzag(x.in, x.n);
        if (r == x.out)
            std::cout << "\": PASSED\n";
        else
            std::cout << "\": FAILED, got \"" << r << "\"\n";
    }
}

So from a generic perspective I would suggest to cut the task into different sub tasks

Determine, which row the character is in. You can determine this generically from the position the character has in the string.

Note that this is completely determined by the size of the zigzag pattern and effectively independentof the string itself.

Then sort the string by that index. If your algorithm creates an [x,y] Coordinate you simply need so sort via

constexpr friend bool operator<(const Coordinate& a, const Coordinate& b) {
    return std::tie(a.y, a.x) < std::tie(b.y,b.x);
}

So one simple way would be to create an array of indexes and sort that as coordinate and then create the string from the the index array


Binary Search implemented in C++ This is my implementation of a recursive binary search in C++. I'm still a newbie at C++ so:

Have I used any bad practices?
How could the code be improved upon in terms of efficiency?
Could recursion cause a stack overflow error, and if so how can I avoid this?

Criticism is welcome and appreciated!

#include<iostream>
#include<sstream>
#include<vector>

template <typename T>
void outputVector(std::vector<T> input){
    std::cout << "[";
    for(int i = 0; i < input.size(); i++){
        std::cout << input[i];
        if(i != input.size()-1){
            std::cout << ", ";
        }
    }
    std::cout << "]\n";
}

int binarySearch(std::vector<double> input, double target, int startIndex = 0){
    if(input.size() < 2){
        //std::cout << "Closest match found\n";
        return startIndex;
    }

    int middleIndex = int(input.size())/2;
    double middle = input[middleIndex];

    if(target == middle){
        return middleIndex + startIndex;
    }
    else if(target < middle){
        std::vector<double>::const_iterator begin = input.begin();
        std::vector<double>::const_iterator last = input.begin() + middleIndex;
        std::vector<double> firstHalf(begin, last);
        //std::cout << "First Half: ";
        //outputVector(firstHalf);
        return binarySearch(firstHalf, target, startIndex);
    }
    else if(target > middle){
        std::vector<double>::const_iterator begin = input.begin() + middleIndex + 1;
        std::vector<double>::const_iterator last = input.begin() + input.size();
        std::vector<double> secondHalf(begin, last);
        //std::cout << "Second Half: ";
        //outputVector(secondHalf);
        return binarySearch(secondHalf, target, startIndex+middleIndex+1);
    }
    else{
        return -1;
    }
}

std::vector<double> doubleVectorInput(){
    std::string inputString;
    getline(std::cin, inputString);
    std::vector<double> array;
    std::istringstream iss(inputString);
    float val;
    while(iss >> val){
        array.push_back(val);
    }
    return array;
}

int main(){
    double target;
    std::cout << "Vector to search: ";
    std::vector<double> input = doubleVectorInput();
    std::cout << "Target: "; std::cin >> target;
    std::cout << "Target index: " << binarySearch(input, target) << "\n";
    return 0;
}

Thank you.

Space Complexity

This is quite inefficient in terms of the storage space it uses.

Assuming the bisection works as we'd hope, and the vector is cut exactly in half every time, the recursive call copies half the input vector. If, for example, we started with an array of a million items, the first recursive call will copy half a million items. The second recursive call will copy a quarter of a million items.

Since the first call also passed the input array by value, we expect that our search of one million items ends up copying about two million items (in addition to the original vector the user passed).

Time Complexity

Copying those items takes a fair amount of time as well. To be more specific, it takes linear time.

Summary

We normally expect a binary search to have complexity of about \$O(log N)\$, and only descend to \$O(N)\$ in relatively rare worst cases. In this case, we get \$O(N)\$ in about the best case, and \$O(N^2)\$ in the bad cases.

This violates the normal expectations of a binary search badly enough that it effectively only barely qualifies as a binary search at all. In many cases, we can expect its performance to be substantially worse than a simple linear search.

Recommendation

You really don't want to pass the vector by value. Either pass it by reference, along with a pair of indexes into the vector giving the current upper and lower bounds of the part you want to search, or else pass a pair of iterators (there are probably other options as well, but in any case you want to pass an indication of the data to search rather than copying all the data you're going to search).

There are other parts of the code that could use changing as well, but that set of changes is likely to render many (most?) of them obsolete anyway, so I'm not going to get into other areas for now.

For an algorithm to be classified as a binary search it must have worse-case time complexity of O(log n).

As mentioned by Jerry, here you have a worse case O(N ^2), I suggest you re-think your method.

I'm not sure why you'd perform this operation recursively in C++ (not a great language for such an approach).


Return the state of collection based on individual object states The following code is a simplified version of what I want to achieve. Each object may have one of several states, and the overall collection state depends on the state of each of these objects. Currently, the code is written in the form of several if-else blocks, however, it seems ugly and non-extensible. By that, I mean, tomorrow if additional object states are added, I would end up adding several more flags and if-conditions, making the code difficult to read and maintain. Can this be done in a better way?

internal enum State
{
    Error,
    Warning,
    Okay
}

internal enum CollectionState
{
    Error,
    AllWarnings,
    AllOkay,
    OkayAndWarnings
}

internal class MyClass
{
    //other properties...

    internal State State { get; set; }
}

internal class MyBusinessLogic
{
    /// <summary>
    /// 1. If any object is in Error state, CollectionState is Error.
    /// 2. If all objects are in Warning state, CollectionState is AllWarnings.
    /// 3. If all objects are in Okay state, CollectionState is AllOkay.
    /// 4. If some objects are in Okay state and some in Warning state, CollectionState is OkayAndWarnings
    /// </summary>
    internal CollectionState GetCollectionState(IList<MyClass> objectList)
    {
        bool isError, isOkay, isWarning;
        isError = isOkay = isWarning = false;
        foreach (var myObject in objectList)
        {
            if (myObject.State == State.Error)
            {
                isError = true;
                break;
            }
            if(myObject.State == State.Okay)
            {
                isOkay = true;
            }
            else if (myObject.State == State.Warning)
            {
                isWarning = true;
            }
        }

        if (isError)
        {
            return CollectionState.Error;
        }
        else if(!isOkay && isWarning)
        {
            return CollectionState.AllWarnings;
        }
        else if (isOkay && !isWarning)
        {
            return CollectionState.AllOkay;
        }
        else //if(isOkay && isWarning)
        {
            return CollectionState.OkayAndWarnings;
        }
    }
}

You can also use bitwise flags for that:

[Flags]
internal enum State
{
    None = 0,
    Error = 1,
    Warning = 2,
    Okay = 4,

    OkayAndWarning = Okay | Warning,
}

internal State GetCollectionState(IList<MyClass> objectList)
{
    return objectList.Select(o => o.State)
                     .Aggregate((result, next) => result |= next);
}

You can then check the aggregated state using HasFlag method or equality operator:

var state = GetCollectionState(...);
var error = state.HasFlag(State.Error);
var allWarnings = state == State.Warning;
var allOkay = state == State.Okay;
var okayAndWarnings = state == State.OkayAndWarning;
//etc.

On the plus side this approach is easy to implement and State enum is easy to extend when needed. Combining flags is a trivial process of applying | (bitwise OR) operator.

On the minus side you give up some of the type safety. With this approach any flag combination is a valid state, (including no flags at all). Also there is no longer a distinction between individual state and collection state (both use State enum), compiler will not warn you if you accidentally mix them. So in general, you'll have to be more careful, when checking states.

Consider a collection class.

It is still messy when you add more states but you can use some tricks from other answers.

I don't like calling a property the same name and the enum.

I like lower case for internal but that may not be the standard.

Don't skip all for OkayAndWarnings.

internal enum States { error, okay, warning, none };
public class MyClass
{
    //other properties
    internal States State { get; set; }
}
internal enum CollectionState { error, allWarnings, allOkay, allOkayAndWarnings, none }
internal class MyClassCol
{
    internal List<MyClass> MyClasses { get; } = new List<MyClass>();

    internal CollectionState MyClassColState
    {   get
        {
            bool isOkay, isWarning;
            isOkay = isWarning = false;
            foreach (MyClass myClass in MyClasses)
            {
                if(myClass.State == States.error)
                {
                    return CollectionState.error;
                }
                if(myClass.State == States.okay)
                {
                    isOkay = true;
                }
                if (myClass.State == States.warning)
                {
                    isWarning = true;
                }
            }
            if(isOkay && isWarning)
            {
                return CollectionState.allOkayAndWarnings;
            }
            if (!isOkay && isWarning)
            {
                return CollectionState.allWarnings;
            }
            if (isOkay && !isWarning)
            {
                return CollectionState.allOkay;
            }
            return CollectionState.none;
        }
    }
}