Finding the first duplicate in an array My code returns the correct answer the first duplicate is 3 (represented as int f) . I am struggling finding a more efficient way to find the first duplicate?

My constraints are \$ 1 ≤ a.length ≤ 10^5, 1 ≤ a[i] ≤ a.length \$ .

class Program
{
  static void Main(string[] args)
  {
    int[] a = {2, 3, 3, 1, 5, 2};

    int f = FirstDuplicate(a);

    Console.ReadLine();
   }
public static int FirstDuplicate(int[] a)
{

    int[] answer = new int[2];
    answer[0] = -1;
    answer[1] = a.Length;
    for (int i = 0; i < a.Length - 1; i++)
        for (int j = i; j < a.Length; j++)
            if (a[i] == a[j] && i != j)
            {
                if (i < answer[1])
                {
                    answer[0] = a[i];
                    answer[1] = j;
                    break;

                }
            }

    return answer[0];
  }
}

For a small array or list of a handful of items, this should be very fast.  But is it efficient as you would scale out?  No.

Before getting on to a better way to achieve the it for a larger number of items, let's look at your current code.  You have some checks that aren't needed.

if (a[i] == a[j] && i != j)

The check of i != j is not needed if you were to simply initialize j to be i + 1 as in:

for (int j = i + 1; j < a.Length; j++)
    if (a[i] == a[j])

The check of if (i < answer[1]) is totally unnecessary.  In fact, answer as an array is not needed at all as you do not really do anything meaningful with answer[1].

But having 2 loops is not efficient as the input array could be larger.  As mentioned in a comment, the use of a HashSet could be used.  I am changing a few things.

The method signature is now IEnumerable<int> to allow for arrays,
lists, or any collection of integers.
I use a HashSet to keep track of items I have already
encountered.
The return type is a Nullable<int>, also written as int?, so that
if there are no duplicates found a null is returned.

The resulting method also looks cleaner.  Someone looking at this understands more readily what the code is trying to achieve:

public static int? FirstDuplicate(IEnumerable<int> items)
{

    HashSet<int> set = new HashSet<int>();
    foreach (int item in items)
    {
        if (set.Contains(item))
        {
            return item;
        }
        set.Add(item);
    }
    return null;
}

As shown in @Denis's answer, and echoed by @Corak, is the method could be made slightly faster and shorter by reducing the logic inside the foreach loop:

if (!set.Add(item))
{
    return item;
}

While this again is slightly faster, slighter cleaner, and recommended in practice, it still lags well behind @vnp 's array solution.

UPDATED Worst Case Array

I composed a worst case array to test of 100000 items where the very last item is the first duplicate of the 2nd to last item.  Your code and mine returned the correct value.

public static int[] WorstCaseArray
{
    get
    {
        int[] items = new int[(int)Math.Pow(10, 5)];
        for (int i = 0; i < items.Length; i++)
        {
            items[i] = i + 1;
        }
        items[items.Length - 1] = items[items.Length - 2];
        return items;
    }
}

UPDATE #2 - Worst Case

Obviously we should be working with the largest array allowed.

As @PeterCordes states in a comment to @vnp's answer, the list should be randomized.

As @Denis states, the worst case for a HashSet would be if the only duplicate was the first and last item.  An extremely close second would be if the last and 2nd to last item were the only duplicate.  Note this second worst case for HashSet would be the worst case for @vnp's array solution, so I will use it as my worst case.

public static int[] WorstCaseArray
{
    get
    {
        // Worst case is largest allowed array ...
        int max = (int)Math.Pow(10, 5);
        int[] items = new int[max];
        for (int i = 0; i < items.Length; i++)
        {
            items[i] = i + 1;
        }
        // ... that is randomly shuffled ...
        Shuffle(items);
        // ... with the only duplicate being the last item matching the 2nd to last!
        items[items.Length - 1] = items[items.Length - 2];
        return items;
    }
}

private static Random random = new Random();
public static void Shuffle(int[] items)
{
    //Fisher-Yates method to shuffle
    for (int i = items.Length - 1; i > 0; --i)
    {
        int randomIndex = random.Next(i + 1);
        int temp = items[i];
        items[i] = items[randomIndex];
        items[randomIndex] = temp;
    }
}

UPDATED Worst Case Timings:

Yours: 14.5105877 seconds
Mine : 00.0065640 seconds
vnp's: 00.0012733 seconds

Tiny array with many duplicates

However, as I said for only a handful of items yours is quite fast.  The problem is - it produces the wrong answer!  Using this array:

int[] array =  new[] { 1, 2, 3, 4, 5, 6, 3, 4, 5, 6 };

Where the expected answer would be 3, yours returns 6.

Yours: took 0.0006141 seconds but returned 6 (wrong)
Mine : took 0.0006162 seconds
vnp's : 0.0003843 seconds

Large, Random Array with No Duplicates

To round out the answer, let's look at the largest allowed array, randomized, but without any duplicates.  Using the shuffle routine from above:

public static int[] NoDuplicates()
{
    int[] items = Enumerable.Range(1, (int)Math.Pow(10, 5)).ToArray();
    Shuffle(items);
    return items;
}

Timings:

Yours: 15.1978462 seconds
Mine  : 00.0055364 seconds
vnp's : 00.0014876 seconds

BEST ANSWER

Also, since you have tagged this question with [time-limit-exceeded], vnp's answer is not just clever, but also the fastest in all tested scendarios, which makes it the best answer (regarding performance).


Generating a sequence of MAC addresses The following code will generate a sequence of N mac addresses based on the input from user. The input must be atleast one octet to generate the sequence , ie :

input  : A0 , 5
output : A0:00:00:00:00:01 - A0:00:00:00:00:05

My actual code is working fine and I wanted to make it optimized if it could be :

public static List<String> getMacSequences(final String macAddress, final Integer count)
    {
        List<String> macList = new ArrayList<String>();
        final List<Integer> macOctets = new ArrayList<Integer>();
        String [] macSplit = macAddress.split(":");

        int incPos = macAddress.split(":").length; // need to throw exception when the length is 0
        if  (incPos >= 1)
        {
            for (int bp=0; bp <6; bp++)
            {
                if (bp >= incPos)
                {
                    macOctets.add(0);
                }
                else
                {
                    macOctets.add(Integer.parseInt(macSplit[bp], 16));
                }
            }

            for (int i = 0 ; i<count; i++)
            {
                int lastOctet = macOctets.get(5);
                macOctets.set(5, ++lastOctet);
                for (int j = 5; j >= 0; j--)
                {
                    if (j == 0 && macOctets.get(j) > 255)
                    {
                        Collections.fill(macOctets, 0);
                    }

                    if (macOctets.get(j) > 255)
                    {
                        macOctets.set(j, 0);
                        Integer macValue = macOctets.get(j - 1);
                        macOctets.set(j - 1, ++macValue);
                    }
                    else{
                    break;
                    }
                }
                macList.add(String.format("%02X:%02X:%02X:%02X:%02X:%02X",  macOctets.toArray()));
            }
        }
        return macList;
    }

I have done micro-benchmarking to determine the performance for generating 100 mac addresses in sequence and the result of average value for 10 sampling (100 itr/sampling) as follows (in ms) :

7.62   7.43    6.87    7.4     7.24    7.21    7.16    7.59    7.67    8.13

and the overall sampling average is : 7.432 ms

Your code defines in the second loop: for (int j = 5; j >= 0; j--) and  later you use j in this statement: macOctets.set(j - 1, ++macValue); If j==0 however, you will get an IndexOutOfBounceException as -1 is not a valid index. You avoid the exception by checking explicitely for j==0 and set the value to 0, though this check will be done for every segment of the mac address. This check therefore can be refactored out of the loop and to a later position.

Next, you split the input string twice. Why? Something like

String [] macSplit = macAddress.split(":");

int incPos = macAddress.split(":").length; // need to throw exception when the length is 0
if  (incPos >= 1) {
    ...
}

can be replaced with this:

String[] macSplit = macAddress.split(":");
if (macSplit.length >= 1) {
    ...
}

This will avoid splitting the input string multiple times.

I'd also initially fill the macOctet array with 0 values and then simply set the respective defined mac value like this:

for(int i=0; i<macSplit.length; i++)
{
    macOctets[i] = Integer.parseInt(macSplit[i], 16);
}

If I execute the below code and compare it to your original code:

public static List<String> getMacSequences(final String macAddress, final Integer count)
{
    List<String> macList = new ArrayList<String>();
    Integer[] macOctet = new Integer[] { 0, 0, 0, 0, 0, 0};
    String[] macSplit = macAddress.split(":");
    if (macSplit.length > 0)
    {
        for (int i=0; i<macSplit.length; i++)
        {
            macOctet[i] = Integer.parseInt(macSplit[i], 16);
        }
    }
    for (int i=0 ; i<count; i++)
    {
        macOctets[5]++;
        for (int j=5; j>0; j--)
        {
            if (macOctets[j] > 255)
            {
                macOctets[j] = 0;
                macOctets[j-1]++;
            }
            else
            {
                break;
            }
        }
        if (macOctets[0] > 255)
        {
            macOctets[0] = 0;
        }
        macList.add(String.format("%02X:%02X:%02X:%02X:%02X:%02X",  macOctets));
    }
    return macList;
}

I'll get the following times for 100000 iterations on 260 generated MAC addresses:

Original - Total: 71341.440878 ms, Average: 0.71341440878 ms
Modified - Total: 70220.091388 ms, Average: 0.70220091388 ms

While not a big speed improvement, at least a little faster then the original one.

Further speed improvements

I used Integer[] macOctets = new Integer[] { ... } over a simple int[] macOctets = new int[] { ... } in the sample code above as the string formatter has some issues with int arrays for some reasons with my version of Java (Oracle Java 8 Update 101).

I then implemented my own version of MAC string converter

private static String toHexString(int[] macOctets)
{
    StringBuilder sb = new StringBuilder();
    for (int i=0; i<macOctets.length; i++)
    {
        sb.append(Integer.toHexString(0x100 | macOctets[i]).substring(1).toUpperCase());
        if (i != macOctets.length-1)
        {
            sb.append(":");
        }
    }
    return sb.toString();
}

and changed the initialization of the macOctets array to a simple

int[] macOctets = new int[6];

which internally will initialize each element with its default value, which is 0.

When re-running the comparison test described above, the following output is returned:

Original - Total: 71901.357327 ms, Average: 0.71901357327 ms
Modified - Total: 10287.282422 ms, Average: 0.10287282422 ms

The custom method makes use of a tiny hack in order to print the two byte pairs of a MAC segment.

It seems that the algorithm spends most of the time within the array to hex string formating. I'd therefore highly suggest to change from the String formatter to a custom one as this only requires a fraction of the time the Java native function requires.

For completeness I'll post the complete performance test main method so you can compare for yourself

public static void main(String ... args)
{
    int repetitions = 100000;
    String initialSequene = "A0";
    int macsToGenerate = 260;

    long startTime = System.nanoTime();
    for (int i=0; i<repetitions; i++)
    {
        getMacSequences(initialSequene, macsToGenerate);
    }
    long endTime = System.nanoTime();

    long duration = (endTime - startTime);
    double totalDurationInMs = (duration / 1000000.);
    System.out.println("Original - Total: " + totalDurationInMs + " ms, Average: " + (totalDurationInMs / repetitions) + " ms");

    startTime = System.nanoTime();
    for (int i=0; i<repetitions; i++)
    {
        getMacSequences2(initialSequene, macsToGenerate);
    }
    endTime = System.nanoTime();

    duration = (endTime - startTime);
    totalDurationInMs = (duration / 1000000.);
    System.out.println("Modified - Total: " + totalDurationInMs + " ms, Average: " + (totalDurationInMs / repetitions) + " ms");
}

where getMacSequences(...) is your version and getMacSequences2(...) is my modified version.


How can I layout this class to make it easier to use for API developers? I have an application that allows the user to place a string of text into a PDF document.  I have three different ways that they can do this:

Use a Form Field. Then they have four properties to define:

Provide field name
provide instance of the field
provide an X-axis Offset
provide a Y-axis Offset

Search for a string of text. Then they have four properties to define:

Provide string of text to search for
provide instance of that string of text
provide an X-axis Offset
provide a Y-axis Offset

Define Page Coordinates. Then they have three properties to define:

Provide page number
provide an X-axis Offset
provide a Y-axis Offset

In my API, I want the object to be setup intuitively so that it is clear they are choose one of the three different methods and then whichever one they choose, they have the options for each.

In my old API, all of those options are grouped together as a list of properties like this:

Placement.FormField_FieldName
Placement.FormField_Instance
Placement.SearchText_Text
Placement.SearchText_Instance
Placement.PageCoordinates_PageNumber
Placement.XOffset
Placement.YOffset

I find this to be a little confusing for the developer using the API because you could never use all of the properties together.

You would either use Form Field like this:

Placement myPlacement = new Placement();
myPlacement.FormField_FieldName = "MyPdfFormFieldName";
myPlacement.FormField_Instance = ;
myPlacement.XOffset = 0;
myPlacement.YOffset = 0;

Or use Text Search like this:

Placement myPlacement = new Placement();
myPlacement.SearchText_Text = "My String Of Text";
myPlacement.FormField_Instance = 1;
myPlacement.XOffset = 0;
myPlacement.YOffset = 0;

Or use Page Coordinates like this:

Placement myPlacement = new Placement();
myPlacement.PageCoordinates_PageNumber = "My String Of Text";
myPlacement.XOffset = 0;
myPlacement.YOffset = 0;

Is there a better way to setup the class (or sub classes) to make it easier for the developer to understand how to use this functionality?

UPDATE

This is what I currently am using because the abstract class thing didn't work.

public class SignatureBlock
{
    public List<SignatureBlockPlacement> SignatureBlockPlacements = new List<SignatureBlockPlacement>();
}

public class SignatureBlockPlacement
{
    public Enumerations.SignatureBlock.SignatureBlockTypes Type { get; set; }
    public string FormField_FieldName { get; set; }
    public int FormField_InstanceOfField { get; set; }
    public string SearchText_TextToSearch { get; set; }
    public int SearchText_InstanceOfText { get; set; }
    public string PageCoordinates_PageNumber { get; set; }
    public int XOffset { get; set; }
    public int YOffset { get; set; }
}

Which I'm implementing like this:

SignatureBlock sigBlock = new SignatureBlock();

SignatureBlockPlacement sig = new SignatureBlockPlacement();
sig.Type = Enumerations.SignatureBlock.SignatureBlockTypes.Signature;
sig.FormField_FieldName = "SignatureField";
sig.FormField_InstanceOfField = 1;

SignatureBlockPlacement title = new SignatureBlockPlacement();
title.Type = Enumerations.SignatureBlock.SignatureBlockTypes.Title;
title.FormField_FieldName = "SignatureField";
title.FormField_InstanceOfField = 1;
title.XOffset = 0;
title.YOffset = -20;

SignatureBlockPlacement org = new SignatureBlockPlacement();
org.Type = Enumerations.SignatureBlock.SignatureBlockTypes.Organization;
org.FormField_FieldName = "SignatureField";
org.FormField_InstanceOfField = 1;
org.XOffset = 0;
org.YOffset = -40;

sigBlock.SignatureBlockPlacements = new List<SignatureBlockPlacement>() { sig, title, org };

If there's a better way to do this, please let me know.

UPDATE #2

I should have mentioned this is for a WCF SOAP web service.

UPDATE #3

Long story short on this one, I need to allow the developer three different ways to place the signature but they should only be allowed to use one of the three ways .  Each way shares the X/Y Offset properties but their other properties are unique to each way.

not very sure about your placement class, whether it's a pure class designed to encapsulate options, or a win-form.

If it's the former, i would suggest you use Polymorphism to have certain abstract layer, a quick sample:

At your side:

public abstract class Placement
{
    public int XOffset { get; set; }
    public int YOffset { get; set; }

    public class FrmField:Placement
    {
        public string FieldName { get; set; }
        public int Instance { get; set; }
    }

    public class TextSearch:Placement
    {
        public TextSearch(){}
        public string Text { get; set; }
        public int Instance { get; set; }
    }

    public class PageCoord : Placement
    {
        public string PageNumber { get; set; }
    }
}

public class SomeObjRequriePlacement
{
    public void DoSomething(Placement pla)
    {
        // this class and method may be a winform that do something based on given options
    }
}

Other dev using your api:

public class TestAtDev
{
    public void TestGeneric()
    {
        Placement pla = null;
        if (false) //some condition
        {
            pla = new Placement.FrmField() { FieldName = "", Instance = 1, XOffset = 1, YOffset = 2 };
        }
        else if (true) // some condition
        {
            pla = new Placement.PageCoord() { PageNumber = "xxx", XOffset = 1, YOffset = 2 };
        }
        else
        {
            pla = new Placement.TextSearch() { Text = "", XOffset = 1, YOffset = 2 };
        }

        var someObj = new SomeObjRequriePlacement();
        someObj.DoSomething(pla);
    }
}

if your Placement class is already an UI components, i would then suggest you separate the options away and wrap them as PlacementOption and use it in your placement class.

and if necessary, you may define an interface to make the code more independent. And Generic (like Type constraint) could make your code even tidier.

EDIT:

use type constraint to have strong typed object where necessary - but the basic implementation of the placement is still the same.

public abstract class HandlePlacement<T> where T:Placement
{
    public virtual void DoWork(T thePlacement) { }
}

public class HandleTextSearch : HandlePlacement<Placement.TextSearch>
{
    public override void DoWork(Placement.TextSearch thePlacement)
    {
        // do some specific work for text search option
    }
}

/*implementations for other placement options*/
/* ... */

public class TestAtDev
{
    public void DoTextSearch()
    {
        Placement.TextSearch ts = new Placement.TextSearch() { XOffset = 0, YOffset = 1, Instance = 2, Text = "" };
        HandlePlacement<Placement.TextSearch> hanlder = new HandleTextSearch();
        hanlder.DoWork(ts);
    }
}

another sample:

public abstract class SomeHandler
{
    public virtual void DoWork() { }
}

public abstract class SomeHandler<T>:SomeHandler where T : Placement
{
    protected SomeHandler(T thePlacement)
    {
        this.Placement = thePlacement;
    }

    protected T Placement { get; set; }
}

public class SomeHandlerForTextSearch : SomeHandler<Placement.TextSearch>
{
    public SomeHandlerForTextSearch(Placement.TextSearch ts):base(ts){}

    public override void DoWork()
    {
        // some specific handling for text search
    }
}

public class SomeHandlerForPageCoord : SomeHandler<Placement.PageCoord>
{
    public SomeHandlerForPageCoord(Placement.PageCoord pc) : base(pc) { }

    public override void DoWork()
    {
        // some specific handling for page coordinates
    }
}

public class TestAtDev2
{
    public void DoSomeTest()
    {
        SomeHandler h = null;
        if (true) // some condition
        {
            h = new SomeHandlerForTextSearch(new Placement.TextSearch() { XOffset = 1, YOffset = 2, Instance = 3, Text = "test" });
        }
        else if (true) // some condition
        {
            h = new SomeHandlerForPageCoord(new Placement.PageCoord() { XOffset = 1, YOffset = 2, PageNumber = "PageNo." });
        }
        h.DoWork();
    }
}


Create quantile slices from dataframe columns I have the following, working code:

import pandas as pd

hld_per = 12    # Holding period in months
quantiles = 10  # Number of bins/buckets; Deciles, use 10; Quartiles, use 4; etc.
permnos = pd.read_csv('Ranks.csv')
my_headers = list(permnos.columns)
total_cols = len(permnos.columns)

pntls = permnos.copy(deep=True)

# Compute the percentile of each element based on position (each column in permnos is sorted best-to-worst)
for i in range(total_cols):
    for j in range(pntls.iloc[:,i].count()):
        pntls.iloc[j,i] = (j+1)/pntls.iloc[:,i].count()

# Create slices by column based on pntls values and choice of quantiles.
# Write resultant portfolios to files (# of portfolios = # in quantile).
for i in range(quantiles):
    ports = []
    for j in range(total_cols-(hld_per-1)):
        permlist = []
        for k in range(hld_per):
            slc = (pntls.iloc[:,j+k] > i/quantiles) & (pntls.iloc[:,j+k] <=(i+1)/quantiles)
            col_slice = permnos[slc].iloc[:,j+k].tolist()
            permlist += col_slice
        ports.append(permlist)

    matrix = pd.DataFrame(ports).T
    matrix.columns = my_headers[0:len(matrix.columns)]
    matrix.to_csv("portstst2_" + str(i+1) + ".csv", sep=',', index=False, header=True)

I built this code as a solution to a question I posted on stackoverflow in this thread.  While it works, it is not very fast.  Additionally, as someone new to Python, I imagine there are much better ways to write this.

The purpose of this code is to read in a csv file which is organized as follows:  Column headers are dates in the format YYYYMMDD, descending monthly moving left to right (e.g. the first column is 20131231 followed by 20131130,etc).  The data in the rows are identifiers which are sorted from "best" to "worst" in a separate program.  The rows are of varying lengths, and the number of rows in each column determines an identifier's rank/percentile/bucket for the month/column.

The data in each column is broken into quantiles using the values calculated in the pntls dataframe.  An example:  If hld_per is 3 and quantiles is 4, the code takes the top 25% of identifiers in column 0 of permnos and places them in column 0 of the list ports.  The top 25% of identifiers in column 1 of permnos is then appended to ports.  Then the top 25% of identifiers in column 2 are appended to ports.  This will populate the first column of matrix.  The code next follows the same procedure on columns 1, 2, and 3 of permnos to populate the second column of matrix.  And so forth.  A csv file is written for each quantile.

My Specific Questions

As noted in the stackoverflow post, I initially tried to approach this using qcut(). I was unable to adapt the suggested solution to work, but perhaps it is a better method than my solution.  So question 1 is: Is there a better method to generate my slices used in the second embedded for-loop (qcut() or otherwise)?  The primary requirement is that the slices within a column are of roughly the same size for each quantile.

Question 2: Should I use a second dataframe (in this case, pntls) to do my conditioning, or should I append the calculations in the first loop to permnos?

Question 3: Given a method to determine the slices, is my second for-loop an efficient way to create ports and matrix?

Question 4: For my data, the dates that are the column headers of Ranks.csv change often.  As such, I prefer to use selection by position as opposed to selection by label.  However, most of the posts I found when writing this code use selection by label.  Is there any reason to avoid selection by position/prefer selection by label?

Finally, any other points/critiques of my code are greatly appreciated.  Please point out anything I've done in an inefficient manner, especially with regards to speed.

To answer your questions in order:

Yes: your code could be optimised by calculating .count() (with respect to each slice of pntls) only once per outer loop, instead of n + 1 times (where n is the value of .count()) as per your current implementation. In the full example below, I show how you can use pandas.qcut to make this much more efficient in the add_quantiles function.
Re-using calculations is more efficient, but a procedure that doesn't alter the dataframe is more flexible: in the exmaple below, the calculations are re-used, but the main dataset isn't changed (everything is done from a copy). This may not be desiarable if: (a) you don't intend on using this code for any other purpose (or for more than one use, where keeping the original dataset untouched means you can re-run the procedure), or (b) if the size of the data in memory is large.
It could be improved: if I have understood corretly, the results of calculating col_slice in the inner most loop is actually used hld_per number of times (except when the column being passed to col_slice is within hld_per columns from the end of the dataset). We can reduce the number of calculations of col_slice by a factor of hld_per, therefore, if we re-use the results. In the example I've given below, I show how you can do this to improve performance.
Only if you don't know your data structure ahead of time; you've used indexing by position rather than label to good effect.
Apart from the points above: your code was easy to read (thanks, too, for the detailed question!)!

My suggested re-write using the above suggestions is given below. The main function to run is create_csvs (nb: this uses label indexing instead of the positional indexes that you use - this could be switch out if you prefer). This will create csv files in the same format as your example code.

NB: I have assumed that the order of the rows in each column of the output CSVs doesn't matter. If this does matter, I can suggest a change.

    from collections import defaultdict

    colname = lambda col, suffix: '{}_{}'.format(suffix, col)

    def add_quantiles(data, columns, suffix, quantiles=4, labels=None):
        """ For each column name in columns, create a new categorical column with
            the same name as colum, with the suffix specified added, that
            specifies the quantile of the row in the original column using
            pandas.qcut().

            Input
            _____
            data:
                pandas dataframe
            columns:
                list of column names in `data` for which this function is to create
                quantiles
            suffix:
                string suffix for new column names ({`suffix`}_{collumn_name})
            labels:
                list of labels for each quantile (should have length equal to `quantiles`)

            Output
            ______
            pandas dataframe containing original columns, plus new columns with quantile
            information for specified columns.
        """
        d = data.copy()
        quantile_labels = labels or [
            '{:.0f}%'.format(i*100/quantiles) for i in range(1, quantiles+1)
        ]
        for column in columns:
            d[colname(column, suffix)] = pd.qcut(d[column], quantiles, labels=quantile_labels)
        return d

    def create_csvs(data, columns, hld_per=3, quantiles=4, suffix='quant'):
        """
            Input
            _____
            data:
                the full dataset
            columns:
                the ordered list of names of the date columns
            hld_per:
                holding period
            suffix:
                string suffix for quantile columns (no effect on output)
            quantiles:
                number of quantiles

            Output
            ______
            None

            Effect
            ______
            writes csv files
        """
        quantile_labels = ['{:.0f}%'.format(i*100/quantiles) for i in range(1, quantiles+1)]
        d = add_quantiles(data, columns, suffix, quantiles, quantile_labels)

        period_dict = lambda: defaultdict(list)
        ports = defaultdict(period_dict)
        for i in range(len(columns) - hld_per + 1):
            column = columns[i]
            period_columns = columns[max(i-hld_per, 0):i+1]
            for label in quantile_labels:
                for period in period_columns:
                    ports[label][period] += list(d[column][d[colname(column, suffix)] == label].values)

        for i, label in enumerate(quantile_labels):
            # Fill each list in `ports` with `np.null` so that they all have the same length of `maxlen`
            max_len = max(map(lambda v: len(v[1]), ports[label].items()))
            for c in ports[label]:
                ports[label][c] += [np.NaN]*(max_len-len(ports[label][c]))
            pd.DataFrame(ports[label]).to_csv("portstst2_" + str(i+1) + ".csv", sep=',', index=False, header=True)


Simple game to learn common port numbers for Sec+ exam This is a basic game asking for the common port number for a protocol used. I designed it with the intent on helping myself and others who are going after their Sec+ exam to memorize the ports needed. Any in cleaning up the code or better ways to process the lists would be most appreciated.

import random
import sys
from os import system

#List of lists of ports and protocols for SEC+ exam
numbers= {'TCP 22':('ssh','scp','sftp'),'TCP AND UDP 53':('DNS'),
'TCP 20 and 21':('FTP'),'UDP 69':('TFTP'),'443':('FTPS,HTTPS,SSL VPN443'),'TCP 25':('SMTP'),
'TCP 110':('POP3'), 'TCP 143':('Imap'),'TCP 23':('TelNet'),'TCP 49':('TACACS+'),'UDP 49':('TACACS'),
'UDP 1701':('L2TP'),'TCP and UDP 1723':('PPTP'),'TCP and UDP 3389':('RDP'),
'UDP 67,68':('DHCP'),'TCP 80':('HTTP'),'TCP AND UDP 88':('Kerberos'), 'TCP 119':('NNTP'),'UDP 161':('SNMP'),
'UDP 514':('Syslog'),'TCP 443':('HTTPS/SSL VPN/FTPS'),'UDP 67 and 68':('DHCP'),'UDP 500':('ISAKMP VPN'),'TCP AND UDP 162':('SNMP Trap')
        }

#List of ports for random choice
random_gen= ('TCP 20 and 21','TCP 22','UDP 69','TCP 443','TCP 25','TCP 110','TCP 143','TCP 23','TCP 49','UDP 49','UDP 500'
,'UDP 1701','TCP and UDP 1723','TCP and UDP 3389','TCP AND UDP 53','UDP 67 and 68','TCP 80','TCP AND UDP 88','TCP 119','UDP 161',
'TCP AND UDP 162','UDP 514'
            )

def Sec_port_game():
    correct = 0
    incorrect = 0
    system('cls')
    print("""This will ask you for port number and weather or not is uses TCP or UDP for communication. Answer must be formatted like 'TCP 443'
if the uses both the answer must be formatted 'TCP and UDP' followed by the port number. If it uses more than one port it must be formatted like
67 and 68. The lowest number will always come first.""")

    while (1==1):
        #Keeps track of total answred
        total = incorrect + correct
        #prints the amount of answers that you have correct, incorrect and total
        print(f"correct {correct} incorrect {incorrect}: {total} total answered out of 20 questions.")
        #creates the random port for question
        rand_choice= random.choice(random_gen)
        #Total of 20 random ports will ne assesed. Gives the total correct, incorrect and % coorect.
        if(total == 20):
            print(f"Total correct is {correct} total incorrectis {incorrect}")
            print("Precent correct is")
            print(correct/20 * 100)
            #Asks the player if they would like to play again
            play_again=input('''Would you like to play again? "yes" or "no" any other response to exit the program
:''')
            #if player wants to play agin sets the correct and incorrect to 0 to start exit if and start over.
            if(play_again.upper() == 'YES') or (play_again.upper() == 'Y'):
                correct =0
                incorrect = 0
            #If the player does not wish to play again thanks them for playing and break out of the while loop wo exit the game.
            else:
                print("Thank you for playing")
                break
        #Takes the answer for the random port
        answer = input(f"""Port number for {numbers[rand_choice]}?
:""")

        #Checks the answer to see if it's correct
        if (answer.upper() == rand_choice.upper()):
            print('CORRECT!')
            correct += 1

        else:
            print(f"""Nice try but the correct port is {rand_choice}""")
            incorrect += 1

#starts the game
Sec_port_game()

Naming things is hard

Two of your variables have terrible names. To see what I mean, try to guess what the following variables contain, by only looking at their name, but not at your code:

numbers
random_gen

All the other variables are named really good. They name exactly their purpose or accurately describe their content, like incorrect or answer.

Source code layout

In the first section you list the quiz questions. Each of these questions should be on its own line of the code, to clearly show to the reader how the code is structured. That way, all quiz questions are listed nicely below each other, and it's easily possible to count the questions by checking their line numbers in the editor.

It's also easy to see that the 443 question is missing the protocol (TCP), which is difficult to see in the current layout.

The 443 question uses a single string as answer, while the 22 question uses a tuple of the application protocols. Which one is correct, or are they both? I doubt so.

What is the difference between AND and and in the questions? If there is none, choose one spelling and stick to it.

Redundancy

Instead of repeating the question names in random_gen, you can just use the expression numbers.keys().

Instead of writing the number 20 everywhere, better describe what you really mean by that number, which is len(questions). (By now you should have renamed the numbers variable to the more appropriate questions).

Typos

Imap is spelled IMAP
VPN443 is spelled VPN
incorrectis is spelled incorrect is

Bug

The program does not guarantee that you get each question once. Instead of using random_choice for each of the questions you should rather shuffle the questions and then just iterate over them.

Code improvements

Instead of (1==1), better write True.
Instead of var == first or var == second, better write var in (first, second).
There is no need to enclose each condition of an if or while in parentheses. That's only necessary in C and related languages. In Python and Go, it is considered bad style.

The function Sec_port_game should simply be named quiz since that is more specific than game, and at the same time makes the code independent from the Sec+ exam.

The function quiz should take the questions as a parameter, so that you can call it like this:

quiz(secplus_questions)
quiz(popular_rock_bands_questions)

It could be implemented like this:

import random
from typing import Dict

def quiz(questions: Dict[str, str]):

    shuffled_keys = random.shuffle(list(questions.keys()))
    for key in shuffled_keys:
        question = questions[key]

        # TODO: ask the question, compare the answer


Failing fast by raising an exception with a custom message This is a script that utilizes lazy exception throwing. What I do is decorate a function that might throw an exception to throw an exception with a formatted string.

from functools import wraps

def on_fail(message):
    def on_fail_dec(func):
        @wraps(func)
        def on_fail_wrapper(*args, **kw):
            ret = None
            success = True
            message_addon = ""
            try:
                ret = func(*args, **kw)
            except Exception as e:
                success = False
                message_addon = str(e)
            # raise Exception if failed
            if not success:
                raise Exception("{0}\nBecause: {1}".format(message, message_addon))

            return ret
        return on_fail_wrapper
    return on_fail_dec

# Usage :

@on_fail("Division Failed")
def divide(a, b):
    return a / b

test_cases = [(1, 2), (20, 5), (3, 0)]

for a,b in test_cases:
    print("Trying to divide {0} from {1}".format(a, b))
    print(divide(a, b))

Output:

Trying to divide 1 from 2
0.5
Trying to divide 20 from 5
4.0
Trying to divide 3 from 0
Traceback (most recent call last):
  File "<string>", line 30, in <module>
  File "<string>", line 16, in on_fail_wrapper
Exception: Division Failed
Because: division by zero

Instead of handling just "Division Failed" you should extend your function to handle any possible error, plus it's better if you pass the exception object along with the message to the decorator as well, by doing this you'll know exactly which error you're going to handle because a except Exception will handle any Exception not just what we actually intended it to.
Second there's no need to use flag variables here, we can simply return the return value from the try-block and in case of exception we re-raise the exception after it is caught.

Code:

from functools import wraps

def on_fail(exception, message):
    def on_fail_dec(func):
        @wraps(func)
        def on_fail_wrapper(*args, **kw):
            message_addon = ""
            try:
                # do something before
                ret = func(*args, **kw)
                # do something after
                return ret
            except exception as e:
               message_addon = str(e)
            raise exception("{0}\nBecause: {1}".format(message, message_addon))
        return on_fail_wrapper
    return on_fail_dec

@on_fail(ZeroDivisionError, "Division Failed")
def divide(a, b):
    return a / b

test_cases = [(1, 2), (20, 5), (3, 0)]

for a, b in test_cases:
    print("Trying to divide {0} from {1}".format(a, b))
    print(divide(a, b))

Note that in above code I didn't use something simple like:

except exception as e:
    raise exception("{0}\nBecause: {1}".format(message, e))

That's because Python 3 introduced something called exception chaining, which means if an exception occurs while handling another exception then the previous exception still remains available to us(Read PEP 3134 - PEP 3134 -- Exception Chaining and Embedded Tracebacks), but this is not useful here as it will end up raising something like:

Trying to divide 3 from 0
Traceback (most recent call last):
  File "/home/ashwini/py/so.py", line 10, in on_fail_wrapper
    ret = func(*args, **kw)
  File "/home/ashwini/py/so.py", line 20, in divide
    return a / b
ZeroDivisionError: division by zero

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/ashwini/py/so.py", line 26, in <module>
    print(divide(a, b))
  File "/home/ashwini/py/so.py", line 13, in on_fail_wrapper
    raise exception("{0}\nBecause: {1}".format(message, e))
ZeroDivisionError: Division Failed
Because: division by zero


Nice design of my hash service class I would like to know, if my design of this class is good OOP.
Should I implement for every hash algorithm a separate class?
I'm asking this, because the HashService can be used with a KeyedHashAlgorithm which wouldn't work correctly?!

public class HashService : IDisposable
{
    public HashService(HashAlgorithm algorithm)
    {
        HashAlgorithm = algorithm;
        Encoder = Encoding.UTF8;
    }

    protected HashAlgorithm HashAlgorithm { get; set; }

    /// <summary>
    /// The size, in bits, of the computed hash code.
    /// </summary>
    public int HashSize
    {
        get { return HashAlgorithm.HashSize; }
    }

    /// <summary>
    /// Gets or sets the encoding for strings.
    /// </summary>
    public Encoding Encoder { get; set; }

    public string ComputeHash(string input)
    {
        byte[] bytes = Encoder.GetBytes(input);
        byte[] hash = ComputeHash(bytes);
        return ToHex(hash); // method impl. omitted
    }

    /// <summary>
    /// Computes the hash value for the specified byte array
    /// </summary>
    /// <param name="buffer">The input to compute the hash code for.</param>
    /// <returns>The computed hash code.</returns>
    public byte[] ComputeHash(byte[] buffer)
    {
        return HashAlgorithm.ComputeHash(buffer);
    }

    public byte[] ComputeHash(byte[] buffer, int offset, int count)
    {
        return HashAlgorithm.ComputeHash(buffer, offset, count);
    }

    public byte[] ComputeHash(Stream inputStream)
    {
        return HashAlgorithm.ComputeHash(inputStream);
    }

    #region Implementation of IDisposable

    // omitted

    #endregion

    // ReSharper disable InconsistentNaming
    public static HashService CreateMd5()
    {
        return new HashService(new MD5CryptoServiceProvider());
    }

    public static HashService CreateRIPEMD160()
    {
        return new HashService(new RIPEMD160Managed());
    }

    public static HashService CreateSHA256()
    {
        return new HashService(new SHA256Managed());
    }

    public static HashService CreateSHA384()
    {
        return new HashService(new SHA384Managed());
    }

    public static HashService CreateSHA512()
    {
        return new HashService(new SHA512Managed());
    }

    public static HashService CreateSHA1()
    {
        return new HashService(new SHA1Managed());
    }

    public static HashService CreateCrc32()
    {
        return new HashService(new Crc32Managed());
    }

    // ReSharper restore InconsistentNaming
}

Overall, its nice code.  Easy to read and good use of white space.

I would move the creating of the HashService to a Factory class though.  The way you are using it, I don't think it belongs in the class.

public class HashServiceFactory
{
    public Create(HashAlgorithm algorithm)
    {
        return new HashService(algorithm);
    }
}

// Usage

var factory = new HashServiceFactory();

var sha256Managed = factory.Create(new SHA256Managed());
var md5CryptoServiceProvider = factory.Create(new MD5CryptoServiceProvider());


Neatly Transforming Anorm ResultSet Into Map[Parent , (Set[Child1], Set[Child2]))] I have a table Bill containing Bills with 0...n relationships to Material and Activity (line items on the bill).

My SQL query is essentially bill left join material left join activity.

I have the following insanely tangled code to transform the results into a Map[Bill, (HashSet[Activity], HashSet[Material])].

What's wrong with this code?  Is this whole approach wrong?  I'm new to Scala and the functional world, but it doesn't seem like this could be the right way.

 SQL(
          """
            |SELECT
            | b.id, b.dateDue
            | ,a.id as activityId, a.notes, a.startTime, a.endTime
            | ,m.id as materialId, m.cost, m.description, m.storageLocation
            |FROM Bill b
            |LEFT JOIN Activity a ON a.bill_id = b.id
            |LEFT JOIN Material m ON m.bill_id = b.id
            |WHERE b.id = {id}
          """.stripMargin)
        .on("id" -> id)
        .as(betterParser *)
        .groupBy( x=> (x._1, x._2))
        .map( q => q._1 ->
                q._2.foldLeft( (HashSet[Activity](), HashSet[Material]()) )
                ( (acc, item) =>  (acc._1 + item._3, acc._2 + item._4)))

betterParser transforms each row from the query into a (Long, DateTime, Activity, Material) where the  (Long, DateTime) portion are overall bill poperties.

 val betterParser = {
    billParser ~ activityParser ~ materialParser map {
      case bill ~ activity ~ material =>
       (bill._1, bill._2, activity, material)
   }
 }

I was hoping someone would review your scala but I don't think it's going to happen. So I thought I would at least say something about sql.

Alias names

Table aliases are handy especially for very long table names, however I feel that your naming is too minimal. Single-letter aliases tell Mr. Maintainer nothing about what the table means, and it can get confusing if you have multiple tables starting with the same letter. b, a, m are not very useful. Instead I would use Bill, Act, Mat or something similar.

Formatting

Your formatting is not bad, I see you've kept columns from the same table on the same line and that's good; but inline SQL can be a bit difficult to read especially when you introduce column aliases. My recommendation is to use a line break after each column you SELECT. I also noticed a bit of inconsistency in capitalization of key words.

This:

 SQL(
          """
            |SELECT
            | b.id, b.dateDue
            | ,a.id as activityId, a.notes, a.startTime, a.endTime
            | ,m.id as materialId, m.cost, m.description, m.storageLocation
            |FROM Bill b
            |LEFT JOIN Activity a ON a.bill_id = b.id
            |LEFT JOIN Material m ON m.bill_id = b.id
            |WHERE b.id = {id}
          """.stripMargin)

I would instead write like this:

 SQL(
          """
            |SELECT Bill.id
            | ,Bill.dateDue
            | ,Act.id AS activityId
            | ,Act.notes
            | ,Act.startTime
            | ,Act.endTime
            | ,Mat.id AS materialId
            | ,Mat.cost
            | ,Mat.description
            | ,Mat.storageLocation
            |FROM Bill
            |LEFT JOIN Activity AS Act ON Act.bill_id = Bill.id
            |LEFT JOIN Material AS Mat ON Mat.bill_id = Bill.id
            |WHERE Bill.id = {id}
          """.stripMargin)


Plots of prior and posterior distributions for a model I have an exercise that ask to plot the prior and posterior distribution in the Poisson-Gamma model. I did it (I think it's correct) inspired
in the answer of this question https://stats.stackexchange.com/questions/70661/how-does-the-beta-prior-affect-the-posterior-under-a-binomial-likelihood

Is there anything that can improve the code?

colors = c("red","blue","green","orange","purple")

n = 10

lambda = .2

x = rpois(n,lambda)
grid = seq(0,7,.01)

alpha = c(.5,5,1,2,2)
beta = c(.5,1,3,2,5)

plot(grid,grid,type="n",xlim=c(0,5),ylim=c(0,4),xlab="",ylab="Prior Density",
     main="Prior Distributions", las=1)
for(i in 1:length(alpha)){
  prior = dgamma(grid,shape=alpha[i],rate=1/beta[i])
  lines(grid,prior,col=colors[i],lwd=2)
}

legend("topleft", legend=c("Gamma(0.5,0.5)", "Gamma(5,1)", "Gamma(1,3)", "Gamma(2,2)", "Gamma(2,5)"),
       lwd=rep(2,5), col=colors, bty="n", ncol=3)

for(i in 1:length(alpha)){
  dev.new()
  plot(grid,grid,type="n",xlim=c(0,5),ylim=c(0,10),xlab="",ylab="Density",xaxs="i",yaxs="i",
       main="Prior and Posterior Distribution")

  alpha.star = alpha[i] + sum(x)
  beta.star = beta[i] + n
  prior = dgamma(grid,shape=alpha[i],rate=1/beta[i])
  post = dgamma(grid,shape=alpha.star,rate=beta.star)

  lines(grid,post,lwd=2)
  lines(grid,prior,col=colors[i],lwd=2)
  legend("topright",c("Prior","Posterior"),col=c(colors[i],"black"),lwd=2)

}

There was a couple of errors in the code as originally posted. The code was OK as far as base R goes.

First thing I did was run styler and then lintr on your code; these two things help clean up the coding style in your scripts.

That does things like this:

colors = c("red","blue","green","orange","purple")

# changed to (spaces / idiomatic assignment):
colors <- c("red", "blue", "green", "orange", "purple")

Then I changed your 1:length(alphas) to seq_along(alphas). The latter is a bit safer since the former can fail with empty input.

Then I replaced your 5-separate prior/posterior plots with a single plot that contains 5 panels. This makes it easier to compare the appropriateness of the different priors. To do this, I removed your dev.new()s, added a call to par(mfrow = c(number_of_rows, number_of_columns)) and obviously, tidied this up afterwards (returning to a 1x1 grid)

par(mfrow = c(2, ceiling(length(alpha) / 2)))

for (i in seq_along(alpha)) {
    # removed dev.new()
   ... plotting code ...
   )
 }

par(mfrow = c(1, 1))

Then I cleaned up your experimental data and your prior-parameters / plotting parameters; they were all winding around each other. I also renamed your alpha / beta vectors - in R, these correspond to the shape and rate parameters that are passed into dgamma:

# ---- experimental data

num_observations <- 10

lambda <- .2

x <- rpois(num_observations, lambda)

# ---- prior parameters

# assumed 'beta' was a rate parameter
# - this, since there was confusion in the parameterisation of dgamma():
#   - early section used rate = 1 / beta[i];
#   - later section used rate = beta[i]; and
#   - definition of beta_star = beta[i] + n; implied beta was definitely a rate

shape <- c(.5, 5, 1, 2, 2)
rate <- c(.5, 1, 3, 2, 5)

# ---- plotting parameters

colors <- c("red", "blue", "green", "orange", "purple")

# ---- search parameters

grid <- seq(0, 2, .01)

Then I made a function to do your prior-comparison stuff (the first set of plots). Any parameters that were to be passed through to plot were passed in using the ... argument.

# ---- comparison of the prior distributions

plot_priors <- function(grid, shapes, rates, colors,
                        legend_text, lwd = 2, ...) {
  plot(grid, grid, type = "n", ...)

  for (i in seq_along(shape)) {
    prior <- dgamma(grid, shape = shape[i], rate = rate[i])
    lines(grid, prior, col = colors[i], lwd = lwd)
  }

  legend(
    "topleft",
    legend = legend_text, lwd = lwd, col = colors, bty = "n", ncol = 2
  )
}

This can be called like:

plot_priors(
  grid, shape, rate, colors,
  legend_text = paste0("Gamma(", c("0.5,0.5", "5,1", "1,3", "2,2", "2,5"), ")"),
  xlim = c(0, 1), ylim = c(0, 4), xlab = "", ylab = "Prior Density",
  main = "Prior Distributions", las = 1
)

It's useful to split your computations away from your plotting code - so I extracted the code you used to compute the posterior params:

compute_posterior_parameters <- function(observations,
                                         prior_shape,
                                         prior_rate) {
  list(
    shape = prior_shape + sum(observations),
    rate = prior_rate + length(observations)
  )
}

Then I pulled the plotting code for your prior/posterior comparisons into a function (similarly to the above)

plot_prior_post_comparison <- function(
                                       observations,
                                       grid, shapes, rates, colors,
                                       lwd = 2,
                                       ...) {
  # make a grid for plotting
  par(mfrow = c(2, ceiling(length(shapes) / 2)))

  for (i in seq_along(shapes)) {
    # details of the prior and post distributions
    posterior_params <- compute_posterior_parameters(
      observations,
      prior_shape = shapes[i], prior_rate = rates[i]
    )
    prior <- dgamma(
      grid,
      shape = shapes[i],
      rate = rates[i]
    )
    post <- dgamma(
      grid,
      shape = posterior_params$shape,
      rate = posterior_params$rate
    )

    # plotting code
    plot(grid, grid, type = "n", ...)
    lines(grid, post, lwd = lwd)
    lines(grid, prior, col = colors[i], lwd = lwd)
    legend("topright",
      c("Prior", "Posterior"),
      col = c(colors[i], "black"), lwd = lwd
    )
  }

  # revert the plotting grid back to 1x1
  par(mfrow = c(1, 1))
}

note that the par calls, which change the plotting grid are all nested inside the function, so any subsequent plots should be unaffected.

Then I called that function:

# ---- prior/posterior comparison

plot_prior_post_comparison(
  observations = x,
  grid = grid, shapes = shape, rates = rate, colors = colors,
  xlim = c(0, 1), ylim = c(0, 10), xlab = "", ylab = "Density",
  xaxs = "i", yaxs = "i",
  main = "Prior and Posterior Distribution"
)

Then I put all the functions at the start and all the calls at the end of a script:

The full code:

# ---- comparison of the prior distributions

plot_priors <- function(grid, shapes, rates, colors,
                        legend_text, lwd = 2, ...) {
  plot(grid, grid, type = "n", ...)

  for (i in seq_along(shape)) {
    prior <- dgamma(grid, shape = shape[i], rate = rate[i])
    lines(grid, prior, col = colors[i], lwd = lwd)
  }

  legend(
    "topleft",
    legend = legend_text, lwd = lwd, col = colors, bty = "n", ncol = 2
  )
}

# ---- prior:posterior analysis

compute_posterior_parameters <- function(observations,
                                         prior_shape,
                                         prior_rate) {
  list(
    shape = prior_shape + sum(observations),
    rate = prior_rate + length(observations)
  )
}

plot_prior_post_comparison <- function(
                                       observations,
                                       grid, shapes, rates, colors,
                                       lwd = 2,
                                       ...) {
  # make a grid for plotting
  par(mfrow = c(2, ceiling(length(shapes) / 2)))

  for (i in seq_along(shapes)) {
    # details of the prior and post distributions
    posterior_params <- compute_posterior_parameters(
      observations,
      prior_shape = shapes[i], prior_rate = rates[i]
    )
    prior <- dgamma(
      grid,
      shape = shapes[i],
      rate = rates[i]
    )
    post <- dgamma(
      grid,
      shape = posterior_params$shape,
      rate = posterior_params$rate
    )

    # plotting code
    plot(grid, grid, type = "n", ...)
    lines(grid, post, lwd = lwd)
    lines(grid, prior, col = colors[i], lwd = lwd)
    legend("topright",
      c("Prior", "Posterior"),
      col = c(colors[i], "black"), lwd = lwd
    )
  }

  # revert the plotting grid back to 1x1
  par(mfrow = c(1, 1))
}

# ----

# ---- experimental data

num_observations <- 10

lambda <- .2

x <- rpois(num_observations, lambda)

# ---- prior parameters

# assumed 'beta' was a rate parameter
# - this, since there was confusion in the parameterisation of dgamma():
#   - early section used rate = 1 / beta[i];
#   - later section used rate = beta[i]; and
#   - definition of beta_star = beta[i] + n; implied beta was definitely a rate

shape <- c(.5, 5, 1, 2, 2)
rate <- c(.5, 1, 3, 2, 5)

# ---- plotting parameters

colors <- c("red", "blue", "green", "orange", "purple")

# ---- search parameters

grid <- seq(0, 2, .01)

# ---- comparison of priors

plot_priors(
  grid, shape, rate, colors,
  legend_text = paste0("Gamma(", c("0.5,0.5", "5,1", "1,3", "2,2", "2,5"), ")"),
  xlim = c(0, 1), ylim = c(0, 4), xlab = "", ylab = "Prior Density",
  main = "Prior Distributions", las = 1
)

# ---- prior/posterior comparison

plot_prior_post_comparison(
  observations = x,
  grid = grid, shapes = shape, rates = rate, colors = colors,
  xlim = c(0, 1), ylim = c(0, 10), xlab = "", ylab = "Density",
  xaxs = "i", yaxs = "i",
  main = "Prior and Posterior Distribution"
)

I don't thnk its perfect - the plotting and calculation steps are still pretty tied together - but it should be more easy to add extra pairs of rate/shape values into your prior distributions, for example.

One place the code coould be further improved is by passing in a data.frame where each row contains the shape/rate/colour values and any annotations for a given prior. The functions contain too many arguments at the moment, and this would fix that (and guarantee that there is a shape for each rate).


Sort Facebook Messenger contacts by idle time I wrote a UserScript that auto-sorts contacts by offline time using the MutationObserver API. I tried to make it as DRY as I could think, and I also tried to make my code as straight-forward as possible, commenting thoroughly. Please let me know any improvements I can make.

// ==UserScript==
// @name         Facebook Messenger Chat Sorter
// @namespace    http://tampermonkey.net/
// @version      0.1
// @description  Auto-sort contacts by offline time
// @author       Patrick Roberts
// @match        https://www.facebook.com/*
// @grant        none
// ==/UserScript==

(function() {
  'use strict';

  // mutation observer to locate chat sidebar
  var exists = new MutationObserver(function(mutations) {
    // if chat sidebar found in any mutations
    if (mutations.some(function(mutation) {
      // check if element with class is added
      if (mutation.type === 'childList') {
        // return if any added element is the chat sidebar
        return Array.from(mutation.addedNodes).some(initFbChatSidebar);
      // check if class is changed in existing element
      } else {
        // return if changed element is the chat sidebar
        return initFbChatSidebar(mutation.target);
      }
    })) {
      // then de-initialize this observer
      exists.disconnect();
      exists.takeRecords();
    }
  });

  // start locating sidebar
  exists.observe(document.body, { attributes: true, childList: true, subtree: true, attributeFilter: ['class'] });

  // returns if any element is the chat sidebar, and inits it
  function initFbChatSidebar(el) {
    if (el instanceof HTMLElement && el.classList.contains('fbChatSidebar')) {
      initChatSort(el.querySelector('ul'));

      return true;
    }

    return false;
  }

  // initializes auto-sorting
  function initChatSort(ul) {
    var li = ul.children;

    // observer to sort contacts
    var observer = new MutationObserver(sort);

    // start sorting whenever list is mutated
    observer.observe(ul, { childList: true, characterData: true, subtree: true });

    // also sort on initialization since list is already there
    sort();

    function sort() {
      // invoke native sorting algorithm
      Array.from(li).sort(function(la, lb) {
        // get offline times
        var a = getOfflineTime(la);
        var b = getOfflineTime(lb);

        // compare offline times
        return a - b;
      }).forEach(function(li) {
        // add list items to list in sorted order
        ul.appendChild(li);
      });

      // remove mutations from queue caused by sorting
      observer.takeRecords();
    }
  }

  // parse offline time from contact list item
  function getOfflineTime(li) {
    var str = li.querySelector('[class$="on"] [class$="-"]').textContent;
    var num, unit;

    num = +str.slice(0, -1);
    unit = str.slice(-1);

    // multiply time by 60 if hour
    if (unit === 'h') {
      num *= 60;
    // multiply time by 60 * 24 if day
    } else if (unit === 'd') {
      num *= 60 * 24;
    }

    return num;
  }
})();

Normally I would suggest pulling up the functions. Though they get hoisted by JS, it's nice from a readability point of view that you know they exist first before code tries to use them.

// if chat sidebar found in any mutations
if (mutations.some(function(mutation) {

This is a huuuge if statement. You might want to consider pulling out that callback function. It doesn't use mutations anyways.

if (mutation.type === 'childList') {
    // return if any added element is the chat sidebar
    return Array.from(mutation.addedNodes).some(initFbChatSidebar);
  // check if class is changed in existing element
  } else {
    // return if changed element is the chat sidebar
    return initFbChatSidebar(mutation.target);
  }

Can be simplified into:

var isChildListMutation = mutation.type === 'childList';
return isChildListMutation  ? Array.from(mutation.addedNodes).some(initFbChatSidebar);
                            : initFbChatSidebar(mutation.target);

// start locating sidebar
exists.observe(document.body, { attributes: true, childList: true, subtree: true, attributeFilter: ['class'] });

Consider breaking the config into newlines. As a single line, it isn't really readable.

  Array.from(li).sort(function(la, lb) {
    // get offline times
    var a = getOfflineTime(la);
    var b = getOfflineTime(lb);

    // compare offline times
    return a - b;
  }).forEach(function(li) {
    // add list items to list in sorted order
    ul.appendChild(li);
  });

Consider appending to a document fragment instead of directly to the DOM. Only after you append every li to the fragment, append the fragment to the ul.

var num, unit;

num = +str.slice(0, -1);
unit = str.slice(-1);

Consider putting var on the assignment operation instead. Save an extra line. Additionally, your num is not guarded against NaN.


Creating text without repetition The basic idea is not to allow the same text and add +1 at the end.

Imagine that I have a datagrid (WPF) with names, and every time I click the add button, I will put a new name on the list automatically by running this method to ensure that no two names are alike.

My standard used: if there is repeated text, add to the end "(" + count + ")", similar to Windows Explorer.

Are there ways to make this using LINQ or Regex?

private void Test()
{
    var result = CreateText("t", new string[] { "t", "t(1)", "t(2)", "t(3)", "t(4)", "t(5)", "t(6)", "t(7)", "t(8)", "t(9)", "t(10)" });
    Assert.IsTrue(result == "t(11)");
}

public static String CreateText(string newText, String[] texts)
{
    int addCount = 0;
    foreach (String text in texts)
    {
        if (text.IndexOf(newText) == 0)
        {
            if (text == newText && addCount == 0)
            {
                addCount = 1;
                continue;
            }
            else
            {
                if (text.LastIndexOf('(') == newText.Length && text.LastIndexOf(')') == text.Length - 1)
                {
                    try
                    {
                        var initial = text.LastIndexOf('(') + 1;
                        var size = text.LastIndexOf(')') - initial;
                        int count = Int16.Parse(text.Substring(initial, size));
                        if (addCount <= count)
                        {
                            addCount = count + 1;
                        }
                    }
                    catch (Exception)
                    {
                        addCount = 0;
                    }
                }
            }
        }
    }
    if (addCount > 0)
    {
        newText += "(" + addCount + ")";
    }
    return newText;
}

Here is shorter method with regex

    private static String CreateText(string textIdentifier, IEnumerable<string> texts)
    {
        const int defaultNumber = 1;

        Regex regex = new Regex(@"\(\s*(?<num>\d+)\s*\)");

        var numbers =
            texts.Select(p => p.Trim()).Where(p => p.StartsWith(textIdentifier))
            .Select(str => { var m = regex.Match(str); return m.Success ? int.Parse(m.Groups["num"].Value) + 1 : defaultNumber; });

        int max = numbers.Any() ? numbers.Max() : 0;

        return max == 0 ? textIdentifier : string.Format("{0}({1})", textIdentifier, max);
    }

+1 to @dreza for tests ;)


Ruby Linked List implementation I've made a Ruby linked list data structure class and a lookup method. I know that lookup will only fetch the first occurrence and I'm ok with that.

Is it a bad implementation? Why is it bad? What can I improve to make it more idiomatic?

# linear collection of data elements, in which linear order is not given by
# their physical placement in memory. Instead, each element points to the next.
class LinkedList
  attr_accessor :value, :next

  def lookup(lookup_value)
    return self if value.eql? lookup_value
    return self.next.lookup(lookup_value) unless self.next.nil?
  end
end

Test code

root = LinkedList.new
root.value = 5

node1 = LinkedList.new
node1.value = 10

node2 = LinkedList.new
node2.value = 15

node3 = LinkedList.new
node3.value = 12

root.next = node1
node1.next = node2
node2.next = node3

p root.lookup(2) # nil
p root.lookup(15) # #<LinkedList:0x007f83db0a95b8 @value=15, @next=#<LinkedList:0x007f83db0a9590 @value=12>>
p root.lookup(12) # #<LinkedList:0x007fcbe91196a0 @value=12>

Looks pretty good to me. You don't need the explicit return as the last statement of your lookup method. Also, in newer versions of Ruby (2.3+ I believe), you don't need the explicit nil check--instead, you can use the Safe Navigation Operator (&.).

  def lookup(lookup_value)
    return self if value.eql? lookup_value
    self.next&.lookup(lookup_value)
  end


Edis: Emacs driven indentation script Motivation

I recently picked up Emacs. As part of the process, I've started to configure the editor to fit my needs with Emacs Lisp. The Elisp indentation rules are still foreign to me, so I pretty much depend on Emacs' electric-indent-mode to work. There's always C-x h and C-M-\ to the rescue, though.

That being said, I recently came across a user who cannot use this functionality, as the Emacs user interface in its default state is apparently incompatible with their screen reader. Still, I think that Emacs' Lisp indentation works great, so I tried to make it more accessible. And since I also needed some experience with Lisp, why not write it in Elisp?

Emacs driven indentation script

The Emacs driven indentation script (Edis) is single script to indent files with Emacs' indentation logic. It has no requirement except an Emacs installation and should—in theory—work on all supported platforms and in most Emacs versions, although I only tested Linux and Emacs 26 so far.

Files must be given as command line arguments. It's a script, after all, and doesn't use any graphical interface:

/path/to/edis file1.elisp file2.elisp file3.elisp …

On Windows, a small wrapper is necessary, since it doesn't support shebangs, but that's probably just two lines of CMD or PowerShell.

Mistyped file names should get reported, but other than that, there shouldn't be any restriction on the number of files unless enforced by the OS, shell or Emacs. In theory, you can even use TRAMP to indent remote files, but I haven't tested that exhaustively (try edis /ssh:server:/path/to/file if you're interested).

#!/usr/bin/emacs --script
(defun edis-indent-file (filename)
  "Re-indents a given file"
  (interactive "fFile to indent")
  (cond
   ((file-exists-p filename)
    (find-file filename)
    (indent-region (point-min) (point-max))
    (save-buffer))
   (t
    (message "File not found (need existing file): %s" filename))))

(mapc 'edis-indent-file argv)

The code has been indented with itself. As usual, it will respect file and global variables, so you can specify for example (setq lisp-indent-offset 2) in your init.el or use file local variables, for example:

;; -*- lisp-body-indent: 4 ; lisp-indent-offset: 2 -*-

This is the first Emacs Lisp code I've written so far that's outside my
init.el (where I mostly use use-package, to be honest). Since Emacs is, after all, a text editor, all necessary tools seem to be at hand, so the code isn't that long.

I haven't read the complete Elisp manual yet and got the functions from the C-h k or quick searches in the manual, so I'm not sure whether the code is idiomatic Elisp or completely fails any good practice, so feel free to comment on any part of the code.

One of the pitfalls with any language using formatting constructs you're not that familiar with, is that it becomes an unreadable mess. Your code though is properly formatted, properly indented (it would be poor style for an indentation script to be badly indented itself, wouldn't it?) and even self-documenting. On every line, it's perfectly clear where we are, what's going on and how it's done.

If we're really going to nitpick, the documentation of Emacs Lisp uses the following indentation style for conditionals:

(cond ((file-exists-p filename)
        (find-file filename)
        (indent-region (point-min) (point-max))
        (save-buffer))
      (t (message "File not found (need existing file): %s" filename))))

Which slightly differs from yours, and is more in line with the rest of the language. But I can understand a preference for either.


Script to calculate width, and either resize, show, or hide HTML menu I have created a script that handles the responsive liabilities of my HTML menu. The menu will have a variable amount of <li class="menu-item">, resulting in a variable total width. I have 3 functions.

This function is responsible for looping through each li element and calculating the greatest width of the set of elements.

function resize_menu() {

    var maxWidth = -1;

    $(".menu-item").each(function() {
        maxWidth = maxWidth > $(this).width() ? maxWidth : $(this).width();
    });

    $(".menu-item").each(function() {
        $(this).width(maxWidth);
    });
}

This function is responsible for displaying the mobile menu, and hiding the standard menu.

function mobile_menu() {
    $("#mobile-menu").css("display", "block");

    $(".menu-primary-menu-container").css("display", "none");
}

This function is responsible for showing the standard menu, and hiding the mobile menu, invert of the above.

function show_menu() {
    $("#mobile-menu").css("display", "none");

    $(".menu-primary-menu-container").css("display", "initial");
}

Now to execute functions above, I have created instances in which they need to be fired.

$( window ).load(function() {
    //declare variables
    var resizeTimer;
    var maxWidth = -1;
    var count = $(".menu-item").length;

    // Loop menu items, and retrieve greatest width, to be multiplied by count
    $(".menu-item").each(function() {
        maxWidth = maxWidth > $(this).width() ? maxWidth : $(this).width();
    });

    // Only resize li elements if screen width is greater than menu width
    if ($(window).width() > maxWidth * count) {
        resize_menu();
    } else {
    // or else show the mobile menu
        mobile_menu();
    };

    // On a resize (delayed for performance)
    $(window).on('resize', function(e) {
        clearTimeout(resizeTimer);
        resizeTimer = setTimeout(function() {
            // again, if window is less than menu, show mobile
            if ($(window).width() < maxWidth * count) {
                mobile_menu();
            } else {
            // or else, show regular menu
            // Note: resize, incase screen was loaded smaller than menu
                resize_menu();
                show_menu();
            };
        }, 50);
    });

    // if the mobile menu icon is clicked, show menu
    $("#mobile-menu").click(function() {
        show_menu();
    });
});

This code does work, however there are some performance issues. When I resize the window, some times the hiding of the regular menu, and showing of the mobile is delayed, caused probably by the timeout I set, is it worth getting rid of? Also, I know this code is wet, lots of repetition. I could use advice on that. Finally, how can I store my selectors in variables and use them throughout the script?

Suggestion: The Math.max method is shorter than ternary operator:

maxWidth = Math.max(maxWidth, $(this).width());
maxWidth = maxWidth > $(this).width() ? maxWidth : $(this).width();


Clean code attempt at ATM problem on codechef.com The problem asks you to take an integer (debit amount) and a double (credit or initial balance amount) and process the requested debit verifying that 1 it's a multiple of the minimum denomimation amount of $5 and that it's also smaller than the credit/balance.  If either is untrue, it is supposed to return the initial deposit amount otherwise it will return the new balance.

Full problem description

I have created 3 objects for this problem:

Transaction - This object reads in the two initial values given and then is used in ATM
ATM - Takes the transaction and applies them to the account and then displays the new balance.
Account - This object keeps track of the current account balance and updates the balance if the ATM passes it a value.

Limitations:

I understand that it can only process a single account, but that is more a limitation set by the problem description than it is me not accounting for multiple accounts.  Also no error is returned if the balance cannot be updated, but it is not a requirement.  I also understand I made a mountain out of a molehill with this problem as it can be solved by much less code.

In what ways can I improve this code other than the limitations mentioned?

#include <istream>
#include <iostream>
#include <iomanip>

class Account {
public:
    Account()
        :   mBalance(0.0)
    {}

    void updateBalance(double transaction) {
        mBalance += transaction;
    }

    double getBalance() {
        return mBalance;
    }

private:
    double mBalance;
};

class Transaction {
public:
    Transaction()
        :   mDebit(0)
        ,   mCredit(0.0)
    {}

    int getDebit() {
        return mDebit;
    }

    double getCredit() {
        return mCredit;
    }

    friend std::istream& operator>>(std::istream& input, Transaction& transaction) {
        input >> transaction.mDebit;
        input >> transaction.mCredit;
        return input;
    }

private:
    int     mDebit;
    double  mCredit;
};

class ATM {
public:
    ATM()
        :   mAccount()
        ,   mMinDenomination(5)
        ,   kWithdrawal_fee(0.50)
    {}

    void processTransaction(Transaction& transaction) {
        credit(transaction);
        debit(transaction);
    }

    void displayBalance() {
        std::cout << mAccount.getBalance() << '\n';
    }

private:
    Account             mAccount;
    int                 mMinDenomination;
    const double        kWithdrawal_fee;

    bool debit(Transaction& transaction) {
        if(isWithdrawable(transaction.getDebit())){
            mAccount.updateBalance(-1*(transaction.getDebit() + kWithdrawal_fee));
            return true;
        }
        return false;
    }

    void credit(Transaction& transaction) {
        if(transaction.getCredit() > 0) {
            mAccount.updateBalance(transaction.getCredit());
        }
    }

    bool isWithdrawable(int transaction) {
        if(transaction % mMinDenomination == 0) {
            return mAccount.getBalance() >= transaction + kWithdrawal_fee;
        }
        return false;
    }
};

int main() {
    std::iostream::sync_with_stdio(false);
    std::cout << std::setprecision(2) << std::fixed;

    Transaction transaction;
    ATM         atm;

    std::cin >> transaction;
    atm.processTransaction(transaction);
    atm.displayBalance();

    return 0;
}

This looks quite readable and easy to follow.  I just have a few things that have stuck out to me:

It would not be good to use a floating-point type for currency, and it's especially worse for a more accurate type like a double.  For instance, you can wind up with a value like 0.0001, which is not an ideal value to deal with.  Unfortunately, C++ doesn't have a standard library to deal with this, such as Java's BigDecimal.  You need an integer type for this, which avoids these problems.

Refer to this, this and this for more info.  There are already many resources on this very topic.
Your "getters" should be const as they're not supposed to modify data members:

int getSomeMember() const {
    return someMember;
}

mMinDenominations should also be const for the same reason.  Initializer lists do also allow initialization of const members (be aware that constructors cannot do this).

Moreover, consider redesigning this to avoid getters.  They (and also setters) are generally bad for encapsulation as they expose implementation details.  You may not even need them here, either.
You can replace displayBalance() with an overload of operator<<:

friend std::ostream& operator<<(std::ostream& out, ATM const& obj) {
    return out << obj.mAccount.getBalance() << '\n';
}

Since mAccount is private, this will have to be defined within the class.  This will still work, even if you remove the getters.


Set default value for NULL in datatable I have the following codes, which I created as extension for datatable in my project.
It actually worked. Just wonder if there is any optimization can be made through. Thanks. =)

<Extension()>
Public Function HasNull(ByVal dataTable As DataTable) As Boolean
    For Each column As DataColumn In dataTable.Columns
        If dataTable.Rows.OfType(Of DataRow)().Any(Function(r) r.IsNull(column)) Then
            Return True
        End If
    Next
    Return False
End Function

<Extension()>
Public Function SetDefaultForNull(ByVal dataTable As DataTable) As DataTable
    For Each row As DataRow In dataTable.Rows
        For Each col As DataColumn In dataTable.Columns
            Dim value As Object = row(col)
            If IsDBNull(value) Then
                Dim dataType As String = col.DataType.ToString

                Select Case dataType
                    Case "System.DateTime"
                        value = New DateTime
                    Case "System.Decimal", "System.Int16", "System.Int32", "System.Int64"
                        value = 0
                    Case "System.String"
                        value = String.Empty
                    Case "System.Boolean"
                        value = False
                    Case Else
                        value = 0
                End Select
                row(col) = value
            End If
        Next
    Next
    Return dataTable
End Function

A couple of small points.

Single letter identifiers and variable names are the devil incarnate. Replace r with something meaningful.
There's not much point in using a variable for col.DataType.ToString. You only use the variable dataType once, so it would be perfectly acceptable to get rid of a line of a code and do this instead.

Select Case col.DataType.ToString
    Case "System.DateTime"
        value = New DateTime

I also don't see much benefit to the value variable. You could just set row(col) = to whatever you would set value to.
There's no reason to explicitly check for Case "System.Decimal", "System.Int16", "System.Int32", "System.Int64". It's taken care of in the Else case.

Select Case col.DataType.ToString
    Case "System.DateTime"
        row(col) = New DateTime
    Case "System.String"
        row(col) = String.Empty
    Case "System.Boolean"
        row(col) = False
    Case Else
        row(col) = 0
End Select


Optimize function that returns length of a subarray I did an exercise today on where the task is to write a function that returns the length of the longest subarray where the difference between the smallest and biggest value in the subarray is no more than 1. I had a solution that works but it's too slow to pass the tests that have big arrays. What is it about my solution that makes it very slow with huge arrays?
def longestSubarray(arr):
    longest = 0
    arr_len = len(arr)
    for i in range(arr_len):
        for j in range(i, arr_len):
            sub = arr[i: j+1]
            if max(sub) - min(sub) > 1:
                continue
            length = len(sub)
            if length > longest:
                longest = length
    return longest

Specific suggestions:

In many situations it would probably be useful to return sub rather than len(sub). This makes the code somewhat simpler and gives the caller more information. Depending on the context that may or may not be useful, but it simplifies the code, which makes it easier to focus on the core issue.
The code iterates over all sub-lists, calculating min and max of each of them. This is a lot of duplicate work. You can instead iterate only once over the list (with some backtracking):

Set the longest sub-list to the empty list.
Set the working sub-list to a list containing just the first item in the input list.
Set working sub-list min and max to the first item in the working sub-list.
Move along the input list, updating min and max as long as the difference between them ends up being less than one and adding the current item to the working sub-list.
Once you encounter a number which makes the difference between min and max more than one (or the end of the list, at which point you'd terminate after this step), check if the working sub-list is longer than the longest sub-list and update the latter if so.
Once you encounter a number which makes the difference between min and max more than one you can't simply start the new working sub-list from that list item, because valid sub-lists may overlap (for example, [1, 1, 2, 3] has valid sub-lists [1, 1, 2] and [2, 3]). At this point you need to set the new working sub-list to include all the items at the tail end of the previous working sub-list which make a valid sub-list with the new item (if any).
Go to 3 until you reach the end of the list.

Tool support

black can automatically format your code to be more idiomatic, for example adding spaces around infix mathematical operators like +.

flake8 can give you more hints to write idiomatic Python beyond black:
 [flake8]
 ignore = W503,E203

I would then recommend validating your type hints using a strict mypy configuration:
 [mypy]
 check_untyped_defs = true
 disallow_any_generics = true
 disallow_untyped_defs = true
 ignore_missing_imports = true
 no_implicit_optional = true
 warn_redundant_casts = true
 warn_return_any = true
 warn_unused_ignores = true

In your case it looks like the type of arr should be Iterable[int] and the output is int.


Opening all the links I am looking to improve my code structure. In this code it is opening all the links with ?pid= in the url. However, the structure of the code can be improved.

for a in self.soup.find_all(href=True):
    href = a['href']
    if not href or len(href) <= 1:
        continue
    if href[0] == '/':
        href = (domain_link + href).strip()
        if href.lower().find("?pid=") != -1:
            href = href.strip()
        elif 'javascript:' in href.lower():
            continue
        elif 'reviews' in href.lower():
            continue
    elif href[:4] == 'http':
        if href.lower().find("?pid=") != -1:
            href = href.strip()
    elif href[0] != '/' and href[:4] != 'http':
        href = ( domain_link + '/' + href ).strip()
        if href.lower().find("?pid=") != -1:
            href = href.strip()
    if '#' in href:
        indx = href.index('#')
        href = href[:indx].strip()
    if href in links:
        continue
    links.append(self.re_encode(href))
self.dict['a_href'] = links

High-level notes:

There are no comments in this code. It would be easier if you provided some explanation for the logic – for example, why do we skip URLs that contain “javascript:” or “reviews” – because it will make the code easier to review and update later.
There’s quite a bit of repetition. Pulling that out will make it easier to read and follow.
This is a moderately complicated and self-contained block of code. It might be worth pulling it out into its own method/function normalise_url that can be tested independently. If you don’t want to use the URL, return None. Then you have:

href = normalise_url(a['href'])
if (href is not None) and (href not in links):
    links.append(href)

And the logic of deciding whether to use the URL, and how much to use, can be managed elsewhere.

Line-specific comments:

Your if branches go:

if href[0] == '/':
    ...
elif href[:4] == 'http':
    ...
elif href[0] != '/' and href[:4] != 'http':
    ...

If we reach the second elif, then we know that both of those conditions are true, so we can reduce it to else:.

Note also that a better way to check if a string starts with a particular substring is to use .startswith().
You’re using str.find() to check whether the URL contains the string ?pid=. The better approach is to use in (and indeed, this is recommended by the docs):

if '?pid=' in href.lower():
    ...

However, it should be noted that on both occasions when you use this, the only thing you do is apply .strip() to a string which has already had .strip()applied. With no arguments, this just removes whitespace – which means the second application does nothing. You could cut out this branch entirely.
When you construct the href with the domain link, you could call .lower() there to save you remembering to call it later; i.e.,

href = (domain_link + href).strip().lower()
...
href = (domain_link + '/' + href).strip().lower()

When you check whether the URL contains javascript: or reviews, you could compress the condition onto a single line without sacrificing readability:

if ('javascript:' in href) or ('reviews' in href):
    continue

When you’re looking for the hash (#) and splitting the string, you’re doing it in quite an unPythonic way. Rather than looking up the index of # in the URL and taking a slice, just take the first part of .split(); i.e.

href = href.split('#')[0].strip()

This works whether or not there's a hash in the string.
I would tidy up the final if condition to check if (href not in links), as it makes the code a little cleaner. It saves you a continue when you’re very close to the end of the loop, and makes it explicit when you’re going to add a URL.

(Alternatively, you could make links a set, and then you could add every URL and trust Python to sift out the duplicates. Of course, that depends on whether order is important.)

With those comments in mind, here’s my final version of your code:

for a in self.soup.find_all(href=True):
    href = a['href']
    if not href or len(href) <= 1:
        continue

    if href[0] == '/':
        href = (domain_link + href).strip().lower()
        if ('javascript:' in href) or ('reviews' in href):
            continue
    elif not href.startswith('http'):
        href = (domain_link + '/' + href).strip().lower()

    href = href.split('#')[0].strip()

    if (href not in links):
        links.append(self.re_encode(href))

I’ve also added some blank lines to break it up into logical units – I think this makes it easier to skim the structure.


Ensure queue is in ascending order The program reads in a queue of integers and returns determines whether or not it is valid (ascending).
How can this code be improved?
I assume iterator include statement is unnecessary?

#include <iostream>
#include <queue>
#include <deque>
#include <iterator>

template<typename T, typename Container=std::deque<T> >
class iterable_queue : public std::queue<T,Container>
{
public:
    typedef typename Container::iterator iterator;
    typedef typename Container::const_iterator const_iterator;

    iterator begin() { return this->c.begin(); }
    iterator end() { return this->c.end(); }
    const_iterator begin() const { return this->c.begin(); }
    const_iterator end() const { return this->c.end(); }
};

bool checkValidity(iterable_queue<int>& q) {
    if (q.empty() || q.size() <= 1){
        std::cout << "invalid entry, insufficient elements" << '\n';
        return false;
    }
    while(q.size()){
        auto i = q.begin();
        auto j = ++q.begin();
        for(; i < q.end() && j < ++q.end();){
            std::cout << *i << " " << *j << '\n';
            if (*(i) > *(j)) {
                std::cout << "invalid entry, not properly sorted" << '\n';
                return false;
            }
            i++, j++;
        }
        std::cout << "valid entry, properly sorted" << '\n';
        return true;
    }
    std::cout << "invalid entry, insufficient elements" << '\n';
    return false;
}

const char* bool_cast(const bool b) {
    return b ? "true" : "false";
}

int main () {
    iterable_queue<int> numbers;
    int temp;

    std::cout << "Pushing..." << '\n';
    while(temp >= 0){
        std::cout << "Enter numbers: ";
        std::cin >> temp;
        if(temp >= 0){
            numbers.push(temp);
        }
    }

    bool ck = checkValidity(numbers);
    std::cout << bool_cast(ck) << '\n';

    std::cout << "{ ";
    while(numbers.size() > 0){
        std::cout << numbers.front();
        numbers.pop();
        std::cout << " ";
    }
    std::cout << "}" << '\n';

    return 0;
}

Queues are not designed to be used like this.  Your iterable_queue clearly shows that you are not using queues the way they are supposed to be used.  And the checkValidity function should be simplified with std::is_sorted from the standard library.  In q.empty() || q.size() <= 1, the first condition is subsumed by the second.  The while(q.size()){ loop also seems to have no significance.  ++q.end() is also undefined behavior.  The bool_cast functionality is already provided by std::boolalpha.

Overall, I think you are seriously over-complicating everything:

#include <algorithm>
#include <iostream>
#include <iterator>
#include <vector>

int main()
{
    std::vector<int> numbers;
    for (int num; std::cin >> num && num >= 0;) {
        numbers.push_back(num);
    }

    bool valid = numbers.size() > 1 && std::is_sorted(numbers.begin(), numbers.end());
    std::cout << "Valid? " << std::boolalpha << valid << '\n';

    std::cout << "{ ";
    for (int num : numbers) {
        std::cout << num << ' ';
    }
    std::cout << "}\n";
}

(live demo)


Increasing performance of OpenMP based advection equation solver for Xeon Phi I am solving linear advection equation in order to learn parallel programming. I am running this code on Xeon Phi co processor. Below is a plot of how it scales up with increasing number of threads. I am still in learning process and this is my first "parallel" code. I am also not very familiar with architecture specific optimization.

Comments on how to improve the performance of this code and what other stuff should I look into will be great. Also link for writeup or further tutorials to follow will be really helpful.

The plot is for nx=100000.

#include <stdlib.h>
#include <stdio.h>
#include <omp.h>
#include <sys/time.h>
#include <unistd.h>

int64_t TimeInMicros() {
    struct timeval tv;
    gettimeofday(&tv, NULL);
    return tv.tv_sec*1000000 + tv.tv_usec;
}

struct Grid{
  int nx;
  double *u, *f, *res;
  double a, cfl, dx, tf;
};

struct ThreadData{
  int tid, maxthreads;
  struct Grid *grid;

};

void *solver(void *args){

  struct ThreadData *td = (struct ThreadData *)args;

  int tid = td->tid;

  struct Grid *grid = td->grid;

  double *u = grid->u;
  double *f = grid->f;
  double *res = grid->res;

  int nx = grid->nx;
  double tf = grid->tf;
  double cfl = grid->cfl;
  double a = grid->a;
  double dx = grid->dx;

  double dt = cfl*dx/a;
  double t = 0.0;

  int chunk = nx/(td->maxthreads);
  int start = tid*chunk;

  int rc;

  while(t < tf){
    // sync here
#pragma omp barrier

    if(start == 0){
      f[start+1:chunk] = a*u[start:chunk];
    }
    else{
      f[start:chunk+1] = a*u[start-1:chunk+1];
    }

    // sync here
#pragma omp barrier

    if(start == 0){
      f[start] = f[nx-1];
    }

    res[start:chunk] = -(f[start+1:chunk] - f[start:chunk])/dx;
    u[start:chunk] += res[start:chunk]*dt;
    t+=dt;
  }
  return NULL;
}

int main(int argc, char*argv[]){
  int nx=atoi(argv[1]);
  int nthreads=atoi(argv[2]);

  if(nx%nthreads != 0){
    printf("ERROR: Number of cells should be integral multiple of number of threads \n");
    exit(1);
  }

  double *u = new double[nx]();
  double *res = new double[nx]();
  double *f = new double[nx+1]();

  double dx = 1.0/nx;
  double cfl = 0.9;
  double a = 1.0;
  double tf = 1.0;
  int i;

  // initialize
  u[0:nx] = 0.0;
  u[nx/4:nx/2] = 1.0;
  f[0:nx+1] = 0.0;
  res[0:nx] = 0.0;

  struct Grid grid;
  grid.nx = nx;
  grid.a = a;
  grid.cfl = cfl;
  grid.dx = dx;
  grid.u = u;
  grid.res = res;
  grid.f = f;
  grid.tf = tf;

  int64_t t1 = TimeInMicros();

  #pragma omp parallel num_threads(nthreads)
  {
    struct ThreadData td;

    td.tid = omp_get_thread_num();
    td.grid = &grid;
    td.maxthreads = omp_get_num_threads();
    if(nthreads != omp_get_num_threads()) {
      if(td.tid == 0) {
        printf("ERROR: Number of cells should be integral multiple of number of threads \n");
        exit(-1);
      }
    }
    else {
      solver(&td);
    }
  }

  int64_t t2 = TimeInMicros();
  printf("Execution time: %.10f\n", (t2-t1)*1e-6);

  FILE * outfile;
  outfile = fopen("results.txt", "w+");
  for(i = 0; i < nx; i++){
    fprintf(outfile, "%.10f %.10f\n", i*dx, grid.u[i]);
  }
  fclose(outfile);

  delete[] u;
  delete[] res;
  delete[] f;

}

You should first check for the number of command line arguments via argc.  If there are too few, print an error message and then terminate the program.

if (argc < 3) {
    // print an error message...
    return EXIT_FAILURE;
}

In addition, your first existing check in main() should also return EXIT_FAILURE, instead of calling exit(1).  The latter is non-portable, unlike the former, and it's already safe to return from main() if the program cannot continue execution.  More info about that here.
All of your erroneous outputs shouldn't be printed to printf():

printf("ERROR: Number of cells should be integral multiple of number of threads \n");

They should instead be printed to fprintf() and stderr:

fprintf(stderr, "Number of cells should be integral multiple of number of threads \n");

This comment is misleading:

// initialize
u[0:nx] = 0.0;
u[nx/4:nx/2] = 1.0;
f[0:nx+1] = 0.0;
res[0:nx] = 0.0;

They're not actually being initialized; they're being assigned after declaration.  Initialization involves giving a variable its type and its initial value.  You should do that here, which will keep variables as close in scope as possible, which is generally preferred for maintenance concerns.

Either way, you don't really need that comment.  It's already clear what's being done (even after making this correction), and comments shouldn't state the obvious.
Also regarding closest possible variable scope: you have i declared towards the top of main(), but you don't use it until the for loop towards the end.  If you don't have C99 (and thus cannot initialize i within the loop statement instead), declare i right before the loop.
This line doesn't look right:

td.maxthreads = omp_get_num_threads();

It appears that you're looking for omp_get_max_threads():

td.maxthreads = omp_get_max_threads();

This will return the maximum number of available threads for a parallel environment.  If this is not really your intent, then rename maxthreads to something more accurate.